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

Mutagen optimization #63

Merged
merged 34 commits into from
Apr 21, 2020
Merged

Mutagen optimization #63

merged 34 commits into from
Apr 21, 2020

Conversation

yeegor
Copy link
Contributor

@yeegor yeegor commented Feb 19, 2020

Bring the project up using Mutagen.
TODO: in closest time make docker-compose files to be able to bring the project up without Mutagen.

- Add volume core-frontend-data for core frontend container
- Implement mutagen configuration for both core and frontend dc configs
- FIx node container not having wget
- Keep maildev configuration in local setup only
Make them be mounted into frontend container on deploy
instead of copied on build
Optimize sync making it two-way-resolved
Propagate www-data as default file owner
@yeegor yeegor requested a review from eli-l February 19, 2020 15:51
@yeegor
Copy link
Contributor Author

yeegor commented Feb 21, 2020

For now I am unable to install backend module using this setup. Investigating.

- remove nginx session by introducing named volume
- extend mutagen permission config with default file mode
build/php/Dockerfile Outdated Show resolved Hide resolved
deploy/shared/conf/local-ssl/ca.conf Outdated Show resolved Hide resolved
deploy/start-demo.sh Outdated Show resolved Hide resolved
deploy/start.sh Outdated Show resolved Hide resolved
@alfredsgenkins
Copy link
Contributor

Please try adding container_name: my-web-container for all setvices docker-compose.mutagen.yml or in docker-compose.local.yml to resolve naming issue. Note: this prevents from scaling.

@alfredsgenkins
Copy link
Contributor

Please link a doc for this, otherwise Ilja won't review this.

@alfredsgenkins
Copy link
Contributor

@eli-l eli-l self-assigned this Feb 24, 2020
@eli-l eli-l added the enhancement New feature or request label Feb 24, 2020
build/frontend/start.sh Outdated Show resolved Hide resolved
deploy/start-demo.sh Outdated Show resolved Hide resolved
deploy/start-with-frontend.sh Outdated Show resolved Hide resolved
deploy/start.sh Outdated Show resolved Hide resolved
@@ -53,7 +54,7 @@ services:
volumes:
- ./deploy/shared/conf/nginx/magento.conf:/etc/nginx/conf.d/default.conf
- ./deploy/shared/conf/nginx/cache-router.conf:/etc/nginx/conf.d/cache-router.conf
- app-data:/var/www/public
- nginx-data:/var/www/public/pub
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason of introducing new volume? Is that mutagen limitation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not mutagen-related, but apparently it is faster to sync many small named volumes with their containers than one huge app-data volume with many containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not the only volume introduced, see core-frontend-data.
I also wanted to create a frontend-data volume to mount it into the frontend container, but the issues occur because we then would need to mount vendor and app/design/frontend/scandiweb/pwa folders separately and vendor doesn't want to be mounted separately for some reason :(

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not mutagen-related, but apparently it is faster to sync many small named volumes with their containers than one huge app-data volume with many containers.

Ok, means that you prefer "faster sync" to developer, who is adding additional file and want it to be served directly from nginx, to spend TONS of time to find out it is not mounted to nginx? This approach is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current solution considers mounting app-data into nginx container. This way we will not have one more docker-compose file. Is there any difference in mounting app-data volume into nginx or creating a bind mount? Additional sync sessions will be slower than direct named volume mount, should I create them between nginx and local fs instead of mounting app-data volume into nginx?

to resolve container naming issue in yml files
@yeegor
Copy link
Contributor Author

yeegor commented Feb 24, 2020

Known issue: Currently I am experiencing problems with configuring Xdebug on my Mac using this configuration.
Investigating.

@yeegor
Copy link
Contributor Author

yeegor commented Feb 24, 2020

Try specifying user in docker-compose like this: https://docs.docker.com/compose/compose-file/#userns_mode#domainname-hostname-ipc-mac_address-privileged-read_only-shm_size-stdin_open-tty-user-working_dir

When I refer to user 'user' by name, the container does not start.
When I refer to user 'user' by UID 1000, I get the following error:

mutagen-powered-app | Starting with UID : 0
mutagen-powered-app | useradd: Permission denied.
mutagen-powered-app | useradd: cannot lock /etc/passwd; try again later.
mutagen-powered-app | error: failed switching to "user": unable to find user user: no matching entries in passwd file

@@ -8,9 +8,4 @@ ARG BASEPATH=/var/www/public
# Set working directory so any relative configuration or scripts wont fail
WORKDIR $BASEPATH

Copy link
Contributor

Choose a reason for hiding this comment

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

Why these scripts were removed? These files contains deployment logic, not application version/dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returned a single script to build frontend container

@@ -1,4 +1,12 @@
#!/bin/bash

echo "Waiting for initial files to be synced"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be more up to enforce re-building when dependencies are changed, rather than to use such a hacky solution. It is needed not because Docker is not-to-good, but because we are moving away from it's best practices to far.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you propose to do install dependencies during the frontend container build and remove npm ci from this script at all, leaving it with a single purpose of launching pm2-runtime? Then we can get rid of these deployment scripts of frontend container and do that with docker-compose file command: ['npm', 'run', 'pm2-watch'] directive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm ci was moved to frontend container build script.

build/frontend/start.sh Outdated Show resolved Hide resolved
build/frontend/start.sh Show resolved Hide resolved
build/rendertron/Dockerfile Show resolved Hide resolved
deploy/start-demo.sh Show resolved Hide resolved
@@ -286,6 +284,12 @@ function exit_catch {

### Deploy pipe start

echo "${blue}${bold}Waiting for Mutagen to sync initial filese${normal}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Composer.json/lock is always in a container, as these are added during the build.
Does mutagen add additional layer that is empty on top of containers FS, similar to local mount? Even so - this behavior is related only to local.

Copy link
Contributor

Choose a reason for hiding this comment

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

For mutagen - use another start scripts, hopefully we'll unify it one day.

Copy link
Contributor

@eli-l eli-l left a comment

Choose a reason for hiding this comment

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

Global comment:

  • pay attention and rework workdirs, don't rework/redefine them unless absolutely necessary for Mutagen (never?)
  • source code must be shared between nginx and app containers, no matter how you run it (no mounting, docker native, mutagen), it is done for a reason.
  • revisit all the changes (all existing files that have been edited by you), and unless this is absolutely good for ALL environments and use cases - move these changes to a separate file (even it is 300 lines long).
  • remove changes to the existing services, that are not related to your task

@@ -286,6 +284,12 @@ function exit_catch {

### Deploy pipe start

echo "${blue}${bold}Waiting for Mutagen to sync initial filese${normal}"
Copy link
Contributor

Choose a reason for hiding this comment

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

For mutagen - use another start scripts, hopefully we'll unify it one day.

deploy/start-with-frontend.sh Outdated Show resolved Hide resolved
deploy/start.sh Outdated Show resolved Hide resolved
deploy/start.sh Outdated
@@ -264,6 +263,13 @@ function exit_catch {

### Deploy pipe start

echo "${blue}${bold}Waiting for Mutagen to sync initial filese${normal}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the default file - nothing related to local, mutagen or other side-cases must be added here.
create deploy/mutagen directory, put there everything related to Mutagen sync, anyway you have mutagen dedicated docker-compose files, controlling start script is extremely easy there.

@@ -14,9 +14,15 @@ services:
- NODEJS_VERSION=${NODEJS_VERSION}
volumes:
- ./src/localmodules/base-theme:/var/www/public
command: ["npm", "run", "dev-server-core"]
- ./build/frontend/start-core.sh:/start-core.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Build file into container, do not mount it from the host.

@alfredsgenkins alfredsgenkins changed the base branch from 2.x-stable to 2.x-mutagen April 21, 2020 15:22
@alfredsgenkins alfredsgenkins merged commit ae0df1c into scandipwa:2.x-mutagen Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants