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