diff --git a/pkg/api/resourceclaimspec/util.go b/pkg/api/resourceclaimspec/util.go new file mode 100644 index 00000000000..989e27e82d5 --- /dev/null +++ b/pkg/api/resourceclaimspec/util.go @@ -0,0 +1,177 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package resourceclaimspec + +import ( + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/kubernetes/pkg/apis/resource" + "k8s.io/kubernetes/pkg/features" +) + +// DropDisabledFields removes disabled fields from the spec unless they were in +// use there before. +// +// Theoretically some features could be in use in an old ResourceClaim status +// and not in use in the spec. This can only occur in a spec update, which is +// currently prevented because the entire spec is immutable. Even if it was +// allowed, preventing adding disabled fields to the spec is the right thing to +// do regardless of what may have ended up in the status earlier. +func DropDisabledFields(new, old *resource.ResourceClaimSpec) { + dropDisabledDRAPrioritizedListFields(new, old) + dropDisabledDRADeviceTaintsFields(new, old) // Intentionally after dropDisabledDRAPrioritizedListFields to avoid iterating over FirstAvailable slice which needs to be dropped. + dropDisabledDRAAdminAccessFields(new, old) + dropDisabledDRAResourceClaimConsumableCapacityFields(new, old) +} + +func dropDisabledDRADeviceTaintsFields(new, old *resource.ResourceClaimSpec) { + if utilfeature.DefaultFeatureGate.Enabled(features.DRADeviceTaints) || + draDeviceTaintsInUse(old) { + return + } + + for i, req := range new.Devices.Requests { + if exactly := req.Exactly; exactly != nil { + exactly.Tolerations = nil + } + for e := range req.FirstAvailable { + new.Devices.Requests[i].FirstAvailable[e].Tolerations = nil + } + } +} + +func draDeviceTaintsInUse(spec *resource.ResourceClaimSpec) bool { + if spec == nil { + return false + } + + for _, req := range spec.Devices.Requests { + if exactly := req.Exactly; exactly != nil && len(exactly.Tolerations) > 0 { + return true + } + for _, sub := range req.FirstAvailable { + if len(sub.Tolerations) > 0 { + return true + } + } + } + + return false +} + +func dropDisabledDRAPrioritizedListFields(new, old *resource.ResourceClaimSpec) { + if utilfeature.DefaultFeatureGate.Enabled(features.DRAPrioritizedList) { + return + } + if draPrioritizedListFeatureInUse(old) { + return + } + + for i := range new.Devices.Requests { + new.Devices.Requests[i].FirstAvailable = nil + } +} + +func draPrioritizedListFeatureInUse(spec *resource.ResourceClaimSpec) bool { + if spec == nil { + return false + } + + for _, request := range spec.Devices.Requests { + if len(request.FirstAvailable) > 0 { + return true + } + } + + return false +} + +func dropDisabledDRAAdminAccessFields(new, old *resource.ResourceClaimSpec) { + if utilfeature.DefaultFeatureGate.Enabled(features.DRAAdminAccess) || + DRAAdminAccessFeatureInUse(old) { + // No need to drop anything. + return + } + + for i := range new.Devices.Requests { + if new.Devices.Requests[i].Exactly != nil { + new.Devices.Requests[i].Exactly.AdminAccess = nil + } + } +} + +// DRAAdminAccessFeatureInUse checks whether the feature is in use in the spec. +func DRAAdminAccessFeatureInUse(spec *resource.ResourceClaimSpec) bool { + if spec == nil { + return false + } + + for _, request := range spec.Devices.Requests { + if request.Exactly != nil && request.Exactly.AdminAccess != nil { + return true + } + } + + return false +} + +func dropDisabledDRAResourceClaimConsumableCapacityFields(new, old *resource.ResourceClaimSpec) { + if utilfeature.DefaultFeatureGate.Enabled(features.DRAConsumableCapacity) || + DRAConsumableCapacityFeatureInUse(old) { + // No need to drop anything. + return + } + + for _, constaint := range new.Devices.Constraints { + constaint.DistinctAttribute = nil + } + + for i := range new.Devices.Requests { + if new.Devices.Requests[i].Exactly != nil { + new.Devices.Requests[i].Exactly.Capacity = nil + } + request := new.Devices.Requests[i] + for j := range request.FirstAvailable { + new.Devices.Requests[i].FirstAvailable[j].Capacity = nil + } + } +} + +// DRAConsumableCapacityFeatureInUse checks whether the feature is in use in the spec. +func DRAConsumableCapacityFeatureInUse(spec *resource.ResourceClaimSpec) bool { + if spec == nil { + return false + } + + for _, constaint := range spec.Devices.Constraints { + if constaint.DistinctAttribute != nil { + return true + } + } + + for _, request := range spec.Devices.Requests { + if request.Exactly != nil && request.Exactly.Capacity != nil { + return true + } + for _, subRequest := range request.FirstAvailable { + if subRequest.Capacity != nil { + return true + } + } + } + + return false +} diff --git a/pkg/registry/resource/resourceclaim/strategy.go b/pkg/registry/resource/resourceclaim/strategy.go index cc6e8185696..5dd6db1ba34 100644 --- a/pkg/registry/resource/resourceclaim/strategy.go +++ b/pkg/registry/resource/resourceclaim/strategy.go @@ -38,6 +38,7 @@ import ( v1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/dynamic-resource-allocation/structured" "k8s.io/kubernetes/pkg/api/legacyscheme" + "k8s.io/kubernetes/pkg/api/resourceclaimspec" "k8s.io/kubernetes/pkg/apis/resource" "k8s.io/kubernetes/pkg/apis/resource/validation" "k8s.io/kubernetes/pkg/features" @@ -92,7 +93,7 @@ func (*resourceclaimStrategy) PrepareForCreate(ctx context.Context, obj runtime. // Status must not be set by user on create. claim.Status = resource.ResourceClaimStatus{} - dropDisabledFields(claim, nil) + dropDisabledSpecFields(claim, nil) } func (s *resourceclaimStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { @@ -119,7 +120,7 @@ func (*resourceclaimStrategy) PrepareForUpdate(ctx context.Context, obj, old run oldClaim := old.(*resource.ResourceClaim) newClaim.Status = oldClaim.Status - dropDisabledFields(newClaim, oldClaim) + dropDisabledSpecFields(newClaim, oldClaim) } func (s *resourceclaimStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { @@ -174,8 +175,8 @@ func (*resourceclaimStatusStrategy) PrepareForUpdate(ctx context.Context, obj, o newClaim.Spec = oldClaim.Spec metav1.ResetObjectMetaForStatus(&newClaim.ObjectMeta, &oldClaim.ObjectMeta) - dropDeallocatedStatusDevices(newClaim, oldClaim) - dropDisabledFields(newClaim, oldClaim) + dropDisabledStatusFields(newClaim, oldClaim) + dropDeallocatedStatusDevices(newClaim, oldClaim) // NOP if fields got dropped, so do this last. } func (r *resourceclaimStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { @@ -222,43 +223,23 @@ func toSelectableFields(claim *resource.ResourceClaim) fields.Set { return fields } -// dropDisabledFields removes fields which are covered by a feature gate. -func dropDisabledFields(newClaim, oldClaim *resource.ResourceClaim) { - dropDisabledDRAPrioritizedListFields(newClaim, oldClaim) - dropDisabledDRAAdminAccessFields(newClaim, oldClaim) +// dropDisabledSpecFields removes fields from the spec which are covered by a feature gate. +func dropDisabledSpecFields(newClaim, oldClaim *resource.ResourceClaim) { + var oldClaimSpec *resource.ResourceClaimSpec + if oldClaim != nil { + oldClaimSpec = &oldClaim.Spec + } + resourceclaimspec.DropDisabledFields(&newClaim.Spec, oldClaimSpec) +} + +// dropDisabledStatusFields removes fields from the status which are covered by a feature gate. +func dropDisabledStatusFields(newClaim, oldClaim *resource.ResourceClaim) { dropDisabledDRAResourceClaimDeviceStatusFields(newClaim, oldClaim) - dropDisabledDRADeviceBindingConditionsFields(newClaim, oldClaim) - dropDisabledDRAResourceClaimConsumableCapacityFields(newClaim, oldClaim) + dropDisabledDRAAdminAccessStatusFields(newClaim, oldClaim) + dropDisabledDRAResourceClaimConsumableCapacityStatusFields(newClaim, oldClaim) } -func dropDisabledDRAPrioritizedListFields(newClaim, oldClaim *resource.ResourceClaim) { - if utilfeature.DefaultFeatureGate.Enabled(features.DRAPrioritizedList) { - return - } - if draPrioritizedListFeatureInUse(oldClaim) { - return - } - - for i := range newClaim.Spec.Devices.Requests { - newClaim.Spec.Devices.Requests[i].FirstAvailable = nil - } -} - -func draPrioritizedListFeatureInUse(claim *resource.ResourceClaim) bool { - if claim == nil { - return false - } - - for _, request := range claim.Spec.Devices.Requests { - if len(request.FirstAvailable) > 0 { - return true - } - } - - return false -} - -func dropDisabledDRAAdminAccessFields(newClaim, oldClaim *resource.ResourceClaim) { +func dropDisabledDRAAdminAccessStatusFields(newClaim, oldClaim *resource.ResourceClaim) { if utilfeature.DefaultFeatureGate.Enabled(features.DRAAdminAccess) { // No need to drop anything. return @@ -273,15 +254,10 @@ func dropDisabledDRAAdminAccessFields(newClaim, oldClaim *resource.ResourceClaim return } - for i := range newClaim.Spec.Devices.Requests { - if newClaim.Spec.Devices.Requests[i].Exactly != nil { - newClaim.Spec.Devices.Requests[i].Exactly.AdminAccess = nil - } - } - if newClaim.Status.Allocation == nil { return } + for i := range newClaim.Status.Allocation.Devices.Results { newClaim.Status.Allocation.Devices.Results[i].AdminAccess = nil } @@ -292,10 +268,8 @@ func draAdminAccessFeatureInUse(claim *resource.ResourceClaim) bool { return false } - for _, request := range claim.Spec.Devices.Requests { - if request.Exactly != nil && request.Exactly.AdminAccess != nil { - return true - } + if resourceclaimspec.DRAAdminAccessFeatureInUse(&claim.Spec) { + return true } if allocation := claim.Status.Allocation; allocation != nil { @@ -322,6 +296,10 @@ func dropDisabledDRAResourceClaimDeviceStatusFields(newClaim, oldClaim *resource // dropDeallocatedStatusDevices removes the status.devices that were allocated // in the oldClaim and that have been removed in the newClaim. +// +// In other words, it removes stale status entries after deallocation. Doing +// this in the apiserver avoids having to update clients which might be unaware +// of the status feature. func dropDeallocatedStatusDevices(newClaim, oldClaim *resource.ResourceClaim) { if !utilfeature.DefaultFeatureGate.Enabled(features.DRAResourceClaimDeviceStatus) && !isDRAResourceClaimDeviceStatusInUse(oldClaim) { return @@ -368,64 +346,13 @@ func dropDeallocatedStatusDevices(newClaim, oldClaim *resource.ResourceClaim) { } } -// TODO: add tests after partitionable devices is merged (code conflict!) - -// dropDisabledDRADeviceBindingConditionsFields removes fields which are covered by a feature gate. -func dropDisabledDRADeviceBindingConditionsFields(newClaim, oldClaim *resource.ResourceClaim) { - if utilfeature.DefaultFeatureGate.Enabled(features.DRADeviceBindingConditions) && utilfeature.DefaultFeatureGate.Enabled(features.DRAResourceClaimDeviceStatus) || - draBindingConditionsFeatureInUse(oldClaim) { - return - } - - if newClaim.Status.Allocation == nil { - return - } - newClaim.Status.Allocation.AllocationTimestamp = nil - - for i := range newClaim.Status.Allocation.Devices.Results { - newClaim.Status.Allocation.Devices.Results[i].BindingConditions = nil - newClaim.Status.Allocation.Devices.Results[i].BindingFailureConditions = nil - } -} - -func draBindingConditionsFeatureInUse(claim *resource.ResourceClaim) bool { - if claim == nil || claim.Status.Allocation == nil { - return false - } - - if claim.Status.Allocation.AllocationTimestamp != nil { - return true - } - - for _, result := range claim.Status.Allocation.Devices.Results { - if len(result.BindingConditions) != 0 || len(result.BindingFailureConditions) != 0 { - return true - } - } - - return false -} - func draConsumableCapacityFeatureInUse(claim *resource.ResourceClaim) bool { if claim == nil { return false } - for _, constaint := range claim.Spec.Devices.Constraints { - if constaint.DistinctAttribute != nil { - return true - } - } - - for _, request := range claim.Spec.Devices.Requests { - if request.Exactly != nil && request.Exactly.Capacity != nil { - return true - } - for _, subRequest := range request.FirstAvailable { - if subRequest.Capacity != nil { - return true - } - } + if resourceclaimspec.DRAConsumableCapacityFeatureInUse(&claim.Spec) { + return true } if allocation := claim.Status.Allocation; allocation != nil { @@ -446,30 +373,15 @@ func draConsumableCapacityFeatureInUse(claim *resource.ResourceClaim) bool { return false } -// dropDisabledDRAResourceClaimConsumableCapacityFields drops any new feature fields -// from the newClaim if they were not used in the oldClaim. -func dropDisabledDRAResourceClaimConsumableCapacityFields(newClaim, oldClaim *resource.ResourceClaim) { +// dropDisabledDRAResourceClaimConsumableCapacityStatusFields drops any new feature fields +// from the newClaim status if they were not used in the oldClaim. +func dropDisabledDRAResourceClaimConsumableCapacityStatusFields(newClaim, oldClaim *resource.ResourceClaim) { if utilfeature.DefaultFeatureGate.Enabled(features.DRAConsumableCapacity) || draConsumableCapacityFeatureInUse(oldClaim) { // No need to drop anything. return } - for _, constaint := range newClaim.Spec.Devices.Constraints { - constaint.DistinctAttribute = nil - } - - // Drop any CapacityRequests newly added. - for i := range newClaim.Spec.Devices.Requests { - if newClaim.Spec.Devices.Requests[i].Exactly != nil { - newClaim.Spec.Devices.Requests[i].Exactly.Capacity = nil - } - request := newClaim.Spec.Devices.Requests[i] - for j := range request.FirstAvailable { - newClaim.Spec.Devices.Requests[i].FirstAvailable[j].Capacity = nil - } - } - if allocation := newClaim.Status.Allocation; allocation != nil { for i := range allocation.Devices.Results { newClaim.Status.Allocation.Devices.Results[i].ShareID = nil diff --git a/pkg/registry/resource/resourceclaim/strategy_test.go b/pkg/registry/resource/resourceclaim/strategy_test.go index ddbd022049b..51f843870a6 100644 --- a/pkg/registry/resource/resourceclaim/strategy_test.go +++ b/pkg/registry/resource/resourceclaim/strategy_test.go @@ -221,6 +221,27 @@ var objWithAdminAccessStatusInNonAdminNamespace = &resource.ResourceClaim{ }, } +var objWithDeviceTaints = &resource.ResourceClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim", + Namespace: "kube-system", + }, + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + Exactly: &resource.ExactDeviceRequest{ + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeAll, + Tolerations: []resource.DeviceToleration{{Key: "some-key", Operator: resource.DeviceTolerationOpExists}}, + }, + }, + }, + }, + }, +} + var objWithPrioritizedList = &resource.ResourceClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim", @@ -245,6 +266,31 @@ var objWithPrioritizedList = &resource.ResourceClaim{ }, } +var objWithDeviceTaintsInPrioritizedList = &resource.ResourceClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim", + Namespace: "default", + }, + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + FirstAvailable: []resource.DeviceSubRequest{ + { + Name: "subreq-0", + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeExactCount, + Count: 1, + Tolerations: []resource.DeviceToleration{{Key: "some-key", Operator: resource.DeviceTolerationOpExists}}, + }, + }, + }, + }, + }, + }, +} + var objWithAdminAccessStatus = &resource.ResourceClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim", @@ -375,6 +421,7 @@ func TestStrategyCreate(t *testing.T) { testcases := map[string]struct { obj *resource.ResourceClaim adminAccess bool + deviceTaints bool prioritizedList bool bindingConditions bool deviceStatus bool @@ -430,6 +477,48 @@ func TestStrategyCreate(t *testing.T) { } }, }, + "drop-fields-device-taints": { + obj: objWithDeviceTaints, + deviceTaints: false, + expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-device-taints": { + obj: objWithDeviceTaints, + deviceTaints: true, + expectObj: objWithDeviceTaints, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "drop-fields-device-taints-in-prioritized-list": { + obj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: false, + prioritizedList: true, + expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-device-taints-in-prioritized-list": { + obj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: true, + prioritizedList: true, + expectObj: objWithDeviceTaintsInPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, "drop-fields-prioritized-list": { obj: objWithPrioritizedList, prioritizedList: false, @@ -532,6 +621,7 @@ func TestStrategyCreate(t *testing.T) { mockNSClient := fakeClient.CoreV1().Namespaces() featuregatetesting.SetFeatureGatesDuringTest(t, utilfeature.DefaultFeatureGate, featuregatetesting.FeatureOverrides{ features.DRAAdminAccess: tc.adminAccess, + features.DRADeviceTaints: tc.deviceTaints, features.DRAPrioritizedList: tc.prioritizedList, features.DRAConsumableCapacity: tc.consumableCapacity, }) @@ -562,9 +652,10 @@ func TestStrategyUpdate(t *testing.T) { oldObj *resource.ResourceClaim newObj *resource.ResourceClaim adminAccess bool - expectValidationError string + deviceTaints bool prioritizedList bool consumableCapacity bool + expectValidationError string expectObj *resource.ResourceClaim verify func(*testing.T, []testclient.Action) }{ @@ -748,6 +839,102 @@ func TestStrategyUpdate(t *testing.T) { } }, }, + "drop-fields-device-taints": { + oldObj: obj, + newObj: objWithDeviceTaints, + deviceTaints: false, + prioritizedList: true, + expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-device-taints": { + oldObj: obj, + newObj: objWithDeviceTaints, + deviceTaints: true, + prioritizedList: true, + expectValidationError: fieldImmutableError, // Spec is immutable, cannot add tolerations. + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-device-taints": { + oldObj: objWithDeviceTaints, + newObj: objWithDeviceTaints, + deviceTaints: true, + prioritizedList: true, + expectObj: objWithDeviceTaints, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-device-taints-disabled-feature": { + oldObj: objWithDeviceTaints, + newObj: objWithDeviceTaints, + deviceTaints: false, + prioritizedList: true, + expectObj: objWithDeviceTaints, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "drop-fields-device-taints-in-prioritized-list": { + oldObj: objWithPrioritizedList, + newObj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: false, + prioritizedList: true, + expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-device-taints-in-prioritized-list": { + oldObj: objWithPrioritizedList, + newObj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: true, + prioritizedList: true, + expectValidationError: fieldImmutableError, // Spec is immutable, cannot add tolerations. + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-device-taints-in-prioritized-list": { + oldObj: objWithDeviceTaintsInPrioritizedList, + newObj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: true, + prioritizedList: true, + expectObj: objWithDeviceTaintsInPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-device-taints-in-prioritized-list-disabled-feature": { + oldObj: objWithDeviceTaintsInPrioritizedList, + newObj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: false, + prioritizedList: true, + expectObj: objWithDeviceTaintsInPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, } for name, tc := range testcases { @@ -757,6 +944,7 @@ func TestStrategyUpdate(t *testing.T) { featuregatetesting.SetFeatureGatesDuringTest(t, utilfeature.DefaultFeatureGate, featuregatetesting.FeatureOverrides{ features.DRAAdminAccess: tc.adminAccess, + features.DRADeviceTaints: tc.deviceTaints, features.DRAPrioritizedList: tc.prioritizedList, features.DRAConsumableCapacity: tc.consumableCapacity, }) diff --git a/pkg/registry/resource/resourceclaimtemplate/strategy.go b/pkg/registry/resource/resourceclaimtemplate/strategy.go index 967bf85286b..f53e31ba753 100644 --- a/pkg/registry/resource/resourceclaimtemplate/strategy.go +++ b/pkg/registry/resource/resourceclaimtemplate/strategy.go @@ -26,12 +26,11 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/storage/names" - utilfeature "k8s.io/apiserver/pkg/util/feature" v1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/kubernetes/pkg/api/legacyscheme" + "k8s.io/kubernetes/pkg/api/resourceclaimspec" "k8s.io/kubernetes/pkg/apis/resource" "k8s.io/kubernetes/pkg/apis/resource/validation" - "k8s.io/kubernetes/pkg/features" resourceutils "k8s.io/kubernetes/pkg/registry/resource" ) @@ -112,115 +111,9 @@ func toSelectableFields(template *resource.ResourceClaimTemplate) fields.Set { } func dropDisabledFields(newClaimTemplate, oldClaimTemplate *resource.ResourceClaimTemplate) { - dropDisabledDRAPrioritizedListFields(newClaimTemplate, oldClaimTemplate) - dropDisabledDRAAdminAccessFields(newClaimTemplate, oldClaimTemplate) - dropDisabledDRAResourceClaimConsumableCapacityFields(newClaimTemplate, oldClaimTemplate) -} - -func dropDisabledDRAPrioritizedListFields(newClaimTemplate, oldClaimTemplate *resource.ResourceClaimTemplate) { - if utilfeature.DefaultFeatureGate.Enabled(features.DRAPrioritizedList) { - return - } - if draPrioritizedListFeatureInUse(oldClaimTemplate) { - return - } - - for i := range newClaimTemplate.Spec.Spec.Devices.Requests { - newClaimTemplate.Spec.Spec.Devices.Requests[i].FirstAvailable = nil - } -} - -func draPrioritizedListFeatureInUse(claimTemplate *resource.ResourceClaimTemplate) bool { - if claimTemplate == nil { - return false - } - - for _, request := range claimTemplate.Spec.Spec.Devices.Requests { - if len(request.FirstAvailable) > 0 { - return true - } - } - - return false -} - -func dropDisabledDRAAdminAccessFields(newClaimTemplate, oldClaimTemplate *resource.ResourceClaimTemplate) { - if utilfeature.DefaultFeatureGate.Enabled(features.DRAAdminAccess) { - // No need to drop anything. - return - } - if draAdminAccessFeatureInUse(oldClaimTemplate) { - // If anything was set in the past, then fields must not get - // dropped on potentially unrelated updates. - return - } - - for i := range newClaimTemplate.Spec.Spec.Devices.Requests { - if newClaimTemplate.Spec.Spec.Devices.Requests[i].Exactly != nil { - newClaimTemplate.Spec.Spec.Devices.Requests[i].Exactly.AdminAccess = nil - } - } -} - -func draAdminAccessFeatureInUse(claimTemplate *resource.ResourceClaimTemplate) bool { - if claimTemplate == nil { - return false - } - - for _, request := range claimTemplate.Spec.Spec.Devices.Requests { - if request.Exactly != nil && request.Exactly.AdminAccess != nil { - return true - } - } - - return false -} - -func draConsumableCapacityFeatureInUse(claimTemplate *resource.ResourceClaimTemplate) bool { - if claimTemplate == nil { - return false - } - - for _, constaint := range claimTemplate.Spec.Spec.Devices.Constraints { - if constaint.DistinctAttribute != nil { - return true - } - } - - for _, request := range claimTemplate.Spec.Spec.Devices.Requests { - if request.Exactly != nil && request.Exactly.Capacity != nil { - return true - } - for _, subRequest := range request.FirstAvailable { - if subRequest.Capacity != nil { - return true - } - } - } - - return false -} - -// dropDisabledDRAResourceClaimConsumableCapacityFields drops any new feature field -// from the newClaimTemplate if they were not used in the oldClaimTemplate. -func dropDisabledDRAResourceClaimConsumableCapacityFields(newClaimTemplate, oldClaimTemplate *resource.ResourceClaimTemplate) { - if utilfeature.DefaultFeatureGate.Enabled(features.DRAConsumableCapacity) || - draConsumableCapacityFeatureInUse(oldClaimTemplate) { - // No need to drop anything. - return - } - - for _, constaint := range newClaimTemplate.Spec.Spec.Devices.Constraints { - constaint.DistinctAttribute = nil - } - - for i := range newClaimTemplate.Spec.Spec.Devices.Requests { - if newClaimTemplate.Spec.Spec.Devices.Requests[i].Exactly != nil { - newClaimTemplate.Spec.Spec.Devices.Requests[i].Exactly.Capacity = nil - } - request := newClaimTemplate.Spec.Spec.Devices.Requests[i] - for j := range request.FirstAvailable { - newClaimTemplate.Spec.Spec.Devices.Requests[i].FirstAvailable[j].Capacity = nil - } - } + var oldClaimSpec *resource.ResourceClaimSpec + if oldClaimTemplate != nil { + oldClaimSpec = &oldClaimTemplate.Spec.Spec + } + resourceclaimspec.DropDisabledFields(&newClaimTemplate.Spec.Spec, oldClaimSpec) } diff --git a/pkg/registry/resource/resourceclaimtemplate/strategy_test.go b/pkg/registry/resource/resourceclaimtemplate/strategy_test.go index 33b834c54ce..670addae080 100644 --- a/pkg/registry/resource/resourceclaimtemplate/strategy_test.go +++ b/pkg/registry/resource/resourceclaimtemplate/strategy_test.go @@ -79,6 +79,28 @@ var objWithAdminAccess = &resource.ResourceClaimTemplate{ }, } +var objInNonAdminNamespace = &resource.ResourceClaimTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim-template", + Namespace: "default", + }, + Spec: resource.ResourceClaimTemplateSpec{ + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + Exactly: &resource.ExactDeviceRequest{ + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeAll, + }, + }, + }, + }, + }, + }, +} + var objWithAdminAccessInNonAdminNamespace = &resource.ResourceClaimTemplate{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim-template", @@ -102,6 +124,29 @@ var objWithAdminAccessInNonAdminNamespace = &resource.ResourceClaimTemplate{ }, } +var objWithDeviceTaints = &resource.ResourceClaimTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim-template", + Namespace: "kube-system", + }, + Spec: resource.ResourceClaimTemplateSpec{ + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + Exactly: &resource.ExactDeviceRequest{ + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeAll, + Tolerations: []resource.DeviceToleration{{Key: "some-key", Operator: resource.DeviceTolerationOpExists}}, + }, + }, + }, + }, + }, + }, +} + var objWithPrioritizedList = &resource.ResourceClaimTemplate{ ObjectMeta: metav1.ObjectMeta{ Name: "valid-claim-template", @@ -170,6 +215,33 @@ func addSpecDeviceRequestWithCapacityRequests(resourceClaimTemplate *resource.Re resourceClaimTemplate.Spec.Spec.Devices.Requests = append(resourceClaimTemplate.Spec.Spec.Devices.Requests, r) } +var objWithDeviceTaintsInPrioritizedList = &resource.ResourceClaimTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-claim-template", + Namespace: "default", + }, + Spec: resource.ResourceClaimTemplateSpec{ + Spec: resource.ResourceClaimSpec{ + Devices: resource.DeviceClaim{ + Requests: []resource.DeviceRequest{ + { + Name: "req-0", + FirstAvailable: []resource.DeviceSubRequest{ + { + Name: "subreq-0", + DeviceClassName: "class", + AllocationMode: resource.DeviceAllocationModeExactCount, + Count: 1, + Tolerations: []resource.DeviceToleration{{Key: "some-key", Operator: resource.DeviceTolerationOpExists}}, + }, + }, + }, + }, + }, + }, + }, +} + var ns1 = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "default", @@ -206,9 +278,10 @@ func TestClaimTemplateStrategyCreate(t *testing.T) { testcases := map[string]struct { obj *resource.ResourceClaimTemplate adminAccess bool - expectValidationError string + deviceTaints bool prioritizedList bool consumableCapacity bool + expectValidationError string expectObj *resource.ResourceClaimTemplate verify func(*testing.T, []testclient.Action) }{ @@ -259,6 +332,48 @@ func TestClaimTemplateStrategyCreate(t *testing.T) { } }, }, + "drop-fields-device-taints": { + obj: objWithDeviceTaints, + deviceTaints: false, + expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-device-taints": { + obj: objWithDeviceTaints, + deviceTaints: true, + expectObj: objWithDeviceTaints, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "drop-fields-device-taints-in-prioritized-list": { + obj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: false, + prioritizedList: true, + expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-device-taints-in-prioritized-list": { + obj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: true, + prioritizedList: true, + expectObj: objWithDeviceTaintsInPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, "drop-fields-prioritized-list": { obj: objWithPrioritizedList, prioritizedList: false, @@ -361,6 +476,7 @@ func TestClaimTemplateStrategyCreate(t *testing.T) { mockNSClient := fakeClient.CoreV1().Namespaces() featuregatetesting.SetFeatureGatesDuringTest(t, utilfeature.DefaultFeatureGate, featuregatetesting.FeatureOverrides{ features.DRAAdminAccess: tc.adminAccess, + features.DRADeviceTaints: tc.deviceTaints, features.DRAPrioritizedList: tc.prioritizedList, }) strategy := NewStrategy(mockNSClient) @@ -450,3 +566,270 @@ func TestClaimTemplateStrategyUpdate(t *testing.T) { } }) } + +func TestStrategyUpdate(t *testing.T) { + ctx := genericapirequest.NewDefaultContext() + testcases := map[string]struct { + oldObj *resource.ResourceClaimTemplate + newObj *resource.ResourceClaimTemplate + adminAccess bool + deviceTaints bool + prioritizedList bool + expectValidationError string + expectObj *resource.ResourceClaimTemplate + verify func(*testing.T, []testclient.Action) + }{ + "no-changes-okay": { + oldObj: obj, + newObj: obj, + expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "name-change-not-allowed": { + oldObj: obj, + newObj: func() *resource.ResourceClaimTemplate { + obj := obj.DeepCopy() + obj.Name += "-2" + return obj + }(), + expectValidationError: fieldImmutableError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "drop-fields-admin-access": { + oldObj: obj, + newObj: objWithAdminAccess, + adminAccess: false, + expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-admin-access": { + oldObj: obj, + newObj: objWithAdminAccess, + adminAccess: true, + expectValidationError: fieldImmutableError, // Spec is immutable. + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-admin-access": { + oldObj: objWithAdminAccess, + newObj: objWithAdminAccess, + adminAccess: true, + expectObj: objWithAdminAccess, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "admin-access-admin-namespace": { + oldObj: objWithAdminAccess, + newObj: objWithAdminAccess, + adminAccess: true, + expectObj: objWithAdminAccess, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "admin-access-non-admin-namespace": { + oldObj: objInNonAdminNamespace, + newObj: objWithAdminAccessInNonAdminNamespace, + adminAccess: true, + expectValidationError: fieldImmutableError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "drop-fields-prioritized-list": { + oldObj: obj, + newObj: objWithPrioritizedList, + prioritizedList: false, + expectValidationError: deviceRequestError, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-prioritized-list": { + oldObj: obj, + newObj: objWithPrioritizedList, + prioritizedList: true, + expectValidationError: fieldImmutableError, // Spec is immutable. + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-prioritized-list": { + oldObj: objWithPrioritizedList, + newObj: objWithPrioritizedList, + prioritizedList: true, + expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-prioritized-list-disabled-feature": { + oldObj: objWithPrioritizedList, + newObj: objWithPrioritizedList, + prioritizedList: false, + expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "drop-fields-device-taints": { + oldObj: obj, + newObj: objWithDeviceTaints, + deviceTaints: false, + prioritizedList: true, + expectObj: obj, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-device-taints": { + oldObj: obj, + newObj: objWithDeviceTaints, + deviceTaints: true, + prioritizedList: true, + expectValidationError: fieldImmutableError, // Spec is immutable, cannot add tolerations. + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-device-taints": { + oldObj: objWithDeviceTaints, + newObj: objWithDeviceTaints, + deviceTaints: true, + prioritizedList: true, + expectObj: objWithDeviceTaints, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-device-taints-disabled-feature": { + oldObj: objWithDeviceTaints, + newObj: objWithDeviceTaints, + deviceTaints: false, + prioritizedList: true, + expectObj: objWithDeviceTaints, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "drop-fields-device-taints-in-prioritized-list": { + oldObj: objWithPrioritizedList, + newObj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: false, + prioritizedList: true, + expectObj: objWithPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-fields-device-taints-in-prioritized-list": { + oldObj: objWithPrioritizedList, + newObj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: true, + prioritizedList: true, + expectValidationError: fieldImmutableError, // Spec is immutable, cannot add tolerations. + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-device-taints-in-prioritized-list": { + oldObj: objWithDeviceTaintsInPrioritizedList, + newObj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: true, + prioritizedList: true, + expectObj: objWithDeviceTaintsInPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + "keep-existing-fields-device-taints-in-prioritized-list-disabled-feature": { + oldObj: objWithDeviceTaintsInPrioritizedList, + newObj: objWithDeviceTaintsInPrioritizedList, + deviceTaints: false, + prioritizedList: true, + expectObj: objWithDeviceTaintsInPrioritizedList, + verify: func(t *testing.T, as []testclient.Action) { + if len(as) != 0 { + t.Errorf("expected no action to be taken") + } + }, + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset(ns1, ns2) + mockNSClient := fakeClient.CoreV1().Namespaces() + + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAAdminAccess, tc.adminAccess) + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRADeviceTaints, tc.deviceTaints) + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DRAPrioritizedList, tc.prioritizedList) + strategy := NewStrategy(mockNSClient) + + oldObj := tc.oldObj.DeepCopy() + newObj := tc.newObj.DeepCopy() + newObj.ResourceVersion = "4" + + strategy.PrepareForUpdate(ctx, newObj, oldObj) + if errs := strategy.ValidateUpdate(ctx, newObj, oldObj); len(errs) != 0 { + assert.ErrorContains(t, errs[0], tc.expectValidationError, "the error message should have contained the expected error message") + return + } + if tc.expectValidationError != "" { + t.Fatal("expected validation error(s), got none") + } + if warnings := strategy.WarningsOnUpdate(ctx, newObj, oldObj); len(warnings) != 0 { + t.Fatalf("unexpected warnings: %q", warnings) + } + strategy.Canonicalize(newObj) + expectObj := tc.expectObj.DeepCopy() + expectObj.ResourceVersion = "4" + assert.Equal(t, expectObj, newObj) + tc.verify(t, fakeClient.Actions()) + }) + } +}