From 85ea6ac899945adf493e9eb25a00954b4a3001d4 Mon Sep 17 00:00:00 2001 From: Eshiv Pandey Date: Wed, 9 Sep 2026 23:02:03 +0530 Subject: [PATCH 1/6] fix(scheduler): fail closed on invalid HAMi device requests instead of binding GPU-less pods An invalid HAMi device request (for example an mthreads core limit outside 0-100) was silently dropped by GenerateResourceRequests, so the pod looked device-less, was bound to a node, and ran with no GPU at all. - Devices.GenerateResourceRequests now returns (request, error), so each backend can distinguish an invalid request from no request. - All 16 backends return device.ErrInvalidDeviceRequest on invalid input. - Resourcereqs propagates the error; Filter rejects the pod with a FilteringFailed event instead of returning all nodes. - The admission webhook returns the real error instead of a generic quota message. - mthreads and iluvatar: the MutateAdmission count*cores total is divided back to a per card value, and uneven totals are rejected. Signed-off-by: Eshiv Pandey --- pkg/device/amd/device.go | 12 ++-- pkg/device/amd/device_test.go | 18 +++--- pkg/device/ascend/device.go | 16 ++--- pkg/device/ascend/device_test.go | 12 ++-- pkg/device/awsneuron/device.go | 14 ++--- pkg/device/awsneuron/device_test.go | 2 +- pkg/device/awsneuron/device_wholecore_test.go | 4 +- pkg/device/biren/device.go | 8 +-- pkg/device/biren/device_test.go | 2 +- pkg/device/cambricon/device.go | 12 ++-- pkg/device/cambricon/device_test.go | 4 +- pkg/device/devices.go | 48 ++++++++++++--- pkg/device/devices_test.go | 16 ++--- pkg/device/enflame/device.go | 14 ++--- pkg/device/enflame/device_test.go | 6 +- pkg/device/enflame/gcu.go | 6 +- pkg/device/enflame/gcu_test.go | 2 +- pkg/device/hygon/device.go | 15 ++--- pkg/device/hygon/device_test.go | 8 +-- pkg/device/iluvatar/device.go | 33 +++++++--- pkg/device/iluvatar/device_test.go | 2 +- pkg/device/kunlun/device.go | 8 +-- pkg/device/kunlun/device_test.go | 2 +- pkg/device/kunlun/vdevice.go | 6 +- pkg/device/kunlun/vdevice_test.go | 2 +- pkg/device/metax/device.go | 8 +-- pkg/device/metax/device_test.go | 2 +- pkg/device/metax/sdevice.go | 16 ++--- pkg/device/metax/sdevice_test.go | 4 +- pkg/device/mthreads/device.go | 35 ++++++++--- pkg/device/mthreads/device_test.go | 61 ++++++++++++++++++- pkg/device/nvidia/device.go | 12 ++-- pkg/device/nvidia/device_test.go | 41 ++++++++++--- pkg/device/quota_test.go | 4 +- pkg/device/vastai/device.go | 8 +-- pkg/device/vastai/device_test.go | 2 +- pkg/scheduler/config/config_test.go | 3 +- pkg/scheduler/scheduler.go | 15 ++++- pkg/scheduler/scheduler_test.go | 36 ++++++++++- pkg/scheduler/score_test.go | 4 +- pkg/scheduler/webhook.go | 20 +++--- pkg/scheduler/webhook_test.go | 36 +++++------ 42 files changed, 392 insertions(+), 187 deletions(-) diff --git a/pkg/device/amd/device.go b/pkg/device/amd/device.go index c8a0916daf..cbdd50c223 100644 --- a/pkg/device/amd/device.go +++ b/pkg/device/amd/device.go @@ -200,7 +200,7 @@ func (dev *AMDDevices) GetResourceNames() device.ResourceNames { } } -func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count AMD devices for container ", ctr.Name) amdResourceCount := corev1.ResourceName(dev.resourceCountName) amdResourceMemory := corev1.ResourceName(dev.resourceMemoryName) @@ -210,7 +210,7 @@ func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co if n, ok := count.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "amd device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "amd", Reason: fmt.Sprintf("device count %d is out of range", n)} } memnum := int32(0) mem, memOK := ctr.Resources.Limits[amdResourceMemory] @@ -218,7 +218,7 @@ func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co memnums, ok := mem.AsInt64() if !ok || memnums < 0 || memnums > math.MaxInt32 { klog.ErrorS(nil, "amd device memory request is out of range", "container", ctr.Name, "request", mem.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "amd", Reason: fmt.Sprintf("memory request %s is out of range", mem.String())} } memnum = int32(memnums) } @@ -234,7 +234,7 @@ func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co corePercentageNums, ok := corePercentage.AsInt64() if !ok || corePercentageNums < 1 || corePercentageNums > 100 { klog.ErrorS(nil, "amd device core percentage request is out of range", "container", ctr.Name, "request", corePercentage.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "amd", Reason: fmt.Sprintf("core percentage request %s is out of range (must be an integer between 1 and 100)", corePercentage.String())} } corePercentageNum = int32(corePercentageNums) } @@ -248,10 +248,10 @@ func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co Memreq: memnum, MemPercentagereq: 0, Coresreq: corePercentageNum, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *AMDDevices) ScoreNode(node *corev1.Node, podDevices device.PodSingleDevice, previous []*device.DeviceUsage, policy string) float32 { diff --git a/pkg/device/amd/device_test.go b/pkg/device/amd/device_test.go index 2d2dbac07c..68aec9b511 100644 --- a/pkg/device/amd/device_test.go +++ b/pkg/device/amd/device_test.go @@ -238,7 +238,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) assert.DeepEqual(t, device.ContainerDeviceRequest{ Nums: 2, Type: AMDDevice, @@ -257,7 +257,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) assert.DeepEqual(t, device.ContainerDeviceRequest{}, got) }) @@ -270,7 +270,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) assert.Equal(t, int32(100), got.Coresreq) }) @@ -285,7 +285,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) assert.Equal(t, int32(42), got.Coresreq) }) @@ -299,7 +299,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) assert.Equal(t, int32(cores), got.Coresreq) } for _, cores := range []int64{0, 101, 150, 200, -1} { @@ -311,7 +311,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) assert.DeepEqual(t, device.ContainerDeviceRequest{}, got) } for _, rawCore := range []string{"50m", "99.1"} { @@ -323,7 +323,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) assert.DeepEqual(t, device.ContainerDeviceRequest{}, got) } }) @@ -346,7 +346,9 @@ func Test_GenerateResourceRequests(t *testing.T) { } limits[tc.resource] = *resource.NewQuantity(tc.value, resource.DecimalSI) ctr := &corev1.Container{Resources: corev1.ResourceRequirements{Limits: limits}} - assert.DeepEqual(t, device.ContainerDeviceRequest{}, dev.GenerateResourceRequests(ctr)) + got, err := dev.GenerateResourceRequests(ctr) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, got) + assert.ErrorContains(t, err, "out of range") }) } } diff --git a/pkg/device/ascend/device.go b/pkg/device/ascend/device.go index 9054304be0..9a50cc9f8a 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -331,7 +331,7 @@ func (dev *Devices) CheckHealth(devType string, n *corev1.Node) (bool, bool) { return device.CheckHealth(devType, dev.GetResourceNames().ResourceCountName, n) } -func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { ascendResourceCount := corev1.ResourceName(dev.config.ResourceName) ascendResourceMem := corev1.ResourceName(dev.config.ResourceMemoryName) ascendResourceCore := corev1.ResourceName(dev.config.ResourceCoreName) @@ -346,7 +346,7 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta klog.Info("Found AscendDevices devices") if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "ascend device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: dev.config.CommonWord, Reason: fmt.Sprintf("device count %d is out of range", n)} } memnum := 0 mem, ok := ctr.Resources.Limits[ascendResourceMem] @@ -357,7 +357,7 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta // Negative quantities such as -1m return ok=false from AsInt64, so reject by sign first. if mem.Sign() < 0 { klog.ErrorS(nil, "ascend device memory request is negative", "container", ctr.Name, "request", mem.String(), "device", dev.config.CommonWord) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: dev.config.CommonWord, Reason: fmt.Sprintf("memory request %s is negative", mem.String())} } memnums, ok := mem.AsInt64() if ok { @@ -365,7 +365,7 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta if memnums > math.MaxInt32 { klog.ErrorS(nil, "ascend device memory request is out of range; memory unit is treated as MB not Byte, so a quantity such as 16Gi is invalid, request 16384 for 16GB instead", "container", ctr.Name, "request", mem.String(), "device", dev.config.CommonWord) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: dev.config.CommonWord, Reason: fmt.Sprintf("memory request %s is out of range; memory unit is treated as MB not Byte", mem.String())} } if dev.config.MemoryFactor > 1 { rawMemnums := memnums @@ -374,7 +374,7 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta if memnums > math.MaxInt32 { klog.ErrorS(nil, "ascend device memory request overflows int32 after applying memory factor; memory unit is treated as MB not Byte", "container", ctr.Name, "raw", rawMemnums, "scaled", memnums, "factor", dev.config.MemoryFactor) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: dev.config.CommonWord, Reason: fmt.Sprintf("memory request %d overflows int32 after applying memory factor %d", rawMemnums, dev.config.MemoryFactor)} } klog.V(4).Infof("Update Ascend memory request. before %d, after %d, factor %d", rawMemnums, memnums, dev.config.MemoryFactor) } @@ -393,7 +393,7 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta corenums, valid := cv.AsInt64() if !valid || corenums < 0 || corenums > 100 { klog.ErrorS(nil, "ascend device core request is out of range", "container", ctr.Name, "request", cv.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: dev.config.CommonWord, Reason: fmt.Sprintf("core request %s is out of range (must be an integer between 0 and 100)", cv.String())} } corenum = int32(corenums) } @@ -410,10 +410,10 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta Memreq: int32(memnum), MemPercentagereq: int32(mempnum), Coresreq: corenum, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *Devices) ScoreNode(node *corev1.Node, podDevices device.PodSingleDevice, previous []*device.DeviceUsage, policy string) float32 { diff --git a/pkg/device/ascend/device_test.go b/pkg/device/ascend/device_test.go index 9136de7e9b..5500d33d22 100644 --- a/pkg/device/ascend/device_test.go +++ b/pkg/device/ascend/device_test.go @@ -1533,7 +1533,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - result := dev.GenerateResourceRequests(&test.args) + result, _ := dev.GenerateResourceRequests(&test.args) assert.Equal(t, result, test.want) }) @@ -1583,7 +1583,7 @@ func Test_GenerateResourceRequests_VNPUCoreMode(t *testing.T) { }, }, } - result := dev.GenerateResourceRequests(&test.args) + result, _ := dev.GenerateResourceRequests(&test.args) assert.Equal(t, result, test.want) }) @@ -1728,7 +1728,7 @@ func Test_GenerateResourceRequestsFactor(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - result := test.dev.GenerateResourceRequests(&req) + result, _ := test.dev.GenerateResourceRequests(&req) assert.Equal(t, result, test.want) }) } @@ -1848,7 +1848,7 @@ func Test_GenerateResourceRequests_OutOfRangeValues(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - result := test.dev.GenerateResourceRequests(&test.args) + result, _ := test.dev.GenerateResourceRequests(&test.args) assert.Equal(t, result, test.want) }) } @@ -1908,7 +1908,7 @@ func Test_GenerateResourceRequests_MemoryFactorOverflow(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - result := test.dev.GenerateResourceRequests(&test.args) + result, _ := test.dev.GenerateResourceRequests(&test.args) assert.Equal(t, result, device.ContainerDeviceRequest{}) }) } @@ -3183,7 +3183,7 @@ func Test_GenerateResourceRequests_CoresValidation(t *testing.T) { }, }, } - req := dev.GenerateResourceRequests(ctr) + req, _ := dev.GenerateResourceRequests(ctr) if tt.wantReq { assert.Equal(t, int32(1), req.Nums) assert.Equal(t, int32(tt.cores), req.Coresreq) diff --git a/pkg/device/awsneuron/device.go b/pkg/device/awsneuron/device.go index 423b1033ed..29b54c9d34 100644 --- a/pkg/device/awsneuron/device.go +++ b/pkg/device/awsneuron/device.go @@ -286,7 +286,7 @@ func (dev *AWSNeuronDevices) GetResourceNames() device.ResourceNames { } } -func (dev *AWSNeuronDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *AWSNeuronDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count awsNeuron devices for container ", ctr.Name) awsResourceCount := corev1.ResourceName(dev.resourceCountName) awsResourceCores := corev1.ResourceName(dev.resourceCoreName) @@ -295,7 +295,7 @@ func (dev *AWSNeuronDevices) GenerateResourceRequests(ctr *corev1.Container) dev n, err := validateResourceRequest(v, dev.resourceCountName, maxAWSNeuronDeviceCount) if err != nil { klog.ErrorS(err, "Invalid awsNeuron device request", "container", ctr.Name) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "awsneuron", Reason: err.Error()} } klog.InfoS("Detected awsNeuron device request", "container", ctr.Name, @@ -306,19 +306,19 @@ func (dev *AWSNeuronDevices) GenerateResourceRequests(ctr *corev1.Container) dev Memreq: 0, MemPercentagereq: 0, Coresreq: int32(dev.coresPerDevice()), - } + }, nil } else { core, ok := resourceQuantity(ctr, awsResourceCores) if ok { n, err := validateResourceRequest(core, dev.resourceCoreName, maxAWSNeuronCoreCount) if err != nil { klog.ErrorS(err, "Invalid awsNeuron core request", "container", ctr.Name) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "awsneuron", Reason: err.Error()} } nums, coresreq, err := dev.splitCoreRequest(n) if err != nil { klog.ErrorS(err, "Invalid awsNeuron core request", "container", ctr.Name) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "awsneuron", Reason: err.Error()} } klog.InfoS("Detected awsNeuron device request", "container", ctr.Name, @@ -329,10 +329,10 @@ func (dev *AWSNeuronDevices) GenerateResourceRequests(ctr *corev1.Container) dev Memreq: 0, MemPercentagereq: 0, Coresreq: coresreq, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *AWSNeuronDevices) ScoreNode(node *corev1.Node, podDevices device.PodSingleDevice, previous []*device.DeviceUsage, policy string) float32 { diff --git a/pkg/device/awsneuron/device_test.go b/pkg/device/awsneuron/device_test.go index b67743241b..a09a6d9396 100644 --- a/pkg/device/awsneuron/device_test.go +++ b/pkg/device/awsneuron/device_test.go @@ -649,7 +649,7 @@ func Test_GenerateResourceRequests(t *testing.T) { } dev := InitAWSNeuronDevice(config) dev.coresPerAWSNeuron = 2 - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } diff --git a/pkg/device/awsneuron/device_wholecore_test.go b/pkg/device/awsneuron/device_wholecore_test.go index 83fcf12f2b..9c467f425a 100644 --- a/pkg/device/awsneuron/device_wholecore_test.go +++ b/pkg/device/awsneuron/device_wholecore_test.go @@ -47,7 +47,7 @@ func Test_GenerateResourceRequests_WholeDeviceCapsCores(t *testing.T) { }, }, } - req := dev.GenerateResourceRequests(ctr) + req, _ := dev.GenerateResourceRequests(ctr) assert.Equal(t, req.Coresreq, int32(maxCoresPerNeuronDevice)) } @@ -91,7 +91,7 @@ func Test_Fit_WholeDeviceNotShared(t *testing.T) { CustomInfo: map[string]any{AWSNodeType: "inf1.6xlarge"}, } podA := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "default", Annotations: map[string]string{}}} - wholeDevice := dev.GenerateResourceRequests(&corev1.Container{ + wholeDevice, _ := dev.GenerateResourceRequests(&corev1.Container{ Name: "ctr", Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{"aws.amazon.com/neuron": resource.MustParse("1")}, diff --git a/pkg/device/biren/device.go b/pkg/device/biren/device.go index 9373232684..a0e6389c33 100644 --- a/pkg/device/biren/device.go +++ b/pkg/device/biren/device.go @@ -125,7 +125,7 @@ func (dev *BirenDevices) CheckHealth(devType string, n *corev1.Node) (bool, bool return device.CheckHealth(devType, dev.GetResourceNames().ResourceCountName, n) } -func (dev *BirenDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *BirenDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.V(5).Info("Start to count biren devices for container ", ctr.Name) BirenResourceCount := corev1.ResourceName(BirenResourceCount) v, ok := ctr.Resources.Limits[BirenResourceCount] @@ -136,7 +136,7 @@ func (dev *BirenDevices) GenerateResourceRequests(ctr *corev1.Container) device. if n, ok := v.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "biren device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "biren", Reason: fmt.Sprintf("device count %d is out of range", n)} } klog.Info("Found biren devices") memnum := 0 @@ -149,10 +149,10 @@ func (dev *BirenDevices) GenerateResourceRequests(ctr *corev1.Container) device. Memreq: int32(memnum), MemPercentagereq: int32(mempnum), Coresreq: corenum, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *BirenDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd device.PodDevices) map[string]string { diff --git a/pkg/device/biren/device_test.go b/pkg/device/biren/device_test.go index 511ffea649..8fb33d15e1 100644 --- a/pkg/device/biren/device_test.go +++ b/pkg/device/biren/device_test.go @@ -469,7 +469,7 @@ func Test_GenerateResourceRequests(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { dev := BirenDevices{} - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } diff --git a/pkg/device/cambricon/device.go b/pkg/device/cambricon/device.go index e5c0494ddf..ce22e123c5 100644 --- a/pkg/device/cambricon/device.go +++ b/pkg/device/cambricon/device.go @@ -253,7 +253,7 @@ func (dev *CambriconDevices) checkType(annos map[string]string, d device.DeviceU return false, false, false } -func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count mlu devices for container ", ctr.Name) mluResourceCount := corev1.ResourceName(MLUResourceCount) mluResourceMem := corev1.ResourceName(MLUResourceMemory) @@ -269,7 +269,7 @@ func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) dev if n, ok := v.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "cambricon device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "cambricon", Reason: fmt.Sprintf("device count %d is out of range", n)} } klog.Info("Found cambricon devices") memnum := 0 @@ -284,7 +284,7 @@ func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) dev if !parsed || memnums < 0 || memnums > int64(math.MaxInt32)/int64(MemoryFactor) { klog.ErrorS(nil, "cambricon memory request is not a plain integer within the int32 range; rejecting to avoid silent under-allocation", "container", ctr.Name) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "cambricon", Reason: fmt.Sprintf("memory request %s is not a plain integer within the int32 range", mem.String())} } memnum = int(memnums) * MemoryFactor } @@ -297,7 +297,7 @@ func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) dev corenums, ok := core.AsInt64() if !ok || corenums < 0 || corenums > 100 { klog.ErrorS(nil, "cambricon core request is out of range (must be 0-100)", "container", ctr.Name, "request", core.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "cambricon", Reason: fmt.Sprintf("core request %s is out of range (must be 0-100)", core.String())} } corenum = int32(corenums) } @@ -313,12 +313,12 @@ func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) dev Memreq: int32(memnum), MemPercentagereq: int32(mempnum), Coresreq: corenum, - } + }, nil } } return device.ContainerDeviceRequest{ Nums: 0, - } + }, nil } func (dev *CambriconDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd device.PodDevices) map[string]string { diff --git a/pkg/device/cambricon/device_test.go b/pkg/device/cambricon/device_test.go index 31b7aa591c..91260e7dfd 100644 --- a/pkg/device/cambricon/device_test.go +++ b/pkg/device/cambricon/device_test.go @@ -664,7 +664,7 @@ func Test_GenerateResourceRequests(t *testing.T) { } InitMLUDevice(config) dev := CambriconDevices{} - result := dev.GenerateResourceRequests(&test.args) + result, _ := dev.GenerateResourceRequests(&test.args) assert.Equal(t, test.want, result) }) } @@ -1937,7 +1937,7 @@ func Test_GenerateResourceRequests_CoresValidation(t *testing.T) { }, }, } - req := dev.GenerateResourceRequests(ctr) + req, _ := dev.GenerateResourceRequests(ctr) if tt.wantReq { assert.Equal(t, int32(1), req.Nums) assert.Equal(t, int32(tt.cores), req.Coresreq) diff --git a/pkg/device/devices.go b/pkg/device/devices.go index 7fd0f89ed4..a545f499d6 100644 --- a/pkg/device/devices.go +++ b/pkg/device/devices.go @@ -40,7 +40,12 @@ type Devices interface { GetNodeDevices(n corev1.Node) ([]*DeviceInfo, error) LockNode(n *corev1.Node, p *corev1.Pod) error ReleaseNodeLock(n *corev1.Node, p *corev1.Pod) error - GenerateResourceRequests(ctr *corev1.Container) ContainerDeviceRequest + // GenerateResourceRequests translates a container's resource limits into a + // device request. A zero Nums with a nil error means the container does not + // request this vendor's devices. A non-nil error means it does, but the + // request is invalid and the caller must fail closed instead of silently + // treating the pod as device-less. + GenerateResourceRequests(ctr *corev1.Container) (ContainerDeviceRequest, error) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd PodDevices) map[string]string ScoreNode(node *corev1.Node, podDevices PodSingleDevice, previous []*DeviceUsage, policy string) float32 AddResourceUsage(pod *corev1.Pod, n *DeviceUsage, ctr *ContainerDevice) error @@ -668,7 +673,24 @@ func ExtractMigTemplatesFromUUID(uuid string) (int, int, error) { return templateIdx, slotIdx, nil } -func Resourcereqs(pod *corev1.Pod) (counts PodDeviceRequests) { +// ErrInvalidDeviceRequest is returned by GenerateResourceRequests when a +// container declares this vendor's resources but the values cannot form a +// valid request. Callers must treat it as a hard failure: dropping the entry +// would make the pod look device-less and let it schedule with no device. +type ErrInvalidDeviceRequest struct { + Container string + Device string + Reason string +} + +func (e *ErrInvalidDeviceRequest) Error() string { + return fmt.Sprintf("invalid %s request for container %q: %s", e.Device, e.Container, e.Reason) +} + +// Resourcereqs collects the device requests of every container. A container +// whose request is invalid aborts the collection with the backend's error, so +// the scheduler rejects the pod instead of scheduling it with no device. +func Resourcereqs(pod *corev1.Pod) (counts PodDeviceRequests, err error) { // Total containers = init containers + regular containers totalContainers := len(pod.Spec.InitContainers) + len(pod.Spec.Containers) counts = make(PodDeviceRequests, totalContainers) @@ -689,7 +711,10 @@ func Resourcereqs(pod *corev1.Pod) (counts PodDeviceRequests) { "containerIndex", i, "containerName", pod.Spec.InitContainers[i].Name) for idx, val := range devices { - request := val.GenerateResourceRequests(&pod.Spec.InitContainers[i]) + request, reqErr := val.GenerateResourceRequests(&pod.Spec.InitContainers[i]) + if reqErr != nil { + return nil, reqErr + } if request.Nums > 0 { cnt += request.Nums counts[i][idx] = request @@ -707,7 +732,10 @@ func Resourcereqs(pod *corev1.Pod) (counts PodDeviceRequests) { "containerIndex", initContainerOffset+i, "containerName", pod.Spec.Containers[i].Name) for idx, val := range devices { - request := val.GenerateResourceRequests(&pod.Spec.Containers[i]) + request, reqErr := val.GenerateResourceRequests(&pod.Spec.Containers[i]) + if reqErr != nil { + return nil, reqErr + } if request.Nums > 0 { cnt += request.Nums counts[initContainerOffset+i][idx] = request @@ -719,7 +747,7 @@ func Resourcereqs(pod *corev1.Pod) (counts PodDeviceRequests) { } else { klog.V(4).InfoS("Resource requirements collected", "pod", klog.KObj(pod), "requests", counts) } - return counts + return counts, nil } func CheckUUID(annos map[string]string, id, useKey, noUseKey, deviceType string) bool { @@ -770,18 +798,22 @@ func CheckType(annos map[string]string, cardType, useKey, noUseKey string) bool } // PodRequiresDevice returns true if any container (init container or regular container) -// in the pod requests resources from the specified device generator. +// in the pod requests resources from the specified device generator. An invalid +// request still counts as requiring the device: the pod is not device-less and +// must be rejected rather than silently passed through. func PodRequiresDevice(dev Devices, p *corev1.Pod) bool { if p == nil || dev == nil { return false } for i := range p.Spec.InitContainers { - if dev.GenerateResourceRequests(&p.Spec.InitContainers[i]).Nums > 0 { + req, err := dev.GenerateResourceRequests(&p.Spec.InitContainers[i]) + if err != nil || req.Nums > 0 { return true } } for i := range p.Spec.Containers { - if dev.GenerateResourceRequests(&p.Spec.Containers[i]).Nums > 0 { + req, err := dev.GenerateResourceRequests(&p.Spec.Containers[i]) + if err != nil || req.Nums > 0 { return true } } diff --git a/pkg/device/devices_test.go b/pkg/device/devices_test.go index b8e26800d4..ab0c84ec53 100644 --- a/pkg/device/devices_test.go +++ b/pkg/device/devices_test.go @@ -1428,14 +1428,14 @@ func (m *mockDevices) GetNodeDevices(_ corev1.Node) ([]*DeviceInfo, error) { } func (m *mockDevices) LockNode(_ *corev1.Node, _ *corev1.Pod) error { return nil } func (m *mockDevices) ReleaseNodeLock(_ *corev1.Node, _ *corev1.Pod) error { return nil } -func (m *mockDevices) GenerateResourceRequests(ctr *corev1.Container) ContainerDeviceRequest { +func (m *mockDevices) GenerateResourceRequests(ctr *corev1.Container) (ContainerDeviceRequest, error) { // Return the mock request only if the container has the resource annotation we look for for rName := range ctr.Resources.Limits { if string(rName) == "nvidia.com/gpu" { - return m.resourceRequest + return m.resourceRequest, nil } } - return ContainerDeviceRequest{} + return ContainerDeviceRequest{}, nil } func (m *mockDevices) PatchAnnotations(_ *corev1.Pod, _ *map[string]string, _ PodDevices) map[string]string { return nil @@ -1485,7 +1485,7 @@ func TestResourcereqs_OnlyRegularContainers(t *testing.T) { }, } - counts := Resourcereqs(pod) + counts, _ := Resourcereqs(pod) // No init containers, so length == number of regular containers assert.Equal(t, len(counts), 1) @@ -1542,7 +1542,7 @@ func TestResourcereqs_WithInitContainers(t *testing.T) { }, } - counts := Resourcereqs(pod) + counts, _ := Resourcereqs(pod) assert.Equal(t, len(counts), 3, "Should have 3 container request maps") @@ -1568,7 +1568,7 @@ func TestResourcereqs_WithInitContainers(t *testing.T) { func TestResourcereqs_EmptyPod(t *testing.T) { pod := &corev1.Pod{Spec: corev1.PodSpec{}} - counts := Resourcereqs(pod) + counts, _ := Resourcereqs(pod) assert.Equal(t, len(counts), 0) } @@ -1612,7 +1612,7 @@ func TestResourcereqs_NoDeviceRequests(t *testing.T) { }, } - counts := Resourcereqs(pod) + counts, _ := Resourcereqs(pod) // Total = 1 init + 1 regular = 2 assert.Equal(t, len(counts), 2) @@ -1681,7 +1681,7 @@ func TestResourcereqs_MultipleInitAndRegularContainers(t *testing.T) { }, } - counts := Resourcereqs(pod) + counts, _ := Resourcereqs(pod) // Total = 2 init + 2 regular = 4 assert.Equal(t, len(counts), 4) diff --git a/pkg/device/enflame/device.go b/pkg/device/enflame/device.go index 1cc538f671..e49157f59b 100644 --- a/pkg/device/enflame/device.go +++ b/pkg/device/enflame/device.go @@ -315,7 +315,7 @@ func (dev *EnflameDevices) CheckHealth(devType string, n *corev1.Node) (bool, bo return true, true } -func (dev *EnflameDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *EnflameDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count enflame devices for container ", ctr.Name) resourceCount := corev1.ResourceName(EnflameResourceNameDRSGCU) v, ok := ctr.Resources.Limits[resourceCount] @@ -327,7 +327,7 @@ func (dev *EnflameDevices) GenerateResourceRequests(ctr *corev1.Container) devic klog.Info("Found enflame devices") if n > math.MaxInt32 { klog.ErrorS(nil, "drs request is too large", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "enflame", Reason: fmt.Sprintf("drs request %d is too large", n)} } return device.ContainerDeviceRequest{ Nums: 1, @@ -335,21 +335,21 @@ func (dev *EnflameDevices) GenerateResourceRequests(ctr *corev1.Container) devic Memreq: int32(n), MemPercentagereq: enflameRequestModeDirect, Coresreq: enflameUnknownCoreRequest, - } + }, nil } } memReq, hasMem := getContainerResourceRequest(ctr, corev1.ResourceName(EnflameResourceNameGCUMemory)) coreReq, hasCore := getContainerResourceRequest(ctr, corev1.ResourceName(EnflameResourceNameGCUCore)) if !hasMem && !hasCore { - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } if hasMem && memReq > math.MaxInt32 { klog.ErrorS(nil, "gcu memory request is too large", "container", ctr.Name, "request", memReq) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "enflame", Reason: fmt.Sprintf("gcu memory request %d is too large", memReq)} } if hasCore && (coreReq < 0 || coreReq > 100) { klog.ErrorS(nil, "gcu core request is out of range (must be 0-100)", "container", ctr.Name, "request", coreReq) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "enflame", Reason: fmt.Sprintf("gcu core request %d is out of range (must be 0-100)", coreReq)} } klog.Info("Found enflame memory/core based request") return device.ContainerDeviceRequest{ @@ -358,7 +358,7 @@ func (dev *EnflameDevices) GenerateResourceRequests(ctr *corev1.Container) devic Memreq: int32(memReq), MemPercentagereq: enflameRequestModeBySpec, Coresreq: int32(coreReq), - } + }, nil } func (dev *EnflameDevices) ScoreNode(node *corev1.Node, podDevices device.PodSingleDevice, previous []*device.DeviceUsage, policy string) float32 { diff --git a/pkg/device/enflame/device_test.go b/pkg/device/enflame/device_test.go index e68100d5b4..e61d071fe5 100644 --- a/pkg/device/enflame/device_test.go +++ b/pkg/device/enflame/device_test.go @@ -128,7 +128,7 @@ func TestGenerateResourceRequests(t *testing.T) { }, }, } - req := dev.GenerateResourceRequests(container) + req, _ := dev.GenerateResourceRequests(container) assert.Equal(t, req.Nums, int32(1)) assert.Equal(t, req.Memreq, int32(3)) assert.Equal(t, req.MemPercentagereq, enflameRequestModeDirect) @@ -149,7 +149,7 @@ func TestGenerateResourceRequests_ByMemoryCore(t *testing.T) { }, }, } - req := dev.GenerateResourceRequests(container) + req, _ := dev.GenerateResourceRequests(container) assert.Equal(t, req.Nums, int32(1)) assert.Equal(t, req.Type, EnflameVGCUDevice) assert.Equal(t, req.Memreq, int32(20480)) @@ -571,7 +571,7 @@ func Test_GenerateResourceRequests_CoresValidation(t *testing.T) { }, }, } - req := dev.GenerateResourceRequests(ctr) + req, _ := dev.GenerateResourceRequests(ctr) if tt.wantReq { assert.Equal(t, req.Nums, int32(1)) assert.Equal(t, req.Coresreq, int32(tt.cores)) diff --git a/pkg/device/enflame/gcu.go b/pkg/device/enflame/gcu.go index 642adbe37f..521ebbff9e 100644 --- a/pkg/device/enflame/gcu.go +++ b/pkg/device/enflame/gcu.go @@ -88,7 +88,7 @@ func (dev *GCUDevices) GetNodeDevices(n corev1.Node) ([]*device.DeviceInfo, erro return nodedevices, nil } -func (dev *GCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *GCUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count enflame devices for container ", ctr.Name) enflameResourceCount := corev1.ResourceName(EnflameResourceNameGCU) v, ok := ctr.Resources.Limits[enflameResourceCount] @@ -104,10 +104,10 @@ func (dev *GCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co Memreq: 100, MemPercentagereq: 100, Coresreq: 100, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *GCUDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd device.PodDevices) map[string]string { diff --git a/pkg/device/enflame/gcu_test.go b/pkg/device/enflame/gcu_test.go index cb0df277c7..9b4dbdb84a 100644 --- a/pkg/device/enflame/gcu_test.go +++ b/pkg/device/enflame/gcu_test.go @@ -424,7 +424,7 @@ func TestGCUDevices_GenerateResourceRequests(t *testing.T) { } InitGCUDevice(config) dev := &GCUDevices{} - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } diff --git a/pkg/device/hygon/device.go b/pkg/device/hygon/device.go index 8d4684eeb4..45f3c7e6ba 100644 --- a/pkg/device/hygon/device.go +++ b/pkg/device/hygon/device.go @@ -19,6 +19,7 @@ package hygon import ( "errors" "flag" + "fmt" "math" "slices" "strings" @@ -151,7 +152,7 @@ func (dev *HCUDevices) checkType(annos map[string]string, d device.DeviceUsage, return false, false, false } -func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count hcu devices for container ", ctr.Name) hcuResourceCount := corev1.ResourceName(HygonResourceCount) hcuResourceMem := corev1.ResourceName(HygonResourceMemory) @@ -164,7 +165,7 @@ func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co if n, ok := v.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "hcu device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "hcu", Reason: fmt.Sprintf("device count %d is out of range", n)} } klog.Info("Found hcu devices") memnum := 0 @@ -177,14 +178,14 @@ func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co if ok { if memnums < 0 || memnums > math.MaxInt32 { klog.ErrorS(nil, "hcu device memory request is out of range", "container", ctr.Name, "request", mem.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "hcu", Reason: fmt.Sprintf("memory request %s is out of range", mem.String())} } if MemoryFactor > 1 { rawMemnums := memnums memnums = memnums * int64(MemoryFactor) if memnums > math.MaxInt32 { klog.ErrorS(nil, "hcu device memory request overflows int32 after applying memory factor", "container", ctr.Name, "raw", rawMemnums, "scaled", memnums, "factor", MemoryFactor) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "hcu", Reason: fmt.Sprintf("memory request %d overflows int32 after applying memory factor %d", rawMemnums, MemoryFactor)} } klog.V(4).Infof("Update memory request. before %d, after %d, factor %d", rawMemnums, memnums, MemoryFactor) } @@ -200,7 +201,7 @@ func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co corenums, valid := core.AsInt64() if !valid || corenums < 0 || corenums > 100 { klog.ErrorS(nil, "hcu device core request is out of range", "container", ctr.Name, "request", core.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "hcu", Reason: fmt.Sprintf("core request %s is out of range (must be an integer between 0 and 100)", core.String())} } corenum = int32(corenums) } @@ -216,10 +217,10 @@ func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co Memreq: int32(memnum), MemPercentagereq: int32(mempnum), Coresreq: corenum, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *HCUDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd device.PodDevices) map[string]string { diff --git a/pkg/device/hygon/device_test.go b/pkg/device/hygon/device_test.go index ecdc1b6111..50eff863c5 100644 --- a/pkg/device/hygon/device_test.go +++ b/pkg/device/hygon/device_test.go @@ -610,7 +610,7 @@ func Test_GenerateResourceRequests(t *testing.T) { dev := HCUDevices{} fs := flag.FlagSet{} ParseConfig(&fs) - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } @@ -687,7 +687,7 @@ func Test_GenerateResourceRequests_OutOfRangeValues(t *testing.T) { dev := HCUDevices{} fs := flag.FlagSet{} ParseConfig(&fs) - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } @@ -713,7 +713,7 @@ func Test_GenerateResourceRequests_MemoryFactorOverflow(t *testing.T) { }, }, } - result := dev.GenerateResourceRequests(ctr) + result, _ := dev.GenerateResourceRequests(ctr) assert.DeepEqual(t, result, device.ContainerDeviceRequest{}) } @@ -1574,7 +1574,7 @@ func Test_GenerateResourceRequests_CoresValidation(t *testing.T) { }, }, } - req := dev.GenerateResourceRequests(ctr) + req, _ := dev.GenerateResourceRequests(ctr) if tt.wantOk { assert.Equal(t, req.Nums, int32(1)) assert.Equal(t, req.Coresreq, tt.wantVal) diff --git a/pkg/device/iluvatar/device.go b/pkg/device/iluvatar/device.go index bd79c7b78e..abb3bd2b53 100644 --- a/pkg/device/iluvatar/device.go +++ b/pkg/device/iluvatar/device.go @@ -184,7 +184,7 @@ func (dev *IluvatarDevices) CheckHealth(devType string, n *corev1.Node) (bool, b return device.CheckHealth(devType, dev.GetResourceNames().ResourceCountName, n) } -func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count iluvatar devices for container ", ctr.Name) iluvatarResourceCount := corev1.ResourceName(dev.config.ResourceCountName) iluvatarResourceMem := corev1.ResourceName(dev.config.ResourceMemoryName) @@ -197,7 +197,7 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) devi if n, ok := v.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "iluvatar device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "iluvatar", Reason: fmt.Sprintf("device count %d is out of range", n)} } klog.Info("Found iluvatar devices") memnum := 0 @@ -210,7 +210,7 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) devi if !parsed || memnums < 0 || memnums > int64(math.MaxInt32)/int64(MemoryFactor) { klog.ErrorS(nil, "iluvatar memory request is not a plain integer within the int32 range; rejecting to avoid silent under-allocation", "container", ctr.Name) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "iluvatar", Reason: fmt.Sprintf("memory request %s is not a plain integer within the int32 range", mem.String())} } memnum = int(memnums) * MemoryFactor } @@ -221,9 +221,26 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) devi } if ok { corenums, ok := core.AsInt64() - if !ok || corenums < 0 || corenums > 100 { - klog.ErrorS(nil, "iluvatar core request is out of range (must be 0-100)", "container", ctr.Name, "request", core.String()) - return device.ContainerDeviceRequest{} + if !ok || corenums < 0 { + klog.ErrorS(nil, "iluvatar core request is not a non-negative integer", "container", ctr.Name, "request", core.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "iluvatar", Reason: fmt.Sprintf("core request %s is not a non-negative integer", core.String())} + } + // Coresreq is a per card percentage, but MutateAdmission rewrites + // the limit to count*100 only when more than one device is + // requested, so a value above 100 with multiple devices is a + // total across the cards and has to be divided back. A value at + // or below 100 is already per card, and a value above 100 with a + // single requested device is plain invalid because + // MutateAdmission never writes one. + if corenums > 100 && n > 1 { + if corenums%n != 0 { + klog.ErrorS(nil, "iluvatar core request does not divide evenly across the requested devices", "container", ctr.Name, "request", core.String(), "devices", n) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "iluvatar", Reason: fmt.Sprintf("core request %d does not divide evenly across %d devices", corenums, n)} + } + corenums /= n + } else if corenums > 100 { + klog.ErrorS(nil, "iluvatar core request exceeds the per card limit", "container", ctr.Name, "request", core.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "iluvatar", Reason: fmt.Sprintf("core request %d exceeds the per card limit of 100", corenums)} } corenum = int32(corenums) } @@ -239,10 +256,10 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) devi Memreq: int32(memnum), MemPercentagereq: int32(mempnum), Coresreq: corenum, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *IluvatarDevices) ScoreNode(node *corev1.Node, podDevices device.PodSingleDevice, previous []*device.DeviceUsage, policy string) float32 { diff --git a/pkg/device/iluvatar/device_test.go b/pkg/device/iluvatar/device_test.go index e89e5328bf..34016aa8af 100644 --- a/pkg/device/iluvatar/device_test.go +++ b/pkg/device/iluvatar/device_test.go @@ -531,7 +531,7 @@ func Test_GenerateResourceRequests(t *testing.T) { } fs := flag.FlagSet{} ParseConfig(&fs) - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } diff --git a/pkg/device/kunlun/device.go b/pkg/device/kunlun/device.go index 847aa9a640..0756a4a712 100644 --- a/pkg/device/kunlun/device.go +++ b/pkg/device/kunlun/device.go @@ -129,7 +129,7 @@ func (dev *KunlunDevices) CheckHealth(devType string, n *corev1.Node) (bool, boo return true, true } -func (dev *KunlunDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *KunlunDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count kunlun devices for container ", ctr.Name) kunlunResourceCount := corev1.ResourceName(KunlunResourceCount) v, ok := ctr.Resources.Limits[kunlunResourceCount] @@ -140,7 +140,7 @@ func (dev *KunlunDevices) GenerateResourceRequests(ctr *corev1.Container) device if n, ok := v.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "kunlun device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "kunlun", Reason: fmt.Sprintf("device count %d is out of range", n)} } klog.Info("Found kunlunxin devices") @@ -150,10 +150,10 @@ func (dev *KunlunDevices) GenerateResourceRequests(ctr *corev1.Container) device Memreq: 0, MemPercentagereq: 100, Coresreq: 0, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *KunlunDevices) ScoreNode(node *corev1.Node, podDevices device.PodSingleDevice, previous []*device.DeviceUsage, policy string) float32 { diff --git a/pkg/device/kunlun/device_test.go b/pkg/device/kunlun/device_test.go index cbca3830ab..09e3c5f8d9 100644 --- a/pkg/device/kunlun/device_test.go +++ b/pkg/device/kunlun/device_test.go @@ -119,7 +119,7 @@ func Test_KunlunDevices_GenerateResourceRequests(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { dev := KunlunDevices{} - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } diff --git a/pkg/device/kunlun/vdevice.go b/pkg/device/kunlun/vdevice.go index 89a2b8ad83..e2470d976e 100644 --- a/pkg/device/kunlun/vdevice.go +++ b/pkg/device/kunlun/vdevice.go @@ -154,7 +154,7 @@ func (dev *KunlunVDevices) CheckType(annos map[string]string, d device.DeviceUsa return false, false } -func (dev *KunlunVDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *KunlunVDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { xpuResourceCount := corev1.ResourceName(KunlunResourceVCount) xpuResourceMem := corev1.ResourceName(KunlunResourceVMemory) v, ok := ctr.Resources.Limits[xpuResourceCount] @@ -189,10 +189,10 @@ func (dev *KunlunVDevices) GenerateResourceRequests(ctr *corev1.Container) devic Memreq: int32(memnum), //int32(dev.config.MemoryMax), MemPercentagereq: int32(mempnum), Coresreq: int32(cores), - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *KunlunVDevices) ScoreNode(node *corev1.Node, podDevices device.PodSingleDevice, previous []*device.DeviceUsage, policy string) float32 { diff --git a/pkg/device/kunlun/vdevice_test.go b/pkg/device/kunlun/vdevice_test.go index 6e89ae958a..31d8919bc7 100644 --- a/pkg/device/kunlun/vdevice_test.go +++ b/pkg/device/kunlun/vdevice_test.go @@ -490,7 +490,7 @@ func Test_KunlunVDevices_GenerateResourceRequests(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := dev.GenerateResourceRequests(test.ctr) + got, _ := dev.GenerateResourceRequests(test.ctr) assert.DeepEqual(t, got, test.want) }) } diff --git a/pkg/device/metax/device.go b/pkg/device/metax/device.go index 9e0bf31622..7b69f945bc 100644 --- a/pkg/device/metax/device.go +++ b/pkg/device/metax/device.go @@ -132,7 +132,7 @@ func (dev *MetaxDevices) CheckHealth(devType string, n *corev1.Node) (bool, bool return count > 0, true } -func (dev *MetaxDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *MetaxDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count metax devices for container ", ctr.Name) metaxResourceCount := corev1.ResourceName(MetaxResourceCount) v, ok := ctr.Resources.Limits[metaxResourceCount] @@ -143,7 +143,7 @@ func (dev *MetaxDevices) GenerateResourceRequests(ctr *corev1.Container) device. if n, ok := v.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "metax device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax", Reason: fmt.Sprintf("device count %d is out of range", n)} } klog.Info("Found metax devices") return device.ContainerDeviceRequest{ @@ -152,10 +152,10 @@ func (dev *MetaxDevices) GenerateResourceRequests(ctr *corev1.Container) device. Memreq: 0, MemPercentagereq: 100, Coresreq: 100, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func parseMetaxAnnos(annos string, index int) float32 { diff --git a/pkg/device/metax/device_test.go b/pkg/device/metax/device_test.go index 75207267ae..e823f88e21 100644 --- a/pkg/device/metax/device_test.go +++ b/pkg/device/metax/device_test.go @@ -360,7 +360,7 @@ func Test_GenerateResourceRequests(t *testing.T) { fs := flag.FlagSet{} ParseConfig(&fs) dev := MetaxDevices{} - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } diff --git a/pkg/device/metax/sdevice.go b/pkg/device/metax/sdevice.go index 58dc29078a..c3afc80e77 100644 --- a/pkg/device/metax/sdevice.go +++ b/pkg/device/metax/sdevice.go @@ -200,21 +200,21 @@ func (sdev *MetaxSDevices) CheckHealth(devType string, n *corev1.Node) (bool, bo return len(devices) > 0, true } -func (sdev *MetaxSDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (sdev *MetaxSDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { value, ok := ctr.Resources.Limits[corev1.ResourceName(MetaxResourceNameVCount)] if !ok { - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } count, ok := value.AsInt64() if !ok { klog.Errorf("container<%s> resource<%s> cannot decode to int64", ctr.Name, MetaxResourceNameVCount) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax sgpu", Reason: fmt.Sprintf("count request %s cannot decode to int64", value.String())} } if count <= 0 || count > math.MaxInt32 { klog.ErrorS(nil, "metax sgpu device count request is out of range", "container", ctr.Name, "request", count) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax sgpu", Reason: fmt.Sprintf("device count %d is out of range", count)} } core := int64(100) @@ -226,7 +226,7 @@ func (sdev *MetaxSDevices) GenerateResourceRequests(ctr *corev1.Container) devic v, valid := coreQuantity.AsInt64() if !valid || v < 0 || v > 100 { klog.ErrorS(nil, "metax sgpu device core request is out of range", "container", ctr.Name, "request", coreQuantity.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax sgpu", Reason: fmt.Sprintf("core request %s is out of range (must be an integer between 0 and 100)", coreQuantity.String())} } core = v } @@ -245,13 +245,13 @@ func (sdev *MetaxSDevices) GenerateResourceRequests(ctr *corev1.Container) devic } else { if v < 0 || v > int64(math.MaxInt32)/int64(MemoryFactor) { klog.ErrorS(nil, "metax sgpu device memory request is out of range", "container", ctr.Name, "request", memQuantity.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax sgpu", Reason: fmt.Sprintf("memory request %s is out of range", memQuantity.String())} } mem = v * int64(MemoryFactor) } if mem < 0 || mem > math.MaxInt32 { klog.ErrorS(nil, "metax sgpu device memory request is out of range", "container", ctr.Name, "request", memQuantity.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax sgpu", Reason: fmt.Sprintf("memory request %s is out of range", memQuantity.String())} } } } @@ -269,7 +269,7 @@ func (sdev *MetaxSDevices) GenerateResourceRequests(ctr *corev1.Container) devic Memreq: int32(mem), MemPercentagereq: int32(memPercent), Coresreq: int32(core), - } + }, nil } // ScoreNode returns a policy-independent score for the node following a diff --git a/pkg/device/metax/sdevice_test.go b/pkg/device/metax/sdevice_test.go index 0bd892beff..ed2ccbdd17 100644 --- a/pkg/device/metax/sdevice_test.go +++ b/pkg/device/metax/sdevice_test.go @@ -598,7 +598,7 @@ func TestGenerateResourceRequests(t *testing.T) { fs := flag.FlagSet{} ParseConfig(&fs) - result := metaxSDevice.GenerateResourceRequests(ts.container) + result, _ := metaxSDevice.GenerateResourceRequests(ts.container) if !reflect.DeepEqual(ts.expected, result) { t.Errorf("GenerateResourceRequests failed: result %v, expected %v", @@ -3285,7 +3285,7 @@ func Test_GenerateResourceRequests_CoresValidation(t *testing.T) { }, }, } - req := sdev.GenerateResourceRequests(ctr) + req, _ := sdev.GenerateResourceRequests(ctr) if tt.wantOk { assert.Equal(t, req.Nums, int32(1)) assert.Equal(t, req.Coresreq, tt.wantVal) diff --git a/pkg/device/mthreads/device.go b/pkg/device/mthreads/device.go index 30ba4b4ffc..4b0ff1d7e2 100644 --- a/pkg/device/mthreads/device.go +++ b/pkg/device/mthreads/device.go @@ -199,7 +199,7 @@ func (dev *MthreadsDevices) CheckHealth(devType string, n *corev1.Node) (bool, b return true, true } -func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.Info("Start to count mthreads devices for container ", ctr.Name) mthreadsResourceCount := corev1.ResourceName(MthreadsResourceCount) mthreadsResourceMem := corev1.ResourceName(MthreadsResourceMemory) @@ -214,7 +214,7 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) devi "container", ctr.Name, "deviceCount", n) if n <= 0 || n > math.MaxInt32 { - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("device count %d is out of range", n)} } memnum := 0 mem, ok := ctr.Resources.Limits[mthreadsResourceMem] @@ -226,7 +226,7 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) devi if !parsed || memnums < 0 || memnums > int64(math.MaxInt32)/int64(MemoryFactor) { klog.ErrorS(nil, "mthreads memory request is not a plain integer within the int32 range; rejecting to avoid silent under-allocation", "container", ctr.Name) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("memory request %s is not a plain integer within the int32 range", mem.String())} } memnum = int(memnums) * MemoryFactor klog.InfoS("Memory allocation calculated", @@ -241,9 +241,26 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) devi } if ok { corenums, ok := core.AsInt64() - if !ok || corenums < 0 || corenums > 100 { - klog.ErrorS(nil, "mthreads core request is out of range (must be 0-100)", "container", ctr.Name, "request", core.String()) - return device.ContainerDeviceRequest{} + if !ok || corenums < 0 { + klog.ErrorS(nil, "mthreads core request is not a non-negative integer", "container", ctr.Name, "request", core.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("core request %s is not a non-negative integer", core.String())} + } + // Coresreq is a per card percentage, but MutateAdmission rewrites + // the limit to count*coresPerMthreadsGPU (16) only when more than + // one device is requested, so a value above 100 with multiple + // devices is a total across the cards and has to be divided + // back. A value at or below 100 is already per card, and a value + // above 100 with a single requested device is plain invalid + // because MutateAdmission never writes one. + if corenums > 100 && n > 1 { + if corenums%n != 0 { + klog.ErrorS(nil, "mthreads core request does not divide evenly across the requested devices", "container", ctr.Name, "request", core.String(), "devices", n) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("core request %d does not divide evenly across %d devices", corenums, n)} + } + corenums /= n + } else if corenums > 100 { + klog.ErrorS(nil, "mthreads core request exceeds the per card limit", "container", ctr.Name, "request", core.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("core request %d exceeds the per card limit of 100", corenums)} } corenum = int32(corenums) } @@ -258,11 +275,11 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) devi Type: MthreadsGPUDevice, Memreq: int32(memnum) / int32(n), MemPercentagereq: int32(mempnum), - Coresreq: corenum / int32(n), - } + Coresreq: corenum, + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *MthreadsDevices) customFilterRule(allocated *device.PodDevices, request device.ContainerDeviceRequest, toAllocate device.ContainerDevices, device *device.DeviceUsage) bool { diff --git a/pkg/device/mthreads/device_test.go b/pkg/device/mthreads/device_test.go index cbbe9d4834..7a18835ecf 100644 --- a/pkg/device/mthreads/device_test.go +++ b/pkg/device/mthreads/device_test.go @@ -709,7 +709,7 @@ func Test_GenerateResourceRequests(t *testing.T) { } InitMthreadsDevice(config) dev := MthreadsDevices{} - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } @@ -1446,3 +1446,62 @@ func TestFit_CoresValidation(t *testing.T) { assert.Equal(t, reason, "core limit out of range") }) } + +// TestGenerateResourceRequests_MultiCardCoresDivision covers the case from +// issue #2987: MutateAdmission rewrites the core limit to count*16 when more +// than one device is requested, and GenerateResourceRequests has to divide it +// back to the per card value instead of silently dropping or misreporting it. +func TestGenerateResourceRequests_MultiCardCoresDivision(t *testing.T) { + config := MthreadsConfig{ + ResourceCountName: "mthreads.com/vgpu", + ResourceMemoryName: "mthreads.com/sgpu-memory", + ResourceCoreName: "mthreads.com/sgpu-core", + } + InitMthreadsDevice(config) + dev := MthreadsDevices{} + + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "mthreads.com/vgpu": resource.MustParse("7"), + }, + }, + } + // What MutateAdmission writes for a 7 card request without explicit + // per card cores. + mutated, err := dev.MutateAdmission(ctr, &corev1.Pod{}) + assert.NilError(t, err) + assert.Assert(t, mutated) + mutatedCore := ctr.Resources.Limits["mthreads.com/sgpu-core"] + assert.Equal(t, mutatedCore.Value(), int64(112)) + + result, err := dev.GenerateResourceRequests(ctr) + assert.NilError(t, err) + assert.Equal(t, result.Nums, int32(7)) + assert.Equal(t, result.Coresreq, int32(16)) +} + +// TestGenerateResourceRequests_UnevenCoresFailsClosed makes sure a core total +// that does not divide evenly across the requested cards is rejected instead +// of producing a device-less pod. +func TestGenerateResourceRequests_UnevenCoresFailsClosed(t *testing.T) { + config := MthreadsConfig{ + ResourceCountName: "mthreads.com/vgpu", + ResourceMemoryName: "mthreads.com/sgpu-memory", + ResourceCoreName: "mthreads.com/sgpu-core", + } + InitMthreadsDevice(config) + dev := MthreadsDevices{} + + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "mthreads.com/vgpu": resource.MustParse("3"), + "mthreads.com/sgpu-core": resource.MustParse("130"), + }, + }, + } + result, err := dev.GenerateResourceRequests(ctr) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, result) + assert.ErrorContains(t, err, "does not divide evenly") +} diff --git a/pkg/device/nvidia/device.go b/pkg/device/nvidia/device.go index 7b7a27af86..ace1d8ddbb 100644 --- a/pkg/device/nvidia/device.go +++ b/pkg/device/nvidia/device.go @@ -525,7 +525,7 @@ func (dev *NvidiaGPUDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[st return *annoinput } -func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { resourceName := corev1.ResourceName(dev.config.ResourceCountName) resourceMem := corev1.ResourceName(dev.config.ResourceMemoryName) resourceMemPercentage := corev1.ResourceName(dev.config.ResourceMemoryPercentageName) @@ -538,7 +538,7 @@ func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) dev if n, ok := v.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "nvidia device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "nvidia", Reason: fmt.Sprintf("device count %d is out of range", n)} } memnum := 0 mem, ok := ctr.Resources.Limits[resourceMem] @@ -551,7 +551,7 @@ func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) dev if !parsed || memnums < 0 || memnums > int64(math.MaxInt32)/factor { klog.ErrorS(nil, "nvidia memory request is not a plain integer within the int32 range; rejecting to avoid silent under-allocation", "container", ctr.Name) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "nvidia", Reason: fmt.Sprintf("memory request %s is not a plain integer within the int32 range", mem.String())} } if factor > 1 { rawMemnums := memnums @@ -597,7 +597,7 @@ func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) dev corenums, ok := core.AsInt64() if !ok || corenums < 0 || corenums > 100 { klog.ErrorS(nil, "nvidia core request is out of range", "container", ctr.Name, "request", core.String()) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "nvidia", Reason: fmt.Sprintf("core request %s is out of range (must be an integer between 0 and 100)", core.String())} } corenum = int32(corenums) } @@ -607,10 +607,10 @@ func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) dev Memreq: int32(memnum), MemPercentagereq: mempnum, Coresreq: corenum, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *NvidiaGPUDevices) CustomFilterRule(allocated *device.PodDevices, request device.ContainerDeviceRequest, toAllocate device.ContainerDevices, devusage *device.DeviceUsage) bool { diff --git a/pkg/device/nvidia/device_test.go b/pkg/device/nvidia/device_test.go index 09b9ff8600..7cb81ea591 100644 --- a/pkg/device/nvidia/device_test.go +++ b/pkg/device/nvidia/device_test.go @@ -2191,7 +2191,7 @@ func TestGenerateResourceRequests(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := dev.GenerateResourceRequests(tt.ctr) + result, _ := dev.GenerateResourceRequests(tt.ctr) assert.DeepEqual(t, result, tt.want) }) } @@ -2214,11 +2214,11 @@ func TestGenerateResourceRequests_MemoryFactor(t *testing.T) { }, }, } - result := dev.GenerateResourceRequests(ctr) + result, _ := dev.GenerateResourceRequests(ctr) assert.Equal(t, result.Memreq, int32(2048)) ctr.Resources.Limits["nvidia.com/gpumem"] = resource.MustParse("1Gi") - result = dev.GenerateResourceRequests(ctr) + result, _ = dev.GenerateResourceRequests(ctr) assert.DeepEqual(t, result, device.ContainerDeviceRequest{}) } @@ -2239,19 +2239,19 @@ func TestGenerateResourceRequests_DefaultMemory(t *testing.T) { }, }, } - result := dev.GenerateResourceRequests(ctr) + result, _ := dev.GenerateResourceRequests(ctr) assert.Equal(t, result.Memreq, int32(512)) assert.Equal(t, result.MemPercentagereq, int32(101)) // a percentage of 0 is unset, so it lands on defaultMemory just like nvidia.com/gpumem: 0 ctr.Resources.Limits["nvidia.com/gpumem-percentage"] = *resource.NewQuantity(0, resource.DecimalSI) - result = dev.GenerateResourceRequests(ctr) + result, _ = dev.GenerateResourceRequests(ctr) assert.Equal(t, result.Memreq, int32(512)) assert.Equal(t, result.MemPercentagereq, int32(101)) delete(ctr.Resources.Limits, "nvidia.com/gpumem-percentage") ctr.Resources.Limits["nvidia.com/gpumem"] = *resource.NewQuantity(0, resource.DecimalSI) - control := dev.GenerateResourceRequests(ctr) + control, _ := dev.GenerateResourceRequests(ctr) assert.DeepEqual(t, result, control) } @@ -2285,7 +2285,7 @@ func TestZeroMemoryPercentageIsAccountedAsWholeCard(t *testing.T) { Type: NvidiaGPUDevice, Health: true, }} } - req := dev.GenerateResourceRequests(ctr) + req, _ := dev.GenerateResourceRequests(ctr) fit, result, reason := dev.Fit(newCard(0), req, pod, &device.NodeInfo{}, &device.PodDevices{}) assert.Assert(t, fit, "empty card should fit, reason: %s", reason) @@ -3407,7 +3407,7 @@ func Test_GenerateResourceRequests_CoresValidation(t *testing.T) { }, }, } - req := dev.GenerateResourceRequests(ctr) + req, _ := dev.GenerateResourceRequests(ctr) if tt.wantOk { assert.Equal(t, req.Nums, int32(1)) assert.Equal(t, req.Coresreq, tt.wantVal) @@ -3616,3 +3616,28 @@ func TestDistinctCardCandidates(t *testing.T) { }) } } + +// TestGenerateResourceRequests_InvalidCoresFailsClosed makes sure a core +// request outside 0-100 returns an error instead of a zero request, so the +// scheduler rejects the pod rather than binding it without devices. +func TestGenerateResourceRequests_InvalidCoresFailsClosed(t *testing.T) { + config := NvidiaConfig{ + ResourceCountName: "nvidia.com/gpu", + ResourceMemoryName: "nvidia.com/gpumem", + ResourceCoreName: "nvidia.com/gpucores", + ResourceMemoryPercentageName: "nvidia.com/gpumem-percentage", + } + dev := InitNvidiaDevice(config) + + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "nvidia.com/gpu": resource.MustParse("1"), + "nvidia.com/gpucores": resource.MustParse("150"), + }, + }, + } + result, err := dev.GenerateResourceRequests(ctr) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, result) + assert.ErrorContains(t, err, "out of range") +} diff --git a/pkg/device/quota_test.go b/pkg/device/quota_test.go index 23eeb10461..9dbfe144b0 100644 --- a/pkg/device/quota_test.go +++ b/pkg/device/quota_test.go @@ -61,8 +61,8 @@ func (m *MockDevices) ReleaseNodeLock(n *corev1.Node, p *corev1.Pod) error { return nil } -func (m *MockDevices) GenerateResourceRequests(ctr *corev1.Container) ContainerDeviceRequest { - return ContainerDeviceRequest{} +func (m *MockDevices) GenerateResourceRequests(ctr *corev1.Container) (ContainerDeviceRequest, error) { + return ContainerDeviceRequest{}, nil } func (m *MockDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd PodDevices) map[string]string { diff --git a/pkg/device/vastai/device.go b/pkg/device/vastai/device.go index 8fbe24a87f..55450363d5 100644 --- a/pkg/device/vastai/device.go +++ b/pkg/device/vastai/device.go @@ -127,7 +127,7 @@ func (dev *VastaiDevices) CheckHealth(devType string, n *corev1.Node) (bool, boo return device.CheckHealth(devType, dev.GetResourceNames().ResourceCountName, n) } -func (dev *VastaiDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *VastaiDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { klog.V(5).Info("Start to count vastai devices for container ", ctr.Name) vastaiResourceCount := corev1.ResourceName(VastaiResourceCount) v, ok := ctr.Resources.Limits[vastaiResourceCount] @@ -138,7 +138,7 @@ func (dev *VastaiDevices) GenerateResourceRequests(ctr *corev1.Container) device if n, ok := v.AsInt64(); ok { if n <= 0 || n > math.MaxInt32 { klog.ErrorS(nil, "vastai device count request is out of range", "container", ctr.Name, "request", n) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "vastai", Reason: fmt.Sprintf("device count %d is out of range", n)} } klog.Info("Found vastai devices") memnum := 0 @@ -151,10 +151,10 @@ func (dev *VastaiDevices) GenerateResourceRequests(ctr *corev1.Container) device Memreq: int32(memnum), MemPercentagereq: int32(mempnum), Coresreq: corenum, - } + }, nil } } - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, nil } func (dev *VastaiDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd device.PodDevices) map[string]string { diff --git a/pkg/device/vastai/device_test.go b/pkg/device/vastai/device_test.go index 44e4b724cb..76ab1659c3 100644 --- a/pkg/device/vastai/device_test.go +++ b/pkg/device/vastai/device_test.go @@ -458,7 +458,7 @@ func Test_GenerateResourceRequests(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { dev := VastaiDevices{} - result := dev.GenerateResourceRequests(test.args) + result, _ := dev.GenerateResourceRequests(test.args) assert.DeepEqual(t, result, test.want) }) } diff --git a/pkg/scheduler/config/config_test.go b/pkg/scheduler/config/config_test.go index bf7e5fc95f..09f2f5e015 100644 --- a/pkg/scheduler/config/config_test.go +++ b/pkg/scheduler/config/config_test.go @@ -781,7 +781,8 @@ func Test_Resourcereqs(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := device.Resourcereqs(test.args) + got, err := device.Resourcereqs(test.args) + assert.NilError(t, err) assert.DeepEqual(t, test.want, got) }) } diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 4f4e322d8d..481b01bda1 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -1093,7 +1093,20 @@ func (s *Scheduler) Bind(args extenderv1.ExtenderBindingArgs) (*extenderv1.Exten func (s *Scheduler) Filter(args extenderv1.ExtenderArgs) (*extenderv1.ExtenderFilterResult, error) { klog.InfoS("Starting schedule filter process", "pod", args.Pod.Name, "uuid", args.Pod.UID, "namespace", args.Pod.Namespace) - resourceReqs := device.Resourcereqs(args.Pod) + resourceReqs, reqErr := device.Resourcereqs(args.Pod) + if reqErr != nil { + // A container declared HAMi resources but the request is invalid + // (for example a core limit out of the 0-100 range). Failing closed + // rejects the pod here; treating it as device-less would bind it + // with no device at all. + err := fmt.Errorf("invalid device request for pod %v: %w", args.Pod.Name, reqErr) + klog.ErrorS(nil, "Rejecting pod with an invalid device request", "pod", klog.KObj(args.Pod), "error", reqErr) + s.recordScheduleFilterResultEvent(args.Pod, EventReasonFilteringFailed, "", err) + return &extenderv1.ExtenderFilterResult{ + FailedNodes: map[string]string{}, + Error: err.Error(), + }, err + } hasHAMiResource := false diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go index 5e0a758abc..075864585c 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -1370,8 +1370,8 @@ func (m *registerMockDevice) GetNodeDevices(node corev1.Node) ([]*device.DeviceI } func (m *registerMockDevice) LockNode(_ *corev1.Node, _ *corev1.Pod) error { return nil } func (m *registerMockDevice) ReleaseNodeLock(_ *corev1.Node, _ *corev1.Pod) error { return nil } -func (m *registerMockDevice) GenerateResourceRequests(_ *corev1.Container) device.ContainerDeviceRequest { - return device.ContainerDeviceRequest{} +func (m *registerMockDevice) GenerateResourceRequests(_ *corev1.Container) (device.ContainerDeviceRequest, error) { + return device.ContainerDeviceRequest{}, nil } func (m *registerMockDevice) PatchAnnotations(_ *corev1.Pod, _ *map[string]string, _ device.PodDevices) map[string]string { return nil @@ -3484,3 +3484,35 @@ func TestSchedulerIsSynced(t *testing.T) { assert.Equal(t, true, s.IsSynced()) } + +// TestFilterInvalidDeviceRequestFailsClosed makes sure a pod whose container +// declares a HAMi device resource with an invalid value is rejected by the +// filter instead of being treated as device-less and bound without any GPU. +func TestFilterInvalidDeviceRequestFailsClosed(t *testing.T) { + require.NoError(t, config.InitDevicesWithConfig(&config.Config{ + NvidiaConfig: nvidia.NvidiaConfig{ + ResourceCountName: "hami.io/gpu", ResourceMemoryName: "hami.io/gpumem", + ResourceCoreName: "hami.io/gpucores", DefaultGPUNum: 1, + }, + })) + s := NewScheduler() + client.KubeClient = fake.NewClientset() + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{UID: "uid-ic", Name: "invalid-cores", Namespace: "ns-invalid"}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{ + Name: "c", + Resources: corev1.ResourceRequirements{Limits: corev1.ResourceList{ + "hami.io/gpu": *resource.NewQuantity(1, resource.BinarySI), + "hami.io/gpucores": *resource.NewQuantity(150, resource.BinarySI), + }}, + }}}, + } + nodeNames := []string{"node1"} + res, err := s.Filter(extenderv1.ExtenderArgs{Pod: pod, NodeNames: &nodeNames}) + require.Error(t, err) + assert.ErrorContains(t, err, "out of range") + assert.Assert(t, res != nil) + assert.Equal(t, res.Error, err.Error()) + // No node may survive: the pod must not be schedulable as device-less. + assert.Assert(t, res.Nodes == nil && res.NodeNames == nil) +} diff --git a/pkg/scheduler/score_test.go b/pkg/scheduler/score_test.go index df591f6cb8..fc23198f40 100644 --- a/pkg/scheduler/score_test.go +++ b/pkg/scheduler/score_test.go @@ -4437,8 +4437,8 @@ func (m *fitMockDevice) GetNodeDevices(_ corev1.Node) ([]*device.DeviceInfo, err } func (m *fitMockDevice) LockNode(_ *corev1.Node, _ *corev1.Pod) error { return nil } func (m *fitMockDevice) ReleaseNodeLock(_ *corev1.Node, _ *corev1.Pod) error { return nil } -func (m *fitMockDevice) GenerateResourceRequests(_ *corev1.Container) device.ContainerDeviceRequest { - return device.ContainerDeviceRequest{} +func (m *fitMockDevice) GenerateResourceRequests(_ *corev1.Container) (device.ContainerDeviceRequest, error) { + return device.ContainerDeviceRequest{}, nil } func (m *fitMockDevice) PatchAnnotations(_ *corev1.Pod, _ *map[string]string, _ device.PodDevices) map[string]string { return nil diff --git a/pkg/scheduler/webhook.go b/pkg/scheduler/webhook.go index b9d4068e65..8d71b3ac4b 100644 --- a/pkg/scheduler/webhook.go +++ b/pkg/scheduler/webhook.go @@ -127,8 +127,8 @@ func (h *webhook) Handle(_ context.Context, req admission.Request) admission.Res return admission.Denied("pod has node assigned") } } - if !fitResourceQuota(pod) { - return admission.Denied("exceeding resource quota") + if err := fitResourceQuota(pod); err != nil { + return admission.Denied(err.Error()) } marshaledPod, err := json.Marshal(pod) if err != nil { @@ -158,7 +158,7 @@ func isPrivilegedContainer(ctr *corev1.Container) bool { *ctr.SecurityContext.Privileged } -func fitResourceQuota(pod *corev1.Pod) bool { +func fitResourceQuota(pod *corev1.Pod) error { for deviceName, dev := range device.GetDevices() { resourceNames := dev.GetResourceNames() if len(resourceNames.ResourceMemoryName) == 0 && len(resourceNames.ResourceCoreName) == 0 { @@ -171,7 +171,10 @@ func fitResourceQuota(pod *corev1.Pod) bool { // so this keeps admission and the scheduler on the same numbers. var appMemoryReq, appCoresReq int64 for i := range pod.Spec.Containers { - req := dev.GenerateResourceRequests(&pod.Spec.Containers[i]) + req, reqErr := dev.GenerateResourceRequests(&pod.Spec.Containers[i]) + if reqErr != nil { + return reqErr + } if req.Nums == 0 { continue } @@ -183,7 +186,10 @@ func fitResourceQuota(pod *corev1.Pod) bool { var sidecarMemoryReq, sidecarCoresReq int64 for i := range pod.Spec.InitContainers { c := &pod.Spec.InitContainers[i] - req := dev.GenerateResourceRequests(c) + req, reqErr := dev.GenerateResourceRequests(c) + if reqErr != nil { + return reqErr + } if req.Nums == 0 { continue } @@ -209,8 +215,8 @@ func fitResourceQuota(pod *corev1.Pod) bool { klog.V(5).Infof("Checking quota for device %s: memory %d, cores %d, factor %d", deviceName, memoryReq, coresReq, resourceNames.MemoryFactor) if !device.GetLocalCache().FitQuota(pod.Namespace, memoryReq, resourceNames.MemoryFactor, coresReq, deviceName) { klog.Infof(template+" - Denying admission", pod.Namespace, pod.Name, pod.UID) - return false + return fmt.Errorf("exceeding resource quota for device %s", deviceName) } } - return true + return nil } diff --git a/pkg/scheduler/webhook_test.go b/pkg/scheduler/webhook_test.go index cd81fddc35..07a051bfeb 100644 --- a/pkg/scheduler/webhook_test.go +++ b/pkg/scheduler/webhook_test.go @@ -505,9 +505,9 @@ func TestFitResourceQuota(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result := fitResourceQuota(tc.pod) - if tc.fit != result { - t.Errorf("Expected %v, but got %v", tc.fit, result) + err := fitResourceQuota(tc.pod) + if (err == nil) != tc.fit { + t.Errorf("Expected fit=%v, but got error: %v", tc.fit, err) } }) } @@ -641,8 +641,8 @@ func TestFitResourceQuotaNonNvidia(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - if got := fitResourceQuota(tc.pod); got != tc.fit { - t.Errorf("fitResourceQuota() = %v, want %v", got, tc.fit) + if err := fitResourceQuota(tc.pod); (err == nil) != tc.fit { + t.Errorf("fitResourceQuota() error = %v, want fit %v", err, tc.fit) } }) } @@ -682,8 +682,8 @@ func TestFitResourceQuotaCountsEveryDevice(t *testing.T) { pod.Spec.Containers[0].Resources.Limits["cambricon.com/mlu"] = resource.MustParse("2") // 2 x 40 units is 80, past the 60 unit limit, even though one device fits. - if fitResourceQuota(pod) { - t.Error("fitResourceQuota() = true, want false: two devices should each count against the quota") + if err := fitResourceQuota(pod); err == nil { + t.Errorf("fitResourceQuota() = nil error, want a denial: two devices should each count against the quota: %v", err) } } @@ -759,12 +759,12 @@ func TestFitResourceQuotaAscendMemoryFactor(t *testing.T) { } // 4096 x factor 4 is 16384, against a limit of 8192 x 4 = 32768. - if !fitResourceQuota(ascendPod("4096")) { - t.Error("fitResourceQuota() = false, want true: the limit must be scaled by the same factor as the request") + if err := fitResourceQuota(ascendPod("4096")); err != nil { + t.Errorf("fitResourceQuota() = %v, want nil: the limit must be scaled by the same factor as the request", err) } // 8192 x 4 is 32768 used against 32768 available, and 8193 pushes it over. - if fitResourceQuota(ascendPod("8193")) { - t.Error("fitResourceQuota() = true, want false: the request exceeds the scaled limit") + if err := fitResourceQuota(ascendPod("8193")); err == nil { + t.Error("fitResourceQuota() = nil error, want a denial: the request exceeds the scaled limit") } } @@ -1053,8 +1053,8 @@ func TestFitResourceQuota_InitContainerPeakSequence(t *testing.T) { // Step 1: Pod1 (init 20000, app 10000) should be allowed pod1 := makePod("pod1", 20000, 10000) - if !fitResourceQuota(pod1) { - t.Fatal("Step 1 failed: pod1 should be allowed (peak 20000 ≤ 30000)") + if err := fitResourceQuota(pod1); err != nil { + t.Fatalf("Step 1 failed: pod1 should be allowed (peak 20000 ≤ 30000): %v", err) } // Simulate pod1 scheduled → record its peak usage (20000) @@ -1068,7 +1068,7 @@ func TestFitResourceQuota_InitContainerPeakSequence(t *testing.T) { // Step 2: Pod2 (same) must be DENIED pod2 := makePod("pod2", 20000, 10000) - if fitResourceQuota(pod2) { + if err := fitResourceQuota(pod2); err == nil { t.Fatal("Step 2 failed: pod2 should be denied (total used 20000 + request 20000 > 30000)") } @@ -1080,8 +1080,8 @@ func TestFitResourceQuota_InitContainerPeakSequence(t *testing.T) { } // Now pod2 should be allowed - if !fitResourceQuota(pod2) { - t.Fatal("Step 3 failed: pod2 should be allowed after pod1 init finished (total 10000+20000=30000)") + if err := fitResourceQuota(pod2); err != nil { + t.Fatalf("Step 3 failed: pod2 should be allowed after pod1 init finished (total 10000+20000=30000): %v", err) } } @@ -1207,8 +1207,8 @@ func TestFitResourceQuota_SidecarOrdering(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - if got := fitResourceQuota(tc.pod); got != tc.fit { - t.Errorf("fitResourceQuota() = %v, want %v", got, tc.fit) + if err := fitResourceQuota(tc.pod); (err == nil) != tc.fit { + t.Errorf("fitResourceQuota() error = %v, want fit %v", err, tc.fit) } }) } From f76ee3e9e097878bf5f1d58f9d98e8f647d7cfbf Mon Sep 17 00:00:00 2001 From: Eshiv Pandey Date: Thu, 10 Sep 2026 00:43:17 +0530 Subject: [PATCH 2/6] fix(device): address review feedback on invalid device request validation - enflame GCU and kunlun vdevice: reject device counts outside the int32 range before narrowing to int32, instead of silently wrapping. - mthreads: divide the admission-generated core total back to a per card value whenever more than one device is requested, so totals at or below 100 (count*16 for two to six cards) are normalized too, and keep the per card limit check after the division. Signed-off-by: Eshiv Pandey --- pkg/device/enflame/gcu.go | 5 +++++ pkg/device/enflame/gcu_test.go | 16 ++++++++++++++++ pkg/device/kunlun/vdevice.go | 5 +++++ pkg/device/kunlun/vdevice_test.go | 16 ++++++++++++++++ pkg/device/mthreads/device.go | 16 ++++++++-------- pkg/device/mthreads/device_test.go | 30 ++++++++++++++++++++++++++++++ 6 files changed, 80 insertions(+), 8 deletions(-) diff --git a/pkg/device/enflame/gcu.go b/pkg/device/enflame/gcu.go index 521ebbff9e..e054f7b6ff 100644 --- a/pkg/device/enflame/gcu.go +++ b/pkg/device/enflame/gcu.go @@ -18,6 +18,7 @@ package enflame import ( "fmt" + "math" "slices" corev1 "k8s.io/api/core/v1" @@ -97,6 +98,10 @@ func (dev *GCUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.C } if ok { if n, ok := v.AsInt64(); ok { + if n <= 0 || n > math.MaxInt32 { + klog.ErrorS(nil, "enflame device count request is out of range", "container", ctr.Name, "request", n) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "enflame", Reason: fmt.Sprintf("device count %d is out of range", n)} + } klog.Info("Found enflame devices") return device.ContainerDeviceRequest{ Nums: int32(n), diff --git a/pkg/device/enflame/gcu_test.go b/pkg/device/enflame/gcu_test.go index 9b4dbdb84a..59b0673943 100644 --- a/pkg/device/enflame/gcu_test.go +++ b/pkg/device/enflame/gcu_test.go @@ -727,3 +727,19 @@ func TestGCUDevices_AddResourceUsage(t *testing.T) { }) } } + +func TestGCUDevices_GenerateResourceRequests_CountRange(t *testing.T) { + dev := GCUDevices{} + for _, count := range []string{"0", "-1", "4294967296"} { + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceName(EnflameResourceNameGCU): resource.MustParse(count), + }, + }, + } + result, err := dev.GenerateResourceRequests(ctr) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, result) + assert.ErrorContains(t, err, "out of range") + } +} diff --git a/pkg/device/kunlun/vdevice.go b/pkg/device/kunlun/vdevice.go index e2470d976e..d05bc1a31c 100644 --- a/pkg/device/kunlun/vdevice.go +++ b/pkg/device/kunlun/vdevice.go @@ -19,6 +19,7 @@ package kunlun import ( "errors" "fmt" + "math" "github.com/Project-HAMi/HAMi/pkg/device" "github.com/Project-HAMi/HAMi/pkg/device/common" @@ -164,6 +165,10 @@ func (dev *KunlunVDevices) GenerateResourceRequests(ctr *corev1.Container) (devi if ok { klog.V(3).Infof("Counting %s devices", dev.CommonWord()) if n, ok := v.AsInt64(); ok { + if n <= 0 || n > math.MaxInt32 { + klog.ErrorS(nil, "kunlun vdevice count request is out of range", "container", ctr.Name, "request", n) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "kunlun", Reason: fmt.Sprintf("device count %d is out of range", n)} + } memnum := 0 mem, ok := ctr.Resources.Limits[xpuResourceMem] if !ok { diff --git a/pkg/device/kunlun/vdevice_test.go b/pkg/device/kunlun/vdevice_test.go index 31d8919bc7..7745191280 100644 --- a/pkg/device/kunlun/vdevice_test.go +++ b/pkg/device/kunlun/vdevice_test.go @@ -567,3 +567,19 @@ func Test_FitVXPU_direct(t *testing.T) { }) } } + +func Test_KunlunVDevices_GenerateResourceRequests_CountRange(t *testing.T) { + dev := InitKunlunVDevice(testVConfig()) + for _, count := range []string{"0", "-1", "4294967296"} { + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceName(KunlunResourceVCount): resource.MustParse(count), + }, + }, + } + result, err := dev.GenerateResourceRequests(ctr) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, result) + assert.ErrorContains(t, err, "out of range") + } +} diff --git a/pkg/device/mthreads/device.go b/pkg/device/mthreads/device.go index 4b0ff1d7e2..fb67a34b8c 100644 --- a/pkg/device/mthreads/device.go +++ b/pkg/device/mthreads/device.go @@ -246,19 +246,19 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) (dev return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("core request %s is not a non-negative integer", core.String())} } // Coresreq is a per card percentage, but MutateAdmission rewrites - // the limit to count*coresPerMthreadsGPU (16) only when more than - // one device is requested, so a value above 100 with multiple - // devices is a total across the cards and has to be divided - // back. A value at or below 100 is already per card, and a value - // above 100 with a single requested device is plain invalid - // because MutateAdmission never writes one. - if corenums > 100 && n > 1 { + // the limit to count*coresPerMthreadsGPU (16) whenever more than + // one device is requested, so with multiple devices the value + // is a total across the cards and has to be divided back. With + // a single device MutateAdmission never writes a total, so a + // value above 100 is plain invalid. + if n > 1 { if corenums%n != 0 { klog.ErrorS(nil, "mthreads core request does not divide evenly across the requested devices", "container", ctr.Name, "request", core.String(), "devices", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("core request %d does not divide evenly across %d devices", corenums, n)} } corenums /= n - } else if corenums > 100 { + } + if corenums > 100 { klog.ErrorS(nil, "mthreads core request exceeds the per card limit", "container", ctr.Name, "request", core.String()) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("core request %d exceeds the per card limit of 100", corenums)} } diff --git a/pkg/device/mthreads/device_test.go b/pkg/device/mthreads/device_test.go index 7a18835ecf..a795efdefd 100644 --- a/pkg/device/mthreads/device_test.go +++ b/pkg/device/mthreads/device_test.go @@ -18,6 +18,7 @@ package mthreads import ( "flag" + "fmt" "testing" "github.com/Project-HAMi/HAMi/pkg/device" @@ -1505,3 +1506,32 @@ func TestGenerateResourceRequests_UnevenCoresFailsClosed(t *testing.T) { assert.DeepEqual(t, device.ContainerDeviceRequest{}, result) assert.ErrorContains(t, err, "does not divide evenly") } + +// TestGenerateResourceRequests_MultiCardCoresPerCard makes sure that with +// multiple devices every core total, including ones at or below 100 that +// MutateAdmission can produce (count*16), is divided back to the per card +// value and checked against the per card limit. +func TestGenerateResourceRequests_MultiCardCoresPerCard(t *testing.T) { + config := MthreadsConfig{ + ResourceCountName: "mthreads.com/vgpu", + ResourceMemoryName: "mthreads.com/sgpu-memory", + ResourceCoreName: "mthreads.com/sgpu-core", + } + InitMthreadsDevice(config) + dev := MthreadsDevices{} + + for cards := int64(2); cards <= 6; cards++ { + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "mthreads.com/vgpu": resource.MustParse(fmt.Sprint(cards)), + "mthreads.com/sgpu-core": resource.MustParse(fmt.Sprint(cards * 16)), + }, + }, + } + result, err := dev.GenerateResourceRequests(ctr) + assert.NilError(t, err) + assert.Equal(t, result.Nums, int32(cards)) + assert.Equal(t, result.Coresreq, int32(16)) + } +} From ae968d50798b73d5e8a0f2c516a6ae39446f2176 Mon Sep 17 00:00:00 2001 From: Eshiv Pandey Date: Thu, 10 Sep 2026 22:10:56 +0530 Subject: [PATCH 3/6] test(cambricon): assert the error return in Test_GenerateResourceRequests The error-path cases (zero/negative/overflowing counts, malformed memory) only compared the empty request and discarded the returned error, so they no longer proved the request is rejected. Capture the error and assert its message, and require a nil error for the accepted cases. Signed-off-by: Eshiv Pandey --- pkg/device/cambricon/device_test.go | 55 +++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/pkg/device/cambricon/device_test.go b/pkg/device/cambricon/device_test.go index 91260e7dfd..f7aae00912 100644 --- a/pkg/device/cambricon/device_test.go +++ b/pkg/device/cambricon/device_test.go @@ -422,9 +422,11 @@ func Test_checkType(t *testing.T) { func Test_GenerateResourceRequests(t *testing.T) { tests := []struct { - name string - args corev1.Container - want device.ContainerDeviceRequest + name string + args corev1.Container + want device.ContainerDeviceRequest + wantErr bool + errContains string }{ { name: "don't set to limits and request", @@ -516,7 +518,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "not a plain integer", }, { name: "zero count must not silently bypass quota", @@ -527,7 +531,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "out of range", }, { name: "decimal-form memory request is rejected, not treated as zero", @@ -539,7 +545,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "not a plain integer", }, { name: "negative count must be rejected", @@ -550,7 +558,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "out of range", }, { name: "max int32 count is accepted", @@ -578,7 +588,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "out of range", }, { name: "memory overflowing int32 is rejected, not truncated to zero", @@ -590,7 +602,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "not a plain integer", }, { name: "zero count must not silently bypass quota", @@ -601,7 +615,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "out of range", }, { name: "decimal-form memory request is rejected, not treated as zero", @@ -613,7 +629,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "not a plain integer", }, { name: "negative count must be rejected", @@ -624,7 +642,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "out of range", }, { name: "max int32 count is accepted", @@ -652,7 +672,9 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, + errContains: "out of range", }, } for _, test := range tests { @@ -664,8 +686,13 @@ func Test_GenerateResourceRequests(t *testing.T) { } InitMLUDevice(config) dev := CambriconDevices{} - result, _ := dev.GenerateResourceRequests(&test.args) + result, err := dev.GenerateResourceRequests(&test.args) assert.Equal(t, test.want, result) + if test.wantErr { + assert.ErrorContains(t, err, test.errContains) + } else { + require.NoError(t, err) + } }) } } From c83cbf00fd40773bee805c0b44b4496004e0b39c Mon Sep 17 00:00:00 2001 From: Eshiv Pandey Date: Mon, 14 Sep 2026 00:22:57 +0530 Subject: [PATCH 4/6] fix(device): treat an explicit zero device count as device-less, not invalid The fail-closed change rejected any count <= 0, but zero is not a malformed request: it is how a workload says it wants none of this vendor's devices. PR #2740 established that reading, returning an empty request so the pod is admitted and scheduled without a device, and charts commonly render a disabled GPU count as 0. fitResourceQuota runs GenerateResourceRequests on every pod the webhook sees, not only on pods carrying device resources, so an error for zero turned ordinary CPU pods with "nvidia.com/gpu: 0" into admission denials. Split the guard: zero returns an empty request with a nil error, while a negative or overflowing count still fails closed. A backend for which zero really is malformed keeps rejecting it in MutateAdmission, which only rejects containers that actually carry its resources -- awsneuron's shared validator is left untouched for exactly that reason. Also rewrite the mthreads Coresreq comment. The divide is unconditional on purpose and cannot adopt iluvatar's "corenums > 100 && n > 1" gate: iluvatar writes count*100 so every multi-card total exceeds 100, while mthreads writes count*16, whose 2-to-6 card totals are 32 to 96. Gating on > 100 there would leave those totals undivided and report 16x too many cores per card. A test pins the reasoning. Signed-off-by: Eshiv Pandey --- pkg/device/amd/device.go | 7 ++++- pkg/device/amd/device_test.go | 10 ++++++- pkg/device/ascend/device.go | 7 ++++- pkg/device/awsneuron/device.go | 6 +++++ pkg/device/biren/device.go | 7 ++++- pkg/device/cambricon/device.go | 7 ++++- pkg/device/cambricon/device_test.go | 12 +++------ pkg/device/devices.go | 7 +++++ pkg/device/enflame/gcu.go | 7 ++++- pkg/device/enflame/gcu_test.go | 15 ++++++++++- pkg/device/hygon/device.go | 7 ++++- pkg/device/iluvatar/device.go | 7 ++++- pkg/device/kunlun/device.go | 7 ++++- pkg/device/kunlun/vdevice.go | 7 ++++- pkg/device/kunlun/vdevice_test.go | 15 ++++++++++- pkg/device/metax/device.go | 7 ++++- pkg/device/metax/sdevice.go | 7 ++++- pkg/device/mthreads/device.go | 26 +++++++++++++----- pkg/device/mthreads/device_test.go | 42 +++++++++++++++++++++++++++++ pkg/device/nvidia/device.go | 9 ++++++- pkg/device/nvidia/device_test.go | 38 ++++++++++++++++++++++++++ pkg/device/vastai/device.go | 7 ++++- pkg/scheduler/webhook_test.go | 40 +++++++++++++++++++++++++++ 23 files changed, 273 insertions(+), 31 deletions(-) diff --git a/pkg/device/amd/device.go b/pkg/device/amd/device.go index cbdd50c223..a3caf2df36 100644 --- a/pkg/device/amd/device.go +++ b/pkg/device/amd/device.go @@ -208,7 +208,12 @@ func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) (device.C count, ok := ctr.Resources.Limits[amdResourceCount] if ok { if n, ok := count.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "amd device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "amd", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/amd/device_test.go b/pkg/device/amd/device_test.go index 68aec9b511..fe1a58b0a1 100644 --- a/pkg/device/amd/device_test.go +++ b/pkg/device/amd/device_test.go @@ -328,12 +328,20 @@ func Test_GenerateResourceRequests(t *testing.T) { } }) + t.Run("zero count is device-less, not invalid", func(t *testing.T) { + ctr := &corev1.Container{Resources: corev1.ResourceRequirements{Limits: corev1.ResourceList{ + "amd.com/gpu": *resource.NewQuantity(0, resource.DecimalSI), + }}} + got, err := dev.GenerateResourceRequests(ctr) + assert.NilError(t, err) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, got) + }) + for _, tc := range []struct { name string resource corev1.ResourceName value int64 }{ - {name: "rejects zero count", resource: "amd.com/gpu", value: 0}, {name: "rejects overflowing count", resource: "amd.com/gpu", value: math.MaxInt32 + 1}, {name: "rejects negative memory", resource: "amd.com/gpu-mem", value: -1}, {name: "rejects overflowing memory", resource: "amd.com/gpu-mem", value: math.MaxInt32 + 1}, diff --git a/pkg/device/ascend/device.go b/pkg/device/ascend/device.go index 9a50cc9f8a..62ed3ef112 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -344,7 +344,12 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) (device.Cont klog.V(3).Infof("Counting %s devices", dev.config.CommonWord) if n, ok := v.AsInt64(); ok { klog.Info("Found AscendDevices devices") - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "ascend device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: dev.config.CommonWord, Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/awsneuron/device.go b/pkg/device/awsneuron/device.go index 29b54c9d34..c64c2437a9 100644 --- a/pkg/device/awsneuron/device.go +++ b/pkg/device/awsneuron/device.go @@ -292,6 +292,12 @@ func (dev *AWSNeuronDevices) GenerateResourceRequests(ctr *corev1.Container) (de awsResourceCores := corev1.ResourceName(dev.resourceCoreName) v, ok := resourceQuantity(ctr, awsResourceCount) if ok { + // An explicit zero count means no device is requested, not an + // invalid request. See the nvidia backend. MutateAdmission keeps + // using the shared validator, which still rejects zero there. + if zero, isInt := v.AsInt64(); isInt && zero == 0 { + return device.ContainerDeviceRequest{}, nil + } n, err := validateResourceRequest(v, dev.resourceCountName, maxAWSNeuronDeviceCount) if err != nil { klog.ErrorS(err, "Invalid awsNeuron device request", "container", ctr.Name) diff --git a/pkg/device/biren/device.go b/pkg/device/biren/device.go index a0e6389c33..79b5bf18d4 100644 --- a/pkg/device/biren/device.go +++ b/pkg/device/biren/device.go @@ -134,7 +134,12 @@ func (dev *BirenDevices) GenerateResourceRequests(ctr *corev1.Container) (device } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "biren device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "biren", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/cambricon/device.go b/pkg/device/cambricon/device.go index ce22e123c5..28579492f9 100644 --- a/pkg/device/cambricon/device.go +++ b/pkg/device/cambricon/device.go @@ -267,7 +267,12 @@ func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) (de } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "cambricon device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "cambricon", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/cambricon/device_test.go b/pkg/device/cambricon/device_test.go index f7aae00912..1d8edd5912 100644 --- a/pkg/device/cambricon/device_test.go +++ b/pkg/device/cambricon/device_test.go @@ -523,7 +523,7 @@ func Test_GenerateResourceRequests(t *testing.T) { errContains: "not a plain integer", }, { - name: "zero count must not silently bypass quota", + name: "zero count is device-less, not invalid", args: corev1.Container{ Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -531,9 +531,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, - wantErr: true, - errContains: "out of range", + want: device.ContainerDeviceRequest{}, }, { name: "decimal-form memory request is rejected, not treated as zero", @@ -607,7 +605,7 @@ func Test_GenerateResourceRequests(t *testing.T) { errContains: "not a plain integer", }, { - name: "zero count must not silently bypass quota", + name: "zero count is device-less, not invalid", args: corev1.Container{ Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -615,9 +613,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, - wantErr: true, - errContains: "out of range", + want: device.ContainerDeviceRequest{}, }, { name: "decimal-form memory request is rejected, not treated as zero", diff --git a/pkg/device/devices.go b/pkg/device/devices.go index a545f499d6..e75657d496 100644 --- a/pkg/device/devices.go +++ b/pkg/device/devices.go @@ -45,6 +45,13 @@ type Devices interface { // request this vendor's devices. A non-nil error means it does, but the // request is invalid and the caller must fail closed instead of silently // treating the pod as device-less. + // + // An explicitly requested count of zero is "no device", not an invalid + // request, so it belongs in the first case. Admission runs this on every + // pod, and charts commonly render a disabled GPU count as 0, so returning + // an error for it would deny ordinary CPU pods. A backend for which zero + // really is malformed should reject it in MutateAdmission instead, which + // only rejects containers that actually carry its resources. GenerateResourceRequests(ctr *corev1.Container) (ContainerDeviceRequest, error) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd PodDevices) map[string]string ScoreNode(node *corev1.Node, podDevices PodSingleDevice, previous []*DeviceUsage, policy string) float32 diff --git a/pkg/device/enflame/gcu.go b/pkg/device/enflame/gcu.go index e054f7b6ff..9b74252b0b 100644 --- a/pkg/device/enflame/gcu.go +++ b/pkg/device/enflame/gcu.go @@ -98,7 +98,12 @@ func (dev *GCUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.C } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "enflame device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "enflame", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/enflame/gcu_test.go b/pkg/device/enflame/gcu_test.go index 59b0673943..fffb62f279 100644 --- a/pkg/device/enflame/gcu_test.go +++ b/pkg/device/enflame/gcu_test.go @@ -730,7 +730,7 @@ func TestGCUDevices_AddResourceUsage(t *testing.T) { func TestGCUDevices_GenerateResourceRequests_CountRange(t *testing.T) { dev := GCUDevices{} - for _, count := range []string{"0", "-1", "4294967296"} { + for _, count := range []string{"-1", "4294967296"} { ctr := &corev1.Container{ Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -742,4 +742,17 @@ func TestGCUDevices_GenerateResourceRequests_CountRange(t *testing.T) { assert.DeepEqual(t, device.ContainerDeviceRequest{}, result) assert.ErrorContains(t, err, "out of range") } + + // A count of zero says "no device", so the container is device-less + // rather than invalid and must not be rejected. + zero := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceName(EnflameResourceNameGCU): resource.MustParse("0"), + }, + }, + } + zeroResult, err := dev.GenerateResourceRequests(zero) + assert.NilError(t, err) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, zeroResult) } diff --git a/pkg/device/hygon/device.go b/pkg/device/hygon/device.go index 45f3c7e6ba..a4d373b16b 100644 --- a/pkg/device/hygon/device.go +++ b/pkg/device/hygon/device.go @@ -163,7 +163,12 @@ func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.C } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "hcu device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "hcu", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/iluvatar/device.go b/pkg/device/iluvatar/device.go index abb3bd2b53..054e5484b8 100644 --- a/pkg/device/iluvatar/device.go +++ b/pkg/device/iluvatar/device.go @@ -195,7 +195,12 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) (dev } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "iluvatar device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "iluvatar", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/kunlun/device.go b/pkg/device/kunlun/device.go index 0756a4a712..058edfecb1 100644 --- a/pkg/device/kunlun/device.go +++ b/pkg/device/kunlun/device.go @@ -138,7 +138,12 @@ func (dev *KunlunDevices) GenerateResourceRequests(ctr *corev1.Container) (devic } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "kunlun device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "kunlun", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/kunlun/vdevice.go b/pkg/device/kunlun/vdevice.go index d05bc1a31c..0e07fa18e7 100644 --- a/pkg/device/kunlun/vdevice.go +++ b/pkg/device/kunlun/vdevice.go @@ -165,7 +165,12 @@ func (dev *KunlunVDevices) GenerateResourceRequests(ctr *corev1.Container) (devi if ok { klog.V(3).Infof("Counting %s devices", dev.CommonWord()) if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "kunlun vdevice count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "kunlun", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/kunlun/vdevice_test.go b/pkg/device/kunlun/vdevice_test.go index 7745191280..0362233083 100644 --- a/pkg/device/kunlun/vdevice_test.go +++ b/pkg/device/kunlun/vdevice_test.go @@ -570,7 +570,7 @@ func Test_FitVXPU_direct(t *testing.T) { func Test_KunlunVDevices_GenerateResourceRequests_CountRange(t *testing.T) { dev := InitKunlunVDevice(testVConfig()) - for _, count := range []string{"0", "-1", "4294967296"} { + for _, count := range []string{"-1", "4294967296"} { ctr := &corev1.Container{ Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -582,4 +582,17 @@ func Test_KunlunVDevices_GenerateResourceRequests_CountRange(t *testing.T) { assert.DeepEqual(t, device.ContainerDeviceRequest{}, result) assert.ErrorContains(t, err, "out of range") } + + // A count of zero says "no device", so the container is device-less + // rather than invalid and must not be rejected. + zero := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceName(KunlunResourceVCount): resource.MustParse("0"), + }, + }, + } + zeroResult, err := dev.GenerateResourceRequests(zero) + assert.NilError(t, err) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, zeroResult) } diff --git a/pkg/device/metax/device.go b/pkg/device/metax/device.go index 7b69f945bc..2b9631792f 100644 --- a/pkg/device/metax/device.go +++ b/pkg/device/metax/device.go @@ -141,7 +141,12 @@ func (dev *MetaxDevices) GenerateResourceRequests(ctr *corev1.Container) (device } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "metax device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/metax/sdevice.go b/pkg/device/metax/sdevice.go index c3afc80e77..8c04ac4ff0 100644 --- a/pkg/device/metax/sdevice.go +++ b/pkg/device/metax/sdevice.go @@ -212,7 +212,12 @@ func (sdev *MetaxSDevices) GenerateResourceRequests(ctr *corev1.Container) (devi ctr.Name, MetaxResourceNameVCount) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax sgpu", Reason: fmt.Sprintf("count request %s cannot decode to int64", value.String())} } - if count <= 0 || count > math.MaxInt32 { + if count == 0 { + // An explicit zero count means no device is requested, not an + // invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if count < 0 || count > math.MaxInt32 { klog.ErrorS(nil, "metax sgpu device count request is out of range", "container", ctr.Name, "request", count) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax sgpu", Reason: fmt.Sprintf("device count %d is out of range", count)} } diff --git a/pkg/device/mthreads/device.go b/pkg/device/mthreads/device.go index fb67a34b8c..eadca20ddc 100644 --- a/pkg/device/mthreads/device.go +++ b/pkg/device/mthreads/device.go @@ -213,7 +213,12 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) (dev klog.InfoS("Detected mthreads device request", "container", ctr.Name, "deviceCount", n) - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("device count %d is out of range", n)} } memnum := 0 @@ -245,12 +250,19 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) (dev klog.ErrorS(nil, "mthreads core request is not a non-negative integer", "container", ctr.Name, "request", core.String()) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("core request %s is not a non-negative integer", core.String())} } - // Coresreq is a per card percentage, but MutateAdmission rewrites - // the limit to count*coresPerMthreadsGPU (16) whenever more than - // one device is requested, so with multiple devices the value - // is a total across the cards and has to be divided back. With - // a single device MutateAdmission never writes a total, so a - // value above 100 is plain invalid. + // Coresreq is a per card value. With more than one device the + // limit held here is always a total across the cards, never a + // per card value, because MutateAdmission overwrites the core + // limit with count*coresPerMthreadsGPU (16) whenever count > 1. + // So the total is divided back unconditionally. + // + // This deliberately does not copy the iluvatar backend's + // "corenums > 100 && n > 1" gate. That gate works there because + // iluvatar writes count*100, so every multi card total exceeds + // 100. Here a total is count*16, which for 2 to 6 cards is 32 to + // 96, all at or below 100, so gating on > 100 would leave those + // totals undivided and report 16 times too many cores per card. + // The per card limit is still enforced, just after the division. if n > 1 { if corenums%n != 0 { klog.ErrorS(nil, "mthreads core request does not divide evenly across the requested devices", "container", ctr.Name, "request", core.String(), "devices", n) diff --git a/pkg/device/mthreads/device_test.go b/pkg/device/mthreads/device_test.go index a795efdefd..a5fa009fa1 100644 --- a/pkg/device/mthreads/device_test.go +++ b/pkg/device/mthreads/device_test.go @@ -1535,3 +1535,45 @@ func TestGenerateResourceRequests_MultiCardCoresPerCard(t *testing.T) { assert.Equal(t, result.Coresreq, int32(16)) } } + +// TestGenerateResourceRequests_MultiCardCoresAreAlwaysATotal pins why the +// division above is unconditional for n > 1 rather than gated on a value above +// 100 the way the iluvatar backend gates it. +// +// A user-supplied per card value cannot survive to GenerateResourceRequests +// when more than one card is requested: MutateAdmission overwrites the core +// limit with count*16 first. So a spec asking for 2 cards at 60 cores each is +// rewritten to a total of 32 and read back as 16 per card, and the "60 becomes +// 30" reading never arises. Gating the division on a value above 100 would +// instead leave the count*16 totals for 2 to 6 cards (32 to 96) undivided. +func TestGenerateResourceRequests_MultiCardCoresAreAlwaysATotal(t *testing.T) { + config := MthreadsConfig{ + ResourceCountName: "mthreads.com/vgpu", + ResourceMemoryName: "mthreads.com/sgpu-memory", + ResourceCoreName: "mthreads.com/sgpu-core", + } + InitMthreadsDevice(config) + dev := MthreadsDevices{} + + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "mthreads.com/vgpu": resource.MustParse("2"), + "mthreads.com/sgpu-core": resource.MustParse("60"), + }, + }, + } + + mutated, err := dev.MutateAdmission(ctr, &corev1.Pod{}) + assert.NilError(t, err) + assert.Assert(t, mutated) + + // The per card 60 is gone by the time the request is generated. + rewritten := ctr.Resources.Limits["mthreads.com/sgpu-core"] + assert.Equal(t, rewritten.Value(), int64(32)) + + result, err := dev.GenerateResourceRequests(ctr) + assert.NilError(t, err) + assert.Equal(t, result.Nums, int32(2)) + assert.Equal(t, result.Coresreq, int32(16)) +} diff --git a/pkg/device/nvidia/device.go b/pkg/device/nvidia/device.go index ace1d8ddbb..7e68732dcd 100644 --- a/pkg/device/nvidia/device.go +++ b/pkg/device/nvidia/device.go @@ -536,7 +536,14 @@ func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) (de } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count is how a workload asks for no + // device at all, so the container is device-less rather + // than invalid. Returning an error here would deny every + // pod that templates its count down to 0. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "nvidia device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "nvidia", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/device/nvidia/device_test.go b/pkg/device/nvidia/device_test.go index 7cb81ea591..6efa020e32 100644 --- a/pkg/device/nvidia/device_test.go +++ b/pkg/device/nvidia/device_test.go @@ -3641,3 +3641,41 @@ func TestGenerateResourceRequests_InvalidCoresFailsClosed(t *testing.T) { assert.DeepEqual(t, device.ContainerDeviceRequest{}, result) assert.ErrorContains(t, err, "out of range") } + +// TestGenerateResourceRequests_ZeroCountIsDeviceLess pins the distinction +// between an explicit zero count and a genuinely invalid one. "nvidia.com/gpu: 0" +// is a common way to say "no GPU" (chart templates render it that way whenever +// GPU support is switched off), so it has to stay a device-less pod that is +// admitted and scheduled normally rather than being denied. Only a negative or +// out-of-int32 count is an error. +func TestGenerateResourceRequests_ZeroCountIsDeviceLess(t *testing.T) { + config := NvidiaConfig{ + ResourceCountName: "nvidia.com/gpu", + ResourceMemoryName: "nvidia.com/gpumem", + ResourceCoreName: "nvidia.com/gpucores", + ResourceMemoryPercentageName: "nvidia.com/gpumem-percentage", + } + dev := InitNvidiaDevice(config) + + zero := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "nvidia.com/gpu": *resource.NewQuantity(0, resource.BinarySI), + }, + }, + } + zeroResult, err := dev.GenerateResourceRequests(zero) + assert.NilError(t, err) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, zeroResult) + + negative := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "nvidia.com/gpu": *resource.NewQuantity(-1, resource.BinarySI), + }, + }, + } + negativeResult, err := dev.GenerateResourceRequests(negative) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, negativeResult) + assert.ErrorContains(t, err, "out of range") +} diff --git a/pkg/device/vastai/device.go b/pkg/device/vastai/device.go index 55450363d5..2c89f535ca 100644 --- a/pkg/device/vastai/device.go +++ b/pkg/device/vastai/device.go @@ -136,7 +136,12 @@ func (dev *VastaiDevices) GenerateResourceRequests(ctr *corev1.Container) (devic } if ok { if n, ok := v.AsInt64(); ok { - if n <= 0 || n > math.MaxInt32 { + if n == 0 { + // An explicit zero count means no device is requested, + // not an invalid request. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil + } + if n < 0 || n > math.MaxInt32 { klog.ErrorS(nil, "vastai device count request is out of range", "container", ctr.Name, "request", n) return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "vastai", Reason: fmt.Sprintf("device count %d is out of range", n)} } diff --git a/pkg/scheduler/webhook_test.go b/pkg/scheduler/webhook_test.go index 07a051bfeb..477a0069b4 100644 --- a/pkg/scheduler/webhook_test.go +++ b/pkg/scheduler/webhook_test.go @@ -687,6 +687,46 @@ func TestFitResourceQuotaCountsEveryDevice(t *testing.T) { } } +// A count of zero is how a workload says "no device", not a malformed request, +// so admission has to let it through. The webhook runs fitResourceQuota on +// every pod it sees, so treating zero as invalid here would deny ordinary CPU +// pods whose chart renders the GPU count as 0. +func TestFitResourceQuotaAllowsZeroCount(t *testing.T) { + config.SchedulerName = "hami-scheduler" + + sConfig := &config.Config{ + NvidiaConfig: nvidia.NvidiaConfig{ + ResourceCountName: "nvidia.com/gpu", + ResourceMemoryName: "nvidia.com/gpumem", + ResourceMemoryPercentageName: "nvidia.com/gpumem-percentage", + ResourceCoreName: "nvidia.com/gpucores", + DefaultGPUNum: 1, + MemoryFactor: 1, + }, + } + if err := config.InitDevicesWithConfig(sConfig); err != nil { + t.Fatalf("failed to initialize devices: %v", err) + } + + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "zero-gpu", Namespace: "zero-gpu-ns"}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "app", + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "nvidia.com/gpu": *resource.NewQuantity(0, resource.BinarySI), + }, + }, + }}, + }, + } + + if err := fitResourceQuota(pod); err != nil { + t.Errorf("fitResourceQuota() = %v, want nil: a zero count means no device is requested, not an invalid request", err) + } +} + // Ascend applies a configurable factor to the memory it records, so the limit // has to be raised by the same factor or pods that are within quota get denied. func TestFitResourceQuotaAscendMemoryFactor(t *testing.T) { From 58930a200c5b13fe739fe412973f67018d634911 Mon Sep 17 00:00:00 2001 From: Eshiv Pandey Date: Mon, 14 Sep 2026 03:01:13 +0530 Subject: [PATCH 5/6] fix(device): reject a device count that does not fit in an int64 Quantity.AsInt64 fails for a count that is integral but larger than an int64, such as 1Ei or 1e19. The apiserver accepts those for an extended resource, so they reach the backends, where the failed conversion fell through to an empty request with a nil error. That is the silently device-less pod this change set exists to remove, so report it as an invalid request instead. Assert both this and the zero-is-device-less contract across every registered backend at once, since either is easy to regress one backend at a time. Signed-off-by: Eshiv Pandey --- pkg/device/amd/device.go | 6 ++++ pkg/device/ascend/device.go | 6 ++++ pkg/device/biren/device.go | 6 ++++ pkg/device/cambricon/device.go | 6 ++++ pkg/device/enflame/device.go | 11 +++++++- pkg/device/enflame/gcu.go | 6 ++++ pkg/device/hygon/device.go | 6 ++++ pkg/device/iluvatar/device.go | 6 ++++ pkg/device/kunlun/device.go | 6 ++++ pkg/device/kunlun/vdevice.go | 6 ++++ pkg/device/metax/device.go | 6 ++++ pkg/device/mthreads/device.go | 6 ++++ pkg/device/nvidia/device.go | 6 ++++ pkg/device/nvidia/device_test.go | 14 ++++++++++ pkg/device/vastai/device.go | 6 ++++ pkg/scheduler/config/config_test.go | 43 +++++++++++++++++++++++++++++ 16 files changed, 145 insertions(+), 1 deletion(-) diff --git a/pkg/device/amd/device.go b/pkg/device/amd/device.go index a3caf2df36..59902e647e 100644 --- a/pkg/device/amd/device.go +++ b/pkg/device/amd/device.go @@ -255,6 +255,12 @@ func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) (device.C Coresreq: corePercentageNum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "amd device count request is not a plain integer", "container", ctr.Name, "request", count.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "amd", Reason: fmt.Sprintf("device count %s is not a plain integer", count.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/ascend/device.go b/pkg/device/ascend/device.go index 62ed3ef112..8c8a6971ee 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -417,6 +417,12 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) (device.Cont Coresreq: corenum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "ascend device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "ascend", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/biren/device.go b/pkg/device/biren/device.go index 79b5bf18d4..1d145b46b5 100644 --- a/pkg/device/biren/device.go +++ b/pkg/device/biren/device.go @@ -156,6 +156,12 @@ func (dev *BirenDevices) GenerateResourceRequests(ctr *corev1.Container) (device Coresreq: corenum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "biren device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "biren", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/cambricon/device.go b/pkg/device/cambricon/device.go index 28579492f9..0b3a2bae6c 100644 --- a/pkg/device/cambricon/device.go +++ b/pkg/device/cambricon/device.go @@ -320,6 +320,12 @@ func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) (de Coresreq: corenum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "cambricon device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "cambricon", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{ Nums: 0, diff --git a/pkg/device/enflame/device.go b/pkg/device/enflame/device.go index e49157f59b..257545a184 100644 --- a/pkg/device/enflame/device.go +++ b/pkg/device/enflame/device.go @@ -323,7 +323,16 @@ func (dev *EnflameDevices) GenerateResourceRequests(ctr *corev1.Container) (devi v, ok = ctr.Resources.Requests[resourceCount] } if ok { - if n, ok := v.AsInt64(); ok && n > 0 { + n, isInt := v.AsInt64() + if !isInt { + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through to the memory/core + // path would report the container as device-less, which is the + // fail-open this change exists to remove. + klog.ErrorS(nil, "enflame drs request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "enflame", Reason: fmt.Sprintf("drs request %s is not a plain integer", v.String())} + } + if n > 0 { klog.Info("Found enflame devices") if n > math.MaxInt32 { klog.ErrorS(nil, "drs request is too large", "container", ctr.Name, "request", n) diff --git a/pkg/device/enflame/gcu.go b/pkg/device/enflame/gcu.go index 9b74252b0b..16e034dc66 100644 --- a/pkg/device/enflame/gcu.go +++ b/pkg/device/enflame/gcu.go @@ -116,6 +116,12 @@ func (dev *GCUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.C Coresreq: 100, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "enflame device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "enflame", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/hygon/device.go b/pkg/device/hygon/device.go index a4d373b16b..058bfb37e8 100644 --- a/pkg/device/hygon/device.go +++ b/pkg/device/hygon/device.go @@ -224,6 +224,12 @@ func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.C Coresreq: corenum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "hygon device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "hygon", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/iluvatar/device.go b/pkg/device/iluvatar/device.go index 054e5484b8..d277c0f7d1 100644 --- a/pkg/device/iluvatar/device.go +++ b/pkg/device/iluvatar/device.go @@ -263,6 +263,12 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) (dev Coresreq: corenum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "iluvatar device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "iluvatar", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/kunlun/device.go b/pkg/device/kunlun/device.go index 058edfecb1..40bc3fd92c 100644 --- a/pkg/device/kunlun/device.go +++ b/pkg/device/kunlun/device.go @@ -157,6 +157,12 @@ func (dev *KunlunDevices) GenerateResourceRequests(ctr *corev1.Container) (devic Coresreq: 0, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "kunlun device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "kunlun", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/kunlun/vdevice.go b/pkg/device/kunlun/vdevice.go index 0e07fa18e7..ab3be9e34c 100644 --- a/pkg/device/kunlun/vdevice.go +++ b/pkg/device/kunlun/vdevice.go @@ -201,6 +201,12 @@ func (dev *KunlunVDevices) GenerateResourceRequests(ctr *corev1.Container) (devi Coresreq: int32(cores), }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "kunlun device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "kunlun", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/metax/device.go b/pkg/device/metax/device.go index 2b9631792f..e0490896b9 100644 --- a/pkg/device/metax/device.go +++ b/pkg/device/metax/device.go @@ -159,6 +159,12 @@ func (dev *MetaxDevices) GenerateResourceRequests(ctr *corev1.Container) (device Coresreq: 100, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "metax device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "metax", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/mthreads/device.go b/pkg/device/mthreads/device.go index eadca20ddc..51e11bccce 100644 --- a/pkg/device/mthreads/device.go +++ b/pkg/device/mthreads/device.go @@ -290,6 +290,12 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) (dev Coresreq: corenum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "mthreads device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "mthreads", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/nvidia/device.go b/pkg/device/nvidia/device.go index 7e68732dcd..1e0c9b6df5 100644 --- a/pkg/device/nvidia/device.go +++ b/pkg/device/nvidia/device.go @@ -616,6 +616,12 @@ func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) (de Coresreq: corenum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "nvidia device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "nvidia", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/device/nvidia/device_test.go b/pkg/device/nvidia/device_test.go index 6efa020e32..9d38bc7e9c 100644 --- a/pkg/device/nvidia/device_test.go +++ b/pkg/device/nvidia/device_test.go @@ -3678,4 +3678,18 @@ func TestGenerateResourceRequests_ZeroCountIsDeviceLess(t *testing.T) { negativeResult, err := dev.GenerateResourceRequests(negative) assert.DeepEqual(t, device.ContainerDeviceRequest{}, negativeResult) assert.ErrorContains(t, err, "out of range") + + // The apiserver accepts 1Ei for an extended resource, but it does not + // fit in an int64, so AsInt64 reports failure. That has to fail closed + // rather than fall through to a device-less request. + tooLarge := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "nvidia.com/gpu": resource.MustParse("1Ei"), + }, + }, + } + tooLargeResult, err := dev.GenerateResourceRequests(tooLarge) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, tooLargeResult) + assert.ErrorContains(t, err, "not a plain integer") } diff --git a/pkg/device/vastai/device.go b/pkg/device/vastai/device.go index 2c89f535ca..fdb3be294c 100644 --- a/pkg/device/vastai/device.go +++ b/pkg/device/vastai/device.go @@ -158,6 +158,12 @@ func (dev *VastaiDevices) GenerateResourceRequests(ctr *corev1.Container) (devic Coresreq: corenum, }, nil } + // A quantity the apiserver accepts as an integer can still be too + // large for int64 (1Ei, 1e19). Falling through would report the + // container as device-less, which is the fail-open this change + // exists to remove. + klog.ErrorS(nil, "vastai device count request is not a plain integer", "container", ctr.Name, "request", v.String()) + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "vastai", Reason: fmt.Sprintf("device count %s is not a plain integer", v.String())} } return device.ContainerDeviceRequest{}, nil } diff --git a/pkg/scheduler/config/config_test.go b/pkg/scheduler/config/config_test.go index 09f2f5e015..8aaebad9bb 100644 --- a/pkg/scheduler/config/config_test.go +++ b/pkg/scheduler/config/config_test.go @@ -787,3 +787,46 @@ func Test_Resourcereqs(t *testing.T) { }) } } + +// A count the apiserver accepts as an integer can still be too large for +// int64, which makes Quantity.AsInt64 report failure. Every backend has to +// fail closed on that rather than report the container as device-less, and +// every backend has to treat an explicit zero as device-less rather than +// invalid. Both are easy to regress one backend at a time, so assert them +// across all of them at once. +func TestGenerateResourceRequests_CountEdgesAcrossBackends(t *testing.T) { + InitDefaultDevices() + + for commonWord, dev := range device.DevicesMap { + countName := dev.GetResourceNames().ResourceCountName + if countName == "" { + continue + } + + t.Run(commonWord+"/too large for int64 fails closed", func(t *testing.T) { + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceName(countName): resource.MustParse("1Ei"), + }, + }, + } + got, err := dev.GenerateResourceRequests(ctr) + assert.Assert(t, err != nil, "%s accepted a count that does not fit in an int64", commonWord) + assert.DeepEqual(t, device.ContainerDeviceRequest{}, got) + }) + + t.Run(commonWord+"/zero is device-less", func(t *testing.T) { + ctr := &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceName(countName): resource.MustParse("0"), + }, + }, + } + got, err := dev.GenerateResourceRequests(ctr) + assert.NilError(t, err) + assert.Equal(t, got.Nums, int32(0), "%s reported devices for a zero count", commonWord) + }) + } +} From e8e79fe109713246729587bd2b57cb165942a937 Mon Sep 17 00:00:00 2001 From: Eshiv Pandey Date: Wed, 23 Sep 2026 21:55:56 +0530 Subject: [PATCH 6/6] fix(device-plugin): fail closed on config load error instead of Fatalf LoadNvidiaDevicePluginConfig aborted the process with klog.Fatalf when the device config file failed to load. After master's strict-YAML config parsing (#2941), that path became reachable from TestLoadNvidiaDevicePluginConfigFailsWhenTheNodeCannotBeRead, which fed a minimal config and expected a returned error; the Fatalf killed the whole test binary instead. Return the error like the node-read path already does, so a bad config surfaces to the caller (factory.go already handles it) rather than taking the plugin down. Signed-off-by: Eshiv Pandey --- pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go b/pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go index fd19d945f8..00c4a95853 100644 --- a/pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go +++ b/pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go @@ -167,7 +167,10 @@ func readFromConfigFile(sConfig *nvidia.NvidiaConfig, path string) (string, erro func LoadNvidiaDevicePluginConfig() (*config.Config, string, error) { sConfig, err := config.LoadConfig(*ConfigFile) if err != nil { - klog.Fatalf(`failed to load device config file %s: %v`, *ConfigFile, err) + // Fail closed rather than aborting the process: a bad config file + // must surface as an error the caller can act on, not a Fatalf that + // takes the plugin (and any test binary) down with it. + return nil, "", fmt.Errorf("load device config file %s: %w", *ConfigFile, err) } mode, err := readFromConfigFile(&sConfig.NvidiaConfig, ConfigFilePath) if err != nil {