Skip to content
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

Conversation

akhilerm
Copy link
Member

@akhilerm akhilerm commented Nov 7, 2022

containerd v1.5.x supports CRI v1alpha2, the API that was available at the time of release for containerd v1.5.
containerd v1.6.x has support for both CRI v1alpha2 and v1; and is being designated a long term support release.

kubelet master is removing support for CRI v1alpha2, this action has the effect of forcing kubernetes master(and kubernetes r.next+) users to move up to containerd v1.6.x where both CRI v1 and v1alpha2 is supported.

Therefore we need to separate out the pull-containerd-node-e2e job for containerd 1.5 branch, so that patches can still be made to 1.5 branch till its EOL. Instead of running against kubernetes master, it will run against k8s release-1.25 branch (the last release which supports CRI v1alpha2)

Ref: kubernetes/kubernetes#110618

Signed-off-by: Akhil Mohan makhil@vmware.com

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 7, 2022
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added area/config Issues or PRs related to code in /config area/jobs sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Nov 7, 2022
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 7, 2022
@akhilerm
Copy link
Member Author

akhilerm commented Nov 7, 2022

/cc @dims @adisky @mikebrow

We need this job to run till containerd 1.5 EOL, so that the 1.5 branch can still accept patches

@dims
Copy link
Member

dims commented Nov 7, 2022

/assign @mikebrow

@mikebrow
Copy link
Member

mikebrow commented Nov 7, 2022

nit on the explanation text in the PR...
containerd v1.5.x supports CRI v1alpha2, the api that was available at the time of release for containerd v.1.5
containerd v1.6.x has support for both CRI v1alpha2 and v1; and is being designated a long term support release

kubelet master is removing support for CRI v1alpha2, this action has the effect of forcing kubernetes master(and kubernetes r.next+) users to move up to containerd v1.6.x where both CRI v1 and v1alpha2 is supported...

Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with the configuration here, but this LGTM.

@k8s-ci-robot
Copy link
Contributor

@samuelkarp: changing LGTM is restricted to collaborators

In response to this:

I'm not super familiar with the configuration here, but this LGTM.

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.

Copy link

@qiutongs qiutongs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2022
containerd v1.5.x supports CRI v1alpha2, the API that was available at
the time of release for containerd v1.5. containerd v1.6.x has support
for both CRI v1alpha2 and v1; and is being designated a long term
support release.

kubelet master is removing support for CRI v1alpha2, this action has the
effect of forcing kubernetes master(and kubernetes r.next+) users to move
up to containerd v1.6.x where both CRI v1 and v1alpha2 is supported.

Therefore we need to separate out the pull-containerd-node-e2e job for
containerd 1.5 branch, so that patches can still be made to 1.5 branch
till its EOL. Instead of running against kubernetes master, it will run
against k8s release-1.25 branch (the last release which supports CRI v1alpha2)

Ref: kubernetes/kubernetes#110618

Signed-off-by: Akhil Mohan <makhil@vmware.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2022
@akhilerm
Copy link
Member Author

akhilerm commented Nov 8, 2022

@mikebrow Updated the PR description

Copy link
Contributor

@adisky adisky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @akhilerm
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2022
@@ -6,3 +6,4 @@ CONTAINERD_PKG_PREFIX: 'containerd-cni'
CONTAINERD_EXTRA_RUNTIME_HANDLER: 'test-handler'
CONTAINERD_EXTRA_RUNTIME_OPTIONS: |
BinaryName = "/home/containerd/usr/local/sbin/runc"
CONTAINERD_SYSTEMD_CGROUP: 'true'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobbypage Should we add the CONTAINERD_COS_CGROUP_MODE: 'v2' also here?

@akhilerm akhilerm requested review from adisky and removed request for dims, Random-Liu and dchen1107 November 9, 2022 04:20
@akhilerm
Copy link
Member Author

akhilerm commented Nov 9, 2022

/retest

@k8s-ci-robot
Copy link
Contributor

@akhilerm: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@ameukam
Copy link
Member

ameukam commented Nov 9, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 9, 2022
Copy link
Member

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
^
node e2e change

I'll defer to @bobbypage on the COS_CGROUP_MODE thing.

/hold
^
unhold when ready

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2022
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 9, 2022
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akhilerm, endocrimes, mikebrow, qiutongs, samuelkarp

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@SergeyKanzhelev
Copy link
Member

/unhold

lgtm

@bobbypage lmk if something needs to be changed

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2022
@k8s-ci-robot k8s-ci-robot merged commit c2e018d into kubernetes:master Nov 9, 2022
SIG Node PR Triage automation moved this from Triage to Done Nov 9, 2022
@k8s-ci-robot
Copy link
Contributor

@akhilerm: Updated the job-config configmap in namespace default at cluster test-infra-trusted using the following files:

  • key containerd-presubmit-jobs.yaml using file config/jobs/containerd/containerd/containerd-presubmit-jobs.yaml

In response to this:

containerd v1.5.x supports CRI v1alpha2, the API that was available at the time of release for containerd v1.5.
containerd v1.6.x has support for both CRI v1alpha2 and v1; and is being designated a long term support release.

kubelet master is removing support for CRI v1alpha2, this action has the effect of forcing kubernetes master(and kubernetes r.next+) users to move up to containerd v1.6.x where both CRI v1 and v1alpha2 is supported.

Therefore we need to separate out the pull-containerd-node-e2e job for containerd 1.5 branch, so that patches can still be made to 1.5 branch till its EOL. Instead of running against kubernetes master, it will run against k8s release-1.25 branch (the last release which supports CRI v1alpha2)

Ref: kubernetes/kubernetes#110618

Signed-off-by: Akhil Mohan makhil@vmware.com

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.

@bobbypage
Copy link
Member

/unhold

lgtm

@bobbypage lmk if something needs to be changed

Yes, we need to ensure that the systemd cgroup driver is configured for all the containerd jobs using COS M97+. Looks like this will be taken care of in #27943, thanks @akhilerm for followup PR.

@akhilerm akhilerm deleted the pull-containerd-node-e2e-1_5 branch November 10, 2022 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/jobs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

None yet