fix(scheduler): stop leaked test informers to prevent data races - #2399
fix(scheduler): stop leaked test informers to prevent data races#2399v0idheaven wants to merge 1 commit into
Conversation
|
Welcome @v0idheaven! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughScheduler tests now use automatic cleanup to stop schedulers and informer factories. Several tests remove manual stop-channel setup. Bind-lock retry cleanup restores shared state and stops the scheduler. ChangesScheduler test cleanup
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: Poem
🚥 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: 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/scheduler/scheduler_test.go`:
- Line 368: Update the informer test cleanups in pkg/scheduler/scheduler_test.go
at lines 368, 480, 1312, 1382, 2098, and 2142 so each cleanup closes s.stopCh
and waits for all informer/event-handler goroutines to finish before returning.
Ensure handlers cannot be triggered after shutdown begins when they cannot exit
promptly.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fc5448f-5272-4ec6-9b22-d6b825e2b4de
📒 Files selected for processing (1)
pkg/scheduler/scheduler_test.go
Replace t.Cleanup(func() { close(s.stopCh) }) with t.Cleanup(informerFactory.Shutdown).
Closing stopCh signals goroutines to stop but does not wait for them to exit. informerFactory.Shutdown() both signals the stop and blocks until all event-handler goroutines have returned, ensuring no informer goroutine outlives the test that started it.
Also add informerFactory.Shutdown() to setupBindLockRetryTest's existing cleanup func alongside close(s.stopCh) for the same reason.
Addresses review feedback on Project-HAMi#2399.
|
Updated per review feedback from @coderabbitai. Changed from .Cleanup(func() { close(s.stopCh) }) to .Cleanup(informerFactory.Shutdown). Why this is better:
This ensures the informer goroutines are fully drained before the test function returns, preventing any possibility of them outliving the test and racing against subsequent tests. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git diff --unified=80 origin/"$(git remote show origin | sed -n '/HEAD branch/s/.*: //p')" -- pkg/scheduler/scheduler_test.go || true
printf '\n--- Relevant factory, startup, cleanup, and stop-channel sites ---\n'
rg -n -C 5 'NewSharedInformerFactory|\.Start\(|\.Shutdown\(|t\.Cleanup|stopCh' pkg/scheduler/scheduler_test.goLength of output: 47556 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Kubernetes dependency versions ---'
rg -n -C 2 'k8s.io/client-go|k8s.io/apimachinery' go.mod go.sum
printf '%s\n' '--- SharedInformerFactory Shutdown implementation ---'
module_dir="$(go env GOPATH 2>/dev/null)/pkg/mod"
if [ -d "$module_dir" ]; then
rg -n -U -C 12 'func \(.*\*sharedInformerFactory\) Shutdown\(\) \{' "$module_dir/k8s.io/client-go"* 2>/dev/null || true
else
echo "Go module cache is unavailable."
fiLength of output: 2041 🤖 web_search executed:
Length of output: 15243
With the current cleanup, the six affected tests can block in Use both operations in this order: t.Cleanup(func() {
close(s.stopCh)
informerFactory.Shutdown()
})This signals the informers to exit and waits until they have exited. The existing 🐇🔍 ✏️ Learnings added
You are interacting with an AI system. |
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/scheduler/scheduler_test.go`:
- Line 374: Update each test cleanup registering informerFactory.Shutdown to use
a closure that closes s.stopCh before calling informerFactory.Shutdown. Apply
this at all corresponding cleanup sites, preserving the existing shutdown
behavior while ensuring informer goroutines can exit.
- Line 2220: In setupBindLockRetryTest, declare informerFactory before defining
the cleanup closure so cleanup’s informerFactory.Shutdown() reference is in
scope; preserve the existing cleanup behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7a9fc47-336d-4fc5-9f67-b637f71f1bc6
📒 Files selected for processing (1)
pkg/scheduler/scheduler_test.go
|
@v0idheaven wlcm to the community. pls sign off your commits dco check is failing. |
Eshiv-Pandey
left a comment
Eshiv-Pandey
left a comment
There was a problem hiding this comment.
this still regresses. Shutdown() only waits for goroutines it doesnt stop them. The stop signal is close(s.stopCh), which these six tests never call, so Shutdown() blocks until the test times out. Like setupBindLockRetryTest, use:
t.Cleanup(func() {
close(s.stopCh)
informerFactory.Shutdown()
})
a 12x-race loop should hang on the first run with the current changes.
Tests that call informerFactory.Start(s.stopCh) without closing stopCh leave informer goroutines running after the test ends. These goroutines keep dispatching pod events into onAddPod, which reads device.SupportDevices — a package-level map with no mutex — causing data races under -race. Add t.Cleanup(s.Stop) to each test that starts an informer factory but never stops it. s.Stop() closes s.stopCh, which is the stop signal the informer goroutines watch, causing them to exit cleanly. Also remove redundant s.stopCh reassignments in Test_RegisterFromNodeAnnotations and Test_RegisterFromNodeAnnotations_NIL since NewScheduler() already initialises the channel. In setupBindLockRetryTest, replace the manual close(s.stopCh) in the existing cleanup closure with s.Stop() for consistency. Affected tests: - Test_getPodUsage - Test_Filter - TestRegisterSkipsCleanupForUntrackedVendor - Test_ResourceQuota - Test_Bind_DelPodOnGetPodFailure - Test_Bind_DelPodOnGetNodeFailure - setupBindLockRetryTest Fixes Project-HAMi#2389 Signed-off-by: v0idheaven <dahiyavarun2007@gmail.com>
85e959a to
02e4b90
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Thanks for the review @Eshiv-Pandey! Updated the fix:
Let me know if anything else needs changing! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, v0idheaven 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 |
|
please sync with master to pass the CI |
|
/lgtm cancel |
| */ | ||
| func Test_Filter(t *testing.T) { | ||
| s := NewScheduler() | ||
| t.Cleanup(s.Stop) |
There was a problem hiding this comment.
@v0idheaven I test this PR with the following commands
go test ./pkg/scheduler/ \
-run '^(Test_Filter|Test_onAddPod_BadDeviceAnnotation)$' \
-count=1 -raceAnd here comes an error
E0809 23:37:49.760989 973437 scheduler.go:162] "failed to decode pod devices" err="pod annotation format error, missing fields, do not use nodeName in task spec" pod="default/bad-anno-pod"
--- FAIL: Test_onAddPod_BadDeviceAnnotation (0.00s)
testing.go:1712: race detected during execution of testIs this an expected error?
|
You can view the relevant rule here. |
5 participants
What this PR does
Fixes #2389
Tests that start informers via
informerFactory.Start(s.stopCh)but never closestopChleave goroutines running after the test completes. These leaked informer goroutines keep dispatching pod events intoonAddPod, which readsdevice.SupportDevicesa package-levelmap[string]stringwith no mutex, causing unsynchronised concurrent map access and intermittent-racefailures.The root cause:
device.SupportDevices["test"] = "hami.io/test-allocated"informerFactory.Start(s.stopCh)and never closestopCh, the listener goroutines outlive the test that created them and keep dispatching pod events intoonAddPodfor the rest of the binary's lifeChanges
Added
t.Cleanup(func() { close(s.stopCh) })to every test that starts an informer without a corresponding stop:Test_getPodUsageTest_FilterTestRegisterSkipsCleanupForUntrackedVendorTest_ResourceQuotaTest_Bind_DelPodOnGetPodFailureTest_Bind_DelPodOnGetNodeFailureAlso removed redundant
s.stopCh = make(chan struct{})reassignments inTest_RegisterFromNodeAnnotationsandTest_RegisterFromNodeAnnotations_NIL,NewScheduler()already initialises the channel, so recreating it orphans the original.How to verify
Should pass consistently. Before this fix it fails on attempt 3-4 out of 12 on average (reproduced with
go test ./pkg/scheduler/ --raceon master).Summary by CodeRabbit