curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

Thanks for the comments, some replies to the inline questions.

Can you look at https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/396462 first?

I changed the ProbertParser.is_mpath_* methods to just forward to the multipath.is_mpath_* functions, which meant I had to change one of the latter to work via udev. We don't need any of this knowledge even more spread around that it was already...

Diff comments:

> diff --git a/curtin/block/multipath.py b/curtin/block/multipath.py
> index 7ad1791..ef67165 100644
> --- a/curtin/block/multipath.py
> +++ b/curtin/block/multipath.py
> @@ -81,18 +81,18 @@ def is_mpath_partition(devpath, info=None):
>      if devpath.startswith('/dev/dm-'):
>          if not info:
>              info = udev.udevadm_info(devpath)
> -        if 'DM_PART' in udev.udevadm_info(devpath):
> +        if 'DM_PART' in info and 'DM_MPATH' in info:

To be definite about this requires understanding https://github.com/gebi/multipath-tools/blob/master/kpartx/kpartx_id#L41 which is written in a style of shell I find very hard to reason about but I think not. It would definitely be unexpected.

>              result = True
>  
>      LOG.debug("%s is multipath device partition? %s", devpath, result)
>      return result
>  
>  
> -def mpath_partition_to_mpath_id(devpath):
> -    """ Return the mpath id of a multipath partition. """
> +def mpath_partition_to_mpath_id_and_partnumber(devpath):
> +    """ Return the mpath id and partition number of a multipath partition. """
>      info = udev.udevadm_info(devpath)
> -    if 'DM_MPATH' in info:
> -        return info['DM_MPATH']
> +    if 'DM_MPATH' in info and 'DM_PART' in info:
> +        return info['DM_MPATH'], info['DM_PART']

This is from https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/396462 which it would actually make sense to review and agree on before this one.

>  
>      return None
>  
> diff --git a/curtin/storage_config.py b/curtin/storage_config.py
> index e6c33cc..dfe13a8 100644
> --- a/curtin/storage_config.py
> +++ b/curtin/storage_config.py
> @@ -453,72 +453,11 @@ class ProbertParser(object):
>  
>          return None
>  
> -    def is_mpath(self, blockdev):
> -        if blockdev.get('DM_MULTIPATH_DEVICE_PATH') == "1":
> -            return True
> +    def is_mpath_disk(self, blockdev):
> +        return blockdev.get('DM_UUID', '').startswith('mpath-')

The diff is confusing here. I'm not rewriting this function, I'm removing is_mpath (which asks "is blockdev a multipath member") and adding is_mpath_disk (which asks "is blockdev a multipathed disk"). I should retain the former though, as your comments below suggest.

>  
> -        return bool('mpath-' in blockdev.get('DM_UUID', ''))
> -
> -    def get_mpath_name(self, blockdev):
> -        mpath_data = self.probe_data.get('multipath')
> -        if not mpath_data:
> -            return None
> -
> -        bd_name = blockdev['DEVNAME']
> -        if blockdev['DEVTYPE'] == 'partition':
> -            bd_name = self.partition_parent_devname(blockdev)
> -        bd_name = os.path.basename(bd_name)
> -        for path in mpath_data.get('paths', []):
> -            if bd_name == path.get('device'):
> -                rv = path.get('multipath')
> -                return rv
> -
> -    def find_mpath_member(self, blockdev):
> -        if blockdev.get('DM_MULTIPATH_DEVICE_PATH') == "1":
> -            # find all other DM_MULTIPATH_DEVICE_PATH devs with same serial
> -            serial = blockdev.get('ID_SERIAL')
> -            members = sorted([os.path.basename(dev['DEVNAME'])
> -                              for dev in self.blockdev_data.values()
> -                              if dev.get("ID_SERIAL", "") == serial and
> -                              dev['DEVTYPE'] == blockdev['DEVTYPE']])
> -            # [/dev/sda, /dev/sdb]
> -            # [/dev/sda1, /dev/sda2, /dev/sdb1, /dev/sdb2]
> -
> -        else:
> -            dm_mpath = blockdev.get('DM_MPATH')
> -            dm_uuid = blockdev.get('DM_UUID')
> -            dm_part = blockdev.get('DM_PART')
> -            dm_name = blockdev.get('DM_NAME')
> -
> -            if dm_mpath:
> -                multipath = dm_mpath
> -            elif dm_name:
> -                multipath = dm_name
> -            else:
> -                # part1-mpath-30000000000000064
> -                # mpath-30000000000000064
> -                # mpath-36005076306ffd6b60000000000002406
> -                match = re.search(r'mpath\-([a-zA-Z]*|\d*)+$', dm_uuid)
> -                if not match:
> -                    LOG.debug('Failed to extract multipath ID pattern from '
> -                              'DM_UUID value: "%s"', dm_uuid)
> -                    return None
> -                # remove leading 'mpath-'
> -                multipath = match.group(0)[6:]
> -            mpath_data = self.probe_data.get('multipath')
> -            if not mpath_data:
> -                return None
> -            members = sorted([path['device'] for path in mpath_data['paths']
> -                              if path['multipath'] == multipath])
> -
> -            # append partition number if present
> -            if dm_part:
> -                members = [member + dm_part for member in members]
> -
> -        if len(members):
> -            return members[0]
> -
> -        return None
> +    def is_mpath_partition(self, blockdev):
> +        return "DM_MPATH" in blockdev and "DM_PART" in blockdev
>  
>      def blockdev_to_id(self, blockdev):
>          """ Examine a blockdev dictionary and return a tuple of curtin
> @@ -539,15 +478,13 @@ class ProbertParser(object):
>              if 'DM_LV_NAME' in blockdev:
>                  devtype = 'lvm-partition'
>                  name = blockdev['DM_LV_NAME']
> -            elif self.is_mpath(blockdev):
> -                # extract a multipath member device
> -                member = self.find_mpath_member(blockdev)
> -                if member:
> -                    name = member
> -                else:
> -                    name = blockdev['DM_UUID']
> -                if 'DM_PART' in blockdev:
> -                    devtype = 'partition'
> +            elif self.is_mpath_disk(blockdev):
> +                devtype = 'mpath-disk'
> +                name = blockdev['DM_NAME']
> +            elif self.is_mpath_partition(blockdev):
> +                devtype = 'mpath-partition'

This 'devtype' is only used for the 'id' field of the output, not the 'type' -- it's purely cosmetic!

> +                name = '{}-part{}'.format(
> +                    blockdev['DM_MPATH'], blockdev['DM_PART'])
>              elif is_dmcrypt(blockdev):
>                  devtype = 'dmcrypt'
>                  name = blockdev['DM_NAME']
> @@ -681,10 +618,15 @@ class BlockdevParser(ProbertParser):
>          errors = []
>  
>          for devname, data in self.blockdev_data.items():
> -            # skip composed devices here, except partitions
> +            # skip composed devices here, except partitions and multipath
>              if data.get('DEVPATH', '').startswith('/devices/virtual/block'):
> -                if data.get('DEVTYPE', '') != "partition":
> -                    continue
> +                if not self.is_mpath_disk(data):
> +                    if not self.is_mpath_partition(data):
> +                        if data.get('DEVTYPE', '') != "partition":
> +                            continue
> +            # skip disks that are members of multipath devices
> +            if data.get('DM_MULTIPATH_DEVICE_PATH') == '1':

Well not exactly that but it should be is_mpath_member(data) or something, yes.

> +                continue
>              entry = self.asdict(data)
>              if entry:
>                  try:
> @@ -698,7 +640,10 @@ class BlockdevParser(ProbertParser):
>      def valid_id(self, id_value):
>          # reject wwn=0x0+
>          if id_value.lower().startswith('0x'):
> -            return int(id_value, 16) > 0
> +            try:
> +                return int(id_value, 16) > 0
> +            except ValueError:
> +                return True

I don't entirely know how but I had DM_WWN=0xQEMU_QEMU_HARDDISK_001 when testing in qemu. It's possible this will never come up with real hardware but well.

>          # accept non-empty (removing whitspace) strings
>          return len(''.join(id_value.split())) > 0
>  
> @@ -796,23 +751,35 @@ class BlockdevParser(ProbertParser):
>  
>          if entry['type'] == 'partition':
>              attrs = blockdev_data['attrs']
> -            entry['number'] = int(attrs['partition'])
> -            parent_devname = self.partition_parent_devname(blockdev_data)
> -            parent_blockdev = self.blockdev_data[parent_devname]
> -            if 'ID_PART_TABLE_TYPE' not in parent_blockdev:
> -                # Exclude the fake partition that the kernel creates
> -                # for an otherwise unformatted FBA dasd.
> -                dasds = self.probe_data.get('dasd', {})
> -                dasd_config = dasds.get(parent_devname, {})
> -                if dasd_config.get('type', 'ECKD') == 'FBA':
> -                    return None
> +            if self.is_mpath_partition(blockdev_data):
> +                entry['number'] = int(blockdev_data['DM_PART'])
> +                parent_mpath = blockdev_data['DM_MPATH']
> +                for data in self.blockdev_data.values():
> +                    if data.get('DM_NAME') == parent_mpath:
> +                        parent_blockdev = data
> +                        break
> +                else:
> +                    raise ValueError("cannot find parent")

Makes sense. I'm actually not especially happy about this chunk of code on re-reading, it depends on sfdisk details a bit much really.

> +                self_name = '/dev/mapper/{}-part{}'.format(
> +                    parent_mpath, entry['number'])
> +            else:
> +                entry['number'] = int(attrs['partition'])
> +                self_name = blockdev_data['DEVNAME']
> +                parent_devname = self.partition_parent_devname(blockdev_data)
> +                parent_blockdev = self.blockdev_data[parent_devname]
> +                if 'ID_PART_TABLE_TYPE' not in parent_blockdev:
> +                    # Exclude the fake partition that the kernel creates
> +                    # for an otherwise unformatted FBA dasd.
> +                    dasds = self.probe_data.get('dasd', {})
> +                    dasd_config = dasds.get(parent_devname, {})
> +                    if dasd_config.get('type', 'ECKD') == 'FBA':
> +                        return None
>              ptable = parent_blockdev.get('partitiontable')
>              if ptable:
>                  part = None
>                  for pentry in ptable['partitions']:
>                      node = pentry['node']
> -                    node_p = node.replace(parent_devname, '')
> -                    if node_p.replace('p', '') == attrs['partition']:
> +                    if node == self_name:
>                          part = pentry
>                          break
>  
> @@ -880,6 +847,9 @@ class FilesystemParser(ProbertParser):
>                  errors.append(err)
>                  continue
>  
> +            if blockdev_data.get('DM_MULTIPATH_DEVICE_PATH') == "1":

is_mpath_member()!

> +                continue
> +
>              # no floppy, no cdrom
>              if blockdev_data['MAJOR'] in ["11", "2"]:
>                  continue


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/392353
Your team curtin developers is subscribed to branch curtin:master.