Skip to content

Commit

Permalink
Use update to remove finalizers instead of patch with get retries
Browse files Browse the repository at this point in the history
Signed-off-by: Tiger Kaovilai <[email protected]>
  • Loading branch information
kaovilai committed Feb 26, 2024
1 parent db6559f commit 931529b
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 174 deletions.
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/common-controller/groupsnapshot_controller_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand Down
17 changes: 5 additions & 12 deletions pkg/common-controller/snapshot_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand Down Expand Up @@ -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 {
Expand Down
77 changes: 0 additions & 77 deletions pkg/utils/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@ package utils
import (
"context"
"encoding/json"
"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"
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
Expand Down Expand Up @@ -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 {
Expand Down
83 changes: 0 additions & 83 deletions pkg/utils/patch_test.go

This file was deleted.

72 changes: 72 additions & 0 deletions pkg/utils/update.go
Original file line number Diff line number Diff line change
@@ -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
}
8 changes: 8 additions & 0 deletions pkg/utils/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"os"
"slices"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit 931529b

Please sign in to comment.