(Bug) Fix InferenceService readiness bootstrap from missing workload conditions#560
Open
YouNeedCryDear wants to merge 4 commits intomainfrom
Open
(Bug) Fix InferenceService readiness bootstrap from missing workload conditions#560YouNeedCryDear wants to merge 4 commits intomainfrom
YouNeedCryDear wants to merge 4 commits intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the ome-manager and model-agent images to v0.1.5 and refactors status condition handling to use explicit Unknown states during initialization and retrieval. It also adds logic to re-fetch Deployment and LeaderWorkerSet resources to capture status updates. The review feedback points out that context.TODO() should be replaced with proper context propagation and warns that immediate re-fetching might return cached data rather than the updated status from the API server.
fb1cd13 to
b244392
Compare
c2709da to
724d7b1
Compare
389cb2a to
6c735dc
Compare
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 this PR does
Unknownconditions instead of silent no-opsInferenceServiceconditions on first observation so fresh raw services do not publish readiness before authoritative workload status existsWhy we need it
Fresh raw
InferenceServiceobjects can reportReadybeforeDeploymentAvailable/LeaderWorkerSetAvailablehas been populated. This closes the bootstrap hole and makes missing workload status block readiness instead of leaking through asReady=True.Fixes #545
How to test
go test ./pkg/controller/v1beta1/inferenceservice/statusgo test ./pkg/controller/v1beta1/inferenceservice/reconcilers/deploymentgo test ./pkg/controller/v1beta1/inferenceservice/reconcilers/lwsgo test ./pkg/controller/v1beta1/inferenceservice/...currently fails in the envtest-backed controller suite in this sandbox withlisten tcp 127.0.0.1:0: bind: operation not permittedChecklist
make testpasses locally