Add hook status to backup/restore describer

Signed-off-by: allenxu404 <[email protected]>

internal/hook/item_hook_handler.go

@@ -48,6 +48,8 @@ type hookPhase string
 const (
 	PhasePre  hookPhase = "pre"
 	PhasePost hookPhase = "post"
+	// HookErrLogPrefix is prepended to each error message logged from a hook, which helps distinguish hook error logs from other logs.
+	HookErrLogPrefix string = "hook error:"
 )
 
 const (
@@ -229,6 +231,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
 			},
 		)
 		if err := h.PodCommandExecutor.ExecutePodCommand(hookLog, obj.UnstructuredContent(), namespace, name, "<from-annotation>", hookFromAnnotations); err != nil {
+			err = fmt.Errorf("%s %v", HookErrLogPrefix, err.Error())
 			hookLog.WithError(err).Error("Error executing hook")
 			if hookFromAnnotations.OnError == velerov1api.HookErrorModeFail {
 				return err
@@ -263,6 +266,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
 					)
 					err := h.PodCommandExecutor.ExecutePodCommand(hookLog, obj.UnstructuredContent(), namespace, name, resourceHook.Name, hook.Exec)
 					if err != nil {
+						err = fmt.Errorf("%s %v", HookErrLogPrefix, err.Error())
 						hookLog.WithError(err).Error("Error executing hook")
 						if hook.Exec.OnError == velerov1api.HookErrorModeFail {
 							return err

pkg/cmd/util/output/backup_describer.go

@@ -97,11 +97,13 @@ func DescribeBackup(
 			}
 		}
 
+		resultMap, resultErr := downloadResult(ctx, kbClient, backup.Namespace, backup.Name, velerov1api.DownloadTargetKindBackupResults, downloadRequestTimeout, insecureSkipTLSVerify, caCertFile)
+
 		d.Println()
-		DescribeBackupResults(ctx, kbClient, d, backup, insecureSkipTLSVerify, caCertFile)
+		DescribeBackupResults(ctx, kbClient, d, backup, resultMap, resultErr)
 
 		d.Println()
-		DescribeBackupSpec(d, backup.Spec)
+		DescribeBackupSpec(d, backup.Spec, resultMap)
 
 		d.Println()
 		DescribeBackupStatus(ctx, kbClient, d, backup, details, insecureSkipTLSVerify, caCertFile)
@@ -131,7 +133,7 @@ func DescribeResourcePolicies(d *Describer, resPolicies *v1.TypedLocalObjectRefe
 }
 
 // DescribeBackupSpec describes a backup spec in human-readable format.
-func DescribeBackupSpec(d *Describer, spec velerov1api.BackupSpec) {
+func DescribeBackupSpec(d *Describer, spec velerov1api.BackupSpec, resultMap map[string]results.Result) {
 	// TODO make a helper for this and use it in all the describers.
 	d.Printf("Namespaces:\n")
 	var s string
@@ -232,6 +234,7 @@ func DescribeBackupSpec(d *Describer, spec velerov1api.BackupSpec) {
 	d.Printf("ItemOperationTimeout:\t%s\n", spec.ItemOperationTimeout.Duration)
 
 	d.Println()
+	d.Printf("Hook Failed:\t%s\n", hookFailed(resultMap))
 	if len(spec.Hooks.Resources) == 0 {
 		d.Printf("Hooks:\t" + emptyDisplay + "\n")
 	} else {
@@ -694,28 +697,17 @@ func DescribeVSC(d *Describer, details bool, vsc snapshotv1api.VolumeSnapshotCon
 }
 
 // DescribeBackupResults describes errors and warnings in human-readable format.
-func DescribeBackupResults(ctx context.Context, kbClient kbclient.Client, d *Describer, backup *velerov1api.Backup, insecureSkipTLSVerify bool, caCertPath string) {
+func DescribeBackupResults(ctx context.Context, kbClient kbclient.Client, d *Describer, backup *velerov1api.Backup, resultMap map[string]results.Result, resultErr error) {
 	if backup.Status.Warnings == 0 && backup.Status.Errors == 0 {
 		return
 	}
 
-	var buf bytes.Buffer
-	var resultMap map[string]results.Result
-
-	// If err 'ErrNotFound' occurs, it means the backup bundle in the bucket has already been there before the backup-result file is introduced.
-	// We only display the count of errors and warnings in this case.
-	err := downloadrequest.Stream(ctx, kbClient, backup.Namespace, backup.Name, velerov1api.DownloadTargetKindBackupResults, &buf, downloadRequestTimeout, insecureSkipTLSVerify, caCertPath)
-	if err == downloadrequest.ErrNotFound {
+	if resultErr == downloadrequest.ErrNotFound {
 		d.Printf("Errors:\t%d\n", backup.Status.Errors)
 		d.Printf("Warnings:\t%d\n", backup.Status.Warnings)
 		return
-	} else if err != nil {
-		d.Printf("Warnings:\t<error getting warnings: %v>\n\nErrors:\t<error getting errors: %v>\n", err, err)
-		return
-	}
-
-	if err := json.NewDecoder(&buf).Decode(&resultMap); err != nil {
-		d.Printf("Warnings:\t<error decoding warnings: %v>\n\nErrors:\t<error decoding errors: %v>\n", err, err)
+	} else if resultErr != nil {
+		d.Printf("Warnings:\t<error getting warnings: %v>\n\nErrors:\t<error getting errors: %v>\n", resultErr, resultErr)
 		return
 	}
 

pkg/cmd/util/output/backup_structured_describer.go

@@ -64,9 +64,11 @@ func DescribeBackupInSF(
 			d.Describe("validationErrors", status.ValidationErrors)
 		}
 
-		DescribeBackupResultsInSF(ctx, kbClient, d, backup, insecureSkipTLSVerify, caCertFile)
+		resultMap, resultErr := downloadResult(ctx, kbClient, backup.Namespace, backup.Name, velerov1api.DownloadTargetKindBackupResults, downloadRequestTimeout, insecureSkipTLSVerify, caCertFile)
 
-		DescribeBackupSpecInSF(d, backup.Spec)
+		DescribeBackupResultsInSF(ctx, kbClient, d, backup, resultMap, resultErr)
+
+		DescribeBackupSpecInSF(d, backup.Spec, resultMap)
 
 		DescribeBackupStatusInSF(ctx, kbClient, d, backup, details, insecureSkipTLSVerify, caCertFile)
 
@@ -85,7 +87,7 @@ func DescribeBackupInSF(
 }
 
 // DescribeBackupSpecInSF describes a backup spec in structured format.
-func DescribeBackupSpecInSF(d *StructuredDescriber, spec velerov1api.BackupSpec) {
+func DescribeBackupSpecInSF(d *StructuredDescriber, spec velerov1api.BackupSpec, resultMap map[string]results.Result) {
 	backupSpecInfo := make(map[string]interface{})
 	var s string
 
@@ -152,6 +154,7 @@ func DescribeBackupSpecInSF(d *StructuredDescriber, spec velerov1api.BackupSpec)
 	backupSpecInfo["CSISnapshotTimeout"] = spec.CSISnapshotTimeout.Duration.String()
 
 	// describe hooks
+	backupSpecInfo["hookFailed"] = hookFailed(resultMap)
 	hooksInfo := make(map[string]interface{})
 	hooksResources := make(map[string]interface{})
 	for _, backupResourceHookSpec := range spec.Hooks.Resources {
@@ -468,36 +471,24 @@ func DescribeVSCInSF(details bool, vsc snapshotv1api.VolumeSnapshotContent, vscD
 }
 
 // DescribeBackupResultsInSF describes errors and warnings in structured format.
-func DescribeBackupResultsInSF(ctx context.Context, kbClient kbclient.Client, d *StructuredDescriber, backup *velerov1api.Backup, insecureSkipTLSVerify bool, caCertPath string) {
+func DescribeBackupResultsInSF(ctx context.Context, kbClient kbclient.Client, d *StructuredDescriber, backup *velerov1api.Backup, resultMap map[string]results.Result, resultErr error) {
 	if backup.Status.Warnings == 0 && backup.Status.Errors == 0 {
 		return
 	}
 
-	var buf bytes.Buffer
-	var resultMap map[string]results.Result
-
 	errors, warnings := make(map[string]interface{}), make(map[string]interface{})
 	defer func() {
 		d.Describe("errors", errors)
 		d.Describe("warnings", warnings)
 	}()
 
-	// If 'ErrNotFound' occurs, it means the backup bundle in the bucket has already been there before the backup-result file is introduced.
-	// We only display the count of errors and warnings in this case.
-	err := downloadrequest.Stream(ctx, kbClient, backup.Namespace, backup.Name, velerov1api.DownloadTargetKindBackupResults, &buf, downloadRequestTimeout, insecureSkipTLSVerify, caCertPath)
-	if err == downloadrequest.ErrNotFound {
+	if resultErr == downloadrequest.ErrNotFound {
 		errors["count"] = backup.Status.Errors
 		warnings["count"] = backup.Status.Warnings
 		return
-	} else if err != nil {
-		errors["errorGettingErrors"] = fmt.Errorf("<error getting errors: %v>", err)
-		warnings["errorGettingWarnings"] = fmt.Errorf("<error getting warnings: %v>", err)
-		return
-	}
-
-	if err := json.NewDecoder(&buf).Decode(&resultMap); err != nil {
-		errors["errorGettingErrors"] = fmt.Errorf("<error decoding errors: %v>", err)
-		warnings["errorGettingWarnings"] = fmt.Errorf("<error decoding warnings: %v>", err)
+	} else if resultErr != nil {
+		errors["errorGettingErrors"] = fmt.Errorf("<error getting errors: %v>", resultErr)
+		warnings["errorGettingWarnings"] = fmt.Errorf("<error getting warnings: %v>", resultErr)
 		return
 	}
 

pkg/cmd/util/output/describe.go

@@ -18,14 +18,21 @@ package output
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"fmt"
 	"sort"
 	"strings"
 	"text/tabwriter"
+	"time"
 
 	"github.com/fatih/color"
+	"github.com/vmware-tanzu/velero/internal/hook"
+	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	"github.com/vmware-tanzu/velero/pkg/cmd/util/downloadrequest"
+	"github.com/vmware-tanzu/velero/pkg/util/results"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 type Describer struct {
@@ -167,3 +174,45 @@ func (d *StructuredDescriber) JSONEncode() string {
 	_ = encoder.Encode(d.output)
 	return byteBuffer.String()
 }
+
+func downloadResult(ctx context.Context, kbClient kbclient.Client, namespace, name string, kind velerov1api.DownloadTargetKind, timeout time.Duration, insecureSkipTLSVerify bool, caCertFile string) (map[string]results.Result, error) {
+	var buf bytes.Buffer
+	var resultMap map[string]results.Result
+
+	// If 'ErrNotFound' occurs, it means the backup bundle in the bucket was already there before the backup-result file was introduced.
+	// We only display the count of errors and warnings instead.
+	err := downloadrequest.Stream(ctx, kbClient, namespace, name, kind, &buf, timeout, insecureSkipTLSVerify, caCertFile)
+	if err != nil {
+		return resultMap, err
+	}
+
+	if err := json.NewDecoder(&buf).Decode(&resultMap); err != nil {
+		return resultMap, err
+	}
+	return resultMap, nil
+}
+
+func hookFailed(resultMap map[string]results.Result) string {
+	if len(resultMap) == 0 {
+		return "Unknown"
+	}
+
+	if _, ok := resultMap["errors"]; !ok {
+		return "Unknown"
+	}
+
+	for _, msg := range resultMap["errors"].Velero {
+		if strings.Contains(msg, hook.HookErrLogPrefix) {
+			return "True"
+		}
+	}
+
+	for _, msgs := range resultMap["errors"].Namespaces {
+		for _, msg := range msgs {
+			if strings.Contains(msg, hook.HookErrLogPrefix) {
+				return "True"
+			}
+		}
+	}
+	return "False"
+}

pkg/cmd/util/output/restore_describer.go

@@ -96,7 +96,8 @@ func DescribeRestore(ctx context.Context, kbClient kbclient.Client, restore *vel
 			}
 		}
 
-		describeRestoreResults(ctx, kbClient, d, restore, insecureSkipTLSVerify, caCertFile)
+		resultMap, resultErr := downloadResult(ctx, kbClient, restore.Namespace, restore.Name, velerov1api.DownloadTargetKindRestoreResults, downloadRequestTimeout, insecureSkipTLSVerify, caCertFile)
+		describeRestoreResults(ctx, kbClient, d, restore, resultMap, resultErr)
 
 		d.Println()
 		d.Printf("Backup:\t%s\n", restore.Spec.BackupName)
@@ -136,6 +137,9 @@ func DescribeRestore(ctx context.Context, kbClient kbclient.Client, restore *vel
 
 		d.Printf("\tCluster-scoped:\t%s\n", BoolPointerString(restore.Spec.IncludeClusterResources, "excluded", "included", "auto"))
 
+		d.Println()
+		d.Printf("Hook Failed:\t%s\n", hookFailed(resultMap))
+
 		d.Println()
 		d.DescribeMap("Namespace mappings", restore.Spec.NamespaceMapping)
 
@@ -214,21 +218,13 @@ func describeRestoreItemOperations(ctx context.Context, kbClient kbclient.Client
 	}
 }
 
-func describeRestoreResults(ctx context.Context, kbClient kbclient.Client, d *Describer, restore *velerov1api.Restore, insecureSkipTLSVerify bool, caCertPath string) {
+func describeRestoreResults(ctx context.Context, kbClient kbclient.Client, d *Describer, restore *velerov1api.Restore, resultMap map[string]results.Result, resultErr error) {
 	if restore.Status.Warnings == 0 && restore.Status.Errors == 0 {
 		return
 	}
 
-	var buf bytes.Buffer
-	var resultMap map[string]results.Result
-
-	if err := downloadrequest.Stream(ctx, kbClient, restore.Namespace, restore.Name, velerov1api.DownloadTargetKindRestoreResults, &buf, downloadRequestTimeout, insecureSkipTLSVerify, caCertPath); err != nil {
-		d.Printf("Warnings:\t<error getting warnings: %v>\n\nErrors:\t<error getting errors: %v>\n", err, err)
-		return
-	}
-
-	if err := json.NewDecoder(&buf).Decode(&resultMap); err != nil {
-		d.Printf("Warnings:\t<error decoding warnings: %v>\n\nErrors:\t<error decoding errors: %v>\n", err, err)
+	if resultErr != nil {
+		d.Printf("Warnings:\t<error getting warnings: %v>\n\nErrors:\t<error getting errors: %v>\n", resultErr, resultErr)
 		return
 	}
 

pkg/cmd/util/output/schedule_describer.go

@@ -74,7 +74,7 @@ func DescribeScheduleSpec(d *Describer, spec v1.ScheduleSpec) {
 	d.Println()
 	d.Println("Backup Template:")
 	d.Prefix = "\t"
-	DescribeBackupSpec(d, spec.Template)
+	DescribeBackupSpec(d, spec.Template, nil)
 	d.Prefix = ""
 }
 

pkg/restore/restore.go

@@ -1955,8 +1955,9 @@ func (ctx *restoreContext) waitExec(createdObj *unstructured.Unstructured) {
 		podNs := createdObj.GetNamespace()
 		pod := new(v1.Pod)
 		if err := runtime.DefaultUnstructuredConverter.FromUnstructured(createdObj.UnstructuredContent(), &pod); err != nil {
-			ctx.log.WithError(err).Error("error converting unstructured pod")
-			ctx.hooksErrs <- hook.HookErrInfo{Namespace: podNs, Err: err}
+			msg := "error converting unstructured pod"
+			ctx.log.WithError(err).Error(msg)
+			ctx.hooksErrs <- hook.HookErrInfo{Namespace: podNs, Err: errors.New(hook.HookErrLogPrefix + msg + err.Error())}
 			return
 		}
 		execHooksByContainer, err := hook.GroupRestoreExecHooks(
@@ -1965,18 +1966,20 @@ func (ctx *restoreContext) waitExec(createdObj *unstructured.Unstructured) {
 			ctx.log,
 		)
 		if err != nil {
-			ctx.log.WithError(err).Errorf("error getting exec hooks for pod %s/%s", pod.Namespace, pod.Name)
-			ctx.hooksErrs <- hook.HookErrInfo{Namespace: podNs, Err: err}
+			msg := fmt.Sprintf("error getting exec hooks for pod %s/%s", pod.Namespace, pod.Name)
+			ctx.log.WithError(err).Errorf(msg)
+			ctx.hooksErrs <- hook.HookErrInfo{Namespace: podNs, Err: errors.New(hook.HookErrLogPrefix + msg + err.Error())}
 			return
 		}
 
 		if errs := ctx.waitExecHookHandler.HandleHooks(ctx.hooksContext, ctx.log, pod, execHooksByContainer); len(errs) > 0 {
-			ctx.log.WithError(kubeerrs.NewAggregate(errs)).Error("unable to successfully execute post-restore hooks")
+			msg := "unable to successfully execute post-restore hooks"
+			ctx.log.WithError(kubeerrs.NewAggregate(errs)).Error(msg)
 			ctx.hooksCancelFunc()
 
 			for _, err := range errs {
 				// Errors are already logged in the HandleHooks method.
-				ctx.hooksErrs <- hook.HookErrInfo{Namespace: podNs, Err: err}
+				ctx.hooksErrs <- hook.HookErrInfo{Namespace: podNs, Err: errors.New(hook.HookErrLogPrefix + msg + err.Error())}
 			}
 		}
 	}()