Repository: kubernetes-csi/external-snapshotter (go)

Issue:
VolumeSnapshot cannot be deleted
Kubernetes version: v1.18.5
CSI snapshotter version: v2.1.1

ouptput of `kubectl get volumesnapshot`
```log
NAME                              READYTOUSE   SOURCEPVC          SOURCESNAPSHOTCONTENT   RESTORESIZE   SNAPSHOTCLASS               SNAPSHOTCONTENT                                    CREATIONTIME   AGE
test-snapshot-dynamic-provision   false        quobyte-csi-test                                         quobyte-csi-snapshotclass   snapcontent-e5af5c7e-89bd-42a4-bd5d-e00f205f243c                  14h
```
events from `kubectl describe volumesnapshot`
```log
Events:
  Type     Reason                         Age   From                 Message
  ----     ------                         ----  ----                 -------
  Warning  SnapshotContentCreationFailed  54m   snapshot-controller  Failed to create snapshot content with error snapshot controller failed to update quobyte-csi-test on API server: cannot add finalizer on claim because it is being deleted
```

logs from `snapshot-controller`
```log
I0811 09:58:46.942919       1 snapshot_controller.go:893] Keeping PVC default/quobyte-csi-test, it is used by snapshot default/test-snapshot-dynamic-provision
I0811 09:58:46.942935       1 snapshot_controller.go:229] PVC default/quobyte-csi-test is being used by snapshot default/test-snapshot-dynamic-provision as source
I0811 09:58:46.942945       1 snapshot_controller.go:609] createSnapshotContent: Creating content for snapshot default/test-snapshot-dynamic-provision through the plugin ...
E0811 09:58:46.942953       1 snapshot_controller.go:838] cannot add finalizer on claim [quobyte-csi-test] for snapshot [test-snapshot-dynamic-provision]: claim is being deleted
E0811 09:58:46.942957       1 snapshot_controller.go:615] createSnapshotContent failed to add finalizer for source of snapshot snapshot controller failed to update quobyte-csi-test on API server: cannot add finalizer on claim because it is being deleted
E0811 09:58:46.942973       1 snapshot_controller_base.go:378] could not sync volume "default/test-snapshot-dynamic-provision": snapshot controller failed to update quobyte-csi-test on API server: cannot add finalizer on claim because it is being deleted
I0811 09:59:46.850093       1 snapshot_controller.go:893] Keeping PVC default/quobyte-csi-test, it is used by snapshot default/test-snapshot-dynamic-provision
I0811 09:59:46.850107       1 snapshot_controller.go:229] PVC default/quobyte-csi-test is being used by snapshot default/test-snapshot-dynamic-provision as source
I0811 09:59:46.850113       1 snapshot_controller.go:609] createSnapshotContent: Creating content for snapshot default/test-snapshot-dynamic-provision through the plugin ...
E0811 09:59:46.850117       1 snapshot_controller.go:838] cannot add finalizer on claim [quobyte-csi-test] for snapshot [test-snapshot-dynamic-provision]: claim is being deleted
E0811 09:59:46.850120       1 snapshot_controller.go:615] createSnapshotContent failed to add finalizer for source of snapshot snapshot controller failed to update quobyte-csi-test on API server: cannot add finalizer on claim because it is being deleted
E0811 09:59:46.850127       1 snapshot_controller_base.go:378] could not sync volume "default/test-snapshot-dynamic-provision": snapshot controller failed to update quobyte-csi-test on API server: cannot add finalizer on claim because it is being deleted
```

PVC status
```
NAME               STATUS        VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
quobyte-csi-test   Terminating   pvc-39577f6c-c107-4417-9da8-0cd559441d76   1Gi        RWO            quobyte-csi    15h
````

snapshotter sidecar does not have any relevant logs.

`Workaround:` Setting finalizers to null as below on the PVC on which the snapshot is created, removed both PVC and volume snapshot.
```
# Set finalizers on the PVC to null
kubectl get pvc <PVC> -o=json | jq '.metadata.finalizers = null' | kubectl apply -f -
```

Pull request description:
Fix the race between PVC finalizer and snapshot finalizer removal
**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
Fixes #349 

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
```release-note
Fix a problem deleting the PVC finalizer and snapshot finalizer.
```


Patch:
diff --git a/go.mod b/go.mod
index 5b2357ec7..5fd39af33 100644
--- a/go.mod
+++ b/go.mod
@@ -1,6 +1,6 @@
 module github.com/kubernetes-csi/external-snapshotter/v2
 
-go 1.15
+go 1.12
 
 require (
 	github.com/container-storage-interface/spec v1.2.0
diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go
index c02ab2618..803e94949 100644
--- a/pkg/common-controller/snapshot_controller.go
+++ b/pkg/common-controller/snapshot_controller.go
@@ -182,7 +182,7 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap
 	klog.V(5).Infof("syncSnapshot [%s]: check if we should remove finalizer on snapshot PVC source and remove it if we can", utils.SnapshotKey(snapshot))
 
 	// Check if we should remove finalizer on PVC and remove it if we can.
-	if err := ctrl.checkandRemovePVCFinalizer(snapshot); err != nil {
+	if err := ctrl.checkandRemovePVCFinalizer(snapshot, false); err != nil {
 		klog.Errorf("error check and remove PVC finalizer for snapshot [%s]: %v", snapshot.Name, err)
 		// Log an event and keep the original error from checkandRemovePVCFinalizer
 		ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "ErrorPVCFinalizer", "Error check and remove PVC Finalizer for VolumeSnapshot")
@@ -198,11 +198,8 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap
 		return err
 	}
 
-	// Proceed with snapshot deletion only if snapshot is not in the middled of being
-	// 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) {
+	// Proceed with snapshot deletion and remove finalizers when needed
+	if snapshot.ObjectMeta.DeletionTimestamp != nil {
 		return ctrl.processSnapshotWithDeletionTimestamp(snapshot)
 	}
 
@@ -234,27 +231,6 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap
 	return ctrl.syncReadySnapshot(snapshot)
 }
 
-// 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
-	}
-
-	// Check if there is a Finalizer on PVC. If not, return false
-	if !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) {
-		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
-	}
-	return false
-}
-
 // processSnapshotWithDeletionTimestamp processes finalizers and deletes the content when appropriate. It has the following steps:
 // 1. Get the content which the to-be-deleted VolumeSnapshot points to and verifies bi-directional binding.
 // 2. Call checkandRemoveSnapshotFinalizersAndCheckandDeleteContent() with information obtained from step 1. This function name is very long but the name suggests what it does. It determines whether to remove finalizers on snapshot and whether to delete content.
@@ -858,7 +834,7 @@ func (ctrl *csiSnapshotCommonController) ensurePVCFinalizer(snapshot *crdv1.Volu
 }
 
 // removePVCFinalizer removes a Finalizer for VolumeSnapshot Source PVC.
-func (ctrl *csiSnapshotCommonController) removePVCFinalizer(pvc *v1.PersistentVolumeClaim, snapshot *crdv1.VolumeSnapshot) error {
+func (ctrl *csiSnapshotCommonController) removePVCFinalizer(pvc *v1.PersistentVolumeClaim) error {
 	// Get snapshot source which is a PVC
 	// TODO(xyang): We get PVC from informer but it may be outdated
 	// Should get it from API server directly before removing finalizer
@@ -874,8 +850,9 @@ func (ctrl *csiSnapshotCommonController) removePVCFinalizer(pvc *v1.PersistentVo
 	return nil
 }
 
-// isPVCBeingUsed checks if a PVC is being used as a source to create a snapshot
-func (ctrl *csiSnapshotCommonController) isPVCBeingUsed(pvc *v1.PersistentVolumeClaim, snapshot *crdv1.VolumeSnapshot) bool {
+// isPVCBeingUsed checks if a PVC is being used as a source to create a snapshot.
+// If skipCurrentSnapshot is true, skip checking if the current snapshot is using the PVC as source.
+func (ctrl *csiSnapshotCommonController) isPVCBeingUsed(pvc *v1.PersistentVolumeClaim, snapshot *crdv1.VolumeSnapshot, skipCurrentSnapshot bool) bool {
 	klog.V(5).Infof("Checking isPVCBeingUsed for snapshot [%s]", utils.SnapshotKey(snapshot))
 
 	// Going through snapshots in the cache (snapshotLister). If a snapshot's PVC source
@@ -886,6 +863,10 @@ func (ctrl *csiSnapshotCommonController) isPVCBeingUsed(pvc *v1.PersistentVolume
 		return false
 	}
 	for _, snap := range snapshots {
+		// Skip the current snapshot
+		if skipCurrentSnapshot && snap.Name == snapshot.Name {
+			continue
+		}
 		// Skip pre-provisioned snapshot without a PVC source
 		if snap.Spec.Source.PersistentVolumeClaimName == nil && snap.Spec.Source.VolumeSnapshotContentName != nil {
 			klog.V(4).Infof("Skipping static bound snapshot %s when checking PVC %s/%s", snap.Name, pvc.Namespace, pvc.Name)
@@ -902,8 +883,9 @@ func (ctrl *csiSnapshotCommonController) isPVCBeingUsed(pvc *v1.PersistentVolume
 }
 
 // checkandRemovePVCFinalizer checks if the snapshot source finalizer should be removed
-// and removed it if needed.
-func (ctrl *csiSnapshotCommonController) checkandRemovePVCFinalizer(snapshot *crdv1.VolumeSnapshot) error {
+// and removed it if needed. If skipCurrentSnapshot is true, skip checking if the current
+// snapshot is using the PVC as source.
+func (ctrl *csiSnapshotCommonController) checkandRemovePVCFinalizer(snapshot *crdv1.VolumeSnapshot, skipCurrentSnapshot bool) error {
 	if snapshot.Spec.Source.PersistentVolumeClaimName == nil {
 		// PVC finalizer is only needed for dynamic provisioning
 		return nil
@@ -922,10 +904,10 @@ func (ctrl *csiSnapshotCommonController) checkandRemovePVCFinalizer(snapshot *cr
 	if utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) {
 		// There is a Finalizer on PVC. Check if PVC is used
 		// and remove finalizer if it's not used.
-		inUse := ctrl.isPVCBeingUsed(pvc, snapshot)
+		inUse := ctrl.isPVCBeingUsed(pvc, snapshot, skipCurrentSnapshot)
 		if !inUse {
 			klog.Infof("checkandRemovePVCFinalizer[%s]: Remove Finalizer for PVC %s as it is not used by snapshots in creation", snapshot.Name, pvc.Name)
-			err = ctrl.removePVCFinalizer(pvc, snapshot)
+			err = ctrl.removePVCFinalizer(pvc)
 			if err != nil {
 				klog.Errorf("checkandRemovePVCFinalizer [%s]: removePVCFinalizer failed to remove finalizer %v", snapshot.Name, err)
 				return err
@@ -1325,6 +1307,23 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1
 		return nil
 	}
 
+	// NOTE(xyang): We have to make sure PVC finalizer is deleted before
+	// the VolumeSnapshot API object is deleted. Once the VolumeSnapshot
+	// API object is deleted, there won't be any VolumeSnapshot update
+	// event that can trigger the PVC finalizer removal any more.
+	// We also can't remove PVC finalizer too early. PVC finalizer should
+	// not be removed if a VolumeSnapshot API object is still using it.
+	// If we are here, it means we are going to remove finalizers from the
+	// VolumeSnapshot API object so that the VolumeSnapshot API object can
+	// be deleted. This means we no longer need to keep the PVC finalizer
+	// for this particular snapshot.
+	if err := ctrl.checkandRemovePVCFinalizer(snapshot, true); err != nil {
+		klog.Errorf("removeSnapshotFinalizer: error check and remove PVC finalizer for snapshot [%s]: %v", snapshot.Name, err)
+		// Log an event and keep the original error from checkandRemovePVCFinalizer
+		ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "ErrorPVCFinalizer", "Error check and remove PVC Finalizer for VolumeSnapshot")
+		return newControllerUpdateError(snapshot.Name, err.Error())
+	}
+
 	snapshotClone := snapshot.DeepCopy()
 	if removeSourceFinalizer {
 		snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer)
diff --git a/vendor/modules.txt b/vendor/modules.txt
index e839ee41b..32da67251 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -5,7 +5,6 @@ github.com/blang/semver
 # github.com/cespare/xxhash/v2 v2.1.1
 github.com/cespare/xxhash/v2
 # github.com/container-storage-interface/spec v1.2.0
-## explicit
 github.com/container-storage-interface/spec/lib/go/csi
 # github.com/davecgh/go-spew v1.1.1
 github.com/davecgh/go-spew/spew
@@ -19,10 +18,8 @@ github.com/gogo/protobuf/sortkeys
 # github.com/golang/groupcache v0.0.0-20191227052852-215e87163ea7
 github.com/golang/groupcache/lru
 # github.com/golang/mock v1.4.3
-## explicit
 github.com/golang/mock/gomock
 # github.com/golang/protobuf v1.4.2
-## explicit
 github.com/golang/protobuf/descriptor
 github.com/golang/protobuf/proto
 github.com/golang/protobuf/protoc-gen-go/descriptor
@@ -38,7 +35,6 @@ github.com/google/go-cmp/cmp/internal/flags
 github.com/google/go-cmp/cmp/internal/function
 github.com/google/go-cmp/cmp/internal/value
 # github.com/google/gofuzz v1.1.0
-## explicit
 github.com/google/gofuzz
 # github.com/googleapis/gnostic v0.4.1
 github.com/googleapis/gnostic/compiler
@@ -48,25 +44,21 @@ github.com/googleapis/gnostic/openapiv2
 github.com/hashicorp/golang-lru
 github.com/hashicorp/golang-lru/simplelru
 # github.com/imdario/mergo v0.3.9
-## explicit
 github.com/imdario/mergo
 # github.com/inconshreveable/mousetrap v1.0.0
 github.com/inconshreveable/mousetrap
 # github.com/json-iterator/go v1.1.10
 github.com/json-iterator/go
 # github.com/kubernetes-csi/csi-lib-utils v0.7.0
-## explicit
 github.com/kubernetes-csi/csi-lib-utils/connection
 github.com/kubernetes-csi/csi-lib-utils/leaderelection
 github.com/kubernetes-csi/csi-lib-utils/metrics
 github.com/kubernetes-csi/csi-lib-utils/protosanitizer
 github.com/kubernetes-csi/csi-lib-utils/rpc
 # github.com/kubernetes-csi/csi-test v2.0.0+incompatible
-## explicit
 github.com/kubernetes-csi/csi-test/driver
 github.com/kubernetes-csi/csi-test/utils
 # github.com/kubernetes-csi/external-snapshotter/client/v2 v2.0.0-00010101000000-000000000000 => ./client
-## explicit
 github.com/kubernetes-csi/external-snapshotter/client/v2/apis/volumesnapshot/v1beta1
 github.com/kubernetes-csi/external-snapshotter/client/v2/clientset/versioned
 github.com/kubernetes-csi/external-snapshotter/client/v2/clientset/versioned/fake
@@ -87,15 +79,12 @@ github.com/modern-go/reflect2
 # github.com/pkg/errors v0.9.1
 github.com/pkg/errors
 # github.com/prometheus/client_golang v1.7.1
-## explicit
 github.com/prometheus/client_golang/prometheus
 github.com/prometheus/client_golang/prometheus/internal
 github.com/prometheus/client_golang/prometheus/promhttp
 # github.com/prometheus/client_model v0.2.0
-## explicit
 github.com/prometheus/client_model/go
 # github.com/prometheus/common v0.10.0
-## explicit
 github.com/prometheus/common/expfmt
 github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg
 github.com/prometheus/common/model
@@ -104,7 +93,6 @@ github.com/prometheus/procfs
 github.com/prometheus/procfs/internal/fs
 github.com/prometheus/procfs/internal/util
 # github.com/spf13/cobra v1.0.0
-## explicit
 github.com/spf13/cobra
 # github.com/spf13/pflag v1.0.5
 github.com/spf13/pflag
@@ -144,7 +132,6 @@ google.golang.org/appengine/urlfetch
 # google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013
 google.golang.org/genproto/googleapis/rpc/status
 # google.golang.org/grpc v1.28.0
-## explicit
 google.golang.org/grpc
 google.golang.org/grpc/attributes
 google.golang.org/grpc/backoff
@@ -224,7 +211,6 @@ gopkg.in/inf.v0
 # gopkg.in/yaml.v2 v2.2.8
 gopkg.in/yaml.v2
 # k8s.io/api v0.19.0 => k8s.io/api v0.19.0
-## explicit
 k8s.io/api/admission/v1
 k8s.io/api/admission/v1beta1
 k8s.io/api/admissionregistration/v1
@@ -269,7 +255,6 @@ k8s.io/api/storage/v1
 k8s.io/api/storage/v1alpha1
 k8s.io/api/storage/v1beta1
 # k8s.io/apimachinery v0.19.0 => k8s.io/apimachinery v0.19.0
-## explicit
 k8s.io/apimachinery/pkg/api/apitesting/fuzzer
 k8s.io/apimachinery/pkg/api/errors
 k8s.io/apimachinery/pkg/api/meta
@@ -314,7 +299,6 @@ k8s.io/apimachinery/pkg/watch
 k8s.io/apimachinery/third_party/forked/golang/json
 k8s.io/apimachinery/third_party/forked/golang/reflect
 # k8s.io/client-go v0.19.0 => k8s.io/client-go v0.19.0
-## explicit
 k8s.io/client-go/discovery
 k8s.io/client-go/discovery/fake
 k8s.io/client-go/informers
@@ -526,19 +510,15 @@ k8s.io/client-go/util/homedir
 k8s.io/client-go/util/keyutil
 k8s.io/client-go/util/workqueue
 # k8s.io/component-base v0.19.0 => k8s.io/component-base v0.19.0
-## explicit
 k8s.io/component-base/metrics
 k8s.io/component-base/version
 # k8s.io/klog v1.0.0
-## explicit
 k8s.io/klog
 # k8s.io/klog/v2 v2.2.0
-## explicit
 k8s.io/klog/v2
 # k8s.io/kube-openapi v0.0.0-20200805222855-6aeccd4b50c6
 k8s.io/kube-openapi/pkg/util/proto
 # k8s.io/kubernetes v1.19.0
-## explicit
 k8s.io/kubernetes/pkg/apis/admission/fuzzer
 # k8s.io/utils v0.0.0-20200729134348-d5654de09c73
 k8s.io/utils/buffer
@@ -548,25 +528,3 @@ k8s.io/utils/trace
 sigs.k8s.io/structured-merge-diff/v4/value
 # sigs.k8s.io/yaml v1.2.0
 sigs.k8s.io/yaml
-# github.com/kubernetes-csi/external-snapshotter/client/v2 => ./client
-# k8s.io/api => k8s.io/api v0.19.0
-# k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.19.0
-# k8s.io/apimachinery => k8s.io/apimachinery v0.19.0
-# k8s.io/apiserver => k8s.io/apiserver v0.19.0
-# k8s.io/cli-runtime => k8s.io/cli-runtime v0.19.0
-# k8s.io/client-go => k8s.io/client-go v0.19.0
-# k8s.io/cloud-provider => k8s.io/cloud-provider v0.19.0
-# k8s.io/cluster-bootstrap => k8s.io/cluster-bootstrap v0.19.0
-# k8s.io/code-generator => k8s.io/code-generator v0.19.0
-# k8s.io/component-base => k8s.io/component-base v0.19.0
-# k8s.io/cri-api => k8s.io/cri-api v0.19.0
-# k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.19.0
-# k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.19.0
-# k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.19.0
-# k8s.io/kube-proxy => k8s.io/kube-proxy v0.19.0
-# k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.19.0
-# k8s.io/kubectl => k8s.io/kubectl v0.19.0
-# k8s.io/kubelet => k8s.io/kubelet v0.19.0
-# k8s.io/legacy-cloud-providers => k8s.io/legacy-cloud-providers v0.19.0
-# k8s.io/metrics => k8s.io/metrics v0.19.0
-# k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.19.0


Test patch:
diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go
index 023f52c0f..c69edddd5 100644
--- a/pkg/common-controller/framework_test.go
+++ b/pkg/common-controller/framework_test.go
@@ -1147,7 +1147,7 @@ func testAddPVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotRea
 }
 
 func testRemovePVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
-	return ctrl.checkandRemovePVCFinalizer(test.initialSnapshots[0])
+	return ctrl.checkandRemovePVCFinalizer(test.initialSnapshots[0], false)
 }
 
 func testAddSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {