Skip to content
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

Jump to bottom

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

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 25 commits into from

Conversation

almoelda
Copy link
Contributor

@almoelda almoelda commented Jan 29, 2025 •
edited
Loading

Closes #21613

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

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@almoelda almoelda requested a review from a team as a code owner January 29, 2025 11:03
@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Jan 29, 2025 •
edited
Loading

🔴 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

@almoelda almoelda force-pushed the cli-hangs-on-invalid-spec branch from 3057641 to 753d4f8 Compare January 29, 2025 11:04
@almoelda 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 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
@almoelda
adding context cancellation to get cmd and cancelling the context bef…
858a48b 
…ore printFinalStatus on sync command

Signed-off-by: Almo Elda <[email protected]>
@jagpreetstamber @agaudreault @almoelda
feat: add support for azure workload identity in Microsoft Entra SSO (a…
af34343 
…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]>
@keithchong @almoelda
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]>
@dependabot @almoelda
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]>
@dependabot @almoelda
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]>
@crenshaw-dev @almoelda
chore(metrics)!: remove deprecated metrics (argoproj#21697)
9790d93 
Signed-off-by: Michael Crenshaw <[email protected]>
Signed-off-by: Almo Elda <[email protected]>
@almoelda
codegen
265536c 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
add cancel call to defer on app get
ac12366 
Signed-off-by: Almo Elda <[email protected]>
@almoelda almoelda force-pushed the cli-hangs-on-invalid-spec branch from 58ef4e8 to ac12366 Compare January 29, 2025 11:28
@codecov Codecov
Copy link

codecov bot commented Jan 29, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 45.20548% with 40 lines in your changes missing coverage. Please review.

Project coverage is 55.61%. Comparing base (b4a63ae) to head (5106b19).

Files with missing lines Patch % Lines
cmd/argocd/commands/app.go 45.20% 40 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #21702      +/-   ##
==========================================
+ Coverage   55.59%   55.61%   +0.01%     
==========================================
  Files         340      340              
  Lines       57419    57474      +55     
==========================================
+ Hits        31924    31965      +41     
- Misses      22807    22817      +10     
- Partials     2688     2692       +4

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

nitishfy
nitishfy reviewed Jan 29, 2025
View reviewed changes
cmd/argocd/commands/app.go Outdated
cancel()
printFinalStatus(app)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
printFinalStatus(app)
printFinalStatus(app)

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks

Copy link
Member

Choose a reason for hiding this comment

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

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
nitishfy reviewed Jan 29, 2025
View reviewed changes
cmd/argocd/commands/app.go Outdated

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")
Copy link
Member

Choose a reason for hiding this comment

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

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

I don't think it's necessary.

nitishfy
nitishfy reviewed Jan 29, 2025
View reviewed changes
cmd/argocd/commands/app.go Outdated
@@ -337,6 +337,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
refresh bool
hardRefresh bool
output string
timeout int
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
timeout int
timeout uiint

This should be uint.

nitishfy
nitishfy reviewed Jan 29, 2025
View reviewed changes
cmd/argocd/commands/app.go Outdated
@@ -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.")
Copy link
Member

Choose a reason for hiding this comment

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

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
nitishfy suggested changes Jan 29, 2025
View reviewed changes
Copy link
Member

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

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?

@almoelda
Copy link
Contributor Author

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
Copy link
Member

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?

@agaudreault agaudreault self-assigned this Jan 29, 2025
nitishfy
nitishfy suggested changes Jan 29, 2025
View reviewed changes
Copy link
Member

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

PTAL now.

agaudreault
agaudreault requested changes Feb 1, 2025
View reviewed changes
cmd/argocd/commands/app.go Show resolved Hide resolved
cmd/argocd/commands/app.go Outdated Show resolved Hide resolved
cmd/argocd/commands/app.go Show resolved Hide resolved
almoelda and others added 13 commits February 1, 2025 11:54
@almoelda @agaudreault
Update cmd/argocd/commands/app.go
6f13f63 
Co-authored-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: almoelda <[email protected]>
@almoelda
codegen
0061e3f 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
adding a test and a simulateTimeout to the mock client
e69263c 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
timeout handling
7c13a30 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
Merge remote-tracking branch 'upstream/master' into cli-hangs-on-inva…
61caf4e 
…lid-spec

Signed-off-by: Almo Elda <[email protected]>
@almoelda
lose debug code
74b736e 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
introduce getAppStateWithRetry to ensure retry without refresh after …
0984859 
…timeout

Signed-off-by: Almo Elda <[email protected]>
@almoelda
revert time sleep in app_test
9bd679e 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
introduce lock sync on app in waitOnApplicationStatus
6c69778 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
fix lock sync on app in waitOnApplicationStatus
a44bde2 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
change app variable inside waitOnApplicationStatus.AfterFunc
49ad06f 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
remove unnecessary new lines
13e9e8d 
Signed-off-by: Almo Elda <[email protected]>
@almoelda
lint fix
5106b19 
Signed-off-by: Almo Elda <[email protected]>
@almoelda almoelda requested a review from agaudreault February 3, 2025 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nitishfy nitishfy nitishfy requested changes

@rumstead rumstead rumstead left review comments

@agaudreault agaudreault Awaiting requested review from agaudreault

Requested changes must be addressed to merge this pull request.

Assignees

@agaudreault agaudreault

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

app sync --timeout x and app get --refresh hangs forever if application spec is invalid
7 participants
@almoelda @nitishfy @rumstead @agaudreault @jagpreetstamber @keithchong @crenshaw-dev