Merge ~mwhudson/curtin:v2-fixes into curtin:master

[Michael Hudson-Doyle]

I don't expect to commit this as is, but I do want to see how it does in CI.

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

FAILED: Continuous integration, rev:3924a3d671b242b20c95ebd266d56f189a67833c

No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself):
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/415779/+edit-commit-message

https://jenkins.ubuntu.com/server/job/curtin-ci/244/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/244/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/244/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/244/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/244/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/244//rebuild

review: Needs Fixing (continuous-integration)

f525099... by Michael Hudson-Doyle on 2022-02-21

lazily call sfdisk_info

because calling sfdisk_info when there is no partition table makes the vmtests fail

[Michael Hudson-Doyle (mwhudson)]

The vmtests pass now for me -- mostly. There is definitely a race or two lurking.

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

FAILED: Continuous integration, rev:f52509936f29c993f47900dcfa4ca52682d260cd

No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself):
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/415779/+edit-commit-message

https://jenkins.ubuntu.com/server/job/curtin-ci/245/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/245/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/245/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/245/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/245/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/245//rebuild

review: Needs Fixing (continuous-integration)

a9a241a... by Michael Hudson-Doyle on 2022-02-21

debugging by printing

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

FAILED: Continuous integration, rev:a9a241a3e5d127fccdd68ed6135bf360fd3bffe0

No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself):
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/415779/+edit-commit-message

https://jenkins.ubuntu.com/server/job/curtin-ci/246/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/246/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/246/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/246/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/246/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/246//rebuild

review: Needs Fixing (continuous-integration)

127ddf6... by Michael Hudson-Doyle on 2022-02-21

what

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

FAILED: Continuous integration, rev:127ddf64bcbabbd1fc17d2073090e75965eea52b

No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself):
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/415779/+edit-commit-message

https://jenkins.ubuntu.com/server/job/curtin-ci/247/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/247/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/247/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/247/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/247/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/247//rebuild

review: Needs Fixing (continuous-integration)

79b2d7c... by Michael Hudson-Doyle on 2022-02-21

write a small novel about why we call udevadm settle here

767c011... by Michael Hudson-Doyle on 2022-02-21

refactor things to allow a test case to override storage config version, add some v2 tests

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

FAILED: Continuous integration, rev:767c01129935de2630367b0baca328015913d9ec

No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself):
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/415779/+edit-commit-message

https://jenkins.ubuntu.com/server/job/curtin-ci/248/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/248/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/248/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/248/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/248/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/248//rebuild

review: Needs Fixing (continuous-integration)

Unmerged commits

767c011... by Michael Hudson-Doyle on 2022-02-21

refactor things to allow a test case to override storage config version, add some v2 tests

79b2d7c... by Michael Hudson-Doyle on 2022-02-21

write a small novel about why we call udevadm settle here

127ddf6... by Michael Hudson-Doyle on 2022-02-21

what

a9a241a... by Michael Hudson-Doyle on 2022-02-21

debugging by printing

f525099... by Michael Hudson-Doyle on 2022-02-21

lazily call sfdisk_info

because calling sfdisk_info when there is no partition table makes the vmtests fail

3924a3d... by Michael Hudson-Doyle on 2022-02-18

convert examples/tests/basic.yaml to v2

and some fixes to make it work

49c35be... by Michael Hudson-Doyle on 2022-02-18

block_meta_v2: fix implicit offset calculation for dos partitions

5e46883... by Michael Hudson-Doyle on 2022-02-18

block_meta_v2: do not use aliases for partition types

These are not supported by sfdisk in focal.

diff --git a/curtin/block/deps.py b/curtin/block/deps.py
index 38581a8..db449d8 100644
--- a/curtin/block/deps.py
+++ b/curtin/block/deps.py
@@ -96,8 +96,12 @@ def detect_required_packages_mapping(osfamily=DISTROS.debian):
     if osfamily not in distro_mapping:
         raise ValueError('No block package mapping for distro: %s' % osfamily)
 
-    return {1: {'handler': storage_config_required_packages,
-                'mapping': distro_mapping.get(osfamily)}}
+    cfg_map = {
+        'handler': storage_config_required_packages,
+        'mapping': distro_mapping.get(osfamily),
+        }
+
+    return {1: cfg_map, 2: cfg_map}
 
 
 # vi: ts=4 expandtab syntax=python
diff --git a/curtin/commands/block_meta_v2.py b/curtin/commands/block_meta_v2.py
index 772c347..a2a0708 100644
--- a/curtin/commands/block_meta_v2.py
+++ b/curtin/commands/block_meta_v2.py
@@ -5,6 +5,7 @@ from typing import Optional
 import attr
 
 from curtin import (block, util)
+from curtin.udev import udevadm_settle
 from curtin.commands.block_meta import (
     disk_handler as disk_handler_v1,
     get_path_to_storage_volume,
@@ -53,6 +54,11 @@ def align_down(size, block_size):
 FLAG_TO_GUID = {
     flag: guid for (guid, (flag, typecode)) in GPT_GUID_TO_CURTIN_MAP.items()
     }
+FLAG_TO_MBR_TYPE = {
+    flag: typecode[:2].upper() for (guid, (flag, typecode))
+    in GPT_GUID_TO_CURTIN_MAP.items()
+    }
+FLAG_TO_MBR_TYPE['extended'] = '05'
 
 
 class SFDiskPartTable:
@@ -69,10 +75,16 @@ class SFDiskPartTable:
     def apply(self, device):
         sfdisk_script = self.render()
         LOG.debug("sfdisk input:\n---\n%s\n---\n", sfdisk_script)
-        util.subp(
-            ['sfdisk', '--lock', '--no-tell', device],
-            data=sfdisk_script.encode('ascii'))
-        util.subp(['partprobe', device])
+        util.subp(['sfdisk', device], data=sfdisk_script.encode('ascii'))
+        # sfdisk (as invoked here) uses ioctls to inform the kernel that the
+        # partition table has changed so it can add and remove device nodes for
+        # the partitions as needed. Unfortunately this is asynchronous: sfdisk
+        # can exit before the nodes are present in /dev (or /sys for that
+        # matter). Calling "udevadm settle" is slightly incoherent as udev has
+        # nothing to do with creating these nodes, but at the same time, udev
+        # won't finish processing the events triggered by the sfdisk until
+        # after the nodes for the partitions have been updated by the kernel.
+        udevadm_settle()
 
 
 class GPTPartTable(SFDiskPartTable):
@@ -144,20 +156,12 @@ class DOSPartTable(SFDiskPartTable):
                 for entry in self.entries:
                     if entry.number <= 4:
                         prev = entry
-                        break
                 if prev is None:
                     start = ONE_MIB_SECTORS
                 else:
                     start = align_up(prev.start + prev.size, ONE_MIB_SECTORS)
         size = int(util.human2bytes(action['size'])) // SECTOR_BYTES
-        FLAG_TO_TYPE = {
-            'extended': 'extended',
-            'boot': 'uefi',
-            'swap': 'swap',
-            'lvm': 'lvm',
-            'raid': 'raid',
-            }
-        type = FLAG_TO_TYPE.get(flag)
+        type = FLAG_TO_MBR_TYPE.get(flag)
         if flag == 'boot':
             bootable = True
         else:
@@ -217,10 +221,14 @@ def disk_handler_v2(info, storage_config, handlers):
     preserved_offsets = set()
     wipes = {}
 
-    sfdisk_info = block.sfdisk_info(disk)
+    sfdisk_info = None
     for action in part_actions:
+        LOG.debug('processing partition action %s', action)
         entry = table.add(action)
+        LOG.debug('made entry %s', entry)
         if action.get('preserve', False):
+            if sfdisk_info is None:
+                sfdisk_info = block.sfdisk_info(disk)
             part_info = _find_part_info(sfdisk_info, entry.start)
             partition_verify_sfdisk(action, sfdisk_info['label'], part_info)
             preserved_offsets.add(entry.start)
diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py
index fd119c5..a415c20 100644
--- a/tests/integration/test_block_meta.py
+++ b/tests/integration/test_block_meta.py
@@ -116,19 +116,22 @@ class TestBlockMeta(IntegrationTestCase):
         psize = 40 << 20
         img = self.tmp_path('image.img')
         config = StorageConfigBuilder(version=version)
-        config.add_image(path=img, size='100M', ptable=ptable)
+        config.add_image(path=img, size='200M', ptable=ptable)
         p1 = config.add_part(size=psize, number=1)
         p2 = config.add_part(size=psize, number=2)
+        p3 = config.add_part(size=psize, number=3)
         self.run_bm(config.render())
 
         with loop_dev(img) as dev:
             self.assertEqual(
                 summarize_partitions(dev), [
-                    PartData(number=1, offset=1 << 20,           size=psize),
-                    PartData(number=2, offset=(1 << 20) + psize, size=psize),
+                    PartData(number=1, offset=1 << 20,             size=psize),
+                    PartData(number=2, offset=(1 << 20) + psize,   size=psize),
+                    PartData(number=3, offset=(1 << 20) + 2*psize, size=psize),
                 ])
         p1['offset'] = 1 << 20
         p2['offset'] = (1 << 20) + psize
+        p3['offset'] = (1 << 20) + 2*psize
         config.set_preserve()
         self.run_bm(config.render())
 
diff --git a/tests/vmtests/__init__.py b/tests/vmtests/__init__.py
index fd6c246..c83e60e 100644
--- a/tests/vmtests/__init__.py
+++ b/tests/vmtests/__init__.py
@@ -630,6 +630,7 @@ class VMBaseClass(TestCase):
         '/MAAS/api/version/': '2.0',
         '/MAAS/api/2.0/version/':
             json.dumps({'version': '2.5.0+curtin-vmtest'})}
+    override_storage_config_version = None
 
     # these get set from base_vm_classes
     release = None
@@ -2008,19 +2009,34 @@ class VMBaseClass(TestCase):
 
     @classmethod
     def get_class_storage_config(cls):
+        cfg = yaml.safe_load(cls.load_conf_file())
+        storage_config = cfg.get('storage', {}).get('config', {})
+        changed = False
+
         if cls.target_arch == 'ppc64el':
-            cfg = yaml.safe_load(cls.load_conf_file())
-            new_sc = inject_prep_partition(cfg)
+            if len(storage_config) == 0:
+                logger.debug('No storage config, skipping prep injection')
+            else:
+                inject_prep_partition(storage_config)
+                changed = True
+
+        if cls.override_storage_config_version is not None:
+            if storage_config:
+                cfg['storage']['version'] = cls.override_storage_config_version
+                changed = True
+
+        if changed:
             # copy testcase YAML to a temp file in order to use updated yaml
-            temp_yaml = tempfile.NamedTemporaryFile(prefix=cls.td.tmpdir + '/',
-                                                    mode='w+t', delete=False)
-            util.write_file(temp_yaml.name, yaml.dump(new_sc,
-                            default_flow_style=False, indent=4))
+            temp_yaml = tempfile.NamedTemporaryFile(
+                prefix=cls.td.tmpdir + '/',
+                mode='w+t',
+                delete=False)
+            util.write_file(
+                temp_yaml.name,
+                yaml.dump(cfg, default_flow_style=False, indent=4))
             cls.conf_file = temp_yaml.name
 
-        cfg = cls.load_conf_file()
-        sc = yaml.safe_load(cfg).get('storage', {}).get('config', {})
-        return sc
+        return storage_config
 
     def get_storage_config(self):
         cfg = load_config(self.collect_path("root/curtin-install-cfg.yaml"))
@@ -2573,20 +2589,15 @@ def prep_partition_for_device(device):
         'size': '8M',
         'flag': 'prep',
         'guid': '9e1a2d38-c612-4316-aa26-8b49521e5a8b',
-        'offset': '1M',
         'wipe': 'zero',
         'grub_device': True,
-        'device': device}
+        'device': device,
+        }
 
 
-def inject_prep_partition(config):
+def inject_prep_partition(storage_config):
     """ Parse a curtin configuration file and inject
         a prep partition if possible"""
-    storage_config = config.get('storage', {}).get('config', {})
-    if len(storage_config) == 0:
-        logger.debug('No storage config, skipping prep injection')
-        return config
-
     logger.info('Injecting PReP Partition')
     disks = [item for item in storage_config
              if item['type'] == 'disk']
@@ -2631,9 +2642,6 @@ def inject_prep_partition(config):
         if 'flag' in part and part['flag'] in ['boot', 'bios_grub']:
             del part['flag']
 
-    config['storage']['config'] = storage_config
-    return config
-
 
 apply_keep_settings()
 logger = _initialize_logging()
diff --git a/tests/vmtests/test_basic.py b/tests/vmtests/test_basic.py
index 6059bd9..4e0dc8f 100644
--- a/tests/vmtests/test_basic.py
+++ b/tests/vmtests/test_basic.py
@@ -255,10 +255,20 @@ class BionicTestBasic(relbase.bionic, TestBasicAbs):
     __test__ = True
 
 
+class BionicTestBasicV2(relbase.bionic, TestBasicAbs):
+    __test__ = True
+    override_storage_config_version = 2
+
+
 class FocalTestBasic(relbase.focal, TestBasicAbs):
     __test__ = True
 
 
+class FocalTestBasicV2(relbase.focal, TestBasicAbs):
+    __test__ = True
+    override_storage_config_version = 2
+
+
 class HirsuteTestBasic(relbase.hirsute, TestBasicAbs):
     __test__ = True
 
@@ -267,6 +277,11 @@ class ImpishTestBasic(relbase.impish, TestBasicAbs):
     __test__ = True
 
 
+class ImpishTestBasicV2(relbase.impish, TestBasicAbs):
+    __test__ = True
+    override_storage_config_version = 2
+
+
 class TestBasicScsiAbs(TestBasicAbs):
     arch_skip = [
         'arm64',  # arm64 is UEFI only