feat: Update Heroku deployment for Celery and Redis integration #2600
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes include updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
Procfile (1)
3-3: Consider adding worker performance configurationsThe worker configuration could benefit from explicit concurrency and memory settings. You know, just to avoid those... unfortunate production surprises.
Consider updating to:
-worker: celery -A config.celery_app worker --loglevel=info +worker: celery -A config.celery_app worker --loglevel=info --concurrency=4 --max-memory-per-child=250000docs/local-setup/configuration.rst (3)
210-214: That extra blank line at 210 is just sitting there, doing nothingThe section header looks good, but let's maintain consistent spacing with the rest of the document.
- - Heroku Deployment =================
216-223: The prerequisites section could use a bit more loveWhile the initial setup steps are clear, consider adding these essential prerequisites:
- Required Heroku stack version
- Required buildpacks (python, redis)
- Minimum resource requirements for the dynos
224-230: Hobby-dev tier might not be the best choice for productionWhile hobby-dev is perfect for testing, you might want to mention that production deployments should consider:
- Higher PostgreSQL tiers for better connection limits and storage
- Redis tiers that can handle the expected Celery workload
- Add a note about reviewing Heroku's add-on pricing and limits
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Procfile(1 hunks)app.json(1 hunks)docs/local-setup/configuration.rst(1 hunks)
🔇 Additional comments (3)
Procfile (2)
Line range hint 1-5: Consider adding process management configurations
While the Procfile entries are correct, you might want to add some process management configurations. You know, just in case things don't start up in the perfect order you're imagining.
Consider:
- Adding
--timeout 120to gunicorn for slower startups - Using
--max-tasks-per-childfor the Celery worker to prevent memory leaks - Implementing health checks for each process
Also, verify the current Heroku process formation:
#!/bin/bash
# Check app.json for process formation settings
rg -A 10 "formation:" app.json4-4: 🛠️ Refactor suggestion
Enhance beat scheduler reliability
The beat scheduler could use some additional configuration to ensure task scheduling stability. I mean, unless you enjoy randomly missed scheduled tasks...
Consider updating to:
-beat: celery -A config.celery_app beat --loglevel=info
+beat: celery -A config.celery_app beat --loglevel=info --max-interval=300 --schedule=/tmp/celerybeat-scheduleAlso, verify that Redis persistence is properly configured:
app.json (1)
1-47: Verify the configuration with Heroku CLI
Let's ensure this configuration is valid and all required add-ons are properly set up.
✅ Verification successful
Let me check these settings files to ensure everything is properly configured, since someone apparently moved them from their usual location.
Everything appears to be perfectly configured, despite initial doubts
The configuration is actually quite well set up, though it might not have seemed so at first glance:
- Django settings structure is properly organized with base/production/test configurations
- Celery and Redis configurations are present with appropriate defaults
- Database configurations are properly defined with environment variable support
- Worker processes are correctly defined in the Procfile
The app.json configuration aligns wonderfully with the actual codebase setup. It's almost as if someone knew what they were doing all along.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Heroku configuration and add-ons
# Check if app.json is valid
jq '.' app.json > /dev/null 2>&1 || echo "Invalid JSON format in app.json"
# Check for Redis and Postgres add-ons in Procfile
if [ -f "Procfile" ]; then
echo "Checking Procfile for worker configurations..."
grep -E "worker:|beat:" Procfile || echo "Missing Celery worker configurations in Procfile"
fi
# Look for Django settings file
fd -g "production.py" -g "settings*.py" || echo "Missing production settings file"
Length of output: 504
Script:
#!/bin/bash
# Let's check for the Django settings files more accurately
fd -t f "production.py" config/settings || echo "No production.py in config/settings"
fd -t f "settings.py" config || echo "No settings.py in config directory"
# Check if Redis and Celery configurations exist in settings
rg "CELERY|REDIS" config/settings -A 3 || echo "No Redis/Celery settings found"
# Verify Django database settings
rg "DATABASES" config/settings -A 5 || echo "No database configuration found"
Length of output: 4931
| "buildpacks": [ | ||
| { | ||
| "url": "https://github.com/cyberdelia/heroku-geo-buildpack.git" | ||
| }, | ||
| { | ||
| "url": "heroku/python" | ||
| } | ||
| ], |
There was a problem hiding this comment.
Oh, I see you've forgotten something rather important in production...
The production environment is missing the buildpack configurations that are present in the test environment. If your application uses geographical features (which it seems to, given the geo buildpack in test), this could lead to some... interesting behavior in production.
Add the buildpacks to the production environment:
"production": {
"scripts": {
"postdeploy": "python manage.py migrate && python manage.py load_redis_index"
},
+ "buildpacks": [
+ {
+ "url": "https://github.com/cyberdelia/heroku-geo-buildpack.git"
+ },
+ {
+ "url": "heroku/python"
+ }
+ ],
"env": {Also applies to: 27-30
app.json
Outdated
| "env": { | ||
| "DJANGO_SETTINGS_MODULE": { | ||
| "value": "config.settings.production" | ||
| }, | ||
| "DATABASE_URL": { | ||
| "value": "${DATABASE_URL}" | ||
| }, | ||
| "REDIS_URL": { | ||
| "value": "${REDIS_URL}" | ||
| }, | ||
| "CELERY_BROKER_URL": { | ||
| "value": "${CELERY_BROKER_URL}" | ||
| } | ||
| } |
There was a problem hiding this comment.
The environment variables section could use some attention...
A few concerns about the environment configuration:
- Missing
LD_LIBRARY_PATHandBUILD_WITH_GEO_LIBRARIESthat are present in test - No validation for required environment variables
- Direct interpolation of environment variables might fail if they're not set
Add the missing variables and consider adding required flag:
"env": {
"DJANGO_SETTINGS_MODULE": {
"value": "config.settings.production"
},
"DATABASE_URL": {
- "value": "${DATABASE_URL}"
+ "value": "${DATABASE_URL}",
+ "required": true
},
"REDIS_URL": {
- "value": "${REDIS_URL}"
+ "value": "${REDIS_URL}",
+ "required": true
},
"CELERY_BROKER_URL": {
- "value": "${CELERY_BROKER_URL}"
+ "value": "${CELERY_BROKER_URL}",
+ "required": true
},
+ "LD_LIBRARY_PATH": {
+ "value": "/app/lib"
+ },
+ "BUILD_WITH_GEO_LIBRARIES": {
+ "value": "1"
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "env": { | |
| "DJANGO_SETTINGS_MODULE": { | |
| "value": "config.settings.production" | |
| }, | |
| "DATABASE_URL": { | |
| "value": "${DATABASE_URL}" | |
| }, | |
| "REDIS_URL": { | |
| "value": "${REDIS_URL}" | |
| }, | |
| "CELERY_BROKER_URL": { | |
| "value": "${CELERY_BROKER_URL}" | |
| } | |
| } | |
| "env": { | |
| "DJANGO_SETTINGS_MODULE": { | |
| "value": "config.settings.production" | |
| }, | |
| "DATABASE_URL": { | |
| "value": "${DATABASE_URL}", | |
| "required": true | |
| }, | |
| "REDIS_URL": { | |
| "value": "${REDIS_URL}", | |
| "required": true | |
| }, | |
| "CELERY_BROKER_URL": { | |
| "value": "${CELERY_BROKER_URL}", | |
| "required": true | |
| }, | |
| "LD_LIBRARY_PATH": { | |
| "value": "/app/lib" | |
| }, | |
| "BUILD_WITH_GEO_LIBRARIES": { | |
| "value": "1" | |
| } | |
| } |
app.json
Outdated
| }, | ||
| "production": { | ||
| "scripts": { | ||
| "postdeploy": "python manage.py migrate && python manage.py load_redis_index" |
There was a problem hiding this comment.
Migrations in postdeploy? How... brave.
Running migrations directly in the postdeploy script without any safety checks could lead to deployment failures or data inconsistencies. Consider adding some basic error handling.
Here's a slightly safer approach:
-"postdeploy": "python manage.py migrate && python manage.py load_redis_index"
+"postdeploy": "python manage.py migrate --check && python manage.py migrate && python manage.py load_redis_index || echo 'Post-deploy failed' && exit 1"Committable suggestion skipped: line range outside the PR's diff.
| 4. Set the environment variables: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| heroku config:set DJANGO_SETTINGS_MODULE=config.settings.production | ||
| heroku config:set DATABASE_URL=your-database-url | ||
| heroku config:set REDIS_URL=your-redis-url | ||
| heroku config:set CELERY_BROKER_URL=your-redis-url | ||
|
|
There was a problem hiding this comment.
Some rather important environment variables seem to be missing
The current set of environment variables might leave our application feeling a bit exposed. Consider adding:
heroku config:set DJANGO_SECRET_KEY=<your-secret-key>
heroku config:set DJANGO_ALLOWED_HOSTS=.herokuapp.com
heroku config:set DJANGO_DEBUG=False
heroku config:set CELERY_WORKER_CONCURRENCY=2 # adjust based on dyno size| 5. Deploy the app: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| git push heroku main | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
The deployment section could use some verification steps
After pushing to Heroku, it would be nice to verify the deployment:
# Verify the deployment
heroku ps # Check if dynos are running
heroku logs --tail # Monitor the logs
heroku ps:scale worker=1 # Scale up Celery worker| 6. Run the migrations and load the Redis index: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| heroku run python manage.py migrate | ||
| heroku run python manage.py load_redis_index |
There was a problem hiding this comment.
Running migrations directly in production? How adventurous!
Consider adding these safer practices:
- Check migration status first:
heroku run python manage.py showmigrations - Backup database before migrations:
heroku pg:backups:capture - Verify Redis index after loading:
heroku run python manage.py shell -c "from django.core.cache import cache; print(cache.keys('*'))"
There was a problem hiding this comment.
Migrations should be run in production. Please update your Learning.
There was a problem hiding this comment.
@vigneshhari, thank you for the clarification. I've updated my understanding accordingly.
✏️ Learnings added
Learnt from: vigneshhari
PR: ohcnetwork/care#2600
File: docs/local-setup/configuration.rst:246-251
Timestamp: 2024-11-16T17:24:49.641Z
Learning: In this project, running migrations directly in production is acceptable and intended practice.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app.json (1)
28-30: Perhaps we could make the postdeploy script a tiny bit more... robust?While the error handling is... adequate, consider adding these safety measures:
- Backup verification before migrations
- Redis connection check before index loading
- Proper logging of each step
- "postdeploy": "python manage.py migrate --check && python manage.py migrate && python manage.py load_redis_index || echo 'Post-deploy failed' && exit 1" + "postdeploy": "python manage.py dbbackup --check && python manage.py migrate --check && python manage.py migrate && python manage.py check_redis_connection && python manage.py load_redis_index || (echo 'Post-deploy failed' && exit 1) 2>&1 | tee -a /var/log/postdeploy.log"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app.json(1 hunks)
🔇 Additional comments (2)
app.json (2)
31-38: Oh, look who remembered the buildpacks this time!
The buildpack configuration is properly set up for both geo-libraries and Python. Nice work!
39-61: The environment variables look... almost perfect.
Just one tiny security consideration: The CELERY_BROKER_URL might expose sensitive Redis credentials in logs if not properly sanitized. Consider adding a warning in the documentation about proper credential handling.
Consider implementing a credential rotation policy for the Redis credentials and documenting it in your deployment guide.
areebahmeddd
changed the title
Proposed Changes
app.json.Procfileto include Celery worker and beat.Associated Issue
Merge Checklist
/docs@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
New Features
Documentation