code review feedback and other fixes

Signed-off-by: Steve Kriss <[email protected]>

pkg/cmd/cli/delete/delete.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 Heptio Inc.
+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.

pkg/controller/backup_controller.go

@@ -234,8 +234,10 @@ func (controller *backupController) processBackup(key string) error {
 	// set backup version
 	backup.Status.Version = backupVersion
 
-	// add GC finalizer
-	backup.Finalizers = append(backup.Finalizers, gcFinalizer)
+	// 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 {

pkg/controller/gc_controller.go

@@ -18,13 +18,15 @@ package controller
 
 import (
 	"context"
+	"encoding/json"
 	"time"
 
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/clock"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/cache"
@@ -89,61 +91,92 @@ func NewGCController(
 
 	backupInformer.Informer().AddEventHandler(
 		cache.ResourceEventHandlerFuncs{
-			UpdateFunc: func(old interface{}, new interface{}) {
-				var (
-					backup = new.(*api.Backup)
-					log    = logger.WithField("backup", kube.NamespaceAndName(backup))
-					idx    = -1
-				)
-
-				// we're only interested in backups that have a deletionTimestamp and at
-				// least one finalizer.
-				if backup.DeletionTimestamp == nil || len(backup.Finalizers) == 0 {
-					return
-				}
-				log.Debugf("Backup has finalizers %s", backup.Finalizers)
-
-				for x, finalizer := range backup.Finalizers {
-					if finalizer == gcFinalizer {
-						idx = x
-						break
-					}
-				}
-
-				// GC finalizer not found
-				if idx == -1 {
-					return
-				}
-
-				log.Infof("Garbage-collecting backup")
-				if err := c.garbageCollect(backup, log); err != nil {
-					log.WithError(err).Error("Error deleting backup's related objects")
-					return
-				}
-
-				backup.Finalizers = append(backup.Finalizers[0:idx], backup.Finalizers[idx+1:]...)
-
-				upd, err := backupClient.Backups(api.DefaultNamespace).Update(backup)
-				if err != nil {
-					log.WithError(errors.WithStack(err)).Error("Error updating backup")
-					return
-				}
-
-				// no more finalizers: we don't need to issue another delete
-				if len(upd.Finalizers) == 0 {
-					return
-				}
-
-				if err := backupClient.Backups(api.DefaultNamespace).Delete(upd.Name, &metav1.DeleteOptions{}); err != nil {
-					log.WithError(errors.WithStack(err)).Error("Error deleting backup")
-				}
-			},
+			UpdateFunc: c.handleFinalizer,
 		},
 	)
 
 	return c
 }
 
+// handleFinalizer runs garbage-collection on a backup that has the Ark GC
+// finalizer and a deletionTimestamp.
+func (c *gcController) handleFinalizer(_, newObj interface{}) {
+	var (
+		backup = newObj.(*api.Backup)
+		log    = c.logger.WithField("backup", kube.NamespaceAndName(backup))
+	)
+
+	// we're only interested in backups that have a deletionTimestamp and at
+	// least one finalizer.
+	if backup.DeletionTimestamp == nil || len(backup.Finalizers) == 0 {
+		return
+	}
+	log.Debugf("Backup has finalizers %s", backup.Finalizers)
+
+	// GC finalizer not found
+	if !has(backup.Finalizers, gcFinalizer) {
+		return
+	}
+
+	log.Infof("Garbage-collecting backup")
+	if err := c.garbageCollect(backup, log); err != nil {
+		log.WithError(err).Error("Error deleting backup's related objects")
+		return
+	}
+
+	patchMap := map[string]interface{}{
+		"metadata": map[string]interface{}{
+			"finalizers": except(backup.Finalizers, gcFinalizer),
+		},
+	}
+
+	patchBytes, err := json.Marshal(patchMap)
+	if err != nil {
+		log.WithError(err).Error("Error marshaling finalizers patch")
+	}
+
+	upd, err := c.backupClient.Backups(api.DefaultNamespace).Patch(backup.Name, types.MergePatchType, patchBytes)
+	if err != nil {
+		log.WithError(errors.WithStack(err)).Error("Error patching backup")
+		return
+	}
+
+	// no more finalizers: we don't need to issue another delete
+	if len(upd.Finalizers) == 0 {
+		return
+	}
+
+	if err := c.backupClient.Backups(api.DefaultNamespace).Delete(upd.Name, &metav1.DeleteOptions{}); err != nil {
+		log.WithError(errors.WithStack(err)).Error("Error deleting backup")
+	}
+}
+
+// has returns true if the `items` slice contains the
+// value `val`, or false otherwise.
+func has(items []string, val string) bool {
+	for _, itm := range items {
+		if itm == val {
+			return true
+		}
+	}
+
+	return false
+}
+
+// except returns a new string slice that contains all of the entries
+// from `items` except `val`.
+func except(items []string, val string) []string {
+	var newItems []string
+
+	for _, itm := range items {
+		if itm != val {
+			newItems = append(newItems, itm)
+		}
+	}
+
+	return newItems
+}
+
 // Run is a blocking function that runs a single worker to garbage-collect backups
 // from object/block storage and the Ark API. It will return when it receives on the
 // ctx.Done() channel.