[lvm2] Fix warning due to locking_type=0 option

[nrwahl2]

On RHEL 8, the option --config="global{locking_type=0}" is deprecated.
This option is included in the lvm_opts variable and thus produces
deprecation warnings in the outputs of pvscan, vgscan, pvs, vgs,
and lvs:

    WARNING: locking_type (0) is deprecated, using --nolocking.

This patch fixes the issue by removing locking_type=0 from the config
string and appending --nolocking to lvm_opts.

Related to RHBZ#1849248.

Signed-off-by: Reid Wahl nrwahl@protonmail.com

---

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

• Is the commit message split over multiple lines and hard-wrapped at 72 characters?
• Is the subject and message clear and concise?
• Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
• Does the commit contain a Signed-off-by: First Lastname email@example.com?
• If this commit closes an existing issue, is the line Closes: #ISSUENUMBER included in an independent line?
• If this commit resolves an existing pull request, is the line Resolves: #PRNUMBER included in an independent line?

[nrwahl2]

As far as I can tell, --nolocking is available as of the earliest RHEL 7 release of the lvm2 package. We should make an effort to verify that it is available on modern versions of other distros. I suspect it will be.

[pmoravec]

As far as I can tell, --nolocking is available as of the earliest RHEL 7 release of the lvm2 package. We should make an effort to verify that it is available on modern versions of other distros. I suspect it will be.

I don't think so:

# rpm -q lvm2
lvm2-2.02.186-7.el7_8.1.x86_64
# vgdisplay -vv --config="global{metadata_read_only=1}" --nolocking
vgdisplay: unrecognized option '--nolocking'
  Error during parsing of command line.
#

If I am right, the change in lvm2 was implemented in commit lvmteam/lvm2@e6bb780 that appears since release v2_03_00 (i.e. lvm2-2.03.*). That is present in all supported versions of Fedora and in RHEL8. sos upstream is not planned to be (fully) backported to RHEL7, similarly to CentOS, I think. So Red Hat distros should be OK.

Some feedback from @slashdd or others from Ubuntu/Debian is welcomed, if it can't break something.

[pmoravec]

(optionally, we can make the lvm_opts of conditional content, depending on lvm2 package version. We do so in networking and process plugins as well; but I expect it won't be required here)

[nrwahl2]

@pmoravec

I don't think so:

Ah yes, you're right. I must not have checked the display commands as I should have done.

I checked the pvs/vgs/lvs commands on RHEL 7, and they supported --nolocking. I also did some grepping for nolocking through the source for the oldest RHEL 7 lvm2 release, and it appeared to recognize the option. (I did not downgrade to the older package to test.)

Clearly the scope of that option is not as extensive on earlier releases as it is on RHEL 8.

[bmr-cymru]

I would suggest wrapping something around a predicate to produce the desired change in string based on a version check, or failing that (since it may depend too closely on distro NVR specifics) base on the output of lvs --help containing --nolocking.

[nrwahl2]

Updated the commit to check vgdisplay -h output. lvs already supported --nolocking on RHEL 7, so choosing a display command seems like a better test.

I tested using self.get_command_output() on RHEL 7 and 8. The commit uses self.exec_cmd() to match upstream, but I have not tested on upstream specifically. Let me know if you see any issues that I missed.

[TurboTurtle]

We have a somewhat recent addition in the API in the form of predicates that handles this directly.

It'd be something like:

nolock = {'cmd': vgdisplay -h', 'output': '--nolocking'}
if self.test_predicate(self, pred=SoSPredicate(self, cmd_outputs=nolock):
    lvm_opts = 'foo'
else:
    lvm_opts = 'bar'

The grub2 plugin has an example of this kind of predicate.

[nrwahl2]

Thanks! That seems to work (though the first self arg to test_predicate is unnecessary here and in grub2.py).

[bmr-cymru]

You don't really need to use test_predicate() here since this is a known predicate for a specific condition - the only difference between self.test_predicate(pred) and bool(pred) is that test_predicate() will use the default command or plugin predicate instead of the pred= kwarg if they are set and it is not - that's probably not the behaviour you want here, even if it currently gives the same result.

The test_predicate() call has self as the first argument since it is a method of the Plugin class (in order to retrieve the stored command or plugin predicate).

Same comment applies to the use in grub2: it can simply be rewritten as:

        if bool(SoSPredicate(self, cmd_outputs=co)):
            grub_cmd += ' --no-grubenv-update'

And likewise here - generally speaking plugins should not need to call Plugin.test_predicate() themselves - its main use is to consistently apply the predicate precedence rules when evaluating predicates in the Plugin helper methods (add_copy_spec(), add_cmd_output(), add_string_as_file(), collect_cmd_output(), and exec_cmd(): these need to properly evaluate any predicate passed in the method arguments as well as any stored command or plugin predicate. For this kind of one-off use where a single, specific predicate guards an action that's not required and it makes the code harder to follow.

[nrwahl2]

Great to know! That's much cleaner.

[nrwahl2]

Updated based on @TurboTurtle 's feedback. I tested with upstream sos on RHEL 7 and RHEL 8, and it appears to work as intended. Let me know if you find that it doesn't.

[bmr-cymru]

Ack to the general approach but please drop the unnecessary test_predicate() and just use the boolean value of the predicate.