curtin-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Ryan Harper has proposed merging ~raharper/curtin:fix/block-sfdisk-parsing into curtin:master.

Commit message:
Fix handing of reusing msdos partitions and flags

Disks with an MSDOS partition table and one partition with the
'bootable' flag set exposed parsing error in block.sfdisk_info;
further digging revealed that curtin was not setting the bootable
flag on MSDOS partitions when required.  Not setting isn't fatal,
grub still boots the partition, but we should set the flag if told
to do so.  Additionally storage-config conversion of probe data
where the MSDOS boot flag is set was missing.  The following changes
are done to fix this issue

- switch to sfdisk --json output and use/introduce util.load_json()
- block-meta: update parted command to set boot flag if present
- block-discover: prefer ID_PART_ENTRY_FLAGS when present, and 0x80
- Add vmtest to verify reuse of msdos partition and bootable flag
- Fix boot, extended, logical flag verfication
- FIx extended partition size verification

LP: #1875903


Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/383180
-- 
Your team curtin developers is requested to review the proposed merge of ~raharper/curtin:fix/block-sfdisk-parsing into curtin:master.

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index a7fe22f..97596e1 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -248,46 +248,47 @@ def _lsblock(args=None):
     return _lsblock_pairs_to_dict(out)
 
 
-def _sfdisk_parse(lines):
-    info = {}
-    for line in lines:
-        if ':' not in line:
-            continue
-        lhs, _, rhs = line.partition(':')
-        key = lhs.strip()
-        value = rhs.strip()
-        if "," in rhs:
-            value = dict((item.split('=')
-                         for item in rhs.replace(' ', '').split(',')))
-        info[key] = value
-
-    return info
-
-
 def sfdisk_info(devpath):
     ''' returns dict of sfdisk info about disk partitions
     {
-     "/dev/vda1": {
-        "size": "20744159",
-        "start": "227328",
-        "type": "0FC63DAF-8483-4772-8E79-3D69D8477DE4",
-        "uuid": "29983666-2A66-4F14-8533-7CE13B715462"
-     },
-     "device": "/dev/vda",
-     "first-lba": "34",
-     "label": "gpt",
-     "label-id": "E94FCCFE-953D-4D4B-9511-451BBCC17A9A",
-     "last-lba": "20971486",
-     "unit": "sectors"
+      "label": "gpt",
+      "id": "877716F7-31D0-4D56-A1ED-4D566EFE418E",
+      "device": "/dev/vda",
+      "unit": "sectors",
+      "firstlba": 34,
+      "lastlba": 41943006,
+      "partitions": [
+         {"node": "/dev/vda1", "start": 227328, "size": 41715679,
+          "type": "0FC63DAF-8483-4772-8E79-3D69D8477DE4",
+          "uuid": "60541CAF-E2AC-48CD-BF89-AF16051C833F"},
+      ]
+    }
+    {
+      "label":"dos",
+      "id":"0xb0dbdde1",
+      "device":"/dev/vdb",
+      "unit":"sectors",
+      "partitions": [
+         {"node":"/dev/vdb1", "start":2048, "size":8388608,
+          "type":"83", "bootable":true},
+         {"node":"/dev/vdb2", "start":8390656, "size":8388608, "type":"83"},
+         {"node":"/dev/vdb3", "start":16779264, "size":62914560, "type":"5"},
+         {"node":"/dev/vdb5", "start":16781312, "size":31457280, "type":"83"},
+         {"node":"/dev/vdb6", "start":48240640, "size":10485760, "type":"83"},
+         {"node":"/dev/vdb7", "start":58728448, "size":20965376, "type":"83"}
+      ]
     }
     '''
     (parent, partnum) = get_blockdev_for_partition(devpath)
     try:
-        (out, _err) = util.subp(['sfdisk', '--dump', parent], capture=True)
+        (out, _err) = util.subp(['sfdisk', '--json', parent], capture=True)
     except util.ProcessExecutionError as e:
+        out = None
         LOG.exception(e)
-        out = ""
-    return _sfdisk_parse(out.splitlines())
+    if out is not None:
+        return util.load_json(out).get('partitiontable', {})
+
+    return {}
 
 
 def dmsetup_info(devname):
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 1ad49b7..52eae94 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -48,6 +48,12 @@ SGDISK_FLAGS = {
     "linux": '8300'
 }
 
+MSDOS_FLAGS = {
+    'boot': 'boot',
+    'extended': 'extended',
+    'logical': 'logical',
+}
+
 DNAME_BYID_KEYS = ['DM_UUID', 'ID_WWN_WITH_EXTENSION', 'ID_WWN', 'ID_SERIAL',
                    'ID_SERIAL_SHORT']
 CMD_ARGUMENTS = (
@@ -715,8 +721,28 @@ def verify_exists(devpath):
         raise RuntimeError("Device %s does not exist" % devpath)
 
 
-def verify_size(devpath, expected_size_bytes):
-    found_size_bytes = block.read_sys_block_size_bytes(devpath)
+def _sfdisk_get_partition_info(devpath, sfdisk_info=None):
+    if not sfdisk_info:
+        sfdisk_info = block.sfdisk_info(devpath)
+
+    entry = [part for part in sfdisk_info['partitions']
+             if part['node'] == devpath]
+    if len(entry) != 1:
+        raise RuntimeError('Device %s not present in sfdisk dump:\n%s' %
+                           devpath, util.json_dumps(sfdisk_info))
+    return entry.pop()
+
+
+def verify_size(devpath, expected_size_bytes, sfdisk_info=None):
+    if not sfdisk_info:
+        sfdisk_info = block.sfdisk_info(devpath)
+
+    part_info = _sfdisk_get_partition_info(devpath, sfdisk_info=sfdisk_info)
+    # FIXME: stuff this list of msdos extended partition type codes elsewhere
+    if part_info.get('type') in ('5', 'f', '85', 'c5'):
+        found_size_bytes = int(part_info['size']) * 512
+    else:
+        found_size_bytes = block.read_sys_block_size_bytes(devpath)
     msg = (
         'Verifying %s size, expecting %s bytes, found %s bytes' % (
          devpath, expected_size_bytes, found_size_bytes))
@@ -725,19 +751,30 @@ def verify_size(devpath, expected_size_bytes):
         raise RuntimeError(msg)
 
 
-def verify_ptable_flag(devpath, expected_flag):
-    if not SGDISK_FLAGS.get(expected_flag):
+def verify_ptable_flag(devpath, expected_flag, sfdisk_info=None):
+    if expected_flag not in (list(SGDISK_FLAGS.keys()) +
+                             list(MSDOS_FLAGS.keys())):
         raise RuntimeError(
-            'Cannot verify unknown partition flag: %s', expected_flag)
+            'Cannot verify unknown partition flag: %s' % expected_flag)
 
-    info = block.sfdisk_info(devpath)
-    if devpath not in info:
-        raise RuntimeError('Device %s not present in sfdisk dump:\n%s' %
-                           devpath, util.json_dumps(info))
+    if not sfdisk_info:
+        sfdisk_info = block.sfdisk_info(devpath)
+    if not sfdisk_info:
+        raise RuntimeError('Failed to extract sfdisk info from %s' % devpath)
 
-    entry = info[devpath]
+    entry = _sfdisk_get_partition_info(devpath, sfdisk_info=sfdisk_info)
     LOG.debug("Device %s ptable entry: %s", devpath, util.json_dumps(entry))
-    (found_flag, code) = ptable_uuid_to_flag_entry(entry['type'])
+    found_flag = None
+    if (sfdisk_info['label'] in ('dos', 'msdos')):
+        if expected_flag == 'boot':
+            found_flag = 'boot' if entry.get('bootable') is True else None
+        elif expected_flag == 'extended':
+            (found_flag, _code) = ptable_uuid_to_flag_entry(entry['type'])
+        elif expected_flag == 'logical':
+            (_parent, partnumber) = block.get_blockdev_for_partition(devpath)
+            found_flag = 'logical' if int(partnumber) > 4 else None
+    else:
+        (found_flag, _code) = ptable_uuid_to_flag_entry(entry['type'])
     msg = (
         'Verifying %s partition flag, expecting %s, found %s' % (
          devpath, expected_flag, found_flag))
@@ -748,10 +785,12 @@ def verify_ptable_flag(devpath, expected_flag):
 
 def partition_verify(devpath, info):
     verify_exists(devpath)
-    verify_size(devpath, int(util.human2bytes(info['size'])))
+    sfdisk_info = block.sfdisk_info(devpath)
+    verify_size(devpath, int(util.human2bytes(info['size'])),
+                sfdisk_info=sfdisk_info)
     expected_flag = info.get('flag')
     if expected_flag:
-        verify_ptable_flag(devpath, info['flag'])
+        verify_ptable_flag(devpath, info['flag'], sfdisk_info=sfdisk_info)
 
 
 def partition_handler(info, storage_config):
@@ -889,6 +928,9 @@ def partition_handler(info, storage_config):
             cmd = ["parted", disk, "--script", "mkpart", partition_type,
                    "%ss" % offset_sectors, "%ss" % str(offset_sectors +
                                                        length_sectors)]
+            if flag == 'boot':
+                cmd.extend(['set', str(partnumber), 'boot', 'on'])
+
             util.subp(cmd, capture=True)
         elif disk_ptable == "gpt":
             if flag and flag in SGDISK_FLAGS:
diff --git a/curtin/storage_config.py b/curtin/storage_config.py
index eccb96b..c0e351d 100644
--- a/curtin/storage_config.py
+++ b/curtin/storage_config.py
@@ -782,6 +782,10 @@ class BlockdevParser(ProbertParser):
                 entry['size'] *= 512
 
             ptype = blockdev_data.get('ID_PART_ENTRY_TYPE')
+            # use PART_ENTRY_FLAGS if set, msdos
+            ptype_flag = blockdev_data.get('ID_PART_ENTRY_FLAGS')
+            if ptype_flag:
+                ptype = ptype_flag
             flag_name, _flag_code = ptable_uuid_to_flag_entry(ptype)
 
             # logical partitions are not tagged in data, however
@@ -1234,13 +1238,18 @@ def ptable_uuid_to_flag_entry(guid):
         '9E1A2D38-C612-4316-AA26-8B49521E5A8B': ('prep', '4200'),
         'A19D880F-05FC-4D3B-A006-743F0F84911E': ('raid', 'fd00'),
         '0657FD6D-A4AB-43C4-84E5-0933C84B4F4F': ('swap', '8200'),
-        '0X83': ('linux', '83'),
         '0XF': ('extended', 'f'),
         '0X5': ('extended', 'f'),
+        '0X80': ('boot', '80'),
+        '0X83': ('linux', '83'),
         '0X85': ('extended', 'f'),
         '0XC5': ('extended', 'f'),
     }
     name = code = None
+
+    # prefix non-uuid guid values with 0x
+    if guid and '-' not in guid and not guid.upper().startswith('0X'):
+        guid = '0x' + guid
     if guid and guid.upper() in guid_map:
         name, code = guid_map[guid.upper()]
 
diff --git a/curtin/util.py b/curtin/util.py
index fa4f3f3..afef58d 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -574,6 +574,15 @@ def decode_binary(blob, encoding='utf-8', errors='replace'):
     return blob.decode(encoding, errors=errors)
 
 
+def load_json(text, root_types=(dict,)):
+    decoded = json.loads(text)
+    if not isinstance(decoded, tuple(root_types)):
+        expected_types = ", ".join([str(t) for t in root_types])
+        raise TypeError("(%s) root types expected, got %s instead"
+                        % (expected_types, type(decoded)))
+    return decoded
+
+
 def file_size(path):
     """get the size of a file"""
     with open(path, 'rb') as fp:
diff --git a/examples/tests/reuse-msdos-partitions.yaml b/examples/tests/reuse-msdos-partitions.yaml
new file mode 100644
index 0000000..bc1dbf1
--- /dev/null
+++ b/examples/tests/reuse-msdos-partitions.yaml
@@ -0,0 +1,135 @@
+showtrace: true
+install:
+   unmount: disabled
+
+# The point of this test is to test installing to a disk that contains
+# a partition that used to be a RAID member where the other parts of
+# the RAID are not present (the scenario is that the disk was just
+# grabbed out of a pile of previously used disks and shoved into a
+# server).
+
+# So what it does is to create a RAID0 out of partitions on two disks,
+# stop the RAID, wipe the superblock on one of them and then install
+# to the other using a standard partitioning scheme.
+
+bucket:
+  - &setup |
+    parted /dev/disk/by-id/virtio-disk-a --script -- \
+        mklabel msdos              \
+        mkpart primary 1MiB 3073MiB \
+        mkpart extended 3074MiB 8193MiB \
+        mkpart logical 3075MiB 5122MiB \
+        mkpart logical 5123MiB 8192MiB \
+        set 1 boot on
+    udevadm settle
+
+early_commands:
+  00-setup-raid: [sh, -exuc, *setup]
+
+
+showtrace: true
+storage:
+    version: 1
+    config:
+      - id: sda
+        type: disk
+        ptable: msdos
+        model: QEMU HARDDISK
+        serial: disk-a
+        name: main_disk
+        preserve: true
+        grub_device: true
+      - id: sdb
+        type: disk
+        ptable: msdos
+        model: QEMU HARDDISK
+        serial: disk-b
+        name: extra_disk
+        wipe: superblock-recursive
+      - id: sda1
+        type: partition
+        number: 1
+        size: 3072M
+        device: sda
+        flag: boot
+        preserve: true
+        wipe: superblock
+      - id: sda2
+        type: partition
+        number: 2
+        size: 5119M
+        flag: extended
+        device: sda
+        preserve: true
+      - id: sda5
+        type: partition
+        number: 5
+        size: 2047M
+        flag: logical
+        device: sda
+        preserve: true
+        wipe: superblock
+      - id: sda6
+        type: partition
+        number: 6
+        size: 3069M
+        flag: logical
+        device: sda
+        preserve: true
+        wipe: superblock
+      - id: sdb1
+        type: partition
+        number: 1
+        size: 4GB
+        device: sdb
+      - id: volgroup1
+        name: vg1
+        type: lvm_volgroup
+        devices:
+            - sda5
+            - sda6
+      - id: lvmpart1
+        name: lv1
+        size: 1G
+        type: lvm_partition
+        volgroup: volgroup1
+      - id: lvmpart2
+        name: lv2
+        type: lvm_partition
+        volgroup: volgroup1
+      - id: volgroup2
+        name: ubuntu-vg
+        type: lvm_volgroup
+        devices:
+            - sdb1
+      - id: ubuntulv1
+        name: my-storage
+        size: 1G
+        type: lvm_partition
+        volgroup: volgroup2
+      - id: sda1_root
+        type: format
+        fstype: ext4
+        volume: sda1
+      - id: lv1_fs
+        name: storage
+        type: format
+        fstype: fat32
+        volume: lvmpart1
+      - id: lv2_fs
+        name: storage
+        type: format
+        fstype: ext3
+        volume: lvmpart2
+      - id: sda1_mount
+        type: mount
+        path: /
+        device: sda1_root
+      - id: lv1_mount
+        type: mount
+        path: /srv/data
+        device: lv1_fs
+      - id: lv2_mount
+        type: mount
+        path: /srv/backup
+        device: lv2_fs
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 5f1b99e..c6d2ced 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -335,7 +335,8 @@ class TestBlockMeta(CiTestCase):
                                                      exclusive=False)
         self.mock_subp.assert_has_calls(
             [call(['parted', disk_kname, '--script',
-                   'mkpart', 'primary', '2048s', '1001471s'], capture=True)])
+                   'mkpart', 'primary', '2048s', '1001471s',
+                   'set', '1', 'boot', 'on'], capture=True)])
 
     @patch('curtin.util.write_file')
     def test_mount_handler_defaults(self, mock_write_file):
diff --git a/tests/vmtests/test_reuse_msdos_partitions.py b/tests/vmtests/test_reuse_msdos_partitions.py
new file mode 100644
index 0000000..94cd0a8
--- /dev/null
+++ b/tests/vmtests/test_reuse_msdos_partitions.py
@@ -0,0 +1,32 @@
+# This file is part of curtin. See LICENSE file for copyright and license info.
+
+from . import VMBaseClass
+from .releases import base_vm_classes as relbase
+
+
+class TestReuseMSDOSPartitions(VMBaseClass):
+    """ Curtin can reuse MSDOS partitions with flags. """
+    conf_file = "examples/tests/reuse-msdos-partitions.yaml"
+    extra_disks = ['10G']
+    test_stype = 'storage'
+
+    def test_simple(self):
+        pass
+
+
+class BionicTestReuseMSDOSPartitions(relbase.bionic,
+                                     TestReuseMSDOSPartitions):
+    __test__ = True
+
+
+class EoanTestReuseMSDOSPartitions(relbase.eoan,
+                                   TestReuseMSDOSPartitions):
+    __test__ = True
+
+
+class FocalTestReuseMSDOSPartitions(relbase.focal,
+                                    TestReuseMSDOSPartitions):
+    __test__ = True
+
+
+# vi: ts=4 expandtab syntax=python