Only delete unused backup if they are complete

Fixes #705

Signed-off-by: Carlisia <carlisia@grokkingtech.io>

pkg/controller/backup_sync_controller.go

@@ -30,6 +30,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/cache"
 
+	api "github.com/heptio/ark/pkg/apis/ark/v1"
 	"github.com/heptio/ark/pkg/cloudprovider"
 	arkv1client "github.com/heptio/ark/pkg/generated/clientset/versioned/typed/ark/v1"
 	informers "github.com/heptio/ark/pkg/generated/informers/externalversions/ark/v1"
@@ -130,7 +131,8 @@ func (c *backupSyncController) run() {
 	return
 }
 
-// deleteUnused deletes backup objects from Kubernetes if there is no corresponding backup in the object storage.
+// deleteUnused deletes backup objects from Kubernetes if they are complete
+// and there is no corresponding backup in the object storage.
 func (c *backupSyncController) deleteUnused(cloudBackupNames sets.String) {
 	// Backups objects in Kubernetes
 	backups, err := c.backupLister.Backups(c.namespace).List(labels.Everything())
@@ -141,9 +143,10 @@ func (c *backupSyncController) deleteUnused(cloudBackupNames sets.String) {
 		return
 	}
 
-	// For each backup object in Kubernetes, verify if has a corresponding backup in the object storage. If not, delete it.
+	// For each completed backup object in Kubernetes, delete it if it
+	// does not have a corresponding backup in object storage
 	for _, backup := range backups {
-		if !cloudBackupNames.Has(backup.Name) {
+		if backup.Status.Phase == api.BackupPhaseCompleted && !cloudBackupNames.Has(backup.Name) {
 			if err := c.client.Backups(backup.Namespace).Delete(backup.Name, nil); err != nil {
 				c.logger.WithError(errors.WithStack(err)).Error("Error deleting unused backup from Kubernetes")
 			} else {

pkg/controller/backup_sync_controller_test.go

@@ -212,6 +212,39 @@ func TestDeleteUnused(t *testing.T) {
 			},
 			expectedDeletes: sets.NewString(),
 		},
+		{
+			name:      "no overlapping backups but including backups that are not complete",
+			namespace: "ns-1",
+			cloudBackups: []*v1.Backup{
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-1").Backup,
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-2").Backup,
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-3").Backup,
+			},
+			k8sBackups: []*arktest.TestBackup{
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backupA").WithPhase(v1.BackupPhaseCompleted),
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("Deleting").WithPhase(v1.BackupPhaseDeleting),
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("Failed").WithPhase(v1.BackupPhaseFailed),
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("FailedValidation").WithPhase(v1.BackupPhaseFailedValidation),
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("InProgress").WithPhase(v1.BackupPhaseInProgress),
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("New").WithPhase(v1.BackupPhaseNew),
+			},
+			expectedDeletes: sets.NewString("backupA"),
+		},
+		{
+			name:      "all overlapping backups and all backups that are not complete",
+			namespace: "ns-1",
+			cloudBackups: []*v1.Backup{
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-1").Backup,
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-2").Backup,
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-3").Backup,
+			},
+			k8sBackups: []*arktest.TestBackup{
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-1").WithPhase(v1.BackupPhaseFailed),
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-2").WithPhase(v1.BackupPhaseFailedValidation),
+				arktest.NewTestBackup().WithNamespace("ns-1").WithName("backup-3").WithPhase(v1.BackupPhaseInProgress),
+			},
+			expectedDeletes: sets.NewString(),
+		},
 	}
 
 	for _, test := range tests {