curtin-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Ryan Harper has proposed merging ~raharper/curtin:fix/block-meta-get-device-paths-skip-missing into curtin:master.

Commit message:
block-meta: skip wipe device paths if not present

Curtin extracts the devices from storage config with wipe enabled.
These devices may not exist yet (curtin will be creating them) so
there's no need to clear them, instead drop them from the list of
devices to send to clear-holders.

Other changes:

multipath:
 - Clarify multipath logging on device type checking
block-meta:
 - Stop iterating through disk lookup keys after we find one
 - Use udevadm_settle after parted partition creation before
   calling kpartx to prevent race with kernel partition update
clear-holders:
 - Log the inputs to clear-holders
block:
 - Log a warning when sys_block_path is called on a non-existent
   device path.
 - Log the return value from block.lookup_disk()

LP: #1869075

Requested reviews:
  curtin developers (curtin-dev)
Related bugs:
  Bug #1869075 in curtin: "lvcreate fails with duplicate paths to physical volume"
  https://bugs.launchpad.net/curtin/+bug/1869075

For more details, see:
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/382718
-- 
Your team curtin developers is requested to review the proposed merge of ~raharper/curtin:fix/block-meta-get-device-paths-skip-missing into curtin:master.

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index 54ee263..a7fe22f 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -143,6 +143,8 @@ def sys_block_path(devname, add=None, strict=True):
     toks = ['/sys/class/block']
     # insert parent dev if devname is partition
     devname = os.path.normpath(devname)
+    if devname.startswith('/dev/') and not os.path.exists(devname):
+        LOG.warning('block.sys_block_path: devname %s does not exist', devname)
     (parent, partnum) = get_blockdev_for_partition(devname, strict=strict)
     if partnum:
         toks.append(path_to_kname(parent))
@@ -906,6 +908,7 @@ def lookup_disk(serial):
     if not os.path.exists(path):
         raise ValueError("path '%s' to block device for disk with serial '%s' \
             does not exist" % (path, serial_udev))
+    LOG.debug('block.lookup_disk() returning path %s', path)
     return path
 
 
diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index 14ad644..279fad8 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -605,6 +605,7 @@ def clear_holders(base_paths, try_preserve=False):
     # handle single path
     if not isinstance(base_paths, (list, tuple)):
         base_paths = [base_paths]
+    LOG.info('Generating device storage trees for path(s): %s', base_paths)
 
     # get current holders and plan how to shut them down
     holder_trees = [gen_holders_tree(path) for path in base_paths]
diff --git a/curtin/block/multipath.py b/curtin/block/multipath.py
index a488e85..21fc0bd 100644
--- a/curtin/block/multipath.py
+++ b/curtin/block/multipath.py
@@ -52,35 +52,40 @@ def dmname_to_blkdev_mapping():
 
 def is_mpath_device(devpath, info=None):
     """ Check if devpath is a multipath device, returns boolean. """
+    result = False
     if not info:
         info = udev.udevadm_info(devpath)
     if info.get('DM_UUID', '').startswith('mpath-'):
-        LOG.debug('%s is multipath device', devpath)
-        return True
+        result = True
 
-    return False
+    LOG.debug('%s is multipath device? %s', devpath, result)
+    return result
 
 
 def is_mpath_member(devpath, info=None):
     """ Check if a device is a multipath member (a path), returns boolean. """
+    result = False
     try:
         util.subp(['multipath', '-c', devpath], capture=True)
-        LOG.debug('%s is multipath device member', devpath)
-        return True
+        result = True
     except util.ProcessExecutionError:
-        return False
+        pass
+
+    LOG.debug('%s is multipath device member? %s', devpath, result)
+    return result
 
 
 def is_mpath_partition(devpath, info=None):
     """ Check if a device is a multipath partition, returns boolean. """
+    result = False
     if devpath.startswith('/dev/dm-'):
         if not info:
             info = udev.udevadm_info(devpath)
         if 'DM_PART' in udev.udevadm_info(devpath):
-            LOG.debug("%s is multipath device partition", devpath)
-            return True
+            result = True
 
-    return False
+    LOG.debug("%s is multipath device partition? %s", devpath, result)
+    return result
 
 
 def mpath_partition_to_mpath_id(devpath):
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 36d3146..7fbd89f 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -465,7 +465,9 @@ def get_path_to_storage_volume(volume, storage_config):
             except ValueError:
                 continue
             # verify path exists otherwise try the next key
-            if not os.path.exists(volume_path):
+            if os.path.exists(volume_path):
+                break
+            else:
                 volume_path = None
 
         if volume_path is None:
@@ -906,8 +908,9 @@ def partition_handler(info, storage_config):
 
         # ensure partition exists
         if multipath.is_mpath_device(disk):
+            udevadm_settle()  # allow partition creation to happen
             # update device mapper table mapping to mpathX-partN
-            util.subp(['kpartx', '-v', '-a', '-p-part', disk])
+            util.subp(['kpartx', '-v', '-a', '-s', '-p', '-part', disk])
             part_path = disk + "-part%s" % partnumber
         else:
             part_path = block.dev_path(block.partition_kname(disk_kname,
@@ -1697,7 +1700,7 @@ def zfs_handler(info, storage_config):
 
 def get_device_paths_from_storage_config(storage_config):
     """Returns a list of device paths in a storage config which have wipe
-       config enabled.
+       config enabled filtering out constructed paths that do not exist.
 
     :param: storage_config: Ordered dict of storage configation
     """
@@ -1706,8 +1709,10 @@ def get_device_paths_from_storage_config(storage_config):
         if v.get('type') in ['disk', 'partition']:
             if config.value_as_boolean(v.get('wipe')):
                 try:
-                    dpaths.append(
-                        get_path_to_storage_volume(k, storage_config))
+                    # skip paths that do not exit, nothing to wipe
+                    dpath = get_path_to_storage_volume(k, storage_config)
+                    if os.path.exists(dpath):
+                        dpaths.append(dpath)
                 except Exception:
                     pass
     return dpaths
diff --git a/examples/tests/multipath-lvm.yaml b/examples/tests/multipath-lvm.yaml
index 1215d8b..68c3271 100644
--- a/examples/tests/multipath-lvm.yaml
+++ b/examples/tests/multipath-lvm.yaml
@@ -69,24 +69,28 @@ storage:
         name: root_disk
         multipath: mpatha
         serial: disk-a
+        path: /dev/sda
         grub_device: true
-        wipe: superblock-recursive
+        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
diff --git a/examples/tests/multipath.yaml b/examples/tests/multipath.yaml
index 4bafe2d..11838d1 100644
--- a/examples/tests/multipath.yaml
+++ b/examples/tests/multipath.yaml
@@ -11,17 +11,21 @@ storage:
         name: mpath_a
         wipe: superblock
         grub_device: true
+        multipath: mpatha
+        path: /dev/sda
       - id: sda1
         type: partition
         number: 1
         size: 3GB
         device: sda
         flag: boot
+        wipe: superblock
       - id: sda2
         type: partition
         number: 2
         size: 1GB
         device: sda
+        wipe: superblock
       - id: sda1_root
         type: format
         fstype: ext4
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index dcdcf51..2b0116b 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -658,7 +658,7 @@ class TestSlaveKnames(CiTestCase):
         # construct side effects to os.path.exists
         # and os.listdir based on mapping.
         dirs = []
-        exists = []
+        exists = [True] if device.startswith('/dev') else []
         for (dev, slvs) in cfg.items():
             # sys_block_dev checks if dev exists
             exists.append(True)