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

replicate crio presubmits using kubetest2 #33550

Conversation

elieser1101
Copy link
Contributor

This is part of an effort to start using kubetest2 for crio e2e node tests. Being tracked in #32567. starting with presubmits

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/config Issues or PRs related to code in /config area/jobs needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 30, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @elieser1101. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 30, 2024
@elieser1101
Copy link
Contributor Author

/cc krzyzacy

@kannon92
Copy link
Contributor

kannon92 commented Oct 3, 2024

can you combine the presubmits into a single PR?

Copy link
Member

@krzyzacy krzyzacy left a comment

Choose a reason for hiding this comment

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

/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 Oct 7, 2024
@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 Oct 7, 2024
@@ -1346,6 +1346,59 @@ presubmits:
env:
- name: IGNITION_INJECT_GCE_SSH_PUBLIC_KEY_FILE
value: "1"
- name: pull-kubernetes-node-crio-cgrpv2-userns-e2e-serial-kubetest2
Copy link
Member

Choose a reason for hiding this comment

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

^^ Indentation here is wrong

/hold

@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 Oct 7, 2024
@krzyzacy
Copy link
Member

krzyzacy commented Oct 7, 2024

also +1 to can you combine the presubmits into a single PR? :-)

@kannon92
Copy link
Contributor

kannon92 commented Oct 8, 2024

@elieser1101
Thank you for taking this work on. I went ahead and closed the other PRs. Please combine all these presubmits into a single PR.

/hold

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2024
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2024
@elieser1101 elieser1101 changed the title replicate pull-kubernetes-node-crio-cgrpv2-userns-e2e-serial using kubetest2 replicate crio presubmits using kubetest2 Oct 8, 2024
@elieser1101
Copy link
Contributor Author

Thanks for the help, sorry for the noise.
Think now everything is in place

@kannon92 @krzyzacy

@kannon92
Copy link
Contributor

kannon92 commented Oct 8, 2024

/hold cancel

@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 Oct 8, 2024
@kannon92
Copy link
Contributor

kannon92 commented Oct 8, 2024

At a glance this looks good to me.

So we want to add the kubetest2 jobs and leave the nonkubetest ones around,

When will we remove the legacy jobs?

@kannon92
Copy link
Contributor

kannon92 commented Oct 8, 2024

Have you confirmed that the tests that have merged already are working without issue?

@elieser1101
Copy link
Contributor Author

The idea would be to merge these test, then create a PR on which validate all the kubetest2 presubmits works as expected and refactor anything broken.

After we validate these new test consistently work for some period of time(not sure how much) then we can remove the legacy ones?

@kannon92
Copy link
Contributor

kannon92 commented Oct 8, 2024

You merged a canary one a few weeks ago. Have you tested that one to see if it worked?

@elieser1101
Copy link
Contributor Author

3 out of 5 of the merged PR were tested and worked here:
kubernetes/kubernetes#120459

@krzyzacy
Copy link
Member

krzyzacy commented Oct 8, 2024

I'm fine with adding these jobs since they are optional and non-blocking, we need to be a bit careful when we removing the older jobs and make sure the new ones become blocking and don't accidentally lose coverage

@krzyzacy
Copy link
Member

krzyzacy commented Oct 8, 2024

(see if @kannon92 has any further concerns, otherwise I'll be happy to approve this bulk and track triaging in kubernetes/kubernetes#127831)

@elieser1101
Copy link
Contributor Author

sure, my plan is keep working on this till test are fully working

Copy link
Member

@krzyzacy krzyzacy 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 k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elieser1101, krzyzacy

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

@k8s-ci-robot k8s-ci-robot merged commit 7cadf33 into kubernetes:master Oct 10, 2024
6 of 7 checks passed
@k8s-ci-robot
Copy link
Contributor

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

  • key sig-node-presubmit.yaml using file config/jobs/kubernetes/sig-node/sig-node-presubmit.yaml

In response to this:

This is part of an effort to start using kubetest2 for crio e2e node tests. Being tracked in #32567. starting with presubmits

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-sigs/prow repository.

@pohly
Copy link
Contributor

pohly commented Oct 14, 2024

pull-kubernetes-node-e2e-crio-cgrpv1-dra-kubetest2 and pull-kubernetes-node-e2e-crio-cgrpv2-dra-kubetest2 are not working. Apparently the --label-filter argument is passed to kubetest2, which fails (Error: unknown flag: --label-filter).

cc @klueska @bart0sh

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

5 participants