From 89d9e2316bfbfc3761b5322554150484f6436b3e Mon Sep 17 00:00:00 2001 From: panxiao81 Date: Fri, 11 Sep 2026 15:04:15 +0000 Subject: [PATCH] refactor: consolidate Instance phase handlers --- .../postgresqlinstance_controller.go | 67 +--------- .../postgresqlinstance_state_machine.go | 117 +++++++++++++----- internal/controller/state_machine_test.go | 83 +++++++++++-- 3 files changed, 164 insertions(+), 103 deletions(-) diff --git a/internal/controller/postgresqlinstance_controller.go b/internal/controller/postgresqlinstance_controller.go index aaccc23..7c4dd9f 100644 --- a/internal/controller/postgresqlinstance_controller.go +++ b/internal/controller/postgresqlinstance_controller.go @@ -18,7 +18,6 @@ package controller import ( "context" - "errors" "reflect" "time" @@ -66,17 +65,12 @@ func (r *PostgreSQLInstanceReconciler) Reconcile(ctx context.Context, req ctrl.R } before := instance.DeepCopy() - phaseResult := newInstanceStateMachine().reconcile(instance) - instance.Status.Phase = phaseResult.phase - if phaseResult.reconcilingMessage != "" { - setReconcilingCondition(&instance.Status.Conditions, instance.Generation, phaseResult.reconcilingMessage) - } - - var reconcileErr error - if r.Initializer != nil && instance.DeletionTimestamp.IsZero() && - before.Status.Phase != "" && before.Status.Phase != databasev1alpha1.PostgreSQLInstancePhasePending { - reconcileErr = r.reconcileDependencies(ctx, instance) + if r.Timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, r.Timeout) + defer cancel() } + result, reconcileErr := newInstanceStateMachine(r.Initializer).reconcile(ctx, instance) if !reflect.DeepEqual(before.Status, instance.Status) { if err := r.Status().Patch(ctx, instance, client.MergeFrom(before)); err != nil { @@ -87,56 +81,7 @@ func (r *PostgreSQLInstanceReconciler) Reconcile(ctx context.Context, req ctrl.R } } - return ctrl.Result{}, reconcileErr -} - -func (r *PostgreSQLInstanceReconciler) reconcileDependencies( - ctx context.Context, - instance *databasev1alpha1.PostgreSQLInstance, -) error { - if r.Timeout > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, r.Timeout) - defer cancel() - } - - if instance.Status.Phase == databasev1alpha1.PostgreSQLInstancePhaseReady && - instance.Status.ObservedGeneration != instance.Generation { - instance.Status.Phase = databasev1alpha1.PostgreSQLInstancePhaseValidating - setReconcilingCondition(&instance.Status.Conditions, instance.Generation, "instance dependencies are being validated") - return nil - } - - var version string - var err error - switch instance.Status.Phase { - case databasev1alpha1.PostgreSQLInstancePhaseValidating: - version, err = r.Initializer.Validate(ctx, instance) - if err == nil { - instance.Status.PostgreSQLVersion = version - instance.Status.Phase = databasev1alpha1.PostgreSQLInstancePhaseInitializingRegistry - setReconcilingCondition(&instance.Status.Conditions, instance.Generation, "PostgreSQL registry is being initialized") - } - case databasev1alpha1.PostgreSQLInstancePhaseInitializingRegistry: - version, err = r.Initializer.InitializeRegistry(ctx, instance) - if err == nil { - instance.Status.PostgreSQLVersion = version - instance.Status.Phase = databasev1alpha1.PostgreSQLInstancePhaseReady - instance.Status.ObservedGeneration = instance.Generation - setReadyCondition(&instance.Status.Conditions, instance.Generation, "instance dependencies are ready") - } - } - if err != nil { - instance.Status.ObservedGeneration = instance.Generation - reason := databasev1alpha1.ReasonDependencyUnavailable - var categorized interface{ ConditionReason() string } - if errors.As(err, &categorized) { - reason = categorized.ConditionReason() - } - setFailedCondition(&instance.Status.Conditions, instance.Generation, reason, - "instance dependency validation failed") - } - return err + return result, reconcileErr } // SetupWithManager sets up the controller with the Manager. diff --git a/internal/controller/postgresqlinstance_state_machine.go b/internal/controller/postgresqlinstance_state_machine.go index ddad1c1..4e08995 100644 --- a/internal/controller/postgresqlinstance_state_machine.go +++ b/internal/controller/postgresqlinstance_state_machine.go @@ -16,58 +16,115 @@ limitations under the License. package controller -import databasev1alpha1 "git.ddupan.top/panxiao81/postgresql-tenant-operator/api/v1alpha1" +import ( + "context" + "errors" + "time" -type instancePhaseResult struct { - phase databasev1alpha1.PostgreSQLInstancePhase - reconcilingMessage string -} + databasev1alpha1 "git.ddupan.top/panxiao81/postgresql-tenant-operator/api/v1alpha1" + ctrl "sigs.k8s.io/controller-runtime" +) -type instancePhaseHandler func(*databasev1alpha1.PostgreSQLInstance) instancePhaseResult +type instancePhaseHandler func(context.Context, *databasev1alpha1.PostgreSQLInstance) (ctrl.Result, error) type instanceStateMachine struct { - handlers map[databasev1alpha1.PostgreSQLInstancePhase]instancePhaseHandler + initializer PostgreSQLInstanceInitializer + handlers map[databasev1alpha1.PostgreSQLInstancePhase]instancePhaseHandler } -func newInstanceStateMachine() instanceStateMachine { - return instanceStateMachine{handlers: map[databasev1alpha1.PostgreSQLInstancePhase]instancePhaseHandler{ - databasev1alpha1.PostgreSQLInstancePhasePending: reconcileInstancePending, - databasev1alpha1.PostgreSQLInstancePhaseValidating: keepInstancePhase, - databasev1alpha1.PostgreSQLInstancePhaseInitializingRegistry: keepInstancePhase, - databasev1alpha1.PostgreSQLInstancePhaseReady: keepInstancePhase, - databasev1alpha1.PostgreSQLInstancePhaseDeleting: reconcileInstanceDeleting, - }} +func newInstanceStateMachine(initializer PostgreSQLInstanceInitializer) *instanceStateMachine { + m := &instanceStateMachine{initializer: initializer} + m.handlers = map[databasev1alpha1.PostgreSQLInstancePhase]instancePhaseHandler{ + databasev1alpha1.PostgreSQLInstancePhasePending: m.pending, + databasev1alpha1.PostgreSQLInstancePhaseValidating: m.validate, + databasev1alpha1.PostgreSQLInstancePhaseInitializingRegistry: m.initializeRegistry, + databasev1alpha1.PostgreSQLInstancePhaseReady: m.ready, + databasev1alpha1.PostgreSQLInstancePhaseDeleting: m.deleting, + } + return m } -func (m instanceStateMachine) reconcile(instance *databasev1alpha1.PostgreSQLInstance) instancePhaseResult { +func (m *instanceStateMachine) reconcile(ctx context.Context, instance *databasev1alpha1.PostgreSQLInstance) (ctrl.Result, error) { phase := instance.Status.Phase if !instance.DeletionTimestamp.IsZero() { phase = databasev1alpha1.PostgreSQLInstancePhaseDeleting } else if phase == "" { phase = databasev1alpha1.PostgreSQLInstancePhasePending } - handler, found := m.handlers[phase] if !found { - return instancePhaseResult{phase: phase} + handler = m.pending } - return handler(instance) + result, err := handler(ctx, instance) + if err != nil { + instance.Status.ObservedGeneration = instance.Generation + reason := databasev1alpha1.ReasonDependencyUnavailable + var categorized interface{ ConditionReason() string } + if errors.As(err, &categorized) { + reason = categorized.ConditionReason() + } + setFailedCondition(&instance.Status.Conditions, instance.Generation, reason, "instance dependency validation failed") + } + return result, err } -func reconcileInstancePending(*databasev1alpha1.PostgreSQLInstance) instancePhaseResult { - return instancePhaseResult{ - phase: databasev1alpha1.PostgreSQLInstancePhaseValidating, - reconcilingMessage: "instance dependencies are being validated", - } +func (m *instanceStateMachine) pending(_ context.Context, instance *databasev1alpha1.PostgreSQLInstance) (ctrl.Result, error) { + return advanceInstance(instance, databasev1alpha1.PostgreSQLInstancePhaseValidating, + "instance dependencies are being validated"), nil } -func reconcileInstanceDeleting(*databasev1alpha1.PostgreSQLInstance) instancePhaseResult { - return instancePhaseResult{ - phase: databasev1alpha1.PostgreSQLInstancePhaseDeleting, - reconcilingMessage: "instance deletion is reconciling", +func (m *instanceStateMachine) validate(ctx context.Context, instance *databasev1alpha1.PostgreSQLInstance) (ctrl.Result, error) { + if m.initializer == nil { + return ctrl.Result{}, nil } + version, err := m.initializer.Validate(ctx, instance) + if err != nil { + return ctrl.Result{}, err + } + instance.Status.PostgreSQLVersion = version + return advanceInstance(instance, databasev1alpha1.PostgreSQLInstancePhaseInitializingRegistry, + "PostgreSQL registry is being initialized"), nil } -func keepInstancePhase(instance *databasev1alpha1.PostgreSQLInstance) instancePhaseResult { - return instancePhaseResult{phase: instance.Status.Phase} +func (m *instanceStateMachine) initializeRegistry(ctx context.Context, instance *databasev1alpha1.PostgreSQLInstance) (ctrl.Result, error) { + if m.initializer == nil { + return ctrl.Result{}, nil + } + version, err := m.initializer.InitializeRegistry(ctx, instance) + if err != nil { + return ctrl.Result{}, err + } + instance.Status.PostgreSQLVersion = version + instance.Status.Phase = databasev1alpha1.PostgreSQLInstancePhaseReady + instance.Status.ObservedGeneration = instance.Generation + setReadyCondition(&instance.Status.Conditions, instance.Generation, "instance dependencies are ready") + return ctrl.Result{RequeueAfter: time.Minute}, nil +} + +func (m *instanceStateMachine) ready(ctx context.Context, instance *databasev1alpha1.PostgreSQLInstance) (ctrl.Result, error) { + if instance.Status.ObservedGeneration != instance.Generation { + return m.pending(ctx, instance) + } + if m.initializer == nil { + return ctrl.Result{}, nil + } + version, err := m.initializer.Validate(ctx, instance) + if err != nil { + instance.Status.Phase = databasev1alpha1.PostgreSQLInstancePhaseValidating + return ctrl.Result{}, err + } + instance.Status.PostgreSQLVersion = version + return ctrl.Result{RequeueAfter: time.Minute}, nil +} + +func (m *instanceStateMachine) deleting(_ context.Context, instance *databasev1alpha1.PostgreSQLInstance) (ctrl.Result, error) { + instance.Status.Phase = databasev1alpha1.PostgreSQLInstancePhaseDeleting + setReconcilingCondition(&instance.Status.Conditions, instance.Generation, "instance deletion is reconciling") + return ctrl.Result{}, nil +} + +func advanceInstance(instance *databasev1alpha1.PostgreSQLInstance, phase databasev1alpha1.PostgreSQLInstancePhase, message string) ctrl.Result { + instance.Status.Phase = phase + setReconcilingCondition(&instance.Status.Conditions, instance.Generation, message) + return ctrl.Result{RequeueAfter: time.Millisecond} } diff --git a/internal/controller/state_machine_test.go b/internal/controller/state_machine_test.go index ca58648..4b54995 100644 --- a/internal/controller/state_machine_test.go +++ b/internal/controller/state_machine_test.go @@ -34,6 +34,8 @@ type fakeInstanceInitializer struct { err error } +const testPostgreSQLVersion = "17.6" + func (f fakeInstanceInitializer) Validate(context.Context, *databasev1alpha1.PostgreSQLInstance) (string, error) { return f.validateVersion, f.err } @@ -43,19 +45,74 @@ func (f fakeInstanceInitializer) InitializeRegistry(context.Context, *databasev1 } var _ = Describe("phase handler state machines", func() { + It("persists validation intent before touching unavailable dependencies", func() { + instance := &databasev1alpha1.PostgreSQLInstance{} + machine := newInstanceStateMachine(fakeInstanceInitializer{err: errors.New("must not be called")}) + result, err := machine.reconcile(context.Background(), instance) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + Expect(instance.Status.Phase).To(Equal(databasev1alpha1.PostgreSQLInstancePhaseValidating)) + Expect(instance.Status.Conditions[0].Status).To(Equal(metav1.ConditionUnknown)) + }) + + It("persists a new validation checkpoint when a Ready Instance generation changes", func() { + instance := &databasev1alpha1.PostgreSQLInstance{ + ObjectMeta: metav1.ObjectMeta{Generation: 4}, + Status: databasev1alpha1.PostgreSQLInstanceStatus{ + Phase: databasev1alpha1.PostgreSQLInstancePhaseReady, ObservedGeneration: 3, + }, + } + machine := newInstanceStateMachine(fakeInstanceInitializer{err: errors.New("must not be called")}) + _, err := machine.reconcile(context.Background(), instance) + Expect(err).NotTo(HaveOccurred()) + Expect(instance.Status.Phase).To(Equal(databasev1alpha1.PostgreSQLInstancePhaseValidating)) + Expect(instance.Status.ObservedGeneration).To(Equal(int64(3))) + }) + + It("detects a dependency outage after Ready and recovers through registry initialization", func() { + instance := &databasev1alpha1.PostgreSQLInstance{ + ObjectMeta: metav1.ObjectMeta{Generation: 3}, + Status: databasev1alpha1.PostgreSQLInstanceStatus{ + Phase: databasev1alpha1.PostgreSQLInstancePhaseReady, ObservedGeneration: 3, + }, + } + machine := newInstanceStateMachine(fakeInstanceInitializer{err: errors.New("unavailable")}) + _, err := machine.reconcile(context.Background(), instance) + Expect(err).To(MatchError("unavailable")) + Expect(instance.Status.Phase).To(Equal(databasev1alpha1.PostgreSQLInstancePhaseValidating)) + Expect(instance.Status.Conditions[0].Status).To(Equal(metav1.ConditionFalse)) + machine.initializer = fakeInstanceInitializer{ + validateVersion: testPostgreSQLVersion, + registryVersion: testPostgreSQLVersion, + } + _, err = machine.reconcile(context.Background(), instance) + Expect(err).NotTo(HaveOccurred()) + Expect(instance.Status.Phase).To(Equal(databasev1alpha1.PostgreSQLInstancePhaseInitializingRegistry)) + _, err = machine.reconcile(context.Background(), instance) + Expect(err).NotTo(HaveOccurred()) + Expect(instance.Status.Conditions[0].Status).To(Equal(metav1.ConditionTrue)) + result, err := machine.reconcile(context.Background(), instance) + Expect(err).NotTo(HaveOccurred()) + Expect(instance.Status.Phase).To(Equal(databasev1alpha1.PostgreSQLInstancePhaseReady)) + Expect(result.RequeueAfter).To(Equal(time.Minute)) + }) + It("advances an Instance through external validation and registry initialization", func() { instance := &databasev1alpha1.PostgreSQLInstance{ ObjectMeta: metav1.ObjectMeta{Generation: 3}, Status: databasev1alpha1.PostgreSQLInstanceStatus{Phase: databasev1alpha1.PostgreSQLInstancePhaseValidating}, } - reconciler := &PostgreSQLInstanceReconciler{Initializer: fakeInstanceInitializer{ - validateVersion: "17.6", registryVersion: "17.6", - }} - Expect(reconciler.reconcileDependencies(context.Background(), instance)).To(Succeed()) + machine := newInstanceStateMachine(fakeInstanceInitializer{ + validateVersion: testPostgreSQLVersion, + registryVersion: testPostgreSQLVersion, + }) + _, err := machine.reconcile(context.Background(), instance) + Expect(err).NotTo(HaveOccurred()) Expect(instance.Status.Phase).To(Equal(databasev1alpha1.PostgreSQLInstancePhaseInitializingRegistry)) - Expect(reconciler.reconcileDependencies(context.Background(), instance)).To(Succeed()) + _, err = machine.reconcile(context.Background(), instance) + Expect(err).NotTo(HaveOccurred()) Expect(instance.Status.Phase).To(Equal(databasev1alpha1.PostgreSQLInstancePhaseReady)) - Expect(instance.Status.PostgreSQLVersion).To(Equal("17.6")) + Expect(instance.Status.PostgreSQLVersion).To(Equal(testPostgreSQLVersion)) Expect(instance.Status.ObservedGeneration).To(Equal(int64(3))) Expect(instance.Status.Conditions).To(ConsistOf(And( HaveField("Status", metav1.ConditionTrue), HaveField("Reason", databasev1alpha1.ReasonReady), @@ -67,8 +124,9 @@ var _ = Describe("phase handler state machines", func() { ObjectMeta: metav1.ObjectMeta{Generation: 2}, Status: databasev1alpha1.PostgreSQLInstanceStatus{Phase: databasev1alpha1.PostgreSQLInstancePhaseValidating}, } - reconciler := &PostgreSQLInstanceReconciler{Initializer: fakeInstanceInitializer{err: errors.New("unavailable")}} - Expect(reconciler.reconcileDependencies(context.Background(), instance)).To(MatchError("unavailable")) + machine := newInstanceStateMachine(fakeInstanceInitializer{err: errors.New("unavailable")}) + _, err := machine.reconcile(context.Background(), instance) + Expect(err).To(MatchError("unavailable")) Expect(instance.Status.Phase).To(Equal(databasev1alpha1.PostgreSQLInstancePhaseValidating)) Expect(instance.Status.Conditions).To(ConsistOf(And( HaveField("Status", metav1.ConditionFalse), HaveField("Reason", databasev1alpha1.ReasonDependencyUnavailable), @@ -77,12 +135,13 @@ var _ = Describe("phase handler state machines", func() { DescribeTable("dispatches Instance phases", func(instance *databasev1alpha1.PostgreSQLInstance, expected databasev1alpha1.PostgreSQLInstancePhase, hasMessage bool) { - result := newInstanceStateMachine().reconcile(instance) - Expect(result.phase).To(Equal(expected)) + _, err := newInstanceStateMachine(nil).reconcile(context.Background(), instance) + Expect(err).NotTo(HaveOccurred()) + Expect(instance.Status.Phase).To(Equal(expected)) if hasMessage { - Expect(result.reconcilingMessage).NotTo(BeEmpty()) + Expect(instance.Status.Conditions).NotTo(BeEmpty()) } else { - Expect(result.reconcilingMessage).To(BeEmpty()) + Expect(instance.Status.Conditions).To(BeEmpty()) } }, Entry("starts validation from an empty checkpoint",