Skip to content

Commit

Permalink
Merge pull request #876 from shubham-pampattiwar/fix-issue-748
Browse files Browse the repository at this point in the history 
Update VolumeSnapshot and VolumeSnapshotContent using JSON patch
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Sep 19, 2023
2 parents 4c45d58 + ff71329 commit 215f12f
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 @@ -1381,8 +1381,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 @@ -1606,18 +1613,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 @@ -1647,19 +1660,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 215f12f

Please sign in to comment.