From 9e0046fcd49d454c8754d2193b5cec0487aa480c Mon Sep 17 00:00:00 2001 From: Deepak Kinni Date: Fri, 5 Nov 2021 13:48:08 -0700 Subject: [PATCH] sig-storage-lib changes to support PV Deletion protection finalizer Signed-off-by: Deepak Kinni --- controller/controller.go | 90 ++++++- controller/controller_test.go | 438 +++++++++++++++++++++++++++++----- 2 files changed, 452 insertions(+), 76 deletions(-) diff --git a/controller/controller.go b/controller/controller.go index 915d357..f8f239a 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -1098,6 +1098,28 @@ func (ctrl *ProvisionController) syncVolume(ctx context.Context, obj interface{} if !ok { return fmt.Errorf("expected volume but got %+v", obj) } + var modifiedFinalizers []string + var modified bool + reclaimPolicy := volume.Spec.PersistentVolumeReclaimPolicy + // 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) + } + + if modified { + volume.ObjectMeta.Finalizers = modifiedFinalizers + newVolume, err := ctrl.updatePersistentVolume(ctx, volume) + if err != nil { + return fmt.Errorf("failed to modify finalizers to %+v on volume %s err: %+v", modifiedFinalizers, volume.Name, err) + } + volume = newVolume + } if ctrl.shouldDelete(ctx, volume) { startTime := time.Now() @@ -1174,10 +1196,15 @@ func (ctrl *ProvisionController) shouldDelete(ctx context.Context, volume *v1.Pe } } - if ctrl.addFinalizer && !ctrl.checkFinalizer(volume, finalizerPV) && volume.ObjectMeta.DeletionTimestamp != nil { - return false - } else if volume.ObjectMeta.DeletionTimestamp != nil { - return false + if ctrl.addFinalizer { + if !ctrl.checkFinalizer(volume, finalizerPV) && volume.ObjectMeta.DeletionTimestamp != nil { + // The finalizer was removed, i.e. the volume has been already deleted. + return false + } + } else { + if volume.ObjectMeta.DeletionTimestamp != nil { + return false + } } if volume.Status.Phase != v1.VolumeReleased { @@ -1243,6 +1270,22 @@ 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 + } + return newVolume, nil +} + // rescheduleProvisioning signal back to the scheduler to retry dynamic provisioning // by removing the annSelectedNode annotation func (ctrl *ProvisionController) rescheduleProvisioning(ctx context.Context, claim *v1.PersistentVolumeClaim) error { @@ -1459,15 +1502,10 @@ func (ctrl *ProvisionController) deleteVolumeOperation(ctx context.Context, volu if !ok { return fmt.Errorf("expected volume but got %+v", volumeObj) } - finalizers := make([]string, 0) - for _, finalizer := range newVolume.ObjectMeta.Finalizers { - if finalizer != finalizerPV { - finalizers = append(finalizers, finalizer) - } - } + finalizers, modified := removeFinalizer(newVolume.ObjectMeta.Finalizers, finalizerPV) // Only update the finalizers if we actually removed something - if len(finalizers) != len(newVolume.ObjectMeta.Finalizers) { + if modified { newVolume.ObjectMeta.Finalizers = finalizers if _, err = ctrl.client.CoreV1().PersistentVolumes().Update(ctx, newVolume, metav1.UpdateOptions{}); err != nil { if !apierrs.IsNotFound(err) { @@ -1490,6 +1528,36 @@ func (ctrl *ProvisionController) deleteVolumeOperation(ctx context.Context, volu return nil } +func removeFinalizer(finalizers []string, finalizerToRemove string) ([]string, bool) { + modified := false + modifiedFinalizers := make([]string, 0) + for _, finalizer := range finalizers { + if finalizer != finalizerToRemove { + modifiedFinalizers = append(modifiedFinalizers, finalizer) + } + } + if len(modifiedFinalizers) == 0 { + modifiedFinalizers = nil + } + if len(modifiedFinalizers) != len(finalizers) { + modified = true + } + return modifiedFinalizers, modified +} + +func addFinalizer(finalizers []string, finalizerToAdd string) ([]string, bool) { + modifiedFinalizers := make([]string, 0) + for _, finalizer := range finalizers { + if finalizer == finalizerToAdd { + // finalizer already exists + return finalizers, false + } + } + modifiedFinalizers = append(modifiedFinalizers, finalizers...) + modifiedFinalizers = append(modifiedFinalizers, finalizerToAdd) + return modifiedFinalizers, true +} + func logOperation(operation, format string, a ...interface{}) string { return fmt.Sprintf(fmt.Sprintf("%s: %s", operation, format), a...) } diff --git a/controller/controller_test.go b/controller/controller_test.go index f836bae..37f097f 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -64,7 +64,7 @@ var ( // TODO clean this up, e.g. remove redundant params (provisionerName: "foo.bar/baz") func TestController(t *testing.T) { var reflectorCallCount int - + timestamp := metav1.NewTime(time.Now()) tests := []struct { name string objs []runtime.Object @@ -81,6 +81,8 @@ func TestController(t *testing.T) { volumeQueueStore bool expectedStoredVolumes []*v1.PersistentVolume expectedMetrics testMetrics + deletionTimestamp *metav1.Time + addFinalizer bool }{ { name: "provision for claim-1 but not claim-2", @@ -93,7 +95,7 @@ func TestController(t *testing.T) { provisionerName: "foo.bar/baz", provisioner: newTestProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, expectedMetrics: testMetrics{ provisioned: counts{ @@ -106,12 +108,12 @@ func TestController(t *testing.T) { objs: []runtime.Object{ newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), - newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, provisionerName: "foo.bar/baz", provisioner: newTestProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, expectedMetrics: testMetrics{ provisioned: counts{ @@ -143,7 +145,7 @@ func TestController(t *testing.T) { additionalProvisionerNames: []string{"foo.bar/baz", "foo.xyz/baz"}, provisioner: newTestProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, expectedMetrics: testMetrics{ provisioned: counts{ @@ -154,13 +156,13 @@ func TestController(t *testing.T) { { name: "delete volume-1 but not volume-2", objs: []runtime.Object{ - newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), - newVolume("volume-2", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "abc.def/ghi"}), + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), + newVolume("volume-2", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "abc.def/ghi"}, nil, nil), }, provisionerName: "foo.bar/baz", provisioner: newTestProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newVolume("volume-2", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "abc.def/ghi"}), + *newVolume("volume-2", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "abc.def/ghi"}, nil, nil), }, expectedMetrics: testMetrics{ deleted: counts{ @@ -194,23 +196,23 @@ func TestController(t *testing.T) { { name: "don't delete volume-1 because it's still bound", objs: []runtime.Object{ - newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, provisionerName: "foo.bar/baz", provisioner: newTestProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + *newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, }, { name: "don't delete volume-1 because its reclaim policy is not delete", objs: []runtime.Object{ - newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, provisionerName: "foo.bar/baz", provisioner: newTestProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, }, { @@ -231,12 +233,12 @@ func TestController(t *testing.T) { { name: "provisioner fails to delete volume-1: pv is not deleted", objs: []runtime.Object{ - newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, provisionerName: "foo.bar/baz", provisioner: newBadTestProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, expectedMetrics: testMetrics{ deleted: counts{ @@ -266,7 +268,7 @@ func TestController(t *testing.T) { { name: "try to delete volume-1 but fail to delete the pv object", objs: []runtime.Object{ - newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, provisionerName: "foo.bar/baz", provisioner: newTestProvisioner(), @@ -275,7 +277,7 @@ func TestController(t *testing.T) { return true, nil, errors.New("fake error") }, expectedVolumes: []v1.PersistentVolume{ - *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, expectedMetrics: testMetrics{ deleted: counts{ @@ -307,7 +309,7 @@ func TestController(t *testing.T) { provisionerName: "foo.bar/baz", provisioner: newIgnoredProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, expectedMetrics: testMetrics{ provisioned: counts{ @@ -324,7 +326,7 @@ func TestController(t *testing.T) { provisionerName: "foo.bar/baz", provisioner: newTestProvisioner(), expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolumeWithReclaimPolicy(newStorageClassWithReclaimPolicy("class-1", "foo.bar/baz", v1.PersistentVolumeReclaimRetain), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolumeWithReclaimPolicy(newStorageClassWithReclaimPolicy("class-1", "foo.bar/baz", v1.PersistentVolumeReclaimRetain), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, expectedMetrics: testMetrics{ provisioned: counts{ @@ -341,7 +343,7 @@ func TestController(t *testing.T) { provisionerName: "foo.bar/baz", provisioner: newProvisioner(t, "pvc-uid-1-1", ProvisioningFinished, nil), expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, expectedMetrics: testMetrics{ provisioned: counts{ @@ -461,7 +463,7 @@ func TestController(t *testing.T) { provisioner: newProvisioner(t, "pvc-uid-1-1", ProvisioningFinished, nil), expectedClaimsInProgress: []string{}, expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, expectedMetrics: testMetrics{ provisioned: counts{ @@ -484,7 +486,7 @@ func TestController(t *testing.T) { expectedVolumes: []v1.PersistentVolume(nil), volumeQueueStore: true, expectedStoredVolumes: []*v1.PersistentVolume{ - newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, expectedMetrics: testMetrics{ provisioned: counts{ @@ -509,7 +511,7 @@ func TestController(t *testing.T) { return false, nil, nil }, expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, volumeQueueStore: true, expectedStoredVolumes: []*v1.PersistentVolume{}, @@ -592,6 +594,126 @@ func TestController(t *testing.T) { }, }, }, + { + name: "provision for ext provisioner with finalizer", + objs: []runtime.Object{ + newStorageClass("class-1", "foo.bar/baz"), + newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), + }, + addFinalizer: true, + provisionerName: "foo.bar/baz", + provisioner: newProvisioner(t, "pvc-uid-1-1", ProvisioningFinished, nil), + expectedVolumes: []v1.PersistentVolume{ + *newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), []string{finalizerPV}), + }, + expectedMetrics: testMetrics{ + provisioned: counts{ + "class-1": count{success: 1}, + }, + }, + }, + { + name: "ensure finalizer is removed if the addFinalizer config option is false", + objs: []runtime.Object{ + newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + }, + addFinalizer: false, + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + expectedVolumes: []v1.PersistentVolume{ + *newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), + }, + }, + { + name: "ensure finalizer is removed if the reclaim policy is Retain or Recycle with addFinalizer enabled", + objs: []runtime.Object{ + newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + }, + addFinalizer: true, + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + expectedVolumes: []v1.PersistentVolume{ + *newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), + }, + }, + { + name: "ensure finalizer is not added if the volume is under deletion, also ensures that volume is not deleted", + objs: []runtime.Object{ + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, ×tamp), + }, + addFinalizer: true, + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + expectedVolumes: []v1.PersistentVolume{ + *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, ×tamp), + }, + }, + { + name: "ensure volume with finalizer is deleted if it is in a Released state and reclaim policy is Delete", + objs: []runtime.Object{ + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + }, + addFinalizer: true, + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + expectedMetrics: testMetrics{ + deleted: counts{ + "": count{success: 1}, + }, + }, + }, + { + name: "ensure volume with finalizer is deleted if it is in a Released state and reclaim policy is Delete and already under deletion", + objs: []runtime.Object{ + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, ×tamp), + }, + addFinalizer: true, + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + expectedMetrics: testMetrics{ + deleted: counts{ + "": count{success: 1}, + }, + }, + }, + { + name: "provisioner fails to delete the volume with finalizer, the pv is not deleted", + objs: []runtime.Object{ + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + }, + addFinalizer: true, + provisionerName: "foo.bar/baz", + provisioner: newBadTestProvisioner(), + expectedVolumes: []v1.PersistentVolume{ + *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + }, + expectedMetrics: testMetrics{ + deleted: counts{ + "": count{failed: 1}, + }, + }, + }, + { + name: "volume deletion succeeds but the pv deletion fails, the pv still exists", + objs: []runtime.Object{ + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + }, + addFinalizer: true, + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + verbs: []string{"delete"}, + reaction: func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake error") + }, + expectedVolumes: []v1.PersistentVolume{ + *newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + }, + expectedMetrics: testMetrics{ + deleted: counts{ + "": count{failed: 1}, + }, + }, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -605,10 +727,14 @@ func TestController(t *testing.T) { } var ctrl testProvisionController + provisionerOptions := make([]func(*ProvisionController) error, 0) + if test.addFinalizer { + provisionerOptions = append(provisionerOptions, AddFinalizer(true)) + } if test.additionalProvisionerNames == nil { - ctrl = newTestProvisionController(client, test.provisionerName, test.provisioner) + ctrl = newTestProvisionController(client, test.provisionerName, test.provisioner, provisionerOptions...) } else { - ctrl = newTestProvisionControllerWithAdditionalNames(client, test.provisionerName, test.provisioner, test.additionalProvisionerNames) + ctrl = newTestProvisionControllerWithAdditionalNames(client, test.provisionerName, test.provisioner, test.additionalProvisionerNames, provisionerOptions...) } for _, claim := range test.claimsInProgress { ctrl.claimsInProgress.Store(string(claim.UID), claim) @@ -948,66 +1074,63 @@ func TestShouldDelete(t *testing.T) { { name: "should delete", provisionerName: "foo.bar/baz", - volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), expectedShould: true, }, { name: "failed: shouldn't delete", provisionerName: "foo.bar/baz", - volume: newVolume("volume-1", v1.VolumeFailed, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + volume: newVolume("volume-1", v1.VolumeFailed, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), expectedShould: false, }, { name: "volume still bound", provisionerName: "foo.bar/baz", - volume: newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + volume: newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), expectedShould: false, }, { name: "non-delete reclaim policy", provisionerName: "foo.bar/baz", - volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), expectedShould: false, }, { name: "not this provisioner's job", provisionerName: "foo.bar/baz", - volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "abc.def/ghi"}), + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "abc.def/ghi"}, nil, nil), expectedShould: false, }, { name: "non-nil deletion timestamp", provisionerName: "foo.bar/baz", - volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), deletionTimestamp: ×tamp, expectedShould: false, }, { name: "nil deletion timestamp", provisionerName: "foo.bar/baz", - volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), expectedShould: true, }, { name: "migrated to", provisionerName: "csi.driver", - volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, - map[string]string{annDynamicallyProvisioned: "foo.bar/baz", annMigratedTo: "csi.driver"}), - expectedShould: true, + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz", annMigratedTo: "csi.driver"}, nil, nil), + expectedShould: true, }, { name: "migrated to random", provisionerName: "csi.driver", - volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, - map[string]string{annDynamicallyProvisioned: "foo.bar/baz", annMigratedTo: "some.foo.driver"}), - expectedShould: false, + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz", annMigratedTo: "some.foo.driver"}, nil, nil), + expectedShould: false, }, { name: "csidriver but no migrated annotation", provisionerName: "csi.driver", - volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, - map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), - expectedShould: false, + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), + expectedShould: false, }, } for _, test := range tests { @@ -1025,6 +1148,175 @@ func TestShouldDelete(t *testing.T) { } } +func TestShouldDeleteWithFinalizer(t *testing.T) { + timestamp := metav1.NewTime(time.Now()) + tests := []struct { + name string + provisionerName string + volume *v1.PersistentVolume + deletionTimestamp *metav1.Time + expectedShould bool + }{ + { + // Represents a normal deletion where the PVC is deleted and the PV with `Delete` reclaim policy is in + // `Released` state. The PV has the finalizer that was previously added to it by `syncVolume`. + name: "should delete with finalizer present and no deletionTimestamp", + provisionerName: "foo.bar/baz", + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + expectedShould: true, + }, + { + // Represents a scenario where the `syncVolume` has manged to add the finalizer, this is followed by PV + // deletion first, then the PVC deletion, in this case, assuming the PV has `Delete` reclaim policy, the PV + // would be in a `Released` state with deletionTimestamp set. + name: "should delete with finalizer present and with deletionTimestamp", + provisionerName: "foo.bar/baz", + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + deletionTimestamp: ×tamp, + expectedShould: true, + }, + { + // Represents some update on the PV, the PV has the finalizer due to `syncVolume`. For example, annotation + // addition or deletion. + name: "should not delete when volume still bound and the finalizer exists with no deletionTimestamp", + provisionerName: "foo.bar/baz", + volume: newVolume("volume-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{finalizerPV}, nil), + expectedShould: false, + }, + { + // Represents scenario where the reclaim policy is changed to `Retain` or `Recycle`, the syncVolume removes + // the finalizer. Volume should not be deleted in this case, here, the deletionTimestamp does not matter. + name: "should not delete for non-delete reclaim policy", + provisionerName: "foo.bar/baz", + volume: newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimRetain, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, []string{}, nil), + expectedShould: false, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + client := fake.NewSimpleClientset() + provisioner := newTestProvisioner() + provisionerOptions := make([]func(*ProvisionController) error, 0) + provisionerOptions = append(provisionerOptions, AddFinalizer(true)) + ctrl := newTestProvisionController(client, test.provisionerName, provisioner, provisionerOptions...) + test.volume.ObjectMeta.DeletionTimestamp = test.deletionTimestamp + + should := ctrl.shouldDelete(context.Background(), test.volume) + if test.expectedShould != should { + t.Errorf("expected should delete %v but got %v\n", test.expectedShould, should) + } + }) + } +} + +func TestRemoveFinalizer(t *testing.T) { + tests := []struct { + name string + finalizers []string + finalizerToRemove string + expectedFinalizers []string + expectedModified bool + }{ + { + name: "ensure the finalizer removal returns nil finalizers and modified", + finalizers: []string{finalizerPV}, + finalizerToRemove: finalizerPV, + expectedFinalizers: nil, + expectedModified: true, + }, + { + name: "if finalizer does not exist then it should not be modified", + finalizers: []string{"dummy"}, + finalizerToRemove: finalizerPV, + expectedFinalizers: []string{"dummy"}, + expectedModified: false, + }, + { + name: "remove only the finalizerToAdd from multiple finalizers", + finalizers: []string{"dummy", finalizerPV}, + finalizerToRemove: finalizerPV, + expectedFinalizers: []string{"dummy"}, + expectedModified: true, + }, + { + name: "no modified if the original finalizer is empty", + finalizers: []string{}, + finalizerToRemove: finalizerPV, + expectedFinalizers: nil, + expectedModified: false, + }, + { + name: "remove from nil finalizer", + finalizers: nil, + finalizerToRemove: finalizerPV, + expectedFinalizers: nil, + expectedModified: false, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + + modifiedFinalizers, modified := removeFinalizer(test.finalizers, test.finalizerToRemove) + if test.expectedModified != modified { + t.Errorf("expected modified %v but got %v\n", test.expectedModified, modified) + } + if !reflect.DeepEqual(test.expectedFinalizers, modifiedFinalizers) { + t.Errorf("expected finalizers %v but got %v\n", test.expectedFinalizers, modifiedFinalizers) + } + }) + } +} + +func TestAddFinalizer(t *testing.T) { + tests := []struct { + name string + finalizers []string + finalizerToAdd string + expectedFinalizers []string + expectedModified bool + }{ + { + name: "if finalizer already exists then no modification needed", + finalizers: []string{finalizerPV}, + finalizerToAdd: finalizerPV, + expectedFinalizers: []string{finalizerPV}, + expectedModified: false, + }, + { + name: "if original finalizer is nil then return with finalizer to add", + finalizers: nil, + finalizerToAdd: finalizerPV, + expectedFinalizers: []string{finalizerPV}, + expectedModified: true, + }, + { + name: "if original finalizer is empty then return with finalizer to add", + finalizers: []string{}, + finalizerToAdd: finalizerPV, + expectedFinalizers: []string{finalizerPV}, + expectedModified: true, + }, + { + name: "ensure finalizers are appended to existing finalizer", + finalizers: []string{"dummy"}, + finalizerToAdd: finalizerPV, + expectedFinalizers: []string{"dummy", finalizerPV}, + expectedModified: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + modifiedFinalizers, modified := addFinalizer(test.finalizers, test.finalizerToAdd) + if test.expectedModified != modified { + t.Errorf("expected modified %v but got %v\n", test.expectedModified, modified) + } + if !reflect.DeepEqual(test.expectedFinalizers, modifiedFinalizers) { + t.Errorf("expected finalizers %v but got %v\n", test.expectedFinalizers, modifiedFinalizers) + } + }) + } +} + func TestCanProvision(t *testing.T) { const ( provisionerName = "foo.bar/baz" @@ -1123,13 +1415,13 @@ func TestControllerSharedInformers(t *testing.T) { }, provisionerName: "foo.bar/baz", expectedVolumes: []v1.PersistentVolume{ - *newProvisionedVolumeWithReclaimPolicy(newStorageClassWithReclaimPolicy("class-1", "foo.bar/baz", v1.PersistentVolumeReclaimDelete), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), + *newProvisionedVolumeWithReclaimPolicy(newStorageClassWithReclaimPolicy("class-1", "foo.bar/baz", v1.PersistentVolumeReclaimDelete), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), }, }, { name: "delete volume-1", objs: []runtime.Object{ - newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}), + newVolume("volume-1", v1.VolumeReleased, v1.PersistentVolumeReclaimDelete, map[string]string{annDynamicallyProvisioned: "foo.bar/baz"}, nil, nil), }, provisionerName: "foo.bar/baz", expectedVolumes: []v1.PersistentVolume{}, @@ -1224,18 +1516,25 @@ func newTestProvisionController( client kubernetes.Interface, provisionerName string, provisioner Provisioner, + options ...func(*ProvisionController) error, ) testProvisionController { m := metrics.New(string(uuid.NewUUID())) + provisionerOptions := []func(*ProvisionController) error{ + MetricsInstance(m), + ResyncPeriod(resyncPeriod), + CreateProvisionedPVInterval(10 * time.Millisecond), + LeaseDuration(2 * resyncPeriod), + RenewDeadline(resyncPeriod), + RetryPeriod(resyncPeriod / 2), + } + if len(options) > 0 { + provisionerOptions = append(provisionerOptions, options...) + } ctrl := NewProvisionController( client, provisionerName, provisioner, - MetricsInstance(m), - ResyncPeriod(resyncPeriod), - CreateProvisionedPVInterval(10*time.Millisecond), - LeaseDuration(2*resyncPeriod), - RenewDeadline(resyncPeriod), - RetryPeriod(resyncPeriod/2)) + provisionerOptions...) return testProvisionController{ ProvisionController: ctrl, metrics: &m, @@ -1247,19 +1546,26 @@ func newTestProvisionControllerWithAdditionalNames( provisionerName string, provisioner Provisioner, additionalProvisionerNames []string, + options ...func(*ProvisionController) error, ) testProvisionController { m := metrics.New(string(uuid.NewUUID())) + provisionerOptions := []func(*ProvisionController) error{ + MetricsInstance(m), + ResyncPeriod(resyncPeriod), + CreateProvisionedPVInterval(10 * time.Millisecond), + LeaseDuration(2 * resyncPeriod), + RenewDeadline(resyncPeriod), + RetryPeriod(resyncPeriod / 2), + AdditionalProvisionerNames(additionalProvisionerNames), + } + if len(options) > 0 { + provisionerOptions = append(provisionerOptions, options...) + } ctrl := NewProvisionController( client, provisionerName, provisioner, - MetricsInstance(m), - ResyncPeriod(resyncPeriod), - CreateProvisionedPVInterval(10*time.Millisecond), - LeaseDuration(2*resyncPeriod), - RenewDeadline(resyncPeriod), - RetryPeriod(resyncPeriod/2), - AdditionalProvisionerNames(additionalProvisionerNames)) + provisionerOptions...) return testProvisionController{ ProvisionController: ctrl, metrics: &m, @@ -1388,12 +1694,15 @@ func newClaimWithVolumeMode(name, claimUID, class, provisioner, volumeName strin return claim } -func newVolume(name string, phase v1.PersistentVolumePhase, policy v1.PersistentVolumeReclaimPolicy, annotations map[string]string) *v1.PersistentVolume { +func newVolume(name string, phase v1.PersistentVolumePhase, policy v1.PersistentVolumeReclaimPolicy, + annotations map[string]string, finalizers []string, deletionTimestamp *metav1.Time) *v1.PersistentVolume { pv := &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ - Name: name, - Annotations: annotations, - SelfLink: "/api/v1/persistentvolumes/" + name, + Name: name, + Annotations: annotations, + Finalizers: finalizers, + DeletionTimestamp: deletionTimestamp, + SelfLink: "/api/v1/persistentvolumes/" + name, }, Spec: v1.PersistentVolumeSpec{ PersistentVolumeReclaimPolicy: policy, @@ -1413,31 +1722,30 @@ func newVolume(name string, phase v1.PersistentVolumePhase, policy v1.Persistent 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) *v1.PersistentVolume { +func newProvisionedVolume(storageClass *storage.StorageClass, claim *v1.PersistentVolumeClaim, pvFinalizers []string) *v1.PersistentVolume { volume := constructProvisionedVolumeWithoutStorageClassInfo(claim, v1.PersistentVolumeReclaimDelete) // pv.Annotations["pv.kubernetes.io/provisioned-by"] MUST be set to name of the external provisioner. This provisioner will be used to delete the volume. volume.Annotations = map[string]string{annDynamicallyProvisioned: storageClass.Provisioner} // pv.Spec.StorageClassName must be set to the name of the storage class requested by the claim volume.Spec.StorageClassName = storageClass.Name - + volume.ObjectMeta.Finalizers = pvFinalizers return volume } -func newProvisionedVolumeWithReclaimPolicy(storageClass *storage.StorageClass, claim *v1.PersistentVolumeClaim) *v1.PersistentVolume { +func newProvisionedVolumeWithReclaimPolicy(storageClass *storage.StorageClass, claim *v1.PersistentVolumeClaim, pvFinalizers []string) *v1.PersistentVolume { volume := constructProvisionedVolumeWithoutStorageClassInfo(claim, *storageClass.ReclaimPolicy) // pv.Annotations["pv.kubernetes.io/provisioned-by"] MUST be set to name of the external provisioner. This provisioner will be used to delete the volume. volume.Annotations = map[string]string{annDynamicallyProvisioned: storageClass.Provisioner} // pv.Spec.StorageClassName must be set to the name of the storage class requested by the claim volume.Spec.StorageClassName = storageClass.Name - + volume.ObjectMeta.Finalizers = pvFinalizers return volume } @@ -1633,7 +1941,7 @@ func (i *ignoredProvisioner) Provision(ctx context.Context, options ProvisionOpt return nil, ProvisioningFinished, &IgnoredError{"Ignored"} } - return newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), ProvisioningFinished, nil + return newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil), nil), ProvisioningFinished, nil } func (i *ignoredProvisioner) Delete(ctx context.Context, volume *v1.PersistentVolume) error {