-
-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enabling first volume creation on Puppet > 4 #24
Conversation
Solving #23 |
There are additional places where the
If you build a cluster to establish peers, and then add a volume, the above should work because the facts will be populated. If you try to set up a brand-new installation that includes a volume definition, these will fail because the facts are empty. Obviously, it's desirable to be able to build a complete cluster in one shot. (That is to say: one shot at defining the configuration. Multiple Puppet runs will be required; but what we don't want to do is define an initial config, run Puppet, apply some more config, re-run Puppet, and then have a cluster.) I've got it working with the following changes: L105:
And then on (the new) L175, modify the
Can you please double-check my work here to help make sure we're not missing anything? |
@tux-o-matic another way to tackle this is to update the custom fact to actually return an empty string for Doing so causes problems on line 142 of Other problems might be lurking elsewhere with empty strings versus undefined facts, of course. |
And if we're changing custom facts, we might as well use structured facts to return native hashes and arrays, in order to avoid a lot of the contortions we go through in the manifest to convert comma-separated lists into hashes and arrays. |
@tux-o-matic I've just transferred this module to the Puppet Community organization. Your PR was transferred, too; but please update your bookmarks and your |
I won't have an environment to test a new approach with volume creation from scratch for a few days. Which should be aimed for? Validating my PR with your additions in volume.pp or fixing the facts to never return undef? |
As discussed in voxpupuli#24 (comment) extra check added to handle creation of volumes from scratch on a Gluster pool.
With my latest batch of commits, I've been able to resolve issues blocking the creation of a Gluster volume from scratch (no pre-existing Gluster volume in the pool). @skpy There was a middle ground to find between my first commits and your latest suggestion as the Fact 'gluster_volume_list' can be undef when no Gluster volumes exist. So the Fact being defined should not be a requirement to create a volume or else the first volume can never be created by this module. if $::gluster_peer_list != undef {
$minimal_requirements = true
} else {
$minimal_requirements = false
}
if $::gluster_volume_list != undef and member( split( $::gluster_volume_list, ',' ), $title ) {
$already_exists = true
} else {
$already_exists = false
}
if $minimal_requirements and $already_exists == false { If it makes sense to you then the checks can be further simplified by having '$::gluster_peer_list != undef' replace altogether the $minimal_requirements bool. Some changes were also needed to peer.pp I also included some corrections from #31 to be able to complete Puppet runs without errors. |
I also created more Gluster volumes on the same nodes/pool following the first one done from scratch by Puppet and can confirm that the new code does not interfere with this original feature. |
@skpy Anything holding this PR from being merged? |
@tux-o-matic please squash. |
On a node with no pre-existing Gluster volume, this module will not populate the Fact 'gluster_volume_list' there for we should check if the current volume already exists ONLY if the nodes already has some volumes. Work on conditional statement readability Puppet DSL doesn't allow variable re-assignment Of course! So reformating conditional statement to make everything checked within a single 'if'. Adding checks for setup from scratch As discussed in voxpupuli#24 (comment) extra check added to handle creation of volumes from scratch on a Gluster pool. Added check for undef fact Ensure first run doesn't throw an error because Fact for gluster_peer_list may return as undef. Related to voxpupuli#23 Handle different state/member in pool Adding check to enable creation of pool and volume from scratch by the module. voxpupuli#23 Removing blocking check for dry run Fact $::gluster_volume_list presence should not be checked on L 105 as it may be undef on very first run in a pool without pre-existing volumes Correcting typo in var ref and pattern in regsubst Prevent error being thrown by pattern in regsubst() function. As explained and corrected in voxpupuli#31
CI is failing all of a sudden. Is that due to changes on the repository? Maybe time to update RPM version in tests and maybe YUM repo base. |
On a node with no pre-existing Gluster volume, this module will not populate the Fact 'gluster_volume_list' there for we should check if the current volume already exists ONLY if the nodes already has some volumes.
@jyaworski Squashed. |
I'm going to merge. The failures are due to strict variable nonsense in the tests. Someone has to get around to refactoring the tests.... |
Enabling first volume creation on Puppet > 4
Enabling first volume creation on Puppet > 4
Volume creation depends on the 'gluster_volume_list' Fact which is empty if no volume already exists in this pool. On Puppet > 4, an undefined Fact cannot be used in the split() function. There for the code should proceed if any two cases are met:
-They are no pre-existing columes in the pool
-None of the existing volumes contain the one defined in this resource.