Merge ~mwhudson/curtin:vmtest-raid-partition-to-disk into curtin:master

Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: 630af5b94cc75ff95e49a320d8075e9d451d48c2
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~mwhudson/curtin:vmtest-raid-partition-to-disk
Merge into: curtin:master
Diff against target: 110 lines (+98/-0)
2 files modified
examples/tests/raid-partition-to-disk.yaml (+70/-0)
tests/vmtests/test_raid_partition_to_disk.py (+28/-0)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve
Paride Legovini Needs Fixing
Review via email: mp+370028@code.launchpad.net

Commit message

vmtests: test using a disk with RAID partition on it directly in a RAID

Description of the change

A vmtest that tries to reuse a disk that contains a partition that is part of a RAID, and fails. It includes a patch from Ryan that might possibly have helped, but it did not.

The full failure log is at http://paste.ubuntu.com/p/DMTxdSdPt4/ but the basic failure seems to be that it fails to detect the raid array as a "holder" of the disk and then fails to exclusively open the device node with "Device or resource busy"

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Oh wait, adding wipe: superblock to disks containing the raid partitions makes this work OK. subiquity currently never sets wipe on disks, which is pretty clearly wrong :(

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Updated to a working config. (should using a disk in a new compound device imply some kind of wiping?)

Revision history for this message
Ryan Harper (raharper) wrote :

Generally, disks should always have wipe (which we've had); unless they need to be preserved in some way.

This almost compares to the current partial raid bug, and it's easy to see where this will fail.

If vdc had *two* partitions, one for the raid, and one for a filesystem to be preserved; it's clear that we *can't* wipe vdc, but clearly we need to wipe the partition.

For that case, then we'd want to preserve: true on vdc, wipe: superblock on vdc-part1, and preserve: true on vdc-part2.

And with the fix to allow partitions to be accepted into the clear-holders list; I believe that should work.

So, can we update this config to include the second partition on vdc to be preserved, as above.

I suspect this will fail until we add the fix to block_meta that I sent you the other day:

% git diff -- curtin/commands/block_meta.py
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 79493fc..5d53c0b 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -67,7 +67,7 @@ def block_meta(args):
     if devices is None:
         devices = []
         if 'storage' in cfg:
- devices = get_disk_paths_from_storage_config(
+ devices = get_device_paths_from_storage_config(
                 extract_storage_ordered_dict(cfg))
         if len(devices) == 0:
             devices = cfg.get('block-meta', {}).get('devices', [])
@@ -1505,9 +1505,9 @@ def zfs_handler(info, storage_config):
             util.write_file(state['fstab'], fstab_entry, omode='a')

-def get_disk_paths_from_storage_config(storage_config):
- """Returns a list of disk paths in a storage config filtering out
- preserved or disks which do not have wipe configuration.
+def get_device_paths_from_storage_config(storage_config):
+ """Returns a list of device paths in a storage config filtering out
+ preserved or devices which do not have wipe configuration.

     :param: storage_config: Ordered dict of storage configation
     """
@@ -1516,7 +1516,7 @@ def get_disk_paths_from_storage_config(storage_config):

     return [get_path_if_present(k, storage_config)
             for (k, v) in storage_config.items()
- if v.get('type') == 'disk' and
+ if v.get('type') in ['disk', 'partition'] and
             config.value_as_boolean(v.get('wipe')) and
             not config.value_as_boolean(v.get('preserve'))]

Revision history for this message
Ryan Harper (raharper) wrote :

Once this one lands:

https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/370843

We can rebase and see if this also just works.

Revision history for this message
Ryan Harper (raharper) wrote :

This has now landed, care to rebase and see if this works now?

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Sure, rebased.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

Disco is dead, add focal

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Updated.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

This looks stale. I wonder if we should try to rebase, retest, remerge?

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Paride Legovini (paride) wrote :

Now it's clear it needs a rebase :)

review: Needs Fixing
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

The new vmtest passed in jenkins so let's merge this.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) :
review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/examples/tests/raid-partition-to-disk.yaml b/examples/tests/raid-partition-to-disk.yaml
2new file mode 100644
3index 0000000..9c16c26
4--- /dev/null
5+++ b/examples/tests/raid-partition-to-disk.yaml
6@@ -0,0 +1,70 @@
7+showtrace: true
8+
9+bucket:
10+ - &setup |
11+ parted /dev/disk/by-id/virtio-disk-b --script -- \
12+ mklabel gpt \
13+ mkpart primary 1GiB 9GiB
14+ parted /dev/disk/by-id/virtio-disk-c --script -- \
15+ mklabel gpt \
16+ mkpart primary 1GiB 9GiB
17+ udevadm settle
18+ mdadm --create --metadata 1.2 --level 1 -n 2 /dev/md1 --assume-clean \
19+ /dev/disk/by-id/virtio-disk-b-part1 /dev/disk/by-id/virtio-disk-c-part1
20+ udevadm settle
21+ mdadm --stop /dev/md1
22+ udevadm settle
23+
24+# Create a RAID now to test curtin's reuse of existing RAIDs.
25+early_commands:
26+ 00-setup-raid: [sh, -exuc, *setup]
27+
28+storage:
29+ config:
30+ - type: disk
31+ id: disk-a
32+ serial: disk-a
33+ ptable: gpt
34+ wipe: superblock
35+ - type: disk
36+ id: disk-b
37+ serial: disk-b
38+ wipe: superblock
39+ - type: disk
40+ id: disk-c
41+ serial: disk-c
42+ wipe: superblock
43+
44+ - type: partition
45+ id: disk-a-p1
46+ device: disk-a
47+ flag: boot
48+ number: 1
49+ size: 512M
50+
51+ - type: raid
52+ id: md1
53+ name: md1
54+ devices:
55+ - disk-b
56+ - disk-c
57+ raidlevel: raid1
58+
59+ - type: format
60+ id: id_efi_format
61+ volume: disk-a-p1
62+ fstype: fat32
63+ - type: format
64+ id: id_root_format
65+ volume: md1
66+ fstype: ext4
67+
68+ - type: mount
69+ device: id_root_format
70+ id: id_root_mount
71+ path: /
72+ - type: mount
73+ id: id_efi_mount
74+ device: id_efi_format
75+ path: /boot/efi
76+ version: 1
77diff --git a/tests/vmtests/test_raid_partition_to_disk.py b/tests/vmtests/test_raid_partition_to_disk.py
78new file mode 100644
79index 0000000..1bb2cc5
80--- /dev/null
81+++ b/tests/vmtests/test_raid_partition_to_disk.py
82@@ -0,0 +1,28 @@
83+# This file is part of curtin. See LICENSE file for copyright and license info.
84+
85+from . import VMBaseClass
86+from .releases import base_vm_classes as relbase
87+
88+
89+class TestRAIDPartitionToDisk(VMBaseClass):
90+ """Convert a RAID made of partitions to one made of disks."""
91+ conf_file = "examples/tests/raid-partition-to-disk.yaml"
92+ extra_disks = ['10G', '10G', '10G']
93+ uefi = True
94+
95+ def test_simple(self):
96+ pass
97+
98+
99+class BionicTestRAIDPartitionToDisk(relbase.bionic, TestRAIDPartitionToDisk):
100+ __test__ = True
101+
102+
103+class EoanTestRAIDPartitionToDisk(relbase.eoan, TestRAIDPartitionToDisk):
104+ __test__ = True
105+
106+
107+class FocalTestRAIDPartitionToDisk(relbase.focal, TestRAIDPartitionToDisk):
108+ __test__ = True
109+
110+# vi: ts=4 expandtab syntax=python

Subscribers

People subscribed via source and target branches