-
Notifications
You must be signed in to change notification settings - Fork 1
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
Replace MinIO / S3 with cached WhiteNoise and FileSystemStorage #91
Conversation
Enables support for Django 4.1
Only files that match the provided glob pattern are copied. This avoids copying files like .DS_Store.
Following the advice in Django's documentation: https://docs.djangoproject.com/en/4.2/topics/db/models/#overriding-model-methods
.dockerignore file now ignores more directories and files that are not used for the Dockerfile at all.
This reverts commit 0feb924.
django.utils.timezone has been replaced by datetime.timezone
Context processors require a Request object. This may not always be available, especially for emails and documents. Instead, a 'settings' template tag is used: {% settings "NAME" %}
In the Dockerfile, we remove the static files from the app directories. Consequently, the 'finders' would not be able to find the files if only app dir finders are specified. Instead, use 'staticfiles_storage.open' to read the stylesheets.
Fixes compatibility with Python 3.11
Considering the time this might take to deploy (change NGINX config, install and configure NFS), when would be the best time to deploy, @ax-sc? Is there an important event upcoming? The changes are already all done and ready for deployment. I tested on local virtual machines. Before deploying, I would of course take a snapshot to revert all changes if something goes wrong. Deploying for the dev environment first of course. |
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 bunch for putting this together @jnsdrtlf ! I refrained to do local testing on my end as you seem to have done this very thoroughly. While looking into the changes nothing appeared suspicious to me immediately and you did a great job in summarising and describing your changes in the PR already.
de4b7b0 partially fixes #75 (correct?), what do you think about adding a way (a django-admin command?) to clear all documents (to be used as described in the issue) to this PR now that you are working on the files anyway?
Another very minor suggestion would be to change the time when the file task is run to maybe the night after we run the general database maintenance (btw, should we shift this to the night as well?), assuming that this is what could cause at least a few document entries to become out of sync.
I am happy to already approve the changes as they are now although we should coordinate merging and deploying as indicated by your comment. I would suggest to not deploy the changes before 24 July as we are aware of a few groups using the web application regularly as part activities of the GewäserCampus project at least until 21 July. If possible, we should try to schedule the deployment for an evening in the week after and be prepared for a rollback (as you already planned). I will ensure that our backups are up-to-date before but it's still a good idea to keep a snapshot until we are confident that everything went as planned.
Thanks for the fast response! As for issue #75: Sure, I can do that. Do you mean a command that is run from within the admin interface (/admin)? I don't know if thats possible just like that, as commands are always run on a queryset (i.e. the user selects for example 5 measurements and runs a command on them). The cleanup tasks however span over multiple models. Maybe there is an option to add general actions into the admin interface. I will check this. The document cleanup command is already scheduled to run at 3:40 in the night from Friday to Saturday. The other maintenance tasks were set to be run during the day as they might notify teachers when a course is about to be deactivated. I think we can leave that for now. |
I thought of a Forgot the notifications, you're right, we should keep those at the time they are. |
This pull request incorporates many performance related changes that should lead to an overall simpler and more stable deployment.
Additionally many small changes improve code quality, fix deprecation warnings, and remove dependencies where possible.
Major changes
FileSystemStorage
for serving documents. This requires shared Docker volumes when using Docker swarm. We implemented these using NFS.Performance Analysis
I tested the performance of the static files storage using
hey
:The results (see plot below) show that even without caching, WhiteNoise served by Django itself delivers an overall better performance.
To avoid stress on the server, an NGINX cache is implemented. As WhiteNoise provides the
CompressedManifestStaticFilesStorage
, this is now possible without having to worry about cache invalidation. The old S3 storage backend did not have unique names for files that might have changed after updating.The cached version is faster and avoids load on the Django backend. The second bump comes from the initial round of requests when content was not yet cached.
Other changes