fix(nodelock): handle empty lock annotations#1727
Conversation
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: maishivamhoo123 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 |
![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 implements in-memory node locking within the scheduler's Bind function and transitions to "blind patches" to mitigate Kubernetes API 409 conflicts during high-concurrency pod submissions. Feedback highlights a critical deadlock risk where the same mutex is acquired multiple times during the binding process, as well as redundant package imports. Furthermore, the LockNode logic was found to be inefficient by performing unnecessary API calls when a node is already locked, and the new concurrency test requires updates to properly validate node state transitions.
| nodelock.LockNodeMemory(args.Node) | ||
| defer nodelock.UnlockNodeMemory(args.Node) // Safely releases even on goto ReleaseNodeLocks |
There was a problem hiding this comment.
This call to nodelock.LockNodeMemory(args.Node) acquires a per-node mutex. However, the subsequent call to val.LockNode (line 687) eventually calls nodelock.SetNodeLock, which attempts to acquire the same mutex. Since Go's sync.Mutex is not reentrant, this will result in a deadlock for every pod binding attempt. To implement in-memory queuing safely, you should either use a separate mutex for the queuing phase or refactor the nodelock package to allow internal calls without re-acquiring the lock.
| "github.com/Project-HAMi/HAMi/pkg/util/nodelock" | ||
| nodelockutil "github.com/Project-HAMi/HAMi/pkg/util/nodelock" |
There was a problem hiding this comment.
The package github.com/Project-HAMi/HAMi/pkg/util/nodelock is now imported twice: once with the default name and once with the alias nodelockutil. This is redundant and inconsistent. Please remove the new import and use the existing nodelockutil alias for the new calls in the Bind function.
| } | ||
|
|
||
| return fmt.Errorf("node %s has been locked within %v", nodeName, NodeLockTimeout) | ||
| return SetNodeLock(nodeName, lockname, pods) |
There was a problem hiding this comment.
This change causes LockNode to call SetNodeLock even when the node is already known to be locked and the lock has not expired. SetNodeLock will perform an unnecessary API Get call and attempt to acquire the mutex (which causes a deadlock if called from Bind) only to return the same error message. It is more efficient to return the error directly as in the previous version.
| return SetNodeLock(nodeName, lockname, pods) | |
| return fmt.Errorf("node %s has been locked within %v", nodeName, NodeLockTimeout) |
| fakeClient.PrependReactor("patch", "nodes", func(action core.Action) (bool, k8sruntime.Object, error) { | ||
| atomic.AddInt32(&patchCount, 1) | ||
|
|
||
| // Simulate network latency. If the in-memory sync.Mutex isn't working, | ||
| // these concurrent overlapping calls would normally fail or race. | ||
| time.Sleep(5 * time.Millisecond) | ||
|
|
||
| return true, nil, nil // Return true to indicate we handled the patch successfully | ||
| }) |
There was a problem hiding this comment.
The patch reactor returns true, nil, nil without updating the node's state in the fake client's object tracker. Consequently, every concurrent call to LockNode in the test will see the node as unlocked because the annotation is never actually added to the node object. This means the test is not verifying the logic's ability to block pods when a lock exists; it only verifies that the Patch calls are serialized by the mutex.
There was a problem hiding this comment.
- Please fix the lint error. https://github.com/Project-HAMi/HAMi/actions/runs/23972391100/job/69924021159?pr=1727
- Please take a look at Gemini's comments. Some of them might be very useful
hi @maishivamhoo123 Have you tested whether adding mutex lock in the bind phase can effectively mitigate this problem? |
Shouren
left a comment
Shouren
left a comment
There was a problem hiding this comment.
@maishivamhoo123 Please solve the golang-ci lint issues first
3 participants
Description:
This PR resolves the 15-30 minute scheduling delays during high-concurrency pod submissions and fixes time.Parse panics during node locking.
Key Code Changes:
In-Memory Queuing (scheduler.go & nodelock.go): Added LockNodeMemory() and applied it directly inside the scheduler's Bind function. This queues concurrent pods locally instead of instantly rejecting them, bypassing the kube-scheduler's 5-minute exponential backoff trap.
Blind API Patching (nodelock.go): Removed resourceVersion from the JSON patch payloads in SetNodeLock and ReleaseNodeLock. This safely updates the Node annotation for the Device Plugin handshake while eliminating HTTP 409 Resource Conflict storms.
Empty Annotation Fix (nodelock.go): Added a safeguard (if !ok || lockVal == "") in LockNode to prevent panics when attempting to parse an empty string as a time value.
Test Fix (nodelock.go): Corrected the timeout error message string in SetNodeLock to satisfy existing unit test assertions.
New Concurrency Test (nodelock_test.go): Added TestLockNode_ConcurrentPatchSimulation to explicitly verify that a burst of 40+ pods processes successfully without triggering K8s API conflicts or backoff errors.