Skip to content

test: add batch pod deletion for kubelet e2e tests #132980

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ylink-lfs
Copy link
Contributor

@ylink-lfs ylink-lfs commented Jul 16, 2025

What type of PR is this?

/kind cleanup
/sig node
/sig tests
/priority backlog

What this PR does / why we need it:

e2e test simplicity and speedup purpose

Which issue(s) this PR is related to:

Clean up e2e pods in parallel #132974

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

@Copilot Copilot AI review requested due to automatic review settings July 16, 2025 01:27
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 16, 2025
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 16, 2025
@k8s-ci-robot k8s-ci-robot requested a review from andrewsykim July 16, 2025 01:28
@k8s-ci-robot
Copy link
Contributor

Hi @ylink-lfs. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 16, 2025
@k8s-ci-robot
Copy link
Contributor

@ylink-lfs: The label(s) sig/tests cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this?

/kind cleanup
/sig node
/sig tests
/prioroty backlog

What this PR does / why we need it:

e2e test simplicity and speedup purpose

Which issue(s) this PR is related to:

Clean up e2e pods in parallel #132974

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

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 area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 16, 2025
@ylink-lfs
Copy link
Contributor Author

/assign tallclair

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a batch deletion helper for pod cleanup in node e2e tests to streamline parallel pod removals and speed up test execution.

  • Replace sequential DeletePodWithWait calls with DeletePodWithWaitByBatch in node e2e test code
  • Add DeletePodWithWaitByBatch implementation in the framework with concurrent deletion logic
  • Update imports to include slices and sync for the new helper

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
test/e2e/node/pod_resize.go Swapped individual pod deletions for batch deletion calls
test/e2e/node/kubelet.go Updated AfterEach cleanup to use batch deletion
test/e2e/framework/pod/delete.go Added DeletePodWithWaitByBatch with concurrency support
Comments suppressed due to low confidence (1)

test/e2e/framework/pod/delete.go:61

  • The new batch deletion helper lacks unit or integration tests. Adding tests to verify parallel deletion behavior and correct error ordering would ensure this function works as expected under different conditions.
// DeletePodWithWaitByBatch deletes the passed-in pods, waits for the pod to be terminated, return deletion error if exists

@ylink-lfs ylink-lfs force-pushed the test/pod_delete_batch branch 2 times, most recently from 7057a13 to b11bf74 Compare July 16, 2025 01:58
@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 16, 2025
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 16, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ylink-lfs
Once this PR has been reviewed and has the lgtm label, please assign pohly for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 removed area/apiserver area/code-generation area/conformance Issues or PRs related to kubernetes conformance tests area/ipvs area/kube-proxy area/release-eng Issues or PRs related to the Release Engineering subproject labels Jul 16, 2025
@enj enj moved this to Needs Triage in SIG Auth Jul 16, 2025
@k8s-ci-robot k8s-ci-robot removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/windows Categorizes an issue or PR as relevant to SIG Windows. wg/device-management Categorizes an issue or PR as relevant to WG Device Management. labels Jul 16, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 16, 2025

@ylink-lfs: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-node-e2e-crio-cgrpv1-dra a271277 link false /test pull-kubernetes-node-e2e-crio-cgrpv1-dra
pull-kubernetes-e2e-kind-alpha-beta-features a271277 link false /test pull-kubernetes-e2e-kind-alpha-beta-features
pull-kubernetes-e2e-gce-storage-snapshot a271277 link false /test pull-kubernetes-e2e-gce-storage-snapshot
pull-kubernetes-e2e-gce-network-policies a271277 link false /test pull-kubernetes-e2e-gce-network-policies
pull-kubernetes-e2e-gce-cos-alpha-features a271277 link false /test pull-kubernetes-e2e-gce-cos-alpha-features
pull-kubernetes-e2e-gci-gce-ingress a271277 link false /test pull-kubernetes-e2e-gci-gce-ingress
pull-kubernetes-e2e-gce-csi-serial a271277 link false /test pull-kubernetes-e2e-gce-csi-serial
pull-kubernetes-e2e-gce-storage-slow a271277 link false /test pull-kubernetes-e2e-gce-storage-slow

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@ylink-lfs ylink-lfs requested a review from HirazawaUi July 16, 2025 14:19
@ylink-lfs
Copy link
Contributor Author

/retest

@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@@ -56,6 +57,20 @@ func DeletePodWithWait(ctx context.Context, c clientset.Interface, pod *v1.Pod)
return DeletePodWithWaitByName(ctx, c, pod.GetName(), pod.GetNamespace())
}

// DeletePodWithWaitByBatch deletes the passed-in pods, waits for the pod to be terminated, return deletion error if exists
func DeletePodWithWaitByBatch(ctx context.Context, c clientset.Interface, pods []*v1.Pod) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: should this be named like this:

Suggested change
func DeletePodWithWaitByBatch(ctx context.Context, c clientset.Interface, pods []*v1.Pod) {
func DeletePodsWithWaitByBatch(ctx context.Context, c clientset.Interface, pods []*v1.Pod) {

framework.ExpectNoError(DeletePodWithWait(ctx, c, pod), "Error deleting Pod %s", pod.GetName())
}(i, pod)
}
wg.Wait()
Copy link
Member

Choose a reason for hiding this comment

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

it may be a good improvement to fail test here instead of in each goroutine. This way test will clean up it's Pods more consistently, even if one of them failed to be deleted.

@SergeyKanzhelev SergeyKanzhelev moved this from Triage to PRs - Needs Reviewer in SIG Node CI/Test Board Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. 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.
Projects
Status: 🆕 New
Status: Needs Triage
Status: Needs Triage
Status: PRs - Needs Reviewer
Development

Successfully merging this pull request may close these issues.

6 participants