-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix tests in sig-node related to CRIv1 update #28071
fix tests in sig-node related to CRIv1 update #28071
Conversation
Signed-off-by: Akhil Mohan <[email protected]>
Updated cos images to cos-stable Updated ubuntu to ubuntu-2204-lts image_family This update will fix the following jobs: - ci-kubernetes-node-kubelet-containerd-eviction - ci-kubernetes-node-kubelet-serial-containerd - pull-kubernetes-node-kubelet-serial-containerd - pull-kubernetes-node-kubelet-serial-containerd-kubetest2 - pull-kubernetes-node-kubelet-serial-pod-disruption-conditions Signed-off-by: Akhil Mohan <[email protected]>
Hi @akhilerm. Thanks for your PR. I'm waiting for a kubernetes 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. |
machine: n1-standard-2 # These tests need a lot of memory | ||
metadata: "user-data</workspace/test-infra/jobs/e2e_node/containerd/init.yaml,cni-template</workspace/test-infra/jobs/e2e_node/containerd/cni.template,containerd-config</workspace/test-infra/jobs/e2e_node/containerd/config.toml" | ||
cos-stable2: | ||
image_family: cos-93-lts # deprecated after October 2023 (https://cloud.google.com/container-optimized-os/docs/release-notes) | ||
image_family: cos-stable |
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.
@SergeyKanzhelev Why were we maintaining 2 separate cos versions for the tests in image-config-serial ?
/cc @SergeyKanzhelev @bobbypage @adisky Created this as a separate PR from #27943 so as not to make the review process of that more complex. |
/ok-to-test |
image_family: pipeline-1-24 | ||
project: ubuntu-os-gke-cloud | ||
image_family: ubuntu-2204-lts | ||
project: ubuntu-os-cloud | ||
machine: n1-standard-2 # These tests need a lot of memory | ||
metadata: "user-data</workspace/test-infra/jobs/e2e_node/containerd/init.yaml,cni-template</workspace/test-infra/jobs/e2e_node/containerd/cni.template,containerd-config</workspace/test-infra/jobs/e2e_node/containerd/config.toml" |
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.
These need to point to containerd config file that will set CONTAINERD_SYSTEMD_CGROUP
so that systemd cgroup driver will be configured on containerd side as well. I don't see that being the case currently?
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.
It looks like these jobs just use this helper script (https://github.com/kubernetes/test-infra/blob/master/jobs/e2e_node/containerd/init.yaml) which installs this containerd config file - https://github.com/kubernetes/test-infra/blob/master/jobs/e2e_node/containerd/config.toml
Do we need to maybe make a copy of that config.toml
for systemd cgroup driver and point the test to use that config file?
The other option is to configure this test to also use containerd from main like the some of the other jobs, so we can use CONTAINERD_SYSTEMD_CGROUP
setting.
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 other option is to configure this test to also use containerd from main like the some of the other jobs, so we can use CONTAINERD_SYSTEMD_CGROUP setting.
What was the reason for these tests using separate config.toml file? If we change to how its done in other tests, like you suggested, is there something else that need to be taken care of?
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 created a new config-systemd.toml
that will be used instead of config.toml
to use systemd as cgroup driver.
540ed38
to
7ae16b9
Compare
use the config file with systemd enabled in tests which use cgroupv2 Signed-off-by: Akhil Mohan <[email protected]>
7ae16b9
to
ff69de5
Compare
@bobbypage I have made the changes. Can you take a look at it. |
# Enable registry.k8s.io as the primary mirror for k8s.gcr.io | ||
# See: https://github.com/kubernetes/k8s.io/issues/3411 | ||
[plugins."io.containerd.grpc.v1.cri".registry.mirrors."k8s.gcr.io"] | ||
endpoint = ["https://registry.k8s.io", "https://k8s.gcr.io",] | ||
|
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.
Now registry.k8s.io is GA (and introduced in the master branch of k/k) there is no specific value to do this.
we should remove this as a follow-up.
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.
Will create a separate cleanup PR for that, so that the original config.toml can also be fixed.
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.
@@ -3,14 +3,14 @@ | |||
# `gcloud compute --project <to-project> images create <image-name> --source-disk=<image-name>` | |||
images: | |||
ubuntu: | |||
image_family: pipeline-1-24 | |||
project: ubuntu-os-gke-cloud | |||
image_family: ubuntu-2204-lts |
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.
Out of curiosity, why stop use the GKE images ?
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.
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.
Ok. I had a wrong assumption of pipeline-1-24
. I thought since this family is ubuntu-based:
$gcloud compute images describe-from-family pipeline-1-24 --project ubuntu-os-gke-cloud --format='value(name,selfLink)'
ubuntu-gke-2204-1-24-v20221128 https://www.googleapis.com/compute/v1/projects/ubuntu-os-gke-cloud/global/images/ubuntu-gke-2204-1-24-v20221128
the node(s) will have cgroupsv2 enabled.
/cc @bobbypage |
One small change needed, other than that LGTM, thank you for updating! |
Signed-off-by: Akhil Mohan <[email protected]>
/lgtm |
/assign @dims |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akhilerm, dims 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 |
@akhilerm: Updated the
In response to this:
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. |
it looks like https://testgrid.k8s.io/sig-node-containerd#pull-node-e2e went red right around when these two PRs merged: |
Fixes tests under
The following tests are fixed: