Skip to content

Commit 8fe4025

Browse files
shubham-pampattiwarphoenix-bjoern
shubham-pampattiwar
authored and
phoenix-bjoern
committed
Update VolumeSnapshot and VolumeSnapshotContent using JSON patch
fix unit tests operate on cloned copy revert to update call for removeSnapshotFinalizer fix content delete finalizer
1 parent f7ea027 commit 8fe4025

File tree

4 files changed

+76
-46
lines changed

4 files changed

+76
-46
lines changed

‎pkg/common-controller/snapshot_controller.go‎

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1381,8 +1381,15 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
13811381
}
13821382
klog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name)
13831383
snapshotClone := snapshot.DeepCopy()
1384-
snapshotClone.Spec.VolumeSnapshotClassName = &(defaultClasses[0].Name)
1385-
newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
1384+
patches := []utils.PatchOp{
1385+
{
1386+
Op: "replace",
1387+
Path: "/spec/volumeSnapshotClassName",
1388+
Value: &(defaultClasses[0].Name),
1389+
},
1390+
}
1391+
1392+
newSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset)
13861393
if err != nil {
13871394
klog.V(4).Infof("updating VolumeSnapshot[%s] default class failed %v", utils.SnapshotKey(snapshot), err)
13881395
}
@@ -1606,18 +1613,24 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content
16061613
return content, nil
16071614
}
16081615

1616+
var patches []utils.PatchOp
16091617
contentClone := content.DeepCopy()
16101618
if hasLabel {
16111619
// Need to remove the label
1612-
delete(contentClone.Labels, utils.VolumeSnapshotContentInvalidLabel)
1620+
patches = append(patches, utils.PatchOp{
1621+
Op: "remove",
1622+
Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel,
1623+
})
1624+
16131625
} else {
16141626
// Snapshot content is invalid and does not have the label. Need to add the label
1615-
if contentClone.ObjectMeta.Labels == nil {
1616-
contentClone.ObjectMeta.Labels = make(map[string]string)
1617-
}
1618-
contentClone.ObjectMeta.Labels[utils.VolumeSnapshotContentInvalidLabel] = ""
1627+
patches = append(patches, utils.PatchOp{
1628+
Op: "add",
1629+
Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel,
1630+
Value: "",
1631+
})
16191632
}
1620-
updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
1633+
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
16211634
if err != nil {
16221635
return content, newControllerUpdateError(content.Name, err.Error())
16231636
}
@@ -1647,19 +1660,24 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapsho
16471660
return snapshot, nil
16481661
}
16491662

1663+
var patches []utils.PatchOp
16501664
snapshotClone := snapshot.DeepCopy()
16511665
if hasLabel {
16521666
// Need to remove the label
1653-
delete(snapshotClone.Labels, utils.VolumeSnapshotInvalidLabel)
1667+
patches = append(patches, utils.PatchOp{
1668+
Op: "remove",
1669+
Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel,
1670+
})
16541671
} else {
16551672
// Snapshot is invalid and does not have the label. Need to add the label
1656-
if snapshotClone.ObjectMeta.Labels == nil {
1657-
snapshotClone.ObjectMeta.Labels = make(map[string]string)
1658-
}
1659-
snapshotClone.ObjectMeta.Labels[utils.VolumeSnapshotInvalidLabel] = ""
1673+
patches = append(patches, utils.PatchOp{
1674+
Op: "add",
1675+
Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel,
1676+
Value: "",
1677+
})
16601678
}
16611679

1662-
updatedSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
1680+
updatedSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset)
16631681
if err != nil {
16641682
return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
16651683
}

‎pkg/common-controller/snapshot_finalizer_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,9 @@ limitations under the License.
1717
package common_controller
1818

1919
import (
20-
"testing"
21-
2220
"github.com/kubernetes-csi/external-snapshotter/v6/pkg/utils"
2321
v1 "k8s.io/api/core/v1"
22+
"testing"
2423
)
2524

2625
// Test single call to ensurePVCFinalizer, checkandRemovePVCFinalizer, addSnapshotFinalizer, removeSnapshotFinalizer

‎pkg/sidecar-controller/snapshot_controller.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -542,10 +542,16 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V
542542
// the finalizer does not exit, return directly
543543
return nil
544544
}
545+
var patches []utils.PatchOp
545546
contentClone := content.DeepCopy()
546-
contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
547+
patches = append(patches,
548+
utils.PatchOp{
549+
Op: "replace",
550+
Path: "/metadata/finalizers",
551+
Value: utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer),
552+
})
547553

548-
updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
554+
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
549555
if err != nil {
550556
return newControllerUpdateError(content.Name, err.Error())
551557
}
@@ -638,9 +644,15 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con
638644
return content, nil
639645
}
640646
contentClone := content.DeepCopy()
641-
delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated)
647+
annotationPatchPath := strings.ReplaceAll(utils.AnnVolumeSnapshotBeingCreated, "/", "~1")
648+
649+
var patches []utils.PatchOp
650+
patches = append(patches, utils.PatchOp{
651+
Op: "remove",
652+
Path: "/metadata/annotations/" + annotationPatchPath,
653+
})
642654

643-
updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
655+
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
644656
if err != nil {
645657
return content, newControllerUpdateError(content.Name, err.Error())
646658
}

‎pkg/sidecar-controller/snapshot_delete_test.go‎

Lines changed: 27 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,15 @@ import (
2929
)
3030

3131
var (
32-
defaultSize int64 = 1000
33-
emptySize int64
34-
deletePolicy = crdv1.VolumeSnapshotContentDelete
35-
retainPolicy = crdv1.VolumeSnapshotContentRetain
36-
timeNow = time.Now()
37-
timeNowMetav1 = metav1.Now()
38-
False = false
39-
True = true
32+
defaultSize int64 = 1000
33+
emptySize int64
34+
deletePolicy = crdv1.VolumeSnapshotContentDelete
35+
retainPolicy = crdv1.VolumeSnapshotContentRetain
36+
timeNow = time.Now()
37+
timeNowMetav1 = metav1.Now()
38+
nonFractionalTime = metav1.NewTime(time.Now().Truncate(time.Second))
39+
False = false
40+
True = true
4041
)
4142

4243
var class1Parameters = map[string]string{
@@ -153,8 +154,8 @@ func TestDeleteSync(t *testing.T) {
153154
tests := []controllerTest{
154155
{
155156
name: "1-1 - content non-nil DeletionTimestamp with delete policy will delete snapshot",
156-
initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
157-
expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1),
157+
initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &nonFractionalTime),
158+
expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &nonFractionalTime),
158159
expectedEvents: noevents,
159160
errors: noerrors,
160161
initialSecrets: []*v1.Secret{secret()},
@@ -177,8 +178,8 @@ func TestDeleteSync(t *testing.T) {
177178
},
178179
{
179180
name: "1-2 - content non-nil DeletionTimestamp with retain policy will not delete snapshot",
180-
initialContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, true, &timeNowMetav1),
181-
expectedContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, false, &timeNowMetav1),
181+
initialContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, true, &nonFractionalTime),
182+
expectedContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, false, &nonFractionalTime),
182183
expectedEvents: noevents,
183184
errors: noerrors,
184185
expectedCreateCalls: []createCall{
@@ -280,8 +281,8 @@ func TestDeleteSync(t *testing.T) {
280281
},
281282
{
282283
name: "1-9 - continue deletion with snapshot class that has nonexistent secret, bound finalizer removed",
283-
initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
284-
expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
284+
initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
285+
expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
285286
expectedEvents: noevents,
286287
expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil}},
287288
errors: noerrors,
@@ -291,8 +292,8 @@ func TestDeleteSync(t *testing.T) {
291292
},
292293
{
293294
name: "1-10 - (dynamic)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer",
294-
initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
295-
expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
295+
initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &nonFractionalTime),
296+
expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &nonFractionalTime),
296297
expectedEvents: noevents,
297298
expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil}},
298299
errors: noerrors,
@@ -301,17 +302,17 @@ func TestDeleteSync(t *testing.T) {
301302
},
302303
{
303304
name: "1-11 - (dynamic)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.",
304-
initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
305-
expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &timeNowMetav1),
305+
initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
306+
expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &nonFractionalTime),
306307
expectedEvents: noevents,
307308
errors: noerrors,
308309
expectedDeleteCalls: []deleteCall{{"sid1-11", nil, nil}},
309310
test: testSyncContent,
310311
},
311312
{
312313
name: "1-12 - (pre-provision)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer",
313-
initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
314-
expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
314+
initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &nonFractionalTime),
315+
expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &nonFractionalTime),
315316
expectedEvents: noevents,
316317
expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil}},
317318
errors: noerrors,
@@ -320,26 +321,26 @@ func TestDeleteSync(t *testing.T) {
320321
},
321322
{
322323
name: "1-13 - (pre-provision)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.",
323-
initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
324-
expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &timeNowMetav1),
324+
initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
325+
expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &nonFractionalTime),
325326
expectedEvents: noevents,
326327
errors: noerrors,
327328
expectedDeleteCalls: []deleteCall{{"sid1-13", nil, nil}},
328329
test: testSyncContent,
329330
},
330331
{
331332
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.",
332-
initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
333-
expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &timeNowMetav1),
333+
initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
334+
expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &nonFractionalTime),
334335
expectedEvents: noevents,
335336
errors: noerrors,
336337
expectedDeleteCalls: []deleteCall{{"sid1-14", nil, nil}},
337338
test: testSyncContent,
338339
},
339340
{
340341
name: "1-15 - (dynamic)deletion of content with no snapshotclass should succeed",
341-
initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
342-
expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
342+
initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
343+
expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
343344
errors: noerrors,
344345
expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}},
345346
test: testSyncContent,

0 commit comments

Comments
 (0)