From a7b1fcba40784af64c949b28e7a2ca03433a3a75 Mon Sep 17 00:00:00 2001 From: Deepak Kinni Date: Thu, 31 Mar 2022 21:16:53 +0530 Subject: [PATCH] Add or Remove PV deletion protection finalizer considering the recalimPolicy Signed-off-by: Deepak Kinni --- .../volume/persistentvolume/delete_test.go | 18 ++- .../volume/persistentvolume/framework_test.go | 67 +++++++++- .../volume/persistentvolume/provision_test.go | 9 +- .../volume/persistentvolume/pv_controller.go | 9 +- .../persistentvolume/pv_controller_base.go | 118 +++++++++++------- .../persistentvolume/pv_controller_test.go | 118 +++++++++--------- 6 files changed, 226 insertions(+), 113 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/delete_test.go b/pkg/controller/volume/persistentvolume/delete_test.go index 131ed9b8f45..4b4b55695e8 100644 --- a/pkg/controller/volume/persistentvolume/delete_test.go +++ b/pkg/controller/volume/persistentvolume/delete_test.go @@ -18,6 +18,9 @@ package persistentvolume import ( "errors" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/kubernetes/pkg/features" "testing" v1 "k8s.io/api/core/v1" @@ -31,6 +34,9 @@ import ( // 2. Call the syncVolume *once*. // 3. Compare resulting volumes with expected volumes. func TestDeleteSync(t *testing.T) { + const gceDriver = "pd.csi.storage.gke.io" + // Default enable the HonorPVReclaimPolicy feature gate. + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HonorPVReclaimPolicy, true)() tests := []controllerTest{ { // delete volume bound by controller @@ -132,8 +138,8 @@ func TestDeleteSync(t *testing.T) { { // PV requires external deleter "8-10 - external deleter", - newVolumeArray("volume8-10", "1Gi", "uid10-1", "claim10-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnBoundByController), - newVolumeArray("volume8-10", "1Gi", "uid10-1", "claim10-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnBoundByController), + []*v1.PersistentVolume{newExternalProvisionedVolume("volume8-10", "1Gi", "uid10-1", "claim10-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, gceDriver, nil, volume.AnnBoundByController)}, + []*v1.PersistentVolume{newExternalProvisionedVolume("volume8-10", "1Gi", "uid10-1", "claim10-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classEmpty, gceDriver, nil, volume.AnnBoundByController)}, noclaims, noclaims, noevents, noerrors, @@ -169,12 +175,12 @@ func TestDeleteSync(t *testing.T) { // external provisioner. "8-12 - two PVs externally provisioned for a single claim", []*v1.PersistentVolume{ - newVolume("volume8-12-1", "1Gi", "uid8-12", "claim8-12", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnDynamicallyProvisioned), - newVolume("volume8-12-2", "1Gi", "uid8-12", "claim8-12", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnDynamicallyProvisioned), + newExternalProvisionedVolume("volume8-12-1", "1Gi", "uid8-12", "claim8-12", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, gceDriver, nil, volume.AnnDynamicallyProvisioned), + newExternalProvisionedVolume("volume8-12-2", "1Gi", "uid8-12", "claim8-12", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, gceDriver, nil, volume.AnnDynamicallyProvisioned), }, []*v1.PersistentVolume{ - newVolume("volume8-12-1", "1Gi", "uid8-12", "claim8-12", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnDynamicallyProvisioned), - newVolume("volume8-12-2", "1Gi", "uid8-12", "claim8-12", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnDynamicallyProvisioned), + newExternalProvisionedVolume("volume8-12-1", "1Gi", "uid8-12", "claim8-12", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classEmpty, gceDriver, nil, volume.AnnDynamicallyProvisioned), + newExternalProvisionedVolume("volume8-12-2", "1Gi", "uid8-12", "claim8-12", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty, gceDriver, nil, volume.AnnDynamicallyProvisioned), }, // the claim is bound to volume8-12-2 -> volume8-12-1 has lost the race and will be "Released" newClaimArray("claim8-12", "uid8-12", "10Gi", "volume8-12-2", v1.ClaimBound, nil), diff --git a/pkg/controller/volume/persistentvolume/framework_test.go b/pkg/controller/volume/persistentvolume/framework_test.go index 0018b99d7f3..feca867f1b6 100644 --- a/pkg/controller/volume/persistentvolume/framework_test.go +++ b/pkg/controller/volume/persistentvolume/framework_test.go @@ -254,7 +254,6 @@ 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{ @@ -298,6 +297,72 @@ func newVolume(name, capacity, boundToClaimUID, boundToClaimName string, phase v return &volume } +// newExternalProvisionedVolume returns a new volume with given attributes +func newExternalProvisionedVolume(name, capacity, boundToClaimUID, boundToClaimName string, phase v1.PersistentVolumePhase, reclaimPolicy v1.PersistentVolumeReclaimPolicy, class string, driverName string, finalizers []string, annotations ...string) *v1.PersistentVolume { + fs := v1.PersistentVolumeFilesystem + volume := v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + ResourceVersion: "1", + Finalizers: finalizers, + }, + Spec: v1.PersistentVolumeSpec{ + Capacity: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(capacity), + }, + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + Driver: driverName, + VolumeHandle: "527b55dc-c7db-4574-9226-2e33318b06a3", + ReadOnly: false, + FSType: "ext4", + VolumeAttributes: map[string]string{ + "Test-Key": "Test-Value", + }, + }, + }, + AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce, v1.ReadOnlyMany}, + PersistentVolumeReclaimPolicy: reclaimPolicy, + StorageClassName: class, + VolumeMode: &fs, + }, + Status: v1.PersistentVolumeStatus{ + Phase: phase, + }, + } + + if boundToClaimName != "" { + volume.Spec.ClaimRef = &v1.ObjectReference{ + Kind: "PersistentVolumeClaim", + APIVersion: "v1", + UID: types.UID(boundToClaimUID), + Namespace: testNamespace, + Name: boundToClaimName, + } + } + + if len(annotations) > 0 { + volume.Annotations = make(map[string]string) + for _, a := range annotations { + switch a { + case storagehelpers.AnnDynamicallyProvisioned: + volume.Annotations[a] = driverName + default: + volume.Annotations[a] = "yes" + } + } + } + + return &volume +} + +// newVolume returns a new volume with given attributes +func newVolumeWithFinalizers(name, capacity, boundToClaimUID, boundToClaimName string, phase v1.PersistentVolumePhase, reclaimPolicy v1.PersistentVolumeReclaimPolicy, class string, finalizers []string, annotations ...string) *v1.PersistentVolume { + retVolume := newVolume(name, capacity, boundToClaimUID, boundToClaimName, phase, reclaimPolicy, class, annotations...) + retVolume.SetFinalizers(finalizers) + return retVolume +} + // withLabels applies the given labels to the first volume in the array and // returns the array. Meant to be used to compose volumes specified inline in // a test. diff --git a/pkg/controller/volume/persistentvolume/provision_test.go b/pkg/controller/volume/persistentvolume/provision_test.go index 259260c404a..1ba073ed18b 100644 --- a/pkg/controller/volume/persistentvolume/provision_test.go +++ b/pkg/controller/volume/persistentvolume/provision_test.go @@ -19,6 +19,9 @@ package persistentvolume import ( "context" "errors" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/kubernetes/pkg/features" "testing" v1 "k8s.io/api/core/v1" @@ -167,6 +170,8 @@ var provision2Success = provisionCall{ // 2. Call the syncVolume *once*. // 3. Compare resulting volumes with expected volumes. func TestProvisionSync(t *testing.T) { + // Default enable the HonorPVReclaimPolicy feature gate. + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HonorPVReclaimPolicy, true)() tests := []controllerTest{ { // Provision a volume (with a default class) @@ -493,8 +498,8 @@ func TestProvisionSync(t *testing.T) { "11-23 - skip finding PV and provision for PVC annotated with AnnSelectedNode", newVolumeArray("volume11-23", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classCopper), []*v1.PersistentVolume{ - newVolume("volume11-23", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classCopper), - newVolume("pvc-uid11-23", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), + newVolumeWithFinalizers("volume11-23", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classCopper, nil /*No Finalizer is added here since the test doesn't trigger syncVolume, instead just syncClaim*/), + newVolumeWithFinalizers("pvc-uid11-23", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, []string{volume.PVDeletionInTreeProtectionFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), }, claimWithAnnotation(volume.AnnSelectedNode, "node1", newClaimArray("claim11-23", "uid11-23", "1Gi", "", v1.ClaimPending, &classCopper)), diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index b0f1897b4d3..b5ca21856f2 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/pkg/controller/volume/persistentvolume/pv_controller.go @@ -1459,11 +1459,12 @@ func (ctrl *PersistentVolumeController) removeDeletionProtectionFinalizer(ctx co var err error pvUpdateNeeded := false // Retrieve latest version - volume, err = ctrl.kubeClient.CoreV1().PersistentVolumes().Get(context.TODO(), volume.Name, metav1.GetOptions{}) + newVolume, err := ctrl.kubeClient.CoreV1().PersistentVolumes().Get(ctx, volume.Name, metav1.GetOptions{}) if err != nil { klog.Errorf("error reading persistent volume %q: %v", volume.Name, err) return err } + volume = newVolume volumeClone := volume.DeepCopy() pvFinalizers := volumeClone.Finalizers if pvFinalizers != nil && slice.ContainsString(pvFinalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) { @@ -1661,8 +1662,10 @@ func (ctrl *PersistentVolumeController) provisionClaimOperation( metav1.SetMetaDataAnnotation(&volume.ObjectMeta, storagehelpers.AnnDynamicallyProvisioned, plugin.GetPluginName()) if utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { - // Add finalizer here - volume.SetFinalizers([]string{storagehelpers.PVDeletionInTreeProtectionFinalizer}) + if volume.Spec.PersistentVolumeReclaimPolicy == v1.PersistentVolumeReclaimDelete { + // Add In-Tree protection finalizer here only when the reclaim policy is `Delete` + volume.SetFinalizers([]string{storagehelpers.PVDeletionInTreeProtectionFinalizer}) + } } // Try to create the PV object several times diff --git a/pkg/controller/volume/persistentvolume/pv_controller_base.go b/pkg/controller/volume/persistentvolume/pv_controller_base.go index eddc7b9c8dd..efce1e36448 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_base.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_base.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "strconv" + "strings" "time" v1 "k8s.io/api/core/v1" @@ -334,7 +335,7 @@ func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx cont // when no modifications are required this function could sometimes return a // copy of the volume and sometimes return a ref to the original claimClone := claim.DeepCopy() - modified := updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, nil, true) + modified := updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, true) if !modified { return claimClone, nil } @@ -352,11 +353,14 @@ func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx cont func (ctrl *PersistentVolumeController) updateVolumeMigrationAnnotationsAndFinalizers(ctx context.Context, volume *v1.PersistentVolume) (*v1.PersistentVolume, error) { volumeClone := volume.DeepCopy() - modified := updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator, - volumeClone.Annotations, &volumeClone.Finalizers, false) - if !modified { + annModified := updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, volumeClone.Annotations, false) + modifiedFinalizers, finalizersModified := modifyDeletionFinalizers(ctrl.csiMigratedPluginManager, volumeClone) + if !annModified && !finalizersModified { return volumeClone, nil } + if finalizersModified { + volumeClone.ObjectMeta.SetFinalizers(modifiedFinalizers) + } newVol, err := ctrl.kubeClient.CoreV1().PersistentVolumes().Update(ctx, volumeClone, metav1.UpdateOptions{}) if err != nil { return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations or finalizer: %v", err) @@ -368,17 +372,68 @@ func (ctrl *PersistentVolumeController) updateVolumeMigrationAnnotationsAndFinal return newVol, nil } -// updateMigrationAnnotationsAndFinalizers takes an Annotations map and finalizers slice and checks for a -// provisioner name depending if the annotation came from a PVC or not. -// It will then add a "pv.kubernetes.io/migrated-to" annotation if migration with -// the CSI driver name for that provisioner is "on" based on feature flags, it will also -// remove the PV deletion protection finalizer and "pv.kubernetes.io/migrated-to" annotation -// if migration is "off" for that provisioner in rollback scenarios. Returns true if the annotations -// map or finalizers were modified and false otherwise. -// Parameters: -// - claim: true means the ann came from a PVC, false means the ann came from a PV -func updateMigrationAnnotationsAndFinalizers(cmpm CSIMigratedPluginManager, translator CSINameTranslator, - ann map[string]string, finalizers *[]string, claim bool) bool { +// modifyDeletionFinalizers updates the finalizers based on the reclaim policy and if it is a in-tree volume or not. +// The in-tree PV deletion protection finalizer is only added if the reclaimPolicy associated with the PV is `Delete`. +// The in-tree PV deletion protection finalizer is removed if the reclaimPolicy associated with the PV is `Retain` or +// `Recycle`, removing the finalizer is necessary to reflect the recalimPolicy updates on the PV. +// The method also removes any external PV Deletion Protection finalizers added on the PV, this represents CSI migration +// rollback/disable scenarios. +func modifyDeletionFinalizers(cmpm CSIMigratedPluginManager, volume *v1.PersistentVolume) ([]string, bool) { + modified := false + var outFinalizers []string + if !utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { + return volume.Finalizers, false + } + if !metav1.HasAnnotation(volume.ObjectMeta, storagehelpers.AnnDynamicallyProvisioned) { + // PV deletion protection finalizer is currently supported only for dynamically + // provisioned volumes. + return volume.Finalizers, false + } + if volume.Finalizers != nil { + outFinalizers = append(outFinalizers, volume.Finalizers...) + } + provisioner := volume.Annotations[storagehelpers.AnnDynamicallyProvisioned] + if cmpm.IsMigrationEnabledForPlugin(provisioner) { + // Remove in-tree delete finalizer on the PV as migration is enabled. + if slice.ContainsString(outFinalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) { + outFinalizers = slice.RemoveString(outFinalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) + modified = true + } + return outFinalizers, modified + } + // Check if it is a in-tree volume. + if !strings.HasPrefix(provisioner, "kubernetes.io/") { + // The provision plugin does not begin with known in-tree plugin volume prefix annotation. + return volume.Finalizers, false + } + reclaimPolicy := volume.Spec.PersistentVolumeReclaimPolicy + // Add back the in-tree PV deletion protection finalizer if does not already exists + if reclaimPolicy == v1.PersistentVolumeReclaimDelete && !slice.ContainsString(outFinalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) { + klog.V(4).Infof("Adding in-tree pv deletion protection finalizer on %s", volume.Name) + outFinalizers = append(outFinalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer) + modified = true + } else if (reclaimPolicy == v1.PersistentVolumeReclaimRetain || reclaimPolicy == v1.PersistentVolumeReclaimRecycle) && slice.ContainsString(outFinalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) { + // Remove the in-tree PV deletion protection finalizer if the reclaim policy is 'Retain' or 'Recycle' + klog.V(4).Infof("Removing in-tree pv deletion protection finalizer on %s", volume.Name) + outFinalizers = slice.RemoveString(outFinalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) + modified = true + } + // Remove the external PV deletion protection finalizer + if slice.ContainsString(outFinalizers, storagehelpers.PVDeletionProtectionFinalizer, nil) { + klog.V(4).Infof("Removing external pv deletion protection finalizer on %s", volume.Name) + outFinalizers = slice.RemoveString(outFinalizers, storagehelpers.PVDeletionProtectionFinalizer, nil) + modified = true + } + return outFinalizers, modified +} + +// updateMigrationAnnotations takes an Annotations map and checks for a +// provisioner name using the provisionerKey. It will then add a +// "pv.kubernetes.io/migrated-to" annotation if migration with the CSI +// driver name for that provisioner is "on" based on feature flags, it will also +// remove the annotation is migration is "off" for that provisioner in rollback +// scenarios. Returns true if the annotations map was modified and false otherwise. +func updateMigrationAnnotations(cmpm CSIMigratedPluginManager, translator CSINameTranslator, ann map[string]string, claim bool) bool { var csiDriverName string var err error @@ -402,14 +457,13 @@ func updateMigrationAnnotationsAndFinalizers(cmpm CSIMigratedPluginManager, tran return false } } else { - // Volume not dynamically provisioned. Ignore + // Volume Statically provisioned. return false } } migratedToDriver := ann[storagehelpers.AnnMigratedTo] if cmpm.IsMigrationEnabledForPlugin(provisioner) { - modified := false csiDriverName, err = translator.GetCSINameFromInTreeName(provisioner) if err != nil { klog.Errorf("Could not update volume migration annotations. Migration enabled for plugin %s but could not find corresponding driver name: %v", provisioner, err) @@ -417,37 +471,13 @@ func updateMigrationAnnotationsAndFinalizers(cmpm CSIMigratedPluginManager, tran } if migratedToDriver != csiDriverName { ann[storagehelpers.AnnMigratedTo] = csiDriverName - modified = true + return true } - // Remove in-tree delete finalizer on the PV as migration is enabled. - if !claim && utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { - if finalizers != nil && slice.ContainsString(*finalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) { - *finalizers = slice.RemoveString(*finalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) - modified = true - } - } - return modified } else { if migratedToDriver != "" { // Migration annotation exists but the driver isn't migrated currently delete(ann, storagehelpers.AnnMigratedTo) - if !claim && utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { - modified := false - if finalizers == nil { - finalizers = &([]string{}) - } - // Add back the in-tree PV deletion protection finalizer if does not already exists - if !slice.ContainsString(*finalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer, nil) { - *finalizers = append(*finalizers, storagehelpers.PVDeletionInTreeProtectionFinalizer) - modified = true - } - // Remove the external PV deletion protection finalizer - if slice.ContainsString(*finalizers, storagehelpers.PVDeletionProtectionFinalizer, nil) { - *finalizers = slice.RemoveString(*finalizers, storagehelpers.PVDeletionProtectionFinalizer, nil) - modified = true - } - return modified - } + return true } } return false @@ -607,7 +637,7 @@ func (ctrl *PersistentVolumeController) setClaimProvisioner(ctx context.Context, // TODO: remove the beta storage provisioner anno after the deprecation period metav1.SetMetaDataAnnotation(&claimClone.ObjectMeta, storagehelpers.AnnBetaStorageProvisioner, provisionerName) metav1.SetMetaDataAnnotation(&claimClone.ObjectMeta, storagehelpers.AnnStorageProvisioner, provisionerName) - updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, nil, true) + updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, true) newClaim, err := ctrl.kubeClient.CoreV1().PersistentVolumeClaims(claim.Namespace).Update(context.TODO(), claimClone, metav1.UpdateOptions{}) if err != nil { return newClaim, err diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index 73c5d84cc42..a2dd56b61c5 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -52,6 +52,8 @@ 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) { + // Default enable the HonorPVReclaimPolicy feature gate. + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HonorPVReclaimPolicy, true)() tests := []controllerTest{ // [Unit test set 5] - controller tests. // We test the controller as if @@ -162,10 +164,8 @@ func TestControllerSync(t *testing.T) { // deleteClaim with a bound claim makes bound volume released with external deleter pending // there should be an entry in operation timestamps cache in controller "5-6 - delete claim and waiting for external volume deletion", - volumesWithAnnotation(volume.AnnDynamicallyProvisioned, "gcr.io/vendor-csi", - newVolumeArray("volume5-6", "10Gi", "uid5-6", "claim5-6", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classExternal, volume.AnnBoundByController)), - volumesWithAnnotation(volume.AnnDynamicallyProvisioned, "gcr.io/vendor-csi", - newVolumeArray("volume5-6", "10Gi", "uid5-6", "claim5-6", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classExternal, volume.AnnBoundByController)), + volumesWithAnnotation(volume.AnnDynamicallyProvisioned, "gcr.io/vendor-csi", []*v1.PersistentVolume{newExternalProvisionedVolume("volume5-6", "10Gi", "uid5-6", "claim5-6", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classExternal, "fake.driver.csi", nil, volume.AnnBoundByController)}), + volumesWithAnnotation(volume.AnnDynamicallyProvisioned, "gcr.io/vendor-csi", []*v1.PersistentVolume{newExternalProvisionedVolume("volume5-6", "10Gi", "uid5-6", "claim5-6", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classExternal, "fake.driver.csi", nil, volume.AnnBoundByController)}), claimWithAnnotation(volume.AnnStorageProvisioner, "gcr.io/vendor-csi", newClaimArray("claim5-6", "uid5-6", "1Gi", "volume5-6", v1.ClaimBound, &classExternal, volume.AnnBoundByController, volume.AnnBindCompleted)), noclaims, @@ -288,7 +288,7 @@ func TestControllerSync(t *testing.T) { { // Test that the finalizer gets removed if CSI migration is disabled. The in-tree finalizer is added // back on the PV since migration is disabled. - "5-9 - volume has its PV deletion protection finalizer removed as CSI migration is disabled", + "5-9 - volume has its external PV deletion protection finalizer removed as CSI migration is disabled", volumesWithFinalizers( volumesWithAnnotation(volume.AnnMigratedTo, "pd.csi.storage.gke.io", newVolumeArray("volume-5-9", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty, volume.AnnDynamicallyProvisioned)), @@ -590,14 +590,14 @@ func TestAnnealMigrationAnnotations(t *testing.T) { } if tc.volumeAnnotations != nil { ann := tc.volumeAnnotations - updateMigrationAnnotationsAndFinalizers(cmpm, translator, ann, nil, false) + updateMigrationAnnotations(cmpm, translator, ann, false) if !reflect.DeepEqual(tc.expVolumeAnnotations, ann) { t.Errorf("got volume annoations: %v, but expected: %v", ann, tc.expVolumeAnnotations) } } if tc.claimAnnotations != nil { ann := tc.claimAnnotations - updateMigrationAnnotationsAndFinalizers(cmpm, translator, ann, nil, true) + updateMigrationAnnotations(cmpm, translator, ann, true) if !reflect.DeepEqual(tc.expClaimAnnotations, ann) { t.Errorf("got volume annoations: %v, but expected: %v", ann, tc.expVolumeAnnotations) } @@ -607,26 +607,26 @@ func TestAnnealMigrationAnnotations(t *testing.T) { } } -func TestUpdateFinalizer(t *testing.T) { +func TestModifyDeletionFinalizers(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" tests := []struct { name string + initialVolume *v1.PersistentVolume volumeAnnotations map[string]string - volumeFinalizers []string expVolumeFinalizers []string expModified bool migratedDriverGates []featuregate.Feature }{ { - // Represents a volume provisioned through external-provisioner + // Represents a CSI volume provisioned through external-provisioner, no CSI migration enabled. name: "13-1 migration was never enabled, volume has the finalizer", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gceDriver}, - volumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, + initialVolume: newExternalProvisionedVolume("volume-13-1", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, gceDriver, []string{volume.PVDeletionProtectionFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, expModified: false, migratedDriverGates: []featuregate.Feature{}, @@ -635,8 +635,7 @@ func TestUpdateFinalizer(t *testing.T) { // Represents a volume provisioned through external-provisioner but the external-provisioner has // yet to sync the volume to add the new finalizer name: "13-2 migration was never enabled, volume does not have the finalizer", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gceDriver}, - volumeFinalizers: nil, + initialVolume: newExternalProvisionedVolume("volume-13-2", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, gceDriver, nil, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: nil, expModified: false, migratedDriverGates: []featuregate.Feature{}, @@ -647,17 +646,15 @@ func TestUpdateFinalizer(t *testing.T) { // pre-existing finalizer, for example the pv-protection finalizer. When csi-migration is disabled, // the migrated-to annotation will be removed shortly when updateVolumeMigrationAnnotationsAndFinalizers // is called followed by adding back the in-tree pv protection finalizer. - name: "13-3 migration was disabled but still has migrated-to annotation, volume does not have pv deletion protection finalizer", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gcePlugin, volume.AnnMigratedTo: gceDriver}, - volumeFinalizers: []string{customFinalizer}, + name: "13-3 migration was disabled, volume has existing custom finalizer, does not have in-tree pv deletion protection finalizer", + initialVolume: newVolumeWithFinalizers("volume-13-3", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, []string{customFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: []string{customFinalizer, volume.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{}, }, { - name: "13-4 migration was disabled but still has migrated-to annotation, volume has no finalizers", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gcePlugin, volume.AnnMigratedTo: gceDriver}, - volumeFinalizers: nil, + name: "13-4 migration was disabled, volume has no finalizers", + initialVolume: newVolumeWithFinalizers("volume-13-4", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, nil, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: []string{volume.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{}, @@ -666,9 +663,8 @@ func TestUpdateFinalizer(t *testing.T) { // Represents roll back scenario where the external-provisioner has added the pv deletion protection // finalizer and later the csi migration was disabled. The pv deletion protection finalizer added through // external-provisioner will be removed and the in-tree pv deletion protection finalizer will be added. - name: "13-5 migration was disabled as it has the migrated-to annotation, volume has the finalizer", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gcePlugin, volume.AnnMigratedTo: gceDriver}, - volumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, + name: "13-5 migration was disabled, volume has external PV deletion finalizer", + initialVolume: newVolumeWithFinalizers("volume-13-5", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, []string{volume.PVDeletionProtectionFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: []string{volume.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{}, @@ -677,9 +673,8 @@ func TestUpdateFinalizer(t *testing.T) { // Represents roll-back of csi-migration as 13-5, here there are multiple finalizers, only the pv deletion // protection finalizer added by external-provisioner will be removed and the in-tree pv deletion protection // finalizer will be added. - name: "13-6 migration was disabled as it has the migrated-to annotation, volume has multiple finalizers", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gcePlugin, volume.AnnMigratedTo: gceDriver}, - volumeFinalizers: []string{volume.PVDeletionProtectionFinalizer, customFinalizer}, + name: "13-6 migration was disabled, volume has multiple finalizers", + initialVolume: newVolumeWithFinalizers("volume-13-6", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, []string{volume.PVDeletionProtectionFinalizer, customFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: []string{customFinalizer, volume.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{}, @@ -687,9 +682,9 @@ func TestUpdateFinalizer(t *testing.T) { { // csi migration is enabled, the pv controller should not delete the finalizer added by the // external-provisioner and the in-tree finalizer should be deleted. - name: "13-7 migration is enabled, has the migrated-to annotation, volume has the finalizer", + name: "13-7 migration is enabled, volume has both the in-tree and external PV deletion protection finalizer", + initialVolume: newVolumeWithFinalizers("volume-13-7", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, []string{volume.PVDeletionProtectionFinalizer, volume.PVDeletionInTreeProtectionFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gcePlugin, volume.AnnMigratedTo: gceDriver}, - volumeFinalizers: []string{volume.PVDeletionProtectionFinalizer, volume.PVDeletionInTreeProtectionFinalizer}, expVolumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{features.CSIMigration, features.CSIMigrationGCE}, @@ -697,9 +692,8 @@ func TestUpdateFinalizer(t *testing.T) { { // csi-migration is not completely enabled as the specific plugin feature is not present. This is equivalent // of disabled csi-migration. - name: "13-8 migration is enabled but plugin migration feature is disabled, has the migrated-to annotation, volume has the finalizer", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gcePlugin, volume.AnnMigratedTo: gceDriver}, - volumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, + name: "13-8 migration is enabled but plugin migration feature is disabled, volume has the external PV deletion protection finalizer", + initialVolume: newVolumeWithFinalizers("volume-13-8", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, []string{volume.PVDeletionProtectionFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: []string{volume.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{features.CSIMigration}, @@ -707,9 +701,8 @@ func TestUpdateFinalizer(t *testing.T) { { // same as 13-8 but multiple finalizers exists, only the pv deletion protection finalizer needs to be // removed and the in-tree pv deletion protection finalizer needs to be added. - name: "13-9 migration is enabled but plugin migration feature is disabled, has the migrated-to annotation, volume has multiple finalizers", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gcePlugin, volume.AnnMigratedTo: gceDriver}, - volumeFinalizers: []string{volume.PVDeletionProtectionFinalizer, customFinalizer}, + name: "13-9 migration is enabled but plugin migration feature is disabled, volume has multiple finalizers including external PV deletion protection finalizer", + initialVolume: newVolumeWithFinalizers("volume-13-9", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, []string{volume.PVDeletionProtectionFinalizer, customFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: []string{customFinalizer, volume.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{features.CSIMigration}, @@ -717,37 +710,49 @@ func TestUpdateFinalizer(t *testing.T) { { // corner error case. name: "13-10 missing annotations but finalizers exist", - volumeAnnotations: nil, - volumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, + initialVolume: newVolumeWithFinalizers("volume-13-10", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, []string{volume.PVDeletionProtectionFinalizer}), expVolumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, expModified: false, migratedDriverGates: []featuregate.Feature{}, }, { name: "13-11 missing annotations and finalizers", - volumeAnnotations: nil, - volumeFinalizers: nil, + initialVolume: newVolumeWithFinalizers("volume-13-11", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, nil), expVolumeFinalizers: nil, expModified: false, migratedDriverGates: []featuregate.Feature{}, }, { - // corner error case - name: "13-12 missing provisioned-by annotation, existing finalizers", - volumeAnnotations: map[string]string{"fake": gcePlugin}, - volumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, - expVolumeFinalizers: []string{volume.PVDeletionProtectionFinalizer}, + // When ReclaimPolicy is Retain ensure that in-tree pv deletion protection finalizer is not added. + name: "13-12 migration is disabled, volume has no finalizers, reclaimPolicy is Retain", + initialVolume: newVolumeWithFinalizers("volume-13-12", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classCopper, nil, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), + expVolumeFinalizers: nil, expModified: false, migratedDriverGates: []featuregate.Feature{}, }, { - // csi migration is enabled, the pv controller should delete the in-tree finalizer - name: "13-13 migration is enabled, has the migrated-to annotation, volume has the in-tree finalizer", - volumeAnnotations: map[string]string{volume.AnnDynamicallyProvisioned: gcePlugin, volume.AnnMigratedTo: gceDriver}, - volumeFinalizers: []string{volume.PVDeletionInTreeProtectionFinalizer}, + // When ReclaimPolicy is Recycle ensure that in-tree pv deletion protection finalizer is not added. + name: "13-13 migration is disabled, volume has no finalizers, reclaimPolicy is Recycle", + initialVolume: newVolumeWithFinalizers("volume-13-13", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimRecycle, classCopper, nil, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), + expVolumeFinalizers: nil, + expModified: false, + migratedDriverGates: []featuregate.Feature{}, + }, + { + // When ReclaimPolicy is Retain ensure that in-tree pv deletion protection finalizer present is removed. + name: "13-14 migration is disabled, volume has in-tree pv deletion finalizers, reclaimPolicy is Retain", + initialVolume: newVolumeWithFinalizers("volume-13-14", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classCopper, []string{volume.PVDeletionInTreeProtectionFinalizer}, volume.AnnDynamicallyProvisioned, volume.AnnBoundByController), expVolumeFinalizers: nil, expModified: true, - migratedDriverGates: []featuregate.Feature{features.CSIMigration, features.CSIMigrationGCE}, + migratedDriverGates: []featuregate.Feature{}, + }, + { + // Statically provisioned volumes should not have the in-tree pv deletion protection finalizer + name: "13-15 migration is disabled, statically provisioned PV", + initialVolume: newVolumeWithFinalizers("volume-13-14", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classCopper, nil), + expVolumeFinalizers: nil, + expModified: false, + migratedDriverGates: []featuregate.Feature{}, }, } @@ -760,15 +765,14 @@ func TestUpdateFinalizer(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, f, true)() } if tc.volumeAnnotations != nil { - ann := tc.volumeAnnotations - finalizers := tc.volumeFinalizers - modified := updateMigrationAnnotationsAndFinalizers(cmpm, translator, ann, &finalizers, false) - if modified != tc.expModified { - t.Errorf("got modified: %v, but expected: %v", modified, tc.expModified) - } - if !reflect.DeepEqual(tc.expVolumeFinalizers, finalizers) { - t.Errorf("got volume finaliers: %v, but expected: %v", finalizers, tc.expVolumeFinalizers) - } + tc.initialVolume.SetAnnotations(tc.volumeAnnotations) + } + modifiedFinalizers, modified := modifyDeletionFinalizers(cmpm, tc.initialVolume) + if modified != tc.expModified { + t.Errorf("got modified: %v, but expected: %v", modified, tc.expModified) + } + if !reflect.DeepEqual(tc.expVolumeFinalizers, modifiedFinalizers) { + t.Errorf("got volume finaliers: %v, but expected: %v", modifiedFinalizers, tc.expVolumeFinalizers) } })