diff --git a/controllers/apps/apimanager_controller.go b/controllers/apps/apimanager_controller.go index 4e29a63dd..829546a89 100644 --- a/controllers/apps/apimanager_controller.go +++ b/controllers/apps/apimanager_controller.go @@ -108,7 +108,7 @@ func (r *APIManagerReconciler) Reconcile(ctx context.Context, req ctrl.Request) if statusErr != nil { return ctrl.Result{}, statusErr } - if statusResult.Requeue { + if statusResult.Requeue || statusResult.RequeueAfter > 0 { logger.Info("Reconciling not finished. Requeueing.") return statusResult, nil } @@ -138,7 +138,7 @@ func (r *APIManagerReconciler) Reconcile(ctx context.Context, req ctrl.Request) if statusErr != nil { return ctrl.Result{}, statusErr } - if statusResult.Requeue { + if statusResult.Requeue || statusResult.RequeueAfter > 0 { logger.Info("Reconciling not finished. Requeueing.") return statusResult, nil } @@ -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..88fd7d678 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,28 @@ 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.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 + } + return reconcile.Result{}, fmt.Errorf("failed to update status: %w", updateErr) } - - 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..80f592747 100644 --- a/controllers/apps/apimanager_status_reconciler_test.go +++ b/controllers/apps/apimanager_status_reconciler_test.go @@ -609,38 +609,100 @@ 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_requeueBehaviour verifies the requeue semantics +// under three scenarios: +// +// - True→False transition: status changes; RequeueAfter is set +// - Unavailable steady-state: status is already equal (False); no write, RequeueAfter still set +// - Available steady-state: status is already equal (True); no write, RequeueAfter is zero +func TestAPIManagerStatusReconciler_Reconcile_requeueBehaviour(t *testing.T) { namespace := "test-namespace" t.Setenv("PREFLIGHT_CHECKS_BYPASS", "true") - // Seed the CR with Available=True in its current status (old state). - am := getTestAPIManager(namespace) - am.Status.Conditions = common.Conditions{ - {Type: appsv1alpha1.APIManagerAvailableConditionType, Status: corev1.ConditionTrue}, + tests := []struct { + name string + deploymentsUp bool + steadyState bool // if true, pre-seed exact status so equalStatus==true + seedAvailable corev1.ConditionStatus // Available condition _before_ Reconcile - only used if steadyState == false + wantRequeueAfter bool + }{ + { + name: "unavailable transition (True to False)", + deploymentsUp: false, + steadyState: false, + seedAvailable: corev1.ConditionTrue, + wantRequeueAfter: true, + }, + { + name: "unavailable steady-state (False to False, equal)", + deploymentsUp: false, + steadyState: true, + wantRequeueAfter: true, + }, + { + name: "available transition (False to True)", + deploymentsUp: true, + steadyState: false, + seedAvailable: corev1.ConditionFalse, + wantRequeueAfter: false, + }, + { + name: "available steady-state (True to True, equal)", + deploymentsUp: true, + steadyState: true, + wantRequeueAfter: false, + }, } - // Cluster state: deployments not available, so calculateStatus() will return Available=False. - objects := concat( - getAllStandardDeployments(namespace, string(am.UID), false), - getRequiredSecrets(namespace), - getRequiredRoutes(namespace, testWildcardDomain, testTenantName), - []runtime.Object{am}, - ) - - s := &APIManagerStatusReconciler{ - BaseReconciler: getAPIManagerBaseReconciler(objects...), - apimanagerResource: am, - logger: logr.Discard(), - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + am := getTestAPIManager(namespace) - result, err := s.Reconcile() - 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") + objects := concat( + getAllStandardDeployments(namespace, string(am.UID), tt.deploymentsUp), + getRequiredSecrets(namespace), + getRequiredRoutes(namespace, testWildcardDomain, testTenantName), + []runtime.Object{am}, + ) + + s := &APIManagerStatusReconciler{ + BaseReconciler: getAPIManagerBaseReconciler(objects...), + apimanagerResource: am, + logger: logr.Discard(), + } + + if tt.steadyState { + // Pre-seed am.Status with the exact value calculateStatus() will compute so + // that Equals() returns true and the equalStatus short-circuit is exercised. + computed, err := s.calculateStatus() + if err != nil { + t.Fatalf("calculateStatus() unexpected error: %v", err) + } + am.Status = *computed + } else { + am.Status.Conditions = common.Conditions{ + {Type: appsv1alpha1.APIManagerAvailableConditionType, Status: tt.seedAvailable}, + } + } + + result, err := s.Reconcile() + if err != nil { + t.Fatalf("Reconcile() unexpected error: %v", err) + } + + if result.Requeue { + t.Errorf("Requeue = true, normal (non-error) reconcile should only return requeueAfter") + } + + if tt.wantRequeueAfter { + if result.RequeueAfter == 0 { + t.Errorf("RequeueAfter = 0, want non-zero when unavailable") + } + } else { + if result.RequeueAfter != 0 { + t.Errorf("RequeueAfter = %v, want 0 when available", result.RequeueAfter) + } + } + }) } }