From d37f14d0ae092035bd9cadfc0aaaffce8c25b4d3 Mon Sep 17 00:00:00 2001 From: Deepak Kinni Date: Wed, 9 Feb 2022 09:50:56 -0800 Subject: [PATCH] Support for in-tree PV Deletion protection finalizer Signed-off-by: Deepak Kinni --- .../volume/persistentvolume/delete_test.go | 11 ++++ .../volume/persistentvolume/pv_controller.go | 49 +++++++++++++-- .../persistentvolume/pv_controller_base.go | 27 +++++++-- .../persistentvolume/pv_controller_test.go | 60 +++++++++---------- .../volume/persistentvolume/util/util.go | 3 + pkg/features/kube_features.go | 3 +- 6 files changed, 113 insertions(+), 40 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/delete_test.go b/pkg/controller/volume/persistentvolume/delete_test.go index 620caa9b356..50402d7e2a9 100644 --- a/pkg/controller/volume/persistentvolume/delete_test.go +++ b/pkg/controller/volume/persistentvolume/delete_test.go @@ -187,6 +187,17 @@ func TestDeleteSync(t *testing.T) { return testSyncVolume(ctrl, reactor, test) }, }, + { + // 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, pvutil.AnnBoundByController)), []string{pvutil.PVDeletionInTreeProtectionFinalizer}), + volumesWithFinalizers(withVolumeDeletionTimestamp(newVolumeArray("volume8-13", "1Gi", "uid8-13", "claim8-13", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classEmpty, pvutil.AnnBoundByController)), []string{pvutil.PVDeletionInTreeProtectionFinalizer}), + noclaims, + noclaims, + noevents, noerrors, + wrapTestWithReclaimCalls(operationDelete, []error{nil}, testSyncVolume), + }, } runSyncTests(t, tests, []*storage.StorageClass{}, []*v1.Pod{}) } diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index 151569e8f4b..61429c316aa 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/pkg/controller/volume/persistentvolume/pv_controller.go @@ -23,6 +23,10 @@ import ( "strings" "time" + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/kubernetes/pkg/features" + "k8s.io/kubernetes/pkg/util/slice" + v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -1247,9 +1251,11 @@ func (ctrl *PersistentVolumeController) deleteVolumeOperation(volume *v1.Persist return "", nil } - if newVolume.GetDeletionTimestamp() != nil { - klog.V(3).Infof("Volume %q is already being deleted", volume.Name) - return "", nil + if !utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { + if newVolume.GetDeletionTimestamp() != nil { + klog.V(3).Infof("Volume %q is already being deleted", volume.Name) + return "", nil + } } needsReclaim, err := ctrl.isVolumeReleased(newVolume) if err != nil { @@ -1439,11 +1445,41 @@ func (ctrl *PersistentVolumeController) doDeleteVolume(volume *v1.PersistentVolu // Deleter failed return pluginName, false, err } - klog.V(2).Infof("volume %q deleted", volume.Name) + // Remove in-tree delete finalizer on the PV as the volume has been deleted from the underlying storage + if utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { + err = ctrl.removeDeletionProtectionFinalizer(context.TODO(), volume) + if err != nil { + return pluginName, true, err + } + } return pluginName, true, nil } +func (ctrl *PersistentVolumeController) removeDeletionProtectionFinalizer(ctx context.Context, volume *v1.PersistentVolume) error { + var err error + pvUpdateNeeded := false + volumeClone := volume.DeepCopy() + pvFinalizers := volumeClone.Finalizers + if pvFinalizers != nil && slice.ContainsString(pvFinalizers, pvutil.PVDeletionInTreeProtectionFinalizer, nil) { + pvUpdateNeeded = true + pvFinalizers = slice.RemoveString(pvFinalizers, pvutil.PVDeletionInTreeProtectionFinalizer, nil) + } + if pvUpdateNeeded { + volumeClone.SetFinalizers(pvFinalizers) + _, err = ctrl.kubeClient.CoreV1().PersistentVolumes().Update(ctx, volumeClone, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("persistent volume controller can't update finalizer: %v", err) + } + _, err = ctrl.storeVolumeUpdate(volumeClone) + if err != nil { + return fmt.Errorf("persistent Volume Controller can't anneal migration finalizer: %v", err) + } + klog.V(2).Infof("PV in-tree protection finalizer removed from volume: %q", volume.Name) + } + return nil +} + // provisionClaim starts new asynchronous operation to provision a claim if // provisioning is enabled. func (ctrl *PersistentVolumeController) provisionClaim(ctx context.Context, claim *v1.PersistentVolumeClaim) error { @@ -1619,6 +1655,11 @@ func (ctrl *PersistentVolumeController) provisionClaimOperation( metav1.SetMetaDataAnnotation(&volume.ObjectMeta, pvutil.AnnBoundByController, "yes") metav1.SetMetaDataAnnotation(&volume.ObjectMeta, pvutil.AnnDynamicallyProvisioned, plugin.GetPluginName()) + if utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { + // Add finalizer here + volume.SetFinalizers([]string{pvutil.PVDeletionInTreeProtectionFinalizer}) + } + // Try to create the PV object several times for i := 0; i < ctrl.createProvisionedPVRetryCount; i++ { klog.V(4).Infof("provisionClaimOperation [%s]: trying to save volume %s", claimToClaimKey(claim), volume.Name) diff --git a/pkg/controller/volume/persistentvolume/pv_controller_base.go b/pkg/controller/volume/persistentvolume/pv_controller_base.go index dfa2f602992..5f57add8581 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_base.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_base.go @@ -409,6 +409,7 @@ func updateMigrationAnnotationsAndFinalizers(cmpm CSIMigratedPluginManager, tran migratedToDriver := ann[pvutil.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) @@ -416,19 +417,37 @@ func updateMigrationAnnotationsAndFinalizers(cmpm CSIMigratedPluginManager, tran } if migratedToDriver != csiDriverName { ann[pvutil.AnnMigratedTo] = csiDriverName - return true + modified = 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, pvutil.PVDeletionInTreeProtectionFinalizer, nil) { + *finalizers = slice.RemoveString(*finalizers, pvutil.PVDeletionInTreeProtectionFinalizer, nil) + modified = true + } + } + return modified } else { if migratedToDriver != "" { // Migration annotation exists but the driver isn't migrated currently delete(ann, pvutil.AnnMigratedTo) - // Remove the PV deletion protection finalizer on volumes. - if !claim && finalizers != nil && utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) { + 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, pvutil.PVDeletionInTreeProtectionFinalizer, nil) { + *finalizers = append(*finalizers, pvutil.PVDeletionInTreeProtectionFinalizer) + modified = true + } + // Remove the external PV deletion protection finalizer if slice.ContainsString(*finalizers, pvutil.PVDeletionProtectionFinalizer, nil) { *finalizers = slice.RemoveString(*finalizers, pvutil.PVDeletionProtectionFinalizer, nil) + modified = true } + return modified } - return true } } return false diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index 8f69f3c3770..645fdb08160 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -294,29 +294,15 @@ func TestControllerSync(t *testing.T) { }, }, { - // delete success(?) - volume has deletion timestamp before doDelete() starts - "8-13 - volume is has deletion timestamp and is not processed", - withVolumeDeletionTimestamp(newVolumeArray("volume8-13", "1Gi", "uid8-13", "claim8-13", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty)), - withVolumeDeletionTimestamp(newVolumeArray("volume8-13", "1Gi", "uid8-13", "claim8-13", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, classEmpty)), - noclaims, - noclaims, - noevents, noerrors, - // We don't need to do anything in test function because deletion will be noticed automatically and synced. - // Attempting to use testSyncVolume here will cause an error because of race condition between manually - // calling testSyncVolume and volume loop running. - func(ctrl *PersistentVolumeController, reactor *pvtesting.VolumeReactor, test controllerTest) error { - return nil - }, - }, - { - // Test that the finalizer gets removed if CSI migration is disabled. + // 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", volumesWithFinalizers( volumesWithAnnotation(pvutil.AnnMigratedTo, "pd.csi.storage.gke.io", newVolumeArray("volume-5-9", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty, pvutil.AnnDynamicallyProvisioned)), []string{pvutil.PVDeletionProtectionFinalizer}, ), - newVolumeArray("volume-5-9", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty, pvutil.AnnDynamicallyProvisioned), + volumesWithFinalizers(newVolumeArray("volume-5-9", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty, pvutil.AnnDynamicallyProvisioned), []string{pvutil.PVDeletionInTreeProtectionFinalizer}), noclaims, noclaims, noevents, @@ -743,12 +729,13 @@ func TestUpdateFinalizer(t *testing.T) { { // Represents an in-tree volume that has the migrated-to annotation but the external-provisioner is // yet to sync the volume and add the pv deletion protection finalizer. The custom finalizer is some - // pre-existing finalizer, for example the pv-protection finalizer. The csi-migration is disabled, - // the migrated-to annotation will be removed shortly when updateVolumeMigrationAnnotationsAndFinalizers is called. + // 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{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, volumeFinalizers: []string{customFinalizer}, - expVolumeFinalizers: []string{customFinalizer}, + expVolumeFinalizers: []string{customFinalizer, pvutil.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{}, }, @@ -756,39 +743,40 @@ func TestUpdateFinalizer(t *testing.T) { name: "13-4 migration was disabled but still has migrated-to annotation, volume has no finalizers", volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, volumeFinalizers: nil, - expVolumeFinalizers: nil, + expVolumeFinalizers: []string{pvutil.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{}, }, { // 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. + // 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{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, - expVolumeFinalizers: nil, + expVolumeFinalizers: []string{pvutil.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{}, }, { // Represents roll-back of csi-migration as 13-5, here there are multiple finalizers, only the pv deletion - // protection finalizer added by external-provisioner needs to be removed. + // 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{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer, customFinalizer}, - expVolumeFinalizers: []string{customFinalizer}, + expVolumeFinalizers: []string{customFinalizer, pvutil.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{}, }, { // csi migration is enabled, the pv controller should not delete the finalizer added by the - // external-provisioner. + // 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", volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, - volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, + volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer, pvutil.PVDeletionInTreeProtectionFinalizer}, expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, - expModified: false, + expModified: true, migratedDriverGates: []featuregate.Feature{features.CSIMigration, features.CSIMigrationGCE}, }, { @@ -797,16 +785,17 @@ func TestUpdateFinalizer(t *testing.T) { 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{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, - expVolumeFinalizers: nil, + expVolumeFinalizers: []string{pvutil.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{features.CSIMigration}, }, { - // same as 13-8 but multiple finalizers exists, only the pv deletion protection finalizer needs to be removed. + // 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{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer, customFinalizer}, - expVolumeFinalizers: []string{customFinalizer}, + expVolumeFinalizers: []string{customFinalizer, pvutil.PVDeletionInTreeProtectionFinalizer}, expModified: true, migratedDriverGates: []featuregate.Feature{features.CSIMigration}, }, @@ -836,6 +825,15 @@ func TestUpdateFinalizer(t *testing.T) { 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{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, + volumeFinalizers: []string{pvutil.PVDeletionInTreeProtectionFinalizer}, + expVolumeFinalizers: nil, + expModified: true, + migratedDriverGates: []featuregate.Feature{features.CSIMigration, features.CSIMigrationGCE}, + }, } translator := csitrans.New() diff --git a/pkg/controller/volume/persistentvolume/util/util.go b/pkg/controller/volume/persistentvolume/util/util.go index eec8f889ab3..184f1b24aea 100644 --- a/pkg/controller/volume/persistentvolume/util/util.go +++ b/pkg/controller/volume/persistentvolume/util/util.go @@ -78,6 +78,9 @@ const ( //PVDeletionProtectionFinalizer is the finalizer added by the external-provisioner on the PV PVDeletionProtectionFinalizer = "external-provisioner.volume.kubernetes.io/finalizer" + + // PVDeletionInTreeProtectionFinalizer is the finalizer added to protect PV deletion for in-tree volumes. + PVDeletionInTreeProtectionFinalizer = "kubernetes.io/pv-controller" ) // IsDelayBindingProvisioning checks if claim provisioning with selected-node annotation diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index cbb35119c22..c3fe034313d 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -801,6 +801,7 @@ const ( // owner: @deepakkinni @xing-yang // kep: http://kep.k8s.io/2680 // alpha: v1.23 + // beta: v1.24 // // Honor Persistent Volume Reclaim Policy when it is "Delete" irrespective of PV-PVC // deletion ordering. @@ -943,7 +944,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS JobMutableNodeSchedulingDirectives: {Default: true, PreRelease: featuregate.Beta}, IdentifyPodOS: {Default: false, PreRelease: featuregate.Alpha}, PodAndContainerStatsFromCRI: {Default: false, PreRelease: featuregate.Alpha}, - HonorPVReclaimPolicy: {Default: false, PreRelease: featuregate.Alpha}, + HonorPVReclaimPolicy: {Default: false, PreRelease: featuregate.Beta}, RecoverVolumeExpansionFailure: {Default: false, PreRelease: featuregate.Alpha}, GRPCContainerProbe: {Default: false, PreRelease: featuregate.Alpha}, LegacyServiceAccountTokenNoAutoGeneration: {Default: true, PreRelease: featuregate.Beta},