Addressed comments

This commit is contained in:
Morten Torkildsen 2025-02-28 20:47:35 +00:00
parent 7555cbca90
commit e2d1fcc162
10 changed files with 96 additions and 30 deletions

View File

@ -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

View File

@ -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 == "" {

View File

@ -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",
},
},
},
},
}

View File

@ -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 == "" {

View File

@ -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",
},
},
},
},
}

View File

@ -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
}

View File

@ -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)

View File

@ -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

View File

@ -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.
//

View File

@ -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.
//