Repository: kubernetes-csi/external-snapshotter
Pull request #466 (2021-02-02 10:27:26)

Cherry pick of #287: Updated sidecar to not require VolumeSnapshotClass for snapshot deletion
/kind bug

Cherry pick of #287 on release-2.1.

#287: Updated sidecar to not require VolumeSnapshotClass for snapshot deletion 

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Ref #306

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Cherry pick of #287: Allows the sidecar to delete volume snapshots if the volume snapshot class is not found.
```


k8s-ci-robot: Hi @ialidzhikov. Thanks for your PR.

I'm waiting for a [kubernetes-csi](https://github.com/orgs/kubernetes-csi/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://git.k8s.io/community/community-membership.md#member) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-snapshotter).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-snapshotter/pull/466#" title="Author self-approved">ialidzhikov</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **xing-yang** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @xing-yang` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-snapshotter).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-snapshotter/blob/release-2.1/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["xing-yang"]} -->
ialidzhikov: /cc @huffmanca @xing-yang
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-snapshotter/pull/466#" title="Author self-approved">ialidzhikov</a>*, *<a href="https://github.com/kubernetes-csi/external-snapshotter/pull/466#pullrequestreview-582264227" title="Approved">rfranzke</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **xing-yang** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @xing-yang` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-snapshotter).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-snapshotter/blob/release-2.1/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["xing-yang"]} -->
xing-yang: /ok-to-test
k8s-ci-robot: @ialidzhikov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 | d7ba894349829b763ddd356eba1a26d11ce0b8fa | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/466/pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20/1357421736169050112) | `/test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-snapshotter&pr=466). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Aialidzhikov). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-snapshotter/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
ialidzhikov: /test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20
k8s-ci-robot: @ialidzhikov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 | d7ba894349829b763ddd356eba1a26d11ce0b8fa | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/466/pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20/1358338736370225152) | `/test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-snapshotter&pr=466). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Aialidzhikov). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-snapshotter/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
ialidzhikov: @xing-yang , pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 fails to clone v1.20.0 of kubernetes/kubernetes. I don’t think that it is something related to this PR.
ialidzhikov: /assign @xing-yang
xing-yang: /test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20
k8s-ci-robot: @ialidzhikov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 | d7ba894349829b763ddd356eba1a26d11ce0b8fa | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/466/pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20/1358788788037357568) | `/test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-snapshotter&pr=466). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Aialidzhikov). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-snapshotter/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
ialidzhikov: @xing-yang , I believe the job configuration is wrong.

It currently tries to clone 1.20.0 but there is no such tag. It should be v1.20.0.

```diff
-git clone --single-branch --branch 1.20.0 https://github.com/kubernetes/kubernetes /tmp/kubernetes
+git clone --single-branch --branch v1.20.0 https://github.com/kubernetes/kubernetes /tmp/kubernetes
```

Where lives the configuration this job?
xing-yang: @pohly any idea why "v" is missing from "v1.20.0" for this backport?  It is there for PRs running in master branch.
xing-yang: I see a difference between the two:

https://github.com/kubernetes-csi/external-snapshotter/blob/release-2.1/release-tools/prow.sh#L573
https://github.com/kubernetes-csi/external-snapshotter/blob/master/release-tools/prow.sh#L575
ialidzhikov: > I see a difference between the two:
> 
> https://github.com/kubernetes-csi/external-snapshotter/blob/release-2.1/release-tools/prow.sh#L573
> https://github.com/kubernetes-csi/external-snapshotter/blob/master/release-tools/prow.sh#L575
> 
> https://github.com/kubernetes-csi/external-snapshotter/blob/master/release-tools/prow.sh#L68-L78

See https://github.com/kubernetes-csi/external-snapshotter/pull/464/files ->  `version_to_git` func. I believe somewhere in https://github.com/kubernetes-csi/external-snapshotter/pull/464/files there is a fix for the failing git clone in `pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20`..

k8s-ci-robot: @ialidzhikov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 | b436899bda47ff6a770b72516883d7f68c3206dd | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/466/pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20/1358817739199746048) | `/test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-snapshotter&pr=466). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Aialidzhikov). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-snapshotter/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
xing-yang: I think you can submit a separate PR just to add that version_to_git logic in release-2.1. 

CC @pohly @mattcary 
ialidzhikov: The PR was based on old revision from `release-2.1` branch. I rebased it to latest `release-2.1` but it failed with the same error. @xing-yang , I have locally a change that can fix the `git clone` for `pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 `. Should I create a PR with it to tackle this issue or? I see that this job/check is not labelled as required.
xing-yang: We can't merge a PR that can't pass a CI.
ialidzhikov: @xing-yang , I create https://github.com/kubernetes/test-infra/pull/20792 which should address the issue with the K8s 1.20 tests that should not be executed against `external-snapshotter@release-2.1`.
pohly: > We can't merge a PR that can't pass a CI.

1-20-on-kubernetes-1-20 is intentionally not required to pass at this time, therefore you can merge PRs even when it is failing.


Patch:
diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go
index 68e48ac0f..aff73f1bf 100644
--- a/pkg/sidecar-controller/snapshot_controller.go
+++ b/pkg/sidecar-controller/snapshot_controller.go
@@ -26,6 +26,7 @@ import (
 	codes "google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/klog"
 	"k8s.io/kubernetes/pkg/util/goroutinemap"
@@ -367,7 +368,7 @@ func (ctrl *csiSnapshotSideCarController) deleteCSISnapshotOperation(content *cr
 	klog.V(5).Infof("deleteCSISnapshotOperation [%s] started", content.Name)
 
 	_, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content)
-	if err != nil {
+	if err != nil && !errors.IsNotFound(err) {
 		ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to get snapshot class or credentials")
 		return fmt.Errorf("failed to get input parameters to delete snapshot for content %s: %q", content.Name, err)
 	}
@@ -479,7 +480,7 @@ func (ctrl *csiSnapshotSideCarController) getSnapshotClass(className string) (*c
 	class, err := ctrl.classLister.Get(className)
 	if err != nil {
 		klog.Errorf("failed to retrieve snapshot class %s from the informer: %q", className, err)
-		return nil, fmt.Errorf("failed to retrieve snapshot class %s from the informer: %q", className, err)
+		return nil, err
 	}
 
 	return class, nil


Test patch:
diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go
index babc144a8..c211c51d5 100644
--- a/pkg/sidecar-controller/content_create_test.go
+++ b/pkg/sidecar-controller/content_create_test.go
@@ -163,7 +163,7 @@ func TestSyncContent(t *testing.T) {
 					SnapshotHandle: toStringPointer("sid1-6"),
 					RestoreSize:    &defaultSize,
 					ReadyToUse:     &False,
-					Error:          newSnapshotError("Failed to check and update snapshot content: failed to get input parameters to create snapshot for content content1-6: \"failed to retrieve snapshot class bad-class from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"bad-class\\\\\\\" not found\\\"\""),
+					Error:          newSnapshotError("Failed to check and update snapshot content: failed to get input parameters to create snapshot for content content1-6: \"volumesnapshotclass.snapshot.storage.k8s.io \\\"bad-class\\\" not found\""),
 				}),
 			expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"},
 			expectedCreateCalls: []createCall{
diff --git a/pkg/sidecar-controller/snapshot_delete_test.go b/pkg/sidecar-controller/snapshot_delete_test.go
index ae25a8bb8..17c4d4948 100644
--- a/pkg/sidecar-controller/snapshot_delete_test.go
+++ b/pkg/sidecar-controller/snapshot_delete_test.go
@@ -219,12 +219,13 @@ func TestDeleteSync(t *testing.T) {
 			test:                testSyncContent,
 		},
 		{
-			name:             "1-4 - fail to delete with a snapshot class which has invalid secret parameter, bound finalizer should remain",
-			initialContents:  newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-4-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
-			expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-4-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
-			expectedEvents:   noevents,
-			errors:           noerrors,
-			test:             testSyncContent,
+			name:                "1-4 - fail to delete with a snapshot class which has invalid secret parameter, bound finalizer should remain",
+			initialContents:     newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-4-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
+			expectedContents:    newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-4-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
+			expectedEvents:      noevents,
+			expectedDeleteCalls: []deleteCall{{"sid1-1", nil, fmt.Errorf("mock csi driver delete error")}},
+			errors:              noerrors,
+			test:                testSyncContent,
 		},
 		{
 			name:                "1-5 - csi driver delete snapshot returns error, bound finalizer should remain",
@@ -330,10 +331,9 @@ func TestDeleteSync(t *testing.T) {
 			test:                testSyncContent,
 		},
 		{
-			name:                "1-15 - (dynamic)deletion of content with no snapshotclass should produce error",
+			name:                "1-15 - (dynamic)deletion of content with no snapshotclass should succeed",
 			initialContents:     newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
 			expectedContents:    newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
-			expectedEvents:      []string{"Warning SnapshotDeleteError"},
 			errors:              noerrors,
 			expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}},
 			test:                testSyncContent,