From 466956bbd0857f792c562f6953aba5b73875a667 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 28 Oct 2021 11:41:00 -0700 Subject: [PATCH] Delete EFI boot entry duplicate labels first Some firmware seems to take an objection with EFI nvram entries being deleted after one is added, resulting in the entire entry table being reset to the last known good state. This is problematic, as ultimately deployments can time out if we previously booted with Networking, and the machine, while commanded to do other wise, reboots back to networking regardless. We will now delete entries first, before proceeding. Additionally, for general use, this pattern may serve the community better by avoiding cases where we would have previously just relied upon efibootmgr[0] to warn us of duplicate entries. [0]: https://github.com/rhboot/efibootmgr/blob/103aa22ece98f09fe3ea2a0c83988f0ee2d0e5a8/src/efibootmgr.c#L228 Change-Id: Ib61a7100a059e79a8b0901fd8f46b9bc41d657dc Story: 2009649 Task: 43808 (cherry picked from commit 67eddfa7e3fedbb530045f5b43a2c89db832fa2a) (cherry picked from commit 33b39705a50513c5af411216b48e2a6f6ac9ab14) (cherry picked from commit 8fca1457399ac7892427fac1b9ab74a4ac653f05) (cherry picked from commit 47ac40a7f943d0ce6e58f43330a73ac144c23aa1) (cherry picked from commit bb5b1ba8de8464dc56f4f6ceacfdbb276fac4a46) --- ironic_python_agent/extensions/image.py | 34 ++++++++++-------- .../tests/unit/extensions/test_image.py | 35 ++++++++++++++----- ...e-duplicate-by-label-baa090c5b1bff992.yaml | 6 ++++ 3 files changed, 51 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/de-duplicate-by-label-baa090c5b1bff992.yaml diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index 0fdf3c5e0..46822db83 100644 --- a/ironic_python_agent/extensions/image.py +++ b/ironic_python_agent/extensions/image.py @@ -268,12 +268,10 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition, # Before updating let's get information about the bootorder LOG.debug("Getting information about boot order.") - utils.execute('efibootmgr', '-v') - # NOTE(iurygregory): regex used to identify the Warning in the stderr after - # we add the new entry. Example: - # "efibootmgr: ** Warning ** : Boot0004 has same label ironic" - duplicated_label = re.compile(r'^.*:\s\*\*.*\*\*\s:\s.*' - r'Boot([0-9a-f-A-F]+)\s.*$') + original_efi_output = utils.execute('efibootmgr', '-v') + # NOTE(TheJulia): regex used to identify entries in the efibootmgr + # output on stdout. + entry_label = re.compile(r'Boot([0-9a-f-A-F]+):\s(.*).*$') label_id = 1 for v_bl in valid_efi_bootloaders: if 'csv' in v_bl.lower(): @@ -295,20 +293,26 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition, v_efi_bl_path = '\\' + v_bl.replace('/', '\\') label = 'ironic' + str(label_id) + # Iterate through standard out, and look for duplicates + for line in original_efi_output[0].split('\n'): + match = entry_label.match(line) + # Look for the base label in the string if a line match + # occurs, so we can identify if we need to eliminate the + # entry. + if match and label in match.group(2): + boot_num = match.group(1) + LOG.debug("Found bootnum %s matching label", boot_num) + utils.execute('efibootmgr', '-b', boot_num, '-B') + LOG.debug("Adding loader %(path)s on partition %(part)s of device " " %(dev)s", {'path': v_efi_bl_path, 'part': efi_partition, 'dev': device}) # Update the nvram using efibootmgr # https://linux.die.net/man/8/efibootmgr - cmd = utils.execute('efibootmgr', '-v', '-c', '-d', device, - '-p', efi_partition, '-w', '-L', label, - '-l', v_efi_bl_path) - for line in cmd[1].split('\n'): - match = duplicated_label.match(line) - if match: - boot_num = match.group(1) - LOG.debug("Found bootnum %s matching label", boot_num) - utils.execute('efibootmgr', '-b', boot_num, '-B') + utils.execute('efibootmgr', '-v', '-c', '-d', device, + '-p', efi_partition, '-w', '-L', label, + '-l', v_efi_bl_path) + # Increment the ID in case the loop runs again. label_id += 1 diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index 52969dd22..30fad20d7 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -318,13 +318,17 @@ class TestImageExtension(base.IronicAgentTest): mock_partition.return_value = self.fake_dev mock_utils_efi_part.return_value = '1' mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] - stdeer_msg = """ -efibootmgr: ** Warning ** : Boot0004 has same label ironic1\n -efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n -""" + stdout_msg = """ +BootCurrent: 0001 +Timeout: 0 seconds +BootOrder: 0000,00001 +Boot0000: ironic1 HD(1, GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI) +Boot0001: ironic2 HD(1, GPT,4f3c6294-bf9b-4208-9808-111111111112)File(\EFI\Boot\BOOTX64.EFI) +Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) +""" # noqa This is a giant literal string for testing. mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - ('', ''), ('', stdeer_msg), + (stdout_msg, ''), ('', ''), ('', ''), ('', ''), ('', ''), ('', '')]) @@ -335,12 +339,11 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-b', '0000', '-B'), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', '\\EFI\\BOOT\\BOOTX64.EFI'), - mock.call('efibootmgr', '-b', '0004', '-B'), - mock.call('efibootmgr', '-b', '0005', '-B'), mock.call('umount', self.fake_dir + '/boot/efi', attempts=3, delay_on_retry=True), mock.call('sync')] @@ -355,7 +358,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi') mock_execute.assert_has_calls(expected) mock_utils_efi_part.assert_called_once_with(self.fake_dev) - self.assertEqual(10, mock_execute.call_count) + self.assertEqual(9, mock_execute.call_count) @mock.patch.object(hardware, 'is_md_device', lambda *_: False) @mock.patch.object(os.path, 'exists', lambda *_: False) @@ -1684,8 +1687,19 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n # Mild difference, Ubuntu ships a file without a 0xFEFF delimiter # at the start of the file, where as Red Hat *does* csv_file_data = u'shimx64.efi,Vendor String,,Grub2MadeUSDoThis\n' + # This test also handles deleting a pre-existing matching vendor + # string in advance. + dupe_entry = """ +BootCurrent: 0001 +Timeout: 0 seconds +BootOrder: 0000,00001 +Boot0000: Vendor String HD(1, GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI) +Boot0001: Vendor String HD(2, GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI) +Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) +""" # noqa This is a giant literal string for testing. mock_execute.side_effect = iter([('', ''), ('', ''), + ('', ''), (dupe_entry, ''), ('', ''), ('', ''), ('', ''), ('', ''), ('', '')]) @@ -1696,6 +1710,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-b', '0000', '-B'), + mock.call('efibootmgr', '-b', '0001', '-B'), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'Vendor String', '-l', @@ -1715,7 +1731,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n 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) - self.assertEqual(7, mock_execute.call_count) + self.assertEqual(9, mock_execute.call_count) @mock.patch.object(os.path, 'exists', lambda *_: False) @mock.patch.object(image, '_get_efi_bootloaders', autospec=True) @@ -1883,6 +1899,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock_execute.assert_has_calls(expected) def test__run_efibootmgr(self, mock_execute, mock_dispatch): + mock_execute.return_value = ('', '') result = image._run_efibootmgr(['EFI/BOOT/BOOTX64.EFI'], self.fake_dev, self.fake_efi_system_part, diff --git a/releasenotes/notes/de-duplicate-by-label-baa090c5b1bff992.yaml b/releasenotes/notes/de-duplicate-by-label-baa090c5b1bff992.yaml new file mode 100644 index 000000000..52f6fc0ee --- /dev/null +++ b/releasenotes/notes/de-duplicate-by-label-baa090c5b1bff992.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes cases where duplicates may not be found in the UEFI + firmware NVRAM boot entry table by explicitly looking for, and deleting + for matching labels in advance of creating the EFI boot loader entry.