From 137474e2839547c3f3599b778cdb564c81231a22 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Fri, 14 Jul 2023 12:04:27 -0400 Subject: [PATCH] Fix validation options for old pvc Also update comments on allocatedresourcestatuses fields --- pkg/apis/core/helper/helpers.go | 7 +++++ pkg/apis/core/types.go | 30 ++++++++++++------ pkg/apis/core/validation/validation.go | 4 +++ pkg/apis/core/validation/validation_test.go | 35 ++++++++++----------- staging/src/k8s.io/api/core/v1/types.go | 32 +++++++++++++------ 5 files changed, 70 insertions(+), 38 deletions(-) diff --git a/pkg/apis/core/helper/helpers.go b/pkg/apis/core/helper/helpers.go index 77464b25766..85fbc061610 100644 --- a/pkg/apis/core/helper/helpers.go +++ b/pkg/apis/core/helper/helpers.go @@ -360,6 +360,13 @@ func ContainsAccessMode(modes []core.PersistentVolumeAccessMode, mode core.Persi return false } +func ClaimContainsAllocatedResources(pvc *core.PersistentVolumeClaim) bool { + if pvc.Status.AllocatedResourceStatuses != nil || pvc.Status.AllocatedResources != nil { + return true + } + return false +} + // GetTolerationsFromPodAnnotations gets the json serialized tolerations data from Pod.Annotations // and converts it to the []Toleration type in core. func GetTolerationsFromPodAnnotations(annotations map[string]string) ([]core.Toleration, error) { diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 5b878816723..3566b454c97 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -521,7 +521,7 @@ const ( type ClaimResourceStatus string const ( - // State set when resize controller starts expanding the volume in control-plane + // State set when resize controller starts resizing the volume in control-plane PersistentVolumeClaimControllerResizeInProgress ClaimResourceStatus = "ControllerResizeInProgress" // State set when resize has failed in resize controller with a terminal error. @@ -566,11 +566,13 @@ type PersistentVolumeClaimStatus struct { // +optional Conditions []PersistentVolumeClaimCondition // AllocatedResources tracks the resources allocated to a PVC including its capacity. - // Following are valid key names for allocatedResources: - // - storage - // - example.com/foobar + // Key names follow standard Kubernetes label syntax. Valid values are either: + // * Un-prefixed keys: + // - storage - the capacity of the volume. + // * Custom resources must use implementation-defined prefixed names such as "example.com/my-custom-resource" // Apart from above values - keys that are unprefixed or have kubernetes.io prefix are considered // reserved and hence may not be used. + // // Capacity reported here may be larger than the actual capacity when a volume expansion operation // is requested. // For storage quota, the larger value from allocatedResources and PVC.spec.resources is used. @@ -578,19 +580,27 @@ type PersistentVolumeClaimStatus struct { // If a volume expansion capacity request is lowered, allocatedResources is only // lowered if there are no expansion operations in progress and if the actual volume capacity // is equal or lower than the requested capacity. + // + // A controller that receives PVC update with previously unknown resourceName + // should ignore the update for the purpose it was designed. For example - a controller that + // only is responsible for resizing capacity of the volume, should ignore PVC updates that change other valid + // resources associated with PVC. + // // This is an alpha field and requires enabling RecoverVolumeExpansionFailure feature. // +featureGate=RecoverVolumeExpansionFailure // +optional AllocatedResources ResourceList // AllocatedResourceStatuses stores status of resource being resized for the given PVC. - // Following are valid key names for allocatedResourceStatuses: - // - storage - // - example.com/foobar + // Key names follow standard Kubernetes label syntax. Valid values are either: + // * Un-prefixed keys: + // - storage - the capacity of the volume. + // * Custom resources must use implementation-defined prefixed names such as "example.com/my-custom-resource" // Apart from above values - keys that are unprefixed or have kubernetes.io prefix are considered // reserved and hence may not be used. + // // ClaimResourceStatus can be in any of following states: // - ControllerResizeInProgress: - // State set when resize controller starts expanding the volume in control-plane. + // State set when resize controller starts resizing the volume in control-plane. // - ControllerResizeFailed: // State set when resize has failed in resize controller with a terminal error. // - NodeResizePending: @@ -608,10 +618,12 @@ type PersistentVolumeClaimStatus struct { // - pvc.status.allocatedResourceStatus['storage'] = "NodeResizeInProgress" // - pvc.status.allocatedResourceStatus['storage'] = "NodeResizeFailed" // When this field is not set, it means that no resize operation is in progress for the given PVC. + // // A controller that receives PVC update with previously unknown resourceName or ClaimResourceStatus // should ignore the update for the purpose it was designed. For example - a controller that - // only is responsible for resizing capacity of the volume, should ignore pvc updates that change other valid + // only is responsible for resizing capacity of the volume, should ignore PVC updates that change other valid // resources associated with PVC. + // // This is an alpha field and requires enabling RecoverVolumeExpansionFailure feature. // +featureGate=RecoverVolumeExpansionFailure // +mapType=granular diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 2a4d97b9d17..ded0900c11f 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2046,6 +2046,10 @@ func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolum // If the old object allowed "ReadWriteOncePod", continue to allow it in the new object opts.AllowReadWriteOncePod = true } + + if helper.ClaimContainsAllocatedResources(oldPvc) { + opts.EnableRecoverFromExpansionFailure = true + } return opts } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index b21009c14ca..bd12b18e0c6 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -18450,34 +18450,29 @@ func TestValidatePersistentVolumeClaimStatusUpdate(t *testing.T) { isExpectedFailure bool oldClaim *core.PersistentVolumeClaim newClaim *core.PersistentVolumeClaim - enableResize bool enableRecoverFromExpansion bool }{ "condition-update-with-enabled-feature-gate": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validConditionUpdate, - enableResize: true, }, "status-update-with-valid-allocatedResources-feature-enabled": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validAllocatedResources, - enableResize: true, enableRecoverFromExpansion: true, }, "status-update-with-invalid-allocatedResources-native-key-feature-enabled": { isExpectedFailure: true, oldClaim: validClaim, newClaim: invalidNativeResourceAllocatedKey, - enableResize: true, enableRecoverFromExpansion: true, }, "status-update-with-valid-allocatedResources-external-key-feature-enabled": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validExternalAllocatedResource, - enableResize: true, enableRecoverFromExpansion: true, }, @@ -18485,85 +18480,87 @@ func TestValidatePersistentVolumeClaimStatusUpdate(t *testing.T) { isExpectedFailure: true, oldClaim: validClaim, newClaim: invalidAllocatedResources, - enableResize: true, enableRecoverFromExpansion: true, }, "status-update-with-no-storage-update": { isExpectedFailure: true, oldClaim: validClaim, newClaim: noStoraegeClaimStatus, - enableResize: true, enableRecoverFromExpansion: true, }, "staus-update-with-controller-resize-failed": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validResizeStatusControllerResizeFailed, - enableResize: true, enableRecoverFromExpansion: true, }, "staus-update-with-node-resize-pending": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validNodeResizePending, - enableResize: true, enableRecoverFromExpansion: true, }, "staus-update-with-node-resize-inprogress": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validNodeResizeInProgress, - enableResize: true, enableRecoverFromExpansion: true, }, "staus-update-with-node-resize-failed": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validNodeResizeFailed, - enableResize: true, enableRecoverFromExpansion: true, }, "staus-update-with-invalid-native-resource-status-key": { isExpectedFailure: true, oldClaim: validClaim, newClaim: invalidNativeResizeStatusPVC, - enableResize: true, enableRecoverFromExpansion: true, }, "staus-update-with-valid-external-resource-status-key": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validExternalResizeStatusPVC, - enableResize: true, enableRecoverFromExpansion: true, }, "status-update-with-multiple-resources-key": { isExpectedFailure: false, oldClaim: validClaim, newClaim: multipleResourceStatusPVC, - enableResize: true, enableRecoverFromExpansion: true, }, "status-update-with-valid-pvc-resize-status": { isExpectedFailure: false, oldClaim: validClaim, newClaim: validResizeStatusPVC, - enableResize: true, enableRecoverFromExpansion: true, }, "status-update-with-invalid-pvc-resize-status": { isExpectedFailure: true, oldClaim: validClaim, newClaim: invalidResizeStatusPVC, - enableResize: true, enableRecoverFromExpansion: true, }, + "status-update-with-old-pvc-valid-resourcestatus-newpvc-invalid-recovery-disabled": { + isExpectedFailure: true, + oldClaim: validResizeStatusPVC, + newClaim: invalidResizeStatusPVC, + enableRecoverFromExpansion: false, + }, + "status-update-with-old-pvc-valid-allocatedResource-newpvc-invalid-recovery-disabled": { + isExpectedFailure: true, + oldClaim: validExternalAllocatedResource, + newClaim: invalidNativeResourceAllocatedKey, + enableRecoverFromExpansion: false, + }, } for name, scenario := range scenarios { t.Run(name, func(t *testing.T) { - validateOpts := PersistentVolumeClaimSpecValidationOptions{ - EnableRecoverFromExpansionFailure: scenario.enableRecoverFromExpansion, - } + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecoverVolumeExpansionFailure, scenario.enableRecoverFromExpansion)() + + validateOpts := ValidationOptionsForPersistentVolumeClaim(scenario.newClaim, scenario.oldClaim) + // ensure we have a resource version specified for updates scenario.oldClaim.ResourceVersion = "1" scenario.newClaim.ResourceVersion = "1" diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index 1f2332c2aee..a2422820d1d 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -564,7 +564,7 @@ const ( type ClaimResourceStatus string const ( - // State set when resize controller starts expanding the volume in control-plane + // State set when resize controller starts resizing the volume in control-plane. PersistentVolumeClaimControllerResizeInProgress ClaimResourceStatus = "ControllerResizeInProgress" // State set when resize has failed in resize controller with a terminal error. @@ -620,11 +620,13 @@ type PersistentVolumeClaimStatus struct { // +patchStrategy=merge Conditions []PersistentVolumeClaimCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,4,rep,name=conditions"` // allocatedResources tracks the resources allocated to a PVC including its capacity. - // Following are valid key names for allocatedResources: - // - storage - // - example.com/foobar + // Key names follow standard Kubernetes label syntax. Valid values are either: + // * Un-prefixed keys: + // - storage - the capacity of the volume. + // * Custom resources must use implementation-defined prefixed names such as "example.com/my-custom-resource" // Apart from above values - keys that are unprefixed or have kubernetes.io prefix are considered // reserved and hence may not be used. + // // Capacity reported here may be larger than the actual capacity when a volume expansion operation // is requested. // For storage quota, the larger value from allocatedResources and PVC.spec.resources is used. @@ -632,6 +634,12 @@ type PersistentVolumeClaimStatus struct { // If a volume expansion capacity request is lowered, allocatedResources is only // lowered if there are no expansion operations in progress and if the actual volume capacity // is equal or lower than the requested capacity. + // + // A controller that receives PVC update with previously unknown resourceName + // should ignore the update for the purpose it was designed. For example - a controller that + // only is responsible for resizing capacity of the volume, should ignore PVC updates that change other valid + // resources associated with PVC. + // // This is an alpha field and requires enabling RecoverVolumeExpansionFailure feature. // +featureGate=RecoverVolumeExpansionFailure // +optional @@ -641,14 +649,16 @@ type PersistentVolumeClaimStatus struct { // ResizeStatus *PersistentVolumeClaimResizeStatus `json:"resizeStatus,omitempty" protobuf:"bytes,6,opt,name=resizeStatus,casttype=PersistentVolumeClaimResizeStatus"` // allocatedResourceStatuses stores status of resource being resized for the given PVC. - // Following are valid key names for allocatedResourceStatuses: - // - storage - // - example.com/foobar + // Key names follow standard Kubernetes label syntax. Valid values are either: + // * Un-prefixed keys: + // - storage - the capacity of the volume. + // * Custom resources must use implementation-defined prefixed names such as "example.com/my-custom-resource" // Apart from above values - keys that are unprefixed or have kubernetes.io prefix are considered // reserved and hence may not be used. + // // ClaimResourceStatus can be in any of following states: // - ControllerResizeInProgress: - // State set when resize controller starts expanding the volume in control-plane. + // State set when resize controller starts resizing the volume in control-plane. // - ControllerResizeFailed: // State set when resize has failed in resize controller with a terminal error. // - NodeResizePending: @@ -659,17 +669,19 @@ type PersistentVolumeClaimStatus struct { // - NodeResizeFailed: // State set when resizing has failed in kubelet with a terminal error. Transient errors don't set // NodeResizeFailed. - // For example expanding a PVC for more capacity - this field can be one of the following states: + // For example: if expanding a PVC for more capacity - this field can be one of the following states: // - pvc.status.allocatedResourceStatus['storage'] = "ControllerResizeInProgress" // - pvc.status.allocatedResourceStatus['storage'] = "ControllerResizeFailed" // - pvc.status.allocatedResourceStatus['storage'] = "NodeResizePending" // - pvc.status.allocatedResourceStatus['storage'] = "NodeResizeInProgress" // - pvc.status.allocatedResourceStatus['storage'] = "NodeResizeFailed" // When this field is not set, it means that no resize operation is in progress for the given PVC. + // // A controller that receives PVC update with previously unknown resourceName or ClaimResourceStatus // should ignore the update for the purpose it was designed. For example - a controller that - // only is responsible for resizing capacity of the volume, should ignore pvc updates that change other valid + // only is responsible for resizing capacity of the volume, should ignore PVC updates that change other valid // resources associated with PVC. + // // This is an alpha field and requires enabling RecoverVolumeExpansionFailure feature. // +featureGate=RecoverVolumeExpansionFailure // +mapType=granular