Skip to content

Added PodLifecycleSleepActionAllowZero to sig-node-presubmit jobs#34483

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
sreeram-venkitesh:add-pod-lifecycle-sleep-action-allow-zero
Apr 11, 2025
Merged

Added PodLifecycleSleepActionAllowZero to sig-node-presubmit jobs#34483
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
sreeram-venkitesh:add-pod-lifecycle-sleep-action-allow-zero

Conversation

@sreeram-venkitesh
Copy link
Member

This PR adds the e2e test for PodLifecycleSleepActionAllowZero to sig-node-presubmit jobs. This is based on @HirazawaUi's PR #33687 based on this comment: kubernetes/kubernetes#130621 (comment).

@HirazawaUi Please take a look!

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 11, 2025
@k8s-ci-robot k8s-ci-robot added area/config Issues or PRs related to code in /config area/jobs size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 11, 2025
@sreeram-venkitesh sreeram-venkitesh force-pushed the add-pod-lifecycle-sleep-action-allow-zero branch from 4855662 to da62595 Compare March 11, 2025 14:10
@HirazawaUi
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2025
@HirazawaUi
Copy link
Contributor

/assign @kannon92
for approval

requests:
cpu: 4
memory: 6Gi
- name: pull-kubernetes-e2e-pod-lifecycle-sleep-action-allow-zero
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is using deprecated kubernetetes_e2e.py. Can you update this using kubetest2?

@kannon92
Copy link
Contributor

kannon92 commented Mar 11, 2025

/hold

Please try to use kubetest2 for this.

#32567

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 11, 2025
@sreeram-venkitesh
Copy link
Member Author

@kannon92 Can you please review 74f5b1f? I referred another job in the same file which uses kubetest2 for making this change.

@kannon92
Copy link
Contributor

What kind of test is this? e2e or e2e_node?

I see that you picked crio for this one and I'm not sure if that is intendend.

@sreeram-venkitesh
Copy link
Member Author

This is an e2e test (It comes under e2e/common/node/lifecycle_hook.go). I'm going through sig-node-presubmit.yaml now and all the test are e2e_node ones I'm guessing? Sorry for the confusion, this is my first time doing this.

@kannon92
Copy link
Contributor

We could try it!

Please change the image config to use cgroup v2 and I'll approve.

There are other containerd examples for kubetest2 also but if crio works then I think it is fine.

@sreeram-venkitesh
Copy link
Member Author

sreeram-venkitesh commented Mar 11, 2025

Thanks I've updated it! I would like to try running this job locally once before merging it though. I can do this tomorrow and update here. I can also try with both containerd and crio if that works. What do you think?

@kannon92
Copy link
Contributor

@sreeram-venkitesh have you coordinated with @AxeZhan on this?

You are adding a test for your sub feature but do we have testing for the PodLifecycleSleepAction? We could just add this job to that one?

@sreeram-venkitesh
Copy link
Member Author

@kannon92 I could find this PR from @AxeZhan where the feature gate is added to the pull-kubernetes-e2e-gce-cos-alpha-features job. Would a similar change be enough in this case?

@kannon92
Copy link
Contributor

AFAIK kubernetes/kubernetes#128046 was added to try and graduate this feature.

The test flaked as it was not yet added to a periodic

I think you could add your test to that existing job.

- --repo-root=.
- --gcp-zone=us-west1-b
- --parallelism=1
- --focus-regex=\[Feature:PodLifecycleSleepActionAllowZero\]
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to take PodLifecycleSleepAction with this.

@kannon92
Copy link
Contributor

Thanks for following up on this.

I don’t think you removed that skip from all relevant jobs.

I see https://testgrid.k8s.io/sig-node-cri-o#ci-crio-cgroupv1-node-e2e-features and I think we also have some presubmits for these feature jobs which may need to be updated.

im fine with this as a follow up though.

/lgtm
/approve

you may need other approves for this.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2025
@pacoxu
Copy link
Member

pacoxu commented Mar 17, 2025

/approve
/assign @andrewsykim @cheftako

@ffromani
Copy link
Contributor

/hold cancel

per @kannon92 further comments it seems to me the hold no longer applies

@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 Mar 18, 2025
@mrunalp
Copy link

mrunalp commented Mar 19, 2025

Can we clean up the commits here?

@kannon92
Copy link
Contributor

/hold cancel

per @kannon92 further comments it seems to me the hold no longer applies

I gave my approval for node but I don't have rights for cloud provider.

@sreeram-venkitesh sreeram-venkitesh force-pushed the add-pod-lifecycle-sleep-action-allow-zero branch from df77189 to 6f78b60 Compare March 19, 2025 20:25
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2025
@sreeram-venkitesh
Copy link
Member Author

@mrunalp I've squashed all the commits.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2025
@sreeram-venkitesh
Copy link
Member Author

We can merge this now that kubernetes/kubernetes#130621 has been merged.

@kannon92
Copy link
Contributor

We can merge this now that kubernetes/kubernetes#130621 has been merged.

@andrewsykim @cheftako

can you please give your blessing?

@kannon92
Copy link
Contributor

Sad.. it would be nice to get this PR in.

@dims or @BenTheElder are you able to help here?

@BenTheElder
Copy link
Member

looking

Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
in 1.34 we'll iterate more on how this works to be a bit easier, expect a psa to dev@kubernetes.io

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, kannon92, mrunalp, pacoxu, sreeram-venkitesh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2025
@BenTheElder
Copy link
Member

/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 Apr 11, 2025
@BenTheElder
Copy link
Member

/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 Apr 11, 2025
@k8s-ci-robot k8s-ci-robot merged commit 8e76d39 into kubernetes:master Apr 11, 2025
7 checks passed
@k8s-ci-robot
Copy link
Contributor

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

  • key gcp-gce.yaml using file config/jobs/kubernetes/sig-cloud-provider/gcp/gcp-gce.yaml
  • key crio.yaml using file config/jobs/kubernetes/sig-node/crio.yaml
Details

In response to this:

This PR adds the e2e test for PodLifecycleSleepActionAllowZero to sig-node-presubmit jobs. This is based on @HirazawaUi's PR #33687 based on this comment: kubernetes/kubernetes#130621 (comment).

@HirazawaUi Please take a look!

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.

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 area/provider/gcp Issues or PRs related to gcp provider 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. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Comments