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

Remove race conditions in docker volumes during build/runtime #22912

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Dec 2, 2024

Fixes: mozilla/addons#15046

Description

  • creates base "olympia" service responsible for mounting shared docker volumes between services
  • adds olympia as a dependency to any service using a shared volume.

Context

It is not feasible to test a race condition like this, so instead the configuration and runtime checks are setup in a way to virtually guarantee:

  • no volume used by more than one container can be created by any container other than "olympia"
  • the correct volumes are used based on the given input parameters
  • the compose project and make up can run in any of the given configurations and pass the basic "check" command

This comes here where tests are currently failing due to missing this patch.

Testing

There isn't really a good way to test this PR outside of adding the commit from the other patch, and then running the test_setup tests to verify statically the configuration is valid.

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.

docker-compose.yml Outdated Show resolved Hide resolved
@KevinMind KevinMind force-pushed the addons-15046 branch 2 times, most recently from 1befc19 to 1525885 Compare December 6, 2024 12:46
@KevinMind KevinMind requested a review from diox December 6, 2024 12:46
@KevinMind
Copy link
Contributor Author

KevinMind commented Dec 6, 2024

@diox there is a hypotehtical even more stop gap solution that would remove the named data_olympia_prod/dev with the original anonymous volume. I could do that, but it felt like that goes well with this topic.. 🤷

Edit: Split to 3 commits:

  • add the tests to verify CI without any other code changes (workflow run)
  • add the minimal changes to fix race conditions (workflow run)
  • add the olympia mount to properly fix with additional checks

@KevinMind KevinMind force-pushed the addons-15046 branch 23 times, most recently from 1b7905e to b8f67ae Compare December 9, 2024 11:31
@KevinMind KevinMind force-pushed the addons-15046 branch 4 times, most recently from d8b2979 to d756e11 Compare December 9, 2024 13:14
- Renamed 'worker' service to 'olympia' in docker-compose.ci.yml and docker-compose.yml for clarity.
- Introduced a new 'olympia' service with a persistent volume for site static files.
- Updated 'worker' service to extend from 'olympia' and adjusted volume mappings accordingly.
- Added dependency for 'nginx' service on 'olympia' to ensure proper startup order.
- Changed base image for pip development stage in Dockerfile to improve build consistency.

This commit enhances the organization of Docker services and improves the overall structure of the Docker setup.
Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

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

Since this is the stopgap, can we reference the ideal solution somewhere ? Maybe in the Dockerfile next to the line you're changing ?

docker-compose.ci.yml Outdated Show resolved Hide resolved
@KevinMind
Copy link
Contributor Author

Since this is the stopgap, can we reference the ideal solution somewhere ? Maybe in the Dockerfile next to the line you're changing ?

It's a moving target so I'll hold off on that.

@KevinMind KevinMind merged commit b8a0aa4 into master Dec 9, 2024
31 checks passed
@KevinMind KevinMind deleted the addons-15046 branch December 9, 2024 19:12
chrstinalin pushed a commit that referenced this pull request Jan 10, 2025
* Prevent race conditions in docker volume mounts

- Renamed 'worker' service to 'olympia' in docker-compose.ci.yml and docker-compose.yml for clarity.
- Introduced a new 'olympia' service with a persistent volume for site static files.
- Updated 'worker' service to extend from 'olympia' and adjusted volume mappings accordingly.
- Added dependency for 'nginx' service on 'olympia' to ensure proper startup order.
- Changed base image for pip development stage in Dockerfile to improve build consistency.

This commit enhances the organization of Docker services and improves the overall structure of the Docker setup.
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.

[Bug]: Race condition mounting static build files to web/worker containers
2 participants