chore(backport release-1.3): fix: soak time bug (#3739)

Co-authored-by: Kent Rancourt <[email protected]>

Author: hiddeco

api/v1alpha1/stage_helpers.go

@@ -141,7 +141,7 @@ func (s *Stage) ListAvailableFreight(
 				AvailabilityStrategy: req.Sources.AvailabilityStrategy,
 			}
 			if requiredSoak := req.Sources.RequiredSoakTime; requiredSoak != nil {
-				listOpts.VerifiedBefore = &metav1.Time{Time: time.Now().Add(-requiredSoak.Duration)}
+				listOpts.RequiredSoakTime = &requiredSoak.Duration
 			}
 		}
 		freightFromWarehouse, err := warehouse.ListFreight(ctx, c, listOpts)

api/v1alpha1/stage_helpers_test.go

@@ -335,7 +335,7 @@ func TestStage_ListAvailableFreight(t *testing.T) {
 					Status: FreightStatus{
 						VerifiedIn: map[string]VerifiedStage{
 							testStage: {
-								VerifiedAt: &metav1.Time{Time: time.Now()},
+								LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
 							},
 						},
 					},
@@ -349,7 +349,7 @@ func TestStage_ListAvailableFreight(t *testing.T) {
 					Status: FreightStatus{
 						VerifiedIn: map[string]VerifiedStage{
 							testStage: {
-								VerifiedAt: &metav1.Time{Time: time.Now().Add(-time.Hour * 2)},
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 						},
 					},

api/v1alpha1/warehouse_helpers.go

@@ -82,11 +82,10 @@ type ListWarehouseFreightOptions struct {
 	//
 	// IMPORTANT: This is OR'ed with the ApprovedFor field.
 	VerifiedIn []string
-	// VerifiedBefore optionally specifies a time before which a Freight verified
-	// in any of the Stages named in the VerifiedIn field must have been verified.
-	// This is useful for filtering out Freight whose soak time has not yet
-	// elapsed.
-	VerifiedBefore *metav1.Time
+	// RequiredSoakTime optionally specifies a minimum duration that a piece of
+	// Freight must have continuously remained in a Stage at any time after being
+	// verified.
+	RequiredSoakTime *time.Duration
 	// AvailabilityStrategy specifies the semantics for how Freight is determined
 	// to be available. If not set, the default is to consider Freight available
 	// if it has been verified in any of the provided VerifiedIn stages.
@@ -193,7 +192,7 @@ func (w *Warehouse) ListFreight(
 		return lhs.Name == rhs.Name
 	})
 
-	if len(opts.VerifiedIn) == 0 || opts.VerifiedBefore == nil {
+	if len(opts.VerifiedIn) == 0 || opts.RequiredSoakTime == nil {
 		// Nothing left to do
 		return freight, nil
 	}
@@ -211,11 +210,9 @@ func (w *Warehouse) ListFreight(
 		// Track set of Stages that have passed the verification soak time
 		// for the Freight.
 		verifiedStages := sets.New[string]()
-		for stage, ver := range f.Status.VerifiedIn {
-			if verifiedAt := ver.VerifiedAt; verifiedAt != nil {
-				if verifiedAt.Time.Before(opts.VerifiedBefore.Time) {
-					verifiedStages.Insert(stage)
-				}
+		for stage := range f.Status.VerifiedIn {
+			if f.GetLongestSoak(stage) >= *opts.RequiredSoakTime {
+				verifiedStages.Insert(stage)
 			}
 		}
 

api/v1alpha1/warehouse_helpers_test.go

@@ -172,12 +172,11 @@ func TestWarehouse_ListFreight(t *testing.T) {
 			},
 		},
 		{
-			name: "success with VerifiedIn and VerifiedBefore options",
+			name: "success with VerifiedIn and RequiredSoakTime options",
 			opts: &ListWarehouseFreightOptions{
-				ApprovedFor:    testStage,
-				VerifiedIn:     []string{testUpstreamStage},
-				VerifiedBefore: &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
-			},
+				ApprovedFor:      testStage,
+				VerifiedIn:       []string{testUpstreamStage},
+				RequiredSoakTime: ptr.To(time.Hour)},
 			objects: []client.Object{
 				&Freight{ // This should not be returned
 					ObjectMeta: metav1.ObjectMeta{
@@ -194,7 +193,9 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						ApprovedFor: map[string]ApprovedStage{testStage: {}},
 						// Doesn't matter that it's verified upstream, because this is the
 						// wrong warehouse
-						VerifiedIn: map[string]VerifiedStage{testUpstreamStage: {}},
+						VerifiedIn: map[string]VerifiedStage{testUpstreamStage: {
+							LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
+						}},
 					},
 				},
 				&Freight{ // This should not be returned
@@ -236,7 +237,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// yet elapsed
 						VerifiedIn: map[string]VerifiedStage{
 							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.Now()),
+								LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
 							},
 						},
 					},
@@ -255,7 +256,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// elapsed
 						VerifiedIn: map[string]VerifiedStage{
 							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 						},
 					},
@@ -276,7 +277,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 				AvailabilityStrategy: FreightAvailabilityStrategyAll,
 				ApprovedFor:          testStage,
 				VerifiedIn:           []string{testUpstreamStage, testUpstreamStage2},
-				VerifiedBefore:       &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
+				RequiredSoakTime:     ptr.To(time.Hour),
 			},
 			objects: []client.Object{
 				&Freight{ // This should be returned as its approved for the Stage
@@ -292,11 +293,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// This is approved for the Stage
 						ApprovedFor: map[string]ApprovedStage{testStage: {}},
 						// This is only verified in a single upstream Stage
-						VerifiedIn: map[string]VerifiedStage{
-							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.Now()),
-							},
-						},
+						VerifiedIn: map[string]VerifiedStage{},
 					},
 				},
 				&Freight{ // This should be returned because its verified in both upstream Stages and soak time has lapsed
@@ -314,10 +311,10 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// This is verified in all of the upstream Stages and the soak time has lapsed
 						VerifiedIn: map[string]VerifiedStage{
 							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 							testUpstreamStage2: {
-								VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 						},
 					},
@@ -336,9 +333,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						ApprovedFor: map[string]ApprovedStage{},
 						// This is not verified in all of the upstream Stages
 						VerifiedIn: map[string]VerifiedStage{
-							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.Now()),
-							},
+							testUpstreamStage: {},
 						},
 					},
 				},
@@ -357,10 +352,10 @@ func TestWarehouse_ListFreight(t *testing.T) {
 						// This is verified in all of the upstream Stages but only passed the soak time of one
 						VerifiedIn: map[string]VerifiedStage{
 							testUpstreamStage: {
-								VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 							testUpstreamStage2: {
-								VerifiedAt: ptr.To(metav1.Now()),
+								LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
 							},
 						},
 					},
@@ -381,7 +376,7 @@ func TestWarehouse_ListFreight(t *testing.T) {
 				AvailabilityStrategy: "Invalid",
 				ApprovedFor:          testStage,
 				VerifiedIn:           []string{testUpstreamStage, testUpstreamStage2},
-				VerifiedBefore:       &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
+				RequiredSoakTime:     ptr.To(time.Hour),
 			},
 			assertions: func(t *testing.T, _ []Freight, err error) {
 				require.ErrorContains(t, err, "unsupported AvailabilityStrategy")

internal/controller/stages/regular_stages_test.go

@@ -4809,7 +4809,7 @@ func TestRegularStageReconciler_autoPromoteFreight(t *testing.T) {
 			},
 		},
 		{
-			name: "handles verified freight from upstream stages with verification duration requirement",
+			name: "handles verified freight from upstream stages with soak time requirement",
 			stage: &kargoapi.Stage{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "fake-project",
@@ -4889,9 +4889,8 @@ func TestRegularStageReconciler_autoPromoteFreight(t *testing.T) {
 					Status: kargoapi.FreightStatus{
 						VerifiedIn: map[string]kargoapi.VerifiedStage{
 							"upstream-stage": {
-								// Should be selected because it was verified
-								// after the required duration.
-								VerifiedAt: &metav1.Time{Time: now.Add(-2 * time.Hour)},
+								// Should be selected because the soak time has elapsed
+								LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
 							},
 						},
 					},