curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

> > > But in groovy+, the udev rule from multipath-tools that has
> > > always attempted to remove the devices nodes for the partitions
> > > of a disk that is a multipath member actually succeeds, and
> >
> > Won't this break curtin on older multipaths that don't have this rule?
>
> No, if the device node for a partition of a disk that is a multipath member is still present, this branch ignores it (or at least that's the intent...)
>
> > Or will this work with older multipath output since we're only referring to
> > the mpath dev (rather than a path of the mpath device)?
>
> Er, I think so. Not completely sure about the distinction you're making here.

In the description, in groovy+ we will no longer have the partitions on the
path members IIUC.  For example (say a 2 path disk with 1 partition) we'd see:

/dev/sda
/dev/sda1
/dev/sdb
/dev/sdb1
/dev/dm-0 (mpatha)
/dev/dm-1 (mpatha-part1)

And on Groovy+ we see:

/dev/sda
/dev/sdb
/dev/dm-0 (mpatha)
/dev/dm-1 (mpatha-part1)

Is that correct?

My concern was if the single-path partitions are present (as they are on
Focal and older) does the new code still handle things OK?

I *think* it's yes due to the fact that curtin knows how to create partitions
on MP devices since 20.04.

I've added some replies in-line, awkwardly, you have to select your previous
commit to see those.


Diff comments:

> diff --git a/curtin/storage_config.py b/curtin/storage_config.py
> index 494b142..658ebb3 100644
> --- a/curtin/storage_config.py
> +++ b/curtin/storage_config.py
> @@ -453,72 +453,17 @@ class ProbertParser(object):
>  
>          return None
>  
> -    def is_mpath(self, blockdev):
> -        if blockdev.get('DM_MULTIPATH_DEVICE_PATH') == "1":
> -            return True
> -
> -        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_disk(self, blockdev):
> +        return blockdev.get('DM_UUID', '').startswith('mpath-')
> +
> +    def is_mpath_partition(self, blockdev):
> +        dm_uuid = blockdev.get('DM_UUID')

OK, then we should also update curtin/block/multipath.py which I think also uses DM_PART in udev output as an indication of multipath-partition (rather than device-mapper partition).

> +        if dm_uuid is None:
> +            return False
> +        parts = dm_uuid.split('-', 2)
> +        if len(parts) < 2:
> +            return False
> +        return parts[0].startswith('part') and parts[1] == 'mpath'
>  
>      def blockdev_to_id(self, blockdev):
>          """ Examine a blockdev dictionary and return a tuple of curtin
> @@ -539,15 +484,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'

In the unittest, you expect this value to be present in the generated storage_config dict; it's not an action; it's a storage config which should validate with the schema.

> +                name = '{}-part{}'.format(
> +                    blockdev['DM_MPATH'], blockdev['DM_PART'])
>              elif is_dmcrypt(blockdev):
>                  devtype = 'dmcrypt'
>                  name = blockdev['DM_NAME']


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