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

feat: Dockerfile! (devcontainer + production) #205

Merged
merged 18 commits into from
Sep 9, 2024

Conversation

jamesread
Copy link
Contributor

@jamesread jamesread commented Sep 7, 2024

What kind of change does this PR introduce?

This is an attempt at the highly requested dockerfile. It's likely to need a few tweaks before merging, but it builds and runs on my machine.

I did try and copy the raw repo over to Docker and do a npm install, but because of the 2Gb of node dependencies (!!!), it actually starts to issue out of file descriptor errors (186k files!). Therefore, it is necessary to do a npm install on the host first, then that gets copied over with the build context, and then the nx build completes without issue.

Things on the todo list;

  • I tried for quite a while to inline the node_module dependencies as part of the npm run build, but whatever settings I changed in package.json, the output was always the same. I think this would be good to do, so that we don't have to ship a 2Gb node_modules directory, and just the stuff the build /actually/ depends on.
  • Implement supervisord healthchecks.
  • Test both containers like crazy!
  • Split out database migrations, and pause gracefully until migrations are ready. At the moment you'll have to do the migrations sepately until this gets pulled out. If you accept my other PR for the config:check via the commands app, I think it would be good to add a db:migrate as a command, or similar.

Why was this change needed?

Erm, some people asked for it :-)

Copy link

vercel bot commented Sep 7, 2024

@jamesread is attempting to deploy a commit to the Listinai Team on Vercel.

A member of the Team first needs to authorize it.

@nevo-david
Copy link
Contributor

This is great 🎉 but a few things:
We need to decide if it's for production or dev.
If it's dev, you don't have to build or copy the directories. All you need to do is run npm run dev.
If it's production, then after the npm run build, we need four entry points:

  1. npm run start:prod
  2. npm run start:prod:frontend
  3. npm run start:prod:workers
  4. npm run start:prod:cron

In that case, I think we would have to run docker-compose.

@jamesread
Copy link
Contributor Author

This is great 🎉 but a few things:
We need to decide if it's for production or dev.
If it's dev, you don't have to build or copy the directories. All you need to do is run npm run dev.
If it's production, then after the npm run build, we need four entry points:

  1. npm run start:prod
  2. npm run start:prod:frontend
  3. npm run start:prod:workers
  4. npm run start:prod:cron

In that case, I think we would have to run docker-compose.

Heya, many thanks. This is certainly for production/users, as for Dev it's too slow to keep rebuilding the container (the build takes several minutes due to the number of files that need to be copied).

I was being lazy during testing last night, so I set the ENTRYPOINT to "npm run dev" and forgot to chbsge it before committing.

You can only have one ENTRYPOINT in a Dockerfile, so I'll find a way to start these 4 in parallel.

Many many thanks for the comments from @jonathan-irvin too, I see you were interested in this from another discussion but were unsure how to do the npm/nx stuff I believe - that toolchain is also new to me, so this was my best effort! I've tried to address your comments as best I can.

@scottgrobinson
Copy link

This is great 🎉 but a few things:
We need to decide if it's for production or dev.
If it's dev, you don't have to build or copy the directories. All you need to do is run npm run dev.
If it's production, then after the npm run build, we need four entry points:

  1. npm run start:prod
  2. npm run start:prod:frontend
  3. npm run start:prod:workers
  4. npm run start:prod:cron

In that case, I think we would have to run docker-compose.

Heya, many thanks. This is certainly for production/users, as for Dev it's too slow to keep rebuilding the container (the build takes several minutes due to the number of files that need to be copied).

I was being lazy during testing last night, so I set the ENTRYPOINT to "npm run dev" and forgot to chbsge it before committing.

You can only have one ENTRYPOINT in a Dockerfile, so I'll find a way to start these 4 in parallel.

Many many thanks for the comments from @jonathan-irvin too, I see you were interested in this from another discussion but were unsure how to do the npm/nx stuff I believe - that toolchain is also new to me, so this was my best effort! I've tried to address your comments as best I can.

I'd suggest docker-compose with 4 seperate containers rather than trying to run them all in one.

@jamesread
Copy link
Contributor Author

Hey @scottgrobinson , the image is big, 1.3Gb because of all the dependencies, creating 4 separate containers is of course possible, but the app doesn't yet support scaling in this way really, so what would be gained? It would just use more disk space / RAM.

@nevo-david
Copy link
Contributor

nevo-david commented Sep 7, 2024

@jamesread, we should have one for development and one for production.
Production will most likely need four separate containers (because, over time, you will need to scale the workers - microservice).

For dev we don't need 4 commands, we just need to npm run dev

@jonathan-irvin
Copy link
Contributor

I'll take a look at this today and take a stab at a container image.

@jamesread
Copy link
Contributor Author

I'm totally open to making changes based on feedback, I just want the best technical decisions to win.

@nevo-david what's the virtue of having a dev container? It takes a while to build, so it would be frustrating to develop in this way.

Does the code support active-active (multiple deployments) today? Does your production instance have multiple instances deployed? I ask because there's quite a lot of overhead splitting up the images wheb it comes to running them (the code is the easy bit). There will be people wanting to run this on a raspberry pi for example.

@jonathan-irvin can we work together? I'm on the discord, would love to chat.

@nevo-david
Copy link
Contributor

@jamesread for the development container, you want to mount the directory but not copy it so people can work on it locally.

@jamesread
Copy link
Contributor Author

Heya, just a quick post from me. @nevo-david and I just had a great discussion on discord.

We discussed several scenarios, and what we roughly said was 3 different Dockerfiles could make sense;

  1. dev container, as lots of people are struggling with node/npm versions and installing all the dependencies, this container should bind-mount the apps directory, allowing for easier local dev.

  2. allinone production container (this is what I was trying to do), for small scale self hosters.

  3. a scale-out set of containers, which each service separately for bigger scale.

I don't get to make the final decision, @nevo-david does! I'm also not fussed who makes the commits, I just like being part of helping out in this project :-)

@jonathan-irvin
Copy link
Contributor

@jamesread I'll handle the scaling side of things with helm and kubernetes. We can accomplish this many ways. Also @nevo-david do we have to host with nx run frontend:serve:production or could we serve this with nginx?

@jamesread
Copy link
Contributor Author

jamesread commented Sep 7, 2024

@jonathan-irvin , massive Kubernetes fan here too, that's where I intend to run it too!

My work here was on the allinone use case which I still think is valid for small scale self hosters.

You're intending on tackling the scape up use case, keen to see what you come up with for that. Maybe what I started here helps you, but you may well take an entirely different approach that ends up working better.

There is also the dev use case @nevo-david needs as well - maybe we can work together on that?

@jonathan-irvin
Copy link
Contributor

Yeah, IMO as far as a dev container is concerned, the first thing we can do is make sure the documentation is up-to-date.

I'm finding some missing dependencies that are assumed. I will put up a PR shortly.

@jonathan-irvin
Copy link
Contributor

This is great 🎉 but a few things: We need to decide if it's for production or dev. If it's dev, you don't have to build or copy the directories. All you need to do is run npm run dev. If it's production, then after the npm run build, we need four entry points:

  1. npm run start:prod
  2. npm run start:prod:frontend
  3. npm run start:prod:workers
  4. npm run start:prod:cron

In that case, I think we would have to run docker-compose.

@nevo-david

I don't think that docker-compose should be a requirement for building in production. I want to know how to produce artifacts for hosting the frontend, backend, and workers without needing a container to already be running.

@jonathan-irvin
Copy link
Contributor

I setup #206 for this

@jamesread jamesread changed the title feat: Dockerfile! feat: Dockerfile! (production/small "allinone") Sep 8, 2024
@jamesread jamesread changed the title feat: Dockerfile! (production/small "allinone") feat: Dockerfile! (devcontainer + production) Sep 9, 2024
@jamesread
Copy link
Contributor Author

Okay, it's been a long evening of hacking on a70ef27, but it's 2:45am and I need to go to bed!

There were lots of discussions on discord that led to these changes. Improvements and comments addressed in this commit summarized;

  • Base image switched to Arch - @jonathan-irvin made the excellent point that while I know Fedora, fewer people here have experience with it compared to the node:alpine image, and we need something that lots of people know.
  • Rather than the plan of having 3 containers - a devcontainer, allinone and a "scale out", @jonathan-irvin made the excellent suggestion to boot certain services depending on environment variables (defaulting to everything). This means one image that can optionally be scaled service by service, AND run in a allinone configuration as well. This leads to less disk space and wastage, just 2 containers, and a single dockerfile.
  • .devcontainer directory added (not really tested much yet), so people can just npm install devcontainer; devcontainer up to start hacking on postiz.
  • Implemented supervisord (still some work to be done there) to start the services
  • Switched to npm run start:prod etc, rather than using NX, to keep memory usage down, as per @nevo-david 's findings.

TODO;

  • I tried for quite a while to inline the node_module dependencies as part of the npm run build, but whatever settings I changed in package.json, the output was always the same. I think this would be good to do, so that we don't have to ship a 2Gb node_modules directory, and just the stuff the build /actually/ depends on.
  • Implement supervisord healthchecks.
  • Test both containers like crazy!

@jamesread jamesread marked this pull request as draft September 9, 2024 01:56
Copy link
Contributor

@jonathan-irvin jonathan-irvin left a comment

Choose a reason for hiding this comment

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

Had some time to take a good look at this.

Copy link
Contributor

@jonathan-irvin jonathan-irvin left a comment

Choose a reason for hiding this comment

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

A couple more feedback items, but we're looking pretty!

Copy link
Contributor

@jonathan-irvin jonathan-irvin left a comment

Choose a reason for hiding this comment

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

We don't need node for building docker

Copy link
Contributor Author

@jamesread jamesread left a comment

Choose a reason for hiding this comment

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

Ready for comprehensive re-review :-)

@jamesread jamesread requested review from scottgrobinson and jonathan-irvin and removed request for jonathanirvin-ast September 9, 2024 20:00
@jamesread jamesread self-assigned this Sep 9, 2024
@jamesread jamesread marked this pull request as ready for review September 9, 2024 20:07
Copy link
Contributor

@jonathan-irvin jonathan-irvin left a comment

Choose a reason for hiding this comment

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

I had some suggestions to the Dockerfile, but let's not let perfect be the enemy of good. We can iterate on it and make improvements in future PRs.

@jonathan-irvin jonathan-irvin merged commit d93d644 into gitroomhq:main Sep 9, 2024
6 of 8 checks passed
@jamesread jamesread deleted the dockerfile branch September 9, 2024 21:22
naruki1024 pushed a commit to naruki1024/postiz-app that referenced this pull request Jan 10, 2025
feat: add dockerfile for production and developer use
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants