Skip to content
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

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

Merged
merged 2 commits into from Nov 15, 2019

Conversation

@kolyshkin
Copy link
Contributor

kolyshkin commented Nov 7, 2019

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 Nov 7, 2019
@kolyshkin

This comment has been minimized.

Copy link
Contributor Author

kolyshkin commented Nov 7, 2019

@Caligatio could you test this?

@thaJeztah @dmcgowan PTAL

kolyshkin added 2 commits Nov 7, 2019
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 kolyshkin force-pushed the kolyshkin:rootless-overlay branch from 8c3a366 to 649e4c8 Nov 8, 2019
@kolyshkin

This comment has been minimized.

Copy link
Contributor Author

kolyshkin commented Nov 8, 2019

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

@Caligatio

This comment has been minimized.

Copy link

Caligatio commented Nov 9, 2019

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

@Caligatio

This comment has been minimized.

Copy link

Caligatio commented Nov 11, 2019

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
Member

AkihiroSuda left a comment

@AkihiroSuda AkihiroSuda merged commit 649e4c8 into moby:master Nov 15, 2019
2 checks passed
2 checks passed
DCO DCO
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@AkihiroSuda

This comment has been minimized.

Copy link
Member

AkihiroSuda commented Nov 15, 2019

merged via #40210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.