fix(scheduler): exponential backoff for acquireNodeLocks retries - #2594
fix(scheduler): exponential backoff for acquireNodeLocks retries#2594Brijesh-Thakkar wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Brijesh-Thakkar The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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)
📝 WalkthroughWalkthroughThe scheduler replaces fixed node-lock retry delays with deadline-bounded exponential backoff, jitter, logging, and a one-second cap. Tests and benchmarks cover timing, retry budgets, desynchronization, and concurrent contention. ChangesNode-lock retry backoff
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant acquireNodeLocks
participant node_lock_device
participant retry_backoff
acquireNodeLocks->>node_lock_device: Attempt node-lock acquisition
node_lock_device-->>acquireNodeLocks: Return contention error
acquireNodeLocks->>retry_backoff: Calculate jittered exponential delay
retry_backoff-->>acquireNodeLocks: Return deadline-bounded delay
retry_backoff-->>acquireNodeLocks: Wait or receive shutdown interruption
acquireNodeLocks->>node_lock_device: Retry acquisition
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 |
Replace fixed 100ms retry interval with exponential backoff + jitter, matching the DefaultStrategy pattern already used in nodelock.go. Reduces API-server patch volume ~3-4x under PodGroup contention at the cost of increased per-pod bind latency during active contention. See PR description for full benchmark tradeoff. Signed-off-by: Brijesh-Thakkar <brijeshthakkariitian5126@gmail.com>
7ecc495 to
6b54181
Compare
|
@archlitchi |
![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`:
- Around line 2499-2507: The contention benchmark mock must model lock ownership
by pod UID. In pkg/scheduler/scheduler_test.go lines 2499-2507, update
sharedLockMockDevice.LockNode and ReleaseNodeLock to record the acquiring pod’s
UID and only clear the holder when the releasing pod owns it; in lines
2483-2484, release the lock after each successful acquisition so benchmark pods
can complete.
🪄 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: c096ac99-0e15-436e-9b37-bf50be0e9660
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Implements exponential backoff + jitter for acquireNodeLocks contention retries (with a 1s cap), adds shutdown-aware waiting, and introduces unit/benchmark coverage to validate timing/budget/desync behavior.
Changes:
- Replace fixed 100ms retry sleep with exponential backoff (100ms → … → 1s cap) + jitter in
acquireNodeLocks - Add
s.stopCh-aware waiting and V=4 structured logging for contention retries - Add tests/benchmarks to validate cap, timeout budget adherence, and jitter desynchronization
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| pkg/scheduler/scheduler.go | Adds a reusable backoff policy and integrates it into acquireNodeLocks with jitter + shutdown-aware waiting |
| pkg/scheduler/scheduler_test.go | Adds timing-based tests for the new backoff behavior plus a contention benchmark |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -929,10 +940,17 @@ func (s *Scheduler) acquireNodeLocks(node *corev1.Node, pod *corev1.Pod) error { | |||
| return fmt.Errorf("timed out after %v waiting for node %s to be unlocked: %w", | |||
| config.NodeLockRetryTimeout, node.Name, nodelockutil.ErrNodeLockContention) | |||
| } | |||
| delay := backoff.Step() | |||
| if remaining := time.Until(deadline); delay > remaining { | |||
| delay = remaining | |||
| } | |||
There was a problem hiding this comment.
Good catch — fixed. The timeout check now uses time.Until(deadline) <= 0, and delay <= 0 is treated as an immediate timeout instead of falling through to a near-zero sleep. Verified with -race -count=10 with no regressions.
| select { | ||
| case <-s.stopCh: | ||
| return fmt.Errorf("scheduler shutting down while waiting for node lock: %w", nodelockutil.ErrNodeLockContention) | ||
| case <-time.After(100 * time.Millisecond): | ||
| case <-time.After(delay): | ||
| } |
There was a problem hiding this comment.
Fair point, but I'm leaving this as-is for this PR. time.After only allocates during active lock contention, not on the hot path, so the churn is bounded to retry scenarios. Happy to revisit with time.NewTimer/Reset if this becomes a concern, but I didn't want to add that complexity for a rarely-hit path in a small PR.
| } | ||
| results := make([]runResult, numRuns) | ||
|
|
||
| for i := range numRuns { |
There was a problem hiding this comment.
Verified that this repository targets Go 1.26.5, so range-over-integer is fully supported. No change needed.
| func BenchmarkAcquireNodeLocksContention(b *testing.B) { | ||
| for _, numPods := range []int{2, 4, 8, 16} { | ||
| b.Run(fmt.Sprintf("pods=%d", numPods), func(b *testing.B) { | ||
| for range b.N { |
| device.DevicesMap = map[string]device.Devices{"shared-lock-mock": mock} | ||
|
|
||
| done := make(chan struct{}) | ||
| for i := range numPods { |
| done <- struct{}{} | ||
| }() | ||
| } | ||
| for range numPods { |
| // With 50% jitter on a ~400ms base, the spread across 6 runs should be > 50ms. | ||
| require.Greater(t, spread, 50*time.Millisecond, | ||
| "retry intervals at gap %d should vary due to jitter, but spread was only %v (intervals: %v)", | ||
| checkIdx, spread, intervals) |
There was a problem hiding this comment.
Understand the concern with the wall-clock-based assertion. I ran TestAcquireNodeLocks_BackoffDesync 10x with -race -count=10, and it passed cleanly every time. The 50ms threshold has wide headroom over the ~400ms base with 50% jitter. I'm open to a deterministic RNG-injection approach if preferred, but that would be a larger refactor of the backoff configuration for this PR's scope.
github-actions
Bot
added
kind/bug
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Use time.Until(deadline) <= 0 instead of time.Now().After(deadline) for the timeout check, and treat delay <= 0 as an immediate timeout rather than allowing a near-zero sleep to continue the retry loop. Addresses review feedback from GitHub Copilot. Signed-off-by: Brijesh-Thakkar <brijeshthakkariitian5126@gmail.com>
Signed-off-by: Brijesh-Thakkar <brijeshthakkariitian5126@gmail.com>
|
This is being closed because it does not comply with the contribution guidelines. |
|
@mesutoezdil |
Summary
This PR changes the pod allocation lock retry strategy from a fixed 100ms delay to an exponential backoff with jitter.
Under contention, a fixed retry interval causes multiple pods to retry at the same time, increasing API-server load and creating unnecessary contention. Exponential backoff spreads retries over time, reducing synchronized retries and improving allocation latency as the number of contending pods increases.
Changes
Replace the fixed 100ms lock retry interval with exponential backoff.
Add jitter to retry delays to reduce synchronized retry behavior.
Preserve the existing lock ownership semantics and retry behavior.
Update the benchmark to accurately model lock acquisition and release under contention.
Benchmark Results
Update (post-review): An earlier version of the benchmark had a mock bug where losing pods could clear the winning pod's lock — this was caught by CodeRabbit. After fixing the lock-ownership semantics in the test mock, the corrected results confirm that this is a genuine latency improvement under realistic contention, rather than simply an API-load tradeoff.
Contending pods | Fixed 100ms (before) | Exp backoff (after) | Δ -- | -- | -- | -- 2 | 105.6 ms | 132.5 ms | +25% 4 | 306.5 ms | 255.0 ms | -17% 8 | 708.2 ms | 535.5 ms | -24% 16 | 1,511 ms | 1,163 ms | -23%The results show that while exponential backoff introduces a small overhead at very low contention (2 pods), it provides a significant latency reduction once contention becomes realistic (4+ pods), with improvements of approximately 17–24%.
Why
With a fixed retry interval, contending pods tend to retry simultaneously:
This creates synchronized bursts of lock requests.
With exponential backoff and jitter, retries become more distributed:
This reduces contention on the lock/API server and allows the winning pod to make progress with less interference.
Testing
Updated and validated the lock contention benchmark.
Fixed the benchmark mock to ensure only the lock owner can release the lock.
Verified behavior across multiple contention levels.
Confirmed the latency improvement is reproducible under 4+ contending pods.
Fixes #2589