diff --git a/pkg/device/metax/device.go b/pkg/device/metax/device.go index bcb909d210..b3891b696b 100644 --- a/pkg/device/metax/device.go +++ b/pkg/device/metax/device.go @@ -39,6 +39,12 @@ const ( MetaxGPUCommonWord = "Metax-GPU" MetaxAnnotationLoss = "metax-tech.com/gpu.topology.losses" MetaxAnnotationScore = "metax-tech.com/gpu.topology.scores" + + // metaxTopologyLossBase converts a topology "loss" (where lower is better) + // onto the same "higher is better" scale as the scores annotation, via + // metaxTopologyLossBase - loss. It must stay larger than the largest loss a + // node can publish so the converted score remains positive. + metaxTopologyLossBase = 2000 ) var ( @@ -165,54 +171,72 @@ func (dev *MetaxDevices) customFilterRule(allocated *device.PodDevices, request return true } -func parseMetaxAnnos(annos string, index int) float32 { +// parseMetaxAnnos parses a Metax topology annotation (a JSON object mapping a +// requested device count to a topology value) and returns the value for index +// together with whether it was found. A missing annotation entry, malformed +// JSON, or an index that is absent from the map all yield (0, false) so callers +// can distinguish "no usable topology data" from a genuine value of zero. +func parseMetaxAnnos(annos string, index int) (float32, bool) { scoreMap := map[int]int{} err := json.Unmarshal([]byte(annos), &scoreMap) if err != nil { klog.Warningf("annos[%s] Unmarshal failed, %v", annos, err) - return 0 + return 0, false } res, ok := scoreMap[index] if !ok { klog.Warningf("scoreMap[%v] not contains [%d]", scoreMap, index) - return 0 + return 0, false } - return float32(res) + return float32(res), true } +// ScoreNode returns a policy-independent score for the node following a +// "higher score is a better node" convention. The shared scheduler policy layer +// is responsible for weighting this score and adapting it to the active +// scheduling policy (for example, inverting it under the Spread policy). +// +// Metax publishes two topology annotations for the requested device count: a +// "scores" map where higher is better and a "losses" map where lower is better. +// The scores annotation is used directly; when it is absent or does not carry a +// usable value for the requested count, the losses annotation is converted onto +// the same "higher is better" scale. This keeps a node's topology preference +// effective regardless of which of the two annotations it publishes. A node +// that advertises no usable topology data scores a neutral 0 so it never +// outranks a node that does. func (dev *MetaxDevices) ScoreNode(node *corev1.Node, podDevices device.PodSingleDevice, previous []*device.DeviceUsage, policy string) float32 { sum := 0 for _, dev := range podDevices { sum += len(dev) } - res := float32(0) - if policy == string(util.NodeSchedulerPolicyBinpack) { - lossAnno, ok := node.Annotations[MetaxAnnotationLoss] - if ok { - // it's preferred to select the node with lower loss - loss := parseMetaxAnnos(lossAnno, sum) - res = 2000 - loss - - klog.InfoS("Detected annotations", "policy", policy, "key", MetaxAnnotationLoss, "value", lossAnno, "requesting", sum, "extract", loss) + if scoreAnno, ok := node.Annotations[MetaxAnnotationScore]; ok { + // it's preferred to select the node with higher score + if score, found := parseMetaxAnnos(scoreAnno, sum); found { + klog.InfoS("Detected annotations", "key", MetaxAnnotationScore, "value", scoreAnno, "requesting", sum, "extract", score) + return score } - } else if policy == string(util.NodeSchedulerPolicySpread) { - scoreAnno, ok := node.Annotations[MetaxAnnotationScore] - if ok { - // it's preferred to select the node with higher score - // But we have to give it a smaller value because of Spread policy - score := parseMetaxAnnos(scoreAnno, sum) - res = 2000 - score - - klog.InfoS("Detected annotations", "policy", policy, "key", MetaxAnnotationScore, "value", scoreAnno, "requesting", sum, "extract", score) + } + + if lossAnno, ok := node.Annotations[MetaxAnnotationLoss]; ok { + // it's preferred to select the node with lower loss, so convert the + // loss onto a "higher is better" scale. + if loss, found := parseMetaxAnnos(lossAnno, sum); found { + klog.InfoS("Detected annotations", "key", MetaxAnnotationLoss, "value", lossAnno, "requesting", sum, "extract", loss) + return metaxTopologyLossBase - loss } } - return res + return 0 } +// PolicyNeutralScore marks MetaxDevices as returning a policy-independent score +// from ScoreNode, so the shared scheduler policy layer owns the weighting and +// Spread-policy sign inversion. +func (dev *MetaxDevices) PolicyNeutralScore() {} + func (dev *MetaxDevices) AddResourceUsage(pod *corev1.Pod, n *device.DeviceUsage, ctr *device.ContainerDevice) error { n.Used++ n.Usedcores += ctr.Usedcores diff --git a/pkg/device/metax/device_test.go b/pkg/device/metax/device_test.go index 95ff9cf0cc..d6c8d3f55a 100644 --- a/pkg/device/metax/device_test.go +++ b/pkg/device/metax/device_test.go @@ -26,6 +26,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/Project-HAMi/HAMi/pkg/device" + "github.com/Project-HAMi/HAMi/pkg/util" ) func TestGetNodeDevices(t *testing.T) { @@ -100,56 +101,82 @@ func TestGetNodeDevices(t *testing.T) { func TestParseMetaxAnnos(t *testing.T) { tests := []struct { - name string - index int - value float32 + name string + index int + value float32 + wantFound bool }{ { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", - index: 1, - value: 0, + name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + index: 1, + value: 0, + wantFound: true, }, { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", - index: 2, - value: 110, + name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + index: 2, + value: 110, + wantFound: true, }, { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", - index: 3, - value: 270, + name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + index: 3, + value: 270, + wantFound: true, }, { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", - index: 4, - value: 540, + name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + index: 4, + value: 540, + wantFound: true, }, { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", - index: 5, - value: 580, + name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + index: 5, + value: 580, + wantFound: true, }, { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", - index: 6, - value: 730, + name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + index: 6, + value: 730, + wantFound: true, }, { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", - index: 7, - value: 930, + name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + index: 7, + value: 930, + wantFound: true, }, { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", - index: 8, - value: 1240, + name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + index: 8, + value: 1240, + wantFound: true, + }, + { + // index not present in the map: not found, neutral value. + name: "{\"1\":0,\"2\":110}", + index: 3, + value: 0, + wantFound: false, + }, + { + // malformed JSON: not found, neutral value. + name: "not-json", + index: 1, + value: 0, + wantFound: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - value := parseMetaxAnnos(tt.name, tt.index) + value, found := parseMetaxAnnos(tt.name, tt.index) if value != tt.value { - t.Errorf("Expected index %f, got %f", tt.value, value) + t.Errorf("Expected value %f, got %f", tt.value, value) + } + if found != tt.wantFound { + t.Errorf("Expected found %v, got %v", tt.wantFound, found) } }) } @@ -398,90 +425,145 @@ func Test_CustomFilterRule(t *testing.T) { } func Test_ScoreNode(t *testing.T) { + // twoDevices requests two GPUs, so the topology annotations are looked up + // with index 2. + twoDevices := device.PodSingleDevice{ + device.ContainerDevices{ + {Idx: 0, UUID: "test-0", Type: MetaxGPUDevice, Usedmem: 1000, Usedcores: 1}, + {Idx: 1, UUID: "test-1", Type: MetaxGPUDevice, Usedmem: 1000, Usedcores: 1}, + }, + } + tests := []struct { - name string - args struct { - node *corev1.Node - podDevices device.PodSingleDevice - policy string - } - want float32 + name string + node *corev1.Node + podDevices device.PodSingleDevice + want float32 }{ { - name: "policy is binpack", - args: struct { - node *corev1.Node - podDevices device.PodSingleDevice - policy string - }{ - node: &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - "metax-tech.com/gpu.topology.losses": "{\"1\":100,\"2\":200}", - }, + name: "scores annotation is used directly", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + MetaxAnnotationScore: "{\"1\":100,\"2\":200}", }, }, - podDevices: device.PodSingleDevice{ - device.ContainerDevices{ - { - Idx: int(0), - UUID: "test-0", - Type: MetaxGPUDevice, - Usedmem: int32(1000), - Usedcores: int32(1), - }, - { - Idx: int(1), - UUID: "test-1", - Type: MetaxGPUDevice, - Usedmem: int32(1000), - Usedcores: int32(1), - }, + }, + podDevices: twoDevices, + want: float32(200), + }, + { + name: "losses annotation is used when scores is absent", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + MetaxAnnotationLoss: "{\"1\":100,\"2\":200}", }, }, - policy: "binpack", }, - want: float32(1800), + podDevices: twoDevices, + want: float32(1800), }, { - name: "policy is spread", - args: struct { - node *corev1.Node - podDevices device.PodSingleDevice - policy string - }{ - node: &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - "metax-tech.com/gpu.topology.scores": "{\"1\":100,\"2\":200}", - }, + name: "scores is preferred over losses when both are present", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + MetaxAnnotationScore: "{\"2\":200}", + MetaxAnnotationLoss: "{\"2\":50}", }, }, - podDevices: device.PodSingleDevice{ - device.ContainerDevices{ - { - Idx: int(0), - UUID: "test-0", - Type: MetaxGPUDevice, - Usedmem: int32(1000), - Usedcores: int32(1), - }, + }, + podDevices: twoDevices, + want: float32(200), + }, + { + // A scores annotation that has no entry for the requested count must + // not suppress a usable losses annotation. + name: "falls back to losses when scores lacks the requested count", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + MetaxAnnotationScore: "{\"4\":200}", + MetaxAnnotationLoss: "{\"2\":50}", + }, + }, + }, + podDevices: twoDevices, + want: float32(1950), + }, + { + // A malformed scores annotation must not suppress a usable losses + // annotation. + name: "falls back to losses when scores is malformed", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + MetaxAnnotationScore: "not-json", + MetaxAnnotationLoss: "{\"2\":50}", + }, + }, + }, + podDevices: twoDevices, + want: float32(1950), + }, + { + // A losses annotation with no usable value must score a neutral 0, + // not the maximum (metaxTopologyLossBase - 0), so a node with no + // topology data never outranks one that has it. + name: "losses without the requested count scores neutral zero", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + MetaxAnnotationLoss: "{\"4\":50}", }, }, - policy: "spread", }, - want: float32(1900), + podDevices: twoDevices, + want: float32(0), }, + { + name: "no topology annotation scores zero", + node: &corev1.Node{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}}, + podDevices: twoDevices, + want: float32(0), + }, + } + + // ScoreNode must be policy-independent: the same inputs produce the same + // score regardless of the scheduling policy. The shared scheduler policy + // layer (OverrideScore) is responsible for adapting the score to binpack or + // spread. + policies := []string{ + util.NodeSchedulerPolicyBinpack.String(), + util.NodeSchedulerPolicySpread.String(), + "", } for _, test := range tests { t.Run(test.name, func(t *testing.T) { dev := MetaxDevices{} - result := dev.ScoreNode(test.args.node, test.args.podDevices, []*device.DeviceUsage{}, test.args.policy) - assert.DeepEqual(t, result, test.want) + for _, policy := range policies { + result := dev.ScoreNode(test.node, test.podDevices, []*device.DeviceUsage{}, policy) + assert.DeepEqual(t, result, test.want) + } }) } } +// TestMetaxDevicesImplementsPolicyNeutralScorer verifies that MetaxDevices +// exposes the PolicyNeutralScore marker method, which is how the shared +// scheduler policy layer detects that ScoreNode is policy-independent and +// applies the weight and Spread sign inversion. +func TestMetaxDevicesImplementsPolicyNeutralScorer(t *testing.T) { + type policyNeutralScorer interface { + PolicyNeutralScore() + } + var dev any = &MetaxDevices{} + if _, ok := dev.(policyNeutralScorer); !ok { + t.Errorf("MetaxDevices does not implement the PolicyNeutralScore marker") + } +} + func TestMetaxDevices_Fit(t *testing.T) { config := MetaxConfig{ ResourceCountName: "metax-tech.com/gpu", diff --git a/pkg/scheduler/policy/node_policy_test.go b/pkg/scheduler/policy/node_policy_test.go index 79d46232fa..b78188dc08 100644 --- a/pkg/scheduler/policy/node_policy_test.go +++ b/pkg/scheduler/policy/node_policy_test.go @@ -17,6 +17,8 @@ limitations under the License. package policy import ( + "fmt" + "sort" "testing" "k8s.io/klog/v2" @@ -24,6 +26,7 @@ import ( "github.com/Project-HAMi/HAMi/pkg/device" "github.com/Project-HAMi/HAMi/pkg/device/nvidia" "github.com/Project-HAMi/HAMi/pkg/scheduler/config" + "github.com/Project-HAMi/HAMi/pkg/util" "gotest.tools/v3/assert" corev1 "k8s.io/api/core/v1" @@ -255,8 +258,53 @@ func TestOverrideScore(t *testing.T) { Usedmem: 0, }, }, + // Metax-GPU implements the policy-neutral scorer, so OverrideScore + // weights its raw "higher is better" score by 10000 under Binpack. + // The node only carries the losses annotation, so the raw score + // falls back to 2000 - loss = 2000 - 321 = 1679 for the requested + // two devices, giving a weighted result of 16790000. policy: "binpack", - wantScore: 1679, + wantScore: 16790000, + }, + { + // Under Spread the same policy-neutral raw score of 1679 is inverted + // (weight -10000), producing -16790000. Because Binpack picks the + // highest score and Spread picks the lowest, inverting the sign + // preserves the node ranking across both policies. + name: "Metax-GPU with spread policy returns inverted weighted score", + nodeScore: &NodeScore{ + Node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + Annotations: map[string]string{ + "metax-tech.com/gpu.topology.losses": "{\"1\":123,\"2\":321}", + }, + }, + }, + NodeID: "node1", + Devices: device.PodDevices{ + "Metax-GPU": device.PodSingleDevice{ + device.ContainerDevices{ + {Idx: 1, UUID: "uuid1", Type: "gpu", Usedmem: 1024, Usedcores: 2}, + {Idx: 2, UUID: "uuid2", Type: "gpu", Usedmem: 2048, Usedcores: 4}, + }, + }, + }, + Score: 0, + }, + devices: []*device.DeviceUsage{ + { + Count: 4, + Totalcore: 8, + Totalmem: 4096, + Type: "gpu", + Used: 0, + Usedcores: 0, + Usedmem: 0, + }, + }, + policy: "spread", + wantScore: -16790000, }, { name: "Device score equal to zero", @@ -413,6 +461,138 @@ func TestOverrideScore(t *testing.T) { } } +// TestOverrideScoreMetaxGPUOrderingUnchanged verifies that decoupling +// MetaxDevices.ScoreNode from the scheduler policy string does not change which +// node wins for Metax-GPU. +// +// The pre-decoupling code implemented two distinct selection rules: under +// Binpack it preferred the node with the lowest topology loss, and under Spread +// it preferred the node with the highest topology score. When a node advertises +// both annotations they describe the same underlying topology preference, so the +// lowest-loss node is also the highest-score node. This test builds such +// consistent nodes, derives the winner each original rule would have chosen, and +// asserts that the new single policy-neutral score — once weighted and sorted by +// the shared OverrideScore/Less layer exactly as the scheduler does — selects the +// same node under both policies. +func TestOverrideScoreMetaxGPUOrderingUnchanged(t *testing.T) { + setup(t, &config.Config{ + NvidiaConfig: nvidia.NvidiaConfig{ + ResourceCountName: "hami.io/gpu", + ResourceMemoryName: "hami.io/gpumem", + ResourceMemoryPercentageName: "hami.io/gpumem-percentage", + ResourceCoreName: "hami.io/gpucores", + DefaultGPUNum: 1, + }, + }) + + // Each node requests two Metax-GPU devices, so the topology annotations are + // looked up at index "2". + type metaxNode struct { + name string + loss int + score int + } + + // buildNode returns a NodeScore that advertises both topology annotations for + // the two requested devices. + buildNode := func(n metaxNode) *NodeScore { + return &NodeScore{ + NodeID: n.name, + Node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: n.name, + Annotations: map[string]string{ + "metax-tech.com/gpu.topology.losses": fmt.Sprintf("{\"2\":%d}", n.loss), + "metax-tech.com/gpu.topology.scores": fmt.Sprintf("{\"2\":%d}", n.score), + }, + }, + }, + Devices: device.PodDevices{ + "Metax-GPU": device.PodSingleDevice{ + device.ContainerDevices{ + {Idx: 0, UUID: n.name + "-0", Type: "gpu", Usedmem: 1024, Usedcores: 2}, + {Idx: 1, UUID: n.name + "-1", Type: "gpu", Usedmem: 1024, Usedcores: 2}, + }, + }, + }, + } + } + + // originalBinpackWinner is the node the pre-decoupling code selected under + // Binpack: the lowest topology loss. + originalBinpackWinner := func(nodes []metaxNode) string { + best := nodes[0] + for _, n := range nodes[1:] { + if n.loss < best.loss { + best = n + } + } + return best.name + } + + // originalSpreadWinner is the node the pre-decoupling code selected under + // Spread: the highest topology score. + originalSpreadWinner := func(nodes []metaxNode) string { + best := nodes[0] + for _, n := range nodes[1:] { + if n.score > best.score { + best = n + } + } + return best.name + } + + // newWinner reproduces the scheduler's selection: weight every node with + // OverrideScore, sort with the policy-aware Less, and take the last element + // (see scheduler.go). + newWinner := func(nodes []metaxNode, policy string) string { + list := NodeScoreList{Policy: policy} + for _, n := range nodes { + ns := buildNode(n) + ns.OverrideScore([]*device.DeviceUsage{}, policy) + list.NodeList = append(list.NodeList, ns) + } + sort.Sort(&list) + return list.NodeList[len(list.NodeList)-1].NodeID + } + + tests := []struct { + name string + nodes []metaxNode + }{ + { + name: "best node has both lowest loss and highest score", + nodes: []metaxNode{ + {name: "node-a", loss: 300, score: 100}, + {name: "node-b", loss: 100, score: 300}, + {name: "node-c", loss: 200, score: 200}, + }, + }, + { + name: "best node is first in the list", + nodes: []metaxNode{ + {name: "node-a", loss: 50, score: 500}, + {name: "node-b", loss: 400, score: 100}, + {name: "node-c", loss: 250, score: 250}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + wantBinpack := originalBinpackWinner(tt.nodes) + wantSpread := originalSpreadWinner(tt.nodes) + // Consistent annotations must agree on the best node; otherwise a + // single policy-neutral score could not preserve both rankings and + // this test's premise would not hold. + assert.Equal(t, wantBinpack, wantSpread) + + assert.Equal(t, wantBinpack, newWinner(tt.nodes, util.NodeSchedulerPolicyBinpack.String())) + assert.Equal(t, wantSpread, newWinner(tt.nodes, util.NodeSchedulerPolicySpread.String())) + }) + } +} + func TestComputeDefaultScore(t *testing.T) { device1 := &device.DeviceUsage{ ID: "device1",