backup delete/describe enhancements

Always request DeleteBackupRequests for a given backup so we can show
failed deletion attempts if you try to delete a backup that has PV
snapshots when Ark doesn't have a persistentVolumeProvider configured.

When creating a DeleteBackupRequest, include a label for the UID so we
can match based on name and UID when associated DeleteBackupRequests
with a given backup.

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

pkg/apis/ark/v1/delete_backup_request.go

@@ -34,7 +34,10 @@ const (
 	// DeleteBackupRequestPhaseProcessed means the DeleteBackupRequest has been processed.
 	DeleteBackupRequestPhaseProcessed DeleteBackupRequestPhase = "Processed"
 
+	// BackupNameLabel is the label key used by a DeleteBackupRequest to identify its backup by name.
 	BackupNameLabel = "ark.heptio.com/backup-name"
+	// BackupUIDLabel is the label key used by a DeleteBackupRequest to identify its backup by uid.
+	BackupUIDLabel = "ark.heptio.com/backup-uid"
 )
 
 // DeleteBackupRequestStatus is the current status of a DeleteBackupRequest.

pkg/backup/delete_helpers.go

@@ -23,13 +23,14 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-// NewDeleteBackupRequest creates a DeleteBackupRequest for the backup identified by name.
-func NewDeleteBackupRequest(name string) *v1.DeleteBackupRequest {
+// NewDeleteBackupRequest creates a DeleteBackupRequest for the backup identified by name and uid.
+func NewDeleteBackupRequest(name string, uid string) *v1.DeleteBackupRequest {
 	return &v1.DeleteBackupRequest{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: name + "-",
 			Labels: map[string]string{
 				v1.BackupNameLabel: name,
+				v1.BackupUIDLabel:  uid,
 			},
 		},
 		Spec: v1.DeleteBackupRequestSpec{
@@ -39,9 +40,9 @@ func NewDeleteBackupRequest(name string) *v1.DeleteBackupRequest {
 }
 
 // NewDeleteBackupRequestListOptions creates a ListOptions with a label selector configured to
-// find DeleteBackupRequests for the backup identified by name.
-func NewDeleteBackupRequestListOptions(name string) metav1.ListOptions {
+// find DeleteBackupRequests for the backup identified by name and uid.
+func NewDeleteBackupRequestListOptions(name, uid string) metav1.ListOptions {
 	return metav1.ListOptions{
-		LabelSelector: fmt.Sprintf("%s=%s", v1.BackupNameLabel, name),
+		LabelSelector: fmt.Sprintf("%s=%s,%s=%s", v1.BackupNameLabel, name, v1.BackupUIDLabel, uid),
 	}
 }

pkg/cmd/cli/backup/delete.go

@@ -23,6 +23,7 @@ import (
 	"os"
 	"strings"
 
+	"github.com/heptio/ark/pkg/apis/ark/v1"
 	"github.com/heptio/ark/pkg/backup"
 	clientset "github.com/heptio/ark/pkg/generated/clientset/versioned"
 	"github.com/spf13/cobra"
@@ -33,6 +34,7 @@ import (
 	"github.com/heptio/ark/pkg/cmd"
 )
 
+// NewDeleteCommand creates a new command that deletes a backup.
 func NewDeleteCommand(f client.Factory, use string) *cobra.Command {
 	o := &DeleteOptions{}
 
@@ -52,31 +54,22 @@ func NewDeleteCommand(f client.Factory, use string) *cobra.Command {
 	return c
 }
 
+// DeleteOptions contains parameters for deleting a backup.
 type DeleteOptions struct {
 	Name    string
 	Confirm bool
 
 	client    clientset.Interface
 	namespace string
+	backup    *v1.Backup
 }
 
+// BindFlags binds options for this command to flags.
 func (o *DeleteOptions) BindFlags(flags *pflag.FlagSet) {
 	flags.BoolVar(&o.Confirm, "confirm", o.Confirm, "Confirm deletion")
 }
 
-func (o *DeleteOptions) Validate(c *cobra.Command, args []string, f client.Factory) error {
-	if o.client == nil {
-		return errors.New("Ark client is not set; unable to proceed")
-	}
-
-	_, err := o.client.ArkV1().Backups(f.Namespace()).Get(args[0], metav1.GetOptions{})
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
+// Complete fills out the remainder of the parameters based on user input.
 func (o *DeleteOptions) Complete(f client.Factory, args []string) error {
 	o.Name = args[0]
 
@@ -88,22 +81,42 @@ func (o *DeleteOptions) Complete(f client.Factory, args []string) error {
 	}
 	o.client = client
 
+	backup, err := o.client.ArkV1().Backups(f.Namespace()).Get(o.Name, metav1.GetOptions{})
+	if err != nil {
+		return err
+	}
+	o.backup = backup
+
 	return nil
 }
 
+// Validate ensures all of the parameters have been filled in correctly.
+func (o *DeleteOptions) Validate(c *cobra.Command, args []string, f client.Factory) error {
+	if o.client == nil {
+		return errors.New("Ark client is not set; unable to proceed")
+	}
+
+	if o.backup == nil {
+		return errors.New("backup is not set; unable to proceed")
+	}
+
+	return nil
+}
+
+// Run performs the delete backup operation.
 func (o *DeleteOptions) Run() error {
 	if !o.Confirm && !getConfirmation() {
 		// Don't do anything unless we get confirmation
 		return nil
 	}
 
-	deleteRequest := backup.NewDeleteBackupRequest(o.Name)
+	deleteRequest := backup.NewDeleteBackupRequest(o.backup.Name, string(o.backup.UID))
 
 	if _, err := o.client.ArkV1().DeleteBackupRequests(o.namespace).Create(deleteRequest); err != nil {
 		return err
 	}
 
-	fmt.Printf("Request to delete backup %q submitted successfully.\nThe backup will be fully deleted after all associated data (disk snapshots, backup files, restores) are removed.\n", o.Name)
+	fmt.Printf("Request to delete backup %q submitted successfully.\nThe backup will be fully deleted after all associated data (disk snapshots, backup files, restores) are removed.\n", o.backup.Name)
 	return nil
 }
 

pkg/cmd/cli/backup/describe.go

@@ -55,17 +55,13 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command {
 
 			first := true
 			for _, backup := range backups.Items {
-				var deleteRequests []v1.DeleteBackupRequest
-				if backup.Status.Phase == v1.BackupPhaseDeleting {
-					deleteRequestListOptions := pkgbackup.NewDeleteBackupRequestListOptions(backup.Name)
-					deleteRequestList, err := arkClient.ArkV1().DeleteBackupRequests(f.Namespace()).List(deleteRequestListOptions)
-					if err != nil {
-						fmt.Fprintf(os.Stderr, "error getting DeleteBackupRequests for backup %s: %v\n", backup.Name, err)
-					}
-					deleteRequests = deleteRequestList.Items
+				deleteRequestListOptions := pkgbackup.NewDeleteBackupRequestListOptions(backup.Name, string(backup.UID))
+				deleteRequestList, err := arkClient.ArkV1().DeleteBackupRequests(f.Namespace()).List(deleteRequestListOptions)
+				if err != nil {
+					fmt.Fprintf(os.Stderr, "error getting DeleteBackupRequests for backup %s: %v\n", backup.Name, err)
 				}
 
-				s := output.DescribeBackup(&backup, deleteRequests)
+				s := output.DescribeBackup(&backup, deleteRequestList.Items)
 				if first {
 					first = false
 					fmt.Print(s)

pkg/cmd/util/output/backup_describer.go

@@ -34,11 +34,7 @@ func DescribeBackup(backup *v1.Backup, deleteRequests []v1.DeleteBackupRequest)
 		if phase == "" {
 			phase = v1.BackupPhaseNew
 		}
-		d.Printf("Phase:\t%s", phase)
-		if count := failedDeletionCount(deleteRequests); count > 0 {
-			d.Printf(" (%d failed attempt(s))", count)
-		}
-		d.Println()
+		d.Printf("Phase:\t%s\n", phase)
 
 		d.Println()
 		DescribeBackupSpec(d, backup.Spec)
@@ -199,7 +195,11 @@ func DescribeBackupStatus(d *Describer, status v1.BackupStatus) {
 
 // DescribeDeleteBackupRequests describes delete backup requests in human-readable format.
 func DescribeDeleteBackupRequests(d *Describer, requests []v1.DeleteBackupRequest) {
-	d.Printf("Deletion Attempts:\n")
+	d.Printf("Deletion Attempts")
+	if count := failedDeletionCount(requests); count > 0 {
+		d.Printf(" (%d failed)", count)
+	}
+	d.Println(":")
 
 	started := false
 	for _, req := range requests {

pkg/controller/backup_deletion_controller.go

@@ -232,7 +232,7 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e
 
 	// Everything deleted correctly, so we can delete all DeleteBackupRequests for this backup
 	if len(errs) == 0 {
-		listOptions := pkgbackup.NewDeleteBackupRequestListOptions(backup.Name)
+		listOptions := pkgbackup.NewDeleteBackupRequestListOptions(backup.Name, string(backup.UID))
 		err = c.deleteBackupRequestClient.DeleteBackupRequests(req.Namespace).DeleteCollection(nil, listOptions)
 		if err != nil {
 			// If this errors, all we can do is log it.

pkg/controller/backup_deletion_controller_test.go

@@ -42,7 +42,7 @@ import (
 )
 
 func TestBackupDeletionControllerControllerHasUpdateFunc(t *testing.T) {
-	req := pkgbackup.NewDeleteBackupRequest("foo")
+	req := pkgbackup.NewDeleteBackupRequest("foo", "uid")
 	req.Namespace = "heptio-ark"
 	expected := kube.NamespaceAndName(req)
 
@@ -127,7 +127,7 @@ func TestBackupDeletionControllerProcessQueueItem(t *testing.T) {
 	assert.NoError(t, err)
 
 	// Already processed
-	req := pkgbackup.NewDeleteBackupRequest("foo")
+	req := pkgbackup.NewDeleteBackupRequest("foo", "uid")
 	req.Namespace = "foo"
 	req.Name = "foo-abcde"
 	req.Status.Phase = v1.DeleteBackupRequestPhaseProcessed
@@ -179,7 +179,7 @@ func setupBackupDeletionControllerTest(objects ...runtime.Object) *backupDeletio
 	sharedInformers := informers.NewSharedInformerFactory(client, 0)
 	backupService := &arktest.BackupService{}
 	snapshotService := &arktest.FakeSnapshotService{SnapshotsTaken: sets.NewString()}
-	req := pkgbackup.NewDeleteBackupRequest("foo")
+	req := pkgbackup.NewDeleteBackupRequest("foo", "uid")
 
 	data := &backupDeletionControllerTestData{
 		client:          client,
@@ -299,6 +299,7 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) {
 
 	t.Run("full delete, no errors", func(t *testing.T) {
 		backup := arktest.NewTestBackup().WithName("foo").WithSnapshot("pv-1", "snap-1").Backup
+		backup.UID = "uid"
 
 		restore1 := arktest.NewTestRestore("heptio-ark", "restore-1", v1.RestorePhaseCompleted).WithBackup("foo").Restore
 		restore2 := arktest.NewTestRestore("heptio-ark", "restore-2", v1.RestorePhaseCompleted).WithBackup("foo").Restore
@@ -372,7 +373,7 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) {
 			core.NewDeleteCollectionAction(
 				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
 				td.req.Namespace,
-				pkgbackup.NewDeleteBackupRequestListOptions(td.req.Spec.BackupName),
+				pkgbackup.NewDeleteBackupRequestListOptions(td.req.Spec.BackupName, "uid"),
 			),
 		}
 

pkg/controller/gc_controller.go

@@ -132,7 +132,7 @@ func (c *gcController) processQueueItem(key string) error {
 
 	log.Info("Backup has expired. Creating a DeleteBackupRequest.")
 
-	req := pkgbackup.NewDeleteBackupRequest(name)
+	req := pkgbackup.NewDeleteBackupRequest(backup.Name, string(backup.UID))
 
 	_, err = c.deleteBackupRequestClient.DeleteBackupRequests(ns).Create(req)
 	if err != nil {