From 0f87b2d5c981c205ef0014c4c46e85257e41a4ac Mon Sep 17 00:00:00 2001 From: Aditya Raut Date: Fri, 7 Aug 2026 10:50:55 +0530 Subject: [PATCH 1/2] fix(scheduler): don't cache a nil NodeUsage when node is missing from snapshot getNodesUsage() takes an overallnodeMap snapshot via ListNodes(), then for each candidate node calls GetNode(nodeID) and unconditionally does cachenodeMap[node.ID] = overallnodeMap[node.ID]. ListNodes() silently skips any node whose Node field is nil, but GetNode() applies no such filter and returns success anyway. So a node accepted by GetNode() but absent from the snapshot got cached as a nil *NodeUsage, with no entry recorded in failedNodes. calcScoreWithOptions later ranges over that map and calls viewStatus(*node) unconditionally per node, so a nil entry panics the scoring goroutine. Nothing recovers goroutine panics in this package, so the panic takes down the whole scheduler process. The same nil-map outcome is also reachable via a narrower race: a node registering between the ListNodes() snapshot and the per-node GetNode() call. Check the snapshot lookup explicitly and record the node as failed instead of caching nil. Adds Test_getNodesUsage_NodeMissingFromSnapshotIsNotCachedAsNil, which reproduces the bug deterministically via the ListNodes()/GetNode() nil-Node divergence. Signed-off-by: Aditya Raut --- pkg/scheduler/scheduler.go | 12 +++++++++- pkg/scheduler/scheduler_test.go | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 47d530a625..250691fa43 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -847,7 +847,17 @@ func (s *Scheduler) getNodesUsage(nodes *[]string, task *corev1.Pod) (*map[strin failedNodes[nodeID] = "node unregistered" continue } - cachenodeMap[node.ID] = overallnodeMap[node.ID] + usage, ok := overallnodeMap[node.ID] + if !ok { + // GetNode() succeeded but the node is absent from the overallnodeMap snapshot + // (e.g. registered between the ListNodes() snapshot and this lookup, or filtered + // out of ListNodes() by a nil Node). Treat it as unavailable instead of caching a + // nil *NodeUsage, which would later be dereferenced unconditionally by the scorer. + klog.V(5).InfoS("node usage not found in snapshot", "node", nodeID) + failedNodes[nodeID] = "node usage unavailable" + continue + } + cachenodeMap[node.ID] = usage } return &cachenodeMap, &overallnodeMap, failedNodes, nil } diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go index 8ca6e15533..b8450e3111 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -130,6 +130,47 @@ func Test_getNodesUsage(t *testing.T) { assert.Equal(t, v.Devices.DeviceLists[0].Device.Usedcores, int32(20)) } +// Regression: ListNodes() silently skips nodes whose Node field is nil, while GetNode() +// does not apply that same filter and returns success. Before the fix, getNodesUsage() +// blindly assigned cachenodeMap[id] = overallnodeMap[id] for every node GetNode() accepted, +// so a node present in nodeManager but absent from the ListNodes() snapshot produced a nil +// *NodeUsage entry with no corresponding failedNodes record. That nil is later dereferenced +// unconditionally by calcScoreWithOptions (viewStatus(*node)), panicking the scheduler. +func Test_getNodesUsage_NodeMissingFromSnapshotIsNotCachedAsNil(t *testing.T) { + nodeMage := newNodeManager() + nodeMage.addNode("node1", &device.NodeInfo{ + ID: "node1", + Node: nil, + Devices: map[string][]device.DeviceInfo{ + nvidia.NvidiaGPUDevice: {{ + ID: "GPU0", + Index: 0, + Count: 10, + Devmem: 1024, + Devcore: 100, + Numa: 1, + Mode: "hami", + Health: true, + }}, + }, + }) + podMap := device.NewPodManager() + s := Scheduler{ + nodeManager: nodeMage, + podManager: podMap, + } + nodes := []string{"node1"} + cachenodeMap, _, failedNodes, err := s.getNodesUsage(&nodes, nil) + if err != nil { + t.Fatal(err) + } + assert.Equal(t, 0, len(*cachenodeMap)) + v, present := (*cachenodeMap)["node1"] + assert.Assert(t, !present, "node1 must not be cached at all, got %v", v) + _, failed := failedNodes["node1"] + assert.Assert(t, failed, "node1 should be recorded as a failed node") +} + func Test_getNodesUsage_StalePodDeviceAllocation(t *testing.T) { t.Run("StaleOnly", func(t *testing.T) { nodeMage := newNodeManager() From ee25e52c35e80840d3ac0dca29b01aff1a55cfbe Mon Sep 17 00:00:00 2001 From: Aditya Raut Date: Fri, 7 Aug 2026 11:54:20 +0530 Subject: [PATCH 2/2] fix(scheduler): remove explanatory comment per review feedback Per archlitchi's review on #2436, drop the inline comment block above the snapshot-miss check; the log message and failedNodes reason already say what's happening. Signed-off-by: Aditya Raut --- pkg/scheduler/scheduler.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 250691fa43..37c12f9e17 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -849,10 +849,6 @@ func (s *Scheduler) getNodesUsage(nodes *[]string, task *corev1.Pod) (*map[strin } usage, ok := overallnodeMap[node.ID] if !ok { - // GetNode() succeeded but the node is absent from the overallnodeMap snapshot - // (e.g. registered between the ListNodes() snapshot and this lookup, or filtered - // out of ListNodes() by a nil Node). Treat it as unavailable instead of caching a - // nil *NodeUsage, which would later be dereferenced unconditionally by the scorer. klog.V(5).InfoS("node usage not found in snapshot", "node", nodeID) failedNodes[nodeID] = "node usage unavailable" continue