From cc13d97f42f994369ed7296945ec2cff51794038 Mon Sep 17 00:00:00 2001 From: Arne Wiebalck Date: Wed, 16 Aug 2023 15:51:59 +0200 Subject: [PATCH 1/2] Conditional creation of RAIDed ESP for UEFI Software RAID Rebuilding an instance on a RAIDed ESPs will fail due to sgdisk running against an non-clean disk and bailing out. Check if there is a RAIDed ESP already and skip creation if it exists. Change-Id: I13617ae77515a9d34bc4bb3caf9fae73d5e4e578 Conflicts: ironic_python_agent/tests/unit/test_raid_utils.py --- ironic_python_agent/raid_utils.py | 112 +++++++++++------- .../tests/unit/samples/hardware_samples.py | 30 +++++ .../tests/unit/test_raid_utils.py | 26 +++- .../rebuild_on_esp_raid-33f359bdf5ccaa09.yaml | 5 + 4 files changed, 126 insertions(+), 47 deletions(-) create mode 100644 releasenotes/notes/rebuild_on_esp_raid-33f359bdf5ccaa09.yaml diff --git a/ironic_python_agent/raid_utils.py b/ironic_python_agent/raid_utils.py index 0a53eec5e..e1bc760d0 100644 --- a/ironic_python_agent/raid_utils.py +++ b/ironic_python_agent/raid_utils.py @@ -12,6 +12,7 @@ import copy import re +import shlex from ironic_lib import disk_utils from ironic_lib import utils as il_utils @@ -302,50 +303,58 @@ def prepare_boot_partitions_for_softraid(device, holders, efi_part, if efi_part: efi_part = '{}p{}'.format(device, efi_part['number']) - LOG.info("Creating EFI partitions on software RAID holder disks") - # We know that we kept this space when configuring raid,see - # hardware.GenericHardwareManager.create_configuration. - # We could also directly get the EFI partition size. - partsize_mib = ESP_SIZE_MIB - partlabel_prefix = 'uefi-holder-' - efi_partitions = [] - for number, holder in enumerate(holders): - # NOTE: see utils.get_partition_table_type_from_specs - # for uefi we know that we have setup a gpt partition table, - # sgdisk can be used to edit table, more user friendly - # for alignment and relative offsets - partlabel = '{}{}'.format(partlabel_prefix, number) - out, _u = utils.execute('sgdisk', '-F', holder) - start_sector = '{}s'.format(out.splitlines()[-1].strip()) - out, _u = utils.execute( - 'sgdisk', '-n', '0:{}:+{}MiB'.format(start_sector, - partsize_mib), - '-t', '0:ef00', '-c', '0:{}'.format(partlabel), holder) - - # Refresh part table - utils.execute("partprobe") - utils.execute("blkid") - - target_part, _u = utils.execute( - "blkid", "-l", "-t", "PARTLABEL={}".format(partlabel), holder) - - target_part = target_part.splitlines()[-1].split(':', 1)[0] - efi_partitions.append(target_part) - - LOG.debug("EFI partition %s created on holder disk %s", - target_part, holder) - - # RAID the ESPs, metadata=1.0 is mandatory to be able to boot - md_device = get_next_free_raid_device() - LOG.debug("Creating md device %(md_device)s for the ESPs " - "on %(efi_partitions)s", - {'md_device': md_device, 'efi_partitions': efi_partitions}) - utils.execute('mdadm', '--create', md_device, '--force', - '--run', '--metadata=1.0', '--level', '1', - '--name', 'esp', '--raid-devices', len(efi_partitions), - *efi_partitions) - - disk_utils.trigger_device_rescan(md_device) + # check if we have a RAIDed ESP already + md_device = find_esp_raid() + if md_device: + LOG.info("Found RAIDed ESP %s, skip creation", md_device) + else: + LOG.info("Creating EFI partitions on software RAID holder disks") + # We know that we kept this space when configuring raid,see + # hardware.GenericHardwareManager.create_configuration. + # We could also directly get the EFI partition size. + partsize_mib = ESP_SIZE_MIB + partlabel_prefix = 'uefi-holder-' + efi_partitions = [] + for number, holder in enumerate(holders): + # NOTE: see utils.get_partition_table_type_from_specs + # for uefi we know that we have setup a gpt partition table, + # sgdisk can be used to edit table, more user friendly + # for alignment and relative offsets + partlabel = '{}{}'.format(partlabel_prefix, number) + out, _u = utils.execute('sgdisk', '-F', holder) + start_sector = '{}s'.format(out.splitlines()[-1].strip()) + out, _u = utils.execute( + 'sgdisk', '-n', '0:{}:+{}MiB'.format(start_sector, + partsize_mib), + '-t', '0:ef00', '-c', '0:{}'.format(partlabel), holder) + + # Refresh part table + utils.execute("partprobe") + utils.execute("blkid") + + target_part, _u = utils.execute( + "blkid", "-l", "-t", "PARTLABEL={}".format(partlabel), + holder) + + target_part = target_part.splitlines()[-1].split(':', 1)[0] + efi_partitions.append(target_part) + + LOG.debug("EFI partition %s created on holder disk %s", + target_part, holder) + + # RAID the ESPs, metadata=1.0 is mandatory to be able to boot + md_device = get_next_free_raid_device() + LOG.debug("Creating md device %(md_device)s for the ESPs " + "on %(efi_partitions)s", + {'md_device': md_device, + 'efi_partitions': efi_partitions}) + utils.execute('mdadm', '--create', md_device, '--force', + '--run', '--metadata=1.0', '--level', '1', + '--name', 'esp', '--raid-devices', + len(efi_partitions), + *efi_partitions) + + disk_utils.trigger_device_rescan(md_device) if efi_part: # Blockdev copy the source ESP and erase it @@ -380,3 +389,18 @@ def prepare_boot_partitions_for_softraid(device, holders, efi_part, # disk, as in virtual disk, where to load the data from. # Since there is a structural difference, this means it will # fail. + + +def find_esp_raid(): + """Find the ESP md device in case of a rebuild.""" + + # find devices of type 'RAID1' and fstype 'VFAT' + lsblk = utils.execute('lsblk', '-PbioNAME,TYPE,FSTYPE') + report = lsblk[0] + for line in report.split('\n'): + dev = {} + vals = shlex.split(line) + for key, val in (v.split('=', 1) for v in vals): + dev[key] = val.strip() + if dev.get('TYPE') == 'raid1' and dev.get('FSTYPE') == 'vfat': + return '/dev/' + dev.get('NAME') diff --git a/ironic_python_agent/tests/unit/samples/hardware_samples.py b/ironic_python_agent/tests/unit/samples/hardware_samples.py index 056f41ef8..743bc4b86 100644 --- a/ironic_python_agent/tests/unit/samples/hardware_samples.py +++ b/ironic_python_agent/tests/unit/samples/hardware_samples.py @@ -1593,3 +1593,33 @@ ' `-+- policy=\'service-time 0\' prio=1 status=active\n' ' `- 0:0:0:0 device s 8:0 active ready running\n' ) + +LSBLK_OUPUT = (""" +NAME="sda" TYPE="disk" FSTYPE="" +NAME="sdb" TYPE="disk" FSTYPE="" +""") + +LSBLK_OUPUT_ESP_RAID = (""" +NAME="sda" TYPE="disk" FSTYPE="" +NAME="sda1" TYPE="part" FSTYPE="linux_raid_member" +NAME="md127" TYPE="raid1" FSTYPE="" +NAME="md127p1" TYPE="md" FSTYPE="xfs" +NAME="md127p2" TYPE="md" FSTYPE="iso9660" +NAME="md127p14" TYPE="md" FSTYPE="" +NAME="md127p15" TYPE="md" FSTYPE="" +NAME="sda2" TYPE="part" FSTYPE="linux_raid_member" +NAME="md126" TYPE="raid0" FSTYPE="" +NAME="sda3" TYPE="part" FSTYPE="linux_raid_member" +NAME="md125" TYPE="raid1" FSTYPE="vfat" +NAME="sdb" TYPE="disk" FSTYPE="" +NAME="sdb1" TYPE="part" FSTYPE="linux_raid_member" +NAME="md127" TYPE="raid1" FSTYPE="" +NAME="md127p1" TYPE="md" FSTYPE="xfs" +NAME="md127p2" TYPE="md" FSTYPE="iso9660" +NAME="md127p14" TYPE="md" FSTYPE="" +NAME="md127p15" TYPE="md" FSTYPE="" +NAME="sdb2" TYPE="part" FSTYPE="linux_raid_member" +NAME="md126" TYPE="raid0" FSTYPE="" +NAME="sdb3" TYPE="part" FSTYPE="linux_raid_member" +NAME="md125" TYPE="raid1" FSTYPE="vfat" +""") diff --git a/ironic_python_agent/tests/unit/test_raid_utils.py b/ironic_python_agent/tests/unit/test_raid_utils.py index 5b8577e27..85e6d68f5 100644 --- a/ironic_python_agent/tests/unit/test_raid_utils.py +++ b/ironic_python_agent/tests/unit/test_raid_utils.py @@ -117,6 +117,7 @@ def test_create_raid_device_fail_read_device(self, mock_execute, raid_utils.create_raid_device, 0, logical_disk) + @mock.patch.object(raid_utils, 'find_esp_raid', autospec=True) @mock.patch.object(disk_utils, 'trigger_device_rescan', autospec=True) @mock.patch.object(raid_utils, 'get_next_free_raid_device', autospec=True, return_value='/dev/md42') @@ -125,7 +126,7 @@ def test_create_raid_device_fail_read_device(self, mock_execute, @mock.patch.object(disk_utils, 'find_efi_partition', autospec=True) def test_prepare_boot_partitions_for_softraid_uefi_gpt( self, mock_efi_part, mock_execute, mock_dispatch, - mock_free_raid_device, mock_rescan): + mock_free_raid_device, mock_rescan, mock_find_esp): mock_efi_part.return_value = {'number': '12'} mock_execute.side_effect = [ ('451', None), # sgdisk -F @@ -142,6 +143,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt( (None, None), # cp (None, None), # wipefs ] + mock_find_esp.return_value = None efi_part = raid_utils.prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], None, @@ -173,6 +175,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt( self.assertEqual(efi_part, '/dev/md42') mock_rescan.assert_called_once_with('/dev/md42') + @mock.patch.object(raid_utils, 'find_esp_raid', autospec=True) @mock.patch.object(disk_utils, 'trigger_device_rescan', autospec=True) @mock.patch.object(raid_utils, 'get_next_free_raid_device', autospec=True, return_value='/dev/md42') @@ -182,7 +185,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt( @mock.patch.object(ilib_utils, 'mkfs', autospec=True) def test_prepare_boot_partitions_for_softraid_uefi_gpt_esp_not_found( self, mock_mkfs, mock_efi_part, mock_execute, mock_dispatch, - mock_free_raid_device, mock_rescan): + mock_free_raid_device, mock_rescan, mock_find_esp): mock_efi_part.return_value = None mock_execute.side_effect = [ ('451', None), # sgdisk -F @@ -197,6 +200,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt_esp_not_found( ('/dev/sdb14: whatever', None), # blkid (None, None), # mdadm ] + mock_find_esp.return_value = None efi_part = raid_utils.prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], None, @@ -226,6 +230,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt_esp_not_found( self.assertEqual(efi_part, '/dev/md42') mock_rescan.assert_called_once_with('/dev/md42') + @mock.patch.object(raid_utils, 'find_esp_raid', autospec=True) @mock.patch.object(disk_utils, 'trigger_device_rescan', autospec=True) @mock.patch.object(raid_utils, 'get_next_free_raid_device', autospec=True, return_value='/dev/md42') @@ -233,7 +238,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt_esp_not_found( @mock.patch.object(ilib_utils, 'execute', autospec=True) def test_prepare_boot_partitions_for_softraid_uefi_gpt_efi_provided( self, mock_execute, mock_dispatch, mock_free_raid_device, - mock_rescan): + mock_rescan, mock_find_esp): mock_execute.side_effect = [ ('451', None), # sgdisk -F (None, None), # sgdisk create part @@ -249,6 +254,7 @@ def test_prepare_boot_partitions_for_softraid_uefi_gpt_efi_provided( (None, None), # cp (None, None), # wipefs ] + mock_find_esp.return_value = None efi_part = raid_utils.prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], '/dev/md0p15', @@ -353,3 +359,17 @@ def test_no_device(self, mock_dispatch): ] self.assertRaises(errors.SoftwareRAIDError, raid_utils.get_next_free_raid_device) + + +@mock.patch.object(utils, 'execute', autospec=True) +class TestFindESPRAID(base.IronicAgentTest): + + def test_no_esp_raid(self, mock_execute): + mock_execute.side_effect = [(hws.LSBLK_OUPUT, '')] + result = raid_utils.find_esp_raid() + self.assertIsNone(result) + + def test_esp_raid(self, mock_execute): + mock_execute.side_effect = [(hws.LSBLK_OUPUT_ESP_RAID, '')] + result = raid_utils.find_esp_raid() + self.assertEqual('/dev/md125', result) diff --git a/releasenotes/notes/rebuild_on_esp_raid-33f359bdf5ccaa09.yaml b/releasenotes/notes/rebuild_on_esp_raid-33f359bdf5ccaa09.yaml new file mode 100644 index 000000000..09e8bf4d1 --- /dev/null +++ b/releasenotes/notes/rebuild_on_esp_raid-33f359bdf5ccaa09.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes an issue with rebuilding instances on Software RAID with + RAIDed ESP partitions. From 862458b8fb84a8d855583139e5b2f281272f30fe Mon Sep 17 00:00:00 2001 From: Maryna Savchenko Date: Tue, 7 Nov 2023 14:38:47 +0100 Subject: [PATCH 2/2] Fix referencing to the raid_device var which is not set Change-Id: I11180e5d61d893a78583ace555f6e90ba8845950 --- ironic_python_agent/hardware.py | 2 +- .../notes/fix-raid_device-not-set-8b03688ce83ce22e.yaml | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/fix-raid_device-not-set-8b03688ce83ce22e.yaml diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 07494d9c2..0d8847afc 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -2543,7 +2543,7 @@ def _delete_config_pass(self, raid_devices): except processutils.ProcessExecutionError as e: LOG.warning('Failed to remove superblock from' '%(device)s: %(err)s', - {'device': raid_device.name, 'err': e}) + {'device': blk.name, 'err': e}) # Erase all partition tables we created all_holder_disks_uniq = list( diff --git a/releasenotes/notes/fix-raid_device-not-set-8b03688ce83ce22e.yaml b/releasenotes/notes/fix-raid_device-not-set-8b03688ce83ce22e.yaml new file mode 100644 index 000000000..fcb7cd4c1 --- /dev/null +++ b/releasenotes/notes/fix-raid_device-not-set-8b03688ce83ce22e.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes referencing to raid_device variable before assignment, + is replaced by blk variable.