Followup fixes to validator #742
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 5, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 7, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 7, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
changed the title
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 7, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
Commit fec9c3c added some logging to Clean. First, it forgot to add newlines, resulting in some garbage in the output. Fix by using fmt.Println. Second, it logged error from Kill which is very frequent, since the container might already be stopped. Fix by introducing and using ForceDelete (aka delete --force). Signed-off-by: Kir Kolyshkin <[email protected]>
It is always called with (true, true) so it doesn't make sense. If a caller would want to keep a bundle dir, they should use r.Delete or r.ForceDelete. Signed-off-by: Kir Kolyshkin <[email protected]>
This test expects CAP_SYS_ADMIN to be set (in order to perform mounts). Currently, if this capability is not set, it returns bare unix errno (EPERM) from unix.Mount, which is very confusing, since the test just prints "Operation not permitted" and exits. Do the following changes: - move the first mount to before the switch, and skip the test when it returns EPERM; - wrap all unix.Mount errors to provide more context. Signed-off-by: Kir Kolyshkin <[email protected]>
This code calls g.SetupPrivileged because validateRootfsPropagation needs to perform mounts. In fact, setting CAP_SYS_ADMIN and removing the generated seccomp profile (which does not enable mount/umount) is sufficient. Using the whole g.SetupPrivileged problematic when running tests inside Docker running on a machine with new Linux kernels. The problem is Docker does not (yet) know about recently added capabilities (specifically, CAP_PERFMON, CAP_BPF, and CAP_CHECKPOINT_RESTORE, brought in via commit 8e1a3b5), and thus runs a --privileged container without those. This code, though, tries to set them all, relying on the value of last capability to /proc/sys/kernel/cap_last_cap. This results in an error setting new capabilities, and thus the test errors out. While at it, make the test name more descriptive, adding the propagation that we're testing. NOTE the same problem exists in validation/process_capabilities, and needs to be addressed separately. Fixes: 8e1a3b5 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 7, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 7, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 7, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
Contributor
Author
|
No longer a draft; tests are passing: |
kolyshkin
changed the title
tianon
approved these changes
Jan 8, 2022
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 8, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/crun
that referenced
this pull request
Jan 8, 2022
Testing opencontainers/runtime-tools#742 Signed-off-by: Kir Kolyshkin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is another follow up to PRs #728 and #732, fixing a few issues introduced by them, with some related cleanups and fixes. Please see individual commits for more details.
In particular, this fixes rootfsPropagation test failure when running the test inside a Docker container), caused by 8e1a3b5 (which adds new capabilities to the list, and those caps are not yet known to Docker daemon used for CI).
Currently a draft which I am about to test in crun repo.