From 836ace46a0e8da2193f9524f10cfa3559c414f7d Mon Sep 17 00:00:00 2001 From: Deepak Kinni Date: Sat, 26 Mar 2022 01:18:17 +0530 Subject: [PATCH] Default enable flag for beta feature HonorPVReclaimPolicy Signed-off-by: Deepak Kinni --- pkg/controller/volume/persistentvolume/delete_test.go | 5 ++--- pkg/controller/volume/persistentvolume/framework_test.go | 1 + pkg/controller/volume/persistentvolume/provision_test.go | 6 +++--- pkg/controller/volume/persistentvolume/pv_controller.go | 6 ++++++ .../volume/persistentvolume/pv_controller_base.go | 2 +- .../volume/persistentvolume/pv_controller_test.go | 2 -- pkg/features/kube_features.go | 2 +- 7 files changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/delete_test.go b/pkg/controller/volume/persistentvolume/delete_test.go index 24d83cfefe8..131ed9b8f45 100644 --- a/pkg/controller/volume/persistentvolume/delete_test.go +++ b/pkg/controller/volume/persistentvolume/delete_test.go @@ -188,11 +188,10 @@ func TestDeleteSync(t *testing.T) { }, }, { - // TODO: Change the expectedVolumes to novolumes after HonorPVReclaimPolicy is enabled by default. // delete success - volume has deletion timestamp before doDelete() starts "8-13 - volume has deletion timestamp and processed", volumesWithFinalizers(withVolumeDeletionTimestamp(newVolumeArray("volume8-13", "1Gi", "uid8-13", "claim8-13", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnBoundByController)), []string{volume.PVDeletionInTreeProtectionFinalizer}), - volumesWithFinalizers(withVolumeDeletionTimestamp(newVolumeArray("volume8-13", "1Gi", "uid8-13", "claim8-13", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnBoundByController)), []string{volume.PVDeletionInTreeProtectionFinalizer}), + novolumes, noclaims, noclaims, noevents, noerrors, @@ -222,7 +221,7 @@ func TestDeleteMultiSync(t *testing.T) { // delete failure - delete returns error. The controller should // try again. "9-1 - delete returns error", - newVolumeArray("volume9-1", "1Gi", "uid9-1", "claim9-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), + volumesWithFinalizers(newVolumeArray("volume9-1", "1Gi", "uid9-1", "claim9-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), []string{volume.PVDeletionInTreeProtectionFinalizer}), novolumes, noclaims, noclaims, diff --git a/pkg/controller/volume/persistentvolume/framework_test.go b/pkg/controller/volume/persistentvolume/framework_test.go index c566510ab01..0018b99d7f3 100644 --- a/pkg/controller/volume/persistentvolume/framework_test.go +++ b/pkg/controller/volume/persistentvolume/framework_test.go @@ -254,6 +254,7 @@ func newVolume(name, capacity, boundToClaimUID, boundToClaimName string, phase v ObjectMeta: metav1.ObjectMeta{ Name: name, ResourceVersion: "1", + Finalizers: []string{storagehelpers.PVDeletionInTreeProtectionFinalizer}, }, Spec: v1.PersistentVolumeSpec{ Capacity: v1.ResourceList{ diff --git a/pkg/controller/volume/persistentvolume/provision_test.go b/pkg/controller/volume/persistentvolume/provision_test.go index e10d631b68c..259260c404a 100644 --- a/pkg/controller/volume/persistentvolume/provision_test.go +++ b/pkg/controller/volume/persistentvolume/provision_test.go @@ -172,7 +172,7 @@ func TestProvisionSync(t *testing.T) { // Provision a volume (with a default class) "11-1 - successful provision with storage class 1", novolumes, - newVolumeArray("pvc-uid11-1", "1Gi", "uid11-1", "claim11-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classGold, volume.AnnBoundByController, volume.AnnDynamicallyProvisioned), + volumesWithFinalizers(newVolumeArray("pvc-uid11-1", "1Gi", "uid11-1", "claim11-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classGold, volume.AnnBoundByController, volume.AnnDynamicallyProvisioned), []string{volume.PVDeletionInTreeProtectionFinalizer}), newClaimArray("claim11-1", "uid11-1", "1Gi", "", v1.ClaimPending, &classGold), // Binding will be completed in the next syncClaim newClaimArray("claim11-1", "uid11-1", "1Gi", "", v1.ClaimPending, &classGold, volume.AnnStorageProvisioner, volume.AnnBetaStorageProvisioner), @@ -242,7 +242,7 @@ func TestProvisionSync(t *testing.T) { // second retry succeeds "11-8 - cannot save provisioned volume", novolumes, - newVolumeArray("pvc-uid11-8", "1Gi", "uid11-8", "claim11-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classGold, volume.AnnBoundByController, volume.AnnDynamicallyProvisioned), + volumesWithFinalizers(newVolumeArray("pvc-uid11-8", "1Gi", "uid11-8", "claim11-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classGold, volume.AnnBoundByController, volume.AnnDynamicallyProvisioned), []string{volume.PVDeletionInTreeProtectionFinalizer}), newClaimArray("claim11-8", "uid11-8", "1Gi", "", v1.ClaimPending, &classGold), // Binding will be completed in the next syncClaim newClaimArray("claim11-8", "uid11-8", "1Gi", "", v1.ClaimPending, &classGold, volume.AnnStorageProvisioner, volume.AnnBetaStorageProvisioner), @@ -363,7 +363,7 @@ func TestProvisionSync(t *testing.T) { // Provision a volume (with non-default class) "11-13 - successful provision with storage class 2", novolumes, - newVolumeArray("pvc-uid11-13", "1Gi", "uid11-13", "claim11-13", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classSilver, volume.AnnBoundByController, volume.AnnDynamicallyProvisioned), + volumesWithFinalizers(newVolumeArray("pvc-uid11-13", "1Gi", "uid11-13", "claim11-13", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classSilver, volume.AnnBoundByController, volume.AnnDynamicallyProvisioned), []string{volume.PVDeletionInTreeProtectionFinalizer}), newClaimArray("claim11-13", "uid11-13", "1Gi", "", v1.ClaimPending, &classSilver), // Binding will be completed in the next syncClaim newClaimArray("claim11-13", "uid11-13", "1Gi", "", v1.ClaimPending, &classSilver, volume.AnnStorageProvisioner, volume.AnnBetaStorageProvisioner), diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index 69599f8b207..b0f1897b4d3 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/pkg/controller/volume/persistentvolume/pv_controller.go @@ -1458,6 +1458,12 @@ func (ctrl *PersistentVolumeController) doDeleteVolume(volume *v1.PersistentVolu func (ctrl *PersistentVolumeController) removeDeletionProtectionFinalizer(ctx context.Context, volume *v1.PersistentVolume) error { var err error pvUpdateNeeded := false + // Retrieve latest version + volume, err = ctrl.kubeClient.CoreV1().PersistentVolumes().Get(context.TODO(), volume.Name, metav1.GetOptions{}) + if err != nil { + klog.Errorf("error reading persistent volume %q: %v", volume.Name, err) + return err + } volumeClone := volume.DeepCopy() pvFinalizers := volumeClone.Finalizers if pvFinalizers != nil && slice.ContainsString(pvFinalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) { diff --git a/pkg/controller/volume/persistentvolume/pv_controller_base.go b/pkg/controller/volume/persistentvolume/pv_controller_base.go index 6fbfa0bc9ba..eddc7b9c8dd 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_base.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_base.go @@ -434,7 +434,7 @@ func updateMigrationAnnotationsAndFinalizers(cmpm CSIMigratedPluginManager, tran if !claim && utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { modified := false if finalizers == nil { - *finalizers = []string{} + finalizers = &([]string{}) } // Add back the in-tree PV deletion protection finalizer if does not already exists if !slice.ContainsString(*finalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) { diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index da3650b3bdc..73c5d84cc42 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -52,7 +52,6 @@ import ( // can't reliably simulate periodic sync of volumes/claims - it would be // either very timing-sensitive or slow to wait for real periodic sync. func TestControllerSync(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HonorPVReclaimPolicy, true)() tests := []controllerTest{ // [Unit test set 5] - controller tests. // We test the controller as if @@ -612,7 +611,6 @@ func TestUpdateFinalizer(t *testing.T) { // This set of tests ensures that protection finalizer is removed when CSI migration is disabled // and PV controller needs to remove finalizers added by the external-provisioner. defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIMigrationGCE, false)() - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HonorPVReclaimPolicy, true)() const gcePlugin = "kubernetes.io/gce-pd" const gceDriver = "pd.csi.storage.gke.io" const customFinalizer = "test.volume.kubernetes.io/finalizer" diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 28fba47802a..def0f6fbe5f 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -958,7 +958,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS JobMutableNodeSchedulingDirectives: {Default: true, PreRelease: featuregate.Beta}, IdentifyPodOS: {Default: true, PreRelease: featuregate.Beta}, PodAndContainerStatsFromCRI: {Default: false, PreRelease: featuregate.Alpha}, - HonorPVReclaimPolicy: {Default: false, PreRelease: featuregate.Beta}, + HonorPVReclaimPolicy: {Default: true, PreRelease: featuregate.Beta}, RecoverVolumeExpansionFailure: {Default: false, PreRelease: featuregate.Alpha}, GRPCContainerProbe: {Default: true, PreRelease: featuregate.Beta}, LegacyServiceAccountTokenNoAutoGeneration: {Default: true, PreRelease: featuregate.Beta},