This repository was archived by the owner on Feb 22, 2023. It is now read-only.
Use pm2 in production docker image#1331
Merged
sarayourfriend merged 1 commit intomainfrom May 10, 2022
Merged
Conversation
sarayourfriend
added
🟩 priority: low
Contributor
Author
|
@WordPress/openverse-developers can y'all take a look at this and see if it makes sense and works for you? It's a necessary step for getting our production deployment to use docker. |
Contributor
AetherUnbound
left a comment
AetherUnbound
left a comment
There was a problem hiding this comment.
When I tried building locally, I got:
Peer dependencies that should be installed:
@babel/core@">=7.11.0 <8.0.0"
@nuxt/vue-app@^2.15
@testing-library/dom@>=7.21.4
@types/node@"*"
babel-loader@">=8.0.0 <9.0.0"
consola@"*"
react-dom@">=16.8.0 <17.0.0 || >=17.0.1 <18.0.0"
react@">=16.13.1 <17.0.0 || >=17.0.1 <18.0.0"
vue-template-compiler@">=2.6.10 <3.0.0"
vue@">=3.0.0-0 <3.0.0 || >=2.6.14 <3.0.0"
webpack@">=4.36.0 <5.0.0 || >=5.0.0-0 <5.0.0 || >=5.0.0 <6.0.0"
The command '/bin/sh -c pnpm install --store=pnpm-store' returned a non-zero code: 1
Also you may need to add a . at the end of the suggested build command, when I tried to run that directly Docker complained there were no arguments.
Dockerfile
Outdated
Comment on lines
24
to
30
| COPY src /usr/app/src | ||
| COPY typings /usr/app/typings | ||
| COPY nuxt-template-overrides /usr/app/nuxt-template-overrides | ||
| COPY tsconfig.json /usr/app/tsconfig.json | ||
| COPY nuxt.config.ts /usr/app/nuxt.config.ts | ||
| COPY tailwind.config.js /usr/app/tailwind.config.js | ||
| COPY tailwind.safelist.txt /usr/app/tailwind.safelist.txt |
Contributor
There was a problem hiding this comment.
Would it be easier to specify a .dockerignore file with explicit allow statements and just run COPY . /usr/app? That's what we do in the API
Contributor
Author
|
@AetherUnbound Are you using BuildKit by any chance? And if not, did you notice if it failed during the |
Contributor
|
Here's the full logs: Error LogThis happens both when I run |
Contributor
Author
|
Ah, that's the issue being fixed by #1385, this PR will need to be re-written once that merges anyway (it'll simplify this one a lot too). I'll draft this PR until then! |
sarayourfriend
force-pushed
the
add/pm2-docker-target
branch
from
1c23a9c to
238e44c
Compare
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/1331 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
Contributor
Author
|
This is ready for review now cc @AetherUnbound @dhruvkb and @obulat. |
|
Size Change: +5 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
5 tasks
Member
|
What is pm2 needed for? |
Contributor
Author
|
To get the benefits from We already run it with pm2 in production, so this is just making the docker image inline with the production ec2 deployment. |
Member
|
@krysal PM2 also handles simpler things like automatically restarting the
A similar, unsupported library is forever. I don't know how common it is to use libraries like this anymore, to be honest, but it seems good for us to be consistent with prod for now and we can figure out deprecating pm2 later if beneficial. Footnotes |
zackkrida
approved these changes
May 9, 2022
Member
zackkrida
left a comment
zackkrida
left a comment
There was a problem hiding this comment.
Working well locally! Nice.
Contributor
Author
|
All of those things you mentioned, Zack, are also features of Kubernetes, ECS, etc. I agree it's good to just align with the existing deployment and then re-evaluate. One thing I'm not sure of is whether FARGATE even has more than one CPU core in the configuration we'd use to run the cluster mode on. |
krysal
approved these changes
May 9, 2022
Member
krysal
left a comment
krysal
left a comment
There was a problem hiding this comment.
Thanks for the explanations, @sarayourfriend @zackkrida. Feels better knowing the context 😄 The image continues to run on macOS 👍
Contributor
|
Hmm, I still can't seem to get this to build locally 😞 Be that as it may, if this works for the frontend devs then feel free to merge now 🙂 Docker build error |
Contributor
Author
|
@AetherUnbound Are you using BuildKit? I think it might only work under BuildKit currently. @krysal had some ideas of how to make it work more generally though. We skipped fixing that for now because it's just meant to be built in CI 99% of the time anyway. |
Contributor
I was not! I tried with |
Contributor
Author
|
Are you running the build with the If so, can you make sure you're also pulled down on the latest version of the branch? The build runs successfully in CI, FWIW. |
obulat
approved these changes
May 10, 2022
Contributor
obulat
left a comment
obulat
left a comment
There was a problem hiding this comment.
I can confirm that Mac cannot build this image, but can run the gchr container.
Is this supposed to run in a cluster of 4 (what?). I get the log saying that it's running in a cluster mode, and there are 4 repeated messages saying that Nuxt app is running and listening (on the same port).
Contributor
Author
|
@obulat by default pm2 in cluster mode runs one instance per available CPU core I think. The configuration is copied from the production EC2 configuration. |
Contributor
Okay, I'm not sure what the issue was but I tried it again on a different computer off of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR runs the application in Docker with pm2.
We do this so we can get the benefits of cluster mode as described here: https://pm2.keymetrics.io/docs/usage/cluster-mode/
Our current production deployment already uses pm2, so this is making our docker file inline with the production configuration (and therefore production ready for a dockerfied deployment).
Testing Instructions
If you're on a Mac or Windows PC then you probably won't be able to build the image. In that case, wait until the docker image push action is finished and use the image from GHCR.
If you're on Linux (or WSL) you build the image locally following the instructions in the README:
The run
docker run -it -p 127.0.0.1:8443:8443/tcp openverse-frontend:latestand verify that you can access the app locally on port8443.If you're running the version from GHCR, replace
openverse-frontend:latestin the run command above with the fullghcr.io/wordpress/openverse-frontend:sha-{commit}tag that is generated by the action. You can find the full image name at the end of the action output.Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin