Skip to content

Commit

Permalink
Update VolumeSnapshot and VolumeSnapshotContent using JSON patch
Browse files Browse the repository at this point in the history 
fix unit tests

operate on cloned copy

revert to update call for removeSnapshotFinalizer

fix content delete finalizer
  • Loading branch information
@shubham-pampattiwar @kaovilai
shubham-pampattiwar authored and kaovilai committed Feb 7, 2024
1 parent 5ce4b5c commit 39f27f0
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 46 deletions.
46 changes: 32 additions & 14 deletions pkg/common-controller/snapshot_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1380,8 +1380,15 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
}
klog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name)
snapshotClone := snapshot.DeepCopy()
snapshotClone.Spec.VolumeSnapshotClassName = &(defaultClasses[0].Name)
newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
patches := []utils.PatchOp{
{
Op: "replace",
Path: "/spec/volumeSnapshotClassName",
Value: &(defaultClasses[0].Name),
},
}

newSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset)
if err != nil {
klog.V(4).Infof("updating VolumeSnapshot[%s] default class failed %v", utils.SnapshotKey(snapshot), err)
}
Expand Down Expand Up @@ -1605,18 +1612,24 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content
return content, nil
}

var patches []utils.PatchOp
contentClone := content.DeepCopy()
if hasLabel {
// Need to remove the label
delete(contentClone.Labels, utils.VolumeSnapshotContentInvalidLabel)
patches = append(patches, utils.PatchOp{
Op: "remove",
Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel,
})

} else {
// Snapshot content is invalid and does not have the label. Need to add the label
if contentClone.ObjectMeta.Labels == nil {
contentClone.ObjectMeta.Labels = make(map[string]string)
}
contentClone.ObjectMeta.Labels[utils.VolumeSnapshotContentInvalidLabel] = ""
patches = append(patches, utils.PatchOp{
Op: "add",
Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel,
Value: "",
})
}
updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
if err != nil {
return content, newControllerUpdateError(content.Name, err.Error())
}
Expand Down Expand Up @@ -1646,19 +1659,24 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapsho
return snapshot, nil
}

var patches []utils.PatchOp
snapshotClone := snapshot.DeepCopy()
if hasLabel {
// Need to remove the label
delete(snapshotClone.Labels, utils.VolumeSnapshotInvalidLabel)
patches = append(patches, utils.PatchOp{
Op: "remove",
Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel,
})
} else {
// Snapshot is invalid and does not have the label. Need to add the label
if snapshotClone.ObjectMeta.Labels == nil {
snapshotClone.ObjectMeta.Labels = make(map[string]string)
}
snapshotClone.ObjectMeta.Labels[utils.VolumeSnapshotInvalidLabel] = ""
patches = append(patches, utils.PatchOp{
Op: "add",
Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel,
Value: "",
})
}

updatedSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
updatedSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset)
if err != nil {
return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
}
Expand Down
3 changes: 1 addition & 2 deletions pkg/common-controller/snapshot_finalizer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ limitations under the License.
package common_controller

import (
"testing"

"github.com/kubernetes-csi/external-snapshotter/v6/pkg/utils"
v1 "k8s.io/api/core/v1"
"testing"
)

// Test single call to ensurePVCFinalizer, checkandRemovePVCFinalizer, addSnapshotFinalizer, removeSnapshotFinalizer
Expand Down
20 changes: 16 additions & 4 deletions pkg/sidecar-controller/snapshot_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,10 +542,16 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V
// the finalizer does not exit, return directly
return nil
}
var patches []utils.PatchOp
contentClone := content.DeepCopy()
contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
patches = append(patches,
utils.PatchOp{
Op: "replace",
Path: "/metadata/finalizers",
Value: utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer),
})

updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
if err != nil {
return newControllerUpdateError(content.Name, err.Error())
}
Expand Down Expand Up @@ -638,9 +644,15 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con
return content, nil
}
contentClone := content.DeepCopy()
delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated)
annotationPatchPath := strings.ReplaceAll(utils.AnnVolumeSnapshotBeingCreated, "/", "~1")

var patches []utils.PatchOp
patches = append(patches, utils.PatchOp{
Op: "remove",
Path: "/metadata/annotations/" + annotationPatchPath,
})

updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
if err != nil {
return content, newControllerUpdateError(content.Name, err.Error())
}
Expand Down
53 changes: 27 additions & 26 deletions pkg/sidecar-controller/snapshot_delete_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,15 @@ import (
)

var (
defaultSize int64 = 1000
emptySize int64
deletePolicy = crdv1.VolumeSnapshotContentDelete
retainPolicy = crdv1.VolumeSnapshotContentRetain
timeNow = time.Now()
timeNowMetav1 = metav1.Now()
False = false
True = true
defaultSize int64 = 1000
emptySize int64
deletePolicy = crdv1.VolumeSnapshotContentDelete
retainPolicy = crdv1.VolumeSnapshotContentRetain
timeNow = time.Now()
timeNowMetav1 = metav1.Now()
nonFractionalTime = metav1.NewTime(time.Now().Truncate(time.Second))
False = false
True = true
)

var class1Parameters = map[string]string{
Expand Down Expand Up @@ -153,8 +154,8 @@ func TestDeleteSync(t *testing.T) {
tests := []controllerTest{
{
name: "1-1 - content non-nil DeletionTimestamp with delete policy will delete snapshot",
initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents,
errors: noerrors,
initialSecrets: []*v1.Secret{secret()},
Expand All @@ -177,8 +178,8 @@ func TestDeleteSync(t *testing.T) {
},
{
name: "1-2 - content non-nil DeletionTimestamp with retain policy will not delete snapshot",
initialContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents,
errors: noerrors,
expectedCreateCalls: []createCall{
Expand Down Expand Up @@ -280,8 +281,8 @@ func TestDeleteSync(t *testing.T) {
},
{
name: "1-9 - continue deletion with snapshot class that has nonexistent secret, bound finalizer removed",
initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors,
Expand All @@ -291,8 +292,8 @@ func TestDeleteSync(t *testing.T) {
},
{
name: "1-10 - (dynamic)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer",
initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &nonFractionalTime),
expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors,
Expand All @@ -301,17 +302,17 @@ func TestDeleteSync(t *testing.T) {
},
{
name: "1-11 - (dynamic)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.",
initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents,
errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-11", nil, nil}},
test: testSyncContent,
},
{
name: "1-12 - (pre-provision)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer",
initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &nonFractionalTime),
expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors,
Expand All @@ -320,26 +321,26 @@ func TestDeleteSync(t *testing.T) {
},
{
name: "1-13 - (pre-provision)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.",
initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents,
errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-13", nil, nil}},
test: testSyncContent,
},
{
name: "1-14 - (pre-provision)deletion of content with deletion policy and no snapshotclass should trigger CSI call, update status, and remove bound finalizer removed.",
initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents,
errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-14", nil, nil}},
test: testSyncContent,
},
{
name: "1-15 - (dynamic)deletion of content with no snapshotclass should succeed",
initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}},
test: testSyncContent,
Expand Down

0 comments on commit 39f27f0

Please sign in to comment.