diff --git a/pkg/device/conformance_test.go b/pkg/device/conformance_test.go new file mode 100644 index 0000000000..d9fa732340 --- /dev/null +++ b/pkg/device/conformance_test.go @@ -0,0 +1,355 @@ +/* +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. +*/ + +// This file holds a backend-agnostic conformance suite for the device.Devices +// interface (see devices.go). Every hardware backend implements the same +// interface, but each is otherwise tested in isolation, so the same class of +// contract violation has repeatedly been fixed one backend at a time (e.g. nil +// map / nil pointer panics on the admission and Fit paths in PRs #2254 and +// #2294). The suite runs one shared set of contract assertions against every +// constructible backend so a regression in any of them fails here immediately. +// +// It lives in the external device_test package on purpose: the backend +// sub-packages import github.com/Project-HAMi/HAMi/pkg/device, so an internal +// (package device) test that imported them back would create an import cycle. +package device_test + +import ( + "math" + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + + "github.com/Project-HAMi/HAMi/pkg/device" + "github.com/Project-HAMi/HAMi/pkg/device/amd" + "github.com/Project-HAMi/HAMi/pkg/device/awsneuron" + "github.com/Project-HAMi/HAMi/pkg/device/biren" + "github.com/Project-HAMi/HAMi/pkg/device/cambricon" + "github.com/Project-HAMi/HAMi/pkg/device/enflame" + "github.com/Project-HAMi/HAMi/pkg/device/hygon" + "github.com/Project-HAMi/HAMi/pkg/device/kunlun" + "github.com/Project-HAMi/HAMi/pkg/device/metax" + "github.com/Project-HAMi/HAMi/pkg/device/mthreads" + "github.com/Project-HAMi/HAMi/pkg/device/nvidia" + "github.com/Project-HAMi/HAMi/pkg/device/vastai" +) + +// conformanceCase pairs a constructed backend with a human-readable name used as +// the subtest label. +type conformanceCase struct { + name string + dev device.Devices +} + +// conformanceCases constructs every backend that can be built from a plain +// in-memory config, using representative resource names that mirror the +// production defaults wired up in pkg/scheduler/config.InitDevicesWithConfig. +// +// The ascend and iluvatar backends are intentionally omitted for now: their +// constructors return a slice gated behind an enable flag and need per-template +// VNPU / vGPU configuration, so they are left for a follow-up that extends this +// registry rather than making the first pass more fragile. +func conformanceCases() []conformanceCase { + return []conformanceCase{ + {"nvidia", nvidia.InitNvidiaDevice(nvidia.NvidiaConfig{ + ResourceCountName: "nvidia.com/gpu", + ResourceMemoryName: "nvidia.com/gpumem", + ResourceMemoryPercentageName: "nvidia.com/gpumem-percentage", + ResourceCoreName: "nvidia.com/gpucores", + })}, + {"cambricon", cambricon.InitMLUDevice(cambricon.CambriconConfig{ + ResourceCountName: "cambricon.com/vmlu", + ResourceMemoryName: "cambricon.com/mlu.smlu.vmemory", + ResourceCoreName: "cambricon.com/mlu.smlu.vcore", + })}, + {"hygon", hygon.InitDCUDevice(hygon.HygonConfig{ + ResourceCountName: "hygon.com/dcunum", + ResourceMemoryName: "hygon.com/dcumem", + ResourceCoreName: "hygon.com/dcucores", + })}, + {"enflame-gcu", enflame.InitGCUDevice(enflame.EnflameConfig{ + ResourceNameGCU: "enflame.com/gcu", + })}, + {"enflame-vgcu", enflame.InitEnflameDevice(enflame.EnflameConfig{ + ResourceNameDRSGCU: "enflame.com/drs-gcu", + ResourceNameMemory: "enflame.com/gcu-memory", + ResourceNameCore: "enflame.com/gcu-core", + })}, + {"mthreads", mthreads.InitMthreadsDevice(mthreads.MthreadsConfig{ + ResourceCountName: "mthreads.com/vgpu", + ResourceMemoryName: "mthreads.com/sgpu-memory", + ResourceCoreName: "mthreads.com/sgpu-core", + })}, + {"metax-gpu", metax.InitMetaxDevice(metax.MetaxConfig{ + ResourceCountName: "metax-tech.com/gpu", + })}, + {"metax-sgpu", metax.InitMetaxSDevice(metax.MetaxConfig{ + ResourceVCountName: "metax-tech.com/sgpu", + ResourceVMemoryName: "metax-tech.com/vmemory", + ResourceVCoreName: "metax-tech.com/vcore", + })}, + {"kunlun", kunlun.InitKunlunDevice(kunlun.KunlunConfig{ + ResourceCountName: "kunlunxin.com/xpu", + })}, + {"kunlun-vxpu", kunlun.InitKunlunVDevice(kunlun.KunlunConfig{ + ResourceVCountName: "kunlunxin.com/vxpu", + ResourceVMemoryName: "kunlunxin.com/vxpu-memory", + })}, + {"awsneuron", awsneuron.InitAWSNeuronDevice(awsneuron.AWSNeuronConfig{ + ResourceCountName: "aws.amazon.com/neuron", + ResourceCoreName: "aws.amazon.com/neuroncore", + })}, + {"amd", amd.InitAMDGPUDevice(amd.AMDConfig{ + ResourceCountName: "amd.com/gpu", + ResourceMemoryName: "amd.com/gpumem", + ResourceCoreName: "amd.com/gpucores", + })}, + {"vastai", vastai.InitVastaiDevice(vastai.VastaiConfig{ + ResourceCountName: "vastaitech.com/vastai", + })}, + {"biren", biren.InitBirenDevice(biren.BirenConfig{ + ResourceCountName: "biren.com/gpu", + })}, + } +} + +// mustNotPanic runs f and fails the test (instead of crashing the whole run) if +// it panics. The contract is that these interface methods degrade gracefully on +// empty / unrelated input rather than panicking. +func mustNotPanic(t *testing.T, op string, f func()) { + t.Helper() + defer func() { + if r := recover(); r != nil { + t.Errorf("%s panicked, want graceful handling: %v", op, r) + } + }() + f() +} + +// TestConformanceRegistry is a guard on the suite itself: every case must have a +// name and a non-nil backend, so a constructor that silently returns nil cannot +// hide from the other conformance tests. +func TestConformanceRegistry(t *testing.T) { + cases := conformanceCases() + if len(cases) == 0 { + t.Fatal("conformanceCases() returned no backends") + } + for _, c := range cases { + if c.name == "" { + t.Errorf("conformance case with empty name for %T", c.dev) + } + if c.dev == nil { + t.Errorf("backend %q constructed as nil", c.name) + } + } +} + +// TestConformanceResourceNamesNonEmpty asserts every backend advertises at least +// one resource name. The scheduler and quota code route a request to a backend +// by these names, so a backend that returns none is effectively unreachable. +func TestConformanceResourceNamesNonEmpty(t *testing.T) { + for _, c := range conformanceCases() { + t.Run(c.name, func(t *testing.T) { + rn := c.dev.GetResourceNames() + if rn.ResourceCountName == "" && rn.ResourceMemoryName == "" && rn.ResourceCoreName == "" { + t.Errorf("GetResourceNames() returned no resource names; the backend is unreachable by the scheduler") + } + }) + } +} + +// TestConformanceUnrelatedContainerYieldsNoRequest asserts that a container +// requesting none of a backend's resources produces a zero-device request. The +// scheduler and ResourceQuota checks rely on Nums == 0 to skip such containers; +// a non-zero result would make an unrelated container look like a GPU consumer. +func TestConformanceUnrelatedContainerYieldsNoRequest(t *testing.T) { + for _, c := range conformanceCases() { + t.Run(c.name, func(t *testing.T) { + ctr := &corev1.Container{ + Name: "no-accelerator", + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + }, + } + var req device.ContainerDeviceRequest + mustNotPanic(t, "GenerateResourceRequests", func() { + req = c.dev.GenerateResourceRequests(ctr) + }) + if req.Nums != 0 { + t.Errorf("GenerateResourceRequests() = Nums %d for a container with no accelerator request; want 0", req.Nums) + } + }) + } +} + +// TestConformanceFitEmptyDevicesNoPanic asserts that fitting a real request +// against an empty candidate list returns false without panicking. This is the +// nil / empty-input path behind fixes such as #2294; on a node with no devices +// of a backend's type the scheduler still calls Fit for that backend. +func TestConformanceFitEmptyDevicesNoPanic(t *testing.T) { + pod := &corev1.Pod{} + nodeInfo := &device.NodeInfo{Node: &corev1.Node{}} + deviceLists := []struct { + name string + devices []*device.DeviceUsage + }{ + {"nil", nil}, + {"empty", []*device.DeviceUsage{}}, + } + for _, c := range conformanceCases() { + t.Run(c.name, func(t *testing.T) { + for _, dl := range deviceLists { + t.Run(dl.name, func(t *testing.T) { + allocated := device.PodDevices{} + req := device.ContainerDeviceRequest{Nums: 1, Type: c.dev.CommonWord()} + var fit bool + mustNotPanic(t, "Fit", func() { + fit, _, _ = c.dev.Fit(dl.devices, req, pod, nodeInfo, &allocated) + }) + if fit { + t.Errorf("Fit() = true for a 1-device request against a %s candidate list; want false", dl.name) + } + }) + } + }) + } +} + +// TestConformanceMutateAdmissionNoPanic asserts that admission mutation of a pod +// that requests none of a backend's resources does not panic. This is the nil / +// empty-map path behind fixes such as #2254. +func TestConformanceMutateAdmissionNoPanic(t *testing.T) { + for _, c := range conformanceCases() { + t.Run(c.name, func(t *testing.T) { + ctr := corev1.Container{ + Name: "no-accelerator", + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("1")}, + }, + } + pod := &corev1.Pod{Spec: corev1.PodSpec{Containers: []corev1.Container{ctr}}} + mustNotPanic(t, "MutateAdmission", func() { + _, _ = c.dev.MutateAdmission(&pod.Spec.Containers[0], pod) + }) + }) + } +} + +// overflowSkipList holds backends that currently violate the overflow invariant +// and have open tracking issues. Each entry must be paired with a linked issue +// number; backends remain here only until their fix merges. Only backends that +// are in conformanceCases() belong here — iluvatar is affected by the same bug +// (#2284) but is not yet in the registry (see the conformanceCases comment), so +// it is not listed. +var overflowSkipList = map[string]string{ + "cambricon": "#2278", // unchecked int32(memnum) at cambricon/device.go:257 + "mthreads": "#2284", // unchecked int32(memnum) at mthreads/device.go:227 +} + +// TestConformanceNoNegativeResourceRequestOnOverflow asserts that +// GenerateResourceRequests never returns a negative Nums, Memreq, or Coresreq +// for a container request whose values are in range before any backend-specific +// scale factor is applied. This guards the int32 overflow family of bugs tracked +// in #2278, #2284, and #2336: when a request × MemoryFactor exceeds math.MaxInt32, +// the unchecked conversion silently wraps to a negative value, which the scheduler +// and ResourceQuota machinery then treat as zero, allowing oversubscription. +// +// The invariant is: for a request that fits in int32 _after_ scaling, the result +// fields must be non-negative. Backends that legitimately reject an out-of-range +// request can return Nums == 0; they must not return negative fields. +func TestConformanceNoNegativeResourceRequestOnOverflow(t *testing.T) { + for _, c := range conformanceCases() { + t.Run(c.name, func(t *testing.T) { + if issue, skip := overflowSkipList[c.name]; skip { + t.Skipf("overflow invariant not yet enforced: open issue %s", issue) + } + + rn := c.dev.GetResourceNames() + if rn.ResourceCountName == "" { + t.Skip("backend has no count resource; overflow test not applicable") + } + + // Test case 1: request 1 device with a memory value that, when multiplied + // by a typical MemoryFactor (256 or 512), stays well within int32 range. + ctr := &corev1.Container{ + Name: "small-request", + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceName(rn.ResourceCountName): resource.MustParse("1"), + }, + }, + } + if rn.ResourceMemoryName != "" { + // 1000 MiB × 512 = 512000, well under math.MaxInt32 + ctr.Resources.Limits[corev1.ResourceName(rn.ResourceMemoryName)] = resource.MustParse("1000") + } + if rn.ResourceCoreName != "" { + ctr.Resources.Limits[corev1.ResourceName(rn.ResourceCoreName)] = resource.MustParse("50") + } + + var req device.ContainerDeviceRequest + mustNotPanic(t, "GenerateResourceRequests", func() { + req = c.dev.GenerateResourceRequests(ctr) + }) + + if req.Nums < 0 { + t.Errorf("GenerateResourceRequests() returned Nums = %d (negative); want >= 0", req.Nums) + } + if req.Memreq < 0 { + t.Errorf("GenerateResourceRequests() returned Memreq = %d (negative); want >= 0", req.Memreq) + } + if req.Coresreq < 0 { + t.Errorf("GenerateResourceRequests() returned Coresreq = %d (negative); want >= 0", req.Coresreq) + } + + // Test case 2: a larger memory request that would overflow with an unchecked + // int(largeValue) × factor conversion. A correct implementation either clamps + // or rejects; an incorrect one wraps negative. + if rn.ResourceMemoryName != "" && rn.MemoryFactor > 0 { + // Pick a value that overflows when scaled: (math.MaxInt32 / factor) + 1000 + overflowThreshold := int64(math.MaxInt32/rn.MemoryFactor) + 1000 + largeCtr := &corev1.Container{ + Name: "overflow-candidate", + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceName(rn.ResourceCountName): resource.MustParse("1"), + corev1.ResourceName(rn.ResourceMemoryName): *resource.NewQuantity(overflowThreshold, resource.DecimalSI), + }, + }, + } + + var largeReq device.ContainerDeviceRequest + mustNotPanic(t, "GenerateResourceRequests (overflow case)", func() { + largeReq = c.dev.GenerateResourceRequests(largeCtr) + }) + + // The backend may reject the request (Nums == 0) or clamp Memreq; it must + // not return a negative value. + if largeReq.Memreq < 0 { + t.Errorf("GenerateResourceRequests() for overflow-range memory (%d MiB) returned Memreq = %d (negative); "+ + "want >= 0 or Nums == 0 rejection", overflowThreshold, largeReq.Memreq) + } + if largeReq.Nums < 0 { + t.Errorf("GenerateResourceRequests() for overflow-range memory returned Nums = %d (negative); want >= 0", largeReq.Nums) + } + } + }) + } +} diff --git a/pkg/device/metax/sdevice.go b/pkg/device/metax/sdevice.go index d716fdc59c..7a5405a33c 100644 --- a/pkg/device/metax/sdevice.go +++ b/pkg/device/metax/sdevice.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "maps" + "math" "slices" "sort" "strconv" @@ -255,6 +256,14 @@ func (sdev *MetaxSDevices) GenerateResourceRequests(ctr *corev1.Container) devic } } + // Reject out-of-range memory requests that would overflow int32. + // MemoryFactor is 1024, so a Gi request without a unit multiplies by that; + // a request above math.MaxInt32 wraps negative on the unchecked cast below. + if mem < 0 || mem > math.MaxInt32 { + klog.Errorf("container<%s> metax-sgpu memory request %d MiB is out of range", ctr.Name, mem) + return device.ContainerDeviceRequest{} + } + memPercent := 0 if mem == 0 { memPercent = 100