feat(scheduler): retry NodeLock in Bind for PodGroup members - #2066
Conversation
📝 WalkthroughWalkthroughAdds PodGroup-aware node-lock retry configuration and centralized lock handling during scheduler binding. Contention is now programmatically identifiable, with tests covering retries, timeout cleanup, and non-contention errors. ChangesPodGroup node-lock retry
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Bind
participant Scheduler
participant DevicesMap
participant NodeLock
Bind->>Scheduler: acquireNodeLocks(node, pod)
Scheduler->>DevicesMap: LockNode for each device
DevicesMap->>NodeLock: attempt node lock
NodeLock-->>Scheduler: contention or success
Scheduler->>DevicesMap: release locks before retry
Scheduler->>DevicesMap: retry until timeout
Scheduler-->>Bind: return result
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Add ErrNodeLockContention sentinel and retry loop in Bind (--node-lock-retry-timeout, default 28s) for PodGroup pods. Non-PodGroup behavior unchanged. Signed-off-by: lin121291 <4jp33f9e@gmail.com>
07ba898 to
637b07c
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a retry mechanism for node locks when contended by another PodGroup member, allowing configurable timeout and retry behavior. It adds the NodeLockRetryTimeout configuration, helper functions to identify PodGroup members and node lock contentions, and comprehensive unit tests. Feedback on the changes highlights two issues in the retry loop: a potential partial lock leak if a non-contention error or timeout occurs mid-loop, and the blocking of scheduler shutdown due to time.Sleep. A code suggestion was provided to release locks immediately on failure and use a select block with s.stopCh instead of a direct sleep.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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
🧹 Nitpick comments (4)
pkg/scheduler/scheduler_test.go (2)
1871-1905: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
cleanup()never stops the informer factory / closess.stopCh.
informerFactory.Start(s.stopCh)spins up background goroutines, butcleanup()only restoresconfig.NodeLockRetryTimeoutanddevice.DevicesMap—s.stopChis never closed. Each of the 4 new tests leaks its informer goroutines for the remainder of the test binary's life.♻️ Proposed fix
cleanup := func() { config.NodeLockRetryTimeout = oldRetry device.DevicesMap = oldDevicesMap + close(s.stopCh) }🤖 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/scheduler/scheduler_test.go` around lines 1871 - 1905, Update setupBindLockRetryTest’s cleanup function to stop the scheduler’s informer goroutines by closing or otherwise shutting down s.stopCh, while preserving the existing config.NodeLockRetryTimeout and device.DevicesMap restoration. Ensure cleanup remains safe for each test invocation.
1848-1854: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLint failure: use
atomic.Int32instead of manualint32+sync/atomiccalls.Static analysis reports a lint failure on
lockCalls/releaseCalls. Go 1.19+'ssync/atomic.Int32type enforces atomic-only access and removes the need for explicitatomic.AddInt32/atomic.LoadInt32calls.🔧 Proposed fix using atomic.Int32
type bindLockMockDevice struct { registerMockDevice lockErr error lockErrOnce bool - lockCalls int32 - releaseCalls int32 + lockCalls atomic.Int32 + releaseCalls atomic.Int32 } func (m *bindLockMockDevice) CommonWord() string { return "bind-lock-mock" } func (m *bindLockMockDevice) LockNode(_ *corev1.Node, _ *corev1.Pod) error { - n := atomic.AddInt32(&m.lockCalls, 1) + n := m.lockCalls.Add(1) if m.lockErr != nil && (!m.lockErrOnce || n == 1) { return m.lockErr } return nil } func (m *bindLockMockDevice) ReleaseNodeLock(_ *corev1.Node, _ *corev1.Pod) error { - atomic.AddInt32(&m.releaseCalls, 1) + m.releaseCalls.Add(1) return nil }(callers would then use
mock.lockCalls.Load()instead ofatomic.LoadInt32(&mock.lockCalls))🤖 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/scheduler/scheduler_test.go` around lines 1848 - 1854, Update bindLockMockDevice to declare lockCalls and releaseCalls as sync/atomic.Int32 values instead of int32 fields, then replace all corresponding atomic.AddInt32 and atomic.LoadInt32 usages with the fields’ Add and Load methods while preserving existing call-count behavior.Source: Linters/SAST tools
pkg/scheduler/scheduler.go (1)
760-781: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed-interval retry with no jitter risks thundering-herd contention.
Every contending PodGroup member retries every 100ms in lockstep with no jitter/backoff. Since issue
#1832's scenario is specifically many gang members binding concurrently, this pattern means all losers wake and re-collide on the same node lock simultaneously, which can itself prolong contention rather than resolve it.Consider adding jitter (e.g.
100ms + rand(0, 50ms)) or a small exponential backoff capped below the retry deadline.♻️ Proposed jitter fix
- s.releaseAllDevices(node, pod) - time.Sleep(100 * time.Millisecond) + s.releaseAllDevices(node, pod) + time.Sleep(100*time.Millisecond + time.Duration(rand.Intn(50))*time.Millisecond)🤖 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/scheduler/scheduler.go` around lines 760 - 781, Update acquireNodeLocks to avoid synchronized retries by replacing the fixed 100ms sleep with a jittered delay (for example, 100ms plus a random delay up to 50ms) or a small exponential backoff capped by the remaining NodeLockRetryTimeout. Preserve the existing contention detection, cleanup, timeout, and successful lock acquisition behavior.cmd/scheduler/main.go (1)
79-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo safeguard against
--node-lock-retry-timeoutexceeding the extender's actualhttpTimeout.The flag's help text documents that operators must align this with the extender's
httpTimeout, but nothing enforces or warns about it at startup. If misconfigured (retry timeout ≥ extender timeout), the extender's HTTP client can time out mid-retry, while the scheduler goroutine keeps spinning inacquireNodeLocks— kube-scheduler may then treat the bind as failed and reschedule while the original attempt is still contending for the lock.Consider logging a warning if
NodeLockRetryTimeoutexceeds some conservative bound (e.g. via a related--http-timeoutflag or a hardcoded safety margin), so misconfiguration surfaces early rather than manifesting as intermittent duplicate-bind races.🤖 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 `@cmd/scheduler/main.go` around lines 79 - 80, In the scheduler startup flow that registers and validates `config.NodeLockRetryTimeout`, add an early warning when this duration exceeds the extender HTTP timeout or its established conservative bound. Reuse the existing logger and timeout configuration symbols, and make the warning clearly identify both values while preserving the current flag defaults and lock-acquisition behavior.
🤖 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.go`:
- Around line 754-758: Update Scheduler.releaseAllDevices to capture the error
returned by each val.ReleaseNodeLock(node, pod) call and log release failures
with sufficient context for operators to identify the affected node, pod, and
device. Preserve releasing every device and the existing retry behavior.
---
Nitpick comments:
In `@cmd/scheduler/main.go`:
- Around line 79-80: In the scheduler startup flow that registers and validates
`config.NodeLockRetryTimeout`, add an early warning when this duration exceeds
the extender HTTP timeout or its established conservative bound. Reuse the
existing logger and timeout configuration symbols, and make the warning clearly
identify both values while preserving the current flag defaults and
lock-acquisition behavior.
In `@pkg/scheduler/scheduler_test.go`:
- Around line 1871-1905: Update setupBindLockRetryTest’s cleanup function to
stop the scheduler’s informer goroutines by closing or otherwise shutting down
s.stopCh, while preserving the existing config.NodeLockRetryTimeout and
device.DevicesMap restoration. Ensure cleanup remains safe for each test
invocation.
- Around line 1848-1854: Update bindLockMockDevice to declare lockCalls and
releaseCalls as sync/atomic.Int32 values instead of int32 fields, then replace
all corresponding atomic.AddInt32 and atomic.LoadInt32 usages with the fields’
Add and Load methods while preserving existing call-count behavior.
In `@pkg/scheduler/scheduler.go`:
- Around line 760-781: Update acquireNodeLocks to avoid synchronized retries by
replacing the fixed 100ms sleep with a jittered delay (for example, 100ms plus a
random delay up to 50ms) or a small exponential backoff capped by the remaining
NodeLockRetryTimeout. Preserve the existing contention detection, cleanup,
timeout, and successful lock acquisition 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5c93dc6-e6af-43ad-8ca1-fb110e11e296
📒 Files selected for processing (7)
cmd/scheduler/main.gopkg/scheduler/config/config.gopkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.gopkg/util/nodelock/nodelock.gopkg/util/types.gopkg/util/util.go
- Release partial locks before returning on non-contention error or timeout - Replace time.Sleep with select on stopCh for graceful shutdown - Use atomic.Int32 in tests (modernize linter) Signed-off-by: lin121291 <4jp33f9e@gmail.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: lin121291 <4jp33f9e@gmail.com>
…e leak Signed-off-by: lin121291 <4jp33f9e@gmail.com>
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`:
- Around line 1879-1883: Update setupBindLockRetryTest cleanup to preserve and
restore the original client.KubeClient value, alongside the existing global
state restoration. Capture the value before the test overwrites it, then reset
client.KubeClient in cleanup before closing s.stopCh.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dcc98415-705e-42a8-80e1-f1223aaf5141
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/scheduler/scheduler.go
|
Thanks @mesutoezdil for the review. On the scope of
The other two fixes also turn out to be already covered by existing self-healing paths in the scheduler ( I'll update issue #1832 to reflect this narrower scope with the gist evidence linked, then this PR can stay as |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, lin121291, mesutoezdil 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 |
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
When multiple members of the same PodGroup are bound concurrently to
the same node, they contend on the
hami.io/mutex.lockannotation.The losing pod fails immediately and waits for kube-scheduler backoff
(~2s). This PR adds a retry loop in
Bindso PodGroup pods wait forthe lock to clear (~22ms on real hardware with a device plugin) instead.
ErrNodeLockContentionsentinel innodelockpackageIsPodGroupMember()based onscheduling.x-k8s.io/pod-grouplabel--node-lock-retry-timeoutflag (default 28s, align with extenderhttpTimeoutin KubeSchedulerConfiguration)Which issue(s) this PR fixes:
Fixes #1832
Special notes for your reviewer:
The retry timeout (28s) should be less than the extender
httpTimeoutconfigured in KubeSchedulerConfiguration (HAMi chart default: 30s).
Does this PR introduce a user-facing change?:
Yes. New CLI flag
--node-lock-retry-timeout(default 28s). No breaking changes.Summary by CodeRabbit
--node-lock-retry-timeoutto configure how long the scheduler retries node locks for grouped (PodGroup) pods (28s default;0disables).