fix issues on nodes without lvm

[rtib]

tested manually on nodes with and without lvm

Closes #307

[vchepkov]

According to https://www.puppet.com/docs/puppet/7/fact_overview.html, there should not be code outside setcode

Note: Set all code inside the sections outlined above ⁠— there must not be any code outside setcodeand confine blocks other than an optional has_weight statement in a custom fact.

Also down the text

Any code with possible side-effects, or code pertaining to figuring out the value of a fact, must be kept inside the setcode block. 

[rtib]

That's true and one more cause why these legacy facts should be deprecated, as pointed out in Slack discussion by @cruelsmith

Just note that when puppetlabs would be strict the lvm_vg_pvs, lvm_vg and lvm_pv_* facts should be marked as legacy facts and which are per default disabled with puppet 8. So maybe inventing time to use the new structured facts would be better in the long run.

They just need to work as long there is no alternative source for the particular information they provide and some time to migrate.

[rtib]

The thing is as follows:

TL;DR
Those facts with programmatically generated names, i.e. lvm_vg_<n> and lvm_vg_<vg_name>_pvs rely on code outside any setcode blocks, but require the results of commands executed before. It is not possible to set global variables within a setcode block, that's why those commands need to be executed outside, where they are not guarded by confine statements. This obviously violates the requirement to have all code of this kind within setcode or confine blocks. They could, however, be accessed as fact values, but as they are needed to produce the names of the above facts, that would violate the requirement to have such code within setcode blocks only. Current RSpec implementation isn't able to deal with code outside setcode and confine statements, thus these features cannot be tested within spec.

To me, all that sums up as the code as it is works for now, but cannot be tested thoroughly, and might break in near future. Any code, that can be tested and is future prove, won't be able to produce fact names programmatically - dispute me if I'm wrong.

I'd propose the following solution:

• leave the code within lvm_support.rb as it is in this PR, manually tested
• drop the failing spec tests
• deprecate the legacy facts having programmatically generated names now, and remove them as soon as possible
• implement replacement using a modern approach, i.e. structured/aggregated facts reporting the relations between pvs, vgs and lvs

[cruelsmith]

Mhm what about a more radically approach here and reuse the existing physical_volumes, volume_groups and logical_volumes facts to generate the these ones?

So for the lvm_pvs and lvm_pv_* something like this:

# lvm_pvs: [0-9]+
#   Number of PVs
Facter.add('lvm_pvs') do
  confine lvm_support: true

  setcode do
    physical_volumes = Facter.value('physical_volumes')
    return 0 if physical_volumes.nil?

    physical_volumes.length
  end
end

# lvm_pv_[0-9]+
#   PV name by index
unless Facter.value('physical_volumes').nil?
  Facter.value('physical_volumes').keys.each_with_index do |pv, i|
    Facter.add("lvm_pv_#{i}") do
      setcode { pv }
    end
  end
end

[rtib]

Something like that might work in the wild, though, it doesn't solve any problems we are facing here:

1. violating the format as described here: https://www.puppet.com/docs/puppet/7/fact_overview.html#writing_facts_simple_resolutions-how-to-format-facts
2. cannot write proper spec tests for

Moreover, lvm_pv_<n> can be replaced by using physical_volumes. The main focus is on the contents of lvm_vg_<vg_name>_pvs, which doesn't have an alternative source yet.

[rtib]

rebased onto main

[elfranne]

Tested on Windows 2019 too and works fine.

[vchepkov]

I defer to experts in Puppet's internals. As I stated earlier, v2.0.3 and updated code doesn't conform to the documentation, code in v2.0.2 did.

[Ramesh7]

The below facts are the custom flat facts, so not sure if these are gonna deprecate as well?
AFAIK the custom facts (irrespective of flat or structured facts) these will not be deprecated and Puppet will allow them to define for long run as well. Only the legacy facts which will get deprecated.

[rtib]

Facter code generating fact names dynamically on the fly is problematic: violates coding rules, hard to maintain, even harder to test. That's why we should get rid of them.

[crazymind1337]

Can we please have this released soon?

[Ramesh7]

we are moving with this change for now but for long term we need to review these facts with manifest use-case and update accordingly.

[rajat-puppet]

These changes have been released as part of 2.1.0 release version

[rtib]

Thank You.