-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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.
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
gVisor: initial support for gVisor container runtime #7661
Conversation
Hi @cristicalin. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
784b35d
to
8169002
Compare
a39a4c8
to
e1e1690
Compare
- name: Stop if gvisor_enabled is enabled when container_manager is not containerd | ||
assert: | ||
that: container_manager == 'containerd' | ||
msg: "gvisor_enabled support only compatible with containerd. See https://github.com/kubernetes-sigs/kubespray/issues/7650 for details" | ||
when: gvisor_enabled | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc mentions cri-o too. Which one is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the docs. As per @ianlewis' comment, cri-o is not currently usable though it does support setting the RuntimeClass
; because of this I restricted the validation to containerd
. Once gVisor adds support for cri-o, we can change this validation and the code to support it is already in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cristicalin for this PR!
/ok-to-test |
@@ -293,6 +293,12 @@ | |||
msg: "kata_containers_enabled support only for containerd and crio-o. See https://github.com/kata-containers/documentation/blob/1.11.4/how-to/run-kata-with-k8s.md#install-a-cri-implementation for details" | |||
when: kata_containers_enabled | |||
|
|||
- name: Stop if gvisor_enabled is enabled when container_manager is not containerd | |||
assert: | |||
that: container_manager == 'containerd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional lines of roles/container-engine/meta/main.yml
- role: container-engine/gvisor
when:
- gvisor_enabled
- container_manager in ['docker', 'containerd']
seem docker
also can be container_manager.
I guess I am missing some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oomichi see the discussion on #7650, initially I wanted to support all 3 of our runtimes but it turns out only containerd
is well supported by gVisor today. docker
is actually not able to select the RuntimeClass
and cri-o
support is stil not mature enough, though I'm actually hoping that might evolve in gVisor itself.
- name: Build a list of crio runtimes with gVisor runtime | ||
set_fact: | ||
crio_runtimes: "{{ crio_runtimes + [gvisor_runtime] }}" | ||
when: | ||
- gvisor_enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you wouldn't need this if you aren't going to support cri-o with runsc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this part here because I'm actually hoping gVisor will eventually support cri-o when google/gvisor#193 is fixed.
Is there any intention to properly support cri-o in gVisor? If not then I will remove this part from the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'd remove it for now. cri-o will need some work in order to better support runtimes that aren't vanilla containers. It's not just a matter of supporting systemd-cgroups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, in this case I dropped the support for cri-o until gVisor project decides to support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature works fine for me, thanks for doing this.
/lgtm
Just a question: I'd like to know how to see OCI runtime (gvisor in this case) on kubectl. I can see containerd on kubectl describe node
, but gvisor is not there.
## Usage | ||
|
||
To enable gVisor you should be using a container manager that is compatible with selecting the [RuntimeClass](https://kubernetes.io/docs/concepts/containers/runtime-class/) such as `containerd`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: After testing this pull request locally, I see etcd_deployment_type needs to be host
to pass Stop if etcd deployment type is not host when container_manager != docker
.
This thing is already written on docs/containerd.md, so I don't have strong opinion to write it here.
You can see it if you print the whole yaml via However, I assume by OCI runtime you mean the the CRI runtime handler, and needs to be viewed on the RuntimeClass object as the RuntimeClass name and handler name can be different. So you can get the RuntimeClass from one of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work with that, great to see this implemented in the codebase
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cristicalin, floryut 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 |
Thanks for your help, @ianlewis :-) |
Thanks @cristicalin! |
* master: Update kubernetes-reliability.md (kubernetes-sigs#7724) Fix deployment without openstack cacert (kubernetes-sigs#7723) Clean up residual files (kubernetes-sigs#7722) gVisor: initial support for gVisor container runtime (kubernetes-sigs#7661)
…#7661) * Docker/Containerd: move downloads urls to containerd-common * gVisor: initial support for gVisor container runtime
…#7661) * Docker/Containerd: move downloads urls to containerd-common * gVisor: initial support for gVisor container runtime
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds support to use gVisor as a container runtime. According to the related issue, docker is missing the capability to change the
RuntimeClass
, so onlycontainerd
andcri-o
are viable option.This PR add support for both
containerd
andcri-o
to configure the gVisor runtime but I have yet to be able to getcri-o
to successfully launch gVisor workloads and, as per the linked issue it seems gVisor is also not fully support it either as such the validation step only allows gVisor to be enabled withcontainerd
.Which issue(s) this PR fixes:
Fixes #7650
Special notes for your reviewer:
I implemented molecule testing for ubuntu 20.04 and centos 8, in order to cover centos 8 I also had to move some stuff around between the
docker
andcontainerd-common
roles so I can reliably test withcontainerd
on centos.Does this PR introduce a user-facing change?: