Merge ~raharper/curtin:fix/uefi-remove-dups-v2 into curtin:master

[Ryan Harper]

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

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:cd3de0f570b6e8a26d1bbef2f0378e90f48c8f73
https://jenkins.ubuntu.com/server/job/curtin-ci/177/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/177/
    ABORTED: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/177/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/177/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/177/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/177//rebuild

review: Needs Fixing (continuous-integration)

[Paride Legovini (paride)]

CI failed because the latest versions of pyrsistent are not compatible with py27. Full CI run log at [1]. Can be worked around by pinning the version of pyrsistent:

https://code.launchpad.net/~paride/curtin/+git/curtin/+merge/390446

On the MP itself: do you think it's worth adding a "missing BootCurrent" test case?

[1] https://paste.ubuntu.com/p/fgGwqC6ktv/

review: Needs Information

[Dan Watkins (oddbloke)]

In https://bugs.launchpad.net/maas/+bug/1894217/comments/3, it looks like there are two issues: (a) we're incorrectly detecting duplicates, and (b) we might remove the BootCurrent value. This branch definitely addresses (b); does it also intend to address (a)?

review: Needs Information

[Ryan Harper (raharper)]

@Dan

I thought we were incorrectly detecting dups, but in-fact, they are
duplicates. I asked for followup data from the submitter and confirmed there
are duplicates.

See comment 6:

Boot0000* EFI Network 1 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/MAC(ecf4bbce7b8c,1)/IPv4(0.0.0.00.0.0.0,0,0)
Boot0004* EFI Network 1 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/MAC(ecf4bbce7b8c,1)/IPv4(0.0.0.00.0.0.0,0,0)
Boot0007* EFI Network 1 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/MAC(ecf4bbce7b8c,1)/IPv4(0.0.0.00.0.0.0,0,0)

Boot000C* EFI Network 2 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x1)/MAC(ecf4bbce7b8d,1)/IPv4(0.0.0.00.0.0.0,0,0)
Boot0008* EFI Network 2 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x1)/MAC(ecf4bbce7b8d,1)/IPv4(0.0.0.00.0.0.0,0,0)

Boot0002 EFI Network 3 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x2)/MAC(ecf4bbce7b8e,1)/IPv4(0.0.0.00.0.0.0,0,0)
Boot0009* EFI Network 3 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x2)/MAC(ecf4bbce7b8e,1)/IPv4(0.0.0.00.0.0.0,0,0)
Boot000D* EFI Network 3 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x2)/MAC(ecf4bbce7b8e,1)/IPv4(0.0.0.00.0.0.0,0,0)

Boot0003 EFI Network 4 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x3)/MAC(ecf4bbce7b8f,1)/IPv4(0.0.0.00.0.0.0,0,0)
Boot000B* EFI Network 4 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x3)/MAC(ecf4bbce7b8f,1)/IPv4(0.0.0.00.0.0.0,0,0)
Boot000E* EFI Network 4 PcieRoot(0x0)/Pci(0x1,0x0)/Pci(0x0,0x3)/MAC(ecf4bbce7b8f,1)/IPv4(0.0.0.00.0.0.0,0,0)

So it turns out there was not an issue with duplicate detection, only removing BootCurrent.

[Paride Legovini (paride)]

CI should pass now if the MP gets rebased.

800cd78... by Ryan Harper on 2020-09-09

Refactor remove-dups unittests per feedback

d28422d... by Ryan Harper on 2020-09-09

Add unittest for missing BootCurrent value when removing duplicates

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:d28422dc0bbf0e9066cbf682db1d9be519bc38cb
https://jenkins.ubuntu.com/server/job/curtin-ci/179/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/179/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/179/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/179/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/179/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/179//rebuild

review: Approve (continuous-integration)

[Paride Legovini (paride)]

LGTM, thanks Ryan! I'm not setting the MP to Approved yet in case Dan wants to have another look.

review: Approve

[Dan Watkins (oddbloke)]

Yep, LGTM, thanks!

review: Approve

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..e5fead3 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -1027,47 +1027,57 @@ class TestSetupGrub(CiTestCase):
 
 class TestUefiRemoveDuplicateEntries(CiTestCase):
 
+    efibootmgr_output = {
+        'current': '0000',
+        'entries': {
+            '0000': {
+                'name': 'ubuntu',
+                'path': (
+                    'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+            },
+            '0001': {  # Is duplicate of 0000
+                '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)'),
+            },
+        }
+    }
+
     def setUp(self):
         super(TestUefiRemoveDuplicateEntries, self).setUp()
         self.target = self.tmp_dir()
         self.add_patch('curtin.util.get_efibootmgr', 'm_efibootmgr')
         self.add_patch('curtin.util.subp', 'm_subp')
+        self.m_efibootmgr.return_value = copy.deepcopy(self.efibootmgr_output)
 
     @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,
-            },
-        }
-        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)'),
-                },
-            }
-        }
+        grubcfg = {}
+        curthooks.uefi_remove_duplicate_entries(grubcfg, self.target)
+        self.assertEquals([
+            call(['efibootmgr', '--bootnum=0001', '--delete-bootnum'],
+                 target=self.target),
+            call(['efibootmgr', '--bootnum=0003', '--delete-bootnum'],
+                 target=self.target)
+            ], self.m_subp.call_args_list)
 
-        curthooks.uefi_remove_duplicate_entries(cfg, self.target)
+    @patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
+    def test_uefi_remove_duplicate_entries_no_bootcurrent(self):
+        grubcfg = {}
+        efiout = copy.deepcopy(self.efibootmgr_output)
+        del efiout['current']
+        self.m_efibootmgr.return_value = efiout
+        curthooks.uefi_remove_duplicate_entries(grubcfg, self.target)
         self.assertEquals([
             call(['efibootmgr', '--bootnum=0001', '--delete-bootnum'],
                  target=self.target),
@@ -1076,13 +1086,30 @@ class TestUefiRemoveDuplicateEntries(CiTestCase):
             ], 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,
-            },
+    def test_uefi_remove_duplicate_entries_disabled(self):
+        grubcfg = {
+            'remove_duplicate_entries': False,
         }
+        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 = {}
+        efiout = copy.deepcopy(self.efibootmgr_output)
+        efiout['current'] = '0003'
+        self.m_efibootmgr.return_value = efiout
+        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):
+        grubcfg = {}
         self.m_efibootmgr.return_value = {
             'current': '0000',
             'entries': {
@@ -1103,8 +1130,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)