Skip to content

overlay[2] graphdriver: Fix/improve overlayfs support check for rootless#40194

Merged
AkihiroSuda merged 2 commits intomoby:masterfrom
kolyshkin:rootless-overlay
Nov 15, 2019
Merged

overlay[2] graphdriver: Fix/improve overlayfs support check for rootless#40194
AkihiroSuda merged 2 commits intomoby:masterfrom
kolyshkin:rootless-overlay

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

Inspired by #40131

Overlay check is performed by looking for overlay in /proc/filesystem. This obviously might not work for rootless Docker (fs is there, but one can't use it as non-root, for example, see docker-library/docker#193).

This PR changes the check to perform the actual mount, by reusing the code previously written to check for multiple lower dirs support. The old check is removed from both drivers, as well as the additional check for the multiple lower dirs support in overlay2 since it's now a part of the main check.

The PR is split into two commits for the sake of easier review.

  • First commit moves the supportsMultipleLowerDir to overlayutils with minimal modifications
  • Second commit renames it to SupportsOverlay(), makes the multiple lower dir check optional, and makes both overlay and overlay2 use the new check.

PS nice LOC reduction:

 daemon/graphdriver/overlay/overlay.go           | 37 +++++--------------------------------
 daemon/graphdriver/overlay2/check.go            | 32 --------------------------------
 daemon/graphdriver/overlay2/overlay.go          | 47 +++++------------------------------------------
 daemon/graphdriver/overlayutils/overlayutils.go | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 106 deletions(-)

fixes docker/for-linux#836

@kolyshkin kolyshkin requested a review from dmcgowan as a code owner November 7, 2019 23:31
@kolyshkin
Copy link
Copy Markdown
Contributor Author

@Caligatio could you test this?

@thaJeztah @dmcgowan PTAL

This moves supportsMultipleLowerDir() to overlayutils
so it can be used from both overlay and overlay2.

The only changes made were:
 * replace logger with logrus
 * don't use workDirName mergedDirName constants
 * add mnt var to improve readability a bit

This is a preparation for the next commit.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Before this commit, overlay check was performed by looking for
`overlay` in /proc/filesystem. This obviously might not work
for rootless Docker (fs is there, but one can't use it as non-root).

This commit changes the check to perform the actual mount, by reusing
the code previously written to check for multiple lower dirs support.

The old check is removed from both drivers, as well as the additional
check for the multiple lower dirs support in overlay2 since it's now
a part of the main check.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Copy Markdown
Contributor Author

fixed bad import statement (my system still had old github.com/Sirupsen/logrus)

@Caligatio
Copy link
Copy Markdown

I'm unfortunately away from a Docker-friendly computer for the weekend. I'll check it out on Monday and report back.

@Caligatio
Copy link
Copy Markdown

This works like a champ on CentOS 7.7 choosing between vfs and overlay2. Similar to my own original patch, I have no idea how to test overlay vs overlay2 detection.

Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda AkihiroSuda merged commit 649e4c8 into moby:master Nov 15, 2019
@AkihiroSuda
Copy link
Copy Markdown
Member

merged via #40210

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rootless Docker fails at detecting root-requiring overlay support

4 participants