fix (ascend) - validate module pair count and prevent under allocation for ascend 910C

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,12 +553,25 @@ 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 k.Type == Ascend910CType && originReq > 1 {
+		// 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) {
+			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)
@@ -568,13 +581,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)
@@ -648,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 {
@@ -660,22 +666,19 @@ 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 ascending order.
 	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.

pkg/device/ascend/device_910c_pairing_test.go

@@ -0,0 +1,204 @@
+/*
+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 (
+	"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 tests rejection when partial cards cannot fulfill the request.
+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, 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"}}
+
+	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)
+	}
+	if !strings.Contains(reason, common.AllocatedCardsInsufficientRequest) {
+		t.Errorf("expected reason to contain %q, got %q", common.AllocatedCardsInsufficientRequest, reason)
+	}
+}
+
+// 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: {
+				{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, 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"}}
+
+	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)
+	}
+	if !strings.Contains(reason, common.AllocatedCardsInsufficientRequest) {
+		t.Errorf("expected reason to contain %q, got %q", common.AllocatedCardsInsufficientRequest, reason)
+	}
+}
+
+// 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: {
+				{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, 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"}}
+
+	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)
+	}
+}
+
+// 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: {
+				{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, 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"}}
+
+	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)
+	}
+
+	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, 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 {
+		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)
+	}
+}
+
+// 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: {
+				{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{}},
+			},
+		},
+	}
+
+	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))
+	}
+
+	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{}},
+		{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"}}
+
+	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)
+	}
+}