Repository: kubernetes-csi/external-snapshotter
Pull request #527 (2021-05-27 18:56:27)

Return VolumeSnapshotContent from various functions instead of nil
Signed-off-by: Grant Griffiths <ggriffiths@purestorage.com>

**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:
When removeAnnVolumeSnapshotBeingCreated fails, updatedContent is not returned.

**Which issue(s) this PR fixes**:
Part of #525 

**Special notes for your reviewer**:
This does not completely fix the problem, but it does seem to lower the count.

**Does this PR introduce a user-facing change?**:
```release-note
NONE
```


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/527#" title="Author self-approved">ggriffiths</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **jingxu97** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @jingxu97` 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/master/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":["jingxu97"]} -->
ggriffiths: @jsafrane I have fixed the issue you mentioned with `removeAnnVolumeSnapshotBeingCreated`. Let me know what you think.

This seems to lower the "object has been modified" count a bit, but it's still not consistently low yet. Either way, I think this is probably heading the right direction.
jsafrane: It's a step in the right direction. Every time the sidecar issues `Update` / `UpdateStatus` against the API server, it must use the returned objects (with updated ResourceVersion) in all subsequent `Update` / `UpdateStatus` calls.

* `setAnnVolumeSnapshotBeingCreated` must return `content` it got from the API server
* Whole `createSnapshotWrapper` should not return `nil`, but some `content`.
* When it calls `createSnapshotWrapper`, it should use the returned object in subsequent `updateContentErrorStatusWithEvent` and not the original one, i.e. use `contentObj` instead of `content` in both these places:
  * https://github.com/kubernetes-csi/external-snapshotter/blob/8751eb0ca5b33a8a3c2589d506b48cc26684e0cd/pkg/sidecar-controller/snapshot_controller.go#L104
  * https://github.com/kubernetes-csi/external-snapshotter/blob/8751eb0ca5b33a8a3c2589d506b48cc26684e0cd/pkg/sidecar-controller/snapshot_controller.go#L121
 
And please check all error paths for returning `nil` from `createSnapshotWrapper` / `checkandUpdateContentStatusOperation`.
ggriffiths: Ok, I plan to update this PR soon with the above feedback.
ggriffiths: @jsafrane ready for re-review, I've addressed your comments above.

I've also done the following:
* Searched for `return nil,` and added a content/snapshot object if possible
* Searched for all Update/UpdateStatus calls and made sure we were returning the updated objects.

I still see around the same number "object has been modified" error count in our PR tests, but I think these changes are good to do regardless. 
xing-yang: @ggriffiths are you saying we don't see the "object has been modified" count getting reduced at all with changes in this PR?
ggriffiths: @xing-yang I wrote that comment before the final build ran. Up until then, the error count seemed to not be lowered after each commit.

However, with the most recent commit, it seems to be lowered:

1.19 - 28 occurrences of object has been modified - [link](https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/527/pull-kubernetes-csi-external-snapshotter-1-19-on-kubernetes-1-19/1402771395213004800/artifacts/kube-system/snapshot-controller-58f54d8f6c-nr7s7/snapshot-controller.log)
1.20 - 42 occurrences of object has been modified - [link](https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/527/pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20/1402771395229782016/artifacts/kube-system/snapshot-controller-58f54d8f6c-rl6zh/snapshot-controller.log)
1.21 - 26 occurrences of object has been modified - [link](https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/527/pull-kubernetes-csi-external-snapshotter-1-21-on-kubernetes-1-21/1402771395821178880/artifacts/kube-system/snapshot-controller-58f54d8f6c-9dstp/snapshot-controller.log)

jsafrane: I found one issue in the PR, but the rest looks good to me and it reduces nr. of conflicts, at least in my test case (taking snapshot of  a non-existing CSI hostpath volume - the driver constantly returns `NotFound`).


Patch:
diff --git a/README.md b/README.md
index 343c00467..6af01c3ba 100644
--- a/README.md
+++ b/README.md
@@ -4,7 +4,7 @@
 
 The CSI snapshotter is part of Kubernetes implementation of [Container Storage Interface (CSI)](https://github.com/container-storage-interface/spec).
 
-The volume snapshot feature supports CSI v1.0 and higher. It was introduced as an Alpha feature in Kubernetes v1.12 and has been promoted to a Beta feature in Kubernetes 1.17. In Kubernetes 1.20, the volume snapshot feature moves to GA.
+The volume snapshot feature supports CSI v1.0 and higher. It was introduced as an Alpha feature in Kubernetes v1.12 and has been promoted to an Beta feature in Kubernetes 1.17. In Kubernetes 1.20, the volume snapshot feature moves to GA.
 
 > :warning: **WARNING**: There is a new validating webhook server which provides tightened validation on snapshot objects. This SHOULD be installed by all users of this feature. More details [below](#validating-webhook).
 
@@ -63,7 +63,7 @@ If your Kubernetes distribution does not bundle the snapshot controller, you may
 
 There is a new validating webhook server which provides tightened validation on snapshot objects. The cluster admin or Kubernetes distribution admin should install the webhook alongside the snapshot controllers and CRDs. More details [below](#validating-webhook).
 
-Install Snapshot CRDs:
+Install Snapshot Beta CRDs:
 * kubectl create -f client/config/crd
 * https://github.com/kubernetes-csi/external-snapshotter/tree/master/client/config/crd
 * Do this once per cluster
@@ -96,7 +96,7 @@ If there are no existing invalid v1beta1 objects, after upgrading to v1, the web
 
 If there are existing invalid v1beta1 objects, the user should make sure that the snapshot controller is upgraded to v3.0.0 or higher (v3.0.3 is the latest recommended v3.0.x release) and install the corresponding validation webhook before upgrading to v1 so that those invalid objects will be labeled and can be identified easily and removed before upgrading to v1.
 
-If there are existing invalid v1beta1 objects, and the user didn't upgrade to the snapshot controller 3.0.0 or higher and install the corresponding validation webhook before upgrading to v1, those existing invalid v1beta1 objects will not be labeled by the snapshot controller.
+If there are existing invalid v1beta1 objects and the user didn't upgrade to the snapshot controller 3.0.0 or higher and install the corresponding validation webhook before upgrading to v1, those existing invalid v1beta1 objects will not be labeled by the snapshot controller.
 
 So the recommendation is that before upgrading to v1 CRDs and upgrading snapshot controller and validation webhook to v4.0, the user should upgrade to the snapshot controller 3.0.0 and higher (v3.0.3 is the latest recommended version for 3.0.x) and install the corresponding validation webhook so that all existing invalid objects will be labeled and can be easily identified and deleted.
 
@@ -127,9 +127,6 @@ Read more about how to install the example webhook [here](deploy/kubernetes/webh
 
 * `--worker-threads`: Number of worker threads. Default value is 10.
 
-* `--retry-interval-start`: Initial retry interval of failed volume snapshot creation or deletion. It doubles with each failure, up to retry-interval-max. Default value is 1 second.
-
-*`--retry-interval-max`: Maximum retry interval of failed volume snapshot creation or deletion. Default value is 5 minutes.
 #### Other recognized arguments
 * `--kubeconfig <path>`: Path to Kubernetes client configuration that the snapshot controller uses to connect to Kubernetes API server. When omitted, default token provided by Kubernetes will be used. This option is useful only when the snapshot controller does not run as a Kubernetes pod, e.g. for debugging.
 
@@ -156,9 +153,6 @@ Read more about how to install the example webhook [here](deploy/kubernetes/webh
 
 * `--worker-threads`: Number of worker threads for running create snapshot and delete snapshot operations. Default value is 10.
 
-* `--retry-interval-start`: Initial retry interval of failed volume snapshot creation or deletion. It doubles with each failure, up to retry-interval-max. Default value is 1 second.
-
-*`--retry-interval-max`: Maximum retry interval of failed volume snapshot creation or deletion. Default value is 5 minutes.
 #### Other recognized arguments
 * `--kubeconfig <path>`: Path to Kubernetes client configuration that the CSI external-snapshotter uses to connect to Kubernetes API server. When omitted, default token provided by Kubernetes will be used. This option is useful only when the external-snapshotter does not run as a Kubernetes pod, e.g. for debugging.
 
diff --git a/cmd/csi-snapshotter/main.go b/cmd/csi-snapshotter/main.go
index 7e499c36a..afdfcad27 100644
--- a/cmd/csi-snapshotter/main.go
+++ b/cmd/csi-snapshotter/main.go
@@ -32,7 +32,6 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
-	"k8s.io/client-go/util/workqueue"
 	klog "k8s.io/klog/v2"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
@@ -58,7 +57,7 @@ const (
 var (
 	kubeconfig             = flag.String("kubeconfig", "", "Absolute path to the kubeconfig file. Required only when running out of cluster.")
 	csiAddress             = flag.String("csi-address", "/run/csi/socket", "Address of the CSI driver socket.")
-	resyncPeriod           = flag.Duration("resync-period", 15*time.Minute, "Resync interval of the controller. Default is 15 minutes")
+	resyncPeriod           = flag.Duration("resync-period", 15*time.Minute, "Resync interval of the controller.")
 	snapshotNamePrefix     = flag.String("snapshot-name-prefix", "snapshot", "Prefix to apply to the name of a created snapshot")
 	snapshotNameUUIDLength = flag.Int("snapshot-name-uuid-length", -1, "Length in characters for the generated uuid of a created snapshot. Defaults behavior is to NOT truncate.")
 	showVersion            = flag.Bool("version", false, "Show version.")
@@ -69,11 +68,9 @@ var (
 	leaderElection          = flag.Bool("leader-election", false, "Enables leader election.")
 	leaderElectionNamespace = flag.String("leader-election-namespace", "", "The namespace where the leader election resource exists. Defaults to the pod namespace if not set.")
 
-	metricsAddress     = flag.String("metrics-address", "", "(deprecated) The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.")
-	httpEndpoint       = flag.String("http-endpoint", "", "The TCP network address where the HTTP server for diagnostics, including metrics and leader election health check, will listen (example: `:8080`). The default is empty string, which means the server is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.")
-	metricsPath        = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.")
-	retryIntervalStart = flag.Duration("retry-interval-start", time.Second, "Initial retry interval of failed volume snapshot creation or deletion. It doubles with each failure, up to retry-interval-max. Default is 1 second.")
-	retryIntervalMax   = flag.Duration("retry-interval-max", 5*time.Minute, "Maximum retry interval of failed volume snapshot creation or deletion. Default is 5 minutes.")
+	metricsAddress = flag.String("metrics-address", "", "(deprecated) The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.")
+	httpEndpoint   = flag.String("http-endpoint", "", "The TCP network address where the HTTP server for diagnostics, including metrics and leader election health check, will listen (example: `:8080`). The default is empty string, which means the server is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.")
+	metricsPath    = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.")
 )
 
 var (
@@ -201,7 +198,6 @@ func main() {
 		*snapshotNamePrefix,
 		*snapshotNameUUIDLength,
 		*extraCreateMetadata,
-		workqueue.NewItemExponentialFailureRateLimiter(*retryIntervalStart, *retryIntervalMax),
 	)
 
 	run := func(context.Context) {
diff --git a/cmd/snapshot-controller/main.go b/cmd/snapshot-controller/main.go
index 4f5161172..29afc3289 100644
--- a/cmd/snapshot-controller/main.go
+++ b/cmd/snapshot-controller/main.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"flag"
 	"fmt"
-	"k8s.io/client-go/util/workqueue"
 	"os"
 	"os/signal"
 	"sync"
@@ -56,10 +55,8 @@ var (
 	leaderElection          = flag.Bool("leader-election", false, "Enables leader election.")
 	leaderElectionNamespace = flag.String("leader-election-namespace", "", "The namespace where the leader election resource exists. Defaults to the pod namespace if not set.")
 
-	httpEndpoint       = flag.String("http-endpoint", "", "The TCP network address where the HTTP server for diagnostics, including metrics, will listen (example: :8080). The default is empty string, which means the server is disabled.")
-	metricsPath        = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.")
-	retryIntervalStart = flag.Duration("retry-interval-start", time.Second, "Initial retry interval of failed volume snapshot creation or deletion. It doubles with each failure, up to retry-interval-max. Default is 1 second.")
-	retryIntervalMax   = flag.Duration("retry-interval-max", 5*time.Minute, "Maximum retry interval of failed volume snapshot creation or deletion. Default is 5 minutes.")
+	httpEndpoint = flag.String("http-endpoint", "", "The TCP network address where the HTTP server for diagnostics, including metrics, will listen (example: :8080). The default is empty string, which means the server is disabled.")
+	metricsPath  = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.")
 )
 
 var (
@@ -173,8 +170,6 @@ func main() {
 		coreFactory.Core().V1().PersistentVolumeClaims(),
 		metricsManager,
 		*resyncPeriod,
-		workqueue.NewItemExponentialFailureRateLimiter(*retryIntervalStart, *retryIntervalMax),
-		workqueue.NewItemExponentialFailureRateLimiter(*retryIntervalStart, *retryIntervalMax),
 	)
 
 	if err := ensureCustomResourceDefinitionsExist(snapClient); err != nil {
diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go
index 506c748fe..ef3862d92 100644
--- a/pkg/common-controller/snapshot_controller.go
+++ b/pkg/common-controller/snapshot_controller.go
@@ -166,7 +166,10 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh
 	// Snapshot won't be deleted until content is deleted
 	// due to the finalizer.
 	if snapshot != nil && utils.IsSnapshotDeletionCandidate(snapshot) {
-		return ctrl.setAnnVolumeSnapshotBeingDeleted(content)
+		// Do not need to use the returned content here, as syncContent will get
+		// the correct version from the cache next time. It is also not used after this.
+		_, err = ctrl.setAnnVolumeSnapshotBeingDeleted(content)
+		return err
 	}
 
 	return nil
@@ -311,10 +314,12 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec
 	// a delete operation whenever the content has deletion timestamp set.
 	if content != nil {
 		klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: Set VolumeSnapshotBeingDeleted annotation on the content [%s]", utils.SnapshotKey(snapshot), content.Name)
-		if err := ctrl.setAnnVolumeSnapshotBeingDeleted(content); err != nil {
+		updatedContent, err := ctrl.setAnnVolumeSnapshotBeingDeleted(content)
+		if err != nil {
 			klog.V(4).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: failed to set VolumeSnapshotBeingDeleted annotation on the content [%s]", utils.SnapshotKey(snapshot), content.Name)
 			return err
 		}
+		content = updatedContent
 	}
 
 	// VolumeSnapshot should be deleted. Check and remove finalizers
@@ -811,12 +816,12 @@ func (ctrl *csiSnapshotCommonController) addContentFinalizer(content *crdv1.Volu
 	contentClone := content.DeepCopy()
 	contentClone.ObjectMeta.Finalizers = append(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
 
-	_, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
+	newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
 	if err != nil {
 		return newControllerUpdateError(content.Name, err.Error())
 	}
 
-	_, err = ctrl.storeContentUpdate(contentClone)
+	_, err = ctrl.storeContentUpdate(newContent)
 	if err != nil {
 		klog.Errorf("failed to update content store %v", err)
 	}
@@ -991,13 +996,13 @@ func (ctrl *csiSnapshotCommonController) checkandBindSnapshotContent(snapshot *c
 	newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
 	if err != nil {
 		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", contentClone.Name, err)
-		return nil, err
+		return content, err
 	}
 
 	_, err = ctrl.storeContentUpdate(newContent)
 	if err != nil {
 		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status: cannot update internal cache %v", newContent.Name, err)
-		return nil, err
+		return newContent, err
 	}
 	return newContent, nil
 }
@@ -1303,13 +1308,13 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
 	// Find default snapshot class if available
 	list, err := ctrl.classLister.List(labels.Everything())
 	if err != nil {
-		return nil, nil, err
+		return nil, snapshot, err
 	}
 
 	pvDriver, err := ctrl.pvDriverFromSnapshot(snapshot)
 	if err != nil {
 		klog.Errorf("failed to get pv csi driver from snapshot %s/%s: %q", snapshot.Namespace, snapshot.Name, err)
-		return nil, nil, err
+		return nil, snapshot, err
 	}
 
 	defaultClasses := []*crdv1.VolumeSnapshotClass{}
@@ -1320,11 +1325,11 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
 		}
 	}
 	if len(defaultClasses) == 0 {
-		return nil, nil, fmt.Errorf("cannot find default snapshot class")
+		return nil, snapshot, fmt.Errorf("cannot find default snapshot class")
 	}
 	if len(defaultClasses) > 1 {
 		klog.V(4).Infof("get DefaultClass %d defaults found", len(defaultClasses))
-		return nil, nil, fmt.Errorf("%d default snapshot classes were found", len(defaultClasses))
+		return nil, snapshot, fmt.Errorf("%d default snapshot classes were found", len(defaultClasses))
 	}
 	klog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name)
 	snapshotClone := snapshot.DeepCopy()
@@ -1394,17 +1399,17 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo
 	if addBoundFinalizer {
 		snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
 	}
-	_, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
 	if err != nil {
 		return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
 	}
 
-	_, err = ctrl.storeSnapshotUpdate(snapshotClone)
+	_, err = ctrl.storeSnapshotUpdate(newSnapshot)
 	if err != nil {
 		klog.Errorf("failed to update snapshot store %v", err)
 	}
 
-	klog.V(5).Infof("Added protection finalizer to volume snapshot %s", utils.SnapshotKey(snapshot))
+	klog.V(5).Infof("Added protection finalizer to volume snapshot %s", utils.SnapshotKey(newSnapshot))
 	return nil
 }
 
@@ -1438,12 +1443,12 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1
 	if removeBoundFinalizer {
 		snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
 	}
-	_, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
 	if err != nil {
 		return newControllerUpdateError(snapshot.Name, err.Error())
 	}
 
-	_, err = ctrl.storeSnapshotUpdate(snapshotClone)
+	_, err = ctrl.storeSnapshotUpdate(newSnapshot)
 	if err != nil {
 		klog.Errorf("failed to update snapshot store %v", err)
 	}
@@ -1477,9 +1482,9 @@ func (ctrl *csiSnapshotCommonController) getSnapshotFromStore(snapshotName strin
 	return snapshot, nil
 }
 
-func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(content *crdv1.VolumeSnapshotContent) error {
+func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
 	if content == nil {
-		return nil
+		return content, nil
 	}
 	// Set AnnVolumeSnapshotBeingDeleted if it is not set yet
 	if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) {
@@ -1488,7 +1493,7 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten
 
 		updateContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), content, metav1.UpdateOptions{})
 		if err != nil {
-			return newControllerUpdateError(content.Name, err.Error())
+			return content, newControllerUpdateError(content.Name, err.Error())
 		}
 		// update content if update is successful
 		content = updateContent
@@ -1496,11 +1501,11 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten
 		_, err = ctrl.storeContentUpdate(content)
 		if err != nil {
 			klog.V(4).Infof("setAnnVolumeSnapshotBeingDeleted for content [%s]: cannot update internal cache %v", content.Name, err)
-			return err
+			return content, err
 		}
 		klog.V(5).Infof("setAnnVolumeSnapshotBeingDeleted: volume snapshot content %+v", content)
 	}
-	return nil
+	return content, nil
 }
 
 // checkAndSetInvalidContentLabel adds a label to unlabeled invalid content objects and removes the label from valid ones.
@@ -1531,7 +1536,7 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content
 		return content, newControllerUpdateError(content.Name, err.Error())
 	}
 
-	_, err = ctrl.storeContentUpdate(contentClone)
+	_, err = ctrl.storeContentUpdate(updatedContent)
 	if err != nil {
 		klog.Errorf("failed to update content store %v", err)
 	}
@@ -1573,7 +1578,7 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapsho
 		return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
 	}
 
-	_, err = ctrl.storeSnapshotUpdate(snapshotClone)
+	_, err = ctrl.storeSnapshotUpdate(updatedSnapshot)
 	if err != nil {
 		klog.Errorf("failed to update snapshot store %v", err)
 	}
diff --git a/pkg/common-controller/snapshot_controller_base.go b/pkg/common-controller/snapshot_controller_base.go
index 0d5b46d0b..556c30e95 100644
--- a/pkg/common-controller/snapshot_controller_base.go
+++ b/pkg/common-controller/snapshot_controller_base.go
@@ -76,8 +76,6 @@ func NewCSISnapshotCommonController(
 	pvcInformer coreinformers.PersistentVolumeClaimInformer,
 	metricsManager metrics.MetricsManager,
 	resyncPeriod time.Duration,
-	snapshotRateLimiter workqueue.RateLimiter,
-	contentRateLimiter workqueue.RateLimiter,
 ) *csiSnapshotCommonController {
 	broadcaster := record.NewBroadcaster()
 	broadcaster.StartLogging(klog.Infof)
@@ -92,8 +90,8 @@ func NewCSISnapshotCommonController(
 		resyncPeriod:   resyncPeriod,
 		snapshotStore:  cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc),
 		contentStore:   cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc),
-		snapshotQueue:  workqueue.NewNamedRateLimitingQueue(snapshotRateLimiter, "snapshot-controller-snapshot"),
-		contentQueue:   workqueue.NewNamedRateLimitingQueue(contentRateLimiter, "snapshot-controller-content"),
+		snapshotQueue:  workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "snapshot-controller-snapshot"),
+		contentQueue:   workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "snapshot-controller-content"),
 		metricsManager: metricsManager,
 	}
 
diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go
index 0e2a98f03..ec0c6deb0 100644
--- a/pkg/sidecar-controller/snapshot_controller.go
+++ b/pkg/sidecar-controller/snapshot_controller.go
@@ -79,9 +79,11 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
 	// already true. We don't want to keep calling CreateSnapshot
 	// or ListSnapshots CSI methods over and over again for
 	// performance reasons.
+	var err error
 	if content.Status != nil && content.Status.ReadyToUse != nil && *content.Status.ReadyToUse == true {
 		// Try to remove AnnVolumeSnapshotBeingCreated if it is not removed yet for some reason
-		return ctrl.removeAnnVolumeSnapshotBeingCreated(content)
+		_, err = ctrl.removeAnnVolumeSnapshotBeingCreated(content)
+		return err
 	}
 	return ctrl.checkandUpdateContentStatus(content)
 }
@@ -101,7 +103,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSn
 	klog.V(5).Infof("createSnapshot for content [%s]: started", content.Name)
 	contentObj, err := ctrl.createSnapshotWrapper(content)
 	if err != nil {
-		ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot: %v", err))
+		ctrl.updateContentErrorStatusWithEvent(contentObj, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot: %v", err))
 		klog.Errorf("createSnapshot for content [%s]: error occurred in createSnapshotWrapper: %v", content.Name, err)
 		return err
 	}
@@ -118,7 +120,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatus(content *c
 	klog.V(5).Infof("checkandUpdateContentStatus[%s] started", content.Name)
 	contentObj, err := ctrl.checkandUpdateContentStatusOperation(content)
 	if err != nil {
-		ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotContentCheckandUpdateFailed", fmt.Sprintf("Failed to check and update snapshot content: %v", err))
+		ctrl.updateContentErrorStatusWithEvent(contentObj, v1.EventTypeWarning, "SnapshotContentCheckandUpdateFailed", fmt.Sprintf("Failed to check and update snapshot content: %v", err))
 		klog.Errorf("checkandUpdateContentStatus [%s]: error occurred %v", content.Name, err)
 		return err
 	}
@@ -221,27 +223,27 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c
 			class, err := ctrl.getSnapshotClass(*content.Spec.VolumeSnapshotClassName)
 			if err != nil {
 				klog.Errorf("Failed to get snapshot class %s for snapshot content %s: %v", *content.Spec.VolumeSnapshotClassName, content.Name, err)
-				return nil, fmt.Errorf("failed to get snapshot class %s for snapshot content %s: %v", *content.Spec.VolumeSnapshotClassName, content.Name, err)
+				return content, fmt.Errorf("failed to get snapshot class %s for snapshot content %s: %v", *content.Spec.VolumeSnapshotClassName, content.Name, err)
 			}
 
 			snapshotterListSecretRef, err := utils.GetSecretReference(utils.SnapshotterListSecretParams, class.Parameters, content.GetObjectMeta().GetName(), nil)
 			if err != nil {
 				klog.Errorf("Failed to get secret reference for snapshot content %s: %v", content.Name, err)
-				return nil, fmt.Errorf("failed to get secret reference for snapshot content %s: %v", content.Name, err)
+				return content, fmt.Errorf("failed to get secret reference for snapshot content %s: %v", content.Name, err)
 			}
 
 			snapshotterListCredentials, err = utils.GetCredentials(ctrl.client, snapshotterListSecretRef)
 			if err != nil {
 				// Continue with deletion, as the secret may have already been deleted.
 				klog.Errorf("Failed to get credentials for snapshot content %s: %v", content.Name, err)
-				return nil, fmt.Errorf("failed to get credentials for snapshot content %s: %v", content.Name, err)
+				return content, fmt.Errorf("failed to get credentials for snapshot content %s: %v", content.Name, err)
 			}
 		}
 
 		readyToUse, creationTime, size, err = ctrl.handler.GetSnapshotStatus(content, snapshotterListCredentials)
 		if err != nil {
 			klog.Errorf("checkandUpdateContentStatusOperation: failed to call get snapshot status to check whether snapshot is ready to use %q", err)
-			return nil, err
+			return content, err
 		}
 		driverName = content.Spec.Driver
 		snapshotID = *content.Spec.Source.SnapshotHandle
@@ -254,7 +256,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c
 
 		updatedContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size)
 		if err != nil {
-			return nil, err
+			return content, err
 		}
 		return updatedContent, nil
 	}
@@ -268,7 +270,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V
 
 	class, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get input parameters to create snapshot for content %s: %q", content.Name, err)
+		return content, fmt.Errorf("failed to get input parameters to create snapshot for content %s: %q", content.Name, err)
 	}
 
 	// NOTE(xyang): handle create timeout
@@ -278,14 +280,14 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V
 	// success or permanent failure. If the request times out, annotation will
 	// remain on the content to avoid potential leaking of a snapshot resource on
 	// the storage system.
-	err = ctrl.setAnnVolumeSnapshotBeingCreated(content)
+	content, err = ctrl.setAnnVolumeSnapshotBeingCreated(content)
 	if err != nil {
-		return nil, fmt.Errorf("failed to add VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err)
+		return content, fmt.Errorf("failed to add VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err)
 	}
 
 	parameters, err := utils.RemovePrefixedParameters(class.Parameters)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove CSI Parameters of prefixed keys: %v", err)
+		return content, fmt.Errorf("failed to remove CSI Parameters of prefixed keys: %v", err)
 	}
 	if ctrl.extraCreateMetadata {
 		parameters[utils.PrefixedVolumeSnapshotNameKey] = content.Spec.VolumeSnapshotRef.Name
@@ -300,12 +302,13 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V
 		// storage system has responded with an error
 		klog.Infof("createSnapshotWrapper: CreateSnapshot for content %s returned error: %v", content.Name, err)
 		if isCSIFinalError(err) {
-			if removeAnnotationErr := ctrl.removeAnnVolumeSnapshotBeingCreated(content); removeAnnotationErr != nil {
-				return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %s", content.Name, removeAnnotationErr)
+			var removeAnnotationErr error
+			if content, removeAnnotationErr = ctrl.removeAnnVolumeSnapshotBeingCreated(content); removeAnnotationErr != nil {
+				return content, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %s", content.Name, removeAnnotationErr)
 			}
 		}
 
-		return nil, fmt.Errorf("failed to take snapshot of the volume %s: %q", *content.Spec.Source.VolumeHandle, err)
+		return content, fmt.Errorf("failed to take snapshot of the volume %s: %q", *content.Spec.Source.VolumeHandle, err)
 	}
 
 	klog.V(5).Infof("Created snapshot: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse)
@@ -317,16 +320,16 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V
 	newContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size)
 	if err != nil {
 		klog.Errorf("error updating status for volume snapshot content %s: %v.", content.Name, err)
-		return nil, fmt.Errorf("error updating status for volume snapshot content %s: %v", content.Name, err)
+		return content, fmt.Errorf("error updating status for volume snapshot content %s: %v", content.Name, err)
 	}
 	content = newContent
 
 	// NOTE(xyang): handle create timeout
 	// Remove annotation to indicate storage system has successfully
 	// cut the snapshot
-	err = ctrl.removeAnnVolumeSnapshotBeingCreated(content)
+	content, err = ctrl.removeAnnVolumeSnapshotBeingCreated(content)
 	if err != nil {
-		return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err)
+		return content, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err)
 	}
 
 	return content, nil
@@ -378,7 +381,7 @@ func (ctrl *csiSnapshotSideCarController) clearVolumeContentStatus(
 	}
 	newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().UpdateStatus(context.TODO(), content, metav1.UpdateOptions{})
 	if err != nil {
-		return nil, newControllerUpdateError(contentName, err.Error())
+		return content, newControllerUpdateError(contentName, err.Error())
 	}
 	return newContent, nil
 }
@@ -434,7 +437,7 @@ func (ctrl *csiSnapshotSideCarController) updateSnapshotContentStatus(
 		contentClone.Status = newStatus
 		newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().UpdateStatus(context.TODO(), contentClone, metav1.UpdateOptions{})
 		if err != nil {
-			return nil, newControllerUpdateError(content.Name, err.Error())
+			return contentObj, newControllerUpdateError(content.Name, err.Error())
 		}
 		return newContent, nil
 	}
@@ -520,13 +523,13 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V
 	contentClone := content.DeepCopy()
 	contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
 
-	_, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
+	updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
 	if err != nil {
 		return newControllerUpdateError(content.Name, err.Error())
 	}
 
-	klog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", content.Name)
-	_, err = ctrl.storeContentUpdate(contentClone)
+	klog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", updatedContent.Name)
+	_, err = ctrl.storeContentUpdate(updatedContent)
 	if err != nil {
 		klog.Errorf("failed to update content store %v", err)
 	}
@@ -566,10 +569,10 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap
 // setAnnVolumeSnapshotBeingCreated sets VolumeSnapshotBeingCreated annotation
 // on VolumeSnapshotContent
 // If set, it indicates snapshot is being created
-func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) error {
+func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
 	if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) {
 		// the annotation already exists, return directly
-		return nil
+		return content, nil
 	}
 
 	// Set AnnVolumeSnapshotBeingCreated
@@ -579,7 +582,7 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte
 
 	updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
 	if err != nil {
-		return newControllerUpdateError(content.Name, err.Error())
+		return content, newControllerUpdateError(content.Name, err.Error())
 	}
 	// update content if update is successful
 	content = updatedContent
@@ -590,32 +593,30 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte
 	}
 	klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: volume snapshot content %+v", content)
 
-	return nil
+	return content, nil
 }
 
 // removeAnnVolumeSnapshotBeingCreated removes the VolumeSnapshotBeingCreated
 // annotation from a content if there exists one.
-func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) error {
+func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
 	if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) {
 		// the annotation does not exist, return directly
-		return nil
+		return content, nil
 	}
 	contentClone := content.DeepCopy()
 	delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated)
 
 	updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
 	if err != nil {
-		return newControllerUpdateError(content.Name, err.Error())
+		return content, newControllerUpdateError(content.Name, err.Error())
 	}
-	// update content if update is successful
-	content = updatedContent
 
 	klog.V(5).Infof("Removed VolumeSnapshotBeingCreated annotation from volume snapshot content %s", content.Name)
-	_, err = ctrl.storeContentUpdate(content)
+	_, err = ctrl.storeContentUpdate(updatedContent)
 	if err != nil {
 		klog.Errorf("failed to update content store %v", err)
 	}
-	return nil
+	return updatedContent, nil
 }
 
 // This function checks if the error is final
diff --git a/pkg/sidecar-controller/snapshot_controller_base.go b/pkg/sidecar-controller/snapshot_controller_base.go
index 0d2030652..56b9849ba 100644
--- a/pkg/sidecar-controller/snapshot_controller_base.go
+++ b/pkg/sidecar-controller/snapshot_controller_base.go
@@ -72,7 +72,6 @@ func NewCSISnapshotSideCarController(
 	snapshotNamePrefix string,
 	snapshotNameUUIDLength int,
 	extraCreateMetadata bool,
-	contentRateLimiter workqueue.RateLimiter,
 ) *csiSnapshotSideCarController {
 	broadcaster := record.NewBroadcaster()
 	broadcaster.StartLogging(klog.Infof)
@@ -88,7 +87,7 @@ func NewCSISnapshotSideCarController(
 		handler:             NewCSIHandler(snapshotter, timeout, snapshotNamePrefix, snapshotNameUUIDLength),
 		resyncPeriod:        resyncPeriod,
 		contentStore:        cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc),
-		contentQueue:        workqueue.NewNamedRateLimitingQueue(contentRateLimiter, "csi-snapshotter-content"),
+		contentQueue:        workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "csi-snapshotter-content"),
 		extraCreateMetadata: extraCreateMetadata,
 	}
 


Test patch:
diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go
index 414cfcc42..81b340c38 100644
--- a/pkg/common-controller/framework_test.go
+++ b/pkg/common-controller/framework_test.go
@@ -19,7 +19,6 @@ package common_controller
 import (
 	"errors"
 	"fmt"
-	"k8s.io/client-go/util/workqueue"
 	"reflect"
 	sysruntime "runtime"
 	"strconv"
@@ -750,8 +749,6 @@ func newTestController(kubeClient kubernetes.Interface, clientset clientset.Inte
 		coreFactory.Core().V1().PersistentVolumeClaims(),
 		metricsManager,
 		60*time.Second,
-		workqueue.NewItemExponentialFailureRateLimiter(1*time.Millisecond, 1*time.Minute),
-		workqueue.NewItemExponentialFailureRateLimiter(1*time.Millisecond, 1*time.Minute),
 	)
 
 	ctrl.eventRecorder = record.NewFakeRecorder(1000)
diff --git a/pkg/sidecar-controller/framework_test.go b/pkg/sidecar-controller/framework_test.go
index 078cfd0a0..7a8f0f0e8 100644
--- a/pkg/sidecar-controller/framework_test.go
+++ b/pkg/sidecar-controller/framework_test.go
@@ -46,7 +46,6 @@ import (
 	core "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
-	"k8s.io/client-go/util/workqueue"
 	klog "k8s.io/klog/v2"
 )
 
@@ -523,7 +522,6 @@ func newTestController(kubeClient kubernetes.Interface, clientset clientset.Inte
 		"snapshot",
 		-1,
 		true,
-		workqueue.NewItemExponentialFailureRateLimiter(1*time.Millisecond, 1*time.Minute),
 	)
 
 	ctrl.eventRecorder = record.NewFakeRecorder(1000)