[Gluster-devel] Puppet-Gluster+ThinP

[Keith Schincke]

James,

Here is an other small one:
The number of extents used on the lvcreate should be a changable value. 
The RHS 2.1 Admin guide recommends leaving 15% to 20% of the space 
available for future snapshotting. There may also be other times where 
the admin does not wish to use all of the remaining space withing the VG>

Keith


On 04/09/2014 12:39 PM, James wrote:
> On Wed, 2014-04-09 at 11:56 -0400, Keith Schincke wrote:
>> Here is a quick set of reviews.
>>
>> What package and distro version includes the lvmthin man page?
> I think it's still in git, but it's available online.
>
>> On line 653 of Documentation.md, you refer to "man -7 thin". This should
>> be "man -7 lvmthin"
> Good catch, thanks!
>
>> This same is done in the README.md. Never mind. I see the sym link.
> Yeah. This makes the github people happy.
>
>>   From line 178 to 208, you are doing a lot of work to build the thin lvm
>> command line. Should you wrap this in a if $lvm_thinp conditional. This
>> will keep all the thin provisioning code  and a possible vgs system call
>> from occurring if $lvm_thinp is not true.
> Actually it's all declarative, so it's safe without it. The conditional
> is here at 211:
>
> $lvm_lvcreate = $lvm_thinp ? {
> 	true => "${lvm_thinp_lvcreate}",
> 	default => "/sbin/lvcreate --extents 100%PVS -n ${lvm_lvname}
> ${lvm_vgname}",
> }
>
>
>> Could you also update your lines 218 - 220 to this:
>>
>> $dev2 = $lvm ? {
>>       true => "/dev/${lvm_vgname}/${lvm_lvname}"} ,
>>       default => "${dev1}",
>> }
>>
>> The long "if" with a short/trivial "else" is almost an "oh, by the way"
>> kind of statement. It can be easy to overlook. The separate conditional
>> can help an other reviewer follow along more easily.
> Yeah, I like this actually. Thanks for the idea.
>
> Branch updated (rebased):
> https://github.com/purpleidea/puppet-gluster/tree/feat/thinp
>
> Commit at:
> https://github.com/purpleidea/puppet-gluster/commit/d204fe5c4b80f0bc6d7850da6ccc90cb695c5873
>
> Also I added a small warning if someone enables thinp but disables LVM.
>
> James
>
>> Keith
>>
>>
>> On 04/09/2014 11:13 AM, James wrote:
>>> Okay,
>>>
>>> Here's a first draft of puppet-gluster w/ thin-p. This patch includes
>>> documentation updates too! (w00t!)
>>>
>>> https://github.com/purpleidea/puppet-gluster/tree/feat/thinp
>>>
>>> FYI: I'll probably rebase this branch.
>>> FYI: Somewhat untested. Read the commit message.
>>>
>>> Comments welcome :)
>>>
>>> I'm most interested to hear about if everyone is pleased with the way I
>>> run the thin-p lv command. I think this makes the most sense, but let me
>>> know if anyone has improvements. Also I'd love to hear about what the
>>> default values for the parameters should be, but that's a one line
>>> patch, so no rush for me.
>>>
>>> Cheers,
>>> James
>>>
>>>
>>>

-- 
Keith Schincke
Senior Software Engineer (RHCA)
Systems Engineering