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

Faster post build startup: #22938

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Faster post build startup: #22938

merged 1 commit into from
Dec 13, 2024

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Dec 11, 2024

Fixes: mozilla/addons#15239

Description

  • use named volumes that mount faster and don't need renewing
  • skip docker compose healthcheck
  • add healthcheck script to initialize

Context

Faster post build by parallelizing initialize and startup

Testing

Verify time of startup

You can run the steps of the build independently to get a feel for relative times.

  1. Build: setup and building the image

Try building the image first without cache.

Warning

This is an aggressive command that will clear local images/volumes and docker state

make clean_docker

Make sure you don't have any dangling volumes or images.. cleaning up docker can be a chore....

time make up_pre

Check the time, then run the command again. The second time should be much, much faster.

Try it after changing a file in ./src this will invalidate some of the cache and give a more realistic cached time.

  1. Start: starting the containers

Now start the containers, running

time make up_start

The first time, it should take some time to download images. Run it again and it should be quite fast.

Note

Before running the second time, run make down so that the containers are not running but are pulled.

  1. Initialize: setting up container state and waiting to be healthy

Now run the initialize command to initialize the container state and ensure everything is healthy.

time make up_post

If you have a truly clean environment this could take some time as it must migrate and populate the database. Run it again and it should be relatively fast.

Verify no anonymous or unnamed volumes

You can independently verify the non existence of anonymous volumes via this query that returns volumes from all containers that are:

  • bind mount (anonymous volumes)
  • volume mount missing source (no name)
docker compose config | yq -r '.services.*.volumes[]? | select((.type == "bind") or (has("source") | not))'

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@KevinMind KevinMind marked this pull request as draft December 12, 2024 09:23
@KevinMind
Copy link
Contributor Author

KevinMind commented Dec 12, 2024

Command Uncached Time Cached Time Difference (s)
make up_pre 3:58.51 2.541 236.0
make up_start 2:15.34 3.822 131.5
make up_post 1:29.98 15.859 74.1
Total 7:43.83 22.222 441.6

@KevinMind KevinMind force-pushed the addons-15239 branch 2 times, most recently from 5d26929 to 84d2a02 Compare December 12, 2024 10:42
- Updated docker-compose files to use named volumes instead of bind mounts for better portability and consistency.
- Introduced a new healthcheck script to monitor the status of the Celery worker and web service.
- Removed deprecated healthcheck configurations from services in docker-compose.yml.
- Adjusted Makefile to better separate the up recipe and include new healtch check in initialize recipe
- Cleaned up Dockerfile by linking package.json and package-lock.json to the correct paths.
@KevinMind KevinMind requested review from a team and eviljeff and removed request for a team December 12, 2024 13:41
@KevinMind KevinMind marked this pull request as ready for review December 12, 2024 13:41
Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

I got a little lost during the test steps a few times so I don't really have comparable figures for all the steps, but my time make up after a make down and changing a file in `./src/ is

real 1m52.840s
user 0m2.189s
sys 0m1.734s

which is somewhat of an improvement - but not quite as speedy as your figures.

fwiw, [+] Building 45.6s (29/29) FINISHED - so the docker part is 45 seconds; the rest is the django initialization (even with no migrations, it takes a little bit of time) + es reindexing, pretty much.

@@ -65,7 +65,8 @@ jobs:
version: ${{ matrix.version }}
compose_file: ${{ matrix.compose_file }}
EOF
- uses: ./.github/actions/run-docker
- name: ${{ matrix.version == 'local' && 'Uncached Build' || 'Pull' }} Check
Copy link
Member

Choose a reason for hiding this comment

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

Does the syntax support brackets? It's not immediately obvious to me (or future me) if it's doing (A and B) or C or A and (B or C)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty standard JS syntax afaik (GHA operators), it's the first one. The statements are evaluated left to right.

image

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's 'Uncached build' if matrix.version == 'local' else 'Pull' in python syntax

@KevinMind
Copy link
Contributor Author

I got a little lost during the test steps a few times so I don't really have comparable figures for all the steps, but my time make up after a make down and changing a file in `./src/ is

real 1m52.840s user 0m2.189s sys 0m1.734s

which is somewhat of an improvement - but not quite as speedy as your figures.

fwiw, [+] Building 45.6s (29/29) FINISHED - so the docker part is 45 seconds; the rest is the django initialization (even with no migrations, it takes a little bit of time) + es reindexing, pretty much.

Yeah the goal of this PR is to make the 2 and 3 of the 3 steps faster. It doesn't really impact the build itself. The "better" way of measuring the impact here would be to make the changes to the make recipes first, then time the original state, and then add the changes on this patch so you can get per phase timing... It's faster. At the end of this epic I plan to test the July state of the code to the end state and we can get some real data on the impact.

@KevinMind KevinMind merged commit 46f608f into master Dec 13, 2024
36 checks passed
@KevinMind KevinMind deleted the addons-15239 branch December 13, 2024 18:12
chrstinalin pushed a commit that referenced this pull request Jan 10, 2025
- Updated docker-compose files to use named volumes instead of bind mounts for better portability and consistency.
- Introduced a new healthcheck script to monitor the status of the Celery worker and web service.
- Removed deprecated healthcheck configurations from services in docker-compose.yml.
- Adjusted Makefile to better separate the up recipe and include new healtch check in initialize recipe
- Cleaned up Dockerfile by linking package.json and package-lock.json to the correct paths.
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.

Faster docker compose up
2 participants