8000 Fix volume limit e2e test cleanup by jsafrane · Pull Request #132674 · kubernetes/kubernetes · GitHub
[go: up one dir, main page]
Skip to content

Fix volume limit e2e test cleanup #132674

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 3, 2025
Merged

Fix volume limit e2e test cleanup #132674

merged 1 commit into from
Jul 3, 2025

Conversation

jsafrane
Copy link
Member
@jsafrane jsafrane commented Jul 2, 2025 •
edited
Loading

Pass the local test state to cleanupTest() as a pointer. It is mostly empty at the point of calling DeferCleanup(cleanupTest, ...), so using ginkgo.DeferCleanup(cleanupTest, ..., l.podNames, ...) just passes empty podNames to the cleanup and the test pods are not deleted.
l.podNames gets populated later and the cleanup must use the updated one.

And now that the cleanup actually does something, fix the error handling in it.

As result, the test deletes all test pods while csi-driver-hostpath is still installed and the pods can be actually deleted.

What type of PR is this?

/kind bug
/kind failing-test
Fixes: #132673

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

Fixed e2e test "[Driver: csi-hostpath] [Testpattern: Dynamic PV (filesystem volmode)] volumeLimits should support volume limits]" not to leak Pods and namespaces.

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 2, 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 needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 2, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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 added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test sig/storage Categorizes an issue or PR as relevant to SIG Storage. 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 2, 2025
@k8s-ci-robot k8s-ci-robot requested review from gnufied and humblec July 2, 2025 11:18
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 2, 2025
@jsafrane
Copy link
Member Author
jsafrane commented Jul 2, 2025

/retest
I can see ImagePullBackOff in pulling CSI driver images. This PR does not change those.

@openshift-ci-robot openshift-ci-robot mentioned this pull request Jul 2, 2025
Merged
gnufied
gnufied reviewed Jul 2, 2025
View reviewed changes
test/e2e/storage/testsuites/volumelimits.go Outdated
@@ -169,7 +170,7 @@ func (t *volumeLimitsTestSuite) DefineTests(driver storageframework.TestDriver,

l.resource = storageframework.CreateVolumeResource(ctx, driver, l.config, pattern, testVolumeSizeRange)
ginkgo.DeferCleanup(l.resource.CleanupResource)
ginkgo.DeferCleanup(cleanupTest, l.cs, l.ns.Name, l.podNames, l.pvcNames, l.pvNames, testSlowMultiplier*f.Timeouts.PVDelete)
ginkgo.DeferCleanup(cleanupTest, &l, testSlowMultiplier*f.Timeouts.PVDelete)
Copy link
Member

Choose a reason for hiding this comment

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

We have pretty well established pattern of each test suite defining a local cleanup function -

kubernetes/test/e2e/storage/testsuites/volume_modify.go

Line 138 in 33cd2e1

cleanup := func(ctx context.Context) {
which will also probably avoid problems like these. I personally think, we should refactor this code to use that pattern, but I won't hold you to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to closure, same as volume_modify.go.

@jsafrane
Fix volume limit e2e test cleanup
ebea328 
Pass the local test state to cleanupTest() as a pointer. It is mostly empty
at the point of calling `DeferCleanup(cleanupTest)`, so using
`ginkgo.DeferCleanup(cleanupTest, ..., l.podNames, ...)` will pass empty
podNames to the cleanup.

And now that the cleanup actually does something, fix the error handling in
it.
@jsafrane jsafrane force-pushed the fix-volumelimit-cleanup branch from 54403ef to ebea328 Compare July 3, 2025 08:50
bertinatto
bertinatto reviewed Jul 3, 2025
View reviewed changes
Copy link
Member
@bertinatto bertinatto left a comment

Choose a reason for hiding this comment

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

The issue that this PRs fixes manifested when another test tried to drain the node. I can confirm that I no longer can reproduce it after applying this patch.

/lgtm

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

LGTM label has been added.

Git tree hash: 9381842821b5787fb082b5afd337f81f29e664ec

@bertinatto
Copy link
Member

/retest

@jsafrane
Copy link
Member Author
jsafrane commented Jul 3, 2025

/test pull-kubernetes-unit
etcd and proxy unit tests

@jsafrane
Copy link
Member Author
jsafrane commented Jul 3, 2025

/test pull-kubernetes-unit

@Phaow
Copy link
Contributor
Phaow commented Jul 3, 2025

/test pull-kubernetes-e2e-gce

@jsafrane
Copy link
Member Author
jsafrane commented Jul 3, 2025

/test pull-kubernetes-e2e-gce
That job does not even run the fixed test

@k8s-ci-robot k8s-ci-robot merged commit f93a8c5 into kubernetes:master Jul 3, 2025
17 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.34 milestone Jul 3, 2025
@openshift-ci-robot openshift-ci-robot mentioned this pull request Jul 7, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnufied gnufied gnufied left review comments

@bertinatto bertinatto bertinatto left review comments

@humblec humblec Awaiting requested review from humblec

Assignees

@bertinatto bertinatto

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. 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
None yet
Milestone
v1.34
Development

Successfully merging this pull request may close these issues.

e2e test "volumeLimits should support volume limits" leaks namespace + Pod
5 participants
@jsafrane @k8s-ci-robot @bertinatto @Phaow @gnufied
0