Merge ~mwhudson/curtin:reformat-vs-preserve into curtin:master

[Michael Hudson-Doyle]

disk_handler: check wipe field when deciding whether to reformat raids

Currently preserve=true for raid means "preserve the RAID array"
and ALSO "preserve the partition table". So change the
interpretation of preserve to be solely about preserving the
array and instead check the wipe field to decide if the array
should get a new partition table or not.

LP: #1932976

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

PASSED: Continuous integration, rev:55afc25cc470c5406ebbbf211fc07a83ad8cdc14
https://jenkins.ubuntu.com/server/job/curtin-ci/128/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/128/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/128/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/128/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/128/

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Hrm, I didn't think this was true (that preserve means keeping partition table).

looking at raid_handler, if you set wipe: superblock, it will wipe a verified.

```
create_raid = True
if preserve:
    raid_verify(md_devname, raidlevel, device_paths, spare_device_paths)
    LOG.debug('raid %s already present, skipping create', md_devname)
    create_raid = False

if create_raid:
    mdadm.mdadm_create(md_devname, raidlevel,
                       device_paths, spare_device_paths, container_dev,
                       info.get('mdname', ''), metadata)

wipe_mode = info.get('wipe')
if wipe_mode:
    if wipe_mode == 'superblock' and create_raid:
        # Newly created raid devices already wipe member superblocks at
        # their data offset (this is equivalent to wiping the assembled
        # device, see curtin.block.mdadm.zero_device for more details.
        pass
    else:
        LOG.debug('Wiping raid device %s mode=%s', md_devname, wipe_mode)
        block.wipe_volume(md_devname, mode=wipe_mode, exclusive=False)
```

Curtin will verify the array, lower the create_raid flag, and then if wipe
is set, it will clear the array; it only *skips* clearing if wipe=superblock
and we've just created the array (which already wipes the underlying disks).

review: Needs Information

cbdd142... by Michael Hudson-Doyle on 2021-06-27

add vmtest

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

PASSED: Continuous integration, rev:de27ad6c7519045c23f2834c3ac64aee1dedb6c3
https://jenkins.ubuntu.com/server/job/curtin-ci/129/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/129/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/129/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/129/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/129/

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

review: Approve (continuous-integration)

1e414ee... by Michael Hudson-Doyle on 2021-06-28

fix vmtest

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

PASSED: Continuous integration, rev:cd6f002ef80e433ff8a7af492a2d3c0ea03d80f1
https://jenkins.ubuntu.com/server/job/curtin-ci/130/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/130/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/130/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/130/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/130/

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

review: Approve (continuous-integration)

b16647c... by Michael Hudson-Doyle on 2021-06-29

a different approach

b7283ab... by Michael Hudson-Doyle on 2021-06-29

add a comment. and fix logic to not be 180 degrees wrong

[Michael Hudson-Doyle (mwhudson)]

OK, I think this is ok for another look now (following discussion on the bug)

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

FAILED: Continuous integration, rev:b7283ab45afb3f484064e148d7fdc06f0a448ede
https://jenkins.ubuntu.com/server/job/curtin-ci/131/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/131/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/131/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/131/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/131/

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

review: Needs Fixing (continuous-integration)

760d60a... by Michael Hudson-Doyle on 2021-06-29

add wipe to schema for raid actions

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

PASSED: Continuous integration, rev:760d60ade7eb5dd6b9e32eb8f5e39be4752ca189
https://jenkins.ubuntu.com/server/job/curtin-ci/132/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/132/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/132/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/132/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/132/

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

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

review: Approve

[Ryan Harper (raharper)]

This looks great. Two comments:

1) let's update doc/topics/storage.rst with the comment you added in the disk handler around what preserve/wipe means for existing compound devices.

2) In the vmtest, create a partition table on top of the raid different than the final ptable in the storage config

54a06a6... by Michael Hudson-Doyle on 2021-07-01

address review comments by improving test & documentation

[Michael Hudson-Doyle (mwhudson)]

Good points, done.

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

PASSED: Continuous integration, rev:54a06a6260ac8186db5a02dbedf3db9f6e995965
https://jenkins.ubuntu.com/server/job/curtin-ci/133/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/133/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/133/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/133/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/133/

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Awesome! This looks great.

review: Approve

diff --git a/curtin/block/schemas.py b/curtin/block/schemas.py
index d846505..70de260 100644
--- a/curtin/block/schemas.py
+++ b/curtin/block/schemas.py
@@ -323,6 +323,7 @@ RAID = {
         'metadata': {'type': ['string', 'number']},
         'preserve': {'$ref': '#/definitions/preserve'},
         'ptable': {'$ref': '#/definitions/ptable'},
+        'wipe': {'$ref': '#/definitions/wipe'},
         'spare_devices': {'$ref': '#/definitions/devices'},
         'container': {'$ref': '#/definitions/id'},
         'type': {'const': 'raid'},
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 8cb2784..e5cf659 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -583,7 +583,17 @@ def disk_handler(info, storage_config):
             'Invalid partition table type: %s in %s' % (ptable, info))
 
     disk = get_path_to_storage_volume(info.get('id'), storage_config)
-    if config.value_as_boolean(info.get('preserve')):
+    # For disks, 'preserve' is what indicates whether the partition
+    # table should be reused or recreated but for compound devices
+    # such as raids, it indicates if the raid should be created or
+    # assumed to already exist. So to allow a pre-existing raid to get
+    # a new partition table, we use presence of 'wipe' field to
+    # indicate if the disk should be reformatted or not.
+    if info['type'] == 'disk':
+        preserve_ptable = config.value_as_boolean(info.get('preserve'))
+    else:
+        preserve_ptable = not config.value_as_boolean(info.get('wipe'))
+    if preserve_ptable:
         # Handle preserve flag, verifying if ptable specified in config
         if ptable and ptable != PTABLE_UNSUPPORTED:
             current_ptable = block.get_part_table_type(disk)
@@ -591,8 +601,9 @@ def disk_handler(info, storage_config):
             if current_ptable not in PTABLES_SUPPORTED:
                 raise ValueError(
                     "disk '%s' does not have correct partition table or "
-                    "cannot be read, but preserve is set to true. "
-                    "cannot continue installation." % info.get('id'))
+                    "cannot be read, but preserve is set to true (or wipe is "
+                    "not set).  cannot continue installation." %
+                    info.get('id'))
         LOG.info("disk '%s' marked to be preserved, so keeping partition "
                  "table" % disk)
     else:
diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
index b20d5c1..0f33ec0 100644
--- a/doc/topics/storage.rst
+++ b/doc/topics/storage.rst
@@ -865,6 +865,10 @@ If ``wipe`` option is set to values other than 'superblock', curtin will
 wipe contents of the assembled raid device.  Curtin skips 'superblock` wipes
 as it already clears raid data on the members before assembling the array.
 
+To allow a pre-existing (i.e. ``preserve=true``) raid to get a new partition
+table, set the ``wipe`` field to indicate the disk should be
+reformatted (this is different from disk actions, where the preserve field is
+used for this. But that means something different for raid devices).
 
 **Config Example**::
 
@@ -930,6 +934,7 @@ the bcache device.  This includes checking that backing device and cache
 device are enabled and bound correctly (backing device is cached by expected
 cache device).  If ``cache-mode`` is specified, verify that the mode matches.
 
+
 **wipe**: *superblock, superblock-recursive, pvremove, zero, random*
 
 If ``wipe`` option is set, curtin will wipe the contents of the bcache device.
diff --git a/examples/tests/partition-existing-raid.yaml b/examples/tests/partition-existing-raid.yaml
new file mode 100644
index 0000000..07cf8d2
--- /dev/null
+++ b/examples/tests/partition-existing-raid.yaml
@@ -0,0 +1,115 @@
+showtrace: true
+
+bucket:
+  - &setup |
+    parted /dev/disk/by-id/virtio-disk-b --script -- \
+        mklabel gpt              \
+        mkpart primary 1GiB 9GiB \
+        set 1 boot on
+    parted /dev/disk/by-id/virtio-disk-c --script -- \
+        mklabel gpt              \
+        mkpart primary 1GiB 9GiB \
+        set 1 boot on
+    udevadm settle
+    mdadm --create --metadata 1.2 --level 1 -n 2 /dev/md1 --assume-clean \
+        /dev/disk/by-id/virtio-disk-b-part1 /dev/disk/by-id/virtio-disk-c-part1
+    udevadm settle
+    parted /dev/md1 --script -- \
+        mklabel dos
+    udevadm settle
+    mdadm --stop /dev/md1
+    udevadm settle
+
+# Create a RAID now to test curtin's reuse of existing RAIDs.
+early_commands:
+  00-setup-raid: [sh, -exuc, *setup]
+
+storage:
+  config:
+  - type: disk
+    id: id_disk0
+    serial: disk-a
+    ptable: gpt
+    wipe: superblock
+  - type: disk
+    id: id_disk1
+    serial: disk-b
+    ptable: gpt
+    preserve: true
+  - type: disk
+    id: id_disk2
+    serial: disk-c
+    ptable: gpt
+    preserve: true
+  - type: partition
+    id: id_disk0_part1
+    device: id_disk0
+    flag: boot
+    number: 1
+    size: 512M
+  - type: partition
+    id: id_disk0_part2
+    device: id_disk0
+    number: 2
+    size: 3G
+  - type: partition
+    id: id_disk0_part3
+    device: id_disk0
+    number: 3
+    size: 3G
+  - type: partition
+    id: id_disk1_part1
+    device: id_disk1
+    flag: boot
+    number: 1
+    size: 8G
+    preserve: true
+  - type: partition
+    id: id_disk2_part1
+    device: id_disk2
+    flag: boot
+    number: 1
+    size: 8G
+    preserve: true
+  - type: raid
+    id: raid-md1
+    name: md1
+    raidlevel: raid1
+    devices:
+      - id_disk1_part1
+      - id_disk2_part1
+    spare_devices: []
+    metadata: 1.2
+    preserve: true
+    wipe: superblock
+    ptable: gpt
+  - type: partition
+    id: id_raid1_part1
+    device: raid-md1
+    number: 1
+    size: 7G
+  - type: format
+    id: id_efi_format
+    volume: id_disk0_part1
+    fstype: fat32
+  - type: format
+    id: id_root_format
+    volume: id_disk0_part2
+    fstype: ext4
+  - type: format
+    id: id_raid-md1_format
+    volume: id_raid1_part1
+    fstype: ext4
+  - type: mount
+    device: id_root_format
+    id: id_root_mount
+    path: /
+  - type: mount
+    id: id_efi_mount
+    device: id_efi_format
+    path: /boot/efi
+  - type: mount
+    id: id_raid-md1_mount
+    device: id_raid-md1_format
+    path: /srv
+  version: 1
diff --git a/tests/vmtests/test_preserve_raid.py b/tests/vmtests/test_preserve_raid.py
index 7fc6daa..8c85a20 100644
--- a/tests/vmtests/test_preserve_raid.py
+++ b/tests/vmtests/test_preserve_raid.py
@@ -37,4 +37,39 @@ class GroovyTestPreserveRAID(relbase.groovy, TestPreserveRAID):
     __test__ = True
 
 
+class TestPartitionExistingRAID(VMBaseClass):
+    """ Test that curtin can repartition an existing RAID. """
+    conf_file = "examples/tests/partition-existing-raid.yaml"
+    extra_disks = ['10G', '10G', '10G']
+    uefi = True
+    extra_collect_scripts = [textwrap.dedent("""
+        cd OUTPUT_COLLECT_D
+        lsblk --nodeps --noheading --raw --output PTTYPE /dev/md1 > md1-pttype
+        exit 0
+        """)]
+
+    def test_correct_ptype(self):
+        self.assertEqual('gpt', self.load_collect_file('md1-pttype').strip())
+
+
+class BionicTestPartitionExistingRAID(
+        relbase.bionic, TestPartitionExistingRAID):
+    __test__ = True
+
+
+class FocalTestPartitionExistingRAID(
+        relbase.focal, TestPartitionExistingRAID):
+    __test__ = True
+
+
+class HirsuteTestPartitionExistingRAID(
+        relbase.hirsute, TestPartitionExistingRAID):
+    __test__ = True
+
+
+class GroovyTestPartitionExistingRAID(
+        relbase.groovy, TestPartitionExistingRAID):
+    __test__ = True
+
+
 # vi: ts=4 expandtab syntax=python