Skip to content

Commit a062f91

Browse files
LionelJouinLionelJouin
committed
[KEP-4817] Fixes based on review
* Rename HWAddress to HardwareAddress * Fix condition validation * Remove feature gate validation * Fix drop field on disabled feature gate Signed-off-by: Lionel Jouin <[email protected]>
1 parent 5df47a6 commit a062f91

File tree

8 files changed

+120
-62
lines changed

8 files changed

+120
-62
lines changed

‎pkg/apis/resource/types.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,8 +1056,8 @@ type NetworkDeviceData struct {
10561056
// +listType=atomic
10571057
Addresses []string
10581058

1059-
// HWAddress represents the hardware address (e.g. MAC Address) of the device's network interface.
1059+
// HardwareAddress represents the hardware address (e.g. MAC Address) of the device's network interface.
10601060
//
10611061
// +optional
1062-
HWAddress string
1062+
HardwareAddress string
10631063
}

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

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -25,20 +25,18 @@ import (
2525

2626
apiequality "k8s.io/apimachinery/pkg/api/equality"
2727
apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
28-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
28+
metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
2929
"k8s.io/apimachinery/pkg/runtime"
3030
"k8s.io/apimachinery/pkg/types"
3131
"k8s.io/apimachinery/pkg/util/sets"
3232
"k8s.io/apimachinery/pkg/util/validation"
3333
"k8s.io/apimachinery/pkg/util/validation/field"
3434
"k8s.io/apiserver/pkg/cel"
3535
"k8s.io/apiserver/pkg/cel/environment"
36-
utilfeature "k8s.io/apiserver/pkg/util/feature"
3736
dracel "k8s.io/dynamic-resource-allocation/cel"
3837
"k8s.io/dynamic-resource-allocation/structured"
3938
corevalidation "k8s.io/kubernetes/pkg/apis/core/validation"
4039
"k8s.io/kubernetes/pkg/apis/resource"
41-
"k8s.io/kubernetes/pkg/features"
4240
)
4341

4442
var (
@@ -268,20 +266,18 @@ func validateResourceClaimStatusUpdate(status, oldStatus *resource.ResourceClaim
268266
func(consumer resource.ResourceClaimConsumerReference) (types.UID, string) { return consumer.UID, "uid" },
269267
fldPath.Child("reservedFor"))...)
270268

271-
if utilfeature.DefaultFeatureGate.Enabled(features.DRAResourceClaimDeviceStatus) {
272-
var allocatedDevices sets.Set[structured.DeviceID]
273-
if status.Allocation != nil {
274-
allocatedDevices = gatherAllocatedDevices(&status.Allocation.Devices)
275-
}
276-
allErrs = append(allErrs, validateSet(status.Devices, -1,
277-
func(device resource.AllocatedDeviceStatus, fldPath *field.Path) field.ErrorList {
278-
return validateDeviceStatus(device, fldPath, allocatedDevices)
279-
},
280-
func(device resource.AllocatedDeviceStatus) (structured.DeviceID, string) {
281-
return structured.DeviceID{Driver: device.Driver, Pool: device.Pool, Device: device.Device}, "deviceID"
282-
},
283-
fldPath.Child("devices"))...)
269+
var allocatedDevices sets.Set[structured.DeviceID]
270+
if status.Allocation != nil {
271+
allocatedDevices = gatherAllocatedDevices(&status.Allocation.Devices)
284272
}
273+
allErrs = append(allErrs, validateSet(status.Devices, -1,
274+
func(device resource.AllocatedDeviceStatus, fldPath *field.Path) field.ErrorList {
275+
return validateDeviceStatus(device, fldPath, allocatedDevices)
276+
},
277+
func(device resource.AllocatedDeviceStatus) (structured.DeviceID, string) {
278+
return structured.DeviceID{Driver: device.Driver, Pool: device.Pool, Device: device.Device}, "deviceID"
279+
},
280+
fldPath.Child("devices"))...)
285281

286282
// Now check for invariants that must be valid for a ResourceClaim.
287283
if len(status.ReservedFor) > 0 {
@@ -748,21 +744,17 @@ func validateDeviceStatus(device resource.AllocatedDeviceStatus, fldPath *field.
748744
if !allocatedDevices.Has(deviceID) {
749745
allErrs = append(allErrs, field.Invalid(fldPath, deviceID, "must be an allocated device in the claim"))
750746
}
751-
allErrs = append(allErrs, validateSlice(device.Conditions, -1,
752-
func(condition metav1.Condition, fldPath *field.Path) field.ErrorList {
753-
var allErrs field.ErrorList
754-
return allErrs
755-
}, fldPath.Child("conditions"))...)
747+
allErrs = append(allErrs, metav1validation.ValidateConditions(device.Conditions, fldPath.Child("conditions"))...)
756748
if len(device.Data.Raw) > 0 { // Data is an optional field.
757749
allErrs = append(allErrs, validateRawExtension(device.Data, fldPath.Child("data"))...)
758750
}
759751
allErrs = append(allErrs, validateNetworkDeviceData(device.NetworkData, fldPath.Child("networkData"))...)
760752
return allErrs
761753
}
762754

755+
// validateRawExtension validates RawExtension as in https://github.com/kubernetes/kubernetes/pull/125549/
763756
func validateRawExtension(rawExtension runtime.RawExtension, fldPath *field.Path) field.ErrorList {
764757
var allErrs field.ErrorList
765-
// Validation of RawExtension as in https://github.com/kubernetes/kubernetes/pull/125549/
766758
var v any
767759
if len(rawExtension.Raw) == 0 {
768760
allErrs = append(allErrs, field.Required(fldPath, ""))

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

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -995,8 +995,11 @@ func TestValidateClaimStatusUpdate(t *testing.T) {
995995
Device: goodName,
996996
Conditions: []metav1.Condition{
997997
{
998-
Type: "test",
999-
Status: metav1.ConditionTrue,
998+
Type: "test",
999+
Status: metav1.ConditionTrue,
1000+
Reason: "test_reason",
1001+
LastTransitionTime: metav1.Now(),
1002+
ObservedGeneration: 0,
10001003
},
10011004
},
10021005
Data: runtime.RawExtension{
@@ -1008,7 +1011,7 @@ func TestValidateClaimStatusUpdate(t *testing.T) {
10081011
"10.9.8.0/24",
10091012
"2001:db8::/64",
10101013
},
1011-
HWAddress: "ea:9f:cb:40:b1:7b",
1014+
HardwareAddress: "ea:9f:cb:40:b1:7b",
10121015
},
10131016
},
10141017
}
@@ -1098,8 +1101,10 @@ func TestValidateClaimStatusUpdate(t *testing.T) {
10981101
deviceStatusFeatureGate: true,
10991102
},
11001103
"invalid-device-status-duplicate-disabled-feature-gate": {
1101-
wantFailures: nil,
1102-
oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(),
1104+
wantFailures: field.ErrorList{
1105+
field.Duplicate(field.NewPath("status", "devices").Index(1).Child("deviceID"), structured.DeviceID{Driver: goodName, Pool: goodName, Device: goodName}),
1106+
},
1107+
oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(),
11031108
update: func(claim *resource.ResourceClaim) *resource.ResourceClaim {
11041109
claim.Status.Devices = []resource.AllocatedDeviceStatus{
11051110
{
@@ -1118,8 +1123,10 @@ func TestValidateClaimStatusUpdate(t *testing.T) {
11181123
deviceStatusFeatureGate: false,
11191124
},
11201125
"invalid-network-device-status-disabled-feature-gate": {
1121-
wantFailures: nil,
1122-
oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(),
1126+
wantFailures: field.ErrorList{
1127+
field.Invalid(field.NewPath("status", "devices").Index(0).Child("networkData", "addresses").Index(0), "300.9.8.0/24", "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)"),
1128+
},
1129+
oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(),
11231130
update: func(claim *resource.ResourceClaim) *resource.ResourceClaim {
11241131
claim.Status.Devices = []resource.AllocatedDeviceStatus{
11251132
{
@@ -1138,8 +1145,10 @@ func TestValidateClaimStatusUpdate(t *testing.T) {
11381145
deviceStatusFeatureGate: false,
11391146
},
11401147
"invalid-data-device-status-disabled-feature-gate": {
1141-
wantFailures: nil,
1142-
oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(),
1148+
wantFailures: field.ErrorList{
1149+
field.Invalid(field.NewPath("status", "devices").Index(0).Child("data"), "<value omitted>", "error parsing data: invalid character 'o' in literal false (expecting 'a')"),
1150+
},
1151+
oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(),
11431152
update: func(claim *resource.ResourceClaim) *resource.ResourceClaim {
11441153
claim.Status.Devices = []resource.AllocatedDeviceStatus{
11451154
{
@@ -1156,8 +1165,10 @@ func TestValidateClaimStatusUpdate(t *testing.T) {
11561165
deviceStatusFeatureGate: false,
11571166
},
11581167
"invalid-device-status-no-device-disabled-feature-gate": {
1159-
wantFailures: nil,
1160-
oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(),
1168+
wantFailures: field.ErrorList{
1169+
field.Invalid(field.NewPath("status", "devices").Index(0), structured.DeviceID{Driver: "b", Pool: "a", Device: "r"}, "must be an allocated device in the claim"),
1170+
},
1171+
oldClaim: func() *resource.ResourceClaim { return validAllocatedClaim }(),
11611172
update: func(claim *resource.ResourceClaim) *resource.ResourceClaim {
11621173
claim.Status.Devices = []resource.AllocatedDeviceStatus{
11631174
{

‎pkg/registry/resource/resourceclaim/strategy.go‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -237,15 +237,18 @@ func draAdminAccessFeatureInUse(claim *resource.ResourceClaim) bool {
237237
}
238238

239239
func dropDisabledDRAResourceClaimDeviceStatusFields(newClaim, oldClaim *resource.ResourceClaim) {
240-
if !utilfeature.DefaultFeatureGate.Enabled(features.DRAResourceClaimDeviceStatus) {
240+
isDRAResourceClaimDeviceStatusInUse := (oldClaim != nil && len(oldClaim.Status.Devices) > 0)
241+
// drop resourceClaim.Status.Devices field if feature gate is not enabled and it was not in use
242+
if !utilfeature.DefaultFeatureGate.Enabled(features.DRAResourceClaimDeviceStatus) && !isDRAResourceClaimDeviceStatusInUse {
241243
newClaim.Status.Devices = nil
242244
}
243245
}
244246

245247
// dropDeallocatedStatusDevices removes the status.devices that were allocated
246248
// in the oldClaim and that have been removed in the newClaim.
247249
func dropDeallocatedStatusDevices(newClaim, oldClaim *resource.ResourceClaim) {
248-
if !utilfeature.DefaultFeatureGate.Enabled(features.DRAResourceClaimDeviceStatus) {
250+
isDRAResourceClaimDeviceStatusInUse := (oldClaim != nil && len(oldClaim.Status.Devices) > 0)
251+
if !utilfeature.DefaultFeatureGate.Enabled(features.DRAResourceClaimDeviceStatus) && !isDRAResourceClaimDeviceStatusInUse {
249252
return
250253
}
251254

@@ -268,18 +271,19 @@ func dropDeallocatedStatusDevices(newClaim, oldClaim *resource.ResourceClaim) {
268271
}
269272

270273
// Remove from newClaim.Status.Devices.
271-
for i := len(newClaim.Status.Devices) - 1; i >= 0; i-- {
274+
n := 0
275+
for _, device := range newClaim.Status.Devices {
272276
deviceID := structured.DeviceID{
273-
Driver: newClaim.Status.Devices[i].Driver,
274-
Pool: newClaim.Status.Devices[i].Pool,
275-
Device: newClaim.Status.Devices[i].Device,
277+
Driver: device.Driver,
278+
Pool: device.Pool,
279+
Device: device.Device,
276280
}
277-
// Device was in the oldClaim.Status.Allocation.Devices but is no longer in the
278-
// newClaim.Status.Allocation.Devices so it must be removed from the newClaim.Status.Devices.
279-
if deallocatedDevices.Has(deviceID) {
280-
newClaim.Status.Devices = append(newClaim.Status.Devices[:i], newClaim.Status.Devices[i+1:]...)
281+
if !deallocatedDevices.Has(deviceID) {
282+
newClaim.Status.Devices[n] = device
283+
n++
281284
}
282285
}
286+
newClaim.Status.Devices = newClaim.Status.Devices[:n]
283287

284288
if len(newClaim.Status.Devices) == 0 {
285289
newClaim.Status.Devices = nil

‎pkg/registry/resource/resourceclaim/strategy_test.go‎

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -133,10 +133,12 @@ var objWithAdminAccessStatus = &resource.ResourceClaim{
133133
},
134134
}
135135

136-
var testRequest = "test-request"
137-
var testDriver = "test-driver"
138-
var testPool = "test-pool"
139-
var testDevice = "test-device"
136+
const (
137+
testRequest = "test-request"
138+
testDriver = "test-driver"
139+
testPool = "test-pool"
140+
testDevice = "test-device"
141+
)
140142

141143
func TestStrategy(t *testing.T) {
142144
if !Strategy.NamespaceScoped() {
@@ -375,6 +377,30 @@ func TestStatusStrategyUpdate(t *testing.T) {
375377
return obj
376378
}(),
377379
},
380+
"keep-fields-devices-status-disable-feature-gate": {
381+
oldObj: func() *resource.ResourceClaim {
382+
obj := obj.DeepCopy()
383+
addSpecDevicesRequest(obj, testRequest)
384+
addStatusAllocationDevicesResults(obj, testDriver, testPool, testDevice, testRequest)
385+
addStatusDevices(obj, testDriver, testPool, testDevice)
386+
return obj
387+
}(),
388+
newObj: func() *resource.ResourceClaim { // Status is added
389+
obj := obj.DeepCopy()
390+
addSpecDevicesRequest(obj, testRequest)
391+
addStatusAllocationDevicesResults(obj, testDriver, testPool, testDevice, testRequest)
392+
addStatusDevices(obj, testDriver, testPool, testDevice)
393+
return obj
394+
}(),
395+
deviceStatusFeatureGate: false,
396+
expectObj: func() *resource.ResourceClaim { // Status is still there (as the status was set in the old object)
397+
obj := obj.DeepCopy()
398+
addSpecDevicesRequest(obj, testRequest)
399+
addStatusAllocationDevicesResults(obj, testDriver, testPool, testDevice, testRequest)
400+
addStatusDevices(obj, testDriver, testPool, testDevice)
401+
return obj
402+
}(),
403+
},
378404
"keep-fields-devices-status": {
379405
oldObj: func() *resource.ResourceClaim {
380406
obj := obj.DeepCopy()
@@ -419,6 +445,27 @@ func TestStatusStrategyUpdate(t *testing.T) {
419445
return obj
420446
}(),
421447
},
448+
"drop-status-deallocated-device-disable-feature-gate": {
449+
oldObj: func() *resource.ResourceClaim {
450+
obj := obj.DeepCopy()
451+
addSpecDevicesRequest(obj, testRequest)
452+
addStatusAllocationDevicesResults(obj, testDriver, testPool, testDevice, testRequest)
453+
addStatusDevices(obj, testDriver, testPool, testDevice)
454+
return obj
455+
}(),
456+
newObj: func() *resource.ResourceClaim { // device is deallocated
457+
obj := obj.DeepCopy()
458+
addSpecDevicesRequest(obj, testRequest)
459+
addStatusDevices(obj, testDriver, testPool, testDevice)
460+
return obj
461+
}(),
462+
deviceStatusFeatureGate: false,
463+
expectObj: func() *resource.ResourceClaim { // Status is no longer there
464+
obj := obj.DeepCopy()
465+
addSpecDevicesRequest(obj, testRequest)
466+
return obj
467+
}(),
468+
},
422469
}
423470

424471
for name, tc := range testcases {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1067,8 +1067,8 @@ type NetworkDeviceData struct {
10671067
// +listType=atomic
10681068
Addresses []string `json:"addresses,omitempty" protobuf:"bytes,2,opt,name=addresses"`
10691069

1070-
// HWAddress represents the hardware address (e.g. MAC Address) of the device's network interface.
1070+
// HardwareAddress represents the hardware address (e.g. MAC Address) of the device's network interface.
10711071
//
10721072
// +optional
1073-
HWAddress string `json:"hwAddress,omitempty" protobuf:"bytes,3,opt,name=hwAddress"`
1073+
HardwareAddress string `json:"hardwareAddress,omitempty" protobuf:"bytes,3,opt,name=hardwareAddress"`
10741074
}

‎test/e2e/dra/dra.go‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -437,12 +437,12 @@ var _ = framework.SIGDescribe("node")("DRA", feature.DynamicResourceAllocation,
437437
Driver: allocatedResourceClaim.Status.Allocation.Devices.Results[0].Driver,
438438
Pool: allocatedResourceClaim.Status.Allocation.Devices.Results[0].Pool,
439439
Device: allocatedResourceClaim.Status.Allocation.Devices.Results[0].Device,
440-
Conditions: []metav1.Condition{{Type: "a", Status: "b", Message: "c", Reason: "d"}},
440+
Conditions: []metav1.Condition{{Type: "a", Status: "True", Message: "c", Reason: "d", LastTransitionTime: metav1.NewTime(time.Now().Truncate(time.Second))}},
441441
Data: runtime.RawExtension{Raw: []byte(`{"foo":"bar"}`)},
442442
NetworkData: &resourceapi.NetworkDeviceData{
443-
InterfaceName: "inf1",
444-
Addresses: []string{"10.9.8.0/24", "2001:db8::/64"},
445-
HWAddress: "bc:1c:b6:3e:b8:25",
443+
InterfaceName: "inf1",
444+
Addresses: []string{"10.9.8.0/24", "2001:db8::/64"},
445+
HardwareAddress: "bc:1c:b6:3e:b8:25",
446446
},
447447
})
448448
// Updates the ResourceClaim from the driver on the same node as the pod.
@@ -456,19 +456,23 @@ var _ = framework.SIGDescribe("node")("DRA", feature.DynamicResourceAllocation,
456456
Driver: allocatedResourceClaim.Status.Allocation.Devices.Results[0].Driver,
457457
Pool: allocatedResourceClaim.Status.Allocation.Devices.Results[0].Pool,
458458
Device: allocatedResourceClaim.Status.Allocation.Devices.Results[0].Device,
459-
Conditions: []metav1.Condition{{Type: "e", Status: "f", Message: "g", Reason: "h"}},
459+
Conditions: []metav1.Condition{{Type: "e", Status: "True", Message: "g", Reason: "h", LastTransitionTime: metav1.NewTime(time.Now().Truncate(time.Second))}},
460460
Data: runtime.RawExtension{Raw: []byte(`{"bar":"foo"}`)},
461461
NetworkData: &resourceapi.NetworkDeviceData{
462-
InterfaceName: "inf2",
463-
Addresses: []string{"10.9.8.1/24", "2001:db8::1/64"},
464-
HWAddress: "bc:1c:b6:3e:b8:26",
462+
InterfaceName: "inf2",
463+
Addresses: []string{"10.9.8.1/24", "2001:db8::1/64"},
464+
HardwareAddress: "bc:1c:b6:3e:b8:26",
465465
},
466466
}
467467
updatedResourceClaim2, err := driver.Nodes[scheduledPod.Spec.NodeName].ExamplePlugin.UpdateStatus(ctx, updatedResourceClaim)
468468
framework.ExpectNoError(err)
469469
gomega.Expect(updatedResourceClaim2).ToNot(gomega.BeNil())
470470
gomega.Expect(updatedResourceClaim2.Status.Devices).To(gomega.Equal(updatedResourceClaim.Status.Devices))
471471

472+
getResourceClaim, err := b.f.ClientSet.ResourceV1alpha3().ResourceClaims(b.f.Namespace.Name).Get(ctx, claim.Name, metav1.GetOptions{})
473+
framework.ExpectNoError(err)
474+
gomega.Expect(getResourceClaim).ToNot(gomega.BeNil())
475+
gomega.Expect(getResourceClaim.Status.Devices).To(gomega.Equal(updatedResourceClaim.Status.Devices))
472476
})
473477
}
474478

‎test/integration/resourceclaim/feature_enable_disable_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ func TestEnableDisableDRAResourceClaimDeviceStatus(t *testing.T) {
9090
"10.9.8.0/24",
9191
"2001:db8::/64",
9292
},
93-
HWAddress: "ea:9f:cb:40:b1:7b",
93+
HardwareAddress: "ea:9f:cb:40:b1:7b",
9494
},
9595
},
9696
},
@@ -162,7 +162,7 @@ func TestEnableDisableDRAResourceClaimDeviceStatus(t *testing.T) {
162162
"10.9.8.0/24",
163163
"2001:db8::/64",
164164
},
165-
HWAddress: "ea:9f:cb:40:b1:7b",
165+
HardwareAddress: "ea:9f:cb:40:b1:7b",
166166
},
167167
},
168168
},
@@ -198,7 +198,7 @@ func TestEnableDisableDRAResourceClaimDeviceStatus(t *testing.T) {
198198
"10.9.8.0/24",
199199
"2001:db8::/64",
200200
},
201-
HWAddress: "ea:9f:cb:40:b1:7b",
201+
HardwareAddress: "ea:9f:cb:40:b1:7b",
202202
},
203203
},
204204
},

0 commit comments

Comments
 (0)