chore: manually backport #5297: allow argocd-update step to return recommended requeue interval (#5301)

Signed-off-by: Kent Rancourt <[email protected]>

Author: krancour

pkg/cli/cmd/update/freight.go

@@ -73,7 +73,7 @@ func (o *updateFreightAliasOptions) addFlags(cmd *cobra.Command) {
 		cmd.Flags(), &o.Project, o.Config.Project,
 		"The project the freight belongs to. If not set, the default project will be used.",
 	)
-	option.Name(cmd.Flags(), &o.Name, "The name of the freight to to be updated.")
+	option.Name(cmd.Flags(), &o.Name, "The name of the freight to be updated.")
 	option.OldAlias(cmd.Flags(), &o.OldAlias, "The existing alias of the freight to be updated.")
 	option.NewAlias(cmd.Flags(), &o.NewAlias, "The new alias to be assigned to the freight.")
 

pkg/controller/promotions/promotions.go

@@ -80,7 +80,7 @@ type reconciler struct {
 		kargoapi.Promotion,
 		*kargoapi.Stage,
 		*kargoapi.Freight,
-	) (*kargoapi.PromotionStatus, error)
+	) (*kargoapi.PromotionStatus, *time.Duration, error)
 
 	terminatePromotionFn func(
 		context.Context,
@@ -328,6 +328,8 @@ func (r *reconciler) Reconcile(
 
 	newStatus := promo.Status.DeepCopy()
 
+	var suggestedRequeueInterval *time.Duration
+
 	// Wrap the promoteFn() call in an anonymous function to recover() any panics, so
 	// we can update the promo's phase with Error if it does. This breaks an infinite
 	// cycle of a bad promo continuously failing to reconcile, and surfaces the error.
@@ -343,7 +345,9 @@ func (r *reconciler) Reconcile(
 				newStatus.Message = fmt.Sprintf("%v", err)
 			}
 		}()
-		otherStatus, promoteErr := r.promoteFn(
+		var otherStatus *kargoapi.PromotionStatus
+		var promoteErr error
+		otherStatus, suggestedRequeueInterval, promoteErr = r.promoteFn(
 			promoCtx,
 			*promo,
 			stage,
@@ -457,7 +461,9 @@ func (r *reconciler) Reconcile(
 	// If the promotion is still running, we'll need to periodically check on
 	// it.
 	if newStatus.Phase == kargoapi.PromotionPhaseRunning {
-		return ctrl.Result{RequeueAfter: calculateRequeueInterval(promo)}, nil
+		return ctrl.Result{
+			RequeueAfter: calculateRequeueInterval(promo, suggestedRequeueInterval),
+		}, nil
 	}
 	return ctrl.Result{}, nil
 }
@@ -467,19 +473,22 @@ func (r *reconciler) promote(
 	promo kargoapi.Promotion,
 	stage *kargoapi.Stage,
 	targetFreight *kargoapi.Freight,
-) (*kargoapi.PromotionStatus, error) {
+) (*kargoapi.PromotionStatus, *time.Duration, error) {
 	logger := logging.LoggerFromContext(ctx)
 	stageName := stage.Name
 	stageNamespace := promo.Namespace
 
 	if targetFreight == nil {
 		// nolint:staticcheck
-		return nil, fmt.Errorf("Freight %q not found in namespace %q", promo.Spec.Freight, promo.Namespace)
+		return nil, nil, fmt.Errorf(
+			"Freight %q not found in namespace %q",
+			promo.Spec.Freight, promo.Namespace,
+		)
 	}
 
 	if !stage.IsFreightAvailable(targetFreight) {
 		// nolint:staticcheck
-		return nil, fmt.Errorf(
+		return nil, nil, fmt.Errorf(
 			"Freight %q is not available to Stage %q in namespace %q",
 			promo.Spec.Freight,
 			stageName,
@@ -549,7 +558,7 @@ func (r *reconciler) promote(
 		promoCtx.StepExecutionMetadata = nil
 		workingPromo.Status.HealthChecks = nil
 	} else if !os.IsExist(err) {
-		return nil, fmt.Errorf("error creating working directory: %w", err)
+		return nil, nil, fmt.Errorf("error creating working directory: %w", err)
 	}
 	defer func() {
 		if workingPromo.Status.Phase.IsTerminal() {
@@ -576,7 +585,7 @@ func (r *reconciler) promote(
 	}
 	if err != nil {
 		workingPromo.Status.Phase = kargoapi.PromotionPhaseErrored
-		return &workingPromo.Status, err
+		return &workingPromo.Status, nil, err
 	}
 
 	logger.Debug("promotion", "phase", workingPromo.Status.Phase)
@@ -606,7 +615,11 @@ func (r *reconciler) promote(
 		}
 	}
 
-	return &workingPromo.Status, nil
+	if workingPromo.Status.Phase == kargoapi.PromotionPhaseRunning {
+		return &workingPromo.Status, res.RetryAfter, nil
+	}
+
+	return &workingPromo.Status, nil, nil
 }
 
 // buildTargetFreightCollection constructs a FreightCollection that contains all
@@ -721,31 +734,38 @@ func parseCreateActorAnnotation(promo *kargoapi.Promotion) string {
 
 var defaultRequeueInterval = 5 * time.Minute
 
-func calculateRequeueInterval(p *kargoapi.Promotion) time.Duration {
+func calculateRequeueInterval(
+	p *kargoapi.Promotion,
+	suggestedRequeueInterval *time.Duration,
+) time.Duration {
+	requeueInterval := defaultRequeueInterval
+	if suggestedRequeueInterval != nil {
+		requeueInterval = *suggestedRequeueInterval
+	}
+
 	// Ensure we have a step for the current step index.
 	if int(p.Status.CurrentStep) >= len(p.Spec.Steps) {
-		return defaultRequeueInterval
+		return requeueInterval
 	}
 
 	step := p.Spec.Steps[p.Status.CurrentStep]
 	registration := promotion.GetStepRunnerRegistration(step.Uses)
 	timeout := step.Retry.GetTimeout(registration.Metadata.DefaultTimeout)
 
-	// If the timeout is 0 (no timeout), we should requeue at the default
-	// interval.
+	// If the timeout is 0 (no timeout), we should requeue at the full interval.
 	if timeout == 0 {
-		return defaultRequeueInterval
+		return requeueInterval
 	}
 
 	// Ensure we have an execution metadata entry for the current step.
 	if int(p.Status.CurrentStep) >= len(p.Status.StepExecutionMetadata) {
-		return defaultRequeueInterval
+		return requeueInterval
 	}
 
 	md := p.Status.StepExecutionMetadata[p.Status.CurrentStep]
 	targetTimeout := md.StartedAt.Add(timeout)
-	if targetTimeout.Before(time.Now().Add(defaultRequeueInterval)) {
+	if targetTimeout.Before(time.Now().Add(requeueInterval)) {
 		return time.Until(targetTimeout)
 	}
-	return defaultRequeueInterval
+	return requeueInterval
 }

pkg/controller/promotions/promotions_test.go

@@ -11,6 +11,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -63,8 +64,11 @@ func TestReconcile(t *testing.T) {
 	testCases := []struct {
 		name      string
 		promos    []client.Object
-		promoteFn func(context.Context, kargoapi.Promotion,
-			*kargoapi.Freight) (*kargoapi.PromotionStatus, error)
+		promoteFn func(
+			context.Context,
+			kargoapi.Promotion,
+			*kargoapi.Freight,
+		) (*kargoapi.PromotionStatus, *time.Duration, error)
 		terminateFn             func(context.Context, *kargoapi.Promotion) error
 		promoToReconcile        *types.NamespacedName // if nil, uses the first of the promos
 		expectPromoteFnCalled   bool
@@ -238,7 +242,11 @@ func TestReconcile(t *testing.T) {
 				newPromo("fake-namespace", "fake-promo", "fake-stage", kargoapi.PromotionPhasePending, before),
 			},
 			promoToReconcile: &types.NamespacedName{Namespace: "fake-namespace", Name: "fake-promo"},
-			promoteFn: func(_ context.Context, _ kargoapi.Promotion, _ *kargoapi.Freight) (*kargoapi.PromotionStatus, error) {
+			promoteFn: func(
+				context.Context,
+				kargoapi.Promotion,
+				*kargoapi.Freight,
+			) (*kargoapi.PromotionStatus, *time.Duration, error) {
 				panic("expected panic")
 			},
 		},
@@ -263,8 +271,12 @@ func TestReconcile(t *testing.T) {
 				newPromo("fake-namespace", "fake-promo", "fake-stage", kargoapi.PromotionPhasePending, before),
 			},
 			promoToReconcile: &types.NamespacedName{Namespace: "fake-namespace", Name: "fake-promo"},
-			promoteFn: func(_ context.Context, _ kargoapi.Promotion, _ *kargoapi.Freight) (*kargoapi.PromotionStatus, error) {
-				return nil, errors.New("expected error")
+			promoteFn: func(
+				context.Context,
+				kargoapi.Promotion,
+				*kargoapi.Freight,
+			) (*kargoapi.PromotionStatus, *time.Duration, error) {
+				return nil, nil, errors.New("expected error")
 			},
 		},
 		{
@@ -300,12 +312,14 @@ func TestReconcile(t *testing.T) {
 				p kargoapi.Promotion,
 				_ *kargoapi.Stage,
 				f *kargoapi.Freight,
-			) (*kargoapi.PromotionStatus, error) {
+			) (*kargoapi.PromotionStatus, *time.Duration, error) {
 				promoteWasCalled = true
 				if tc.promoteFn != nil {
 					return tc.promoteFn(ctx, p, f)
 				}
-				return &kargoapi.PromotionStatus{Phase: kargoapi.PromotionPhaseSucceeded}, nil
+				return &kargoapi.PromotionStatus{
+					Phase: kargoapi.PromotionPhaseSucceeded,
+				}, nil, nil
 			}
 
 			terminateWasCalled := false
@@ -620,9 +634,10 @@ func Test_calculateRequeueInterval(t *testing.T) {
 	require.Greater(t, testTimeout, defaultRequeueInterval)
 
 	testCases := []struct {
-		name       string
-		promo      *kargoapi.Promotion
-		assertions func(*testing.T, time.Duration)
+		name                     string
+		promo                    *kargoapi.Promotion
+		suggestedRequeueInterval *time.Duration
+		assertions               func(*testing.T, time.Duration)
 	}{
 		{
 			name: "current step out of bounds",
@@ -698,7 +713,62 @@ func Test_calculateRequeueInterval(t *testing.T) {
 			},
 		},
 		{
-			name: "timeout occurs after next interval",
+			name:                     "timeout would occur after suggested interval elapses",
+			suggestedRequeueInterval: ptr.To(time.Minute),
+			promo: &kargoapi.Promotion{
+				Spec: kargoapi.PromotionSpec{
+					Steps: []kargoapi.PromotionStep{{
+						Uses: testStepKindWithTimeout,
+					}},
+				},
+				Status: kargoapi.PromotionStatus{
+					CurrentStep: 0,
+					StepExecutionMetadata: []kargoapi.StepExecutionMetadata{{
+						// If the step started now and times out after an interval greater
+						// than the default requeue interval, then the wall clock time of
+						// the timeout will be AFTER the wall clock time of the next
+						// reconciliation.
+						StartedAt: &metav1.Time{Time: time.Now()},
+					}},
+				},
+			},
+			assertions: func(t *testing.T, requeueInterval time.Duration) {
+				// The request should be requeued according to the suggestion.
+				require.Equal(t, time.Minute, requeueInterval)
+				// Sanity check that the requeue interval is always greater than 0.
+				require.Greater(t, requeueInterval, time.Duration(0))
+			},
+		},
+		{
+			name:                     "timeout would occur before suggested interval elapses",
+			suggestedRequeueInterval: ptr.To(time.Minute),
+			promo: &kargoapi.Promotion{
+				Spec: kargoapi.PromotionSpec{
+					Steps: []kargoapi.PromotionStep{{
+						Uses: testStepKindWithTimeout,
+					}},
+				},
+				Status: kargoapi.PromotionStatus{
+					CurrentStep: 0,
+					StepExecutionMetadata: []kargoapi.StepExecutionMetadata{{
+						// If the step has only a minute to go before timeout, then the wall
+						// clock time of the timeout will be BEFORE the wall clock time of
+						// the next reconciliation.
+						StartedAt: &metav1.Time{
+							Time: metav1.Now().Add(-testTimeout).Add(time.Minute),
+						},
+					}},
+				},
+			},
+			assertions: func(t *testing.T, requeueInterval time.Duration) {
+				// The interval to the next reconciliation should be shortened.
+				require.Less(t, requeueInterval, time.Minute)
+				// Sanity check that the requeue interval is always greater than 0.
+				require.Greater(t, requeueInterval, time.Duration(0))
+			},
+		},
+		{
+			name: "timeout would occur after default interval elapses",
 			promo: &kargoapi.Promotion{
 				Spec: kargoapi.PromotionSpec{
 					Steps: []kargoapi.PromotionStep{{
@@ -724,7 +794,7 @@ func Test_calculateRequeueInterval(t *testing.T) {
 			},
 		},
 		{
-			name: "timeout occurs before next interval",
+			name: "timeout would occur before next default interval elapses",
 			promo: &kargoapi.Promotion{
 				Spec: kargoapi.PromotionSpec{
 					Steps: []kargoapi.PromotionStep{{
@@ -753,7 +823,13 @@ func Test_calculateRequeueInterval(t *testing.T) {
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			testCase.assertions(t, calculateRequeueInterval(testCase.promo))
+			testCase.assertions(
+				t,
+				calculateRequeueInterval(
+					testCase.promo,
+					testCase.suggestedRequeueInterval,
+				),
+			)
 		})
 	}
 }

pkg/promotion/promotion.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"slices"
 	"strings"
+	"time"
 
 	gocache "github.com/patrickmn/go-cache"
 	"github.com/stretchr/testify/assert/yaml"
@@ -401,6 +402,10 @@ type Result struct {
 	StepExecutionMetadata kargoapi.StepExecutionMetadataList
 	// State is the current state of the promotion process.
 	State State
+	// RetryAfter is an optional, SUGGESTED duration after which a Promotion
+	// reporting itself to be in a Running status should be retried. Note: This is
+	// unrelated to retrying upon non-terminal failures.
+	RetryAfter *time.Duration
 }
 
 // getTaskOutputs returns the outputs of a task that are relevant to the current
@@ -505,4 +510,8 @@ type StepResult struct {
 	// returned by some StepRunner upon successful execution of a Step. These
 	// criteria can be used later as input to a health.Checker.
 	HealthCheck *health.Criteria
+	// RetryAfter is an optional, SUGGESTED duration after which a step reporting
+	// itself to be in a Running status should be retried. Note: This is unrelated
+	// to retrying upon non-terminal failures.
+	RetryAfter *time.Duration
 }