curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

Michael Hudson-Doyle has proposed merging ~mwhudson/curtin:rprivate-more into curtin:master.

Commit message:
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.


Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/406418

a tweak to my recent change.
-- 
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:rprivate-more into curtin:master.

diff --git a/curtin/util.py b/curtin/util.py
index d8f9c47..166f549 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -519,9 +519,29 @@ 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.
     # return boolean indicating if mountpoint was previously mounted.
-    mp = os.path.abspath(mountpoint)
+    if recursive:
+        # 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', mp])
     ret = False
     for line in reversed(load_file("/proc/mounts", decode=True).splitlines()):
         curmp = line.split()[1]
@@ -695,25 +715,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_commands_install.py b/tests/unittests/test_commands_install.py
index 9a64a79..bd73c09 100644
--- a/tests/unittests/test_commands_install.py
+++ b/tests/unittests/test_commands_install.py
@@ -115,6 +115,7 @@ class TestCmdInstall(CiTestCase):
             'curtin.commands.install.copy_install_log', 'm_copy_log')
         self.add_patch(
             'curtin.commands.install.tempfile.mkdtemp', 'm_mkdtemp')
+        self.add_patch('curtin.util.do_umount', 'm_umount')
         self.m_mkdtemp.return_value = working_dir
         with self.assertRaises(ValueError) as context_manager:
             install.cmd_install(myargs)
@@ -162,6 +163,7 @@ class TestCmdInstall(CiTestCase):
             'curtin.commands.install.copy_install_log', 'm_copy_log')
         self.add_patch(
             'curtin.commands.install.tempfile.mkdtemp', 'm_mkdtemp')
+        self.add_patch('curtin.util.do_umount', 'm_umount')
         self.m_mkdtemp.return_value = working_dir
         with self.assertRaises(ValueError) as context_manager:
             install.cmd_install(myargs)
diff --git a/tests/unittests/test_pack.py b/tests/unittests/test_pack.py
index cb0b135..974e838 100644
--- a/tests/unittests/test_pack.py
+++ b/tests/unittests/test_pack.py
@@ -70,6 +70,7 @@ class TestPack(TestCase):
             cfg_file = tempfile.mktemp(dir=self.tmpd)
             mcfg['install'] = cfg.get('install', {})
             mcfg['install']['log_file'] = log_file
+            mcfg['install']['unmount'] = 'disabled'
             mcfg['sources'] = {'testsrc': src_url}
             util.write_file(cfg_file, json.dumps(mcfg))
             print(json.dumps(mcfg))
@@ -87,7 +88,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 = {