Fix exposed service Kueue admission race - #633
Draft
gmolto wants to merge 1 commit into
Draft
Conversation
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.
Summary
Admitted=Trueinstead of treating the first non-admitted snapshot as a definitive rejectionProblem
The issue was detected while running the OSCAR metrics population Robot suite against a local cluster. The suite created four regular services successfully, but creation of the exposed nginx service intermittently returned HTTP 500.
Manager logs showed the generic message:
The Kueue events showed that the workload was initially
Pendingwhile the controller evaluated or waited for quota. OSCAR checked the workload immediately after creating it and interpreted the absence ofAdmitted=Trueas a permanent rejection. It then deleted the workload and failed service creation before Kueue could publish a later admission update.This is a race between the synchronous exposed-service creation path and Kueue's asynchronous status reconciliation. A workload that is not admitted in the first snapshot is not necessarily rejected.
Root cause
CheckWorkloadAdmitedstarted and synchronized an informer, read the workload once, and returned an error whenever that snapshot did not already containAdmitted=True. The update handler did not wait for or act on subsequent status transitions.The codebase already had
onlyCheckWorkloadAdmited, which checks the initial informer store and then waits for an admission update until a configurable timeout. The exposed-service path was not using it.Change
CheckWorkloadAdmitednow delegates admission waiting toonlyCheckWorkloadAdmitedwith the existing 30-second default timeout. If admission succeeds, deployment activation continues as before. If it does not complete within the timeout, the workload is cleaned up and the returned error identifies the timeout.The shared helper now receives the workload namespace and requires both namespace and name to match, preventing an admitted workload with the same name in another user namespace from satisfying the check.
Impact
Exposed-service creation continues immediately when Kueue has already admitted the workload. When Kueue reconciliation takes longer, the API waits only for the actual delay, up to 30 seconds, instead of returning a premature HTTP 500.
Requests that remain pending because quota is genuinely unavailable still fail after the timeout and preserve the existing cleanup behavior.
Validation
go test ./...