-
Notifications
You must be signed in to change notification settings - Fork 36
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
Drop root privileges for celery worker #3272
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this fixes the warning for me :)
celerybeat-schedule | ||
celerybeat-schedule.db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
celerybeat-schedule.db | |
celerybeat-schedule.dat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
celerybeat-schedule.db
is apparently also a thing 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm wondering where the .dat file is coming from. I never had that :D
celerybeat-schedule | ||
celerybeat-schedule.db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
celerybeat-schedule.db
is apparently also a thing 😅
@@ -47,7 +47,7 @@ done | |||
listen_for_devserver & | |||
|
|||
# Run Celery worker process | |||
celery -A integreat_cms.integreat_celery worker -l INFO -B --concurrency=1 & | |||
deescalate_privileges celery -A integreat_cms.integreat_celery worker -l INFO -B --concurrency=1 & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more a question than a comment, but: are we dependent on celery for the dev env? Because as far as I can tell, I can still do everything I normally would in the CMS when commenting out this line. I get that background workers won't be available, but will this crash the dev server, or just lead to errors in the logs?
If it is "optional", I would love the option to set an environment variable to skip starting celery (and thus not need redis installed).
If it is mandatory, I think the docs should mention that redis is also a mandatory prerequisite to running the dev server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also be in favor of not doing this by default in debug mode, because it can really get annoying ^^
And sometimes its just necessary to not have redis running (For example to debug some db calls)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong opinion here. But I'd recommend to move all cron tasks to Celery scheduled tasks. Then the application can manage tasks in source code instead of cron configurations on the server. In the end Celery will simply not work if Redis is not running.
Short description
The run.sh is running with root privileges. However, celery does not need those.
Proposed changes
Side effects
Resolved issues
Fixes: https://chat.tuerantuer.org/digitalfabrik/pl/qcemkcp5aid8dnhes6obwtiooo
Pull Request Review Guidelines