Looks good, ping @rhvgoyal

LGTM.

We already do this with the help of sysctl interface. So over a boot, all the sysctl settings are applied and this is already enabled by the time docker runs.

Wonderful! I was just going to work on a similar PR, but for the devmapper graph driver only (as its "deferred deletion" feature only works in case fs.may_detach_mounts=1). I did not realize it might be helpful in non-devmapper case, too. This patch will also help in case of autodetection whether deferred deletion is working or not (see mbentley/docker-devicemapper-setup#4).

The alternative approach is to add a /usr/lib/sysctl.d/90-docker.conf file to docker rpm spec, containing fs.may_detach_mounts=1, and adding a kludge to the .spec so that the setting is also applied upon rpm installation. This would be cleaner (and this is what Red Hat's runc rpm does) -- the only downside is anyone who's not installing from an rpm would not get it.

Yet another alternative is something like:

// -q: not display the value set, -e: ignore non-existent key
err := exec.Command("sysctl", "-q", "-e", "fs.may_detach_mounts=1").Run()
...

While exec sn ugly in general, this is only done once upon daemon (re)start so it's OK.

Yet another alternative is something like:

I do like the alternative approach (cleaner); would the permissions denied error still be properly returned with that code?

I do like the alternative approach (cleaner); would the permissions denied error
still be properly returned with that code?

Well, what we'll get is an exit code (most probably a generic 1) and a text from stderr (which we can parse for "permission denied" but it's a slippery slope as the locale might change that). I suggest treating any error, not just EPERM, in the same way, like:

s := "fs.may_detach_mounts=1"
// -q: not display the value set, -e: ignore non-existent key
err := exec.Command("sysctl", "-q", "-e", s).Run()
// ignore the error if the daemon is inside userns
if err != nil && !rsystem.RunningInUserNS() {
	logrus.Warnf("Error setting %s: %v", s, err)
}

Or maybe even not try setting this at all if we're in userns:

// do not try to set sysctls from inside userns
if !rsystem.RunningInUserNS() {
	s := "fs.may_detach_mounts=1"
	// -q: not display the value set, -e: ignore non-existent key
	if err := exec.Command("sysctl", "-q", "-e", s).Run(); err != nil {
		logrus.Warnf("Error setting %s sysctl: %v", s, err)
}

(If @cpuguy83 agrees) feel free to open a PR with that change

Why use exec here?

I'd be fine with setting this in the systemd conf, however that's really a packaging issue which we don't really deal with in this repo (though there are some init scripts in contrib).

Why use exec here?

As I said earlier, While exec sn ugly in general, this is only done once upon daemon (re)start so it's OK.

The benefits I see are:

• using more standard interface
• less code