From 3887681169ca46392085e46bebb0cd97fe97437f Mon Sep 17 00:00:00 2001 From: Prabal Poddar Date: Sat, 1 Aug 2026 17:09:32 +0530 Subject: [PATCH 1/6] fix(ascend): validate module-pair count and prevent under-allocation for Ascend 910C Signed-off-by: Prabal Poddar --- pkg/device/ascend/device.go | 36 ++++-- pkg/device/ascend/device_910c_pairing_test.go | 108 ++++++++++++++++++ 2 files changed, 135 insertions(+), 9 deletions(-) create mode 100644 pkg/device/ascend/device_910c_pairing_test.go diff --git a/pkg/device/ascend/device.go b/pkg/device/ascend/device.go index 2b6cecbd90..ebc5ee5003 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -560,6 +560,25 @@ func (npu *Devices) Fit(devices []*device.DeviceUsage, request device.ContainerD } if needTopology { + if k.Type == Ascend910CType && originReq > 1 { + // Ascend 910C requires full module-pair allocation (2 NPUs per + // physical card). Always run the pairing filter here, even when + // the raw candidate count already equals originReq, because + // candidates satisfying the count alone may still be spread + // across incomplete/partial modules rather than full pairs. + combination := npu.computeBestCombination910C(nodeInfo, int(originReq), tmpDevs[k.Type]) + if len(combination) != int(originReq) { + // Never report success on a short allocation: doing so silently + // under-allocates NPUs relative to what the pod requested. + reason[common.AllocatedCardsInsufficientRequest] = len(tmpDevs) + klog.V(5).InfoS(common.AllocatedCardsInsufficientRequest, "pod", klog.KObj(pod), "request", originReq, "allocated", len(combination)) + return false, tmpDevs, common.GenReason(reason, len(devices)) + } + tmpDevs[k.Type] = combination + klog.V(5).InfoS("device allocate success", "pod", klog.KObj(pod), "best device combination", tmpDevs) + return true, tmpDevs, "" + } + if len(tmpDevs[k.Type]) == int(originReq) { klog.V(5).InfoS("device allocate success", "pod", klog.KObj(pod), "allocate device", tmpDevs) return true, tmpDevs, "" @@ -568,13 +587,7 @@ func (npu *Devices) Fit(devices []*device.DeviceUsage, request device.ContainerD tmpDevs[k.Type] = device.ContainerDevices{tmpDevs[k.Type][0]} } else { // If requesting multiple devices, select the best combination of cards. - var combination device.ContainerDevices - if k.Type == Ascend910CType { - // Use topology-aware allocation for Ascend910C: only select full modules (2 NPUs per card). - combination = npu.computeBestCombination910C(nodeInfo, int(originReq), tmpDevs[k.Type]) - } else { - combination = npu.computeBestCombination(nodeInfo, int(originReq), tmpDevs[k.Type]) - } + combination := npu.computeBestCombination(nodeInfo, int(originReq), tmpDevs[k.Type]) tmpDevs[k.Type] = combination } klog.V(5).InfoS("device allocate success", "pod", klog.KObj(pod), "best device combination", tmpDevs) @@ -673,9 +686,14 @@ func (npudev *Devices) computeBestCombination910C(nodeInfo *device.NodeInfo, req cardTopSlice = append(cardTopSlice, card) } - // Sort cards by the number of available NPUs in ascending order. + // Sort cards by the number of available NPUs in descending order, so that + // full cards are considered before partial ones. Note: partial cards are + // still excluded outright below via the MaxCardNPUNum equality check, so + // this ordering only affects which full cards get picked first when more + // full cards are available than requested — it does not by itself change + // whether a request succeeds or fails. sort.Slice(cardTopSlice, func(i, j int) bool { - return len(cardTopSlice[i]) < len(cardTopSlice[j]) + return len(cardTopSlice[i]) > len(cardTopSlice[j]) }) // Select NPUs card by card, preferring full cards. diff --git a/pkg/device/ascend/device_910c_pairing_test.go b/pkg/device/ascend/device_910c_pairing_test.go new file mode 100644 index 0000000000..7a09e9f451 --- /dev/null +++ b/pkg/device/ascend/device_910c_pairing_test.go @@ -0,0 +1,108 @@ +package ascend + +import ( + "testing" + + "github.com/Project-HAMi/HAMi/pkg/device" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// TestAscend910C_FitPartialAllocationBug reproduces the case where a pod +// requests 4 NPUs but the node only has 1 full module (2 NPUs) plus 3 +// partial modules (1 NPU each). Before the fix, Fit() would incorrectly +// report success while allocating only 2 devices. +func TestAscend910C_FitPartialAllocationBug(t *testing.T) { + dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} + nodeInfo := &device.NodeInfo{ + Node: &corev1.Node{}, + Devices: map[string][]device.DeviceInfo{ + Ascend910CType: { + {ID: "dev-0", Index: 0, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-1", Index: 1, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-2", Index: 2, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-4", Index: 4, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-6", Index: 6, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + }, + }, + } + devices := []*device.DeviceUsage{ + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-4", Index: 4, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-6", Index: 6, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + } + req := device.ContainerDeviceRequest{Nums: 4, Type: Ascend910CType} + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod"}} + + fit, tmpDevs, reason := dev.Fit(devices, req, pod, nodeInfo, nil) + + if fit { + t.Errorf("expected fit=false when only 2 of 4 requested NPUs form full module pairs, got fit=true, allocated=%d, reason=%q", + len(tmpDevs[Ascend910CType]), reason) + } +} + +// TestAscend910C_FitExactCountBypassBug reproduces the L568-571 bypass: a +// pod requests 2 NPUs, and exactly 2 candidate NPUs exist on the node, but +// they sit on two different partial modules rather than one full pair. +// Before the fix, Fit() returned true without ever validating module +// pairing, because the candidate count already matched originReq. +func TestAscend910C_FitExactCountBypassBug(t *testing.T) { + dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} + nodeInfo := &device.NodeInfo{ + Node: &corev1.Node{}, + Devices: map[string][]device.DeviceInfo{ + Ascend910CType: { + // dev-0 (module 0) and dev-2 (module 1) are each the sole + // occupied NPU of their respective module - no full pair. + {ID: "dev-0", Index: 0, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-2", Index: 2, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + }, + }, + } + devices := []*device.DeviceUsage{ + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + } + req := device.ContainerDeviceRequest{Nums: 2, Type: Ascend910CType} + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod-2"}} + + fit, tmpDevs, reason := dev.Fit(devices, req, pod, nodeInfo, nil) + + if fit { + t.Errorf("expected fit=false when the 2 candidate NPUs come from two different partial modules (not one full pair), got fit=true, allocated=%d, reason=%q", + len(tmpDevs[Ascend910CType]), reason) + } +} + +// TestAscend910C_FitFullPairSucceeds is the positive-path sanity check: +// a request for 2 NPUs against one genuinely full module (2 NPUs, same +// card) should still succeed after the fix. +func TestAscend910C_FitFullPairSucceeds(t *testing.T) { + dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} + nodeInfo := &device.NodeInfo{ + Node: &corev1.Node{}, + Devices: map[string][]device.DeviceInfo{ + Ascend910CType: { + // Index 0 and 1 belong to the same module (idx/2 == 0 for both). + {ID: "dev-0", Index: 0, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-1", Index: 1, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + }, + }, + } + devices := []*device.DeviceUsage{ + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + } + req := device.ContainerDeviceRequest{Nums: 2, Type: Ascend910CType} + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod-3"}} + + fit, tmpDevs, reason := dev.Fit(devices, req, pod, nodeInfo, nil) + + if !fit || len(tmpDevs[Ascend910CType]) != 2 { + t.Errorf("expected fit=true with 2 allocated devices for one full module pair, got fit=%v, allocated=%d, reason=%q", + fit, len(tmpDevs[Ascend910CType]), reason) + } +} From 75ded93ca852cb343b67caccc1bae5e7b39338ba Mon Sep 17 00:00:00 2001 From: Prabal Poddar Date: Sun, 2 Aug 2026 10:06:02 +0530 Subject: [PATCH 2/6] fix(ascend): enforce 910C module-pair validation without topology and fix reason reporting Always run computeBestCombination910C for multi-device Ascend 910C requests even when NetworkID is absent or needTopology is false. Update AllocatedCardsInsufficientRequest reason to report the selected NPU count and pass originReq to GenReason. Signed-off-by: Prabal Poddar --- pkg/device/ascend/device.go | 40 ++++++------ pkg/device/ascend/device_910c_pairing_test.go | 64 ++++++++++++++++++- 2 files changed, 83 insertions(+), 21 deletions(-) diff --git a/pkg/device/ascend/device.go b/pkg/device/ascend/device.go index ebc5ee5003..ebb8f006b2 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -541,7 +541,7 @@ func (npu *Devices) Fit(devices []*device.DeviceUsage, request device.ContainerD } if k.Nums > 0 { klog.V(5).InfoS("find fit device", "pod", klog.KObj(pod), "device", dev.ID) - if !needTopology { + if !needTopology && !(k.Type == Ascend910CType && originReq > 1) { k.Nums-- } tmpDevs[k.Type] = append(tmpDevs[k.Type], device.ContainerDevice{ @@ -553,32 +553,32 @@ func (npu *Devices) Fit(devices []*device.DeviceUsage, request device.ContainerD CustomInfo: dev.CustomInfo, }) } - if k.Nums == 0 && !needTopology { + if k.Nums == 0 && !needTopology && !(k.Type == Ascend910CType && originReq > 1) { klog.V(4).InfoS("device allocate success", "pod", klog.KObj(pod), "allocate device", tmpDevs) return true, tmpDevs, "" } } - if needTopology { - if k.Type == Ascend910CType && originReq > 1 { - // Ascend 910C requires full module-pair allocation (2 NPUs per - // physical card). Always run the pairing filter here, even when - // the raw candidate count already equals originReq, because - // candidates satisfying the count alone may still be spread - // across incomplete/partial modules rather than full pairs. - combination := npu.computeBestCombination910C(nodeInfo, int(originReq), tmpDevs[k.Type]) - if len(combination) != int(originReq) { - // Never report success on a short allocation: doing so silently - // under-allocates NPUs relative to what the pod requested. - reason[common.AllocatedCardsInsufficientRequest] = len(tmpDevs) - klog.V(5).InfoS(common.AllocatedCardsInsufficientRequest, "pod", klog.KObj(pod), "request", originReq, "allocated", len(combination)) - return false, tmpDevs, common.GenReason(reason, len(devices)) - } - tmpDevs[k.Type] = combination - klog.V(5).InfoS("device allocate success", "pod", klog.KObj(pod), "best device combination", tmpDevs) - return true, tmpDevs, "" + if k.Type == Ascend910CType && originReq > 1 { + // Ascend 910C requires full module-pair allocation (2 NPUs per + // physical card). Always run the pairing filter here, even when + // NetworkID is absent or needTopology is false, because + // candidates satisfying the count alone may still be spread + // across incomplete/partial modules rather than full pairs. + combination := npu.computeBestCombination910C(nodeInfo, int(originReq), tmpDevs[k.Type]) + if len(combination) != int(originReq) { + // Never report success on a short allocation: doing so silently + // under-allocates NPUs relative to what the pod requested. + reason[common.AllocatedCardsInsufficientRequest] = len(combination) + klog.V(5).InfoS(common.AllocatedCardsInsufficientRequest, "pod", klog.KObj(pod), "request", originReq, "allocated", len(combination)) + return false, tmpDevs, common.GenReason(reason, int(originReq)) } + tmpDevs[k.Type] = combination + klog.V(5).InfoS("device allocate success", "pod", klog.KObj(pod), "best device combination", tmpDevs) + return true, tmpDevs, "" + } + if needTopology { if len(tmpDevs[k.Type]) == int(originReq) { klog.V(5).InfoS("device allocate success", "pod", klog.KObj(pod), "allocate device", tmpDevs) return true, tmpDevs, "" diff --git a/pkg/device/ascend/device_910c_pairing_test.go b/pkg/device/ascend/device_910c_pairing_test.go index 7a09e9f451..39f7fd8a14 100644 --- a/pkg/device/ascend/device_910c_pairing_test.go +++ b/pkg/device/ascend/device_910c_pairing_test.go @@ -1,11 +1,28 @@ +/* +Copyright 2024 The HAMi Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package ascend import ( "testing" - "github.com/Project-HAMi/HAMi/pkg/device" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/Project-HAMi/HAMi/pkg/device" ) // TestAscend910C_FitPartialAllocationBug reproduces the case where a pod @@ -106,3 +123,48 @@ func TestAscend910C_FitFullPairSucceeds(t *testing.T) { fit, len(tmpDevs[Ascend910CType]), reason) } } + +// TestAscend910C_FitWithoutNetworkID_ValidatesPairing ensures that Ascend 910C +// full-pair validation runs for multi-device requests even when CustomInfo["NetworkID"] +// is absent on devices (needTopology=false). +func TestAscend910C_FitWithoutNetworkID_ValidatesPairing(t *testing.T) { + dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} + nodeInfo := &device.NodeInfo{ + Node: &corev1.Node{}, + Devices: map[string][]device.DeviceInfo{ + Ascend910CType: { + // dev-0 (module 0) and dev-2 (module 1) without NetworkID in CustomInfo. + {ID: "dev-0", Index: 0, CustomInfo: map[string]any{}}, + {ID: "dev-2", Index: 2, CustomInfo: map[string]any{}}, + }, + }, + } + devices := []*device.DeviceUsage{ + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + } + req := device.ContainerDeviceRequest{Nums: 2, Type: Ascend910CType} + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod-no-netid"}} + + fit, tmpDevs, reason := dev.Fit(devices, req, pod, nodeInfo, nil) + + if fit { + t.Errorf("expected fit=false when NetworkID is absent and the 2 candidate NPUs belong to different modules (indices 0 and 2), got fit=true, allocated=%d, reason=%q", + len(tmpDevs[Ascend910CType]), reason) + } + + // Positive check without NetworkID when candidate devices form a full module pair (indices 0 and 1). + nodeInfo.Devices[Ascend910CType] = []device.DeviceInfo{ + {ID: "dev-0", Index: 0, CustomInfo: map[string]any{}}, + {ID: "dev-1", Index: 1, CustomInfo: map[string]any{}}, + } + devicesPair := []*device.DeviceUsage{ + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + } + fitPair, tmpDevsPair, reasonPair := dev.Fit(devicesPair, req, pod, nodeInfo, nil) + if !fitPair || len(tmpDevsPair[Ascend910CType]) != 2 { + t.Errorf("expected fit=true when NetworkID is absent but candidate devices form a full module pair (indices 0 and 1), got fit=%v, allocated=%d, reason=%q", + fitPair, len(tmpDevsPair[Ascend910CType]), reasonPair) + } +} From 7e60d516f03d5364af4df8b1582560a2b636fbaf Mon Sep 17 00:00:00 2001 From: Prabal Poddar Date: Sun, 2 Aug 2026 19:06:28 +0530 Subject: [PATCH 3/6] test(ascend): add TestComputeBestCombination910C_NoFullPairsReturnsEmpty Verify that computeBestCombination910C returns empty slice and Fit returns fit=false when no candidate NPUs share a full module and NetworkID is absent. Signed-off-by: Prabal Poddar --- pkg/device/ascend/device_910c_pairing_test.go | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/pkg/device/ascend/device_910c_pairing_test.go b/pkg/device/ascend/device_910c_pairing_test.go index 39f7fd8a14..f0e425c65c 100644 --- a/pkg/device/ascend/device_910c_pairing_test.go +++ b/pkg/device/ascend/device_910c_pairing_test.go @@ -168,3 +168,59 @@ func TestAscend910C_FitWithoutNetworkID_ValidatesPairing(t *testing.T) { fitPair, len(tmpDevsPair[Ascend910CType]), reasonPair) } } + +// TestComputeBestCombination910C_NoFullPairsReturnsEmpty directly answers +// whether computeBestCombination910C returns an empty combination (rather +// than panicking or fabricating a partial/incorrect pairing) when none of +// the candidate NPUs share a full physical module and no NetworkID is set +// anywhere in CustomInfo. It also verifies that Fit() turns that empty +// combination into a clean fit=false rejection, so such pods are correctly +// left unschedulable rather than crashing the scheduler or being silently +// under-allocated. +func TestComputeBestCombination910C_NoFullPairsReturnsEmpty(t *testing.T) { + dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} + nodeInfo := &device.NodeInfo{ + Node: &corev1.Node{}, + Devices: map[string][]device.DeviceInfo{ + Ascend910CType: { + // Four singleton NPUs, each alone on its own module + // (indices 0, 2, 4, 6 -> module IDs 0, 1, 2, 3), no + // NetworkID present anywhere in CustomInfo. + {ID: "dev-0", Index: 0, CustomInfo: map[string]any{}}, + {ID: "dev-2", Index: 2, CustomInfo: map[string]any{}}, + {ID: "dev-4", Index: 4, CustomInfo: map[string]any{}}, + {ID: "dev-6", Index: 6, CustomInfo: map[string]any{}}, + }, + }, + } + + // Unit-level check: computeBestCombination910C itself must return an + // empty slice here, not panic and not fabricate a mismatched pairing. + candidates := device.ContainerDevices{ + {Idx: 0, UUID: "dev-0"}, + {Idx: 2, UUID: "dev-2"}, + {Idx: 4, UUID: "dev-4"}, + {Idx: 6, UUID: "dev-6"}, + } + combination := dev.computeBestCombination910C(nodeInfo, 4, candidates) + if len(combination) != 0 { + t.Errorf("expected computeBestCombination910C to return an empty combination when no candidates share a full module, got %d devices", len(combination)) + } + + // End-to-end check: Fit() must turn that empty combination into a clean + // rejection, not a panic and not a false "success". + devices := []*device.DeviceUsage{ + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + {ID: "dev-4", Index: 4, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + {ID: "dev-6", Index: 6, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + } + req := device.ContainerDeviceRequest{Nums: 4, Type: Ascend910CType} + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod-no-netid-no-pairs"}} + + fit, tmpDevs, reason := dev.Fit(devices, req, pod, nodeInfo, nil) + if fit { + t.Errorf("expected fit=false when no candidates without NetworkID form full module pairs, got fit=true, allocated=%d, reason=%q", + len(tmpDevs[Ascend910CType]), reason) + } +} From f4f05fffb5583e382ca32143dcffb32e9c6b7c40 Mon Sep 17 00:00:00 2001 From: Prabal Poddar Date: Wed, 5 Aug 2026 00:54:00 +0530 Subject: [PATCH 4/6] test(ascend): set Health: true in 910C pairing unit tests Signed-off-by: Prabal Poddar --- pkg/device/ascend/device_910c_pairing_test.go | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/pkg/device/ascend/device_910c_pairing_test.go b/pkg/device/ascend/device_910c_pairing_test.go index f0e425c65c..050439a9ba 100644 --- a/pkg/device/ascend/device_910c_pairing_test.go +++ b/pkg/device/ascend/device_910c_pairing_test.go @@ -44,11 +44,11 @@ func TestAscend910C_FitPartialAllocationBug(t *testing.T) { }, } devices := []*device.DeviceUsage{ - {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, - {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, - {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, - {ID: "dev-4", Index: 4, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, - {ID: "dev-6", Index: 6, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-4", Index: 4, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-6", Index: 6, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, } req := device.ContainerDeviceRequest{Nums: 4, Type: Ascend910CType} pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod"}} @@ -80,8 +80,8 @@ func TestAscend910C_FitExactCountBypassBug(t *testing.T) { }, } devices := []*device.DeviceUsage{ - {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, - {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, } req := device.ContainerDeviceRequest{Nums: 2, Type: Ascend910CType} pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod-2"}} @@ -110,8 +110,8 @@ func TestAscend910C_FitFullPairSucceeds(t *testing.T) { }, } devices := []*device.DeviceUsage{ - {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, - {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, + {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{"NetworkID": float64(0)}}, } req := device.ContainerDeviceRequest{Nums: 2, Type: Ascend910CType} pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod-3"}} @@ -140,8 +140,8 @@ func TestAscend910C_FitWithoutNetworkID_ValidatesPairing(t *testing.T) { }, } devices := []*device.DeviceUsage{ - {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, - {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, + {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, } req := device.ContainerDeviceRequest{Nums: 2, Type: Ascend910CType} pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod-no-netid"}} @@ -159,8 +159,8 @@ func TestAscend910C_FitWithoutNetworkID_ValidatesPairing(t *testing.T) { {ID: "dev-1", Index: 1, CustomInfo: map[string]any{}}, } devicesPair := []*device.DeviceUsage{ - {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, - {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, + {ID: "dev-1", Index: 1, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, } fitPair, tmpDevsPair, reasonPair := dev.Fit(devicesPair, req, pod, nodeInfo, nil) if !fitPair || len(tmpDevsPair[Ascend910CType]) != 2 { @@ -210,10 +210,10 @@ func TestComputeBestCombination910C_NoFullPairsReturnsEmpty(t *testing.T) { // End-to-end check: Fit() must turn that empty combination into a clean // rejection, not a panic and not a false "success". devices := []*device.DeviceUsage{ - {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, - {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, - {ID: "dev-4", Index: 4, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, - {ID: "dev-6", Index: 6, Count: 1, Used: 0, Totalmem: 32000, CustomInfo: map[string]any{}}, + {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, + {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, + {ID: "dev-4", Index: 4, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, + {ID: "dev-6", Index: 6, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, } req := device.ContainerDeviceRequest{Nums: 4, Type: Ascend910CType} pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod-no-netid-no-pairs"}} From 6fa265b1bdc097ae6941f419b6d0cc10f60dcbb6 Mon Sep 17 00:00:00 2001 From: Prabal Poddar Date: Wed, 5 Aug 2026 01:56:37 +0530 Subject: [PATCH 5/6] fix(ascend): fix lint and strengthen test reason assertions Apply De Morgan's law to QF1001 staticcheck findings in Fit(). Add AllocatedCardsInsufficientRequest reason checks to 910C pairing tests. Signed-off-by: Prabal Poddar --- pkg/device/ascend/device.go | 4 ++-- pkg/device/ascend/device_910c_pairing_test.go | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/device/ascend/device.go b/pkg/device/ascend/device.go index ebb8f006b2..0922a68ff1 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -541,7 +541,7 @@ func (npu *Devices) Fit(devices []*device.DeviceUsage, request device.ContainerD } if k.Nums > 0 { klog.V(5).InfoS("find fit device", "pod", klog.KObj(pod), "device", dev.ID) - if !needTopology && !(k.Type == Ascend910CType && originReq > 1) { + if !needTopology && (k.Type != Ascend910CType || originReq <= 1) { k.Nums-- } tmpDevs[k.Type] = append(tmpDevs[k.Type], device.ContainerDevice{ @@ -553,7 +553,7 @@ func (npu *Devices) Fit(devices []*device.DeviceUsage, request device.ContainerD CustomInfo: dev.CustomInfo, }) } - if k.Nums == 0 && !needTopology && !(k.Type == Ascend910CType && originReq > 1) { + if k.Nums == 0 && !needTopology && (k.Type != Ascend910CType || originReq <= 1) { klog.V(4).InfoS("device allocate success", "pod", klog.KObj(pod), "allocate device", tmpDevs) return true, tmpDevs, "" } diff --git a/pkg/device/ascend/device_910c_pairing_test.go b/pkg/device/ascend/device_910c_pairing_test.go index 050439a9ba..4baa4c9981 100644 --- a/pkg/device/ascend/device_910c_pairing_test.go +++ b/pkg/device/ascend/device_910c_pairing_test.go @@ -17,12 +17,14 @@ limitations under the License. package ascend import ( + "strings" "testing" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/Project-HAMi/HAMi/pkg/device" + "github.com/Project-HAMi/HAMi/pkg/device/common" ) // TestAscend910C_FitPartialAllocationBug reproduces the case where a pod @@ -59,6 +61,9 @@ func TestAscend910C_FitPartialAllocationBug(t *testing.T) { t.Errorf("expected fit=false when only 2 of 4 requested NPUs form full module pairs, got fit=true, allocated=%d, reason=%q", len(tmpDevs[Ascend910CType]), reason) } + if !strings.Contains(reason, common.AllocatedCardsInsufficientRequest) { + t.Errorf("expected reason to contain %q, got %q", common.AllocatedCardsInsufficientRequest, reason) + } } // TestAscend910C_FitExactCountBypassBug reproduces the L568-571 bypass: a @@ -92,6 +97,9 @@ func TestAscend910C_FitExactCountBypassBug(t *testing.T) { t.Errorf("expected fit=false when the 2 candidate NPUs come from two different partial modules (not one full pair), got fit=true, allocated=%d, reason=%q", len(tmpDevs[Ascend910CType]), reason) } + if !strings.Contains(reason, common.AllocatedCardsInsufficientRequest) { + t.Errorf("expected reason to contain %q, got %q", common.AllocatedCardsInsufficientRequest, reason) + } } // TestAscend910C_FitFullPairSucceeds is the positive-path sanity check: From a4ef56c6ae1c8f77780996f0b3b6b2a24a75d06a Mon Sep 17 00:00:00 2001 From: Prabal Poddar Date: Thu, 6 Aug 2026 14:39:11 +0530 Subject: [PATCH 6/6] refactor(ascend): clean up redundant comments in 910C pairing logic and tests Signed-off-by: Prabal Poddar --- pkg/device/ascend/device.go | 17 +------- pkg/device/ascend/device_910c_pairing_test.go | 40 +++---------------- 2 files changed, 6 insertions(+), 51 deletions(-) diff --git a/pkg/device/ascend/device.go b/pkg/device/ascend/device.go index 0922a68ff1..05288282a3 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -560,15 +560,9 @@ func (npu *Devices) Fit(devices []*device.DeviceUsage, request device.ContainerD } if k.Type == Ascend910CType && originReq > 1 { - // Ascend 910C requires full module-pair allocation (2 NPUs per - // physical card). Always run the pairing filter here, even when - // NetworkID is absent or needTopology is false, because - // candidates satisfying the count alone may still be spread - // across incomplete/partial modules rather than full pairs. + // Ascend 910C requires full module-pair allocation (2 NPUs per physical card). combination := npu.computeBestCombination910C(nodeInfo, int(originReq), tmpDevs[k.Type]) if len(combination) != int(originReq) { - // Never report success on a short allocation: doing so silently - // under-allocates NPUs relative to what the pod requested. reason[common.AllocatedCardsInsufficientRequest] = len(combination) klog.V(5).InfoS(common.AllocatedCardsInsufficientRequest, "pod", klog.KObj(pod), "request", originReq, "allocated", len(combination)) return false, tmpDevs, common.GenReason(reason, int(originReq)) @@ -661,7 +655,6 @@ func (npudev *Devices) computeBestCombination(nodeInfo *device.NodeInfo, reqNum } func (npudev *Devices) computeBestCombination910C(nodeInfo *device.NodeInfo, reqNum int, containerDevices device.ContainerDevices) device.ContainerDevices { - // Build a mapping from NPU index to device object for quick lookup. indexToDevice := make(map[int]device.ContainerDevice) var npuIndices []int for _, dev := range containerDevices { @@ -673,25 +666,17 @@ func (npudev *Devices) computeBestCombination910C(nodeInfo *device.NodeInfo, req // Each physical card hosts exactly 2 NPUs (Ascend 910C module design). const MaxCardNPUNum = 2 - // Group NPU indices by the module and Sort cardTopology := make(map[int][]int) for _, idx := range npuIndices { cardId := idx / MaxCardNPUNum cardTopology[cardId] = append(cardTopology[cardId], idx) } - // Convert the card topology map into a slice for sorting. cardTopSlice := make([][]int, 0, len(cardTopology)) for _, card := range cardTopology { cardTopSlice = append(cardTopSlice, card) } - // Sort cards by the number of available NPUs in descending order, so that - // full cards are considered before partial ones. Note: partial cards are - // still excluded outright below via the MaxCardNPUNum equality check, so - // this ordering only affects which full cards get picked first when more - // full cards are available than requested — it does not by itself change - // whether a request succeeds or fails. sort.Slice(cardTopSlice, func(i, j int) bool { return len(cardTopSlice[i]) > len(cardTopSlice[j]) }) diff --git a/pkg/device/ascend/device_910c_pairing_test.go b/pkg/device/ascend/device_910c_pairing_test.go index 4baa4c9981..6c3d264e3f 100644 --- a/pkg/device/ascend/device_910c_pairing_test.go +++ b/pkg/device/ascend/device_910c_pairing_test.go @@ -27,10 +27,7 @@ import ( "github.com/Project-HAMi/HAMi/pkg/device/common" ) -// TestAscend910C_FitPartialAllocationBug reproduces the case where a pod -// requests 4 NPUs but the node only has 1 full module (2 NPUs) plus 3 -// partial modules (1 NPU each). Before the fix, Fit() would incorrectly -// report success while allocating only 2 devices. +// TestAscend910C_FitPartialAllocationBug tests rejection when partial cards cannot fulfill the request. func TestAscend910C_FitPartialAllocationBug(t *testing.T) { dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} nodeInfo := &device.NodeInfo{ @@ -66,19 +63,13 @@ func TestAscend910C_FitPartialAllocationBug(t *testing.T) { } } -// TestAscend910C_FitExactCountBypassBug reproduces the L568-571 bypass: a -// pod requests 2 NPUs, and exactly 2 candidate NPUs exist on the node, but -// they sit on two different partial modules rather than one full pair. -// Before the fix, Fit() returned true without ever validating module -// pairing, because the candidate count already matched originReq. +// TestAscend910C_FitExactCountBypassBug tests pairing validation when candidate count matches originReq. func TestAscend910C_FitExactCountBypassBug(t *testing.T) { dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} nodeInfo := &device.NodeInfo{ Node: &corev1.Node{}, Devices: map[string][]device.DeviceInfo{ Ascend910CType: { - // dev-0 (module 0) and dev-2 (module 1) are each the sole - // occupied NPU of their respective module - no full pair. {ID: "dev-0", Index: 0, CustomInfo: map[string]any{"NetworkID": float64(0)}}, {ID: "dev-2", Index: 2, CustomInfo: map[string]any{"NetworkID": float64(0)}}, }, @@ -102,16 +93,13 @@ func TestAscend910C_FitExactCountBypassBug(t *testing.T) { } } -// TestAscend910C_FitFullPairSucceeds is the positive-path sanity check: -// a request for 2 NPUs against one genuinely full module (2 NPUs, same -// card) should still succeed after the fix. +// TestAscend910C_FitFullPairSucceeds tests successful allocation for full module pairs. func TestAscend910C_FitFullPairSucceeds(t *testing.T) { dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} nodeInfo := &device.NodeInfo{ Node: &corev1.Node{}, Devices: map[string][]device.DeviceInfo{ Ascend910CType: { - // Index 0 and 1 belong to the same module (idx/2 == 0 for both). {ID: "dev-0", Index: 0, CustomInfo: map[string]any{"NetworkID": float64(0)}}, {ID: "dev-1", Index: 1, CustomInfo: map[string]any{"NetworkID": float64(0)}}, }, @@ -132,16 +120,13 @@ func TestAscend910C_FitFullPairSucceeds(t *testing.T) { } } -// TestAscend910C_FitWithoutNetworkID_ValidatesPairing ensures that Ascend 910C -// full-pair validation runs for multi-device requests even when CustomInfo["NetworkID"] -// is absent on devices (needTopology=false). +// TestAscend910C_FitWithoutNetworkID_ValidatesPairing tests pairing validation when NetworkID is missing. func TestAscend910C_FitWithoutNetworkID_ValidatesPairing(t *testing.T) { dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} nodeInfo := &device.NodeInfo{ Node: &corev1.Node{}, Devices: map[string][]device.DeviceInfo{ Ascend910CType: { - // dev-0 (module 0) and dev-2 (module 1) without NetworkID in CustomInfo. {ID: "dev-0", Index: 0, CustomInfo: map[string]any{}}, {ID: "dev-2", Index: 2, CustomInfo: map[string]any{}}, }, @@ -161,7 +146,6 @@ func TestAscend910C_FitWithoutNetworkID_ValidatesPairing(t *testing.T) { len(tmpDevs[Ascend910CType]), reason) } - // Positive check without NetworkID when candidate devices form a full module pair (indices 0 and 1). nodeInfo.Devices[Ascend910CType] = []device.DeviceInfo{ {ID: "dev-0", Index: 0, CustomInfo: map[string]any{}}, {ID: "dev-1", Index: 1, CustomInfo: map[string]any{}}, @@ -177,23 +161,13 @@ func TestAscend910C_FitWithoutNetworkID_ValidatesPairing(t *testing.T) { } } -// TestComputeBestCombination910C_NoFullPairsReturnsEmpty directly answers -// whether computeBestCombination910C returns an empty combination (rather -// than panicking or fabricating a partial/incorrect pairing) when none of -// the candidate NPUs share a full physical module and no NetworkID is set -// anywhere in CustomInfo. It also verifies that Fit() turns that empty -// combination into a clean fit=false rejection, so such pods are correctly -// left unschedulable rather than crashing the scheduler or being silently -// under-allocated. +// TestComputeBestCombination910C_NoFullPairsReturnsEmpty tests empty combination when no candidate NPUs form a full module. func TestComputeBestCombination910C_NoFullPairsReturnsEmpty(t *testing.T) { dev := &Devices{config: VNPUConfig{CommonWord: Ascend910CType}} nodeInfo := &device.NodeInfo{ Node: &corev1.Node{}, Devices: map[string][]device.DeviceInfo{ Ascend910CType: { - // Four singleton NPUs, each alone on its own module - // (indices 0, 2, 4, 6 -> module IDs 0, 1, 2, 3), no - // NetworkID present anywhere in CustomInfo. {ID: "dev-0", Index: 0, CustomInfo: map[string]any{}}, {ID: "dev-2", Index: 2, CustomInfo: map[string]any{}}, {ID: "dev-4", Index: 4, CustomInfo: map[string]any{}}, @@ -202,8 +176,6 @@ func TestComputeBestCombination910C_NoFullPairsReturnsEmpty(t *testing.T) { }, } - // Unit-level check: computeBestCombination910C itself must return an - // empty slice here, not panic and not fabricate a mismatched pairing. candidates := device.ContainerDevices{ {Idx: 0, UUID: "dev-0"}, {Idx: 2, UUID: "dev-2"}, @@ -215,8 +187,6 @@ func TestComputeBestCombination910C_NoFullPairsReturnsEmpty(t *testing.T) { t.Errorf("expected computeBestCombination910C to return an empty combination when no candidates share a full module, got %d devices", len(combination)) } - // End-to-end check: Fit() must turn that empty combination into a clean - // rejection, not a panic and not a false "success". devices := []*device.DeviceUsage{ {ID: "dev-0", Index: 0, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}}, {ID: "dev-2", Index: 2, Count: 1, Used: 0, Totalmem: 32000, Health: true, CustomInfo: map[string]any{}},