Skip to content

Commit

Permalink
Dont call DeleteSnapshot when VS belong to VGS
Browse files Browse the repository at this point in the history
Dont call the DeleteSnapshot RPC call when a snapshot
is created as part of the VolumeGroupSnapshot.
  • Loading branch information
Madhu-1 committed Dec 4, 2024
1 parent 3cc3131 commit 34854a6
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 7 deletions.
9 changes: 9 additions & 0 deletions pkg/sidecar-controller/framework_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,15 @@ func newContentArrayWithReadyToUse(contentName, boundToSnapshotUID, boundToSnaps
}
}

func newContentWithVolumeGroupSnapshotHandle(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, volumeGroupSnapshotHandle, snapshotClassName, desiredSnapshotHandle,
volumeHandle string, deletionPolicy crdv1.DeletionPolicy, creationTime, size *int64, withFinalizer bool, deletionTime *metav1.Time) []*crdv1.VolumeSnapshotContent {
content := newContentArrayWithDeletionTimestamp(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle, deletionPolicy, creationTime, size, withFinalizer, deletionTime)
for _, c := range content {
c.Status.VolumeGroupSnapshotHandle = &volumeGroupSnapshotHandle
}
return content
}

func newContentArrayWithDeletionTimestamp(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string,
deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64,
withFinalizer bool, deletionTime *metav1.Time) []*crdv1.VolumeSnapshotContent {
Expand Down
15 changes: 8 additions & 7 deletions pkg/sidecar-controller/snapshot_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,11 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
if ctrl.shouldDelete(content) {
klog.V(4).Infof("VolumeSnapshotContent[%s]: the policy is %s", content.Name, content.Spec.DeletionPolicy)
if content.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete &&
content.Status != nil && content.Status.SnapshotHandle != nil {
// issue a CSI deletion call if the snapshot has not been deleted yet from
// underlying storage system. Note that the deletion snapshot operation will
// update content SnapshotHandle to nil upon a successful deletion. At this
content.Status != nil && content.Status.SnapshotHandle != nil && content.Status.VolumeGroupSnapshotHandle == nil {
// issue a CSI deletion call if the snapshot does not belong to volumegroupsnapshot
// and it has not been deleted yet from underlying storage system.
// Note that the deletion snapshot operation will update content SnapshotHandle
// to nil upon a successful deletion. At this
// point, the finalizer on content should NOT be removed to avoid leaking.
err := ctrl.deleteCSISnapshot(content)
if err != nil {
Expand All @@ -73,9 +74,9 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
return false, nil
}
// otherwise, either the snapshot has been deleted from the underlying
// storage system, or the deletion policy is Retain, remove the finalizer
// if there is one so that API server could delete the object if there is
// no other finalizer.
// storage system, or it belongs to a volumegroupsnapshot, or the deletion policy is Retain,
// remove the finalizer if there is one so that API server could delete
// the object if there is no other finalizer.
err := ctrl.removeContentFinalizer(content)
if err != nil {
return true, err
Expand Down
19 changes: 19 additions & 0 deletions pkg/sidecar-controller/snapshot_delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,25 @@ func TestDeleteSync(t *testing.T) {
expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}},
test: testSyncContent,
},

{
name: "1-16 - (dynamic)deletion of content with no VolumeGroupSnapshotHandle should succeed",
initialContents: newContentWithVolumeGroupSnapshotHandle("content1-16", "sid1-16", "snap1-16", "snap1-16", "grp-snap1-16", "", "", "snap1-16-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentWithVolumeGroupSnapshotHandle("content1-16", "sid1-16", "snap1-16", "snap1-16", "grp-snap1-16", "", "", "snap1-16-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
expectSuccess: true,
errors: noerrors,
expectedDeleteCalls: []deleteCall{{"sid1-16", nil, nil}},
test: testSyncContent,
},
{
name: "1-17 - (dynamic)deletion of content with VolumeGroupSnapshotHandle should succeed with no call to CSI driver",
initialContents: newContentWithVolumeGroupSnapshotHandle("content1-17", "sid1-17", "snap1-17", "snap1-17", "grp-snap1-17", "", "", "snap1-17-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
expectedContents: newContentWithVolumeGroupSnapshotHandle("content1-17", "sid1-17", "snap1-17", "snap1-17", "grp-snap1-17", "", "", "snap1-17-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
expectSuccess: true,
errors: noerrors,
expectedDeleteCalls: []deleteCall{},
test: testSyncContent,
},
}
runSyncContentTests(t, tests, snapshotClasses)
}

0 comments on commit 34854a6

Please sign in to comment.