testcontainers-node icon indicating copy to clipboard operation
testcontainers-node copied to clipboard

Allow building with BuildKit

Open schummar opened this issue 1 year ago • 7 comments

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

schummar avatar Apr 27 '24 21:04 schummar

Deploy Preview for testcontainers-node ready!

Name Link
Latest commit 2d6e540be09754f9c611a681b041c12b9a4a6309
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.

netlify[bot] avatar Apr 27 '24 21:04 netlify[bot]

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.

cristianrgreco avatar Apr 28 '24 07:04 cristianrgreco

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 avatar Apr 28 '24 21:04 schummar

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

schummar avatar Jun 03 '24 17:06 schummar

Ok, completely new development:

As @mikeseese pointed out (https://github.com/testcontainers/testcontainers-node/issues/571#issuecomment-2159657555), 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:

  • I suppose the mentioned error is a docker bug. Until it's resolved, should testcontainers emit a warning when using both options together?
  • Currently dockerode types don't contain the version flag. Either we wait for https://github.com/DefinitelyTyped/DefinitelyTyped/pull/69780 or use a cast as ImageBuildOptions to cover it up for now

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...

schummar avatar Jun 11 '24 08:06 schummar

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

cristianrgreco avatar Jun 11 '24 10:06 cristianrgreco

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.

schummar avatar Jun 11 '24 10:06 schummar