Skip to content

Fix: Freight should manage its own discoveryTimestamp, not rely on cr… #6133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
2003Aditya wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: Freight should manage its own discoveryTimestamp, not rely on cr… #6133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/v1alpha1/freight_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ type Freight struct {
// present, the defaulting webhook will choose an available alias and assign
// it to both the field and label.
Alias string `json:"alias,omitempty" protobuf:"bytes,7,opt,name=alias"`
// DiscoveryTimestamp is the time at which the Freight was first discovered
// from its source. This is distinct from metadata.creationTimestamp, which is
// set by Kubernetes and cannot be controlled by users.
DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty" protobuf:"bytes,11,opt,name=discoveryTimestamp"`
Copy link
Copy Markdown
Member

@krancour krancour Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears codegen wasn't run after this change. Among many other things, codegen will also set the protobuf struct tag for you (making it harder to get wrong).

I would suggest this change:

Suggested change
DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty" protobuf:"bytes,11,opt,name=discoveryTimestamp"`
DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty"`

When you run codegen (make hack-codegen), which you will need to do for reasons other than just this, it will probably put it back to exactly as you had it here, but it may do something slightly different. Better safe than sorry.

// Origin describes a kind of Freight in terms of its origin.
//
// +kubebuilder:validation:Required
Expand Down
4 changes: 4 additions & 0 deletions api/v1alpha1/promotion_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ func (s PromotionStepStatus) Compare(other PromotionStepStatus) int {
type Promotion struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
// DiscoveryTimestamp is the time at which the Promotion was created.
// This is distinct from metadata.creationTimestamp, which is
// set by Kubernetes and cannot be controlled by users.
DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty" protobuf:"bytes,4,opt,name=discoveryTimestamp"`
Copy link
Copy Markdown
Member

@krancour krancour Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as here: https://github.com/akuity/kargo/pull/6133/changes#r3118215938

Copy link
Copy Markdown
Member

@jessesuen jessesuen Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this startedAt? That's the field name we use in stepExecutionMetadata, and "discovery" is more of a freight thing.

apiVersion: kargo.akuity.io/v1alpha1
kind: Promotion
metadata:
  creationTimestamp: 2026-04-08T15:50:21Z
  name: dev.01knpwk5z8ngczawgqctc79w39.efe1b47
status:
  startedAt: 2026-04-08T15:40:22Z   # <<<<< my preference
  finishedAt: 2026-04-08T15:51:10Z
  phase: Succeeded
  stepExecutionMetadata:
    - alias: task-1::step-1
      finishedAt: 2026-04-08T15:50:24Z
      startedAt: 2026-04-08T15:50:22Z
      status: Succeeded

Copy link
Copy Markdown
Member

@jessesuen jessesuen Apr 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, unlike Freight, for the promotion object, this needs to be under status. Not a top-level field.

// Spec describes the desired transition of a specific Stage into a specific
// Freight.
//
Expand Down
22 changes: 16 additions & 6 deletions pkg/controller/stages/regular_stages.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1721,10 +1721,15 @@ func (r *RegularStageReconciler) autoPromoteFreight(
)
}

// Find the latest Freight by sorting the available Freight by creation time
// Find the latest Freight by sorting the available Freight by discovery time
// in descending order.
slices.SortFunc(freight, func(lhs, rhs kargoapi.Freight) int {
return rhs.CreationTimestamp.Compare(lhs.CreationTimestamp.Time)
lhsTime := lhs.DiscoveryTimestamp
rhsTime := rhs.DiscoveryTimestamp
if lhsTime == nil || rhsTime == nil {
return rhs.CreationTimestamp.Compare(lhs.CreationTimestamp.Time)
}
return rhsTime.Time.Compare(lhsTime.Time)
Comment on lines +1727 to +1732
Copy link
Copy Markdown
Member

@krancour krancour Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was really smart to fall back on CreationTimestamp since existing Freight will obviously not already have a DiscoveryTimestamp.

I might suggest a small improvement though. Instead of falling back on CreationTimestamp for both when either doesn't have a DiscoveryTimestamp, I would let each of them fall back individually like this:

Suggested change
lhsTime := lhs.DiscoveryTimestamp
rhsTime := rhs.DiscoveryTimestamp
if lhsTime == nil || rhsTime == nil {
return rhs.CreationTimestamp.Compare(lhs.CreationTimestamp.Time)
}
return rhsTime.Time.Compare(lhsTime.Time)
lhsTime := lhs.DiscoveryTimestamp
if lhsTime == nil {
lhsTime = lhs.CreationTimestamp.Time
}
rhsTime := rhs.DiscoveryTimestamp
if rhsTime == nil {
rhsTime = rhs.CreationTimestamp.Time
}
return rhsTime.Time.Compare(lhsTime.Time)

})
latestFreight := freight[0]

Expand Down Expand Up @@ -1811,11 +1816,16 @@ func (r *RegularStageReconciler) autoPromoteFreight(
latestFreight.Name, stage.Namespace, err,
)
}
if len(promotions.Items) > 0 {
// Sort the terminal Promotions by creation time in descending order
slices.SortFunc(promotions.Items, func(lhs, rhs kargoapi.Promotion) int {
if len(promotions.Items) > 0 {
// Sort the terminal Promotions by discovery time in descending order
slices.SortFunc(promotions.Items, func(lhs, rhs kargoapi.Promotion) int {
lhsTime := lhs.DiscoveryTimestamp
rhsTime := rhs.DiscoveryTimestamp
if lhsTime == nil || rhsTime == nil {
return rhs.CreationTimestamp.Compare(lhs.CreationTimestamp.Time)
})
}
return rhsTime.Time.Compare(lhsTime.Time)
Comment on lines +1820 to +1827
Copy link
Copy Markdown
Member

@krancour krancour Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as here: https://github.com/akuity/kargo/pull/6133/changes#r3118278060

I start to wonder whether we should include a generic SortByDiscoveryDate() helper somewhere.

})
newestPromotion := promotions.Items[0]
if newestPromotion.Status.Phase != kargoapi.PromotionPhaseSucceeded {
freightLogger.Debug(
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/warehouses/warehouses.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,7 @@ func (r *reconciler) buildFreightFromLatestArtifacts(
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
},
DiscoveryTimestamp: &metav1.Time{Time: time.Now()},
}

for _, result := range artifacts.Git {
Expand Down
2 changes: 2 additions & 0 deletions pkg/kargo/promotion_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"strings"
"time"

"github.com/oklog/ulid/v2"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -82,6 +83,7 @@ func (b *PromotionBuilder) Build(
Namespace: stage.Namespace,
Annotations: annotations,
},
DiscoveryTimestamp: &metav1.Time{Time: time.Now()},
Copy link
Copy Markdown
Member

@jessesuen jessesuen Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I'm asking for StartedAt timestamp to be a status thing, this will no longer be appropriate to set here. It's also something that the controller would need to set when it transitions the Promotion from Pending to Running.

Spec: kargoapi.PromotionSpec{
Stage: stage.Name,
Freight: freight,
Expand Down
7 changes: 6 additions & 1 deletion pkg/server/query_freights_v1alpha1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,12 @@ func sortFreightSlice(orderBy string, reverse bool, freight []*kargoapi.Freight)
return strings.Compare(lhsTag, rhsTag)
}
}
return lhs.CreationTimestamp.Compare(rhs.CreationTimestamp.Time)
lhsTime := lhs.DiscoveryTimestamp
rhsTime := rhs.DiscoveryTimestamp
if lhsTime == nil || rhsTime == nil {
return lhs.CreationTimestamp.Time.Compare(rhs.CreationTimestamp.Time)
}
return rhsTime.Time.Compare(lhsTime.Time)
Comment on lines +295 to +300
Copy link
Copy Markdown
Member

@krancour krancour Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as here: https://github.com/akuity/kargo/pull/6133/changes#r3118302803

Another point in favor of a generic helper.

})
if reverse {
slices.Reverse(freight)
Expand Down
Loading