Skip to content

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

Copy link

@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)

@ChenRussell ChenRussell requested a review from agilgur5 August 1, 2024 03:39
Copy link

@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

@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.

@ChenRussell ChenRussell requested a review from Joibel August 15, 2024 07:43
@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
@Joibel
Copy link
Member

Joibel commented Aug 15, 2024

Thank you for working through this.

@ChenRussell ChenRussell deleted the archive-workflow-memory branch August 15, 2024 11:57
@agilgur5
Copy link

@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