Skip to content

Commit e2d1fcc

Browse files
mortentmortent
committed
Addressed comments
1 parent 7555cbc commit e2d1fcc

File tree

10 files changed

+96
-30
lines changed

10 files changed

+96
-30
lines changed

‎pkg/apis/resource/types.go‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,9 @@ type DeviceRequest struct {
403403
// request.
404404
//
405405
// A class is required if no subrequests are specified in the
406-
// firstAvailable list. Which classes are available depends on the cluster.
406+
// firstAvailable list and no class can be set if subrequests
407+
// are specified in the firstAvailable list.
408+
// Which classes are available depends on the cluster.
407409
//
408410
// Administrators may use this to restrict which devices may get
409411
// requested by only installing classes with selectors for permitted
@@ -420,6 +422,9 @@ type DeviceRequest struct {
420422
// request. All selectors must be satisfied for a device to be
421423
// considered.
422424
//
425+
// This field can only be set when deviceClassName is set and no subrequests
426+
// are specified in the firstAvailable list.
427+
//
423428
// +optional
424429
// +listType=atomic
425430
Selectors []DeviceSelector
@@ -440,6 +445,9 @@ type DeviceRequest struct {
440445
// the mode is ExactCount and count is not specified, the default count is
441446
// one. Any other requests must specify this field.
442447
//
448+
// This field can only be set when deviceClassName is set and no subrequests
449+
// are specified in the firstAvailable list.
450+
//
443451
// More modes may get added in the future. Clients must refuse to handle
444452
// requests with unknown modes.
445453
//
@@ -449,6 +457,9 @@ type DeviceRequest struct {
449457
// Count is used only when the count mode is "ExactCount". Must be greater than zero.
450458
// If AllocationMode is ExactCount and this field is not specified, the default is one.
451459
//
460+
// This field can only be set when deviceClassName is set and no subrequests
461+
// are specified in the firstAvailable list.
462+
//
452463
// +optional
453464
// +oneOf=AllocationMode
454465
Count int64
@@ -459,6 +470,9 @@ type DeviceRequest struct {
459470
// all ordinary claims to the device with respect to access modes and
460471
// any resource allocations.
461472
//
473+
// This field can only be set when deviceClassName is set and no subrequests
474+
// are specified in the firstAvailable list.
475+
//
462476
// This is an alpha field and requires enabling the DRAAdminAccess
463477
// feature gate. Admin access is disabled if this field is unset or
464478
// set to false, otherwise it is enabled.
@@ -467,9 +481,21 @@ type DeviceRequest struct {
467481
// +featureGate=DRAAdminAccess
468482
AdminAccess *bool
469483

470-
// FirstAvailable contains subrequests, exactly one of which must be satisfied
471-
// in order to satisfy this request. This field may only be set in the
472-
// entries of DeviceClaim.Requests.
484+
// FirstAvailable contains subrequests, of which exactly one will be
485+
// satisfied by the scheduler to satisfy this request. It tries to
486+
// satisfy them in the order in which they are listed here. So if
487+
// there are two entries in the list, the scheduler will only check
488+
// the second one if it determines that the first one cannot be used.
489+
//
490+
// This field may only be set in the entries of DeviceClaim.Requests.
491+
//
492+
// DRA does not yet implement scoring, so the scheduler will
493+
// select the first set of devices that satisfies all the
494+
// requests in the claim. And if the requirements can
495+
// be satisfied on more than one node, other scheduling features
496+
// will determine which node is chosen. This means that the set of
497+
// devices allocated to a claim might not be the optimal set
498+
// available to the cluster. Scoring will be implemented later.
473499
//
474500
// +optional
475501
// +oneOf=deviceRequestType

‎pkg/apis/resource/v1alpha3/defaults.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,10 @@ func addDefaultingFuncs(scheme *runtime.Scheme) error {
2626
}
2727

2828
func SetDefaults_DeviceRequest(obj *resourceapi.DeviceRequest) {
29-
if len(obj.FirstAvailable) > 0 {
29+
// If the deviceClassName is not set, then the request will have
30+
// subrequests and the allocationMode and count fields should not
31+
// be set.
32+
if obj.DeviceClassName == "" {
3033
return
3134
}
3235
if obj.AllocationMode == "" {

‎pkg/apis/resource/v1alpha3/defaults_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,11 @@ func TestSetDefaultAllocationMode(t *testing.T) {
3434
claim := &v1alpha3.ResourceClaim{
3535
Spec: v1alpha3.ResourceClaimSpec{
3636
Devices: v1alpha3.DeviceClaim{
37-
Requests: []v1alpha3.DeviceRequest{{}},
37+
Requests: []v1alpha3.DeviceRequest{
38+
{
39+
DeviceClassName: "device-class",
40+
},
41+
},
3842
},
3943
},
4044
}

‎pkg/apis/resource/v1beta1/defaults.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,10 @@ func addDefaultingFuncs(scheme *runtime.Scheme) error {
2626
}
2727

2828
func SetDefaults_DeviceRequest(obj *resourceapi.DeviceRequest) {
29-
if len(obj.FirstAvailable) > 0 {
29+
// If the deviceClassName is not set, then the request will have
30+
// subrequests and the allocationMode and count fields should not
31+
// be set.
32+
if obj.DeviceClassName == "" {
3033
return
3134
}
3235
if obj.AllocationMode == "" {

‎pkg/apis/resource/v1beta1/defaults_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,11 @@ func TestSetDefaultAllocationMode(t *testing.T) {
3434
claim := &v1beta1.ResourceClaim{
3535
Spec: v1beta1.ResourceClaimSpec{
3636
Devices: v1beta1.DeviceClaim{
37-
Requests: []v1beta1.DeviceRequest{{}},
37+
Requests: []v1beta1.DeviceRequest{
38+
{
39+
DeviceClassName: "device-class",
40+
},
41+
},
3842
},
3943
},
4044
}

‎pkg/apis/resource/validation/validation.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -169,10 +169,16 @@ func gatherAllocatedDevices(allocationResult *resource.DeviceAllocationResult) s
169169

170170
func validateDeviceRequest(request resource.DeviceRequest, fldPath *field.Path, stored bool) field.ErrorList {
171171
allErrs := validateRequestName(request.Name, fldPath.Child("name"))
172-
if len(request.FirstAvailable) > 0 {
173-
if request.DeviceClassName != "" {
174-
allErrs = append(allErrs, field.Invalid(fldPath.Child("deviceClassName"), request.DeviceClassName, "must not be specified when firstAvailable is set"))
175-
}
172+
173+
if request.DeviceClassName == "" && len(request.FirstAvailable) == 0 {
174+
allErrs = append(allErrs, field.Required(fldPath, "exactly one of `deviceClassName` or `firstAvailable` must be specified"))
175+
} else if request.DeviceClassName != "" && len(request.FirstAvailable) > 0 {
176+
allErrs = append(allErrs, field.Invalid(fldPath, nil, "exactly one of `deviceClassName` or `firstAvailable` must be specified"))
177+
} else if request.DeviceClassName != "" {
178+
allErrs = append(allErrs, validateDeviceClass(request.DeviceClassName, fldPath.Child("deviceClassName"))...)
179+
allErrs = append(allErrs, validateSelectorSlice(request.Selectors, fldPath.Child("selectors"), stored)...)
180+
allErrs = append(allErrs, validateDeviceAllocationMode(request.AllocationMode, request.Count, fldPath.Child("allocationMode"), fldPath.Child("count"))...)
181+
} else if len(request.FirstAvailable) > 0 {
176182
if request.Selectors != nil {
177183
allErrs = append(allErrs, field.Invalid(fldPath.Child("selectors"), request.Selectors, "must not be specified when firstAvailable is set"))
178184
}
@@ -193,11 +199,7 @@ func validateDeviceRequest(request resource.DeviceRequest, fldPath *field.Path,
193199
return subRequest.Name, "name"
194200
},
195201
fldPath.Child("firstAvailable"))...)
196-
return allErrs
197202
}
198-
allErrs = append(allErrs, validateDeviceClass(request.DeviceClassName, fldPath.Child("deviceClassName"))...)
199-
allErrs = append(allErrs, validateSelectorSlice(request.Selectors, fldPath.Child("selectors"), stored)...)
200-
allErrs = append(allErrs, validateDeviceAllocationMode(request.AllocationMode, request.Count, fldPath.Child("allocationMode"), fldPath.Child("count"))...)
201203
return allErrs
202204
}
203205

‎pkg/apis/resource/validation/validation_resourceclaim_test.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -245,8 +245,8 @@ func TestValidateClaim(t *testing.T) {
245245
return claim
246246
}(),
247247
},
248-
"missing-classname": {
249-
wantFailures: field.ErrorList{field.Required(field.NewPath("spec", "devices", "requests").Index(0).Child("deviceClassName"), "")},
248+
"missing-classname-and-firstavailable": {
249+
wantFailures: field.ErrorList{field.Required(field.NewPath("spec", "devices", "requests").Index(0), "exactly one of `deviceClassName` or `firstAvailable` must be specified")},
250250
claim: func() *resource.ResourceClaim {
251251
claim := testClaim(goodName, goodNS, validClaimSpec)
252252
claim.Spec.Devices.Requests[0].DeviceClassName = ""
@@ -578,11 +578,7 @@ func TestValidateClaim(t *testing.T) {
578578
},
579579
"prioritized-list-field-on-parent": {
580580
wantFailures: field.ErrorList{
581-
field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("deviceClassName"), goodName, "must not be specified when firstAvailable is set"),
582-
field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("selectors"), validSelector, "must not be specified when firstAvailable is set"),
583-
field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("allocationMode"), resource.DeviceAllocationModeAll, "must not be specified when firstAvailable is set"),
584-
field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("count"), int64(2), "must not be specified when firstAvailable is set"),
585-
field.Invalid(field.NewPath("spec", "devices", "requests").Index(0).Child("adminAccess"), ptr.To(true), "must not be specified when firstAvailable is set"),
581+
field.Invalid(field.NewPath("spec", "devices", "requests").Index(0), nil, "exactly one of `deviceClassName` or `firstAvailable` must be specified"),
586582
},
587583
claim: func() *resource.ResourceClaim {
588584
claim := testClaim(goodName, goodNS, validClaimSpecWithFirstAvailable)

‎pkg/apis/resource/validation/validation_resourceclaimtemplate_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ func TestValidateClaimTemplate(t *testing.T) {
190190
template: testClaimTemplate(goodName, goodNS, validClaimSpecWithFirstAvailable),
191191
},
192192
"proritized-list-class-name-on-parent": {
193-
wantFailures: field.ErrorList{field.Invalid(field.NewPath("spec", "spec", "devices", "requests").Index(0).Child("deviceClassName"), goodName, "must not be specified when firstAvailable is set")},
193+
wantFailures: field.ErrorList{field.Invalid(field.NewPath("spec", "spec", "devices", "requests").Index(0), nil, "exactly one of `deviceClassName` or `firstAvailable` must be specified")},
194194
template: func() *resource.ResourceClaimTemplate {
195195
template := testClaimTemplate(goodName, goodNS, validClaimSpecWithFirstAvailable)
196196
template.Spec.Spec.Devices.Requests[0].DeviceClassName = goodName

‎staging/src/k8s.io/api/resource/v1alpha3/types.go‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,9 @@ type DeviceRequest struct {
400400
// request.
401401
//
402402
// A class is required if no subrequests are specified in the
403-
// firstAvailable list. Which classes are available depends on the cluster.
403+
// firstAvailable list and no class can be set if subrequests
404+
// are specified in the firstAvailable list.
405+
// Which classes are available depends on the cluster.
404406
//
405407
// Administrators may use this to restrict which devices may get
406408
// requested by only installing classes with selectors for permitted
@@ -417,6 +419,9 @@ type DeviceRequest struct {
417419
// request. All selectors must be satisfied for a device to be
418420
// considered.
419421
//
422+
// This field can only be set when deviceClassName is set and no subrequests
423+
// are specified in the firstAvailable list.
424+
//
420425
// +optional
421426
// +listType=atomic
422427
Selectors []DeviceSelector `json:"selectors,omitempty" protobuf:"bytes,3,name=selectors"`
@@ -437,6 +442,9 @@ type DeviceRequest struct {
437442
// the mode is ExactCount and count is not specified, the default count is
438443
// one. Any other requests must specify this field.
439444
//
445+
// This field can only be set when deviceClassName is set and no subrequests
446+
// are specified in the firstAvailable list.
447+
//
440448
// More modes may get added in the future. Clients must refuse to handle
441449
// requests with unknown modes.
442450
//
@@ -446,6 +454,9 @@ type DeviceRequest struct {
446454
// Count is used only when the count mode is "ExactCount". Must be greater than zero.
447455
// If AllocationMode is ExactCount and this field is not specified, the default is one.
448456
//
457+
// This field can only be set when deviceClassName is set and no subrequests
458+
// are specified in the firstAvailable list.
459+
//
449460
// +optional
450461
// +oneOf=AllocationMode
451462
Count int64 `json:"count,omitempty" protobuf:"bytes,5,opt,name=count"`
@@ -456,6 +467,9 @@ type DeviceRequest struct {
456467
// all ordinary claims to the device with respect to access modes and
457468
// any resource allocations.
458469
//
470+
// This field can only be set when deviceClassName is set and no subrequests
471+
// are specified in the firstAvailable list.
472+
//
459473
// This is an alpha field and requires enabling the DRAAdminAccess
460474
// feature gate. Admin access is disabled if this field is unset or
461475
// set to false, otherwise it is enabled.
@@ -467,8 +481,8 @@ type DeviceRequest struct {
467481
// FirstAvailable contains subrequests, of which exactly one will be
468482
// satisfied by the scheduler to satisfy this request. It tries to
469483
// satisfy them in the order in which they are listed here. So if
470-
// there are two entries in the list, the schduler will only check
471-
// the second one if it determines that the first one can not be used.
484+
// there are two entries in the list, the scheduler will only check
485+
// the second one if it determines that the first one cannot be used.
472486
//
473487
// This field may only be set in the entries of DeviceClaim.Requests.
474488
//

‎staging/src/k8s.io/api/resource/v1beta1/types.go‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,9 @@ type DeviceRequest struct {
409409
// request.
410410
//
411411
// A class is required if no subrequests are specified in the
412-
// firstAvailable list. Which classes are available depends on the cluster.
412+
// firstAvailable list and no class can be set if subrequests
413+
// are specified in the firstAvailable list.
414+
// Which classes are available depends on the cluster.
413415
//
414416
// Administrators may use this to restrict which devices may get
415417
// requested by only installing classes with selectors for permitted
@@ -426,6 +428,9 @@ type DeviceRequest struct {
426428
// request. All selectors must be satisfied for a device to be
427429
// considered.
428430
//
431+
// This field can only be set when deviceClassName is set and no subrequests
432+
// are specified in the firstAvailable list.
433+
//
429434
// +optional
430435
// +listType=atomic
431436
Selectors []DeviceSelector `json:"selectors,omitempty" protobuf:"bytes,3,name=selectors"`
@@ -446,6 +451,9 @@ type DeviceRequest struct {
446451
// the mode is ExactCount and count is not specified, the default count is
447452
// one. Any other requests must specify this field.
448453
//
454+
// This field can only be set when deviceClassName is set and no subrequests
455+
// are specified in the firstAvailable list.
456+
//
449457
// More modes may get added in the future. Clients must refuse to handle
450458
// requests with unknown modes.
451459
//
@@ -455,6 +463,9 @@ type DeviceRequest struct {
455463
// Count is used only when the count mode is "ExactCount". Must be greater than zero.
456464
// If AllocationMode is ExactCount and this field is not specified, the default is one.
457465
//
466+
// This field can only be set when deviceClassName is set and no subrequests
467+
// are specified in the firstAvailable list.
468+
//
458469
// +optional
459470
// +oneOf=AllocationMode
460471
Count int64 `json:"count,omitempty" protobuf:"bytes,5,opt,name=count"`
@@ -465,6 +476,9 @@ type DeviceRequest struct {
465476
// all ordinary claims to the device with respect to access modes and
466477
// any resource allocations.
467478
//
479+
// This field can only be set when deviceClassName is set and no subrequests
480+
// are specified in the firstAvailable list.
481+
//
468482
// This is an alpha field and requires enabling the DRAAdminAccess
469483
// feature gate. Admin access is disabled if this field is unset or
470484
// set to false, otherwise it is enabled.
@@ -476,8 +490,8 @@ type DeviceRequest struct {
476490
// FirstAvailable contains subrequests, of which exactly one will be
477491
// satisfied by the scheduler to satisfy this request. It tries to
478492
// satisfy them in the order in which they are listed here. So if
479-
// there are two entries in the list, the schduler will only check
480-
// the second one if it determines that the first one can not be used.
493+
// there are two entries in the list, the scheduler will only check
494+
// the second one if it determines that the first one cannot be used.
481495
//
482496
// This field may only be set in the entries of DeviceClaim.Requests.
483497
//

0 commit comments

Comments
 (0)