From f32302e744e294cd3b61af4f7b5fe1529e42e330 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 21 Feb 2023 15:36:55 +0100 Subject: [PATCH 1/2] api: drop Resources.Claims from PVC and PVC template PVC and containers share the same ResourceRequirements struct. The Claims field in it only makes sense when used in containers. When used in a PVC, the field should have been rejected by validation. This was overlooked when introducing it, so now persisted objects might have it set and/or people may have started to rely on it being accepted even when it has no effect. Therefore we cannot reject it in validation anymore, but we can still strip it out on create or update. --- pkg/api/persistentvolumeclaim/util.go | 4 +++ pkg/api/persistentvolumeclaim/util_test.go | 6 ++++ pkg/api/pod/util.go | 10 ++++++ pkg/api/pod/util_test.go | 38 ++++++++++++++++++++++ pkg/apis/core/types.go | 2 +- staging/src/k8s.io/api/core/v1/types.go | 2 +- 6 files changed, 60 insertions(+), 2 deletions(-) diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go index 4334afcca59..1304c1fe048 100644 --- a/pkg/api/persistentvolumeclaim/util.go +++ b/pkg/api/persistentvolumeclaim/util.go @@ -49,6 +49,10 @@ func DropDisabledFields(pvcSpec, oldPVCSpec *core.PersistentVolumeClaimSpec) { pvcSpec.DataSourceRef = nil } } + + // Setting VolumeClaimTemplate.Resources.Claims should have been caught by validation when + // extending ResourceRequirements in 1.26. Now we can only accept it and drop the field. + pvcSpec.Resources.Claims = nil } // EnforceDataSourceBackwardsCompatibility drops the data source field under certain conditions diff --git a/pkg/api/persistentvolumeclaim/util_test.go b/pkg/api/persistentvolumeclaim/util_test.go index 945ff2f0446..ebbb0fecb5c 100644 --- a/pkg/api/persistentvolumeclaim/util_test.go +++ b/pkg/api/persistentvolumeclaim/util_test.go @@ -263,6 +263,9 @@ func TestDataSourceFilter(t *testing.T) { anyEnabled: true, wantRef: xnsVolumeDataSourceRef, // existing field isn't dropped. }, + "clear Resources.Claims": { + spec: core.PersistentVolumeClaimSpec{Resources: core.ResourceRequirements{Claims: []core.ResourceClaim{{Name: "dra"}}}}, + }, } for testName, test := range tests { @@ -278,6 +281,9 @@ func TestDataSourceFilter(t *testing.T) { t.Errorf("expected condition was not met, test: %s, anyEnabled: %v, xnsEnabled: %v, spec: %+v, expected DataSourceRef: %+v", testName, test.anyEnabled, test.xnsEnabled, test.spec, test.wantRef) } + if test.spec.Resources.Claims != nil { + t.Errorf("expected Resources.Claims to be cleared") + } }) } } diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 9cc7aeb1156..95d9f876b77 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -577,6 +577,16 @@ func dropDisabledDynamicResourceAllocationFields(podSpec, oldPodSpec *api.PodSpe dropEphemeralResourceClaimRequests(podSpec.EphemeralContainers) podSpec.ResourceClaims = nil } + + // Setting VolumeClaimTemplate.Resources.Claims should have been + // treated as an error by validation when extending + // ResourceRequirements in 1.26. Now we can only accept it and drop the + // field. + for i := range podSpec.Volumes { + if podSpec.Volumes[i].Ephemeral != nil && podSpec.Volumes[i].Ephemeral.VolumeClaimTemplate != nil { + podSpec.Volumes[i].Ephemeral.VolumeClaimTemplate.Spec.Resources.Claims = nil + } + } } func dynamicResourceAllocationInUse(podSpec *api.PodSpec) bool { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 17ae3fe78ad..5def1ade4d8 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -2236,3 +2236,41 @@ func TestValidateTopologySpreadConstraintLabelSelectorOption(t *testing.T) { }) } } + +func TestDropVolumesClaimField(t *testing.T) { + pod := &api.Pod{ + Spec: api.PodSpec{ + Volumes: []api.Volume{ + {}, + { + VolumeSource: api.VolumeSource{ + Ephemeral: &api.EphemeralVolumeSource{}, + }, + }, + { + VolumeSource: api.VolumeSource{ + Ephemeral: &api.EphemeralVolumeSource{ + VolumeClaimTemplate: &api.PersistentVolumeClaimTemplate{ + Spec: api.PersistentVolumeClaimSpec{ + Resources: api.ResourceRequirements{ + Claims: []api.ResourceClaim{ + {Name: "dra"}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + DropDisabledPodFields(pod, nil) + + for i, volume := range pod.Spec.Volumes { + if volume.Ephemeral != nil && volume.Ephemeral.VolumeClaimTemplate != nil && volume.Ephemeral.VolumeClaimTemplate.Spec.Resources.Claims != nil { + t.Errorf("volume #%d: Resources.Claim should be nil", i) + } + } +} diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 45514929eba..7b0ea2fb247 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -2190,7 +2190,7 @@ type ResourceRequirements struct { // This is an alpha field and requires enabling the // DynamicResourceAllocation feature gate. // - // This field is immutable. + // This field is immutable. It can only be set for containers. // // +featureGate=DynamicResourceAllocation // +optional diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index b03840ce62e..63d92f2e5c1 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -2319,7 +2319,7 @@ type ResourceRequirements struct { // This is an alpha field and requires enabling the // DynamicResourceAllocation feature gate. // - // This field is immutable. + // This field is immutable. It can only be set for containers. // // +listType=map // +listMapKey=name From 093469fb30cded9c56f94a66e4914ed24456256a Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 21 Feb 2023 16:13:34 +0100 Subject: [PATCH 2/2] api: generated files --- api/openapi-spec/swagger.json | 2 +- api/openapi-spec/v3/api__v1_openapi.json | 2 +- api/openapi-spec/v3/apis__apps__v1_openapi.json | 2 +- api/openapi-spec/v3/apis__batch__v1_openapi.json | 2 +- pkg/generated/openapi/zz_generated.openapi.go | 2 +- staging/src/k8s.io/api/core/v1/generated.proto | 2 +- staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index b9956067643..e2f2920dad4 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -8643,7 +8643,7 @@ "description": "ResourceRequirements describes the compute resource requirements.", "properties": { "claims": { - "description": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable.", + "description": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable. It can only be set for containers.", "items": { "$ref": "#/definitions/io.k8s.api.core.v1.ResourceClaim" }, diff --git a/api/openapi-spec/v3/api__v1_openapi.json b/api/openapi-spec/v3/api__v1_openapi.json index b09341bb9bb..1806f8e0a1d 100644 --- a/api/openapi-spec/v3/api__v1_openapi.json +++ b/api/openapi-spec/v3/api__v1_openapi.json @@ -6239,7 +6239,7 @@ "description": "ResourceRequirements describes the compute resource requirements.", "properties": { "claims": { - "description": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable.", + "description": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable. It can only be set for containers.", "items": { "allOf": [ { diff --git a/api/openapi-spec/v3/apis__apps__v1_openapi.json b/api/openapi-spec/v3/apis__apps__v1_openapi.json index b1f8f6fa4dd..2a9aa1384f6 100644 --- a/api/openapi-spec/v3/apis__apps__v1_openapi.json +++ b/api/openapi-spec/v3/apis__apps__v1_openapi.json @@ -4047,7 +4047,7 @@ "description": "ResourceRequirements describes the compute resource requirements.", "properties": { "claims": { - "description": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable.", + "description": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable. It can only be set for containers.", "items": { "allOf": [ { diff --git a/api/openapi-spec/v3/apis__batch__v1_openapi.json b/api/openapi-spec/v3/apis__batch__v1_openapi.json index 510e2e09934..0e6dd26b2e7 100644 --- a/api/openapi-spec/v3/apis__batch__v1_openapi.json +++ b/api/openapi-spec/v3/apis__batch__v1_openapi.json @@ -3221,7 +3221,7 @@ "description": "ResourceRequirements describes the compute resource requirements.", "properties": { "claims": { - "description": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable.", + "description": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable. It can only be set for containers.", "items": { "allOf": [ { diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index 1fb6906bb93..c89c8ccefa0 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -25092,7 +25092,7 @@ func schema_k8sio_api_core_v1_ResourceRequirements(ref common.ReferenceCallback) }, }, SchemaProps: spec.SchemaProps{ - Description: "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable.", + Description: "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable. It can only be set for containers.", Type: []string{"array"}, Items: &spec.SchemaOrArray{ Schema: &spec.Schema{ diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index e8b17cd878e..3e80ed5f73d 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -4512,7 +4512,7 @@ message ResourceRequirements { // This is an alpha field and requires enabling the // DynamicResourceAllocation feature gate. // - // This field is immutable. + // This field is immutable. It can only be set for containers. // // +listType=map // +listMapKey=name diff --git a/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go b/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go index 5a2a7c47636..13b2f5bbda2 100644 --- a/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go +++ b/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go @@ -2041,7 +2041,7 @@ var map_ResourceRequirements = map[string]string{ "": "ResourceRequirements describes the compute resource requirements.", "limits": "Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", "requests": "Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", - "claims": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable.", + "claims": "Claims lists the names of resources, defined in spec.resourceClaims, that are used by this container.\n\nThis is an alpha field and requires enabling the DynamicResourceAllocation feature gate.\n\nThis field is immutable. It can only be set for containers.", } func (ResourceRequirements) SwaggerDoc() map[string]string {