Run stat command directly for volume ownership test #2236
Conversation
This removes the script previously used which may have been causing containers#2235 Signed-off-by: Nick Carboni <ncarboni@redhat.com>
| @@ -1,5 +1,4 @@ | |||
| FROM alpine | |||
| ADD test-ownership /usr/bin/ | |||
| RUN adduser -D -H testuser && addgroup testgroup | |||
| RUN mkdir -p /vol/subvol | |||
| RUN chown testuser:testgroup /vol/subvol | |||
There was a problem hiding this comment.
Out of curiosity, what is the purpose of the touch /test two lines down? (not an addition in this PR; you'll need to click the expando). I'm assuming it's a copy-paste artifact, and unnecessary for purposes of this test. If so, as long as you're making changes here, could you perhaps remove it to prevent future maintainers from spending time wondering about it? (Or if it's real and necessary, perhaps document why?) Thanks!
There was a problem hiding this comment.
Actually it is necessary. The original bug only showed up when the volume cache was in play. And that only happens if there are additional RUN commands after the VOLUME command.
I chose that particular command because it obviously shouldn't have changed the permissions of the volume ... but it did.
I can add a commit with a comment in the Dockerfile to try to summarize the reasoning if you think it would be helpful.
There was a problem hiding this comment.
Given what you just said, I think a comment is not only helpful but critical: a future maintainer might have no idea of this context, and could easily refactor/optimize away the touch. It is imperative to explain the why, especially in non-obvious cases such as this one. Thanks for your quick explanation.
There was a problem hiding this comment.
Added a comment. Not sure what your feelings are about github links in source, but the comment above the Preserve function has a bunch of information about this.
| @@ -608,7 +608,8 @@ load helpers | |||
| run_buildah bud --signature-policy ${TESTSDIR}/policy.json -t ${target} ${TESTSDIR}/bud/volume-ownership | |||
| run_buildah from --quiet --signature-policy ${TESTSDIR}/policy.json ${target} | |||
| cid=$output | |||
| run_buildah run $cid test-ownership | |||
| run_buildah run $cid stat -c "%U %G" /vol/subvol | |||
There was a problem hiding this comment.
So, I'm a complete ignoramus about the VOLUME directive, and I could test this (and will, when I get some time this morning), but: how would this behave differently if the Containerfile did not specify VOLUME? Is there any way to check that /vol/subvol is actually being treated as a volume, and not just a regular subdirectory in the image?
There was a problem hiding this comment.
tldr; I wasn't able to find a good way.
Volumes are mount points in containers so I figured I could use mountpoint to test the directory, but ...
/ # mount
...
/dev/mapper/luks-5825177c-f5c0-47c1-9865-db72a9bd7528 on /vol/subvol type btrfs (rw,seclabel,nosuid,nodev,relatime,ssd,space_cache,subvolid=257,subvol=/home/ncarboni/.local/share/containers/storage/volumes/b2efdcb7017c80a10b244942da38e4620d3ec5af0de90a9fee020ea44118679c/_data)
...
/ # mountpoint /vol/subvol/
/vol/subvol/ is not a mountpoint😟
I can't seem to get consistent results from stat -c "%i or stat -f inside the container either. We could parse mount or df, but ... gross.
There was a problem hiding this comment.
Understood. Thanks for having looked into it.
There was a problem hiding this comment.
@edsantiago The VOLUME directive tells the container engines to allocate a UNNAMED volume on disk and mount it at the /vol/submount directory.
The problem was podman was creating the new volume with the wrong owner/permissions and then mounting over the /vol/submount. The directory went from ownership testuser testgroup to root root.
There was a problem hiding this comment.
@carbonin, I can confirm: mountpoint -n, -d, blkid, stat -c %i, none of those (or others I've forgotten) produce any useful distinction.
Signed-off-by: Nick Carboni <ncarboni@redhat.com>
|
LGTM. @carbonin thank you for your quick action on this. |
|
LGTM |
|
bors r+ |
2236: Run stat command directly for volume ownership test r=rhatdan a=carbonin #### What type of PR is this? /kind failing-test #### What this PR does / why we need it: This removes the script previously used which may have been causing #2235 cc @edsantiago @rhatdan Co-authored-by: Nick Carboni <ncarboni@redhat.com>
Build failed
|
|
bors retry |
Build succeeded
|
What type of PR is this?
/kind failing-test
What this PR does / why we need it:
This removes the script previously used which may have been
causing #2235
cc @edsantiago @rhatdan