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

Issue:
VolumeSnapshotContent update fails with "object has been modified"
Seeing this consistently on snapshotting e2e tests in the snapshot controller pod:

```
~ curl -s https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/519/pull-kubernetes-csi-external-snapshotter-1-21-on-kubernetes-1-21/1396873247164731392/artifacts/kube-system/snapshot-controller-58f54d8f6c-9gt8c/snapshot-controller.log |  grep 'the object has been modified; please apply your' | wc -l
      56
```

There seems to be multiple root causes for this error. We'll be tracking them here:
- [x] [updateContentErrorStatusWithEvent](https://github.com/kubernetes-csi/external-snapshotter/blob/679e71e89d4644ae4b50fa71874b4d9a77bb9e5a/pkg/sidecar-controller/snapshot_controller.go#L140) - ~~if we perform a Get before Update here, the errors seem to drop by half.~~ Will not do this initially as it will lower performance.
- [x] [functions not using or returning updatedContent after update calls](https://github.com/kubernetes-csi/external-snapshotter/blob/8751eb0ca5b33a8a3c2589d506b48cc26684e0cd/pkg/sidecar-controller/snapshot_controller.go#L599) - various functions do not propagate their `updatedContent` back up the stack to be cached properly (see below for explanation). https://github.com/kubernetes-csi/external-snapshotter/pull/527
- [ ] JSONPatch can also be used to lower the error count - https://github.com/kubernetes-csi/external-snapshotter/pull/526

Pull request description:
Use JSON patch for many VolumeSnapshot and VolumeSnapshotContent updates
Signed-off-by: Grant Griffiths <ggriffiths@purestorage.com>

**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:
This PR addresses the "object has been modified" issue we see a lot in the snapshot-controller/snapshotter.

Before this PR, we were seeing ~30-50 occurrences of this error in the snapshot-controller/snapshotter logs.

This PR only addresses the problematic areas found in the e2e PR tests. If we continue to see more of these errors, we can    address replace the remaining Update/UpdateStatus calls in a separate PR.

**Which issue(s) this PR fixes**:
Fixes #525 

**Special notes for your reviewer**:
n/a

**Does this PR introduce a user-facing change?**:
```release-note
Replaces many VolumeSnapshot/VolumeSnapshotContent Update/UpdateStatus operations with Patch. This lowers the probability of the "object has been modified" update API errors occurring. This change introduces a dependency on two new RBAC rules for the snapshotter: volumesnapshotcontents:patch, volumesnapshotcontents/status:patch and four new RBAC rules for the snapshot-controller: volumesnapshotcontents:patch, volumesnapshotcontents/status:patch, volumesnapshots:patch, and volumesnapshots/status: patch.
```


Patch:
diff --git a/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml b/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml
index 9ca4e00a7..db4aefe80 100644
--- a/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml
+++ b/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml
@@ -35,10 +35,10 @@ rules:
     verbs: ["get", "list", "watch"]
   - apiGroups: ["snapshot.storage.k8s.io"]
     resources: ["volumesnapshotcontents"]
-    verbs: ["create", "get", "list", "watch", "update", "delete"]
+    verbs: ["create", "get", "list", "watch", "update", "delete", "patch"]
   - apiGroups: ["snapshot.storage.k8s.io"]
     resources: ["volumesnapshotcontents/status"]
-    verbs: ["update"]
+    verbs: ["update", "patch"]
 
 ---
 kind: ClusterRoleBinding
diff --git a/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml b/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml
index a6b8c7a9e..9fb7e9f38 100644
--- a/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml
+++ b/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml
@@ -34,13 +34,16 @@ rules:
     verbs: ["get", "list", "watch"]
   - apiGroups: ["snapshot.storage.k8s.io"]
     resources: ["volumesnapshotcontents"]
-    verbs: ["create", "get", "list", "watch", "update", "delete"]
+    verbs: ["create", "get", "list", "watch", "update", "delete", "patch"]
+  - apiGroups: ["snapshot.storage.k8s.io"]
+    resources: ["volumesnapshotcontents/status"]
+    verbs: ["patch"]
   - apiGroups: ["snapshot.storage.k8s.io"]
     resources: ["volumesnapshots"]
-    verbs: ["get", "list", "watch", "update"]
+    verbs: ["get", "list", "watch", "update", "patch"]
   - apiGroups: ["snapshot.storage.k8s.io"]
     resources: ["volumesnapshots/status"]
-    verbs: ["update"]
+    verbs: ["update", "patch"]
 
 ---
 kind: ClusterRoleBinding
diff --git a/go.mod b/go.mod
index 467d61e40..44fb15cb5 100644
--- a/go.mod
+++ b/go.mod
@@ -4,6 +4,7 @@ go 1.16
 
 require (
 	github.com/container-storage-interface/spec v1.5.0
+	github.com/evanphx/json-patch v4.11.0+incompatible
 	github.com/fsnotify/fsnotify v1.4.9
 	github.com/golang/mock v1.4.4
 	github.com/golang/protobuf v1.5.2
diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go
index 48786f846..689e845bb 100644
--- a/pkg/common-controller/snapshot_controller.go
+++ b/pkg/common-controller/snapshot_controller.go
@@ -809,10 +809,25 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap
 
 // addContentFinalizer adds a Finalizer for VolumeSnapshotContent.
 func (ctrl *csiSnapshotCommonController) addContentFinalizer(content *crdv1.VolumeSnapshotContent) error {
-	contentClone := content.DeepCopy()
-	contentClone.ObjectMeta.Finalizers = append(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
 
-	newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
+	var patches []utils.PatchOp
+	if len(content.Finalizers) > 0 {
+		// Add to the end of the finalizers if we have any other finalizers
+		patches = append(patches, utils.PatchOp{
+			Op:    "add",
+			Path:  "/metadata/finalizers/-",
+			Value: utils.VolumeSnapshotContentFinalizer,
+		})
+
+	} else {
+		// Replace finalizers with new array if there are no other finalizers
+		patches = append(patches, utils.PatchOp{
+			Op:    "add",
+			Path:  "/metadata/finalizers",
+			Value: []string{utils.VolumeSnapshotContentFinalizer},
+		})
+	}
+	newContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset)
 	if err != nil {
 		return newControllerUpdateError(content.Name, err.Error())
 	}
@@ -983,15 +998,26 @@ func (ctrl *csiSnapshotCommonController) checkandBindSnapshotContent(snapshot *c
 	} else if content.Spec.VolumeSnapshotRef.UID != "" && content.Spec.VolumeSnapshotClassName != nil {
 		return content, nil
 	}
-	contentClone := content.DeepCopy()
-	contentClone.Spec.VolumeSnapshotRef.UID = snapshot.UID
+
+	patches := []utils.PatchOp{
+		{
+			Op:    "replace",
+			Path:  "/spec/volumeSnapshotRef/uid",
+			Value: string(snapshot.UID),
+		},
+	}
 	if snapshot.Spec.VolumeSnapshotClassName != nil {
 		className := *(snapshot.Spec.VolumeSnapshotClassName)
-		contentClone.Spec.VolumeSnapshotClassName = &className
+		patches = append(patches, utils.PatchOp{
+			Op:    "replace",
+			Path:  "/spec/volumeSnapshotClassName",
+			Value: className,
+		})
 	}
-	newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
+
+	newContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset)
 	if err != nil {
-		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", contentClone.Name, err)
+		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", content.Name, err)
 		return content, err
 	}
 
@@ -1392,24 +1418,55 @@ func isControllerUpdateFailError(err *crdv1.VolumeSnapshotError) bool {
 
 // addSnapshotFinalizer adds a Finalizer for VolumeSnapshot.
 func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot, addSourceFinalizer bool, addBoundFinalizer bool) error {
-	snapshotClone := snapshot.DeepCopy()
-	if addSourceFinalizer {
-		snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer)
-	}
-	if addBoundFinalizer {
-		snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
-	}
-	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
-	if err != nil {
-		return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
+	var updatedSnapshot *crdv1.VolumeSnapshot
+	var err error
+
+	// NOTE(ggriffiths): Must perform an update if no finalizers exist.
+	// Unable to find a patch that correctly updated the finalizers if none currently exist.
+	if len(snapshot.ObjectMeta.Finalizers) == 0 {
+		snapshotClone := snapshot.DeepCopy()
+		if addSourceFinalizer {
+			snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer)
+		}
+		if addBoundFinalizer {
+			snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
+		}
+		updatedSnapshot, err = ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+		if err != nil {
+			return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
+		}
+	} else {
+		// Otherwise, perform a patch
+		var patches []utils.PatchOp
+
+		// If finalizers exist already, add new ones to the end of the array
+		if addSourceFinalizer {
+			patches = append(patches, utils.PatchOp{
+				Op:    "add",
+				Path:  "/metadata/finalizers/-",
+				Value: utils.VolumeSnapshotAsSourceFinalizer,
+			})
+		}
+		if addBoundFinalizer {
+			patches = append(patches, utils.PatchOp{
+				Op:    "add",
+				Path:  "/metadata/finalizers/-",
+				Value: utils.VolumeSnapshotBoundFinalizer,
+			})
+		}
+
+		updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset)
+		if err != nil {
+			return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
+		}
 	}
 
-	_, err = ctrl.storeSnapshotUpdate(newSnapshot)
+	_, err = ctrl.storeSnapshotUpdate(updatedSnapshot)
 	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(newSnapshot))
+	klog.V(5).Infof("Added protection finalizer to volume snapshot %s", utils.SnapshotKey(updatedSnapshot))
 	return nil
 }
 
@@ -1489,14 +1546,21 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten
 	// Set AnnVolumeSnapshotBeingDeleted if it is not set yet
 	if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) {
 		klog.V(5).Infof("setAnnVolumeSnapshotBeingDeleted: set annotation [%s] on content [%s].", utils.AnnVolumeSnapshotBeingDeleted, content.Name)
+		var patches []utils.PatchOp
 		metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes")
+		patches = append(patches, utils.PatchOp{
+			Op:    "replace",
+			Path:  "/metadata/annotations",
+			Value: content.ObjectMeta.GetAnnotations(),
+		})
 
-		updateContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), content, metav1.UpdateOptions{})
+		patchedContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset)
 		if err != nil {
 			return content, newControllerUpdateError(content.Name, err.Error())
 		}
+
 		// update content if update is successful
-		content = updateContent
+		content = patchedContent
 
 		_, err = ctrl.storeContentUpdate(content)
 		if err != nil {
diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go
index ec0c6deb0..313f173db 100644
--- a/pkg/sidecar-controller/snapshot_controller.go
+++ b/pkg/sidecar-controller/snapshot_controller.go
@@ -146,19 +146,26 @@ func (ctrl *csiSnapshotSideCarController) updateContentErrorStatusWithEvent(cont
 		klog.V(4).Infof("updateContentStatusWithEvent[%s]: the same error %v is already set", content.Name, content.Status.Error)
 		return nil
 	}
-	contentClone := content.DeepCopy()
-	if contentClone.Status == nil {
-		contentClone.Status = &crdv1.VolumeSnapshotContentStatus{}
-	}
-	contentClone.Status.Error = &crdv1.VolumeSnapshotError{
-		Time: &metav1.Time{
-			Time: time.Now(),
+
+	ready := false
+	patch := []utils.PatchOp{
+		{
+			Op:   "replace",
+			Path: "/status/error",
+			Value: &crdv1.VolumeSnapshotError{
+				Time: &metav1.Time{
+					Time: time.Now(),
+				},
+				Message: &message,
+			},
+		},
+		{
+			Op:    "replace",
+			Path:  "/status/readyToUse",
+			Value: &ready,
 		},
-		Message: &message,
 	}
-	ready := false
-	contentClone.Status.ReadyToUse = &ready
-	newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().UpdateStatus(context.TODO(), contentClone, metav1.UpdateOptions{})
+	newContent, err := utils.PatchVolumeSnapshotContent(content, patch, ctrl.clientset, "status")
 
 	// Emit the event even if the status update fails so that user can see the error
 	ctrl.eventRecorder.Event(newContent, eventtype, reason, message)
@@ -576,16 +583,28 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte
 	}
 
 	// Set AnnVolumeSnapshotBeingCreated
+	// Combine existing annotations with the new annotations.
+	// If there are no existing annotations, we create a new map.
 	klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: set annotation [%s:yes] on content [%s].", utils.AnnVolumeSnapshotBeingCreated, content.Name)
-	contentClone := content.DeepCopy()
-	metav1.SetMetaDataAnnotation(&contentClone.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, "yes")
+	patchedAnnotations := make(map[string]string)
+	for k, v := range content.GetAnnotations() {
+		patchedAnnotations[k] = v
+	}
+	patchedAnnotations[utils.AnnVolumeSnapshotBeingCreated] = "yes"
 
-	updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
+	var patches []utils.PatchOp
+	patches = append(patches, utils.PatchOp{
+		Op:    "replace",
+		Path:  "/metadata/annotations",
+		Value: patchedAnnotations,
+	})
+
+	patchedContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset)
 	if err != nil {
 		return content, newControllerUpdateError(content.Name, err.Error())
 	}
 	// update content if update is successful
-	content = updatedContent
+	content = patchedContent
 
 	_, err = ctrl.storeContentUpdate(content)
 	if err != nil {
diff --git a/pkg/utils/patch.go b/pkg/utils/patch.go
new file mode 100644
index 000000000..abd77a3d2
--- /dev/null
+++ b/pkg/utils/patch.go
@@ -0,0 +1,58 @@
+package utils
+
+import (
+	"context"
+	"encoding/json"
+
+	crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
+	clientset "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+)
+
+// PatchOp represents a json patch operation
+type PatchOp struct {
+	Op    string      `json:"op"`
+	Path  string      `json:"path"`
+	Value interface{} `json:"value,omitempty"`
+}
+
+// PatchVolumeSnapshotContent patches a volume snapshot content object
+func PatchVolumeSnapshotContent(
+	existingSnapshotContent *crdv1.VolumeSnapshotContent,
+	patch []PatchOp,
+	client clientset.Interface,
+	subresources ...string,
+) (*crdv1.VolumeSnapshotContent, error) {
+	data, err := json.Marshal(patch)
+	if nil != err {
+		return existingSnapshotContent, err
+	}
+
+	newSnapshotContent, err := client.SnapshotV1().VolumeSnapshotContents().Patch(context.TODO(), existingSnapshotContent.Name, types.JSONPatchType, data, metav1.PatchOptions{}, subresources...)
+	if err != nil {
+		return existingSnapshotContent, err
+	}
+
+	return newSnapshotContent, nil
+}
+
+// PatchVolumeSnapshot patches a volume snapshot object
+func PatchVolumeSnapshot(
+	existingSnapshot *crdv1.VolumeSnapshot,
+	patch []PatchOp,
+	client clientset.Interface,
+	subresources ...string,
+) (*crdv1.VolumeSnapshot, error) {
+	data, err := json.Marshal(patch)
+	if nil != err {
+		return existingSnapshot, err
+	}
+
+	newSnapshot, err := client.SnapshotV1().VolumeSnapshots(existingSnapshot.Namespace).Patch(context.TODO(), existingSnapshot.Name, types.JSONPatchType, data, metav1.PatchOptions{}, subresources...)
+	if err != nil {
+		return existingSnapshot, err
+	}
+
+	return newSnapshot, nil
+}
diff --git a/vendor/modules.txt b/vendor/modules.txt
index 541ae632b..c4f646d94 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -10,6 +10,7 @@ github.com/container-storage-interface/spec/lib/go/csi
 # github.com/davecgh/go-spew v1.1.1
 github.com/davecgh/go-spew/spew
 # github.com/evanphx/json-patch v4.11.0+incompatible
+## explicit
 github.com/evanphx/json-patch
 # github.com/fsnotify/fsnotify v1.4.9
 ## explicit


Test patch:
diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go
index c0093f008..78860c07a 100644
--- a/pkg/common-controller/framework_test.go
+++ b/pkg/common-controller/framework_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package common_controller
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"net/http"
@@ -29,8 +30,7 @@ import (
 	"testing"
 	"time"
 
-	"k8s.io/client-go/util/workqueue"
-
+	jsonpatch "github.com/evanphx/json-patch"
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
 	clientset "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned"
 	"github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake"
@@ -55,6 +55,7 @@ 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"
 )
 
@@ -258,6 +259,46 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		klog.V(4).Infof("saved updated content %s", content.Name)
 		return true, content, nil
 
+	case action.Matches("patch", "volumesnapshotcontents"):
+		content := &crdv1.VolumeSnapshotContent{}
+		action := action.(core.PatchAction)
+
+		// Check and bump object version
+		storedSnapshotContent, found := r.contents[action.GetName()]
+		if found {
+			// Apply patch
+			storedSnapshotBytes, err := json.Marshal(storedSnapshotContent)
+			if err != nil {
+				return true, nil, err
+			}
+			contentPatch, err := jsonpatch.DecodePatch(action.GetPatch())
+			if err != nil {
+				return true, nil, err
+			}
+
+			modified, err := contentPatch.Apply(storedSnapshotBytes)
+			if err != nil {
+				return true, nil, err
+			}
+
+			err = json.Unmarshal(modified, content)
+			if err != nil {
+				return true, nil, err
+			}
+
+			storedVer, _ := strconv.Atoi(content.ResourceVersion)
+			content.ResourceVersion = strconv.Itoa(storedVer + 1)
+		} else {
+			return true, nil, fmt.Errorf("cannot update snapshot content %s: snapshot content not found", action.GetName())
+		}
+
+		// Store the updated object to appropriate places.
+		r.contents[content.Name] = content
+		r.changedObjects = append(r.changedObjects, content)
+		r.changedSinceLastSync++
+		klog.V(4).Infof("saved updated content %s", content.Name)
+		return true, content, nil
+
 	case action.Matches("update", "volumesnapshots"):
 		obj := action.(core.UpdateAction).GetObject()
 		snapshot := obj.(*crdv1.VolumeSnapshot)
@@ -284,6 +325,45 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		klog.V(4).Infof("saved updated snapshot %s", snapshot.Name)
 		return true, snapshot, nil
 
+	case action.Matches("patch", "volumesnapshots"):
+		action := action.(core.PatchAction)
+		// Check and bump object version
+		storedSnapshot, found := r.snapshots[action.GetName()]
+		if found {
+			// Apply patch
+			storedSnapshotBytes, err := json.Marshal(storedSnapshot)
+			if err != nil {
+				return true, nil, err
+			}
+			snapPatch, err := jsonpatch.DecodePatch(action.GetPatch())
+			if err != nil {
+				return true, nil, err
+			}
+
+			modified, err := snapPatch.Apply(storedSnapshotBytes)
+			if err != nil {
+				return true, nil, err
+			}
+
+			err = json.Unmarshal(modified, storedSnapshot)
+			if err != nil {
+				return true, nil, err
+			}
+
+			storedVer, _ := strconv.Atoi(storedSnapshot.ResourceVersion)
+			storedSnapshot.ResourceVersion = strconv.Itoa(storedVer + 1)
+		} else {
+			return true, nil, fmt.Errorf("cannot update snapshot %s: snapshot not found", action.GetName())
+		}
+
+		// Store the updated object to appropriate places.
+		r.snapshots[storedSnapshot.Name] = storedSnapshot
+		r.changedObjects = append(r.changedObjects, storedSnapshot)
+		r.changedSinceLastSync++
+
+		klog.V(4).Infof("saved updated snapshot %s", storedSnapshot.Name)
+		return true, storedSnapshot, nil
+
 	case action.Matches("get", "volumesnapshotcontents"):
 		name := action.(core.GetAction).GetName()
 		content, found := r.contents[name]
@@ -437,6 +517,7 @@ func (r *snapshotReactor) checkContents(expectedContents []*crdv1.VolumeSnapshot
 		}
 		gotMap[v.Name] = v
 	}
+
 	if !reflect.DeepEqual(expectedMap, gotMap) {
 		// Print ugly but useful diff of expected and received objects for
 		// easier debugging.
@@ -714,6 +795,8 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset,
 	client.AddReactor("create", "volumesnapshotcontents", reactor.React)
 	client.AddReactor("update", "volumesnapshotcontents", reactor.React)
 	client.AddReactor("update", "volumesnapshots", reactor.React)
+	client.AddReactor("patch", "volumesnapshotcontents", reactor.React)
+	client.AddReactor("patch", "volumesnapshots", reactor.React)
 	client.AddReactor("update", "volumesnapshotclasses", reactor.React)
 	client.AddReactor("get", "volumesnapshotcontents", reactor.React)
 	client.AddReactor("get", "volumesnapshots", reactor.React)
@@ -1169,6 +1252,10 @@ func testAddSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapsh
 	return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], true, true)
 }
 
+func testAddSingleSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
+	return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], false, true)
+}
+
 func testRemoveSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
 	return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true)
 }
@@ -1426,8 +1513,10 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot
 		if funcName == "testAddSnapshotFinalizer" {
 			for _, snapshot := range reactor.snapshots {
 				if test.initialSnapshots[0].Name == snapshot.Name {
-					if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
-						!utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
+					if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
+						utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
+						!utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) &&
+						utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
 						klog.V(4).Infof("test %q succeeded. Finalizers are added to snapshot %s", test.name, snapshot.Name)
 						bHasSnapshotFinalizer = true
 					}
@@ -1435,15 +1524,18 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot
 				}
 			}
 			if test.expectSuccess && !bHasSnapshotFinalizer {
-				t.Errorf("Test %q: failed to add finalizer to Snapshot %s", test.name, test.initialSnapshots[0].Name)
+				t.Errorf("Test %q: failed to add finalizer to Snapshot %s. Finalizers: %s", test.name, test.initialSnapshots[0].Name, test.initialSnapshots[0].GetFinalizers())
 			}
 		}
 		bHasSnapshotFinalizer = true
 		if funcName == "testRemoveSnapshotFinalizer" {
 			for _, snapshot := range reactor.snapshots {
 				if test.initialSnapshots[0].Name == snapshot.Name {
-					if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
-						utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
+					if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
+						!utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
+						utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) &&
+						!utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
+
 						klog.V(4).Infof("test %q succeeded. SnapshotFinalizer is removed from Snapshot %s", test.name, snapshot.Name)
 						bHasSnapshotFinalizer = false
 					}
diff --git a/pkg/common-controller/snapshot_finalizer_test.go b/pkg/common-controller/snapshot_finalizer_test.go
index ecaf02461..8e5a3f062 100644
--- a/pkg/common-controller/snapshot_finalizer_test.go
+++ b/pkg/common-controller/snapshot_finalizer_test.go
@@ -19,6 +19,7 @@ package common_controller
 import (
 	"testing"
 
+	"github.com/kubernetes-csi/external-snapshotter/v4/pkg/utils"
 	v1 "k8s.io/api/core/v1"
 )
 
@@ -70,7 +71,14 @@ func TestSnapshotFinalizer(t *testing.T) {
 			expectSuccess:    true,
 		},
 		{
-			name:             "2-1 - successful remove Snapshot finalizer",
+			name:             "2-2 - successful add single Snapshot finalizer with patch",
+			initialSnapshots: withSnapshotFinalizers(newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, false, nil), utils.VolumeSnapshotBoundFinalizer),
+			initialClaims:    newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
+			test:             testAddSingleSnapshotFinalizer,
+			expectSuccess:    true,
+		},
+		{
+			name:             "2-3 - successful remove Snapshot finalizer",
 			initialSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, true, nil),
 			initialClaims:    newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
 			test:             testRemoveSnapshotFinalizer,
diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go
index 864fe81c2..494082b8e 100644
--- a/pkg/common-controller/snapshot_update_test.go
+++ b/pkg/common-controller/snapshot_update_test.go
@@ -162,7 +162,7 @@ func TestSync(t *testing.T) {
 			expectedSnapshots: newSnapshotArray("snap2-12", "snapuid2-12", "", "content2-12", validSecretClass, "content2-12", &False, nil, nil, newVolumeError("Snapshot failed to bind VolumeSnapshotContent, mock update error"), false, true, nil),
 			errors: []reactorError{
 				// Inject error to the forth client.VolumesnapshotV1().VolumeSnapshots().Update call.
-				{"update", "volumesnapshotcontents", errors.New("mock update error")},
+				{"patch", "volumesnapshotcontents", errors.New("mock update error")},
 			},
 			test: testSyncSnapshot,
 		},
@@ -312,7 +312,7 @@ func TestSync(t *testing.T) {
 			initialSecrets:   []*v1.Secret{secret()},
 			errors: []reactorError{
 				// Inject error to the forth client.VolumesnapshotV1().VolumeSnapshots().Update call.
-				{"update", "volumesnapshotcontents", errors.New("mock update error")},
+				{"patch", "volumesnapshotcontents", errors.New("mock update error")},
 			},
 			expectSuccess: false,
 			test:          testSyncContentError,
diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go
index a980e7b66..af6ea04d8 100644
--- a/pkg/sidecar-controller/content_create_test.go
+++ b/pkg/sidecar-controller/content_create_test.go
@@ -85,7 +85,7 @@ func TestSyncContent(t *testing.T) {
 		{
 			name: "1-3: Basic sync content create snapshot with non-existent secret",
 			initialContents: withContentAnnotations(withContentStatus(newContentArray("content1-3", "snapuid1-3", "snap1-3", "sid1-3", invalidSecretClass, "", "volume-handle-1-3", retainPolicy, nil, &defaultSize, true),
-				nil), map[string]string{
+				&crdv1.VolumeSnapshotContentStatus{}), map[string]string{
 				utils.AnnDeletionSecretRefName:      "",
 				utils.AnnDeletionSecretRefNamespace: "",
 			}),
@@ -94,12 +94,12 @@ func TestSyncContent(t *testing.T) {
 					SnapshotHandle: nil,
 					RestoreSize:    nil,
 					ReadyToUse:     &False,
-					Error:          newSnapshotError("Failed to create snapshot: failed to get input parameters to create snapshot for content content1-3: \"cannot retrieve secrets for snapshot content \\\"content1-3\\\", err: secret name or namespace not specified\""),
+					Error:          newSnapshotError("Failed to check and update snapshot content: failed to get input parameters to create snapshot for content content1-3: \"cannot retrieve secrets for snapshot content \\\"content1-3\\\", err: secret name or namespace not specified\""),
 				}), map[string]string{
 				utils.AnnDeletionSecretRefName:      "",
 				utils.AnnDeletionSecretRefNamespace: "",
 			}), initialSecrets: []*v1.Secret{}, // no initial secret created
-			expectedEvents: []string{"Warning SnapshotCreationFailed"},
+			expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"},
 			errors:         noerrors,
 			test:           testSyncContent,
 		},
@@ -149,7 +149,7 @@ func TestSyncContent(t *testing.T) {
 		{
 			name: "1-5: Basic sync content create snapshot with failed secret call",
 			initialContents: withContentAnnotations(withContentStatus(newContentArray("content1-5", "snapuid1-5", "snap1-5", "sid1-5", invalidSecretClass, "", "volume-handle-1-5", retainPolicy, nil, &defaultSize, true),
-				nil), map[string]string{
+				&crdv1.VolumeSnapshotContentStatus{}), map[string]string{
 				utils.AnnDeletionSecretRefName:      "secret",
 				utils.AnnDeletionSecretRefNamespace: "default",
 			}),
@@ -158,12 +158,12 @@ func TestSyncContent(t *testing.T) {
 					SnapshotHandle: nil,
 					RestoreSize:    nil,
 					ReadyToUse:     &False,
-					Error:          newSnapshotError("Failed to create snapshot: failed to get input parameters to create snapshot for content content1-5: \"cannot get credentials for snapshot content \\\"content1-5\\\"\""),
+					Error:          newSnapshotError(`Failed to check and update snapshot content: failed to get input parameters to create snapshot for content content1-5: "cannot get credentials for snapshot content \"content1-5\""`),
 				}), map[string]string{
 				utils.AnnDeletionSecretRefName:      "secret",
 				utils.AnnDeletionSecretRefNamespace: "default",
 			}), initialSecrets: []*v1.Secret{}, // no initial secret created
-			expectedEvents: []string{"Warning SnapshotCreationFailed"},
+			expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"},
 			errors: []reactorError{
 				// Inject error to the first client.VolumesnapshotV1().VolumeSnapshots().Update call.
 				// All other calls will succeed.
diff --git a/pkg/sidecar-controller/framework_test.go b/pkg/sidecar-controller/framework_test.go
index 078cfd0a0..6a15b21ab 100644
--- a/pkg/sidecar-controller/framework_test.go
+++ b/pkg/sidecar-controller/framework_test.go
@@ -15,6 +15,7 @@ package sidecar_controller
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"reflect"
@@ -25,6 +26,7 @@ import (
 	"testing"
 	"time"
 
+	jsonpatch "github.com/evanphx/json-patch"
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
 	clientset "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned"
 	"github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake"
@@ -215,6 +217,46 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		klog.V(4).Infof("saved updated content %s", content.Name)
 		return true, content, nil
 
+	case action.Matches("patch", "volumesnapshotcontents"):
+		content := &crdv1.VolumeSnapshotContent{}
+		action := action.(core.PatchAction)
+
+		// Check and bump object version
+		storedSnapshotContent, found := r.contents[action.GetName()]
+		if found {
+			// Apply patch
+			storedSnapshotBytes, err := json.Marshal(storedSnapshotContent)
+			if err != nil {
+				return true, nil, err
+			}
+			contentPatch, err := jsonpatch.DecodePatch(action.GetPatch())
+			if err != nil {
+				return true, nil, err
+			}
+
+			modified, err := contentPatch.Apply(storedSnapshotBytes)
+			if err != nil {
+				return true, nil, err
+			}
+
+			err = json.Unmarshal(modified, content)
+			if err != nil {
+				return true, nil, err
+			}
+
+			storedVer, _ := strconv.Atoi(content.ResourceVersion)
+			content.ResourceVersion = strconv.Itoa(storedVer + 1)
+		} else {
+			return true, nil, fmt.Errorf("cannot update snapshot content %s: snapshot content not found", action.GetName())
+		}
+
+		// Store the updated object to appropriate places.
+		r.contents[content.Name] = content
+		r.changedObjects = append(r.changedObjects, content)
+		r.changedSinceLastSync++
+		klog.V(4).Infof("saved updated content %s", content.Name)
+		return true, content, nil
+
 	case action.Matches("get", "volumesnapshotcontents"):
 		name := action.(core.GetAction).GetName()
 		content, found := r.contents[name]
@@ -488,6 +530,7 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset,
 
 	client.AddReactor("create", "volumesnapshotcontents", reactor.React)
 	client.AddReactor("update", "volumesnapshotcontents", reactor.React)
+	client.AddReactor("patch", "volumesnapshotcontents", reactor.React)
 	client.AddReactor("get", "volumesnapshotcontents", reactor.React)
 	client.AddReactor("delete", "volumesnapshotcontents", reactor.React)
 	kubeClient.AddReactor("get", "secrets", reactor.React)