Looks like this test is flaky (or broken) on Windows;

20:51:16 ----------------------------------------------------------------------
20:51:16 FAIL: docker_cli_run_test.go:4381: DockerSuite.TestSlowStdinClosing
20:51:16 
20:51:16 docker_cli_run_test.go:4397:
20:51:16     c.Fatal("running container timed out") // cleanup in teardown
20:51:16 ... Error: running container timed out
20:51:16 

Looks like this can be simplified to something like:

-	return daemon.cleanupMountsByID("")
+	if err := daemon.cleanupMountsByID(""); err != nil {
+		return err
+	}
+
+	logrus.WithField("mountpoint", daemon.root).Debug("unmounting daemon root")
+	return mount.Unmount(daemon.root)

Reason1: keep it simple
Reason2: if there's no mount, there's nothing to unmount and the "not mounted" error is safely ignored by mount.Unmount (which by the way also parses /proc/self/mountinfo but I want to get rid of it, see #36091).

Or, in case you only want to unmount daemon.root if it was mounted by the daemon itself (and, say, not by a sysadmin before starting docker), instead it makes sense to

• introduce a boolean flag (like daemonRootMounted)
• set it upon mounting
• check in cleanupMounts()

Otherwise there are two different checks in two different places, and they might diverge resulting in a breakage.

@kolyshkin Figuring out if the mount was from docker or not is not so simple as we could have an unclean shutdown at some point.
A simple unmount also doesn't work since this could be a volume (for instance)... and do something crazy which I totally didn't do (ok... maybe I did), like wreck the CI 😟

Figuring out if the mount was from docker or not is not so simple as we could have an unclean shutdown at some point

@cpuguy83 Right.

The current way, i.e. check that daemon.root is a mount point with a shared: option is not a reliable way to figure out whether daemon.root was mounted by dockerd or not (or I just fail to see how can it be).

Maybe make a function, like needRemountDaemonRoot(), and use it from both places, i.e. when mounting and unmounting daemon.root? This will solve the diverge issue I mentioned earlier.

@kolyshkin This isn't just checking if the mountpoint is shared, it's checking if it is a mountpoint at all and if the root of the mountpoint is itself.

I can't really see a way to use the same function for both mount and unmount reliably. The one thing we could do is persist a value somewhere so we know that that we at least mounted it at some point, but then we still need to check everything to make sure it should be unmount.

A simple mount.Unmount without a check does not work, and even breaks CI.

ping @kolyshkin what's the status on this one?

LGTM

Flaky test? Not sure I've seen this one before; https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/8485/console

17:23:14 --- FAIL: TestUpdateCPUQUota (1.78s)
17:23:14 	update_linux_test.go:136: expected cgroup value 20000, got: Error: Exec command f923baf709525f6b38f6511126addc5d9bb88fb477eeca1c22440551090fa2bb is already running

While testing this dockerd still seems to leak the root mountpoint (from #36096) for me after it has been shut down.

@cpuguy83 can you restart the z test? It's hard to tell what happened from that output, and could be a flaky case.

@nishanttotla There's a stack trace in the daemon logs for the stuck daemon.

ping @tonistiigi @kolyshkin this is green now; changes still LGTY?

still LGTM

I'm not sure this is a good way to detect if mount was created by dockerd. I think it is safer to either remember that mount happened or use a mount label.

2 issues:

Running dockerd -s overlay2 --data-root /foo/bar in dind (no special mountpoint for /foo/bar) errors because overlay-on-overlay is not supported. After daemon has errored there is a new leaked mountpoint /foo/bar.

In dind, mount --make-private /var/lib/docker && dockerd -D -s overlay2 --data-root /var/lib/docker/docker2. After killing daemon mount /var/lib/docker/docker2 leaks.

^ @thaJeztah @cpuguy83

@tonistiigi The first issue is a simple change, though there are other things that could happen to wind up in that state.
I'll have to look at why the 2nd case is leaking.

When you say add a label, I'm not sure we can just add random labels to the bind mount?
Tracking a file is not ideal as we'd still have to do all these checks. The (non-)existence of the file would just be one more reason not to unmount, but the existence of it does not mean it's ok to unmount.

When you say add a label, I'm not sure we can just add random labels to the bind mount?

Ah, yes, I didn't think of that.

Tracking a file is not ideal as we'd still have to do all these checks. The (non-)existence of the file would just be one more reason not to unmount, but the existence of it does not mean it's ok to unmount.

What do you mean by tracking a file here? Creating a file on mounting to detect it later? Why shouldn't it be safe to unmount if the detected file was only created by docker on mounting?

We'd still have to make sure the mount was actually safe to remove since file itself may be out of sync with the system.

@cpuguy83 How? if someone removes internal files from /var/lib/docker things are ok to break. And nobody can unmount the root while the daemon is running. On crash, next restart should correct the file if needed. Also, unmount happens only if a file exists so even in these contrived cases worse thing that can happen is leaking a mount, not unmounting something that we didn't mount.

@tonistiigi

1. start docker
2. kill docker (file still exists), or crash, or hard machine reset, whatever.
3. make a new mount to /var/lib/docker which is shared
4. start/stop docker

After step 4 if we only relied on the file existing we would unmount the user mount...
I suppose if on startup we don't need to perform the mount we could just remove the file.

Or not because if it's still our mount from before the crash we'd want to clean it up.

Or not because if it's still our mount from before the crash we'd want to clean it up.

I wonder if we have to take very scenario in mind here: what would be the "bad" thing of the mount being left behind after a crash? Restarting the daemon will reuse it, restarting the host will unmount it, correct?

@thaJeztah The case here is that dockerd crashes, user manually unmounts, now for some reason user mounts manually, dockerd is started. In that case on stop dockerd would umount something manually mounted by user that is quite bad. It is a very unlikely scenario though.

@tonistiigi oh, perhaps I was unclear: the "unmount" scenario can be covered by writing a file (we check that file on shutdown, and unmount if it's there, and remove the file on startup if it exists, and there's a mount).

So, the "crash" situation would then only result in us not un-mounting, which would in most situations be no problem (given that we need that mount, and likely start the daemon again after a crash).

@thaJeztah Ok, yes, if we don't attempt to resync the file after a crash then there is no extra unmount. But it means that we will always leak a mount if there is a crash. If we attempt to resync then we don't leak a mount 99.9% of the cases, even on crash, but there is a tiny possibility of false unmount.

Yes, so my train of though is:

daemon crashes, which is an exceptional condition. Either something is very wrong, and manual intervention is needed, or the daemon is started again, and everything recovers.

During start, an existing mount is found:

• we find an existing mount, and file
  • we remove the file
  • we log a message/warning that we'll be reusing an existing mount
• we find an existing mount, no file
  • we log a message/warning that we'll be reusing an existing mount

If the host is rebooted (which could be part of recovering after a crash): situation is reset to "pristine"

I think a false unmount is a worse scenario than a mount leak.
In the case of a false unmount, the daemon would not be able to restore the mount.
In the case of a leaked mount, the only issue I can see if getting an EBUSY when trying to rm -r the daemon root dir.

So, looks like the “file” approach would give us that?