THREESCALE-14654 Fix status reconciler requeue logic

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
 	}

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

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)
+				}
+			}
+		})
 	}
 }