Merge ~raharper/curtin:fix/initramfs-only-once-x86-only into curtin:master

[Ryan Harper]

curthooks: handle s390x/aarch64 kernel install hooks

On s390x, and aarch64, the kernel package directly calls
arch specific commands (zipl, flash-kernel) which fail to run
correctly if an initramfs isn't already created in the image. This
broke installation when we attempt to only create the initramfs
once. This branch adds arch support into the code path to divert
not only 'update-initramfs' but the arch specific tools as well and
restores them prior to running the real 'update-initramfs' command.

LP: #1856038

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

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

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Looks good. A few questions, nits and concerns.

[Chad Smith (chad.smith)]

review: Needs Fixing

[Michael Hudson-Doyle (mwhudson)]

I agree with Chad's comments I think. I wonder how come this doesn't fail for livefs builds?

Do vmtests not catch this because they are installing images that already have kernels in?

9f069ce... by Ryan Harper on 2019-12-16

Use common method for enumerating tools used, add docstrings.

[Ryan Harper (raharper)]

> I agree with Chad's comments I think. I wonder how come this doesn't fail for
> livefs builds?

I don't know.

>
> Do vmtests not catch this because they are installing images that already have
> kernels in?

vmtests catch this; It was landed because the autolander which runs some vmtests before merging doesn't run on s390x/arm64.

We'll file an issue for that

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

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

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

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

LGTM. Glad there are vmtests that catch this, we should make sure they are regularly run as the archive changes in case the set of used tools changes.

review: Approve

[Chad Smith (chad.smith)]

This works for me. Thanks for consolidating the common logic on this Ryan. Let's do the dpkg --print-architecture later so we can make sure we are only matching one set of 'platform' strings throughout and retain consistency throughout the code base.

review: Approve

[Chad Smith (chad.smith)]

Thanks for the back and forth here.

review: Approve

diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index 4b36da2..b5baa06 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -104,48 +104,66 @@ def disable_overlayroot(cfg, target):
         shutil.move(local_conf, local_conf + ".old")
 
 
-def disable_update_initramfs(cfg, target):
-    """ Find 'update-initramfs' in target and if found, change the name. """
-    update_initramfs = util.which('update-initramfs', target=target)
-    if update_initramfs:
-        LOG.debug('Diverting original update-initramfs in target.')
-        rename = update_initramfs + '.curtin-disabled'
-        divert = ['dpkg-divert', '--add', '--rename', '--divert', rename,
-                  update_initramfs]
-        with util.ChrootableTarget(target) as in_chroot:
-            in_chroot.subp(divert)
+def _update_initramfs_tools(machine=None):
+    """ Return a list of binary names used to update an initramfs.
 
-        # create a dummy update-initramfs which just returns true;
-        # this handles postinstall scripts which make invoke update-initramfs
-        # directly
-        util.write_file(target + update_initramfs,
-                        content="#!/bin/true\n# diverted by curtin",
-                        mode=0o755)
+    On some architectures there are helper binaries that are also
+    used and will be included in the list.
+    """
+    tools = ['update-initramfs']
+    if not machine:
+        machine = platform.machine()
+    if machine == 's390x':
+        tools.append('zipl')
+    elif machine == 'aarch64':
+        tools.append('flash-kernel')
+    return tools
+
+
+def disable_update_initramfs(cfg, target, machine=None):
+    """ Find update-initramfs tools in target and change their name. """
+    with util.ChrootableTarget(target) as in_chroot:
+        for tool in _update_initramfs_tools(machine=machine):
+            found = util.which(tool, target=target)
+            if found:
+                LOG.debug('Diverting original %s in target.', tool)
+                rename = found + '.curtin-disabled'
+                divert = ['dpkg-divert', '--add', '--rename',
+                          '--divert', rename, found]
+                in_chroot.subp(divert)
+
+                # create a dummy update-initramfs which just returns true;
+                # this handles postinstall scripts which make invoke $tool
+                # directly
+                util.write_file(target + found,
+                                content="#!/bin/true\n# diverted by curtin",
+                                mode=0o755)
 
 
 def update_initramfs_is_disabled(target):
-    # check if update-initramfs has been diverted
+    """ Return a bool indicating if initramfs tooling is disabled. """
     disabled = []
     with util.ChrootableTarget(target) as in_chroot:
         out, _err = in_chroot.subp(['dpkg-divert', '--list'], capture=True)
         disabled = [divert for divert in out.splitlines()
-                    if divert.endswith('update-initramfs.curtin-disabled')]
+                    if divert.endswith('.curtin-disabled')]
     return len(disabled) > 0
 
 
-def enable_update_initramfs(cfg, target):
-    """ Find 'update-initramfs.curtin-disabled ' in target and if found
-        restore to original name. """
+def enable_update_initramfs(cfg, target, machine=None):
+    """ Enable initramfs update tools by restoring their original name. """
     if update_initramfs_is_disabled(target):
         with util.ChrootableTarget(target) as in_chroot:
-            LOG.info(
-                'Restoring update-initramfs in target for initrd updates.')
-            update_initramfs = util.which('update-initramfs', target=target)
-            # remove the diverted
-            util.del_file(target + update_initramfs)
-            # un-divert and restore original file
-            in_chroot.subp(
-                ['dpkg-divert', '--rename', '--remove', update_initramfs])
+            for tool in _update_initramfs_tools(machine=machine):
+                LOG.info('Restoring %s in target for initrd updates.', tool)
+                found = util.which(tool, target=target)
+                if not found:
+                    continue
+                # remove the diverted
+                util.del_file(target + found)
+                # un-divert and restore original file
+                in_chroot.subp(
+                    ['dpkg-divert', '--rename', '--remove', found])
 
 
 def setup_zipl(cfg, target):
@@ -1373,6 +1391,7 @@ def builtin_curthooks(cfg, target, state):
     LOG.info('Running curtin builtin curthooks')
     stack_prefix = state.get('report_stack_prefix', '')
     state_etcd = os.path.split(state['fstab'])[0]
+    machine = platform.machine()
 
     distro_info = distro.get_distroinfo(target=target)
     if not distro_info:
@@ -1387,7 +1406,7 @@ def builtin_curthooks(cfg, target, state):
                 description="configuring apt configuring apt"):
             do_apt_config(cfg, target)
             disable_overlayroot(cfg, target)
-            disable_update_initramfs(cfg, target)
+            disable_update_initramfs(cfg, target, machine)
 
         # LP: #1742560 prevent zfs-dkms from being installed (Xenial)
         if distro.lsb_release(target=target)['codename'] == 'xenial':
@@ -1521,7 +1540,7 @@ def builtin_curthooks(cfg, target, state):
             description="updating initramfs configuration"):
         if osfamily == DISTROS.debian:
             # re-enable update_initramfs
-            enable_update_initramfs(cfg, target)
+            enable_update_initramfs(cfg, target, machine)
             update_initramfs(target, all_kernels=True)
         elif osfamily == DISTROS.redhat:
             redhat_update_initramfs(target, cfg)
@@ -1530,7 +1549,6 @@ def builtin_curthooks(cfg, target, state):
     # day, but for now, assume no. They do require the initramfs
     # to be updated, and this also triggers boot loader setup via
     # flash-kernel.
-    machine = platform.machine()
     if (machine.startswith('armv7') or
             machine.startswith('s390x') or
             machine.startswith('aarch64') and not util.is_uefi_bootable()):
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index a1e13de..3dde2ee 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -90,8 +90,12 @@ class TestEnableDisableUpdateInitramfs(CiTestCase):
         ccc = 'curtin.commands.curthooks'
         self.add_patch(ccc + '.util.subp', 'mock_subp')
         self.add_patch(ccc + '.util.which', 'mock_which')
+        self.add_patch(ccc + '.platform.machine', 'mock_machine')
         self.target = self.tmp_dir()
+        self.mock_machine.return_value = 'x86_64'
         self.update_initramfs = '/usr/sbin/update-initramfs'
+        self.flash_kernel = '/usr/sbin/flash-kernel'
+        self.zipl = '/sbin/zipl'
 
     def test_disable_does_nothing_if_no_binary(self):
         self.mock_which.return_value = None
@@ -154,6 +158,37 @@ class TestEnableDisableUpdateInitramfs(CiTestCase):
         curthooks.enable_update_initramfs({}, self.target)
         self.assertEqual(0, self.mock_which.call_count)
 
+    def _test_disable_on_machine(self, machine, tools):
+        self.mock_machine.return_value = machine
+        self.mock_which.side_effect = iter(tools)
+        self.mock_subp.side_effect = iter([('', '')] * 10 * len(tools))
+        curthooks.disable_update_initramfs({}, self.target, machine=machine)
+        for tool in tools:
+            tname = os.path.basename(tool)
+            self.assertIn(
+                call(['dpkg-divert', '--add', '--rename', '--divert',
+                      tool + '.curtin-disabled', tool], target=self.target),
+                self.mock_subp.call_args_list)
+            lhs = [call(tname, target=self.target)]
+            self.assertIn(lhs, self.mock_which.call_args_list)
+
+            # make sure we have a dummy binary
+            target_tool = self.target + tool
+            self.assertTrue(os.path.exists(target_tool))
+            self.assertTrue(util.is_exe(target_tool))
+            expected_content = "#!/bin/true\n# diverted by curtin"
+            self.assertEqual(expected_content, util.load_file(target_tool))
+
+    def test_disable_on_s390x_masks_zipl(self):
+        machine = 's390x'
+        tools = [self.update_initramfs, self.zipl]
+        self._test_disable_on_machine(machine, tools)
+
+    def test_disable_on_arm_masks_flash_kernel(self):
+        machine = 'aarch64'
+        tools = [self.update_initramfs, self.flash_kernel]
+        self._test_disable_on_machine(machine, tools)
+
 
 class TestUpdateInitramfs(CiTestCase):
     def setUp(self):