New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip empty directories on prior graphdriver detection #35528

Merged
merged 1 commit into from Nov 28, 2017

Conversation

Projects
None yet
5 participants
@thaJeztah
Member

thaJeztah commented Nov 16, 2017

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
Skip empty directories on prior graphdriver detection
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

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Nov 21, 2017

ping @kolyshkin @dnephin PTAL

return true
}
return false
}

This comment has been minimized.

@dnephin

dnephin Nov 21, 2017

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

?

@dnephin

dnephin Nov 21, 2017

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

?

This comment has been minimized.

@thaJeztah

thaJeztah Nov 21, 2017

Member

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)

@thaJeztah

thaJeztah Nov 21, 2017

Member

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

if _, err = f.Readdirnames(1); err == io.EOF {
return true
}
return false

This comment has been minimized.

@dnephin

dnephin Nov 21, 2017

Member

nit: could be

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

dnephin Nov 21, 2017

Member

nit: could be

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

This comment has been minimized.

@thaJeztah

thaJeztah Nov 21, 2017

Member

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

@thaJeztah

thaJeztah Nov 21, 2017

Member

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

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 21, 2017

Contributor

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 errorsif _, 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.

Contributor

kolyshkin commented Nov 21, 2017

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 errorsif _, 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

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 21, 2017

Contributor

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

Contributor

kolyshkin commented Nov 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 21, 2017

Member

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 😅

Member

thaJeztah commented Nov 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 21, 2017

Contributor

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.

Contributor

kolyshkin commented Nov 21, 2017

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

vieux approved these changes Nov 28, 2017

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

LGTM

@vieux vieux merged commit 9ae6971 into moby:master Nov 28, 2017

5 of 6 checks passed

z Jenkins build Docker-PRs-s390x 6877 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37948 has succeeded
Details
janky Jenkins build Docker-PRs 46662 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7072 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18219 has succeeded
Details

@thaJeztah thaJeztah deleted the thaJeztah:ignore-empty-graphdirs branch Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment