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

Refactor Docker/Compose #4844

Merged
merged 190 commits into from Mar 5, 2024
Merged

Refactor Docker/Compose #4844

merged 190 commits into from Mar 5, 2024

Conversation

jippi
Copy link
Contributor

@jippi jippi commented Dec 28, 2023

Hello friends!

Intro

This is a Request for Feedback (RFC) for improving the Docker experience with Pixelfed!

It's a rather large amount of changes, but I've done my best to document inline why/what things are there for - both to aid in review and for future maintainers/consumers of it.

As an active OSS maintainer myself, I know first hand that big "fly-by" improvements like this can be met with 🙄🙄🙄 and deep sighs for a lot of valid reasons (such as lack of testing, documentation, explanations, etc.) so I've tried my best to make everything relatively accessible and documented from the get-go.

You can read the explanation for a lot of the changes (and the documentation/usage) on my fork branch

Motivation

The motivation for this PR was to improve my own day-to-day experience managing my pixelfed server at https://pixelfed.dk - which is back by Docker and Docker Compose.

I work with big scale platforms and infrastructure in my day job, so when I see an opportunity to improve the Docker experience for myself and others in OSS, I usually jump on it.

Outcomes of the changes

  • Faster Docker builds thanks to ...
    • multiple layers of build/layer caches (~5-10min on warm cache on GitHub Actions - yes, even for arm64)
    • parallel build stages
    • architecture (amd64 and arm64) specific caches - they no longer overwrite each other
    • cached apt / pecl archives and binaries between runs
  • Faster docker pull thanks to smaller, more efficient, layers (especially true when pulling updates!)
  • Flexible Dockerfile via build arguments making customization easy and safe for server owners/consumers without having to hard-fork the Dockerfile
  • Easy maintenance of software versions thanks to build arguments for PHP, Composer, and Nginx versions
  • Merging of fpm and apache Dockerfiles for easier maintenance
  • Introduction of a nginx (Nginx + FPM in one container) runtime build target
  • Smaller Docker images
    • 18% smaller FPM image vs murazaki/pixelfed (from 541.56 MB to 443.18 MB)
    • 18% smaller Apache image vs murazaki/pixelfed (from 545.02 MB to 446.41 MB)
    • 71% smaller FPM image vs contrib/docker/Dockerfile.apache in tag v0.11.9 (from 1480 MB to 443.18 MB)
    • 70% smaller Apache image vs contrib/docker/Dockerfile.fpm in tag v0.11.9 (from 1490 MB to 446.41 MB)
  • Flexible entrypoint system for project-specfific configuration (can be added via --volume in docker run, for example)
  • Easily optimized Apache, Nginx, and FPM images (build steps, config files, etc) thanks to build targets.
  • Automatic build + push to both GitHub Registry and Docker Hub
  • Automatic build + push of all 3 supported runtimes (Nginx, Apache, and FPM)
  • GitHub fork friendly, as it will by default ship Docker images to the projects own GitHub Registry
    • Docker Hub login+push is skipped unless the DOCKER_HUB_TOKEN secret set first.
  • Docker images now include Software Bill Of Materials (SBOM)

Progress updates

Feedback wanted

  1. Is this change is something Pixelfed would be interested in?
    1. If No, what concerns/risks/constraints/changes made it undesirable? Can those issues be addressed?
  2. What kind of X should be made/improved/created for y'all to be confident in merging it?
    1. testing
    2. documentation
    3. automation
    4. training

Note

  • This PR is not mergeable atm, as its based off of the v0.11.9 tag and not develop - if the changes are interesting to the project, I will rebase and update it to target develop

Prior contributions

This Pull Request incorporates/adopts/tweaks unmerged changes, ideas, and concepts from many other folks

And these closed tickets

Fixed (open) tickets

If this PR doesn't fix the issues, they should be reopened with fresh data and context

contrib/docker/Dockerfile Outdated Show resolved Hide resolved
contrib/docker/Dockerfile Outdated Show resolved Hide resolved
contrib/docker/Dockerfile Outdated Show resolved Hide resolved
@jippi jippi changed the title WIP: Refactor Docker RFC: Refactor Docker/Compose Dec 28, 2023
@lbwtaylor
Copy link

I just want to say thank you for spending time on Pixelfed docker. Your ultimate goal, a turn key docker-compose is very exciting and very much needed for Pixelfed. Thank you!

@Murazaki
Copy link
Contributor

Murazaki commented Jan 4, 2024

Thank you for the PR.

I am still a bit disappointed of the response, as this PR is building on top of a lot of work done before and yet doesn't mention any.
Yet I see a lot of closed PRs in favor of this one, also in cases where it doesn't even solve the same issue. Lack of communication doesn't help.

I worry that favoring the voice of one person over the contributions of many might deter some people of contributing, and bring a mentality of overengineering and difficult to read PRs instead of favoring fixes and small iterations. (ofc it comes with the "burden" of answering early and accepting those when they are relevant).

@Sythelux
Copy link

Sythelux commented Jan 4, 2024

Thank you for the PR.

I am still a bit disappointed of the response, as this PR is building on top of a lot of work done before and yet doesn't mention any. Yet I see a lot of closed PRs in favor of this one, also in cases where it doesn't even solve the same issue. Lack of communication doesn't help.

Saw this too and I think it isn't even jippi's fault. Dan just went through and rather silently closed the other Tickets without writing anything here (yet). We will see if he has anything to say.

As feedback to the Pull Request. I don't have too indepth knowledge of docker. I use it mainly as hobby, but run a pixelfed server. I appreciate that jippi made it configurable as I'm not running a default build. I will see how it integrates, but I'm confident it works.

@jippi
Copy link
Contributor Author

jippi commented Jan 4, 2024

Hey!

I certainly apologize if it looks like I've hijacked/copied/stepped on anything other PRs were doing without attribution!

This PR is however just my local fork build over new years that I thought I would love to share back to the community, I honestly hadn't even checked GitHub to see if anyone else had done similar (ADHD + liking this kind of stuff made me dive head first into it on my own).

I mentioned this PR in the (now closed PRs) in hope to get some passionate Docker folks to help review this, as it ended up being pretty sizeable and I'm not (yet) super familiar with Pixelfed and Laravel - and as I wrote in the PR comments, I hope to either bring on these changes in this PR, or have them as improvements rebased on top of this one.

Re the future on Docker with Pixelfed, I chatted with @dansup on Mastodon about this, and offered to help steward and review future improvements to Pixelfed+Docker (https://mastodon.social/@dansup/111686447576547998) - especially since the open + aged PRs in this repo was predominately Docker related, and @dansup didn't seem very keen on reviewing and merging them (https://expressional.social/@jippi/111685968685079968)

@Murazaki
Copy link
Contributor

Murazaki commented Jan 4, 2024

I understand where you come from and the situation with docker on Pixelfed has been bad for a long while, as I can also be a witness of it.
We did have pretty good contributions from a lot of people, and I tried to push for some of those to be integrated, discussing with @dansup on the subject. Lately we had contributions from some important Fediverse advocates too. I did also refrain from pushing a big change on my side to not make our PRs compete.
In the past some people pushed bigger PRs too, but in the sake of making it readable and to select changes, I asked for those to be cut in different PRs.
I'm in no way saying you should do so. bundle PRs can also help a lot. But when so much work has been done, it is best to acknowledge it at least, and in the case of inspiration, to mention it, as it will always help those other devs to feel confident to push new changes again and/or to say in the loop for discussions.

@Strubbl
Copy link
Contributor

Strubbl commented Feb 23, 2024

i have shutdown my instance. my docker setup does not work anymore and i have no time to fix this

@jippi
Copy link
Contributor Author

jippi commented Feb 23, 2024

@Strubbl okay, not sure what to do with that information other than I'm sorry you had a bad experience, and I hope you can provide me (and the community) some feedback on how to do better in the future ❤️

@jippi
Copy link
Contributor Author

jippi commented Feb 24, 2024

I added some helper scripts for Docker users.

./docker/dottie

Dottie is a tool I've been working on to maintain and do various operations against .env files.

For example, ./docker/dottie update (potentially with --no-save flag to DRY run) should be able to take the default .env.docker file and "replay" all your KEY/VALUE pairs from your .env file on top - effectively migrating/updating your .env file with upstream, without having to do it manually.

It will create a .env-dottie-backup with your old .env file (just in case!) so it should be safe to use.

If you want to update from the old .env and .env.docker file but not using this setup yet, can either

install dottie locally

install Dottie and run

dottie \
  update \
  --file .env \
  --source https://raw.githubusercontent.com/jippi/pixelfed/jippi-fork/.env.docker

Use Docker

Or use Docker by running

docker run \
    --rm \
    -it \
    --env TERM \
    --env COLORTERM \
    --volume "${PWD}:/var/www" \
    --workdir /var/www \
    ghcr.io/jippi/dottie \
    update --source https://raw.githubusercontent.com/jippi/pixelfed/jippi-fork/.env.docker

Dottie can also do validation of the .env file (we use that in this container setup already!) beyond just key exists, but will validation of possible options, format, type, etc.

./docker/artisan

I also added ./docker/artisan to easily run php artisan commands in containers without having to do long docker compose exec commands to get it right.

By default it uses the worker container to run commands, it can be changed via the PF_SERVICE env (worker or web is accepted) - matching the Docker compose service name.

./docker/shell

Allow you to easily run commands (or get an interactive shell) for the various containers. Uses same PF_SERVICE environment variable as ./docker/artisan above.

By default ./docker/shell will give you an interactive shell in the worker container.

If you pass in any commands, it will instead run that command in the container, for example, ./docker/shell php artisan is same as ./docker/artisan

./docker/shell date will give you the date from within the container.

@Smartich0ke
Copy link

Is there any idea of when this will be merged? Would love to see some public docker images, this seems like a really great project.

@Murazaki
Copy link
Contributor

Is there any idea of when this will be merged? Would love to see some public docker images, this seems like a really great project.

Just a friendly reminder that this is available on jippi's fork, as well as pixelfed-glitch/pixelfed 😊

@jippi
Copy link
Contributor Author

jippi commented Feb 29, 2024

@Smartich0ke I tail Pixelfed on my "fork" (https://github.com/jippi/pixelfed/tree/jippi-fork) + https://jippi.github.io/pixelfed-docs-next/pr-preview/pr-1/ until its merged - the fork is pure, so no other changes than the Docker related work, so should be a smooth transition to the final merge (eventually)

@Smartich0ke
Copy link

Smartich0ke commented Feb 29, 2024

Ok thanks! I'll build my own images from the fork for now and switch to official image if it comes available.

@jippi
Copy link
Contributor Author

jippi commented Feb 29, 2024

alright, they are also pre-build on my fork https://github.com/jippi/pixelfed/pkgs/container/pixelfed (which the docs also point to)

@Wave6677
Copy link

Wave6677 commented Mar 2, 2024

Hi jippi, I was wondering if you can help me troubleshoot your container? If you're not interested in that, that's okay, i can go without PixelFed, but I'm having trouble getting the container to connect to my reverse proxy, Traefik. I disabled the built in containers in the env, and commented them out of the docker-compose.yml. The pixelfed worker container seems to go unhealthy and then back to healthy, as well as traefik giving me either a "Bad Gateway" or "Internal Server Error" message depending on which way I want to forward the router. Any help is appreciated but no help is also okay, i know everything here is provided for free without warranty.

@jippi
Copy link
Contributor Author

jippi commented Mar 2, 2024

@Wave6677 happy to help - are you in the Discord community so we can do it real time? :)

@Smartich0ke
Copy link

Could you send the invite link? I'd love to join it too but I couldn't find it on the website or in the readme. I'm surprised there wouldn't be something like a Martix room since this project is all about decentralization?

@Wave6677
Copy link

Wave6677 commented Mar 2, 2024

Could you send the invite link? I'd love to join it too but I couldn't find it on the website or in the readme. I'm surprised there wouldn't be something like a Martix room since this project is all about decentralization?

I just joined I think, if it's the one in your copy of the Next documentation :) (I have the absurd username that is very hard to miss)

@Smartich0ke
Copy link

Smartich0ke commented Mar 3, 2024

I've been playing with the GHCR image and it's really cool! but I've hit a couple of snags.

First up, having to load an .env file by mounting it isn't ideal, especially in Kubernetes where you usually set env vars directly when the container runs. Normally, if we need a file, we'd stick it in a ConfigMap or Secret and mount that, but then the file has to be read-only, so no chown or chmod on the .env file, which should be fine since the permissions the permissions will likely be correct anyway. Plus, in Kubernetes, you can't just mount a single file with PVCs - it's got to be a whole directory, and obviously we don't want to mount the entire /var/www or mess up the laravel app's structure by putting it in a non-standard location.

So, what about this: why not just bake the default .env.docker file into the container at build-time through the dockerfile, turning it into .env? Then, if users need to change anything, they can just set those env vars directly at runtime, overriding the defaults. For Docker Compose people, they could either simply drop them into the environment: section or use an env_file: if they prefer sticking with a file.

Sorry if I'm jumping the gun a bit here with the kubernetes stuff. The current system does work fine for standalone docker which is probably what matters most at this point in time.

The second thing is after the entrypoint script completes, everything seems well but it appears to hang on this before crashlooping or exiting:

Releasing lock [/var/www/storage/docker/lock/entrypoint.sh]

Not sure if this is an issue with my setup or I just have to let the container wait a bit longer?

@dansup dansup merged commit 0bd3e0a into pixelfed:staging Mar 5, 2024
2 checks passed
@gutocarvalho
Copy link

Thanks for the work.

I usually do this to build the new image

$ git pull
$ git checkout v0.11.12
$ docker build . -t pixelfed:v0.11.12 -f contrib/docker/Dockerfile.apache

And that's it.

I'll read the new docs to understand the proper way to do it now.

Cheers,

@lnlyssg
Copy link

lnlyssg commented Mar 5, 2024

I'm trying to follow the Path A migration but I can't get it working:

❯ docker compose -f docker-compose.migrate.yml run migrate bash
WARN[0000] The "DOCKER_APP_HOST_STORAGE_PATH" variable is not set. Defaulting to a blank string.
WARN[0000] The "DOCKER_DB_HOST_DATA_PATH" variable is not set. Defaulting to a blank string.
WARN[0000] The "DOCKER_REDIS_HOST_DATA_PATH" variable is not set. Defaulting to a blank string.
invalid spec: :/migrate/app-storage/new: empty section between colons

I'm guessing I've missed something....

@jippi
Copy link
Contributor Author

jippi commented Mar 5, 2024

please file new tickets for any bugs / issues so they are captured correctly and can be actioned separately ❤️

@lnlyssg
Copy link

lnlyssg commented Mar 5, 2024

I'm not sure if this is an issue or if I've missed something obvious 😆 I've created a discussion topic instead - #4977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ Deployment Related to specific deployments or configurations ❗❗ Priority: High Important
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet