Merge ~raharper/curtin:fix/uefi-remove-duplicate-entries into curtin:master

[Ryan Harper]

uefi: refactor efibootmg handling to support removing duplicate entries

When curtin is reusing an existing ESP, it may already contain a previous
ubuntu entry. efibootmgr complains about duplicate labels, but does not
provide a mechanism to remove the duplicate automatically. In curtin
we use our efibootmgr output parser to find duplicates and remove them.

To enable this we need to ensure that 'efivarfs' is mounted inside the
target. In ChrootableTarget we will add the additional mount automatically
if we're running on a uefi system.

This branch also refactors use of ChrootableTarget context manager to
minimize the number of chroot calls (which include 4 mounts and unmounts).

LP: #1864257

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

PASSED: Continuous integration, rev:14b7e262c94eeecd1391355bd1f7ecafe688fc6c
https://jenkins.ubuntu.com/server/job/curtin-ci/3846/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3846/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3846/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3846/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3846/

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

review: Approve (continuous-integration)

5a3485b... by Ryan Harper on 2020-02-26

Add unittests and walk entries via sorted key list

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

PASSED: Continuous integration, rev:44089e7e15d5908a714a89cc007623c3bf97dc12
https://jenkins.ubuntu.com/server/job/curtin-ci/3849/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3849/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3849/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3849/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3849/

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Minor unit test suggestion and +1. Thanks for this. I approve once that patch is landed.

review: Approve

77bcff3... by Ryan Harper on 2020-03-04

Add non-duplicate entry with different path to verify we compare both path and name in entries before removing

[Chad Smith (chad.smith)]

LGTM! awaiting CI

review: Approve

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

PASSED: Continuous integration, rev:77bcff3fc6d5e1959000d3fa552acf5007d8b2f3
https://jenkins.ubuntu.com/server/job/curtin-ci/3856/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3856/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3856/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3856/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3856/

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

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

This branch switches to invoking efibootmgr from curtin's environment vs invoking the one in /target. Is that intended? It breaks subiquity installs on UEFI currently :(

diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index fd48a5b..3c5d38f 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -443,26 +443,43 @@ def uefi_reorder_loaders(grubcfg, target):
     front of the BootOrder.
     """
     if grubcfg.get('reorder_uefi', True):
-        efi_output = util.get_efibootmgr(target)
-        currently_booted = efi_output.get('current', None)
-        boot_order = efi_output.get('order', [])
-        if currently_booted:
-            if currently_booted in boot_order:
-                boot_order.remove(currently_booted)
-            boot_order = [currently_booted] + boot_order
-            new_boot_order = ','.join(boot_order)
-            LOG.debug(
-                "Setting currently booted %s as the first "
-                "UEFI loader.", currently_booted)
-            LOG.debug(
-                "New UEFI boot order: %s", new_boot_order)
-            with util.ChrootableTarget(target) as in_chroot:
-                in_chroot.subp(['efibootmgr', '-o', new_boot_order])
+        with util.ChrootableTarget(target):
+            efi_output = util.get_efibootmgr(target=target)
+            currently_booted = efi_output.get('current', None)
+            boot_order = efi_output.get('order', [])
+            if currently_booted:
+                if currently_booted in boot_order:
+                    boot_order.remove(currently_booted)
+                boot_order = [currently_booted] + boot_order
+                new_boot_order = ','.join(boot_order)
+                LOG.debug(
+                    "Setting currently booted %s as the first "
+                    "UEFI loader.", currently_booted)
+                LOG.debug(
+                    "New UEFI boot order: %s", new_boot_order)
+                util.subp(['efibootmgr', '-o', new_boot_order])
     else:
         LOG.debug("Skipped reordering of UEFI boot methods.")
         LOG.debug("Currently booted UEFI loader might no longer boot.")
 
 
+def uefi_remove_duplicate_entries(grubcfg, target):
+    seen = set()
+    with util.ChrootableTarget(target):
+        efi_output = util.get_efibootmgr(target=target)
+        entries = efi_output.get('entries', {})
+        for bootnum in sorted(entries):
+            entry = entries[bootnum]
+            t = tuple(entry.items())
+            if t not in seen:
+                seen.add(t)
+            else:
+                LOG.debug('Removing duplicate EFI entry (%s, %s)',
+                          bootnum, entry)
+                util.subp(['efibootmgr', '--bootnum=%s' % bootnum,
+                           '--delete-bootnum'])
+
+
 def setup_grub(cfg, target, osfamily=DISTROS.debian):
     # target is the path to the mounted filesystem
 
@@ -583,7 +600,8 @@ def setup_grub(cfg, target, osfamily=DISTROS.debian):
     else:
         instdevs = ["none"]
 
-    if util.is_uefi_bootable() and grubcfg.get('update_nvram', True):
+    uefi_bootable = util.is_uefi_bootable()
+    if uefi_bootable and grubcfg.get('update_nvram', True):
         uefi_remove_old_loaders(grubcfg, target)
 
     LOG.debug("installing grub to %s [replace_default=%s]",
@@ -591,7 +609,7 @@ def setup_grub(cfg, target, osfamily=DISTROS.debian):
 
     with util.ChrootableTarget(target):
         args = ['install-grub']
-        if util.is_uefi_bootable():
+        if uefi_bootable:
             args.append("--uefi")
             LOG.debug("grubcfg: %s", grubcfg)
             if grubcfg.get('update_nvram', True):
@@ -609,7 +627,8 @@ def setup_grub(cfg, target, osfamily=DISTROS.debian):
             join_stdout_err + args + instdevs, env=env, capture=True)
         LOG.debug("%s\n%s\n", args + instdevs, out)
 
-    if util.is_uefi_bootable() and grubcfg.get('update_nvram', True):
+    if uefi_bootable and grubcfg.get('update_nvram', True):
+        uefi_remove_duplicate_entries(grubcfg, target)
         uefi_reorder_loaders(grubcfg, target)
 
 
diff --git a/curtin/util.py b/curtin/util.py
index eb2228f..fa4f3f3 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -633,6 +633,8 @@ class ChrootableTarget(object):
             self.mounts = mounts
         else:
             self.mounts = ["/dev", "/proc", "/run", "/sys"]
+            if is_uefi_bootable():
+                self.mounts.append('/sys/firmware/efi/efivars')
         self.umounts = []
         self.disabled_daemons = False
         self.allow_daemons = allow_daemons
@@ -856,7 +858,7 @@ def parse_efibootmgr(content):
     return output
 
 
-def get_efibootmgr(target):
+def get_efibootmgr(target=None):
     """Return mapping of EFI information.
 
     Calls `efibootmgr` inside the `target`.
@@ -879,7 +881,7 @@ def get_efibootmgr(target):
             }
         }
     """
-    with ChrootableTarget(target) as in_chroot:
+    with ChrootableTarget(target=target) as in_chroot:
         stdout, _ = in_chroot.subp(['efibootmgr', '-v'], capture=True)
         output = parse_efibootmgr(stdout)
         return output
diff --git a/examples/tests/uefi_reuse_esp.yaml b/examples/tests/uefi_reuse_esp.yaml
index c53064c..37a30d3 100644
--- a/examples/tests/uefi_reuse_esp.yaml
+++ b/examples/tests/uefi_reuse_esp.yaml
@@ -1,4 +1,6 @@
 showtrace: true
+install:
+  unmount: disabled
 
 bucket:
   - &setup |
diff --git a/helpers/common b/helpers/common
index 1def1bc..8230577 100644
--- a/helpers/common
+++ b/helpers/common
@@ -970,8 +970,6 @@ install_grub() {
                         --disk $efi_disk --part $efi_part_num --loader $loader
                     rc=$?
                     [ "$rc" != "0" ] && { exit $rc; }
-                    # check and remove duplicates
-                    efibootmgr --verbose --remove-dups
                 else
                     echo "skip EFI entry creation due to \"$no_nvram\" flag"
                 fi
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index ff38240..72bb24e 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -195,7 +195,9 @@ class TestUpdateInitramfs(CiTestCase):
         super(TestUpdateInitramfs, self).setUp()
         self.add_patch('curtin.util.subp', 'mock_subp')
         self.add_patch('curtin.util.which', 'mock_which')
+        self.add_patch('curtin.util.is_uefi_bootable', 'mock_uefi')
         self.mock_which.return_value = self.random_string()
+        self.mock_uefi.return_value = False
         self.target = self.tmp_dir()
         self.boot = os.path.join(self.target, 'boot')
         os.makedirs(self.boot)
@@ -787,12 +789,93 @@ class TestSetupGrub(CiTestCase):
                 },
             }
         }
-        self.in_chroot_subp_output.append(('', ''))
+        self.subp_output.append(('', ''))
         self.mock_haspkg.return_value = False
         curthooks.setup_grub(cfg, self.target, osfamily=self.distro_family)
         self.assertEquals(
             (['efibootmgr', '-o', '0001,0000'],),
-            self.mock_in_chroot_subp.call_args_list[0][0])
+            self.mock_subp.call_args_list[1][0])
+
+
+class TestUefiRemoveDuplicateEntries(CiTestCase):
+
+    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')
+
+    @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)'),
+                },
+            }
+        }
+
+        curthooks.uefi_remove_duplicate_entries(cfg, self.target)
+        self.assertEquals([
+            call(['efibootmgr', '--bootnum=0001', '--delete-bootnum']),
+            call(['efibootmgr', '--bootnum=0003', '--delete-bootnum'])],
+            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,
+            },
+        }
+        self.m_efibootmgr.return_value = {
+            'current': '0000',
+            'entries': {
+                '0000': {
+                    'name': 'ubuntu',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\ubuntu\\shimx64.efi)'),
+                },
+                '0001': {
+                    'name': 'centos',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\centos\\shimx64.efi)'),
+                },
+                '0002': {
+                    'name': 'sles',
+                    'path': (
+                        'HD(1,GPT)/File(\\EFI\\sles\\shimx64.efi)'),
+                },
+            }
+        }
+
+        curthooks.uefi_remove_duplicate_entries(cfg, self.target)
+        self.assertEquals([], self.m_subp.call_args_list)
 
 
 class TestUbuntuCoreHooks(CiTestCase):
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 80bb85f..438f7ee 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -585,12 +585,23 @@ class TestRunInChroot(CiTestCase):
 class TestChrootableTargetMounts(CiTestCase):
     """Test ChrootableTargets mounts dirs"""
 
+    @mock.patch('curtin.util.is_uefi_bootable')
     @mock.patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
-    def test_chrootable_target_default_mounts(self):
+    def test_chrootable_target_default_mounts(self, m_uefi):
+        m_uefi.return_value = False
         in_chroot = util.ChrootableTarget("mytarget")
         default_mounts = ['/dev', '/proc', '/run', '/sys']
         self.assertEqual(sorted(default_mounts), sorted(in_chroot.mounts))
 
+    @mock.patch('curtin.util.is_uefi_bootable')
+    @mock.patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
+    def test_chrootable_target_default_mounts_uefi(self, m_uefi):
+        m_uefi.return_value = True
+        in_chroot = util.ChrootableTarget("mytarget")
+        default_mounts = ['/dev', '/proc', '/run', '/sys',
+                          '/sys/firmware/efi/efivars']
+        self.assertEqual(sorted(default_mounts), sorted(in_chroot.mounts))
+
     @mock.patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
     def test_chrootable_target_custom_mounts(self):
         my_mounts = ['/foo', '/bar', '/wark']