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

Shell out to docker buildx build to build images #1402

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

yuvipanda
Copy link
Collaborator

@yuvipanda yuvipanda commented Jan 29, 2025

pydocker uses the HTTP API (equivalent to docker build) which has been dead for ages. docker buildx (the interface to BuildKit) is what we should be using, but alas pydocker does not currently support it and I suspect it never will (docker/docker-py#2230).

This PR simply shells out, as I think that's the way.

I'm going to list some intentional choices I've made here.

  1. Intentionally replace the build functionality of the existing DockerEngine than make a new one. The old way is deprecated and we should not support it.
  2. Continue using dockerpy for all other non-build operations. It works fine and I don't see a need to change it right now.
  3. Simply extract the tarball we construct elsewhere. This is kinda double work, but we can treat this as a possible optimization pathway in the future.
  4. Currently ignore container_limits (because buildx doesn't support it, it should be set at the level of the builder instead)

Questions to answer

  • What do we do about container_limits here?
  • We probably need to deprecate / throw errors on some existing traitlets as they won't really have any effect here. What traitlets are those?
  • We should treat this as a breaking change and make a release after, right?

Traitlets to deprecate

  • Repo2Docker.build_memory_limit - this must be instead set up by the builder
  • Repo2Docker.extra_build_args - as this is purely about pydocker's build method that we no longer use

Fixes #875

pydocker uses the HTTP API (equivalent to `docker build`) which
has been dead for ages. `docker buildx` (the interface to BuildKit)
is what we *should* be using, but alas pydocker does not currently
support it and I suspect it never will (docker/docker-py#2230).

This PR simply shells out, as I think that's the way.

Fixes jupyterhub#875
@yuvipanda
Copy link
Collaborator Author

This isn't complete, putting it up here to see how many tests fail.

@yuvipanda yuvipanda requested review from manics and minrk January 29, 2025 04:41
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Nice! This will also unblock using COPY --chmod as in #1395

Re: extra_build_args, presumably we do want to keep an equivalent passthrough for extra args to pass to buildx build on the command-line (a list this time)?

repo2docker/docker.py Show resolved Hide resolved
platform=platform,
**kwargs,
)
if not shutil.which("docker"):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like rm and forcerm don't have equivalents. Just noting that this is intentional. Are there likely to be significant changes in the cache behavior with buildkit we need to pay attention to?

@yuvipanda
Copy link
Collaborator Author

@minrk I added extra_buildx_build_args in what feels like the right place for it. Will need to figure out a test.

yuvipanda added a commit to yuvipanda/mybinder.org-deploy that referenced this pull request Jan 30, 2025
Trying to fix https://jupyter.zulipchat.com/#narrow/channel/469744-jupyterhub/topic/try.20mybinder.20jupyterleab.20demo.20broken,
which is breaking because mamba gets OOM killed. We have memory limits
set on dind as a whole, and memory_limit for builds (in its current,
per-build form) is going away as part of jupyterhub/repo2docker#1402
as well. So let's remove it and see.
@@ -69,6 +74,14 @@ class DockerEngine(ContainerEngine):
config=True,
)

extra_buildx_build_args = List(
[],
Copy link
Member

Choose a reason for hiding this comment

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

If you want to specify the type of items in the list, give a single TraitType instance here (not list):

Suggested change
[],
Unicode(),

@minrk
Copy link
Member

minrk commented Jan 30, 2025

Will need to figure out a test.

iidfile looks like it might be a good arg to test with, since it just creates a file to check. Or --check appears to skip the build entirely, so should be quick.

@minrk
Copy link
Member

minrk commented Jan 30, 2025

Here's a small test for extra buildx args with --check that goes really quick since it doesn't actually build the image:

python3 -m repo2docker --no-run '--DockerEngine.extra_buildx_build_args=["--check"]' .

which outputs (among other things):

Check complete

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.

Switch to using Buildkit
2 participants