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.

Sign up for GitHub

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

Jump to bottom

Run stat command directly for volume ownership test #2236

Merged
merged 2 commits into from
Mar 19, 2020
Merged

Run stat command directly for volume ownership test #2236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10e59b5
Run stat command directly for volume ownership test
carbonin Mar 19, 2020
85fe5cc
Add comment for RUN command in volume ownership test
carbonin Mar 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tests/bud.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Understood. Thanks for having looked into it.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

@carbonin, I can confirm: mountpoint -n, -d, blkid, stat -c %i, none of those (or others I've forgotten) produce any useful distinction.

expect_output "testuser testgroup"
}

@test "bud-from-glob" {
Expand Down
5 changes: 4 additions & 1 deletion tests/bud/volume-ownership/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
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
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

VOLUME /vol/subvol

# Run some command after VOLUME to ensure that the volume cache behavior is invoked
# See https://github.com/containers/buildah/blob/843d15de3e797bd912607d27324d13a9d5c27dfb/imagebuildah/stage_executor.go#L61-L72 and
# for more details
RUN touch /test
6 changes: 0 additions & 6 deletions tests/bud/volume-ownership/test-ownership
View file
Open in desktop

This file was deleted.