From 9a9dee819d5e7d24c0ee8eead123179153b49239 Mon Sep 17 00:00:00 2001 From: Boris Urbanik Date: Thu, 7 May 2026 21:24:27 +0100 Subject: [PATCH] Fix status reconciler requeue logic: RequeueAfter and skip equal-status write Two issues in the previous requeue logic: 1. The guard `equalStatus && newAvailable` caused the unavailable-but-equal case to fall through to a status write even when nothing had changed, producing a no-op write on every reconcile while the instance remained unavailable. The new `if equalStatus` guard short-circuits both the available and unavailable steady states, avoiding the unnecessary write. 2. Both unavailable return paths used `Requeue: true` (immediate requeue), which causes tight-loop reconciliation against an instance that is still unavailable. Switching to `RequeueAfter: 30s` gives components time to recover between checks and avoids extending backoff counter. Co-Authored-By: Claude Sonnet 4.6 --- controllers/apps/apimanager_controller.go | 2 +- .../apps/apimanager_status_reconciler.go | 34 +++++++------------ .../apps/apimanager_status_reconciler_test.go | 12 +++---- 3 files changed, 19 insertions(+), 29 deletions(-) diff --git a/controllers/apps/apimanager_controller.go b/controllers/apps/apimanager_controller.go index 4e29a63dd..302e8c7a9 100644 --- a/controllers/apps/apimanager_controller.go +++ b/controllers/apps/apimanager_controller.go @@ -175,7 +175,7 @@ func (r *APIManagerReconciler) Reconcile(ctx context.Context, req ctrl.Request) return specResult, nil } - if statusResult.Requeue { + if statusResult.Requeue || statusResult.RequeueAfter > 0 { logger.Info("Reconciling not finished. Requeueing.") return statusResult, nil } diff --git a/controllers/apps/apimanager_status_reconciler.go b/controllers/apps/apimanager_status_reconciler.go index b277c341e..6f0638d28 100644 --- a/controllers/apps/apimanager_status_reconciler.go +++ b/controllers/apps/apimanager_status_reconciler.go @@ -5,6 +5,7 @@ import ( "fmt" "sort" "strings" + "time" appsv1alpha1 "github.com/3scale/3scale-operator/apis/apps/v1alpha1" subController "github.com/3scale/3scale-operator/controllers/subscription" @@ -49,33 +50,22 @@ func (s *APIManagerStatusReconciler) Reconcile() (reconcile.Result, error) { return reconcile.Result{}, fmt.Errorf("failed to calculate status: %w", err) } - // Read availability from the newly computed status, not the stale pre-reconcile snapshot. - // Using old status caused a True-to-False requeue gap: old=Available=True would suppress - // the requeue even after writing Available=False to the API server (THREESCALE-10754). - newAvailable := newStatus.Conditions.IsTrueFor(appsv1alpha1.APIManagerAvailableConditionType) - equalStatus := s.apimanagerResource.Status.Equals(newStatus, s.logger) - s.logger.V(1).Info("Status", "status is different", !equalStatus) - if equalStatus && newAvailable { - // Steady state - s.logger.V(1).Info("Status was not updated") - return reconcile.Result{}, nil - } - - s.apimanagerResource.Status = *newStatus - updateErr := s.Client().Status().Update(s.Context(), s.apimanagerResource) - if updateErr != nil { - // Ignore conflicts, resource might just be outdated. - if errors.IsConflict(updateErr) { - s.logger.Info("Failed to update status: resource might just be outdated") - return reconcile.Result{Requeue: true}, nil + if !equalStatus { + s.logger.V(1).Info("Status is different") + s.apimanagerResource.Status = *newStatus + if err := s.Client().Status().Update(s.Context(), s.apimanagerResource); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to update status: %w", err) } - - return reconcile.Result{}, fmt.Errorf("failed to update status: %w", updateErr) + } else { + s.logger.V(1).Info("Status was not updated") } + // Re-check status periodically specifically only when availability is failing; this + // is an optimization - once we're available we rely only on watch events + re-sync interval + newAvailable := newStatus.Conditions.IsTrueFor(appsv1alpha1.APIManagerAvailableConditionType) if !newAvailable { - return reconcile.Result{Requeue: true}, nil + return reconcile.Result{RequeueAfter: 30 * time.Second}, nil } return reconcile.Result{}, nil diff --git a/controllers/apps/apimanager_status_reconciler_test.go b/controllers/apps/apimanager_status_reconciler_test.go index 04664bb9b..3cd05c9da 100644 --- a/controllers/apps/apimanager_status_reconciler_test.go +++ b/controllers/apps/apimanager_status_reconciler_test.go @@ -609,10 +609,10 @@ func TestAPIManagerStatusReconciler_Reconcile_statusConditions(t *testing.T) { } } -// TestAPIManagerStatusReconciler_Reconcile_requeueOnTrueToFalseTransition is a regression -// test for the stale-read requeue bug: when Available transitions from True to False the -// reconciler must requeue, not settle silently at Available=False. -func TestAPIManagerStatusReconciler_Reconcile_requeueOnTrueToFalseTransition(t *testing.T) { +// TestAPIManagerStatusReconciler_Reconcile_requeueAfterWhenUnavailable verifies that when +// Available transitions from True to False the reconciler schedules a requeue rather than +// settling silently at Available=False. +func TestAPIManagerStatusReconciler_Reconcile_requeueAfterWhenUnavailable(t *testing.T) { namespace := "test-namespace" t.Setenv("PREFLIGHT_CHECKS_BYPASS", "true") @@ -640,7 +640,7 @@ func TestAPIManagerStatusReconciler_Reconcile_requeueOnTrueToFalseTransition(t * if err != nil { t.Fatalf("Reconcile() unexpected error: %v", err) } - if !result.Requeue { - t.Errorf("Reconcile() Requeue = false, want true on Available True-to-False transition") + if result.RequeueAfter == 0 { + t.Errorf("Reconcile() RequeueAfter = 0, want non-zero on Available True-to-False transition") } }