Skip empty directories on prior graphdriver detection

[thaJeztah]

When starting the daemon, the /var/lib/docker directory is scanned for existing directories, so that the previously selected graphdriver will automatically be used.

In some situations, empty directories are present (those directories can be created during feature detection of graph-drivers), in which case the daemon refuses to start.

This patch improves detection, and skips empty directories, so that leftover directories don't cause the daemon to fail.

Before this change:

$ mkdir /var/lib/docker /var/lib/docker/aufs /var/lib/docker/overlay2
$ dockerd
...
Error starting daemon: error initializing graphdriver: /var/lib/docker contains several valid graphdrivers: overlay2, aufs; Please cleanup or explicitly choose storage driver (-s <DRIVER>)

With this patch applied:

$ mkdir /var/lib/docker /var/lib/docker/aufs /var/lib/docker/overlay2
$ dockerd
...
INFO[2017-11-16T17:26:43.207739140Z] Docker daemon                                 commit=ab90bc296 graphdriver(s)=overlay2 version=dev
INFO[2017-11-16T17:26:43.208033095Z] Daemon has completed initialization

And on restart (prior graphdriver is still picked up):

$ dockerd
...
INFO[2017-11-16T17:27:52.260361465Z] [graphdriver] using prior storage driver: overlay2

[thaJeztah]

ping @kolyshkin @dnephin PTAL

[dnephin]

info, err := ioutil.ReadDir(name)
return err == nil && len(info) == 0

?

[thaJeztah]

ioutil.ReadDir(name) would do a stat on all files/directories inside name? While this is only on daemon start, I can imagine it could be an issue if a lot of directories are present inside (having #31462 in mind)

[dnephin]

LGTM

[dnephin]

nit: could be

_, err = f.Readdirnames(1)
return err == io.EOF

[thaJeztah]

Thanks for reviewing: I had this initially, but thought it would be more difficult to "grasp", so changed to a simple if

[kolyshkin]

While reviewing this, I have an alternative idea, let me tell it, maybe it makes sense. Something like this:

 	for driver := range drivers {
  		p := filepath.Join(root, driver)
+  		unix.Rmdir(p) // remove if directory is empty. ignore any errors
  		if _, err := os.Stat(p); err == nil && driver != "vfs" {

This way has two benefits:

1. this is very simple way to detect if a directory is (well, was -- but who cares) empty
2. in the meantime we do some minor "garbage collection"

We can actually make it even simple by removing the os.Stat() call and relying on return of Rmdir -- ENOEMPTY (or EEXIST in some UNIX versions) would mean the directory exists and is not empty. But maybe it hurts readability.

[kolyshkin]

Here are the possible errors from unix.Rmdir and ways to handle these:

code	explanation	action
0(no error) ENOENT	empty directory was removed no such directory	skip it silently
ENOTEMPTY EEXIST	directory exists and is not empty	try to use this graphdriver
EBUSY	opened files under this directory, or directory is a mount point	try to use this graphdriver
ENOTDIR	either not a directory, or a symlink to a directory	a separate stat() is required to distinguish between the cases
any other error	unexpected (should not happen)	give a warning and skip it

v2: updated with EBUSY
v3: updated with ENOTDIR (which ruins the approach of using Rmdir w/o Stat)

So, ultimately, we should use os.Stat() nevertheless

[thaJeztah]

Heh, I like the creative approach. The rm proposal would make sense in a "hot" path perhaps, but I'm not sure we should delete directories in a detection loop 😅

[kolyshkin]

but I'm not sure we should delete directories in a detection loop

Why not? Those are empty directories that were (most probably) created by dockerd itself are within a directory tree that is owned and controlled by dockerd.

The only case I see we should not remove a directory like this is when such a directory is to be used as a mount point, but for some reason it is not mounted. If we remove it, the subsequent mount will fail. From the other case, if dockerd is started but such directory is not mounted it is an error.

OK I guess yet another reason to not do it is when such a directory is in fact a symlink. Nope, we are fine in this case, Rmdir will return "not a directory" -- which should be ignored. One more reason to NOT rely on return value from Rmdir but leave a separate Stat() call in place as I originally suggested.

[vieux]

not a big fan of (most probably) I prefer @thaJeztah approach

LGTM