WIP

britaniar · Jul 18, 2024 · 31a2288 · 31a2288
1 parent dc3b75a
commit 31a2288
Show file tree

Hide file tree

Showing 2 changed files with 192 additions and 1 deletion.
diff --git a/pkg/controllers/rollout/controller.go b/pkg/controllers/rollout/controller.go
@@ -9,6 +9,7 @@ package rollout
 import (
 	"context"
 	"fmt"
+	"math"
 	"strconv"
 	"time"
 
@@ -161,10 +162,29 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim
 	// to avoid the case that the rollout process stalling because the time based binding readiness does not trigger any event.
 	// We wait for 1/5 of the UnavailablePeriodSeconds so we can catch the next ready one early.
 	// TODO: only wait the time we need to wait for the first applied but not ready binding to be ready
-	return runtime.Result{RequeueAfter: time.Duration(*crp.Spec.Strategy.RollingUpdate.UnavailablePeriodSeconds) * time.Second / 5},
+	return runtime.Result{RequeueAfter: waitForFirstAppliedButNotReadyBinding(allBindings, time.Now().Add(-time.Duration(*crp.Spec.Strategy.RollingUpdate.UnavailablePeriodSeconds)*time.Second))},
 		r.updateBindings(ctx, toBeUpdatedBindings)
 }
 
+// `waitForFirstAppliedButNotReadyBinding` returns the minimum amount of time that needs to elapse before the first
+// binding that has been applied but not yet marked as ready becomes ready.
+func waitForFirstAppliedButNotReadyBinding(bindings []*fleetv1beta1.ClusterResourceBinding, readyTimeCutOff time.Time) time.Duration {
+	minValue := time.Duration(math.MaxInt64)
+	for _, binding := range bindings {
+		appliedCondition := binding.GetCondition(string(fleetv1beta1.ResourceBindingApplied))
+		if condition.IsConditionStatusTrue(appliedCondition, binding.Generation) {
+			waitTime, bindingReady := isBindingReady(binding, readyTimeCutOff)
+			if !bindingReady {
+				if waitTime < minValue {
+					minValue = waitTime
+				}
+			}
+		}
+	}
+	klog.V(2).InfoS("Picked the first applied but ready binding", "waitTime", minValue)
+	return minValue * time.Second
+}
+
 func (r *Reconciler) checkAndUpdateStaleBindingsStatus(ctx context.Context, bindings []*fleetv1beta1.ClusterResourceBinding) error {
 	if len(bindings) == 0 {
 		return nil

diff --git a/pkg/controllers/rollout/controller_test.go b/pkg/controllers/rollout/controller_test.go
@@ -8,6 +8,7 @@ package rollout
 import (
 	"context"
 	"errors"
+	"math"
 	"testing"
 	"time"
 
@@ -726,6 +727,176 @@ func TestIsBindingReady(t *testing.T) {
 	}
 }
 
+func TestWaitForFirstAppliedButNotReadyBinding(t *testing.T) {
+	tests := map[string]struct {
+		bindings         []*fleetv1beta1.ClusterResourceBinding
+		readyTimeCutOff  time.Time
+		wantWaitDuration time.Duration
+	}{
+		"all bindings ready": {
+			bindings: []*fleetv1beta1.ClusterResourceBinding{
+				{
+					Status: fleetv1beta1.ResourceBindingStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:               string(fleetv1beta1.ResourceBindingApplied),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+							{
+								Type:               string(fleetv1beta1.ResourceBindingAvailable),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+						},
+					},
+				},
+				{
+					Status: fleetv1beta1.ResourceBindingStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:               string(fleetv1beta1.ResourceBindingApplied),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+							{
+								Type:               string(fleetv1beta1.ResourceBindingAvailable),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+						},
+					},
+				},
+			},
+			readyTimeCutOff:  time.Now(),
+			wantWaitDuration: time.Duration(math.MaxInt64),
+		},
+		"one binding not ready": {
+			bindings: []*fleetv1beta1.ClusterResourceBinding{
+				{
+					Status: fleetv1beta1.ResourceBindingStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:               string(fleetv1beta1.ResourceBindingApplied),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+							{
+								Type:               string(fleetv1beta1.ResourceBindingAvailable),
+								Status:             metav1.ConditionFalse,
+								ObservedGeneration: 1,
+							},
+						},
+					},
+				},
+				{
+					Status: fleetv1beta1.ResourceBindingStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:               string(fleetv1beta1.ResourceBindingApplied),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+							{
+								Type:               string(fleetv1beta1.ResourceBindingAvailable),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+						},
+					},
+				},
+			},
+			readyTimeCutOff:  time.Now(),
+			wantWaitDuration: 0,
+		},
+		"multiple bindings not ready": {
+			bindings: []*fleetv1beta1.ClusterResourceBinding{
+				{
+					Status: fleetv1beta1.ResourceBindingStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:               string(fleetv1beta1.ResourceBindingApplied),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+							{
+								Type:               string(fleetv1beta1.ResourceBindingAvailable),
+								Status:             metav1.ConditionFalse,
+								ObservedGeneration: 1,
+							},
+						},
+					},
+				},
+				{
+					Status: fleetv1beta1.ResourceBindingStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:               string(fleetv1beta1.ResourceBindingApplied),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+							{
+								Type:               string(fleetv1beta1.ResourceBindingAvailable),
+								Status:             metav1.ConditionFalse,
+								ObservedGeneration: 1,
+							},
+						},
+					},
+				},
+			},
+			readyTimeCutOff:  time.Now(),
+			wantWaitDuration: time.Duration(math.MaxInt64),
+		},
+		"one binding not applied": {
+			bindings: []*fleetv1beta1.ClusterResourceBinding{
+				{
+					Status: fleetv1beta1.ResourceBindingStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:               string(fleetv1beta1.ResourceBindingApplied),
+								Status:             metav1.ConditionFalse,
+								ObservedGeneration: 1,
+							},
+							{
+								Type:               string(fleetv1beta1.ResourceBindingAvailable),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+						},
+					},
+				},
+				{
+					Status: fleetv1beta1.ResourceBindingStatus{
+						Conditions: []metav1.Condition{
+							{
+								Type:               string(fleetv1beta1.ResourceBindingApplied),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+							{
+								Type:               string(fleetv1beta1.ResourceBindingAvailable),
+								Status:             metav1.ConditionTrue,
+								ObservedGeneration: 1,
+							},
+						},
+					},
+				},
+			},
+			readyTimeCutOff:  time.Now(),
+			wantWaitDuration: time.Duration(math.MaxInt64),
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			gotWaitDuration := waitForFirstAppliedButNotReadyBinding(tt.bindings, tt.readyTimeCutOff)
+			if gotWaitDuration != tt.wantWaitDuration {
+				t.Errorf("waitForFirstAppliedButNotReadyBinding test `%s` gotWaitDuration = %v, wantWaitDuration %v", name, gotWaitDuration, tt.wantWaitDuration)
+			}
+		})
+	}
+}
+
 func TestPickBindingsToRoll(t *testing.T) {
 	noMaxUnavailableCRP := clusterResourcePlacementForTest("test",
 		createPlacementPolicyForTest(fleetv1beta1.PickNPlacementType, 5))