Use patch instead of update for GroupSnapshots, VolumeSnapshots, PVCs

pkg/common-controller/framework_test.go

@@ -271,6 +271,8 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		// Check and bump object version
 		storedSnapshotContent, found := r.contents[action.GetName()]
 		if found {
+			// don't modify existing object
+			storedSnapshotContent = storedSnapshotContent.DeepCopy()
 			// Apply patch
 			storedSnapshotBytes, err := json.Marshal(storedSnapshotContent)
 			if err != nil {
@@ -335,6 +337,8 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		// Check and bump object version
 		storedSnapshot, found := r.snapshots[action.GetName()]
 		if found {
+			// don't modify existing object
+			storedSnapshot = storedSnapshot.DeepCopy()
 			// Apply patch
 			storedSnapshotBytes, err := json.Marshal(storedSnapshot)
 			if err != nil {
@@ -349,7 +353,8 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 			if err != nil {
 				return true, nil, err
 			}
-
+			// following unmarshal removes the time millisecond precision which was present in the original object
+			// make sure time used in tests are in seconds precision
 			err = json.Unmarshal(modified, storedSnapshot)
 			if err != nil {
 				return true, nil, err
@@ -1425,7 +1430,6 @@ func runFinalizerTests(t *testing.T, tests []controllerTest, snapshotClasses []*
 	snapshotscheme.AddToScheme(scheme.Scheme)
 	for _, test := range tests {
 		klog.V(4).Infof("starting test %q", test.name)
-
 		// Initialize the controller
 		kubeClient := &kubefake.Clientset{}
 		client := &fake.Clientset{}

pkg/common-controller/groupsnapshot_controller_helper.go

@@ -23,9 +23,9 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	ref "k8s.io/client-go/tools/reference"
 	klog "k8s.io/klog/v2"
@@ -154,8 +154,7 @@ func (ctrl *csiSnapshotCommonController) SetDefaultGroupSnapshotClass(groupSnaps
 	}
 	klog.V(5).Infof("setDefaultGroupSnapshotClass [%s]: default VolumeGroupSnapshotClassName [%s]", groupSnapshot.Name, defaultClasses[0].Name)
 	groupSnapshotClone := groupSnapshot.DeepCopy()
-	groupSnapshotClone.Spec.VolumeGroupSnapshotClassName = &(defaultClasses[0].Name)
-	newGroupSnapshot, err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshots(groupSnapshotClone.Namespace).Update(context.TODO(), groupSnapshotClone, metav1.UpdateOptions{})
+	newGroupSnapshot, err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshots(groupSnapshotClone.Namespace).Patch(context.TODO(), groupSnapshotClone.Name, types.MergePatchType, []byte(fmt.Sprintf(`{"spec":{"volumeGroupSnapshotClassName":"%s"}}`, defaultClasses[0].Name)), metav1.PatchOptions{})
 	if err != nil {
 		klog.V(4).Infof("updating VolumeGroupSnapshot[%s] default group snapshot class failed %v", utils.GroupSnapshotKey(groupSnapshot), err)
 	}
@@ -780,7 +779,7 @@ func (ctrl *csiSnapshotCommonController) createGroupSnapshotContent(groupSnapsho
 	klog.V(5).Infof("volume group snapshot content %#v", groupSnapshotContent)
 	// Try to create the VolumeGroupSnapshotContent object
 	klog.V(5).Infof("createGroupSnapshotContent [%s]: trying to save volume group snapshot content %s", utils.GroupSnapshotKey(groupSnapshot), groupSnapshotContent.Name)
-	if updateGroupSnapshotContent, err = ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshotContents().Create(context.TODO(), groupSnapshotContent, metav1.CreateOptions{}); err == nil || apierrs.IsAlreadyExists(err) {
+	if updateGroupSnapshotContent, err = ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshotContents().Create(context.TODO(), groupSnapshotContent, metav1.CreateOptions{}); err == nil || errors.IsAlreadyExists(err) {
 		// Save succeeded.
 		if err != nil {
 			klog.V(3).Infof("volume group snapshot content %q for group snapshot %q already exists, reusing", groupSnapshotContent.Name, utils.GroupSnapshotKey(groupSnapshot))
@@ -1018,33 +1017,19 @@ func (ctrl *csiSnapshotCommonController) addGroupSnapshotFinalizer(groupSnapshot
 	var updatedGroupSnapshot *crdv1alpha1.VolumeGroupSnapshot
 	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(groupSnapshot.ObjectMeta.Finalizers) == 0 {
-		groupSnapshotClone := groupSnapshot.DeepCopy()
-		if addBoundFinalizer {
-			groupSnapshotClone.ObjectMeta.Finalizers = append(groupSnapshotClone.ObjectMeta.Finalizers, utils.VolumeGroupSnapshotBoundFinalizer)
-		}
-		updatedGroupSnapshot, err = ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshots(groupSnapshotClone.Namespace).Update(context.TODO(), groupSnapshotClone, metav1.UpdateOptions{})
-		if err != nil {
-			return newControllerUpdateError(utils.GroupSnapshotKey(groupSnapshot), err.Error())
-		}
-	} else {
-		// Otherwise, perform a patch
-		var patches []utils.PatchOp
+	var patches []utils.PatchOp
 
-		if addBoundFinalizer {
-			patches = append(patches, utils.PatchOp{
-				Op:    "add",
-				Path:  "/metadata/finalizers/-",
-				Value: utils.VolumeGroupSnapshotBoundFinalizer,
-			})
-		}
+	if addBoundFinalizer {
+		patches = append(patches, utils.PatchOp{
+			Op:    "add",
+			Path:  "/metadata/finalizers/-",
+			Value: utils.VolumeGroupSnapshotBoundFinalizer,
+		})
+	}
 
-		updatedGroupSnapshot, err = utils.PatchVolumeGroupSnapshot(groupSnapshot, patches, ctrl.clientset)
-		if err != nil {
-			return newControllerUpdateError(utils.GroupSnapshotKey(groupSnapshot), err.Error())
-		}
+	updatedGroupSnapshot, err = utils.PatchVolumeGroupSnapshot(groupSnapshot, patches, ctrl.clientset)
+	if err != nil {
+		return newControllerUpdateError(utils.GroupSnapshotKey(groupSnapshot), err.Error())
 	}
 
 	_, err = ctrl.storeGroupSnapshotUpdate(updatedGroupSnapshot)
@@ -1114,7 +1099,7 @@ func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimesta
 	for _, snapshotRef := range groupSnapshot.Status.VolumeSnapshotRefList {
 		snapshot, err := ctrl.snapshotLister.VolumeSnapshots(snapshotRef.Namespace).Get(snapshotRef.Name)
 		if err != nil {
-			if apierrs.IsNotFound(err) {
+			if errors.IsNotFound(err) {
 				continue
 			}
 			return err
@@ -1161,7 +1146,7 @@ func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimesta
 	// Delete the individual snapshots part of the group snapshot
 	for _, snapshot := range groupSnapshot.Status.VolumeSnapshotRefList {
 		err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Delete(context.TODO(), snapshot.Name, metav1.DeleteOptions{})
-		if err != nil && !apierrs.IsNotFound(err) {
+		if err != nil && !errors.IsNotFound(err) {
 			msg := fmt.Sprintf("failed to delete snapshot API object %s/%s part of group snapshot %s: %v", snapshot.Namespace, snapshot.Name, utils.GroupSnapshotKey(groupSnapshot), err)
 			klog.Error(msg)
 			ctrl.eventRecorder.Event(groupSnapshot, v1.EventTypeWarning, "SnapshotDeleteError", msg)
@@ -1224,8 +1209,7 @@ func (ctrl *csiSnapshotCommonController) removeGroupSnapshotFinalizer(groupSnaps
 	// TODO: Remove PVC Finalizer
 
 	groupSnapshotClone := groupSnapshot.DeepCopy()
-	groupSnapshotClone.ObjectMeta.Finalizers = utils.RemoveString(groupSnapshotClone.ObjectMeta.Finalizers, utils.VolumeGroupSnapshotBoundFinalizer)
-	newGroupSnapshot, err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshots(groupSnapshotClone.Namespace).Update(context.TODO(), groupSnapshotClone, metav1.UpdateOptions{})
+	newGroupSnapshot, err := utils.PatchRemoveFinalizers(groupSnapshotClone, ctrl.clientset, utils.VolumeGroupSnapshotBoundFinalizer)
 	if err != nil {
 		return newControllerUpdateError(groupSnapshot.Name, err.Error())
 	}

pkg/common-controller/groupsnapshot_controller_helper_test.go

@@ -0,0 +1,100 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package common_controller
+
+import (
+	"context"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1"
+	"github.com/kubernetes-csi/external-snapshotter/client/v7/clientset/versioned/fake"
+	"github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/cache"
+	// crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1"
+	// clientset "github.com/kubernetes-csi/external-snapshotter/client/v7/clientset/versioned"
+	// groupsnapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumegroupsnapshot/v1alpha1"
+	// snapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumesnapshot/v1"
+	// "github.com/kubernetes-csi/external-snapshotter/v2/pkg/metrics"
+	// "k8s.io/client-go/kubernetes"
+	// corelisters "k8s.io/client-go/listers/core/v1"
+	// "k8s.io/client-go/tools/cache"
+	// "k8s.io/client-go/tools/record"
+	// "k8s.io/client-go/util/workqueue"
+)
+
+func Test_csiSnapshotCommonController_removeGroupSnapshotFinalizer(t *testing.T) {
+	type args struct {
+		groupSnapshot        *crdv1alpha1.VolumeGroupSnapshot
+		removeBoundFinalizer bool
+	}
+	tests := []struct {
+		name               string
+		args               args
+		wantErr            bool
+		expectedFinalizers []string
+	}{
+		{
+			name: "Test removeGroupSnapshotFinalizer",
+			args: args{
+				removeBoundFinalizer: true,
+				groupSnapshot: &crdv1alpha1.VolumeGroupSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "test-group-snapshot",
+						Finalizers: []string{utils.VolumeGroupSnapshotBoundFinalizer},
+					},
+				},
+			},
+		},
+		{
+			name: "Test removeGroupSnapshotFinalizer and not something else",
+			args: args{
+				removeBoundFinalizer: true,
+				groupSnapshot: &crdv1alpha1.VolumeGroupSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "test-group-snapshot",
+						Finalizers: []string{"somethingElse", utils.VolumeGroupSnapshotBoundFinalizer},
+					},
+				},
+			},
+			expectedFinalizers: []string{"somethingElse"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctrl := &csiSnapshotCommonController{
+				clientset:          fake.NewSimpleClientset(tt.args.groupSnapshot),
+				groupSnapshotStore: cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc),
+			}
+			if err := ctrl.removeGroupSnapshotFinalizer(tt.args.groupSnapshot, tt.args.removeBoundFinalizer); (err != nil) != tt.wantErr {
+				t.Errorf("csiSnapshotCommonController.removeGroupSnapshotFinalizer() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			vgs, err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshots(tt.args.groupSnapshot.Namespace).Get(context.TODO(), tt.args.groupSnapshot.Name, metav1.GetOptions{})
+			if err != nil {
+				t.Errorf("Error getting volume group snapshot: %v", err)
+			}
+			if tt.expectedFinalizers == nil && vgs.Finalizers != nil {
+				tt.expectedFinalizers = []string{} // if expectedFinalizers is nil, then it should be an empty slice so that cmp.Diff does not panic
+			}
+			if vgs.Finalizers != nil && cmp.Diff(vgs.Finalizers, tt.expectedFinalizers) != "" {
+				t.Errorf("Finalizers not expected: %v", cmp.Diff(vgs.Finalizers, tt.expectedFinalizers))
+			}
+
+		})
+	}
+}

pkg/common-controller/snapshot_controller.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	ref "k8s.io/client-go/tools/reference"
 	corev1helpers "k8s.io/component-helpers/scheduling/corev1"
@@ -932,8 +933,11 @@ func (ctrl *csiSnapshotCommonController) ensurePVCFinalizer(snapshot *crdv1.Volu
 	} else {
 		// If PVC is not being deleted and PVCFinalizer is not added yet, add the PVCFinalizer.
 		pvcClone := pvc.DeepCopy()
-		pvcClone.ObjectMeta.Finalizers = append(pvcClone.ObjectMeta.Finalizers, utils.PVCFinalizer)
-		_, err = ctrl.client.CoreV1().PersistentVolumeClaims(pvcClone.Namespace).Update(context.TODO(), pvcClone, metav1.UpdateOptions{})
+		patchBytes, err := utils.PatchOpsBytesToAddFinalizers(pvcClone, utils.PVCFinalizer)
+		if err != nil {
+			return newControllerUpdateError(pvcClone.Name, err.Error())
+		}
+		_, err = ctrl.client.CoreV1().PersistentVolumeClaims(pvcClone.Namespace).Patch(context.TODO(), pvcClone.Name, types.JSONPatchType, patchBytes, metav1.PatchOptions{})
 		if err != nil {
 			klog.Errorf("cannot add finalizer on claim [%s/%s] for snapshot [%s/%s]: [%v]", pvc.Namespace, pvc.Name, snapshot.Namespace, snapshot.Name, err)
 			return newControllerUpdateError(pvcClone.Name, err.Error())
@@ -950,9 +954,11 @@ func (ctrl *csiSnapshotCommonController) removePVCFinalizer(pvc *v1.PersistentVo
 	// TODO(xyang): We get PVC from informer but it may be outdated
 	// Should get it from API server directly before removing finalizer
 	pvcClone := pvc.DeepCopy()
-	pvcClone.ObjectMeta.Finalizers = utils.RemoveString(pvcClone.ObjectMeta.Finalizers, utils.PVCFinalizer)
-
-	_, err := ctrl.client.CoreV1().PersistentVolumeClaims(pvcClone.Namespace).Update(context.TODO(), pvcClone, metav1.UpdateOptions{})
+	patchBytes, err := utils.PatchOpsBytesToRemoveFinalizers(pvcClone, utils.PVCFinalizer)
+	if err != nil {
+		return newControllerUpdateError(pvcClone.Name, err.Error())
+	}
+	_, err = ctrl.client.CoreV1().PersistentVolumeClaims(pvcClone.Namespace).Patch(context.TODO(), pvcClone.Name, types.JSONPatchType, patchBytes, metav1.PatchOptions{})
 	if err != nil {
 		return newControllerUpdateError(pvcClone.Name, err.Error())
 	}
@@ -1499,44 +1505,27 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo
 	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
+	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,
-			})
-		}
+	// 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())
-		}
+	updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset)
+	if err != nil {
+		return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
 	}
 
 	_, err = ctrl.storeSnapshotUpdate(updatedSnapshot)
@@ -1570,22 +1559,29 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1
 		ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "ErrorPVCFinalizer", "Error check and remove PVC Finalizer for VolumeSnapshot")
 		return newControllerUpdateError(snapshot.Name, err.Error())
 	}
-
 	snapshotClone := snapshot.DeepCopy()
+	stringsToRemove := []string{}
 	if removeSourceFinalizer {
-		snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer)
+		stringsToRemove = append(stringsToRemove, utils.VolumeSnapshotAsSourceFinalizer)
 	}
 	if removeBoundFinalizer {
-		snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
+		stringsToRemove = append(stringsToRemove, utils.VolumeSnapshotBoundFinalizer)
 	}
 	if removeGroupFinalizer {
-		snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotInGroupFinalizer)
+		stringsToRemove = append(stringsToRemove, utils.VolumeSnapshotInGroupFinalizer)
 	}
-	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+	patchBytes, err := utils.PatchOpsBytesToRemoveFinalizers(snapshot, stringsToRemove...)
 	if err != nil {
 		return newControllerUpdateError(snapshot.Name, err.Error())
 	}
-
+	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Patch(context.TODO(), snapshotClone.Name, types.JSONPatchType, patchBytes, metav1.PatchOptions{})
+	if err != nil {
+		return newControllerUpdateError(snapshot.Name, err.Error())
+	}
+	if len(newSnapshot.Finalizers) == 0 {
+		// some tests require 0 length finalizers to be nil
+		newSnapshot.Finalizers = nil
+	}
 	_, err = ctrl.storeSnapshotUpdate(newSnapshot)
 	if err != nil {
 		klog.Errorf("failed to update snapshot store %v", err)

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/v7/apis/volumesnapshot/v1"
 	"github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils"
@@ -49,7 +50,12 @@ var class5Parameters = map[string]string{
 	utils.PrefixedSnapshotterSecretNamespaceKey: "default",
 }
 
-var timeNowMetav1 = metav1.Now()
+var timeNowMetav1 = func() metav1.Time {
+	// json.unmarshal does not restore millisecond precision
+	// so we need to round the time to seconds
+	timeNow := timeNow.Round(time.Second)
+	return metav1.NewTime(timeNow)
+}()
 
 var (
 	content31 = "content3-1"