OTA-1159: [3/3] Propagate reconciliation errors via ReconciliationIssues condition

pkg/cvo/cvo.go

@@ -681,7 +681,7 @@ func (optr *Operator) sync(ctx context.Context, key string) error {
 	// handle the case of a misconfigured CVO by doing nothing
 	if len(desired.Image) == 0 {
 		return optr.syncStatus(ctx, original, config, &SyncWorkerStatus{
-			Failure: &payload.UpdateError{
+			FailureSummary: &payload.UpdateError{
 				Reason:  "NoDesiredImage",
 				Message: "No configured operator version, unable to update cluster",
 			},

pkg/cvo/cvo_scenarios_test.go

@@ -11,6 +11,8 @@ import (
 	"time"
 
 	"github.com/davecgh/go-spew/spew"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/google/uuid"
 
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -3473,7 +3475,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) {
 			Done:        2,
 			Total:       3,
 			VersionHash: "DL-FFQ2Uem8=", Architecture: architecture,
-			Failure: &payload.UpdateError{
+			FailureSummary: &payload.UpdateError{
 				Nested:  fmt.Errorf("unable to proceed"),
 				Reason:  "UpdatePayloadFailed",
 				Message: "Could not update test \"file-yml\" (3 of 3)",
@@ -3644,7 +3646,7 @@ func TestCVO_ParallelError(t *testing.T) {
 	//
 	// verify we observe the remaining changes in the first sync
 	for status := range worker.StatusCh() {
-		if status.Failure == nil {
+		if status.FailureSummary == nil {
 			if status.Done == 0 || (status.Done == 1 && status.Total == 3) {
 				if !reflect.DeepEqual(status, SyncWorkerStatus{
 					Initial:      true,
@@ -3673,17 +3675,42 @@ func TestCVO_ParallelError(t *testing.T) {
 			}
 			continue
 		}
-		err := status.Failure
+		err := status.FailureSummary
 		uErr, ok := err.(*payload.UpdateError)
 		if !ok || uErr.Reason != "MultipleErrors" || uErr.Message != "Multiple errors are preventing progress:\n* Failed to reconcile 10-a-file resource\n* Failed to reconcile 20-b-file resource" {
 			t.Fatalf("unexpected error: %v", err)
 		}
 		if status.LastProgress.IsZero() {
 			t.Fatalf("unexpected last progress: %v", status.LastProgress)
 		}
-		if !reflect.DeepEqual(status, SyncWorkerStatus{
-			Initial:      true,
-			Failure:      err,
+		if diff := cmp.Diff(status, SyncWorkerStatus{
+			Initial:        true,
+			FailureSummary: err,
+			Failures: []error{
+				&payload.UpdateError{
+					UpdateEffect: payload.UpdateEffectNone,
+					Message:      "Failed to reconcile 10-a-file resource",
+					Task: &payload.Task{
+						Index: 1,
+						Total: 3,
+						Manifest: &manifest.Manifest{
+							OriginalFilename: "0000_10_a_file.yaml",
+							GVK:              schema.GroupVersionKind{Version: "v1", Kind: "Test"},
+						},
+					},
+				},
+				&payload.UpdateError{
+					UpdateEffect: payload.UpdateEffectNone,
+					Message:      "Failed to reconcile 20-b-file resource",
+					Task: &payload.Task{
+						Index: 3,
+						Total: 3,
+						Manifest: &manifest.Manifest{
+							OriginalFilename: "0000_20_b_file.yaml",
+							GVK:              schema.GroupVersionKind{Version: "v1", Kind: "Test"},
+						},
+					},
+				}},
 			Done:         1,
 			Total:        3,
 			VersionHash:  "Gyh2W6qcDO4=",
@@ -3703,8 +3730,8 @@ func TestCVO_ParallelError(t *testing.T) {
 				LastTransitionTime: status.loadPayloadStatus.LastTransitionTime,
 				Update:             configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"},
 			},
-		}) {
-			t.Fatalf("unexpected final: %v", status)
+		}, cmp.AllowUnexported(SyncWorkerStatus{}), cmpopts.IgnoreFields(manifest.Manifest{}, "id", "Raw", "Obj")); diff != "" {
+			t.Fatalf("finals differs from expected: %s", diff)
 		}
 		break
 	}
@@ -3964,6 +3991,10 @@ func verifyAllStatusOptionalDone(t *testing.T, stepName string, ignoreDone bool,
 			t.Fatalf("%s: channel closed after reading only %d items", testName, i)
 		}
 
+		if expect.FailureSummary != nil {
+			expect.Failures = []error{expect.FailureSummary}
+		}
+
 		if nextTime := actual.LastProgress; !nextTime.Equal(lastTime) {
 			actual.LastProgress = time.Unix(count, 0)
 			count++

pkg/cvo/cvo_test.go

@@ -194,7 +194,7 @@ func TestOperator_sync(t *testing.T) {
 			syncStatus: &SyncWorkerStatus{
 				Reconciling: false,
 				Actual:      configv1.Release{Version: "0.0.1-abc", Image: "image/image:v4.0.1"},
-				Failure: &payload.UpdateError{
+				FailureSummary: &payload.UpdateError{
 					Reason:  "UpdatePayloadIntegrity",
 					Message: "unable to apply object",
 				},
@@ -275,7 +275,7 @@ func TestOperator_sync(t *testing.T) {
 					Returns: &SyncWorkerStatus{
 						Reconciling: true,
 						Actual:      configv1.Release{Version: "0.0.1-abc", Image: "image/image:v4.0.1"},
-						Failure: &payload.UpdateError{
+						FailureSummary: &payload.UpdateError{
 							Reason:  "UpdatePayloadIntegrity",
 							Message: "unable to apply object",
 						},
@@ -352,7 +352,7 @@ func TestOperator_sync(t *testing.T) {
 						Reconciling: true,
 						Completed:   2,
 						Actual:      configv1.Release{Version: "0.0.1-abc", Image: "image/image:v4.0.1"},
-						Failure: &payload.UpdateError{
+						FailureSummary: &payload.UpdateError{
 							Reason:  "UpdatePayloadIntegrity",
 							Message: "unable to apply object",
 						},
@@ -426,9 +426,9 @@ func TestOperator_sync(t *testing.T) {
 				name:      "default",
 				configSync: &fakeSyncRecorder{
 					Returns: &SyncWorkerStatus{
-						Actual:      configv1.Release{Version: "0.0.1-abc", Image: "image/image:v4.0.1"},
-						Failure:     fmt.Errorf("injected error"),
-						VersionHash: "foo",
+						Actual:         configv1.Release{Version: "0.0.1-abc", Image: "image/image:v4.0.1"},
+						FailureSummary: fmt.Errorf("injected error"),
+						VersionHash:    "foo",
 					},
 				},
 				client: fake.NewSimpleClientset(
@@ -491,8 +491,8 @@ func TestOperator_sync(t *testing.T) {
 		{
 			name: "invalid image reports image error",
 			syncStatus: &SyncWorkerStatus{
-				Failure: os.ErrNotExist,
-				Actual:  configv1.Release{Image: "image/image:v4.0.1", Version: "4.0.1"},
+				FailureSummary: os.ErrNotExist,
+				Actual:         configv1.Release{Image: "image/image:v4.0.1", Version: "4.0.1"},
 			},
 			optr: &Operator{
 				release: configv1.Release{
@@ -546,10 +546,10 @@ func TestOperator_sync(t *testing.T) {
 		{
 			name: "invalid image while progressing preserves progressing order and partial history",
 			syncStatus: &SyncWorkerStatus{
-				Done:    600,
-				Total:   1000,
-				Failure: os.ErrNotExist,
-				Actual:  configv1.Release{Image: "image/image:v4.0.1", Version: "4.0.1"},
+				Done:           600,
+				Total:          1000,
+				FailureSummary: os.ErrNotExist,
+				Actual:         configv1.Release{Image: "image/image:v4.0.1", Version: "4.0.1"},
 			},
 			optr: &Operator{
 				release: configv1.Release{

pkg/cvo/reconciliation_issues.go

@@ -1,13 +1,76 @@
 package cvo
 
-import v1 "github.com/openshift/api/config/v1"
+import (
+	"encoding/json"
+	"strings"
+
+	configv1 "github.com/openshift/api/config/v1"
+	"github.com/openshift/cluster-version-operator/pkg/payload"
+)
 
 const (
-	reconciliationIssuesConditionType v1.ClusterStatusConditionType = "ReconciliationIssues"
+	reconciliationIssuesConditionType configv1.ClusterStatusConditionType = "ReconciliationIssues"
 
 	noReconciliationIssuesReason  string = "NoIssues"
 	noReconciliationIssuesMessage string = "No issues found during reconciliation"
 
-	reconciliationIssuesFoundReason  string = "IssuesFound"
-	reconciliationIssuesFoundMessage string = "Issues found during reconciliation"
+	reconciliationIssuesFoundReason string = "IssuesFound"
 )
+
+type ReconciliationIssueUpdateError struct {
+	NestedMessage string `json:"nestedMessage,omitempty"`
+	Effect        string `json:"effect,omitempty"`
+	Reason        string `json:"reason,omitempty"`
+	PluralReason  string `json:"pluralReason,omitempty"`
+	Message       string `json:"message,omitempty"`
+	Name          string `json:"name,omitempty"`
+	Names         string `json:"names,omitempty"`
+
+	ManifestFilename string `json:"manifestFilename,omitempty"`
+	ResourceGroup    string `json:"manifestGroup,omitempty"`
+	ResourceVersion  string `json:"resourceVersion,omitempty"`
+	ResourceKind     string `json:"resourceKind,omitempty"`
+	ResourceSummary  string `json:"resourceSummary,omitempty"`
+}
+
+type ReconciliationIssueString struct {
+	Message string `json:"message"`
+}
+
+type ReconciliationIssue struct {
+	UpdateError *ReconciliationIssueUpdateError `json:"updateError,omitempty"`
+	SimpleError *ReconciliationIssueString      `json:"simpleError,omitempty"`
+}
+
+func reconciliationIssuesFromErrors(errs []error) string {
+	var reconciliationIssues []ReconciliationIssue
+	for _, err := range errs {
+		if updateError, ok := err.(*payload.UpdateError); ok {
+			reconciliationIssues = append(reconciliationIssues, ReconciliationIssue{
+				UpdateError: &ReconciliationIssueUpdateError{
+					NestedMessage:    updateError.Nested.Error(),
+					Effect:           string(updateError.UpdateEffect),
+					Reason:           updateError.Reason,
+					PluralReason:     updateError.PluralReason,
+					Message:          updateError.Message,
+					Name:             updateError.Name,
+					Names:            strings.Join(updateError.Names, ", "),
+					ManifestFilename: updateError.Task.Manifest.OriginalFilename,
+					ResourceGroup:    updateError.Task.Manifest.GVK.Group,
+					ResourceVersion:  updateError.Task.Manifest.GVK.Version,
+					ResourceKind:     updateError.Task.Manifest.GVK.Kind,
+				},
+			})
+		} else {
+			reconciliationIssues = append(reconciliationIssues, ReconciliationIssue{
+				SimpleError: &ReconciliationIssueString{
+					Message: err.Error(),
+				},
+			})
+		}
+	}
+	if raw, err := json.Marshal(reconciliationIssues); err == nil {
+		return string(raw)
+	}
+	return ""
+}

pkg/cvo/status.go

@@ -179,7 +179,7 @@ const ImplicitlyEnabledCapabilities configv1.ClusterStatusConditionType = "Impli
 func (optr *Operator) syncStatus(ctx context.Context, original, config *configv1.ClusterVersion, status *SyncWorkerStatus, validationErrs field.ErrorList) error {
 	// Be more verbose when we are syncing something interesting
 	verbosityLevel := klog.Level(4)
-	if len(validationErrs) != 0 || status.Failure != nil {
+	if len(validationErrs) != 0 || status.FailureSummary != nil {
 		verbosityLevel = klog.Level(2)
 	}
 	klog.V(verbosityLevel).Infof("Synchronizing status errs=%#v status=%#v", validationErrs, status)
@@ -295,7 +295,7 @@ func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status
 
 	progressReason, progressMessage, skipFailure := convertErrorToProgressing(cvStatus.History, now.Time, status)
 
-	if err := status.Failure; err != nil && !skipFailure {
+	if err := status.FailureSummary; err != nil && !skipFailure {
 		var reason string
 		msg := progressMessage
 		if uErr, ok := err.(*payload.UpdateError); ok {
@@ -389,10 +389,14 @@ func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status
 			Reason:  noReconciliationIssuesReason,
 			Message: noReconciliationIssuesMessage,
 		}
-		if status.Failure != nil {
+		if status.FailureSummary != nil {
 			riCondition.Status = configv1.ConditionTrue
 			riCondition.Reason = reconciliationIssuesFoundReason
-			riCondition.Message = fmt.Sprintf("%s: %s", reconciliationIssuesFoundMessage, status.Failure.Error())
+			if len(status.Failures) == 0 { // this should not happen with non-nil FailureSummary but lets be defensive
+				riCondition.Message = reconciliationIssuesFromErrors([]error{status.FailureSummary})
+			} else {
+				riCondition.Message = reconciliationIssuesFromErrors(status.Failures)
+			}
 		}
 		resourcemerge.SetOperatorStatusCondition(&cvStatus.Conditions, riCondition)
 	} else if oldRiCondition != nil {
@@ -476,10 +480,10 @@ func setDesiredReleaseAcceptedCondition(cvStatus *configv1.ClusterVersionStatus,
 // failing. An error may indicate the update is failing or that if the error continues for a defined interval the
 // update is failing.
 func convertErrorToProgressing(history []configv1.UpdateHistory, now time.Time, status *SyncWorkerStatus) (reason string, message string, ok bool) {
-	if len(history) == 0 || status.Failure == nil || status.Reconciling {
+	if len(history) == 0 || status.FailureSummary == nil || status.Reconciling {
 		return "", "", false
 	}
-	uErr, ok := status.Failure.(*payload.UpdateError)
+	uErr, ok := status.FailureSummary.(*payload.UpdateError)
 	if !ok {
 		return "", "", false
 	}