curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

Ryan Harper has proposed merging ~raharper/curtin:fix/uefi-remove-dups-v2 into curtin:master.

Commit message:
curthooks: UEFI remove dupes: don't remove BootCurrent, config option

When removing duplicate UEFI bootmenu entries do not remove the
BootCurrent entry.  Fix this by adding BootCurrent to the seen set
before processing the list and then skip it during iteration of the
entries.

- Add grub config option: remove_duplicate_entries bool
- Add documentation around remove_duplicate_entries grub config
- Add unittests to verify we don't remove boot current and to
  check that dupe removal can be disabled.

LP: #1894217

Requested reviews:
  curtin developers (curtin-dev)
Related bugs:
  Bug #1894217 in MAAS: "2.8.2 deploy and commission fails corrupted bootorder variable detected"
  https://bugs.launchpad.net/maas/+bug/1894217

For more details, see:
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/390431
-- 
Your team curtin developers is requested to review the proposed merge of ~raharper/curtin:fix/uefi-remove-dups-v2 into curtin:master.

diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index 92230fc..eaaae5e 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -558,11 +558,21 @@ def uefi_reorder_loaders(grubcfg, target, efi_orig=None, variant=None):
 
 
 def uefi_remove_duplicate_entries(grubcfg, target):
+    if not grubcfg.get('remove_duplicate_entries', True):
+        LOG.debug("Skipped removing duplicate UEFI boot entries per config.")
+        return
     seen = set()
     to_remove = []
     efi_output = util.get_efibootmgr(target=target)
     entries = efi_output.get('entries', {})
+    current_bootnum = efi_output.get('current', None)
+    # adding BootCurrent to seen first allows us to remove any other duplicate
+    # entry of BootCurrent.
+    if current_bootnum:
+        seen.add(tuple(entries[current_bootnum].items()))
     for bootnum in sorted(entries):
+        if bootnum == current_bootnum:
+            continue
         entry = entries[bootnum]
         t = tuple(entry.items())
         if t not in seen:
diff --git a/doc/topics/config.rst b/doc/topics/config.rst
index 951f07f..ec8a109 100644
--- a/doc/topics/config.rst
+++ b/doc/topics/config.rst
@@ -255,6 +255,13 @@ even if BootCurrent is present if *reorder_uefi_force_fallback* is True.
 
 This setting is ignored if *update_nvram* or *reorder_uefi* are False.
 
+**remove_duplicate_entries**: <*boolean: default True>*
+
+When curtin updates UEFI NVRAM it will remove duplicate entries that are
+present in the UEFI menu.  If you do not wish for curtin to remove duplicate
+entries setting *remove_duplicate_entries* to False.
+
+This setting is ignored if *update_nvram* is False.
 
 **Example**::
 
@@ -264,6 +271,7 @@ This setting is ignored if *update_nvram* or *reorder_uefi* are False.
      replace_linux_default: False
      update_nvram: True
      terminal: serial
+     remove_duplicate_entries: True
 
 **Default terminal value, GRUB_TERMINAL=console**::
 
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index ddae280..51df0c1 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -1035,11 +1035,9 @@ class TestUefiRemoveDuplicateEntries(CiTestCase):
 
     @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
     def test_uefi_remove_duplicate_entries(self):
-        cfg = {
-            'grub': {
-                'install_devices': ['/dev/vdb'],
-                'update_nvram': True,
-            },
+        grubcfg = {
+            'install_devices': ['/dev/vdb'],
+            'update_nvram': True,
         }
         self.m_efibootmgr.return_value = {
             'current': '0000',
@@ -1067,7 +1065,7 @@ class TestUefiRemoveDuplicateEntries(CiTestCase):
             }
         }
 
-        curthooks.uefi_remove_duplicate_entries(cfg, self.target)
+        curthooks.uefi_remove_duplicate_entries(grubcfg, self.target)
         self.assertEquals([
             call(['efibootmgr', '--bootnum=0001', '--delete-bootnum'],
                  target=self.target),
@@ -1076,12 +1074,86 @@ class TestUefiRemoveDuplicateEntries(CiTestCase):
             ], self.m_subp.call_args_list)
 
     @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
+    def test_uefi_remove_duplicate_entries_disabled(self):
+        grubcfg = {
+            'install_devices': ['/dev/vdb'],
+            'update_nvram': True,
+            'remove_duplicate_entries': False,
+        }
+        self.m_efibootmgr.return_value = {
+            'current': '0000',
+            'entries': {
+                '0000': {
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+                '0001': {
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+                '0002': {  # Is not a duplicate because of unique path
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(2,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+                '0003': {  # Is duplicate of 0000
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+            }
+        }
+
+        curthooks.uefi_remove_duplicate_entries(grubcfg, self.target)
+        self.assertEquals([], self.m_subp.call_args_list)
+
+    @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
+    def test_uefi_remove_duplicate_entries_skip_bootcurrent(self):
+        grubcfg = {
+            'install_devices': ['/dev/vdb'],
+            'update_nvram': True,
+        }
+        self.m_efibootmgr.return_value = {
+            'current': '0003',
+            'entries': {
+                '0000': {
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+                '0001': {
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+                '0002': {  # Is not a duplicate because of unique path
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(2,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+                '0003': {  # Is duplicate of 0000
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+            }
+        }
+
+        curthooks.uefi_remove_duplicate_entries(grubcfg, self.target)
+        self.assertEquals([
+            call(['efibootmgr', '--bootnum=0000', '--delete-bootnum'],
+                 target=self.target),
+            call(['efibootmgr', '--bootnum=0001', '--delete-bootnum'],
+                 target=self.target),
+            ], self.m_subp.call_args_list)
+
+    @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
     def test_uefi_remove_duplicate_entries_no_change(self):
-        cfg = {
-            'grub': {
-                'install_devices': ['/dev/vdb'],
-                'update_nvram': True,
-            },
+        grubcfg = {
+            'install_devices': ['/dev/vdb'],
+            'update_nvram': True,
         }
         self.m_efibootmgr.return_value = {
             'current': '0000',
@@ -1104,7 +1176,7 @@ class TestUefiRemoveDuplicateEntries(CiTestCase):
             }
         }
 
-        curthooks.uefi_remove_duplicate_entries(cfg, self.target)
+        curthooks.uefi_remove_duplicate_entries(grubcfg, self.target)
         self.assertEquals([], self.m_subp.call_args_list)