Skip to content

Commit 50fa049

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request kubernetes-csi#143 from kaovilai/cherry-pick-ff71329-ocp/release-4.15
OCPBUGS-29336: cherry-pick:release-4.15: OCPBUGS-29244 Update VolumeSnapshot and VolumeSnapshotContent using JSON patch
2 parents 090cd57 + 8bcec53 commit 50fa049

File tree

4 files changed

+76
-46
lines changed

4 files changed

+76
-46
lines changed

‎pkg/common-controller/snapshot_controller.go

+32-14
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

+1-2
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

+16-4
Original file line numberDiff line numberDiff line change
@@ -558,10 +558,16 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V
558558
// the finalizer does not exit, return directly
559559
return nil
560560
}
561+
var patches []utils.PatchOp
561562
contentClone := content.DeepCopy()
562-
contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
563+
patches = append(patches,
564+
utils.PatchOp{
565+
Op: "replace",
566+
Path: "/metadata/finalizers",
567+
Value: utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer),
568+
})
563569

564-
updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
570+
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
565571
if err != nil {
566572
return newControllerUpdateError(content.Name, err.Error())
567573
}
@@ -654,9 +660,15 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con
654660
return content, nil
655661
}
656662
contentClone := content.DeepCopy()
657-
delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated)
663+
annotationPatchPath := strings.ReplaceAll(utils.AnnVolumeSnapshotBeingCreated, "/", "~1")
664+
665+
var patches []utils.PatchOp
666+
patches = append(patches, utils.PatchOp{
667+
Op: "remove",
668+
Path: "/metadata/annotations/" + annotationPatchPath,
669+
})
658670

659-
updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
671+
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
660672
if err != nil {
661673
return content, newControllerUpdateError(content.Name, err.Error())
662674
}

‎pkg/sidecar-controller/snapshot_delete_test.go

+27-26
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{
@@ -282,8 +283,8 @@ func TestDeleteSync(t *testing.T) {
282283
},
283284
{
284285
name: "1-9 - continue deletion with snapshot class that has nonexistent secret, bound finalizer removed",
285-
initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
286-
expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
286+
initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
287+
expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
287288
expectedEvents: noevents,
288289
expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil}},
289290
expectSuccess: true,
@@ -294,8 +295,8 @@ func TestDeleteSync(t *testing.T) {
294295
},
295296
{
296297
name: "1-10 - (dynamic)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer",
297-
initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
298-
expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
298+
initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &nonFractionalTime),
299+
expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &nonFractionalTime),
299300
expectedEvents: noevents,
300301
expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil}},
301302
expectSuccess: true,
@@ -305,8 +306,8 @@ func TestDeleteSync(t *testing.T) {
305306
},
306307
{
307308
name: "1-11 - (dynamic)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.",
308-
initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
309-
expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &timeNowMetav1),
309+
initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
310+
expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &nonFractionalTime),
310311
expectedEvents: noevents,
311312
expectSuccess: true,
312313
errors: noerrors,
@@ -315,8 +316,8 @@ func TestDeleteSync(t *testing.T) {
315316
},
316317
{
317318
name: "1-12 - (pre-provision)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer",
318-
initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
319-
expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
319+
initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &nonFractionalTime),
320+
expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &nonFractionalTime),
320321
expectedEvents: noevents,
321322
expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil}},
322323
expectSuccess: true,
@@ -326,8 +327,8 @@ func TestDeleteSync(t *testing.T) {
326327
},
327328
{
328329
name: "1-13 - (pre-provision)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.",
329-
initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
330-
expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &timeNowMetav1),
330+
initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
331+
expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &nonFractionalTime),
331332
expectedEvents: noevents,
332333
expectSuccess: true,
333334
errors: noerrors,
@@ -336,8 +337,8 @@ func TestDeleteSync(t *testing.T) {
336337
},
337338
{
338339
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.",
339-
initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
340-
expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &timeNowMetav1),
340+
initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
341+
expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &nonFractionalTime),
341342
expectedEvents: noevents,
342343
expectSuccess: true,
343344
errors: noerrors,
@@ -346,8 +347,8 @@ func TestDeleteSync(t *testing.T) {
346347
},
347348
{
348349
name: "1-15 - (dynamic)deletion of content with no snapshotclass should succeed",
349-
initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
350-
expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
350+
initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
351+
expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
351352
expectSuccess: true,
352353
errors: noerrors,
353354
expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}},

0 commit comments

Comments
 (0)