diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index 306e5c4a0..822a89d9f 100644 --- a/ironic_python_agent/extensions/image.py +++ b/ironic_python_agent/extensions/image.py @@ -270,12 +270,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(): @@ -292,20 +290,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 c17bd6ada..3e4d892cf 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -344,13 +344,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, ''), ('', ''), ('', ''), ('', ''), ('', ''), ('', '')]) @@ -361,12 +365,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')] @@ -381,7 +384,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) @@ -2150,8 +2153,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, ''), ('', ''), ('', ''), ('', ''), ('', ''), ('', '')]) @@ -2162,6 +2176,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', @@ -2176,7 +2192,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) @@ -2344,6 +2360,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.