-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
ci: separarte pull-containerd-node-e2e for 1.5 branch #27912
Merged
k8s-ci-robot
merged 3 commits into
kubernetes:master
from
akhilerm:pull-containerd-node-e2e-1_5
Nov 9, 2022
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
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.
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.
This image config is pointing to to
cos-stable
(https://github.com/kubernetes/test-infra/blob/master/jobs/e2e_node/containerd/containerd-main-presubmit/image-config-presubmit.yaml#L3-L5)cos-stable is using COS 101 which has cgroupv2 enabled. cgroupv2 should enable the systemd cgroup driver since the default (cgroupfs driver is unsupported).
I think we should also add https://github.com/kubernetes/test-infra/blob/master/jobs/e2e_node/containerd/containerd-main/cgroupv2/env-cgroupv2#L10 in
CONTAINERD_SYSTEMD_CGROUP: 'true'
and add
--cgroup-driver=systemd
to kubelet flags like https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-node/containerd.yaml#L238There 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.
Since all the images for containerd are updated to
cos-stable
, should we add the cgroup_env to all the tests exceptcontainerd-main/cgroupv1
?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.
Yup, that makes sense. Thanks!
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 have one more doubt related to the cgroupv2, these env is currently not set on the image files for other tests, but its still passing. So isnt it a mandatory requirement?
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.
Not mandatory (if it is not specified it will use the cgroupfs driver). But the systemd driver is what is recommended to be used with cgroupv2 (https://kubernetes.io/docs/concepts/architecture/cgroups/#requirements) and what we are recommending to all k8s users on cgroupv2 OS images.
We want to make it default in future version of containerd - containerd/containerd#7319
(Docker already defaults to using systemd cgroup driver on cgroupv2) - moby/moby#40846
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.
Shouldnt we also add the
CONTAINERD_COS_CGROUP_MODE: 'v2'
to the env? Or is it set by default in cos-101?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.
@akhilerm I think its fine not to set
CONTAINERD_COS_CGROUP_MODE: 'v2'
as cgroups v2 is enabled by default in cos 101@bobbypage @mikebrow so this pull job will cover the cgroupsv2 only, do we need to add an additional pull job for cgroupsv1?
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.
Yes, it's fine to not set
CONTAINERD_COS_CGROUP_MODE
, this is only needed to force COS to enable cgroupv2 but since it's already enabled started from COS97 this is unnecessary