Repository: akuity/kargo
Pull request #3740 (2025-03-28 12:09:59)

fix: always honor availability strategy
#3257 introduced availability strategies (verified + soaked in any upstream vs all upstreams).

The change was properly implemented as new criterion when _listing_ available Freight, but was overlooked in the function that answers whether a single piece of Freight is available to a given Stage.

The effect is that Freight that was not verified and adequately soaked in _all_ upstream stages may sometimes have been deemed available to a Stage if it met availability criteria in _any_ upstream stage.

This PR fixes that.

netlify[bot]: ### <span aria-hidden="true">✅</span> Deploy Preview for *docs-kargo-io* ready!


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 632c4884f222abce0d431c077790a7eda4d5ae32 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/docs-kargo-io/deploys/67e6919d7108ec000844b5be |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-3740.docs.kargo.io](https://deploy-preview-3740.docs.kargo.io) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTM3NDAuZG9jcy5rYXJnby5pbyJ9.eJ3kF2WAiJBpz8kUPMsQW_FYGMfgUsxODiopNoNcGvk)<br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [docs-kargo-io Preview](https://deploy-preview-3740.docs.kargo.io) -->
_To edit notification comments on pull requests, go to your [Netlify site configuration](https://app.netlify.com/sites/docs-kargo-io/configuration/notifications#deploy-webhooks)._
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/akuity/kargo/pull/3740?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 21.92%. Comparing base [(`f3329fc`)](https://app.codecov.io/gh/akuity/kargo/commit/f3329fccc567e89ba09a91e45ea10cfc5c61ef6c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) to head [(`632c488`)](https://app.codecov.io/gh/akuity/kargo/commit/632c4884f222abce0d431c077790a7eda4d5ae32?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).
> Report is 2 commits behind head on main.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #3740      +/-   ##
==========================================
+ Coverage   21.90%   21.92%   +0.01%     
==========================================
  Files         313      313              
  Lines       65708    65719      +11     
==========================================
+ Hits        14396    14407      +11     
  Misses      50536    50536              
  Partials      776      776              
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/akuity/kargo/pull/3740?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).

<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>

Patch:
diff --git a/api/v1alpha1/freight_types.go b/api/v1alpha1/freight_types.go
index 75e1af55..3a9cae68 100644
--- a/api/v1alpha1/freight_types.go
+++ b/api/v1alpha1/freight_types.go
@@ -157,6 +157,19 @@ func (f *Freight) GetLongestSoak(stage string) time.Duration {
 	return time.Duration(max(longestCompleted.Nanoseconds(), current.Nanoseconds()))
 }
 
+// HasSoakedIn returns whether the Freight has soaked in the specified Stage for
+// at least the specified duration. If the specified duration is nil, this
+// method will return true.
+func (f *Freight) HasSoakedIn(stage string, dur *metav1.Duration) bool {
+	if f == nil {
+		return false
+	}
+	if dur == nil {
+		return true
+	}
+	return f.GetLongestSoak(stage) >= dur.Duration
+}
+
 // AddCurrentStage updates the Freight status to reflect that the Freight is
 // currently in the specified Stage.
 func (f *FreightStatus) AddCurrentStage(stage string, since time.Time) {
diff --git a/api/v1alpha1/stage_types.go b/api/v1alpha1/stage_types.go
index 174c2b77..0f7045b2 100644
--- a/api/v1alpha1/stage_types.go
+++ b/api/v1alpha1/stage_types.go
@@ -181,16 +181,27 @@ func (s *Stage) IsFreightAvailable(freight *Freight) bool {
 		return true
 	}
 	for _, req := range s.Spec.RequestedFreight {
-		if freight.Origin.Equals(&req.Origin) {
-			if req.Sources.Direct {
-				return true
-			}
-			for _, source := range req.Sources.Stages {
-				if freight.IsVerifiedIn(source) {
-					return req.Sources.RequiredSoakTime == nil ||
-						freight.GetLongestSoak(source) >= req.Sources.RequiredSoakTime.Duration
+		if !freight.Origin.Equals(&req.Origin) {
+			continue
+		}
+
+		if req.Sources.Direct {
+			return true
+		}
+		if req.Sources.AvailabilityStrategy == FreightAvailabilityStrategyAll {
+			// Make sure Freight is verified and soaked in all upstream Stages
+			for _, upstream := range req.Sources.Stages {
+				if !freight.IsVerifiedIn(upstream) || !freight.HasSoakedIn(upstream, req.Sources.RequiredSoakTime) {
+					return false
 				}
 			}
+			return true
+		}
+		// Make sure Freight is verified and soaked in at least one upstream Stage
+		for _, upstream := range req.Sources.Stages {
+			if freight.IsVerifiedIn(upstream) && freight.HasSoakedIn(upstream, req.Sources.RequiredSoakTime) {
+				return true
+			}
 		}
 	}
 	return false
diff --git a/internal/api/stage.go b/internal/api/stage.go
index 6053e9a2..3cb1690b 100644
--- a/internal/api/stage.go
+++ b/internal/api/stage.go
@@ -81,9 +81,7 @@ func ListFreightAvailableToStage(
 				ApprovedFor:          s.Name,
 				VerifiedIn:           req.Sources.Stages,
 				AvailabilityStrategy: req.Sources.AvailabilityStrategy,
-			}
-			if requiredSoak := req.Sources.RequiredSoakTime; requiredSoak != nil {
-				listOpts.RequiredSoakTime = &requiredSoak.Duration
+				RequiredSoakTime:     req.Sources.RequiredSoakTime,
 			}
 		}
 		freightFromWarehouse, err := ListFreightFromWarehouse(
diff --git a/internal/api/warehouse.go b/internal/api/warehouse.go
index 89f6adb6..be11b795 100644
--- a/internal/api/warehouse.go
+++ b/internal/api/warehouse.go
@@ -87,7 +87,7 @@ type ListWarehouseFreightOptions struct {
 	// 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
+	RequiredSoakTime *metav1.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.
@@ -214,7 +214,7 @@ func ListFreightFromWarehouse(
 		// for the Freight.
 		verifiedStages := sets.New[string]()
 		for stage := range f.Status.VerifiedIn {
-			if f.GetLongestSoak(stage) >= *opts.RequiredSoakTime {
+			if f.HasSoakedIn(stage, opts.RequiredSoakTime) {
 				verifiedStages.Insert(stage)
 			}
 		}


Test patch:
diff --git a/api/v1alpha1/stage_types_test.go b/api/v1alpha1/stage_types_test.go
index 72c54ae3..7c8c9c17 100644
--- a/api/v1alpha1/stage_types_test.go
+++ b/api/v1alpha1/stage_types_test.go
@@ -185,6 +185,65 @@ func TestStage_IsFreightAvailable(t *testing.T) {
 			},
 			expected: true,
 		},
+		{
+			name: "availability strategy all + freight is verified and soaked in one upstream stage, but not all",
+			stage: &Stage{
+				ObjectMeta: testStageMeta,
+				Spec: StageSpec{
+					RequestedFreight: []FreightRequest{{
+						Origin: testOrigin,
+						Sources: FreightSources{
+							AvailabilityStrategy: FreightAvailabilityStrategyAll,
+							Stages:               []string{"upstream-stage-1", "upstream-stage-2"},
+							RequiredSoakTime:     &metav1.Duration{Duration: time.Hour},
+						},
+					}},
+				},
+			},
+			freight: &Freight{
+				ObjectMeta: testFreightMeta,
+				Origin:     testOrigin,
+				Status: FreightStatus{
+					VerifiedIn: map[string]VerifiedStage{
+						"upstream-stage-1": {
+							LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
+						},
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "availability strategy all + freight is verified and soaked in all upstream stages",
+			stage: &Stage{
+				ObjectMeta: testStageMeta,
+				Spec: StageSpec{
+					RequestedFreight: []FreightRequest{{
+						Origin: testOrigin,
+						Sources: FreightSources{
+							AvailabilityStrategy: FreightAvailabilityStrategyAll,
+							Stages:               []string{"upstream-stage-1", "upstream-stage-2"},
+							RequiredSoakTime:     &metav1.Duration{Duration: time.Hour},
+						},
+					}},
+				},
+			},
+			freight: &Freight{
+				ObjectMeta: testFreightMeta,
+				Origin:     testOrigin,
+				Status: FreightStatus{
+					VerifiedIn: map[string]VerifiedStage{
+						"upstream-stage-1": {
+							LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
+						},
+						"upstream-stage-2": {
+							LongestCompletedSoak: &metav1.Duration{Duration: 2 * time.Hour},
+						},
+					},
+				},
+			},
+			expected: true,
+		},
 		{
 			name: "freight from origin not requested",
 			stage: &Stage{
diff --git a/internal/api/warehouse_test.go b/internal/api/warehouse_test.go
index b8d3b6dd..b634a35b 100644
--- a/internal/api/warehouse_test.go
+++ b/internal/api/warehouse_test.go
@@ -10,7 +10,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
@@ -178,7 +177,8 @@ func TestListFreightFromWarehouse(t *testing.T) {
 			opts: &ListWarehouseFreightOptions{
 				ApprovedFor:      testStage,
 				VerifiedIn:       []string{testUpstreamStage},
-				RequiredSoakTime: ptr.To(time.Hour)},
+				RequiredSoakTime: &metav1.Duration{Duration: time.Hour},
+			},
 			objects: []client.Object{
 				&kargoapi.Freight{ // This should not be returned
 					ObjectMeta: metav1.ObjectMeta{
@@ -279,7 +279,7 @@ func TestListFreightFromWarehouse(t *testing.T) {
 				AvailabilityStrategy: kargoapi.FreightAvailabilityStrategyAll,
 				ApprovedFor:          testStage,
 				VerifiedIn:           []string{testUpstreamStage, testUpstreamStage2},
-				RequiredSoakTime:     ptr.To(time.Hour),
+				RequiredSoakTime:     &metav1.Duration{Duration: time.Hour},
 			},
 			objects: []client.Object{
 				&kargoapi.Freight{ // This should be returned as its approved for the Stage
@@ -379,7 +379,7 @@ func TestListFreightFromWarehouse(t *testing.T) {
 				AvailabilityStrategy: "Invalid",
 				ApprovedFor:          testStage,
 				VerifiedIn:           []string{testUpstreamStage, testUpstreamStage2},
-				RequiredSoakTime:     ptr.To(time.Hour),
+				RequiredSoakTime:     &metav1.Duration{Duration: time.Hour},
 			},
 			assertions: func(t *testing.T, _ []kargoapi.Freight, err error) {
 				require.ErrorContains(t, err, "unsupported AvailabilityStrategy")