Repository navigation
test: consolidate isPodReady on the stricter semantics - #1969
Merged
volcano-sh-bot merged 1 commit intoNov 6, 2026
Merged
Conversation
utils.IsPodReady treated a terminating Pod as ready, because a Pod keeps Running and its Ready condition for a while after deletion starts. Three call sites checked DeletionTimestamp themselves and a fourth kept a private copy of the helper that did. Move the check into the shared helper and drop the four workarounds. Signed-off-by: iamtanishqjain <tanishqjain7014@gmail.com>
Contributor
Author
|
/cc @FAUST-BENCHOU |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the test-only cleanup centralizing deletion-aware Pod readiness checks.
No open issues 🎉
What changed in this PR
This test-only cleanup centralizes stricter Pod readiness checks in the shared end-to-end test helper, preventing terminating Pods from being counted as ready.
Changes:
- Make
utils.IsPodReadyreject Pods with a deletion timestamp. - Remove redundant deletion checks from callers.
- Replace the private in-place-update readiness helper with the shared helper.
| File | Description |
|---|---|
test/e2e/utils/utils.go |
Exclude terminating Pods and document the readiness behavior. |
test/e2e/router/shared.go |
Remove the redundant deletion check in the sticky-session test. |
test/e2e/controller-manager/model_serving_test.go |
Simplify the Pod recovery readiness check. |
test/e2e/controller-manager/model_serving_inplace_test.go |
Replace the private readiness helper with utils.IsPodReady. |
test/e2e/controller-manager/model_serving_coordination_test.go |
Use the shared helper for deletion-aware readiness checks. |
Review effort: Balanced
Contributor
Author
|
/cc @YaoZengzeng PTAL sir, all CI passed |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FAUST-BENCHOU The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
github-project-automation
Bot
moved this from Accepted
to Done
in Kthena Release Tracking
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
utils.IsPodReadyreturns true for a terminating Pod, because a Pod keepsRunningand itsReadycondition for a while after its deletion starts. A test waiting for N ready Pods can therefore count one that is going away.Four call sites already work around this in three different ways:
model_serving_coordination_test.go:135checksDeletionTimestampbefore the callmodel_serving_test.go:524does the samerouter/shared.go:2205does the same with the condition invertedmodel_serving_inplace_test.go:97keeps a private copy of the helper that checks itThe remaining 11 callers do not check at all.
This moves the check into the shared helper and removes the four workarounds.
Which issue(s) this PR fixes:
N/A
Bug evidence (required for bug-related PRs):
N/A, test-only.
Special notes for your reviewer:
I could not run the e2e suites locally, so I am relying on CI here.
Does this PR introduce a user-facing change?