From f0191d9e0549ea4f752a7f5eb45bae20647e06b9 Mon Sep 17 00:00:00 2001 From: Arne Wiebalck Date: Wed, 14 Apr 2021 08:50:16 +0200 Subject: [PATCH] Software RAID: RAID the ESPs For software RAID in UEFI mode, we create ESPs on all holder disks and copy the bootloader there. Since there is no mechanism to keep the ESPs in sync, e.g. on kernel upgrades or when kernel parameters are updated, the ESPs will get out of sync eventually. This may lead to a situation where a node boots with outdated parameters or does not have any of the installed kernels in the boot menu anymore. This change proposes to RAID the ESPs. While the UEFI firmware will find an ESP partition (one leg of the mirror), the node will see an md device and all subsequent updates will go to all member disks. Also, remove the source ESP after copying in order to avoid mount confusion (same UUID!). Story: #2008745 Task: #42103 Change-Id: I9078ef37f1e94382c645ae98ce724ac9ed87c287 (cherry picked from commit c2d04dc1566bb947d0e6afd040b82be55c925b11) --- ironic_python_agent/extensions/image.py | 177 +++++++++--------- .../tests/unit/extensions/test_image.py | 66 +++---- ...tware-raid-raid-ESPs-25a2aa117b99620a.yaml | 7 + 3 files changed, 124 insertions(+), 126 deletions(-) create mode 100644 releasenotes/notes/software-raid-raid-ESPs-25a2aa117b99620a.yaml diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index c482b69ca..a728139a1 100644 --- a/ironic_python_agent/extensions/image.py +++ b/ironic_python_agent/extensions/image.py @@ -373,8 +373,9 @@ def _prepare_boot_partitions_for_softraid(device, holders, efi_part, target_boot_mode): """Prepare boot partitions when relevant. - Create either efi partitions or bios boot partitions for softraid, - according to both target boot mode and disk holders partition table types. + Create either a RAIDed EFI partition or bios boot partitions for software + RAID, according to both target boot mode and disk holders partition table + types. :param device: the softraid device path :param holders: the softraid drive members @@ -383,11 +384,9 @@ def _prepare_boot_partitions_for_softraid(device, holders, efi_part, :param target_boot_mode: target boot mode can be bios/uefi/None or anything else for unspecified - :returns: the efi partition paths on softraid disk holders when target - boot mode is uefi, empty list otherwise. + :returns: the path to the ESP md device when target boot mode is uefi, + nothing otherwise. """ - efi_partitions = [] - # Actually any fat partition could be a candidate. Let's assume the # partition also has the esp flag if target_boot_mode == 'uefi': @@ -412,6 +411,7 @@ def _prepare_boot_partitions_for_softraid(device, holders, efi_part, # We could also directly get the EFI partition size. partsize_mib = raid_utils.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, @@ -433,26 +433,31 @@ def _prepare_boot_partitions_for_softraid(device, holders, efi_part, "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) - if efi_part: - LOG.debug("Relocating EFI %s to holder part %s", efi_part, - target_part) - # Blockdev copy - utils.execute("cp", efi_part, target_part) - else: - # Creating a label is just to make life easier - if number == 0: - fslabel = 'efi-part' - else: - # bak, label is limited to 11 chars - fslabel = 'efi-part-b' - ilib_utils.mkfs(fs='vfat', path=target_part, label=fslabel) - efi_partitions.append(target_part) - # TBD: Would not hurt to destroy source efi part when defined, - # for clarity. + # RAID the ESPs, metadata=1.0 is mandatory to be able to boot + md_device = '/dev/md/esp' + LOG.debug("Creating md device {} for the ESPs on {}".format( + md_device, efi_partitions)) + utils.execute('mdadm', '--create', md_device, '--force', + '--run', '--metadata=1.0', '--level', '1', + '--raid-devices', len(efi_partitions), + *efi_partitions) + + if efi_part: + # Blockdev copy the source ESP and erase it + LOG.debug("Relocating EFI %s to %s", efi_part, md_device) + utils.execute('cp', efi_part, md_device) + LOG.debug("Erasing EFI partition %s", efi_part) + utils.execute('wipefs', '-a', efi_part) + else: + fslabel = 'efi-part' + ilib_utils.mkfs(fs='vfat', path=md_device, label=fslabel) + + return md_device elif target_boot_mode == 'bios': partlabel_prefix = 'bios-boot-part-' @@ -476,9 +481,6 @@ def _prepare_boot_partitions_for_softraid(device, holders, efi_part, # Since there is a structural difference, this means it will # fail. - # Just an empty list if not uefi boot mode, nvm, not used anyway - return efi_partitions - def _umount_all_partitions(path, path_variable, umount_warn_msg): """Umount all partitions we may have mounted""" @@ -508,7 +510,7 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None, """Install GRUB2 bootloader on a given device.""" LOG.debug("Installing GRUB2 bootloader on device %s", device) - efi_partitions = None + efi_partition = None efi_part = None efi_partition_mount_point = None efi_mounted = False @@ -545,15 +547,14 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None, path = tempfile.mkdtemp() if efi_system_part_uuid: efi_part = _get_partition(device, uuid=efi_system_part_uuid) - efi_partitions = [efi_part] - + efi_partition = efi_part if hardware.is_md_device(device): holders = hardware.get_holder_disks(device) - efi_partitions = _prepare_boot_partitions_for_softraid( + efi_partition = _prepare_boot_partitions_for_softraid( device, holders, efi_part, target_boot_mode ) - if efi_partitions: + if efi_partition: efi_partition_mount_point = os.path.join(path, "boot/efi") # For power we want to install grub directly onto the PreP partition @@ -580,7 +581,7 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None, # point if we have no efi partitions at all. efi_preserved = _try_preserve_efi_assets( device, path, efi_system_part_uuid, - efi_partitions, efi_partition_mount_point) + efi_partition, efi_partition_mount_point) if efi_preserved: _append_uefi_to_fstab(path, efi_system_part_uuid) # Success preserving efi assets @@ -609,50 +610,48 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None, {'path': path}, shell=True, env_variables={'PATH': path_variable}) - if efi_partitions: + if efi_partition: if not os.path.exists(efi_partition_mount_point): os.makedirs(efi_partition_mount_point) - LOG.warning("GRUB2 will be installed for UEFI on efi partitions " + LOG.warning("GRUB2 will be installed for UEFI on efi partition " "%s using the install command which does not place " - "Secure Boot signed binaries.", efi_partitions) - for efi_partition in efi_partitions: - utils.execute( - 'mount', efi_partition, efi_partition_mount_point) - efi_mounted = True - # FIXME(rg): does not work in cross boot mode case (target - # boot mode differs from ramdisk one) - # Probe for the correct target (depends on the arch, example - # --target=x86_64-efi) - utils.execute('chroot %(path)s /bin/sh -c ' - '"%(bin)s-install"' % - {'path': path, 'bin': binary_name}, - shell=True, - env_variables={ - 'PATH': path_variable - }) - # Also run grub-install with --removable, this installs grub to - # the EFI fallback path. Useful if the NVRAM wasn't written - # correctly, was reset or if testing with virt as libvirt - # resets the NVRAM on instance start. - # This operation is essentially a copy operation. Use of the - # --removable flag, per the grub-install source code changes - # the default file to be copied, destination file name, and - # prevents NVRAM from being updated. - # We only run grub2_install for uefi if we can't verify the - # uefi bits - utils.execute('chroot %(path)s /bin/sh -c ' - '"%(bin)s-install --removable"' % - {'path': path, 'bin': binary_name}, - shell=True, - env_variables={ - 'PATH': path_variable - }) - utils.execute('umount', efi_partition_mount_point, attempts=3, - delay_on_retry=True) - efi_mounted = False + "Secure Boot signed binaries.", efi_partition) + utils.execute('mount', efi_partition, efi_partition_mount_point) + efi_mounted = True + # FIXME(rg): does not work in cross boot mode case (target + # boot mode differs from ramdisk one) + # Probe for the correct target (depends on the arch, example + # --target=x86_64-efi) + utils.execute('chroot %(path)s /bin/sh -c ' + '"%(bin)s-install"' % + {'path': path, 'bin': binary_name}, + shell=True, + env_variables={ + 'PATH': path_variable + }) + # Also run grub-install with --removable, this installs grub to + # the EFI fallback path. Useful if the NVRAM wasn't written + # correctly, was reset or if testing with virt as libvirt + # resets the NVRAM on instance start. + # This operation is essentially a copy operation. Use of the + # --removable flag, per the grub-install source code changes + # the default file to be copied, destination file name, and + # prevents NVRAM from being updated. + # We only run grub2_install for uefi if we can't verify the + # uefi bits + utils.execute('chroot %(path)s /bin/sh -c ' + '"%(bin)s-install --removable"' % + {'path': path, 'bin': binary_name}, + shell=True, + env_variables={ + 'PATH': path_variable + }) + utils.execute('umount', efi_partition_mount_point, attempts=3, + delay_on_retry=True) + efi_mounted = False # NOTE: probably never needed for grub-mkconfig, does not hurt in # case of doubt, cleaned in the finally clause anyway - utils.execute('mount', efi_partitions[0], + utils.execute('mount', efi_partition, efi_partition_mount_point) efi_mounted = True else: @@ -784,7 +783,7 @@ def _mount_for_chroot(path): def _try_preserve_efi_assets(device, path, efi_system_part_uuid, - efi_partitions, + efi_partition, efi_partition_mount_point): """Attempt to preserve UEFI boot assets. @@ -794,8 +793,8 @@ def _try_preserve_efi_assets(device, path, which we should examine to preserve assets from. :param efi_system_part_uuid: The partition ID representing the created EFI system partition. - :param efi_partitions: The list of partitions upon wich to - write the preserved assets to. + :param efi_partition: The partitions upon wich to write the preserved + assets to. :param efi_partition_mount_point: The folder at which to mount the assets for the process of preservation. @@ -818,7 +817,7 @@ def _try_preserve_efi_assets(device, path, # But first, if we have grub, we should try to build a grub config! LOG.debug('EFI asset folder detected, attempting to preserve assets.') if _preserve_efi_assets(path, efi_assets_folder, - efi_partitions, + efi_partition, efi_partition_mount_point): try: # Since we have preserved the assets, we should be able @@ -898,21 +897,21 @@ def _efi_boot_setup(device, efi_system_part_uuid=None, target_boot_mode=None): return False -def _preserve_efi_assets(path, efi_assets_folder, efi_partitions, +def _preserve_efi_assets(path, efi_assets_folder, efi_partition, efi_partition_mount_point): """Preserve the EFI assets in a partition image. :param path: The path used for the mounted image filesystem. :param efi_assets_folder: The folder where we can find the UEFI assets required for booting. - :param efi_partitions: The list of partitions upon which to - write the perserved assets to. + :param efi_partition: The partition upon which to write the + perserved assets to. :param efi_partition_mount_point: The folder at which to mount the assets for the process of preservation. :returns: True if EFI assets were able to be located and preserved to their appropriate locations based upon the supplied - efi_partitions list. + efi_partition. False if any error is encountered in this process. """ try: @@ -961,18 +960,16 @@ def _preserve_efi_assets(path, efi_assets_folder, efi_partitions, except (IOError, OSError, shutil.SameFileError) as e: LOG.warning('Failed to copy grubenv file. ' 'Error: %s', e) - # Loop through partitions because software RAID. - for efi_part in efi_partitions: - utils.execute('mount', '-t', 'vfat', efi_part, - efi_partition_mount_point) - shutil.copytree(save_efi, efi_assets_folder) - LOG.debug('Files preserved to %(disk)s for %(part)s. ' - 'Files: %(filelist)s From: %(from)s', - {'disk': efi_part, - 'part': efi_partition_mount_point, - 'filelist': os.listdir(efi_assets_folder), - 'from': save_efi}) - utils.execute('umount', efi_partition_mount_point) + utils.execute('mount', '-t', 'vfat', efi_partition, + efi_partition_mount_point) + shutil.copytree(save_efi, efi_assets_folder) + LOG.debug('Files preserved to %(disk)s for %(part)s. ' + 'Files: %(filelist)s From: %(from)s', + {'disk': efi_partition, + 'part': efi_partition_mount_point, + 'filelist': os.listdir(efi_assets_folder), + 'from': save_efi}) + utils.execute('umount', efi_partition_mount_point) return True except Exception as e: LOG.debug('Failed to preserve EFI assets. Error %s', e) diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index d7093b6b2..4c4ce03a2 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -1200,7 +1200,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock_preserve_efi_assets.assert_called_with( self.fake_dir, self.fake_dir + '/boot/efi/EFI', - ['/dev/fake1'], + '/dev/fake1', self.fake_dir + '/boot/efi') mock_append_to_fstab.assert_called_with(self.fake_dir, self.fake_efi_system_part_uuid) @@ -1526,16 +1526,17 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n (None, None), # partprobe (None, None), # blkid ('/dev/sda12: dsfkgsdjfg', None), # blkid - (None, None), # cp ('452', None), # sgdisk -F (None, None), # sgdisk create part (None, None), # partprobe (None, None), # blkid ('/dev/sdb14: whatever', None), # blkid + (None, None), # mdadm (None, None), # cp + (None, None), # wipefs ] - efi_parts = image._prepare_boot_partitions_for_softraid( + efi_part = image._prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], None, target_boot_mode='uefi') @@ -1548,7 +1549,6 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock.call('blkid'), mock.call('blkid', '-l', '-t', 'PARTLABEL=uefi-holder-0', '/dev/sda'), - mock.call('cp', '/dev/md0p12', '/dev/sda12'), mock.call('sgdisk', '-F', '/dev/sdb'), mock.call('sgdisk', '-n', '0:452s:+550MiB', '-t', '0:ef00', '-c', '0:uefi-holder-1', '/dev/sdb'), @@ -1556,10 +1556,14 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock.call('blkid'), mock.call('blkid', '-l', '-t', 'PARTLABEL=uefi-holder-1', '/dev/sdb'), - mock.call('cp', '/dev/md0p12', '/dev/sdb14') + mock.call('mdadm', '--create', '/dev/md/esp', '--force', '--run', + '--metadata=1.0', '--level', '1', '--raid-devices', 2, + '/dev/sda12', '/dev/sdb14'), + mock.call('cp', '/dev/md0p12', '/dev/md/esp'), + mock.call('wipefs', '-a', '/dev/md0p12') ] mock_execute.assert_has_calls(expected, any_order=False) - self.assertEqual(efi_parts, ['/dev/sda12', '/dev/sdb14']) + self.assertEqual(efi_part, '/dev/md/esp') @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True) @mock.patch.object(ilib_utils, 'mkfs', autospec=True) @@ -1577,9 +1581,10 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n (None, None), # partprobe (None, None), # blkid ('/dev/sdb14: whatever', None), # blkid + (None, None), # mdadm ] - efi_parts = image._prepare_boot_partitions_for_softraid( + efi_part = image._prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], None, target_boot_mode='uefi') @@ -1602,10 +1607,9 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n ] mock_execute.assert_has_calls(expected, any_order=False) mock_mkfs.assert_has_calls([ - mock.call(path='/dev/sda12', label='efi-part', fs='vfat'), - mock.call(path='/dev/sdb14', label='efi-part-b', fs='vfat'), + mock.call(path='/dev/md/esp', label='efi-part', fs='vfat'), ], any_order=False) - self.assertEqual(efi_parts, ['/dev/sda12', '/dev/sdb14']) + self.assertEqual(efi_part, '/dev/md/esp') def test__prepare_boot_partitions_for_softraid_uefi_gpt_efi_provided( self, mock_execute, mock_dispatch): @@ -1615,16 +1619,17 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n (None, None), # partprobe (None, None), # blkid ('/dev/sda12: dsfkgsdjfg', None), # blkid - (None, None), # cp ('452', None), # sgdisk -F (None, None), # sgdisk create part (None, None), # partprobe (None, None), # blkid ('/dev/sdb14: whatever', None), # blkid + (None, None), # mdadm create (None, None), # cp + (None, None), # wipefs ] - efi_parts = image._prepare_boot_partitions_for_softraid( + efi_part = image._prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], '/dev/md0p15', target_boot_mode='uefi') @@ -1636,7 +1641,6 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock.call('blkid'), mock.call('blkid', '-l', '-t', 'PARTLABEL=uefi-holder-0', '/dev/sda'), - mock.call('cp', '/dev/md0p15', '/dev/sda12'), mock.call('sgdisk', '-F', '/dev/sdb'), mock.call('sgdisk', '-n', '0:452s:+550MiB', '-t', '0:ef00', '-c', '0:uefi-holder-1', '/dev/sdb'), @@ -1644,17 +1648,21 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock.call('blkid'), mock.call('blkid', '-l', '-t', 'PARTLABEL=uefi-holder-1', '/dev/sdb'), - mock.call('cp', '/dev/md0p15', '/dev/sdb14') + mock.call('mdadm', '--create', '/dev/md/esp', '--force', '--run', + '--metadata=1.0', '--level', '1', '--raid-devices', 2, + '/dev/sda12', '/dev/sdb14'), + mock.call('cp', '/dev/md0p15', '/dev/md/esp'), + mock.call('wipefs', '-a', '/dev/md0p15') ] mock_execute.assert_has_calls(expected, any_order=False) - self.assertEqual(efi_parts, ['/dev/sda12', '/dev/sdb14']) + self.assertEqual(efi_part, '/dev/md/esp') @mock.patch.object(utils, 'scan_partition_table_type', autospec=True, return_value='msdos') def test__prepare_boot_partitions_for_softraid_bios_msdos( self, mock_label_scan, mock_execute, mock_dispatch): - efi_parts = image._prepare_boot_partitions_for_softraid( + efi_part = image._prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], 'notusedanyway', target_boot_mode='bios') @@ -1663,7 +1671,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock.call('/dev/sdb'), ] mock_label_scan.assert_has_calls(expected, any_order=False) - self.assertEqual(efi_parts, []) + self.assertIsNone(efi_part) @mock.patch.object(utils, 'scan_partition_table_type', autospec=True, return_value='gpt') @@ -1677,7 +1685,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n (None, None), # bios boot grub ] - efi_parts = image._prepare_boot_partitions_for_softraid( + efi_part = image._prepare_boot_partitions_for_softraid( '/dev/md0', ['/dev/sda', '/dev/sdb'], 'notusedanyway', target_boot_mode='bios') @@ -1698,7 +1706,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n ] mock_execute.assert_has_calls(expected_exec, any_order=False) - self.assertEqual(efi_parts, []) + self.assertIsNone(efi_part) @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: True) @mock.patch.object(hardware, 'is_md_device', autospec=True) @@ -1711,7 +1719,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n @mock.patch.object(image, '_get_partition', autospec=True) @mock.patch.object(image, '_prepare_boot_partitions_for_softraid', autospec=True, - return_value=['/dev/sda1', '/dev/sdb2']) + return_value='/dev/md/esp') @mock.patch.object(image, '_has_dracut', autospec=True, return_value=False) @@ -1746,7 +1754,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n (self.fake_dir)), shell=True, env_variables={ 'PATH': '/sbin:/bin:/usr/sbin:/sbin'}), - mock.call('mount', '/dev/sda1', + mock.call('mount', '/dev/md/esp', self.fake_dir + '/boot/efi'), mock.call(('chroot %s /bin/sh -c "grub-install"' % self.fake_dir), shell=True, @@ -1760,21 +1768,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock.call( 'umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), - mock.call('mount', '/dev/sdb2', - self.fake_dir + '/boot/efi'), - mock.call(('chroot %s /bin/sh -c "grub-install"' % - self.fake_dir), shell=True, - env_variables={ - 'PATH': '/sbin:/bin:/usr/sbin:/sbin'}), - mock.call(('chroot %s /bin/sh -c ' - '"grub-install --removable"' % - self.fake_dir), shell=True, - env_variables={ - 'PATH': '/sbin:/bin:/usr/sbin:/sbin'}), - mock.call( - 'umount', self.fake_dir + '/boot/efi', - attempts=3, delay_on_retry=True), - mock.call('mount', '/dev/sda1', + mock.call('mount', '/dev/md/esp', '/tmp/fake-dir/boot/efi'), mock.call(('chroot %s /bin/sh -c ' '"grub-mkconfig -o ' diff --git a/releasenotes/notes/software-raid-raid-ESPs-25a2aa117b99620a.yaml b/releasenotes/notes/software-raid-raid-ESPs-25a2aa117b99620a.yaml new file mode 100644 index 000000000..a45a7ff43 --- /dev/null +++ b/releasenotes/notes/software-raid-raid-ESPs-25a2aa117b99620a.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Mirrors the previously disconnected EFI system partitions (ESPs) in UEFI + software RAID setups. Disconnected ESPs can lead to nodes booting with + outdated kernel parameters or the UEFI firmware not finding bootable + kernels at all.