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

Update docker + docker compose for dev and prod #1021

Closed
wants to merge 43 commits into from
Closed

Update docker + docker compose for dev and prod #1021

wants to merge 43 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 12, 2024

First of all, I apologize for the size of the PR 🙏, but I think it would've been difficult to break it down.

This is a PR to rework the docker setup for dev and prod. The highlights are below, but basically the setup was not working well for development (not clear how to set it up, which files were necessary to be where, which env vars were important, updating code locally didn't reflect in the docker environment, etc.).

Now, developers should be able to spin up a development environment with minimal effort with docker installed, see their changes show up on reloads, and even use a debugger (XDebug).

The changes in the dev setup led to changes in production setup as well, though they should be minimal. Most importantly caddy was pulled out of the image to break the tight coupling and allow any reverse proxy with fast CGI to sit in front of mbin.

Highlights

Features

  • Create a dev env using docker usable by noobs like me
ln -s compose.dev.yml compose.override.yml
docker compose up
  • Support XDebug call from docker on linux with host.docker.internal
Details

host.docker.internal doesn't exist on linux because it's not running docker in a VM. Add following block to php service.

   extra_hosts:
      - "host.docker.internal:host-gateway"

  • Support for arbitrary reverse proxy
  • Smaller docker image (only necessary stuff is copied into image)
  • Documentation for dev docker setup

Bug fixes

  • get rid of FATAL: role "root" does not exist in db service created by its healthcheck

TODO

  • update docker prod documentation
  • mention that resetting means sudo rm -rf docker/storage
    • move .gitignore out of storage otherwise resetting still creates a commit by deleting .gitignore
  • docker/storage needs to be writable to mbin user!
    • sudo chmod 777 docker/storage/*

Abandoned tasks for later PRs

  • Use whitelist approach in .dockerignore instead of blacklist
  • MESSENGER_TRANSPORT_DSN=doctrine://default as documented doesn't work

.dockerignore Outdated Show resolved Hide resolved
compose.prod.yml Outdated Show resolved Hide resolved
@ghost ghost added enhancement New feature or request docker Issues and pull requests related to docker environment labels Aug 12, 2024
@CocoPoops
Copy link
Contributor

just a thought but maybe we should use 1 compose file with profiles instead of multiple compose files for prod and dev as i think being able to keep the compose.override file separate for admins to make changes to the compose.yml instead of editing it directly while also being able to keep up with git changes

https://docs.docker.com/compose/profiles/

@ghost
Copy link
Author

ghost commented Aug 14, 2024

@CocoPoops that doesn't seem like the right feature to use, unfortunately.

Services can be assigned to one or more profiles; unassigned services start by default, while assigned ones only start when their profile is active

Additionally, if you look at compose.dev.yml and compose.prod.yml, you'll see that the services apply different overrides. For example in compose.dev.yml the php and messenger services mount the CWD under /var/www/mbin so that local changes will be visible upon reloading the page in the browser. That isn't necessary in production because the image will have the source and build files within the image.

We could use a justfile to have a dev and prod target which execute docker compose -f compose.yml -f compose.$env.yml, but that's out of scope for this PR.

@CocoPoops
Copy link
Contributor

@LoveIsGrief hopefully this explains what i meant better

For example, It would allow us to create php-prod and php-dev services in one compose.yml, along with services, and then assign them to corresponding profiles or both if no changes are between prod and dev.

This would allow administrators to override settings for either production or development services independently through the compose.override.yml file.

This flexibility is particularly useful in scenarios like mine, where I use Traefik as a reverse proxy configured via Docker labels. By organizing the Compose files this way, it would be much easier to switch between production and development environments for debugging purposes without the risk of exposing sensitive debug secrets.

@CocoPoops
Copy link
Contributor

CocoPoops commented Aug 15, 2024

I think it would also be easier that way if for some reason we need to change the compose file down the line as we should be recommending admins only change things via the compose.override file

@ghost
Copy link
Author

ghost commented Aug 16, 2024

@LoveIsGrief Could you fork my branch and make an example?

For your scenario, you could use include (see doc).

compose.override.yml

include:
  - compose.prod.yml

services:
  traefik: ...

@CocoPoops
Copy link
Contributor

For your scenario, you could use include (see doc).

compose.override.yml

include:
  - compose.prod.yml

services:
  traefik: ...

ill just do this as i just tested using the compose profiles and it doesnt really seem worth it as it kinda makes it a pain to maintain

if anyone wants do that include method for overriding
i had to do this way as you cant override included compose files from the same file they are included in

compose.override.yml

include:
  - compose.prod.yml
  - compose.prod.override.yml 

compose.prod.override.yml

services:
  www:
   labels:
     - add docker labels
  other-services: ...

@CocoPoops
Copy link
Contributor

also just tried switching my instance over to test the image is working and im getting 404's on some images but not others

tried clearing php cache and flushing the redis db but that didnt help

is there anything i would have to change in my .env to get working?

Screenshot_20240313

@ghost
Copy link
Author

ghost commented Aug 17, 2024 via email

@melroy89 melroy89 self-requested a review August 19, 2024 14:39
@melroy89
Copy link
Member

Press "Ready for review" (and remove "WIP" from the title) if you think we should review.

@ghost ghost marked this pull request as ready for review August 19, 2024 19:01
@ghost ghost changed the title WIP: Update docker + docker compose for dev and prod Update docker + docker compose for dev and prod Aug 19, 2024
@ghost
Copy link
Author

ghost commented Aug 19, 2024

@melroy89 I finally have it in a state where the code should be reviewable. I don't know if you guys squash-merge or not. If not, once the reviews are in, I can rebase and rework the commits to get rid of the wip: ... and other commits.

@CocoPoops I think I fixed the issue you were facing with missing media. Please see the migration guide section in the installation/docker.md docs.

@melroy89
Copy link
Member

Yes, we normally squash merge the commits. Especially in this PR that is a must haha.

container_name: mbin-db
restart: unless-stopped
networks:
- mbin_external_network
Copy link
Member

@melroy89 melroy89 Aug 24, 2024

Choose a reason for hiding this comment

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

PHP, DB & Redis can definitely be an internal network (internal: true). Yes, you can have multiple networks in Docker. And in Compose you can also add multiple networks to a single service (eg. messenger can both have internal and external docker networks).

Copy link
Author

Choose a reason for hiding this comment

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

🤔 I'm not sure what advantage that would have for the php and messenger services. If I'm not mistaken those require outgoing access to external networks in order to download stuff and communicate with other instances. The php service should probably be able to access direct incoming traffic. Dunno about the messenger.

The DB and redis could probably be purely internal as I don't believe they'll ever need to access external resources.

Should this be done in another PR? I feel like it could expand the scope of this PR and it might be good to have a separate PR with one concern (docker compose networking) for posterity.

Copy link
Member

Choose a reason for hiding this comment

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

As fair as I know only the www service (and maybe also the rabbitmq if you enabled the management interface) needs access to the external network, the rest of all the services can do internal communication between the services.

Sure, this can also be done in a follow-up PR.

Copy link
Author

Choose a reason for hiding this comment

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

Created a ticket so it won't be forgotten.

Copy link
Member

@melroy89 melroy89 Sep 2, 2024

Choose a reason for hiding this comment

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

I now get:

Error response from daemon: driver failed programming external connectivity on endpoint mbin-db (c1c3e1b40be94b691d7a32acbdcde8723b9a7402b68bbb64598c5533e354b1e5): failed to bind port 127.0.0.1:5432/tcp: Error starting userland proxy: listen tcp4 127.0.0.1:5432: bind: address already in use

So this external network will interfere with any host ports that run for example PostgreSQL. Therefore, I reopened this conversation.

compose.prod.yml Outdated Show resolved Hide resolved
docker/php/Dockerfile Outdated Show resolved Hide resolved
@BentiGorlich BentiGorlich assigned ghost Aug 27, 2024
@ghost ghost requested a review from melroy89 August 27, 2024 18:37
@TheVillageGuy TheVillageGuy self-requested a review September 1, 2024 20:43
TheVillageGuy
TheVillageGuy previously approved these changes Sep 1, 2024
Copy link
Contributor

@TheVillageGuy TheVillageGuy left a comment

Choose a reason for hiding this comment

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

As Bentigerolich asked for this PR and Average Dude is waiting for approval I am approving this

docker/php/entrypoint.sh Outdated Show resolved Hide resolved
LoveIsGrief and others added 27 commits September 2, 2024 19:06
curl isn't installed in the image and the health check
was constantly failing
/public files are copied into a common volume shared between reverse proxy and PHP-FPM service.
Otherwise the logs get filled with 'FATAL:  role "root" does not exist' everytime the health is checked
After supporting arbitrary reverse proxies, media uploads were broken.
They are uploaded to public/media/ by `php` service, but the previous solution just provided a copy
of public/media/ at first run. New uploads weren't shared with the `www` service.

Now, instead of just providing a copy, a mount is provided. To do so the strategy was changed and a lead taken
out of nextcloud's book: https://github.com/nextcloud/docker/blob/65138b6d22bec1ac15e2f0f125426290640bb97a/docker-entrypoint.sh

**The problem**

The image had the mbin sources in /var/www/mbin. Mounting docker/storage/public into /var/www/mbin/public would
replace the image's public/ with a (hitherto) empty directory.

**The solution**

The image has its sources in /usr/src/mbin and on startup, syncs that into the new mount /var/www/mbin using `rsync`.
In this manner, previous uploads are preserved and source changes update /var/www/mbin.
Separate images are now available because mbin isn't solely bound to caddy and can be used with any reverse proxy.
These come due to the refactoring:

 - `docker/compose.yml` --> `/compose.yml`
 - support for arbitrary reverse proxies without running caddy
 - smaller docker image
 - allow overrides without changing versioned files
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
Co-authored-by: Melroy van den Berg <melroy@melroy.org>
It wouldn't start without the file and I realised it had been there the entire time
when testing the documentation. @melroy tested it without and discovered the bug
This follows what is done for the production guide, which has 2 guides/files as well.
@melroy89
Copy link
Member

melroy89 commented Sep 2, 2024

I stopped postgresql locally, so it now starts the docker images. Going to http://localhost:8080 gives me:

An exception has been thrown during the rendering of a template ("Asset manifest file "/var/www/mbin/public/build/manifest.json" does not exist. Did you forget to build the assets with npm or yarn?").

@ghost ghost closed this by deleting the head repository Sep 3, 2024
@melroy89 melroy89 mentioned this pull request Sep 4, 2024
3 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker Issues and pull requests related to docker environment enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants