Adds error location in error results

This commit extracts the login in `conditions.go` that searches for the failed pod and its container. It merges the information of the error location (pod and container) into the error results.

Fix tests

deploy/crds/shipwright.io_buildruns.yaml

@@ -466,9 +466,17 @@ spec:
                   pod:
                     type: string
                 type: object
-              failure:
-                description: Failure contains error details that are collected and surfaced from TaskRun
+              failureDetails:
+                description: FailureDetails contains error details that are collected and surfaced from TaskRun
                 properties:
+                  location:
+                    description: FailedAt describes the location where the failure happened
+                    properties:
+                      container:
+                        type: string
+                      pod:
+                        type: string
+                    type: object
                   message:
                     type: string
                   reason:

pkg/apis/build/v1alpha1/buildrun_types.go

@@ -149,9 +149,9 @@ type BuildRunStatus struct {
 	// +optional
 	FailedAt *FailedAt `json:"failedAt,omitempty"`
 
-	// Failure contains error details that are collected and surfaced from TaskRun
+	// FailureDetails contains error details that are collected and surfaced from TaskRun
 	// +optional
-	Failure *Failure `json:"failure,omitempty"`
+	FailureDetails *FailureDetails `json:"failureDetails,omitempty"`
 }
 
 // FailedAt describes the location where the failure happened
@@ -160,10 +160,11 @@ type FailedAt struct {
 	Container string `json:"container,omitempty"`
 }
 
-// Failure describes an error while building images
-type Failure struct {
-	Reason  string `json:"reason,omitempty"`
-	Message string `json:"message,omitempty"`
+// FailureDetails describes an error while building images
+type FailureDetails struct {
+	Reason   string    `json:"reason,omitempty"`
+	Message  string    `json:"message,omitempty"`
+	Location *FailedAt `json:"location,omitempty"`
 }
 
 // BuildRef can be used to refer to a specific instance of a Build.

pkg/reconciler/buildrun/buildrun.go

@@ -308,7 +308,7 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 				return reconcile.Result{}, err
 			}
 
-			resources.UpdateBuildRunUsingTaskFailures(buildRun, lastTaskRun)
+			resources.UpdateBuildRunUsingTaskFailures(ctx, r.client, buildRun, lastTaskRun)
 			taskRunStatus := trCondition.Status
 
 			// check if we should delete the generated service account by checking the build run spec and that the task run is complete

pkg/reconciler/buildrun/resources/conditions.go

@@ -11,13 +11,11 @@ import (
 	"knative.dev/pkg/apis"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
+	"github.com/shipwright-io/build/pkg/ctxlog"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
-
-	buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
-	"github.com/shipwright-io/build/pkg/ctxlog"
 )
 
 // Common condition strings for reason, kind, etc.
@@ -70,8 +68,8 @@ func UpdateBuildRunUsingTaskRunCondition(ctx context.Context, client client.Clie
 
 	case v1beta1.TaskRunReasonFailed:
 		if taskRun.Status.CompletionTime != nil {
-			var pod corev1.Pod
-			if err := client.Get(ctx, types.NamespacedName{Namespace: taskRun.Namespace, Name: taskRun.Status.PodName}, &pod); err != nil {
+			pod, failedContainer, err := extractFailedPodAndContainer(ctx, client, taskRun)
+			if err != nil {
 				// when trying to customize the Condition Message field, ensure the Message cover the case
 				// when a Pod is deleted.
 				// Note: this is an edge case, but not doing this prevent a BuildRun from being marked as Failed
@@ -85,23 +83,6 @@ func UpdateBuildRunUsingTaskRunCondition(ctx context.Context, client client.Clie
 
 			buildRun.Status.FailedAt = &buildv1alpha1.FailedAt{Pod: pod.Name}
 
-			// Since the container status list is not sorted, as a quick workaround mark all failed containers
-			var failures = make(map[string]struct{})
-			for _, containerStatus := range pod.Status.ContainerStatuses {
-				if containerStatus.State.Terminated != nil && containerStatus.State.Terminated.ExitCode != 0 {
-					failures[containerStatus.Name] = struct{}{}
-				}
-			}
-
-			// Find the first container that failed
-			var failedContainer *corev1.Container
-			for i, container := range pod.Spec.Containers {
-				if _, has := failures[container.Name]; has {
-					failedContainer = &pod.Spec.Containers[i]
-					break
-				}
-			}
-
 			if failedContainer != nil {
 				buildRun.Status.FailedAt.Container = failedContainer.Name
 				message = fmt.Sprintf("buildrun step %s failed in pod %s, for detailed information: kubectl --namespace %s logs %s --container=%s",

pkg/reconciler/buildrun/resources/failures.go

@@ -1,10 +1,14 @@
 package resources
 
 import (
+	"context"
 	"encoding/json"
 	buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"knative.dev/pkg/apis"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
@@ -13,17 +17,17 @@ const (
 )
 
 // UpdateBuildRunUsingTaskFailures is extracting failures from taskRun steps and adding them to buildRun (mutates)
-func UpdateBuildRunUsingTaskFailures(buildRun *buildv1alpha1.BuildRun, taskRun *v1beta1.TaskRun) {
+func UpdateBuildRunUsingTaskFailures(ctx context.Context, client client.Client, buildRun *buildv1alpha1.BuildRun, taskRun *v1beta1.TaskRun) {
 	trCondition := taskRun.Status.GetCondition(apis.ConditionSucceeded)
 
 	// only extract failures when failing condition is present
 	if trCondition != nil && v1beta1.TaskRunReason(trCondition.Reason) == v1beta1.TaskRunReasonFailed {
-		buildRun.Status.Failure = extractErrorFromTaskRun(taskRun)
+		buildRun.Status.FailureDetails = extractFailureDetails(ctx, client, taskRun)
 	}
 }
 
-func extractErrorFromTaskRun(taskRun *v1beta1.TaskRun) *buildv1alpha1.Failure {
-	shipError := buildv1alpha1.Failure{}
+func extractFailureReasonAndMessage(taskRun *v1beta1.TaskRun) *buildv1alpha1.FailureDetails {
+	shipError := buildv1alpha1.FailureDetails{}
 
 	for _, step := range taskRun.Status.Steps {
 		message := step.Terminated.Message
@@ -50,3 +54,44 @@ func extractErrorFromTaskRun(taskRun *v1beta1.TaskRun) *buildv1alpha1.Failure {
 
 	return &shipError
 }
+
+func extractFailedPodAndContainer(ctx context.Context, client client.Client, taskRun *v1beta1.TaskRun) (*v1.Pod, *v1.Container, error) {
+	var pod v1.Pod
+	if err := client.Get(ctx, types.NamespacedName{Namespace: taskRun.Namespace, Name: taskRun.Status.PodName}, &pod); err != nil {
+		return nil, nil, err
+	}
+
+	var failures = make(map[string]struct{})
+	for _, containerStatus := range pod.Status.ContainerStatuses {
+		if containerStatus.State.Terminated != nil && containerStatus.State.Terminated.ExitCode != 0 {
+			failures[containerStatus.Name] = struct{}{}
+		}
+	}
+
+	// Find the first container that failed
+	var failedContainer *v1.Container
+	for i, container := range pod.Spec.Containers {
+		if _, has := failures[container.Name]; has {
+			failedContainer = &pod.Spec.Containers[i]
+			break
+		}
+	}
+
+	return &pod, failedContainer, nil
+}
+
+func extractFailureDetails(ctx context.Context, client client.Client, taskRun *v1beta1.TaskRun) (failure *buildv1alpha1.FailureDetails) {
+	if failure = extractFailureReasonAndMessage(taskRun); failure == nil {
+		return nil
+	}
+
+	pod, container, _ := extractFailedPodAndContainer(ctx, client, taskRun)
+
+	if pod == nil || container == nil {
+		return failure
+	}
+
+	failure.Location = &buildv1alpha1.FailedAt{Container: container.Name, Pod: pod.Name}
+
+	return failure
+}

pkg/reconciler/buildrun/resources/failures_test.go

@@ -1,8 +1,10 @@
 package resources
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
+	"github.com/shipwright-io/build/pkg/controller/fakes"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -14,6 +16,8 @@ import (
 
 var _ = Describe("Surfacing errors", func() {
 	Context("resources.UpdateBuildRunUsingTaskFailures", func() {
+		ctx := context.Background()
+		client := &fakes.FakeClient{}
 
 		It("surfaces errors to BuildRun in failed TaskRun", func() {
 			redTaskRun := v1beta1.TaskRun{}
@@ -37,10 +41,10 @@ var _ = Describe("Surfacing errors", func() {
 			redTaskRun.Status.Steps = append(redTaskRun.Status.Steps, failedStep)
 			redBuild := v1alpha1.BuildRun{}
 
-			UpdateBuildRunUsingTaskFailures(&redBuild, &redTaskRun)
+			UpdateBuildRunUsingTaskFailures(ctx, client, &redBuild, &redTaskRun)
 
-			Expect(redBuild.Status.Failure.Message).To(Equal(errorMessageValue))
-			Expect(redBuild.Status.Failure.Reason).To(Equal(errorReasonValue))
+			Expect(redBuild.Status.FailureDetails.Message).To(Equal(errorMessageValue))
+			Expect(redBuild.Status.FailureDetails.Reason).To(Equal(errorReasonValue))
 		})
 
 		It("failed TaskRun without error reason and message", func() {
@@ -58,37 +62,37 @@ var _ = Describe("Surfacing errors", func() {
 			redTaskRun.Status.Steps = append(redTaskRun.Status.Steps, failedStep)
 			redBuild := v1alpha1.BuildRun{}
 
-			UpdateBuildRunUsingTaskFailures(&redBuild, &redTaskRun)
+			UpdateBuildRunUsingTaskFailures(ctx, client, &redBuild, &redTaskRun)
 
-			Expect(redBuild.Status.Failure).To(BeNil())
+			Expect(redBuild.Status.FailureDetails).To(BeNil())
 		})
 
 		It("no errors present in BuildRun for successful TaskRun", func() {
 			greenTaskRun := v1beta1.TaskRun{}
 			greenTaskRun.Status.Conditions = append(greenTaskRun.Status.Conditions, apis.Condition{Type: apis.ConditionSucceeded})
 			greenBuildRun := v1alpha1.BuildRun{}
 
-			UpdateBuildRunUsingTaskFailures(&greenBuildRun, &greenTaskRun)
+			UpdateBuildRunUsingTaskFailures(ctx, client, &greenBuildRun, &greenTaskRun)
 
-			Expect(greenBuildRun.Status.Failure).To(BeNil())
+			Expect(greenBuildRun.Status.FailureDetails).To(BeNil())
 		})
 
 		It("TaskRun has not condition succeeded so nothing to do", func() {
 			unfinishedTaskRun := v1beta1.TaskRun{}
 			unfinishedTaskRun.Status.Conditions = append(unfinishedTaskRun.Status.Conditions, apis.Condition{Type: apis.ConditionReady})
 			unfinishedBuildRun := v1alpha1.BuildRun{}
 
-			UpdateBuildRunUsingTaskFailures(&unfinishedBuildRun, &unfinishedTaskRun)
-			Expect(unfinishedBuildRun.Status.Failure).To(BeNil())
+			UpdateBuildRunUsingTaskFailures(ctx, client, &unfinishedBuildRun, &unfinishedTaskRun)
+			Expect(unfinishedBuildRun.Status.FailureDetails).To(BeNil())
 		})
 
 		It("TaskRun has a unknown reason", func() {
 			unknownTaskRun := v1beta1.TaskRun{}
 			unknownTaskRun.Status.Conditions = append(unknownTaskRun.Status.Conditions, apis.Condition{Type: apis.ConditionSucceeded, Reason: "random"})
 			unknownBuildRun := v1alpha1.BuildRun{}
 
-			UpdateBuildRunUsingTaskFailures(&unknownBuildRun, &unknownTaskRun)
-			Expect(unknownBuildRun.Status.Failure).To(BeNil())
+			UpdateBuildRunUsingTaskFailures(ctx, client, &unknownBuildRun, &unknownTaskRun)
+			Expect(unknownBuildRun.Status.FailureDetails).To(BeNil())
 		})
 	})
 })

pkg/reconciler/buildrun/resources/sources/git.go

@@ -51,10 +51,10 @@ func AppendGitStep(
 		fmt.Sprintf("$(results.%s-source-%s-%s.path)", prefixParamsResultsVolumes, name, commitSHAResult),
 		"--result-file-commit-author",
 		fmt.Sprintf("$(results.%s-source-%s-%s.path)", prefixParamsResultsVolumes, name, commitAuthorResult),
-		"--result-error-reason",
-		fmt.Sprintf("$(results.%s-error-reason.path)", prefixParamsResultsVolumes),
 		"--result-error-message",
 		fmt.Sprintf("$(results.%s-error-message.path)", prefixParamsResultsVolumes),
+		"--result-error-reason",
+		fmt.Sprintf("$(results.%s-error-reason.path)", prefixParamsResultsVolumes),
 	}
 
 	// Check if a revision is defined

pkg/reconciler/buildrun/resources/sources/git_test.go

@@ -53,6 +53,10 @@ var _ = Describe("Git", func() {
 				"$(results.shp-source-default-commit-sha.path)",
 				"--result-file-commit-author",
 				"$(results.shp-source-default-commit-author.path)",
+				"--result-error-message",
+				"$(results.shp-error-message.path)",
+				"--result-error-reason",
+				"$(results.shp-error-reason.path)",
 			}))
 		})
 	})
@@ -100,6 +104,10 @@ var _ = Describe("Git", func() {
 				"$(results.shp-source-default-commit-sha.path)",
 				"--result-file-commit-author",
 				"$(results.shp-source-default-commit-author.path)",
+				"--result-error-message",
+				"$(results.shp-error-message.path)",
+				"--result-error-reason",
+				"$(results.shp-error-reason.path)",
 				"--secret-path",
 				"/workspace/shp-source-secret",
 			}))

pkg/reconciler/buildrun/resources/taskrun_test.go

@@ -92,6 +92,10 @@ var _ = Describe("GenerateTaskrun", func() {
 					"$(results.shp-source-default-commit-sha.path)",
 					"--result-file-commit-author",
 					"$(results.shp-source-default-commit-author.path)",
+					"--result-error-message",
+					"$(results.shp-error-message.path)",
+					"--result-error-reason",
+					"$(results.shp-error-reason.path)",
 				}))
 			})