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

Implemented Docker multi-stage build #1819

Closed
wants to merge 2 commits into from

Conversation

pbratkowski
Copy link
Contributor

@pbratkowski pbratkowski commented Dec 10, 2024

This solves the first part of #1817, where tox fails to install test dependencies due to missing packages.

  • Added target inputs to GitHub workflows.
  • Added stages to Dockerfile.
  • Moved package removal to production stage.
  • Added target dev to compose file.

Notes (and unresolved questions)

Marking this a draft PR for now, as it could use some feedback :)

  • I haven't tested the GH actions, as I am more of a GitLab user, but as per https://github.com/docker/build-push-action?tab=readme-ov-file#inputs, I believe that simply adding the target should suffice.
  • I'm not 100% sure whether the Action stages are correct, as I am not quite sure how they are run but I used the dev stage for tests, production stage for publishing.
  • This solves the install_deps step, and allows the tests to run, but the actual tests still fail when run.
  • Any feedback regarding the stage names (django-web-base, django-web-dev, django-web-prod), or anything else, let me know.
  • I'm not sure whether this should be merged as-is (I think it's fine as a standalone commit), or we should fix the test run itself too prior to merging this.

@bmispelon
Copy link
Member

This is lookin great, thanks for taking the time! 🎸

I haven't tested the GH actions [...]

Not sure what kind of tests you meant, but the modified workflows have been run in this PR and things didn't break (that's a good sign 😁 ).
With that said, I think both actions should target the prod stage, since we want the CI to complain if a PR will break building the image (rather than getting informed after the PR is merged). That means it should be fine to omit the target argument (I'm assuming that the build-push-action will then build all stages by default which is what we want).
Looking at docker-test-build, I don't really understand why it's also run on push: branch: main. That seems unnecessary since docker-publish will be run also (and it contains a superset of docker-test-build's actions). Maybe @tobiasmcnulty can shed some light here?

Any feedback regarding the stage names [...]

The names seem quite good as-is, but if you're open for changes I'd use www instead of web (this matches the convention we use for naming things internally), and maybe djangoproject instead of django. But that's a nitpick really.

I'm not sure whether this should be merged as-is [...]

I don't have a strong preference, but since the original issue was about tox not working, I'd be inclined to keep everything in this PR.
When it comes time to merge I think I'll probably keep the multi-stage changes in their own commit since it seems worth separating it from the rest.
So for now I'd suggest pushing each new changes as a separate commit, and we can see about prettying up the commit history when it's time to merge.

@pbratkowski pbratkowski force-pushed the feat/docker-multistage branch from d4f8800 to c93147a Compare December 11, 2024 14:55
@pbratkowski
Copy link
Contributor Author

Thanks for the feedback!

But that's a nitpick really.

Nitpicks are good, if it helps me follow conventions :)

That means it should be fine to omit the target argument (I'm assuming that the build-push-action will then build all stages by default which is what we want).

Docker's default is to build the last stage. As prod depends on the previous stages, it should build all of them, and use the production stage in the final image.

I dropped the changes to the workflow files, so they use the default behavior, and changed the naming convention to djangoproject-www-[stage].

This solves the first part of django#1817, where tox fails to install test
dependencies due to missing packages.

- Added stages to Dockerfile.
- Moved package removal to production stage.
- Added target dev to compose file.
@pbratkowski pbratkowski force-pushed the feat/docker-multistage branch from c93147a to 5424f72 Compare December 13, 2024 13:32
Added 'tracdb' container to compose file. Simplified docker settings to
remove duplication. Added support for 'secrets.json' in the docker dev
stage. Added the 'DJANGOPROJECT_DATA_DIR' env var to tox.
@pbratkowski pbratkowski force-pushed the feat/docker-multistage branch from 19356bc to 9fbab4a Compare December 13, 2024 15:18
Copy link
Member

@tobiasmcnulty tobiasmcnulty left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I have not reviewed the other parts of this PR, but wanted to offer a couple thoughts/context on the Dockerfile since @bmispelon tagged me on it. Consider these nitpicks only and please feel free to proceed as y'all see fit.


FROM djangoproject-www-dev AS djangoproject-www-prod

RUN apt-get purge --assume-yes --auto-remove \
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the initial layer for djangoproject-www-prod will still have these packages, so this won't actually reduce the size of the prod image. The purpose of removing the packages in the same layer as they are installed as that they don't end up as part of the layer.

The usual way to work around this with a multi-stage build is to start fresh and copy out of the first layer only the needed files, rather than depending on it in full. In my opinion (others will certainly differ), since Python venvs aren't really self-contained (as in this situation, they depend on system libraries), this is often more trouble than its worth for Python images.

A couple alternative ideas:

  • Uninstall the -dev packages only when REQ_FILE is requirements/prod.txt
  • Uninstall the other packages but not libpq-dev always, which should fix the underlying issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, thanks.

I don't think uninstalling other packages would work, as tox installs psycopg from source, and I believe they are all build requirements. At least, purging different packages led to different errors in my tests.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks. That makes sense. If gcc is needed too, my second suggestion certainly doesn't make sense :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revisit this tomorrow, but it should be doable via something like a Docker build arg (KEEP_DEPENDENCIES), and making the package uninstall conditional.

@pbratkowski pbratkowski deleted the feat/docker-multistage branch December 14, 2024 14:25
@pbratkowski pbratkowski restored the feat/docker-multistage branch December 14, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants