From 0bb838cfe387ed08baffa314595fc157db2dbd60 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 2 Jun 2026 15:19:35 +0100 Subject: [PATCH 1/5] 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 4f645e294104b16a0d83c39e6403aee1c187c265 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 2 Jun 2026 15:22:17 +0100 Subject: [PATCH 2/5] 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 7faa6f726..5bb70208f 100644 --- a/Makefile +++ b/Makefile @@ -25,11 +25,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-controllers bin/machine-api-migration bin/crd-compatibility-checker manifests-gen ## Build all binaries +.PHONY: generate +generate: + go generate ./... + +build: generate bin/capi-operator bin/capi-controllers bin/machine-api-migration bin/crd-compatibility-checker manifests-gen ## Build all binaries clean: rm -rf bin/* diff --git a/go.mod b/go.mod index a550e009e..700f52023 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 735fc4d92..e7fe96bf7 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1784,6 +1784,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 910438409cdca7222d4d622e19a63b17e61d2a2e Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 2 Jun 2026 15:39:03 +0100 Subject: [PATCH 3/5] 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 7189785a8252c6927590a6c10e0eba80640c53b4 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 2 Jun 2026 15:42:06 +0100 Subject: [PATCH 4/5] 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, 706 insertions(+), 147 deletions(-) diff --git a/cmd/capi-operator/main.go b/cmd/capi-operator/main.go index d2ede0aa0..8fcfae5e5 100644 --- a/cmd/capi-operator/main.go +++ b/cmd/capi-operator/main.go @@ -47,7 +47,10 @@ import ( "github.com/openshift/cluster-capi-operator/pkg/util" ) -var errPodIdentityNotSet = errors.New("POD_NAME and POD_NAMESPACE must be set") +var ( + errPodIdentityNotSet = errors.New("POD_NAME and POD_NAMESPACE must be set") + errInfrastructurePlatformStatusNotSet = errors.New("infrastructure platform status is not set") +) const ( managerName = "capi-operator" @@ -116,22 +119,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{{ From 181c66bfdf114835c2c4d19597a15ee7d1e33702 Mon Sep 17 00:00:00 2001 From: stefanonardo Date: Thu, 4 Jun 2026 13:56:30 +0200 Subject: [PATCH 5/5] Migrate four controllers to per-controller condition reporting Migrate CoreCluster, InfraCluster, Kubeconfig, and SecretSync controllers from ClusterOperatorStatusClient (merge-patch, shared top-level conditions) to ControllerResultGenerator (server-side apply, per-controller prefixed conditions). Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/capi-controllers/main.go | 30 +-- pkg/commoncmdoptions/commonoptions.go | 16 -- .../clusteroperator_controller.go | 13 +- .../clusteroperator_controller_test.go | 146 +++++++----- .../corecluster/corecluster_controller.go | 48 ++-- .../corecluster_controller_test.go | 57 +++-- .../infracluster/infracluster_controller.go | 82 +++---- .../infracluster_controller_test.go | 49 +++- pkg/controllers/kubeconfig/kubeconfig.go | 67 +++--- pkg/controllers/kubeconfig/kubeconfig_test.go | 86 +++++-- .../secretsync/secret_sync_controller.go | 103 ++------- .../secretsync/secret_sync_controller_test.go | 78 +++++-- pkg/operatorstatus/operator_status.go | 210 ------------------ 13 files changed, 442 insertions(+), 543 deletions(-) delete mode 100644 pkg/operatorstatus/operator_status.go diff --git a/cmd/capi-controllers/main.go b/cmd/capi-controllers/main.go index a8d6054d5..e6e34ee5d 100644 --- a/cmd/capi-controllers/main.go +++ b/cmd/capi-controllers/main.go @@ -172,35 +172,37 @@ func setupPlatformReconcilers(log logr.Logger, mgr manager.Manager, operatorConf func setupReconcilers(mgr manager.Manager, operatorConfig commoncmdoptions.OperatorConfig, infra *configv1.Infrastructure, platform configv1.PlatformType, infraClusterObject client.Object) error { if err := (&corecluster.CoreClusterController{ - ClusterOperatorStatusClient: operatorConfig.GetClusterOperatorStatusClient(mgr, platform, "cluster-resource"), - Cluster: &clusterv1.Cluster{}, - Platform: platform, - Infra: infra, + Client: mgr.GetClient(), + ManagedNamespace: *operatorConfig.CAPINamespace, + Cluster: &clusterv1.Cluster{}, + Platform: platform, + Infra: infra, }).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create corecluster controller: %w", err) } if err := (&secretsync.UserDataSecretController{ - ClusterOperatorStatusClient: operatorConfig.GetClusterOperatorStatusClient(mgr, platform, "user-data-secret"), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + ManagedNamespace: *operatorConfig.CAPINamespace, + Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create user-data-secret controller: %w", err) } if err := (&kubeconfig.KubeconfigReconciler{ - ClusterOperatorStatusClient: operatorConfig.GetClusterOperatorStatusClient(mgr, platform, "kubeconfig"), - Scheme: mgr.GetScheme(), - RestCfg: mgr.GetConfig(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + RestCfg: mgr.GetConfig(), }).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create kubeconfig controller: %w", err) } if err := (&infracluster.InfraClusterController{ - ClusterOperatorStatusClient: operatorConfig.GetClusterOperatorStatusClient(mgr, platform, "infracluster"), - Scheme: mgr.GetScheme(), - RestCfg: mgr.GetConfig(), - Platform: platform, - Infra: infra, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + RestCfg: mgr.GetConfig(), + Platform: platform, + Infra: infra, }).SetupWithManager(mgr, infraClusterObject); err != nil { return fmt.Errorf("unable to create infracluster controller: %w", err) } diff --git a/pkg/commoncmdoptions/commonoptions.go b/pkg/commoncmdoptions/commonoptions.go index da09d6a8a..6c08665bf 100644 --- a/pkg/commoncmdoptions/commonoptions.go +++ b/pkg/commoncmdoptions/commonoptions.go @@ -39,12 +39,9 @@ import ( "k8s.io/klog/v2" "k8s.io/klog/v2/textlogger" - configv1 "github.com/openshift/api/config/v1" libgocrypto "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/cluster-capi-operator/pkg/controllers" - "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" - "github.com/openshift/cluster-capi-operator/pkg/util" ) // The default durations for the leader election operations. @@ -179,19 +176,6 @@ func InitOperatorConfig(ctx context.Context, cfg *rest.Config, scheme *runtime.S }, initManager(cfg, setupSecurityProfileWatcher), nil } -// GetClusterOperatorStatusClient returns a ClusterOperatorStatusClient struct which has been -// initialised with values from the command line. -func (opts *OperatorConfig) GetClusterOperatorStatusClient(mgr ctrl.Manager, platform configv1.PlatformType, controllerName string) operatorstatus.ClusterOperatorStatusClient { - return operatorstatus.ClusterOperatorStatusClient{ - Client: mgr.GetClient(), - Recorder: mgr.GetEventRecorderFor(opts.managerName + "-" + controllerName), - ReleaseVersion: util.GetReleaseVersion(), - ManagedNamespace: *opts.CAPINamespace, - OperatorNamespace: *opts.OperatorNamespace, - Platform: platform, - } -} - func initManager(cfg *rest.Config, securityProfileWatcher func(ctrl.Manager, context.CancelFunc) error) func(ctx context.Context, cancel context.CancelFunc, mgrOpts ctrl.Options) (ctrl.Manager, error) { return func(ctx context.Context, cancel context.CancelFunc, mgrOpts ctrl.Options) (ctrl.Manager, error) { mgr, err := ctrl.NewManager(cfg, mgrOpts) diff --git a/pkg/controllers/clusteroperator/clusteroperator_controller.go b/pkg/controllers/clusteroperator/clusteroperator_controller.go index f32633323..825a40b95 100644 --- a/pkg/controllers/clusteroperator/clusteroperator_controller.go +++ b/pkg/controllers/clusteroperator/clusteroperator_controller.go @@ -29,8 +29,12 @@ import ( 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/corecluster" + "github.com/openshift/cluster-capi-operator/pkg/controllers/infracluster" "github.com/openshift/cluster-capi-operator/pkg/controllers/installer" + "github.com/openshift/cluster-capi-operator/pkg/controllers/kubeconfig" "github.com/openshift/cluster-capi-operator/pkg/controllers/revision" + "github.com/openshift/cluster-capi-operator/pkg/controllers/secretsync" "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" "github.com/openshift/cluster-capi-operator/pkg/util" ) @@ -175,11 +179,10 @@ func (r *ClusterOperatorController) aggregatedStatus(currentConditions []configv subControllers := []operatorstatus.ControllerResultGenerator{ installer.ResultGenerator, revision.ResultGenerator, - // TBD as they are migrated: - // - corecluster - // - infracluster - // - secretsync - // - kubeconfig + corecluster.ResultGenerator, + infracluster.ResultGenerator, + kubeconfig.ResultGenerator, + secretsync.ResultGenerator, } subcontrollerStatuses := util.SliceMap(subControllers, func(subController operatorstatus.ControllerResultGenerator) subcontrollerStatus { diff --git a/pkg/controllers/clusteroperator/clusteroperator_controller_test.go b/pkg/controllers/clusteroperator/clusteroperator_controller_test.go index bbe35611f..ec50befb1 100644 --- a/pkg/controllers/clusteroperator/clusteroperator_controller_test.go +++ b/pkg/controllers/clusteroperator/clusteroperator_controller_test.go @@ -92,12 +92,7 @@ var _ = Describe("ClusterOperator controller", func() { )) }, 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"), - }, + allSubControllersSuccessful(), test.HaveCondition(configv1.OperatorAvailable). WithStatus(configv1.ConditionTrue). WithReason(operatorstatus.ReasonAsExpected), @@ -106,12 +101,9 @@ var _ = Describe("ClusterOperator controller", func() { WithReason(operatorstatus.ReasonAsExpected), defaultNodeTimeout), Entry("when installer controller is progressing but was previously available", - []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ - subCondition("InstallerControllerAvailable", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + withOverrides(allSubControllersSuccessful(), 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), @@ -121,12 +113,9 @@ var _ = Describe("ClusterOperator controller", func() { 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"), + withOverrides(allSubControllersSuccessful(), subCondition("RevisionControllerProgressing", configv1.ConditionTrue, operatorstatus.ReasonProgressing, "Updating revisions"), - }, + ), test.HaveCondition(configv1.OperatorAvailable). WithStatus(configv1.ConditionTrue). WithReason(operatorstatus.ReasonAsExpected), @@ -135,13 +124,11 @@ var _ = Describe("ClusterOperator controller", func() { 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"), + Entry("when both installer and revision controllers are progressing but were previously available", + withOverrides(allSubControllersSuccessful(), 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), @@ -154,12 +141,10 @@ var _ = Describe("ClusterOperator controller", func() { )), defaultNodeTimeout), Entry("when installer controller has a non-retryable error", - []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + withOverrides(allSubControllersSuccessful(), 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). @@ -170,12 +155,10 @@ var _ = Describe("ClusterOperator controller", func() { 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"), + withOverrides(allSubControllersSuccessful(), 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). @@ -186,12 +169,9 @@ var _ = Describe("ClusterOperator controller", func() { 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"), + withOverrides(allSubControllersSuccessful(), 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), @@ -201,10 +181,10 @@ var _ = Describe("ClusterOperator controller", func() { 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"), - }, + withoutConditions(allSubControllersSuccessful(), + "InstallerControllerAvailable", + "InstallerControllerProgressing", + ), test.HaveCondition(configv1.OperatorAvailable). WithStatus(configv1.ConditionFalse). WithReason(operatorstatus.ReasonUninitialized). @@ -222,6 +202,10 @@ var _ = Describe("ClusterOperator controller", func() { WithMessage(SatisfyAll( ContainSubstring("InstallerController"), ContainSubstring("RevisionController"), + ContainSubstring("CoreClusterController"), + ContainSubstring("InfraClusterController"), + ContainSubstring("KubeconfigController"), + ContainSubstring("SecretSyncController"), ContainSubstring("initializing"), )), test.HaveCondition(configv1.OperatorProgressing). @@ -230,6 +214,10 @@ var _ = Describe("ClusterOperator controller", func() { WithMessage(SatisfyAll( ContainSubstring("InstallerController"), ContainSubstring("RevisionController"), + ContainSubstring("CoreClusterController"), + ContainSubstring("InfraClusterController"), + ContainSubstring("KubeconfigController"), + ContainSubstring("SecretSyncController"), ContainSubstring("initializing"), )), defaultNodeTimeout), @@ -253,12 +241,11 @@ var _ = Describe("ClusterOperator controller", func() { )), defaultNodeTimeout), Entry("when one controller has a non-retryable error and the other is progressing", - []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + withOverrides(allSubControllersSuccessful(), 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). @@ -271,13 +258,13 @@ var _ = Describe("ClusterOperator controller", func() { ContainSubstring("RevisionController"), )), defaultNodeTimeout), - Entry("when both controllers have non-retryable errors", - []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + Entry("when both installer and revision controllers have non-retryable errors", + withOverrides(allSubControllersSuccessful(), 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). @@ -316,12 +303,10 @@ var _ = Describe("ClusterOperator controller", func() { // 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"), + withOverrides(allSubControllersSuccessful(), 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), @@ -334,12 +319,12 @@ var _ = Describe("ClusterOperator controller", func() { )), defaultNodeTimeout), Entry("should report the highest priority available reason", - []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{ + withOverrides(allSubControllersSuccessful(), 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). @@ -481,3 +466,64 @@ func stopManager(ctx context.Context) { Eventually(mgrDone).WithContext(ctx).WithTimeout(defaultEventuallyTimeout).Should(BeClosed()) }) } + +func allSubControllersSuccessful() []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration { + controllers := []string{ + "InstallerController", + "RevisionController", + "CoreClusterController", + "InfraClusterController", + "KubeconfigController", + "SecretSyncController", + } + + conds := make([]*configv1apply.ClusterOperatorStatusConditionApplyConfiguration, 0, 2*len(controllers)) + for _, c := range controllers { + conds = append(conds, + subCondition(c+"Available", configv1.ConditionTrue, operatorstatus.ReasonAsExpected, "Success"), + subCondition(c+"Progressing", configv1.ConditionFalse, operatorstatus.ReasonAsExpected, "Success"), + ) + } + + return conds +} + +func withoutConditions(base []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration, types ...string) []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration { + exclude := make(map[configv1.ClusterStatusConditionType]bool, len(types)) + for _, t := range types { + exclude[configv1.ClusterStatusConditionType(t)] = true + } + + result := make([]*configv1apply.ClusterOperatorStatusConditionApplyConfiguration, 0, len(base)) + + for _, c := range base { + if !exclude[*c.Type] { + result = append(result, c) + } + } + + return result +} + +func withOverrides(base []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration, overrides ...*configv1apply.ClusterOperatorStatusConditionApplyConfiguration) []*configv1apply.ClusterOperatorStatusConditionApplyConfiguration { + result := append([]*configv1apply.ClusterOperatorStatusConditionApplyConfiguration{}, base...) + + for _, override := range overrides { + found := false + + for i, c := range result { + if *c.Type == *override.Type { + result[i] = override + found = true + + break + } + } + + if !found { + result = append(result, override) + } + } + + return result +} diff --git a/pkg/controllers/corecluster/corecluster_controller.go b/pkg/controllers/corecluster/corecluster_controller.go index e4f324859..0d11f267c 100644 --- a/pkg/controllers/corecluster/corecluster_controller.go +++ b/pkg/controllers/corecluster/corecluster_controller.go @@ -46,6 +46,9 @@ const ( capiInfraClusterAPIVersionV1Beta1 = "infrastructure.cluster.x-k8s.io/v1beta1" capiInfraClusterAPIVersionV1Beta2 = "infrastructure.cluster.x-k8s.io/v1beta2" capiInfraClusterAPIGroup = "infrastructure.cluster.x-k8s.io" + + // ResultGenerator is the controller result generator for the CoreClusterController. + ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) ) var ( @@ -57,10 +60,11 @@ var ( // CoreClusterController reconciles a Cluster object. type CoreClusterController struct { - operatorstatus.ClusterOperatorStatusClient - Cluster *clusterv1.Cluster - Infra *configv1.Infrastructure - Platform configv1.PlatformType + client.Client + ManagedNamespace string + Cluster *clusterv1.Cluster + Infra *configv1.Infrastructure + Platform configv1.PlatformType } // SetupWithManager sets the CoreClusterReconciler controller up with the given manager. @@ -81,39 +85,39 @@ func (r *CoreClusterController) SetupWithManager(mgr ctrl.Manager) error { } // Reconcile reconciles the core cluster object for the openshift-cluster-api namespace. -func (r *CoreClusterController) Reconcile(ctx context.Context, req reconcile.Request) (ctrl.Result, error) { - logger := logf.FromContext(ctx).WithName(controllerName) +func (r *CoreClusterController) Reconcile(ctx context.Context, _ reconcile.Request) (ctrl.Result, error) { + log := logf.FromContext(ctx).WithName(controllerName) + log.Info("Reconciling core cluster") + + reconcileResult := r.reconcile(ctx, log) - logger.Info("Reconciling core cluster") - defer logger.Info("Finished reconciling core cluster") + if err := reconcileResult.WriteClusterOperatorStatus(ctx, log, r.Client); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to write conditions: %w", err) + } + + return reconcileResult.Result() +} +func (r *CoreClusterController) reconcile(ctx context.Context, log logr.Logger) operatorstatus.ReconcileResult { ocpInfrastructureName, err := getOCPInfrastructureName(r.Infra) if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to obtain infrastructure name: %w", err) + return ResultGenerator.Error(fmt.Errorf("failed to obtain infrastructure name: %w", err)) } - cluster, err := r.ensureCoreCluster(ctx, client.ObjectKey{Namespace: r.ManagedNamespace, Name: ocpInfrastructureName}, logger) + cluster, err := r.ensureCoreCluster(ctx, client.ObjectKey{Namespace: r.ManagedNamespace, Name: ocpInfrastructureName}, log) if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to ensure core cluster: %w", err) + return ResultGenerator.Error(fmt.Errorf("failed to ensure core cluster: %w", err)) } if !cluster.DeletionTimestamp.IsZero() { - if err := r.SetStatusAvailable(ctx, ""); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set status available: %w", err) - } - - return ctrl.Result{}, nil + return ResultGenerator.Success() } if err := r.ensureCoreClusterControlPlaneInitializedCondition(ctx, cluster); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to ensure core cluster has the ControlPlaneInitializedCondition: %w", err) - } - - if err := r.SetStatusAvailable(ctx, ""); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set status available: %w", err) + return ResultGenerator.Error(fmt.Errorf("failed to ensure core cluster has the ControlPlaneInitializedCondition: %w", err)) } - return ctrl.Result{}, nil + return ResultGenerator.Success() } // ensureCoreCluster creates a cluster with the given name and returns the cluster object. diff --git a/pkg/controllers/corecluster/corecluster_controller_test.go b/pkg/controllers/corecluster/corecluster_controller_test.go index 1dfea3f05..74b720f51 100644 --- a/pkg/controllers/corecluster/corecluster_controller_test.go +++ b/pkg/controllers/corecluster/corecluster_controller_test.go @@ -38,6 +38,7 @@ import ( 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" ) var _ = Describe("Reconcile Core cluster", func() { @@ -48,7 +49,6 @@ var _ = Describe("Reconcile Core cluster", func() { testInfraName := "test-ocp-infrastructure-name" testRegionName := "eu-west-2" - desiredOperatorReleaseVersion := "this-is-the-desired-release-version" var ( infra *configv1.Infrastructure @@ -74,14 +74,11 @@ var _ = Describe("Reconcile Core cluster", func() { Expect(err).ToNot(HaveOccurred(), "Manager should be able to be created") r := &CoreClusterController{ - ClusterOperatorStatusClient: operatorstatus.ClusterOperatorStatusClient{ - Client: cl, - ManagedNamespace: testNamespaceName, - ReleaseVersion: desiredOperatorReleaseVersion, - }, - Cluster: &clusterv1.Cluster{}, - Infra: infra.DeepCopy(), - Platform: infra.Status.PlatformStatus.Type, + Client: cl, + ManagedNamespace: testNamespaceName, + Cluster: &clusterv1.Cluster{}, + Infra: infra.DeepCopy(), + Platform: infra.Status.PlatformStatus.Type, } Expect(r.SetupWithManager(mgr)).To(Succeed(), "Reconciler should be able to setup with manager") @@ -166,6 +163,17 @@ var _ = Describe("Reconcile Core cluster", func() { testCoreCluster := clusterv1resourcebuilder.Cluster().WithName(testInfraName).WithNamespace(testNamespaceName).Build() Consistently(komega.Get(testCoreCluster)).Should(MatchError("clusters.cluster.x-k8s.io \"test-ocp-infrastructure-name\" not found")) }) + + It("should set Progressing=True with EphemeralError on the ClusterOperator", func() { + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Eventually(komega.Object(co)).Should( + HaveField("Status.Conditions", SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType("Progressing")). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonEphemeralError), + )), + ) + }) }) Context("When there is an infra cluster", func() { @@ -191,14 +199,16 @@ var _ = Describe("Reconcile Core cluster", func() { }) Context("With a ClusterOperator", func() { - It("should update the ClusterOperator status to be available, upgradeable, non-progressing, non-degraded", func() { - co := komega.Object(configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build()) - Eventually(co).Should( + It("should set Available=True and Progressing=False on success", func() { + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Eventually(komega.Object(co)).Should( HaveField("Status.Conditions", SatisfyAll( - ContainElement(And(HaveField("Type", Equal(configv1.OperatorAvailable)), HaveField("Status", Equal(configv1.ConditionTrue)))), - 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)))), + test.HaveCondition(ResultGenerator.SubConditionType("Available")). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(ResultGenerator.SubConditionType("Progressing")). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), )), ) }) @@ -242,6 +252,21 @@ var _ = Describe("Reconcile Core cluster", func() { )), ) }) + + It("should set Available=True and Progressing=False on the ClusterOperator", func() { + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + + Eventually(komega.Object(co)).Should( + HaveField("Status.Conditions", SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType("Available")). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(ResultGenerator.SubConditionType("Progressing")). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), + )), + ) + }) }) }) }) diff --git a/pkg/controllers/infracluster/infracluster_controller.go b/pkg/controllers/infracluster/infracluster_controller.go index 3e29ea1db..bb6316b4a 100644 --- a/pkg/controllers/infracluster/infracluster_controller.go +++ b/pkg/controllers/infracluster/infracluster_controller.go @@ -43,14 +43,6 @@ import ( ) const ( - // Controller conditions for the Cluster Operator resource. - - // InfraClusterControllerAvailableCondition is the condition type that indicates the InfraCluster controller is available. - InfraClusterControllerAvailableCondition = "InfraClusterControllerAvailable" - - // InfraClusterControllerDegradedCondition is the condition type that indicates the InfraCluster controller is degraded. - InfraClusterControllerDegradedCondition = "InfraClusterControllerDegraded" - defaultCAPINamespace = "openshift-cluster-api" defaultMAPINamespace = "openshift-machine-api" controllerName = "InfraClusterController" @@ -60,6 +52,9 @@ const ( kubeSystemNamespace = "kube-system" vSphereCredentialsName = "vsphere-creds" //nolint:gosec + + // ResultGenerator is the controller result generator for the InfraClusterController. + ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) ) var ( @@ -71,7 +66,7 @@ var ( // InfraClusterController is a controller that manages infrastructure cluster objects. type InfraClusterController struct { - operatorstatus.ClusterOperatorStatusClient + client.Client Scheme *runtime.Scheme RestCfg *rest.Config Platform configv1.PlatformType @@ -81,40 +76,40 @@ type InfraClusterController struct { } // Reconcile reconciles the cluster-api ClusterOperator object. -func (r *InfraClusterController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *InfraClusterController) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx).WithName(controllerName) - log.Info("Reconciling InfraCluster") - res, err := r.reconcile(ctx, log) - if err != nil { - return ctrl.Result{}, fmt.Errorf("error during reconcile: %w", err) - } + reconcileResult := r.reconcile(ctx, log) - if err := r.setAvailableCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for InfraCluster controller: %w", err) + if err := reconcileResult.WriteClusterOperatorStatus(ctx, log, r.Client); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to write conditions: %w", err) } - return res, nil + return reconcileResult.Result() } -func (r *InfraClusterController) reconcile(ctx context.Context, log logr.Logger) (ctrl.Result, error) { +func (r *InfraClusterController) reconcile(ctx context.Context, log logr.Logger) operatorstatus.ReconcileResult { infraCluster, err := r.ensureInfraCluster(ctx, log) if err != nil && errors.Is(err, errPlatformNotSupported) { log.Info("Could not find or create an InfraCluster on this platform as it is not yet supported.") - return ctrl.Result{}, nil + return ResultGenerator.Success() } else if err != nil { - return ctrl.Result{}, fmt.Errorf("unable to ensure InfraCluster: %w", err) + return ResultGenerator.Error(fmt.Errorf("unable to ensure InfraCluster: %w", err)) } // At this point, the InfraCluster exists. // Check if it has the managedByAnnotation. - return r.reconcileInfraCluster(ctx, log, infraCluster) + if err := r.reconcileInfraCluster(ctx, log, infraCluster); err != nil { + return ResultGenerator.Error(err) + } + + return ResultGenerator.Success() } // reconcileInfraCluster reconciles the InfraCluster object. // It first determines if the infra cluster should be managed before setting the infra cluster ready. -func (r *InfraClusterController) reconcileInfraCluster(ctx context.Context, log logr.Logger, infraCluster client.Object) (ctrl.Result, error) { +func (r *InfraClusterController) reconcileInfraCluster(ctx context.Context, log logr.Logger, infraCluster client.Object) error { managedByAnnotationVal, foundAnnotation := infraCluster.GetAnnotations()[clusterv1.ManagedByAnnotation] switch { @@ -126,7 +121,7 @@ func (r *InfraClusterController) reconcileInfraCluster(ctx context.Context, log " - skipping as this is managed directly by the CAPI infrastructure provider", infraCluster.GetNamespace(), infraCluster.GetName())) - return ctrl.Result{}, nil + return nil case managedByAnnotationVal != managedByAnnotationValueClusterCAPIOperatorInfraClusterController: // At this point it is not this controller's responsibility to manage this InfraCluster object, nor it is // the CAPI infra providers responsbility to do so. This means this object was created outside of these two entities - thus @@ -135,37 +130,37 @@ func (r *InfraClusterController) reconcileInfraCluster(ctx context.Context, log " - skipping as it is not managed by this controller", infraCluster.GetNamespace(), infraCluster.GetName(), managedByAnnotationVal)) - return ctrl.Result{}, nil + return nil } // At this point it is this controller's responsibility to manage this InfraCluster object. isReady, err := getReadiness(infraCluster) if err != nil { - return ctrl.Result{}, fmt.Errorf("unable to get readiness for InfraCluster: %w", err) + return fmt.Errorf("unable to get readiness for InfraCluster: %w", err) } if isReady { // The Infrastructure for this CAPI Cluster is already ready - nothing to do. - return ctrl.Result{}, nil + return nil } infraClusterPatchCopy, ok := infraCluster.DeepCopyObject().(client.Object) if !ok { - return ctrl.Result{}, errCouldNotDeepCopyInfraObject + return errCouldNotDeepCopyInfraObject } // Set Status.Ready=true to indicate that cluster's infrastructure ready. if err := setReadiness(infraCluster, true); err != nil { - return ctrl.Result{}, fmt.Errorf("unable to set readiness for InfraCluster: %w", err) + return fmt.Errorf("unable to set readiness for InfraCluster: %w", err) } if err := r.Client.Status().Patch(ctx, infraCluster, client.MergeFrom(infraClusterPatchCopy)); err != nil { - return ctrl.Result{}, fmt.Errorf("unable to patch InfraCluster: %w", err) + return fmt.Errorf("unable to patch InfraCluster: %w", err) } log.Info(fmt.Sprintf("InfraCluster '%s/%s' successfully set to Ready", infraCluster.GetNamespace(), infraCluster.GetName())) - return ctrl.Result{}, nil + return nil } // ensureInfraCluster ensures an InfraCluster object exists in the cluster. @@ -236,29 +231,6 @@ func (r *InfraClusterController) ensureInfraCluster(ctx context.Context, log log return infraCluster, nil } -// setAvailableCondition sets the ClusterOperator status condition to Available. -func (r *InfraClusterController) setAvailableCondition(ctx context.Context, log logr.Logger) error { - co, err := r.GetOrCreateClusterOperator(ctx) - if err != nil { - return fmt.Errorf("failed to get cluster operator: %w", err) - } - - conds := []configv1.ClusterOperatorStatusCondition{ - operatorstatus.NewClusterOperatorStatusCondition(InfraClusterControllerAvailableCondition, configv1.ConditionTrue, operatorstatus.ReasonAsExpected, - "InfraCluster Controller works as expected"), - operatorstatus.NewClusterOperatorStatusCondition(InfraClusterControllerDegradedCondition, configv1.ConditionFalse, operatorstatus.ReasonAsExpected, - "InfraCluster Controller works as expected"), - } - - log.V(2).Info("InfraCluster Controller is Available") - - if err := r.SyncStatus(ctx, co, operatorstatus.WithConditions(conds)); err != nil { - return fmt.Errorf("failed to sync status: %w", err) - } - - return nil -} - // SetupWithManager sets up the controller with the Manager. func (r *InfraClusterController) SetupWithManager(mgr ctrl.Manager, watchedObject client.Object) error { // Allow the namespaces to be set externally for test purposes, when not set, @@ -278,7 +250,7 @@ func (r *InfraClusterController) SetupWithManager(mgr ctrl.Manager, watchedObjec Watches( watchedObject, handler.EnqueueRequestsFromMapFunc(operatorstatus.ToClusterOperator), - builder.WithPredicates(infraClusterPredicate(r.ManagedNamespace)), + builder.WithPredicates(infraClusterPredicate(r.CAPINamespace)), ). // Watch CPMS as the primary source for deriving a provider spec during InfraCluster // generation. CPMS events should retrigger reconciliation so we re-evaluate InfraCluster creation and CO status. diff --git a/pkg/controllers/infracluster/infracluster_controller_test.go b/pkg/controllers/infracluster/infracluster_controller_test.go index 09414445c..65007f04e 100644 --- a/pkg/controllers/infracluster/infracluster_controller_test.go +++ b/pkg/controllers/infracluster/infracluster_controller_test.go @@ -26,6 +26,7 @@ import ( mapiv1beta1 "github.com/openshift/api/machine/v1beta1" "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-api-actuator-pkg/testutils" configv1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/config/v1" @@ -146,6 +147,20 @@ var _ = Describe("InfraCluster", func() { )) }) + It("should set Available=True and Progressing=False on the ClusterOperator", func() { + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Eventually(komega.Object(co)).Should( + HaveField("Status.Conditions", SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionAvailableSuffix)). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionProgressingSuffix)). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), + )), + ) + }) + Context("When there is a ControlPlaneLoadBalancer and a SecondaryControlPlaneLoadBalancer", func() { It("should order two load balancers preferring '-int' as primary", func() { internalLB := &awsv1.AWSLoadBalancerSpec{Name: ptr.To("testClusterID-int"), LoadBalancerType: awsv1.LoadBalancerTypeNLB, Scheme: &awsv1.ELBSchemeInternal} @@ -214,6 +229,20 @@ var _ = Describe("InfraCluster", func() { HaveField("Status.Ready", BeTrue()), ) }) + + It("should still report success conditions on the ClusterOperator", func() { + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Eventually(komega.Object(co)).Should( + HaveField("Status.Conditions", SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionAvailableSuffix)). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionProgressingSuffix)). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), + )), + ) + }) }) Context("When the InfraCluster is not Ready", func() { @@ -285,6 +314,21 @@ var _ = Describe("InfraCluster", func() { HaveField("Annotations", HaveKeyWithValue(clusterv1.ManagedByAnnotation, managedByAnnotationValueClusterCAPIOperatorInfraClusterController)), )) }) + + It("should set Available=True and Progressing=False on the ClusterOperator", func() { + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + + Eventually(komega.Object(co)).Should( + HaveField("Status.Conditions", SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionAvailableSuffix)). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionProgressingSuffix)). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), + )), + ) + }) }) }) }) @@ -316,10 +360,7 @@ func startManager(mgrCtx context.Context, mgrDone chan struct{}, ocpInfra *confi Expect(err).ToNot(HaveOccurred(), "Manager should be able to be created") r := &InfraClusterController{ - ClusterOperatorStatusClient: operatorstatus.ClusterOperatorStatusClient{ - Client: cl, - ManagedNamespace: capiNamespace, - }, + Client: cl, Infra: ocpInfra, Platform: ocpInfra.Status.PlatformStatus.Type, CAPINamespace: capiNamespace, diff --git a/pkg/controllers/kubeconfig/kubeconfig.go b/pkg/controllers/kubeconfig/kubeconfig.go index 7324586df..25b96da5f 100644 --- a/pkg/controllers/kubeconfig/kubeconfig.go +++ b/pkg/controllers/kubeconfig/kubeconfig.go @@ -47,11 +47,14 @@ const ( tokenSecretName = "capi-controllers-token" tokenRefreshAnnotationKey = "cluster-api.openshift.io/last-token-refresh" tokenMaxAge = 30 * time.Minute + + // ResultGenerator is the controller result generator for the KubeconfigController. + ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) ) // KubeconfigReconciler reconciles a ClusterOperator object. type KubeconfigReconciler struct { - operatorstatus.ClusterOperatorStatusClient + client.Client Scheme *runtime.Scheme RestCfg *rest.Config clusterName string @@ -81,53 +84,38 @@ func (r *KubeconfigReconciler) SetupWithManager(mgr ctrl.Manager) error { func (r *KubeconfigReconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx).WithName(controllerName) + reconcileResult := r.reconcile(ctx, log) + + if err := reconcileResult.WriteClusterOperatorStatus(ctx, log, r.Client); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to write conditions: %w", err) + } + + return reconcileResult.Result() +} + +func (r *KubeconfigReconciler) reconcile(ctx context.Context, log logr.Logger) operatorstatus.ReconcileResult { infra := &configv1.Infrastructure{} if err := r.Get(ctx, client.ObjectKey{Name: controllers.InfrastructureResourceName}, infra); err != nil { log.Error(err, "Unable to retrieve Infrastructure object") - - if err := r.SetStatusDegraded(ctx, err); err != nil { - return ctrl.Result{}, fmt.Errorf("error syncing ClusterOperatorStatus: %w", err) - } - - return ctrl.Result{}, fmt.Errorf("unable to retrieve Infrastructure object: %w", err) + return ResultGenerator.Error(fmt.Errorf("unable to retrieve Infrastructure object: %w", err)) } if infra.Status.PlatformStatus == nil { log.Info("No platform status exists in infrastructure object. Skipping kubeconfig reconciliation...") - - if err := r.SetStatusAvailable(ctx, ""); err != nil { - return ctrl.Result{}, fmt.Errorf("error syncing ClusterOperatorStatus: %w", err) - } - - return ctrl.Result{}, nil + return ResultGenerator.WaitingOnExternal("infrastructure platform status").WithRequeueAfter(1 * time.Minute) } r.clusterName = infra.Status.InfrastructureName log.Info("Reconciling kubeconfig secret") - res, err := r.reconcileKubeconfig(ctx, log) - if err != nil { - log.Error(err, "Error reconciling kubeconfig") - - if err := r.SetStatusDegraded(ctx, err); err != nil { - return ctrl.Result{}, fmt.Errorf("error syncing ClusterOperatorStatus: %w", err) - } - - return ctrl.Result{}, fmt.Errorf("error reconciling kubeconfig: %w", err) - } - - if err := r.SetStatusAvailable(ctx, ""); err != nil { - return ctrl.Result{}, fmt.Errorf("error syncing ClusterOperatorStatus: %w", err) - } - - return res, nil + return r.reconcileKubeconfig(ctx, log) } // reconcileKubeconfig reconciles the kubeconfig secret. // //nolint:funlen -func (r *KubeconfigReconciler) reconcileKubeconfig(ctx context.Context, log logr.Logger) (ctrl.Result, error) { +func (r *KubeconfigReconciler) reconcileKubeconfig(ctx context.Context, log logr.Logger) operatorstatus.ReconcileResult { // Get the token secret tokenSecret := &corev1.Secret{} tokenSecretKey := client.ObjectKey{ @@ -138,11 +126,10 @@ func (r *KubeconfigReconciler) reconcileKubeconfig(ctx context.Context, log logr if err := r.Get(ctx, tokenSecretKey, tokenSecret); err != nil { if kerrors.IsNotFound(err) { log.Info("Waiting for token secret to be created") - - return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil + return ResultGenerator.WaitingOnExternal("token secret").WithRequeueAfter(1 * time.Minute) } - return ctrl.Result{}, fmt.Errorf("unable to retrieve Secret object: %w", err) + return ResultGenerator.Error(fmt.Errorf("unable to retrieve Secret object: %w", err)) } // Determine the token age from the refresh annotation, or fall back to CreationTimestamp. @@ -169,10 +156,10 @@ func (r *KubeconfigReconciler) reconcileKubeconfig(ctx context.Context, log logr tokenSecret.Annotations[tokenRefreshAnnotationKey] = time.Now().UTC().Format(time.RFC3339) if err := r.Update(ctx, tokenSecret); err != nil { - return ctrl.Result{}, fmt.Errorf("unable to clear token secret data: %w", err) + return ResultGenerator.Error(fmt.Errorf("unable to clear token secret data: %w", err)) } - return ctrl.Result{}, nil + return ResultGenerator.Success() } kubeconfigOptions := kubeconfigOptions{ @@ -188,11 +175,11 @@ func (r *KubeconfigReconciler) reconcileKubeconfig(ctx context.Context, log logr // the token secret has not been populated by the kubernetes control plane yet. // Requeue to wait for the token secret to be populated. log.Info("Token secret has not been populated by the control plane yet, waiting..") - return ctrl.Result{}, nil + return ResultGenerator.WaitingOnExternal("token secret population") } // If the validation fails with other errors throw a reconciler error instead. - return ctrl.Result{}, fmt.Errorf("invalid kubeconfig options: %w", err) + return ResultGenerator.Error(fmt.Errorf("invalid kubeconfig options: %w", err)) } kubeconfig := generateKubeconfig(kubeconfigOptions) @@ -200,7 +187,7 @@ func (r *KubeconfigReconciler) reconcileKubeconfig(ctx context.Context, log logr // Create a secret with generated kubeconfig out, err := clientcmd.Write(*kubeconfig) if err != nil { - return ctrl.Result{}, fmt.Errorf("error writing kubeconfig: %w", err) + return ResultGenerator.Error(fmt.Errorf("error writing kubeconfig: %w", err)) } kubeconfigSecret := newKubeConfigSecret(r.clusterName, out) @@ -213,10 +200,10 @@ func (r *KubeconfigReconciler) reconcileKubeconfig(ctx context.Context, log logr return nil }); err != nil { - return ctrl.Result{}, fmt.Errorf("error reconciling kubeconfig secret: %w", err) + return ResultGenerator.Error(fmt.Errorf("error reconciling kubeconfig secret: %w", err)) } - return ctrl.Result{}, nil + return ResultGenerator.Success() } func newKubeConfigSecret(clusterName string, data []byte) *corev1.Secret { diff --git a/pkg/controllers/kubeconfig/kubeconfig_test.go b/pkg/controllers/kubeconfig/kubeconfig_test.go index f18bb1fde..e02dee050 100644 --- a/pkg/controllers/kubeconfig/kubeconfig_test.go +++ b/pkg/controllers/kubeconfig/kubeconfig_test.go @@ -27,6 +27,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + configv1 "github.com/openshift/api/config/v1" + configv1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/config/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" @@ -44,9 +46,7 @@ var _ = Describe("Reconcile kubeconfig secret", func() { BeforeEach(func() { r = &KubeconfigReconciler{ - ClusterOperatorStatusClient: operatorstatus.ClusterOperatorStatusClient{ - Client: cl, - }, + Client: cl, clusterName: "test-cluster", RestCfg: cfg, } @@ -63,28 +63,47 @@ var _ = Describe("Reconcile kubeconfig secret", func() { } Expect(cl.Create(ctx, tokenSecret)).To(Succeed()) + + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Expect(cl.Create(ctx, co)).To(Succeed()) }) AfterEach(func() { - Expect(test.CleanupAndWait(ctx, cl, tokenSecret, kubeconfigSecret)).To(Succeed()) + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Expect(test.CleanupAndWait(ctx, cl, tokenSecret, kubeconfigSecret, co)).To(Succeed()) }) It("should create a kubeconfig secret when it doesn't exist", func() { - _, err := r.reconcileKubeconfig(ctx, log) - Expect(err).To(Succeed()) + result := r.reconcileKubeconfig(ctx, log) + Expect(result.Error()).ToNot(HaveOccurred()) Expect(cl.Get(ctx, client.ObjectKey{ Name: fmt.Sprintf("%s-kubeconfig", r.clusterName), Namespace: controllers.DefaultCAPINamespace, }, kubeconfigSecret)).To(Succeed()) Expect(kubeconfigSecret.Data).To(HaveKey("value")) // kubeconfig content is tested separately + + By("Verifying ClusterOperator conditions") + + Expect(result.WriteClusterOperatorStatus(ctx, log, cl)).To(Succeed()) + + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Expect(cl.Get(ctx, client.ObjectKeyFromObject(co), co)).To(Succeed()) + Expect(co.Status.Conditions).To(SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionAvailableSuffix)). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionProgressingSuffix)). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), + )) }) It("should reconcile existing kubeconfig secret when it doesn't exist", func() { - _, err := r.reconcileKubeconfig(ctx, log) - Expect(err).To(Succeed()) - _, err = r.reconcileKubeconfig(ctx, log) - Expect(err).To(Succeed()) + result := r.reconcileKubeconfig(ctx, log) + Expect(result.Error()).ToNot(HaveOccurred()) + result = r.reconcileKubeconfig(ctx, log) + Expect(result.Error()).ToNot(HaveOccurred()) Expect(cl.Get(ctx, client.ObjectKey{ Name: fmt.Sprintf("%s-kubeconfig", r.clusterName), @@ -99,9 +118,22 @@ var _ = Describe("Reconcile kubeconfig secret", func() { return cl.Get(ctx, client.ObjectKeyFromObject(tokenSecret), tokenSecret) }, timeout).Should(Not(Succeed())) - res, err := r.reconcileKubeconfig(ctx, log) - Expect(err).To(Succeed()) + result := r.reconcileKubeconfig(ctx, log) + Expect(result.Error()).ToNot(HaveOccurred()) + res, _ := result.Result() Expect(res.RequeueAfter).To(Equal(1 * time.Minute)) + + By("Verifying ClusterOperator conditions show WaitingOnExternal") + + Expect(result.WriteClusterOperatorStatus(ctx, log, cl)).To(Succeed()) + + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Expect(cl.Get(ctx, client.ObjectKeyFromObject(co), co)).To(Succeed()) + Expect(co.Status.Conditions).To(SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionProgressingSuffix)). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonWaitingOnExternal), + )) }) It("should clear token secret data if its old and requeue", func() { @@ -111,9 +143,8 @@ var _ = Describe("Reconcile kubeconfig secret", func() { tokenSecret.SetCreationTimestamp(metav1.Time{Time: time.Now().Add(-1 * time.Hour)}) Expect(fakeClient.Update(ctx, tokenSecret)).To(Succeed()) - res, err := r.reconcileKubeconfig(ctx, log) - Expect(err).To(Succeed()) - Expect(res).To(Equal(ctrl.Result{})) + result := r.reconcileKubeconfig(ctx, log) + Expect(result.Error()).ToNot(HaveOccurred()) // Verify the secret still exists but data was cleared updatedSecret := &corev1.Secret{} @@ -123,5 +154,30 @@ var _ = Describe("Reconcile kubeconfig secret", func() { // Verify the refresh annotation was set Expect(updatedSecret.Annotations).To(HaveKey(tokenRefreshAnnotationKey)) }) + + It("should report success after refreshing an expired token", func() { + tokenSecret.Annotations = map[string]string{ + tokenRefreshAnnotationKey: time.Now().Add(-1 * time.Hour).UTC().Format(time.RFC3339), + } + Expect(cl.Update(ctx, tokenSecret)).To(Succeed()) + + result := r.reconcileKubeconfig(ctx, log) + Expect(result.Error()).ToNot(HaveOccurred()) + + By("Verifying ClusterOperator conditions show success") + + Expect(result.WriteClusterOperatorStatus(ctx, log, cl)).To(Succeed()) + + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Expect(cl.Get(ctx, client.ObjectKeyFromObject(co), co)).To(Succeed()) + Expect(co.Status.Conditions).To(SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionAvailableSuffix)). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionProgressingSuffix)). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), + )) + }) }) }) diff --git a/pkg/controllers/secretsync/secret_sync_controller.go b/pkg/controllers/secretsync/secret_sync_controller.go index ff0dc8111..548ef8caf 100644 --- a/pkg/controllers/secretsync/secret_sync_controller.go +++ b/pkg/controllers/secretsync/secret_sync_controller.go @@ -31,7 +31,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" - configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" ) @@ -41,13 +40,12 @@ const ( // SecretSourceNamespace is the source namespace to copy the user data secret from. SecretSourceNamespace = "openshift-machine-api" - // Controller conditions for the Cluster Operator resource. - secretSyncControllerAvailableCondition = "SecretSyncControllerAvailable" - secretSyncControllerDegradedCondition = "SecretSyncControllerDegraded" - mapiUserDataKey = "userData" capiUserDataKey = "value" controllerName = "SecretSyncController" + + // ResultGenerator is the controller result generator for the SecretSyncController. + ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) ) var ( @@ -56,15 +54,26 @@ var ( // UserDataSecretController reconciles a Secret object containing machine user data, from the Machine API to Cluster API namespaces. type UserDataSecretController struct { - operatorstatus.ClusterOperatorStatusClient - Scheme *runtime.Scheme + client.Client + ManagedNamespace string + Scheme *runtime.Scheme } // Reconcile reconciles the user data secret. -func (r *UserDataSecretController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *UserDataSecretController) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx).WithName(controllerName) log.Info("reconciling worker user data secret") + reconcileResult := r.reconcile(ctx, log) + + if err := reconcileResult.WriteClusterOperatorStatus(ctx, log, r.Client); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to write conditions: %w", err) + } + + return reconcileResult.Result() +} + +func (r *UserDataSecretController) reconcile(ctx context.Context, log logr.Logger) operatorstatus.ReconcileResult { defaultSourceSecretObjectKey := client.ObjectKey{ Name: managedUserDataSecretName, Namespace: SecretSourceNamespace, } @@ -72,12 +81,7 @@ func (r *UserDataSecretController) Reconcile(ctx context.Context, req ctrl.Reque if err := r.Get(ctx, defaultSourceSecretObjectKey, sourceSecret); err != nil { log.Error(err, "unable to get source secret for sync") - - if err := r.setDegradedCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for secret sync controller: %w", err) - } - - return ctrl.Result{}, fmt.Errorf("failed to get source secret: %w", err) + return ResultGenerator.Error(fmt.Errorf("failed to get source secret: %w", err)) } targetSecret := &corev1.Secret{} @@ -89,39 +93,20 @@ func (r *UserDataSecretController) Reconcile(ctx context.Context, req ctrl.Reque // If the secret does not exist, it will be created later, so we can ignore a Not Found error if err := r.Get(ctx, targetSecretKey, targetSecret); err != nil && !apierrors.IsNotFound(err) { log.Error(err, "unable to get target secret for sync") - - if err := r.setDegradedCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for secret controller: %w", err) - } - - return ctrl.Result{}, fmt.Errorf("failed to get target secret: %w", err) + return ResultGenerator.Error(fmt.Errorf("failed to get target secret: %w", err)) } if r.areSecretsEqual(sourceSecret, targetSecret) { log.Info("user data in source and target secrets is the same, no sync needed") - - if err := r.setAvailableCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for user data secret controller: %w", err) - } - - return ctrl.Result{}, nil + return ResultGenerator.Success() } if err := r.syncSecretData(ctx, sourceSecret, targetSecret); err != nil { log.Error(err, "unable to sync user data secret") - - if err := r.setDegradedCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for user data secret controller: %w", err) - } - - return ctrl.Result{}, err - } - - if err := r.setAvailableCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for user data secret controller: %w", err) + return ResultGenerator.Error(err) } - return ctrl.Result{}, nil + return ResultGenerator.Success() } func (r *UserDataSecretController) areSecretsEqual(source *corev1.Secret, target *corev1.Secret) bool { @@ -184,47 +169,3 @@ func (r *UserDataSecretController) SetupWithManager(mgr ctrl.Manager) error { return nil } - -func (r *UserDataSecretController) setAvailableCondition(ctx context.Context, log logr.Logger) error { - co, err := r.GetOrCreateClusterOperator(ctx) - if err != nil { - return fmt.Errorf("unable to get cluster operator: %w", err) - } - - conds := []configv1.ClusterOperatorStatusCondition{ - operatorstatus.NewClusterOperatorStatusCondition(secretSyncControllerAvailableCondition, configv1.ConditionTrue, operatorstatus.ReasonAsExpected, - "User Data Secret Controller works as expected"), - operatorstatus.NewClusterOperatorStatusCondition(secretSyncControllerDegradedCondition, configv1.ConditionFalse, operatorstatus.ReasonAsExpected, - "User Data Secret Controller works as expected"), - } - - log.Info("user Data Secret Controller is available") - - if err := r.SyncStatus(ctx, co, operatorstatus.WithConditions(conds)); err != nil { - return fmt.Errorf("failed to sync status: %w", err) - } - - return nil -} - -func (r *UserDataSecretController) setDegradedCondition(ctx context.Context, log logr.Logger) error { - co, err := r.GetOrCreateClusterOperator(ctx) - if err != nil { - return fmt.Errorf("unable to get cluster operator: %w", err) - } - - conds := []configv1.ClusterOperatorStatusCondition{ - operatorstatus.NewClusterOperatorStatusCondition(secretSyncControllerAvailableCondition, configv1.ConditionFalse, operatorstatus.ReasonEphemeralError, - "User Data Secret Controller failed to sync secret"), - operatorstatus.NewClusterOperatorStatusCondition(secretSyncControllerDegradedCondition, configv1.ConditionTrue, operatorstatus.ReasonEphemeralError, - "User Data Secret Controller failed to sync secret"), - } - - log.Info("user Data Secret Controller is degraded") - - if err := r.SyncStatus(ctx, co, operatorstatus.WithConditions(conds)); err != nil { - return fmt.Errorf("failed to sync status: %w", err) - } - - return nil -} diff --git a/pkg/controllers/secretsync/secret_sync_controller_test.go b/pkg/controllers/secretsync/secret_sync_controller_test.go index 963718aa7..24715883c 100644 --- a/pkg/controllers/secretsync/secret_sync_controller_test.go +++ b/pkg/controllers/secretsync/secret_sync_controller_test.go @@ -26,7 +26,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/tools/record" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -34,6 +33,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/metrics/server" configv1 "github.com/openshift/api/config/v1" + configv1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/config/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" @@ -84,8 +84,6 @@ var _ = Describe("areSecretsEqual reconciler method", func() { }) var _ = Describe("User Data Secret controller", func() { - var rec *record.FakeRecorder - var ( mgrCtxCancel context.CancelFunc mgrStopped chan struct{} @@ -113,16 +111,17 @@ var _ = Describe("User Data Secret controller", func() { Expect(err).ToNot(HaveOccurred(), "Manager should be able to be created") reconciler = &UserDataSecretController{ - ClusterOperatorStatusClient: operatorstatus.ClusterOperatorStatusClient{ - Client: cl, - Recorder: rec, - ManagedNamespace: controllers.DefaultCAPINamespace, - }, - - Scheme: scheme.Scheme, + Client: cl, + ManagedNamespace: controllers.DefaultCAPINamespace, + Scheme: scheme.Scheme, } Expect(reconciler.SetupWithManager(mgr)).To(Succeed()) + By("Creating the ClusterOperator") + + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + Expect(cl.Create(ctx, co)).To(Succeed()) + By("Creating needed Secret") sourceSecret = makeUserDataSecret() @@ -165,11 +164,6 @@ var _ = Describe("User Data Secret controller", func() { } sourceSecret = nil - - // Creating the cluster api operator - co = &configv1.ClusterOperator{} - co.SetName(controllers.ClusterOperatorName) - Expect(cl.Create(context.Background(), co.DeepCopy())).To(Succeed()) }) It("secret should be synced up after first reconcile", func() { @@ -190,6 +184,22 @@ var _ = Describe("User Data Secret controller", func() { return bytes.Equal(syncedUserDataSecret.Data[capiUserDataKey], []byte(defaultSecretValue)), nil }, timeout).Should(BeTrue()) + + By("Verifying ClusterOperator conditions show success") + + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + + Eventually(func(g Gomega) { + g.Expect(cl.Get(ctx, client.ObjectKeyFromObject(co), co)).To(Succeed()) + g.Expect(co.Status.Conditions).To(SatisfyAll( + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionAvailableSuffix)). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonAsExpected), + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionProgressingSuffix)). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonAsExpected), + )) + }, timeout).Should(Succeed()) }) It("secret should be synced up if managed user data secret changed", func() { @@ -244,6 +254,44 @@ var _ = Describe("User Data Secret controller", func() { Expect(test.CleanupAndWait(ctx, cl, sourceSecret)).To(Succeed()) }) + It("should set Progressing=True with EphemeralError when source secret is missing", func() { + By("Waiting for the initial sync to complete") + Eventually(func() error { + return cl.Get(ctx, syncedSecretKey, &corev1.Secret{}) + }, timeout).Should(Succeed()) + + By("Deleting the source secret") + Expect(cl.Delete(ctx, sourceSecret)).To(Succeed()) + + By("Triggering a reconcile by modifying the target secret") + + targetSecret := &corev1.Secret{} + + Eventually(func() error { + return cl.Get(ctx, syncedSecretKey, targetSecret) + }, timeout).Should(Succeed()) + + targetSecret.Data[capiUserDataKey] = []byte("trigger-reconcile") + Expect(cl.Update(ctx, targetSecret)).To(Succeed()) + + By("Verifying ClusterOperator conditions show EphemeralError") + + co := configv1resourcebuilder.ClusterOperator().WithName(controllers.ClusterOperatorName).Build() + + Eventually(func(g Gomega) { + g.Expect(cl.Get(ctx, client.ObjectKeyFromObject(co), co)).To(Succeed()) + g.Expect(co.Status.Conditions).To( + test.HaveCondition(ResultGenerator.SubConditionType(operatorstatus.ConditionProgressingSuffix)). + WithStatus(configv1.ConditionTrue). + WithReason(operatorstatus.ReasonEphemeralError), + ) + }, timeout).Should(Succeed()) + + // Re-create the source secret so AfterEach cleanup works + sourceSecret = makeUserDataSecret() + Expect(cl.Create(ctx, sourceSecret)).To(Succeed()) + }) + It("secret not be updated if source and target secret contents are identical", func() { syncedUserDataSecret := &corev1.Secret{} diff --git a/pkg/operatorstatus/operator_status.go b/pkg/operatorstatus/operator_status.go deleted file mode 100644 index 5ecb19ea9..000000000 --- a/pkg/operatorstatus/operator_status.go +++ /dev/null @@ -1,210 +0,0 @@ -/* -Copyright 2024 Red Hat, Inc. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ -package operatorstatus - -import ( - "context" - "fmt" - - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/tools/record" - "k8s.io/utils/clock" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - - configv1 "github.com/openshift/api/config/v1" - "github.com/openshift/cluster-capi-operator/pkg/controllers" - "github.com/openshift/library-go/pkg/config/clusteroperator/v1helpers" -) - -// ClusterOperatorStatusClient is a client for managing the status of the ClusterOperator object. -type ClusterOperatorStatusClient struct { - client.Client - Recorder record.EventRecorder - ManagedNamespace string - OperatorNamespace string - ReleaseVersion string - Platform configv1.PlatformType -} - -// SetStatusAvailable sets the Available condition to True, with the given reason -// and message, and sets both the Progressing and Degraded conditions to False. -func (r *ClusterOperatorStatusClient) SetStatusAvailable(ctx context.Context, availableConditionMsg string, opts ...SyncStatusOption) error { - log := ctrl.LoggerFrom(ctx) - - co, err := r.GetOrCreateClusterOperator(ctx) - if err != nil { - log.Error(err, "unable to set cluster operator status available") - return err - } - - if availableConditionMsg == "" { - availableConditionMsg = fmt.Sprintf("Cluster CAPI Operator is available at %s", r.ReleaseVersion) - } - - conds := []configv1.ClusterOperatorStatusCondition{ - NewClusterOperatorStatusCondition(configv1.OperatorAvailable, configv1.ConditionTrue, ReasonAsExpected, availableConditionMsg), - NewClusterOperatorStatusCondition(configv1.OperatorProgressing, configv1.ConditionFalse, ReasonAsExpected, ""), - NewClusterOperatorStatusCondition(configv1.OperatorDegraded, configv1.ConditionFalse, ReasonAsExpected, ""), - NewClusterOperatorStatusCondition(configv1.OperatorUpgradeable, configv1.ConditionTrue, ReasonAsExpected, ""), - } - - log.Info("syncing status: available") - - return r.SyncStatus(ctx, co, append(opts, WithConditions(conds))...) -} - -// SetStatusDegraded sets the Degraded condition to True, with the given reason and -// message, and sets the upgradeable condition. It does not modify any existing -// Available or Progressing conditions. -func (r *ClusterOperatorStatusClient) SetStatusDegraded(ctx context.Context, reconcileErr error, opts ...SyncStatusOption) error { - log := ctrl.LoggerFrom(ctx) - - co, err := r.GetOrCreateClusterOperator(ctx) - if err != nil { - log.Error(err, "unable to set cluster operator status degraded") - return err - } - - message := fmt.Sprintf("Failed to resync because %v", reconcileErr) - - conds := []configv1.ClusterOperatorStatusCondition{ - NewClusterOperatorStatusCondition(configv1.OperatorDegraded, configv1.ConditionTrue, ReasonEphemeralError, message), - NewClusterOperatorStatusCondition(configv1.OperatorUpgradeable, configv1.ConditionFalse, ReasonAsExpected, message), - } - - r.Recorder.Eventf(co, corev1.EventTypeWarning, "Status degraded", reconcileErr.Error()) - log.Info("syncing status: degraded", "message", message) - - return r.SyncStatus(ctx, co, append(opts, WithConditions(conds))...) -} - -// GetOrCreateClusterOperator is responsible for fetching the cluster operator should it exist, -// or creating a new cluster operator if it does not already exist. -func (r *ClusterOperatorStatusClient) GetOrCreateClusterOperator(ctx context.Context) (*configv1.ClusterOperator, error) { - log := ctrl.LoggerFrom(ctx) - - co := &configv1.ClusterOperator{ - ObjectMeta: metav1.ObjectMeta{ - Name: controllers.ClusterOperatorName, - }, - } - - err := r.Get(ctx, client.ObjectKey{Name: controllers.ClusterOperatorName}, co) - if errors.IsNotFound(err) { - log.Info("ClusterOperator does not exist, creating a new one.") - - err = r.Create(ctx, co) - if err != nil { - return nil, fmt.Errorf("failed to create cluster operator: %w", err) - } - - return co, nil - } - - if err != nil { - return nil, fmt.Errorf("failed to get clusterOperator %q: %w", controllers.ClusterOperatorName, err) - } - - return co, nil -} - -type syncStatusOptions struct { - conditions []configv1.ClusterOperatorStatusCondition - versions []configv1.OperandVersion -} - -// SyncStatusOption sets an option for the SyncStatus operation. -type SyncStatusOption func(*syncStatusOptions) - -// WithConditions sets conditions for a SyncStatus operation. -func WithConditions(conditions []configv1.ClusterOperatorStatusCondition) SyncStatusOption { - return func(o *syncStatusOptions) { - o.conditions = conditions - } -} - -// WithVersions sets versions for a SyncStatus operation. -func WithVersions(versions []configv1.OperandVersion) SyncStatusOption { - return func(o *syncStatusOptions) { - o.versions = versions - } -} - -// SyncStatus performs a full sync of the ClusterOperator object if any of the -// conditions, versions, or related objects have changed. -func (r *ClusterOperatorStatusClient) SyncStatus(ctx context.Context, co *configv1.ClusterOperator, opts ...SyncStatusOption) error { - syncOptions := syncStatusOptions{} - - for _, opt := range opts { - opt(&syncOptions) - } - - log := ctrl.LoggerFrom(ctx) - patchBase := client.MergeFrom(co.DeepCopy()) - - shouldUpdate := false - - for _, cond := range syncOptions.conditions { - if !isStatusConditionPresentAndEqual(co.Status.Conditions, cond) { - v1helpers.SetStatusCondition(&co.Status.Conditions, cond, clock.RealClock{}) - - shouldUpdate = true - } - } - - if syncOptions.versions != nil && !equality.Semantic.DeepEqual(co.Status.Versions, syncOptions.versions) { - co.Status.Versions = syncOptions.versions - shouldUpdate = true - } - - if shouldUpdate { - log.Info("syncing status", "message", v1helpers.GetStatusDiff(co.Status, co.Status)) - - if err := r.Client.Status().Patch(ctx, co, patchBase); err != nil { - return fmt.Errorf("failed to update cluster operator status: %w", err) - } - } - - return nil -} - -// NewClusterOperatorStatusCondition creates a new ClusterOperatorStatusCondition. -func NewClusterOperatorStatusCondition(conditionType configv1.ClusterStatusConditionType, - conditionStatus configv1.ConditionStatus, reason Reason, - message string) configv1.ClusterOperatorStatusCondition { - return configv1.ClusterOperatorStatusCondition{ - Type: conditionType, - Status: conditionStatus, - LastTransitionTime: metav1.Now(), - Reason: reason.String(), - Message: message, - } -} - -// isStatusConditionPresentAndEqual returns true when cond is present and equal. -func isStatusConditionPresentAndEqual(conditions []configv1.ClusterOperatorStatusCondition, cond configv1.ClusterOperatorStatusCondition) bool { - for _, condition := range conditions { - if condition.Type == cond.Type { - return condition.Status == cond.Status && condition.Reason == cond.Reason && condition.Message == cond.Message - } - } - - return false -}