chore(refactor): remove app destination inferrence logic #21189

crenshaw-dev · 2024-12-16T02:20:11Z

The utils.ValidateDestination function has the side-effect of setting the app destination's server URL if it is not present.

This side effect makes the code hard to read and has introduced subtle bugs and race conditions.

This PR is an attempt to remove the side effect and instead have the code load in the cluster details whenever they're needed. It may result in more network calls, so we need to be aware of performance. We also need to look out for places that still assume the old side-effect behavior and fix them to explicitly pull the cluster details instead.

The code is called in five places in the app controller:

in finalizeApplicationDeletion; since not all code paths in processAppOperationQueueItem require a destCluster, we delegate that to finalizeApplicationDeletion when an app deletion is in progress
in processRequestedAppOperation, which is another code path accessible from processAppOperationQueueItem; we call it as late as possible, and we need it in order to call SyncAppState; there are some error paths in SyncAppState which don't require destCluster, but I think they'll be relatively rare
in processAppRefreshQueueItem; here again there are a few code paths which could potentially short-circuit the need to get the cluster, but the most common paths require it
in canProcessApp, which is called in 5 places; I'd much rather avoid this work for most of those cases, but that requires a bigger refactor
in the app informer's namespace index key generation function; this is another place that call doesn't belong, but it's a bigger refactor

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

bunnyshell · 2024-12-16T02:20:19Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

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

codecov · 2024-12-16T05:10:19Z

Codecov Report

Attention: Patch coverage is 66.16915% with 68 lines in your changes missing coverage. Please review.

Project coverage is 55.27%. Comparing base (f6a84a4) to head (4200551).
Report is 402 commits behind head on master.

Files with missing lines	Patch %	Lines
controller/state.go	50.00%	6 Missing and 3 partials ⚠️
server/application/application.go	50.00%	5 Missing and 4 partials ⚠️
cmd/argocd/commands/admin/cluster.go	27.27%	7 Missing and 1 partial ⚠️
controller/appcontroller.go	85.36%	3 Missing and 3 partials ⚠️
controller/cache/cache.go	71.42%	5 Missing and 1 partial ⚠️
util/argo/argo.go	83.33%	4 Missing and 2 partials ⚠️
controller/sync.go	58.33%	4 Missing and 1 partial ⚠️
server/project/project.go	37.50%	4 Missing and 1 partial ⚠️
cmd/argocd/commands/admin/app.go	50.00%	3 Missing and 1 partial ⚠️
pkg/apis/application/v1alpha1/app_project_types.go	66.66%	2 Missing and 1 partial ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #21189      +/-   ##
==========================================
+ Coverage   55.25%   55.27%   +0.01%     
==========================================
  Files         337      337              
  Lines       56940    56837     -103     
==========================================
- Hits        31461    31414      -47     
+ Misses      22801    22730      -71     
- Partials     2678     2693      +15

☔ 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>

crenshaw-dev

Note to self: there are probably a lot of code paths where we just need the destServerURL, not the whole Cluster object. Consider simplifying those code paths.

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

agaudreault

LGTM. Minor comments. Other people should review this I think because the potential side-effects are extremely hard to spot! Hopefully, we have enough unit test :)

agaudreault · 2024-12-20T22:01:46Z

cmd/argocd/commands/admin/app.go

@@ -437,7 +442,7 @@ func reconcileApplications(
 		sources = append(sources, app.Spec.GetSource())
 		revisions = append(revisions, app.Spec.GetSource().TargetRevision)

-		res, err := appStateManager.CompareAppState(&app, proj, revisions, sources, false, false, nil, false, false)
+		res, err := appStateManager.CompareAppState(destCluster, &app, proj, revisions, sources, false, false, nil, false, false)


I feel like it would be better to do the inference again in CompareAppState instead of adding a parameter here. Or maybe have a private property with an accessor in the app destination object?

As discussed, this is a refactor and can be improved further in another PR. No blocker.

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

leoluz

From the description this seems to be a breaking change. Can you please confirm?

crenshaw-dev · 2025-01-06T21:14:43Z

@leoluz not breaking, behavior should be 100% identical from a user's perspective.

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

util/argo/argo.go

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

leoluz

LGTM

util/argo/argo.go

) * refactor: remove app destination inferrence logic Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more fixes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more fixes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * codegen Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * fix mocks Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * clusters all the way down Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * test 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> * tidy Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * be less radical Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Brett C. Dudo <brett@dudo.io>

) * refactor: remove app destination inferrence logic Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more fixes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more fixes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * codegen Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * fix mocks Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * clusters all the way down Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * test 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> * tidy Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * be less radical Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

) * refactor: remove app destination inferrence logic Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more fixes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more fixes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * codegen Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * fix mocks Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * clusters all the way down Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * test 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> * tidy Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * be less radical Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: flbla <flbla@users.noreply.github.com>

) * refactor: remove app destination inferrence logic Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more fixes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more fixes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * codegen Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * fix mocks Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * clusters all the way down Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * simplify Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * test 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> * tidy Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * be less radical Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

refactor: remove app destination inferrence logic

1a6c7b0

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

crenshaw-dev changed the title ~~refactor: remove app destination inferrence logic~~ chore(refactor): remove app destination inferrence logic Dec 16, 2024

crenshaw-dev added 3 commits December 15, 2024 21:29

simplify

ea853a3

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

more fixes

f40cad8

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

more fixes

58f8554

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

codegen

ecaa8ba

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

crenshaw-dev mentioned this pull request Dec 16, 2024

bug: Race condition when setting application destination inferred server or name #21182

Closed

3 tasks

adriananeci mentioned this pull request Dec 16, 2024

fix: Temporary move TestMaxPodLogsRender to run with no race #21204

Closed

14 tasks

fix mocks

e46477d

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

crenshaw-dev commented Dec 16, 2024

View reviewed changes

crenshaw-dev added 10 commits December 17, 2024 16:01

Merge remote-tracking branch 'origin/master' into no-side-effects

4965dcc

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

simplify

728b56b

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

Merge remote-tracking branch 'origin/master' into no-side-effects

000890f

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

clusters all the way down

d732690

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

simplify

2b292ef

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

Merge remote-tracking branch 'origin/master' into no-side-effects

ab2dc00

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

test

c02885c

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

lint

e7c518c

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

Merge remote-tracking branch 'origin/master' into no-side-effects

3d72f41

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

tidy

5cc575e

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

agaudreault reviewed Dec 20, 2024

View reviewed changes

agaudreault approved these changes Dec 21, 2024

View reviewed changes

adriananeci mentioned this pull request Dec 21, 2024

fix: Race condition when setting application destination inferred server or name #21201

Closed

14 tasks

andrii-korotkov-verkada added the ready-for-review label Dec 29, 2024

crenshaw-dev added 2 commits January 3, 2025 17:08

be less radical

1807a45

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

Merge remote-tracking branch 'origin/master' into no-side-effects

8456ed8

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

crenshaw-dev marked this pull request as ready for review January 6, 2025 19:45

crenshaw-dev requested a review from a team as a code owner January 6, 2025 19:45

Merge remote-tracking branch 'origin/master' into no-side-effects

026d68b

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

leoluz reviewed Jan 6, 2025

View reviewed changes

Merge remote-tracking branch 'origin/master' into no-side-effects

efd74ee

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

leoluz reviewed Jan 8, 2025

View reviewed changes

util/argo/argo.go Show resolved Hide resolved

crenshaw-dev added 2 commits January 13, 2025 10:03

Merge remote-tracking branch 'origin/master' into no-side-effects

ae7fec3

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

Merge remote-tracking branch 'origin/master' into no-side-effects

4200551

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

leoluz approved these changes Jan 13, 2025

View reviewed changes

util/argo/argo.go Show resolved Hide resolved

leoluz merged commit bd37458 into argoproj:master Jan 13, 2025
27 checks passed

crenshaw-dev deleted the no-side-effects branch January 13, 2025 18:18

pvlkov mentioned this pull request Mar 28, 2025

Server-side diff causing excessive validation webhook calls since ArgoCD version > 2.13.2 #22321

Closed

crenshaw-dev mentioned this pull request Jun 5, 2025

fix(metrics): populate dest_server label (#23246) #23269

Merged

This was referenced Jun 20, 2025

Sync with impersonation does not work when app destinations reference server name instead of url #23309

Closed

fix(controller): impersonation with destination name (#23309) #23504

Merged

OpenGuidou mentioned this pull request Jul 4, 2025

Can't create sync windows if a cluster doesn't exist anymore but is still referenced in an app #23650

Open

3 tasks

crenshaw-dev mentioned this pull request Aug 15, 2025

High Application Controller CPU/Memory Usage in version 3.0.11 #24167

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(refactor): remove app destination inferrence logic #21189

chore(refactor): remove app destination inferrence logic #21189

Uh oh!

crenshaw-dev commented Dec 16, 2024 •

edited

Loading

Uh oh!

bunnyshell bot commented Dec 16, 2024 •

edited

Loading

Uh oh!

codecov bot commented Dec 16, 2024 •

edited

Loading

Uh oh!

crenshaw-dev left a comment

Uh oh!

agaudreault left a comment

Uh oh!

agaudreault Dec 20, 2024 •

edited

Loading

Uh oh!

agaudreault Dec 21, 2024

Uh oh!

leoluz left a comment

Uh oh!

crenshaw-dev commented Jan 6, 2025

Uh oh!

Uh oh!

leoluz left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chore(refactor): remove app destination inferrence logic #21189

chore(refactor): remove app destination inferrence logic #21189

Uh oh!

Conversation

crenshaw-dev commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bunnyshell bot commented Dec 16, 2024 • 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 Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

crenshaw-dev left a comment

Choose a reason for hiding this comment

Uh oh!

agaudreault left a comment

Choose a reason for hiding this comment

Uh oh!

agaudreault Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agaudreault Dec 21, 2024

Choose a reason for hiding this comment

Uh oh!

leoluz left a comment

Choose a reason for hiding this comment

Uh oh!

crenshaw-dev commented Jan 6, 2025

Uh oh!

Uh oh!

leoluz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crenshaw-dev commented Dec 16, 2024 •

edited

Loading

bunnyshell bot commented Dec 16, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading

agaudreault Dec 20, 2024 •

edited

Loading