From f09f6c9f1a09c7062d0450b3e0a4d3164fd53f7f Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 4 May 2022 14:45:33 +0200 Subject: [PATCH] Do not try to guess EFI partition path by its number The logic of adding a partition number to the device path does not work for devicemapper devices (e.g. a multipath storage device). Change-Id: I9a445e847d282c50adfa4bad5e7136776861005d --- ironic_python_agent/efi_utils.py | 75 ++++++++++++++----- .../tests/unit/extensions/test_image.py | 34 +++++---- .../tests/unit/samples/hardware_samples.py | 11 +++ .../tests/unit/test_efi_utils.py | 75 +++++++++++++------ .../notes/efi-partuuid-5fe933a462eeede1.yaml | 5 ++ 5 files changed, 142 insertions(+), 58 deletions(-) create mode 100644 releasenotes/notes/efi-partuuid-5fe933a462eeede1.yaml diff --git a/ironic_python_agent/efi_utils.py b/ironic_python_agent/efi_utils.py index bc7156bd4..48e643d3f 100644 --- a/ironic_python_agent/efi_utils.py +++ b/ironic_python_agent/efi_utils.py @@ -29,6 +29,37 @@ from ironic_python_agent import utils LOG = log.getLogger(__name__) +def get_partition_path_by_number(device, part_num): + """Get partition path (/dev/something) by a partition number on device. + + Only works for GPT partition table. + """ + uuid = None + partinfo, _ = utils.execute('sgdisk', '-i', str(part_num), device, + use_standard_locale=True) + for line in partinfo.splitlines(): + if not line.strip(): + continue + + try: + field, value = line.rsplit(':', 1) + except ValueError: + LOG.warning('Invalid sgdisk line: %s', line) + continue + + if 'partition unique guid' in field.lower(): + uuid = value.strip().lower() + LOG.debug('GPT partition number %s on device %s has UUID %s', + part_num, device, uuid) + break + + if uuid is not None: + return partition_utils.get_partition(device, uuid) + else: + LOG.warning('No UUID information provided in sgdisk output for ' + 'partition %s on device %s', part_num, device) + + def manage_uefi(device, efi_system_part_uuid=None): """Manage the device looking for valid efi bootloaders to update the nvram. @@ -52,19 +83,33 @@ def manage_uefi(device, efi_system_part_uuid=None): # Trust the contents on the disk in the event of a whole disk image. efi_partition = disk_utils.find_efi_partition(device) if efi_partition: - efi_partition = efi_partition['number'] + efi_part_num = efi_partition['number'] + efi_partition = get_partition_path_by_number(device, efi_part_num) if not efi_partition and efi_system_part_uuid: - # _get_partition returns + and we only need the + # get_partition returns + and we only need the # partition number - partition = partition_utils.get_partition( + efi_partition = partition_utils.get_partition( device, uuid=efi_system_part_uuid) + # FIXME(dtantsur): this procedure will not work for devicemapper + # devices. To fix that we need a way to convert a UUID to a partition + # number, which is surprisingly non-trivial and may involve looping + # over existing numbers and calling `sgdisk -i` for each of them. + # But I'm not sure we even need this logic: find_efi_partition should + # be sufficient for both whole disk and partition images. try: - efi_partition = int(partition.replace(device, "")) + efi_part_num = int(efi_partition.replace(device, "")) except ValueError: # NVMe Devices get a partitioning scheme that is different from # traditional block devices like SCSI/SATA - efi_partition = int(partition.replace(device + 'p', "")) + try: + efi_part_num = int(efi_partition.replace(device + 'p', "")) + except ValueError as exc: + # At least provide a reasonable error message if the device + # does not follow this procedure. + raise errors.DeviceNotFound( + "Cannot detect the partition number of the device %s: %s" % + (efi_partition, exc)) if not efi_partition: # NOTE(dtantsur): we cannot have a valid EFI deployment without an @@ -82,16 +127,8 @@ def manage_uefi(device, efi_system_part_uuid=None): if not os.path.exists(efi_partition_mount_point): os.makedirs(efi_partition_mount_point) - # The mount needs the device with the partition, in case the - # device ends with a digit we add a `p` and the partition number we - # found, otherwise we just join the device and the partition number - if device[-1].isdigit(): - efi_device_part = '{}p{}'.format(device, efi_partition) - else: - efi_device_part = '{}{}'.format(device, efi_partition) - try: - utils.execute('mount', efi_device_part, efi_partition_mount_point) + utils.execute('mount', efi_partition, efi_partition_mount_point) efi_mounted = True valid_efi_bootloaders = _get_efi_bootloaders(efi_partition_mount_point) @@ -103,7 +140,7 @@ def manage_uefi(device, efi_system_part_uuid=None): if not hardware.is_md_device(device): efi_devices = [device] - efi_partition_numbers = [efi_partition] + efi_partition_numbers = [efi_part_num] efi_label_suffix = '' else: # umount to allow for signature removal (to avoid confusion about @@ -114,7 +151,7 @@ def manage_uefi(device, efi_system_part_uuid=None): holders = hardware.get_holder_disks(device) efi_md_device = raid_utils.prepare_boot_partitions_for_softraid( - device, holders, efi_device_part, target_boot_mode='uefi' + device, holders, efi_partition, target_boot_mode='uefi' ) efi_devices = hardware.get_component_devices(efi_md_device) efi_partition_numbers = [] @@ -131,12 +168,12 @@ def manage_uefi(device, efi_system_part_uuid=None): efi_label_suffix = "(RAID, part%s)" # remount for _run_efibootmgr - utils.execute('mount', efi_device_part, efi_partition_mount_point) + utils.execute('mount', efi_partition, efi_partition_mount_point) efi_mounted = True efi_dev_part = zip(efi_devices, efi_partition_numbers) for i, (efi_dev, efi_part) in enumerate(efi_dev_part): - LOG.debug("Calling efibootmgr with dev %s part %s", + LOG.debug("Calling efibootmgr with dev %s partition number %s", efi_dev, efi_part) if efi_label_suffix: # NOTE (arne_wiebalck): uniqify the labels to prevent @@ -255,7 +292,7 @@ def add_boot_record(device, efi_partition, loader, label): """ # https://linux.die.net/man/8/efibootmgr utils.execute('efibootmgr', '-v', '-c', '-d', device, - '-p', efi_partition, '-w', '-L', label, + '-p', str(efi_partition), '-w', '-L', label, '-l', loader) diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index 00aaf706c..e04fb3bc3 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -219,14 +219,14 @@ class TestImageExtension(base.IronicAgentTest): @mock.patch.object(disk_utils, 'find_efi_partition', autospec=False) @mock.patch.object(os, 'makedirs', autospec=True) def test__uefi_bootloader_given_partition( - self, mkdir_mock, mock_utils_efi_part, mock_partition, + self, mkdir_mock, mock_utils_efi_part, mock_get_partition, mock_efi_bl, mock_execute, mock_dispatch): mock_dispatch.side_effect = [ self.fake_dev, hardware.BootInfo(current_boot_mode='uefi') ] - mock_partition.side_effect = [self.fake_dev, self.fake_efi_system_part] + mock_get_partition.return_value = self.fake_efi_system_part mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] - mock_utils_efi_part.return_value = {'number': '1'} + mock_utils_efi_part.return_value = None mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), @@ -263,16 +263,17 @@ class TestImageExtension(base.IronicAgentTest): @mock.patch.object(hardware, 'is_md_device', lambda *_: False) @mock.patch.object(os.path, 'exists', lambda *_: False) @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) - @mock.patch.object(partition_utils, 'get_partition', autospec=True) + @mock.patch.object(efi_utils, 'get_partition_path_by_number', + autospec=True) @mock.patch.object(disk_utils, 'find_efi_partition', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test__uefi_bootloader_find_partition( - self, mkdir_mock, mock_utils_efi_part, mock_partition, + self, mkdir_mock, mock_utils_efi_part, mock_get_part_path, mock_efi_bl, mock_execute, mock_dispatch): mock_dispatch.side_effect = [ self.fake_dev, hardware.BootInfo(current_boot_mode='uefi') ] - mock_partition.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_utils_efi_part.return_value = {'number': '1'} mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] mock_execute.side_effect = iter([('', ''), ('', ''), @@ -310,16 +311,17 @@ class TestImageExtension(base.IronicAgentTest): @mock.patch.object(hardware, 'is_md_device', lambda *_: False) @mock.patch.object(os.path, 'exists', lambda *_: False) @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) - @mock.patch.object(partition_utils, 'get_partition', autospec=True) + @mock.patch.object(efi_utils, 'get_partition_path_by_number', + autospec=True) @mock.patch.object(disk_utils, 'find_efi_partition', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test__uefi_bootloader_with_entry_removal( - self, mkdir_mock, mock_utils_efi_part, mock_partition, + self, mkdir_mock, mock_utils_efi_part, mock_get_part_path, mock_efi_bl, mock_execute, mock_dispatch): mock_dispatch.side_effect = [ self.fake_dev, hardware.BootInfo(current_boot_mode='uefi') ] - mock_partition.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_utils_efi_part.return_value = {'number': '1'} mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] stdout_msg = """ @@ -367,16 +369,17 @@ Boot0002 VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) @mock.patch.object(hardware, 'is_md_device', lambda *_: False) @mock.patch.object(os.path, 'exists', lambda *_: False) @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) - @mock.patch.object(partition_utils, 'get_partition', autospec=True) + @mock.patch.object(efi_utils, 'get_partition_path_by_number', + autospec=True) @mock.patch.object(disk_utils, 'find_efi_partition', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test__uefi_bootloader_with_entry_removal_lenovo( - self, mkdir_mock, mock_utils_efi_part, mock_partition, + self, mkdir_mock, mock_utils_efi_part, mock_get_part_path, mock_efi_bl, mock_execute, mock_dispatch): mock_dispatch.side_effect = [ self.fake_dev, hardware.BootInfo(current_boot_mode='uefi') ] - mock_partition.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_utils_efi_part.return_value = {'number': '1'} mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] # NOTE(TheJulia): This test string was derived from a lenovo SR650 @@ -429,16 +432,17 @@ Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640 @mock.patch.object(hardware, 'is_md_device', lambda *_: False) @mock.patch.object(os.path, 'exists', lambda *_: False) @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) - @mock.patch.object(partition_utils, 'get_partition', autospec=True) + @mock.patch.object(efi_utils, 'get_partition_path_by_number', + autospec=True) @mock.patch.object(disk_utils, 'find_efi_partition', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test__add_multi_bootloaders( - self, mkdir_mock, mock_utils_efi_part, mock_partition, + self, mkdir_mock, mock_utils_efi_part, mock_get_part_path, mock_efi_bl, mock_execute, mock_dispatch): mock_dispatch.side_effect = [ self.fake_dev, hardware.BootInfo(current_boot_mode='uefi') ] - mock_partition.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_utils_efi_part.return_value = {'number': '1'} mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI', 'WINDOWS/system32/winload.efi'] diff --git a/ironic_python_agent/tests/unit/samples/hardware_samples.py b/ironic_python_agent/tests/unit/samples/hardware_samples.py index 8cc9b1ea2..660c61b53 100644 --- a/ironic_python_agent/tests/unit/samples/hardware_samples.py +++ b/ironic_python_agent/tests/unit/samples/hardware_samples.py @@ -1500,3 +1500,14 @@ NVME_CLI_INFO_TEMPLATE_FORMAT_UNSUPPORTED = (""" ] } """) + + +SGDISK_INFO_TEMPLATE = (""" +Partition GUID code: C12A7328-F81F-11D2-BA4B-00A0C93EC93B (EFI system partition) +Partition unique GUID: FAED7408-6D92-4FC6-883B-9069E2274ECA +First sector: 2048 (at 1024.0 KiB) +Last sector: 1050623 (at 513.0 MiB) +Partition size: 1048576 sectors (512.0 MiB) +Attribute flags: 0000000000000000 +Partition name: 'EFI System Partition' +""") # noqa diff --git a/ironic_python_agent/tests/unit/test_efi_utils.py b/ironic_python_agent/tests/unit/test_efi_utils.py index fef15e173..64de61bce 100644 --- a/ironic_python_agent/tests/unit/test_efi_utils.py +++ b/ironic_python_agent/tests/unit/test_efi_utils.py @@ -24,6 +24,7 @@ from ironic_python_agent import hardware from ironic_python_agent import partition_utils from ironic_python_agent import raid_utils from ironic_python_agent.tests.unit import base +from ironic_python_agent.tests.unit.samples import hardware_samples from ironic_python_agent import utils @@ -133,6 +134,7 @@ class TestRunEfiBootmgr(base.IronicAgentTest): @mock.patch.object(utils, 'rescan_device', autospec=True) @mock.patch.object(utils, 'execute', autospec=True) @mock.patch.object(partition_utils, 'get_partition', autospec=True) +@mock.patch.object(efi_utils, 'get_partition_path_by_number', autospec=True) @mock.patch.object(disk_utils, 'find_efi_partition', autospec=True) class TestManageUefi(base.IronicAgentTest): @@ -143,6 +145,7 @@ class TestManageUefi(base.IronicAgentTest): fake_dir = '/tmp/fake-dir' def test_no_partition(self, mock_utils_efi_part, + mock_get_part_path, mock_get_part_uuid, mock_execute, mock_rescan): mock_utils_efi_part.return_value = None @@ -152,6 +155,7 @@ class TestManageUefi(base.IronicAgentTest): mock_rescan.assert_called_once_with(self.fake_dev) def test_empty_partition_by_uuid(self, mock_utils_efi_part, + mock_get_part_path, mock_get_part_uuid, mock_execute, mock_rescan): mock_utils_efi_part.return_value = None @@ -165,10 +169,10 @@ class TestManageUefi(base.IronicAgentTest): @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test_ok(self, mkdir_mock, mock_efi_bl, mock_is_md_device, - mock_utils_efi_part, mock_get_part_uuid, mock_execute, - mock_rescan): + mock_utils_efi_part, mock_get_part_path, mock_get_part_uuid, + mock_execute, mock_rescan): mock_utils_efi_part.return_value = {'number': '1'} - mock_get_part_uuid.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_is_md_device.return_value = False mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] @@ -202,10 +206,10 @@ class TestManageUefi(base.IronicAgentTest): @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test_found_csv(self, mkdir_mock, mock_efi_bl, mock_is_md_device, - mock_utils_efi_part, mock_get_part_uuid, mock_execute, - mock_rescan): + mock_utils_efi_part, mock_get_part_path, + mock_get_part_uuid, mock_execute, mock_rescan): mock_utils_efi_part.return_value = {'number': '1'} - mock_get_part_uuid.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_efi_bl.return_value = ['EFI/vendor/BOOTX64.CSV'] mock_is_md_device.return_value = False @@ -255,9 +259,9 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test_nvme_device(self, mkdir_mock, mock_efi_bl, mock_is_md_device, - mock_utils_efi_part, mock_get_part_uuid, - mock_execute, mock_rescan): - mock_utils_efi_part.return_value = {'number': '1'} + mock_utils_efi_part, mock_get_part_path, + mock_get_part_uuid, mock_execute, mock_rescan): + mock_utils_efi_part.return_value = None mock_get_part_uuid.return_value = '/dev/fakenvme0p1' mock_is_md_device.return_value = False @@ -290,10 +294,10 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test_wholedisk(self, mkdir_mock, mock_efi_bl, mock_is_md_device, - mock_utils_efi_part, mock_get_part_uuid, mock_execute, - mock_rescan): + mock_utils_efi_part, mock_get_part_path, + mock_get_part_uuid, mock_execute, mock_rescan): mock_utils_efi_part.return_value = {'number': '1'} - mock_get_part_uuid.side_effect = Exception + mock_get_part_path.return_value = self.fake_efi_system_part mock_is_md_device.return_value = False mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] @@ -319,6 +323,7 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi') mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi') mock_execute.assert_has_calls(expected) + mock_get_part_uuid.assert_not_called() @mock.patch.object(os.path, 'exists', lambda *_: False) @mock.patch.object(hardware, 'get_component_devices', autospec=True) @@ -332,10 +337,10 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) def test_software_raid(self, mkdir_mock, mock_efi_bl, mock_is_md_device, mock_get_holder_disks, mock_prepare, mock_get_component_devices, - mock_utils_efi_part, mock_get_part_uuid, - mock_execute, mock_rescan): + mock_utils_efi_part, mock_get_part_path, + mock_get_part_uuid, mock_execute, mock_rescan): mock_utils_efi_part.return_value = {'number': '1'} - mock_get_part_uuid.side_effect = Exception + mock_get_part_path.return_value = self.fake_efi_system_part mock_is_md_device.return_value = True mock_get_holder_disks.return_value = ['/dev/sda', '/dev/sdb'] mock_prepare.return_value = '/dev/md125' @@ -378,10 +383,10 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) @mock.patch.object(os, 'makedirs', autospec=True) def test_failure(self, mkdir_mock, mock_efi_bl, mock_is_md_device, - mock_utils_efi_part, mock_get_part_uuid, mock_execute, - mock_rescan): + mock_utils_efi_part, mock_get_part_path, + mock_get_part_uuid, mock_execute, mock_rescan): mock_utils_efi_part.return_value = {'number': '1'} - mock_get_part_uuid.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_is_md_device.return_value = False mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] @@ -403,10 +408,10 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) @mock.patch.object(os, 'makedirs', autospec=True) def test_failure_after_mount(self, mkdir_mock, mock_efi_bl, mock_is_md_device, mock_utils_efi_part, - mock_get_part_uuid, mock_execute, - mock_rescan): + mock_get_part_path, mock_get_part_uuid, + mock_execute, mock_rescan): mock_utils_efi_part.return_value = {'number': '1'} - mock_get_part_uuid.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_is_md_device.return_value = False mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] @@ -440,10 +445,10 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) @mock.patch.object(os, 'makedirs', autospec=True) def test_failure_after_failure(self, mkdir_mock, mock_efi_bl, mock_is_md_device, mock_utils_efi_part, - mock_get_part_uuid, mock_execute, - mock_rescan): + mock_get_part_path, mock_get_part_uuid, + mock_execute, mock_rescan): mock_utils_efi_part.return_value = {'number': '1'} - mock_get_part_uuid.return_value = self.fake_dev + mock_get_part_path.return_value = self.fake_efi_system_part mock_is_md_device.return_value = False mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] @@ -469,3 +474,25 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mock_execute.assert_has_calls(expected) self.assertEqual(3, mock_execute.call_count) mock_rescan.assert_called_once_with(self.fake_dev) + + +@mock.patch.object(partition_utils, 'get_partition', autospec=True) +@mock.patch.object(utils, 'execute', autospec=True) +class TestGetPartitionPathByNumber(base.IronicAgentTest): + + def test_ok(self, mock_execute, mock_get_partition): + mock_execute.return_value = (hardware_samples.SGDISK_INFO_TEMPLATE, '') + mock_get_partition.return_value = '/dev/fake1' + + result = efi_utils.get_partition_path_by_number('/dev/fake', 1) + self.assertEqual('/dev/fake1', result) + + mock_execute.assert_called_once_with('sgdisk', '-i', '1', '/dev/fake', + use_standard_locale=True) + + def test_broken(self, mock_execute, mock_get_partition): + mock_execute.return_value = ('I am a teaport', '') + + self.assertIsNone( + efi_utils.get_partition_path_by_number('/dev/fake', 1)) + mock_get_partition.assert_not_called() diff --git a/releasenotes/notes/efi-partuuid-5fe933a462eeede1.yaml b/releasenotes/notes/efi-partuuid-5fe933a462eeede1.yaml new file mode 100644 index 000000000..f1a7924f0 --- /dev/null +++ b/releasenotes/notes/efi-partuuid-5fe933a462eeede1.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes configuring UEFI boot when the EFI partition is located on a + devicemapper device.