From bfd5f23a0b36f16f12ac1a132e325b5d59d888be Mon Sep 17 00:00:00 2001 From: Deepak Kinni Date: Tue, 19 Oct 2021 14:26:28 -0700 Subject: [PATCH] PV controller changes to support PV Deletion protection finalizer Signed-off-by: Deepak Kinni --- .../volume/persistentvolume/framework_test.go | 9 + .../volume/persistentvolume/pv_controller.go | 5 +- .../persistentvolume/pv_controller_base.go | 38 ++-- .../persistentvolume/pv_controller_test.go | 176 +++++++++++++++++- .../volume/persistentvolume/util/util.go | 3 + pkg/features/kube_features.go | 9 + 6 files changed, 222 insertions(+), 18 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/framework_test.go b/pkg/controller/volume/persistentvolume/framework_test.go index 8a0212d62f6..4aa66c7cad5 100644 --- a/pkg/controller/volume/persistentvolume/framework_test.go +++ b/pkg/controller/volume/persistentvolume/framework_test.go @@ -370,6 +370,15 @@ func withVolumeDeletionTimestamp(pvs []*v1.PersistentVolume) []*v1.PersistentVol return result } +func volumesWithFinalizers(pvs []*v1.PersistentVolume, finalizers []string) []*v1.PersistentVolume { + result := []*v1.PersistentVolume{} + for _, pv := range pvs { + pv.SetFinalizers(finalizers) + result = append(result, pv) + } + return result +} + // newClaim returns a new claim with given attributes func newClaim(name, claimUID, capacity, boundToVolume string, phase v1.PersistentVolumeClaimPhase, class *string, annotations ...string) *v1.PersistentVolumeClaim { fs := v1.PersistentVolumeFilesystem diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go index caaea704e43..151569e8f4b 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller.go +++ b/pkg/controller/volume/persistentvolume/pv_controller.go @@ -537,10 +537,9 @@ func (ctrl *PersistentVolumeController) syncBoundClaim(claim *v1.PersistentVolum // these events. func (ctrl *PersistentVolumeController) syncVolume(ctx context.Context, volume *v1.PersistentVolume) error { klog.V(4).Infof("synchronizing PersistentVolume[%s]: %s", volume.Name, getVolumeStatusForLogging(volume)) - - // Set correct "migrated-to" annotations on PV and update in API server if + // Set correct "migrated-to" annotations and modify finalizers on PV and update in API server if // necessary - newVolume, err := ctrl.updateVolumeMigrationAnnotations(ctx, volume) + newVolume, err := ctrl.updateVolumeMigrationAnnotationsAndFinalizers(ctx, volume) if err != nil { // Nothing was saved; we will fall back into the same // condition in the next call to this method diff --git a/pkg/controller/volume/persistentvolume/pv_controller_base.go b/pkg/controller/volume/persistentvolume/pv_controller_base.go index b56c31d07be..ef5cdda5894 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_base.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_base.go @@ -19,6 +19,8 @@ package persistentvolume import ( "context" "fmt" + "k8s.io/kubernetes/pkg/features" + "k8s.io/kubernetes/pkg/util/slice" "strconv" "time" @@ -323,7 +325,8 @@ func (ctrl *PersistentVolumeController) Run(ctx context.Context) { <-ctx.Done() } -func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx context.Context, claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) { +func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx context.Context, + claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) { // TODO: update[Claim|Volume]MigrationAnnotations can be optimized to not // copy the claim/volume if no modifications are required. Though this // requires some refactoring as well as an interesting change in the @@ -331,7 +334,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 := updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, true) + modified := updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, nil, true) if !modified { return claimClone, nil } @@ -346,33 +349,36 @@ func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx cont return newClaim, nil } -func (ctrl *PersistentVolumeController) updateVolumeMigrationAnnotations(ctx context.Context, volume *v1.PersistentVolume) (*v1.PersistentVolume, error) { +func (ctrl *PersistentVolumeController) updateVolumeMigrationAnnotationsAndFinalizers(ctx context.Context, + volume *v1.PersistentVolume) (*v1.PersistentVolume, error) { volumeClone := volume.DeepCopy() - modified := updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, volumeClone.Annotations, false) + modified := updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator, + volumeClone.Annotations, &volumeClone.Finalizers, false) if !modified { return volumeClone, nil } 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: %v", err) + return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations or finalizer: %v", err) } _, err = ctrl.storeVolumeUpdate(newVol) if err != nil { - return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations: %v", err) + return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations or finalizer: %v", err) } return newVol, nil - } -// updateMigrationAnnotations takes an Annotations map and checks for a +// 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 annotation if migration is "off" for that provisioner in rollback -// scenarios. Returns true if the annotations map was modified and false otherwise. +// 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 updateMigrationAnnotations(cmpm CSIMigratedPluginManager, translator CSINameTranslator, ann map[string]string, claim bool) bool { +func updateMigrationAnnotationsAndFinalizers(cmpm CSIMigratedPluginManager, translator CSINameTranslator, + ann map[string]string, finalizers *[]string, claim bool) bool { var csiDriverName string var err error @@ -416,6 +422,12 @@ func updateMigrationAnnotations(cmpm CSIMigratedPluginManager, translator CSINam 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 slice.ContainsString(*finalizers, pvutil.PVDeletionProtectionFinalizer, nil) { + *finalizers = slice.RemoveString(*finalizers, pvutil.PVDeletionProtectionFinalizer, nil) + } + } return true } } @@ -576,8 +588,8 @@ func (ctrl *PersistentVolumeController) setClaimProvisioner(ctx context.Context, // TODO: remove the beta storage provisioner anno after the deprecation period metav1.SetMetaDataAnnotation(&claimClone.ObjectMeta, pvutil.AnnBetaStorageProvisioner, provisionerName) metav1.SetMetaDataAnnotation(&claimClone.ObjectMeta, pvutil.AnnStorageProvisioner, provisionerName) - updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, true) - newClaim, err := ctrl.kubeClient.CoreV1().PersistentVolumeClaims(claim.Namespace).Update(ctx, claimClone, metav1.UpdateOptions{}) + updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, nil, 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 a6b3ba3dfee..a86a537061b 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -59,6 +59,7 @@ var ( // 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 @@ -295,6 +296,23 @@ func TestControllerSync(t *testing.T) { return nil }, }, + { + // Test that the finalizer gets removed if CSI 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), + noclaims, + noclaims, + noevents, + noerrors, + func(ctrl *PersistentVolumeController, reactor *pvtesting.VolumeReactor, test controllerTest) error { + return nil + }, + }, } doit := func(test controllerTest) { @@ -642,14 +660,14 @@ func TestAnnealMigrationAnnotations(t *testing.T) { } if tc.volumeAnnotations != nil { ann := tc.volumeAnnotations - updateMigrationAnnotations(cmpm, translator, ann, false) + updateMigrationAnnotationsAndFinalizers(cmpm, translator, ann, nil, 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 - updateMigrationAnnotations(cmpm, translator, ann, true) + updateMigrationAnnotationsAndFinalizers(cmpm, translator, ann, nil, true) if !reflect.DeepEqual(tc.expClaimAnnotations, ann) { t.Errorf("got volume annoations: %v, but expected: %v", ann, tc.expVolumeAnnotations) } @@ -658,3 +676,157 @@ func TestAnnealMigrationAnnotations(t *testing.T) { }) } } + +func TestUpdateFinalizer(t *testing.T) { + 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 + volumeAnnotations map[string]string + volumeFinalizers []string + expVolumeFinalizers []string + expModified bool + migratedDriverGates []featuregate.Feature + }{ + { + // Represents a volume provisioned through external-provisioner + name: "13-1 migration was never enabled, volume has the finalizer", + volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gceDriver}, + volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, + expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, + expModified: false, + migratedDriverGates: []featuregate.Feature{}, + }, + { + // 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{pvutil.AnnDynamicallyProvisioned: gceDriver}, + volumeFinalizers: nil, + expVolumeFinalizers: nil, + expModified: false, + migratedDriverGates: []featuregate.Feature{}, + }, + { + // 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. + 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}, + 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{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, + volumeFinalizers: nil, + expVolumeFinalizers: nil, + 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. + 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, + 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. + 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}, + expModified: true, + migratedDriverGates: []featuregate.Feature{}, + }, + { + // csi migration is enabled, the pv controller should not delete the finalizer added by the + // external-provisioner. + 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}, + expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, + expModified: false, + migratedDriverGates: []featuregate.Feature{features.CSIMigration, features.CSIMigrationGCE}, + }, + { + // 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{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, + volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, + expVolumeFinalizers: nil, + 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. + 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}, + expModified: true, + migratedDriverGates: []featuregate.Feature{features.CSIMigration}, + }, + { + // corner error case. + name: "13-10 missing annotations but finalizers exist", + volumeAnnotations: nil, + volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, + expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, + expModified: false, + migratedDriverGates: []featuregate.Feature{}, + }, + { + name: "13-11 missing annotations and finalizers", + volumeAnnotations: nil, + volumeFinalizers: 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{pvutil.PVDeletionProtectionFinalizer}, + expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer}, + expModified: false, + migratedDriverGates: []featuregate.Feature{}, + }, + } + + translator := csitrans.New() + cmpm := csimigration.NewPluginManager(translator, utilfeature.DefaultFeatureGate) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + for _, f := range tc.migratedDriverGates { + 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) + } + } + + }) + } +} diff --git a/pkg/controller/volume/persistentvolume/util/util.go b/pkg/controller/volume/persistentvolume/util/util.go index df532d550c4..754c79adaed 100644 --- a/pkg/controller/volume/persistentvolume/util/util.go +++ b/pkg/controller/volume/persistentvolume/util/util.go @@ -75,6 +75,9 @@ const ( // TODO: remove beta anno once deprecation period ends AnnStorageProvisioner = "volume.kubernetes.io/storage-provisioner" AnnBetaStorageProvisioner = "volume.beta.kubernetes.io/storage-provisioner" + + //PVDeletionProtectionFinalizer is the finalizer added by the external-provisioner on the PV + PVDeletionProtectionFinalizer = "external-provisioner.volume.kubernetes.io/finalizer" ) // IsDelayBindingProvisioning checks if claim provisioning with selected-node annotation diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index fe36edd4a62..58d6986d00a 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -792,6 +792,14 @@ const ( // Configures the Kubelet to use the CRI to populate pod and container stats, instead of supplimenting with stats from cAdvisor. // Requires the CRI implementation supports supplying the required stats. PodAndContainerStatsFromCRI featuregate.Feature = "PodAndContainerStatsFromCRI" + + // owner: @deepakkinni @xing-yang + // kep: http://kep.k8s.io/2680 + // alpha: v1.23 + // + // Honor Persistent Volume Reclaim Policy when it is "Delete" irrespective of PV-PVC + // deletion ordering. + HonorPVReclaimPolicy featuregate.Feature = "HonorPVReclaimPolicy" ) func init() { @@ -908,6 +916,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}, // inherited features from generic apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: