Skip to content

fix: Hook should run before deleting resources - #5185

Open
zirain wants to merge 2 commits into
kubernetes-sigs:mainfrom
zirain-dev:hook
Open

fix: Hook should run before deleting resources#5185
zirain wants to merge 2 commits into
kubernetes-sigs:mainfrom
zirain-dev:hook

Conversation

@zirain

@zirain zirain commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

/area conformance-test

What this PR does / why we need it:

#5143 changed the execute order, Hook should run before deleting resource.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Hook should run before deleting resources

Signed-off-by: zirain <zirain2009@gmail.com>
@kubernetes-prow kubernetes-prow Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. area/conformance-test Issues or PRs related to Conformance tests. labels Aug 19, 2026
@kubernetes-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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

@kubernetes-prow kubernetes-prow Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 19, 2026
@kubernetes-prow
kubernetes-prow Bot requested a review from davidjumani August 19, 2026 12:12
@kubernetes-prow kubernetes-prow Bot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 19, 2026
@zirain

zirain commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

/cherry-pick release-1.6

@k8s-infra-cherrypick-robot

Copy link
Copy Markdown
Contributor

@zirain: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.6

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.

@rikatz

rikatz commented Aug 19, 2026

Copy link
Copy Markdown
Member

/cc @kl52752

@kubernetes-prow
kubernetes-prow Bot requested a review from kl52752 August 19, 2026 17:17
@kl52752

kl52752 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

@zirain Can you explain more what is the problem with current solution?

From go spec: https://pkg.go.dev/testing#B.Cleanup

When sub test finished then recordTest and after that a Hook will be called.
In your fix you added another call to subTest.CleanUp function which will put Hook at the end of the queue and this way it will be called before registerTest.
If you want to change the order of this operation and call Hook before register test you can do this in line 535 but I think that we should not do this since Hook is an injected resource.

@zirain

zirain commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

check the following log:

=== NAME  TestGatewayAPIConformance/BackendTLSPolicySANValidation
    apply.go:279: 2026-08-19T13:42:16.285132903Z: Deleting multiple-mismatch-sans BackendTLSPolicy
    apply.go:279: 2026-08-19T13:42:16.289631579Z: Deleting multiple-sans BackendTLSPolicy
    apply.go:279: 2026-08-19T13:42:16.294784398Z: Deleting san-uri-mismatch BackendTLSPolicy
    apply.go:279: 2026-08-19T13:42:16.301014259Z: Deleting san-uri BackendTLSPolicy
    apply.go:279: 2026-08-19T13:42:16.307748981Z: Deleting san-dns-mismatch BackendTLSPolicy
    apply.go:279: 2026-08-19T13:42:16.31373237Z: Deleting san-dns BackendTLSPolicy
    apply.go:279: 2026-08-19T13:42:16.319521716Z: Deleting backendtlspolicy-multiple-mismatch-sans-test Service
    apply.go:279: 2026-08-19T13:42:16.344050901Z: Deleting backendtlspolicy-multiple-sans-test Service
    apply.go:279: 2026-08-19T13:42:16.3767764Z: Deleting backendtlspolicy-san-uri-mismatch-test Service
    apply.go:279: 2026-08-19T13:42:16.397505547Z: Deleting backendtlspolicy-san-uri-test Service
    apply.go:279: 2026-08-19T13:42:16.417452466Z: Deleting backendtlspolicy-san-dns-mismatch-test Service
    apply.go:279: 2026-08-19T13:42:16.440044871Z: Deleting backendtlspolicy-san-dns-test Service
    apply.go:279: 2026-08-19T13:42:16.462657792Z: Deleting backendtlspolicy-san-test HTTPRoute
    hook.go:18: 2026-08-19T13:42:16.466663009Z: Run hook for test: BackendTLSPolicySANValidation for result: true
    hook.go:20: 2026-08-19T13:42:16.466750303Z: Test BackendTLSPolicySANValidation failed, collecting and dumping resources
    utils.go:938: 2026-08-19T13:42:16.467035739Z: creating e2e artifacts directory 

Hook run after Deleting xxxxxx, which is my original idea, I want to dump the current status before that happen.

the logs from my fork worked as expected.

=== NAME  TestGatewayAPIConformance/BackendTLSPolicySANValidation
    hook.go:19: 2026-08-19T15:08:08.751204154Z: Test BackendTLSPolicySANValidation failed, collecting and dumping resources
    utils.go:952: 2026-08-19T15:08:08.751475593Z: creating e2e artifacts directory /home/runner/work/gateway/gateway/artifacts/conformance/TestGatewayAPIConformance/BackendTLSPolicySANValidation-2026-08-19T15_08_08
    apply.go:279: 2026-08-19T15:08:09.204177141Z: Deleting multiple-mismatch-sans BackendTLSPolicy
    apply.go:279: 2026-08-19T15:08:09.207998505Z: Deleting multiple-sans BackendTLSPolicy
    apply.go:279: 2026-08-19T15:08:09.212708382Z: Deleting san-uri-mismatch BackendTLSPolicy
    apply.go:279: 2026-08-19T15:08:09.21898597Z: Deleting san-uri BackendTLSPolicy
    apply.go:279: 2026-08-19T15:08:09.223764378Z: Deleting san-dns-mismatch BackendTLSPolicy
    apply.go:279: 2026-08-19T15:08:09.230100588Z: Deleting san-dns BackendTLSPolicy
    apply.go:279: 2026-08-19T15:08:09.236350003Z: Deleting backendtlspolicy-multiple-mismatch-sans-test Service
    apply.go:279: 2026-08-19T15:08:09.250499398Z: Deleting backendtlspolicy-multiple-sans-test Service
    apply.go:279: 2026-08-19T15:08:09.273225401Z: Deleting backendtlspolicy-san-uri-mismatch-test Service
    apply.go:279: 2026-08-19T15:08:09.286509654Z: Deleting backendtlspolicy-san-uri-test Service
    apply.go:279: 2026-08-19T15:08:09.301718555Z: Deleting backendtlspolicy-san-dns-mismatch-test Service
    apply.go:279: 2026-08-19T15:08:09.322977839Z: Deleting backendtlspolicy-san-dns-test Service
    apply.go:279: 2026-08-19T15:08:09.338324478Z: Deleting backendtlspolicy-san-test HTTPRoute
--- FAIL: TestGatewayAPIConformance (441.84s)

Not sure about this When sub test finished then recordTest and after that a Hook will be called., at least, Hook didn't cancel.

Signed-off-by: zirain <zirain2009@gmail.com>
@kubernetes-prow kubernetes-prow Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 23, 2026
@snorwin

snorwin commented Sep 1, 2026

Copy link
Copy Markdown
Member

/cc

@kubernetes-prow
kubernetes-prow Bot requested a review from snorwin September 1, 2026 13:36
@kl52752

kl52752 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@zirain your test failed, can you verify also the scenario where test is passing? Or we have multiple tests?

@zirain

zirain commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@zirain your test failed, can you verify also the scenario where test is passing? Or we have multiple tests?

To be clear, this's added for dump the cluster status for debugging when test failed(with FailFast=true, Parallel=false).

You can check out our test case here, collecting and dumping resources happen twice after deleted resources.

For specified test, you can use t.Cleanup() in the Test function.

@zirain

zirain commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

@kl52752 this bothers me a lot, Cleanup is LIFO, so I think my fixes keep the behavior same as v1.6.0(without #5143).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/conformance-test Issues or PRs related to Conformance tests. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants