Skip to content

Commit 064a273

Browse files
krancourkrancour
authored
fix: soak time bug (akuity#3731)
Signed-off-by: Kent Rancourt <[email protected]>
1 parent 4d836c4 commit 064a273

File tree

5 files changed

+31
-41
lines changed

5 files changed

+31
-41
lines changed

Diff for: ‎internal/api/stage.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func ListFreightAvailableToStage(
8383
AvailabilityStrategy: req.Sources.AvailabilityStrategy,
8484
}
8585
if requiredSoak := req.Sources.RequiredSoakTime; requiredSoak != nil {
86-
listOpts.VerifiedBefore = &metav1.Time{Time: time.Now().Add(-requiredSoak.Duration)}
86+
listOpts.RequiredSoakTime = &requiredSoak.Duration
8787
}
8888
}
8989
freightFromWarehouse, err := ListFreightFromWarehouse(

Diff for: ‎internal/api/stage_test.go

+2-3
Original file line numberDiff line numberDiff line change
@@ -208,8 +208,7 @@ func TestListFreightAvailableToStage(t *testing.T) {
208208
Status: kargoapi.FreightStatus{
209209
VerifiedIn: map[string]kargoapi.VerifiedStage{
210210
testStage: {
211-
VerifiedAt: &metav1.Time{Time: time.Now()},
212-
},
211+
LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute}},
213212
},
214213
},
215214
},
@@ -222,7 +221,7 @@ func TestListFreightAvailableToStage(t *testing.T) {
222221
Status: kargoapi.FreightStatus{
223222
VerifiedIn: map[string]kargoapi.VerifiedStage{
224223
testStage: {
225-
VerifiedAt: &metav1.Time{Time: time.Now().Add(-time.Hour * 2)},
224+
LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
226225
},
227226
},
228227
},

Diff for: ‎internal/api/warehouse.go

+8-11
Original file line numberDiff line numberDiff line change
@@ -84,11 +84,10 @@ type ListWarehouseFreightOptions struct {
8484
//
8585
// IMPORTANT: This is OR'ed with the ApprovedFor field.
8686
VerifiedIn []string
87-
// VerifiedBefore optionally specifies a time before which a Freight verified
88-
// in any of the Stages named in the VerifiedIn field must have been verified.
89-
// This is useful for filtering out Freight whose soak time has not yet
90-
// elapsed.
91-
VerifiedBefore *metav1.Time
87+
// RequiredSoakTime optionally specifies a minimum duration that a piece of
88+
// Freight must have continuously remained in a Stage at any time after being
89+
// verified.
90+
RequiredSoakTime *time.Duration
9291
// AvailabilityStrategy specifies the semantics for how Freight is determined
9392
// to be available. If not set, the default is to consider Freight available
9493
// if it has been verified in any of the provided VerifiedIn stages.
@@ -196,7 +195,7 @@ func ListFreightFromWarehouse(
196195
return lhs.Name == rhs.Name
197196
})
198197

199-
if len(opts.VerifiedIn) == 0 || opts.VerifiedBefore == nil {
198+
if len(opts.VerifiedIn) == 0 || opts.RequiredSoakTime == nil {
200199
// Nothing left to do
201200
return freight, nil
202201
}
@@ -214,11 +213,9 @@ func ListFreightFromWarehouse(
214213
// Track set of Stages that have passed the verification soak time
215214
// for the Freight.
216215
verifiedStages := sets.New[string]()
217-
for stage, ver := range f.Status.VerifiedIn {
218-
if verifiedAt := ver.VerifiedAt; verifiedAt != nil {
219-
if verifiedAt.Time.Before(opts.VerifiedBefore.Time) {
220-
verifiedStages.Insert(stage)
221-
}
216+
for stage := range f.Status.VerifiedIn {
217+
if f.GetLongestSoak(stage) >= *opts.RequiredSoakTime {
218+
verifiedStages.Insert(stage)
222219
}
223220
}
224221

Diff for: ‎internal/api/warehouse_test.go

+17-22
Original file line numberDiff line numberDiff line change
@@ -174,12 +174,11 @@ func TestListFreightFromWarehouse(t *testing.T) {
174174
},
175175
},
176176
{
177-
name: "success with VerifiedIn and VerifiedBefore options",
177+
name: "success with VerifiedIn and RequiredSoakTime options",
178178
opts: &ListWarehouseFreightOptions{
179-
ApprovedFor: testStage,
180-
VerifiedIn: []string{testUpstreamStage},
181-
VerifiedBefore: &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
182-
},
179+
ApprovedFor: testStage,
180+
VerifiedIn: []string{testUpstreamStage},
181+
RequiredSoakTime: ptr.To(time.Hour)},
183182
objects: []client.Object{
184183
&kargoapi.Freight{ // This should not be returned
185184
ObjectMeta: metav1.ObjectMeta{
@@ -196,7 +195,9 @@ func TestListFreightFromWarehouse(t *testing.T) {
196195
ApprovedFor: map[string]kargoapi.ApprovedStage{testStage: {}},
197196
// Doesn't matter that it's verified upstream, because this is the
198197
// wrong warehouse
199-
VerifiedIn: map[string]kargoapi.VerifiedStage{testUpstreamStage: {}},
198+
VerifiedIn: map[string]kargoapi.VerifiedStage{testUpstreamStage: {
199+
LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
200+
}},
200201
},
201202
},
202203
&kargoapi.Freight{ // This should not be returned
@@ -238,7 +239,7 @@ func TestListFreightFromWarehouse(t *testing.T) {
238239
// yet elapsed
239240
VerifiedIn: map[string]kargoapi.VerifiedStage{
240241
testUpstreamStage: {
241-
VerifiedAt: ptr.To(metav1.Now()),
242+
LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
242243
},
243244
},
244245
},
@@ -257,7 +258,7 @@ func TestListFreightFromWarehouse(t *testing.T) {
257258
// elapsed
258259
VerifiedIn: map[string]kargoapi.VerifiedStage{
259260
testUpstreamStage: {
260-
VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
261+
LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
261262
},
262263
},
263264
},
@@ -278,7 +279,7 @@ func TestListFreightFromWarehouse(t *testing.T) {
278279
AvailabilityStrategy: kargoapi.FreightAvailabilityStrategyAll,
279280
ApprovedFor: testStage,
280281
VerifiedIn: []string{testUpstreamStage, testUpstreamStage2},
281-
VerifiedBefore: &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
282+
RequiredSoakTime: ptr.To(time.Hour),
282283
},
283284
objects: []client.Object{
284285
&kargoapi.Freight{ // This should be returned as its approved for the Stage
@@ -294,11 +295,7 @@ func TestListFreightFromWarehouse(t *testing.T) {
294295
// This is approved for the Stage
295296
ApprovedFor: map[string]kargoapi.ApprovedStage{testStage: {}},
296297
// This is only verified in a single upstream Stage
297-
VerifiedIn: map[string]kargoapi.VerifiedStage{
298-
testUpstreamStage: {
299-
VerifiedAt: ptr.To(metav1.Now()),
300-
},
301-
},
298+
VerifiedIn: map[string]kargoapi.VerifiedStage{},
302299
},
303300
},
304301
&kargoapi.Freight{
@@ -317,10 +314,10 @@ func TestListFreightFromWarehouse(t *testing.T) {
317314
// This is verified in all of the upstream Stages and the soak time has lapsed
318315
VerifiedIn: map[string]kargoapi.VerifiedStage{
319316
testUpstreamStage: {
320-
VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
317+
LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
321318
},
322319
testUpstreamStage2: {
323-
VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
320+
LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
324321
},
325322
},
326323
},
@@ -339,9 +336,7 @@ func TestListFreightFromWarehouse(t *testing.T) {
339336
ApprovedFor: map[string]kargoapi.ApprovedStage{},
340337
// This is not verified in all of the upstream Stages
341338
VerifiedIn: map[string]kargoapi.VerifiedStage{
342-
testUpstreamStage: {
343-
VerifiedAt: ptr.To(metav1.Now()),
344-
},
339+
testUpstreamStage: {},
345340
},
346341
},
347342
},
@@ -360,10 +355,10 @@ func TestListFreightFromWarehouse(t *testing.T) {
360355
// This is verified in all of the upstream Stages but only passed the soak time of one
361356
VerifiedIn: map[string]kargoapi.VerifiedStage{
362357
testUpstreamStage: {
363-
VerifiedAt: ptr.To(metav1.NewTime(time.Now().Add(-2 * time.Hour))),
358+
LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
364359
},
365360
testUpstreamStage2: {
366-
VerifiedAt: ptr.To(metav1.Now()),
361+
LongestCompletedSoak: &metav1.Duration{Duration: 30 * time.Minute},
367362
},
368363
},
369364
},
@@ -384,7 +379,7 @@ func TestListFreightFromWarehouse(t *testing.T) {
384379
AvailabilityStrategy: "Invalid",
385380
ApprovedFor: testStage,
386381
VerifiedIn: []string{testUpstreamStage, testUpstreamStage2},
387-
VerifiedBefore: &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
382+
RequiredSoakTime: ptr.To(time.Hour),
388383
},
389384
assertions: func(t *testing.T, _ []kargoapi.Freight, err error) {
390385
require.ErrorContains(t, err, "unsupported AvailabilityStrategy")

Diff for: ‎internal/controller/stages/regular_stages_test.go

+3-4
Original file line numberDiff line numberDiff line change
@@ -4794,7 +4794,7 @@ func TestRegularStageReconciler_autoPromoteFreight(t *testing.T) {
47944794
},
47954795
},
47964796
{
4797-
name: "handles verified freight from upstream stages with verification duration requirement",
4797+
name: "handles verified freight from upstream stages with soak time requirement",
47984798
stage: &kargoapi.Stage{
47994799
ObjectMeta: metav1.ObjectMeta{
48004800
Namespace: "fake-project",
@@ -4874,9 +4874,8 @@ func TestRegularStageReconciler_autoPromoteFreight(t *testing.T) {
48744874
Status: kargoapi.FreightStatus{
48754875
VerifiedIn: map[string]kargoapi.VerifiedStage{
48764876
"upstream-stage": {
4877-
// Should be selected because it was verified
4878-
// after the required duration.
4879-
VerifiedAt: &metav1.Time{Time: now.Add(-2 * time.Hour)},
4877+
// Should be selected because the soak time has elapsed
4878+
LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
48804879
},
48814880
},
48824881
},

0 commit comments

Comments
 (0)