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

app sync --timeout x and app get --refresh hangs forever if application spec is invalid #21613

Open
agaudreault opened this issue Jan 21, 2025 · 4 comments · May be fixed by #21702
Open

app sync --timeout x and app get --refresh hangs forever if application spec is invalid #21613

agaudreault opened this issue Jan 21, 2025 · 4 comments · May be fixed by #21702
Assignees
@agaudreault @nitishfy
Labels
bug Something isn't working component:cli Affects the Argo CD CLI good first issue Good for newcomers

Comments

@agaudreault
Copy link
Member

agaudreault commented Jan 21, 2025 •
edited
Loading

Describe the bug

When an existing Application has an invalid spec that cannot be reconciled anymore, the argocd.argoproj.io/refresh: normal annotations will not be removed by the controller. This causes the get command to never return. Since this get is used without context timeout in sync and get command, the command hangs forever.

To Reproduce

  • Create a valid app
  • (fixed) Modify the app so it is invalid (remove the cluster, change permissions)
  • Scale down application-controller to 0 replicas (simulate very long processing).
  • perform app sync --timeout 5
> argocd app sync test-comparison-fails-if-in-cluster-disabled  --server localhost:8080 --auth-token x.y.z --insecure --timeout 5 --prune        
TIMESTAMP                  GROUP        KIND   NAMESPACE                                                                      NAME    STATUS   HEALTH        HOOK  MESSAGE
2025-01-21T15:22:44-05:00            Service  argocd-e2e--test-comparison-fails-if-in-cluster-disabled-mudys          guestbook-ui    Synced  Healthy              
2025-01-21T15:22:44-05:00   apps  Deployment  argocd-e2e--test-comparison-fails-if-in-cluster-disabled-mudys          guestbook-ui    Synced  Healthy              

This is the state of the app after `wait` timed out:
[HANGS HERE]

Or with app get --refresh

> argocd app get test-comparison-fails-if-in-cluster-disabled  --server localhost:8080 --auth-token x.y.z --insecure --refresh
[HANGS HERE]

The return condition for the get is never met:

argo-cd/server/application/application.go

Lines 793 to 808 in 5ef4faa

for {
select {
case <-ctx.Done():
return nil, errors.New("application refresh deadline exceeded")
case event := <-events:
if appVersion, err := strconv.Atoi(event.Application.ResourceVersion); err == nil && appVersion > minVersion {
annotations := event.Application.GetAnnotations()
if annotations == nil {
annotations = make(map[string]string)
}
if _, ok := annotations[v1alpha1.AnnotationKeyRefresh]; !ok {
return &event.Application, nil
}
}
}
}

Expected behavior

  • (DONE) The Controller should remove the refresh annotation when the reconciliation resulted in the application being invalid.
  • Because refresh may hang, the CLI should have a timeout parameter on the get.
  • The sync command should respect the timeout provided for the sync operation.

Version

3.0 / master
@agaudreault agaudreault added bug Something isn't working component:cli Affects the Argo CD CLI component:refresh labels Jan 21, 2025
@agaudreault agaudreault self-assigned this Jan 21, 2025
@agaudreault agaudreault mentioned this issue Jan 22, 2025
Merged
14 tasks
@agaudreault agaudreault added good first issue Good for newcomers component:cli Affects the Argo CD CLI and removed component:cli Affects the Argo CD CLI component:refresh labels Jan 22, 2025
@nitishfy
Copy link
Member

Is this issue still opened to work?

@agaudreault
Copy link
Member Author

Yes. The remaining 2 items still need to be fixed

  • Because refresh may hang, the CLI should have a timeout parameter on the get.
  • The sync command should respect the timeout provided for the sync operation.

@nitishfy
Copy link
Member

I'll take this up.

@nitishfy nitishfy self-assigned this Jan 24, 2025
@nitishfy
Copy link
Member

nitishfy commented Jan 29, 2025 •
edited
Loading

@agaudreault confirming something here - Do you want to have a parameter of timeout on the Get() function? If no, would you want to include this parameter in the ApplicationQuery?

@almoelda almoelda linked a pull request Jan 29, 2025 that will close this issue
Open
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@agaudreault agaudreault

@nitishfy nitishfy

Labels
bug Something isn't working component:cli Affects the Argo CD CLI good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(cli): Prevent Get & Sync from Hanging on Invalid Application Spec almoelda/argo-cd
2 participants
@agaudreault @nitishfy