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

Limit the number of celery workers that Octopoes can start #3232

Closed
wants to merge 10 commits into from

Conversation

r3boot
Copy link

@r3boot r3boot commented Jul 11, 2024

Changes

By default, when Celery runs without any concurrency limits, it will spawn a process for each visible core. On machines with lots of cores, this leads to a lot of unneeded processes. This PR adds an environment variable for Octopoes which is used to limit the number of celery processes that can be started.

@r3boot r3boot requested a review from a team as a code owner July 11, 2024 12:20
@CLAassistant
Copy link

CLAassistant commented Jul 11, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ underdarknl
✅ dekkers
✅ TwistMeister
✅ ammar92
❌ Lex van Roon


Lex van Roon seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@underdarknl
Copy link
Contributor

It would be good to make sure we have this var as an option, and not by default pin it at 8. As for many small systems 8 will already be too much.
Maybe in conjuction with autoscaling we can set both an upper and a lower limit?
https://docs.celeryq.dev/en/stable/userguide/workers.html#autoscaling

@ammar92
Copy link
Contributor

ammar92 commented Jul 11, 2024

Thanks for your submission.

I suggest setting the CELERY_WORKER_CONCURRENCY environment variable (perhaps as a short term solution). This way, you don't have to change anything within OpenKAT.

If we decide to make this configurable as an OpenKAT feature, we will need to establish a default value and provide documentation for it.

@underdarknl
Copy link
Contributor

Thanks for your submission.

I suggest setting the CELERY_WORKER_CONCURRENCY environment variable (perhaps as a short term solution). This way, you don't have to change anything within OpenKAT.

If we decide to make this configurable as an OpenKAT feature, we will need to establish a default value and provide documentation for it.

Lets add this to our documentation, or at least reference the original documentation on how to do this.

@dekkers
Copy link
Contributor

dekkers commented Jul 19, 2024

Thanks for your submission.

I suggest setting the CELERY_WORKER_CONCURRENCY environment variable (perhaps as a short term solution). This way, you don't have to change anything within OpenKAT.

If we decide to make this configurable as an OpenKAT feature, we will need to establish a default value and provide documentation for it.

I think this needs to both have a better default and be configurable. The current default is the number of cores, but this isn't the right value for a lot of deployment scenarios and for the development environment.

I think the best default is a small number of workers, because I think it is best if OpenKAT works out of the box for small installations because those are most often installed by people who don't have the time/knowledge to do a lot of configuration/tuning.

@stephanie0x00
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.
  • I checked the logs for errors and/or warnings and made issues where necessary

What works:

Looks good. Onboarding works, scheduling of tasks work. Tasks are scheduled and completed.

What doesn't work:

n/a

Bug or feature?:

n/a

@ammar92
Copy link
Contributor

ammar92 commented Aug 6, 2024

What works:

Looks good. Onboarding works, scheduling of tasks work. Tasks are scheduled and completed.

Cool! Just to be sure, since there were no QA notes. Were you able to confirm that the configuration is actually picked up by the Celery worker in the octopoes_api_worker service? Like this: image

jpbruinsslot added a commit that referenced this pull request Aug 13, 2024
* origin:
  Add geo OOI type and Maxmind geoip boefje (#3238)
  Allow MuteFindings to expire by a user specified datetime (#3343)
  Limit the number of Celery workers that Octopoes can start #3232 (#3337)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants