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: implement graceful shutdown for api server #15938

Closed
wants to merge 5 commits into from
Closed

fix: implement graceful shutdown for api server #15938

wants to merge 5 commits into from

Conversation

leoluz
Copy link
Collaborator

@leoluz leoluz commented Oct 13, 2023 •
edited
Loading

Argo CD API server will eventually return 502 during rolling update.
This PR is an attempt to implement graceful shutdown for the API server process.

Partially fix #18642

Signed-off-by: Leonardo Luz Almeida leonardo_almeida@intuit.com

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.
  • Optional. My organization is added to USERS.md.
  • 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.

@leoluz
fix: implement graceful shutdown for api server
697207d 
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
@leoluz leoluz requested a review from a team as a code owner October 13, 2023 00:27
@leoluz leoluz marked this pull request as draft October 13, 2023 00:28
@codecov
Copy link

codecov bot commented Oct 13, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 11.42857% with 62 lines in your changes missing coverage. Please review.

Project coverage is 49.64%. Comparing base (f8f9ae9) to head (7fde5eb).
Report is 742 commits behind head on master.

❗ Current head 7fde5eb differs from pull request most recent head 3eb12d7

Please upload reports for the commit 3eb12d7 to get more accurate results.

Files Patch % Lines
server/server.go 11.42% 60 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #15938      +/-   ##
==========================================
- Coverage   49.69%   49.64%   -0.05%     
==========================================
  Files         267      267              
  Lines       46362    46418      +56     
==========================================
+ Hits        23039    23045       +6     
- Misses      21063    21111      +48     
- Partials     2260     2262       +2

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

leoluz added 3 commits October 12, 2023 22:02
@leoluz
Shutdown servers in parallel
ca76022 
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
@leoluz
add noop shutdown function on api server constructor
6ec6efd 
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
@leoluz
fix waitgroup
7fde5eb 
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
crenshaw-dev
crenshaw-dev requested changes Jun 12, 2024
View reviewed changes
Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Having trouble getting code coverage for api-server. I hoped graceful shutdown would help, so was testing this.

server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
@crenshaw-dev
Graceful api server shutdown tweaks (#4)
3eb12d7 
* more attempts to gracefully shut down

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

* fix import order

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

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
crenshaw-dev
crenshaw-dev reviewed Jun 13, 2024
View reviewed changes
cmd/argocd-server/commands/argocd_server.go
Comment on lines +212 to +213
log.Info("API Server successfully shut down.")
break
Copy link
Member

Choose a reason for hiding this comment

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

I guess this loop can go away completely now.

@crenshaw-dev crenshaw-dev mentioned this pull request Jun 13, 2024
Closed
5 tasks
@andrii-korotkov-verkada
Copy link
Contributor

I spoke with @leoluz about picking up this work. I've filed #20981, since there's been merge conflicts and resolve conflicts didn't work perhaps due to age. I did some fixes and added co-authors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crenshaw-dev crenshaw-dev crenshaw-dev requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement graceful shutdown in all Argo CD components
3 participants
@leoluz @andrii-korotkov-verkada @crenshaw-dev