Fix rapid reconciliation (#332)

Case exists where IsReadyAt will use a stale deploy time, causing it to always return true and reconcile every two seconds. Check if deploy status is stale before using the time from it Authored-by: Eli Wrenn <[email protected]>
carvel-dev · Aug 12, 2021 · 3d43b42 · 3d43b42
1 parent adae427
commit 3d43b42
Show file tree

Hide file tree

Showing 4 changed files with 128 additions and 26 deletions.
diff --git a/pkg/app/reconcile_timer.go b/pkg/app/reconcile_timer.go
@@ -4,10 +4,12 @@
 package app
 
 import (
+	"fmt"
 	"math"
 	"time"
 
 	"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 )
 
@@ -39,19 +41,9 @@ func (rt ReconcileTimer) IsReadyAt(timeAt time.Time) bool {
 		return false
 	}
 
-	var lastReconcileTime time.Time
-
-	// Use latest deploy time if available, otherwise fallback to fetch
-	// If no timestamp is available, enqueue immediately
-	lastDeploy := rt.app.Status.Deploy
-	lastFetch := rt.app.Status.Fetch
-	if lastDeploy != nil && !lastDeploy.UpdatedAt.Time.IsZero() {
-		lastReconcileTime = rt.app.Status.Deploy.UpdatedAt.Time
-	} else {
-		if lastFetch == nil {
-			return true
-		}
-		lastReconcileTime = lastFetch.UpdatedAt.Time
+	lastReconcileTime, err := rt.lastReconcileTime()
+	if err != nil {
+		return true
 	}
 
 	if rt.hasReconcileStatus(v1alpha1.ReconcileFailed) {
@@ -104,3 +96,33 @@ func (rt ReconcileTimer) applyJitter(t time.Duration) time.Duration {
 	const appJitter time.Duration = 5 * time.Second
 	return t - appJitter + wait.Jitter(appJitter, 1.0)
 }
+
+func (rt ReconcileTimer) lastReconcileTime() (time.Time, error) {
+	// Determine latest time from status and use that as the
+	// last reconcile time
+	lastReconcileTime := metav1.Time{}
+	times := []metav1.Time{}
+	if rt.app.Status.Fetch != nil {
+		times = append(times, rt.app.Status.Fetch.UpdatedAt)
+	}
+
+	if rt.app.Status.Template != nil {
+		times = append(times, rt.app.Status.Template.UpdatedAt)
+	}
+
+	if rt.app.Status.Deploy != nil {
+		times = append(times, rt.app.Status.Deploy.UpdatedAt)
+	}
+
+	for _, time := range times {
+		if lastReconcileTime.Before(&time) {
+			lastReconcileTime = time
+		}
+	}
+
+	if lastReconcileTime.IsZero() {
+		return time.Time{}, fmt.Errorf("could not determine time of last reconcile")
+	}
+
+	return lastReconcileTime.Time, nil
+}
diff --git a/pkg/app/reconcile_timer_test.go b/pkg/app/reconcile_timer_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
 	"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -193,3 +194,31 @@ func TestFailedIsReadyAt(t *testing.T) {
 		t.Fatalf("Expected app to not be ready under syncPeriod of 2s")
 	}
 }
+
+func TestIsReadyAtWithStaleDeployTime(t *testing.T) {
+	syncPeriod := 2 * time.Second
+	timeNow := time.Now()
+	timeOfReady := timeNow.Add(syncPeriod)
+
+	app := v1alpha1.App{
+		Spec: v1alpha1.AppSpec{
+			SyncPeriod: &metav1.Duration{Duration: syncPeriod},
+		},
+		Status: v1alpha1.AppStatus{
+			Fetch: &v1alpha1.AppStatusFetch{
+				UpdatedAt: metav1.Time{Time: timeOfReady},
+				Error:     "I've failed you",
+			},
+			Deploy: &v1alpha1.AppStatusDeploy{
+				UpdatedAt: metav1.Time{Time: timeNow},
+			},
+			ConsecutiveReconcileFailures: 1,
+			GenericStatus: v1alpha1.GenericStatus{
+				Conditions: []v1alpha1.AppCondition{{Type: v1alpha1.ReconcileFailed}},
+			},
+		},
+	}
+
+	isReady := NewReconcileTimer(app).IsReadyAt(timeOfReady.Add(1 * time.Second))
+	require.False(t, isReady, "Expected app not to be ready, because deploy time is stale")
+}
diff --git a/pkg/pkgrepository/reconcile_timer.go b/pkg/pkgrepository/reconcile_timer.go
@@ -4,10 +4,12 @@
 package pkgrepository
 
 import (
+	"fmt"
 	"math"
 	"time"
 
 	"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 )
 
@@ -39,19 +41,9 @@ func (rt ReconcileTimer) IsReadyAt(timeAt time.Time) bool {
 		return false
 	}
 
-	var lastReconcileTime time.Time
-
-	// Use latest deploy time if available, otherwise fallback to fetch
-	// If no timestamp is available, enqueue immediately
-	lastDeploy := rt.app.Status.Deploy
-	lastFetch := rt.app.Status.Fetch
-	if lastDeploy != nil && !lastDeploy.UpdatedAt.Time.IsZero() {
-		lastReconcileTime = rt.app.Status.Deploy.UpdatedAt.Time
-	} else {
-		if lastFetch == nil {
-			return true
-		}
-		lastReconcileTime = lastFetch.UpdatedAt.Time
+	lastReconcileTime, err := rt.lastReconcileTime()
+	if err != nil {
+		return true
 	}
 
 	if rt.hasReconcileStatus(v1alpha1.ReconcileFailed) {
@@ -104,3 +96,33 @@ func (rt ReconcileTimer) applyJitter(t time.Duration) time.Duration {
 	const appJitter time.Duration = 5 * time.Second
 	return t - appJitter + wait.Jitter(appJitter, 1.0)
 }
+
+func (rt ReconcileTimer) lastReconcileTime() (time.Time, error) {
+	// Determine latest time from status and use that as the
+	// last reconcile time
+	lastReconcileTime := metav1.Time{}
+	times := []metav1.Time{}
+	if rt.app.Status.Fetch != nil {
+		times = append(times, rt.app.Status.Fetch.UpdatedAt)
+	}
+
+	if rt.app.Status.Template != nil {
+		times = append(times, rt.app.Status.Template.UpdatedAt)
+	}
+
+	if rt.app.Status.Deploy != nil {
+		times = append(times, rt.app.Status.Deploy.UpdatedAt)
+	}
+
+	for _, time := range times {
+		if lastReconcileTime.Before(&time) {
+			lastReconcileTime = time
+		}
+	}
+
+	if lastReconcileTime.IsZero() {
+		return time.Time{}, fmt.Errorf("could not determine time of last reconcile")
+	}
+
+	return lastReconcileTime.Time, nil
+}
diff --git a/pkg/pkgrepository/reconcile_timer_test.go b/pkg/pkgrepository/reconcile_timer_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
 	"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -193,3 +194,31 @@ func TestFailedIsReadyAt(t *testing.T) {
 		t.Fatalf("Expected app to not be ready under syncPeriod of 2s")
 	}
 }
+
+func TestIsReadyAtWithStaleDeployTime(t *testing.T) {
+	syncPeriod := 2 * time.Second
+	timeNow := time.Now()
+	timeOfReady := timeNow.Add(syncPeriod)
+
+	app := v1alpha1.App{
+		Spec: v1alpha1.AppSpec{
+			SyncPeriod: &metav1.Duration{Duration: syncPeriod},
+		},
+		Status: v1alpha1.AppStatus{
+			Fetch: &v1alpha1.AppStatusFetch{
+				UpdatedAt: metav1.Time{Time: timeOfReady},
+				Error:     "I've failed you",
+			},
+			Deploy: &v1alpha1.AppStatusDeploy{
+				UpdatedAt: metav1.Time{Time: timeNow},
+			},
+			ConsecutiveReconcileFailures: 1,
+			GenericStatus: v1alpha1.GenericStatus{
+				Conditions: []v1alpha1.AppCondition{{Type: v1alpha1.ReconcileFailed}},
+			},
+		},
+	}
+
+	isReady := NewReconcileTimer(app).IsReadyAt(timeOfReady.Add(1 * time.Second))
+	require.False(t, isReady, "Expected app not to be ready, because deploy time is stale")
+}