Bug 2091163

More Information:
==================


Found the root cause, the LVM PV commands return the block devices with ':' included

~~~
[cb/LI] root@test# lvm pvdisplay -c
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:1:0-part1:vg_li_lc_2434_app1:20967424:-1:8:8:-1:32768:319:1:318:1yH2JN-ibqU-mvo0-DF9w-oTTo-R02G-bjJrKp
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:0:0-part3:vg_li_lc_2434_root:79685632:-1:8:8:-1:32768:1215:63:1152:CNuWs5-EDMj-BVlY-bBEa-qtd3-Q1YV-X2UrTr
[cb/LI] root@test#
~~~



Prove that downgrading LVM2 to 2.0.3.12 from RHEL8.5 works restores the expected PV devices names without ':' in the names and keeping the 'pvdisplay -c' parsign working for rear.

~~~
[cb/LI] root@test# lvm pvdisplay -c
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:1:0-part1:vg_li_lc_2434_app1:20967424:-1:8:8:-1:32768:319:1:318:1yH2JN-ibqU-mvo0-DF9w-oTTo-R02G-bjJrKp
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:0:0-part3:vg_li_lc_2434_root:79685632:-1:8:8:-1:32768:1215:63:1152:CNuWs5-EDMj-BVlY-bBEa-qtd3-Q1YV-X2UrTr

[cb/LI] root@test# sudo pvs
  PV                                                    VG                 Fmt  Attr PSize   PFree
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:0:0-part3 vg_li_lc_2434_root lvm2 a--  <37.97g <1.97g
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:1:0-part1 vg_li_lc_2434_app1 lvm2 a--   <9.97g 32.00m

[cb/LI] root@test# sudo yum downgrade lvm2
...
Dependencies resolved.
===============================================================================================================================================
 Package                                   Architecture            Version                                Repository                                          Size
===============================================================================================================================================
Downgrading:
 device-mapper                             x86_64                  8:1.02.177-11.el8_5                    rhel-8-for-x86_64-baseos-eus-rpms                  377 k
 device-mapper-event                       x86_64                  8:1.02.177-11.el8_5                    rhel-8-for-x86_64-baseos-eus-rpms                  271 k
 device-mapper-event-libs                  x86_64                  8:1.02.177-11.el8_5                    rhel-8-for-x86_64-baseos-eus-rpms                  270 k
 device-mapper-libs                        x86_64                  8:1.02.177-11.el8_5                    rhel-8-for-x86_64-baseos-eus-rpms                  409 k
 lvm2                                      x86_64                  8:2.03.12-11.el8_5                     rhel-8-for-x86_64-baseos-eus-rpms                  1.6 M
 lvm2-libs                                 x86_64                  8:2.03.12-11.el8_5                     rhel-8-for-x86_64-baseos-eus-rpms                  1.2 M

Transaction Summary
===============================================================================================================================================
Downgrade  6 Packages
...
Downgraded:
  device-mapper-8:1.02.177-11.el8_5.x86_64           device-mapper-event-8:1.02.177-11.el8_5.x86_64      device-mapper-event-libs-8:1.02.177-11.el8_5.x86_64
  device-mapper-libs-8:1.02.177-11.el8_5.x86_64      lvm2-8:2.03.12-11.el8_5.x86_64                      lvm2-libs-8:2.03.12-11.el8_5.x86_64

Complete!

[cb/LI] root@test# pvs
  PV         VG                 Fmt  Attr PSize   PFree
  /dev/sda3  vg_li_lc_2434_root lvm2 a--  <37.97g <1.97g
  /dev/sdb1  vg_li_lc_2434_app1 lvm2 a--   <9.97g 32.00m

[cb/LI] root@test# lvm pvdisplay -c
  /dev/sdb1:vg_li_lc_2434_app1:20967424:-1:8:8:-1:32768:319:1:318:1yH2JN-ibqU-mvo0-DF9w-oTTo-R02G-bjJrKp
  /dev/sda3:vg_li_lc_2434_root:79685632:-1:8:8:-1:32768:1215:63:1152:CNuWs5-EDMj-BVlY-bBEa-qtd3-Q1YV-X2UrTr

[cb/LI] root@test# rear mkrescue
Symlink '/usr/lib/modules/4.18.0-372.9.1.el8.x86_64/build' -> '/usr/src/kernels/4.18.0-372.9.1.el8.x86_64' refers to a non-existing directory on the recovery system.
It will not be copied by default. You can include '/usr/src/kernels/4.18.0-372.9.1.el8.x86_64' via the 'COPY_AS_IS' configuration variable.
Symlink '/usr/lib/modules/4.18.0-372.9.1.el8.x86_64/source' -> '/usr/src/kernels/4.18.0-372.9.1.el8.x86_64' refers to a non-existing directory on the recovery system.
It will not be copied by default. You can include '/usr/src/kernels/4.18.0-372.9.1.el8.x86_64' via the 'COPY_AS_IS' configuration variable.
Broken symlink '/usr/lib/firmware/4.18.0-305.40.2.el8_4.x86_64/intel-ucode/06-86-04' in recovery system because 'readlink' cannot determine its link target
Broken symlink '/usr/lib/firmware/4.18.0-305.40.2.el8_4.x86_64/intel-ucode/06-86-05' in recovery system because 'readlink' cannot determine its link target
Broken symlink '/usr/lib/firmware/4.18.0-305.el8.x86_64/intel-ucode/06-86-04' in recovery system because 'readlink' cannot determine its link target
Broken symlink '/usr/lib/firmware/4.18.0-305.el8.x86_64/intel-ucode/06-86-05' in recovery system because 'readlink' cannot determine its link target
Broken symlink '/usr/lib/firmware/4.18.0-372.9.1.el8.x86_64/intel-ucode/06-86-04' in recovery system because 'readlink' cannot determine its link target
Broken symlink '/usr/lib/firmware/4.18.0-372.9.1.el8.x86_64/intel-ucode/06-86-05' in recovery system because 'readlink' cannot determine its link target


[cb/LI] root@test# ls -l /var/lib/rear/output/
total 412048
-rw-------. 1 root root 421937152 May 25 09:37 rear-li-lc-2434.iso
~~~




Note that we are using an LVM filter that looks like:
~~~
  filter = [ "r|/dev/disk/by-path/.*-usb-|", "a|/dev/disk/by-path/pci-.*-nvme-|", "a|/dev/disk/by-path/pci-.*-scsi-|", "a|loop|", "r|.*|" ]
~~~

The displayed names are matching the 'dev/disk/by-path/pci-.*-scsi-' included in the filter.

Still leaves 2 topics:
- There is a difference then in how it is displayed in pvdisplay/pvs commands compared to RHEL8.4.
- The paths using ':' break the 'pvdisplay -c' output



Below is a fix for Rear 2.6 to handle disk device names containing ':'. The fix is to use the pipe '|' as a separator character:

~~~
--- /usr/share/rear/layout/save/GNU/Linux/220_lvm_layout.sh.220525-1    2022-02-27 22:20:48.000000000 +0000
+++ /usr/share/rear/layout/save/GNU/Linux/220_lvm_layout.sh     2022-05-25 11:21:43.503818223 +0000
@@ -70,14 +70,16 @@
	 # Get physical_device configuration.
	 # Format: lvmdev <volume_group> <device> [<uuid>] [<size(bytes)>]
	 header_printed="no"
-    # Example output of "lvm pvdisplay -c":
-    #   /dev/sda1:system:41940992:-1:8:8:-1:4096:5119:2:5117:7wwpcO-KmNN-qsTE-7sp7-JBJS-vBdC-Zyt1W7
+    # pvdisplay set separator to :: to prevent issues with a column in the path for /dev/disk/by-path that contains a : in the scsi slot name
+    # Example output of "lvm pvdisplay --columns --separator=|":
+    #   /dev/sda1|system|41940992||-1|8|8|-1|4096|5119|2|5117|7wwpcO-KmNN-qsTE-7sp7-JBJS-vBdC-Zyt1W7
+    #   /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:1:0-part1||41940992|:-1|8|8|-1|4096|5119|2|5117|7wwpcO-KmNN-qsTE-7sp7-JBJS-vBdC-Zyt1W7
	 # There are two leading blanks in the output (at least on SLES12-SP4 with LVM 2.02.180).
-    lvm pvdisplay -c | while read line ; do
+    lvm pvdisplay --columns --separator "|" | while read line ; do

		 # With the above example pdev=/dev/sda1
		 # (the "echo $line" makes the leading blanks disappear)
-        pdev=$( echo $line | cut -d ":" -f "1" )
+        pdev=$( echo $line | cut -d "|" -f "1" )

		 # Skip lines that are not describing physical devices
		 # i.e. lines where pdev does not start with a leading / character:
@@ -91,11 +93,11 @@
		 fi

		 # With the above example vgrp=system
-        vgrp=$( echo $line | cut -d ":" -f "2" )
+        vgrp=$( echo $line | cut -d "|" -f "2" )
		 # With the above example size=41940992
-        size=$( echo $line | cut -d ":" -f "3" )
+        size=$( echo $line | cut -d "|" -f "3" )
		 # With the above example uuid=7wwpcO-KmNN-qsTE-7sp7-JBJS-vBdC-Zyt1W7
-        uuid=$( echo $line | cut -d ":" -f "12" )
+        uuid=$( echo $line | cut -d "|" -f "12" )

		 # Translate pdev through diskbyid_mappings file:
		 pdev=$( get_device_mapping $pdev )
~~~

Still leaves that for lvm2 is a compatibility change in the output that it does now print the 'alias' instead of the orignal device (sdX) name.

Thank you for the detailed analysis and the suggested fix, I am able to reproduce the problem quite easily.

I don't think that the change in lvm in RHEL 8.6 is a bug. Since filter contains "r|.*|", device names like /dev/sd* could be considered forbidden in the output (although the documentation does not say explicitly whether filter applies to output, it says only that it "Limits the block devices that are used by LVM commands"). Maybe the previous version resolved symlinks to their targets and the new version does not do it anymore? I agree that it is a bug in ReaR that it does not cope with this kind of output. However, I wonder whether we can guarantee that "|" will not appear in a device name (although it is certainly less usual than ":").

Pavel,

The root cause is that the default value of the setting 'preferred_names' is changed. Checking the rpm changelog and github lvm2 release notes there is nothing written about this change

RHEL8.4:
~~~
$  rpm -q lvm2 ; sudo lvmconfig --type default --withcomments | grep -1 preferred_names
lvm2-2.03.11-5.el8.x86_64

        # Configuration option devices/preferred_names.
        # Select which path name to display for a block device.
--
        # used. Try to avoid using undescriptive /dev/dm-N names, if present.
        # If no preferred name matches, or if preferred_names are not defined,
        # the following built-in preferences are applied in order until one
--
        # Example
        # preferred_names = [ "^/dev/mpath/", "^/dev/mapper/mpath", "^/dev/[hs]d" ]
        # This configuration option has an automatic default value.                        <----------------
        # preferred_names=["^/dev/mpath/","^/dev/mapper/mpath","^/dev/[hs]d"]              <----------------
~~~

RHEL8.6
~~~
$ rpm -q lvm2 ; sudo lvmconfig --type default --withcomments | grep -1 preferred_names
lvm2-2.03.14-3.el8.x86_64

        # Configuration option devices/preferred_names.
        # Select which path name to display for a block device.
--
        # used. Try to avoid using undescriptive /dev/dm-N names, if present.
        # If no preferred name matches, or if preferred_names are not defined,
        # the following built-in preferences are applied in order until one
--
        # Example
        # preferred_names = [ "^/dev/mpath/", "^/dev/mapper/mpath", "^/dev/[hs]d" ]
        # This configuration option does not have a default value defined.                   <----------------
~~~

Ah! Thank you Peter for pointing this out, I was not aware of the preferred_names variable (I am not working on LVM). The undocumented change of its default value is not ideal, but from the ReaR perspective, the current code is fragile anyway and should be fixed.

My patch above does not work, during restore it fails on an incorrect uuid value (using 'lvm2') because the fields are different

~~~
root@testserver:~# lvm pvdisplay -c
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:1:0-part1:vg_data:209713119:-1:8:8:-1:32768:3199:0:3199:JII9fe-4Fch-euGZ-jaZK-Q17J-9pgo-SXRUmo
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:0:0-part3:vg_root:79685632:-1:8:8:-1:32768:1215:63:1152:w1nIAF-gcbs-tVoD-hez3-nJyy-tij5-sbm6fo

root@testserver:~# lvm pvdisplay --columns --separator "|"
  PV|VG|Fmt|Attr|PSize|PFree
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:0:0-part3|vg_li_lc_2440_root|lvm2|a--|<37.97g|<1.97g
  /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:1:0-part1|vg_li_lc_2440_app1|lvm2|a--|<99.97g|0
~~~

When googling if there was maybe already a fix i came across the not fixed rear issue from 2018 https://github.com/rear/rear/issues/1958 that already mentioned this fragile code going wrong as soon as by-path devices were found

Corrected fix to explicit define separator to be '|' and the columns we need and also set units to bytes to have predicatble output

~~~
--- /usr/share/rear/layout/save/GNU/Linux/220_lvm_layout.sh.old
+++ /usr/share/rear/layout/save/GNU/Linux/220_lvm_layout.sh
@@ -70,14 +70,15 @@
     # Get physical_device configuration.
     # Format: lvmdev <volume_group> <device> [<uuid>] [<size(bytes)>]
     header_printed="no"
-    # Example output of "lvm pvdisplay -c":
-    #   /dev/sda1:system:41940992:-1:8:8:-1:4096:5119:2:5117:7wwpcO-KmNN-qsTE-7sp7-JBJS-vBdC-Zyt1W7
+    # pvdisplay set separator to :: to prevent issues with a column in the path for /dev/disk/by-path that contains a : in the scsi slot name
+    # Example output of "lvm pvdisplay -C --separator '|' --noheadings --nosuffix --units=b -o pv_name,vg_name,pv_size,pv_uuid":
+    #   /dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:1:0-part1|system|107340627968|7wwpcO-KmNN-qsTE-7sp7-JBJS-vBdC-Zyt1W7
     # There are two leading blanks in the output (at least on SLES12-SP4 with LVM 2.02.180).
-    lvm pvdisplay -c | while read line ; do
+    lvm pvdisplay -C --separator '|' --noheadings --nosuffix --units=b -o pv_name,vg_name,pv_size,pv_uuid | while read line ; do

-        # With the above example pdev=/dev/sda1
+        # With the above example pdev=/dev/disk/by-path/pci-0000:03:00.0-scsi-0:0:1:0-part1
         # (the "echo $line" makes the leading blanks disappear)
-        pdev=$( echo $line | cut -d ":" -f "1" )
+        pdev=$( echo $line | cut -d "|" -f "1" )

         # Skip lines that are not describing physical devices
         # i.e. lines where pdev does not start with a leading / character:
@@ -91,11 +92,11 @@
         fi

         # With the above example vgrp=system
-        vgrp=$( echo $line | cut -d ":" -f "2" )
-        # With the above example size=41940992
-        size=$( echo $line | cut -d ":" -f "3" )
+        vgrp=$( echo $line | cut -d "|" -f "2" )
+        # With the above example size=107340627968
+        size=$( echo $line | cut -d "|" -f "3" )
         # With the above example uuid=7wwpcO-KmNN-qsTE-7sp7-JBJS-vBdC-Zyt1W7
-        uuid=$( echo $line | cut -d ":" -f "12" )
+        uuid=$( echo $line | cut -d "|" -f "4" )

         # Translate pdev through diskbyid_mappings file:
         pdev=$( get_device_mapping $pdev )
~~~

Thank you a lot for noticing that and correcting the patch. Indeed,  --columns (-C) produces a different format than -c, and one can not specify --separator '|' together with -c.

I have one question about your patch: why does the comment above the code now read

# pvdisplay set separator to :: (...)

when the separator is set to | ? Shouldn't it read "pvdisplay set separator to |"? Otherwise I don't understand the comment.

Sorry for the confusion. The comment is a leftover i have overseen to change, my first idea was using the double colon '::', but then switched to '|' (just like proposed in https://github.com/rear/rear/issues/1958).

This fix is only for pvdisplay, but i think that all LVM commands can benefit to be more robust by using commandline options to enforce an explicit output and units instead of relying on the per-lvm-release or maybe user customization from lvm.conf defaults.

Hello Peter,

I did a survey of how similar problems are handled in other tools. Some Solaris tools (ipadm, dladm, zoneadm) have a special switch (-p) to request machine-parseable format. This format has colon-separated fields, and embedded colons are escaped by backslash to allow parsing by the shell "read" command (this escaping is the key functionality missing in the pvdisplay command). zfs has a -H option which uses an alternative parseable format: in addition to omitting headers it separates the fields by a single TAB character. This would be in principle ideal for our purpose (TABs are unlikely to occur in the values), but in our case the output is processed via an unquoted "echo" command, which would not preserve the TAB character. (This is needed to remove the leading spaces in the output, not sure what their purpose is.)

Given these constraints, I agree that the approach you have chosen with | as the separator is the best one. I tested full backup and restore with your patch and the test was successful. I was afraid that changing the units in your patch (--units=b) would lead to a functionality change, but it turns out that the PV size in disklayout is not used anywhere, so this part is not a problem.

The change is not scheduled for the nearest release of RHEL (we wanted to avoid changing the layout code late in the development process, as it is one of the most critical parts of ReaR), we can build a fully tested hotfix for you instead (supported until the fix is released in an official build).

I will submit the change upstream for review; do you want to be acknowledged as author using your full name and e-mail?

Cahyana,

Thanks for the analysis and research on how in general the challenge of parsable/quoting in output

No need to have my name as author in the PR, originally the issue was already detected in 2018 by somebody else.

I also do not need a hotfix rpm. Today I am already shipping the patch (the modified 220_lvm_layout.sh) myself with Puppet.


Peter

Hello Peter,

your proposed change is approved upstream. Thank you again for providing the patch. I also reviewed previous concerns to this approach raised in https://github.com/rear/rear/issues/1958 and found that they are sufficiently addressed.

Changing "columns" to "colons" (i.e. the punctuation mark ":") in description - I suppose this is what was meant.

By the way, in RHEL 9 the LVM filter will not work this way by default. If one wants the same behavior as in RHEL 8, one has to disable the use of device file, for example by by setting use_devicesfile to 0 in lvm.conf.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (rear bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2023:2900