Merge "Delete EFI boot entry duplicate labels first" into stable/wallaby

3 files changed, 51 insertions, 24 deletions

diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py
index 580d06eb..209c9b23 100644
--- a/ironic_python_agent/extensions/image.py
+++ b/ironic_python_agent/extensions/image.py
@@ -275,12 +275,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():
@@ -297,20 +295,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 5dc46ae6..7a382eeb 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)
@@ -2175,8 +2178,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, ''),
                                          ('', ''), ('', ''),
                                          ('', ''), ('', ''),
                                          ('', '')])
@@ -2187,6 +2201,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',
@@ -2201,7 +2217,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)
@@ -2369,6 +2385,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 00000000..52f6fc0e
--- /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.