curtin-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Ryan Harper has proposed merging ~raharper/curtin:fix/lvm-over-multipath-partition-wipe into curtin:master.

Commit message:
lvm-over-multipath: handle lookups of multipath members

Subiquity provides a 'path' value for disks in storage config.
This triggered an edge case where curtin attempted to wipe the
underlying scsi disk *while* multipath is enabled resulting in
an error when attempting to get exclusive access to the disk.

This branch resolves this by checking if a disk or partition
is related to multipath and if so returning the device mapper
disk path.

LP: #1837214

Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/382605
-- 
Your team curtin developers is requested to review the proposed merge of ~raharper/curtin:fix/lvm-over-multipath-partition-wipe into curtin:master.

diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index 4ead97e..14ad644 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -297,6 +297,10 @@ def wipe_superblock(device):
             mpath_id = multipath.find_mpath_id(blockdev)
             for mp_part_id in multipath.find_mpath_partitions(mpath_id):
                 multipath.remove_partition(mp_part_id)
+        # handle /dev/sdX which are held by multipath layer
+        if multipath.is_mpath_member(blockdev):
+            LOG.debug('Skipping multipath partition path member: %s', blockdev)
+            return
 
     _wipe_superblock(blockdev)
 
diff --git a/curtin/block/multipath.py b/curtin/block/multipath.py
index f0dfbd2..a488e85 100644
--- a/curtin/block/multipath.py
+++ b/curtin/block/multipath.py
@@ -55,6 +55,7 @@ def is_mpath_device(devpath, info=None):
     if not info:
         info = udev.udevadm_info(devpath)
     if info.get('DM_UUID', '').startswith('mpath-'):
+        LOG.debug('%s is multipath device', devpath)
         return True
 
     return False
@@ -64,6 +65,7 @@ def is_mpath_member(devpath, info=None):
     """ Check if a device is a multipath member (a path), returns boolean. """
     try:
         util.subp(['multipath', '-c', devpath], capture=True)
+        LOG.debug('%s is multipath device member', devpath)
         return True
     except util.ProcessExecutionError:
         return False
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index e3c4ce1..ac9f964 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -455,6 +455,10 @@ def get_path_to_storage_volume(volume, storage_config):
                         # sys/class/block access is valid.  ie, there are no
                         # udev generated values in sysfs
                         volume_path = os.path.realpath(vol_value)
+                    # convert /dev/sdX to /dev/mapper/mpathX value
+                    if multipath.is_mpath_member(volume_path):
+                        volume_path = '/dev/mapper/' + (
+                            multipath.get_mpath_id_from_device(volume_path))
                 elif disk_key == 'device_id':
                     dasd_device = dasd.DasdDevice(vol_value)
                     volume_path = dasd_device.devname
diff --git a/examples/tests/multipath-lvm-part-wipe.yaml b/examples/tests/multipath-lvm-part-wipe.yaml
new file mode 100644
index 0000000..bfe39ea
--- /dev/null
+++ b/examples/tests/multipath-lvm-part-wipe.yaml
@@ -0,0 +1,125 @@
+showtrace: true
+install:
+  unmount: disabled
+bucket:
+  - &setup |
+    export DEBIAN_FRONTEND=noninteractive
+    aptopts="--quiet --assume-yes"
+    aptopts="$aptopts --option=Dpkg::options::=--force-unsafe-io"
+    aptopts="$aptopts --option=Dpkg::Options::=--force-confold"
+    if ! command -v multipath; then
+        eatmydata apt-get install -y $aptopts multipath-tools
+        echo -e "defaults {\n    user_friendly_names yes\n}" > /etc/multipath.conf
+        udevadm trigger --subsystem-match=block --action=add
+        udevadm settle
+        multipath -v3 -R3 -r
+    fi
+    multipath -ll
+    dmsetup remove --force --retry /dev/dm-0
+    dmsetup ls
+    multipath -v3 -R3 -f mpatha
+    udevadm settle
+    parted /dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_disk-a --script -- \
+        mklabel gpt              \
+        mkpart primary 1MiB 2MiB \
+        set 1 bios_grub on \
+        mkpart primary 3MiB 999MiB \
+        set 2 boot on \
+        mkpart primary 1000MiB 4099MiB
+    udevadm settle
+    ls -al /dev/disk/by-id
+    vgcreate --force --zero=y --yes root_vg /dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_disk-a-part3
+    pvscan --cache
+    vgscan --verbose --mknodes --cache
+    udevadm settle
+    for x in $(seq 1 10); do
+        if vgs root_vg; then
+            break;
+        fi
+        sleep 1
+    done
+    lvcreate root_vg --name lv1_root --zero=y --wipesignatures=y \
+        --size 2684354560B
+    udevadm settle
+    mkfs.ext4 /dev/root_vg/lv1_root
+    # stop lvm bits
+    for vg in `pvdisplay -C --separator = -o vg_name --noheadings`; do
+        vgchange -an $vg ||:
+    done
+    command -v systemctl && systemctl mask lvm2-pvscan\@.service
+    rm -rf /etc/lvm/archive /etc/lvm/backup  /run/lvm/*
+    multipath -r
+    udevadm settle
+    dmsetup ls
+    multipath -ll
+    ls -al /dev/mapper/
+    sleep 5
+    ls -al /sys/class/block/dm-0/holders/
+    /curtin/bin/curtin \
+        -v --config /curtin/config/multipath-lvm-part-wipe.yaml \
+        clear-holders --shutdown-plan
+
+
+# Create a LVM now to test curtin's reuse of existing LVMs
+early_commands:
+  00-setup-lvm: [bash, -exuc, *setup]
+
+storage:
+    version: 1
+    config:
+      - id: main_disk
+        type: disk
+        ptable: gpt
+        name: root_disk
+        multipath: mpatha
+        serial: disk-a
+        path: /dev/sda
+        grub_device: true
+        wipe: superblock
+      - id: boot_bios
+        type: partition
+        size: 1MB
+        number: 1
+        device: main_disk
+        flag: bios_grub
+        wipe: superblock
+      - id: boot_partition
+        type: partition
+        size: 1GB
+        number: 2
+        device: main_disk
+        wipe: superblock
+      - id: main_disk_p3
+        type: partition
+        number: 3
+        size: 4GB
+        device: main_disk
+        wipe: superblock
+      - id: root_vg
+        type: lvm_volgroup
+        name: root_vg
+        devices:
+            - main_disk_p3
+      - id: root_vg_lv1
+        type: lvm_partition
+        name: lv1_root
+        size: 2.5G
+        volgroup: root_vg
+      - id: lv1_root_fs
+        type: format
+        fstype: ext4
+        volume: root_vg_lv1
+      - id: lvroot_mount
+        path: /
+        type: mount
+        device: lv1_root_fs
+      - fstype: ext4
+        volume: boot_partition
+        preserve: false
+        type: format
+        id: format-0
+      - device: format-0
+        path: /boot
+        type: mount
+        id: mount-0
+
diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
index 767ae3a..f73b49d 100644
--- a/tests/unittests/test_clear_holders.py
+++ b/tests/unittests/test_clear_holders.py
@@ -492,7 +492,7 @@ class TestClearHolders(CiTestCase):
                                                            mock_log, m_time,
                                                            mock_swap,
                                                            mock_mpath):
-        """wipe_superblock wipes dev with multipath enabled but inactive."""
+        """wipe_superblock skips wiping multipath member path."""
         mock_swap.return_value = False
         mock_block.sysfs_to_devpath.return_value = self.test_blockdev
         mock_block.is_extended_partition.return_value = False
@@ -508,8 +508,7 @@ class TestClearHolders(CiTestCase):
         ])
         clear_holders.wipe_superblock(self.test_syspath)
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
-        mock_block.wipe_volume.assert_called_with(
-            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+        self.assertEqual(0, mock_block.wipe_volume.call_count)
 
     @mock.patch('curtin.block.clear_holders.multipath')
     @mock.patch('curtin.block.clear_holders.is_swap_device')
@@ -548,7 +547,7 @@ class TestClearHolders(CiTestCase):
                                                               mock_swap,
                                                               mock_mpath,
                                                               m_get_holders):
-        """wipe_superblock wipes mpath device and removes mp parts first."""
+        """wipe_superblock wipes removes mp parts first and wipes dev."""
         mock_swap.return_value = False
         mock_block.sysfs_to_devpath.return_value = self.test_blockdev
         mock_block.is_zfs_member.return_value = False
@@ -563,12 +562,13 @@ class TestClearHolders(CiTestCase):
         mock_block.get_sysfs_partitions.side_effect = iter([
             [],  # partitions are now gone
         ])
+        mock_mpath.is_mpath_member.return_value = False
         m_get_holders.return_value = []
         clear_holders.wipe_superblock(self.test_syspath)
         mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
+        mock_mpath.remove_partition.assert_called_with('mpatha-part1')
         mock_block.wipe_volume.assert_called_with(
             self.test_blockdev, exclusive=True, mode='superblock', strict=True)
-        mock_mpath.remove_partition.assert_called_with('mpatha-part1')
 
     @mock.patch('curtin.block.clear_holders.LOG')
     @mock.patch('curtin.block.clear_holders.block')
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index 155c3ba..079984f 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -584,8 +584,10 @@ class TestSetupGrub(CiTestCase):
                 self.target, '/dev/vdb'],),
             self.mock_subp.call_args_list[0][0])
 
+    @patch('curtin.commands.block_meta.multipath')
     @patch('curtin.commands.curthooks.os.path.exists')
-    def test_uses_grub_install_on_storage_config(self, m_exists):
+    def test_uses_grub_install_on_storage_config(self, m_exists, m_multipath):
+        m_multipath.is_mpath_member.return_value = False
         cfg = {
             'storage': {
                 'version': 1,
@@ -600,6 +602,7 @@ class TestSetupGrub(CiTestCase):
             },
         }
         self.subp_output.append(('', ''))
+        self.subp_output.append(('', ''))
         m_exists.return_value = True
         curthooks.setup_grub(cfg, self.target, osfamily=self.distro_family)
         self.assertEquals(
@@ -609,10 +612,12 @@ class TestSetupGrub(CiTestCase):
                 self.target, '/dev/vdb'],),
             self.mock_subp.call_args_list[0][0])
 
+    @patch('curtin.commands.block_meta.multipath')
     @patch('curtin.block.is_valid_device')
     @patch('curtin.commands.curthooks.os.path.exists')
     def test_uses_grub_install_on_storage_config_uefi(
-            self, m_exists, m_is_valid_device):
+            self, m_exists, m_is_valid_device, m_multipath):
+        m_multipath.is_mpath_member.return_value = False
         self.mock_is_uefi_bootable.return_value = True
         cfg = {
             'storage': {
diff --git a/tests/vmtests/test_multipath_lvm.py b/tests/vmtests/test_multipath_lvm.py
index c2a5d18..39b8587 100644
--- a/tests/vmtests/test_multipath_lvm.py
+++ b/tests/vmtests/test_multipath_lvm.py
@@ -64,4 +64,13 @@ class FocalTestMultipathLvm(relbase.focal, TestMultipathLvmAbs):
     __test__ = True
 
 
+class TestMultipathLvmPartWipeAbs(TestMultipathLvmAbs):
+    conf_file = "examples/tests/multipath-lvm-part-wipe.yaml"
+
+
+class FocalTestMultipathLvmPartWipe(relbase.focal,
+                                    TestMultipathLvmPartWipeAbs):
+    __test__ = True
+
+
 # vi: ts=4 expandtab syntax=python