From 8ae4d0c27882d98fc81c9181dc67e600cbf24ee8 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Wed, 1 Apr 2020 04:23:53 +0000 Subject: [PATCH 1/2] Check if PVC is being used by a snapshot as source --- pkg/common-controller/snapshot_controller.go | 20 ++++++++++++++++++- pkg/common-controller/snapshot_delete_test.go | 12 +++++------ pkg/common-controller/snapshot_update_test.go | 4 ++-- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 467f3240e..ebd472d7d 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -166,7 +166,10 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "ErrorPVCFinalizer", "Error check and remove PVC Finalizer for VolumeSnapshot") } - if snapshot.ObjectMeta.DeletionTimestamp != nil { + // Proceed with snapshot deletion only if snapshot is not in the middled of being + // created from a PVC. This is to ensure that the PVC finalizer can be removed even + // if a delete snapshot request is received before create snapshot has completed. + if snapshot.ObjectMeta.DeletionTimestamp != nil && !ctrl.isPVCInUseByCurrentSnapshot(snapshot) { err := ctrl.processSnapshotWithDeletionTimestamp(snapshot) if err != nil { return err @@ -190,6 +193,21 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap return nil } +// Check if PVC is being used by the current snapshot as source +func (ctrl *csiSnapshotCommonController) isPVCInUseByCurrentSnapshot(snapshot *crdv1.VolumeSnapshot) bool { + // Get snapshot source which is a PVC + pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot) + if err != nil { + klog.Infof("cannot get claim from snapshot [%s]: [%v] Claim may be deleted already.", snapshot.Name, err) + return false + } + if snapshot.Spec.Source.PersistentVolumeClaimName != nil && pvc.Name == *snapshot.Spec.Source.PersistentVolumeClaimName && !utils.IsSnapshotReady(snapshot) { + klog.V(2).Infof("PVC %s/%s is being used by snapshot %s/%s as source", pvc.Namespace, pvc.Name, snapshot.Namespace, snapshot.Name) + return true + } + return false +} + // checkContentAndBoundStatus is a helper function that checks the following: // - It checks if content exists and returns the content object if it exists and nil otherwise. // - It checks the deletionPolicy and returns true if policy is Delete and false otherwise. diff --git a/pkg/common-controller/snapshot_delete_test.go b/pkg/common-controller/snapshot_delete_test.go index 17ce7ffef..35427874e 100644 --- a/pkg/common-controller/snapshot_delete_test.go +++ b/pkg/common-controller/snapshot_delete_test.go @@ -172,8 +172,8 @@ func TestDeleteSync(t *testing.T) { name: "3-1 - content will be deleted if snapshot deletion timestamp is set", initialContents: newContentArray("content3-1", "", "snap3-1", "sid3-1", validSecretClass, "", "", deletePolicy, nil, nil, true), expectedContents: nocontents, - initialSnapshots: newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &False, nil, nil, nil, false, true, &timeNowMetav1), - expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &False, nil, nil, nil, false, false, &timeNowMetav1), + initialSnapshots: newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &True, nil, nil, nil, false, true, &timeNowMetav1), + expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &True, nil, nil, nil, false, false, &timeNowMetav1), utils.VolumeSnapshotBoundFinalizer, ), initialClaims: newClaimArray("claim3-1", "pvc-uid3-1", "1Gi", "volume3-1", v1.ClaimBound, &classEmpty), @@ -186,8 +186,8 @@ func TestDeleteSync(t *testing.T) { name: "3-2 - content will not be deleted if deletion API call fails", initialContents: newContentArray("content3-2", "", "snap3-2", "sid3-2", validSecretClass, "", "", deletePolicy, nil, nil, true), expectedContents: newContentArray("content3-2", "", "snap3-2", "sid3-2", validSecretClass, "", "", deletePolicy, nil, nil, true), - initialSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &False, nil, nil, nil, false, true, &timeNowMetav1), - expectedSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &False, nil, nil, nil, false, true, &timeNowMetav1), + initialSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &True, nil, nil, nil, false, true, &timeNowMetav1), + expectedSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &True, nil, nil, nil, false, true, &timeNowMetav1), initialClaims: newClaimArray("claim3-2", "pvc-uid3-2", "1Gi", "volume3-2", v1.ClaimBound, &classEmpty), expectedEvents: []string{"Warning SnapshotContentObjectDeleteError"}, initialSecrets: []*v1.Secret{secret()}, @@ -206,8 +206,8 @@ func TestDeleteSync(t *testing.T) { map[string]string{ "snapshot.storage.kubernetes.io/volumesnapshot-being-deleted": "yes", }), - initialSnapshots: newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &False, nil, nil, nil, false, true, &timeNowMetav1), - expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &False, nil, nil, nil, false, false, &timeNowMetav1), + initialSnapshots: newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &True, nil, nil, nil, false, true, &timeNowMetav1), + expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &True, nil, nil, nil, false, false, &timeNowMetav1), utils.VolumeSnapshotBoundFinalizer, ), initialClaims: newClaimArray("claim3-3", "pvc-uid3-3", "1Gi", "volume3-3", v1.ClaimBound, &classEmpty), diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go index 7f71ef063..f0f9550c0 100644 --- a/pkg/common-controller/snapshot_update_test.go +++ b/pkg/common-controller/snapshot_update_test.go @@ -315,8 +315,8 @@ func TestSync(t *testing.T) { }, { name: "5-5 - snapshot deletion candidate marked for deletion by syncSnapshot", - initialSnapshots: newSnapshotArray("snap5-5", "snapuid5-5", "claim5-5", "", validSecretClass, "content5-5", &False, nil, nil, nil, false, true, &timeNowMetav1), - expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap5-5", "snapuid5-5", "claim5-5", "", validSecretClass, "content5-5", &False, nil, nil, nil, false, false, &timeNowMetav1), + initialSnapshots: newSnapshotArray("snap5-5", "snapuid5-5", "claim5-5", "", validSecretClass, "content5-5", &True, nil, nil, nil, false, true, &timeNowMetav1), + expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap5-5", "snapuid5-5", "claim5-5", "", validSecretClass, "content5-5", &True, nil, nil, nil, false, false, &timeNowMetav1), utils.VolumeSnapshotBoundFinalizer, ), initialContents: newContentArray("content5-5", "snapuid5-5", "snap5-5", "sid5-5", validSecretClass, "", "", crdv1.VolumeSnapshotContentRetain, nil, nil, true), From c82c8e0c7eae9d8f40803d4dfc500e713f664e6d Mon Sep 17 00:00:00 2001 From: xing-yang Date: Thu, 2 Apr 2020 02:01:52 +0000 Subject: [PATCH 2/2] Add a check to see if PVC has a finalizer --- pkg/common-controller/snapshot_controller.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index ebd472d7d..eaaf1235e 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -167,9 +167,10 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap } // Proceed with snapshot deletion only if snapshot is not in the middled of being - // created from a PVC. This is to ensure that the PVC finalizer can be removed even - // if a delete snapshot request is received before create snapshot has completed. - if snapshot.ObjectMeta.DeletionTimestamp != nil && !ctrl.isPVCInUseByCurrentSnapshot(snapshot) { + // created from a PVC with a finalizer. This is to ensure that the PVC finalizer + // can be removed even if a delete snapshot request is received before create + // snapshot has completed. + if snapshot.ObjectMeta.DeletionTimestamp != nil && !ctrl.isPVCwithFinalizerInUseByCurrentSnapshot(snapshot) { err := ctrl.processSnapshotWithDeletionTimestamp(snapshot) if err != nil { return err @@ -193,15 +194,21 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap return nil } -// Check if PVC is being used by the current snapshot as source -func (ctrl *csiSnapshotCommonController) isPVCInUseByCurrentSnapshot(snapshot *crdv1.VolumeSnapshot) bool { +// Check if PVC has a finalizer and if it is being used by the current snapshot as source. +func (ctrl *csiSnapshotCommonController) isPVCwithFinalizerInUseByCurrentSnapshot(snapshot *crdv1.VolumeSnapshot) bool { // Get snapshot source which is a PVC pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot) if err != nil { klog.Infof("cannot get claim from snapshot [%s]: [%v] Claim may be deleted already.", snapshot.Name, err) return false } - if snapshot.Spec.Source.PersistentVolumeClaimName != nil && pvc.Name == *snapshot.Spec.Source.PersistentVolumeClaimName && !utils.IsSnapshotReady(snapshot) { + + // Check if there is a Finalizer on PVC. If not, return false + if !slice.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) { + return false + } + + if !utils.IsSnapshotReady(snapshot) { klog.V(2).Infof("PVC %s/%s is being used by snapshot %s/%s as source", pvc.Namespace, pvc.Name, snapshot.Namespace, snapshot.Name) return true }