Switch from update to patch for VolumeSnapshot and Content

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", "delete", "patch"]
   - apiGroups: ["snapshot.storage.k8s.io"]
     resources: ["volumesnapshotcontents/status"]
-    verbs: ["update"]
+    verbs: ["patch"]
 
 ---
 kind: ClusterRoleBinding

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", "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", "patch"]
   - apiGroups: ["snapshot.storage.k8s.io"]
     resources: ["volumesnapshots/status"]
-    verbs: ["update"]
+    verbs: ["patch"]
 
 ---
 kind: ClusterRoleBinding

go.mod

@@ -4,6 +4,7 @@ go 1.15
 
 require (
 	github.com/container-storage-interface/spec v1.3.0
+	github.com/evanphx/json-patch v4.9.0+incompatible
 	github.com/fsnotify/fsnotify v1.4.9
 	github.com/go-logr/logr v0.3.0 // indirect
 	github.com/golang/mock v1.4.4

pkg/common-controller/framework_test.go

@@ -17,8 +17,10 @@ limitations under the License.
 package common_controller
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
+
 	"reflect"
 	sysruntime "runtime"
 	"strconv"
@@ -28,6 +30,7 @@ import (
 	"testing"
 	"time"
 
+	patch "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"
@@ -255,6 +258,49 @@ 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
+			}
+
+			mergedBytes, err := patch.MergePatch(storedSnapshotBytes, action.GetPatch())
+			if err != nil {
+				return true, nil, err
+			}
+			if err = json.Unmarshal(mergedBytes, content); err != nil {
+				return true, nil, err
+			}
+
+			storedVer, _ := strconv.Atoi(storedSnapshotContent.ResourceVersion)
+
+			// Don't modify the existing object
+			content = content.DeepCopy()
+			content.ResourceVersion = strconv.Itoa(storedVer + 1)
+
+			// If we were updating annotations and the new annotations are nil, leave as empty.
+			// This seems to be the behavior for merge-patching nil & empty annotations
+			if !reflect.DeepEqual(storedSnapshotContent.Annotations, content.Annotations) && content.Annotations == nil {
+				content.Annotations = make(map[string]string)
+			}
+		} 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)
@@ -281,6 +327,69 @@ 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"):
+		snapshot := &crdv1.VolumeSnapshot{}
+		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
+			}
+
+			mergedBytes, err := patch.MergePatch(storedSnapshotBytes, action.GetPatch())
+			if err != nil {
+				return true, nil, err
+			}
+			if err = json.Unmarshal(mergedBytes, snapshot); err != nil {
+				return true, nil, err
+			}
+
+			// We must re-assign the restore size as a new quantity.
+			// This is due to the behavior the json patch. The json serialization
+			// of a resource.Quantity is simply a number, i.e.
+			// "status": {
+			//	"boundVolumeSnapshotContentName": "content4-4",
+			//	"restoreSize": "5"
+			//}
+			//
+			// Upon de-serialization back into a resource.Quantity after
+			// the json merge occurs, this "restoreSize": "5" is parsed
+			// as a resource.DecimalSI of value 5, which is incorrect.
+			if snapshot.Status != nil && snapshot.Status.RestoreSize != nil {
+				size, ok := snapshot.Status.RestoreSize.AsInt64()
+				if !ok {
+					return true, nil, errors.New("failed to get restore size as int64")
+				}
+
+				snapshot.Status.RestoreSize = resource.NewQuantity(size, resource.BinarySI)
+			}
+
+			storedVer, _ := strconv.Atoi(storedSnapshot.ResourceVersion)
+
+			// Don't modify the existing object
+			snapshot = snapshot.DeepCopy()
+			snapshot.ResourceVersion = strconv.Itoa(storedVer + 1)
+
+			// If we were updating annotations and the new annotations are nil, leave as empty.
+			// This seems to be the behavior for merge-patching nil & empty annotations
+			if !reflect.DeepEqual(storedSnapshot.Annotations, snapshot.Annotations) && snapshot.Annotations == nil {
+				snapshot.Annotations = make(map[string]string)
+			}
+		} else {
+			return true, nil, fmt.Errorf("cannot update snapshot %s: snapshot not found", action.GetName())
+		}
+
+		// Store the updated object to appropriate places.
+		r.snapshots[snapshot.Name] = snapshot
+		r.changedObjects = append(r.changedObjects, snapshot)
+		r.changedSinceLastSync++
+		klog.Infof("saved patched snapshot %s", snapshot.Name)
+		return true, snapshot, nil
+
 	case action.Matches("get", "volumesnapshotcontents"):
 		name := action.(core.GetAction).GetName()
 		content, found := r.contents[name]
@@ -718,6 +827,8 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset,
 	client.AddReactor("delete", "volumesnapshotcontents", reactor.React)
 	client.AddReactor("delete", "volumesnapshots", reactor.React)
 	client.AddReactor("delete", "volumesnapshotclasses", reactor.React)
+	client.AddReactor("patch", "volumesnapshotcontents", reactor.React)
+	client.AddReactor("patch", "volumesnapshots", reactor.React)
 	kubeClient.AddReactor("get", "persistentvolumeclaims", reactor.React)
 	kubeClient.AddReactor("update", "persistentvolumeclaims", reactor.React)
 	kubeClient.AddReactor("get", "persistentvolumes", reactor.React)
@@ -1093,7 +1204,7 @@ func newVolumeArray(name, volumeUID, volumeHandle, capacity, boundToClaimUID, bo
 
 func newVolumeError(message string) *crdv1.VolumeSnapshotError {
 	return &crdv1.VolumeSnapshotError{
-		Time:    &metav1.Time{},
+		Time:    nil,
 		Message: &message,
 	}
 }

pkg/common-controller/snapshot_controller.go

@@ -758,7 +758,7 @@ func (ctrl *csiSnapshotCommonController) storeContentUpdate(content interface{})
 // given event on the snapshot. It saves the status and emits the event only when
 // the status has actually changed from the version saved in API server.
 // Parameters:
-//   snapshot - snapshot to update
+//   snapshot - snapshot to patch
 //   setReadyToFalse bool - indicates whether to set the snapshot's ReadyToUse status to false.
 //                          if true, ReadyToUse will be set to false;
 //                          otherwise, ReadyToUse will not be changed.
@@ -786,7 +786,7 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap
 		ready := false
 		snapshotClone.Status.ReadyToUse = &ready
 	}
-	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).UpdateStatus(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+	newSnapshot, err := utils.PatchVolumeSnapshot(snapshot, snapshotClone, ctrl.clientset, utils.StatusSubResource)
 
 	// Emit the event even if the status update fails so that user can see the error
 	ctrl.eventRecorder.Event(newSnapshot, eventtype, reason, message)
@@ -810,7 +810,7 @@ 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{})
+	_, err := utils.PatchVolumeSnapshotContent(content, contentClone, ctrl.clientset)
 	if err != nil {
 		return newControllerUpdateError(content.Name, err.Error())
 	}
@@ -987,7 +987,8 @@ func (ctrl *csiSnapshotCommonController) checkandBindSnapshotContent(snapshot *c
 		className := *(snapshot.Spec.VolumeSnapshotClassName)
 		contentClone.Spec.VolumeSnapshotClassName = &className
 	}
-	newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
+
+	newContent, err := utils.PatchVolumeSnapshotContent(content, contentClone, ctrl.clientset)
 	if err != nil {
 		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", contentClone.Name, err)
 		return nil, err
@@ -1163,7 +1164,7 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo
 			ctrl.metricsManager.RecordMetrics(createAndReadyOperation, metrics.NewSnapshotOperationStatus(metrics.SnapshotStatusTypeSuccess), driverName)
 		}
 
-		newSnapshotObj, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).UpdateStatus(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+		newSnapshotObj, err := utils.PatchVolumeSnapshot(snapshot, snapshotClone, ctrl.clientset, utils.StatusSubResource)
 		if err != nil {
 			return nil, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
 		}
@@ -1328,7 +1329,8 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
 	klog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name)
 	snapshotClone := snapshot.DeepCopy()
 	snapshotClone.Spec.VolumeSnapshotClassName = &(defaultClasses[0].Name)
-	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+
+	newSnapshot, err := utils.PatchVolumeSnapshot(snapshot, snapshotClone, ctrl.clientset)
 	if err != nil {
 		klog.V(4).Infof("updating VolumeSnapshot[%s] default class failed %v", utils.SnapshotKey(snapshot), err)
 	}
@@ -1393,7 +1395,7 @@ 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{})
+	_, err := utils.PatchVolumeSnapshot(snapshot, snapshotClone, ctrl.clientset)
 	if err != nil {
 		return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
 	}
@@ -1437,7 +1439,8 @@ 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{})
+
+	_, err := utils.PatchVolumeSnapshot(snapshot, snapshotClone, ctrl.clientset)
 	if err != nil {
 		return newControllerUpdateError(snapshot.Name, err.Error())
 	}
@@ -1483,9 +1486,10 @@ 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)
-		metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes")
+		contentClone := content.DeepCopy()
+		metav1.SetMetaDataAnnotation(&contentClone.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes")
 
-		updateContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), content, metav1.UpdateOptions{})
+		updateContent, err := utils.PatchVolumeSnapshotContent(content, contentClone, ctrl.clientset)
 		if err != nil {
 			return newControllerUpdateError(content.Name, err.Error())
 		}
@@ -1525,7 +1529,8 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content
 		}
 		contentClone.ObjectMeta.Labels[utils.VolumeSnapshotContentInvalidLabel] = ""
 	}
-	updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
+
+	updatedContent, err := utils.PatchVolumeSnapshotContent(content, contentClone, ctrl.clientset)
 	if err != nil {
 		return content, newControllerUpdateError(content.Name, err.Error())
 	}
@@ -1567,7 +1572,7 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapsho
 		snapshotClone.ObjectMeta.Labels[utils.VolumeSnapshotInvalidLabel] = ""
 	}
 
-	updatedSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+	updatedSnapshot, err := utils.PatchVolumeSnapshot(snapshot, snapshotClone, ctrl.clientset)
 	if err != nil {
 		return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
 	}

pkg/common-controller/snapshot_create_test.go

@@ -26,7 +26,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-var timeNow = time.Now()
+var timeNow = time.Now().Round(time.Second)
 var timeNowStamp = timeNow.UnixNano()
 var False = false
 var True = true
@@ -161,10 +161,10 @@ func TestCreateSnapshotSync(t *testing.T) {
 			initialClaims:     newClaimArray("claim7-9", "pvc-uid7-9", "1Gi", "volume7-9", v1.ClaimBound, &classGold),
 			initialVolumes:    newVolumeArray("volume7-9", "pv-uid7-9", "pv-handle7-9", "1Gi", "pvc-uid7-9", "claim7-9", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classGold),
 			errors: []reactorError{
-				{"update", "volumesnapshots", errors.New("mock update error")},
-				{"update", "volumesnapshots", errors.New("mock update error")},
-				{"update", "volumesnapshots", errors.New("mock update error")},
-				{"update", "volumesnapshots", errors.New("mock update error")},
+				{"patch", "volumesnapshots", errors.New("mock update error")},
+				{"patch", "volumesnapshots", errors.New("mock update error")},
+				{"patch", "volumesnapshots", errors.New("mock update error")},
+				{"patch", "volumesnapshots", errors.New("mock update error")},
 			},
 			expectSuccess: false,
 			test:          testSyncSnapshot,

pkg/common-controller/snapshot_delete_test.go

@@ -19,6 +19,7 @@ package common_controller
 import (
 	"errors"
 	"testing"
+	"time"
 
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
 	"github.com/kubernetes-csi/external-snapshotter/v4/pkg/utils"
@@ -49,7 +50,7 @@ var class5Parameters = map[string]string{
 	utils.PrefixedSnapshotterSecretNamespaceKey: "default",
 }
 
-var timeNowMetav1 = metav1.Now()
+var timeNowMetav1 = metav1.Time{Time: time.Now().Round(time.Second)}
 
 var content31 = "content3-1"
 var claim31 = "claim3-1"