curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

Thanks for the comments. I think I'm not going to make any changes in response to them right now.

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+

I don't know. This is all cosmetic really, as the wwn field 'wins' (as doko found out with his drives that have different serial but identical 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

/me tries to remember what this code is about.

On some systems at least, the /dev/mapper name is the "friendly name" so it's /dev/mapper/mpatha, not /dev/mapper/36005076306ffd6b60000000000002407. In the context of subiquity, it doesn't matter if the name used for path is unstable between boots but I guess for some other potential uses it might matter. OTOH, we also emit wwn and serial so... fwiw, we use the /dev/vdX path for virtio disks here, so perhaps just leaving path: /dev/dm-X would be more consistent? I wish I remember why I wrote it this way. Maybe just to match the tests.

Can we leave this for now and change it later?

> +        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.