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

Allow building with BuildKit #761

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

Conversation

schummar
Copy link
Contributor

@schummar schummar commented Apr 27, 2024

Adds a withBuildKit option. With the option enabled, a build will be done using BuildKit - within a docker container.
This should be relatively independent on the host system's BuildKit support.

Questions:

  • Should the env var DOCKER_BUILDKIT=1 enabled the setting by default?
  • Should it be enabled by default in general? For a "just works" for newer dockerfile features like mounts?

Solves #571

Copy link

netlify bot commented Apr 27, 2024

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 2d6e540
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/666804f7c9e93d00083dd011
😎 Deploy Preview https://deploy-preview-761--testcontainers-node.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@cristianrgreco
Copy link
Collaborator

Hi @schummar, thanks for the contribution!

Because build kit is the default image builder I'd say this should be the default behaviour.

However because this implementation requires a separate container for the build, as well as copying potentially large files between images, I'm concerned about performance, which I assume is significantly worse when compared to a non-buildkit build. If we make this functionality opt-in (keeping withBuildKit), then I'm concerned about maintenance as it's a large and complex change. I haven't run the tests yet but I'm assuming issues with other container runtimes like podman too.

My plan was to wait for build kit to be natively supported by Docker engine and Dockerode, which should result in no or minor changes and no performance impact.

@schummar
Copy link
Contributor Author

Fair enough. I think the performance might not too bad in comparison. BuildKit builds are usually run in a container anyway, as far as I know - but my knowledge there is admittedly limited. I totally get the reluctance to add the complexity though.

Unfortunately, it's unclear when this might be supported (the referenced issues are 3,5 years old at this point). Usage of BuildKit features is getting more common - my very first try with testcontainers immediately failed because of this 😉. So, it would be good to have some kind of solution. What do you think about these options:

  1. I can create a separate npm package that provides the functionality. To load the image some access to container runtime would be great. Like keeping the proposed ImageClient.load method. Or exposing the underlying Dockerode instance?
  2. Would you be interested in having that package as a testcontainers module?

@schummar
Copy link
Contributor Author

schummar commented Jun 3, 2024

@cristianrgreco when you have a minute, could you give feedback?

@schummar
Copy link
Contributor Author

schummar commented Jun 11, 2024

Ok, completely new development:

As @mikeseese pointed out (#571 (comment)), the version flag is in fact working.

Upon investigation I found that I can make it work from testcontainers as well with one caveat: When passing pull: "true" and version: "2", the build always fails with "node:10-alpine: failed to resolve source metadata for docker.io/library/node:10-alpine: no active sessions". And because of this issue that is handled in PR #771, pull: "true" is currently always passed and therefore vesion: "2" never seemed to work at all!

That make this PR so much more concise! More open questions:

Edit: Well shit, the same problem arises when the image used in the FROM statement is not locally available yet. For some reason buildKit can't pull at all when run over the api!? At least on my machine...

@cristianrgreco
Copy link
Collaborator

Hi @schummar, thanks for staying on top of this issue! I've checked out this branch and run the test should work with buildKit and it passes. Are you saying that it doesn't for you? If it does could you please add a test for your valid example which is failing so I can also have a look? I'm on Docker version 24.0.6 on an M1 Mac

@schummar
Copy link
Contributor Author

A simple manual way to reproduce it is to run docker image rm node:10-alpine, them execute just that test case: npm run test -- packages/testcontainers/src/generic-container/generic-container-dockerfile.test.ts -t "buildKit". It will fail.

Then run docker pull node:10-alpine and execute the test again and it will succeed.

I can try and come up with a test case later. Removing images is not covered by the ContainerRuntimeClient, so I have to find a way around that.

@cristianrgreco cristianrgreco added enhancement New feature or request minor Backward compatible functionality labels Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Backward compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants