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

Move /deps/node_modules to /data/olympia/node_modules #22955

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Dec 20, 2024

Fixes: mozilla/addons#15066

Description

Enhance the reliability and performance of the make up flow, simplifying development and production environments by making debugging easier.

  • Simplified Docker Setup:
    Removed the OLYMPIA_MOUNT concept, always mounting host files.
    Moved Python and npm dependencies into the /data/olympia directory.

  • New Scripts:
    sync-host-files.py: Synchronizes host files with container files before starting containers.
    compile_locales.py: Replaces the previous compile-mo.sh.
    update_assets.py: Manages asset updates.

  • Nginx Enhancements:
    Added headers to nginx locations to indicate the source of static files.

  • Initialization Improvements:
    Ensured storage directories exist during initialization.

  • GitHub Actions Modifications:
    Modified CI workflows to align with the revised setup.
    Updated configuration files to reflect new paths for dependencies and static assets.

  • Testing:
    Updated tests to validate static file routing from nginx to olympia.
    Ensured file changes in the container reflect on the host and vice versa.
    Cleaned up redundant tests in the check_*.

  • Documentation:
    Updated documentation to describe these changes.

Context

The inspiration for this PR was #22957 which introduces vite which in turn has a strong preference that node_modules is in the same directory as the root /data/olympia. The other changes were the result of finding small bugs or inconsistencies after adding vite support. These changes are pre-requisites for adding vite support and generally improve the local development setup.

This PR contains a number of improvements to reliability and performance of our make up flow. Some of these could probably be shipped independently but I'm not sure it is really worth it.

The key change is there is now no concept of mounting or not mounting the host directory, it is always mounted. This simplifies the architecture of our docker compose project A WHOLE DANG LOT, because we don't need to decide whether to mount or not and the developer doesn't need to wonder if host changes are going to propagate to the container or not. Additionally the files you see in your local file system are "literally" the files in the container. This makes debugging a lot easier.

The draw back is when you want to run a "true" production version you need to checkout the right code and clean up your local fs.. this can be done either via a script (out of scope) or manually by checking out the right commit and cleaning your files.

Testing

Running make up should work and you should be able to run the application.

make up DOCKER_TARGET=development
make up DOCKER_TARGET=production
  • Expect any file changes in the container in /data/olympia to reflect on the host and vice versa.
  • Expect assets and locales to be built and synced to host in production mode.
  • expect static file routing to work and headers returned showing which component is serving the file.
  • Expect to control dependencies as before with OLYMPIA_DEPS

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 force-pushed the vite branch 4 times, most recently from f8c9504 to 097b07d Compare December 22, 2024 20:39
@KevinMind KevinMind force-pushed the vite branch 19 times, most recently from a75e58a to bea5c38 Compare January 1, 2025 22:26
@KevinMind KevinMind force-pushed the vite branch 7 times, most recently from f61d7d8 to b217496 Compare January 6, 2025 15:55
@KevinMind KevinMind force-pushed the vite branch 13 times, most recently from 8e76203 to e0a90e7 Compare January 7, 2025 13:16
…ing make up

- Replaced the locale compilation script from a shell script to a Python script (compile_locales.py) for better error handling and parallel processing.
- Updated the update_assets target in Makefile-docker to use the new update_assets.py script.
- Removed the obsolete compile-mo.sh script.
- Introduced sync_host_files.py to streamline dependency updates and asset synchronization.
- Removed obsolete file checks and user validation from Makefile-docker.
- Updated Nginx configuration to improve static file handling and added headers for better traceability.
- Refactored storage management commands in the Python codebase:
  - Renamed `clean_storage` to `make_storage` for clarity and added a `clean` parameter.
  - Updated command implementations to use the new `make_storage` method.
- Introduced a new system check for Nginx configurations to ensure proper file accessibility and response validation.
- Updated docker-compose.yml to mount dependencies in /data/olympia/deps for better organization.
- Modified Dockerfile to set environment variables for dependency directories and ensure proper ownership.
- Adjusted Makefile-docker to remove NODE_MODULES variable and streamline npm commands.
- Updated documentation to reflect changes in dependency paths.
- Refactored install_deps.py to clean up dependency directories and removed obsolete package.json copying logic.
- Updated settings_base.py to reference new dependency paths.
- Updated docker-compose.yml to simplify volume mounts by using the current directory for all services.
- Removed HOST_MOUNT and HOST_MOUNT_SOURCE environment variables from settings and entrypoint scripts, streamlining the configuration for local and production environments.
- Refactored setup.py to eliminate the get_olympia_mount function, replacing it with a more straightforward approach to determine the Docker target.
- Adjusted GitHub Actions workflows to remove references to the obsolete mount input, ensuring cleaner CI/CD processes.
- Enhanced test cases to reflect changes in Docker configuration and environment variable handling.
@KevinMind KevinMind requested review from a team and diox and removed request for a team January 7, 2025 15:08
@KevinMind KevinMind marked this pull request as ready for review January 7, 2025 15:08
import subprocess


def main():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @diox we discussed this recently. This logic is how we can ensure that the state of the container and host are synced even with mounts and regardless of mode, because on production images we reinstall deps fresh, build locales and assets essentially reflecting the state of the production image (those exact commands are run in the image build)

@diox
Copy link
Member

diox commented Jan 10, 2025

Running make up DOCKER_TARGET=development I eventually got:

SystemCheckError: System check identified some issues:

ERRORS:
?: (setup.E010) Unknown error accessing /data/olympia/src/olympia/../../site-static/test.txt via http://nginx/static/test.txt: [Errno 13] Permission denied: '/data/olympia/src/olympia/../../site-static/test.txt'

@KevinMind
Copy link
Contributor Author

Running make up DOCKER_TARGET=development I eventually got:

SystemCheckError: System check identified some issues:

ERRORS:
?: (setup.E010) Unknown error accessing /data/olympia/src/olympia/../../site-static/test.txt via http://nginx/static/test.txt: [Errno 13] Permission denied: '/data/olympia/src/olympia/../../site-static/test.txt'

Can you check the container id and the owner of site-static?

Can you try make down and make up together?

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.

[Task]: Make up is, like, better
2 participants