Always chown volumes when mounting into a container #22727
Conversation
openshift-ci
bot
added
release-note
approved
|
Still needs a test |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
btw this branch seems very out of date so you must rebase |
|
Logic majorly updated to reflect further observations of Docker. Tests added. |
mheon
force-pushed
the
chown_all_the_time
branch
10 times, most recently
from
09e291e
to
d12c015
Compare
|
Each of the 3 tests I added is taking ~30s to run, which is suboptimal. Maybe I should push the image to Quay and cache it? |
How big is the final image? If the image is the same for all three tests than pushing to quay.io/libpod and adding in the image cache will almost certainly help speed compared to having it to build three times |
If you do decide to push the image in quay then please make sure to add the Containefile + build instructions here in the repo somewhere under test, e.g. like |
|
The final image shouldn't be much bigger than the |
test/e2e/run_volume_test.go
Outdated
| check := podmanTest.Podman([]string{"run", "-v", fmt.Sprintf("%s:%s", volName, volPath), imgName, "ls", "-al", volPath}) | ||
| check.WaitWithDefaultTimeout() | ||
| Expect(check).Should(ExitCleanly()) | ||
| Expect(check.OutputToString()).To(MatchRegexp(`^drwxr-xr-x\.\s+[0-9]+\s+%s\s+%s\s*[0-9]\s+.+\s+\.$`, expectedOwner, expectedOwner)) | ||
| // Should match lines of this format, but with arbitrary username. Permissions always fixed, directory always . | ||
| // drwxr-xr-x. 1 mheon mheon 0 May 22 09:49 . |
There was a problem hiding this comment.
why use ls -al when the output is hard to match? either use ls -n to only get the ids or better use stat -c %u:%g
stat should be part of the regual alpine based image
test/e2e/run_volume_test.go
Outdated
| RUN chown test1:test1 /test1 | ||
| RUN chown test2:test2 /test2 | ||
| RUN chown test3:test3 /test4 | ||
| RUN chmod 755 /test1 /test2 /test3 /test4`, fedoraMinimal) |
There was a problem hiding this comment.
is there a reason for fedora-minimal? The image is still much bigger than alpine based so it slows the build down.
There was a problem hiding this comment.
Presence of useradd, vs adduser, useradd being a lot easier to use with numeric groups. I can refactor to use alpine though.
mheon
force-pushed
the
chown_all_the_time
branch
3 times, most recently
from
ccc623b
to
b6c41c8
Compare
|
LGTM |
When an empty volume is mounted into a container, Docker will chown that volume appropriately for use in the container. Podman does this as well, but there are differences in the details. In Podman, a chown is presently a one-and-done deal; in Docker, it will continue so long as the volume remains empty. Mount into a dozen containers, but never add content, the chown occurs every time. The chown is also linked to copy-up; it will always occur when a copy-up occurred, despite the volume now not being empty. This PR changes our logic to (mostly) match Docker's. For some reason, the chowning also stops if the volume is chowned to root at any point. This feels like a Docker bug, but as they say, bug for bug compatible. In retrospect, using bools for NeedsChown and NeedsCopyUp was a mistake. Docker isn't actually tracking this stuff; they're just doing a copy-up and permissions change unconditionally as long as the volume is empty. They also have the two linked as one operation, seemingly, despite happening at very different times during container init. Replicating that in our stateful system is nontrivial, hence the need for the new CopiedUp field. Basically, we never want to chown a volume with contents in it, except if that data is a result of a copy-up that resulted from mounting into the current container. Tracking who did the copy-up is the easiest way to do this. Fixes containers#22571 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
|
Passing now |
|
/cherry-pick v5.1 |
|
@mheon: once the present PR merges, I will cherry-pick it on top of v5.1 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@mheon: new pull request created: #22790 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This is done to better match Docker's behavior. Our current behavior is to only chown a volume once, the first time it is mounted into a container; that doesn't match what Docker does. Docker chowns every time a container mounts a volume, to ensure it is always accessible to the last container to mount it.
Fortunately, this is a relatively easy fix. We have a bool in volume state as to whether a volume needs to be chowned, which we set to false when the volume was first chowned; so just stop doing that.
I was really hoping to eliminate the NeedsChown bool entirely, but things are a bit of a mess. There are some cases where we do want to completely inhibit chowning, like when the user explicitly requests that a volume be a specific UID and GID. Unfortunately, those are both ints, not *int, so we can't tell whether they were actually set, and the solution we were using was to force the NeedsChown bool to false - so we really need to keep it around. And even if we could change volume UID/GID to pointers, we're setting them in places we really shouldn't be - for example, container anonymous volumes set UID/GID to current container UID/GID, despite the fact that the volume will immediately chown itself from that to match the directory it's mounting over? In short, the current code's a mess and I don't see a way to fix it without breaking changes.
Fixes #22571
Does this PR introduce a user-facing change?