From b2ec87f05f0c1fa9e153788df1d203b8256e602d Mon Sep 17 00:00:00 2001
From: Calle Pettersson <cpettsson@gmail.com>
Date: Tue, 22 May 2018 19:03:20 +0200
Subject: [PATCH] Run post-hooks even if backup actions fail

Signed-off-by: Calle Pettersson <cpettsson@gmail.com>
---
 pkg/backup/item_backupper.go      | 102 +++++++++++++++++-------------
 pkg/backup/item_backupper_test.go |   5 +-
 2 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go
index 28b6fbe7d..8c907a67a 100644
--- a/pkg/backup/item_backupper.go
+++ b/pkg/backup/item_backupper.go
@@ -31,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	kuberrs "k8s.io/apimachinery/pkg/util/errors"
 
 	api "github.com/heptio/ark/pkg/apis/ark/v1"
 	"github.com/heptio/ark/pkg/client"
@@ -165,6 +166,64 @@ func (ib *defaultItemBackupper) backupItem(logger logrus.FieldLogger, obj runtim
 		return err
 	}
 
+	backupErrs := make([]error, 0)
+	err = ib.executeActions(log, obj, groupResource, name, namespace, metadata)
+	if err != nil {
+		log.WithError(err).Error("Error executing item actions")
+		backupErrs = append(backupErrs, err)
+	}
+
+	if groupResource == kuberesource.PersistentVolumes {
+		if ib.snapshotService == nil {
+			log.Debug("Skipping Persistent Volume snapshot because they're not enabled.")
+		} else {
+			if err := ib.takePVSnapshot(obj, ib.backup, log); err != nil {
+				backupErrs = append(backupErrs, err)
+			}
+		}
+	}
+
+	log.Debug("Executing post hooks")
+	if err := ib.itemHookHandler.handleHooks(log, groupResource, obj, ib.resourceHooks, hookPhasePost); err != nil {
+		backupErrs = append(backupErrs, err)
+	}
+
+	if len(backupErrs) != 0 {
+		return kuberrs.NewAggregate(backupErrs)
+	}
+
+	var filePath string
+	if namespace != "" {
+		filePath = filepath.Join(api.ResourcesDir, groupResource.String(), api.NamespaceScopedDir, namespace, name+".json")
+	} else {
+		filePath = filepath.Join(api.ResourcesDir, groupResource.String(), api.ClusterScopedDir, name+".json")
+	}
+
+	itemBytes, err := json.Marshal(obj.UnstructuredContent())
+	if err != nil {
+		return errors.WithStack(err)
+	}
+
+	hdr := &tar.Header{
+		Name:     filePath,
+		Size:     int64(len(itemBytes)),
+		Typeflag: tar.TypeReg,
+		Mode:     0755,
+		ModTime:  time.Now(),
+	}
+
+	if err := ib.tarWriter.WriteHeader(hdr); err != nil {
+		return errors.WithStack(err)
+	}
+
+	if _, err := ib.tarWriter.Write(itemBytes); err != nil {
+		return errors.WithStack(err)
+	}
+
+	return nil
+}
+
+func (ib *defaultItemBackupper) executeActions(log logrus.FieldLogger, obj runtime.Unstructured, groupResource schema.GroupResource, name, namespace string, metadata metav1.Object) error {
 	for _, action := range ib.actions {
 		if !action.resourceIncludesExcludes.ShouldInclude(groupResource.String()) {
 			log.Debug("Skipping action because it does not apply to this resource")
@@ -220,49 +279,6 @@ func (ib *defaultItemBackupper) backupItem(logger logrus.FieldLogger, obj runtim
 		}
 	}
 
-	if groupResource == kuberesource.PersistentVolumes {
-		if ib.snapshotService == nil {
-			log.Debug("Skipping Persistent Volume snapshot because they're not enabled.")
-		} else {
-			if err := ib.takePVSnapshot(obj, ib.backup, log); err != nil {
-				return err
-			}
-		}
-	}
-
-	log.Debug("Executing post hooks")
-	if err := ib.itemHookHandler.handleHooks(log, groupResource, obj, ib.resourceHooks, hookPhasePost); err != nil {
-		return err
-	}
-
-	var filePath string
-	if namespace != "" {
-		filePath = filepath.Join(api.ResourcesDir, groupResource.String(), api.NamespaceScopedDir, namespace, name+".json")
-	} else {
-		filePath = filepath.Join(api.ResourcesDir, groupResource.String(), api.ClusterScopedDir, name+".json")
-	}
-
-	itemBytes, err := json.Marshal(obj.UnstructuredContent())
-	if err != nil {
-		return errors.WithStack(err)
-	}
-
-	hdr := &tar.Header{
-		Name:     filePath,
-		Size:     int64(len(itemBytes)),
-		Typeflag: tar.TypeReg,
-		Mode:     0755,
-		ModTime:  time.Now(),
-	}
-
-	if err := ib.tarWriter.WriteHeader(hdr); err != nil {
-		return errors.WithStack(err)
-	}
-
-	if _, err := ib.tarWriter.Write(itemBytes); err != nil {
-		return errors.WithStack(err)
-	}
-
 	return nil
 }
 
diff --git a/pkg/backup/item_backupper_test.go b/pkg/backup/item_backupper_test.go
index 7d825f510..a172c83e9 100644
--- a/pkg/backup/item_backupper_test.go
+++ b/pkg/backup/item_backupper_test.go
@@ -378,10 +378,7 @@ func TestBackupItemNoSkips(t *testing.T) {
 
 			obj := &unstructured.Unstructured{Object: item}
 			itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, resourceHooks, hookPhasePre).Return(nil)
-			if test.snapshotError == nil && test.additionalItemError == nil {
-				// TODO: Remove if-clause when #511 is resolved.
-				itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, resourceHooks, hookPhasePost).Return(nil)
-			}
+			itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, resourceHooks, hookPhasePost).Return(nil)
 
 			for i, item := range test.customActionAdditionalItemIdentifiers {
 				if test.additionalItemError != nil && i > 0 {