extend structured facts to provide relations between lvs, vgs and pvs

[rtib]

Summary

This is extending the fact logical_volumes with the name of the volume group they are residing on. An additional fact volume_group_map is providing a map of volume groups and their backing physical volumes.

Additional Context

As discussed on Slack, in #298 (comment) and boiled down in #308 (comment), facts having programmatically generated name should be retired. These changes are necessary to close provide all information the deprecated facts were providing.

Checklist

• 🟢 Spec tests.
• 🟢 Acceptance tests.
• Manually verified. (For example puppet apply)

[rtib]

The failed acceptance test broke down outside the test examples, might need to be kicked again.

[cruelsmith]

First of all thanks for your work. 👍

In my eyes the goal should be to prevent any additional fact to add the relation between the vg and pv. It should be as simple as you provided it for the lv.
One way would be to rework the Puppet_X::LVM::Output.parse to something like this:

def self.parse(key, columns, data, prefix2remove)
  data.split("\n").map { |line|
    line.gsub(%r{\s+}, ' ').strip.split
  }.map { |line|
    columns.zip(line).to_h
  }.group_by { |h|
    h[key]
  }.transform_values { |v|
    tmp = {}
    columns.each { |column|
      tmp[column] = []
      v.each { |key,value|
        tmp[column] << key[column]
      }
      tmp[column].uniq!
      tmp[column] = tmp[column][0] if tmp[column].length == 1
    }
    tmp.transform_keys { |key|
      remove_prefix(key, prefix2remove)
    }
  }
end

After that the pv_name can be added to the argument list of the volume_groups facts.
Which then results in

pv_name => [
      "/dev/sdb",
      "/dev/sdc",
      "/dev/sdd",
      "/dev/sde",
      "/dev/sdf"
    ],

or

pv_name => "/dev/sda",

to be added to the entries of the volume_groups fact.

---

Please note that this is only a PoC code, which means that it can probably still be optimized or that there still exist unfixed edge cases.

[rtib]

Hi, @cruelsmith

thank you for your suggestions. First, I also though that it would be simple enough to add an additional field to volume_groups revealing it's physical volumes. Then I stumbled over the strange output of vgs when extending it with pv_name, which actually multiply the rows having multiple pvs. Grouping that sounds straightforward, though, rise a couple of questions and concerns: which field to group by on? (uuid, vg_name?) What if there is a clustered or shared volume group? I don't even have the environment to test all those possibilities.

Remarkably enough, there is no man page listing pv_name as valid field for reporting volume groups!

Such a solution would make lots of assumptions on the underlying API including undocumented features, requiring them to be stable among many versions, including future too. If something change, the whole thing might break.

While the same code which is generating volume_group_map might be added to volume_groups.rb to add an additional field, that would break the nice simplicity of that fact.

That's why I was withdrawing from this approach and implemented a less attractive additional fact.

[jst-cyr]

@rtib : My apologies that nobody has responded on this for so much time. I'm going through our pull requests across modules and came across this request here. Is this still an issue for you?

[rtib]

It has never been a personal issue to me. However, I've moved away from Puppet for now. My support goes either to open source software or to those who pay for it.