From 64eda74cabe41b9ba0f51acc1748db6304134c9b Mon Sep 17 00:00:00 2001 From: Aditya Raut Date: Sat, 1 Aug 2026 10:54:55 +0530 Subject: [PATCH 1/3] fix(nodelock): let a pod re-acquire its own still-valid node lock lockAllDevices calls LockNode once per device vendor a pod requests resources from (device.GetDevices() is iterated per vendor backend), all writing to the same shared node annotation (NodeLockKey). LockNode had no case for "this exact pod already holds the lock": the dangling- lock check only ran for a different pod, so a same-pod, non-expired lock fell straight through to the contention error. A pod requesting resources from two or more HAMi-managed vendors (e.g. nvidia.com/gpu and cambricon.com/vmlu together) would have its first LockNode call succeed and its second one immediately contend with its own lock, making it permanently unschedulable. Add an explicit branch: if the existing lock's namespace/name match the calling pod and it hasn't expired, treat it as already acquired instead of erroring. Test_LockNode's "node has been locked" case had (accidentally) used the exact same pod identity as both the lock holder and the requester, so it was asserting the buggy contention behavior as correct. Split it into a genuine third-party-contention case (locked by a different, still-live pod) and a new dedicated test reproducing lockAllDevices' actual multi-vendor call pattern. Signed-off-by: Aditya Raut --- pkg/util/nodelock/nodelock.go | 11 +++++++++- pkg/util/nodelock/nodelock_test.go | 35 ++++++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/pkg/util/nodelock/nodelock.go b/pkg/util/nodelock/nodelock.go index 331a1254fa..163ead0daf 100644 --- a/pkg/util/nodelock/nodelock.go +++ b/pkg/util/nodelock/nodelock.go @@ -233,9 +233,18 @@ func LockNode(nodeName string, lockname string, pods *corev1.Pod) error { if time.Since(lockTime) > NodeLockTimeout { 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 != "" && (ns != pods.Namespace || previousPodName != pods.Name) { + 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) diff --git a/pkg/util/nodelock/nodelock_test.go b/pkg/util/nodelock/nodelock_test.go index f88015d01d..6baa070e66 100644 --- a/pkg/util/nodelock/nodelock_test.go +++ b/pkg/util/nodelock/nodelock_test.go @@ -59,7 +59,7 @@ func Test_LockNode(t *testing.T) { wantErr: true, }, { - name: "node has been locked", + name: "node has been locked by another pod", args: args{ nodeName: func() string { name := "worker-1" @@ -68,11 +68,16 @@ func Test_LockNode(t *testing.T) { Name: name, Annotations: map[string]string{ NodeLockKey: GenerateNodeLockKeyByPod(&corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{Name: "hami", Namespace: "hami-ns"}, + ObjectMeta: metav1.ObjectMeta{Name: "other-pod", Namespace: "other-ns"}, }), }, }, }, metav1.CreateOptions{}) + // The lock holder ("other-pod"/"other-ns") must exist and not be + // dangling, otherwise LockNode treats it as stale and takes over. + client.KubeClient.CoreV1().Pods("other-ns").Create(context.TODO(), &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "other-pod", Namespace: "other-ns"}, + }, metav1.CreateOptions{}) return name }, pods: &corev1.Pod{ @@ -233,6 +238,32 @@ func TestLockNodeWithDangling(t *testing.T) { } } +// TestLockNodeReentrantSamePod covers lockAllDevices' actual call pattern: +// it calls LockNode once per device vendor a pod requests resources from, so +// a pod requesting e.g. both nvidia.com/gpu and cambricon.com/vmlu locks the +// same node twice for itself in a row. The second call must succeed instead +// of contending with the pod's own still-valid lock, or such a pod could +// never become schedulable. +func TestLockNodeReentrantSamePod(t *testing.T) { + client.KubeClient = fake.NewClientset() + nodeLocks = newNodeLockManager() + nodeName := "multi-vendor-node" + _, err := client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName, Annotations: map[string]string{}}, + }, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create node: %v", err) + } + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "multi-vendor-pod", Namespace: "test-ns"}} + + if err := LockNode(nodeName, "nvidia", pod); err != nil { + t.Fatalf("first LockNode call (simulating the nvidia backend) failed: %v", err) + } + if err := LockNode(nodeName, "cambricon", pod); err != nil { + t.Fatalf("second LockNode call for the same pod (simulating the cambricon backend) should succeed, got: %v", err) + } +} + func TestReleaseNodeLock(t *testing.T) { client.KubeClient = fake.NewClientset() type args struct { From 462ccc19fbef8bbc63d2130167d6fe8a0f51451d Mon Sep 17 00:00:00 2001 From: Aditya Raut Date: Sat, 1 Aug 2026 11:07:53 +0530 Subject: [PATCH 2/3] test(nodelock): cover same-namespace-different-pod branch in LockNode The reentrancy check added in the previous commit is a compound condition, ns == pods.Namespace && previousPodName == pods.Name. Existing tests only exercised the fully-false case (different namespace entirely) and the fully-true case (same pod, the reentrant scenario); nothing exercised same namespace with a different pod name, leaving codecov's patch coverage partial on that line. Add that case: a live pod in the same namespace, but a different name, holding the lock - which must still be treated as third-party contention, not reentrancy. Signed-off-by: Aditya Raut --- pkg/util/nodelock/nodelock_test.go | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/pkg/util/nodelock/nodelock_test.go b/pkg/util/nodelock/nodelock_test.go index 6baa070e66..cb04376461 100644 --- a/pkg/util/nodelock/nodelock_test.go +++ b/pkg/util/nodelock/nodelock_test.go @@ -89,6 +89,40 @@ func Test_LockNode(t *testing.T) { }, wantErr: true, }, + { + name: "node has been locked by another pod in the same namespace", + args: args{ + nodeName: func() string { + name := "worker-1b" + client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Annotations: map[string]string{ + NodeLockKey: GenerateNodeLockKeyByPod(&corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "other-pod-same-ns", Namespace: "hami-ns"}, + }), + }, + }, + }, metav1.CreateOptions{}) + // Same namespace as the requester below, but a different pod + // name: exercises ns == pods.Namespace (true) with + // previousPodName == pods.Name (false), distinct from both the + // "another pod" case above (both false) and the reentrant + // same-pod case (both true). + client.KubeClient.CoreV1().Pods("hami-ns").Create(context.TODO(), &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "other-pod-same-ns", Namespace: "hami-ns"}, + }, metav1.CreateOptions{}) + return name + }, + pods: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hami", + Namespace: "hami-ns", + }, + }, + }, + wantErr: true, + }, { name: "node lock is invalid", args: args{ From 3364efdb3cd7effdcd200013cfa936bfcebe2dce Mon Sep 17 00:00:00 2001 From: Aditya Raut Date: Sat, 1 Aug 2026 13:44:32 +0530 Subject: [PATCH 3/3] test(nodelock): check fixture-creation errors, create pod before lock Addresses CodeRabbit review on PR #2255: - Test_LockNode's "another pod" and "same namespace, different pod" cases ignored the Nodes().Create/Pods().Create fixture errors. If node creation silently failed, LockNode would return a not-found error instead, and wantErr: true would pass without actually exercising the lock-contention path it's meant to test. Fail the subtest via t.Fatalf on any fixture-creation error instead. - TestLockNodeReentrantSamePod never created the requesting pod ("multi-vendor-pod"/"test-ns") in the fake client, so the first LockNode call succeeded only because it was setting a fresh lock, not because anything verified a real, live pod. Create the pod before acquiring the lock so the test reflects lockAllDevices' actual runtime scenario, where the pod genuinely exists throughout. Signed-off-by: Aditya Raut --- pkg/util/nodelock/nodelock_test.go | 58 ++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/pkg/util/nodelock/nodelock_test.go b/pkg/util/nodelock/nodelock_test.go index cb04376461..602c58541e 100644 --- a/pkg/util/nodelock/nodelock_test.go +++ b/pkg/util/nodelock/nodelock_test.go @@ -34,7 +34,7 @@ import ( func Test_LockNode(t *testing.T) { client.KubeClient = fake.NewClientset() type args struct { - nodeName func() string + nodeName func(t *testing.T) string lockname string pods *corev1.Pod } @@ -46,7 +46,7 @@ func Test_LockNode(t *testing.T) { { name: "node not found", args: args{ - nodeName: func() string { + nodeName: func(t *testing.T) string { return "node" }, pods: &corev1.Pod{ @@ -61,9 +61,9 @@ func Test_LockNode(t *testing.T) { { name: "node has been locked by another pod", args: args{ - nodeName: func() string { + nodeName: func(t *testing.T) string { name := "worker-1" - client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ + if _, err := client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: name, Annotations: map[string]string{ @@ -72,12 +72,16 @@ func Test_LockNode(t *testing.T) { }), }, }, - }, metav1.CreateOptions{}) + }, metav1.CreateOptions{}); err != nil { + t.Fatalf("failed to create node fixture: %v", err) + } // The lock holder ("other-pod"/"other-ns") must exist and not be // dangling, otherwise LockNode treats it as stale and takes over. - client.KubeClient.CoreV1().Pods("other-ns").Create(context.TODO(), &corev1.Pod{ + if _, err := client.KubeClient.CoreV1().Pods("other-ns").Create(context.TODO(), &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "other-pod", Namespace: "other-ns"}, - }, metav1.CreateOptions{}) + }, metav1.CreateOptions{}); err != nil { + t.Fatalf("failed to create lock-holder pod fixture: %v", err) + } return name }, pods: &corev1.Pod{ @@ -92,9 +96,9 @@ func Test_LockNode(t *testing.T) { { name: "node has been locked by another pod in the same namespace", args: args{ - nodeName: func() string { + nodeName: func(t *testing.T) string { name := "worker-1b" - client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ + if _, err := client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: name, Annotations: map[string]string{ @@ -103,15 +107,19 @@ func Test_LockNode(t *testing.T) { }), }, }, - }, metav1.CreateOptions{}) + }, metav1.CreateOptions{}); err != nil { + t.Fatalf("failed to create node fixture: %v", err) + } // Same namespace as the requester below, but a different pod // name: exercises ns == pods.Namespace (true) with // previousPodName == pods.Name (false), distinct from both the // "another pod" case above (both false) and the reentrant // same-pod case (both true). - client.KubeClient.CoreV1().Pods("hami-ns").Create(context.TODO(), &corev1.Pod{ + if _, err := client.KubeClient.CoreV1().Pods("hami-ns").Create(context.TODO(), &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "other-pod-same-ns", Namespace: "hami-ns"}, - }, metav1.CreateOptions{}) + }, metav1.CreateOptions{}); err != nil { + t.Fatalf("failed to create lock-holder pod fixture: %v", err) + } return name }, pods: &corev1.Pod{ @@ -126,16 +134,18 @@ func Test_LockNode(t *testing.T) { { name: "node lock is invalid", args: args{ - nodeName: func() string { + nodeName: func(t *testing.T) string { name := "worker-2" - client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ + if _, err := client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: name, Annotations: map[string]string{ NodeLockKey: "lock", }, }, - }, metav1.CreateOptions{}) + }, metav1.CreateOptions{}); err != nil { + t.Fatalf("failed to create node fixture: %v", err) + } return name }, pods: &corev1.Pod{ @@ -150,11 +160,13 @@ func Test_LockNode(t *testing.T) { { name: "successfully set node lock", args: args{ - nodeName: func() string { + nodeName: func(t *testing.T) string { name := "worker-3" - client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ + if _, err := client.KubeClient.CoreV1().Nodes().Create(context.TODO(), &corev1.Node{ ObjectMeta: metav1.ObjectMeta{Name: name, Annotations: map[string]string{}}, - }, metav1.CreateOptions{}) + }, metav1.CreateOptions{}); err != nil { + t.Fatalf("failed to create node fixture: %v", err) + } return name }, pods: &corev1.Pod{ @@ -169,7 +181,7 @@ func Test_LockNode(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := LockNode(tt.args.nodeName(), tt.args.lockname, tt.args.pods); (err != nil) != tt.wantErr { + if err := LockNode(tt.args.nodeName(t), tt.args.lockname, tt.args.pods); (err != nil) != tt.wantErr { t.Errorf("LockNode() error = %v, wantErr %v", err, tt.wantErr) } }) @@ -289,6 +301,14 @@ func TestLockNodeReentrantSamePod(t *testing.T) { t.Fatalf("Failed to create node: %v", err) } pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "multi-vendor-pod", Namespace: "test-ns"}} + // The requesting pod must actually exist for this test to exercise the + // intended live-ownership scenario. Without it, the first LockNode call + // still succeeds, but only because it's setting a fresh lock, not + // because the reentrancy branch has verified anything about a real, + // live pod - so a real dangling-lock path could be masked instead. + if _, err := client.KubeClient.CoreV1().Pods("test-ns").Create(context.TODO(), pod, metav1.CreateOptions{}); err != nil { + t.Fatalf("Failed to create pod: %v", err) + } if err := LockNode(nodeName, "nvidia", pod); err != nil { t.Fatalf("first LockNode call (simulating the nvidia backend) failed: %v", err)