server, runtimeVM: Don't mount /sys and /sys/fs/cgroup using "rslave" #4896
Conversation
Let's only mount "/sys" and "/sys/fs/cgroup" as "rslave" in the case of "OCI" containers, as cri-o#4575 broke privileged containers when used with kata-containers. Fixes: cri-o#4765 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
openshift-ci
bot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fidencio 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 |
openshift-ci
bot
added
the
approved
| // A container is kernel separated if we're using shimv2, or we're using a kata v1 binary | ||
| podIsKernelSeparated := runtimeType == libconfig.RuntimeTypeVM || | ||
| strings.Contains(strings.ToLower(runtimeHandler), "kata") || | ||
| (runtimeHandler == "" && strings.Contains(strings.ToLower(s.config.DefaultRuntime), "kata")) |
There was a problem hiding this comment.
can we add a method to the runtime class s.Runtime().IsKernelSeparated() instead of duplicating this check between here and sandbox_run_linux?
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
We may go for a solution on the kata-containers side, which seems for future proof, as the change on CRI-O basically exposed an issue on the kata-containers side. Let's hold it for now. |
|
@fidencio: The following tests failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
|
I think the solution on the kata-containers is more future proof than making a special case on the CRI-O side. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Let's only mount "/sys" and "/sys/fs/cgroup" as "rslave" in the case of
"OCI" containers, as #4575 broke
privileged containers when used with kata-containers.
Which issue(s) this PR fixes:
Fixes #4765
Special notes for your reviewer:
I've discussed this with @giuseppe earlier Today and he mentioned that he's fine with this approach.
I really would like to get a feedback from both @mrunalp and @lifupan to be sure whether the right layer to solve this issue is here, on CRI-O, or whether a fix for this would be better on Kata Containers side (and @fupanli is debugging it there).
Does this PR introduce a user-facing change?