Support adding external finalizer for static provisioned volumes

Signed-off-by: Deepak Kinni <[email protected]>
kubernetes-sigs · Oct 25, 2022 · 5182976 · 5182976
1 parent a2f2ceb
commit 5182976
Show file tree

Hide file tree

Showing 2 changed files with 125 additions and 17 deletions.
diff --git a/controller/controller.go b/controller/controller.go
@@ -1114,25 +1114,61 @@ func (ctrl *ProvisionController) syncVolume(ctx context.Context, obj interface{}
 }
 
 func (ctrl *ProvisionController) handleProtectionFinalizer(ctx context.Context, volume *v1.PersistentVolume) (*v1.PersistentVolume, error) {
-	var modifiedFinalizers []string
+	//var modifiedFinalizers []string
 	var modified bool
+	klog.Infof("handleProtectionFinalizer Volume : %+v", volume)
 	reclaimPolicy := volume.Spec.PersistentVolumeReclaimPolicy
+	volumeFinalizers := volume.ObjectMeta.Finalizers
+	if metav1.HasAnnotation(volume.ObjectMeta, annDynamicallyProvisioned) {
+		provisionPluginName := volume.Annotations[annDynamicallyProvisioned]
+		migratedAnn := volume.Annotations[annMigratedTo]
+		// Determine if the PV is owned by the current provisioner.
+		if !ctrl.knownProvisioner(provisionPluginName) && !ctrl.knownProvisioner(migratedAnn) {
+			// The current provisioner is not responsible for adding the finalizer
+			return volume, nil
+		}
+		// Add the finalizer only if `addFinalizer` config option is enabled, finalizer doesn't exist and PV is not already
+		// under deletion.
+		if ctrl.addFinalizer && reclaimPolicy == v1.PersistentVolumeReclaimDelete && volume.DeletionTimestamp == nil {
+			volumeFinalizers, modified = addFinalizer(volumeFinalizers, finalizerPV)
+		}
+	} else {
+		// Statically provisioned volume. Check if the volume type is CSI.
+		if volume.Spec.PersistentVolumeSource.CSI != nil {
+			volumeProvisioner := volume.Spec.CSI.Driver
+			if !ctrl.knownProvisioner(volumeProvisioner) {
+				return volume, nil
+			}
+		} else {
+			// Check if the volume is being migrated
+			migratedAnn := volume.Annotations[annMigratedTo]
+			if !ctrl.knownProvisioner(migratedAnn) {
+				// The current provisioner is not responsible for adding the finalizer
+				return volume, nil
+			}
+		}
+		// Add the finalizer only if `addFinalizer` config option is enabled, finalizer doesn't exist and PV is not already
+		// under deletion and the volume is in a Bound state.
+		if ctrl.addFinalizer && reclaimPolicy == v1.PersistentVolumeReclaimDelete && volume.DeletionTimestamp == nil && volume.Status.Phase == v1.VolumeBound {
+			volumeFinalizers, modified = addFinalizer(volumeFinalizers, finalizerPV)
+		}
+		// For statically provisioned volumes remove the external finalizer if the volume is not in a Bound state
+		if volume.Status.Phase != v1.VolumeBound {
+			volumeFinalizers, modified = removeFinalizer(volumeFinalizers, finalizerPV)
+		}
+	}
+
 	// Check if the `addFinalizer` config option is disabled, i.e, rollback scenario, or the reclaim policy is changed
 	// to `Retain` or `Recycle`
 	if !ctrl.addFinalizer || reclaimPolicy == v1.PersistentVolumeReclaimRetain || reclaimPolicy == v1.PersistentVolumeReclaimRecycle {
-		modifiedFinalizers, modified = removeFinalizer(volume.ObjectMeta.Finalizers, finalizerPV)
-	}
-	// Add the finalizer only if `addFinalizer` config option is enabled, finalizer doesn't exist and PV is not already
-	// under deletion.
-	if ctrl.addFinalizer && reclaimPolicy == v1.PersistentVolumeReclaimDelete && volume.DeletionTimestamp == nil {
-		modifiedFinalizers, modified = addFinalizer(volume.ObjectMeta.Finalizers, finalizerPV)
+		volumeFinalizers, modified = removeFinalizer(volumeFinalizers, finalizerPV)
 	}
 
 	if modified {
-		volume.ObjectMeta.Finalizers = modifiedFinalizers
+		volume.ObjectMeta.Finalizers = volumeFinalizers
 		newVolume, err := ctrl.updatePersistentVolume(ctx, volume)
 		if err != nil {
-			return volume, fmt.Errorf("failed to modify finalizers to %+v on volume %s err: %+v", modifiedFinalizers, volume.Name, err)
+			return volume, fmt.Errorf("failed to modify finalizers to %+v on volume %s err: %+v", volumeFinalizers, volume.Name, err)
 		}
 		volume = newVolume
 	}
@@ -1280,14 +1316,6 @@ func (ctrl *ProvisionController) updateDeleteStats(volume *v1.PersistentVolume,
 }
 
 func (ctrl *ProvisionController) updatePersistentVolume(ctx context.Context, volume *v1.PersistentVolume) (*v1.PersistentVolume, error) {
-	if !metav1.HasAnnotation(volume.ObjectMeta, annDynamicallyProvisioned) {
-		return volume, nil
-	}
-	provisionedBy := volume.Annotations[annDynamicallyProvisioned]
-	migratedTo := volume.Annotations[annMigratedTo]
-	if provisionedBy != ctrl.provisionerName && migratedTo != ctrl.provisionerName {
-		return volume, nil
-	}
 	newVolume, err := ctrl.client.CoreV1().PersistentVolumes().Update(ctx, volume, metav1.UpdateOptions{})
 	if err != nil {
 		return volume, err

diff --git a/controller/controller_test.go b/controller/controller_test.go
@@ -714,6 +714,54 @@ func TestController(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "ensure finalizer is not added on statically provisioned, migrated, in-tree volumes if it is not in a Bound state",
+			objs: []runtime.Object{
+				newVolume("volume-1", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, map[string]string{annMigratedTo: "foo.bar/baz"}, nil, nil),
+			},
+			addFinalizer:    true,
+			provisionerName: "foo.bar/baz",
+			provisioner:     newTestProvisioner(),
+			expectedVolumes: []v1.PersistentVolume{
+				*newVolume("volume-1", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, map[string]string{annMigratedTo: "foo.bar/baz"}, nil, nil),
+			},
+		},
+		{
+			name: "ensure finalizer is added on statically provisioned, migrated, in-tree volumes if it is in a Bound state",
+			objs: []runtime.Object{
+				newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annMigratedTo: "foo.bar/baz"}, nil, nil),
+			},
+			addFinalizer:    true,
+			provisionerName: "foo.bar/baz",
+			provisioner:     newTestProvisioner(),
+			expectedVolumes: []v1.PersistentVolume{
+				*newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annMigratedTo: "foo.bar/baz"}, []string{finalizerPV}, nil),
+			},
+		},
+		{
+			name: "ensure finalizer is removed on statically provisioned CSI volumes if it is not a Bound state",
+			objs: []runtime.Object{
+				newCSIVolume("volume-1", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, nil, []string{finalizerPV}, nil, "foo.bar/baz"),
+			},
+			addFinalizer:    true,
+			provisionerName: "foo.bar/baz",
+			provisioner:     newTestProvisioner(),
+			expectedVolumes: []v1.PersistentVolume{
+				*newCSIVolume("volume-1", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, nil, nil, nil, "foo.bar/baz"),
+			},
+		},
+		{
+			name: "ensure finalizer is added on statically provisioned CSI volumes if it is in a Bound state",
+			objs: []runtime.Object{
+				newCSIVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, nil, nil, nil, "foo.bar/baz"),
+			},
+			addFinalizer:    true,
+			provisionerName: "foo.bar/baz",
+			provisioner:     newTestProvisioner(),
+			expectedVolumes: []v1.PersistentVolume{
+				*newCSIVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, nil, []string{finalizerPV}, nil, "foo.bar/baz"),
+			},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -1725,6 +1773,38 @@ func newVolume(name string, phase v1.PersistentVolumePhase, policy v1.Persistent
 	return pv
 }
 
+func newCSIVolume(name string, phase v1.PersistentVolumePhase, policy v1.PersistentVolumeReclaimPolicy,
+	annotations map[string]string, finalizers []string, deletionTimestamp *metav1.Time, provisionerName string) *v1.PersistentVolume {
+	pv := &v1.PersistentVolume{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              name,
+			Annotations:       annotations,
+			Finalizers:        finalizers,
+			DeletionTimestamp: deletionTimestamp,
+			SelfLink:          "/api/v1/persistentvolumes/" + name,
+		},
+		Spec: v1.PersistentVolumeSpec{
+			PersistentVolumeReclaimPolicy: policy,
+			AccessModes:                   []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce, v1.ReadOnlyMany},
+			Capacity: v1.ResourceList{
+				v1.ResourceName(v1.ResourceStorage): resource.MustParse("1Mi"),
+			},
+			PersistentVolumeSource: v1.PersistentVolumeSource{
+				CSI: &v1.CSIPersistentVolumeSource{
+					Driver:       provisionerName,
+					VolumeHandle: "test-volume-id-1",
+					ReadOnly:     false,
+					FSType:       "ext4",
+				},
+			},
+		},
+		Status: v1.PersistentVolumeStatus{
+			Phase: phase,
+		},
+	}
+	return pv
+}
+
 // newProvisionedVolume returns the volume the test controller should provision
 // for the given claim with the given class.
 func newProvisionedVolume(storageClass *storage.StorageClass, claim *v1.PersistentVolumeClaim, pvFinalizers []string) *v1.PersistentVolume {