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.

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

feat(controller): optimize memory with queue when archiving is rate-limited. Fixes #13418 #13419

Merged
merged 4 commits into from
Aug 15, 2024

Conversation

ChenRussell
Copy link
Contributor

Fix #13418

Motivation

When archiving workflows operations are severely rate-limited,there is a big memory usage increase in workflow controller

Modifications

To solve this problem, we can add a archived_wf_queue to archive the workflow asynchronously when listening the Add and Update events instead of archiving the workflow synchronously

Verification

run locally and tested

@agilgur5 agilgur5 changed the title fix: fix the workflow-controller memory increase when archiving workflow operations are rate-limited (#13418) fix(controller): optimize memory with queue when archiving is rate-limited (Fixes #13418) Jul 31, 2024
@agilgur5 agilgur5 changed the title fix(controller): optimize memory with queue when archiving is rate-limited (Fixes #13418) fix(controller): optimize memory with queue when archiving is rate-limited. Fixes #13418 Jul 31, 2024
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Thanks for root causing the memory increase and optimizing it!

I think we'd want to have a few people review this since it adds a new queue (with its own metrics, for instance) and a new flag.
There's also a consideration around naming those two, especially since there are potentially multiple different archive workers (e.g. GC'ing archived workflows a la archiveTTL is another one)

cmd/workflow-controller/main.go Outdated Show resolved Hide resolved
workflow/controller/controller.go Outdated Show resolved Hide resolved
workflow/controller/controller.go Outdated Show resolved Hide resolved
@agilgur5 agilgur5 added area/controller Controller issues, panics area/workflow-archive labels Jul 31, 2024
@agilgur5 agilgur5 changed the title fix(controller): optimize memory with queue when archiving is rate-limited. Fixes #13418 feat(controller): optimize memory with queue when archiving is rate-limited. Fixes #13418 Aug 1, 2024
@agilgur5 agilgur5 added this to the v3.6.0 milestone Aug 1, 2024
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Makes sense to me. @Joibel could you double-check this when you're back from vacation? Specifically as this adds a new queue with metrics. Also if you have any thoughts on the naming

@agilgur5 agilgur5 requested a review from Joibel August 1, 2024 03:54
@ChenRussell
Copy link
Contributor Author

Makes sense to me. @Joibel could you double-check this when you're back from vacation? Specifically as this adds a new queue with metrics. Also if you have any thoughts on the naming

@Joibel could you review this PR when you are free, thanks

@Joibel Joibel self-assigned this Aug 12, 2024
@Joibel
Copy link
Member

Joibel commented Aug 12, 2024

@Joibel could you review this PR when you are free, thanks

Yes, I'll take a look.

workflow/controller/controller.go Outdated Show resolved Hide resolved
workflow/controller/controller.go Outdated Show resolved Hide resolved
workflow/controller/controller.go Outdated Show resolved Hide resolved
@Joibel Joibel enabled auto-merge (squash) August 15, 2024 08:40
Joibel and others added 2 commits August 15, 2024 10:36
auto-merge was automatically disabled August 15, 2024 09:57

Head branch was pushed to by a user without write access

@ChenRussell
Copy link
Contributor Author

@Joibel CI / Windows Unit Tests failed,could you rerun it or merge this PR without it, thanks

@ChenRussell
Copy link
Contributor Author

@Joibel all checks have passed now, could you merge this PR? thanks

@Joibel Joibel merged commit c143e3e into argoproj:main Aug 15, 2024
28 checks passed
@Joibel
Copy link
Member

Joibel commented Aug 15, 2024

Thank you for working through this.

@agilgur5
Copy link
Contributor

@ChenRussell small reminder on the refactor discussed above

@ChenRussell
Copy link
Contributor Author

ChenRussell commented Aug 29, 2024

Yes, I will finish it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize workflow-controller memory usage when archiving workflow operations are rate-limited
4 participants