Skip to content
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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rtib
Copy link
Contributor

@rtib rtib commented Jul 14, 2023

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 rtib changed the title extend structured facts to replace information from legacy extend structured facts to provide relations between lvs, vgs and pvs Jul 14, 2023
@rtib rtib force-pushed the strucutredmapping branch from 9d2cbbf to 3947719 Compare July 18, 2023 12:39
@rtib
Copy link
Contributor Author

rtib commented Jul 18, 2023

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

@rtib rtib marked this pull request as ready for review July 18, 2023 12:55
@rtib rtib requested review from a team and bastelfreak as code owners July 18, 2023 12:55
@cruelsmith
Copy link

cruelsmith commented Jul 21, 2023

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
Copy link
Contributor Author

rtib commented Jul 21, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants