Merge ~mwhudson/curtin:even-more-unmounting-tweaks into curtin:master

[Michael Hudson-Doyle]

tweak making mountpoints private some more

My recent change to move the making of mountpoints private into
do_umount actually made it ineffective. I tried to change things
so that we always made mountpoints private before unmounting them
but that ran into problems of its own. So I changed do_umount to
have a "private" flag ChrootableTarget.__exit__ could use to
request that mountpoints be made private before unmounting, which
makes things work again but I still don't see how to make a
generally robust "curtin unmount" command, as explained at length
in the comment I added.

this time for sure etc

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

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

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

review: Needs Fixing (continuous-integration)

d9e9119... by Michael Hudson-Doyle on 2021-08-04

do not read /proc/mounts twice

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

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

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

review: Needs Fixing (continuous-integration)

bed091e... by Michael Hudson-Doyle on 2021-08-06

updates

686ef9e... by Michael Hudson-Doyle on 2021-08-06

extend essay even more

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

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

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

review: Approve (continuous-integration)

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

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

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

review: Approve (continuous-integration)

fbe90ad... by Michael Hudson-Doyle on 2021-08-06

grammmmeeer

916c338... by Michael Hudson-Doyle on 2021-08-06

more wordsmithing

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

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

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

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

LGTM

review: Approve

diff --git a/curtin/util.py b/curtin/util.py
index ec1d4f9..5b66b55 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -518,31 +518,69 @@ def do_mount(src, target, opts=None):
     return True
 
 
-def do_umount(mountpoint, recursive=False):
+def do_umount(mountpoint, recursive=False, private=False):
     mp = os.path.abspath(mountpoint)
-    # unmount mountpoint. if recursive, unmount all mounts under it.
+    # unmount mountpoint.
     #
-    # note that recursive unmounting is different to umount -R's
-    # behaviour. Consider the following sequence:
+    # if recursive, unmount all mounts under it.  if private (which
+    # implies recursive), mark all mountpoints private before
+    # unmounting.
+    #
+    # To explain the 'private' parameter, 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).
+    # "umount c" will fail with "mountpoint is busy" because the mount
+    # of a/b "propagates" into the subtree at c, i.e. creates a mount
+    # at "c/b". But if you run "umount -R c" the unmount of c/b will
+    # propagate to back to a and unmount a/b (and as in pratice "a/b"
+    # can be something like driver specific mounts in /sys, this would
+    # be Bad(tm)). So what we do here is iterate over the mountpoints
+    # under `mountpoint` and mark them as "private" doing any
+    # unmounting, which means the unmounts do not propagate to any
+    # mount tree they were cloned from.
+    #
+    # So why not do this always? Well! Several systemd services (like
+    # udevd) run in private mount namespaces, set up so that mount
+    # operations on the default namespace propagate into the service's
+    # namespace (but not the other way). This means if you do this:
+    #
+    # mount /dev/sda2 /tmp/my-mount
+    # mount --make-private /tmp/my-mount
+    # umount /tmp/my-mount
+    #
+    # then the mount operation propagates into udevd's mount namespace
+    # but the unmount operation does not and so /dev/sda2 remains
+    # mounted, which causes problems down the road.
+    #
+    # It would be nice to not have to have the caller care about all
+    # this detail. In particular imagine the target system is set up
+    # at /target and has host directories bind-mounted into it, so the
+    # mount tree looks something like this:
+    #
+    # /dev/vda2 is mounted at /target
+    # /dev/vda1 is mounted at /target/boot
+    # /sys is bind-mounted at /target/sys
+    #
+    # And for whatever reason, a mount has appeared at /sys/foo since
+    # this was setup, so there is now an additional mount at
+    # /target/sys/foo.
+    #
+    # What I would like is to be able to run something like "curtin
+    # unmount /target" and have curtin figure out that the mountpoint
+    # at /target/sys/foo should be made private before unmounting and
+    # the others should not. But I don't know how to do that.
+    #
+    # See "Shared subtree operations" in mount(8) for more on all
+    # 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.
+    # You might also 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
@@ -550,10 +588,16 @@ def do_umount(mountpoint, recursive=False):
     #
     # return boolean indicating if mountpoint was previously mounted.
     ret = False
-    for line in reversed(load_file("/proc/mounts", decode=True).splitlines()):
-        curmp = line.split()[1]
+    mountpoints = [
+        line.split()[1]
+        for line in load_file("/proc/mounts", decode=True).splitlines()]
+    if private:
+        recursive = True
+        for curmp in mountpoints:
+            if curmp == mp or curmp.startswith(mp + os.path.sep):
+                subp(['mount', '--make-private', curmp])
+    for curmp in reversed(mountpoints):
         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
@@ -723,7 +767,7 @@ class ChrootableTarget(object):
             log_call(subp, ['udevadm', 'settle'])
 
         for p in reversed(self.umounts):
-            do_umount(p, recursive=True)
+            do_umount(p, private=True)
 
         rconf = paths.target_path(self.target, "/etc/resolv.conf")
         if self.sys_resolvconf and self.rconf_d: