Fix zero-delay resyncs for certain registry update policies.

If the registry update polling interval is greater than and a multiple
of the operator's default resync period (15m), and there are fewer
than 15 minutes remaining until the next poll is due, the
catalog-operator will continuously resync its associated CatalogSource
until the next poll time is reached. This is not beneficial to update
polling and can generate excessive load that negatively impacts useful
work on the cluster.

Signed-off-by: Ben Luddy <[email protected]>

Author: benluddy

pkg/controller/operators/catalog/operator.go

@@ -640,7 +640,7 @@ func (o *Operator) syncRegistryServer(logger *logrus.Entry, in *v1alpha1.Catalog
 	// requeue the catalog sync based on the polling interval, for accurate syncs of catalogs with polling enabled
 	if out.Spec.UpdateStrategy != nil {
 		logger.Debugf("requeuing registry server sync based on polling interval %s", out.Spec.UpdateStrategy.Interval.Duration.String())
-		resyncPeriod := reconciler.SyncRegistryUpdateInterval(out)
+		resyncPeriod := reconciler.SyncRegistryUpdateInterval(out, time.Now())
 		o.catsrcQueueSet.RequeueAfter(out.GetNamespace(), out.GetName(), queueinformer.ResyncWithJitter(resyncPeriod, 0.1)())
 		return
 	}

pkg/controller/registry/reconciler/grpc_polling.go

@@ -12,7 +12,7 @@ import (
 // SyncRegistryUpdateInterval returns a duration to use when requeuing the catalog source for reconciliation.
 // This ensures that the catalog is being synced on the correct time interval based on its spec.
 // Note: this function assumes the catalog has an update strategy set.
-func SyncRegistryUpdateInterval(source *v1alpha1.CatalogSource) time.Duration {
+func SyncRegistryUpdateInterval(source *v1alpha1.CatalogSource, now time.Time) time.Duration {
 	pollingInterval := source.Spec.UpdateStrategy.Interval.Duration
 	latestPoll := source.Status.LatestImageRegistryPoll
 	creationTimestamp := source.CreationTimestamp.Time
@@ -22,19 +22,21 @@ func SyncRegistryUpdateInterval(source *v1alpha1.CatalogSource) time.Duration {
 		return pollingInterval
 	}
 	// Resync based on the delta between the default sync and the actual last poll if the interval is greater than the default
-	return defaultOr(latestPoll, pollingInterval, creationTimestamp)
+	return defaultOr(latestPoll, pollingInterval, creationTimestamp, now)
 }
 
-// defaultOr returns either the default resync period or the modulus of the polling interval and the default.
+// defaultOr returns either the default resync period or the time remaining until the next poll is due, whichever is smaller.
 // For example, if the polling interval is 40 minutes, OLM will sync after ~30 minutes and see that the next sync
-// for this catalog should be sooner than the default (15 minutes) and return 10 minutes (40 % 15).
-func defaultOr(latestPoll *metav1.Time, pollingInterval time.Duration, creationTimestamp time.Time) time.Duration {
+// for this catalog should be in 10 minutes, sooner than the default 15 minutes, and return 10 minutes.
+func defaultOr(latestPoll *metav1.Time, pollingInterval time.Duration, creationTimestamp time.Time, now time.Time) time.Duration {
 	if latestPoll.IsZero() {
 		latestPoll = &metav1.Time{Time: creationTimestamp}
 	}
+
+	remaining := latestPoll.Add(pollingInterval).Sub(now)
 	// sync ahead of the default interval in the case where now + default sync is after the last sync plus the interval
-	if time.Now().Add(queueinformer.DefaultResyncPeriod).After(latestPoll.Add(pollingInterval)) {
-		return (pollingInterval % queueinformer.DefaultResyncPeriod).Round(time.Minute)
+	if remaining < queueinformer.DefaultResyncPeriod {
+		return remaining
 	}
 	// return the default sync period otherwise: the next sync cycle will check again
 	return queueinformer.DefaultResyncPeriod

pkg/controller/registry/reconciler/grpc_polling_test.go

@@ -11,6 +11,7 @@ import (
 )
 
 func TestSyncRegistryUpdateInterval(t *testing.T) {
+	now := time.Date(2021, time.January, 29, 14, 47, 0, 0, time.UTC)
 	tests := []struct {
 		name     string
 		source   *v1alpha1.CatalogSource
@@ -46,12 +47,32 @@ func TestSyncRegistryUpdateInterval(t *testing.T) {
 			},
 			expected: 15 * time.Minute,
 		},
+		{
+			name: "PollingIntervalMultipleOfDefaultResyncPeriod",
+			source: &v1alpha1.CatalogSource{
+				Spec: v1alpha1.CatalogSourceSpec{
+					UpdateStrategy: &v1alpha1.UpdateStrategy{
+						RegistryPoll: &v1alpha1.RegistryPoll{
+							Interval: &metav1.Duration{
+								Duration: 2 * queueinformer.DefaultResyncPeriod,
+							},
+						},
+					},
+				},
+				Status: v1alpha1.CatalogSourceStatus{
+					LatestImageRegistryPoll: &metav1.Time{
+						Time: now.Add(1*time.Second - 2*queueinformer.DefaultResyncPeriod),
+					},
+				},
+			},
+			expected: 1 * time.Second,
+		},
 		{
 			name: "PollingInterval10Minutes/AlreadyUpdated",
 			source: &v1alpha1.CatalogSource{
 				Status: v1alpha1.CatalogSourceStatus{
 					LatestImageRegistryPoll: &metav1.Time{
-						time.Now().Add(-(5 * time.Minute)),
+						Time: now.Add(-(5 * time.Minute)),
 					},
 				},
 				Spec: v1alpha1.CatalogSourceSpec{
@@ -71,7 +92,7 @@ func TestSyncRegistryUpdateInterval(t *testing.T) {
 			source: &v1alpha1.CatalogSource{
 				ObjectMeta: metav1.ObjectMeta{
 					CreationTimestamp: metav1.Time{
-						Time: time.Now().Add(-(35 * time.Minute)),
+						Time: now.Add(-(35 * time.Minute)),
 					},
 				},
 				Spec: v1alpha1.CatalogSourceSpec{
@@ -84,14 +105,14 @@ func TestSyncRegistryUpdateInterval(t *testing.T) {
 					},
 				},
 			},
-			expected: 10 * time.Minute,
+			expected: 5 * time.Minute,
 		},
 		{
 			name: "PollingInterval40Minutes/AlreadyUpdated30MinutesAgo",
 			source: &v1alpha1.CatalogSource{
 				Status: v1alpha1.CatalogSourceStatus{
 					LatestImageRegistryPoll: &metav1.Time{
-						time.Now().Add(-(30 * time.Minute)),
+						Time: now.Add(-(30 * time.Minute)),
 					},
 				},
 				Spec: v1alpha1.CatalogSourceSpec{
@@ -111,7 +132,7 @@ func TestSyncRegistryUpdateInterval(t *testing.T) {
 			source: &v1alpha1.CatalogSource{
 				ObjectMeta: metav1.ObjectMeta{
 					CreationTimestamp: metav1.Time{
-						Time: time.Now().Add(-(15 * time.Minute)),
+						Time: now.Add(-(15 * time.Minute)),
 					},
 				},
 				Spec: v1alpha1.CatalogSourceSpec{
@@ -131,7 +152,7 @@ func TestSyncRegistryUpdateInterval(t *testing.T) {
 			source: &v1alpha1.CatalogSource{
 				Status: v1alpha1.CatalogSourceStatus{
 					LatestImageRegistryPoll: &metav1.Time{
-						time.Now().Add(-(15 * time.Minute)),
+						Time: now.Add(-(15 * time.Minute)),
 					},
 				},
 				Spec: v1alpha1.CatalogSourceSpec{
@@ -149,10 +170,11 @@ func TestSyncRegistryUpdateInterval(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		t.Logf("name %s", tt.name)
-		d := SyncRegistryUpdateInterval(tt.source)
-		if d != tt.expected {
-			t.Fatalf("unexpected registry sync interval for %s: expected %#v got %#v", tt.name, tt.expected, d)
-		}
+		t.Run(tt.name, func(t *testing.T) {
+			d := SyncRegistryUpdateInterval(tt.source, now)
+			if d != tt.expected {
+				t.Errorf("unexpected registry sync interval for %s: expected %s got %s", tt.name, tt.expected, d)
+			}
+		})
 	}
 }