Repository navigation
stop watcher goroutines from outliving their owner - #13705
kubernetes-prow[bot] merged 3 commits into
Conversation
kubernetes-prow
Bot
added
release-note-none
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
kubernetes-prow
Bot
added
the
cncf-cla: yes
kubernetes-prow
Bot
added
the
needs-ok-to-test
|
Hi @andrewseif. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
kubernetes-prow
Bot
added
the
size/M
📝 WalkthroughWalkthroughMultiKueue now tracks remote watcher goroutines, coordinates shutdown without holding the client mutex, and stops watch processing when contexts are canceled or result channels close. Tests cover watcher joining and cleanup. ChangesMultiKueue watcher lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/admissionchecks/multikueue/multikueuecluster_test.go`:
- Around line 697-702: Add table-driven Ginkgo/Gomega scenarios around the
watcher cleanup using controlled fake watchers and bounded or unbuffered
channels to cover blocking ResultChan(), queueWorkloadEvent() on wlUpdateCh, and
queueWatchEndedEvent() on watchEndedCh. For each boundary, invoke StopWatchers()
and assert it returns within a bounded timeout, preserving existing cleanup and
test behavior.
In `@pkg/controller/admissionchecks/multikueue/multikueuecluster.go`:
- Around line 422-424: Serialize watcher startup and shutdown in
updateConfigAndRefreshWatchers and StopWatchers: protect watchCancel access, the
watcher goroutine’s rc.watchers.Add(1) startup, and rc.watchers.Wait() with a
shared lifecycle barrier/lock. Ensure shutdown cannot complete while an overlap
callback is still able to register a watcher, while preserving existing
cancellation and watcher cleanup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 367ae6b9-3bc1-4dcf-8b12-29aa6be5e729
📒 Files selected for processing (2)
pkg/controller/admissionchecks/multikueue/multikueuecluster.gopkg/controller/admissionchecks/multikueue/multikueuecluster_test.go
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
| rc.watchers.Add(1) | ||
| go func() { | ||
| defer rc.watchers.Done() |
There was a problem hiding this comment.
On WaitGroup.Done, there's a comment:
Callers should prefer [WaitGroup.Go].
Which indeed seems nicely applicable in this case. WDYT?
There was a problem hiding this comment.
Done and it turns out this was blocking: verify-ci-lint was failing on exactly this (waitgroupgo: Goroutine creation can be simplified using WaitGroup.Go)
It folded neatly into CodeRabbit's concurrency point: the Add/Done pair became a single rc.watchers.Go(fn) inside a new runWatcher helper that also holds rc.mu, so starting a watcher and stopping the watchers are now mutually exclusive.
| defer rc.watchers.Done() | ||
| log.V(2).Info("Starting watch") | ||
| for r := range newWatcher.ResultChan() { | ||
| resultCh := newWatcher.ResultChan() |
There was a problem hiding this comment.
Nit: this var seems to have just 1 usage; maybe just inline?
| resultCh := newWatcher.ResultChan() | ||
| watching: | ||
| for { | ||
| var r watch.Event |
There was a problem hiding this comment.
Nit: I don't see a need for 2 vars r, ev.
Why not have case r, ok := <-resultCh and then do the whole handling (switch r.Type { ... }) inside that case?
There was a problem hiding this comment.
Done. The switch r.Type now lives inside the receive case and the pre-declared var r watch.Event is gone. break watching still exits the labelled loop from inside the select, so the control flow is unchanged.
| select { | ||
| case rc.wlUpdateCh <- event.GenericEvent{Object: localWl}: | ||
| case <-ctx.Done(): | ||
| } |
There was a problem hiding this comment.
As you're securing rc.wlUpdateCh and rc.watchEndedCh in this way, why not also rc.cqUpdateCh?
There was a problem hiding this comment.
Done. My reasoning for leaving it was that queueEventsForLQ runs on the cache's informer goroutines rather than the ones StopWatchers joins, so it could not wedge the join. That is still true, but it is the same unbounded-block bug and leaving two of three guarded is worse than fixing all three a reader would rightly wonder which rule applies.
I added new test alongside the other two.
WDYT?
There was a problem hiding this comment.
Hmm, actually, can you explain to me what's the purpose of guarding all these rc.xUpdateCh <- y with ctx.Done()?
Apologies, I'm only now realizing I don't get it. Because:
- If it's to prevent sending messages once
ctxis done:
It's speculative; theselectsemantics might still allow it if the channel buffer is not full. - If it's to prevent blocking forever on a
<-to a full channel (from which no-one will ever read):
I don't see such a threat; all theserc.xUpdateChare ultimately consumed by various controllers (via.WatchesRawSource(source.Channel(xUpdateCh, someHandler)))), which will still operate even when the watch context gets cancelled. So I don't see how blocking forever could happen.
Can you help me understand this? Or maybe are these guards all unnecessary?
(I'm not questioning the guard in establishWatcher, just the other ones).
There was a problem hiding this comment.
Good question, I added those guards thinking that it would prevent drainage, because of your comment, I actually went back and read through the controller-runtime properly.
My mental model was that these channels belong to the watch, so when the watch context was
cancelled I assumed the consumer side would go away with it and a send could be left hanging.
Your comment made me go back and verify it properly. I read source.Channel in vendor, and then
I removed the guards and re-ran the package suite under -race repeatedly to confirm nothing
regressed.
Reading it, syncLoop is the only reader of wlUpdateCh, watchEndedCh and cqUpdateCh, and
it runs on the manager's context rather than the watch context:
func (cs *channel[object, request]) syncLoop(ctx context.Context) {
for {
select {
case <-ctx.Done():
cs.doStop()
return
case evt, stillOpen := <-cs.source:
...
cs.distribute(evt)
}
}
}So cancelling a watch does not stop the drain at all. With the manager running those sends cannot block indefinitely.
Running it afterwards confirms nothing depended on them: the package suite is clean over 16 runs
of -race -count=3 with GORACE=log_path set, no failures and no race reports. The watch loop
keeps its ctx.Done() arm, since that is the one that actually fixes the race this PR is about.
you learn something new every day 😄
kubernetes-prow
Bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/admissionchecks/multikueue/multikueuecluster.go`:
- Around line 310-312: Synchronize the full remote-client refresh and cluster
removal/disconnection paths using the same lock: update setRemoteClientConfig,
stopAndRemoveCluster, and disconnectCluster so StopWatchers cannot clear
rc.watchCancel while updateConfigAndRefreshWatchers is still creating callbacks.
Preserve the connected state only after watcher callbacks are established,
preventing connState from remaining connected without running watchers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2886cf14-c3d3-4d49-8b7d-759e061b9bd3
📒 Files selected for processing (2)
pkg/controller/admissionchecks/multikueue/multikueuecluster.gopkg/controller/admissionchecks/multikueue/multikueuecluster_test.go
| var watchCancel context.CancelFunc | ||
| watchCtx, watchCancel = context.WithCancel(watchCtx) | ||
| rc.setWatchCancel(watchCancel) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== callers of updateConfigAndRefreshWatchers =="
rg -n -B5 -A5 'updateConfigAndRefreshWatchers\(' pkg/controller/admissionchecks/multikueue --glob '*.go'
echo "== callers of StopWatchers / stopAndRemoveCluster =="
rg -n -B5 -A5 '\.StopWatchers\(\)|stopAndRemoveCluster\(' pkg/controller/admissionchecks/multikueue --glob '*.go'
echo "== locking around remoteClients map / reconcile loop =="
rg -n -C6 'c\.lock\.(Lock|RLock)\(\)' pkg/controller/admissionchecks/multikueue --glob '*.go'Repository: kubernetes-sigs/kueue
Length of output: 19226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline pkg/controller/admissionchecks/multikueue/multikueuecluster.go --match remoteClient --view expanded || true
echo "== relevant multikueuecluster.go sections =="
sed -n '240,380p' pkg/controller/admissionchecks/multikueue/multikueuecluster.go
echo "---"
sed -n '540,620p' pkg/controller/admissionchecks/multikueue/multikueuecluster.go
echo "---"
sed -n '740,875p' pkg/controller/admissionchecks/multikueue/multikueuecluster.go
echo "== remoteClient definitions and watchers-related methods =="
rg -n -C4 'type remoteClient|setWatchCancel|StopWatchers|runWatcher|startWatcherCallback|startWatcher|markConnected|isConnected|markDisconnected|updateConfigLock|connState|watchCancel|watchCtx' pkg/controller/admissionchecks/multikueue/multikueuecluster.go
echo "== tests around concurrent stop/watchers =="
rg -n -C8 'StopWatchersJoinsParkedWatcher|QueueEventSendsUnblockOnCancel|StopWatchers|mlock|updateConfigAndRefreshWatchers' pkg/controller/admissionchecks/multikueue/multikueuecluster_test.goRepository: kubernetes-sigs/kueue
Length of output: 29601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Reconciler lock acquisition in reconcile flow =="
sed -n '850,895p' pkg/controller/admissionchecks/multikueue/multikueuecluster.go
echo "== Any reconcile loop lock around entire Reconcile =="
rg -n -C4 'c\.lock\.(Lock|Unlock|RLock|RUnlock)\(\)' pkg/controller/admissionchecks/multikueue/multikueuecluster.go | head -n 180
echo "== Test: Start/Reconcile/StopWatchers interleaving around updateConfigAndRefreshWatchers =="
rg -n -C10 'SetRemoteClientConfig|setRemoteClientConfig|hammerSetConfigWithReader|StopWatchersJoinsParkedWatcher|Concurrent|Reconcile' pkg/controller/admissionchecks/multikueue/multikueuecluster_test.go | head -n 260Repository: kubernetes-sigs/kueue
Length of output: 15239
Keep remote-client refresh and removal under the same synchronization.
setRemoteClientConfig() protects only updateConfigAndRefreshWatchers() via client.updateConfigLock, while stopAndRemoveCluster() and disconnectCluster() call rc.StopWatchers() after releasing c.lock. A deletion/disconnection can then clear rc.watchCancel while this refresh is still building the watch callbacks; each callback will skip runWatcher() once rc.watchCancel is nil, leaving connState marked connected with no watcher goroutines. Hold the same lock for the full refresh/removal path, or wait until watcher callbacks have started before calling markConnected().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/admissionchecks/multikueue/multikueuecluster.go` around lines
310 - 312, Synchronize the full remote-client refresh and cluster
removal/disconnection paths using the same lock: update setRemoteClientConfig,
stopAndRemoveCluster, and disconnectCluster so StopWatchers cannot clear
rc.watchCancel while updateConfigAndRefreshWatchers is still creating callbacks.
Preserve the connected state only after watcher callbacks are established,
preventing connState from remaining connected without running watchers.
|
/test pull-kueue-test-e2e-multikueue-sequential-shard-0-main |
| default: | ||
| wlKeys, err := w.WorkloadKeysFor(r.Object) | ||
| if err != nil { | ||
| log.Error(err, "Cannot get workload keys", "jobKind", r.Object.GetObjectKind().GroupVersionKind()) |
There was a problem hiding this comment.
should we rather return error to bounce back with another reconcile?
There was a problem hiding this comment.
I do not think so, but maybe I am wrong, there are mainly 3 reasons for this:
AFSIU, this runs inside the watcher goroutine rather than inside Reconcile, so there is no error return that would trigger a requeue, the only option would be to queue an event explicitly?
More importantly the failure is deterministic for the object in hand: every WorkloadKeysFor implementation fails either on a type assertion (not a workload, not a pod) or on a missing prebuilt-workload annotation. A retry would re-examine the same object and fail identically, so queueing a reconcile would spin rather than recover. And the watch event itself is not replayable, it has already been read off ResultChan(), and a reconcile does not redeliver it.
For context, this line is unchanged behaviour from main (multikueuecluster.go:440); it only moved in the diff because of the re-indent. If you would still like a reconcile queued on this path I am happy to do it.
Maybe a separate PR so it does not ride along with the race fix?
WDYT?
There was a problem hiding this comment.
For context, this line is unchanged behaviour from main (multikueuecluster.go:440); it only moved in the diff because of the re-indent. If you would still like a reconcile queued on this path I am happy to do it.
Ah, I see, let's keep it as is then and focus on the problem we have
| default: | ||
| log.V(3).Info("Watch error with unexpected type", "type", fmt.Sprintf("%T", s)) | ||
| resultCh := newWatcher.ResultChan() | ||
| watching: |
There was a problem hiding this comment.
Using labels in code is notoriously ticky to read, except maybe some rare cases. I'm not sure this is one of them. The code is intended so much (8 levels), that I would prefer rather calling the code as a function.
There was a problem hiding this comment.
I didn't want to add another function, but now that I read this again, I think you are right 😅, let me put it in a function
|
/approve |
|
@mimowo: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
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. |
kubernetes-prow
Bot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: aaab7c720f0e905978512c3d26c63f83cdbe9fcd |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewseif, mimowo 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 |
kubernetes-prow
Bot
added
the
approved
|
@mimowo: new pull request created: #13829 DetailsIn response to this:
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. |
|
@mimowo: #13705 failed to apply on top of branch "release-0.18": DetailsIn response to this:
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. |
|
@andrewseif please prepare a CP for 0.18 |
|
/release-note-edit |
kubernetes-prow
Bot
added
release-note
…ng their owner (#13839) * stop watcher goroutines from outliving their owner * apply comments * apply comments and remove not needed guards
What type of PR is this?
/kind bug
/kind flake
/area multikueue
What this PR does / why we need it:
The multikueue unit-test package fails intermittently under -race as a package-level FAIL that no individual test explains. The cause is a watcher goroutine that outlives the test that started it and logs through that test's logger.
establishWatcherspawns a goroutine that logs as its very first statement, and again afterits loop ends:
logcomes fromctrl.LoggerFrom(ctx), which under test is atestrlogger bound to*testing.T. A watcher still running after its test returns callst.Log()whiletRunneris writing to the same
testing.commonduring teardown. The racing memory is Go's ownstruct, which is why no Kueue-side test owns the failure.
This is an excerpt from the local logs
There are two independent defects here, and neither fix works without the other.
1.
StopWatcherscould not stop anything (multikueuecluster.go).It cancelled the watch context and returned. Cancellation is a request, not a join — and worse,
the watch loop never observed it, because cancelling the context does not close
newWatcher.ResultChan(). A watcher could therefore ignore cancellation indefinitely.ctx.Done()alongside the result channel.remoteClientgains async.WaitGroup; the starter closure registers before spawning, so aconcurrent
StopWatchersnever sees a zero counter while a goroutine is coming up.StopWatchersjoins the group after cancelling, outsiderc.mu— watcher goroutines takethat lock themselves via
getClient, so joining under it would deadlock.queueWorkloadEvent,queueWatchEndedEvent)now yield to cancellation instead of blocking forever. This is required, not tidying:
updateConfigAndRefreshWatcherscallsStopWatcherswhile its caller holdsupdateConfigLock, so a watcher blocked sending onwatchEndedChwould wait on a reconcilethat is itself waiting on the join.
The resulting ordering is what closes the race: cancel → loop exits → the trailing
"Watch ended"log runs → deferredDone()→Wait()returns. Every log the goroutine emitsnow completes before
StopWatchersreturns.2.
TestUpdateConfignever stopped anything (multikueuecluster_test.go).The subtest builds a reconciler, calls
Reconcile, which starts the watchers and returns.It never calls
StopWatchers, andutiltesting.ContextWithLoghands back no cancel func, sonothing tears the watchers down. A
t.Cleanupnow joins every remote client the subtestcreated.
TestRemoteClientGCalready useddefer rc.StopWatchers(); this bringsTestUpdateConfigin line.Which issue(s) this PR fixes:
Related #13290
Special notes for your reviewer:
I reproduced at the exact commit the failing CI run built,
d2a131a954b7ff703df5e87ec32902e3cedb00f9, taken from that run'sclone-log.txt. The tree wasmaterialised with
git archiveand verified by content hash against a fresh archive of the samecommit (identical SHA-256 manifest);
go mod verifypasses against that commit'sown
go.sum.few things I tried while testing this:
WaitGroupand the join deadlocked the unit suite, three watchergoroutines sat in
chan receivefor nine minutes until the package timed out. TheWaitGroupdid not cause that; it exposed goroutines that were already ignoringcancellation, unnoticed because nothing joined them. Hence the context-aware loop.
validation run. A join nothing calls changes nothing. Hence the test cleanup.
One last thing: the reproduction ran on linux/arm6, not on amd64.
Disclosure: this change was developed with AI assistance (Claude Code). The investigation, the change, and the verification were reviewed by me.
Does this PR introduce a user-facing change?
Summary by CodeRabbit