Merge ~mwhudson/curtin:lp-1928839 into curtin:master

[Michael Hudson-Doyle]

fix tearing down ChrootableTarget when mounts appear while it is set up

There are several bug reports that boil down to
ChrootableTarget.__exit__ failing to unmount bind mounts with "target is
busy". For example, ssh-ing in while running tends to do this, because
that creates a mount in /run and then umounting /target/run fails
because of the sub mount. Fix this by marking the mountpoints
recursively private and then unmounting them recursively.

LP: #1928839
LP: #1934775

Hopefully this fixes a problem that's always niggled at me.

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

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

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

review: Needs Fixing (continuous-integration)

af67b1f... by Michael Hudson-Doyle on 2021-07-14

fix tests

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

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

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

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

I had the reporter of https://bugs.launchpad.net/subiquity/+bug/1934775 test a subiquity branch with this change in it and it helped, fwiw.

[Dan Bungert (dbungert)]

review: Approve

[Ryan Harper (raharper)]

This is quite an excellent solution; I had been thinking about using mount namespaces for this but private is much simpler.

review: Approve

3fea636... by Michael Hudson-Doyle on 2021-07-15

add bug references to comment

[Michael Hudson-Doyle (mwhudson)]

Thanks for the reviews! Happy to have a fix for this.

diff --git a/curtin/util.py b/curtin/util.py
index be063d7..d8f9c47 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -695,7 +695,26 @@ class ChrootableTarget(object):
             log_call(subp, ['udevadm', 'settle'])
 
         for p in reversed(self.umounts):
-            do_umount(p)
+            # Consider the following sequence:
+            #
+            # mkdir a a/b c
+            # mount --bind a c
+            # mount -t sysfs sysfs a/b
+            # umount c
+            #
+            # This umount fails with "mountpoint is busy", because of
+            # the mountpoint at c/b. But if we unmount c recursively,
+            # a/b ends up getting unmounted. What we need to do is to
+            # make the mountpoints c and c/b "private" so that
+            # unmounting them does not propagate to the mount tree c
+            # was cloned from (See "Shared subtree operations" in
+            # mount(8) for more on this).
+            #
+            # Related bug reports:
+            # https://bugs.launchpad.net/maas/+bug/1928839
+            # https://bugs.launchpad.net/subiquity/+bug/1934775
+            subp(['mount', '--make-rprivate', p])
+            do_umount(p, recursive=True)
 
         rconf = paths.target_path(self.target, "/etc/resolv.conf")
         if self.sys_resolvconf and self.rconf_d:
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index 41670f7..ca0fc90 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -109,7 +109,7 @@ class TestEnableDisableUpdateInitramfs(CiTestCase):
 
     def test_disable_changes_binary_name_write_dummy_binary(self):
         self.mock_which.return_value = self.update_initramfs
-        self.mock_subp.side_effect = iter([('', '')] * 10)
+        self.mock_subp.side_effect = iter([('', '')] * 15)
         curthooks.disable_update_initramfs({}, self.target)
         self.assertIn(
             call(['dpkg-divert', '--add', '--rename', '--divert',
@@ -215,18 +215,25 @@ class TestUpdateInitramfs(CiTestCase):
         target = os.path.join(self.target, point)
         return call(['mount', '--bind', '/%s' % point, target])
 
+    def _rpriv_call(self, point):
+        target = os.path.join(self.target, point)
+        return call(['mount', '--make-rprivate', target])
+
     def _side_eff(self, cmd_out=None, cmd_err=None):
         if cmd_out is None:
             cmd_out = ''
         if cmd_err is None:
             cmd_err = ''
         effects = ([('mount', '')] * len(self.mounts) +
-                   [(cmd_out, cmd_err)] + [('settle', '')])
+                   [(cmd_out, cmd_err)] +
+                   [('settle', '')] +
+                   [('mount', '')]*len(self.mounts))
         return effects
 
     def _subp_calls(self, mycall):
         pre = [self._mnt_call(point) for point in self.mounts]
-        post = [call(['udevadm', 'settle'])]
+        post = [call(['udevadm', 'settle'])] + [
+            self._rpriv_call(point) for point in reversed(self.mounts)]
         return pre + [mycall] + post
 
     def test_does_nothing_if_binary_diverted(self):
@@ -243,7 +250,7 @@ class TestUpdateInitramfs(CiTestCase):
                      target=self.target)
         calls = self._subp_calls(dcall)
         self.mock_subp.assert_has_calls(calls)
-        self.assertEqual(6, self.mock_subp.call_count)
+        self.assertEqual(10, self.mock_subp.call_count)
 
     def test_mounts_and_runs(self):
         # in_chroot calls to dpkg-divert, update-initramfs
@@ -256,7 +263,7 @@ class TestUpdateInitramfs(CiTestCase):
             call(['update-initramfs', '-c', '-k', self.kversion],
                  target=self.target))
         self.mock_subp.assert_has_calls(subp_calls)
-        self.assertEqual(12, self.mock_subp.call_count)
+        self.assertEqual(20, self.mock_subp.call_count)
 
     def test_mounts_and_runs_for_all_kernels(self):
         kversion2 = '5.4.0-generic'
@@ -280,7 +287,7 @@ class TestUpdateInitramfs(CiTestCase):
             call(['update-initramfs', '-c', '-k', kversion2],
                  target=self.target))
         self.mock_subp.assert_has_calls(subp_calls)
-        self.assertEqual(24, self.mock_subp.call_count)
+        self.assertEqual(40, self.mock_subp.call_count)
 
     def test_calls_update_if_initrd_exists_else_create(self):
         kversion2 = '5.2.0-generic'
@@ -290,7 +297,7 @@ class TestUpdateInitramfs(CiTestCase):
         with open(os.path.join(self.boot, 'initrd.img-' + kversion2), 'w'):
             pass
 
-        effects = self._side_eff() * 3
+        effects = self._side_eff() * 4
         self.mock_subp.side_effect = iter(effects)
         curthooks.update_initramfs(self.target, True)
         subp_calls = self._subp_calls(
@@ -302,7 +309,7 @@ class TestUpdateInitramfs(CiTestCase):
             call(['update-initramfs', '-c', '-k', self.kversion],
                  target=self.target))
         self.mock_subp.assert_has_calls(subp_calls)
-        self.assertEqual(18, self.mock_subp.call_count)
+        self.assertEqual(30, self.mock_subp.call_count)
 
 
 class TestSetupKernelImgConf(CiTestCase):