Re: [Gluster-devel] Puppet-Gluster+ThinP

[James]

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
> >
> >
> >
>

signature.asc
Description: This is a digitally signed message part