From 61672eb4110e041bbfcce053bc1be2fe592ae171 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Andr=C3=A9?= <m.andre@redhat.com>
Date: Fri, 25 Sep 2026 08:24:37 +0200
Subject: [PATCH] Apply jitter to not-reconciling resync requeue

The ShouldReconcile=false path in reconcileNormal scheduled
unjittered requeues via RemainingUntilNextSync. After a
successful resync, the SSA status patch (updating lastSyncTime)
triggers a watch event that re-enqueues the object immediately.
The watch-triggered reconcile runs ShouldReconcile=false and
replaces the pending jittered requeue with an unjittered one,
causing all resources sharing the same resyncPeriod to converge
on identical timing.

Wrap the remaining duration with CalculateJitteredDuration so
that watch-triggered reconciliations preserve jitter and
resources maintain independent schedules.

Fixes #936
---
 .../generic/reconciler/controller.go          |  7 +-
 .../generic/reconciler/controller_test.go     | 82 +++++++++++++++++++
 2 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/internal/controllers/generic/reconciler/controller.go b/internal/controllers/generic/reconciler/controller.go
index d4ac35c61..41bd540a6 100644
--- a/internal/controllers/generic/reconciler/controller.go
+++ b/internal/controllers/generic/reconciler/controller.go
@@ -210,7 +210,12 @@ func (c *Controller[
 	if !ShouldReconcile(objAdapter.GetObject(), objAdapter.GetLastSyncTime(), effectiveResyncPeriod) {
 		log.V(logging.Verbose).Info("Status is up to date: not reconciling")
 		if remaining := resync.RemainingUntilNextSync(objAdapter.GetLastSyncTime(), effectiveResyncPeriod); remaining > 0 {
-			return reconcileStatus.WithRequeue(remaining)
+			// Apply jitter so that watch-triggered reconciliations
+			// (e.g. from status updates) do not replace a previously
+			// jittered requeue with an unjittered one, which would
+			// cause all resources sharing the same resyncPeriod to
+			// synchronise.
+			return reconcileStatus.WithRequeue(resync.CalculateJitteredDuration(remaining))
 		}
 		return reconcileStatus
 	}
diff --git a/internal/controllers/generic/reconciler/controller_test.go b/internal/controllers/generic/reconciler/controller_test.go
index e3a5d48ab..eef3934ee 100644
--- a/internal/controllers/generic/reconciler/controller_test.go
+++ b/internal/controllers/generic/reconciler/controller_test.go
@@ -311,6 +311,88 @@ func TestShouldReconcile_ExistingBehaviorUnchanged_ResyncPeriodZero(t *testing.T
 	}
 }
 
+// simulateNotReconcilingRequeue simulates the ShouldReconcile=false path in
+// reconcileNormal. When the object is up to date but a periodic resync is
+// configured, this path schedules a requeue for the remaining time until the
+// next resync—with jitter applied to prevent synchronisation of resources
+// sharing the same period.
+func simulateNotReconcilingRequeue(lastSyncTime *metav1.Time, resyncPeriod time.Duration) progress.ReconcileStatus {
+	var reconcileStatus progress.ReconcileStatus
+	if remaining := resync.RemainingUntilNextSync(lastSyncTime, resyncPeriod); remaining > 0 {
+		reconcileStatus = reconcileStatus.WithRequeue(resync.CalculateJitteredDuration(remaining))
+	}
+	return reconcileStatus
+}
+
+// TestNotReconcilingRequeue_JitterApplied verifies that the
+// ShouldReconcile=false path applies jitter to the remaining-time requeue.
+// Without jitter, watch-triggered reconciliations (e.g. from status updates)
+// would replace a previously jittered requeue with an unjittered one, causing
+// all resources sharing the same resyncPeriod to synchronise.
+func TestNotReconcilingRequeue_JitterApplied(t *testing.T) {
+	t.Parallel()
+
+	const (
+		resyncPeriod = 10 * time.Minute
+		samples      = 200
+	)
+
+	// Simulate a status update right after a successful resync: lastSyncTime
+	// is very recent, so remaining ≈ resyncPeriod.
+	lastSync := nowPtr()
+
+	unique := make(map[time.Duration]struct{}, samples)
+	for i := range samples {
+		rs := simulateNotReconcilingRequeue(lastSync, resyncPeriod)
+		d := rs.GetRequeue()
+		if d == 0 {
+			t.Fatalf("sample %d: expected non-zero requeue", i)
+		}
+		unique[d] = struct{}{}
+	}
+
+	// With jitter, virtually all samples should be distinct.
+	minUnique := samples * 9 / 10
+	if len(unique) < minUnique {
+		t.Errorf("not-reconciling requeue appears unjittered: only %d unique values out of %d samples (want >= %d)",
+			len(unique), samples, minUnique)
+	}
+}
+
+// TestNotReconcilingRequeue_Range verifies that the not-reconciling requeue
+// is within the expected jitter range relative to the remaining time.
+func TestNotReconcilingRequeue_Range(t *testing.T) {
+	t.Parallel()
+
+	const resyncPeriod = 10 * time.Minute
+
+	// lastSyncTime 2 minutes ago → remaining ≈ 8 minutes.
+	lastSync := agoPtr(2 * time.Minute)
+
+	for i := range 100 {
+		rs := simulateNotReconcilingRequeue(lastSync, resyncPeriod)
+		d := rs.GetRequeue()
+
+		// The remaining time is approximately 8 minutes. Jitter adds
+		// [0%, 20%], so the requeue should be in [~8m, ~9.6m].
+		// We use generous bounds to account for time passing during the test.
+		if d < 7*time.Minute || d > 10*time.Minute {
+			t.Errorf("sample %d: requeue %v outside expected range [7m, 10m]", i, d)
+		}
+	}
+}
+
+// TestNotReconcilingRequeue_DisabledResync verifies that no requeue is
+// scheduled when resync is disabled.
+func TestNotReconcilingRequeue_DisabledResync(t *testing.T) {
+	t.Parallel()
+
+	rs := simulateNotReconcilingRequeue(nowPtr(), 0)
+	if d := rs.GetRequeue(); d != 0 {
+		t.Errorf("expected no requeue when resync disabled; got %v", d)
+	}
+}
+
 // scheduleResyncRequeue simulates the resync scheduling logic added to the end
 // of reconcileNormal:
 //