Skip to content

Commit

Permalink
Merge pull request kubernetes-csi#15 from jsafrane/fix-retain-delete
Browse files Browse the repository at this point in the history 
Bug 1802467: Fix deletion of snapshots with Retain policy
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Mar 9, 2020
2 parents 3da7dad + 1373e47 commit 8c40671
Show file tree
Hide file tree
Showing 7 changed files with 213 additions and 120 deletions.
54 changes: 38 additions & 16 deletions pkg/common-controller/framework_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,17 @@ type reactorError struct {
error error
}

func withSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot) *crdv1.VolumeSnapshot {
snapshot.ObjectMeta.Finalizers = append(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
snapshot.ObjectMeta.Finalizers = append(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer)
snapshot.ObjectMeta.Finalizers = append(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
return snapshot
func withSnapshotFinalizers(snapshots []*crdv1.VolumeSnapshot, finalizers ...string) []*crdv1.VolumeSnapshot {
for i := range snapshots {
for _, f := range finalizers {
snapshots[i].ObjectMeta.Finalizers = append(snapshots[i].ObjectMeta.Finalizers, f)
}
}
return snapshots
}

func withContentFinalizer(content *crdv1.VolumeSnapshotContent) *crdv1.VolumeSnapshotContent {
content.ObjectMeta.Finalizers = append(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes")
return content
}

Expand Down Expand Up @@ -824,6 +825,18 @@ func newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHa
return &content
}

func withContentAnnotations(contents []*crdv1.VolumeSnapshotContent, annotations map[string]string) []*crdv1.VolumeSnapshotContent {
for i := range contents {
if contents[i].ObjectMeta.Annotations == nil {
contents[i].ObjectMeta.Annotations = make(map[string]string)
}
for k, v := range annotations {
contents[i].ObjectMeta.Annotations[k] = v
}
}
return contents
}

func newContentArray(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string,
deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64,
withFinalizer bool) []*crdv1.VolumeSnapshotContent {
Expand Down Expand Up @@ -863,14 +876,15 @@ func newContentWithUnmatchDriverArray(contentName, boundToSnapshotUID, boundToSn
func newSnapshot(
snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName string,
readyToUse *bool, creationTime *metav1.Time, restoreSize *resource.Quantity,
err *crdv1.VolumeSnapshotError, nilStatus bool, withFinalizer bool) *crdv1.VolumeSnapshot {
err *crdv1.VolumeSnapshotError, nilStatus bool, withAllFinalizers bool, deletionTimestamp *metav1.Time) *crdv1.VolumeSnapshot {
snapshot := crdv1.VolumeSnapshot{
ObjectMeta: metav1.ObjectMeta{
Name: snapshotName,
Namespace: testNamespace,
UID: types.UID(snapshotUID),
ResourceVersion: "1",
SelfLink: "/apis/snapshot.storage.k8s.io/v1beta1/namespaces/" + testNamespace + "/volumesnapshots/" + snapshotName,
Name: snapshotName,
Namespace: testNamespace,
UID: types.UID(snapshotUID),
ResourceVersion: "1",
SelfLink: "/apis/snapshot.storage.k8s.io/v1beta1/namespaces/" + testNamespace + "/volumesnapshots/" + snapshotName,
DeletionTimestamp: deletionTimestamp,
},
Spec: crdv1.VolumeSnapshotSpec{
VolumeSnapshotClassName: nil,
Expand Down Expand Up @@ -903,18 +917,18 @@ func newSnapshot(
VolumeSnapshotContentName: &targetContentName,
}
}
if withFinalizer {
return withSnapshotFinalizer(&snapshot)
if withAllFinalizers {
return withSnapshotFinalizers([]*crdv1.VolumeSnapshot{&snapshot}, utils.VolumeSnapshotAsSourceFinalizer, utils.VolumeSnapshotBoundFinalizer)[0]
}
return &snapshot
}

func newSnapshotArray(
snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName string,
readyToUse *bool, creationTime *metav1.Time, restoreSize *resource.Quantity,
err *crdv1.VolumeSnapshotError, nilStatus bool, withFinalizer bool) []*crdv1.VolumeSnapshot {
err *crdv1.VolumeSnapshotError, nilStatus bool, withAllFinalizers bool, deletionTimestamp *metav1.Time) []*crdv1.VolumeSnapshot {
return []*crdv1.VolumeSnapshot{
newSnapshot(snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName, readyToUse, creationTime, restoreSize, err, nilStatus, withFinalizer),
newSnapshot(snapshotName, snapshotUID, pvcName, targetContentName, snapshotClassName, boundContentName, readyToUse, creationTime, restoreSize, err, nilStatus, withAllFinalizers, deletionTimestamp),
}
}

Expand Down Expand Up @@ -1070,6 +1084,14 @@ func testSyncContent(ctrl *csiSnapshotCommonController, reactor *snapshotReactor
return ctrl.syncContent(test.initialContents[0])
}

func testSyncContentError(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
err := ctrl.syncContent(test.initialContents[0])
if err != nil {
return nil
}
return fmt.Errorf("syncContent succeeded when failure was expected")
}

func testAddPVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
return ctrl.ensurePVCFinalizer(test.initialSnapshots[0])
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/common-controller/snapshot_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec
}

klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: set DeletionTimeStamp on content.", utils.SnapshotKey(snapshot))

// If content exists, set DeletionTimeStamp on the content;
// content won't be deleted immediately due to the finalizer
if content != nil && deleteContent && !inUse {
Expand All @@ -261,6 +262,9 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec
}

if !inUse {
klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent: Set VolumeSnapshotBeingDeleted annotation on the content [%s]", content.Name)
ctrl.setAnnVolumeSnapshotBeingDeleted(content)

klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent: Remove Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot))
doesContentExist := false
if content != nil {
Expand Down
28 changes: 14 additions & 14 deletions pkg/common-controller/snapshot_create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ func TestCreateSnapshotSync(t *testing.T) {
name: "6-1 - successful create snapshot with snapshot class gold",
initialContents: nocontents,
expectedContents: newContentArrayNoStatus("snapcontent-snapuid6-1", "snapuid6-1", "snap6-1", "sid6-1", classGold, "", "pv-handle6-1", deletionPolicy, nil, nil, false, false),
initialSnapshots: newSnapshotArray("snap6-1", "snapuid6-1", "claim6-1", "", classGold, "", &False, nil, nil, nil, false, true),
expectedSnapshots: newSnapshotArray("snap6-1", "snapuid6-1", "claim6-1", "", classGold, "snapcontent-snapuid6-1", &False, nil, nil, nil, false, true),
initialSnapshots: newSnapshotArray("snap6-1", "snapuid6-1", "claim6-1", "", classGold, "", &False, nil, nil, nil, false, true, nil),
expectedSnapshots: newSnapshotArray("snap6-1", "snapuid6-1", "claim6-1", "", classGold, "snapcontent-snapuid6-1", &False, nil, nil, nil, false, true, nil),
initialClaims: newClaimArray("claim6-1", "pvc-uid6-1", "1Gi", "volume6-1", v1.ClaimBound, &classEmpty),
initialVolumes: newVolumeArray("volume6-1", "pv-uid6-1", "pv-handle6-1", "1Gi", "pvc-uid6-1", "claim6-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
errors: noerrors,
Expand All @@ -82,8 +82,8 @@ func TestCreateSnapshotSync(t *testing.T) {
name: "6-2 - successful create snapshot with snapshot class silver",
initialContents: nocontents,
expectedContents: newContentArrayNoStatus("snapcontent-snapuid6-2", "snapuid6-2", "snap6-2", "sid6-2", classSilver, "", "pv-handle6-2", deletionPolicy, nil, nil, false, false),
initialSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, true),
expectedSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "snapcontent-snapuid6-2", &False, nil, nil, nil, false, true),
initialSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, true, nil),
expectedSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "snapcontent-snapuid6-2", &False, nil, nil, nil, false, true, nil),
initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
initialVolumes: newVolumeArray("volume6-2", "pv-uid6-2", "pv-handle6-2", "1Gi", "pvc-uid6-2", "claim6-2", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
errors: noerrors,
Expand All @@ -93,8 +93,8 @@ func TestCreateSnapshotSync(t *testing.T) {
name: "7-1 - fail to create snapshot with non-existing snapshot class",
initialContents: nocontents,
expectedContents: nocontents,
initialSnapshots: newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, nil, false, true),
expectedSnapshots: newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-1: \"failed to retrieve snapshot class non-existing from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"non-existing\\\\\\\" not found\\\"\""), false, true),
initialSnapshots: newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, nil, false, true, nil),
expectedSnapshots: newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-1: \"failed to retrieve snapshot class non-existing from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"non-existing\\\\\\\" not found\\\"\""), false, true, nil),
initialClaims: newClaimArray("claim7-1", "pvc-uid7-1", "1Gi", "volume7-1", v1.ClaimBound, &classEmpty),
initialVolumes: newVolumeArray("volume7-1", "pv-uid7-1", "pv-handle7-1", "1Gi", "pvc-uid7-1", "claim7-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
expectedEvents: []string{"Warning SnapshotContentCreationFailed"},
Expand All @@ -107,8 +107,8 @@ func TestCreateSnapshotSync(t *testing.T) {
name: "7-2 - fail to update snapshot reports warning event",
initialContents: newContentArrayWithReadyToUse("snapcontent-snapuid7-2", "snapuid7-2", "snap7-2", "sid7-2", classGold, "sid7-2", "pv-handle7-2", deletionPolicy, nil, nil, &True, false),
expectedContents: newContentArrayWithReadyToUse("snapcontent-snapuid7-2", "snapuid7-2", "snap7-2", "sid7-2", classGold, "sid7-2", "pv-handle7-2", deletionPolicy, nil, nil, &True, false),
initialSnapshots: newSnapshotArray("snap7-2", "snapuid7-2", "claim7-2", "", classGold, "snapcontent-snapuid7-2", &False, nil, nil, nil, false, true),
expectedSnapshots: newSnapshotArray("snap7-2", "snapuid7-2", "claim7-2", "", classGold, "snapcontent-snapuid7-2", &False, nil, nil, newVolumeError("Snapshot status update failed, snapshot controller failed to update default/snap7-2 on API server: mock update error"), false, true),
initialSnapshots: newSnapshotArray("snap7-2", "snapuid7-2", "claim7-2", "", classGold, "snapcontent-snapuid7-2", &False, nil, nil, nil, false, true, nil),
expectedSnapshots: newSnapshotArray("snap7-2", "snapuid7-2", "claim7-2", "", classGold, "snapcontent-snapuid7-2", &False, nil, nil, newVolumeError("Snapshot status update failed, snapshot controller failed to update default/snap7-2 on API server: mock update error"), false, true, nil),
initialClaims: newClaimArray("claim7-2", "pvc-uid7-2", "1Gi", "volume7-2", v1.ClaimBound, &classGold),
initialVolumes: newVolumeArray("volume7-2", "pv-uid7-2", "pv-handle7-2", "1Gi", "pvc-uid7-2", "claim7-2", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classGold),
expectedEvents: []string{"Warning SnapshotStatusUpdateFailed"},
Expand Down Expand Up @@ -138,8 +138,8 @@ func TestCreateSnapshotSync(t *testing.T) {
name: "7-4 - fail create snapshot with no-existing claim",
initialContents: nocontents,
expectedContents: nocontents,
initialSnapshots: newSnapshotArray("snap7-4", "snapuid7-4", "claim7-4", "", classGold, "", &False, nil, nil, nil, false, true),
expectedSnapshots: newSnapshotArray("snap7-4", "snapuid7-4", "claim7-4", "", classGold, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error snapshot controller failed to update snap7-4 on API server: cannot get claim from snapshot"), false, true),
initialSnapshots: newSnapshotArray("snap7-4", "snapuid7-4", "claim7-4", "", classGold, "", &False, nil, nil, nil, false, true, nil),
expectedSnapshots: newSnapshotArray("snap7-4", "snapuid7-4", "claim7-4", "", classGold, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error snapshot controller failed to update snap7-4 on API server: cannot get claim from snapshot"), false, true, nil),
initialVolumes: newVolumeArray("volume7-4", "pv-uid7-4", "pv-handle7-4", "1Gi", "pvc-uid7-4", "claim7-4", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
expectedEvents: []string{"Warning SnapshotContentCreationFailed"},
errors: noerrors,
Expand All @@ -150,8 +150,8 @@ func TestCreateSnapshotSync(t *testing.T) {
name: "7-5 - fail create snapshot with no-existing volume",
initialContents: nocontents,
expectedContents: nocontents,
initialSnapshots: newSnapshotArray("snap7-5", "snapuid7-5", "claim7-5", "", classGold, "", &False, nil, nil, nil, false, true),
expectedSnapshots: newSnapshotArray("snap7-5", "snapuid7-5", "claim7-5", "", classGold, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-5: \"failed to retrieve PV volume7-5 from the API server: \\\"cannot find volume volume7-5\\\"\""), false, true),
initialSnapshots: newSnapshotArray("snap7-5", "snapuid7-5", "claim7-5", "", classGold, "", &False, nil, nil, nil, false, true, nil),
expectedSnapshots: newSnapshotArray("snap7-5", "snapuid7-5", "claim7-5", "", classGold, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-5: \"failed to retrieve PV volume7-5 from the API server: \\\"cannot find volume volume7-5\\\"\""), false, true, nil),
initialClaims: newClaimArray("claim7-5", "pvc-uid7-5", "1Gi", "volume7-5", v1.ClaimBound, &classEmpty),
expectedEvents: []string{"Warning SnapshotContentCreationFailed"},
errors: noerrors,
Expand All @@ -163,8 +163,8 @@ func TestCreateSnapshotSync(t *testing.T) {
name: "7-6 - fail create snapshot with claim that is not yet bound",
initialContents: nocontents,
expectedContents: nocontents,
initialSnapshots: newSnapshotArray("snap7-6", "snapuid7-6", "claim7-6", "", classGold, "", &False, nil, nil, nil, false, true),
expectedSnapshots: newSnapshotArray("snap7-6", "snapuid7-6", "claim7-6", "", classGold, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-6: \"the PVC claim7-6 is not yet bound to a PV, will not attempt to take a snapshot\""), false, true),
initialSnapshots: newSnapshotArray("snap7-6", "snapuid7-6", "claim7-6", "", classGold, "", &False, nil, nil, nil, false, true, nil),
expectedSnapshots: newSnapshotArray("snap7-6", "snapuid7-6", "claim7-6", "", classGold, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-6: \"the PVC claim7-6 is not yet bound to a PV, will not attempt to take a snapshot\""), false, true, nil),
initialClaims: newClaimArray("claim7-6", "pvc-uid7-6", "1Gi", "", v1.ClaimPending, &classEmpty),
expectedEvents: []string{"Warning SnapshotContentCreationFailed"},
errors: noerrors,
Expand Down
Loading

0 comments on commit 8c40671

Please sign in to comment.