Skip to content

Add wait for ingress deletion in NEG test #3540

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add wait for ingress deletion in NEG test #3540

wants to merge 1 commit into from
+15 −9

Conversation

panslava
Copy link
Contributor

@panslava panslava commented Aug 26, 2025 •
edited
Loading

What type of PR is this?
/kind bug
/kind flake

What this PR does / why we need it

  • Ensures NEG tests wait until all Ingress resources are actually deleted before proceeding, reducing end-of-test flakiness.
  • Handles immediate deletions where DeletionTimestamp may be nil by recording deletion phases using a local timestamp fallback, so deletion latency is tracked consistently.

Changes

  • Add explicit wait for Ingress deletion in the NEG test flow:
    • clusterloader2/testing/neg/config.yaml:125
      • Add module step invoking ingress-measurements.yaml with action: waitForDeletion.
  • Record delete phases even when DeletionTimestamp is nil:
    • clusterloader2/pkg/measurement/common/service_creation_latency.go:365
      • Service deletion: set deleting from DeletionTimestamp when present; otherwise use time.Now(). Always set deleted to time.Now().
    • clusterloader2/pkg/measurement/common/service_creation_latency.go:383
      • Ingress deletion: same behavior as Services.

Which issue(s) this PR fixes
Fixes #

  • This aligns NEG cleanup with L4LB tests, which already wait for deletion, making behavior consistent.
  • Mixed time sources: creation uses server time; deletion fallback uses time.Now(). Negative latencies are already clamped to zero in phase_latency.go, so this remains safe, but be aware of potential minor clock skew when interpreting results.

Release note

ClusterLoader2: Ensure NEG tests wait for Ingress deletions before finalizing. ServiceCreationLatency now records deletion phases even when DeletionTimestamp is nil, improving reliability of deletion tracking and reducing test flakiness.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: panslava
Once this PR has been reviewed and has the lgtm label, please assign wojtek-t 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 requested a review from mborsz August 26, 2025 19:47
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 26, 2025
@k8s-ci-robot k8s-ci-robot requested a review from wojtek-t August 26, 2025 19:47
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 26, 2025
@panslava
Fix deletion handling in NEG tests and service creation latency
366cd45 
- Handle nil DeletionTimestamp gracefully by using current time when
  objects are deleted immediately without a deletion timestamp
- Add explicit wait for ingress deletion in NEG test configuration
  to ensure all ingresses are fully removed before test completion
- Remove incorrect warning logs that were preventing proper deletion
  tracking when DeletionTimestamp was nil

These changes improve the reliability of NEG tests by properly
tracking deletion phases even when Kubernetes performs immediate
deletions, and ensuring proper cleanup sequencing.
@panslava panslava force-pushed the wait-for-ing-deletion branch from ef8fa4b to 366cd45 Compare September 3, 2025 13:11
@panslava panslava marked this pull request as ready for review September 3, 2025 13:18
@panslava
Copy link
Contributor Author

panslava commented Sep 3, 2025

/cc @swetharepakula

@panslava panslava changed the title [WIP] Add wait for ingress deletion in NEG test Add wait for ingress deletion in NEG test Sep 4, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2025
panslava
panslava commented Sep 4, 2025
View reviewed changes
clusterloader2/pkg/measurement/common/service_creation_latency.go
@@ -337,7 +337,7 @@ func (s *serviceCreationLatencyMeasurement) handleIngressObject(oldObj, newObj i
return
}
newIngress, ok = newObj.(*networkingv1.Ingress)
if newIngress != nil && !ok {
if newObj != nil && !ok {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just fixes mistaken code I noticed, we should compare newObj, not newIngress (look how we do the same above this for services)

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

@mborsz mborsz Awaiting requested review from mborsz

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

@swetharepakula swetharepakula Awaiting requested review from swetharepakula

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@panslava @k8s-ci-robot