fix(controller): only update lastTransitionTime when needed #23018

crenshaw-dev · 2025-05-16T21:03:43Z

Checklist:

bunnyshell · 2025-05-16T21:03:48Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

codecov · 2025-05-17T17:09:28Z

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.00%. Comparing base (797f1d6) to head (d190760).
⚠️ Report is 471 commits behind head on master.

Files with missing lines	Patch %	Lines
controller/state.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #23018      +/-   ##
==========================================
- Coverage   60.00%   60.00%   -0.01%     
==========================================
  Files         343      343              
  Lines       57848    57847       -1     
==========================================
- Hits        34714    34710       -4     
- Misses      20358    20362       +4     
+ Partials     2776     2775       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> fix tests Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> fix tests Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> fix tests Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> fix tests, lint Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> lint Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

agaudreault · 2025-05-19T15:37:58Z

pkg/apis/application/v1alpha1/types.go

-	// Message is a human-readable informational message describing the health status
-	Message string `json:"message,omitempty" protobuf:"bytes,2,opt,name=message"`


Add back the field as deprecated just so the protobuf byte is reserved.

pkg/apis/application/v1alpha1/types.go

rumstead · 2025-05-19T15:45:06Z

pkg/apis/application/v1alpha1/types.go

@@ -1787,15 +1787,20 @@ type SyncStatus struct {
 }

 // HealthStatus contains information about the currently observed health state of an application or resource


super nit

Suggested change

// HealthStatus contains information about the currently observed health state of an application or resource

// AppHealthStatus contains information about the currently observed health state of an application or resource

rumstead · 2025-05-19T15:47:49Z

From my understanding, you are breaking HealthResource struct by app and resource. I guess that caused some issues when normalizing?

crenshaw-dev · 2025-05-19T16:09:25Z

@rumstead sharing a single field just made the code very confusing. The actual bug is that we update lastTransitionTime when we shouldn't.

I'm going to update the PR to make the changes minimal and backwards-compatible. Then I'm going to make an even more minimal 2.14 cherry-pick to just solve the bug with no other changes.

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> comment Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> simplify, better test Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> e2e test Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> remove unnecessary test - covered elsewhere' Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

agaudreault

LGTM. From my understanding, the change to the protobuf should be backward compatible too.

…#23018) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Lyheng <lyhengtep@gmail.com>

…#23018) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

…#23018) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>

krancour · 2025-07-22T22:19:41Z

Sorry to be commenting on something closed since May, but I'm trying to understand... since this basically undoes #18660 and #21120 is not yet merged, doesn't the deprecation of lastTransitionTime leave us once again in a spot where there is no available (reliable and non-deprecated) indicator of how fresh observations about App health may be?

crenshaw-dev · 2025-07-23T04:02:31Z

@krancour I don't think this change reverts #18660, I think it brings the implementation in line with the intent of that PR. lastTransitionTime was always intended to only be updated when the health status changed. Prior to this PR, it was updated at times even when there was no transition.

This PR deprecates two Application CR status fields, but neither of them is a deprecation of any existing behavior. The app health and resource health status fields previously shared a single struct. That struct contained a lastTransitionTime field, used only for app health, and a message field, used only for resource health. This PR split the shared struct into two specific structs for the two specific use cases. Both structs retained all fields not because all the fields were actually used, but because as a policy we don't remove fields in the v1alpha1 spec. The fields were marked as deprecated to ensure that no future changes erroneously start using those fields that, up to this point, have been unused.

We never had a reliable indicator of how fresh observations about app health are, unless by mistake as part of a buggy implementation of #18660. observedAt, which hasn't been merged yet, was always meant to be the feature to satisfy that use case.

krancour · 2025-07-23T19:52:04Z

@crenshaw-dev thanks. I follow.

"Reliable indicator of health freshness" (paraphrasing) was a very poor word choice on my part. I agree lastTransitionTime didn't at all (unless by accident of a buggy implementation) achieve that.

Unreliable as it was, lastTransitionTime was the only thing we had to go on to, heuristically, guess at freshness, so I suppose my previous comment really boiled down to surprise at seeing it deprecated without the superior observedAt field from #21120 having yet replaced it.

Thanks for the response. Much appreciated.

crenshaw-dev · 2025-07-24T02:45:31Z

You bet! Thanks for following up for clarification, I didn't make very clear on this PR what it was doing.

I wouldn't think of observedAt as superior to lastTransitionTime necessarily... they both serve valid purposes and can make a great combo. I wasn't a fan of observedAt early on, but I think it's useful and fine as an opt-in. I don't have bandwidth to push that PR right now, but I'll be happy to support getting it merged.

And just to be extra clear, this PR didn't deprecate anything meaningful. It deprecated a couple struct fields that were never used in the first place. The only change in behavior in this PR was to limit lastTransitionTime updates to moments when there was actually a health status transition.

crenshaw-dev force-pushed the do-not-reuse-structs branch 2 times, most recently from 6d652f8 to e4701b3 Compare May 16, 2025 21:44

crenshaw-dev mentioned this pull request May 16, 2025

Application Controller is sending many more patch requests to Kubernetes API in v2.14.9 #22739

Closed

3 tasks

crenshaw-dev force-pushed the do-not-reuse-structs branch from e4701b3 to 3494413 Compare May 17, 2025 16:28

crenshaw-dev force-pushed the do-not-reuse-structs branch from 3494413 to 8233906 Compare May 17, 2025 17:27

agaudreault marked this pull request as ready for review May 19, 2025 15:32

agaudreault requested a review from a team as a code owner May 19, 2025 15:32

agaudreault reviewed May 19, 2025

View reviewed changes

pkg/apis/application/v1alpha1/types.go Show resolved Hide resolved

rumstead reviewed May 19, 2025

View reviewed changes

crenshaw-dev marked this pull request as draft May 19, 2025 16:08

agaudreault mentioned this pull request May 19, 2025

fix: infinite reconciliation loop when app is in error #23047

Merged

crenshaw-dev force-pushed the do-not-reuse-structs branch 2 times, most recently from f8dcda3 to 39968f9 Compare May 19, 2025 20:01

crenshaw-dev force-pushed the do-not-reuse-structs branch from 39968f9 to d190760 Compare May 19, 2025 21:03

crenshaw-dev marked this pull request as ready for review May 20, 2025 14:47

agaudreault approved these changes May 20, 2025

View reviewed changes

crenshaw-dev merged commit 45a8c68 into argoproj:master May 20, 2025
40 of 41 checks passed

agaudreault mentioned this pull request May 20, 2025

fix: infinite reconciliation loop when app is in error #23067

Merged

14 tasks

tylerrosnett pushed a commit to StateFarmIns/argo-cd that referenced this pull request May 27, 2025

fix(controller): only update lastTransitionTime when needed (argoproj…

1be87d9

…#23018) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

alexymantha mentioned this pull request May 28, 2025

fix: add nil check for ArgoCD LastTransitionTime argoproj-labs/gitops-promoter#320

Merged

chansuke pushed a commit to chansuke/argo-cd that referenced this pull request Jun 4, 2025

fix(controller): only update lastTransitionTime when needed (argoproj…

923ac6e

…#23018) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

This was referenced Jul 22, 2025

feat: optimize health checks using LastTransitionTime field from ArgoCD akuity/kargo#4608

Closed

check app health's LastTransitionTime field akuity/kargo#4595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(controller): only update lastTransitionTime when needed #23018

fix(controller): only update lastTransitionTime when needed #23018

Uh oh!

crenshaw-dev commented May 16, 2025

Uh oh!

bunnyshell bot commented May 16, 2025 •

edited

Loading

Uh oh!

codecov bot commented May 17, 2025 •

edited

Loading

Uh oh!

agaudreault May 19, 2025

Uh oh!

Uh oh!

rumstead May 19, 2025

Uh oh!

rumstead commented May 19, 2025

Uh oh!

crenshaw-dev commented May 19, 2025

Uh oh!

agaudreault left a comment

Uh oh!

Uh oh!

krancour commented Jul 22, 2025

Uh oh!

crenshaw-dev commented Jul 23, 2025 •

edited

Loading

Uh oh!

krancour commented Jul 23, 2025

Uh oh!

crenshaw-dev commented Jul 24, 2025

Uh oh!

Uh oh!

		// Message is a human-readable informational message describing the health status
		Message string `json:"message,omitempty" protobuf:"bytes,2,opt,name=message"`

		@@ -1787,15 +1787,20 @@ type SyncStatus struct {
		}

		// HealthStatus contains information about the currently observed health state of an application or resource

	// HealthStatus contains information about the currently observed health state of an application or resource
	// AppHealthStatus contains information about the currently observed health state of an application or resource

fix(controller): only update lastTransitionTime when needed #23018

fix(controller): only update lastTransitionTime when needed #23018

Uh oh!

Conversation

crenshaw-dev commented May 16, 2025

Uh oh!

bunnyshell bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Preview Environment deleted from Bunnyshell

Uh oh!

codecov bot commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

agaudreault May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rumstead May 19, 2025

Choose a reason for hiding this comment

Uh oh!

rumstead commented May 19, 2025

Uh oh!

crenshaw-dev commented May 19, 2025

Uh oh!

agaudreault left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krancour commented Jul 22, 2025

Uh oh!

crenshaw-dev commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krancour commented Jul 23, 2025

Uh oh!

crenshaw-dev commented Jul 24, 2025

Uh oh!

Uh oh!

bunnyshell bot commented May 16, 2025 •

edited

Loading

codecov bot commented May 17, 2025 •

edited

Loading

crenshaw-dev commented Jul 23, 2025 •

edited

Loading