Merge ~mwhudson/curtin:rprivate-more into curtin:master

[Michael Hudson-Doyle]

move making mounts recursively private into do_unmount

This means that running "curtin unmount -t /target" gets the robustness
improvments that were recently added to ChrootableTarget.

a tweak to my recent change.

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

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

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

review: Needs Fixing (continuous-integration)

d9b3a59... by Michael Hudson-Doyle on 2021-07-30

adapt to deal with the fact that do_umount is called with things that are not mountpoints

[Dan Bungert (dbungert)]

review: Approve

18bbbd0... by Michael Hudson-Doyle on 2021-07-30

update comment

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

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

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

review: Approve (continuous-integration)

diff --git a/curtin/util.py b/curtin/util.py
index d8f9c47..ec1d4f9 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -519,13 +519,41 @@ def do_mount(src, target, opts=None):
 
 
 def do_umount(mountpoint, recursive=False):
+    mp = os.path.abspath(mountpoint)
     # unmount mountpoint. if recursive, unmount all mounts under it.
+    #
+    # note that recursive unmounting is different to umount -R's
+    # behaviour. Consider the following sequence:
+    #
+    # mkdir a a/b c
+    # mount --bind a c
+    # mount -t sysfs sysfs a/b
+    #
+    # "umount c" will fail, with "mountpoint is busy", because of the
+    # mountpoint at c/b. But "umount -R c" will unmount a/b (and as in
+    # pratice "a/b" is often actually "/sys" itself, this would be
+    # Bad(tm)). So what we do is iterate over the mountpoints under
+    # `mountpoint` and mark each one as "private" before unmounting
+    # it, which means the unmount does not propagate to any mount tree
+    # it was cloned from (See "Shared subtree operations" in mount(8)
+    # for more on this).
+    #
+    # You might think we could replace all this with a call to "mount
+    # --make-rprivate" followed by a call to "umount --recursive" but
+    # that would fail in the case where `mountpoint` is not actually a
+    # mount point, and not doing that is actually relied on by other
+    # parts of curtin.
+    #
+    # Related bug reports:
+    # https://bugs.launchpad.net/maas/+bug/1928839
+    # https://bugs.launchpad.net/subiquity/+bug/1934775
+    #
     # return boolean indicating if mountpoint was previously mounted.
-    mp = os.path.abspath(mountpoint)
     ret = False
     for line in reversed(load_file("/proc/mounts", decode=True).splitlines()):
         curmp = line.split()[1]
         if curmp == mp or (recursive and curmp.startswith(mp + os.path.sep)):
+            subp(['mount', '--make-private', curmp])
             subp(['umount', curmp])
         if curmp == mp:
             ret = True
@@ -695,25 +723,6 @@ class ChrootableTarget(object):
             log_call(subp, ['udevadm', 'settle'])
 
         for p in reversed(self.umounts):
-            # 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")
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index 1e65175..f878d9f 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([('', '')] * 15)
+        self.mock_subp.side_effect = iter([('', '')] * 10)
         curthooks.disable_update_initramfs({}, self.target)
         self.assertIn(
             call(['dpkg-divert', '--add', '--rename', '--divert',
@@ -215,25 +215,18 @@ 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', '')] +
-                   [('mount', '')]*len(self.mounts))
+                   [(cmd_out, cmd_err)] + [('settle', '')])
         return effects
 
     def _subp_calls(self, mycall):
         pre = [self._mnt_call(point) for point in self.mounts]
-        post = [call(['udevadm', 'settle'])] + [
-            self._rpriv_call(point) for point in reversed(self.mounts)]
+        post = [call(['udevadm', 'settle'])]
         return pre + [mycall] + post
 
     def test_does_nothing_if_binary_diverted(self):
@@ -250,7 +243,7 @@ class TestUpdateInitramfs(CiTestCase):
                      target=self.target)
         calls = self._subp_calls(dcall)
         self.mock_subp.assert_has_calls(calls)
-        self.assertEqual(10, self.mock_subp.call_count)
+        self.assertEqual(6, self.mock_subp.call_count)
 
     def test_mounts_and_runs(self):
         # in_chroot calls to dpkg-divert, update-initramfs
@@ -263,7 +256,7 @@ class TestUpdateInitramfs(CiTestCase):
             call(['update-initramfs', '-c', '-k', self.kversion],
                  target=self.target))
         self.mock_subp.assert_has_calls(subp_calls)
-        self.assertEqual(20, self.mock_subp.call_count)
+        self.assertEqual(12, self.mock_subp.call_count)
 
     def test_mounts_and_runs_for_all_kernels(self):
         kversion2 = '5.4.0-generic'
@@ -287,7 +280,7 @@ class TestUpdateInitramfs(CiTestCase):
             call(['update-initramfs', '-c', '-k', kversion2],
                  target=self.target))
         self.mock_subp.assert_has_calls(subp_calls)
-        self.assertEqual(40, self.mock_subp.call_count)
+        self.assertEqual(24, self.mock_subp.call_count)
 
     def test_calls_update_if_initrd_exists_else_create(self):
         kversion2 = '5.2.0-generic'
@@ -297,7 +290,7 @@ class TestUpdateInitramfs(CiTestCase):
         with open(os.path.join(self.boot, 'initrd.img-' + kversion2), 'w'):
             pass
 
-        effects = self._side_eff() * 4
+        effects = self._side_eff() * 3
         self.mock_subp.side_effect = iter(effects)
         curthooks.update_initramfs(self.target, True)
         subp_calls = self._subp_calls(
@@ -309,7 +302,7 @@ class TestUpdateInitramfs(CiTestCase):
             call(['update-initramfs', '-c', '-k', self.kversion],
                  target=self.target))
         self.mock_subp.assert_has_calls(subp_calls)
-        self.assertEqual(30, self.mock_subp.call_count)
+        self.assertEqual(18, self.mock_subp.call_count)
 
 
 class TestSetupKernelImgConf(CiTestCase):
diff --git a/tests/unittests/test_pack.py b/tests/unittests/test_pack.py
index cb0b135..803d7de 100644
--- a/tests/unittests/test_pack.py
+++ b/tests/unittests/test_pack.py
@@ -87,7 +87,7 @@ class TestPack(TestCase):
 
         return out, err, rc, log_contents
 
-    def test_psuedo_install(self):
+    def test_pseudo_install(self):
         # do a install that has only a early stage and only one command.
         mystr = "HI MOM"
         cfg = {