Skip to content

Commit

Permalink
Use patch instead of update for GroupSnapshots and VolumeSnapshots
Browse files Browse the repository at this point in the history
Signed-off-by: Tiger Kaovilai <[email protected]>

remove debugging code

Signed-off-by: Tiger Kaovilai <[email protected]>

remove more update calls

Signed-off-by: Tiger Kaovilai <[email protected]>

Fix patch json unmarshal unitTests comparison failures

Signed-off-by: Tiger Kaovilai <[email protected]>

Fix tests in reactor by not modifying original for patch

Signed-off-by: Tiger Kaovilai <[email protected]>
  • Loading branch information
kaovilai committed Feb 24, 2024
1 parent 10c1032 commit 8ece6ac
Show file tree
Hide file tree
Showing 7 changed files with 343 additions and 72 deletions.
8 changes: 6 additions & 2 deletions pkg/common-controller/framework_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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{}
Expand Down
30 changes: 8 additions & 22 deletions pkg/common-controller/groupsnapshot_controller_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -1018,19 +1017,7 @@ 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

if addBoundFinalizer {
Expand All @@ -1045,7 +1032,7 @@ func (ctrl *csiSnapshotCommonController) addGroupSnapshotFinalizer(groupSnapshot
if err != nil {
return newControllerUpdateError(utils.GroupSnapshotKey(groupSnapshot), err.Error())
}
}


_, err = ctrl.storeGroupSnapshotUpdate(updatedGroupSnapshot)
if err != nil {
Expand Down Expand Up @@ -1114,7 +1101,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
Expand Down Expand Up @@ -1161,7 +1148,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)
Expand Down Expand Up @@ -1224,8 +1211,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())
}
Expand Down
100 changes: 100 additions & 0 deletions pkg/common-controller/groupsnapshot_controller_helper_test.go
Original file line number Diff line number Diff line change
@@ -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))
}

})
}
}
90 changes: 43 additions & 47 deletions pkg/common-controller/snapshot_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
Expand All @@ -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())
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 7 additions & 1 deletion pkg/common-controller/snapshot_delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
Loading

0 comments on commit 8ece6ac

Please sign in to comment.