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

Issue:
Cannot delete VolumeSnapshotContent with 'Retain' policy for imported snapshot using external-snapshotter beta
**Kubernetes version:**
```
Client Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.2", GitCommit:"59603c6e503c87169aea6106f57b9f242f64df89", GitTreeState:"clean", BuildDate:"2020-01-18T23:30:10Z", GoVersion:"go1.13.5", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"17+", GitVersion:"v1.17.0-gke.5", GitCommit:"ef996ac10bf4abe5cff462781fe9508d7468b657", GitTreeState:"clean", BuildDate:"2020-01-30T19:02:11Z", GoVersion:"go1.13.4b4", Compiler:"gc", Platform:"linux/amd64"}
```

**Snapshot common controller image:**
```
quay.io/k8scsi/snapshot-controller:v2.0.0
```

**snapshot sidercar controller image:**
```
quay.io/k8scsi/csi-snapshotter:v2.0.0
```

**Reproduce steps:**
1. Create VolumeSnapshotContent with snapshot handle, the DeletionPolicy is **Retain**:
```
Name:         snapshotcontent-static-binding
Namespace:    
Labels:       <none>
Annotations:  kubectl.kubernetes.io/last-applied-configuration:
                {"apiVersion":"snapshot.storage.k8s.io/v1beta1","kind":"VolumeSnapshotContent","metadata":{"annotations":{},"name":"snapshotcontent-static...
API Version:  snapshot.storage.k8s.io/v1beta1
Kind:         VolumeSnapshotContent
Metadata:
  Creation Timestamp:  2020-02-07T01:13:49Z
  Generation:          1
  Resource Version:    967234
  Self Link:           /apis/snapshot.storage.k8s.io/v1beta1/volumesnapshotcontents/snapshotcontent-static-binding
  UID:                 8e2e5e8b-005d-4719-8397-a1527c5da009
Spec:
  Deletion Policy:  Retain
  Driver:           pd.csi.storage.gke.io
  Source:
    Snapshot Handle:  <handle>
  Volume Snapshot Ref:
    Name:       snapshot-static-binding
    Namespace:  default
Status:
  Creation Time:    1580448624526000000
  Ready To Use:     true
  Restore Size:     5368709120
  Snapshot Handle:  <handle>
Events:             <none>
```

2. Create VolumeSnapshot bound to content:
```
Name:         snapshot-static-binding
Namespace:    default
Labels:       <none>
Annotations:  kubectl.kubernetes.io/last-applied-configuration:
                {"apiVersion":"snapshot.storage.k8s.io/v1beta1","kind":"VolumeSnapshot","metadata":{"annotations":{},"name":"snapshot-static-binding","nam...
API Version:  snapshot.storage.k8s.io/v1beta1
Kind:         VolumeSnapshot
Metadata:
  Creation Timestamp:  2020-02-07T01:15:00Z
  Finalizers:
    snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection
  Generation:        1
  Resource Version:  967584
  Self Link:         /apis/snapshot.storage.k8s.io/v1beta1/namespaces/default/volumesnapshots/snapshot-static-binding
  UID:               c06ceffa-0581-4c94-9a8d-c8458d526209
Spec:
  Source:
    Volume Snapshot Content Name:  snapshotcontent-static-binding
Status:
  Bound Volume Snapshot Content Name:  snapshotcontent-static-binding
  Creation Time:                       2020-01-31T05:30:24Z
  Ready To Use:                        true
  Restore Size:                        5Gi
Events:                                <none>
```

3. Delete VolumeSnapshot, and it is gone successfully.
4. Delete VolumeSnapshotContent, the finalizer is never removed from content:
```
Name:         snapshotcontent-static-binding
Namespace:    
Labels:       <none>
Annotations:  kubectl.kubernetes.io/last-applied-configuration:
                {"apiVersion":"snapshot.storage.k8s.io/v1beta1","kind":"VolumeSnapshotContent","metadata":{"annotations":{},"name":"snapshotcontent-static...
API Version:  snapshot.storage.k8s.io/v1beta1
Kind:         VolumeSnapshotContent
Metadata:
  Creation Timestamp:             2020-02-07T01:13:49Z
  Deletion Grace Period Seconds:  0
  Deletion Timestamp:             2020-02-07T01:16:27Z
  Finalizers:
    snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection
  Generation:        3
  Resource Version:  968001
  Self Link:         /apis/snapshot.storage.k8s.io/v1beta1/volumesnapshotcontents/snapshotcontent-static-binding
  UID:               8e2e5e8b-005d-4719-8397-a1527c5da009
Spec:
  Deletion Policy:  Retain
  Driver:           pd.csi.storage.gke.io
  Source:
    Snapshot Handle:  <handle>
  Volume Snapshot Ref:
    Name:       snapshot-static-binding
    Namespace:  default
    UID:        c06ceffa-0581-4c94-9a8d-c8458d526209
Status:
  Creation Time:    1580448624526000000
  Ready To Use:     true
  Restore Size:     5368709120
  Snapshot Handle:  <handle>
Events:             <none>
```

Finalizer **snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection** on VolumeSnapshotContent is never gone.

Pull request:
Add VolumeSnapshotBeingDeleted annotation to content
**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:
This PR adds VolumeSnapshotBeingDeleted annotation to the VolumeSnapshotContent object before the finalizers on the VolumeSnapshot object is removed. This is to make sure that the annotation is on the Content before VolumeSnapshot object is removed from the API server.

**Which issue(s) this PR fixes**:
Fixes #248 

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
```release-note
NONE
```


Patch:
diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go
index 433027fef..c66e2b099 100644
--- a/pkg/common-controller/snapshot_controller.go
+++ b/pkg/common-controller/snapshot_controller.go
@@ -249,6 +249,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec
 		}
 
 		klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: set DeletionTimeStamp on content.", utils.SnapshotKey(snapshot))
+
 		// If content exists, set DeletionTimeStamp on the content;
 		// content won't be deleted immediately due to the finalizer
 		if content != nil && deleteContent && !inUse {
@@ -261,6 +262,9 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec
 		}
 
 		if !inUse {
+			klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent: Set VolumeSnapshotBeingDeleted annotation on the content [%s]", content.Name)
+			ctrl.setAnnVolumeSnapshotBeingDeleted(content)
+
 			klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent: Remove Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot))
 			doesContentExist := false
 			if content != nil {


Test patch:
diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go
index 9c261ce9a..5ba641bf5 100644
--- a/pkg/common-controller/snapshot_update_test.go
+++ b/pkg/common-controller/snapshot_update_test.go
@@ -21,6 +21,7 @@ import (
 	"testing"
 	"time"
 
+	crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1"
 	"github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils"
 	v1 "k8s.io/api/core/v1"
 	storagev1beta1 "k8s.io/api/storage/v1beta1"
@@ -336,6 +337,20 @@ func TestSync(t *testing.T) {
 			expectSuccess: false,
 			test:          testSyncContentError,
 		},
+		{
+			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),
+				utils.VolumeSnapshotBoundFinalizer,
+			),
+			initialContents:  newContentArray("content5-5", "snapuid5-5", "snap5-5", "sid5-5", validSecretClass, "", "", crdv1.VolumeSnapshotContentRetain, nil, nil, true),
+			expectedContents: withContentAnnotations(newContentArray("content5-5", "snapuid5-5", "snap5-5", "sid5-5", validSecretClass, "", "", crdv1.VolumeSnapshotContentRetain, nil, nil, true), map[string]string{utils.AnnVolumeSnapshotBeingDeleted: "yes"}),
+			initialClaims:    newClaimArray("claim5-5", "pvc-uid5-5", "1Gi", "volume5-5", v1.ClaimBound, &classEmpty),
+			initialVolumes:   newVolumeArray("volume5-5", "pv-uid5-5", "pv-handle5-5", "1Gi", "pvc-uid5-5", "claim5-5", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			initialSecrets:   []*v1.Secret{secret()},
+			expectSuccess:    true,
+			test:             testSyncSnapshot,
+		},
 	}
 
 	runSyncTests(t, tests, snapshotClasses)