Skip to content

Improve rootless Docker overlay support detection#40131

Closed
Caligatio wants to merge 3 commits intomoby:masterfrom
Caligatio:rootless-overlay
Closed

Improve rootless Docker overlay support detection#40131
Caligatio wants to merge 3 commits intomoby:masterfrom
Caligatio:rootless-overlay

Conversation

@Caligatio
Copy link
Copy Markdown

Signed-off-by: Brian Turek brian.turek@gmail.com

- What I did
Fixed overlay support detection for rootless Docker on CentOS (and other non-Ubuntu based distros). The current implementation causes runtime errors when running rootless Docker on CentOS due lack of mounting permissions. See docker/for-linux#836 and docker-library/docker#193 for related tickets

- How I did it
Current overlay detection works by merely checking for filesystem support for overlay rather than overlay support for that particular user (i.e. Ubuntu allows non-root users to use overlay, CentOS does not). This patch changes the logic to attempt a real overlay mount and determine overlay support by checking whether than mount was successful.

Note that this is literally the first time I've written Go so constructive criticism is appreciated.

- How to verify it

  • Run the new dockerd binary as rootless on CentOS, the storage-driver falls back to vfs
  • Run the new dockerd binary as root on CentOS, the storage-driver is overlay2
  • Run the new dockerd binary as rootless on Ubuntu, the storage-driver is overlay2
  • Run the new dockerd binary as root on Ubuntu, the storage-driver is overlay2

- Description for the changelog

Improve rootless Docker overlay support detection

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Brian Turek <brian.turek@gmail.com>
Signed-off-by: Brian Turek <brian.turek@gmail.com>
Signed-off-by: Brian Turek <brian.turek@gmail.com>
@andrewhsu
Copy link
Copy Markdown
Contributor

@tonistiigi fyi

@tonistiigi
Copy link
Copy Markdown
Member

Could we combine this logic into a shared function?

@Caligatio
Copy link
Copy Markdown
Author

For whatever my opinion is worth, the functions are basically the same but only by happenstance rather than design (i.e. they both use overlay mount type versus this being a generic "try to mount it function"). Also, I think there is value in each filesystem detection routine being separate.

Ultimately, it's up to you maintainer folk and I'll happily make the code work either way.

return graphdriver.ErrNotSupported

// Attempt to perform an overlay mount to determine whether the current user (and system) can support it
if err := unix.Mount("overlay", merged, "overlay", 0, fmt.Sprintf("lowerdir=%s,upperdir=%s,workdir=%s", lower, upper, work)); err != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

for overlay2, we should better test multi-lowerdir, but not a huge deal for recent kernels.

So LGTM.

@thaJeztah
Copy link
Copy Markdown
Member

For whatever my opinion is worth, the functions are basically the same but only by happenstance rather than design (i.e. they both use overlay mount type versus this being a generic "try to mount it function"). Also, I think there is value in each filesystem detection routine being separate.

Yes, I personally don't mind having it duplicated

@thaJeztah
Copy link
Copy Markdown
Member

@dmcgowan @kolyshkin PTAL

@kolyshkin
Copy link
Copy Markdown
Contributor

There is already code like this in overlay2, please see #35527

I suggest to reuse that code instead. We can add an option to try multiple lower dirs, move the code into overlayutils, and use it from both overlay and overlay2.

@kolyshkin
Copy link
Copy Markdown
Contributor

@Caligatio do you want to work on that ^^^^^?

@kolyshkin
Copy link
Copy Markdown
Contributor

There is already code like this in overlay2, please see #35527

I suggest to reuse that code instead. We can add an option to try multiple lower dirs, move the code into overlayutils, and use it from both overlay and overlay2.

Here it is: #40194

@AkihiroSuda
Copy link
Copy Markdown
Member

closable?

@kolyshkin
Copy link
Copy Markdown
Contributor

#40194 was merged, this one can be closed

@kolyshkin kolyshkin closed this Nov 26, 2019
@Caligatio
Copy link
Copy Markdown
Author

Yep, closing now.

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.

6 participants