From 3baa4593dd7405b288fadf25abb18af2eeda65a7 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 22 Feb 2024 14:52:51 -0500 Subject: [PATCH 01/10] Use patch instead of update for GroupSnapshots and VolumeSnapshots Signed-off-by: Tiger Kaovilai remove debugging code Signed-off-by: Tiger Kaovilai remove more update calls Signed-off-by: Tiger Kaovilai Fix patch json unmarshal unitTests comparison failures Signed-off-by: Tiger Kaovilai Fix tests in reactor by not modifying original for patch Signed-off-by: Tiger Kaovilai --- pkg/common-controller/framework_test.go | 8 +- .../groupsnapshot_controller_helper.go | 50 +++------ .../groupsnapshot_controller_helper_test.go | 100 ++++++++++++++++++ pkg/common-controller/snapshot_controller.go | 90 ++++++++-------- pkg/common-controller/snapshot_delete_test.go | 8 +- pkg/utils/patch.go | 96 +++++++++++++++++ pkg/utils/patch_test.go | 83 +++++++++++++++ 7 files changed, 352 insertions(+), 83 deletions(-) create mode 100644 pkg/common-controller/groupsnapshot_controller_helper_test.go create mode 100644 pkg/utils/patch_test.go diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index a79695846..713d78399 100644 --- a/pkg/common-controller/framework_test.go +++ b/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{} diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go index 3768f1547..7d838c97c 100644 --- a/pkg/common-controller/groupsnapshot_controller_helper.go +++ b/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()) } diff --git a/pkg/common-controller/groupsnapshot_controller_helper_test.go b/pkg/common-controller/groupsnapshot_controller_helper_test.go new file mode 100644 index 000000000..0274304eb --- /dev/null +++ b/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)) + } + + }) + } +} diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index ae76461c6..196c47dd0 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/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) diff --git a/pkg/common-controller/snapshot_delete_test.go b/pkg/common-controller/snapshot_delete_test.go index 299bf4f07..950d052fa 100644 --- a/pkg/common-controller/snapshot_delete_test.go +++ b/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" diff --git a/pkg/utils/patch.go b/pkg/utils/patch.go index 05fd32f6b..6247c03fe 100644 --- a/pkg/utils/patch.go +++ b/pkg/utils/patch.go @@ -3,12 +3,17 @@ package utils import ( "context" "encoding/json" + "errors" + "fmt" + "slices" + crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1" crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" clientset "github.com/kubernetes-csi/external-snapshotter/client/v7/clientset/versioned" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + kubernetes "k8s.io/client-go/kubernetes" ) // PatchOp represents a json patch operation @@ -97,3 +102,94 @@ func PatchVolumeGroupSnapshotContent( return newGroupSnapshotContent, nil } + +// Remove one or more finalizers from an object +// if finalizers is not empty, only the specified finalizers will be removed +func PatchRemoveFinalizers(object metav1.Object, client clientset.Interface, finalizers ...string) (metav1.Object, error) { + data, err := PatchOpsBytesToRemoveFinalizers(object, finalizers...) + if err != nil { + return nil, err + } + switch object.(type) { + case *crdv1.VolumeSnapshot: + obj, err := client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Patch(context.TODO(), object.GetName(), types.JSONPatchType, data, metav1.PatchOptions{}) + if len(obj.Finalizers) == 0 { + // to satisfy some tests that requires nil rather than []string{} + obj.Finalizers = nil + } + return obj, err + case *crdv1alpha1.VolumeGroupSnapshot: + obj, err := client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Patch(context.TODO(), object.GetName(), types.JSONPatchType, data, metav1.PatchOptions{}) + if len(obj.Finalizers) == 0 { + // to satisfy some tests that requires nil rather than []string{} + obj.Finalizers = nil + } + return obj, err + default: + return nil, errors.New("PatchRemoveFinalizers: unsupported object type") + } +} + +func PatchRemoveFinalizersCorev1(object metav1.Object, client kubernetes.Interface, finalizers ...string) (metav1.Object, error) { + data, err := PatchOpsBytesToRemoveFinalizers(object, finalizers...) + if err != nil { + return nil, err + } + obj, err := client.CoreV1().Pods(object.GetNamespace()).Patch(context.TODO(), object.GetName(), types.JSONPatchType, data, metav1.PatchOptions{}) + if len(obj.Finalizers) == 0 { + // to satisfy some tests that requires nil rather than []string{} + obj.Finalizers = nil + } + return obj, err +} + +func PatchOpsToRemoveFinalizers(object metav1.Object, finalizers ...string) []PatchOp { + patches := []PatchOp{} + if len(finalizers) == 0 { + return patches + } + // remove only the specified finalizers + // get index of all finalizers to be removed + // sort the indexes in descending order + // remove the finalizers in descending order + // this is to avoid the need to adjust the index after each removal + indexes := []int{} + for _, finalizer := range finalizers { + for i, f := range object.GetFinalizers() { + if f == finalizer { + indexes = append(indexes, i) + } + } + } + slices.Sort(indexes) + slices.Reverse(indexes) + for _, i := range indexes { + patches = append(patches, PatchOp{ + Op: "remove", + Path: "/metadata/finalizers/" + fmt.Sprint(i), + }) + } + return patches +} + +func PatchOpsBytesToRemoveFinalizers(object metav1.Object, finalizers ...string) ([]byte, error) { + patches := PatchOpsToRemoveFinalizers(object, finalizers...) + return json.Marshal(patches) +} + +func PatchOpsToAddFinalizers(object metav1.Object, finalizers ...string) []PatchOp { + patches := []PatchOp{} + for _, finalizer := range finalizers { + patches = append(patches, PatchOp{ + Op: "add", + Path: "/metadata/finalizers/-", + Value: finalizer, + }) + } + return patches +} + +func PatchOpsBytesToAddFinalizers(object metav1.Object, finalizers ...string) ([]byte, error) { + patches := PatchOpsToAddFinalizers(object, finalizers...) + return json.Marshal(patches) +} diff --git a/pkg/utils/patch_test.go b/pkg/utils/patch_test.go new file mode 100644 index 000000000..e0c0c889c --- /dev/null +++ b/pkg/utils/patch_test.go @@ -0,0 +1,83 @@ +package utils + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" +) + +func TestPatchOpsBytesToRemoveFinalizers(t *testing.T) { + type args struct { + object *corev1.PersistentVolumeClaim + finalizersToRemove []string + } + tests := []struct { + name string + args args + finalizersAfterPatch []string + wantErr bool + }{ + { + name: "remove all finalizers", + args: args{ + object: &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc1", + Namespace: "default", + Finalizers: []string{"finalizer1", "finalizer2"}, + }, + }, + finalizersToRemove: []string{"finalizer1", "finalizer2"}, + }, + finalizersAfterPatch: []string{}, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := fake.NewSimpleClientset(tt.args.object) + gotObj, err := client.CoreV1().PersistentVolumeClaims(tt.args.object.GetNamespace()).Get(context.Background(), tt.args.object.GetName(), metav1.GetOptions{}) + if err != nil { + t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) + return + } + for i, finalizer := range gotObj.GetFinalizers() { + if finalizer != tt.args.object.Finalizers[i] { + t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) + return + } + } + got, err := PatchOpsBytesToRemoveFinalizers(tt.args.object, tt.args.finalizersToRemove...) + if (err != nil) != tt.wantErr { + t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) + return + } + // try to apply the patch + _, err = client.CoreV1().PersistentVolumeClaims(tt.args.object.GetNamespace()).Patch(context.Background(), tt.args.object.GetName(), types.JSONPatchType, got, metav1.PatchOptions{}) + if err != nil { + t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) + return + } + // check if the finalizers were removed + gotObj, err = client.CoreV1().PersistentVolumeClaims(tt.args.object.GetNamespace()).Get(context.Background(), tt.args.object.GetName(), metav1.GetOptions{}) + if err != nil { + t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) + return + } + if len(gotObj.GetFinalizers()) != len(tt.finalizersAfterPatch) { + t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) + return + } + for i, finalizer := range gotObj.GetFinalizers() { + if finalizer != tt.finalizersAfterPatch[i] { + t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) + return + } + } + }) + } +} From 2e56da847422c99bc5c5dc88a65e80cd74436fc5 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 26 Feb 2024 13:19:32 -0500 Subject: [PATCH 02/10] Use remove all + add instead of removing by index Signed-off-by: Tiger Kaovilai --- pkg/common-controller/framework_test.go | 43 ++++++++++++++- .../groupsnapshot_controller_helper.go | 22 +------- pkg/common-controller/snapshot_controller.go | 33 ++++++------ pkg/utils/patch.go | 54 +++++++++++-------- 4 files changed, 94 insertions(+), 58 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 713d78399..54d000d17 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -462,6 +462,44 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O klog.V(4).Infof("saved updated claim %s", claim.Name) return true, claim, nil + case action.Matches("patch", "persistentvolumeclaims"): + claim := &v1.PersistentVolumeClaim{} + action := action.(core.PatchAction) + + // Check and bump object version + storedClaim, found := r.claims[action.GetName()] + if found { + // don't modify existing object + storedClaim = storedClaim.DeepCopy() + // Apply patch + storedClaimBytes, err := json.Marshal(storedClaim) + if err != nil { + return true, nil, err + } + claimPatch, err := jsonpatch.DecodePatch(action.GetPatch()) + if err != nil { + return true, nil, err + } + modified, err := claimPatch.Apply(storedClaimBytes) + if err != nil { + return true, nil, err + } + err = json.Unmarshal(modified, claim) + if err != nil { + return true, nil, err + } + storedVer, _ := strconv.Atoi(claim.ResourceVersion) + claim.ResourceVersion = strconv.Itoa(storedVer + 1) + } else { + return true, nil, fmt.Errorf("cannot update claim %s: claim not found", action.GetName()) + } + // Store the updated object to appropriate places. + r.claims[claim.Name] = claim + r.changedObjects = append(r.changedObjects, claim) + r.changedSinceLastSync++ + klog.V(4).Infof("saved updated claim %s", claim.Name) + return + case action.Matches("get", "secrets"): name := action.(core.GetAction).GetName() secret, found := r.secrets[name] @@ -816,6 +854,7 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset, client.AddReactor("delete", "volumesnapshotclasses", reactor.React) kubeClient.AddReactor("get", "persistentvolumeclaims", reactor.React) kubeClient.AddReactor("update", "persistentvolumeclaims", reactor.React) + kubeClient.AddReactor("patch", "persistentvolumeclaims", reactor.React) kubeClient.AddReactor("get", "persistentvolumes", reactor.React) kubeClient.AddReactor("get", "secrets", reactor.React) @@ -1260,14 +1299,16 @@ func testSyncContentError(ctrl *csiSnapshotCommonController, reactor *snapshotRe } func testAddPVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { + thisTestVolumeSnapshot = test.initialSnapshots[0] return ctrl.ensurePVCFinalizer(test.initialSnapshots[0]) } func testRemovePVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { return ctrl.checkandRemovePVCFinalizer(test.initialSnapshots[0], false) } - +var thisTestVolumeSnapshot *crdv1.VolumeSnapshot func testAddSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { + thisTestVolumeSnapshot = test.initialSnapshots[0] return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], true, true) } diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go index 7d838c97c..2b8bf9ab0 100644 --- a/pkg/common-controller/groupsnapshot_controller_helper.go +++ b/pkg/common-controller/groupsnapshot_controller_helper.go @@ -953,21 +953,7 @@ func (ctrl *csiSnapshotCommonController) needsUpdateGroupSnapshotStatus(groupSna // addGroupSnapshotContentFinalizer adds a Finalizer for VolumeGroupSnapshotContent. func (ctrl *csiSnapshotCommonController) addGroupSnapshotContentFinalizer(groupSnapshotContent *crdv1alpha1.VolumeGroupSnapshotContent) error { var patches []utils.PatchOp - if len(groupSnapshotContent.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.VolumeGroupSnapshotContentFinalizer, - }) - } 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.VolumeGroupSnapshotContentFinalizer}, - }) - } + patches = append(patches, utils.PatchOpsToAddFinalizers(groupSnapshotContent, utils.VolumeGroupSnapshotContentFinalizer)...) newGroupSnapshotContent, err := utils.PatchVolumeGroupSnapshotContent(groupSnapshotContent, patches, ctrl.clientset) if err != nil { return newControllerUpdateError(groupSnapshotContent.Name, err.Error()) @@ -1020,11 +1006,7 @@ func (ctrl *csiSnapshotCommonController) addGroupSnapshotFinalizer(groupSnapshot var patches []utils.PatchOp if addBoundFinalizer { - patches = append(patches, utils.PatchOp{ - Op: "add", - Path: "/metadata/finalizers/-", - Value: utils.VolumeGroupSnapshotBoundFinalizer, - }) + patches = append(patches, utils.PatchOpsToAddFinalizers(groupSnapshot, utils.VolumeGroupSnapshotBoundFinalizer)...) } updatedGroupSnapshot, err = utils.PatchVolumeGroupSnapshot(groupSnapshot, patches, ctrl.clientset) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 196c47dd0..77d22bc2f 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -937,12 +937,13 @@ func (ctrl *csiSnapshotCommonController) ensurePVCFinalizer(snapshot *crdv1.Volu 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{}) + pvcCloneAfter, 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()) } klog.Infof("Added protection finalizer to persistent volume claim %s/%s", pvc.Namespace, pvc.Name) + fmt.Printf("pvcCloneAfter: %+v", pvcCloneAfter) } return nil @@ -1505,24 +1506,26 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo var updatedSnapshot *crdv1.VolumeSnapshot var err error - var patches []utils.PatchOp - + // var patches []utils.PatchOp + finalizersToAdd := []string{} // 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, - }) + // patches = append(patches, utils.PatchOp{ + // Op: "add", + // Path: "/metadata/finalizers/-", + // Value: utils.VolumeSnapshotAsSourceFinalizer, + // }) + finalizersToAdd = append(finalizersToAdd, utils.VolumeSnapshotAsSourceFinalizer) } if addBoundFinalizer { - patches = append(patches, utils.PatchOp{ - Op: "add", - Path: "/metadata/finalizers/-", - Value: utils.VolumeSnapshotBoundFinalizer, - }) - } - + // patches = append(patches, utils.PatchOp{ + // Op: "add", + // Path: "/metadata/finalizers/-", + // Value: utils.VolumeSnapshotBoundFinalizer, + // }) + finalizersToAdd = append(finalizersToAdd, utils.VolumeSnapshotBoundFinalizer) + } + patches := utils.PatchOpsToAddFinalizers(snapshot, finalizersToAdd...) updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset) if err != nil { return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) diff --git a/pkg/utils/patch.go b/pkg/utils/patch.go index 6247c03fe..77e07744e 100644 --- a/pkg/utils/patch.go +++ b/pkg/utils/patch.go @@ -4,8 +4,6 @@ import ( "context" "encoding/json" "errors" - "fmt" - "slices" crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1" @@ -113,14 +111,14 @@ func PatchRemoveFinalizers(object metav1.Object, client clientset.Interface, fin switch object.(type) { case *crdv1.VolumeSnapshot: obj, err := client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Patch(context.TODO(), object.GetName(), types.JSONPatchType, data, metav1.PatchOptions{}) - if len(obj.Finalizers) == 0 { + if obj != nil && len(obj.Finalizers) == 0 { // to satisfy some tests that requires nil rather than []string{} obj.Finalizers = nil } return obj, err case *crdv1alpha1.VolumeGroupSnapshot: obj, err := client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Patch(context.TODO(), object.GetName(), types.JSONPatchType, data, metav1.PatchOptions{}) - if len(obj.Finalizers) == 0 { + if obj != nil && len(obj.Finalizers) == 0 { // to satisfy some tests that requires nil rather than []string{} obj.Finalizers = nil } @@ -148,27 +146,28 @@ func PatchOpsToRemoveFinalizers(object metav1.Object, finalizers ...string) []Pa if len(finalizers) == 0 { return patches } - // remove only the specified finalizers - // get index of all finalizers to be removed - // sort the indexes in descending order - // remove the finalizers in descending order - // this is to avoid the need to adjust the index after each removal - indexes := []int{} + // map of finalizers to remove + finalizersToRemove := make(map[string]bool, len(finalizers)) for _, finalizer := range finalizers { - for i, f := range object.GetFinalizers() { - if f == finalizer { - indexes = append(indexes, i) - } + finalizersToRemove[finalizer] = true + } + + patches = append(patches, PatchOp{ + Op: "remove", + Path: "/metadata/finalizers", + }) + annotationsToKeep := []string{} + for _, objFinalizer := range object.GetFinalizers() { + // finalizers to keep + if _, ok := finalizersToRemove[objFinalizer]; !ok { + annotationsToKeep = append(annotationsToKeep, objFinalizer) } } - slices.Sort(indexes) - slices.Reverse(indexes) - for _, i := range indexes { - patches = append(patches, PatchOp{ - Op: "remove", - Path: "/metadata/finalizers/" + fmt.Sprint(i), - }) - } + patches = append(patches, PatchOp{ + Op: "add", + Path: "/metadata/finalizers", + Value: annotationsToKeep, + }) return patches } @@ -179,6 +178,17 @@ func PatchOpsBytesToRemoveFinalizers(object metav1.Object, finalizers ...string) func PatchOpsToAddFinalizers(object metav1.Object, finalizers ...string) []PatchOp { patches := []PatchOp{} + if len(finalizers) == 0 { + return patches + } + if object.GetFinalizers() == nil || len(object.GetFinalizers()) == 0{ + patches = append(patches, PatchOp{ + Op: "add", + Path: "/metadata/finalizers", + Value: finalizers, + }) + return patches + } for _, finalizer := range finalizers { patches = append(patches, PatchOp{ Op: "add", From 6740b077d5eaf130de724505af3155dd16382d2f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 26 Feb 2024 14:05:00 -0500 Subject: [PATCH 03/10] fix sync tests Signed-off-by: Tiger Kaovilai --- pkg/common-controller/snapshot_update_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go index 75ce6be3f..d9d69ccea 100644 --- a/pkg/common-controller/snapshot_update_test.go +++ b/pkg/common-controller/snapshot_update_test.go @@ -117,9 +117,10 @@ func TestSync(t *testing.T) { initialVolumes: newVolumeArray("volume2-8", "pv-uid2-8", "pv-handle2-8", "1Gi", "pvc-uid2-8", "claim2-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), initialSecrets: []*v1.Secret{secret()}, errors: []reactorError{ - // Inject error to the first client.VolumesnapshotV1().VolumeSnapshots().Update call. + // Inject error to the first client.VolumesnapshotV1().VolumeSnapshots().Update and .Patch call. // All other calls will succeed. {"update", "volumesnapshots", errors.New("mock update error")}, + {"patch", "volumesnapshots", errors.New("mock update error")}, }, test: testSyncSnapshotError, }, From 84e574f43ff7c0bc799acadf8902d71989e78a1f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 26 Feb 2024 14:05:20 -0500 Subject: [PATCH 04/10] clean up debugging content Signed-off-by: Tiger Kaovilai --- pkg/common-controller/framework_test.go | 3 --- pkg/common-controller/snapshot_controller.go | 11 ----------- 2 files changed, 14 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 54d000d17..790556fb7 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -1299,16 +1299,13 @@ func testSyncContentError(ctrl *csiSnapshotCommonController, reactor *snapshotRe } func testAddPVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { - thisTestVolumeSnapshot = test.initialSnapshots[0] return ctrl.ensurePVCFinalizer(test.initialSnapshots[0]) } func testRemovePVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { return ctrl.checkandRemovePVCFinalizer(test.initialSnapshots[0], false) } -var thisTestVolumeSnapshot *crdv1.VolumeSnapshot func testAddSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { - thisTestVolumeSnapshot = test.initialSnapshots[0] return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], true, true) } diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 77d22bc2f..3f5cae945 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -1506,23 +1506,12 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo var updatedSnapshot *crdv1.VolumeSnapshot var err error - // var patches []utils.PatchOp finalizersToAdd := []string{} // 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, - // }) finalizersToAdd = append(finalizersToAdd, utils.VolumeSnapshotAsSourceFinalizer) } if addBoundFinalizer { - // patches = append(patches, utils.PatchOp{ - // Op: "add", - // Path: "/metadata/finalizers/-", - // Value: utils.VolumeSnapshotBoundFinalizer, - // }) finalizersToAdd = append(finalizersToAdd, utils.VolumeSnapshotBoundFinalizer) } patches := utils.PatchOpsToAddFinalizers(snapshot, finalizersToAdd...) From db6559f045da14cd2140e1d92edd42859265dc6f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 26 Feb 2024 14:25:46 -0500 Subject: [PATCH 05/10] go fmt Signed-off-by: Tiger Kaovilai --- pkg/common-controller/framework_test.go | 2 +- pkg/utils/patch.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 790556fb7..803860e0e 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -498,7 +498,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O r.changedObjects = append(r.changedObjects, claim) r.changedSinceLastSync++ klog.V(4).Infof("saved updated claim %s", claim.Name) - return + return case action.Matches("get", "secrets"): name := action.(core.GetAction).GetName() diff --git a/pkg/utils/patch.go b/pkg/utils/patch.go index 77e07744e..cf7b0fa51 100644 --- a/pkg/utils/patch.go +++ b/pkg/utils/patch.go @@ -181,7 +181,7 @@ func PatchOpsToAddFinalizers(object metav1.Object, finalizers ...string) []Patch if len(finalizers) == 0 { return patches } - if object.GetFinalizers() == nil || len(object.GetFinalizers()) == 0{ + if object.GetFinalizers() == nil || len(object.GetFinalizers()) == 0 { patches = append(patches, PatchOp{ Op: "add", Path: "/metadata/finalizers", From 931529bcf4a6b3a095f941cba56a820300d1d869 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 26 Feb 2024 16:27:05 -0500 Subject: [PATCH 06/10] Use update to remove finalizers instead of patch with get retries Signed-off-by: Tiger Kaovilai --- go.mod | 2 +- .../groupsnapshot_controller_helper.go | 2 +- pkg/common-controller/snapshot_controller.go | 17 ++-- pkg/utils/patch.go | 77 ----------------- pkg/utils/patch_test.go | 83 ------------------- pkg/utils/update.go | 72 ++++++++++++++++ pkg/utils/util.go | 8 ++ 7 files changed, 87 insertions(+), 174 deletions(-) delete mode 100644 pkg/utils/patch_test.go create mode 100644 pkg/utils/update.go diff --git a/go.mod b/go.mod index fb8e5546a..c4bbc6043 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/evanphx/json-patch v5.9.0+incompatible github.com/fsnotify/fsnotify v1.7.0 github.com/golang/mock v1.6.0 + github.com/google/go-cmp v0.6.0 github.com/google/gofuzz v1.2.0 github.com/kubernetes-csi/csi-lib-utils v0.17.0 github.com/kubernetes-csi/csi-test/v5 v5.2.0 @@ -40,7 +41,6 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/google/gnostic-models v0.6.8 // indirect - github.com/google/go-cmp v0.6.0 // indirect github.com/google/uuid v1.4.0 // indirect github.com/imdario/mergo v0.3.13 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go index 2b8bf9ab0..1e0ad82d7 100644 --- a/pkg/common-controller/groupsnapshot_controller_helper.go +++ b/pkg/common-controller/groupsnapshot_controller_helper.go @@ -1191,7 +1191,7 @@ func (ctrl *csiSnapshotCommonController) removeGroupSnapshotFinalizer(groupSnaps // TODO: Remove PVC Finalizer groupSnapshotClone := groupSnapshot.DeepCopy() - newGroupSnapshot, err := utils.PatchRemoveFinalizers(groupSnapshotClone, ctrl.clientset, utils.VolumeGroupSnapshotBoundFinalizer) + newGroupSnapshot, err := utils.UpdateRemoveFinalizers(groupSnapshotClone, ctrl.clientset, utils.VolumeGroupSnapshotBoundFinalizer) if err != nil { return newControllerUpdateError(groupSnapshot.Name, err.Error()) } diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 3f5cae945..6ecd58424 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -955,11 +955,7 @@ 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() - 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{}) + _, err := utils.UpdateRemoveFinalizersCoreV1(pvcClone, ctrl.client, utils.PVCFinalizer) if err != nil { return newControllerUpdateError(pvcClone.Name, err.Error()) } @@ -1562,17 +1558,14 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1 if removeGroupFinalizer { stringsToRemove = append(stringsToRemove, utils.VolumeSnapshotInGroupFinalizer) } - 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{}) + + newSnapshot, err := utils.UpdateRemoveFinalizers(snapshotClone, ctrl.clientset, stringsToRemove...) if err != nil { return newControllerUpdateError(snapshot.Name, err.Error()) } - if len(newSnapshot.Finalizers) == 0 { + if len(newSnapshot.GetFinalizers()) == 0 { // some tests require 0 length finalizers to be nil - newSnapshot.Finalizers = nil + newSnapshot.SetFinalizers(nil) } _, err = ctrl.storeSnapshotUpdate(newSnapshot) if err != nil { diff --git a/pkg/utils/patch.go b/pkg/utils/patch.go index cf7b0fa51..d8ec25230 100644 --- a/pkg/utils/patch.go +++ b/pkg/utils/patch.go @@ -3,7 +3,6 @@ package utils import ( "context" "encoding/json" - "errors" crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1" @@ -11,7 +10,6 @@ import ( clientset "github.com/kubernetes-csi/external-snapshotter/client/v7/clientset/versioned" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - kubernetes "k8s.io/client-go/kubernetes" ) // PatchOp represents a json patch operation @@ -101,81 +99,6 @@ func PatchVolumeGroupSnapshotContent( return newGroupSnapshotContent, nil } -// Remove one or more finalizers from an object -// if finalizers is not empty, only the specified finalizers will be removed -func PatchRemoveFinalizers(object metav1.Object, client clientset.Interface, finalizers ...string) (metav1.Object, error) { - data, err := PatchOpsBytesToRemoveFinalizers(object, finalizers...) - if err != nil { - return nil, err - } - switch object.(type) { - case *crdv1.VolumeSnapshot: - obj, err := client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Patch(context.TODO(), object.GetName(), types.JSONPatchType, data, metav1.PatchOptions{}) - if obj != nil && len(obj.Finalizers) == 0 { - // to satisfy some tests that requires nil rather than []string{} - obj.Finalizers = nil - } - return obj, err - case *crdv1alpha1.VolumeGroupSnapshot: - obj, err := client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Patch(context.TODO(), object.GetName(), types.JSONPatchType, data, metav1.PatchOptions{}) - if obj != nil && len(obj.Finalizers) == 0 { - // to satisfy some tests that requires nil rather than []string{} - obj.Finalizers = nil - } - return obj, err - default: - return nil, errors.New("PatchRemoveFinalizers: unsupported object type") - } -} - -func PatchRemoveFinalizersCorev1(object metav1.Object, client kubernetes.Interface, finalizers ...string) (metav1.Object, error) { - data, err := PatchOpsBytesToRemoveFinalizers(object, finalizers...) - if err != nil { - return nil, err - } - obj, err := client.CoreV1().Pods(object.GetNamespace()).Patch(context.TODO(), object.GetName(), types.JSONPatchType, data, metav1.PatchOptions{}) - if len(obj.Finalizers) == 0 { - // to satisfy some tests that requires nil rather than []string{} - obj.Finalizers = nil - } - return obj, err -} - -func PatchOpsToRemoveFinalizers(object metav1.Object, finalizers ...string) []PatchOp { - patches := []PatchOp{} - if len(finalizers) == 0 { - return patches - } - // map of finalizers to remove - finalizersToRemove := make(map[string]bool, len(finalizers)) - for _, finalizer := range finalizers { - finalizersToRemove[finalizer] = true - } - - patches = append(patches, PatchOp{ - Op: "remove", - Path: "/metadata/finalizers", - }) - annotationsToKeep := []string{} - for _, objFinalizer := range object.GetFinalizers() { - // finalizers to keep - if _, ok := finalizersToRemove[objFinalizer]; !ok { - annotationsToKeep = append(annotationsToKeep, objFinalizer) - } - } - patches = append(patches, PatchOp{ - Op: "add", - Path: "/metadata/finalizers", - Value: annotationsToKeep, - }) - return patches -} - -func PatchOpsBytesToRemoveFinalizers(object metav1.Object, finalizers ...string) ([]byte, error) { - patches := PatchOpsToRemoveFinalizers(object, finalizers...) - return json.Marshal(patches) -} - func PatchOpsToAddFinalizers(object metav1.Object, finalizers ...string) []PatchOp { patches := []PatchOp{} if len(finalizers) == 0 { diff --git a/pkg/utils/patch_test.go b/pkg/utils/patch_test.go deleted file mode 100644 index e0c0c889c..000000000 --- a/pkg/utils/patch_test.go +++ /dev/null @@ -1,83 +0,0 @@ -package utils - -import ( - "context" - "testing" - - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes/fake" -) - -func TestPatchOpsBytesToRemoveFinalizers(t *testing.T) { - type args struct { - object *corev1.PersistentVolumeClaim - finalizersToRemove []string - } - tests := []struct { - name string - args args - finalizersAfterPatch []string - wantErr bool - }{ - { - name: "remove all finalizers", - args: args{ - object: &corev1.PersistentVolumeClaim{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pvc1", - Namespace: "default", - Finalizers: []string{"finalizer1", "finalizer2"}, - }, - }, - finalizersToRemove: []string{"finalizer1", "finalizer2"}, - }, - finalizersAfterPatch: []string{}, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - client := fake.NewSimpleClientset(tt.args.object) - gotObj, err := client.CoreV1().PersistentVolumeClaims(tt.args.object.GetNamespace()).Get(context.Background(), tt.args.object.GetName(), metav1.GetOptions{}) - if err != nil { - t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) - return - } - for i, finalizer := range gotObj.GetFinalizers() { - if finalizer != tt.args.object.Finalizers[i] { - t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) - return - } - } - got, err := PatchOpsBytesToRemoveFinalizers(tt.args.object, tt.args.finalizersToRemove...) - if (err != nil) != tt.wantErr { - t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) - return - } - // try to apply the patch - _, err = client.CoreV1().PersistentVolumeClaims(tt.args.object.GetNamespace()).Patch(context.Background(), tt.args.object.GetName(), types.JSONPatchType, got, metav1.PatchOptions{}) - if err != nil { - t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) - return - } - // check if the finalizers were removed - gotObj, err = client.CoreV1().PersistentVolumeClaims(tt.args.object.GetNamespace()).Get(context.Background(), tt.args.object.GetName(), metav1.GetOptions{}) - if err != nil { - t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) - return - } - if len(gotObj.GetFinalizers()) != len(tt.finalizersAfterPatch) { - t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) - return - } - for i, finalizer := range gotObj.GetFinalizers() { - if finalizer != tt.finalizersAfterPatch[i] { - t.Errorf("PatchOpsBytesToRemoveFinalizers() error = %v, wantErr %v", err, tt.wantErr) - return - } - } - }) - } -} diff --git a/pkg/utils/update.go b/pkg/utils/update.go new file mode 100644 index 000000000..aa6437254 --- /dev/null +++ b/pkg/utils/update.go @@ -0,0 +1,72 @@ +package utils + +import ( + "context" + "errors" + + crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1" + crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" + clientset "github.com/kubernetes-csi/external-snapshotter/client/v7/clientset/versioned" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" +) + +// Remove one or more finalizers from an object +// if finalizers is not empty, only the specified finalizers will be removed +// If update fails due to out of date, it will call get on the object and retry removing the finalizers +func UpdateRemoveFinalizers(object metav1.Object, client clientset.Interface, finalizers ...string) (metav1.Object, error) { + object.SetFinalizers(RemoveStrings(object.GetFinalizers(), finalizers...)) + switch object.(type) { + case *crdv1.VolumeSnapshot: + obj, err := client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Update(context.TODO(), object.(*crdv1.VolumeSnapshot), metav1.UpdateOptions{}) + if err != nil && apierrors.IsConflict(err) { + obj, err = client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Get(context.TODO(), object.GetName(), metav1.GetOptions{}) + if err != nil { + return nil, err + } + return UpdateRemoveFinalizers(obj, client, finalizers...) + } + if obj != nil && len(obj.GetFinalizers()) == 0 { + // to satisfy some tests that requires nil rather than []string{} + obj.SetFinalizers(nil) + } + return obj, err + case *crdv1alpha1.VolumeGroupSnapshot: + obj, err := client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Update(context.TODO(), object.(*crdv1alpha1.VolumeGroupSnapshot), metav1.UpdateOptions{}) + if err != nil && apierrors.IsConflict(err) { + obj, err = client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Get(context.TODO(), object.GetName(), metav1.GetOptions{}) + if err != nil { + return nil, err + } + return UpdateRemoveFinalizers(obj, client, finalizers...) + } + if obj != nil && len(obj.GetFinalizers()) == 0 { + // to satisfy some tests that requires nil rather than []string{} + obj.SetFinalizers(nil) + } + return obj, err + default: + return nil, errors.New("UpdateRemoveFinalizers: unsupported object type") + } +} + +func UpdateRemoveFinalizersCoreV1(object metav1.Object, client kubernetes.Interface, finalizers ...string) (metav1.Object, error) { + object.SetFinalizers(RemoveStrings(object.GetFinalizers(), finalizers...)) + pvc := object.(*corev1.PersistentVolumeClaim) + pvc, err := client.CoreV1().PersistentVolumeClaims(object.GetNamespace()).Update(context.TODO(), pvc, metav1.UpdateOptions{}) + // if out of date, get the object and retry + if err != nil && apierrors.IsConflict(err) { + pvc, err = client.CoreV1().PersistentVolumeClaims(object.GetNamespace()).Get(context.TODO(), pvc.GetName(), metav1.GetOptions{}) + if err != nil { + return nil, err + } + return UpdateRemoveFinalizersCoreV1(pvc, client, finalizers...) + } + if pvc != nil && len(pvc.GetFinalizers()) == 0 { + // to satisfy some tests that requires nil rather than []string{} + pvc.SetFinalizers(nil) + } + return pvc, err +} diff --git a/pkg/utils/util.go b/pkg/utils/util.go index 26f4ba77d..f18489c55 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "os" + "slices" "strconv" "strings" "time" @@ -190,6 +191,13 @@ func RemoveString(slice []string, s string) []string { return newSlice } +func RemoveStrings(slice []string, removes ...string) []string { + newSlice := slices.DeleteFunc(slice, func (remove string) bool { + return slices.Contains(removes, remove) + }) + return newSlice +} + func SnapshotKey(vs *crdv1.VolumeSnapshot) string { return fmt.Sprintf("%s/%s", vs.Namespace, vs.Name) } From 9d6b1383b51b24822aea317f366329d9a11e1329 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 26 Feb 2024 17:02:25 -0500 Subject: [PATCH 07/10] Use generics for Update function Signed-off-by: Tiger Kaovilai --- .../groupsnapshot_controller_helper.go | 2 +- pkg/common-controller/snapshot_controller.go | 2 +- pkg/utils/update.go | 86 ++++++++++--------- 3 files changed, 46 insertions(+), 44 deletions(-) diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go index 1e0ad82d7..49beb8e3d 100644 --- a/pkg/common-controller/groupsnapshot_controller_helper.go +++ b/pkg/common-controller/groupsnapshot_controller_helper.go @@ -1191,7 +1191,7 @@ func (ctrl *csiSnapshotCommonController) removeGroupSnapshotFinalizer(groupSnaps // TODO: Remove PVC Finalizer groupSnapshotClone := groupSnapshot.DeepCopy() - newGroupSnapshot, err := utils.UpdateRemoveFinalizers(groupSnapshotClone, ctrl.clientset, utils.VolumeGroupSnapshotBoundFinalizer) + newGroupSnapshot, err := utils.UpdateRemoveFinalizersSnapshots(groupSnapshotClone, ctrl.clientset, utils.VolumeGroupSnapshotBoundFinalizer) if err != nil { return newControllerUpdateError(groupSnapshot.Name, err.Error()) } diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 6ecd58424..3bfa6c625 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -1559,7 +1559,7 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1 stringsToRemove = append(stringsToRemove, utils.VolumeSnapshotInGroupFinalizer) } - newSnapshot, err := utils.UpdateRemoveFinalizers(snapshotClone, ctrl.clientset, stringsToRemove...) + newSnapshot, err := utils.UpdateRemoveFinalizersSnapshots(snapshotClone, ctrl.clientset, stringsToRemove...) if err != nil { return newControllerUpdateError(snapshot.Name, err.Error()) } diff --git a/pkg/utils/update.go b/pkg/utils/update.go index aa6437254..faa2492d5 100644 --- a/pkg/utils/update.go +++ b/pkg/utils/update.go @@ -16,57 +16,59 @@ import ( // Remove one or more finalizers from an object // if finalizers is not empty, only the specified finalizers will be removed // If update fails due to out of date, it will call get on the object and retry removing the finalizers -func UpdateRemoveFinalizers(object metav1.Object, client clientset.Interface, finalizers ...string) (metav1.Object, error) { +func UpdateRemoveFinalizers[O metav1.Object]( + object O, + updateFunc func(context.Context, O, metav1.UpdateOptions) (O, error), + getFunc func(context.Context, string, metav1.GetOptions) (O, error), + finalizers ...string) (O, error) { object.SetFinalizers(RemoveStrings(object.GetFinalizers(), finalizers...)) - switch object.(type) { - case *crdv1.VolumeSnapshot: - obj, err := client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Update(context.TODO(), object.(*crdv1.VolumeSnapshot), metav1.UpdateOptions{}) - if err != nil && apierrors.IsConflict(err) { - obj, err = client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Get(context.TODO(), object.GetName(), metav1.GetOptions{}) + object, err := updateFunc(context.TODO(), object, metav1.UpdateOptions{}) + if err != nil { + if apierrors.IsConflict(err) { + object, err = getFunc(context.TODO(), object.GetName(), metav1.GetOptions{}) if err != nil { - return nil, err + return object, err } - return UpdateRemoveFinalizers(obj, client, finalizers...) - } - if obj != nil && len(obj.GetFinalizers()) == 0 { - // to satisfy some tests that requires nil rather than []string{} - obj.SetFinalizers(nil) + return UpdateRemoveFinalizers(object, updateFunc, getFunc, finalizers...) + } else { + return object, err } - return obj, err + } + if len(object.GetFinalizers()) == 0 { + // to satisfy some tests that requires nil rather than []string{} + object.SetFinalizers(nil) + } + return object, err +} + +func UpdateRemoveFinalizersSnapshots(object metav1.Object, client clientset.Interface, finalizers ...string) (metav1.Object, error) { + switch object.(type) { + case *crdv1.VolumeSnapshot: + return UpdateRemoveFinalizers[*crdv1.VolumeSnapshot]( + object.(*crdv1.VolumeSnapshot), + client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Update, + client.SnapshotV1().VolumeSnapshots(object.GetNamespace()).Get, + finalizers...) case *crdv1alpha1.VolumeGroupSnapshot: - obj, err := client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Update(context.TODO(), object.(*crdv1alpha1.VolumeGroupSnapshot), metav1.UpdateOptions{}) - if err != nil && apierrors.IsConflict(err) { - obj, err = client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Get(context.TODO(), object.GetName(), metav1.GetOptions{}) - if err != nil { - return nil, err - } - return UpdateRemoveFinalizers(obj, client, finalizers...) - } - if obj != nil && len(obj.GetFinalizers()) == 0 { - // to satisfy some tests that requires nil rather than []string{} - obj.SetFinalizers(nil) - } - return obj, err + return UpdateRemoveFinalizers[*crdv1alpha1.VolumeGroupSnapshot]( + object.(*crdv1alpha1.VolumeGroupSnapshot), + client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Update, + client.GroupsnapshotV1alpha1().VolumeGroupSnapshots(object.GetNamespace()).Get, + finalizers...) default: - return nil, errors.New("UpdateRemoveFinalizers: unsupported object type") + return nil, errors.New("UpdateRemoveFinalizersSnapshots: unsupported object type") } } func UpdateRemoveFinalizersCoreV1(object metav1.Object, client kubernetes.Interface, finalizers ...string) (metav1.Object, error) { - object.SetFinalizers(RemoveStrings(object.GetFinalizers(), finalizers...)) - pvc := object.(*corev1.PersistentVolumeClaim) - pvc, err := client.CoreV1().PersistentVolumeClaims(object.GetNamespace()).Update(context.TODO(), pvc, metav1.UpdateOptions{}) - // if out of date, get the object and retry - if err != nil && apierrors.IsConflict(err) { - pvc, err = client.CoreV1().PersistentVolumeClaims(object.GetNamespace()).Get(context.TODO(), pvc.GetName(), metav1.GetOptions{}) - if err != nil { - return nil, err - } - return UpdateRemoveFinalizersCoreV1(pvc, client, finalizers...) - } - if pvc != nil && len(pvc.GetFinalizers()) == 0 { - // to satisfy some tests that requires nil rather than []string{} - pvc.SetFinalizers(nil) + switch object.(type) { + case *corev1.PersistentVolumeClaim: + return UpdateRemoveFinalizers[*corev1.PersistentVolumeClaim]( + object.(*corev1.PersistentVolumeClaim), + client.CoreV1().PersistentVolumeClaims(object.GetNamespace()).Update, + client.CoreV1().PersistentVolumeClaims(object.GetNamespace()).Get, + finalizers...) + default: + return nil, errors.New("UpdateRemoveFinalizersCoreV1: unsupported object type") } - return pvc, err } From 03a1484dc20cccdec133f3562d7483abf9950a16 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 27 Feb 2024 00:06:37 -0500 Subject: [PATCH 08/10] Remove recursion, add conflict tests Signed-off-by: Tiger Kaovilai --- pkg/common-controller/framework_test.go | 6 +++- .../snapshot_finalizer_test.go | 36 ++++++++++++++++++- pkg/utils/update.go | 26 ++++++++------ 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 803860e0e..6c40776d1 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -1317,6 +1317,10 @@ func testRemoveSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *sna return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true, false) } +func testRemoveSnapshotFinalizerAfterUpdateConflict(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { + return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true, false) +} + func testUpdateSnapshotClass(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error { snap, err := ctrl.checkAndUpdateSnapshotClass(test.initialSnapshots[0]) // syncSnapshotByKey expects that checkAndUpdateSnapshotClass always returns a snapshot @@ -1510,7 +1514,7 @@ func runFinalizerTests(t *testing.T, tests []controllerTest, snapshotClasses []* // Run the tested functions err = test.test(ctrl, reactor, test) - if err != nil { + if test.expectSuccess && err != nil { t.Errorf("Test %q failed: %v", test.name, err) } diff --git a/pkg/common-controller/snapshot_finalizer_test.go b/pkg/common-controller/snapshot_finalizer_test.go index 277181887..4217e10cb 100644 --- a/pkg/common-controller/snapshot_finalizer_test.go +++ b/pkg/common-controller/snapshot_finalizer_test.go @@ -17,9 +17,12 @@ limitations under the License. package common_controller import ( + "testing" + + crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" "github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils" v1 "k8s.io/api/core/v1" - "testing" + "k8s.io/apimachinery/pkg/api/errors" ) // Test single call to ensurePVCFinalizer, checkandRemovePVCFinalizer, addSnapshotFinalizer, removeSnapshotFinalizer @@ -82,6 +85,37 @@ func TestSnapshotFinalizer(t *testing.T) { test: testRemoveSnapshotFinalizer, expectSuccess: true, }, + { + name: "2-4 - successful remove Snapshot finalizer after update conflict", + initialSnapshots: newSnapshotArray("snap2-4", "snapuid2-4", "claim2-4", "", classSilver, "", &False, nil, nil, nil, false, true, nil), + initialClaims: newClaimArray("claim2-4", "pvc-uid2-4", "1Gi", "volume2-4", v1.ClaimBound, &classEmpty), + test: testRemoveSnapshotFinalizerAfterUpdateConflict, + expectSuccess: true, + errors: []reactorError{ + {"update", "volumesnapshots", errors.NewConflict(crdv1.Resource("volumesnapshots"), "snap2-4", nil)}, + }, + }, + { + name: "2-5 - unsuccessful remove Snapshot finalizer after update non-conflict error", + initialSnapshots: newSnapshotArray("snap2-5", "snapuid2-5", "claim2-5", "", classSilver, "", &False, nil, nil, nil, false, true, nil), + initialClaims: newClaimArray("claim2-5", "pvc-uid2-5", "1Gi", "volume2-5", v1.ClaimBound, &classEmpty), + test: testRemoveSnapshotFinalizerAfterUpdateConflict, + expectSuccess: false, + errors: []reactorError{ + {"update", "volumesnapshots", errors.NewForbidden(crdv1.Resource("volumesnapshots"), "snap2-5", nil)}, + }, + }, + { + name: "2-6 - unsuccessful remove Snapshot finalizer after update conflict and get fails", + initialSnapshots: newSnapshotArray("snap2-6", "snapuid2-6", "claim2-6", "", classSilver, "", &False, nil, nil, nil, false, true, nil), + initialClaims: newClaimArray("claim2-6", "pvc-uid2-6", "1Gi", "volume2-6", v1.ClaimBound, &classEmpty), + test: testRemoveSnapshotFinalizerAfterUpdateConflict, + expectSuccess: false, + errors: []reactorError{ + {"update", "volumesnapshots", errors.NewConflict(crdv1.Resource("volumesnapshots"), "snap2-6", nil)}, + {"get", "volumesnapshots", errors.NewServerTimeout(crdv1.Resource("volumesnapshots"), "get", 10)}, + }, + }, } runFinalizerTests(t, tests, snapshotClasses) } diff --git a/pkg/utils/update.go b/pkg/utils/update.go index faa2492d5..d64013cce 100644 --- a/pkg/utils/update.go +++ b/pkg/utils/update.go @@ -21,24 +21,30 @@ func UpdateRemoveFinalizers[O metav1.Object]( updateFunc func(context.Context, O, metav1.UpdateOptions) (O, error), getFunc func(context.Context, string, metav1.GetOptions) (O, error), finalizers ...string) (O, error) { - object.SetFinalizers(RemoveStrings(object.GetFinalizers(), finalizers...)) - object, err := updateFunc(context.TODO(), object, metav1.UpdateOptions{}) - if err != nil { - if apierrors.IsConflict(err) { - object, err = getFunc(context.TODO(), object.GetName(), metav1.GetOptions{}) - if err != nil { + for success := false; !success; { + object.SetFinalizers(RemoveStrings(object.GetFinalizers(), finalizers...)) + updatedObject, err := updateFunc(context.TODO(), object, metav1.UpdateOptions{}) + if err != nil { + if apierrors.IsConflict(err) { + object, err = getFunc(context.TODO(), object.GetName(), metav1.GetOptions{}) + if err != nil { + return object, err + } + // retry removing finalizers + continue + } else { + // return error if it's not a conflict return object, err } - return UpdateRemoveFinalizers(object, updateFunc, getFunc, finalizers...) - } else { - return object, err } + success = true + object = updatedObject } if len(object.GetFinalizers()) == 0 { // to satisfy some tests that requires nil rather than []string{} object.SetFinalizers(nil) } - return object, err + return object, nil } func UpdateRemoveFinalizersSnapshots(object metav1.Object, client clientset.Interface, finalizers ...string) (metav1.Object, error) { From 4541deb0dc417cdf201adcc90622105c4f4b9c39 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 27 Feb 2024 00:08:18 -0500 Subject: [PATCH 09/10] go fmt Signed-off-by: Tiger Kaovilai --- pkg/common-controller/snapshot_controller.go | 2 +- pkg/utils/util.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 3bfa6c625..cd29288e6 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -1558,7 +1558,7 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1 if removeGroupFinalizer { stringsToRemove = append(stringsToRemove, utils.VolumeSnapshotInGroupFinalizer) } - + newSnapshot, err := utils.UpdateRemoveFinalizersSnapshots(snapshotClone, ctrl.clientset, stringsToRemove...) if err != nil { return newControllerUpdateError(snapshot.Name, err.Error()) diff --git a/pkg/utils/util.go b/pkg/utils/util.go index f18489c55..f87993972 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -192,7 +192,7 @@ func RemoveString(slice []string, s string) []string { } func RemoveStrings(slice []string, removes ...string) []string { - newSlice := slices.DeleteFunc(slice, func (remove string) bool { + newSlice := slices.DeleteFunc(slice, func(remove string) bool { return slices.Contains(removes, remove) }) return newSlice From 8c619462a423d940b7f67245037ed1194baf5931 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 27 Feb 2024 00:29:03 -0500 Subject: [PATCH 10/10] add wait backoff Signed-off-by: Tiger Kaovilai --- pkg/utils/update.go | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/pkg/utils/update.go b/pkg/utils/update.go index d64013cce..ea0bb0af6 100644 --- a/pkg/utils/update.go +++ b/pkg/utils/update.go @@ -3,6 +3,8 @@ package utils import ( "context" "errors" + "math" + "time" crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1" crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1" @@ -10,9 +12,20 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" ) +// backoff from main.go +const retryFactor = 1.5 +const initialDuration = 100 * time.Millisecond + +var backoff = wait.Backoff{ + Duration: initialDuration, + Factor: retryFactor, + Steps: math.MaxInt32, // effectively no limit until the timeout is reached +} + // Remove one or more finalizers from an object // if finalizers is not empty, only the specified finalizers will be removed // If update fails due to out of date, it will call get on the object and retry removing the finalizers @@ -21,24 +34,29 @@ func UpdateRemoveFinalizers[O metav1.Object]( updateFunc func(context.Context, O, metav1.UpdateOptions) (O, error), getFunc func(context.Context, string, metav1.GetOptions) (O, error), finalizers ...string) (O, error) { - for success := false; !success; { + err := wait.ExponentialBackoff(backoff, func() (bool, error) { object.SetFinalizers(RemoveStrings(object.GetFinalizers(), finalizers...)) updatedObject, err := updateFunc(context.TODO(), object, metav1.UpdateOptions{}) if err != nil { if apierrors.IsConflict(err) { + // conflict, object is out of date, get the latest version object, err = getFunc(context.TODO(), object.GetName(), metav1.GetOptions{}) if err != nil { - return object, err + return false, err } // retry removing finalizers - continue - } else { - // return error if it's not a conflict - return object, err + return false, nil } + // other errors + return false, err } - success = true + // success object = updatedObject + return true, nil + }) + // } + if err != nil { + return object, err } if len(object.GetFinalizers()) == 0 { // to satisfy some tests that requires nil rather than []string{}