diff --git a/CHANGELOG.md b/CHANGELOG.md index 42ae5a6824..e4dd5d704c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -408,5 +408,7 @@ Add "NVIDIA_VISIBLE_DEVICES=none" to none-gpu tasks - Fix initialization error when using tensor parallelism on vLLM above 0.18 - Fix multiple device typos - Add unit test coverage for node discovery handshake parsing with malformed or empty annotations +- Add context cancellation handling and unit test coverage for NodeLock backoff retries and lock contention (`SetNodeLockWithContext`, `LockNodeWithContext`, `ReleaseNodeLockWithContext`, `TryLockNode`) + diff --git a/pkg/util/nodelock/nodelock.go b/pkg/util/nodelock/nodelock.go index e0ed834eac..f0867693c0 100644 --- a/pkg/util/nodelock/nodelock.go +++ b/pkg/util/nodelock/nodelock.go @@ -126,13 +126,31 @@ func setupNodeLockTimeout() { } } -func SetNodeLock(nodeName string, lockname string, pods *corev1.Pod) error { +// lockMutexWithContext attempts to acquire mu while respecting ctx cancellation. +func lockMutexWithContext(ctx context.Context, mu *sync.Mutex) error { + for { + if mu.TryLock() { + return nil + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(5 * time.Millisecond): + } + } +} + +func SetNodeLockWithContext(ctx context.Context, nodeName string, lockname string, pods *corev1.Pod) error { + if ctx == nil { + ctx = context.Background() + } // Acquire per-node lock instead of global lock nodeLock := nodeLocks.getLock(nodeName) - nodeLock.Lock() + if err := lockMutexWithContext(ctx, nodeLock); err != nil { + return err + } defer nodeLock.Unlock() - ctx := context.Background() node, err := client.GetClient().CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) if err != nil { return err @@ -142,8 +160,14 @@ func SetNodeLock(nodeName string, lockname string, pods *corev1.Pod) error { } lockOwner := NodeLockSep + GeneratePodNamespaceName(pods, NodeLockSep) err = retry.OnError(DefaultStrategy, func(err error) bool { + if ctx.Err() != nil { + return false + } return !IsNodeLockContention(err) }, func() error { + if err := ctx.Err(); err != nil { + return err + } node, err = client.GetClient().CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) if err != nil { klog.ErrorS(err, "Failed to get node when retry to patch", "node", nodeName) @@ -165,6 +189,9 @@ func SetNodeLock(nodeName string, lockname string, pods *corev1.Pod) error { return nil }) if err != nil { + if ctx.Err() != nil { + return ctx.Err() + } return fmt.Errorf("failed to set node lock (node=%s, retry strategy=%+v): %w", nodeName, DefaultStrategy, err) } @@ -172,16 +199,24 @@ func SetNodeLock(nodeName string, lockname string, pods *corev1.Pod) error { return nil } -func ReleaseNodeLock(nodeName string, lockname string, pod *corev1.Pod, skipNodeLockOwnerCheck bool) error { +func SetNodeLock(nodeName string, lockname string, pods *corev1.Pod) error { + return SetNodeLockWithContext(context.Background(), nodeName, lockname, pods) +} + +func ReleaseNodeLockWithContext(ctx context.Context, nodeName string, lockname string, pod *corev1.Pod, skipNodeLockOwnerCheck bool) error { if pod == nil { return fmt.Errorf("cannot release node lock: pod is nil") } + if ctx == nil { + ctx = context.Background() + } // Acquire per-node lock instead of global lock nodeLock := nodeLocks.getLock(nodeName) - nodeLock.Lock() + if err := lockMutexWithContext(ctx, nodeLock); err != nil { + return err + } defer nodeLock.Unlock() - ctx := context.Background() node, err := client.GetClient().CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) if err != nil { return err @@ -200,9 +235,15 @@ func ReleaseNodeLock(nodeName string, lockname string, pod *corev1.Pod, skipNode released := false err = retry.OnError(DefaultStrategy, func(err error) bool { + if ctx.Err() != nil { + return false + } // Retry on any error return true }, func() error { + if err := ctx.Err(); err != nil { + return err + } node, err = client.GetClient().CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) if err != nil { klog.ErrorS(err, "Failed to get node when retry to patch", "node", nodeName) @@ -229,6 +270,9 @@ func ReleaseNodeLock(nodeName string, lockname string, pod *corev1.Pod, skipNode return nil }) if err != nil { + if ctx.Err() != nil { + return ctx.Err() + } return fmt.Errorf("failed to release node lock (node=%s, retry strategy=%+v): %w", nodeName, DefaultStrategy, err) } @@ -238,14 +282,20 @@ func ReleaseNodeLock(nodeName string, lockname string, pod *corev1.Pod, skipNode return nil } -func LockNode(nodeName string, lockname string, pods *corev1.Pod) error { - ctx := context.Background() +func ReleaseNodeLock(nodeName string, lockname string, pod *corev1.Pod, skipNodeLockOwnerCheck bool) error { + return ReleaseNodeLockWithContext(context.Background(), nodeName, lockname, pod, skipNodeLockOwnerCheck) +} + +func LockNodeWithContext(ctx context.Context, nodeName string, lockname string, pods *corev1.Pod) error { + if ctx == nil { + ctx = context.Background() + } node, err := client.GetClient().CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) if err != nil { return err } if _, ok := node.Annotations[NodeLockKey]; !ok { - return SetNodeLock(nodeName, lockname, pods) + return SetNodeLockWithContext(ctx, nodeName, lockname, pods) } lockTime, ns, previousPodName, err := ParseNodeLock(node.Annotations[NodeLockKey]) if err != nil { @@ -257,17 +307,9 @@ func LockNode(nodeName string, lockname string, pods *corev1.Pod) error { klog.InfoS("Node lock expired", "node", nodeName, "lockTime", lockTime, "timeout", NodeLockTimeout) skipOwnerCheck = true } else if ns == pods.Namespace && previousPodName == pods.Name { - // The lock is already held by this exact pod. lockAllDevices calls - // LockNode once per device vendor a pod requests resources from, so - // a pod requesting resources from two or more vendors (e.g. both - // nvidia.com/gpu and cambricon.com/vmlu) would otherwise contend - // with its own still-valid lock on the second call and never - // become schedulable. Treat this as already acquired. klog.V(4).InfoS("Node lock already held by this pod, treating as acquired", "node", nodeName, "podName", pods.Name) return nil - } else - // Check dangling nodeLock - if ns != "" && previousPodName != "" { + } else if ns != "" && previousPodName != "" { if _, err := client.GetClient().CoreV1().Pods(ns).Get(ctx, previousPodName, metav1.GetOptions{}); err != nil { if !apierrors.IsNotFound(err) { klog.ErrorS(err, "Failed to get pod of NodeLock", "podName", previousPodName, "namespace", ns) @@ -279,17 +321,55 @@ func LockNode(nodeName string, lockname string, pods *corev1.Pod) error { } if skipOwnerCheck { - err = ReleaseNodeLock(nodeName, lockname, pods, true) + err = ReleaseNodeLockWithContext(ctx, nodeName, lockname, pods, true) if err != nil { klog.ErrorS(err, "Failed to release node lock", "node", nodeName) return err } - return SetNodeLock(nodeName, lockname, pods) + return SetNodeLockWithContext(ctx, nodeName, lockname, pods) } return fmt.Errorf("node %s has been locked within %v: %w", nodeName, NodeLockTimeout, ErrNodeLockContention) } +func LockNode(nodeName string, lockname string, pods *corev1.Pod) error { + return LockNodeWithContext(context.Background(), nodeName, lockname, pods) +} + +func TryLockNode(nodeName string, lockname string, pods *corev1.Pod) error { + return TryLockNodeWithContext(context.Background(), nodeName, lockname, pods) +} + +func TryLockNodeWithContext(ctx context.Context, nodeName string, lockname string, pods *corev1.Pod) error { + if ctx == nil { + ctx = context.Background() + } + nodeLock := nodeLocks.getLock(nodeName) + if !nodeLock.TryLock() { + return fmt.Errorf("node %s is locked: %w", nodeName, ErrNodeLockContention) + } + defer nodeLock.Unlock() + + node, err := client.GetClient().CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) + if err != nil { + return err + } + if _, ok := node.Annotations[NodeLockKey]; ok { + return fmt.Errorf("node %s is locked: %w", nodeName, ErrNodeLockContention) + } + + patchData := fmt.Sprintf(`{"metadata":{"annotations":{"%s":"%s"},"resourceVersion":"%s"}}`, NodeLockKey, GenerateNodeLockKeyByPod(pods), node.ResourceVersion) + _, err = client.GetClient().CoreV1().Nodes().Patch(ctx, nodeName, types.MergePatchType, []byte(patchData), metav1.PatchOptions{}) + if err != nil { + if apierrors.IsConflict(err) { + return fmt.Errorf("node %s patch conflict: %w", nodeName, ErrNodeLockContention) + } + return err + } + klog.InfoS("Node lock set via TryLock", "node", nodeName, "podName", pods.Name) + return nil +} + func ParseNodeLock(value string) (lockTime time.Time, ns, name string, err error) { if !strings.Contains(value, NodeLockSep) { lockTime, err = time.Parse(time.RFC3339, value) diff --git a/pkg/util/nodelock/nodelock_test.go b/pkg/util/nodelock/nodelock_test.go index 0019e9860d..362c340f23 100644 --- a/pkg/util/nodelock/nodelock_test.go +++ b/pkg/util/nodelock/nodelock_test.go @@ -19,6 +19,7 @@ package nodelock import ( "context" "errors" + "fmt" "runtime" "strings" "sync" @@ -957,3 +958,619 @@ func TestSetupNodeLockTimeout(t *testing.T) { }) } } + +func Test_NodeLock_ContentionBackoffAndTimeout(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "contention-node" + clientSet := fake.NewClientset(&corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName, Annotations: map[string]string{}}, + }) + client.KubeClient = clientSet + + t.Run("Context Cancellation Halts Retry Backoff", func(t *testing.T) { + nodeLocks = newNodeLockManager() + clientSet := fake.NewClientset(&corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName, Annotations: map[string]string{}}, + }) + client.KubeClient = clientSet + + // Prepend a reactor to patch that simulates persistent transient API failure + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated transient API error") + }) + + start := time.Now() + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod-waiter", Namespace: "ns"}} + errChan := make(chan error, 1) + + go func() { + errChan <- SetNodeLockWithContext(ctx, nodeName, "", pod) + }() + + select { + case err := <-errChan: + elapsed := time.Since(start) + if err == nil { + t.Fatalf("Expected error due to context cancellation, got nil") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Expected context.DeadlineExceeded during retries, got %v", err) + } + if elapsed > 300*time.Millisecond { + t.Fatalf("Cancellation was not observed promptly: took %v (expected ~50ms)", elapsed) + } + case <-time.After(500 * time.Millisecond): + t.Fatalf("SetNodeLockWithContext hung after context deadline") + } + }) + + t.Run("High Lock Contention Goroutine Safety", func(t *testing.T) { + nodeLocks = newNodeLockManager() + clientSet = fake.NewClientset(&corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName, Annotations: map[string]string{}}, + }) + client.KubeClient = clientSet + + numGoroutines := 10 + pods := make([]*corev1.Pod, numGoroutines) + for i := range numGoroutines { + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("pod-%d", i), Namespace: "ns"}} + _, err := clientSet.CoreV1().Pods("ns").Create(context.Background(), pod, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create pod fixture: %v", err) + } + pods[i] = pod + } + + startGate := make(chan struct{}) + doneHolding := make(chan struct{}) + var wg sync.WaitGroup + errs := make(chan error, numGoroutines) + + for i := range numGoroutines { + wg.Add(1) + go func(idx int) { + defer wg.Done() + pod := pods[idx] + <-startGate + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + err := LockNodeWithContext(ctx, nodeName, "", pod) + errs <- err + if err == nil { + <-doneHolding + _ = ReleaseNodeLockWithContext(context.Background(), nodeName, "", pod, false) + } + }(i) + } + + close(startGate) + + var successCount int + var failureCount int + for range numGoroutines { + err := <-errs + if err == nil { + successCount++ + } else { + failureCount++ + if !IsNodeLockContention(err) { + t.Errorf("Expected ErrNodeLockContention, got: %v", err) + } + } + } + + close(doneHolding) + wg.Wait() + + if successCount != 1 { + t.Fatalf("Expected exactly 1 successful lock acquisition, got %d", successCount) + } + if failureCount != numGoroutines-1 { + t.Fatalf("Expected %d failed lock attempts, got %d", numGoroutines-1, failureCount) + } + }) + + t.Run("TryLockNode Non-blocking Attempt", func(t *testing.T) { + nodeLocks = newNodeLockManager() + clientSet = fake.NewClientset(&corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName, Annotations: map[string]string{}}, + }) + client.KubeClient = clientSet + + podA := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "try-pod-a", Namespace: "ns"}} + if err := TryLockNode(nodeName, "", podA); err != nil { + t.Fatalf("TryLockNode should succeed when node is unlocked: %v", err) + } + + if _, err := clientSet.CoreV1().Pods("ns").Create(context.Background(), podA, metav1.CreateOptions{}); err != nil { + t.Fatalf("Failed to create pod fixture: %v", err) + } + + podB := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "try-pod-b", Namespace: "ns"}} + if err := TryLockNode(nodeName, "", podB); err == nil { + t.Fatalf("TryLockNode should fail when node is locked by another pod") + } else if !IsNodeLockContention(err) { + t.Fatalf("Expected NodeLockContention error, got: %v", err) + } + }) +} + +func TestLockMutexWithContext_Cancelled(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "mutex-cancel-node" + mu := nodeLocks.getLock(nodeName) + mu.Lock() + defer mu.Unlock() + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) + defer cancel() + + err := lockMutexWithContext(ctx, mu) + if err == nil || !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected context.DeadlineExceeded, got %v", err) + } +} + +func TestSetNodeLockWithContext_Coverage(t *testing.T) { + t.Run("nil context uses background context", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-nil-ctx-set" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + if err := SetNodeLockWithContext(context.TODO(), nodeName, "", pod); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("mutex lock failure when context is canceled", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-mutex-fail-set" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + mu := nodeLocks.getLock(nodeName) + mu.Lock() + defer mu.Unlock() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := SetNodeLockWithContext(ctx, nodeName, "", pod) + if err == nil || !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got %v", err) + } + }) + + t.Run("initial node get error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-get-fail-set" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset() + client.KubeClient = clientSet + + clientSet.PrependReactor("get", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated initial get error") + }) + + err := SetNodeLockWithContext(context.Background(), nodeName, "", pod) + if err == nil || !strings.Contains(err.Error(), "simulated initial get error") { + t.Fatalf("expected initial get error, got %v", err) + } + }) + + t.Run("retry loop node get error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-retry-get-fail-set" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + getCalls := 0 + clientSet.PrependReactor("get", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + getCalls++ + if getCalls > 1 { + return true, nil, errors.New("simulated retry get error") + } + return false, nil, nil + }) + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, apierrors.NewConflict(schema.GroupResource{Resource: "nodes"}, nodeName, errors.New("conflict")) + }) + + err := SetNodeLockWithContext(context.Background(), nodeName, "", pod) + if err == nil || !strings.Contains(err.Error(), "simulated retry get error") { + t.Fatalf("expected retry get error, got %v", err) + } + }) + + t.Run("canceled context during retry", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-retry-cancel-set" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) + defer cancel() + + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated transient error requiring retry") + }) + + err := SetNodeLockWithContext(ctx, nodeName, "", pod) + if err == nil || !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected context.DeadlineExceeded, got %v", err) + } + }) +} + +func TestReleaseNodeLockWithContext_Coverage(t *testing.T) { + t.Run("nil pod returns error", func(t *testing.T) { + err := ReleaseNodeLockWithContext(context.Background(), "node", "", nil, false) + if err == nil || !strings.Contains(err.Error(), "pod is nil") { + t.Fatalf("expected pod is nil error, got %v", err) + } + }) + + t.Run("nil context uses background context", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-nil-ctx-rel" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Annotations: map[string]string{ + NodeLockKey: "2026-08-01T00:00:00Z,ns1,pod1", + }, + }}) + client.KubeClient = clientSet + + if err := ReleaseNodeLockWithContext(context.TODO(), nodeName, "", pod, false); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("mutex lock failure when context is canceled", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-mutex-fail-rel" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + mu := nodeLocks.getLock(nodeName) + mu.Lock() + defer mu.Unlock() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := ReleaseNodeLockWithContext(ctx, nodeName, "", pod, false) + if err == nil || !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got %v", err) + } + }) + + t.Run("initial node get error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-get-fail-rel" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset() + client.KubeClient = clientSet + + clientSet.PrependReactor("get", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated get error") + }) + + err := ReleaseNodeLockWithContext(context.Background(), nodeName, "", pod, false) + if err == nil || !strings.Contains(err.Error(), "simulated get error") { + t.Fatalf("expected get error, got %v", err) + } + }) + + t.Run("retry loop node get error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-retry-get-fail-rel" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Annotations: map[string]string{ + NodeLockKey: "2026-08-01T00:00:00Z,ns1,pod1", + }, + }}) + client.KubeClient = clientSet + + getCalls := 0 + clientSet.PrependReactor("get", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + getCalls++ + if getCalls > 1 { + return true, nil, errors.New("simulated retry get error") + } + return false, nil, nil + }) + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated patch retry error") + }) + + err := ReleaseNodeLockWithContext(context.Background(), nodeName, "", pod, false) + if err == nil || !strings.Contains(err.Error(), "simulated retry get error") { + t.Fatalf("expected retry get error, got %v", err) + } + }) + + t.Run("retry loop annotation missing concurrently", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-deleted-ann-rel" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Annotations: map[string]string{ + NodeLockKey: "2026-08-01T00:00:00Z,ns1,pod1", + }, + }}) + client.KubeClient = clientSet + + getCalls := 0 + clientSet.PrependReactor("get", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + getCalls++ + if getCalls > 1 { + return true, &corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }}, nil + } + return false, nil, nil + }) + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated patch retry error") + }) + + err := ReleaseNodeLockWithContext(context.Background(), nodeName, "", pod, false) + if err != nil { + t.Fatalf("expected nil error when annotation removed concurrently, got %v", err) + } + }) + + t.Run("canceled context during retry", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-retry-cancel-rel" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Annotations: map[string]string{ + NodeLockKey: "2026-08-01T00:00:00Z,ns1,pod1", + }, + }}) + client.KubeClient = clientSet + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) + defer cancel() + + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated transient patch error requiring retry") + }) + + err := ReleaseNodeLockWithContext(ctx, nodeName, "", pod, false) + if err == nil || !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected context.DeadlineExceeded, got %v", err) + } + }) + + t.Run("retry exhausted returns formatted error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-exhausted-rel" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Annotations: map[string]string{ + NodeLockKey: "2026-08-01T00:00:00Z,ns1,pod1", + }, + }}) + client.KubeClient = clientSet + + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("persistent patch failure") + }) + + err := ReleaseNodeLockWithContext(context.Background(), nodeName, "", pod, false) + if err == nil || !strings.Contains(err.Error(), "failed to release node lock") { + t.Fatalf("expected failed to release node lock error, got %v", err) + } + }) +} + +func TestLockNodeWithContext_Coverage(t *testing.T) { + t.Run("nil context uses background context", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-nil-ctx-lock" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + if err := LockNodeWithContext(context.TODO(), nodeName, "", pod); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("expired lock triggers takeover", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-expired-lock" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-pod", Namespace: "new-ns"}} + oldLockTime := time.Now().Add(-10 * time.Minute).Format(time.RFC3339) + expiredLockStr := fmt.Sprintf("%s,old-ns,old-pod", oldLockTime) + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Annotations: map[string]string{ + NodeLockKey: expiredLockStr, + }, + }}) + client.KubeClient = clientSet + + if err := LockNodeWithContext(context.Background(), nodeName, "", pod); err != nil { + t.Fatalf("expected success taking over expired lock, got %v", err) + } + }) + + t.Run("pod get returns non-NotFound error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-pod-get-err" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + freshLockTime := time.Now().Format(time.RFC3339) + lockStr := fmt.Sprintf("%s,other-ns,other-pod", freshLockTime) + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Annotations: map[string]string{ + NodeLockKey: lockStr, + }, + }}) + client.KubeClient = clientSet + + clientSet.PrependReactor("get", "pods", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated pod API error") + }) + + err := LockNodeWithContext(context.Background(), nodeName, "", pod) + if err == nil || !strings.Contains(err.Error(), "simulated pod API error") { + t.Fatalf("expected pod API error, got %v", err) + } + }) + + t.Run("release failure during expired/dangling lock takeover", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-release-takeover-err" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + oldLockTime := time.Now().Add(-10 * time.Minute).Format(time.RFC3339) + expiredLockStr := fmt.Sprintf("%s,old-ns,old-pod", oldLockTime) + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Annotations: map[string]string{ + NodeLockKey: expiredLockStr, + }, + }}) + client.KubeClient = clientSet + + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated patch error during release") + }) + + err := LockNodeWithContext(context.Background(), nodeName, "", pod) + if err == nil || !strings.Contains(err.Error(), "failed to release node lock") { + t.Fatalf("expected release failure error, got %v", err) + } + }) +} + +func TestTryLockNodeWithContext_Coverage(t *testing.T) { + t.Run("nil context uses background context", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-nil-ctx-try" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + if err := TryLockNodeWithContext(context.TODO(), nodeName, "", pod); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("in-memory mutex locked returns contention error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-in-memory-locked" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + mu := nodeLocks.getLock(nodeName) + mu.Lock() + defer mu.Unlock() + + err := TryLockNodeWithContext(context.Background(), nodeName, "", pod) + if err == nil || !IsNodeLockContention(err) { + t.Fatalf("expected ErrNodeLockContention, got %v", err) + } + }) + + t.Run("node get error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-get-fail-try" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset() + client.KubeClient = clientSet + + clientSet.PrependReactor("get", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated get node error") + }) + + err := TryLockNodeWithContext(context.Background(), nodeName, "", pod) + if err == nil || !strings.Contains(err.Error(), "simulated get node error") { + t.Fatalf("expected get node error, got %v", err) + } + }) + + t.Run("patch conflict error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-patch-conflict-try" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, apierrors.NewConflict(schema.GroupResource{Resource: "nodes"}, nodeName, errors.New("simulated conflict")) + }) + + err := TryLockNodeWithContext(context.Background(), nodeName, "", pod) + if err == nil || !IsNodeLockContention(err) || !strings.Contains(err.Error(), "patch conflict") { + t.Fatalf("expected patch conflict contention error, got %v", err) + } + }) + + t.Run("patch general error", func(t *testing.T) { + nodeLocks = newNodeLockManager() + nodeName := "node-patch-gen-err-try" + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", Namespace: "ns1"}} + clientSet := fake.NewClientset(&corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) + client.KubeClient = clientSet + + clientSet.PrependReactor("patch", "nodes", func(k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, nil, errors.New("simulated generic patch error") + }) + + err := TryLockNodeWithContext(context.Background(), nodeName, "", pod) + if err == nil || !strings.Contains(err.Error(), "simulated generic patch error") { + t.Fatalf("expected generic patch error, got %v", err) + } + }) +} + +func TestParseNodeLock_Malformed(t *testing.T) { + _, _, _, err := ParseNodeLock("part1,part2") + if err == nil || !strings.Contains(err.Error(), "malformed lock annotation: expected 3 parts, got 2") { + t.Fatalf("expected malformed lock annotation error, got %v", err) + } +} + +func TestGenerateNodeLockKeyByPod_NilPod(t *testing.T) { + key := GenerateNodeLockKeyByPod(nil) + if key == "" { + t.Fatalf("expected non-empty key for nil pod") + } + if strings.Contains(key, NodeLockSep) { + t.Fatalf("expected timestamp-only key without separator for nil pod, got %q", key) + } +} + +func TestTestHelpers(t *testing.T) { + ResetNodeLocksForTest() + if count := NodeLockCountForTest(); count != 0 { + t.Fatalf("expected 0 locks after reset, got %d", count) + } + EnsureNodeLockForTest("node1") + if count := NodeLockCountForTest(); count != 1 { + t.Fatalf("expected 1 lock after ensure, got %d", count) + } + EnsureNodeLockForTest("node1") + if count := NodeLockCountForTest(); count != 1 { + t.Fatalf("expected 1 lock after no-op ensure, got %d", count) + } +}