Skip to content

Commit

Permalink
status sync: propagate reconciliation errors via RRI condition
Browse files Browse the repository at this point in the history
  • Loading branch information
@petr-muller
petr-muller committed Mar 28, 2024
1 parent ccb0143 commit f9f59ce
Show file tree
Hide file tree
Showing 3 changed files with 167 additions and 10 deletions.
71 changes: 67 additions & 4 deletions pkg/cvo/reconciliation_issues.go
View file
Original file line number Diff line number Diff line change
@@ -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 ""
}
6 changes: 5 additions & 1 deletion pkg/cvo/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,11 @@ func updateClusterVersionStatus(cvStatus *configv1.ClusterVersionStatus, status
if status.FailureSummary != nil {
riCondition.Status = configv1.ConditionTrue
riCondition.Reason = reconciliationIssuesFoundReason
riCondition.Message = fmt.Sprintf("%s: %s", reconciliationIssuesFoundMessage, status.FailureSummary.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 {
Expand Down
100 changes: 95 additions & 5 deletions pkg/cvo/status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,25 @@ package cvo

import (
"context"
"encoding/json"
"fmt"
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/diff"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/client-go/tools/record"

configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/client-go/config/clientset/versioned/fake"
"github.com/openshift/library-go/pkg/manifest"

"github.com/openshift/cluster-version-operator/lib/resourcemerge"
"github.com/openshift/cluster-version-operator/pkg/payload"
)

func Test_mergeEqualVersions(t *testing.T) {
Expand Down Expand Up @@ -246,7 +250,7 @@ func TestUpdateClusterVersionStatus_UnknownVersionAndReconciliationIssues(t *tes
Type: reconciliationIssuesConditionType,
Status: configv1.ConditionTrue,
Reason: reconciliationIssuesFoundReason,
Message: "Issues found during reconciliation: Something happened",
Message: `[{"simpleError":{"message":"Something happened"}}]`,
},
},
{
Expand Down Expand Up @@ -284,6 +288,46 @@ func TestUpdateClusterVersionStatus_UnknownVersionAndReconciliationIssues(t *tes
func TestUpdateClusterVersionStatus_ReconciliationIssues(t *testing.T) {
ignoreLastTransitionTime := cmpopts.IgnoreFields(configv1.ClusterOperatorStatusCondition{}, "LastTransitionTime")

resourceUpdateError := payload.UpdateError{
Nested: fmt.Errorf("This is a nested error"),
UpdateEffect: payload.UpdateEffectReport,
Reason: "SomeReason",
PluralReason: "MultipleReasons",
Message: "Message about update error",
PluralMessageFormat: "We do not carry this to RRI",
Name: "No idea",
Names: []string{"one", "two"},
Task: &payload.Task{
Manifest: &manifest.Manifest{
OriginalFilename: "0000_00_some-configmap.yaml",
GVK: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"},
},
},
}

matchingReconciliationIssue := ReconciliationIssue{
UpdateError: &ReconciliationIssueUpdateError{
NestedMessage: "This is a nested error",
Effect: "Report",
Reason: "SomeReason",
PluralReason: "MultipleReasons",
Message: "Message about update error",
Name: "No idea",
Names: "one, two",
ManifestFilename: "0000_00_some-configmap.yaml",
ResourceGroup: "",
ResourceVersion: "v1",
ResourceKind: "ConfigMap",
ResourceSummary: "",
},
}

riAsJsonRaw, err := json.Marshal(matchingReconciliationIssue)
if err != nil {
t.Fatalf("Failed to marshal reconciliation issue: %v", err)
}
riAsJson := string(riAsJsonRaw)

testCases := []struct {
name string
syncWorkerStatus SyncWorkerStatus
Expand All @@ -293,7 +337,7 @@ func TestUpdateClusterVersionStatus_ReconciliationIssues(t *testing.T) {
expectedCondition *configv1.ClusterOperatorStatusCondition
}{
{
name: "ReconciliationIssues present and happy when gate is enabled and no failures happened",
name: "gate enabled, no failures => condition present and happy",
syncWorkerStatus: SyncWorkerStatus{},
enabled: true,
expectedCondition: &configv1.ClusterOperatorStatusCondition{
Expand All @@ -304,7 +348,7 @@ func TestUpdateClusterVersionStatus_ReconciliationIssues(t *testing.T) {
},
},
{
name: "ReconciliationIssues present and unhappy when gate is enabled and failures happened",
name: "gate enabled, failure summary present but failures empty => condition present and unhappy, backfilled from failure summary",
syncWorkerStatus: SyncWorkerStatus{
FailureSummary: fmt.Errorf("Something happened"),
},
Expand All @@ -313,13 +357,59 @@ func TestUpdateClusterVersionStatus_ReconciliationIssues(t *testing.T) {
Type: reconciliationIssuesConditionType,
Status: configv1.ConditionTrue,
Reason: reconciliationIssuesFoundReason,
Message: "Issues found during reconciliation: Something happened",
Message: `[{"simpleError":{"message":"Something happened"}}]`,
},
},
{
name: "gate enabled, update failure => condition present and unhappy",
syncWorkerStatus: SyncWorkerStatus{
FailureSummary: &resourceUpdateError,
Failures: []error{&resourceUpdateError},
},
enabled: true,
expectedCondition: &configv1.ClusterOperatorStatusCondition{
Type: reconciliationIssuesConditionType,
Status: configv1.ConditionTrue,
Reason: reconciliationIssuesFoundReason,
Message: fmt.Sprintf("[%s]", string(riAsJson)),
},
},
{
name: "gate enabled, non-update error => condition present and unhappy",
syncWorkerStatus: SyncWorkerStatus{
FailureSummary: fmt.Errorf("Something happened"),
Failures: []error{fmt.Errorf("Something happened")},
},
enabled: true,
expectedCondition: &configv1.ClusterOperatorStatusCondition{
Type: reconciliationIssuesConditionType,
Status: configv1.ConditionTrue,
Reason: reconciliationIssuesFoundReason,
Message: `[{"simpleError":{"message":"Something happened"}}]`,
},
},
{
name: "gate enabled, update and non-update error => condition present and unhappy, contains both errors",
syncWorkerStatus: SyncWorkerStatus{
FailureSummary: fmt.Errorf("Some kind of summary of both errors"),
Failures: []error{
fmt.Errorf("Something happened"),
&resourceUpdateError,
},
},
enabled: true,
expectedCondition: &configv1.ClusterOperatorStatusCondition{
Type: reconciliationIssuesConditionType,
Status: configv1.ConditionTrue,
Reason: reconciliationIssuesFoundReason,
Message: fmt.Sprintf(`[{"simpleError":{"message":"Something happened"}},%s]`, string(riAsJson)),
},
},
{
name: "ReconciliationIssues not present when gate is enabled and failures happened",
name: "gate disabled, failure summary and failures present => condition not present",
syncWorkerStatus: SyncWorkerStatus{
FailureSummary: fmt.Errorf("Something happened"),
Failures: []error{fmt.Errorf("Something happened")},
},
enabled: false,
expectedCondition: nil,
Expand Down

0 comments on commit f9f59ce

Please sign in to comment.