Merge ~mwhudson/curtin:FR-2656-attach-device-to-action into curtin:master

[Michael Hudson-Doyle]

storage_config: add 'path' key to all actions that refer to a block device

Disks and partitions already get this, add it to raid, logical volume,
dm_crypt and bcache actions.

This is a bit of an RFC and basically the easiest way to do this. It adds a bunch of attributes that will be accepted by the schema but ignored on the input, which is potentially confusing. We could add a key like 'devpath' or even '_devpath' instead, we could even programmatically mangle the schemas so that it's not valid on input. What do you think?

(This needs doc updates but that can wait until we decide on the above point)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:91dd676ca6c62a4ec6c5236ff6faf6467bfe8d5c
https://jenkins.canonical.com/server-team/job/curtin-ci/34/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/34/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/34/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/34/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/34/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/34//rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:8c1ceaf95e7c6d39c996c3aab39a05f8c624df0d
https://jenkins.canonical.com/server-team/job/curtin-ci/36/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/36/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/36/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/36/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/36/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/36//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

So far so good.

I think the most curtin-like answer is that if a path is provided, it should be correct. So we could have code verifying that. We would then mark such path values as optional and most people can be blissfully unaware of the capability. Or we could log a warning that the values are ignored.

A solution I considered in other areas is that by definition any config output by curtin is valid curtin config, so we could decide to not include it in the schema. However, this is not an answer I love because you wouldn't be able to just feed a config output to curtin back to curtin.

In summary, I recommend we include path in the schema as 'path', and either verify it or something lesser (like the warning that the value is ignored in some cases).

[Michael Hudson-Doyle (mwhudson)]

Right I think it makes sense that you should be able to feed curtin output straight back into curtin. If we are going to move away from that the extra keys that are not valid for curtin should be something very clearly different like "$devpath$" or something but I think that is still pretty ugly. I think you can change the schema to enforce that any unknown attributes have to match a regex (like starting with _ or something) and then document that they are ignored.

I agree that it's most curtinish to check the path values are correct if passed. But that just feels like makework (even though it would probably take less time than adding this comment). I should probably just do it.

[Dan Bungert (dbungert)]

review: Approve

[Server Team CI bot (server-team-bot)]

Commit message lints:
- Line #0 has 3 too many characters. Line starts with: "storage_config: attach"...

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

diff --git a/curtin/block/schemas.py b/curtin/block/schemas.py
index 92f88d0..3278331 100644
--- a/curtin/block/schemas.py
+++ b/curtin/block/schemas.py
@@ -67,6 +67,7 @@ BCACHE = {
             'type': ['string'],
             'enum': ['writethrough', 'writeback', 'writearound', 'none'],
         },
+        'path': {'type': 'string', 'pattern': _path_dev},
     },
 }
 DASD = {
@@ -163,6 +164,7 @@ DM_CRYPT = {
         'volume': {'$ref': '#/definitions/ref_id'},
         'key': {'$ref': '#/definitions/id'},
         'keyfile': {'$ref': '#/definitions/id'},
+        'path': {'type': 'string', 'pattern': _path_dev},
         'preserve': {'$ref': '#/definitions/preserve'},
         'type': {'const': 'dm_crypt'},
     },
@@ -213,6 +215,7 @@ LVM_PARTITION = {
         'size': {'$ref': '#/definitions/size'},  # XXX: This is not used
         'type': {'const': 'lvm_partition'},
         'volgroup': {'$ref': '#/definitions/ref_id'},
+        'path': {'type': 'string', 'pattern': _path_dev},
     },
 }
 LVM_VOLGROUP = {
@@ -286,8 +289,7 @@ PARTITION = {
         'multipath': {'type': 'string'},
         # Permit path to device as output.
         # This value is ignored for input.
-        'path': {'type': 'string',
-                 'pattern': _path_dev},
+        'path': {'type': 'string', 'pattern': _path_dev},
         'name': {'$ref': '#/definitions/name'},
         'offset': {'$ref': '#/definitions/size'},  # XXX: This is not used
         'resize': {'type': 'boolean'},
@@ -335,6 +337,7 @@ RAID = {
         'name': {'$ref': '#/definitions/name'},
         'mdname': {'$ref': '#/definitions/name'},  # XXX: Docs need updating
         'metadata': {'type': ['string', 'number']},
+        'path': {'type': 'string', 'pattern': _path_dev},
         'preserve': {'$ref': '#/definitions/preserve'},
         'ptable': {'$ref': '#/definitions/ptable'},
         'wipe': {'$ref': '#/definitions/wipe'},
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 45bec55..d01333e 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -851,6 +851,15 @@ def partition_verify_fdasd(disk_path, partnumber, info):
         raise RuntimeError("dasd partitions do not support flags")
 
 
+def check_passed_path(info, actual_path):
+    if 'path' not in info:
+        return
+    passed_path = info['path']
+    if os.path.realpath(passed_path) != os.path.realpath(actual_path):
+        LOG.warning("%s action %s specified path %s but path was %s" % (
+            info["type"], info, info['path'], actual_path))
+
+
 def partition_handler(info, storage_config, context):
     device = info.get('device')
     size = info.get('size')
@@ -866,6 +875,7 @@ def partition_handler(info, storage_config, context):
     partnumber = determine_partition_number(info.get('id'), storage_config)
     disk_kname = block.path_to_kname(disk)
     part_path = block.dev_path(block.partition_kname(disk_kname, partnumber))
+    check_passed_path(info, part_path)
     context.id_to_device[info['id']] = part_path
 
     # consider the disks logical sector size when calculating sectors
@@ -1431,6 +1441,7 @@ def lvm_partition_handler(info, storage_config, context):
     lvm.lvm_scan()
 
     lv_path = get_path_to_storage_volume(info['id'], storage_config)
+    check_passed_path(info, lv_path)
     context.id_to_device[info['id']] = lv_path
 
     wipe_mode = info.get('wipe', 'superblock')
@@ -1466,6 +1477,7 @@ def dm_crypt_handler(info, storage_config, context):
     if not dm_name:
         dm_name = info.get('id')
     dmcrypt_dev = os.path.join("/dev", "mapper", dm_name)
+    check_passed_path(info, dmcrypt_dev)
     context.id_to_device[info['id']] = dmcrypt_dev
     preserve = config.value_as_boolean(info.get('preserve'))
     if not volume:
@@ -1606,6 +1618,7 @@ def raid_handler(info, storage_config, context):
     raidlevel = info.get('raidlevel')
     spare_devices = info.get('spare_devices')
     md_devname = block.md_path(info.get('name'))
+    check_passed_path(info, md_devname)
     context.id_to_device[info['id']] = md_devname
     container = info.get('container')
     metadata = info.get('metadata')
@@ -1784,6 +1797,7 @@ def bcache_handler(info, storage_config, context):
         bcache_dev = bcache.create_backing_device(backing_device, cache_device,
                                                   cache_mode, cset_uuid)
         # Not sure what to do in the preserve case here.
+        check_passed_path(info, bcache_dev)
         context.id_to_device[info['id']] = bcache_dev
 
     if cache_mode and not backing_device:
diff --git a/curtin/storage_config.py b/curtin/storage_config.py
index e9e8991..9c522d8 100644
--- a/curtin/storage_config.py
+++ b/curtin/storage_config.py
@@ -589,10 +589,11 @@ class BcacheParser(ProbertParser):
         backing_device = backing_data.get('blockdev')
         cache_device = _find_cache_device(backing_data, self.caching)
         cache_mode = _cache_mode(backing_data)
-        bcache_name = os.path.basename(_find_bcache_devname(backing_uuid,
-                                       backing_data, self.blockdev_data))
+        devname = _find_bcache_devname(backing_uuid,
+                                       backing_data, self.blockdev_data)
+        bcache_name = os.path.basename(devname)
         bcache_entry = {'type': 'bcache', 'id': 'disk-%s' % bcache_name,
-                        'name': bcache_name}
+                        'name': bcache_name, 'path': devname}
 
         if cache_mode:
             bcache_entry['cache_mode'] = cache_mode
@@ -911,6 +912,8 @@ class LvmParser(ProbertParser):
         return {'type': 'lvm_partition',
                 'id': 'lvm-partition-%s' % lv_config['name'],
                 'name': lv_config['name'],
+                'path': self.lookup_devname('/dev/{}/{}'.format(
+                    lv_config['volgroup'], lv_config['name'])),
                 'size': lv_config['size'],
                 'volgroup': 'lvm-volgroup-%s' % lv_config['volgroup']}
 
@@ -1020,6 +1023,7 @@ class DmcryptParser(ProbertParser):
 
         return {'type': 'dm_crypt',
                 'id': 'dmcrypt-%s' % crypt_name,
+                'path': bdev,
                 'volume': bdev_id,
                 'key': '',
                 'dm_name': crypt_name}
@@ -1058,6 +1062,7 @@ class RaidParser(ProbertParser):
             'type': 'raid',
             'id': self.blockdev_to_id(raid_data),
             'name': raidname,
+            'path': devname,
             'raidlevel': raid_data.get('raidlevel'),
             }
 
diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
index aab00e6..1225ee0 100644
--- a/doc/topics/storage.rst
+++ b/doc/topics/storage.rst
@@ -72,6 +72,12 @@ commands include:
 - Zpool Command (``zpool``) **Experimental**
 - ZFS Command (``zfs``)) **Experimental**
 
+Any action that refers to a block device (so things like ``partition``
+and ``dm_crypt`` but not ``lvm_volgroup`` or ``mount``, for example)
+*can* specify a ``path`` key but aside from ``disk`` actions this is
+not used to locate the device. A warning will be emitted if the
+located device does not match the provided path though.
+
 Dasd Command
 ~~~~~~~~~~~~
 
diff --git a/tests/unittests/test_storage_config.py b/tests/unittests/test_storage_config.py
index df48a4d..a109dd7 100644
--- a/tests/unittests/test_storage_config.py
+++ b/tests/unittests/test_storage_config.py
@@ -202,6 +202,7 @@ class TestBcacheParser(CiTestCase):
             'type': 'bcache',
             'id': 'disk-bcache0',
             'name': 'bcache0',
+            'path': '/dev/bcache0',
             'backing_device': 'partition-sda3',
             'cache_device': 'partition-nvme0n1p1',
             'cache_mode': 'writeback',
@@ -221,6 +222,7 @@ class TestBcacheParser(CiTestCase):
             'type': 'bcache',
             'id': 'disk-bcache0',
             'name': 'bcache0',
+            'path': '/dev/bcache0',
             'backing_device': 'partition-sda3',
             'cache_mode': 'writeback',
         }
@@ -674,6 +676,7 @@ class TestLvmParser(CiTestCase):
         expected_dict = {
             'type': 'lvm_partition',
             'name': 'my-storage',
+            'path': '/dev/dm-2',
             'id': 'lvm-partition-my-storage',
             'size': '1073741824B',
             'volgroup': 'lvm-volgroup-ubuntu-vg',
@@ -720,6 +723,7 @@ class TestRaidParser(CiTestCase):
             'type': 'raid',
             'id': 'raid-md0',
             'name': 'md0',
+            'path': '/dev/md0',
             'metadata': '1.2',
             'raidlevel': 'raid5',
             'devices': ['disk-vde', 'disk-vdf', 'disk-vdg'],
@@ -744,6 +748,7 @@ class TestRaidParser(CiTestCase):
             'type': 'raid',
             'id': 'raid-md127',
             'name': 'md127',
+            'path': '/dev/md127',
             'metadata': 'imsm',
             'raidlevel': 'container',
             'devices': ['disk-nvme0n1', 'disk-nvme1n1'],
@@ -758,6 +763,7 @@ class TestRaidParser(CiTestCase):
             'type': 'raid',
             'id': 'raid-md126',
             'name': 'md126',
+            'path': '/dev/md126',
             'raidlevel': 'raid0',
             'container': 'raid-md127',
             }
@@ -851,6 +857,7 @@ class TestDmCryptParser(CiTestCase):
             'type': 'dm_crypt',
             'id': 'dmcrypt-dmcrypt0',
             'dm_name': devname,
+            'path': '/dev/dm-2',
             'key': '',
             'volume': 'lvm-partition-lv3',
         }
@@ -1024,6 +1031,7 @@ class TestExtractStorageConfig(CiTestCase):
         self.assertEqual(1, len(raid_partitions))
         self.assertEqual({'id': 'raid-md1', 'type': 'raid', 'metadata': '1.2',
                           'raidlevel': 'raid1', 'name': 'md1',
+                          'path': '/dev/md1',
                           'devices': ['partition-vdb1', 'partition-vdc1'],
                           'spare_devices': []}, raids[0])
         self.assertEqual({