From e2d1fcc1628557c6fcf37f9714eb3d26ebab3d13 Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Fri, 28 Feb 2025 20:47:35 +0000 Subject: [PATCH] Addressed comments --- pkg/apis/resource/types.go | 34 ++++++++++++++++--- pkg/apis/resource/v1alpha3/defaults.go | 5 ++- pkg/apis/resource/v1alpha3/defaults_test.go | 6 +++- pkg/apis/resource/v1beta1/defaults.go | 5 ++- pkg/apis/resource/v1beta1/defaults_test.go | 6 +++- pkg/apis/resource/validation/validation.go | 18 +++++----- .../validation_resourceclaim_test.go | 10 ++---- .../validation_resourceclaimtemplate_test.go | 2 +- .../src/k8s.io/api/resource/v1alpha3/types.go | 20 +++++++++-- .../src/k8s.io/api/resource/v1beta1/types.go | 20 +++++++++-- 10 files changed, 96 insertions(+), 30 deletions(-) diff --git a/pkg/apis/resource/types.go b/pkg/apis/resource/types.go index 7de57ef6b83..c2cb135a36d 100644 --- a/pkg/apis/resource/types.go +++ b/pkg/apis/resource/types.go @@ -403,7 +403,9 @@ type DeviceRequest struct { // request. // // A class is required if no subrequests are specified in the - // firstAvailable list. Which classes are available depends on the cluster. + // firstAvailable list and no class can be set if subrequests + // are specified in the firstAvailable list. + // Which classes are available depends on the cluster. // // Administrators may use this to restrict which devices may get // requested by only installing classes with selectors for permitted @@ -420,6 +422,9 @@ type DeviceRequest struct { // request. All selectors must be satisfied for a device to be // considered. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // +optional // +listType=atomic Selectors []DeviceSelector @@ -440,6 +445,9 @@ type DeviceRequest struct { // the mode is ExactCount and count is not specified, the default count is // one. Any other requests must specify this field. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // More modes may get added in the future. Clients must refuse to handle // requests with unknown modes. // @@ -449,6 +457,9 @@ type DeviceRequest struct { // Count is used only when the count mode is "ExactCount". Must be greater than zero. // If AllocationMode is ExactCount and this field is not specified, the default is one. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // +optional // +oneOf=AllocationMode Count int64 @@ -459,6 +470,9 @@ type DeviceRequest struct { // all ordinary claims to the device with respect to access modes and // any resource allocations. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // This is an alpha field and requires enabling the DRAAdminAccess // feature gate. Admin access is disabled if this field is unset or // set to false, otherwise it is enabled. @@ -467,9 +481,21 @@ type DeviceRequest struct { // +featureGate=DRAAdminAccess AdminAccess *bool - // FirstAvailable contains subrequests, exactly one of which must be satisfied - // in order to satisfy this request. This field may only be set in the - // entries of DeviceClaim.Requests. + // FirstAvailable contains subrequests, of which exactly one will be + // satisfied by the scheduler to satisfy this request. It tries to + // satisfy them in the order in which they are listed here. So if + // there are two entries in the list, the scheduler will only check + // the second one if it determines that the first one cannot be used. + // + // This field may only be set in the entries of DeviceClaim.Requests. + // + // DRA does not yet implement scoring, so the scheduler will + // select the first set of devices that satisfies all the + // requests in the claim. And if the requirements can + // be satisfied on more than one node, other scheduling features + // will determine which node is chosen. This means that the set of + // devices allocated to a claim might not be the optimal set + // available to the cluster. Scoring will be implemented later. // // +optional // +oneOf=deviceRequestType diff --git a/pkg/apis/resource/v1alpha3/defaults.go b/pkg/apis/resource/v1alpha3/defaults.go index 80dfa4920ca..2d93bc28354 100644 --- a/pkg/apis/resource/v1alpha3/defaults.go +++ b/pkg/apis/resource/v1alpha3/defaults.go @@ -26,7 +26,10 @@ func addDefaultingFuncs(scheme *runtime.Scheme) error { } func SetDefaults_DeviceRequest(obj *resourceapi.DeviceRequest) { - if len(obj.FirstAvailable) > 0 { + // If the deviceClassName is not set, then the request will have + // subrequests and the allocationMode and count fields should not + // be set. + if obj.DeviceClassName == "" { return } if obj.AllocationMode == "" { diff --git a/pkg/apis/resource/v1alpha3/defaults_test.go b/pkg/apis/resource/v1alpha3/defaults_test.go index 10f5a385b90..4d19747a2b0 100644 --- a/pkg/apis/resource/v1alpha3/defaults_test.go +++ b/pkg/apis/resource/v1alpha3/defaults_test.go @@ -34,7 +34,11 @@ func TestSetDefaultAllocationMode(t *testing.T) { claim := &v1alpha3.ResourceClaim{ Spec: v1alpha3.ResourceClaimSpec{ Devices: v1alpha3.DeviceClaim{ - Requests: []v1alpha3.DeviceRequest{{}}, + Requests: []v1alpha3.DeviceRequest{ + { + DeviceClassName: "device-class", + }, + }, }, }, } diff --git a/pkg/apis/resource/v1beta1/defaults.go b/pkg/apis/resource/v1beta1/defaults.go index c7d7b2c24a5..6ce9b1de26a 100644 --- a/pkg/apis/resource/v1beta1/defaults.go +++ b/pkg/apis/resource/v1beta1/defaults.go @@ -26,7 +26,10 @@ func addDefaultingFuncs(scheme *runtime.Scheme) error { } func SetDefaults_DeviceRequest(obj *resourceapi.DeviceRequest) { - if len(obj.FirstAvailable) > 0 { + // If the deviceClassName is not set, then the request will have + // subrequests and the allocationMode and count fields should not + // be set. + if obj.DeviceClassName == "" { return } if obj.AllocationMode == "" { diff --git a/pkg/apis/resource/v1beta1/defaults_test.go b/pkg/apis/resource/v1beta1/defaults_test.go index e9e5913e72f..9b00b0732a1 100644 --- a/pkg/apis/resource/v1beta1/defaults_test.go +++ b/pkg/apis/resource/v1beta1/defaults_test.go @@ -34,7 +34,11 @@ func TestSetDefaultAllocationMode(t *testing.T) { claim := &v1beta1.ResourceClaim{ Spec: v1beta1.ResourceClaimSpec{ Devices: v1beta1.DeviceClaim{ - Requests: []v1beta1.DeviceRequest{{}}, + Requests: []v1beta1.DeviceRequest{ + { + DeviceClassName: "device-class", + }, + }, }, }, } diff --git a/pkg/apis/resource/validation/validation.go b/pkg/apis/resource/validation/validation.go index 361b2856f79..248ddcfcfc9 100644 --- a/pkg/apis/resource/validation/validation.go +++ b/pkg/apis/resource/validation/validation.go @@ -169,10 +169,16 @@ func gatherAllocatedDevices(allocationResult *resource.DeviceAllocationResult) s func validateDeviceRequest(request resource.DeviceRequest, fldPath *field.Path, stored bool) field.ErrorList { allErrs := validateRequestName(request.Name, fldPath.Child("name")) - if len(request.FirstAvailable) > 0 { - if request.DeviceClassName != "" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("deviceClassName"), request.DeviceClassName, "must not be specified when firstAvailable is set")) - } + + if request.DeviceClassName == "" && len(request.FirstAvailable) == 0 { + allErrs = append(allErrs, field.Required(fldPath, "exactly one of `deviceClassName` or `firstAvailable` must be specified")) + } else if request.DeviceClassName != "" && len(request.FirstAvailable) > 0 { + allErrs = append(allErrs, field.Invalid(fldPath, nil, "exactly one of `deviceClassName` or `firstAvailable` must be specified")) + } else if request.DeviceClassName != "" { + allErrs = append(allErrs, validateDeviceClass(request.DeviceClassName, fldPath.Child("deviceClassName"))...) + allErrs = append(allErrs, validateSelectorSlice(request.Selectors, fldPath.Child("selectors"), stored)...) + allErrs = append(allErrs, validateDeviceAllocationMode(request.AllocationMode, request.Count, fldPath.Child("allocationMode"), fldPath.Child("count"))...) + } else if len(request.FirstAvailable) > 0 { if request.Selectors != nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("selectors"), request.Selectors, "must not be specified when firstAvailable is set")) } @@ -193,11 +199,7 @@ func validateDeviceRequest(request resource.DeviceRequest, fldPath *field.Path, return subRequest.Name, "name" }, fldPath.Child("firstAvailable"))...) - return allErrs } - allErrs = append(allErrs, validateDeviceClass(request.DeviceClassName, fldPath.Child("deviceClassName"))...) - allErrs = append(allErrs, validateSelectorSlice(request.Selectors, fldPath.Child("selectors"), stored)...) - allErrs = append(allErrs, validateDeviceAllocationMode(request.AllocationMode, request.Count, fldPath.Child("allocationMode"), fldPath.Child("count"))...) return allErrs } diff --git a/pkg/apis/resource/validation/validation_resourceclaim_test.go b/pkg/apis/resource/validation/validation_resourceclaim_test.go index 3308024ecd1..1a243920e4f 100644 --- a/pkg/apis/resource/validation/validation_resourceclaim_test.go +++ b/pkg/apis/resource/validation/validation_resourceclaim_test.go @@ -245,8 +245,8 @@ func TestValidateClaim(t *testing.T) { return claim }(), }, - "missing-classname": { - wantFailures: field.ErrorList{field.Required(field.NewPath("spec", "devices", "requests").Index(0).Child("deviceClassName"), "")}, + "missing-classname-and-firstavailable": { + wantFailures: field.ErrorList{field.Required(field.NewPath("spec", "devices", "requests").Index(0), "exactly one of `deviceClassName` or `firstAvailable` must be specified")}, claim: func() *resource.ResourceClaim { claim := testClaim(goodName, goodNS, validClaimSpec) claim.Spec.Devices.Requests[0].DeviceClassName = "" @@ -578,11 +578,7 @@ func TestValidateClaim(t *testing.T) { }, "prioritized-list-field-on-parent": { wantFailures: field.ErrorList{ - field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("deviceClassName"), goodName, "must not be specified when firstAvailable is set"), - field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("selectors"), validSelector, "must not be specified when firstAvailable is set"), - field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("allocationMode"), resource.DeviceAllocationModeAll, "must not be specified when firstAvailable is set"), - field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("count"), int64(2), "must not be specified when firstAvailable is set"), - field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("adminAccess"), ptr.To(true), "must not be specified when firstAvailable is set"), + field.Invalid(field.NewPath("spec", "devices", "requests").Index(0), nil, "exactly one of `deviceClassName` or `firstAvailable` must be specified"), }, claim: func() *resource.ResourceClaim { claim := testClaim(goodName, goodNS, validClaimSpecWithFirstAvailable) diff --git a/pkg/apis/resource/validation/validation_resourceclaimtemplate_test.go b/pkg/apis/resource/validation/validation_resourceclaimtemplate_test.go index 586c804c702..2e24d766bbc 100644 --- a/pkg/apis/resource/validation/validation_resourceclaimtemplate_test.go +++ b/pkg/apis/resource/validation/validation_resourceclaimtemplate_test.go @@ -190,7 +190,7 @@ func TestValidateClaimTemplate(t *testing.T) { template: testClaimTemplate(goodName, goodNS, validClaimSpecWithFirstAvailable), }, "proritized-list-class-name-on-parent": { - wantFailures: field.ErrorList{field.Invalid(field.NewPath("spec", "spec", "devices", "requests").Index(0).Child("deviceClassName"), goodName, "must not be specified when firstAvailable is set")}, + wantFailures: field.ErrorList{field.Invalid(field.NewPath("spec", "spec", "devices", "requests").Index(0), nil, "exactly one of `deviceClassName` or `firstAvailable` must be specified")}, template: func() *resource.ResourceClaimTemplate { template := testClaimTemplate(goodName, goodNS, validClaimSpecWithFirstAvailable) template.Spec.Spec.Devices.Requests[0].DeviceClassName = goodName diff --git a/staging/src/k8s.io/api/resource/v1alpha3/types.go b/staging/src/k8s.io/api/resource/v1alpha3/types.go index 354b3658545..b6c6c31840e 100644 --- a/staging/src/k8s.io/api/resource/v1alpha3/types.go +++ b/staging/src/k8s.io/api/resource/v1alpha3/types.go @@ -400,7 +400,9 @@ type DeviceRequest struct { // request. // // A class is required if no subrequests are specified in the - // firstAvailable list. Which classes are available depends on the cluster. + // firstAvailable list and no class can be set if subrequests + // are specified in the firstAvailable list. + // Which classes are available depends on the cluster. // // Administrators may use this to restrict which devices may get // requested by only installing classes with selectors for permitted @@ -417,6 +419,9 @@ type DeviceRequest struct { // request. All selectors must be satisfied for a device to be // considered. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // +optional // +listType=atomic Selectors []DeviceSelector `json:"selectors,omitempty" protobuf:"bytes,3,name=selectors"` @@ -437,6 +442,9 @@ type DeviceRequest struct { // the mode is ExactCount and count is not specified, the default count is // one. Any other requests must specify this field. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // More modes may get added in the future. Clients must refuse to handle // requests with unknown modes. // @@ -446,6 +454,9 @@ type DeviceRequest struct { // Count is used only when the count mode is "ExactCount". Must be greater than zero. // If AllocationMode is ExactCount and this field is not specified, the default is one. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // +optional // +oneOf=AllocationMode Count int64 `json:"count,omitempty" protobuf:"bytes,5,opt,name=count"` @@ -456,6 +467,9 @@ type DeviceRequest struct { // all ordinary claims to the device with respect to access modes and // any resource allocations. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // This is an alpha field and requires enabling the DRAAdminAccess // feature gate. Admin access is disabled if this field is unset or // set to false, otherwise it is enabled. @@ -467,8 +481,8 @@ type DeviceRequest struct { // FirstAvailable contains subrequests, of which exactly one will be // satisfied by the scheduler to satisfy this request. It tries to // satisfy them in the order in which they are listed here. So if - // there are two entries in the list, the schduler will only check - // the second one if it determines that the first one can not be used. + // there are two entries in the list, the scheduler will only check + // the second one if it determines that the first one cannot be used. // // This field may only be set in the entries of DeviceClaim.Requests. // diff --git a/staging/src/k8s.io/api/resource/v1beta1/types.go b/staging/src/k8s.io/api/resource/v1beta1/types.go index 2f2b97521b6..cc1f02161f5 100644 --- a/staging/src/k8s.io/api/resource/v1beta1/types.go +++ b/staging/src/k8s.io/api/resource/v1beta1/types.go @@ -409,7 +409,9 @@ type DeviceRequest struct { // request. // // A class is required if no subrequests are specified in the - // firstAvailable list. Which classes are available depends on the cluster. + // firstAvailable list and no class can be set if subrequests + // are specified in the firstAvailable list. + // Which classes are available depends on the cluster. // // Administrators may use this to restrict which devices may get // requested by only installing classes with selectors for permitted @@ -426,6 +428,9 @@ type DeviceRequest struct { // request. All selectors must be satisfied for a device to be // considered. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // +optional // +listType=atomic Selectors []DeviceSelector `json:"selectors,omitempty" protobuf:"bytes,3,name=selectors"` @@ -446,6 +451,9 @@ type DeviceRequest struct { // the mode is ExactCount and count is not specified, the default count is // one. Any other requests must specify this field. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // More modes may get added in the future. Clients must refuse to handle // requests with unknown modes. // @@ -455,6 +463,9 @@ type DeviceRequest struct { // Count is used only when the count mode is "ExactCount". Must be greater than zero. // If AllocationMode is ExactCount and this field is not specified, the default is one. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // +optional // +oneOf=AllocationMode Count int64 `json:"count,omitempty" protobuf:"bytes,5,opt,name=count"` @@ -465,6 +476,9 @@ type DeviceRequest struct { // all ordinary claims to the device with respect to access modes and // any resource allocations. // + // This field can only be set when deviceClassName is set and no subrequests + // are specified in the firstAvailable list. + // // This is an alpha field and requires enabling the DRAAdminAccess // feature gate. Admin access is disabled if this field is unset or // set to false, otherwise it is enabled. @@ -476,8 +490,8 @@ type DeviceRequest struct { // FirstAvailable contains subrequests, of which exactly one will be // satisfied by the scheduler to satisfy this request. It tries to // satisfy them in the order in which they are listed here. So if - // there are two entries in the list, the schduler will only check - // the second one if it determines that the first one can not be used. + // there are two entries in the list, the scheduler will only check + // the second one if it determines that the first one cannot be used. // // This field may only be set in the entries of DeviceClaim.Requests. //