From 9a94e4e3fa3d0823baea47a68bd0b7f3ec0a6720 Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Fri, 3 May 2024 17:41:14 +0200 Subject: [PATCH 1/4] retry status update on conflict Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- .../observabilityaddon_controller.go | 17 ++- operators/endpointmetrics/pkg/util/status.go | 103 +++++++++++----- .../endpointmetrics/pkg/util/status_test.go | 115 ++++++++++++++---- 3 files changed, 171 insertions(+), 64 deletions(-) diff --git a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go index dfd6b6718..6da6ace85 100644 --- a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go +++ b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go @@ -201,7 +201,10 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R log.Error(err, "OCP prometheus service does not exist") // ACM 8509: Special case for hub/local cluster metrics collection // We do not report status for hub endpoint operator - util.ReportStatus(ctx, r.Client, obsAddon, "NotSupported", !isHubMetricsCollector) + if !isHubMetricsCollector { + util.ReportStatus(ctx, r.Client, util.NotSupportedStatus, obsAddon.Name, obsAddon.Namespace) + } + return ctrl.Result{}, nil } return ctrl.Result{}, fmt.Errorf("failed to check prometheus resource: %w", err) @@ -297,19 +300,21 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R 1, forceRestart) if err != nil { - util.ReportStatus(ctx, r.Client, obsAddon, "Degraded", !isHubMetricsCollector) + if !isHubMetricsCollector { + util.ReportStatus(ctx, r.Client, util.DegradedStatus, obsAddon.Name, obsAddon.Namespace) + } return ctrl.Result{}, fmt.Errorf("failed to update metrics collectors: %w", err) } - if created { - util.ReportStatus(ctx, r.Client, obsAddon, "Deployed", !isHubMetricsCollector) + if created && !isHubMetricsCollector { + util.ReportStatus(ctx, r.Client, util.DeployedStatus, obsAddon.Name, obsAddon.Namespace) } } else { deleted, err := updateMetricsCollectors(ctx, r.Client, obsAddon.Spec, *hubInfo, clusterID, clusterType, 0, false) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to update metrics collectors: %w", err) } - if deleted { - util.ReportStatus(ctx, r.Client, obsAddon, "Disabled", !isHubMetricsCollector) + if deleted && !isHubMetricsCollector { + util.ReportStatus(ctx, r.Client, util.DisabledStatus, obsAddon.Name, obsAddon.Namespace) } } diff --git a/operators/endpointmetrics/pkg/util/status.go b/operators/endpointmetrics/pkg/util/status.go index f1230aa40..14894aae1 100644 --- a/operators/endpointmetrics/pkg/util/status.go +++ b/operators/endpointmetrics/pkg/util/status.go @@ -10,45 +10,84 @@ import ( oav1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) +type StatusConditionName string + +const ( + DeployedStatus StatusConditionName = "Deployed" + DisabledStatus StatusConditionName = "Disabled" + DegradedStatus StatusConditionName = "Degraded" + NotSupportedStatus StatusConditionName = "NotSupported" +) + var ( - conditions = map[string]map[string]string{ - "Deployed": { - "type": "Progressing", - "reason": "Deployed", - "message": "Metrics collector deployed"}, - "Disabled": { - "type": "Disabled", - "reason": "Disabled", - "message": "enableMetrics is set to False"}, - "Degraded": { - "type": "Degraded", - "reason": "Degraded", - "message": "Metrics collector deployment not successful"}, - "NotSupported": { - "type": "NotSupported", - "reason": "NotSupported", - "message": "No Prometheus service found in this cluster"}, + conditions = map[StatusConditionName]*oav1beta1.StatusCondition{ + DeployedStatus: { + Type: "Progressing", + Reason: "Deployed", + Message: "Metrics collector deployed", + Status: metav1.ConditionTrue, + }, + DisabledStatus: { + Type: "Disabled", + Reason: "Disabled", + Message: "enableMetrics is set to False", + Status: metav1.ConditionTrue, + }, + DegradedStatus: { + Type: "Degraded", + Reason: "Degraded", + Message: "Metrics collector deployment not successful", + Status: metav1.ConditionTrue, + }, + NotSupportedStatus: { + Type: "NotSupported", + Reason: "NotSupported", + Message: "No Prometheus service found in this cluster", + Status: metav1.ConditionTrue, + }, } ) -func ReportStatus(ctx context.Context, client client.Client, i *oav1beta1.ObservabilityAddon, t string, reportStatus bool) { - if !reportStatus { - return - } - i.Status.Conditions = []oav1beta1.StatusCondition{ - { - Type: conditions[t]["type"], - Status: metav1.ConditionTrue, - LastTransitionTime: metav1.NewTime(time.Now()), - Reason: conditions[t]["reason"], - Message: conditions[t]["message"], - }, +func ReportStatus(ctx context.Context, client client.Client, condition StatusConditionName, addonName, addonNs string) { + newCondition := conditions[condition].DeepCopy() + newCondition.LastTransitionTime = metav1.NewTime(time.Now()) + + // Fetch the ObservabilityAddon instance in local cluster, and update the status + // Retry on conflict + obsAddon := &oav1beta1.ObservabilityAddon{} + retryErr := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + if err := client.Get(ctx, types.NamespacedName{Name: addonName, Namespace: addonNs}, obsAddon); err != nil { + return err + } + + if !shouldAppendCondition(obsAddon.Status.Conditions, newCondition) { + return nil + } + + obsAddon.Status.Conditions = append(obsAddon.Status.Conditions, *newCondition) + return client.Status().Update(ctx, obsAddon) + }) + if retryErr != nil { + log.Error(retryErr, "Failed to update status for observabilityaddon") } - err := client.Status().Update(ctx, i) - if err != nil { - log.Error(err, "Failed to update status for observabilityaddon") +} + +// shouldAppendCondition checks if the new condition should be appended to the status conditions +// based on the last condition in the slice. +func shouldAppendCondition(conditions []oav1beta1.StatusCondition, newCondition *oav1beta1.StatusCondition) bool { + if len(conditions) == 0 { + return true } + + lastCondition := conditions[len(conditions)-1] + + return lastCondition.Type != newCondition.Type || + lastCondition.Status != newCondition.Status || + lastCondition.Reason != newCondition.Reason || + lastCondition.Message != newCondition.Message } diff --git a/operators/endpointmetrics/pkg/util/status_test.go b/operators/endpointmetrics/pkg/util/status_test.go index 9ef66a8ab..62c5573e1 100644 --- a/operators/endpointmetrics/pkg/util/status_test.go +++ b/operators/endpointmetrics/pkg/util/status_test.go @@ -2,17 +2,22 @@ // Copyright Contributors to the Open Cluster Management project // Licensed under the Apache License 2.0 -package util +package util_test import ( "context" "fmt" "testing" + "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/util" oav1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -38,35 +43,93 @@ func TestReportStatus(t *testing.T) { t.Fatalf("Unable to add oav1beta1 scheme: (%v)", err) } - expectedStatus := []oav1beta1.StatusCondition{ - { - Type: "NotSupported", - Status: metav1.ConditionTrue, - Reason: "NotSupported", - Message: "No Prometheus service found in this cluster", - }, - { - Type: "Progressing", - Status: metav1.ConditionTrue, - Reason: "Deployed", - Message: "Metrics collector deployed", - }, - { - Type: "Disabled", - Status: metav1.ConditionTrue, - Reason: "Disabled", - Message: "enableMetrics is set to False", - }, - } - - statusList := []string{"NotSupported", "Deployed", "Disabled"} + statusList := []util.StatusConditionName{util.NotSupportedStatus, util.DeployedStatus, util.DisabledStatus} s.AddKnownTypes(oav1beta1.GroupVersion, oa) c := fake.NewClientBuilder().WithRuntimeObjects(objs...).Build() for i := range statusList { - ReportStatus(context.TODO(), c, oa, statusList[i], true) - if oa.Status.Conditions[0].Message != expectedStatus[i].Message || oa.Status.Conditions[0].Reason != expectedStatus[i].Reason || oa.Status.Conditions[0].Status != expectedStatus[i].Status || oa.Status.Conditions[0].Type != expectedStatus[i].Type { - t.Errorf("Error: Status not updated. Expected: %s, Actual: %s", expectedStatus[i], fmt.Sprintf("%+v\n", oa.Status.Conditions[0])) + util.ReportStatus(context.Background(), c, statusList[i], oa.Name, oa.Namespace) + runtimeAddon := &oav1beta1.ObservabilityAddon{} + if err := c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: testNamespace}, runtimeAddon); err != nil { + t.Fatalf("Error getting observabilityaddon: (%v)", err) + } + + if len(runtimeAddon.Status.Conditions) != i+1 { + t.Errorf("Status not updated. Expected: %s, Actual: %s", statusList[i], fmt.Sprintf("%+v\n", runtimeAddon.Status.Conditions)) + } + + if runtimeAddon.Status.Conditions[i].Reason != string(statusList[i]) { + t.Errorf("Status not updated. Expected: %s, Actual: %s", statusList[i], runtimeAddon.Status.Conditions[i].Type) } } + // Same status should not be appended + util.ReportStatus(context.Background(), c, util.DisabledStatus, oa.Name, oa.Namespace) + runtimeAddon := &oav1beta1.ObservabilityAddon{} + if err := c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: testNamespace}, runtimeAddon); err != nil { + t.Fatalf("Error getting observabilityaddon: (%v)", err) + } + + if len(runtimeAddon.Status.Conditions) != len(statusList) { + t.Errorf("Status should not be appended. Expected: %d, Actual: %d", len(statusList), len(runtimeAddon.Status.Conditions)) + } +} + +func TestReportStatusConflict(t *testing.T) { + // Conflict on update should be retried + oa := newObservabilityAddon(name, testNamespace) + s := scheme.Scheme + oav1beta1.AddToScheme(s) + fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(oa).Build() + conflictErr := errors.NewConflict(schema.GroupResource{Group: oav1beta1.GroupVersion.Group, Resource: "resource"}, name, fmt.Errorf("conflict")) + + c := newClientWithUpdateError(fakeClient, conflictErr) + util.ReportStatus(context.Background(), c, util.DeployedStatus, name, testNamespace) + if c.UpdateCallsCount() <= 1 { + t.Errorf("Conflict error should be retried, called %d times", c.UpdateCallsCount()) + } +} + +// TestClient wraps a client.Client to customize operations for testing +type TestClient struct { + client.Client + UpdateError error + updateCallsCount int + statusWriter *TestStatusWriter +} + +func newClientWithUpdateError(c client.Client, updateError error) *TestClient { + ret := &TestClient{ + Client: c, + UpdateError: updateError, + } + ret.statusWriter = &TestStatusWriter{SubResourceWriter: c.Status(), updateError: &ret.UpdateError, callsCount: &ret.updateCallsCount} + return ret +} + +func (c *TestClient) Status() client.StatusWriter { + return c.statusWriter +} + +func (c *TestClient) UpdateCallsCount() int { + return c.updateCallsCount +} + +func (c *TestClient) Reset() { + c.updateCallsCount = 0 +} + +type TestStatusWriter struct { + client.SubResourceWriter + updateError *error + callsCount *int +} + +func (f *TestStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + *f.callsCount++ + + if *f.updateError != nil { + return *f.updateError + } + + return f.SubResourceWriter.Update(ctx, obj, opts...) } From 23bae0e43e6252b35daa10e079140fc38752079c Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Mon, 6 May 2024 10:01:29 +0200 Subject: [PATCH 2/4] add maxConditions handling Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- operators/endpointmetrics/pkg/util/status.go | 14 +++++++++---- .../endpointmetrics/pkg/util/status_test.go | 21 +++++++++++++++++-- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/operators/endpointmetrics/pkg/util/status.go b/operators/endpointmetrics/pkg/util/status.go index 14894aae1..1be91c29d 100644 --- a/operators/endpointmetrics/pkg/util/status.go +++ b/operators/endpointmetrics/pkg/util/status.go @@ -18,10 +18,11 @@ import ( type StatusConditionName string const ( - DeployedStatus StatusConditionName = "Deployed" - DisabledStatus StatusConditionName = "Disabled" - DegradedStatus StatusConditionName = "Degraded" - NotSupportedStatus StatusConditionName = "NotSupported" + DeployedStatus StatusConditionName = "Deployed" + DisabledStatus StatusConditionName = "Disabled" + DegradedStatus StatusConditionName = "Degraded" + NotSupportedStatus StatusConditionName = "NotSupported" + MaxStatusConditionsCount = 10 ) var ( @@ -70,6 +71,11 @@ func ReportStatus(ctx context.Context, client client.Client, condition StatusCon } obsAddon.Status.Conditions = append(obsAddon.Status.Conditions, *newCondition) + + if len(obsAddon.Status.Conditions) > MaxStatusConditionsCount { + obsAddon.Status.Conditions = obsAddon.Status.Conditions[len(obsAddon.Status.Conditions)-MaxStatusConditionsCount:] + } + return client.Status().Update(ctx, obsAddon) }) if retryErr != nil { diff --git a/operators/endpointmetrics/pkg/util/status_test.go b/operators/endpointmetrics/pkg/util/status_test.go index 62c5573e1..bb4916f18 100644 --- a/operators/endpointmetrics/pkg/util/status_test.go +++ b/operators/endpointmetrics/pkg/util/status_test.go @@ -43,6 +43,7 @@ func TestReportStatus(t *testing.T) { t.Fatalf("Unable to add oav1beta1 scheme: (%v)", err) } + // New status should be appended statusList := []util.StatusConditionName{util.NotSupportedStatus, util.DeployedStatus, util.DisabledStatus} s.AddKnownTypes(oav1beta1.GroupVersion, oa) c := fake.NewClientBuilder().WithRuntimeObjects(objs...).Build() @@ -62,7 +63,7 @@ func TestReportStatus(t *testing.T) { } } - // Same status should not be appended + // Same status than current one should not be appended util.ReportStatus(context.Background(), c, util.DisabledStatus, oa.Name, oa.Namespace) runtimeAddon := &oav1beta1.ObservabilityAddon{} if err := c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: testNamespace}, runtimeAddon); err != nil { @@ -72,9 +73,25 @@ func TestReportStatus(t *testing.T) { if len(runtimeAddon.Status.Conditions) != len(statusList) { t.Errorf("Status should not be appended. Expected: %d, Actual: %d", len(statusList), len(runtimeAddon.Status.Conditions)) } + + // Number of conditions should not exceed MaxStatusConditionsCount + statusList = []util.StatusConditionName{util.DeployedStatus, util.DisabledStatus, util.DegradedStatus} + for i := 0; i < util.MaxStatusConditionsCount+3; i++ { + status := statusList[i%len(statusList)] + util.ReportStatus(context.Background(), c, status, oa.Name, oa.Namespace) + } + + runtimeAddon = &oav1beta1.ObservabilityAddon{} + if err := c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: testNamespace}, runtimeAddon); err != nil { + t.Fatalf("Error getting observabilityaddon: (%v)", err) + } + + if len(runtimeAddon.Status.Conditions) != util.MaxStatusConditionsCount { + t.Errorf("Number of conditions should not exceed MaxStatusConditionsCount. Expected: %d, Actual: %d", util.MaxStatusConditionsCount, len(runtimeAddon.Status.Conditions)) + } } -func TestReportStatusConflict(t *testing.T) { +func TestReportStatus_Conflict(t *testing.T) { // Conflict on update should be retried oa := newObservabilityAddon(name, testNamespace) s := scheme.Scheme From b8ace116a4c2f80bea77810953c300b23bb1baa4 Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Mon, 6 May 2024 11:02:57 +0200 Subject: [PATCH 3/4] return err Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- .../observabilityaddon_controller.go | 16 ++++++++++++---- operators/endpointmetrics/pkg/util/status.go | 6 ++++-- .../endpointmetrics/pkg/util/status_test.go | 16 ++++++++++++---- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go index 6da6ace85..7f3d1a4b5 100644 --- a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go +++ b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go @@ -202,7 +202,9 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R // ACM 8509: Special case for hub/local cluster metrics collection // We do not report status for hub endpoint operator if !isHubMetricsCollector { - util.ReportStatus(ctx, r.Client, util.NotSupportedStatus, obsAddon.Name, obsAddon.Namespace) + if err := util.ReportStatus(ctx, r.Client, util.NotSupportedStatus, obsAddon.Name, obsAddon.Namespace); err != nil { + log.Error(err, "Failed to report status") + } } return ctrl.Result{}, nil @@ -301,12 +303,16 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R forceRestart) if err != nil { if !isHubMetricsCollector { - util.ReportStatus(ctx, r.Client, util.DegradedStatus, obsAddon.Name, obsAddon.Namespace) + if err := util.ReportStatus(ctx, r.Client, util.DegradedStatus, obsAddon.Name, obsAddon.Namespace); err != nil { + log.Error(err, "Failed to report status") + } } return ctrl.Result{}, fmt.Errorf("failed to update metrics collectors: %w", err) } if created && !isHubMetricsCollector { - util.ReportStatus(ctx, r.Client, util.DeployedStatus, obsAddon.Name, obsAddon.Namespace) + if err := util.ReportStatus(ctx, r.Client, util.DeployedStatus, obsAddon.Name, obsAddon.Namespace); err != nil { + log.Error(err, "Failed to report status") + } } } else { deleted, err := updateMetricsCollectors(ctx, r.Client, obsAddon.Spec, *hubInfo, clusterID, clusterType, 0, false) @@ -314,7 +320,9 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, fmt.Errorf("failed to update metrics collectors: %w", err) } if deleted && !isHubMetricsCollector { - util.ReportStatus(ctx, r.Client, util.DisabledStatus, obsAddon.Name, obsAddon.Namespace) + if err := util.ReportStatus(ctx, r.Client, util.DisabledStatus, obsAddon.Name, obsAddon.Namespace); err != nil { + log.Error(err, "Failed to report status") + } } } diff --git a/operators/endpointmetrics/pkg/util/status.go b/operators/endpointmetrics/pkg/util/status.go index 1be91c29d..9f8573c58 100644 --- a/operators/endpointmetrics/pkg/util/status.go +++ b/operators/endpointmetrics/pkg/util/status.go @@ -54,7 +54,7 @@ var ( } ) -func ReportStatus(ctx context.Context, client client.Client, condition StatusConditionName, addonName, addonNs string) { +func ReportStatus(ctx context.Context, client client.Client, condition StatusConditionName, addonName, addonNs string) error { newCondition := conditions[condition].DeepCopy() newCondition.LastTransitionTime = metav1.NewTime(time.Now()) @@ -79,8 +79,10 @@ func ReportStatus(ctx context.Context, client client.Client, condition StatusCon return client.Status().Update(ctx, obsAddon) }) if retryErr != nil { - log.Error(retryErr, "Failed to update status for observabilityaddon") + return retryErr } + + return nil } // shouldAppendCondition checks if the new condition should be appended to the status conditions diff --git a/operators/endpointmetrics/pkg/util/status_test.go b/operators/endpointmetrics/pkg/util/status_test.go index bb4916f18..248b527f5 100644 --- a/operators/endpointmetrics/pkg/util/status_test.go +++ b/operators/endpointmetrics/pkg/util/status_test.go @@ -48,7 +48,9 @@ func TestReportStatus(t *testing.T) { s.AddKnownTypes(oav1beta1.GroupVersion, oa) c := fake.NewClientBuilder().WithRuntimeObjects(objs...).Build() for i := range statusList { - util.ReportStatus(context.Background(), c, statusList[i], oa.Name, oa.Namespace) + if err := util.ReportStatus(context.Background(), c, statusList[i], oa.Name, oa.Namespace); err != nil { + t.Fatalf("Error reporting status: %v", err) + } runtimeAddon := &oav1beta1.ObservabilityAddon{} if err := c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: testNamespace}, runtimeAddon); err != nil { t.Fatalf("Error getting observabilityaddon: (%v)", err) @@ -64,7 +66,9 @@ func TestReportStatus(t *testing.T) { } // Same status than current one should not be appended - util.ReportStatus(context.Background(), c, util.DisabledStatus, oa.Name, oa.Namespace) + if err := util.ReportStatus(context.Background(), c, util.DisabledStatus, oa.Name, oa.Namespace); err != nil { + t.Fatalf("Error reporting status: %v", err) + } runtimeAddon := &oav1beta1.ObservabilityAddon{} if err := c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: testNamespace}, runtimeAddon); err != nil { t.Fatalf("Error getting observabilityaddon: (%v)", err) @@ -78,7 +82,9 @@ func TestReportStatus(t *testing.T) { statusList = []util.StatusConditionName{util.DeployedStatus, util.DisabledStatus, util.DegradedStatus} for i := 0; i < util.MaxStatusConditionsCount+3; i++ { status := statusList[i%len(statusList)] - util.ReportStatus(context.Background(), c, status, oa.Name, oa.Namespace) + if err := util.ReportStatus(context.Background(), c, status, oa.Name, oa.Namespace); err != nil { + t.Fatalf("Error reporting status: %v", err) + } } runtimeAddon = &oav1beta1.ObservabilityAddon{} @@ -100,7 +106,9 @@ func TestReportStatus_Conflict(t *testing.T) { conflictErr := errors.NewConflict(schema.GroupResource{Group: oav1beta1.GroupVersion.Group, Resource: "resource"}, name, fmt.Errorf("conflict")) c := newClientWithUpdateError(fakeClient, conflictErr) - util.ReportStatus(context.Background(), c, util.DeployedStatus, name, testNamespace) + if err := util.ReportStatus(context.Background(), c, util.DeployedStatus, name, testNamespace); err == nil { + t.Fatalf("Conflict error should be retried and return an error if it fails") + } if c.UpdateCallsCount() <= 1 { t.Errorf("Conflict error should be retried, called %d times", c.UpdateCallsCount()) } From 46a5c02fe29e288b9c05fdcf4abfdd1dcf012113 Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Mon, 13 May 2024 17:34:34 +0200 Subject: [PATCH 4/4] sort status condition Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- operators/endpointmetrics/pkg/util/status.go | 5 +++++ .../endpointmetrics/pkg/util/status_test.go | 19 +++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/operators/endpointmetrics/pkg/util/status.go b/operators/endpointmetrics/pkg/util/status.go index 9f8573c58..d760a7933 100644 --- a/operators/endpointmetrics/pkg/util/status.go +++ b/operators/endpointmetrics/pkg/util/status.go @@ -6,6 +6,7 @@ package util import ( "context" + "sort" "time" oav1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1" @@ -92,6 +93,10 @@ func shouldAppendCondition(conditions []oav1beta1.StatusCondition, newCondition return true } + sort.Slice(conditions, func(i, j int) bool { + return conditions[i].LastTransitionTime.Before(&conditions[j].LastTransitionTime) + }) + lastCondition := conditions[len(conditions)-1] return lastCondition.Type != newCondition.Type || diff --git a/operators/endpointmetrics/pkg/util/status_test.go b/operators/endpointmetrics/pkg/util/status_test.go index 248b527f5..7c31cba96 100644 --- a/operators/endpointmetrics/pkg/util/status_test.go +++ b/operators/endpointmetrics/pkg/util/status_test.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "testing" + "time" "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/util" oav1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1" @@ -63,15 +64,29 @@ func TestReportStatus(t *testing.T) { if runtimeAddon.Status.Conditions[i].Reason != string(statusList[i]) { t.Errorf("Status not updated. Expected: %s, Actual: %s", statusList[i], runtimeAddon.Status.Conditions[i].Type) } + + time.Sleep(1500 * time.Millisecond) // Sleep to ensure LastTransitionTime is different for each condition (1s resolution) + } + + // Change ordering of conditions: Get the list, change the order and update + runtimeAddon := &oav1beta1.ObservabilityAddon{} + if err := c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: testNamespace}, runtimeAddon); err != nil { + t.Fatalf("Error getting observabilityaddon: %v", err) + } + conditions := runtimeAddon.Status.Conditions + conditions[0], conditions[len(conditions)-1] = conditions[len(conditions)-1], conditions[0] + runtimeAddon.Status.Conditions = conditions + if err := c.Status().Update(context.Background(), runtimeAddon); err != nil { + t.Fatalf("Error updating observabilityaddon: (%v)", err) } // Same status than current one should not be appended if err := util.ReportStatus(context.Background(), c, util.DisabledStatus, oa.Name, oa.Namespace); err != nil { t.Fatalf("Error reporting status: %v", err) } - runtimeAddon := &oav1beta1.ObservabilityAddon{} + runtimeAddon = &oav1beta1.ObservabilityAddon{} if err := c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: testNamespace}, runtimeAddon); err != nil { - t.Fatalf("Error getting observabilityaddon: (%v)", err) + t.Fatalf("Error getting observabilityaddon: %v", err) } if len(runtimeAddon.Status.Conditions) != len(statusList) {