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 { diff --git a/pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go b/pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go index 5332ce98dd..d620427df2 100644 --- a/pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go +++ b/pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go @@ -138,7 +138,10 @@ func (plugin *NvidiaDevicePlugin) validateContainerAllocation(ctr *corev1.Contai if !ok { return nil } - req := dev.GenerateResourceRequests(ctr) + req, err := dev.GenerateResourceRequests(ctr) + if err != nil { + return err + } for _, each := range allocated { limit, bounded := plugin.memoryLimitMB(req, each.UUID) if bounded && each.Usedmem > limit { diff --git a/pkg/device/amd/device.go b/pkg/device/amd/device.go index c8a0916daf..59902e647e 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) @@ -208,9 +208,14 @@ func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co 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{} + 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 +223,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 +239,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 +253,16 @@ func (dev *AMDDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co Memreq: memnum, MemPercentagereq: 0, 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{} + 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..fe1a58b0a1 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,17 +323,25 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) assert.DeepEqual(t, device.ContainerDeviceRequest{}, got) } }) + 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}, @@ -346,7 +354,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 562fc89c10..5f64802ffe 100644 --- a/pkg/device/ascend/device.go +++ b/pkg/device/ascend/device.go @@ -364,7 +364,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) @@ -377,9 +377,14 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta 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{} + 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] @@ -390,7 +395,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 { @@ -398,7 +403,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 @@ -407,7 +412,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) } @@ -426,7 +431,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) } @@ -443,10 +448,16 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta Memreq: int32(memnum), MemPercentagereq: int32(mempnum), 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{} + 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 38baf4e904..24e24c894f 100644 --- a/pkg/device/ascend/device_test.go +++ b/pkg/device/ascend/device_test.go @@ -1766,7 +1766,7 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, } - result := dev.GenerateResourceRequests(&test.args) + result, _ := dev.GenerateResourceRequests(&test.args) assert.Equal(t, result, test.want) }) @@ -1816,7 +1816,7 @@ func Test_GenerateResourceRequests_VNPUCoreMode(t *testing.T) { }, }, } - result := dev.GenerateResourceRequests(&test.args) + result, _ := dev.GenerateResourceRequests(&test.args) assert.Equal(t, result, test.want) }) @@ -1961,7 +1961,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) }) } @@ -2081,7 +2081,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) }) } @@ -2141,7 +2141,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{}) }) } @@ -3708,7 +3708,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 6f6d8438a8..1a1cb07f29 100644 --- a/pkg/device/awsneuron/device.go +++ b/pkg/device/awsneuron/device.go @@ -272,16 +272,22 @@ 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) 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) - 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, @@ -294,14 +300,14 @@ func (dev *AWSNeuronDevices) GenerateResourceRequests(ctr *corev1.Container) dev // A zero core request denotes a whole AWS Neuron device. Fit converts // it to the selected device's actual addressable core count. Coresreq: 0, - } + }, 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()} } klog.InfoS("Detected awsNeuron device request", "container", ctr.Name, @@ -312,10 +318,10 @@ func (dev *AWSNeuronDevices) GenerateResourceRequests(ctr *corev1.Container) dev Memreq: 0, MemPercentagereq: 0, TotalCoresreq: n, - } + }, 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_geometry_test.go b/pkg/device/awsneuron/device_geometry_test.go index 8f5d11a972..a5d957c61b 100644 --- a/pkg/device/awsneuron/device_geometry_test.go +++ b/pkg/device/awsneuron/device_geometry_test.go @@ -115,16 +115,21 @@ func TestGenerateResourceRequestsDoesNotDependOnRegisteredNode(t *testing.T) { }, } - want := dev.GenerateResourceRequests(container) + want, err := dev.GenerateResourceRequests(container) + assert.NilError(t, err) assert.Equal(t, want.Coresreq, int32(0)) - _, err := dev.GetNodeDevices(newNeuronNode("inf1-node", "inf1.xlarge", 1, 4)) + _, err = dev.GetNodeDevices(newNeuronNode("inf1-node", "inf1.xlarge", 1, 4)) + assert.NilError(t, err) + got, err := dev.GenerateResourceRequests(container) assert.NilError(t, err) - assert.DeepEqual(t, dev.GenerateResourceRequests(container), want) + assert.DeepEqual(t, got, want) _, err = dev.GetNodeDevices(newNeuronNode("inf2-node", "inf2.xlarge", 1, 2)) assert.NilError(t, err) - assert.DeepEqual(t, dev.GenerateResourceRequests(container), want) + got, err = dev.GenerateResourceRequests(container) + assert.NilError(t, err) + assert.DeepEqual(t, got, want) } func TestMixedNodeGeometryAllocationWorkflow(t *testing.T) { @@ -188,7 +193,8 @@ func TestMixedNodeGeometryAllocationWorkflow(t *testing.T) { }, } - request := dev.GenerateResourceRequests(&pod.Spec.Containers[0]) + request, err := dev.GenerateResourceRequests(&pod.Spec.Containers[0]) + assert.NilError(t, err) fit, allocations, reason := dev.Fit( []*device.DeviceUsage{usage}, request, diff --git a/pkg/device/awsneuron/device_test.go b/pkg/device/awsneuron/device_test.go index af4ff31169..a10fc44911 100644 --- a/pkg/device/awsneuron/device_test.go +++ b/pkg/device/awsneuron/device_test.go @@ -527,9 +527,10 @@ 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 }{ { name: "allocate neuron device", @@ -560,7 +561,8 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, }, { name: "allocate neuron core", @@ -624,7 +626,8 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, }, { name: "reject negative neuron core request when admission is bypassed", @@ -635,7 +638,8 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, }, { name: "reject neuron core request that exceeds the supported maximum when admission is bypassed", @@ -646,7 +650,8 @@ func Test_GenerateResourceRequests(t *testing.T) { }, }, }, - want: device.ContainerDeviceRequest{}, + want: device.ContainerDeviceRequest{}, + wantErr: true, }, } for _, test := range tests { @@ -656,7 +661,12 @@ func Test_GenerateResourceRequests(t *testing.T) { ResourceCoreName: "aws.amazon.com/neuroncore", } dev := InitAWSNeuronDevice(config) - result := dev.GenerateResourceRequests(test.args) + result, err := dev.GenerateResourceRequests(test.args) + if test.wantErr { + assert.Assert(t, err != nil) + return + } + assert.NilError(t, err) 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 43f98aa259..5b0b2a6792 100644 --- a/pkg/device/awsneuron/device_wholecore_test.go +++ b/pkg/device/awsneuron/device_wholecore_test.go @@ -45,7 +45,8 @@ func Test_GenerateResourceRequests_WholeDeviceDefersCoreCountToFit(t *testing.T) }, }, } - req := dev.GenerateResourceRequests(ctr) + req, err := dev.GenerateResourceRequests(ctr) + assert.NilError(t, err) assert.Equal(t, req.Coresreq, int32(0)) } @@ -90,12 +91,13 @@ func Test_Fit_WholeDeviceNotShared(t *testing.T) { }, } podA := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "default", Annotations: map[string]string{}}} - wholeDevice := dev.GenerateResourceRequests(&corev1.Container{ + wholeDevice, err := dev.GenerateResourceRequests(&corev1.Container{ Name: "ctr", Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{"aws.amazon.com/neuron": resource.MustParse("1")}, }, }) + assert.NilError(t, err) fit, tmp, reason := dev.Fit([]*device.DeviceUsage{du}, wholeDevice, podA, &device.NodeInfo{}, &device.PodDevices{}) assert.Equal(t, fit, true, reason) cd := tmp[AWSNeuronDevice][0] @@ -112,11 +114,12 @@ func Test_Fit_FourCoreRequestNeedsEnoughDeviceCapacity(t *testing.T) { ResourceCountName: "aws.amazon.com/neuron", ResourceCoreName: "aws.amazon.com/neuroncore", }) - request := dev.GenerateResourceRequests(&corev1.Container{ + request, err := dev.GenerateResourceRequests(&corev1.Container{ Resources: corev1.ResourceRequirements{Limits: corev1.ResourceList{ "aws.amazon.com/neuroncore": resource.MustParse("4"), }}, }) + assert.NilError(t, err) assert.Equal(t, request.TotalCoresreq, int64(4)) inf1Devices, err := dev.GetNodeDevices(newNeuronNode("inf1", "inf1.6xlarge", 1, 4)) @@ -161,9 +164,10 @@ func Test_Fit_SharedCoresAreDistinctAndReplayable(t *testing.T) { CustomInfo: maps.Clone(registered[0].CustomInfo), } replayed := usage.DeepCopy() - request := dev.GenerateResourceRequests(&corev1.Container{Resources: corev1.ResourceRequirements{ + request, err := dev.GenerateResourceRequests(&corev1.Container{Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{"aws.amazon.com/neuroncore": resource.MustParse("1")}, }}) + assert.NilError(t, err) pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}} for _, wantMask := range []int32{1, 2, 4, 8} { fit, allocations, reason := dev.Fit([]*device.DeviceUsage{usage}, request, pod, &device.NodeInfo{}, &device.PodDevices{}) @@ -193,7 +197,8 @@ func Test_Fit_NeuronCoreRequestRequiresContiguousRange(t *testing.T) { }}, }}}, } - request := dev.GenerateResourceRequests(&pod.Spec.Containers[0]) + request, err := dev.GenerateResourceRequests(&pod.Spec.Containers[0]) + assert.NilError(t, err) for _, test := range []struct { name string @@ -253,9 +258,10 @@ func Test_Fit_MultiDeviceCoreRequestUsesNodeGeometry(t *testing.T) { CustomInfo: maps.Clone(info.CustomInfo), } } - request := dev.GenerateResourceRequests(&corev1.Container{Resources: corev1.ResourceRequirements{ + request, err := dev.GenerateResourceRequests(&corev1.Container{Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{"aws.amazon.com/neuroncore": resource.MustParse(test.requested)}, }}) + assert.NilError(t, err) assert.Equal(t, request.Nums, int32(1)) pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}} fit, allocations, reason := dev.Fit(usages, request, pod, &device.NodeInfo{}, &device.PodDevices{}) diff --git a/pkg/device/biren/device.go b/pkg/device/biren/device.go index 9373232684..1d145b46b5 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] @@ -134,9 +134,14 @@ 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{} + 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 +154,16 @@ func (dev *BirenDevices) GenerateResourceRequests(ctr *corev1.Container) device. Memreq: int32(memnum), MemPercentagereq: int32(mempnum), 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{} + 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..0b3a2bae6c 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) @@ -267,9 +267,14 @@ func (dev *CambriconDevices) 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, "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 +289,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 +302,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 +318,18 @@ func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) dev Memreq: int32(memnum), MemPercentagereq: int32(mempnum), 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, - } + }, 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..1d8edd5912 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,10 +518,12 @@ 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", + name: "zero count is device-less, not invalid", args: corev1.Container{ Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -539,7 +543,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 +556,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 +586,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,10 +600,12 @@ 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", + name: "zero count is device-less, not invalid", args: corev1.Container{ Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -613,7 +625,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 +638,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 +668,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 +682,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) + } }) } } @@ -1937,7 +1960,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 6909cc9c48..215ff1b403 100644 --- a/pkg/device/devices.go +++ b/pkg/device/devices.go @@ -40,7 +40,19 @@ 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. + // + // 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 AddResourceUsage(pod *corev1.Pod, n *DeviceUsage, ctr *ContainerDevice) error @@ -678,7 +690,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) @@ -699,7 +728,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 @@ -717,7 +749,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 @@ -729,7 +764,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 { @@ -780,18 +815,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 8a18663664..852b8c43db 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..257545a184 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] @@ -323,11 +323,20 @@ func (dev *EnflameDevices) GenerateResourceRequests(ctr *corev1.Container) devic 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) - 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 +344,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 +367,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..16e034dc66 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" @@ -88,7 +89,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] @@ -97,6 +98,15 @@ func (dev *GCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co } if ok { if n, ok := v.AsInt64(); ok { + 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)} + } klog.Info("Found enflame devices") return device.ContainerDeviceRequest{ Nums: int32(n), @@ -104,10 +114,16 @@ func (dev *GCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co Memreq: 100, MemPercentagereq: 100, 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{} + 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..fffb62f279 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) }) } @@ -727,3 +727,32 @@ func TestGCUDevices_AddResourceUsage(t *testing.T) { }) } } + +func TestGCUDevices_GenerateResourceRequests_CountRange(t *testing.T) { + dev := GCUDevices{} + for _, count := range []string{"-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") + } + + // 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 8d4684eeb4..058bfb37e8 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) @@ -162,9 +163,14 @@ func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co } 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{} + 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 +183,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 +206,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 +222,16 @@ func (dev *HCUDevices) GenerateResourceRequests(ctr *corev1.Container) device.Co Memreq: int32(memnum), MemPercentagereq: int32(mempnum), 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{} + 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 eb4ffbcfa1..2057704008 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) @@ -195,9 +195,14 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) devi } 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{} + 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 +215,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 } @@ -220,28 +225,31 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) devi core, ok = ctr.Resources.Requests[iluvatarResourceCores] } if ok { - corenums, parsed := core.AsInt64() - if !parsed || corenums < 0 { + corenums, ok := core.AsInt64() + 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{} + 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. MutateAdmission rewrites - // this limit to count*100 when more than one device is - // requested, so a value above 100 is a total across the cards - // and has to be divided back. A value at or below 100 is - // already per card, which is also what an operator writes when - // the admission webhook is disabled or bypassed, so it is left - // alone. - if corenums > 100 { + // 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{} + 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 } + // Re-check after the division: a total that divides evenly can + // still leave a per card value above 100, and a value above 100 + // with a single device never entered the branch above. if corenums > 100 { - klog.ErrorS(nil, "iluvatar core request is out of range (must be 0-100 per device)", "container", ctr.Name, "request", core.String(), "perDevice", corenums) - return device.ContainerDeviceRequest{} + 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) } @@ -257,10 +265,16 @@ func (dev *IluvatarDevices) GenerateResourceRequests(ctr *corev1.Container) devi Memreq: int32(memnum), MemPercentagereq: int32(mempnum), 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{} + 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 3fe0cb4061..11a42edfd8 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) }) } @@ -1032,7 +1032,7 @@ func Test_GenerateResourceRequests_MutatedMultiCard(t *testing.T) { if _, err := dev.MutateAdmission(ctr, &corev1.Pod{}); err != nil { t.Fatalf("MutateAdmission(count=%d): %v", count, err) } - got := dev.GenerateResourceRequests(ctr) + got, _ := dev.GenerateResourceRequests(ctr) if got.Nums != int32(count) { t.Errorf("count=%d: Nums = %d, want %d", count, got.Nums, count) } @@ -1068,7 +1068,7 @@ func Test_GenerateResourceRequests_CoreLimitScales(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := dev.GenerateResourceRequests(iluvatarContainer(test.count, &test.cores)) + got, _ := dev.GenerateResourceRequests(iluvatarContainer(test.count, &test.cores)) if got.Nums != test.wantNums { t.Errorf("Nums = %d, want %d", got.Nums, test.wantNums) } diff --git a/pkg/device/kunlun/device.go b/pkg/device/kunlun/device.go index 847aa9a640..40bc3fd92c 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] @@ -138,9 +138,14 @@ func (dev *KunlunDevices) 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, "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 +155,16 @@ func (dev *KunlunDevices) GenerateResourceRequests(ctr *corev1.Container) device Memreq: 0, MemPercentagereq: 100, 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{} + 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..ab3be9e34c 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" @@ -154,7 +155,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] @@ -164,6 +165,15 @@ func (dev *KunlunVDevices) GenerateResourceRequests(ctr *corev1.Container) devic if ok { klog.V(3).Infof("Counting %s devices", dev.CommonWord()) if n, ok := v.AsInt64(); ok { + 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)} + } memnum := 0 mem, ok := ctr.Resources.Limits[xpuResourceMem] if !ok { @@ -189,10 +199,16 @@ func (dev *KunlunVDevices) GenerateResourceRequests(ctr *corev1.Container) devic Memreq: int32(memnum), //int32(dev.config.MemoryMax), MemPercentagereq: int32(mempnum), 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{} + 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..0362233083 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) }) } @@ -567,3 +567,32 @@ func Test_FitVXPU_direct(t *testing.T) { }) } } + +func Test_KunlunVDevices_GenerateResourceRequests_CountRange(t *testing.T) { + dev := InitKunlunVDevice(testVConfig()) + for _, count := range []string{"-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") + } + + // 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 9e0bf31622..e0490896b9 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] @@ -141,9 +141,14 @@ 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{} + 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 +157,16 @@ func (dev *MetaxDevices) GenerateResourceRequests(ctr *corev1.Container) device. Memreq: 0, MemPercentagereq: 100, 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{} + 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..8c04ac4ff0 100644 --- a/pkg/device/metax/sdevice.go +++ b/pkg/device/metax/sdevice.go @@ -200,21 +200,26 @@ 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 { + 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{} + 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 +231,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 +250,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 +274,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 b259e4f41f..83dbfa9515 100644 --- a/pkg/device/mthreads/device.go +++ b/pkg/device/mthreads/device.go @@ -341,7 +341,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) @@ -355,8 +355,13 @@ func (dev *MthreadsDevices) GenerateResourceRequests(ctr *corev1.Container) devi klog.InfoS("Detected mthreads device request", "container", ctr.Name, "deviceCount", n) - if n <= 0 || n > math.MaxInt32 { - return device.ContainerDeviceRequest{} + 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 mem, ok := ctr.Resources.Limits[mthreadsResourceMem] @@ -368,7 +373,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", @@ -383,9 +388,33 @@ 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 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) + 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 + } + 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) } @@ -400,11 +429,17 @@ 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 + } + // 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{} + 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 ce56c73542..7dd13fcfd7 100644 --- a/pkg/device/mthreads/device_test.go +++ b/pkg/device/mthreads/device_test.go @@ -18,6 +18,7 @@ package mthreads import ( "flag" + "fmt" "math" "strings" "testing" @@ -788,7 +789,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) }) } @@ -1526,6 +1527,136 @@ func TestFit_CoresValidation(t *testing.T) { }) } +// 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") +} + +// 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)) + } +} + +// 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)) +} + func TestBuildLegalMemorySlices(t *testing.T) { tests := []struct { name string diff --git a/pkg/device/nvidia/device.go b/pkg/device/nvidia/device.go index 6adccf6b64..a5d4f18909 100644 --- a/pkg/device/nvidia/device.go +++ b/pkg/device/nvidia/device.go @@ -599,7 +599,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) @@ -610,9 +610,16 @@ func (dev *NvidiaGPUDevices) 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 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{} + 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] @@ -625,7 +632,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 @@ -671,7 +678,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) } @@ -681,10 +688,16 @@ func (dev *NvidiaGPUDevices) GenerateResourceRequests(ctr *corev1.Container) dev Memreq: int32(memnum), MemPercentagereq: mempnum, 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{} + return device.ContainerDeviceRequest{}, nil } func (dev *NvidiaGPUDevices) CustomFilterRule(allocated *device.PodDevices, request device.ContainerDeviceRequest, toAllocate device.ContainerDevices, devusage *device.DeviceUsage, preferred []string) bool { diff --git a/pkg/device/nvidia/device_test.go b/pkg/device/nvidia/device_test.go index b929ec3f82..9eda52e516 100644 --- a/pkg/device/nvidia/device_test.go +++ b/pkg/device/nvidia/device_test.go @@ -2269,7 +2269,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) }) } @@ -2292,11 +2292,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{}) } @@ -2317,19 +2317,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) } @@ -2363,7 +2363,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) @@ -3564,7 +3564,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) @@ -3774,6 +3774,83 @@ 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") +} + +// 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") + + // 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") +} + func TestNormalizeDeviceModel(t *testing.T) { tests := []struct { name string 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/remotegpu/device.go b/pkg/device/remotegpu/device.go index 5afc83e4bd..563a8c1443 100644 --- a/pkg/device/remotegpu/device.go +++ b/pkg/device/remotegpu/device.go @@ -316,22 +316,24 @@ func mountLib(ctr *corev1.Container) { }) } -func (dev *RemoteGPUDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { +func (dev *RemoteGPUDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { count, ok := resourceValue(ctr, RemoteGPUResourceCount) if !ok || count <= 0 { - return device.ContainerDeviceRequest{} + // No device requested (or an explicit zero) is device-less, not + // invalid. See the nvidia backend. + return device.ContainerDeviceRequest{}, nil } nums, err := safecast.Convert[int32](count) if err != nil { klog.ErrorS(err, "remotegpu: device count out of range", "value", count) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "remotegpu", Reason: fmt.Sprintf("device count %d is out of range", count)} } var memreq int32 if mem, ok := resourceValue(ctr, RemoteGPUResourceMemory); ok && mem > 0 { memreq, err = safecast.Convert[int32](mem) if err != nil { klog.ErrorS(err, "remotegpu: memory request out of range", "value", mem) - return device.ContainerDeviceRequest{} + return device.ContainerDeviceRequest{}, &device.ErrInvalidDeviceRequest{Container: ctr.Name, Device: "remotegpu", Reason: fmt.Sprintf("memory request %d is out of range", mem)} } } return device.ContainerDeviceRequest{ @@ -343,7 +345,7 @@ func (dev *RemoteGPUDevices) GenerateResourceRequests(ctr *corev1.Container) dev // the server's GPUs. Memreq: memreq, Coresreq: 100, - } + }, nil } func (dev *RemoteGPUDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd device.PodDevices) map[string]string { diff --git a/pkg/device/remotegpu/device_test.go b/pkg/device/remotegpu/device_test.go index 693f869225..b35a3cedc8 100644 --- a/pkg/device/remotegpu/device_test.go +++ b/pkg/device/remotegpu/device_test.go @@ -351,13 +351,16 @@ func TestGenerateResourceRequests(t *testing.T) { }, } - req := dev.GenerateResourceRequests(ctr) + req, err := dev.GenerateResourceRequests(ctr) + assert.NilError(t, err) assert.Equal(t, req.Nums, int32(2)) assert.Equal(t, req.Memreq, int32(2000)) assert.Equal(t, req.Coresreq, int32(100)) assert.Equal(t, req.Type, RemoteGPUCommonWord) - assert.Equal(t, dev.GenerateResourceRequests(&corev1.Container{}).Nums, int32(0)) + empty, err := dev.GenerateResourceRequests(&corev1.Container{}) + assert.NilError(t, err) + assert.Equal(t, empty.Nums, int32(0)) } func TestPatchAnnotations_WritesResolvedEndpoint(t *testing.T) { diff --git a/pkg/device/vastai/device.go b/pkg/device/vastai/device.go index 8fbe24a87f..fdb3be294c 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] @@ -136,9 +136,14 @@ func (dev *VastaiDevices) 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, "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 +156,16 @@ func (dev *VastaiDevices) GenerateResourceRequests(ctr *corev1.Container) device Memreq: int32(memnum), MemPercentagereq: int32(mempnum), 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{} + 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 9be76b0ab3..d859946788 100644 --- a/pkg/scheduler/config/config_test.go +++ b/pkg/scheduler/config/config_test.go @@ -646,8 +646,8 @@ func (s stubDevices) GetNodeDevices(n corev1.Node) ([]*device.DeviceInfo, error) } func (s stubDevices) LockNode(n *corev1.Node, p *corev1.Pod) error { return nil } func (s stubDevices) ReleaseNodeLock(n *corev1.Node, p *corev1.Pod) error { return nil } -func (s stubDevices) GenerateResourceRequests(ctr *corev1.Container) device.ContainerDeviceRequest { - return device.ContainerDeviceRequest{} +func (s stubDevices) GenerateResourceRequests(ctr *corev1.Container) (device.ContainerDeviceRequest, error) { + return device.ContainerDeviceRequest{}, nil } func (s stubDevices) PatchAnnotations(pod *corev1.Pod, annoinput *map[string]string, pd device.PodDevices) map[string]string { @@ -1005,8 +1005,52 @@ 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) }) } } + +// 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) + }) + } +} diff --git a/pkg/scheduler/remotegpu_integration_test.go b/pkg/scheduler/remotegpu_integration_test.go index 9f8a015528..d18f45f46d 100644 --- a/pkg/scheduler/remotegpu_integration_test.go +++ b/pkg/scheduler/remotegpu_integration_test.go @@ -155,7 +155,8 @@ func TestRemoteGPU_GPUlessNodeFitsAndLupineNodeDoesNot(t *testing.T) { // Both GPU-less nodes see the same fleet. assert.Equal(t, len(nodes["cpu-1"].Devices.DeviceLists), 2) - reqs := device.Resourcereqs(task) + reqs, err := device.Resourcereqs(task) + assert.NilError(t, err) assert.Equal(t, reqs[0]["RemoteGPU"].Nums, int32(2), "GenerateResourceRequests must reach Resourcereqs") s := NewScheduler() @@ -192,7 +193,9 @@ func TestRemoteGPU_AllocationNeverSpansTwoServers(t *testing.T) { s := NewScheduler() failed := map[string]string{} - scores, err := s.calcScore(&nodes, device.Resourcereqs(task), task, failed) + reqs, err := device.Resourcereqs(task) + assert.NilError(t, err) + scores, err := s.calcScore(&nodes, reqs, task, failed) assert.NilError(t, err) assert.Equal(t, len(scores.NodeList), 0, "two cards on two servers must not be combined") assert.Assert(t, failed["cpu-1"] != "") @@ -228,7 +231,9 @@ func TestRemoteGPU_ReservationBlocksSecondClientNode(t *testing.T) { s := NewScheduler() failed := map[string]string{} - scores, err := s.calcScore(&nodes, device.Resourcereqs(task), task, failed) + reqs, err := device.Resourcereqs(task) + assert.NilError(t, err) + scores, err := s.calcScore(&nodes, reqs, task, failed) assert.NilError(t, err) assert.Equal(t, len(scores.NodeList), 0, "the only card is already held by another pod") assert.Assert(t, failed["cpu-2"] != "", "the holder landed on cpu-1, yet cpu-2 must also be refused") @@ -255,7 +260,9 @@ func TestRemoteGPU_FinishedPodReleasesCard(t *testing.T) { nodes := map[string]*NodeUsage{"cpu-1": nodeUsageFor(t, dev, gpulessNode("cpu-1"), task)} s := NewScheduler() - scores, err := s.calcScore(&nodes, device.Resourcereqs(task), task, map[string]string{}) + reqs, err := device.Resourcereqs(task) + assert.NilError(t, err) + scores, err := s.calcScore(&nodes, reqs, task, map[string]string{}) assert.NilError(t, err) assert.Equal(t, len(scores.NodeList), 1) } @@ -306,7 +313,9 @@ func TestRemoteGPU_MemoryRequestFiltersCards(t *testing.T) { s := NewScheduler() failed := map[string]string{} - scores, err := s.calcScore(&nodes, device.Resourcereqs(task), task, failed) + reqs, err := device.Resourcereqs(task) + assert.NilError(t, err) + scores, err := s.calcScore(&nodes, reqs, task, failed) assert.NilError(t, err) assert.Equal(t, len(scores.NodeList), 0) assert.Assert(t, failed["cpu-1"] != "") diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 3ebda8f1d4..08b1a49c8f 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -1294,7 +1294,6 @@ func matchesClaimedPod(claimed, live *corev1.Pod) (*corev1.Pod, error) { // Filter selects a node and reserves devices for a scheduler extender request. 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) - // Simulation callers describe pods that need not exist, and the path they // take reserves nothing, so only the scheduling path resolves the pod. pod := args.Pod @@ -1307,7 +1306,20 @@ func (s *Scheduler) Filter(args extenderv1.ExtenderArgs) (*extenderv1.ExtenderFi pod = live } - resourceReqs := device.Resourcereqs(pod) + resourceReqs, reqErr := device.Resourcereqs(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", pod.Name, reqErr) + klog.ErrorS(nil, "Rejecting pod with an invalid device request", "pod", klog.KObj(pod), "error", reqErr) + s.recordScheduleFilterResultEvent(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 cbeaf9f029..e4727001c1 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -1412,8 +1412,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 @@ -3553,6 +3553,42 @@ 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() + s.kubeClient = client.KubeClient + s.podLister = informers.NewSharedInformerFactoryWithOptions(client.KubeClient, time.Hour).Core().V1().Pods().Lister() + 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), + }}, + }}}, + } + _, err := client.KubeClient.CoreV1().Pods(pod.Namespace).Create(context.Background(), pod, metav1.CreateOptions{}) + require.NoError(t, err) + 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) +} + // Test_register_PrintedLogPrunedOnNodeDelete covers the printedLog bookkeeping // across a node's full lifecycle. The map used to be a loop-local in // RegisterFromNodeAnnotations, so onDelNode could not reach it: entries diff --git a/pkg/scheduler/score_test.go b/pkg/scheduler/score_test.go index ad8b1ba29c..8b78459e5d 100644 --- a/pkg/scheduler/score_test.go +++ b/pkg/scheduler/score_test.go @@ -81,7 +81,10 @@ func TestAWSNeuronWholeDeviceNodeQuota(t *testing.T) { }}, }}}, } - requests := device.Resourcereqs(pod) + requests, err := device.Resourcereqs(pod) + if err != nil { + t.Fatal(err) + } for _, tc := range []struct { name string mask int32 @@ -99,7 +102,7 @@ func TestAWSNeuronWholeDeviceNodeQuota(t *testing.T) { quota.Quotas[namespace] = &device.DeviceQuota{ "aws.amazon.com/neuroncore": &device.Quota{Used: tc.used, Limit: tc.limit, LimitSet: true}, } - if !fitResourceQuota(pod) { + if err := fitResourceQuota(pod); err != nil { t.Fatal("admission should defer whole-device core accounting until node placement") } node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "neuron-node"}} @@ -4505,8 +4508,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 a619251f7c..f40689b291 100644 --- a/pkg/scheduler/webhook.go +++ b/pkg/scheduler/webhook.go @@ -144,8 +144,8 @@ func (h *webhook) Handle(ctx context.Context, req admission.Request) admission.R 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 { @@ -282,7 +282,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 { @@ -295,7 +295,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 } @@ -311,7 +314,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 } @@ -340,8 +346,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 baf32c86c0..8ffa145192 100644 --- a/pkg/scheduler/webhook_test.go +++ b/pkg/scheduler/webhook_test.go @@ -512,9 +512,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) } }) } @@ -648,8 +648,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) } }) } @@ -675,11 +675,11 @@ func TestFitResourceQuotaAWSNeuronCoreRequest(t *testing.T) { }}, }}}, } - if fitResourceQuota(pod) { + if err := fitResourceQuota(pod); err == nil { t.Fatal("six requested NeuronCores must exceed a five-core quota") } pod.Spec.Containers[0].Resources.Limits["aws.amazon.com/neuroncore"] = resource.MustParse("4") - if !fitResourceQuota(pod) { + if err := fitResourceQuota(pod); err != nil { t.Fatal("four requested NeuronCores should fit a five-core quota") } quota.AddUsage(pod, device.PodDevices{awsneuron.AWSNeuronDevice: {{ @@ -724,8 +724,48 @@ 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) + } +} + +// 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) } } @@ -801,12 +841,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") } } @@ -1095,8 +1135,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) @@ -1110,7 +1150,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)") } @@ -1122,8 +1162,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) } } @@ -1249,8 +1289,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) } }) }