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.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
