alexlebens/external-snapshotter
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Update VolumeSnapshot and VolumeSnapshotContent using JSON patch

Browse Source 
fix unit tests

operate on cloned copy

revert to update call for removeSnapshotFinalizer

fix content delete finalizer
This commit is contained in:
Shubham Pampattiwar
2023-07-12 15:10:51 -07:00
parent 304f4bfc47
commit ff71329d8c
4 changed files with 76 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
pkg/common-controller/snapshot_controller.go
View File

@@ -1381,8 +1381,15 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
} }
klog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name) klog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name)
snapshotClone := snapshot.DeepCopy() snapshotClone := snapshot.DeepCopy()
snapshotClone.Spec.VolumeSnapshotClassName = &(defaultClasses[0].Name) patches := []utils.PatchOp{
newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) {
Op: "replace",
Path: "/spec/volumeSnapshotClassName",
Value: &(defaultClasses[0].Name),
},
}
newSnapshot, err := utils.PatchVolumeSnapshot(snapshotClone, patches, ctrl.clientset)
if err != nil { if err != nil {
klog.V(4).Infof("updating VolumeSnapshot[%s] default class failed %v", utils.SnapshotKey(snapshot), err) klog.V(4).Infof("updating VolumeSnapshot[%s] default class failed %v", utils.SnapshotKey(snapshot), err)
} }
@@ -1606,18 +1613,24 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content
return content, nil return content, nil
} }
var patches []utils.PatchOp
contentClone := content.DeepCopy() contentClone := content.DeepCopy()
if hasLabel { if hasLabel {
// Need to remove the label // Need to remove the label
delete(contentClone.Labels, utils.VolumeSnapshotContentInvalidLabel) patches = append(patches, utils.PatchOp{
Op: "remove",
Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel,
})
} else { } else {
// Snapshot content is invalid and does not have the label. Need to add the label // Snapshot content is invalid and does not have the label. Need to add the label
if contentClone.ObjectMeta.Labels == nil { patches = append(patches, utils.PatchOp{
contentClone.ObjectMeta.Labels = make(map[string]string) Op: "add",
} Path: "/metadata/labels/" + utils.VolumeSnapshotContentInvalidLabel,
contentClone.ObjectMeta.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 { if err != nil {
return content, newControllerUpdateError(content.Name, err.Error()) return content, newControllerUpdateError(content.Name, err.Error())
} }
@@ -1647,19 +1660,24 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapsho
return snapshot, nil return snapshot, nil
} }
var patches []utils.PatchOp
snapshotClone := snapshot.DeepCopy() snapshotClone := snapshot.DeepCopy()
if hasLabel { if hasLabel {
// Need to remove the label // Need to remove the label
delete(snapshotClone.Labels, utils.VolumeSnapshotInvalidLabel) patches = append(patches, utils.PatchOp{
Op: "remove",
Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel,
})
} else { } else {
// Snapshot is invalid and does not have the label. Need to add the label // Snapshot is invalid and does not have the label. Need to add the label
if snapshotClone.ObjectMeta.Labels == nil { patches = append(patches, utils.PatchOp{
snapshotClone.ObjectMeta.Labels = make(map[string]string) Op: "add",
} Path: "/metadata/labels/" + utils.VolumeSnapshotInvalidLabel,
snapshotClone.ObjectMeta.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 { if err != nil {
return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
} }

3
pkg/common-controller/snapshot_finalizer_test.go
View File

@@ -17,10 +17,9 @@ limitations under the License.
package common_controller package common_controller
import ( import (
"testing"
"github.com/kubernetes-csi/external-snapshotter/v6/pkg/utils" "github.com/kubernetes-csi/external-snapshotter/v6/pkg/utils"
v1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1"
"testing"
) )
// Test single call to ensurePVCFinalizer, checkandRemovePVCFinalizer, addSnapshotFinalizer, removeSnapshotFinalizer // Test single call to ensurePVCFinalizer, checkandRemovePVCFinalizer, addSnapshotFinalizer, removeSnapshotFinalizer

20
pkg/sidecar-controller/snapshot_controller.go
View File

@@ -542,10 +542,16 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V
// the finalizer does not exit, return directly // the finalizer does not exit, return directly
return nil return nil
} }
var patches []utils.PatchOp
contentClone := content.DeepCopy() 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 { if err != nil {
return newControllerUpdateError(content.Name, err.Error()) return newControllerUpdateError(content.Name, err.Error())
} }
@@ -638,9 +644,15 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con
return content, nil return content, nil
} }
contentClone := content.DeepCopy() contentClone := content.DeepCopy()
delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated) annotationPatchPath := strings.ReplaceAll(utils.AnnVolumeSnapshotBeingCreated, "/", "~1")
updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) var patches []utils.PatchOp
patches = append(patches, utils.PatchOp{
Op: "remove",
Path: "/metadata/annotations/" + annotationPatchPath,
})
updatedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset)
if err != nil { if err != nil {
return content, newControllerUpdateError(content.Name, err.Error()) return content, newControllerUpdateError(content.Name, err.Error())
} }

53
pkg/sidecar-controller/snapshot_delete_test.go
View File

@@ -29,14 +29,15 @@ import (
) )
var ( var (
defaultSize int64 = 1000 defaultSize int64 = 1000
emptySize int64 emptySize int64
deletePolicy = crdv1.VolumeSnapshotContentDelete deletePolicy = crdv1.VolumeSnapshotContentDelete
retainPolicy = crdv1.VolumeSnapshotContentRetain retainPolicy = crdv1.VolumeSnapshotContentRetain
timeNow = time.Now() timeNow = time.Now()
timeNowMetav1 = metav1.Now() timeNowMetav1 = metav1.Now()
False = false nonFractionalTime = metav1.NewTime(time.Now().Truncate(time.Second))
True = true False = false
True = true
) )
var class1Parameters = map[string]string{ var class1Parameters = map[string]string{
@@ -153,8 +154,8 @@ func TestDeleteSync(t *testing.T) {
tests := []controllerTest{ tests := []controllerTest{
{ {
name: "1-1 - content non-nil DeletionTimestamp with delete policy will delete snapshot", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents, expectedEvents: noevents,
errors: noerrors, errors: noerrors,
initialSecrets: []*v1.Secret{secret()}, initialSecrets: []*v1.Secret{secret()},
@@ -177,8 +178,8 @@ func TestDeleteSync(t *testing.T) {
}, },
{ {
name: "1-2 - content non-nil DeletionTimestamp with retain policy will not delete snapshot", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-2", "snapuid1-2", "snap1-2", "sid1-2", classGold, "", "snap1-2-volumehandle", retainPolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents, expectedEvents: noevents,
errors: noerrors, errors: noerrors,
expectedCreateCalls: []createCall{ expectedCreateCalls: []createCall{
@@ -280,8 +281,8 @@ func TestDeleteSync(t *testing.T) {
}, },
{ {
name: "1-9 - continue deletion with snapshot class that has nonexistent secret, bound finalizer removed", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
expectedEvents: noevents, expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil}}, expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors, errors: noerrors,
@@ -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", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &nonFractionalTime),
expectedEvents: noevents, expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil}}, expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors, errors: noerrors,
@@ -301,8 +302,8 @@ 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.", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents, expectedEvents: noevents,
errors: noerrors, errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-11", nil, nil}}, expectedDeleteCalls: []deleteCall{{"sid1-11", nil, nil}},
@@ -310,8 +311,8 @@ func TestDeleteSync(t *testing.T) {
}, },
{ {
name: "1-12 - (pre-provision)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &nonFractionalTime),
expectedEvents: noevents, expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil}}, expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors, errors: noerrors,
@@ -320,8 +321,8 @@ 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.", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents, expectedEvents: noevents,
errors: noerrors, errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-13", nil, nil}}, expectedDeleteCalls: []deleteCall{{"sid1-13", nil, nil}},
@@ -329,8 +330,8 @@ func TestDeleteSync(t *testing.T) {
}, },
{ {
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.", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &nonFractionalTime),
expectedEvents: noevents, expectedEvents: noevents,
errors: noerrors, errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-14", nil, nil}}, expectedDeleteCalls: []deleteCall{{"sid1-14", nil, nil}},
@@ -338,8 +339,8 @@ func TestDeleteSync(t *testing.T) {
}, },
{ {
name: "1-15 - (dynamic)deletion of content with no snapshotclass should succeed", 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), 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, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
errors: noerrors, errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}}, expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}},
test: testSyncContent, test: testSyncContent,