Add deletion secret as annotation to content #165

xing-yang · 2019-08-24T19:55:21Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds deletion secret as annotation to volume snapshot content.
It also disallows annotation in template for snapshotter-secret-name.

Which issue(s) this PR fixes:
Fixes #161 #155

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Adds deletion secret as annotation to volume snapshot content.

k8s-ci-robot · 2019-08-24T19:55:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xing-yang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xing-yang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

xing-yang · 2019-08-26T17:24:34Z

/assign @msau42

xing-yang · 2019-08-26T17:25:29Z

Hi @msau42 If this PR is on the right track, I'll fix the unit tests.

msau42 · 2019-08-27T19:01:04Z

pkg/controller/snapshot_controller.go

+		snapshotterCredentials, err := getCredentials(ctrl.client, snapshotterSecretRef)
+		if err != nil {
+			return nil, err
+		}
 		driverName, snapshotID, creationTime, size, readyToUse, err = ctrl.handler.CreateSnapshot(snapshot, volume, class.Parameters, snapshotterCredentials)


Why are we calling CreateSnapshot here? Shouldn't the snapshot already exist at this point?

This is to check the status of snapshot if uploading is needed after the snapshot is cut. We call CreateSnapshot for dynamic creation case because ListSnapshots is not required and CreateSnapshot is idempotent so it will just return status of the same snapshot.

This sounds non-ideal. Usually Create operations have lower QPS limits than Get/List

We cannot rely on Get/List because ListSnapshots is optional and many drivers don't support that. We just fix a bug recently to check that capability.

#132

Note that for static binding, we will call GetSnapshotStatus (which call ListSnapshots CSI function) because CreateSnapshot is not invoked for static binding. Even in that case, we can only use that if ListSnapshots is supported by the driver. Since it is pre-provisioned, we just assume the snapshot is valid and ready if driver didn't implement ListSnapshots.

For dynamic creation, we can't assume the snapshot is ready. We have to check the status. Note that in the CSI spec, we specified that "calling CreateSnapshot repeatedly is the preferred way to check if the processing is complete".

https://github.com/container-storage-interface/spec/blob/master/spec.md#createsnapshot-deletesnapshot-listsnapshots

ListSnapshots SHALL return with current information regarding the snapshots on the storage system. When processing is complete, the ready_to_use parameter of the snapshot from ListSnapshots SHALL become true. The downside of calling ListSnapshots is that ListSnapshots will not return a gRPC error code if an error occurs during the processing. So calling CreateSnapshot repeatedly is the preferred way to check if the processing is complete.

I don't think using CreateSnapshot when we really want GetSnapshot is ideal for the following reasons:

Most cloud providers have lower QPS limits for create/update/delete calls vs get/list calls. Plugins may also have different concurrency control for read-only operations vs write operations. Repeatedly calling CreateSnapshot can cause scaling issues in the future

Metrics reporting also gets skewed and misrepresented. All CreateSnapshot operations get tracked as "create" even though that's not necessary the case. We cannot distinguish between errors and latency of real create operations vs get operations afterwards. For example, get operations are usually faster than create operations, but since we probably call CreateSnapshot with the "get" intention more often, that's going to cause the latency metric to skew more towards the faster "get" latencies.

Anyway, this is not strictly related to this particular PR, I think we should open a issue to track this as a future enhancement.

Yeah, sounds like we need a different issue to track this for further discussion. Will need to bring this up to CSI community too if we don't want to suggest CO to call CreateSnapshot. Many drivers couldn't support ListSnapshots so the only way to find out the status is to call CreateSnapshot. I remember @jingxu97 strongly suggested to use CreateSnapshot so that we can get gRPC code when this was implemented at that time. Initially we used ListSnapshots to find status for all, but I was asked to use CreateSnapshot when CSI went GA (I submitted that PR right after the Shanghai KubeCon last year: #61). Anyway let's discuss it in a different issue.

msau42 · 2019-08-27T20:56:47Z

pkg/controller/snapshot_controller.go

+
+	// Set AnnSecretRefName and AnnSecretRefNamespace
+	if snapshotterSecretRef != nil {
+		if !metav1.HasAnnotation(snapshotContent.ObjectMeta, AnnSecretRefName) {


Why would the annotation already exist? We are creating a new snapshotContent object every time

Yeah, we don't need this check. I'll remove it.

msau42 · 2019-08-27T20:57:53Z

pkg/controller/snapshot_controller.go

+
+		snapshotterSecretRef := &v1.SecretReference{}
+
+		if annSecretName != "" {


What happens if one of these is empty?

If annotations exist but they don't have valid secret name/namespace, we should just error out. I'll fix this.

msau42 · 2019-08-27T20:59:13Z

pkg/controller/util.go

+
+	// Annotation for secret name and namespace will be added to the content
+	// and used at snapshot content deletion time.
+	AnnSecretRefName      = "snapshot.storage.kubernetes.io/secret-name"


I would call this "deletion-secret" because it's only used for deletion.

msau42 · 2019-08-27T20:59:55Z

pkg/controller/util_test.go

@@ -70,7 +69,7 @@ func TestGetSecretReference(t *testing.T) {
 			expectRef: nil,
 			expectErr: true,
 		},
-		"template - valid": {
+		/*"template - valid": {


change to an invalid case?

I need fix the unit tests.

xing-yang · 2019-08-28T01:20:29Z

Address review comments, but still need to fix unit tests.

msau42 · 2019-08-29T16:56:19Z

pkg/controller/snapshot_controller.go

@@ -687,6 +692,17 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
 			DeletionPolicy:          class.DeletionPolicy,
 		},
 	}
+
+	// Set AnnDeletionSecretRefName and AnnDeletionSecretRefNamespace
+	if !metav1.HasAnnotation(snapshotContent.ObjectMeta, AnnDeletionSecretRefName) && snapshotterSecretRef != nil {


don't think we need to check if the object has the annotation already because we create it immediately above this

Removed this check. Still checking if snapshotterSecretRef != nil as we ran into a nil pointer without it.

msau42 · 2019-08-29T17:00:54Z

pkg/controller/snapshot_controller.go

+
+		snapshotterSecretRef := &v1.SecretReference{}
+
+		if annDeletionSecretName == "" || annDeletionSecretNamespace == "" {


unit test for this case?

Updated unit test 1-4 to use emptyNamespaceSecretAnnotations()

msau42 · 2019-08-29T17:01:27Z

pkg/controller/framework_test.go

@@ -1287,6 +1288,27 @@ func secret() *v1.Secret {
 	}
 }

+func secretAnnotations() map[string]string {
+	return map[string]string{
+		AnnDeletionSecretRefName:      "secret",


Do we actually need to create the secret API object somewhere?

Yes, it is on line 1280.

msau42 · 2019-08-29T17:01:47Z

pkg/controller/framework_test.go

+	}
+}
+
+func invalidSecretAnnotations() map[string]string {


is this used anywhere?

Actually it is missing.

removed this.

msau42 · 2019-08-29T17:02:41Z

pkg/controller/framework_test.go

+	}
+}
+
+func emptySecretAnnotations() map[string]string {


This is not what I would have guessed "empty" to mean. This is more like the secret doesn't exist?

In this case, emptysecret exists, but it does not have any data inside. I'll update the name to emptyDataSecretAnnotations to be more clear.

(I'm helping Xing with these tests)

ggriffiths · 2019-08-29T22:54:52Z

Addressed feedback and squashed to one commit. @msau42

msau42 · 2019-09-05T00:54:08Z

pkg/controller/snapshot_delete_test.go

-			initialContents:   newContentArray("content1-4", invalidSecretClass, "sid1-4", "vuid1-4", "volume1-4", "snapuid1-4", "snap1-4", &deletePolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-4", invalidSecretClass, "sid1-4", "vuid1-4", "volume1-4", "snapuid1-4", "snap1-4", &deletePolicy, nil, nil, true),
+			initialContents:   newContentArray("content1-4", invalidSecretClass, "sid1-4", "vuid1-4", "volume1-4", "snapuid1-4", "snap1-4", &deletePolicy, nil, nil, true, emptyNamespaceSecretAnnotations()),
+			expectedContents:  newContentArray("content1-4", invalidSecretClass, "sid1-4", "vuid1-4", "volume1-4", "snapuid1-4", "snap1-4", &deletePolicy, nil, nil, true, emptyNamespaceSecretAnnotations()),


Is there a test case for if a secret is specified, but it doesn't exist? To mirror the external-provisioner scenario, I believe we continue and pass no secret to the Delete call, and it's up to the plugin to fail if they need it.

Unless I'm missing something, it seems that the current behavior for the snapshotter is to fail in this case:
https://github.com/kubernetes-csi/external-snapshotter/blob/master/pkg/controller/snapshot_controller.go#L748-L751

If we want to mirror the provisioner behavior, I can fix that (and add a test case) in this PR or another one

@ggriffiths I missed the provisioner behavior in this case. Please go ahead to fix it and add a test case. Thanks.

Fixed and added deletion test 1-16 for continuing deletion when secret does not exist. Also added a test for creation failing in this case: 7-10

msau42 · 2019-09-05T17:58:30Z

/lgtm
Thanks!

…hub-links-ref-to-master-to-HEAD docs: make github links reference HEAD instead of master

k8s-ci-robot requested review from jingxu97 and saad-ali August 24, 2019 19:55

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 24, 2019

k8s-ci-robot assigned msau42 Aug 26, 2019

msau42 reviewed Aug 27, 2019

View reviewed changes

xing-yang force-pushed the ann_secret branch from 490ab05 to 3bfc93b Compare August 28, 2019 01:19

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 29, 2019

xing-yang changed the title ~~WIP: Add deletion secret as annotation to content~~ Add deletion secret as annotation to content Aug 29, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2019

msau42 reviewed Aug 29, 2019

View reviewed changes

ggriffiths force-pushed the ann_secret branch from 6b81c25 to 4a39a89 Compare August 29, 2019 22:52

msau42 reviewed Sep 5, 2019

View reviewed changes

Add deletion secret as annotation to content

e647a80

ggriffiths force-pushed the ann_secret branch from 4a39a89 to e647a80 Compare September 5, 2019 03:01

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2019

k8s-ci-robot merged commit a791d62 into kubernetes-csi:master Sep 5, 2019

xing-yang mentioned this pull request Oct 10, 2019

Update VolumeSnapshot CRD version to v1beta #139

Merged

Jiawei0227 mentioned this pull request Jan 29, 2021

CreateSnapshot being called several times when only one Snapshot is wanted. #465

Closed

msau42 mentioned this pull request Oct 6, 2022

KEP-3476: Add Volume Group Snapshot KEP kubernetes/enhancements#1551

Merged

xing-yang mentioned this pull request May 25, 2023

Add secrets in CreateVolumeGroupSnapshots #834

Closed

andyzhangx pushed a commit to andyzhangx/external-snapshotter that referenced this pull request Jun 5, 2024

Merge pull request kubernetes-csi#165 from consideRatio/pr/update-git…

ac8108f

…hub-links-ref-to-master-to-HEAD docs: make github links reference HEAD instead of master


		snapshotterSecretRef := &v1.SecretReference{}

		if annSecretName != "" {


		snapshotterSecretRef := &v1.SecretReference{}

		if annDeletionSecretName == "" \|\| annDeletionSecretNamespace == "" {

Add deletion secret as annotation to content #165

Add deletion secret as annotation to content #165

Conversation

xing-yang commented Aug 24, 2019

k8s-ci-robot commented Aug 24, 2019

xing-yang commented Aug 26, 2019

xing-yang commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented Aug 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggriffiths commented Aug 29, 2019

Choose a reason for hiding this comment

ggriffiths Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggriffiths Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

msau42 commented Sep 5, 2019

ggriffiths Sep 5, 2019 •

edited

Loading

ggriffiths Sep 5, 2019 •

edited

Loading