From 906dd5cc29b1d638923db4391e68dba44e49927b Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 2 Jun 2026 15:19:35 +0100 Subject: [PATCH 1/4] operatorstatus: add WithUpdateOperatorVersion() Add WithUpdateOperatorVersion() to ReconcileResult, allowing controllers to optionally update the operator version in the ClusterOperator status when writing their conditions. Also switches controller_status tests from fake client to envtest for accurate SSA field ownership testing. --- pkg/controllers/common_consts.go | 3 - pkg/operatorstatus/controller_status.go | 107 ++++++-- pkg/operatorstatus/controller_status_test.go | 272 +++++++++++++------ pkg/operatorstatus/operator_status.go | 2 +- 4 files changed, 278 insertions(+), 106 deletions(-) diff --git a/pkg/controllers/common_consts.go b/pkg/controllers/common_consts.go index 99ad82cc0..e5c288309 100644 --- a/pkg/controllers/common_consts.go +++ b/pkg/controllers/common_consts.go @@ -31,9 +31,6 @@ const ( // DefaultOperatorNamespace is the default namespace used for operator resources. DefaultOperatorNamespace = "openshift-cluster-api-operator" - // OperatorVersionKey is the key used to store the operator version in the ClusterOperator status. - OperatorVersionKey = "operator" - // ClusterOperatorName is the name of the ClusterOperator resource. ClusterOperatorName = "cluster-api" diff --git a/pkg/operatorstatus/controller_status.go b/pkg/operatorstatus/controller_status.go index 42928fd3c..814bc127f 100644 --- a/pkg/operatorstatus/controller_status.go +++ b/pkg/operatorstatus/controller_status.go @@ -41,6 +41,14 @@ const ( // server-side apply operations by the CAPI operator. CAPIOperatorIdentifierDomain = "capi-operator.openshift.io" + // ConditionAvailableSuffix is the suffix added to a controller prefix to + // form the controller's available condition type. + ConditionAvailableSuffix = "Available" + + // ConditionProgressingSuffix is the suffix added to a controller prefix to + // form the controller's progressing condition type. + ConditionProgressingSuffix = "Progressing" + // ReasonAsExpected is the reason for the condition when the operator is in a normal state. ReasonAsExpected = "AsExpected" @@ -59,14 +67,11 @@ const ( // ReasonNonRetryableError indicates that the controller encountered a non-retryable error. ReasonNonRetryableError = "NonRetryableError" +) - // ConditionAvailableSuffix is the suffix added to a controller prefix to - // form the controller's available condition type. - ConditionAvailableSuffix = "Available" - - // ConditionProgressingSuffix is the suffix added to a controller prefix to - // form the controller's progressing condition type. - ConditionProgressingSuffix = "Progressing" +const ( + // OperatorVersionKey is the key used to store the operator version in the ClusterOperator status. + OperatorVersionKey = "operator" ) // CAPIFieldOwner returns a qualifiedclient.FieldOwner for the given qualifier. @@ -95,6 +100,10 @@ type ReconcileResult struct { // current state if not set explicitly available *partialCondition + // if operatorVersion is set, we will update the ClusterOperator operator + // version to this value when writing status + operatorVersion string + err error requeueAfter time.Duration } @@ -118,6 +127,13 @@ func (r ReconcileResult) withError(err error) ReconcileResult { return r } +// WithUpdateOperatorVersion causes the reconcile result to also update the +// operator version when writing status to the ClusterOperator. +func (r ReconcileResult) WithUpdateOperatorVersion(operatorVersion string) ReconcileResult { + r.operatorVersion = operatorVersion + return r +} + // Result returns a reconcile.Result for controller-runtime. func (r *ReconcileResult) Result() (ctrl.Result, error) { // controller-runtime requires Result{} to be empty when returning an error. @@ -242,6 +258,65 @@ func (r *ReconcileResult) WriteClusterOperatorStatus(ctx context.Context, log lo return fmt.Errorf("failed to get ClusterOperator: %w", err) } + // Extract currently managed fields. This ensures that we preserve operator version if we're not updating it. + clusterOperatorApplyConfig, err := configv1apply.ExtractClusterOperatorStatus(co, string(CAPIFieldOwner(r.ControllerResultGenerator))) + if err != nil { + return fmt.Errorf("failed to extract ClusterOperator apply configuration: %w", err) + } + + clusterOperatorApplyConfig = clusterOperatorApplyConfig.WithUID(co.UID) + + conditions := r.constructPartialConditions(co) + conditionsUpdated := mergeConditions(conditions, co.Status.Conditions) + + releaseVersionNeedsUpdate := false + if r.operatorVersion != "" { + releaseVersionNeedsUpdate = func() bool { + for _, version := range co.Status.Versions { + if version.Name == OperatorVersionKey { + return version.Version != r.operatorVersion + } + } + + return true + }() + } + + if !conditionsUpdated && !releaseVersionNeedsUpdate { + return nil + } + + status := clusterOperatorApplyConfig.Status + if status == nil { + status = configv1apply.ClusterOperatorStatus() + } + + // Clear previously extracted conditions to avoid duplicates, as + // WithConditions appends to the existing slice. + status.Conditions = nil + + status = status.WithConditions(conditions...) + if r.operatorVersion != "" { + // Clear previously extracted versions to avoid duplicates, as + // WithVersions appends to the existing slice. + status.Versions = nil + status = status.WithVersions( + configv1apply.OperandVersion(). + WithName(OperatorVersionKey). + WithVersion(r.operatorVersion)) + } + + patch := util.ApplyConfigPatch(clusterOperatorApplyConfig.WithStatus(status)) + if err := k8sclient.Status().Patch(ctx, co, patch, CAPIFieldOwner(r.ControllerResultGenerator), client.ForceOwnership); err != nil { + return fmt.Errorf("failed to patch ClusterOperator status: %w", err) + } + + return nil +} + +// constructPartialConditions returns a set of condition apply configurations +// for the ReconcileResult. They do not yet have LastTransitionTime set. +func (r *ReconcileResult) constructPartialConditions(co *configv1.ClusterOperator) []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration { // The following behaviours are intended to implement the semantics of // - configv1.ClusterOperatorStatusAvailable // - configv1.ClusterOperatorStatusProgressing @@ -264,7 +339,6 @@ func (r *ReconcileResult) WriteClusterOperatorStatus(ctx context.Context, log lo // administrator's intervention. Currently we set it for non-retryable // errors. Otherwise we copy the previous state of the Available condition // if it exists. - conditions := []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ r.condition(ConditionProgressingSuffix, r.progressing.status, r.progressing.reason, r.progressing.message), } @@ -281,22 +355,7 @@ func (r *ReconcileResult) WriteClusterOperatorStatus(ctx context.Context, log lo } } - updated := mergeConditions(conditions, co.Status.Conditions) - if !updated { - return nil - } - - clusterOperatorApplyConfig := configv1apply.ClusterOperator(ClusterOperatorName). - WithUID(co.UID). - WithStatus(configv1apply.ClusterOperatorStatus(). - WithConditions(conditions...)) - - patch := util.ApplyConfigPatch(clusterOperatorApplyConfig) - if err := k8sclient.Status().Patch(ctx, co, patch, CAPIFieldOwner(r.ControllerResultGenerator), client.ForceOwnership); err != nil { - return fmt.Errorf("failed to patch ClusterOperator status: %w", err) - } - - return nil + return conditions } func findClusterOperatorCondition(condType configv1.ClusterStatusConditionType, conditions []configv1.ClusterOperatorStatusCondition) *configv1.ClusterOperatorStatusCondition { diff --git a/pkg/operatorstatus/controller_status_test.go b/pkg/operatorstatus/controller_status_test.go index 0b10ef356..3b5ab2fe4 100644 --- a/pkg/operatorstatus/controller_status_test.go +++ b/pkg/operatorstatus/controller_status_test.go @@ -19,6 +19,7 @@ import ( "context" "errors" "fmt" + "os" "testing" "time" @@ -27,16 +28,96 @@ import ( configv1 "github.com/openshift/api/config/v1" configv1apply "github.com/openshift/client-go/config/applyconfigurations/config/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/openshift/cluster-capi-operator/pkg/test" + "github.com/openshift/cluster-capi-operator/pkg/util" +) + +var ( + testEnv *envtest.Environment + cl client.WithWatch ) const testResultGenerator ControllerResultGenerator = "Test" +const defaultReleaseVersion = "1.0.0" + +func TestMain(m *testing.M) { + code, err := runTests(m) + if err != nil { + fmt.Fprintf(os.Stderr, "%v\n", err) + os.Exit(1) + } + + os.Exit(code) +} + +func runTests(m *testing.M) (_ int, err error) { + testEnv = &envtest.Environment{} + + _, cl, err = test.StartEnvTest(testEnv) + if err != nil { + return 1, fmt.Errorf("failed to start envtest: %w", err) + } + + defer func() { err = errors.Join(err, testEnv.Stop()) }() + + return m.Run(), nil +} + +// createClusterOperator creates a ClusterOperator in the envtest API server +// and optionally sets initial status conditions via a status update. It +// registers a cleanup function to delete the object when the test completes. +func createClusterOperator(t *testing.T, conditions []configv1.ClusterOperatorStatusCondition) *configv1.ClusterOperator { + t.Helper() + g := NewWithT(t) + + co := &configv1.ClusterOperator{ + ObjectMeta: metav1.ObjectMeta{ + Name: ClusterOperatorName, + }, + } + + g.Expect(cl.Create(t.Context(), co)).To(Succeed()) + t.Cleanup(func() { + g.Expect(client.IgnoreNotFound(cl.Delete(context.Background(), co))).To(Succeed()) + }) + + if len(conditions) > 0 { + co.Status.Conditions = conditions + g.Expect(cl.Status().Update(t.Context(), co)).To(Succeed()) + } + + return co +} + +// seedOperatorVersion performs an SSA status patch to set status.versions on +// the ClusterOperator under the given field owner. This establishes field +// ownership naturally through the API server's managed fields tracker. +func seedOperatorVersion(ctx context.Context, k8sClient client.Client, fieldOwner client.FieldOwner) error { + co := &configv1.ClusterOperator{} + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ClusterOperatorName}, co); err != nil { + return err + } + + applyConfig := configv1apply.ClusterOperator(ClusterOperatorName). + WithUID(co.UID). + WithStatus(configv1apply.ClusterOperatorStatus(). + WithVersions( + configv1apply.OperandVersion(). + WithName(OperatorVersionKey). + WithVersion(defaultReleaseVersion), + ), + ) + + patch := util.ApplyConfigPatch(applyConfig) + + return k8sClient.Status().Patch(ctx, co, patch, fieldOwner, client.ForceOwnership) +} func TestSuccess(t *testing.T) { g := NewWithT(t) @@ -383,19 +464,6 @@ func TestMergeConditions(t *testing.T) { }) } -func newFakeClient(objs ...client.Object) client.WithWatch { - scheme := runtime.NewScheme() - if err := configv1.AddToScheme(scheme); err != nil { - panic(err) - } - - return fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(objs...). - WithStatusSubresource(&configv1.ClusterOperator{}). - Build() -} - func TestWriteClusterOperatorStatus(t *testing.T) { log := testr.New(t) @@ -466,17 +534,7 @@ func TestWriteClusterOperatorStatus(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - co := &configv1.ClusterOperator{ - ObjectMeta: metav1.ObjectMeta{ - Name: ClusterOperatorName, - UID: types.UID("test-uid"), - }, - Status: configv1.ClusterOperatorStatus{ - Conditions: tc.existingConditions, - }, - } - - cl := newFakeClient(co) + co := createClusterOperator(t, tc.existingConditions) result := tc.result err := result.WriteClusterOperatorStatus(t.Context(), log, cl) @@ -505,33 +563,25 @@ func TestWriteClusterOperatorStatus(t *testing.T) { t.Run("skips patch when conditions unchanged", func(t *testing.T) { g := NewWithT(t) - co := &configv1.ClusterOperator{ - ObjectMeta: metav1.ObjectMeta{ - Name: ClusterOperatorName, - UID: types.UID("test-uid"), + createClusterOperator(t, []configv1.ClusterOperatorStatusCondition{ + { + Type: "TestProgressing", + Status: configv1.ConditionFalse, + Reason: ReasonAsExpected, + Message: "Success", + LastTransitionTime: metav1.Now(), }, - Status: configv1.ClusterOperatorStatus{ - Conditions: []configv1.ClusterOperatorStatusCondition{ - { - Type: "TestProgressing", - Status: configv1.ConditionFalse, - Reason: ReasonAsExpected, - Message: "Success", - LastTransitionTime: metav1.Now(), - }, - { - Type: "TestAvailable", - Status: configv1.ConditionTrue, - Reason: ReasonAsExpected, - Message: "Success", - LastTransitionTime: metav1.Now(), - }, - }, + { + Type: "TestAvailable", + Status: configv1.ConditionTrue, + Reason: ReasonAsExpected, + Message: "Success", + LastTransitionTime: metav1.Now(), }, - } + }) patchCalled := false - cl := interceptor.NewClient(newFakeClient(co), interceptor.Funcs{ + interceptCl := interceptor.NewClient(cl, interceptor.Funcs{ SubResourcePatch: func(ctx context.Context, c client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { patchCalled = true return c.SubResource(subResourceName).Patch(ctx, obj, patch, opts...) @@ -539,7 +589,7 @@ func TestWriteClusterOperatorStatus(t *testing.T) { }) result := testResultGenerator.Success() - err := result.WriteClusterOperatorStatus(t.Context(), log, cl) + err := result.WriteClusterOperatorStatus(t.Context(), log, interceptCl) g.Expect(err).ToNot(HaveOccurred()) g.Expect(patchCalled).To(BeFalse()) }) @@ -547,33 +597,25 @@ func TestWriteClusterOperatorStatus(t *testing.T) { t.Run("patches when conditions changed", func(t *testing.T) { g := NewWithT(t) - co := &configv1.ClusterOperator{ - ObjectMeta: metav1.ObjectMeta{ - Name: ClusterOperatorName, - UID: types.UID("test-uid"), + createClusterOperator(t, []configv1.ClusterOperatorStatusCondition{ + { + Type: "TestProgressing", + Status: configv1.ConditionTrue, + Reason: ReasonProgressing, + Message: "installing components", + LastTransitionTime: metav1.Now(), }, - Status: configv1.ClusterOperatorStatus{ - Conditions: []configv1.ClusterOperatorStatusCondition{ - { - Type: "TestProgressing", - Status: configv1.ConditionTrue, - Reason: ReasonProgressing, - Message: "installing components", - LastTransitionTime: metav1.Now(), - }, - { - Type: "TestAvailable", - Status: configv1.ConditionTrue, - Reason: ReasonAsExpected, - Message: "Success", - LastTransitionTime: metav1.Now(), - }, - }, + { + Type: "TestAvailable", + Status: configv1.ConditionTrue, + Reason: ReasonAsExpected, + Message: "Success", + LastTransitionTime: metav1.Now(), }, - } + }) patchCalled := false - cl := interceptor.NewClient(newFakeClient(co), interceptor.Funcs{ + interceptCl := interceptor.NewClient(cl, interceptor.Funcs{ SubResourcePatch: func(ctx context.Context, c client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { patchCalled = true return c.SubResource(subResourceName).Patch(ctx, obj, patch, opts...) @@ -581,7 +623,7 @@ func TestWriteClusterOperatorStatus(t *testing.T) { }) result := testResultGenerator.Success() - err := result.WriteClusterOperatorStatus(t.Context(), log, cl) + err := result.WriteClusterOperatorStatus(t.Context(), log, interceptCl) g.Expect(err).ToNot(HaveOccurred()) g.Expect(patchCalled).To(BeTrue()) }) @@ -589,14 +631,88 @@ func TestWriteClusterOperatorStatus(t *testing.T) { t.Run("returns error when ClusterOperator not found", func(t *testing.T) { g := NewWithT(t) - // No ClusterOperator seeded - cl := newFakeClient() - + // No ClusterOperator created. result := testResultGenerator.Success() err := result.WriteClusterOperatorStatus(t.Context(), log, cl) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("failed to get ClusterOperator")) }) + + t.Run("does not update versions with a different field owner when WithUpdateOperatorVersion is not set", func(t *testing.T) { + g := NewWithT(t) + + co := createClusterOperator(t, nil) + + // Seed a version under a different field owner. + g.Expect(seedOperatorVersion(t.Context(), cl, client.FieldOwner("other-owner"))).To(Succeed()) + + // Write status without WithUpdateOperatorVersion. + result := testResultGenerator.Success() + g.Expect(result.WriteClusterOperatorStatus(t.Context(), log, cl)).To(Succeed()) + + g.Expect(cl.Get(t.Context(), client.ObjectKeyFromObject(co), co)).To(Succeed()) + g.Expect(co.Status.Versions).To(ContainElement(SatisfyAll( + HaveField("Name", Equal(OperatorVersionKey)), + HaveField("Version", Equal(defaultReleaseVersion)), + ))) + }) + + t.Run("does not update versions with same field owner when WithUpdateOperatorVersion is not set", func(t *testing.T) { + g := NewWithT(t) + + co := createClusterOperator(t, nil) + + // Seed a version under the same field owner as the code under test. + g.Expect(seedOperatorVersion(t.Context(), cl, CAPIFieldOwner(testResultGenerator))).To(Succeed()) + + // Write status without WithUpdateOperatorVersion. + result := testResultGenerator.Success() + g.Expect(result.WriteClusterOperatorStatus(t.Context(), log, cl)).To(Succeed()) + + g.Expect(cl.Get(t.Context(), client.ObjectKeyFromObject(co), co)).To(Succeed()) + g.Expect(co.Status.Versions).To(ContainElement(SatisfyAll( + HaveField("Name", Equal(OperatorVersionKey)), + HaveField("Version", Equal(defaultReleaseVersion)), + ))) + }) + + t.Run("overwrites versions with a different field owner when WithUpdateOperatorVersion is set", func(t *testing.T) { + g := NewWithT(t) + + co := createClusterOperator(t, nil) + + // Seed a version under a different field owner. + g.Expect(seedOperatorVersion(t.Context(), cl, client.FieldOwner("other-owner"))).To(Succeed()) + + // Write status with WithUpdateOperatorVersion to a new version. + result := testResultGenerator.Success().WithUpdateOperatorVersion("2.0.0") + g.Expect(result.WriteClusterOperatorStatus(t.Context(), log, cl)).To(Succeed()) + + g.Expect(cl.Get(t.Context(), client.ObjectKeyFromObject(co), co)).To(Succeed()) + g.Expect(co.Status.Versions).To(ContainElement(SatisfyAll( + HaveField("Name", Equal(OperatorVersionKey)), + HaveField("Version", Equal("2.0.0")), + ))) + }) + + t.Run("overwrites versions with same field owner when WithUpdateOperatorVersion is set", func(t *testing.T) { + g := NewWithT(t) + + co := createClusterOperator(t, nil) + + // Seed a version under the same field owner as the code under test. + g.Expect(seedOperatorVersion(t.Context(), cl, CAPIFieldOwner(testResultGenerator))).To(Succeed()) + + // Write status with WithUpdateOperatorVersion to a new version. + result := testResultGenerator.Success().WithUpdateOperatorVersion("2.0.0") + g.Expect(result.WriteClusterOperatorStatus(t.Context(), log, cl)).To(Succeed()) + + g.Expect(cl.Get(t.Context(), client.ObjectKeyFromObject(co), co)).To(Succeed()) + g.Expect(co.Status.Versions).To(ConsistOf(SatisfyAll( + HaveField("Name", Equal(OperatorVersionKey)), + HaveField("Version", Equal("2.0.0")), + ))) + }) } func TestFindClusterOperatorCondition(t *testing.T) { diff --git a/pkg/operatorstatus/operator_status.go b/pkg/operatorstatus/operator_status.go index 06932b114..caa14a871 100644 --- a/pkg/operatorstatus/operator_status.go +++ b/pkg/operatorstatus/operator_status.go @@ -193,7 +193,7 @@ func (r *ClusterOperatorStatusClient) SyncStatus(ctx context.Context, co *config // OperandVersions returns the operand versions for the ClusterOperator. func (r *ClusterOperatorStatusClient) OperandVersions() []configv1.OperandVersion { - return []configv1.OperandVersion{{Name: controllers.OperatorVersionKey, Version: r.ReleaseVersion}} + return []configv1.OperandVersion{{Name: OperatorVersionKey, Version: r.ReleaseVersion}} } // NewClusterOperatorStatusCondition creates a new ClusterOperatorStatusCondition. From a2b635f84d7474c56c980df112c46a4568b3bfd3 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 2 Jun 2026 15:22:17 +0100 Subject: [PATCH 2/4] operatorstatus: convert Reason to iota type Convert Reason from string constants to an ordered iota type, enabling severity-based comparison for condition aggregation. Replaces ReasonSyncFailed with the standardised ReasonEphemeralError. --- Makefile | 8 +- go.mod | 1 + .../secretsync/secret_sync_controller.go | 4 +- pkg/operatorstatus/controller_status.go | 64 +- pkg/operatorstatus/controller_status_test.go | 54 +- pkg/operatorstatus/operator_status.go | 14 +- pkg/operatorstatus/reason_string.go | 30 + pkg/test/conditions.go | 9 +- .../x/tools/cmd/stringer/stringer.go | 715 ++++++++++++++++++ vendor/modules.txt | 1 + 10 files changed, 846 insertions(+), 54 deletions(-) create mode 100644 pkg/operatorstatus/reason_string.go create mode 100644 vendor/golang.org/x/tools/cmd/stringer/stringer.go diff --git a/Makefile b/Makefile index 73c382919..417f010cd 100644 --- a/Makefile +++ b/Makefile @@ -26,11 +26,15 @@ verify-%: make $* ./hack/verify-diff.sh -verify: fmt lint verify-ocp-manifests ## Run formatting and linting checks +verify: generate fmt lint verify-ocp-manifests ## Run formatting and linting checks test: verify unit ## Run verification and unit tests -build: bin/capi-operator bin/capi-installer bin/capi-controllers bin/machine-api-migration bin/crd-compatibility-checker bin/cluster-capi-operator-tests-ext manifests-gen ## Build all binaries +.PHONY: generate +generate: + go generate ./... + +build: generate bin/capi-operator bin/capi-installer bin/capi-controllers bin/machine-api-migration bin/crd-compatibility-checker bin/cluster-capi-operator-tests-ext manifests-gen ## Build all binaries clean: rm -rf bin/* diff --git a/go.mod b/go.mod index 580e18777..6536f61b0 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ tool ( github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests github.com/openshift/api/operator/v1/zz_generated.crd-manifests github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests + golang.org/x/tools/cmd/stringer sigs.k8s.io/controller-runtime/tools/setup-envtest ) diff --git a/pkg/controllers/secretsync/secret_sync_controller.go b/pkg/controllers/secretsync/secret_sync_controller.go index e3c35749d..ff0dc8111 100644 --- a/pkg/controllers/secretsync/secret_sync_controller.go +++ b/pkg/controllers/secretsync/secret_sync_controller.go @@ -214,9 +214,9 @@ func (r *UserDataSecretController) setDegradedCondition(ctx context.Context, log } conds := []configv1.ClusterOperatorStatusCondition{ - operatorstatus.NewClusterOperatorStatusCondition(secretSyncControllerAvailableCondition, configv1.ConditionFalse, operatorstatus.ReasonSyncFailed, + operatorstatus.NewClusterOperatorStatusCondition(secretSyncControllerAvailableCondition, configv1.ConditionFalse, operatorstatus.ReasonEphemeralError, "User Data Secret Controller failed to sync secret"), - operatorstatus.NewClusterOperatorStatusCondition(secretSyncControllerDegradedCondition, configv1.ConditionTrue, operatorstatus.ReasonSyncFailed, + operatorstatus.NewClusterOperatorStatusCondition(secretSyncControllerDegradedCondition, configv1.ConditionTrue, operatorstatus.ReasonEphemeralError, "User Data Secret Controller failed to sync secret"), } diff --git a/pkg/operatorstatus/controller_status.go b/pkg/operatorstatus/controller_status.go index 814bc127f..89ac700fb 100644 --- a/pkg/operatorstatus/controller_status.go +++ b/pkg/operatorstatus/controller_status.go @@ -48,27 +48,67 @@ const ( // ConditionProgressingSuffix is the suffix added to a controller prefix to // form the controller's progressing condition type. ConditionProgressingSuffix = "Progressing" +) + +//go:generate go run golang.org/x/tools/cmd/stringer -type=Reason -trimprefix=Reason + +// Reason is a type that represents the reason for a condition. +type Reason int + +// Reasons are ordered by severity from least to most severe. When aggregating +// reasons, only the most severe reason will be reported. +const ( + // ReasonUnknown is the default reason for a condition when the reason is not known. + // Nothing should use this. + ReasonUnknown Reason = iota // ReasonAsExpected is the reason for the condition when the operator is in a normal state. - ReasonAsExpected = "AsExpected" + ReasonAsExpected + + // ReasonUninitialized is the reason for the condition when the controller has not yet been initialized. + // This is used to indicate that the controller is not yet available. + ReasonUninitialized // ReasonProgressing indicates that the controller is progressing normally. // An observer should continue to wait. - ReasonProgressing = "Progressing" + ReasonProgressing // ReasonWaitingOnExternal indicates that the controller is waiting on an external event. // An observer should continue to wait. - ReasonWaitingOnExternal = "WaitingOnExternal" + ReasonWaitingOnExternal // ReasonEphemeralError indicates that the controller encountered an ephemeral error. // An observer should continue to wait. // If this condition persists, the ClusterOperator will eventually enter a degraded state. - ReasonEphemeralError = "EphemeralError" + ReasonEphemeralError // ReasonNonRetryableError indicates that the controller encountered a non-retryable error. - ReasonNonRetryableError = "NonRetryableError" + ReasonNonRetryableError ) +// ReasonFromString returns a Reason enum value from a string. It returns +// ReasonUnknown if the string is not a valid Reason. +func ReasonFromString(reason string) Reason { + switch reason { + case ReasonUnknown.String(): + return ReasonUnknown + case ReasonAsExpected.String(): + return ReasonAsExpected + case ReasonUninitialized.String(): + return ReasonUninitialized + case ReasonProgressing.String(): + return ReasonProgressing + case ReasonWaitingOnExternal.String(): + return ReasonWaitingOnExternal + case ReasonEphemeralError.String(): + return ReasonEphemeralError + case ReasonNonRetryableError.String(): + return ReasonNonRetryableError + default: + return ReasonUnknown + } +} + const ( // OperatorVersionKey is the key used to store the operator version in the ClusterOperator status. OperatorVersionKey = "operator" @@ -161,8 +201,8 @@ type ControllerResultGenerator string // Success returns a ReconcileResult indicating that the controller has succeeded. // Returning this result will not requeue the controller. func (c ControllerResultGenerator) Success() ReconcileResult { - return newReconcileResult(c, configv1.ConditionFalse, ReasonAsExpected, "Success"). - withAvailable(configv1.ConditionTrue, ReasonAsExpected, "Success") + return newReconcileResult(c, configv1.ConditionFalse, ReasonAsExpected.String(), "Success"). + withAvailable(configv1.ConditionTrue, ReasonAsExpected.String(), "Success") } // SuccessP is a convenience wrapper around Success that returns a pointer to the ReconcileResult. @@ -175,7 +215,7 @@ func (c ControllerResultGenerator) SuccessP() *ReconcileResult { // immediately, for example after writing status to a watched resource. // Returning this result will not requeue the controller directly. func (c ControllerResultGenerator) Progressing(message string) ReconcileResult { - return newReconcileResult(c, configv1.ConditionTrue, ReasonProgressing, message) + return newReconcileResult(c, configv1.ConditionTrue, ReasonProgressing.String(), message) } // ProgressingP is a convenience wrapper around Progressing that returns a pointer to the ReconcileResult. @@ -190,7 +230,7 @@ func (c ControllerResultGenerator) ProgressingP(message string) *ReconcileResult func (c ControllerResultGenerator) WaitingOnExternal(waitDescription string) ReconcileResult { message := fmt.Sprintf("Waiting on %s", waitDescription) - return newReconcileResult(c, configv1.ConditionTrue, ReasonWaitingOnExternal, message) + return newReconcileResult(c, configv1.ConditionTrue, ReasonWaitingOnExternal.String(), message) } // WaitingOnExternalP is a convenience wrapper around WaitingOnExternal that returns a pointer to the ReconcileResult. @@ -206,7 +246,7 @@ func (c ControllerResultGenerator) Error(err error) ReconcileResult { return c.nonRetryableError(err) } - return newReconcileResult(c, configv1.ConditionTrue, ReasonEphemeralError, err.Error()). + return newReconcileResult(c, configv1.ConditionTrue, ReasonEphemeralError.String(), err.Error()). withError(err) } @@ -233,8 +273,8 @@ func (c ControllerResultGenerator) NonRetryableErrorP(err error) *ReconcileResul } func (c ControllerResultGenerator) nonRetryableError(terminalErr error) ReconcileResult { - return newReconcileResult(c, configv1.ConditionFalse, ReasonNonRetryableError, terminalErr.Error()). - withAvailable(configv1.ConditionFalse, ReasonNonRetryableError, terminalErr.Error()). + return newReconcileResult(c, configv1.ConditionFalse, ReasonNonRetryableError.String(), terminalErr.Error()). + withAvailable(configv1.ConditionFalse, ReasonNonRetryableError.String(), terminalErr.Error()). withError(terminalErr) } diff --git a/pkg/operatorstatus/controller_status_test.go b/pkg/operatorstatus/controller_status_test.go index 3b5ab2fe4..f2b46c2f9 100644 --- a/pkg/operatorstatus/controller_status_test.go +++ b/pkg/operatorstatus/controller_status_test.go @@ -125,13 +125,13 @@ func TestSuccess(t *testing.T) { g.Expect(result.progressing).To(Equal(partialCondition{ status: configv1.ConditionFalse, - reason: ReasonAsExpected, + reason: ReasonAsExpected.String(), message: "Success", })) g.Expect(result.available).To(HaveValue(Equal(partialCondition{ status: configv1.ConditionTrue, - reason: ReasonAsExpected, + reason: ReasonAsExpected.String(), message: "Success", }))) @@ -148,7 +148,7 @@ func TestProgressing(t *testing.T) { g.Expect(result.progressing).To(Equal(partialCondition{ status: configv1.ConditionTrue, - reason: ReasonProgressing, + reason: ReasonProgressing.String(), message: "installing components", })) @@ -163,7 +163,7 @@ func TestWaitingOnExternal(t *testing.T) { g.Expect(result.progressing).To(Equal(partialCondition{ status: configv1.ConditionTrue, - reason: ReasonWaitingOnExternal, + reason: ReasonWaitingOnExternal.String(), message: "Waiting on infrastructure", })) @@ -180,7 +180,7 @@ func TestError(t *testing.T) { g.Expect(result.progressing).To(Equal(partialCondition{ status: configv1.ConditionTrue, - reason: ReasonEphemeralError, + reason: ReasonEphemeralError.String(), message: "connection refused", })) @@ -198,13 +198,13 @@ func TestError(t *testing.T) { g.Expect(result.progressing).To(Equal(partialCondition{ status: configv1.ConditionFalse, - reason: ReasonNonRetryableError, + reason: ReasonNonRetryableError.String(), message: termErr.Error(), })) g.Expect(result.available).To(HaveValue(Equal(partialCondition{ status: configv1.ConditionFalse, - reason: ReasonNonRetryableError, + reason: ReasonNonRetryableError.String(), message: termErr.Error(), }))) @@ -220,13 +220,13 @@ func TestNonRetryableError(t *testing.T) { g.Expect(result.progressing).To(Equal(partialCondition{ status: configv1.ConditionFalse, - reason: ReasonNonRetryableError, + reason: ReasonNonRetryableError.String(), message: "terminal error: bad config", })) g.Expect(result.available).To(HaveValue(Equal(partialCondition{ status: configv1.ConditionFalse, - reason: ReasonNonRetryableError, + reason: ReasonNonRetryableError.String(), message: "terminal error: bad config", }))) @@ -241,13 +241,13 @@ func TestNonRetryableError(t *testing.T) { g.Expect(result.progressing).To(Equal(partialCondition{ status: configv1.ConditionFalse, - reason: ReasonNonRetryableError, + reason: ReasonNonRetryableError.String(), message: "terminal error: already wrapped", })) g.Expect(result.available).To(HaveValue(Equal(partialCondition{ status: configv1.ConditionFalse, - reason: ReasonNonRetryableError, + reason: ReasonNonRetryableError.String(), message: "terminal error: already wrapped", }))) @@ -477,7 +477,7 @@ func TestWriteClusterOperatorStatus(t *testing.T) { { Type: "TestAvailable", Status: configv1.ConditionTrue, - Reason: ReasonAsExpected, + Reason: ReasonAsExpected.String(), Message: "Success", LastTransitionTime: metav1.Now(), }, @@ -493,42 +493,42 @@ func TestWriteClusterOperatorStatus(t *testing.T) { { name: "Success writes Progressing and Available conditions", result: testResultGenerator.Success(), - wantProgressing: expectedCondition{configv1.ConditionFalse, ReasonAsExpected, "Success"}, - wantAvailable: &expectedCondition{configv1.ConditionTrue, ReasonAsExpected, "Success"}, + wantProgressing: expectedCondition{configv1.ConditionFalse, ReasonAsExpected.String(), "Success"}, + wantAvailable: &expectedCondition{configv1.ConditionTrue, ReasonAsExpected.String(), "Success"}, }, { name: "Progressing without existing Available does not write Available", result: testResultGenerator.Progressing("installing components"), - wantProgressing: expectedCondition{configv1.ConditionTrue, ReasonProgressing, "installing components"}, + wantProgressing: expectedCondition{configv1.ConditionTrue, ReasonProgressing.String(), "installing components"}, wantAvailable: nil, }, { name: "Progressing with existing Available preserves Available", existingConditions: existingAvailable, result: testResultGenerator.Progressing("installing components"), - wantProgressing: expectedCondition{configv1.ConditionTrue, ReasonProgressing, "installing components"}, - wantAvailable: &expectedCondition{configv1.ConditionTrue, ReasonAsExpected, "Success"}, + wantProgressing: expectedCondition{configv1.ConditionTrue, ReasonProgressing.String(), "installing components"}, + wantAvailable: &expectedCondition{configv1.ConditionTrue, ReasonAsExpected.String(), "Success"}, }, { name: "Error with existing Available preserves Available", existingConditions: existingAvailable, result: testResultGenerator.Error(fmt.Errorf("connection refused")), - wantProgressing: expectedCondition{configv1.ConditionTrue, ReasonEphemeralError, "connection refused"}, - wantAvailable: &expectedCondition{configv1.ConditionTrue, ReasonAsExpected, "Success"}, + wantProgressing: expectedCondition{configv1.ConditionTrue, ReasonEphemeralError.String(), "connection refused"}, + wantAvailable: &expectedCondition{configv1.ConditionTrue, ReasonAsExpected.String(), "Success"}, }, { name: "WaitingOnExternal with existing Available preserves Available", existingConditions: existingAvailable, result: testResultGenerator.WaitingOnExternal("infrastructure"), - wantProgressing: expectedCondition{configv1.ConditionTrue, ReasonWaitingOnExternal, "Waiting on infrastructure"}, - wantAvailable: &expectedCondition{configv1.ConditionTrue, ReasonAsExpected, "Success"}, + wantProgressing: expectedCondition{configv1.ConditionTrue, ReasonWaitingOnExternal.String(), "Waiting on infrastructure"}, + wantAvailable: &expectedCondition{configv1.ConditionTrue, ReasonAsExpected.String(), "Success"}, }, { name: "NonRetryableError explicitly sets Available=False", existingConditions: existingAvailable, result: testResultGenerator.NonRetryableError(fmt.Errorf("bad config")), - wantProgressing: expectedCondition{configv1.ConditionFalse, ReasonNonRetryableError, "terminal error: bad config"}, - wantAvailable: &expectedCondition{configv1.ConditionFalse, ReasonNonRetryableError, "terminal error: bad config"}, + wantProgressing: expectedCondition{configv1.ConditionFalse, ReasonNonRetryableError.String(), "terminal error: bad config"}, + wantAvailable: &expectedCondition{configv1.ConditionFalse, ReasonNonRetryableError.String(), "terminal error: bad config"}, }, } { t.Run(tc.name, func(t *testing.T) { @@ -567,14 +567,14 @@ func TestWriteClusterOperatorStatus(t *testing.T) { { Type: "TestProgressing", Status: configv1.ConditionFalse, - Reason: ReasonAsExpected, + Reason: ReasonAsExpected.String(), Message: "Success", LastTransitionTime: metav1.Now(), }, { Type: "TestAvailable", Status: configv1.ConditionTrue, - Reason: ReasonAsExpected, + Reason: ReasonAsExpected.String(), Message: "Success", LastTransitionTime: metav1.Now(), }, @@ -601,14 +601,14 @@ func TestWriteClusterOperatorStatus(t *testing.T) { { Type: "TestProgressing", Status: configv1.ConditionTrue, - Reason: ReasonProgressing, + Reason: ReasonProgressing.String(), Message: "installing components", LastTransitionTime: metav1.Now(), }, { Type: "TestAvailable", Status: configv1.ConditionTrue, - Reason: ReasonAsExpected, + Reason: ReasonAsExpected.String(), Message: "Success", LastTransitionTime: metav1.Now(), }, diff --git a/pkg/operatorstatus/operator_status.go b/pkg/operatorstatus/operator_status.go index caa14a871..f43a1206c 100644 --- a/pkg/operatorstatus/operator_status.go +++ b/pkg/operatorstatus/operator_status.go @@ -33,11 +33,6 @@ import ( "github.com/openshift/library-go/pkg/config/clusteroperator/v1helpers" ) -const ( - // ReasonSyncFailed is the reason for the condition when the operator failed to sync resources. - ReasonSyncFailed = "SyncingFailed" -) - // ClusterOperatorStatusClient is a client for managing the status of the ClusterOperator object. type ClusterOperatorStatusClient struct { client.Client @@ -90,9 +85,8 @@ func (r *ClusterOperatorStatusClient) SetStatusDegraded(ctx context.Context, rec message := fmt.Sprintf("Failed to resync because %v", reconcileErr) conds := []configv1.ClusterOperatorStatusCondition{ - NewClusterOperatorStatusCondition(configv1.OperatorDegraded, configv1.ConditionTrue, - ReasonSyncFailed, message), - NewClusterOperatorStatusCondition(configv1.OperatorUpgradeable, configv1.ConditionFalse, ReasonAsExpected, ""), + NewClusterOperatorStatusCondition(configv1.OperatorDegraded, configv1.ConditionTrue, ReasonEphemeralError, message), + NewClusterOperatorStatusCondition(configv1.OperatorUpgradeable, configv1.ConditionFalse, ReasonAsExpected, message), } r.Recorder.Eventf(co, corev1.EventTypeWarning, "Status degraded", reconcileErr.Error()) @@ -198,13 +192,13 @@ func (r *ClusterOperatorStatusClient) OperandVersions() []configv1.OperandVersio // NewClusterOperatorStatusCondition creates a new ClusterOperatorStatusCondition. func NewClusterOperatorStatusCondition(conditionType configv1.ClusterStatusConditionType, - conditionStatus configv1.ConditionStatus, reason string, + conditionStatus configv1.ConditionStatus, reason Reason, message string) configv1.ClusterOperatorStatusCondition { return configv1.ClusterOperatorStatusCondition{ Type: conditionType, Status: conditionStatus, LastTransitionTime: metav1.Now(), - Reason: reason, + Reason: reason.String(), Message: message, } } diff --git a/pkg/operatorstatus/reason_string.go b/pkg/operatorstatus/reason_string.go new file mode 100644 index 000000000..a59938248 --- /dev/null +++ b/pkg/operatorstatus/reason_string.go @@ -0,0 +1,30 @@ +// Code generated by "stringer -type=Reason -trimprefix=Reason"; DO NOT EDIT. + +package operatorstatus + +import "strconv" + +func _() { + // An "invalid array index" compiler error signifies that the constant values have changed. + // Re-run the stringer command to generate them again. + var x [1]struct{} + _ = x[ReasonUnknown-0] + _ = x[ReasonAsExpected-1] + _ = x[ReasonUninitialized-2] + _ = x[ReasonProgressing-3] + _ = x[ReasonWaitingOnExternal-4] + _ = x[ReasonEphemeralError-5] + _ = x[ReasonNonRetryableError-6] +} + +const _Reason_name = "UnknownAsExpectedUninitializedProgressingWaitingOnExternalEphemeralErrorNonRetryableError" + +var _Reason_index = [...]uint8{0, 7, 17, 30, 41, 58, 72, 89} + +func (i Reason) String() string { + idx := int(i) - 0 + if i < 0 || idx >= len(_Reason_index)-1 { + return "Reason(" + strconv.FormatInt(int64(i), 10) + ")" + } + return _Reason_name[_Reason_index[idx]:_Reason_index[idx+1]] +} diff --git a/pkg/test/conditions.go b/pkg/test/conditions.go index f2b67f8c3..1a19df942 100644 --- a/pkg/test/conditions.go +++ b/pkg/test/conditions.go @@ -331,7 +331,14 @@ func toMatcher(v interface{}) types.GomegaMatcher { return matcher } - return gomega.Equal(v) + // Convert fmt.Stringer values (e.g. operatorstatus.Reason) to their + // string representation so that comparisons against string-typed + // condition fields succeed. + if s, ok := v.(fmt.Stringer); ok { + return gomega.BeEquivalentTo(s.String()) + } + + return gomega.BeEquivalentTo(v) } // getStringValue converts a reflect.Value to its string representation. diff --git a/vendor/golang.org/x/tools/cmd/stringer/stringer.go b/vendor/golang.org/x/tools/cmd/stringer/stringer.go new file mode 100644 index 000000000..7ff0ee8d0 --- /dev/null +++ b/vendor/golang.org/x/tools/cmd/stringer/stringer.go @@ -0,0 +1,715 @@ +// Copyright 2014 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Stringer is a tool to automate the creation of methods that satisfy the fmt.Stringer +// interface. Given the name of a (signed or unsigned) integer type T that has constants +// defined, stringer will create a new self-contained Go source file implementing +// +// func (t T) String() string +// +// The file is created in the same package and directory as the package that defines T. +// It has helpful defaults designed for use with go generate. +// +// Stringer works best with constants that are consecutive values such as created using iota, +// but creates good code regardless. In the future it might also provide custom support for +// constant sets that are bit patterns. +// +// For example, given this snippet, +// +// package painkiller +// +// type Pill int +// +// const ( +// Placebo Pill = iota +// Aspirin +// Ibuprofen +// Paracetamol +// Acetaminophen = Paracetamol +// ) +// +// running this command +// +// stringer -type=Pill +// +// in the same directory will create the file pill_string.go, in package painkiller, +// containing a definition of +// +// func (Pill) String() string +// +// That method will translate the value of a Pill constant to the string representation +// of the respective constant name, so that the call fmt.Print(painkiller.Aspirin) will +// print the string "Aspirin". +// +// Typically this process would be run using go generate, like this: +// +// //go:generate stringer -type=Pill +// +// If multiple constants have the same value, the lexically first matching name will +// be used (in the example, Acetaminophen will print as "Paracetamol"). +// +// With no arguments, it processes the package in the current directory. +// Otherwise, the arguments must name a single directory holding a Go package +// or a set of Go source files that represent a single Go package. +// +// The -type flag accepts a comma-separated list of types so a single run can +// generate methods for multiple types. The default output file is t_string.go, +// where t is the lower-cased name of the first type listed. It can be overridden +// with the -output flag. +// +// Types can also be declared in tests, in which case type declarations in the +// non-test package or its test variant are preferred over types defined in the +// package with suffix "_test". +// The default output file for type declarations in tests is t_string_test.go with t picked as above. +// +// The -linecomment flag tells stringer to generate the text of any line comment, trimmed +// of leading spaces, instead of the constant name. For instance, if the constants above had a +// Pill prefix, one could write +// +// PillAspirin // Aspirin +// +// to suppress it in the output. +// +// The -trimprefix flag specifies a prefix to remove from the constant names +// when generating the string representations. For instance, -trimprefix=Pill +// would be an alternative way to ensure that PillAspirin.String() == "Aspirin". +package main // import "golang.org/x/tools/cmd/stringer" + +import ( + "bytes" + "flag" + "fmt" + "go/ast" + "go/constant" + "go/format" + "go/token" + "go/types" + "log" + "os" + "path/filepath" + "sort" + "strings" + + "golang.org/x/tools/go/packages" +) + +var ( + typeNames = flag.String("type", "", "comma-separated list of type names; must be set") + output = flag.String("output", "", "output file name; default srcdir/_string.go") + trimprefix = flag.String("trimprefix", "", "trim the `prefix` from the generated constant names") + linecomment = flag.Bool("linecomment", false, "use line comment text as printed text when present") + buildTags = flag.String("tags", "", "comma-separated list of build tags to apply") +) + +// Usage is a replacement usage function for the flags package. +func Usage() { + fmt.Fprintf(os.Stderr, "Usage of stringer:\n") + fmt.Fprintf(os.Stderr, "\tstringer [flags] -type T [directory]\n") + fmt.Fprintf(os.Stderr, "\tstringer [flags] -type T files... # Must be a single package\n") + fmt.Fprintf(os.Stderr, "For more information, see:\n") + fmt.Fprintf(os.Stderr, "\thttps://pkg.go.dev/golang.org/x/tools/cmd/stringer\n") + fmt.Fprintf(os.Stderr, "Flags:\n") + flag.PrintDefaults() +} + +func main() { + log.SetFlags(0) + log.SetPrefix("stringer: ") + flag.Usage = Usage + flag.Parse() + if len(*typeNames) == 0 { + flag.Usage() + os.Exit(2) + } + types := strings.Split(*typeNames, ",") + var tags []string + if len(*buildTags) > 0 { + tags = strings.Split(*buildTags, ",") + } + + // We accept either one directory or a list of files. Which do we have? + args := flag.Args() + if len(args) == 0 { + // Default: process whole package in current directory. + args = []string{"."} + } + + // Parse the package once. + var dir string + // TODO(suzmue): accept other patterns for packages (directories, list of files, import paths, etc). + if len(args) == 1 && isDirectory(args[0]) { + dir = args[0] + } else { + if len(tags) != 0 { + log.Fatal("-tags option applies only to directories, not when files are specified") + } + dir = filepath.Dir(args[0]) + } + + // For each type, generate code in the first package where the type is declared. + // The order of packages is as follows: + // package x + // package x compiled for tests + // package x_test + // + // Each package pass could result in a separate generated file. + // These files must have the same package and test/not-test nature as the types + // from which they were generated. + // + // Types will be excluded when generated, to avoid repetitions. + pkgs := loadPackages(args, tags, *trimprefix, *linecomment, nil /* logf */) + sort.Slice(pkgs, func(i, j int) bool { + // Put x_test packages last. + iTest := strings.HasSuffix(pkgs[i].name, "_test") + jTest := strings.HasSuffix(pkgs[j].name, "_test") + if iTest != jTest { + return !iTest + } + + return len(pkgs[i].files) < len(pkgs[j].files) + }) + for _, pkg := range pkgs { + g := Generator{ + pkg: pkg, + } + + // Print the header and package clause. + g.Printf("// Code generated by \"stringer %s\"; DO NOT EDIT.\n", strings.Join(os.Args[1:], " ")) + g.Printf("\n") + g.Printf("package %s", g.pkg.name) + g.Printf("\n") + g.Printf("import \"strconv\"\n") // Used by all methods. + + // Run generate for types that can be found. Keep the rest for the remainingTypes iteration. + var foundTypes, remainingTypes []string + for _, typeName := range types { + values := findValues(typeName, pkg) + if len(values) > 0 { + g.generate(typeName, values) + foundTypes = append(foundTypes, typeName) + } else { + remainingTypes = append(remainingTypes, typeName) + } + } + if len(foundTypes) == 0 { + // This package didn't have any of the relevant types, skip writing a file. + continue + } + if len(remainingTypes) > 0 && output != nil && *output != "" { + log.Fatalf("cannot write to single file (-output=%q) when matching types are found in multiple packages", *output) + } + types = remainingTypes + + // Format the output. + src := g.format() + + // Write to file. + outputName := *output + if outputName == "" { + // Type names will be unique across packages since only the first + // match is picked. + // So there won't be collisions between a package compiled for tests + // and the separate package of tests (package foo_test). + outputName = filepath.Join(dir, baseName(pkg, foundTypes[0])) + } + err := os.WriteFile(outputName, src, 0o644) + if err != nil { + log.Fatalf("writing output: %s", err) + } + } + + if len(types) > 0 { + log.Fatalf("no values defined for types: %s", strings.Join(types, ",")) + } +} + +// baseName that will put the generated code together with pkg. +func baseName(pkg *Package, typename string) string { + suffix := "string.go" + if pkg.hasTestFiles { + suffix = "string_test.go" + } + return fmt.Sprintf("%s_%s", strings.ToLower(typename), suffix) +} + +// isDirectory reports whether the named file is a directory. +func isDirectory(name string) bool { + info, err := os.Stat(name) + if err != nil { + log.Fatal(err) + } + return info.IsDir() +} + +// Generator holds the state of the analysis. Primarily used to buffer +// the output for format.Source. +type Generator struct { + buf bytes.Buffer // Accumulated output. + pkg *Package // Package we are scanning. + + logf func(format string, args ...any) // test logging hook; nil when not testing +} + +func (g *Generator) Printf(format string, args ...any) { + fmt.Fprintf(&g.buf, format, args...) +} + +// File holds a single parsed file and associated data. +type File struct { + pkg *Package // Package to which this file belongs. + file *ast.File // Parsed AST. + // These fields are reset for each type being generated. + typeName string // Name of the constant type. + values []Value // Accumulator for constant values of that type. + + trimPrefix string + lineComment bool +} + +type Package struct { + name string + defs map[*ast.Ident]types.Object + files []*File + hasTestFiles bool +} + +// loadPackages analyzes the single package constructed from the patterns and tags. +// loadPackages exits if there is an error. +// +// Returns all variants (such as tests) of the package. +// +// logf is a test logging hook. It can be nil when not testing. +func loadPackages( + patterns, tags []string, + trimPrefix string, lineComment bool, + logf func(format string, args ...any), +) []*Package { + cfg := &packages.Config{ + Mode: packages.NeedName | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedSyntax | packages.NeedFiles, + // Tests are included, let the caller decide how to fold them in. + Tests: true, + BuildFlags: []string{fmt.Sprintf("-tags=%s", strings.Join(tags, " "))}, + Logf: logf, + } + pkgs, err := packages.Load(cfg, patterns...) + if err != nil { + log.Fatal(err) + } + if len(pkgs) == 0 { + log.Fatalf("error: no packages matching %v", strings.Join(patterns, " ")) + } + + out := make([]*Package, len(pkgs)) + for i, pkg := range pkgs { + p := &Package{ + name: pkg.Name, + defs: pkg.TypesInfo.Defs, + files: make([]*File, len(pkg.Syntax)), + } + + for j, file := range pkg.Syntax { + p.files[j] = &File{ + file: file, + pkg: p, + + trimPrefix: trimPrefix, + lineComment: lineComment, + } + } + + // Keep track of test files, since we might want to generated + // code that ends up in that kind of package. + // Can be replaced once https://go.dev/issue/38445 lands. + for _, f := range pkg.GoFiles { + if strings.HasSuffix(f, "_test.go") { + p.hasTestFiles = true + break + } + } + + out[i] = p + } + return out +} + +func findValues(typeName string, pkg *Package) []Value { + values := make([]Value, 0, 100) + for _, file := range pkg.files { + // Set the state for this run of the walker. + file.typeName = typeName + file.values = nil + if file.file != nil { + ast.Inspect(file.file, file.genDecl) + values = append(values, file.values...) + } + } + return values +} + +// generate produces the String method for the named type. +func (g *Generator) generate(typeName string, values []Value) { + // Generate code that will fail if the constants change value. + g.Printf("func _() {\n") + g.Printf("\t// An \"invalid array index\" compiler error signifies that the constant values have changed.\n") + g.Printf("\t// Re-run the stringer command to generate them again.\n") + g.Printf("\tvar x [1]struct{}\n") + for _, v := range values { + g.Printf("\t_ = x[%s - %s]\n", v.originalName, v.str) + } + g.Printf("}\n") + runs := splitIntoRuns(values) + // The decision of which pattern to use depends on the number of + // runs in the numbers. If there's only one, it's easy. For more than + // one, there's a tradeoff between complexity and size of the data + // and code vs. the simplicity of a map. A map takes more space, + // but so does the code. The decision here (crossover at 10) is + // arbitrary, but considers that for large numbers of runs the cost + // of the linear scan in the switch might become important, and + // rather than use yet another algorithm such as binary search, + // we punt and use a map. In any case, the likelihood of a map + // being necessary for any realistic example other than bitmasks + // is very low. And bitmasks probably deserve their own analysis, + // to be done some other day. + switch { + case len(runs) == 1: + g.buildOneRun(runs, typeName) + case len(runs) <= 10: + g.buildMultipleRuns(runs, typeName) + default: + g.buildMap(runs, typeName) + } +} + +// splitIntoRuns breaks the values into runs of contiguous sequences. +// For example, given 1,2,3,5,6,7 it returns {1,2,3},{5,6,7}. +// The input slice is known to be non-empty. +func splitIntoRuns(values []Value) [][]Value { + // We use stable sort so the lexically first name is chosen for equal elements. + sort.Stable(byValue(values)) + // Remove duplicates. Stable sort has put the one we want to print first, + // so use that one. The String method won't care about which named constant + // was the argument, so the first name for the given value is the only one to keep. + // We need to do this because identical values would cause the switch or map + // to fail to compile. + j := 1 + for i := 1; i < len(values); i++ { + if values[i].value != values[i-1].value { + values[j] = values[i] + j++ + } + } + values = values[:j] + runs := make([][]Value, 0, 10) + for len(values) > 0 { + // One contiguous sequence per outer loop. + i := 1 + for i < len(values) && values[i].value == values[i-1].value+1 { + i++ + } + runs = append(runs, values[:i]) + values = values[i:] + } + return runs +} + +// format returns the gofmt-ed contents of the Generator's buffer. +func (g *Generator) format() []byte { + src, err := format.Source(g.buf.Bytes()) + if err != nil { + // Should never happen, but can arise when developing this code. + // The user can compile the output to see the error. + log.Printf("warning: internal error: invalid Go generated: %s", err) + log.Printf("warning: compile the package to analyze the error") + return g.buf.Bytes() + } + return src +} + +// Value represents a declared constant. +type Value struct { + originalName string // The name of the constant. + name string // The name with trimmed prefix. + // The value is stored as a bit pattern alone. The boolean tells us + // whether to interpret it as an int64 or a uint64; the only place + // this matters is when sorting. + // Much of the time the str field is all we need; it is printed + // by Value.String. + value uint64 // Will be converted to int64 when needed. + signed bool // Whether the constant is a signed type. + str string // The string representation given by the "go/constant" package. +} + +func (v *Value) String() string { + return v.str +} + +// byValue lets us sort the constants into increasing order. +// We take care in the Less method to sort in signed or unsigned order, +// as appropriate. +type byValue []Value + +func (b byValue) Len() int { return len(b) } +func (b byValue) Swap(i, j int) { b[i], b[j] = b[j], b[i] } +func (b byValue) Less(i, j int) bool { + if b[i].signed { + return int64(b[i].value) < int64(b[j].value) + } + return b[i].value < b[j].value +} + +// genDecl processes one declaration clause. +func (f *File) genDecl(node ast.Node) bool { + decl, ok := node.(*ast.GenDecl) + if !ok || decl.Tok != token.CONST { + // We only care about const declarations. + return true + } + // The name of the type of the constants we are declaring. + // Can change if this is a multi-element declaration. + typ := "" + // Loop over the elements of the declaration. Each element is a ValueSpec: + // a list of names possibly followed by a type, possibly followed by values. + // If the type and value are both missing, we carry down the type (and value, + // but the "go/types" package takes care of that). + for _, spec := range decl.Specs { + vspec := spec.(*ast.ValueSpec) // Guaranteed to succeed as this is CONST. + if vspec.Type == nil && len(vspec.Values) > 0 { + // "X = 1". With no type but a value. If the constant is untyped, + // skip this vspec and reset the remembered type. + typ = "" + + // If this is a simple type conversion, remember the type. + // We don't mind if this is actually a call; a qualified call won't + // be matched (that will be SelectorExpr, not Ident), and only unusual + // situations will result in a function call that appears to be + // a type conversion. + ce, ok := vspec.Values[0].(*ast.CallExpr) + if !ok { + continue + } + id, ok := ce.Fun.(*ast.Ident) + if !ok { + continue + } + typ = id.Name + } + if vspec.Type != nil { + // "X T". We have a type. Remember it. + ident, ok := vspec.Type.(*ast.Ident) + if !ok { + continue + } + typ = ident.Name + } + if typ != f.typeName { + // This is not the type we're looking for. + continue + } + // We now have a list of names (from one line of source code) all being + // declared with the desired type. + // Grab their names and actual values and store them in f.values. + for _, name := range vspec.Names { + if name.Name == "_" { + continue + } + // This dance lets the type checker find the values for us. It's a + // bit tricky: look up the object declared by the name, find its + // types.Const, and extract its value. + obj, ok := f.pkg.defs[name] + if !ok { + log.Fatalf("no value for constant %s", name) + } + info := obj.Type().Underlying().(*types.Basic).Info() + if info&types.IsInteger == 0 { + log.Fatalf("can't handle non-integer constant type %s", typ) + } + value := obj.(*types.Const).Val() // Guaranteed to succeed as this is CONST. + if value.Kind() != constant.Int { + log.Fatalf("can't happen: constant is not an integer %s", name) + } + i64, isInt := constant.Int64Val(value) + u64, isUint := constant.Uint64Val(value) + if !isInt && !isUint { + log.Fatalf("internal error: value of %s is not an integer: %s", name, value.String()) + } + if !isInt { + u64 = uint64(i64) + } + v := Value{ + originalName: name.Name, + value: u64, + signed: info&types.IsUnsigned == 0, + str: value.String(), + } + if c := vspec.Comment; f.lineComment && c != nil && len(c.List) == 1 { + v.name = strings.TrimSpace(c.Text()) + } else { + v.name = strings.TrimPrefix(v.originalName, f.trimPrefix) + } + f.values = append(f.values, v) + } + } + return false +} + +// Helpers + +// usize returns the number of bits of the smallest unsigned integer +// type that will hold n. Used to create the smallest possible slice of +// integers to use as indexes into the concatenated strings. +func usize(n int) int { + switch { + case n < 1<<8: + return 8 + case n < 1<<16: + return 16 + default: + // 2^32 is enough constants for anyone. + return 32 + } +} + +// declareIndexAndNameVars declares the index slices and concatenated names +// strings representing the runs of values. +func (g *Generator) declareIndexAndNameVars(runs [][]Value, typeName string) { + var indexes, names []string + for i, run := range runs { + index, name := g.createIndexAndNameDecl(run, typeName, fmt.Sprintf("_%d", i)) + if len(run) != 1 { + indexes = append(indexes, index) + } + names = append(names, name) + } + g.Printf("const (\n") + for _, name := range names { + g.Printf("\t%s\n", name) + } + g.Printf(")\n\n") + + if len(indexes) > 0 { + g.Printf("var (") + for _, index := range indexes { + g.Printf("\t%s\n", index) + } + g.Printf(")\n\n") + } +} + +// declareIndexAndNameVar is the single-run version of declareIndexAndNameVars +func (g *Generator) declareIndexAndNameVar(run []Value, typeName string) { + index, name := g.createIndexAndNameDecl(run, typeName, "") + g.Printf("const %s\n", name) + g.Printf("var %s\n", index) +} + +// createIndexAndNameDecl returns the pair of declarations for the run. The caller will add "const" and "var". +func (g *Generator) createIndexAndNameDecl(run []Value, typeName string, suffix string) (string, string) { + b := new(bytes.Buffer) + indexes := make([]int, len(run)) + for i := range run { + b.WriteString(run[i].name) + indexes[i] = b.Len() + } + nameConst := fmt.Sprintf("_%s_name%s = %q", typeName, suffix, b.String()) + nameLen := b.Len() + b.Reset() + fmt.Fprintf(b, "_%s_index%s = [...]uint%d{0, ", typeName, suffix, usize(nameLen)) + for i, v := range indexes { + if i > 0 { + fmt.Fprintf(b, ", ") + } + fmt.Fprintf(b, "%d", v) + } + fmt.Fprintf(b, "}") + return b.String(), nameConst +} + +// declareNameVars declares the concatenated names string representing all the values in the runs. +func (g *Generator) declareNameVars(runs [][]Value, typeName string, suffix string) { + g.Printf("const _%s_name%s = \"", typeName, suffix) + for _, run := range runs { + for i := range run { + g.Printf("%s", run[i].name) + } + } + g.Printf("\"\n") +} + +// buildOneRun generates the variables and String method for a single run of contiguous values. +func (g *Generator) buildOneRun(runs [][]Value, typeName string) { + values := runs[0] + g.Printf("\n") + g.declareIndexAndNameVar(values, typeName) + g.Printf(stringOneRun, typeName, values[0].String()) +} + +// Arguments to format are: +// +// [1]: type name +// [2]: lowest defined value for type, as a string +const stringOneRun = `func (i %[1]s) String() string { + idx := int(i) - %[2]s + if i < %[2]s || idx >= len(_%[1]s_index)-1 { + return "%[1]s(" + strconv.FormatInt(int64(i), 10) + ")" + } + return _%[1]s_name[_%[1]s_index[idx] : _%[1]s_index[idx+1]] +} +` + +// buildMultipleRuns generates the variables and String method for multiple runs of contiguous values. +// For this pattern, a single Printf format won't do. +func (g *Generator) buildMultipleRuns(runs [][]Value, typeName string) { + g.Printf("\n") + g.declareIndexAndNameVars(runs, typeName) + g.Printf("func (i %s) String() string {\n", typeName) + g.Printf("\tswitch {\n") + for i, values := range runs { + if len(values) == 1 { + g.Printf("\tcase i == %s:\n", &values[0]) + g.Printf("\t\treturn _%s_name_%d\n", typeName, i) + continue + } + if values[0].value == 0 && !values[0].signed { + // For an unsigned lower bound of 0, "0 <= i" would be redundant. + g.Printf("\tcase i <= %s:\n", &values[len(values)-1]) + } else { + g.Printf("\tcase %s <= i && i <= %s:\n", &values[0], &values[len(values)-1]) + } + if values[0].value != 0 { + g.Printf("\t\ti -= %s\n", &values[0]) + } + g.Printf("\t\treturn _%s_name_%d[_%s_index_%d[i]:_%s_index_%d[i+1]]\n", + typeName, i, typeName, i, typeName, i) + } + g.Printf("\tdefault:\n") + g.Printf("\t\treturn \"%s(\" + strconv.FormatInt(int64(i), 10) + \")\"\n", typeName) + g.Printf("\t}\n") + g.Printf("}\n") +} + +// buildMap handles the case where the space is so sparse a map is a reasonable fallback. +// It's a rare situation but has simple code. +func (g *Generator) buildMap(runs [][]Value, typeName string) { + g.Printf("\n") + g.declareNameVars(runs, typeName, "") + g.Printf("\nvar _%s_map = map[%s]string{\n", typeName, typeName) + n := 0 + for _, values := range runs { + for _, value := range values { + g.Printf("\t%s: _%s_name[%d:%d],\n", &value, typeName, n, n+len(value.name)) + n += len(value.name) + } + } + g.Printf("}\n\n") + g.Printf(stringMap, typeName) +} + +// Argument to format is the type name. +const stringMap = `func (i %[1]s) String() string { + if str, ok := _%[1]s_map[i]; ok { + return str + } + return "%[1]s(" + strconv.FormatInt(int64(i), 10) + ")" +} +` diff --git a/vendor/modules.txt b/vendor/modules.txt index a6d861e24..753f794c0 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1800,6 +1800,7 @@ golang.org/x/text/width golang.org/x/time/rate # golang.org/x/tools v0.42.0 ## explicit; go 1.24.0 +golang.org/x/tools/cmd/stringer golang.org/x/tools/cover golang.org/x/tools/go/analysis golang.org/x/tools/go/analysis/passes/appends From ab9b3f542c92261fa6c835321d60494b6bd1faaf Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 2 Jun 2026 15:39:03 +0100 Subject: [PATCH 3/4] Move version writing to revision controller ClusterOperator version is now written by the revision controller instead of the clusteroperator controller. --- .../clusteroperator_controller.go | 2 +- .../clusteroperator_controller_test.go | 50 ------------ .../revision/revision_controller.go | 11 ++- .../revision/revision_controller_test.go | 77 +++++++++++++++++++ pkg/operatorstatus/operator_status.go | 5 -- 5 files changed, 87 insertions(+), 58 deletions(-) diff --git a/pkg/controllers/clusteroperator/clusteroperator_controller.go b/pkg/controllers/clusteroperator/clusteroperator_controller.go index ab6b79b74..d101e3328 100644 --- a/pkg/controllers/clusteroperator/clusteroperator_controller.go +++ b/pkg/controllers/clusteroperator/clusteroperator_controller.go @@ -59,7 +59,7 @@ func (r *ClusterOperatorController) Reconcile(ctx context.Context, req ctrl.Requ // represent the meaningful aggregation of all other controller statuses. // We should also only update the version after the aggregator confirms // all controllers have succeeded in the new version. - if err := r.SetStatusAvailable(ctx, message, operatorstatus.WithVersions(r.OperandVersions())); err != nil { + if err := r.SetStatusAvailable(ctx, message); err != nil { return ctrl.Result{}, fmt.Errorf("failed to set conditions for %q ClusterObject: %w", controllers.ClusterOperatorName, err) } diff --git a/pkg/controllers/clusteroperator/clusteroperator_controller_test.go b/pkg/controllers/clusteroperator/clusteroperator_controller_test.go index bbedcc255..8685eabe2 100644 --- a/pkg/controllers/clusteroperator/clusteroperator_controller_test.go +++ b/pkg/controllers/clusteroperator/clusteroperator_controller_test.go @@ -19,14 +19,12 @@ package clusteroperator import ( "context" "fmt" - "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/config" "sigs.k8s.io/controller-runtime/pkg/envtest/komega" "sigs.k8s.io/controller-runtime/pkg/metrics/server" @@ -96,31 +94,6 @@ var _ = Describe("ClusterOperator controller", func() { ), ), "should match the expected ClusterOperator status conditions") }) - - It("should update the ClusterOperator status version to the desired one", func() { - Eventually(komega.Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build()), time.Second*10).Should( - HaveField("Status.Versions", ContainElement(SatisfyAll( - HaveField("Name", Equal("operator")), - HaveField("Version", Equal(desiredOperatorReleaseVersion)), - ))), - ) - }) - - It("should update the ClusterOperator status version to the desired one when an incorrect one is present", func() { - By("setting the ClusterOperator status version to an incorrect one") - - patchBase := client.MergeFrom(capiClusterOperator.DeepCopy()) - capiClusterOperator.Status.Versions = []configv1.OperandVersion{{Name: "operator", Version: "incorrect"}} - Expect(cl.Status().Patch(ctx, capiClusterOperator, patchBase)).To(Succeed()) - - co := komega.Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build()) - Eventually(co).Should( - HaveField("Status.Versions", ContainElement(SatisfyAll( - HaveField("Name", Equal("operator")), - HaveField("Version", Equal(desiredOperatorReleaseVersion)), - ))), - ) - }) }) Context("With an unsupported platform", func() { @@ -142,29 +115,6 @@ var _ = Describe("ClusterOperator controller", func() { ContainElement(And(HaveField("Type", Equal(configv1.OperatorUpgradeable)), HaveField("Status", Equal(configv1.ConditionTrue)))), )), "should match the expected ClusterOperator status conditions") }) - - It("should update the ClusterOperator status version to the desired one", func() { - Eventually(komega.Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build())). - Should(HaveField("Status.Versions", ContainElement(SatisfyAll( - HaveField("Name", Equal("operator")), - HaveField("Version", Equal(desiredOperatorReleaseVersion)), - ))), "should match the expected ClusterOperator status versions") - }) - - It("should update the ClusterOperator status version to the desired one when an incorrect one is present", func() { - By("Setting the ClusterOperator status version to an incorrect one") - - patchBase := client.MergeFrom(capiClusterOperator.DeepCopy()) - capiClusterOperator.Status.Versions = []configv1.OperandVersion{{Name: "operator", Version: "incorrect"}} - Expect(cl.Status().Patch(ctx, capiClusterOperator, patchBase)).To(Succeed()) - - By("Checking the conditions are as expected") - Eventually(komega.Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build())). - Should(HaveField("Status.Versions", ContainElement(SatisfyAll( - HaveField("Name", Equal("operator")), - HaveField("Version", Equal(desiredOperatorReleaseVersion)), - )))) - }) }) }) diff --git a/pkg/controllers/revision/revision_controller.go b/pkg/controllers/revision/revision_controller.go index d276ff8b9..35ba511fd 100644 --- a/pkg/controllers/revision/revision_controller.go +++ b/pkg/controllers/revision/revision_controller.go @@ -122,8 +122,10 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope return opresult.NonRetryableError(errMaxRevisionsAllowed) } + upToDate := clusterAPI.Status.CurrentRevision == apiRevisions[0].Name + // Trim old revisions if the current revision is up to date - if len(apiRevisions) > 0 && clusterAPI.Status.CurrentRevision == apiRevisions[0].Name { + if len(apiRevisions) > 0 && upToDate { apiRevisions = apiRevisions[:1] } @@ -131,7 +133,12 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope return opresult.Error(fmt.Errorf("writing new revision: %w", err)) } - return opresult.Success() + reconcileResult := opresult.Success() + if upToDate { + reconcileResult = reconcileResult.WithUpdateOperatorVersion(r.ReleaseVersion) + } + + return reconcileResult } func (r *RevisionController) generateDesiredRevision(ctx context.Context) (revisiongenerator.RenderedRevision, *operatorstatus.ReconcileResult) { diff --git a/pkg/controllers/revision/revision_controller_test.go b/pkg/controllers/revision/revision_controller_test.go index c303e0e19..06ee92b0f 100644 --- a/pkg/controllers/revision/revision_controller_test.go +++ b/pkg/controllers/revision/revision_controller_test.go @@ -169,6 +169,9 @@ var _ = Describe("RevisionController", Serial, func() { Expect(co.Status.Conditions).To(test.HaveCondition(conditionTypeAvailable). WithStatus(configv1.ConditionTrue). WithReason(operatorstatus.ReasonAsExpected)) + + // Should NOT set operator version (not up to date — CurrentRevision is empty) + Expect(co.Status.Versions).To(BeEmpty()) }, defaultNodeTimeout) It("does not modify up to date revision list", func(ctx context.Context) { @@ -250,6 +253,11 @@ var _ = Describe("RevisionController", Serial, func() { // ObservedRevisionGeneration should match the ClusterAPI generation Expect(clusterAPI.Status.ObservedRevisionGeneration).To(Equal(clusterAPI.Generation)) + + // Should NOT set operator version (new revision not yet installed) + co := &configv1.ClusterOperator{} + Expect(cl.Get(ctx, client.ObjectKey{Name: "cluster-api"}, co)).To(Succeed()) + Expect(co.Status.Versions).To(BeEmpty()) }, defaultNodeTimeout) It("creates revision with empty components when no providers match the platform", func(ctx context.Context) { @@ -301,6 +309,14 @@ var _ = Describe("RevisionController", Serial, func() { Expect(clusterAPI.Status.Revisions[0].Name).To(Equal(latest.Name)) Expect(clusterAPI.Status.DesiredRevision).To(Equal(latest.Name)) Expect(clusterAPI.Status.ObservedRevisionGeneration).To(Equal(clusterAPI.Generation)) + + // Should set operator version (up to date) + co := &configv1.ClusterOperator{} + Expect(cl.Get(ctx, client.ObjectKey{Name: "cluster-api"}, co)).To(Succeed()) + Expect(co.Status.Versions).To(ContainElement(SatisfyAll( + HaveField("Name", Equal(operatorstatus.OperatorVersionKey)), + HaveField("Version", Equal("4.18.0")), + ))) }, defaultNodeTimeout) It("preserves all revisions when content changes and current is set", func(ctx context.Context) { @@ -327,6 +343,67 @@ var _ = Describe("RevisionController", Serial, func() { Expect(clusterAPI.Status.DesiredRevision).NotTo(Equal(rev1Name)) Expect(clusterAPI.Status.CurrentRevision).To(Equal(rev1Name)) Expect(clusterAPI.Status.ObservedRevisionGeneration).To(Equal(clusterAPI.Generation)) + + // Should NOT set operator version (current != latest) + co := &configv1.ClusterOperator{} + Expect(cl.Get(ctx, client.ObjectKey{Name: "cluster-api"}, co)).To(Succeed()) + Expect(co.Status.Versions).To(BeEmpty()) + }, defaultNodeTimeout) + + It("sets operator version when current revision matches latest", func(ctx context.Context) { + // BeforeEach created rev1. Set CurrentRevision to match. + Expect(kWithCtx(ctx).Get(clusterAPI)()).To(Succeed()) + Expect(clusterAPI.Status.Revisions).To(HaveLen(1)) + patch := client.MergeFrom(clusterAPI.DeepCopy()) + clusterAPI.Status.CurrentRevision = clusterAPI.Status.Revisions[0].Name + Expect(cl.Status().Patch(ctx, clusterAPI, patch)).To(Succeed()) + + // Trigger a reconcile + Eventually(kWithCtx(ctx).Update(clusterAPI, func() { + metav1.SetMetaDataAnnotation(&clusterAPI.ObjectMeta, "test", "trigger-version") + })).WithContext(ctx).Should(Succeed()) + + // Wait for the version to appear + co := &configv1.ClusterOperator{} + co.SetName("cluster-api") + Eventually(kWithCtx(ctx).Object(co)). + WithContext(ctx). + Should(HaveField("Status.Versions", ContainElement(SatisfyAll( + HaveField("Name", Equal(operatorstatus.OperatorVersionKey)), + HaveField("Version", Equal("4.18.0")), + )))) + }, defaultNodeTimeout) + + It("corrects stale operator version when current revision matches latest", func(ctx context.Context) { + // BeforeEach created rev1. Set CurrentRevision to match. + Expect(kWithCtx(ctx).Get(clusterAPI)()).To(Succeed()) + Expect(clusterAPI.Status.Revisions).To(HaveLen(1)) + patch := client.MergeFrom(clusterAPI.DeepCopy()) + clusterAPI.Status.CurrentRevision = clusterAPI.Status.Revisions[0].Name + Expect(cl.Status().Patch(ctx, clusterAPI, patch)).To(Succeed()) + + // Seed an incorrect operator version + coKey := client.ObjectKey{Name: "cluster-api"} + co := &configv1.ClusterOperator{} + Expect(cl.Get(ctx, coKey, co)).To(Succeed()) + coPatch := client.MergeFrom(co.DeepCopy()) + co.Status.Versions = []configv1.OperandVersion{{Name: operatorstatus.OperatorVersionKey, Version: "incorrect"}} + Expect(cl.Status().Patch(ctx, co, coPatch)).To(Succeed()) + + // Trigger a reconcile + Eventually(kWithCtx(ctx).Update(clusterAPI, func() { + metav1.SetMetaDataAnnotation(&clusterAPI.ObjectMeta, "test", "trigger-version-correction") + })).WithContext(ctx).Should(Succeed()) + + // Wait for the version to be corrected + co = &configv1.ClusterOperator{} + co.SetName("cluster-api") + Eventually(kWithCtx(ctx).Object(co)). + WithContext(ctx). + Should(HaveField("Status.Versions", ContainElement(SatisfyAll( + HaveField("Name", Equal(operatorstatus.OperatorVersionKey)), + HaveField("Version", Equal("4.18.0")), + )))) }, defaultNodeTimeout) It("sets Available=False with NonRetryableError when max revisions reached", func(ctx context.Context) { diff --git a/pkg/operatorstatus/operator_status.go b/pkg/operatorstatus/operator_status.go index f43a1206c..5ecb19ea9 100644 --- a/pkg/operatorstatus/operator_status.go +++ b/pkg/operatorstatus/operator_status.go @@ -185,11 +185,6 @@ func (r *ClusterOperatorStatusClient) SyncStatus(ctx context.Context, co *config return nil } -// OperandVersions returns the operand versions for the ClusterOperator. -func (r *ClusterOperatorStatusClient) OperandVersions() []configv1.OperandVersion { - return []configv1.OperandVersion{{Name: OperatorVersionKey, Version: r.ReleaseVersion}} -} - // NewClusterOperatorStatusCondition creates a new ClusterOperatorStatusCondition. func NewClusterOperatorStatusCondition(conditionType configv1.ClusterStatusConditionType, conditionStatus configv1.ConditionStatus, reason Reason, From 91201dafd441b0c0e4b0f14a04173234518c9c7e Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 2 Jun 2026 15:42:06 +0100 Subject: [PATCH 4/4] clusteroperator: aggregate sub-controller conditions Rewrite ClusterOperatorController to aggregate per-controller sub-conditions (Available/Progressing) into top-level ClusterOperator conditions. --- cmd/capi-operator/main.go | 20 +- .../clusteroperator_controller.go | 232 ++++++++- .../clusteroperator_controller_test.go | 457 +++++++++++++++--- pkg/controllers/clusteroperator/suite_test.go | 37 +- .../installer/installer_controller.go | 25 +- .../revision/revision_controller.go | 21 +- pkg/operatorstatus/controller_status.go | 23 +- pkg/operatorstatus/controller_status_test.go | 10 +- pkg/operatorstatus/watch_predicates.go | 28 ++ 9 files changed, 705 insertions(+), 148 deletions(-) diff --git a/cmd/capi-operator/main.go b/cmd/capi-operator/main.go index 21d0c2e67..d964ffdc3 100644 --- a/cmd/capi-operator/main.go +++ b/cmd/capi-operator/main.go @@ -45,8 +45,9 @@ import ( ) var ( - errPodIdentityNotSet = errors.New("POD_NAME and POD_NAMESPACE must be set") - errContainerNotInPod = errors.New("container not found in pod spec") + errPodIdentityNotSet = errors.New("POD_NAME and POD_NAMESPACE must be set") + errContainerNotInPod = errors.New("container not found in pod spec") + errInfrastructurePlatformStatusNotSet = errors.New("infrastructure platform status is not set") ) const ( @@ -106,22 +107,21 @@ func setupControllers(ctx context.Context, log logr.Logger, mgr ctrl.Manager, op return fmt.Errorf("unable to get infrastructure: %w", err) } - platform, err := util.GetPlatformFromInfra(infra) - if err != nil { - return fmt.Errorf("unable to get platform: %w", err) - } - featureGates, err := util.GetFeatureGates(ctx, log, managerName, mgr.GetConfig(), cancel) if err != nil { return fmt.Errorf("unable to get feature gates: %w", err) } + if infra.Status.PlatformStatus == nil { + return errInfrastructurePlatformStatusNotSet + } + supportedPlatform := util.IsCAPIEnabledForPlatform(featureGates, infra.Status.PlatformStatus.Type) if err := (&clusteroperator.ClusterOperatorController{ - ClusterOperatorStatusClient: operatorConfig.GetClusterOperatorStatusClient(mgr, platform, "clusteroperator"), - Scheme: mgr.GetScheme(), - IsUnsupportedPlatform: !supportedPlatform, + Client: mgr.GetClient(), + ReleaseVersion: util.GetReleaseVersion(), + IsUnsupportedPlatform: !supportedPlatform, }).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create clusteroperator controller: %w", err) } diff --git a/pkg/controllers/clusteroperator/clusteroperator_controller.go b/pkg/controllers/clusteroperator/clusteroperator_controller.go index d101e3328..f32633323 100644 --- a/pkg/controllers/clusteroperator/clusteroperator_controller.go +++ b/pkg/controllers/clusteroperator/clusteroperator_controller.go @@ -19,15 +19,20 @@ package clusteroperator import ( "context" "fmt" - - "k8s.io/apimachinery/pkg/runtime" + "slices" + "strings" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" configv1 "github.com/openshift/api/config/v1" + configv1apply "github.com/openshift/client-go/config/applyconfigurations/config/v1" "github.com/openshift/cluster-capi-operator/pkg/controllers" + "github.com/openshift/cluster-capi-operator/pkg/controllers/installer" + "github.com/openshift/cluster-capi-operator/pkg/controllers/revision" "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" + "github.com/openshift/cluster-capi-operator/pkg/util" ) const ( @@ -35,42 +40,229 @@ const ( controllerName = "ClusterOperatorController" ) -// ClusterOperatorController watches and keeps the cluster-api ClusterObject up to date. +// ClusterOperatorController watches the cluster-api ClusterOperator and +// aggregates per-controller sub-conditions into top-level conditions. type ClusterOperatorController struct { - operatorstatus.ClusterOperatorStatusClient - Scheme *runtime.Scheme + client.Client + ReleaseVersion string IsUnsupportedPlatform bool } // Reconcile reconciles the cluster-api ClusterOperator object. -func (r *ClusterOperatorController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *ClusterOperatorController) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx).WithName(controllerName) - log.Info(fmt.Sprintf("Reconciling %q ClusterObject", controllers.ClusterOperatorName)) - message := func() string { - if r.IsUnsupportedPlatform { - return capiUnsupportedPlatformMsg - } + co := &configv1.ClusterOperator{} + if err := r.Get(ctx, client.ObjectKey{Name: controllers.ClusterOperatorName}, co); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to get ClusterOperator: %w", err) + } + + log.Info("Reconciling ClusterOperator aggregation") - return "" - }() + var conditions []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration - // TODO: wrap this into status aggregation logic to get these conditions to - // represent the meaningful aggregation of all other controller statuses. - // We should also only update the version after the aggregator confirms - // all controllers have succeeded in the new version. - if err := r.SetStatusAvailable(ctx, message); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for %q ClusterObject: %w", controllers.ClusterOperatorName, err) + if r.IsUnsupportedPlatform { + conditions = r.unsupportedPlatformStatus() + } else { + conditions = r.aggregatedStatus(co.Status.Conditions) + } + + // Merge new conditions with existing conditions and patch if changes are required. + conditionsChanged := operatorstatus.MergeConditions(conditions, co.Status.Conditions) + versionChanged := r.IsUnsupportedPlatform && + currentOperatorVersion(co.Status.Versions, operatorstatus.OperatorVersionKey) != r.ReleaseVersion + + if conditionsChanged || versionChanged { + if err := r.writeStatus(ctx, co, conditions); err != nil { + return ctrl.Result{}, err + } } return ctrl.Result{}, nil } +// currentOperatorVersion returns the version string for the given key in the +// ClusterOperator's status versions list, or an empty string if not found. +func currentOperatorVersion(versions []configv1.OperandVersion, name string) string { + for i := range versions { + if versions[i].Name == name { + return versions[i].Version + } + } + + return "" +} + +func (r *ClusterOperatorController) writeStatus(ctx context.Context, co *configv1.ClusterOperator, conditions []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration) error { + applyConfig := configv1apply.ClusterOperator(controllers.ClusterOperatorName). + WithUID(co.UID). + WithStatus(configv1apply.ClusterOperatorStatus(). + WithConditions(conditions...), + ) + + // We don't run the revision controller on unsupported platforms, so we must + // write the release version here. + if r.IsUnsupportedPlatform { + applyConfig.Status = applyConfig.Status.WithVersions( + configv1apply.OperandVersion(). + WithName(operatorstatus.OperatorVersionKey). + WithVersion(r.ReleaseVersion)) + } + + if err := r.Status().Patch(ctx, co, util.ApplyConfigPatch(applyConfig), + operatorstatus.CAPIFieldOwner(controllerName), client.ForceOwnership); err != nil { + return fmt.Errorf("failed to write ClusterOperator status: %w", err) + } + + return nil +} + +// unsupportedPlatformStatus sets a fixed status with Available=true, +// Progressing=false, Degraded=false, Upgradeable=true when running on an +// unsupported platform. +func (r *ClusterOperatorController) unsupportedPlatformStatus() []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration { + return []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + condition(configv1.OperatorAvailable, configv1.ConditionTrue, operatorstatus.ReasonAsExpected, capiUnsupportedPlatformMsg), + condition(configv1.OperatorProgressing, configv1.ConditionFalse, operatorstatus.ReasonAsExpected, ""), + condition(configv1.OperatorDegraded, configv1.ConditionFalse, operatorstatus.ReasonAsExpected, ""), + condition(configv1.OperatorUpgradeable, configv1.ConditionTrue, operatorstatus.ReasonAsExpected, ""), + } +} + +type subcontrollerStatus struct { + controller operatorstatus.ControllerResultGenerator + available, progressing subcontrollerCondition +} + +type subcontrollerCondition struct { + status configv1.ConditionStatus + reason operatorstatus.Reason + message string +} + +func getSubcontrollerCondition(conditions []configv1.ClusterOperatorStatusCondition, condType configv1.ClusterStatusConditionType) subcontrollerCondition { + for i := range conditions { + if conditions[i].Type == condType { + return subcontrollerCondition{ + status: conditions[i].Status, + reason: operatorstatus.ReasonFromString(conditions[i].Reason), + message: conditions[i].Message, + } + } + } + + return subcontrollerCondition{ + status: configv1.ConditionUnknown, + reason: operatorstatus.ReasonUninitialized, + message: "initializing", + } +} + +func (r *ClusterOperatorController) aggregatedStatus(currentConditions []configv1.ClusterOperatorStatusCondition) []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration { + newConditions := []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + // The capi operator does not yet set the degraded condition. This will + // be added by automatically flagging a Progressing condition which + // lasts longer than some duration. + condition(configv1.OperatorDegraded, configv1.ConditionFalse, operatorstatus.ReasonAsExpected, ""), + + // Nothing the capi operator currently does prevents upgradeability. + // This will be added when CRD compatibility is integrated with the + // installer and revision controllers. + condition(configv1.OperatorUpgradeable, configv1.ConditionTrue, operatorstatus.ReasonAsExpected, ""), + } + + // Sub-controllers whose Progressing and Degraded conditions are aggregated + subControllers := []operatorstatus.ControllerResultGenerator{ + installer.ResultGenerator, + revision.ResultGenerator, + // TBD as they are migrated: + // - corecluster + // - infracluster + // - secretsync + // - kubeconfig + } + + subcontrollerStatuses := util.SliceMap(subControllers, func(subController operatorstatus.ControllerResultGenerator) subcontrollerStatus { + availableType := subController.SubConditionType(operatorstatus.ConditionAvailableSuffix) + progressingType := subController.SubConditionType(operatorstatus.ConditionProgressingSuffix) + + return subcontrollerStatus{ + controller: subController, + available: getSubcontrollerCondition(currentConditions, availableType), + progressing: getSubcontrollerCondition(currentConditions, progressingType), + } + }) + + isProgressing := slices.IndexFunc(subcontrollerStatuses, func(status subcontrollerStatus) bool { + return status.progressing.status == configv1.ConditionTrue || status.progressing.status == configv1.ConditionUnknown + }) >= 0 + progressingReason, progressingMessage := aggregateReasonAndMessage(subcontrollerStatuses, func(s subcontrollerStatus) subcontrollerCondition { + return s.progressing + }) + + switch { + case isProgressing: + newConditions = append(newConditions, condition(configv1.OperatorProgressing, configv1.ConditionTrue, progressingReason, progressingMessage)) + case progressingReason > operatorstatus.ReasonAsExpected: + newConditions = append(newConditions, condition(configv1.OperatorProgressing, configv1.ConditionFalse, progressingReason, progressingMessage)) + default: + newConditions = append(newConditions, condition(configv1.OperatorProgressing, configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "")) + } + + notAvailable := slices.IndexFunc(subcontrollerStatuses, func(status subcontrollerStatus) bool { + return status.available.status != configv1.ConditionTrue + }) >= 0 + availableReason, availableMessage := aggregateReasonAndMessage(subcontrollerStatuses, func(s subcontrollerStatus) subcontrollerCondition { + return s.available + }) + + if notAvailable { + newConditions = append(newConditions, condition(configv1.OperatorAvailable, configv1.ConditionFalse, availableReason, availableMessage)) + } else { + newConditions = append(newConditions, condition(configv1.OperatorAvailable, configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Cluster API Operator is available")) + } + + return newConditions +} + +func aggregateReasonAndMessage(statuses []subcontrollerStatus, extract func(subcontrollerStatus) subcontrollerCondition) (operatorstatus.Reason, string) { + var maxReason operatorstatus.Reason + + var parts []string + + for _, s := range statuses { + cond := extract(s) + if cond.reason <= operatorstatus.ReasonAsExpected { + continue + } + + if cond.reason > maxReason { + maxReason = cond.reason + } + + if cond.message != "" { + parts = append(parts, fmt.Sprintf("%s: %s", s.controller, cond.message)) + } else { + parts = append(parts, string(s.controller)) + } + } + + return maxReason, strings.Join(parts, "; ") +} + +func condition(condType configv1.ClusterStatusConditionType, status configv1.ConditionStatus, reason operatorstatus.Reason, message string) *configv1apply.ClusterOperatorStatusConditionApplyConfiguration { + return configv1apply.ClusterOperatorStatusCondition(). + WithType(condType). + WithStatus(status). + WithReason(reason.String()). + WithMessage(message) +} + // SetupWithManager sets up the controller with the Manager. func (r *ClusterOperatorController) SetupWithManager(mgr ctrl.Manager) error { if err := ctrl.NewControllerManagedBy(mgr). Named(controllerName). - For(&configv1.ClusterOperator{}, builder.WithPredicates(operatorstatus.ClusterOperatorOnceOnly())). + For(&configv1.ClusterOperator{}, builder.WithPredicates(operatorstatus.ClusterOperatorStatusChanged())). Complete(r); err != nil { return fmt.Errorf("failed to create controller: %w", err) } diff --git a/pkg/controllers/clusteroperator/clusteroperator_controller_test.go b/pkg/controllers/clusteroperator/clusteroperator_controller_test.go index 8685eabe2..bbe35611f 100644 --- a/pkg/controllers/clusteroperator/clusteroperator_controller_test.go +++ b/pkg/controllers/clusteroperator/clusteroperator_controller_test.go @@ -18,26 +18,29 @@ package clusteroperator import ( "context" - "fmt" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/config" - "sigs.k8s.io/controller-runtime/pkg/envtest/komega" "sigs.k8s.io/controller-runtime/pkg/metrics/server" configv1 "github.com/openshift/api/config/v1" + configv1apply "github.com/openshift/client-go/config/applyconfigurations/config/v1" "github.com/openshift/cluster-api-actuator-pkg/testutils" configv1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/config/v1" - corev1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/core/v1" "github.com/openshift/cluster-capi-operator/pkg/controllers" "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" + "github.com/openshift/cluster-capi-operator/pkg/test" + "github.com/openshift/cluster-capi-operator/pkg/util" ) -const desiredOperatorReleaseVersion = "this-is-the-desired-release-version" +const ( + desiredOperatorReleaseVersion = "this-is-the-desired-release-version" +) var ( mgrCancel context.CancelFunc @@ -45,79 +48,399 @@ var ( ) var _ = Describe("ClusterOperator controller", func() { - ctx := context.Background() + Context("with a supported platform", func() { + var capiClusterOperator *configv1.ClusterOperator - var ( - capiClusterOperator *configv1.ClusterOperator - testNamespaceName string - ) + BeforeEach(func(ctx context.Context) { + mgrCancel, mgrDone = startManager(false) - BeforeEach(func() { - By("Creating the cluster-api ClusterOperator") + DeferCleanup(stopManager) - capiClusterOperator = &configv1.ClusterOperator{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster-api", - }, - } - Expect(cl.Create(ctx, capiClusterOperator)).To(Succeed(), "should be able to create the 'cluster-api' ClusterOperator object") + By("Creating the cluster-api ClusterOperator with a previous release version", func() { + capiClusterOperator = &configv1.ClusterOperator{ + ObjectMeta: metav1.ObjectMeta{ + Name: controllers.ClusterOperatorName, + }, + } + Expect(cl.Create(ctx, capiClusterOperator)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + testutils.CleanupResources(Default, ctx, testEnv.Config, cl, "", &configv1.ClusterOperator{}) + }) + Expect(cl.Status().Update(ctx, capiClusterOperator)).To(Succeed()) + }) + }, defaultNodeTimeout) - By("Creating the testing namespace") + DescribeTable("rollup aggregation", + func(ctx context.Context, subConditions []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration, + expectedAvailable, expectedProgressing *test.ConditionMatcher) { + if len(subConditions) > 0 { + patchSubConditions(ctx, capiClusterOperator, subConditions...) + } - namespace := corev1resourcebuilder.Namespace().WithGenerateName("test-capi-corecluster-").Build() - Expect(cl.Create(ctx, namespace)).To(Succeed()) - testNamespaceName = namespace.Name - }) + co := kWithCtx(ctx).Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build()) + + Eventually(co). + WithContext(ctx). + WithTimeout(defaultEventuallyTimeout). + Should(SatisfyAll( + HaveField("Status.Conditions", SatisfyAll( + expectedAvailable, + expectedProgressing, + test.HaveCondition(configv1.OperatorDegraded).WithStatus(configv1.ConditionFalse), + test.HaveCondition(configv1.OperatorUpgradeable).WithStatus(configv1.ConditionTrue), + )), + )) + }, + Entry("when all sub-controllers report success", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("InstallerControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), + defaultNodeTimeout), + Entry("when installer controller is progressing but was previously available", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("InstallerControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Installing components"), + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonProgressing). + WithMessage(ContainSubstring("InstallerController")), + defaultNodeTimeout), + Entry("when revision controller is progressing but was previously available", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("InstallerControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Updating revisions"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonProgressing). + WithMessage(ContainSubstring("RevisionController")), + defaultNodeTimeout), + Entry("when both controllers are progressing but were previously available", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("InstallerControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Installing components"), + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Updating revisions"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonProgressing). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + defaultNodeTimeout), + Entry("when installer controller has a non-retryable error", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "install failed"), + subCondition("InstallerControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "install failed"), + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(ContainSubstring("InstallerController")), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(ContainSubstring("InstallerController")), + defaultNodeTimeout), + Entry("when revision controller has a non-retryable error", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("InstallerControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerAvailable", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "revision failed"), + subCondition("RevisionControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "revision failed"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(ContainSubstring("RevisionController")), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(ContainSubstring("RevisionController")), + defaultNodeTimeout), + Entry("when installer controller has an ephemeral error but was previously available", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("InstallerControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonEphemeralError, "transient failure"), + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonEphemeralError). + WithMessage(ContainSubstring("InstallerController")), + defaultNodeTimeout), + Entry("when installer controller sub-conditions are missing", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonUninitialized). + WithMessage(ContainSubstring("InstallerController")), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonUninitialized). + WithMessage(ContainSubstring("InstallerController")), + defaultNodeTimeout), + Entry("when all sub-conditions are missing", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{}, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonUninitialized). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + ContainSubstring("initializing"), + )), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonUninitialized). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + ContainSubstring("initializing"), + )), + defaultNodeTimeout), + Entry("when installer has not yet reported available during initial install", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Installing components"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonUninitialized). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonProgressing). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + defaultNodeTimeout), + Entry("when one controller has a non-retryable error and the other is progressing", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "install failed"), + subCondition("InstallerControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "install failed"), + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Updating revisions"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(ContainSubstring("InstallerController")), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + defaultNodeTimeout), + Entry("when both controllers have non-retryable errors", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "install failed"), + subCondition("InstallerControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "install failed"), + subCondition("RevisionControllerAvailable", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "revision failed"), + subCondition("RevisionControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "revision failed"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + defaultNodeTimeout), + Entry("when installer is waiting on external", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonWaitingOnExternal, "Waiting on ClusterAPI"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonUninitialized). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonWaitingOnExternal). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + defaultNodeTimeout), - AfterEach(func() { - testutils.CleanupResources(Default, ctx, testEnv.Config, cl, testNamespaceName, &configv1.ClusterOperator{}) + // Prioritisation tests: when sub-controllers report different reasons, + // the aggregated condition should report the highest priority reason. + Entry("should report the highest priority progressing reason", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("InstallerControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonEphemeralError, "transient failure"), + subCondition("RevisionControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition("RevisionControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Updating revisions"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonEphemeralError). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + defaultNodeTimeout), + Entry("should report the highest priority available reason", + []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + subCondition("InstallerControllerAvailable", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "install failed"), + subCondition("InstallerControllerProgressing", configv1.ConditionFalse, operatorstatus.ReasonNonRetryableError, "install failed"), + subCondition("RevisionControllerAvailable", configv1.ConditionFalse, operatorstatus.ReasonUninitialized, ""), + subCondition("RevisionControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Updating revisions"), + }, + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + test.HaveCondition(configv1.OperatorProgressing). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(SatisfyAll( + ContainSubstring("InstallerController"), + ContainSubstring("RevisionController"), + )), + defaultNodeTimeout), + ) }) - Context("With a supported platform", func() { - JustBeforeEach(func() { - mgrCancel, mgrDone = startManager(false) - }) + Context("with an unsupported platform", Ordered, func() { + var capiClusterOperator *configv1.ClusterOperator - JustAfterEach(func() { - stopManager() - }) + BeforeAll(func() { + mgrCancel, mgrDone = startManager(true) - It("should update the ClusterOperator status with the running version", func() { - co := komega.Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build()) - Eventually(co).Should(HaveField("Status.Conditions", - SatisfyAll( - ContainElement(And(HaveField("Type", Equal(configv1.OperatorAvailable)), HaveField("Status", Equal(configv1.ConditionTrue)), - HaveField("Message", Equal(fmt.Sprintf("Cluster CAPI Operator is available at %s", desiredOperatorReleaseVersion))))), - ContainElement(And(HaveField("Type", Equal(configv1.OperatorProgressing)), HaveField("Status", Equal(configv1.ConditionFalse)))), - ContainElement(And(HaveField("Type", Equal(configv1.OperatorDegraded)), HaveField("Status", Equal(configv1.ConditionFalse)))), - ContainElement(And(HaveField("Type", Equal(configv1.OperatorUpgradeable)), HaveField("Status", Equal(configv1.ConditionTrue)))), - ), - ), "should match the expected ClusterOperator status conditions") + DeferCleanup(stopManager) }) - }) - Context("With an unsupported platform", func() { - JustBeforeEach(func() { - mgrCancel, mgrDone = startManager(true) - }) + BeforeEach(func(ctx context.Context) { + By("Creating the cluster-api ClusterOperator", func() { + capiClusterOperator = &configv1.ClusterOperator{ + ObjectMeta: metav1.ObjectMeta{ + Name: controllers.ClusterOperatorName, + }, + } + Expect(cl.Create(ctx, capiClusterOperator)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + testutils.CleanupResources(Default, ctx, testEnv.Config, cl, "", &configv1.ClusterOperator{}) + }) + }) + }, defaultNodeTimeout) - JustAfterEach(func() { - stopManager() - }) + It("should set Available=True with unsupported message and write versions without reading sub-conditions", func(ctx context.Context) { + co := kWithCtx(ctx).Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build()) - It("should update the ClusterOperator status with an 'unsupported' message", func() { - Eventually(komega.Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build())). - Should(HaveField("Status.Conditions", SatisfyAll( - ContainElement(And(HaveField("Type", Equal(configv1.OperatorAvailable)), HaveField("Status", Equal(configv1.ConditionTrue)), - HaveField("Message", Equal("Cluster API is not yet implemented on this platform")))), - ContainElement(And(HaveField("Type", Equal(configv1.OperatorProgressing)), HaveField("Status", Equal(configv1.ConditionFalse)))), - ContainElement(And(HaveField("Type", Equal(configv1.OperatorDegraded)), HaveField("Status", Equal(configv1.ConditionFalse)))), - ContainElement(And(HaveField("Type", Equal(configv1.OperatorUpgradeable)), HaveField("Status", Equal(configv1.ConditionTrue)))), - )), "should match the expected ClusterOperator status conditions") - }) + Eventually(co). + WithContext(ctx). + WithTimeout(defaultEventuallyTimeout). + Should(SatisfyAll( + HaveField("Status.Conditions", SatisfyAll( + test.HaveCondition(configv1.OperatorAvailable). + WithStatus(configv1.ConditionTrue). + WithMessage(capiUnsupportedPlatformMsg), + test.HaveCondition(configv1.OperatorProgressing).WithStatus(configv1.ConditionFalse), + test.HaveCondition(configv1.OperatorDegraded).WithStatus(configv1.ConditionFalse), + test.HaveCondition(configv1.OperatorUpgradeable).WithStatus(configv1.ConditionTrue), + )), + HaveField("Status.Versions", ContainElement(SatisfyAll( + HaveField("Name", Equal(operatorstatus.OperatorVersionKey)), + HaveField("Version", Equal(desiredOperatorReleaseVersion)), + ))), + )) + }, defaultNodeTimeout) + + It("should update an incorrect version", func(ctx context.Context) { + By("Setting the ClusterOperator status version to an incorrect one") + + patchBase := client.MergeFrom(capiClusterOperator.DeepCopy()) + capiClusterOperator.Status.Versions = []configv1.OperandVersion{{Name: operatorstatus.OperatorVersionKey, Version: "old"}} + Expect(cl.Status().Patch(ctx, capiClusterOperator, patchBase)).To(Succeed()) + + By("Checking the version is corrected") + Eventually(kWithCtx(ctx).Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build())). + WithContext(ctx). + WithTimeout(defaultEventuallyTimeout). + Should(HaveField("Status.Versions", ContainElement(SatisfyAll( + HaveField("Name", Equal(operatorstatus.OperatorVersionKey)), + HaveField("Version", Equal(desiredOperatorReleaseVersion)), + )))) + }, defaultNodeTimeout) }) }) +func patchSubConditions(ctx context.Context, co *configv1.ClusterOperator, conditions ...*configv1apply.ClusterOperatorStatusConditionApplyConfiguration) { + applyConfig := configv1apply.ClusterOperator(controllers.ClusterOperatorName). + WithUID(co.UID). + WithStatus(configv1apply.ClusterOperatorStatus(). + WithConditions(conditions...)) + + Expect(cl.Status().Patch(ctx, co, util.ApplyConfigPatch(applyConfig), + operatorstatus.CAPIFieldOwner("test-sub-conditions"), client.ForceOwnership)).To(Succeed()) +} + +func subCondition(condType string, status configv1.ConditionStatus, reason operatorstatus.Reason, message string) *configv1apply.ClusterOperatorStatusConditionApplyConfiguration { + return configv1apply.ClusterOperatorStatusCondition(). + WithType(configv1.ClusterStatusConditionType(condType)). + WithStatus(status). + WithReason(reason.String()). + WithMessage(message). + WithLastTransitionTime(metav1.Now()) +} + func startManager(isUnsupportedPlatform bool) (context.CancelFunc, chan struct{}) { mgrCtx, mgrCancel := context.WithCancel(context.Background()) mgrDone := make(chan struct{}) @@ -134,8 +457,9 @@ func startManager(isUnsupportedPlatform bool) (context.CancelFunc, chan struct{} Expect(err).ToNot(HaveOccurred(), "Manager should be able to be created") r := &ClusterOperatorController{ - ClusterOperatorStatusClient: operatorstatus.ClusterOperatorStatusClient{Client: cl, ReleaseVersion: desiredOperatorReleaseVersion}, - IsUnsupportedPlatform: isUnsupportedPlatform, + Client: cl, + ReleaseVersion: desiredOperatorReleaseVersion, + IsUnsupportedPlatform: isUnsupportedPlatform, } Expect(r.SetupWithManager(mgr)).To(Succeed(), "Reconciler should be able to setup with manager") @@ -151,8 +475,9 @@ func startManager(isUnsupportedPlatform bool) (context.CancelFunc, chan struct{} return mgrCancel, mgrDone } -func stopManager() { - By("Stopping the manager") - mgrCancel() - Eventually(mgrDone).Should(BeClosed()) +func stopManager(ctx context.Context) { + By("Stopping the manager", func() { + mgrCancel() + Eventually(mgrDone).WithContext(ctx).WithTimeout(defaultEventuallyTimeout).Should(BeClosed()) + }) } diff --git a/pkg/controllers/clusteroperator/suite_test.go b/pkg/controllers/clusteroperator/suite_test.go index 84f7acae3..4e5e87174 100644 --- a/pkg/controllers/clusteroperator/suite_test.go +++ b/pkg/controllers/clusteroperator/suite_test.go @@ -19,6 +19,7 @@ package clusteroperator import ( "context" "testing" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -33,14 +34,22 @@ import ( "github.com/openshift/cluster-capi-operator/pkg/test" ) +var ( + defaultNodeTimeout = NodeTimeout(15 * time.Second) + defaultEventuallyTimeout = 5 * time.Second +) + var ( testEnv *envtest.Environment cfg *rest.Config cl client.Client testScheme *runtime.Scheme - ctx = context.Background() ) +func kWithCtx(ctx context.Context) komega.Komega { + return komega.New(cl).WithContext(ctx) +} + func TestAPIs(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Controller Suite") @@ -49,21 +58,21 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { logf.SetLogger(klog.Background()) - By("bootstrapping test environment") + By("bootstrapping test environment", func() { + var err error - var err error + testEnv = &envtest.Environment{} + cfg, cl, err = test.StartEnvTest(testEnv) + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + Expect(cl).NotTo(BeNil()) + }) - testEnv = &envtest.Environment{} - cfg, cl, err = test.StartEnvTest(testEnv) - Expect(err).NotTo(HaveOccurred()) - Expect(cfg).NotTo(BeNil()) - Expect(cl).NotTo(BeNil()) + DeferCleanup(func() { + By("tearing down the test environment", func() { + Expect(test.StopEnvTest(testEnv)).To(Succeed()) + }) + }) komega.SetClient(cl) - komega.SetContext(ctx) -}) - -var _ = AfterSuite(func() { - By("tearing down the test environment") - Expect(test.StopEnvTest(testEnv)).To(Succeed()) }) diff --git a/pkg/controllers/installer/installer_controller.go b/pkg/controllers/installer/installer_controller.go index 832d90320..cbd3036f3 100644 --- a/pkg/controllers/installer/installer_controller.go +++ b/pkg/controllers/installer/installer_controller.go @@ -54,7 +54,8 @@ const ( controllerName = "InstallerController" clusterAPIName = "cluster" - opresult = operatorstatus.ControllerResultGenerator(controllerName) + // ResultGenerator is the controller result generator for the InstallerController. + ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) ) // InstallerController reconciles ClusterAPI revisions, using boxcutter to apply @@ -205,18 +206,18 @@ func (c *InstallerController) reconcile(ctx context.Context, log logr.Logger) op clusterAPI := &operatorv1alpha1.ClusterAPI{} if err := c.client.Get(ctx, client.ObjectKey{Name: clusterAPIName}, clusterAPI); err != nil { if apierrors.IsNotFound(err) { - return opresult.WaitingOnExternal("ClusterAPI") + return ResultGenerator.WaitingOnExternal("ClusterAPI") } - return opresult.Error(fmt.Errorf("fetching ClusterAPI: %w", err)) + return ResultGenerator.Error(fmt.Errorf("fetching ClusterAPI: %w", err)) } if len(clusterAPI.Status.Revisions) == 0 { if err := writeRelatedObjects(ctx, c.client, staticRelatedObjects()); err != nil { - return opresult.Error(fmt.Errorf("writing relatedObjects: %w", err)) + return ResultGenerator.Error(fmt.Errorf("writing relatedObjects: %w", err)) } - return opresult.WaitingOnExternal("ClusterAPI revisions") + return ResultGenerator.WaitingOnExternal("ClusterAPI revisions") } revisionReconciler := newRevisionReconciler(c, log) @@ -226,12 +227,12 @@ func (c *InstallerController) reconcile(ctx context.Context, log logr.Logger) op // write never claims ownership of the relatedObjects field. relatedObjects := mergeRelatedObjects(staticRelatedObjects(), revisionReconciler.dynamicRelatedObjects()) if err := writeRelatedObjects(ctx, c.client, relatedObjects); err != nil { - return opresult.Error(fmt.Errorf("writing relatedObjects: %w", err)) + return ResultGenerator.Error(fmt.Errorf("writing relatedObjects: %w", err)) } // Update tracking cache watches for all current revisions if err := c.updateWatches(ctx, log, clusterAPI, revisionReconciler.gvks); err != nil { - return opresult.Error(err) + return ResultGenerator.Error(err) } if reconciledRevision != nil { @@ -242,7 +243,7 @@ func (c *InstallerController) reconcile(ctx context.Context, log logr.Logger) op return c.error(errs) } - return opresult.Progressing(strings.Join(messages, "\n")) + return ResultGenerator.Progressing(strings.Join(messages, "\n")) } func (c *InstallerController) success(ctx context.Context, log logr.Logger, clusterAPI *operatorv1alpha1.ClusterAPI, reconciledRevision operatorv1alpha1.RevisionName, errs []error) operatorstatus.ReconcileResult { @@ -252,10 +253,10 @@ func (c *InstallerController) success(ctx context.Context, log logr.Logger, clus // Write the current revision to the ClusterAPI status in its own SSA transaction if err := c.writeCurrentRevision(ctx, clusterAPI, reconciledRevision); err != nil { - return opresult.Error(fmt.Errorf("writing current revision: %w", err)) + return ResultGenerator.Error(fmt.Errorf("writing current revision: %w", err)) } - return opresult.Success() + return ResultGenerator.Success() } func (c *InstallerController) error(errs []error) operatorstatus.ReconcileResult { @@ -286,10 +287,10 @@ func (c *InstallerController) error(errs []error) operatorstatus.ReconcileResult }) nonTerminalErrors = append(nonTerminalErrors, unwrappedTerminalErrors...) - return opresult.Error(fmt.Errorf("reconciling revisions: %w", errors.Join(nonTerminalErrors...))) + return ResultGenerator.Error(fmt.Errorf("reconciling revisions: %w", errors.Join(nonTerminalErrors...))) } - return opresult.NonRetryableError(fmt.Errorf("reconciling revisions: %w", errors.Join(errs...))) + return ResultGenerator.NonRetryableError(fmt.Errorf("reconciling revisions: %w", errors.Join(errs...))) } func (c *InstallerController) updateWatches(ctx context.Context, log logr.Logger, clusterAPI *operatorv1alpha1.ClusterAPI, allGVKs sets.Set[schema.GroupVersionKind]) error { diff --git a/pkg/controllers/revision/revision_controller.go b/pkg/controllers/revision/revision_controller.go index 35ba511fd..a76d65a6e 100644 --- a/pkg/controllers/revision/revision_controller.go +++ b/pkg/controllers/revision/revision_controller.go @@ -52,7 +52,8 @@ const ( infrastructureName = "cluster" maxRevisionsAllowed = 16 - opresult = operatorstatus.ControllerResultGenerator(controllerName) + // ResultGenerator is the controller result generator for the RevisionController. + ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) ) var ( @@ -99,10 +100,10 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope clusterAPI := &operatorv1alpha1.ClusterAPI{} if err := r.Get(ctx, client.ObjectKey{Name: clusterAPIName}, clusterAPI); err != nil { if apierrors.IsNotFound(err) { - return opresult.WaitingOnExternal("ClusterAPI not found") + return ResultGenerator.WaitingOnExternal("ClusterAPI not found") } - return opresult.Error(fmt.Errorf("fetching ClusterAPI: %w", err)) + return ResultGenerator.Error(fmt.Errorf("fetching ClusterAPI: %w", err)) } // Create a reverse sorted, merged list of revisions. It will prepend the @@ -110,7 +111,7 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope // first, and there is guaranteed to be at least one revision. apiRevisions, err := r.mergeRevisions(log, clusterAPI.Status.Revisions, desiredRevision) if err != nil { - return opresult.Error(fmt.Errorf("error merging revisions: %w", err)) + return ResultGenerator.Error(fmt.Errorf("error merging revisions: %w", err)) } // We can't proceed if we exceed the max number of revisions. In normal @@ -119,7 +120,7 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope // so we should stop. There is no safe way to automatically prune revisions // in this case. This requires manual intervention. if len(apiRevisions) > maxRevisionsAllowed { - return opresult.NonRetryableError(errMaxRevisionsAllowed) + return ResultGenerator.NonRetryableError(errMaxRevisionsAllowed) } upToDate := clusterAPI.Status.CurrentRevision == apiRevisions[0].Name @@ -130,10 +131,10 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope } if err := r.writeRevisions(ctx, log, clusterAPI, apiRevisions); err != nil { - return opresult.Error(fmt.Errorf("writing new revision: %w", err)) + return ResultGenerator.Error(fmt.Errorf("writing new revision: %w", err)) } - reconcileResult := opresult.Success() + reconcileResult := ResultGenerator.Success() if upToDate { reconcileResult = reconcileResult.WithUpdateOperatorVersion(r.ReleaseVersion) } @@ -144,11 +145,11 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope func (r *RevisionController) generateDesiredRevision(ctx context.Context) (revisiongenerator.RenderedRevision, *operatorstatus.ReconcileResult) { infra := &configv1.Infrastructure{} if err := r.Get(ctx, client.ObjectKey{Name: infrastructureName}, infra); err != nil { - return nil, opresult.ErrorP(fmt.Errorf("fetching infrastructure: %w", err)) + return nil, ResultGenerator.ErrorP(fmt.Errorf("fetching infrastructure: %w", err)) } if infra.Status.PlatformStatus == nil { - return nil, opresult.WaitingOnExternalP("Infrastructure PlatformStatus") + return nil, ResultGenerator.WaitingOnExternalP("Infrastructure PlatformStatus") } // Build ordered component list from provider metadata @@ -156,7 +157,7 @@ func (r *RevisionController) generateDesiredRevision(ctx context.Context) (revis revision, err := revisiongenerator.NewRenderedRevision(providerComponents, revisiongenerator.WithManifestSubstitutions(r.manifestSubstitutions)) if err != nil { - return nil, opresult.ErrorP(fmt.Errorf("error creating rendered revision: %w", err)) + return nil, ResultGenerator.ErrorP(fmt.Errorf("error creating rendered revision: %w", err)) } return revision, nil diff --git a/pkg/operatorstatus/controller_status.go b/pkg/operatorstatus/controller_status.go index 89ac700fb..7316ac7e2 100644 --- a/pkg/operatorstatus/controller_status.go +++ b/pkg/operatorstatus/controller_status.go @@ -50,6 +50,11 @@ const ( ConditionProgressingSuffix = "Progressing" ) +const ( + // OperatorVersionKey is the key used to store the operator version in the ClusterOperator status. + OperatorVersionKey = "operator" +) + //go:generate go run golang.org/x/tools/cmd/stringer -type=Reason -trimprefix=Reason // Reason is a type that represents the reason for a condition. @@ -109,11 +114,6 @@ func ReasonFromString(reason string) Reason { } } -const ( - // OperatorVersionKey is the key used to store the operator version in the ClusterOperator status. - OperatorVersionKey = "operator" -) - // CAPIFieldOwner returns a qualifiedclient.FieldOwner for the given qualifier. // The qualifier should identify the writer in the context of the CAPI operator, // for example a controller name. @@ -280,13 +280,14 @@ func (c ControllerResultGenerator) nonRetryableError(terminalErr error) Reconcil func (c ControllerResultGenerator) condition(condType string, status configv1.ConditionStatus, reason, message string) *configv1apply.ClusterOperatorStatusConditionApplyConfiguration { return configv1apply.ClusterOperatorStatusCondition(). - WithType(c.conditionType(condType)). + WithType(c.SubConditionType(condType)). WithStatus(status). WithReason(reason). WithMessage(message) } -func (c ControllerResultGenerator) conditionType(condType string) configv1.ClusterStatusConditionType { +// SubConditionType returns the ClusterStatusConditionType for the given condition type. +func (c ControllerResultGenerator) SubConditionType(condType string) configv1.ClusterStatusConditionType { return configv1.ClusterStatusConditionType(string(c) + condType) } @@ -307,7 +308,7 @@ func (r *ReconcileResult) WriteClusterOperatorStatus(ctx context.Context, log lo clusterOperatorApplyConfig = clusterOperatorApplyConfig.WithUID(co.UID) conditions := r.constructPartialConditions(co) - conditionsUpdated := mergeConditions(conditions, co.Status.Conditions) + conditionsUpdated := MergeConditions(conditions, co.Status.Conditions) releaseVersionNeedsUpdate := false if r.operatorVersion != "" { @@ -389,7 +390,7 @@ func (r *ReconcileResult) constructPartialConditions(co *configv1.ClusterOperato } else { // Infer Available condition from existing state, don't write if not // already present - currentAvailable := findClusterOperatorCondition(r.conditionType(ConditionAvailableSuffix), co.Status.Conditions) + currentAvailable := findClusterOperatorCondition(r.SubConditionType(ConditionAvailableSuffix), co.Status.Conditions) if currentAvailable != nil { conditions = append(conditions, r.condition(ConditionAvailableSuffix, currentAvailable.Status, currentAvailable.Reason, currentAvailable.Message)) } @@ -408,10 +409,10 @@ func findClusterOperatorCondition(condType configv1.ClusterStatusConditionType, return nil } -// mergeConditions sets LastTransitionTime on each new condition based on the +// MergeConditions sets LastTransitionTime on each new condition based on the // existing conditions. If a condition's Status/Reason/Message are unchanged, // LastTransitionTime is preserved from the existing condition. -func mergeConditions(newConditions []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration, existingConditions []configv1.ClusterOperatorStatusCondition) bool { +func MergeConditions(newConditions []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration, existingConditions []configv1.ClusterOperatorStatusCondition) bool { now := metav1.Now() updated := false diff --git a/pkg/operatorstatus/controller_status_test.go b/pkg/operatorstatus/controller_status_test.go index f2b46c2f9..8e8578f62 100644 --- a/pkg/operatorstatus/controller_status_test.go +++ b/pkg/operatorstatus/controller_status_test.go @@ -304,7 +304,7 @@ func TestWithRequeueAfter(t *testing.T) { } // applyCondition builds a ClusterOperatorStatusConditionApplyConfiguration for -// use in mergeConditions tests. +// use in MergeConditions tests. func applyCondition(condType configv1.ClusterStatusConditionType, status configv1.ConditionStatus, reason, message string) *configv1apply.ClusterOperatorStatusConditionApplyConfiguration { return configv1apply.ClusterOperatorStatusCondition(). WithType(condType). @@ -372,7 +372,7 @@ func TestMergeConditions(t *testing.T) { } cond := applyCondition(tc.new.condType, tc.new.status, tc.new.reason, tc.new.message) - updated := mergeConditions( + updated := MergeConditions( []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{cond}, existing, ) @@ -393,7 +393,7 @@ func TestMergeConditions(t *testing.T) { cond := applyCondition("Progressing", configv1.ConditionFalse, "AsExpected", "Success") g.Expect(cond.LastTransitionTime).To(BeNil()) - updated := mergeConditions( + updated := MergeConditions( []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{cond}, nil, ) @@ -418,7 +418,7 @@ func TestMergeConditions(t *testing.T) { unchangedCond := applyCondition("Progressing", configv1.ConditionFalse, "AsExpected", "Success") newCond := applyCondition("Degraded", configv1.ConditionFalse, "AsExpected", "Success") - updated := mergeConditions( + updated := MergeConditions( []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{unchangedCond, newCond}, existing, ) @@ -453,7 +453,7 @@ func TestMergeConditions(t *testing.T) { cond1 := applyCondition("Progressing", configv1.ConditionFalse, "AsExpected", "Success") cond2 := applyCondition("Degraded", configv1.ConditionFalse, "AsExpected", "Success") - updated := mergeConditions( + updated := MergeConditions( []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{cond1, cond2}, existing, ) diff --git a/pkg/operatorstatus/watch_predicates.go b/pkg/operatorstatus/watch_predicates.go index 894e08b83..2b54e73b1 100644 --- a/pkg/operatorstatus/watch_predicates.go +++ b/pkg/operatorstatus/watch_predicates.go @@ -18,6 +18,7 @@ package operatorstatus import ( "context" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" @@ -49,6 +50,33 @@ func ClusterOperatorOnceOnly() predicate.Funcs { } } +// ClusterOperatorStatusChanged returns a predicate that triggers on create +// events for the cluster-api ClusterOperator, and on update events when +// status.Conditions or status.Versions has changed. +func ClusterOperatorStatusChanged() predicate.Funcs { + isClusterOperator := func(obj runtime.Object) bool { + clusterOperator, ok := obj.(*configv1.ClusterOperator) + return ok && clusterOperator.GetName() == controllers.ClusterOperatorName + } + + return predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { return isClusterOperator(e.Object) }, + UpdateFunc: func(e event.UpdateEvent) bool { + if !isClusterOperator(e.ObjectNew) { + return false + } + + oldCO, _ := e.ObjectOld.(*configv1.ClusterOperator) + newCO, _ := e.ObjectNew.(*configv1.ClusterOperator) + + return !equality.Semantic.DeepEqual(oldCO.Status.Conditions, newCO.Status.Conditions) || + !equality.Semantic.DeepEqual(oldCO.Status.Versions, newCO.Status.Versions) + }, + DeleteFunc: func(e event.DeleteEvent) bool { return false }, + GenericFunc: func(e event.GenericEvent) bool { return false }, + } +} + // ToClusterOperator unconditionally returns a reconcile request for the cluster-api ClusterOperator. func ToClusterOperator(_ context.Context, _ client.Object) []reconcile.Request { return []reconcile.Request{{