add delete backup cmd using finalizer and simplify GC process

Signed-off-by: Steve Kriss <steve@heptio.com>

docs/cli-reference/ark.md

@@ -30,6 +30,7 @@ operations can also be performed as 'ark backup get' and 'ark schedule create'.
 ### SEE ALSO
 * [ark backup](ark_backup.md)	 - Work with backups
 * [ark create](ark_create.md)	 - Create ark resources
+* [ark delete](ark_delete.md)	 - Delete ark resources
 * [ark describe](ark_describe.md)	 - Describe ark resources
 * [ark get](ark_get.md)	 - Get ark resources
 * [ark plugin](ark_plugin.md)	 - Work with plugins

docs/cli-reference/ark_backup.md

@@ -29,6 +29,7 @@ Work with backups
 ### SEE ALSO
 * [ark](ark.md)	 - Back up and restore Kubernetes cluster resources.
 * [ark backup create](ark_backup_create.md)	 - Create a backup
+* [ark backup delete](ark_backup_delete.md)	 - Delete a backup
 * [ark backup describe](ark_backup_describe.md)	 - Describe backups
 * [ark backup download](ark_backup_download.md)	 - Download a backup
 * [ark backup get](ark_backup_get.md)	 - Get backups

docs/cli-reference/ark_backup_delete.md

@@ -0,0 +1,35 @@
+## ark backup delete
+
+Delete a backup
+
+### Synopsis
+
+
+Delete a backup
+
+```
+ark backup delete NAME [flags]
+```
+
+### Options
+
+```
+  -h, --help   help for delete
+```
+
+### Options inherited from parent commands
+
+```
+      --alsologtostderr                  log to standard error as well as files
+      --kubeconfig string                Path to the kubeconfig file to use to talk to the Kubernetes apiserver. If unset, try the environment variable KUBECONFIG, as well as in-cluster configuration
+      --log_backtrace_at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
+      --log_dir string                   If non-empty, write log files in this directory
+      --logtostderr                      log to standard error instead of files
+      --stderrthreshold severity         logs at or above this threshold go to stderr (default 2)
+  -v, --v Level                          log level for V logs
+      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging
+```
+
+### SEE ALSO
+* [ark backup](ark_backup.md)	 - Work with backups
+

docs/cli-reference/ark_delete.md

@@ -0,0 +1,32 @@
+## ark delete
+
+Delete ark resources
+
+### Synopsis
+
+
+Delete ark resources
+
+### Options
+
+```
+  -h, --help   help for delete
+```
+
+### Options inherited from parent commands
+
+```
+      --alsologtostderr                  log to standard error as well as files
+      --kubeconfig string                Path to the kubeconfig file to use to talk to the Kubernetes apiserver. If unset, try the environment variable KUBECONFIG, as well as in-cluster configuration
+      --log_backtrace_at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
+      --log_dir string                   If non-empty, write log files in this directory
+      --logtostderr                      log to standard error instead of files
+      --stderrthreshold severity         logs at or above this threshold go to stderr (default 2)
+  -v, --v Level                          log level for V logs
+      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging
+```
+
+### SEE ALSO
+* [ark](ark.md)	 - Back up and restore Kubernetes cluster resources.
+* [ark delete backup](ark_delete_backup.md)	 - Delete a backup
+

docs/cli-reference/ark_delete_backup.md

@@ -0,0 +1,35 @@
+## ark delete backup
+
+Delete a backup
+
+### Synopsis
+
+
+Delete a backup
+
+```
+ark delete backup NAME [flags]
+```
+
+### Options
+
+```
+  -h, --help   help for backup
+```
+
+### Options inherited from parent commands
+
+```
+      --alsologtostderr                  log to standard error as well as files
+      --kubeconfig string                Path to the kubeconfig file to use to talk to the Kubernetes apiserver. If unset, try the environment variable KUBECONFIG, as well as in-cluster configuration
+      --log_backtrace_at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
+      --log_dir string                   If non-empty, write log files in this directory
+      --logtostderr                      log to standard error instead of files
+      --stderrthreshold severity         logs at or above this threshold go to stderr (default 2)
+  -v, --v Level                          log level for V logs
+      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging
+```
+
+### SEE ALSO
+* [ark delete](ark_delete.md)	 - Delete ark resources
+

pkg/cmd/ark/ark.go

@@ -24,6 +24,7 @@ import (
 	"github.com/heptio/ark/pkg/client"
 	"github.com/heptio/ark/pkg/cmd/cli/backup"
 	"github.com/heptio/ark/pkg/cmd/cli/create"
+	"github.com/heptio/ark/pkg/cmd/cli/delete"
 	"github.com/heptio/ark/pkg/cmd/cli/describe"
 	"github.com/heptio/ark/pkg/cmd/cli/get"
 	"github.com/heptio/ark/pkg/cmd/cli/plugin"
@@ -61,6 +62,7 @@ operations can also be performed as 'ark backup get' and 'ark schedule create'.`
 		create.NewCommand(f),
 		runplugin.NewCommand(),
 		plugin.NewCommand(f),
+		delete.NewCommand(f),
 	)
 
 	// add the glog flags

pkg/cmd/cli/backup/backup.go

@@ -35,10 +35,7 @@ func NewCommand(f client.Factory) *cobra.Command {
 		NewLogsCommand(f),
 		NewDescribeCommand(f, "describe"),
 		NewDownloadCommand(f),
-
-		// If you delete a backup and it still exists in object storage, the backup sync controller will
-		// recreate it. Until we have a good UX around this, we're disabling the delete command.
-		// NewDeleteCommand(f),
+		NewDeleteCommand(f, "delete"),
 	)
 
 	return c

pkg/cmd/cli/backup/delete.go

@@ -27,9 +27,9 @@ import (
 	"github.com/heptio/ark/pkg/cmd"
 )
 
-func NewDeleteCommand(f client.Factory) *cobra.Command {
+func NewDeleteCommand(f client.Factory, use string) *cobra.Command {
 	c := &cobra.Command{
-		Use:   "delete NAME",
+		Use:   fmt.Sprintf("%s NAME", use),
 		Short: "Delete a backup",
 		Run: func(c *cobra.Command, args []string) {
 			if len(args) != 1 {

pkg/cmd/cli/delete/delete.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2017 the Heptio Ark contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package delete
+
+import (
+	"github.com/spf13/cobra"
+
+	"github.com/heptio/ark/pkg/client"
+	"github.com/heptio/ark/pkg/cmd/cli/backup"
+)
+
+func NewCommand(f client.Factory) *cobra.Command {
+	c := &cobra.Command{
+		Use:   "delete",
+		Short: "Delete ark resources",
+		Long:  "Delete ark resources",
+	}
+
+	backupCommand := backup.NewDeleteCommand(f, "backup")
+	backupCommand.Aliases = []string{"backups"}
+
+	c.AddCommand(
+		backupCommand,
+	)
+
+	return c
+}

pkg/controller/backup_controller.go

@@ -234,6 +234,11 @@ func (controller *backupController) processBackup(key string) error {
 	// set backup version
 	backup.Status.Version = backupVersion
 
+	// add GC finalizer if it's not there already
+	if !has(backup.Finalizers, gcFinalizer) {
+		backup.Finalizers = append(backup.Finalizers, gcFinalizer)
+	}
+
 	// calculate expiration
 	if backup.Spec.TTL.Duration > 0 {
 		backup.Status.Expiration = metav1.NewTime(controller.clock.Now().Add(backup.Spec.TTL.Duration))

pkg/controller/backup_controller_test.go

@@ -190,6 +190,7 @@ func TestProcessBackup(t *testing.T) {
 				backup.Status.Phase = v1.BackupPhaseInProgress
 				backup.Status.Expiration.Time = expiration
 				backup.Status.Version = 1
+				backup.Finalizers = []string{gcFinalizer}
 				backupper.On("Backup", backup, mock.Anything, mock.Anything, mock.Anything).Return(nil)
 
 				cloudBackups.On("UploadBackup", "bucket", backup.Name, mock.Anything, mock.Anything, mock.Anything).Return(nil)
@@ -225,6 +226,7 @@ func TestProcessBackup(t *testing.T) {
 				res.Status.Version = 1
 				res.Status.Expiration.Time = expiration
 				res.Status.Phase = v1.BackupPhase(phase)
+				res.Finalizers = []string{gcFinalizer}
 
 				return true, res, nil
 			})
@@ -247,14 +249,15 @@ func TestProcessBackup(t *testing.T) {
 			actions := client.Actions()
 			require.Equal(t, 2, len(actions))
 
-			// validate Patch call 1 (setting version, expiration, and phase)
+			// validate Patch call 1 (setting finalizer, version, expiration, and phase)
 			patchAction, ok := actions[0].(core.PatchAction)
 			require.True(t, ok, "action is not a PatchAction")
 
 			patch := make(map[string]interface{})
 			require.NoError(t, json.Unmarshal(patchAction.GetPatch(), &patch), "cannot unmarshal patch")
 
-			assert.Equal(t, 1, len(patch), "patch has wrong number of keys")
+			// should have metadata and status
+			assert.Equal(t, 2, len(patch), "patch has wrong number of keys")
 
 			expectedStatusKeys := 2
 			if test.backup.Spec.TTL.Duration > 0 {
@@ -268,10 +271,19 @@ func TestProcessBackup(t *testing.T) {
 			res, _ := collections.GetMap(patch, "status")
 			assert.Equal(t, expectedStatusKeys, len(res), "patch's status has the wrong number of keys")
 
+			finalizers, err := collections.GetSlice(patch, "metadata.finalizers")
+			require.NoError(t, err, "patch does not contain metadata.finalizers")
+			assert.Equal(t, 1, len(finalizers))
+			assert.Equal(t, gcFinalizer, finalizers[0])
+
+			res, _ = collections.GetMap(patch, "metadata")
+			assert.Equal(t, 1, len(res), "patch's metadata has the wrong number of keys")
+
 			// validate Patch call 2 (setting phase)
 			patchAction, ok = actions[1].(core.PatchAction)
 			require.True(t, ok, "action is not a PatchAction")
 
+			patch = make(map[string]interface{})
 			require.NoError(t, json.Unmarshal(patchAction.GetPatch(), &patch), "cannot unmarshal patch")
 
 			assert.Equal(t, 1, len(patch), "patch has wrong number of keys")