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

Embed the production run command in the dockerfile rather than relying on ECS to override it #2785

Closed
sarayourfriend opened this issue Aug 7, 2023 · 6 comments · Fixed by #2808
Assignees
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: api Related to the Django API
Milestone

Comments

@sarayourfriend
Copy link
Collaborator

Problem

Stems from #2566

Currently, the API dockerfile embeds the local dev run command rather than the production command. This makes it difficult to deploy changes to the production runtime command because we need to do it in two (three) steps instead of one. Under the current approach, if we need to make a significant change to the API live environment command, we need to do the following:

  1. Prepare the API with the code to handle both the existing and new run command and deploy the code that can handle both
  2. Update the task definition template to use the new run command, then deploy the API again
  3. Clean up anything that existed to support the previous run command

Description

We can significantly simplify this by embedding the production run command in the image. This doesn't completely solve the problem, as there are still relevant variables that are only present in the task definition (such as CPU, relevant to worker count) but it eliminates the more likely scenario where we're just changing some settings on the run command (or switching runners, e.g., for ASGI conversion).

To complete this ticket:

  1. Add command: python manage.py runserver 0.0.0.0:8000 to the web service in the docker-compose.yml
  2. Update RUN in api/Dockerfile for the Django image to RUN ["gunicorn"]
    • Passing configuration variables present in the current production runtime (bind, workers, chdir, etc) is unnecessary because all of that is present in the gunicorn.conf.py

Afterwards, we must also remove command from the API task definition (both in concrete/api and concrete/api-thumbnails) so that the task definition doesn't overwrite the image's run command.

This issue should only be closed once both the API codebase and the task definition template are updated.

@sarayourfriend sarayourfriend added 🟧 priority: high Stalls work on the project or its dependents 💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🧱 stack: api Related to the Django API labels Aug 7, 2023
@sarayourfriend sarayourfriend added this to the Django ASGI milestone Aug 7, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Openverse Backlog Aug 7, 2023
@sarayourfriend sarayourfriend self-assigned this Aug 7, 2023
@sarayourfriend sarayourfriend moved this from 📋 Backlog to 🏗 In progress in Openverse Backlog Aug 8, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Openverse Backlog Aug 16, 2023
@github-project-automation github-project-automation bot moved this from ✅ Done to 📋 Backlog in Openverse Backlog Aug 16, 2023
@sarayourfriend
Copy link
Collaborator Author

Reopening the issue to track the required infrastructure work. We will close this once the changes to the infrastructure described in the issue are deployed.

@sarayourfriend sarayourfriend moved this from 📋 Backlog to 🏗 In progress in Openverse Backlog Aug 16, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Openverse Backlog Aug 21, 2023
@sarayourfriend
Copy link
Collaborator Author

sarayourfriend commented Aug 21, 2023

The API task definition templates are updated and I am triggering a production redeployment now. That will officially close this issue.

API: https://github.com/WordPress/openverse-infrastructure/actions/runs/5920419471
Thumbnails: https://github.com/WordPress/openverse-infrastructure/actions/runs/5920420451

Both of the above links require access to the private infrastructure repository.

@sarayourfriend
Copy link
Collaborator Author

@dhruvkb Before deploying the API this week, can you update the task definition templates by applying main in next/production?

@github-project-automation github-project-automation bot moved this from ✅ Done to 📋 Backlog in Openverse Backlog Aug 21, 2023
@dhruvkb
Copy link
Member

dhruvkb commented Aug 21, 2023

@sarayourfriend ACK. Thanks for the heads up. Why does the API deploy need next/production to be updated?

Update: the API deployment on Tuesday (22 Aug 2023) was completed without a tf apply, using only the 'Release App' workflow.

@sarayourfriend sarayourfriend moved this from 📋 Backlog to 🏗 In progress in Openverse Backlog Aug 21, 2023
@sarayourfriend
Copy link
Collaborator Author

Dhruv and I spoke about this and we changed the approach. Now that the API is deployed to production with the image that has the baked in run command, we can deploy this change in Terraform again without causing the issues we saw earlier. I'll do that today.

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Openverse Backlog Aug 25, 2023
@sarayourfriend
Copy link
Collaborator Author

Applied and deployed to production, this work is finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: api Related to the Django API
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants