fix(missing-test): Node Lock Contention Retry & Timeout Test - #2494
fix(missing-test): Node Lock Contention Retry & Timeout Test#2494aniket866 wants to merge 7 commits into
Conversation
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aniket866 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNodeLock APIs now accept caller-provided contexts. Mutex acquisition, retries, release, stale-lock replacement, and try-lock operations handle cancellation and contention. Tests cover concurrency, failures, malformed annotations, nil contexts, and helper behavior. ChangesNodeLock context handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant LockNodeWithContext
participant TryLockNodeWithContext
participant KubernetesAPI
Scheduler->>LockNodeWithContext: request node lock with context
LockNodeWithContext->>TryLockNodeWithContext: attempt non-blocking lock
TryLockNodeWithContext->>KubernetesAPI: read and patch node annotation
KubernetesAPI-->>TryLockNodeWithContext: success or conflict
TryLockNodeWithContext-->>LockNodeWithContext: lock result
LockNodeWithContext-->>Scheduler: success, contention, or cancellation
Possibly related PRs
Suggested labels: 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: 4
🤖 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/util/nodelock/nodelock_test.go`:
- Around line 981-1000: Strengthen the SetNodeLockWithContext cancellation test
by recording the start time before launching the goroutine, requiring
errors.Is(err, context.DeadlineExceeded), and enforcing completion within a
small scheduling allowance after the 50 ms deadline instead of allowing two
seconds. Update the timeout assertion to fail if cancellation is not observed
promptly.
- Around line 1015-1041: Update the high-contention test around
LockNodeWithContext to create each pod fixture in clientSet before launching
goroutines, then synchronize their start so acquisitions compete concurrently.
Track successful and failed lock attempts while keeping the successful owner
lock held, and assert exactly one success; require every other result to equal
ErrNodeLockContention rather than only logging totalErrors.
In `@pkg/util/nodelock/nodelock.go`:
- Around line 321-327: Implement TryLockNodeWithContext as a nonblocking
operation instead of delegating to LockNodeWithContext: perform one immediate
per-node lock acquisition, return ErrNodeLockContention when unavailable, and
make only one node-lock API attempt without stale-lock replacement or retry
logic.
- Around line 129-132: Make per-node lock acquisition context-aware by adding or
reusing a helper that waits on the mutex while honoring context cancellation.
Update SetNodeLockWithContext, ReleaseNodeLockWithContext, and
LockNodeWithContext to use this helper instead of directly calling
sync.Mutex.Lock(), returning the context error when cancellation occurs; apply
the change at pkg/util/nodelock/nodelock.go lines 129-132 and 190-196.
🪄 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: c7bf07b4-b12f-4dae-9c1c-9239a28fe096
📒 Files selected for processing (3)
CHANGELOG.mdpkg/util/nodelock/nodelock.gopkg/util/nodelock/nodelock_test.go
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/util/nodelock/nodelock_test.go (1)
1084-1098: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest local mutex contention for
TryLockNode.The test only observes an existing node annotation. The previous blocking
LockNodeWithContextdelegation can satisfy these assertions.Hold
nodeLocks.getLock(nodeName)before the call. InvokeTryLockNodein a goroutine. Assert that it returnsErrNodeLockContentionwithin a short timeout.🤖 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/util/nodelock/nodelock_test.go` around lines 1084 - 1098, Update the TryLockNode contention test to exercise the local mutex: acquire nodeLocks.getLock(nodeName) before invoking TryLockNode, call TryLockNode in a goroutine, and assert it returns ErrNodeLockContention within a short timeout. Keep the existing node and pod setup only where needed, and release the mutex after the assertion.pkg/util/nodelock/nodelock.go (1)
162-170: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake retry backoff interruptible by context cancellation.
retry.OnErrorwaits withtime.Sleepbetween attempts, so checks in the callback do not stop the current backoff delay. WhenSetNodeLockWithContextorReleaseNodeLockWithContextis canceled, the node-lock RPC can still block for the remainder of that delay. Use a context-aware retry loop that selects onctx.Done()during backoff.
pkg/util/nodelock/nodelock.go#L162-L170pkg/util/nodelock/nodelock.go#L237-L245🤖 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/util/nodelock/nodelock.go` around lines 162 - 170, The retry loops in SetNodeLockWithContext (pkg/util/nodelock/nodelock.go:162-170) and ReleaseNodeLockWithContext (pkg/util/nodelock/nodelock.go:237-245) must make backoff cancellation-aware. Replace retry.OnError’s time.Sleep-based behavior with a context-aware retry loop that selects on ctx.Done() during each delay, while preserving the existing contention checks, retry strategy, and immediate cancellation error propagation.
🤖 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/util/nodelock/nodelock_test.go`:
- Around line 1043-1045: Update the lock-release path in the test around
ReleaseNodeLockWithContext to record and assert its error instead of discarding
it. After wg.Wait(), read the node and assert that NodeLockKey is absent,
ensuring the lock was cleanly released.
---
Outside diff comments:
In `@pkg/util/nodelock/nodelock_test.go`:
- Around line 1084-1098: Update the TryLockNode contention test to exercise the
local mutex: acquire nodeLocks.getLock(nodeName) before invoking TryLockNode,
call TryLockNode in a goroutine, and assert it returns ErrNodeLockContention
within a short timeout. Keep the existing node and pod setup only where needed,
and release the mutex after the assertion.
In `@pkg/util/nodelock/nodelock.go`:
- Around line 162-170: The retry loops in SetNodeLockWithContext
(pkg/util/nodelock/nodelock.go:162-170) and ReleaseNodeLockWithContext
(pkg/util/nodelock/nodelock.go:237-245) must make backoff cancellation-aware.
Replace retry.OnError’s time.Sleep-based behavior with a context-aware retry
loop that selects on ctx.Done() during each delay, while preserving the existing
contention checks, retry strategy, and immediate cancellation error propagation.
🪄 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: a86e4709-06f7-424d-98d7-7f095639ad96
📒 Files selected for processing (2)
pkg/util/nodelock/nodelock.gopkg/util/nodelock/nodelock_test.go
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: aniket866 <iamaniketkumarmaner@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/util/nodelock/test_helpers_test.go (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider delegating to the production accessor.
EnsureNodeLockForTestreimplements the lazy-creation logic of the internalgetLockaccessor. IfgetLockchanges its locking or initialization behavior, this helper drifts. CallnodeLocks.getLock(nodeName)instead to keep one source of truth.♻️ Proposed refactor
func EnsureNodeLockForTest(nodeName string) { - nodeLocks.mu.Lock() - if _, ok := nodeLocks.locks[nodeName]; !ok { - nodeLocks.locks[nodeName] = &sync.Mutex{} - } - nodeLocks.mu.Unlock() + _ = nodeLocks.getLock(nodeName) }Remove the
syncimport if it becomes unused.🤖 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/util/nodelock/test_helpers_test.go` around lines 28 - 34, Update EnsureNodeLockForTest to delegate directly to nodeLocks.getLock(nodeName) instead of duplicating lock initialization logic, and remove the sync import if it is no longer used.
🤖 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.
Nitpick comments:
In `@pkg/util/nodelock/test_helpers_test.go`:
- Around line 28-34: Update EnsureNodeLockForTest to delegate directly to
nodeLocks.getLock(nodeName) instead of duplicating lock initialization logic,
and remove the sync import if it is no longer used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17110f9c-09e4-4013-a8af-e867888ca9af
📒 Files selected for processing (2)
pkg/util/nodelock/nodelock_test.gopkg/util/nodelock/test_helpers_test.go
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
…n nodelock_test.go Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
…ontext test Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
|
@archlitchi @Eshiv-Pandey |
aniket866
changed the title
|
You can view the relevant rule here. |
2 participants
Closes #2492
Node Lock Contention Retry & Timeout Test
TryLockNode,LockNode,ReleaseNodeLockDefaultStrategy), combined with context cancellation / timeout.nodelock_test.gousing mock K8s client with simulated API delays to test lock contention, backoff exponential steps, and context cancellation behavior.Architecture Flow: Before vs After Test Coverage
flowchart LR subgraph Before["Before: Untested & Vulnerable Flow"] direction TB B_Lock["LockNode / TryLockNode\n(nodelock.go)"] B_Delay["Simulated API Latency / Contention"] B_Cancel["Context Timeout / Cancellation"] B_Leak["Goroutine Leak / Unreleased Lease"] B_Lock --> B_Delay B_Delay --> B_Cancel B_Cancel -->|Missing Cancel Guard| B_Leak end Before ==>|nodelock_test.go Backoff & Cancel Test| After subgraph After["After: Validated & Hardened Flow"] direction TB A_Lock["LockNode / TryLockNode\n(nodelock.go)"] A_Test["nodelock_test.go Mock Suite"] A_Cancel["Context Cancellation Signal"] A_Clean["Backoff Loop Terminated"] A_Release["Lease Cleanly Released"] A_Lock --> A_Test A_Test --> A_Cancel A_Cancel --> A_Clean A_Clean --> A_Release end classDef danger fill:#fee2e2,stroke:#ef4444,stroke-width:2px,color:#991b1b; classDef success fill:#dcfce7,stroke:#22c55e,stroke-width:2px,color:#166534; classDef neutral fill:#f3f4f6,stroke:#4b5563,stroke-width:1.5px,color:#1f2937; class B_Leak danger; class A_Clean,A_Release success; class B_Lock,B_Delay,B_Cancel,A_Lock,A_Test,A_Cancel neutral;o-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Bug Fixes
Tests