Merge pull request #1982 from benluddy/continuous-registry-resync

Bug 1920526: Fix zero-delay resyncs for certain registry update policies.

Author: openshift-merge-robot

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