osbuild-worker: improve error "reason" in case of stage failures (HMS-1442)

cmd/osbuild-worker/export_test.go

@@ -1,3 +1,6 @@
 package main
 
-var WorkerClientErrorFrom = workerClientErrorFrom
+var (
+	WorkerClientErrorFrom         = workerClientErrorFrom
+	MakeJobErrorFromOsbuildOutput = makeJobErrorFromOsbuildOutput
+)

cmd/osbuild-worker/jobimpl-depsolve.go

@@ -77,23 +77,31 @@ func workerClientErrorFrom(err error) (*clienterrors.Error, error) {
 	switch e := err.(type) {
 	case dnfjson.Error:
 		// Error originates from dnf-json
+		reason := fmt.Sprintf("DNF error occurred: %s", e.Kind)
+		details := e.Reason
 		switch e.Kind {
 		case "DepsolveError":
-			return clienterrors.WorkerClientError(clienterrors.ErrorDNFDepsolveError, err.Error(), e.Reason), nil
+			return clienterrors.WorkerClientError(clienterrors.ErrorDNFDepsolveError, reason, details), nil
 		case "MarkingErrors":
-			return clienterrors.WorkerClientError(clienterrors.ErrorDNFMarkingErrors, err.Error(), e.Reason), nil
+			return clienterrors.WorkerClientError(clienterrors.ErrorDNFMarkingErrors, reason, details), nil
 		case "RepoError":
-			return clienterrors.WorkerClientError(clienterrors.ErrorDNFRepoError, err.Error(), e.Reason), nil
+			return clienterrors.WorkerClientError(clienterrors.ErrorDNFRepoError, reason, details), nil
 		default:
 			err := fmt.Errorf("Unhandled dnf-json error in depsolve job: %v", err)
 			// This still has the kind/reason format but a kind that's returned
 			// by dnf-json and not explicitly handled here.
-			return clienterrors.WorkerClientError(clienterrors.ErrorDNFOtherError, err.Error(), e.Reason), err
+			return clienterrors.WorkerClientError(clienterrors.ErrorDNFOtherError, reason, details), err
 		}
 	default:
-		err := fmt.Errorf("rpmmd error in depsolve job: %v", err)
+		reason := "rpmmd error in depsolve job"
+		details := "<nil>"
+		if err == nil {
+			err = fmt.Errorf("workerClientErrorFrom expected an error to be processed. Not nil")
+		} else {
+			details = err.Error()
+		}
 		// Error originates from internal/rpmmd, not from dnf-json
-		return clienterrors.WorkerClientError(clienterrors.ErrorRPMMDError, err.Error(), nil), err
+		return clienterrors.WorkerClientError(clienterrors.ErrorRPMMDError, reason, details), err
 	}
 }
 

cmd/osbuild-worker/jobimpl-depsolve_test.go

@@ -18,8 +18,7 @@ func TestWorkerClientErrorFromDnfJson(t *testing.T) {
 	}
 	clientErr, err := worker.WorkerClientErrorFrom(dnfJsonErr)
 	assert.NoError(t, err)
-	// XXX: this is duplicating the details, see https://github.com/osbuild/images/issues/727
-	assert.Equal(t, clientErr.String(), `Code: 20, Reason: DNF error occurred: DepsolveError: something is terribly wrong, Details: something is terribly wrong`)
+	assert.Equal(t, `Code: 20, Reason: DNF error occurred: DepsolveError, Details: something is terribly wrong`, clientErr.String())
 }
 
 func TestWorkerClientErrorFromOtherError(t *testing.T) {
@@ -28,13 +27,12 @@ func TestWorkerClientErrorFromOtherError(t *testing.T) {
 	// XXX: this is probably okay but it seems slightly dangerous to
 	// assume that any "error" we get there is coming from rpmmd, can
 	// we generate a more typed error from dnfjson here for rpmmd errors?
-	assert.EqualError(t, err, "rpmmd error in depsolve job: some error")
-	assert.Equal(t, clientErr.String(), `Code: 23, Reason: rpmmd error in depsolve job: some error, Details: <nil>`)
+	assert.EqualError(t, err, "some error")
+	assert.Equal(t, `Code: 23, Reason: rpmmd error in depsolve job, Details: some error`, clientErr.String())
 }
 
 func TestWorkerClientErrorFromNil(t *testing.T) {
 	clientErr, err := worker.WorkerClientErrorFrom(nil)
-	// XXX: this is wrong, it should generate an internal error
-	assert.EqualError(t, err, "rpmmd error in depsolve job: <nil>")
-	assert.Equal(t, clientErr.String(), `Code: 23, Reason: rpmmd error in depsolve job: <nil>, Details: <nil>`)
+	assert.EqualError(t, err, "workerClientErrorFrom expected an error to be processed. Not nil")
+	assert.Equal(t, `Code: 23, Reason: rpmmd error in depsolve job, Details: <nil>`, clientErr.String())
 }

cmd/osbuild-worker/jobimpl-osbuild.go

@@ -351,6 +351,33 @@ func (impl *OSBuildJobImpl) getPulpClient(targetOptions *target.PulpOSTreeTarget
 	return pulp.NewClientFromFile(address, impl.PulpConfig.CredsFilePath)
 }
 
+func makeJobErrorFromOsbuildOutput(osbuildOutput *osbuild.Result) *clienterrors.Error {
+	var osbErrors []string
+	if osbuildOutput.Error != nil {
+		osbErrors = append(osbErrors, fmt.Sprintf("osbuild error: %s", string(osbuildOutput.Error)))
+	}
+	if osbuildOutput.Errors != nil {
+		for _, err := range osbuildOutput.Errors {
+			osbErrors = append(osbErrors, fmt.Sprintf("manifest validation error: %v", err))
+		}
+	}
+	var failedStage string
+	for _, pipelineLog := range osbuildOutput.Log {
+		for _, stageResult := range pipelineLog {
+			if !stageResult.Success {
+				failedStage = stageResult.Type
+				break
+			}
+		}
+	}
+
+	reason := "osbuild build failed"
+	if len(failedStage) > 0 {
+		reason += " in stage:\n" + failedStage
+	}
+	return clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, reason, osbErrors)
+}
+
 func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 	logWithId := logrus.WithField("jobId", job.Id().String())
 	// Initialize variable needed for reporting back to osbuild-composer.
@@ -565,16 +592,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 
 	// Second handle the case when the build failed, but osbuild finished successfully
 	if !osbuildJobResult.OSBuildOutput.Success {
-		var osbErrors []string
-		if osbuildJobResult.OSBuildOutput.Error != nil {
-			osbErrors = append(osbErrors, fmt.Sprintf("osbuild error: %s", string(osbuildJobResult.OSBuildOutput.Error)))
-		}
-		if osbuildJobResult.OSBuildOutput.Errors != nil {
-			for _, err := range osbuildJobResult.OSBuildOutput.Errors {
-				osbErrors = append(osbErrors, fmt.Sprintf("manifest validation error: %v", err))
-			}
-		}
-		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "osbuild build failed", osbErrors)
+		osbuildJobResult.JobError = makeJobErrorFromOsbuildOutput(osbuildJobResult.OSBuildOutput)
 		return nil
 	}
 

cmd/osbuild-worker/jobimpl-osbuild_test.go

@@ -0,0 +1,81 @@
+package main_test
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/osbuild/images/pkg/osbuild"
+
+	main "github.com/osbuild/osbuild-composer/cmd/osbuild-worker"
+)
+
+func TestMakeJobErrorFromOsbuildOutput(t *testing.T) {
+	tests := []struct {
+		inputData *osbuild.Result
+		expected  string
+	}{
+		{
+			inputData: &osbuild.Result{
+				Success: false,
+				Log: map[string]osbuild.PipelineResult{
+					"fake-os": []osbuild.StageResult{
+						{
+							Type:    "good-stage",
+							Success: true,
+							Output:  "good-output",
+						},
+						{
+							Type:    "bad-stage",
+							Success: false,
+							Output:  "bad-failure",
+						},
+					},
+				},
+			},
+			expected: `Code: 10, Reason: osbuild build failed in stage:
+bad-stage, Details: []`,
+		},
+		{
+			inputData: &osbuild.Result{
+				Success: false,
+				Log: map[string]osbuild.PipelineResult{
+					"fake-os": []osbuild.StageResult{},
+				},
+			},
+			expected: `Code: 10, Reason: osbuild build failed, Details: []`,
+		},
+		{
+			inputData: &osbuild.Result{
+				Error:   json.RawMessage("some_osbuild_error"),
+				Success: false,
+				Log: map[string]osbuild.PipelineResult{
+					"fake-os": []osbuild.StageResult{},
+				},
+			},
+			expected: `Code: 10, Reason: osbuild build failed, Details: [osbuild error: some_osbuild_error]`,
+		},
+		{
+			inputData: &osbuild.Result{
+				Errors: []osbuild.ValidationError{
+					{
+						Message: "validation error message",
+						Path:    []string{"error path"},
+					},
+				},
+				Success: false,
+				Log: map[string]osbuild.PipelineResult{
+					"fake-os": []osbuild.StageResult{},
+				},
+			},
+			expected: `Code: 10, Reason: osbuild build failed, Details: [manifest validation error: {validation error message [error path]}]`,
+		},
+	}
+	for _, testData := range tests {
+		fakeOsbuildResult := testData.inputData
+
+		wce := main.MakeJobErrorFromOsbuildOutput(fakeOsbuildResult)
+		require.Equal(t, testData.expected, wce.String())
+	}
+}