fix: register e2e Service cleanup defer before it can be skipped - #10829
fix: register e2e Service cleanup defer before it can be skipped#10829pujitha24 wants to merge 1 commit into
Conversation
Motivation: In tests/e2e/network/ensureloadbalancer.go, four specs create/expose a Service and only register the cleanup `defer DeleteService(...)` after a later Get(...) call, with one or more Expect(...) assertions running between successful service creation and that defer registration. In Ginkgo v2 + Gomega, a failing Expect(...) unwinds the current goroutine via runtime.Goexit(); any defer not yet registered at that point is skipped. If the Service already exists by then and one of those intervening assertions fails, the cleanup never runs and the Service leaks. Approach: Move each defer to be registered immediately after the point where the Service is first known to exist (right after Create(...) succeeds, or right after the createAndExposeDefaultServiceWithAnnotation(...) helper returns), before any subsequent Expect(...) calls. This mirrors the fix suggested in the issue report. utils.DeleteService already treats a NotFound error as success, so registering the deferred delete this early is safe even on the path where creation itself failed. Validation: - cd tests && go build ./e2e/network/... (passes) - cd tests && go vet ./e2e/network/... (passes, no findings) - gofmt -l tests/e2e/network/ensureloadbalancer.go (no output, file formatted) - cd tests && golangci-lint run --config ../.golangci.yml ./e2e/network/... reports the same 33 pre-existing issues (goconst/ineffassign/staticcheck) before and after this change, confirming no new lint issues were introduced. These specs are Ginkgo e2e tests that require a live Azure/AKS cluster to execute; that is not available in this environment, so the fix was not exercised against a live cluster. The correctness argument rests on Go's documented defer/Goexit semantics and static review of the reordered control flow, not a live reproduction. Report: kubernetes-sigs#10126 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
Hi @pujitha24. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pujitha24 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/assign @bridgetkromhout Small test-only fix, ready for review whenever you have a moment. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
In
tests/e2e/network/ensureloadbalancer.go, four specs create/expose aService and only register the cleanup
defer DeleteService(...)after alater
Get(...)call, with one or moreExpect(...)assertions runningbetween successful service creation and that defer registration. In
Ginkgo v2 + Gomega, a failing
Expect(...)unwinds the current goroutinevia
runtime.Goexit(); any defer not yet registered at that point isskipped. If the Service already exists by then and one of those
intervening assertions fails, the cleanup never runs and the Service is
left behind in the cluster.
This PR moves each
deferto be registered immediately after the pointwhere the Service is first known to exist (right after
Create(...)succeeds, or right after the
createAndExposeDefaultServiceWithAnnotation(...)helper returns), before any subsequent
Expect(...)calls. This mirrorsthe fix suggested in the issue report.
utils.DeleteServicealreadytreats a NotFound error as success, so registering the deferred delete
this early is safe even on the path where creation itself failed.
Which issue(s) this PR fixes:
Fixes #10126
Special notes for your reviewer:
tests/e2e/network/ensureloadbalancer.goisthe only file touched (4 hunks, 14 insertions / 14 deletions).
go build ./e2e/network/...,go vet ./e2e/network/...,gofmt -l, andgolangci-lint runagainst thetestsmodule — all pass,and golangci-lint reports the same 33 pre-existing issues before and after
this change, confirming no new lint issues were introduced.
execute; that isn't available in this environment, so the fix was not
exercised against a live cluster. The correctness argument rests on Go's
documented defer/Goexit semantics and static review of the reordered
control flow, not a live reproduction.
ok-to-test— happy toaddress anything it surfaces.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: