fix(cli): Prevent Get & Sync from Hanging on Invalid Application Spec #21702

almoelda · 2025-01-29T11:03:33Z

Closes #21613

Adding context cancellation to get cmd and cancelling the context before printFinalStatus on sync command

Checklist:

bunnyshell · 2025-01-29T11:03:44Z

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🔵 /bns:start to start the environment
🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to remove the environment

…ore printFinalStatus on sync command Signed-off-by: Almo Elda <[email protected]>

…rgoproj#21433) Signed-off-by: Jagpreet Singh Tamber <[email protected]> Signed-off-by: Alexandre Gaudreault <[email protected]> Co-authored-by: Alexandre Gaudreault <[email protected]> Signed-off-by: Almo Elda <[email protected]>

…rgoproj#21698) Signed-off-by: Keith Chong <[email protected]> Signed-off-by: Almo Elda <[email protected]>

argoproj#21677) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Almo Elda <[email protected]>

…rgoproj#21676) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Almo Elda <[email protected]>

Signed-off-by: Michael Crenshaw <[email protected]> Signed-off-by: Almo Elda <[email protected]>

Signed-off-by: Almo Elda <[email protected]>

codecov · 2025-01-29T12:08:50Z

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 55.60%. Comparing base (e147247) to head (6f13f63).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/argocd/commands/app.go	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #21702      +/-   ##
==========================================
+ Coverage   55.51%   55.60%   +0.08%     
==========================================
  Files         339      340       +1     
  Lines       57273    57424     +151     
==========================================
+ Hits        31797    31929     +132     
- Misses      22790    22813      +23     
+ Partials     2686     2682       -4

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

…lid-spec Signed-off-by: Almo Elda <[email protected]>

nitishfy · 2025-01-29T17:22:24Z

cmd/argocd/commands/app.go

 			cancel()
+			printFinalStatus(app)


Suggested change

printFinalStatus(app)

printFinalStatus(app)

why are you printing after the cancel() has been called?

Good point, thanks

This is correct otherwise the ctx never gets called in the Get function. It is better to give a cancellation signal when the timeout has occured.

nitishfy · 2025-01-29T17:22:54Z

cmd/argocd/commands/app.go


 			appName, appNs := argo.ParseFromQualifiedName(args[0], appNamespace)

+			if timeout != 0 {
+				time.AfterFunc(time.Duration(timeout)*time.Second, func() {
+					fmt.Println("Context cancelled due to timeout")


Suggested change

fmt.Println("Context cancelled due to timeout")

I don't think it's necessary.

nitishfy · 2025-01-29T17:24:42Z

cmd/argocd/commands/app.go

@@ -337,6 +337,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
 		refresh        bool
 		hardRefresh    bool
 		output         string
+		timeout        int


Suggested change

timeout int

timeout uiint

This should be uint.

nitishfy · 2025-01-29T17:25:57Z

cmd/argocd/commands/app.go

@@ -462,6 +473,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
 		},
 	}
 	command.Flags().StringVarP(&output, "output", "o", "wide", "Output format. One of: json|yaml|wide|tree")
+	command.Flags().IntVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.")


Suggested change

command.Flags().IntVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.")

command.Flags().UintVar(&timeout, "timeout", defaultCheckTimeoutSeconds, "Time out after this many seconds")

nitishfy

Works perfectly. I've left a few comments before we merge this PR. The changes work well for the sync command too even though you only added the timeout flag to the get command. I wonder how and why?

Signed-off-by: Almo Elda <[email protected]>

…n also Signed-off-by: Almo Elda <[email protected]>

…lid-spec Signed-off-by: Almo Elda <[email protected]>

almoelda · 2025-01-29T17:53:07Z

Thanks @nitishfy
I've committed the suggested changes
Regarding sync command, it already has a timeout flag propagated into waitOnApplicationStatus, see below

argo-cd/cmd/argocd/commands/app.go

Line 2588 in 8449bab

if timeout != 0 {

nitishfy · 2025-01-29T17:59:03Z

Thanks @nitishfy I've committed the suggested changes Regarding sync command, it already has a timeout flag propagated into waitOnApplicationStatus, see below

argo-cd/cmd/argocd/commands/app.go

Line 2588 in 8449bab

if timeout != 0 {

why can't you call the waitOnApplicationStatus() in the get command too like the way we are calling for argocd app wait command?

nitishfy · 2025-01-29T18:27:07Z

cmd/argocd/commands/app.go

+			if timeout != 0 {
+				time.AfterFunc(time.Duration(timeout)*time.Second, func() {
+					cancel()
+				})
+			}


Suggested change

if timeout != 0 {

time.AfterFunc(time.Duration(timeout)*time.Second, func() {

cancel()

})

}

app, _, err := waitOnApplicationStatus(ctx, acdClient, appName, uint(timeout), watchOpts{operation: true}, nil, output)

errors.CheckError(err)

You don't need all of this. Instead call the waitOnApplicationStatus before making the first GET call.

Yeah sounds good. I wanted to ask about hardRefresh since waitOnApplicationStatus only do a normal refresh
please correct me if got it wrong

I guess (correct me if i'm wrong) if we proceed with waitOnApplicationStatus we would have to flag an hardRefresh somehow. maybe a context key-value or adding a parameter causing us to adjust all callers to this function. let me know what do you think and i'll get on it.

nitishfy

PTAL now.

rumstead · 2025-01-31T16:26:23Z

cmd/argocd/commands/app.go

@@ -382,17 +383,26 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
  		`),

 		Run: func(c *cobra.Command, args []string) {
-			ctx := c.Context()
+			ctx, cancel := context.WithCancel(c.Context())


should we set up the context after we validate args on line 387?

I don't think we need to change the ctx here anymore if we use the waitOnApplicationStatus func.

Generally I agree with you @nitishfy invoking the the wait function looks like the best way
But it will not utilize the hardRefresh

#21702 (comment)

agaudreault · 2025-02-01T02:38:23Z

cmd/argocd/commands/app.go

@@ -2577,7 +2588,7 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client,
 	if timeout != 0 {
 		time.AfterFunc(time.Duration(timeout)*time.Second, func() {
 			_, appClient := acdClient.NewApplicationClientOrDie()
-			app, err := appClient.Get(ctx, &application.ApplicationQuery{


I think when it timeouts, this will call cancel() on the context which will in turn close the appEventCh causing the for-range loop below to break. The last call will be _ = printFinalStatus(app).

This would mean that the AfterFunc should make sure that

It is not also calling printFinalStatus ✅

it should set refresh = false to make sure that the last call to printFinalStatus will not refresh the app.

it should call app, err = appClient.Get(ctx, &application.ApplicationQuery to update the app (without refresh) so it is used by printFinalStatus.

I haven't debugged if it is really what the execution does, but it should be testable in a unit test similar to TestWaitOnApplicationStatus_JSON_YAML_WideOutput.

There are also a few other problem with the code like the connection not being closed in the AfterFunc, and potential race conditions with refresh and app that might now require a lock. TBD

cmd/argocd/commands/app.go

agaudreault · 2025-02-01T03:06:26Z

cmd/argocd/commands/app.go

@@ -382,17 +383,26 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
  		`),

 		Run: func(c *cobra.Command, args []string) {
-			ctx := c.Context()
+			ctx, cancel := context.WithCancel(c.Context())


immediately call defer cancel()

agaudreault · 2025-02-01T03:58:43Z

cmd/argocd/commands/app.go


 			appName, appNs := argo.ParseFromQualifiedName(args[0], appNamespace)

+			if timeout != 0 {
+				time.AfterFunc(time.Duration(timeout)*time.Second, func() {
+					cancel()


I think the logic of a app get --refresh --timeout 10 should be to try to get with the refresh for 10 sec, and after 10 second, print a message, then fallback to the normal get and return that.

Co-authored-by: Alexandre Gaudreault <[email protected]> Signed-off-by: almoelda <[email protected]>

almoelda requested a review from a team as a code owner January 29, 2025 11:03

almoelda force-pushed the cli-hangs-on-invalid-spec branch from 3057641 to 753d4f8 Compare January 29, 2025 11:04

almoelda changed the title ~~Prevent Get & Sync from Hanging on Invalid Application Spec~~ fix(cli): Prevent Get & Sync from Hanging on Invalid Application Spec Jan 29, 2025

almoelda requested a review from a team as a code owner January 29, 2025 11:25

almoelda and others added 8 commits January 29, 2025 13:28

adding context cancellation to get cmd and cancelling the context bef…

858a48b

…ore printFinalStatus on sync command Signed-off-by: Almo Elda <[email protected]>

fix: Update argo-ui dependency to pull in OCI icon (argoproj#18646) (a…

c5153c5

…rgoproj#21698) Signed-off-by: Keith Chong <[email protected]> Signed-off-by: Almo Elda <[email protected]>

chore(deps): bump github.com/bmatcuk/doublestar/v4 from 4.8.0 to 4.8.1 (

312dd15

argoproj#21677) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Almo Elda <[email protected]>

chore(deps): bump google.golang.org/protobuf from 1.36.3 to 1.36.4 (a…

9ef20a5

…rgoproj#21676) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Almo Elda <[email protected]>

chore(metrics)!: remove deprecated metrics (argoproj#21697)

9790d93

Signed-off-by: Michael Crenshaw <[email protected]> Signed-off-by: Almo Elda <[email protected]>

codegen

265536c

Signed-off-by: Almo Elda <[email protected]>

add cancel call to defer on app get

ac12366

Signed-off-by: Almo Elda <[email protected]>

almoelda force-pushed the cli-hangs-on-invalid-spec branch from 58ef4e8 to ac12366 Compare January 29, 2025 11:28

Merge remote-tracking branch 'upstream/master' into cli-hangs-on-inva…

fe3686e

…lid-spec Signed-off-by: Almo Elda <[email protected]>

nitishfy reviewed Jan 29, 2025

View reviewed changes

nitishfy suggested changes Jan 29, 2025

View reviewed changes

almoelda added 3 commits January 29, 2025 19:47

app get timeout to uint, app sync do not print after timeout reached

564b7cb

Signed-off-by: Almo Elda <[email protected]>

app get timeout default change to common variable, changed descriptio…

8449bab

…n also Signed-off-by: Almo Elda <[email protected]>

Merge remote-tracking branch 'upstream/master' into cli-hangs-on-inva…

693ccd2

…lid-spec Signed-off-by: Almo Elda <[email protected]>

agaudreault self-assigned this Jan 29, 2025

nitishfy reviewed Jan 29, 2025

View reviewed changes

nitishfy suggested changes Jan 29, 2025

View reviewed changes

rumstead reviewed Jan 31, 2025

View reviewed changes

agaudreault requested changes Feb 1, 2025

View reviewed changes

agaudreault reviewed Feb 1, 2025

View reviewed changes

Update cmd/argocd/commands/app.go

6f13f63

Co-authored-by: Alexandre Gaudreault <[email protected]> Signed-off-by: almoelda <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): Prevent Get & Sync from Hanging on Invalid Application Spec #21702

fix(cli): Prevent Get & Sync from Hanging on Invalid Application Spec #21702

almoelda commented Jan 29, 2025 •

edited

Loading

bunnyshell bot commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

nitishfy Jan 29, 2025

almoelda Jan 29, 2025

nitishfy Jan 29, 2025

nitishfy Jan 29, 2025

nitishfy Jan 29, 2025

nitishfy Jan 29, 2025

nitishfy left a comment

almoelda commented Jan 29, 2025

nitishfy commented Jan 29, 2025

nitishfy Jan 29, 2025

almoelda Jan 29, 2025

almoelda Jan 29, 2025

nitishfy left a comment

rumstead Jan 31, 2025

nitishfy Jan 31, 2025

almoelda Jan 31, 2025

agaudreault Feb 1, 2025

agaudreault Feb 1, 2025

agaudreault Feb 1, 2025

	command.Flags().IntVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.")
	command.Flags().UintVar(&timeout, "timeout", defaultCheckTimeoutSeconds, "Time out after this many seconds")

fix(cli): Prevent Get & Sync from Hanging on Invalid Application Spec #21702

Are you sure you want to change the base?

fix(cli): Prevent Get & Sync from Hanging on Invalid Application Spec #21702

Conversation

almoelda commented Jan 29, 2025 • edited Loading

bunnyshell bot commented Jan 29, 2025 • edited Loading

🔴 Preview Environment stopped on Bunnyshell

codecov bot commented Jan 29, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitishfy left a comment

Choose a reason for hiding this comment

almoelda commented Jan 29, 2025

nitishfy commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitishfy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almoelda commented Jan 29, 2025 •

edited

Loading

bunnyshell bot commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading