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

Docker Deployment for Kelvin core #504

Merged
merged 12 commits into from
Oct 5, 2024

Conversation

JersyJ
Copy link
Contributor

@JersyJ JersyJ commented Sep 21, 2024

Issue

Part of #503.

Summary

Introduces ability to deploy the Kelvin application (except Django RQ workers) inside Docker with docker-compose.yml (profile "prod"). The deployment includes the following networked services:

  • The Django backend
  • nginx, which will serve the Django backend
  • Postgres (DB)
  • Redis (cache)

Added pre-commit
Introduced additional variables for deployment configuration
Created Dockerfile for the core app
Refactored docker-compose.yml for local development and production deployment with profile "prod" (docker compose --profile prod up)
Created asgi.py for ASGI web servers
Added CSRF trusted domains for reverse proxy (NGINX) usage
Included example NGINX configuration

Testing

Needs Proper Testing! I have tested the basics. Someone with more experience should conduct thorough tests.

Nginx

To run Nginx, you need to place privkey.pem and fullchain.pem in the appropriate folder. For testing purposes, you can generate them via openssl :
openssl req -x509 -nodes -days 365 -newkey rsa:2048 \ -keyout ./privkey.pem \ -out ./fullchain.pem \ -subj "/C=US/ST=YourState/L=YourCity/O=YourOrganization/CN=localhost".

Remaining Tasks:

  • The setup of Django RQ workers is missing

Copy link
Collaborator

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great! Left a few comments inline.

Maybe to clarify a bit, I think that we don't want to commit too many specific things about the deployment into git (e.g. the nginx config). We have some configuration files on the existing server and we'll likely port them over manually, and these will differ on each specific production deployment anyway. The goal is to have Dockerfile + docker-compose.yml prepared in a way so that it can be deployed easily, and that it can be configured, i.e. I can pass it nginx config/certificates/log dir, local_settings.py, paths where Postgre and Redis data should be stored etc.

docker-compose-local.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
kelvin/settings.py Outdated Show resolved Hide resolved
@Kobzol
Copy link
Collaborator

Kobzol commented Sep 21, 2024

Uhh, I only reviewed the first commit, sorry :) I'll take a look at the rest.

@JersyJ
Copy link
Contributor Author

JersyJ commented Sep 21, 2024

Added pre-commit to avoid future CI issues before pushing to remote.

kelvin/settings.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok now I saw all the commits and it makes more sense 😆 Sorry.

In general, this already has pretty much almost everything that I wanted, so that is awesome :) It would be nice to de-duplicate the docker-compose files and maybe simplify the Dockerfile a bit (it seems a bit overkill to me at the moment), but otherwise this looks really good.

I hope that the asgi changes won't affect the normal wsgi operation on the current production deployment, especially since the semester starts in two days 😆

kelvin/settings.py Outdated Show resolved Hide resolved
kelvin/urls.py Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Copy link
Collaborator

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to deploy the website locally using Docker compose and it actually works!! ❤️ That's really awesome 😆

Left a few more comments.

When I build the image locally, it prints this:

useradd warning: uvicorn's uid 1001 is greater than SYS_UID_MAX 999

Maybe we should use an UID below 1000, just to be sure?

Dockerfile Show resolved Hide resolved
nginx.conf Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@JersyJ
Copy link
Contributor Author

JersyJ commented Sep 22, 2024

Maybe we should use an UID below 1000, just to be sure?

True, yes we should. Changed.

Also changed to WSGI, since I looked through the views and it would be better for them to have WSGI.

Also moved the ruff.toml to pyproject.toml, to get rid of files in the root of the project.

@Kobzol
Copy link
Collaborator

Kobzol commented Sep 23, 2024

Thank you very much for working on this :) This looks really great now. Since it's the first week of the semester, I will delay merging until the weekend, because this also has some changes that could in theory affect the production deployment, which is.. brittle, to say the least :)

@Kobzol
Copy link
Collaborator

Kobzol commented Oct 1, 2024

We had some issues with Kelvin over the weekend and had to do a bunch of reverts, so I didn't merge this yet. I'll try it once Kelvin has a bit less traffic.

@geordi
Copy link
Member

geordi commented Oct 2, 2024

We can also "test deploy" it on the development server. Just saying.

@JersyJ
Copy link
Contributor Author

JersyJ commented Oct 3, 2024

If you want some help setting up a proper deployment, feel free to reach out to me.

@JersyJ JersyJ force-pushed the 503-migration-to-docker branch from 0169a90 to 5018188 Compare October 4, 2024 21:02
Kobzol added 2 commits October 5, 2024 15:42
It should not be needed at the moment.
@Kobzol
Copy link
Collaborator

Kobzol commented Oct 5, 2024

I took the liberty of re-creating the requirements.txt file and removing the asgi.py file, which should not be needed, to reduce the change of causing some issues on production. Let's see if everything still works after this is merged :)

Copy link
Collaborator

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much!

version = "3.8.0"
source = { registry = "https://pypi.org/simple" }
dependencies = [
{ name = "cfgv" },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the heck does pre-commit have all these dependencies.. sigh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-commit is powerful tool and is designed to handle much wider range of tools and use-cases. I added it as a dev dependency, so it shouldn’t be an issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently install deps using uv pip sync because of some legacy reasons, so it installs everything 😅 But it doesn't really matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my Dockerfile I added parameter which excludes them. So after you switch , no more dev dependencies 😅

@Kobzol Kobzol merged commit a1e8068 into mrlvsb:master Oct 5, 2024
2 checks passed
@Kobzol
Copy link
Collaborator

Kobzol commented Oct 5, 2024

Looks like everything went through, yay 🎉 The next step is for someone to SSH to the new server and try to deploy Kelvin there using Docker.. :)

@JersyJ
Copy link
Contributor Author

JersyJ commented Oct 5, 2024

I took the liberty of re-creating the requirements.txt file and removing the asgi.py file, which should not be needed, to reduce the change of causing some issues on production. Let's see if everything still works after this is merged :)

Okk, maybe the next step would be to get rid of the requirements.txt after switching to Docker deployment :)

@Kobzol
Copy link
Collaborator

Kobzol commented Oct 5, 2024

Yeah, of course :) requirements.txt is there only as an artifact to keep compatibility with our old virtualenv, which I did not want to recreate. Once we use uv full and/or switch to a Dockerized deployment, we should nuke it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants