curtin-dev team mailing list archive

[Dimitri John Ledkov <dimitri.ledkov+lp@xxxxxxxxxxxxx>]

In general this is good, and implements things as have been originally asked for in the "multipath google doc of doom".

See minor comments.

Also please rebase commits to have only descriptive commit messages without fixups =)

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
> @@ -710,10 +657,16 @@ class BlockdevParser(ProbertParser):
>              blockdev attribute.
>          """
>          uniq = {}
> -        source_keys = {
> -            'wwn': ['ID_WWN_WITH_EXTENSION', 'ID_WWN'],
> -            'serial': ['ID_SERIAL', 'ID_SERIAL_SHORT'],
> -        }
> +        if self.is_mpath_disk(blockdev):
> +            source_keys = {
> +                'wwn': ['DM_WWN'],
> +                'serial': ['DM_SERIAL'],  # only present with focal+

Do we need to regenerate ./tests/data/probert_storage_zlp6.json on focal, such that it has DM_SERIAL in the json?

Mostly for reference purposes, as it is a bit like wwn anyway. Not even sure why there is a new key/name. Or when it is not the same as DM_WWN.

> +            }
> +        else:
> +            source_keys = {
> +                'wwn': ['ID_WWN_WITH_EXTENSION', 'ID_WWN'],
> +                'serial': ['ID_SERIAL', 'ID_SERIAL_SHORT'],
> +            }
>          for skey, id_keys in source_keys.items():
>              for id_key in id_keys:
>                  if id_key in blockdev and skey not in uniq:
> @@ -751,20 +708,24 @@ class BlockdevParser(ProbertParser):
>              return None
>  
>          devname = blockdev_data.get('DEVNAME')
> +        path = devname
>          entry = {
> -            'type': blockdev_data['DEVTYPE'],
> +            'type': dev_type,
>              'id': self.blockdev_to_id(blockdev_data),
>          }
> -        if blockdev_data.get('DM_MULTIPATH_DEVICE_PATH') == "1":
> -            mpath_name = self.get_mpath_name(blockdev_data)
> -            if mpath_name:
> -                entry['multipath'] = mpath_name
> +        if self.is_mpath_disk(blockdev_data):
> +            entry['multipath'] = blockdev_data['DM_NAME']
> +            for link in blockdev_data['DEVLINKS'].split():
> +                if link.startswith('/dev/mapper'):
> +                    path = link

As per https://wiki.ubuntu.com/FSTAB, i thought we would have preffered to use the by-id symlink here, instead of the /dev/mapper one. I.e. /dev/disk/by-id/dm-uuid-mpath-36005076306ffd6b60000000000002407 no? althought that's like almost the same as /dev/mapper/36005076306ffd6b60000000000002407

And if we change this here, we'd also want to change the tests.

> +        elif self.is_mpath_partition(blockdev_data):
> +            entry['multipath'] = blockdev_data['DM_MPATH']
>  
>          # default disks to gpt
>          if entry['type'] == 'disk':
>              uniq_ids = self.get_unique_ids(blockdev_data)
>              # always include path, block_meta will prefer wwn/serial over path
> -            uniq_ids.update({'path': devname})
> +            uniq_ids.update({'path': path})
>              # set wwn, serial, and path
>              entry.update(uniq_ids)
>  


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