FR: oci_tarball loading is not incremental

bazel-contrib / rules_oci

Bazel rules for building OCI containers

Apache License 2.0

287 stars 151 forks source link

FR: oci_tarball loading is not incremental #454

Open alexeagle opened 9 months ago

alexeagle commented 9 months ago

rules_docker has a fancy program https://github.com/bazelbuild/rules_docker/blob/master/container/incremental_load.sh.tpl#L4

thesayyn commented 9 months ago

my 2 cents on this issue;

This is not really a bazel-shaped problem that i am keen on solving anytime soon. oci_tarball never meant to be a node deep in the dependency graph but a leaf.

a quick look at this issue will show us how messy this is and lead to more issues being reported https://github.com/google/go-containerregistry/issues/895

thesayyn commented 9 months ago

one way I solved this in the past was by using a neat trick to start a registry daemon on the host and have docker runtime pull images from there.

juanzolotoochin commented 9 months ago

Here's my prototype implementation of a loader that works incrementally: https://github.com/juanique/bazel-examples/blob/load_image_prototype_7ff37/bazel6/rules_oci/2-go-image/app/BUILD.bazel#L15 if anyone wants to use it.

@thesayyn I understand that oci_tarball may not be meant for this, but the use case of "I need to load this docker image on multiple remote executors for integration tests" is a valid one that worked very well with rules_docker and it's just not well supported on rules_oci (large artifacts need to be passed via remote caching). So at least there should be some guidance on what to do for that use case.

thesayyn commented 9 months ago

I understand the need for such a thing. It's a tradeoff between maintenance and performance for us. Using tarballs for this purpose is not the right solution.

Some things can be done to improve DX but maintaining a loader monstrosity is not one of them.

Bazel has performance issues around TreeArtifacts: https://github.com/bazelbuild/bazel/issues/17804, https://github.com/bazelbuild/bazel/issues/16913
Docker tarballs are not meant to be transferred on wire, docker would have used it for distribution to registries if it was.
Even docker is not willing to address this issue on their end; https://github.com/docker/buildx/issues/107

I am trying to find the right solution rather than a workaround.

FWIW rules_docker's tarball loader shifts the responsibility away from bazel action to runtime where blobs are closest to the executor so it is mostly fast.

thesayyn commented 9 months ago

@jonjohnsonjr tried to implement this in https://github.com/google/go-containerregistry/pull/559

alexeagle commented 9 months ago

BTW to be clear, if anyone was willing to step up and maintain rules_docker, it could be unarchived. Our goal was never to have feature parity with it.

gzm0 commented 9 months ago

FWIW: I have built a clone of the rules_docker tarball loader using dockerode and tar-stream. It uses the same hack of sending the config only first and based on the returned error, identifying the layers that are actually required. Then it sends only these.

It will eventually appear on https://github.com/datahouse/bazel_buildlib. If there is interest, please LMK and I can make that happen sooner.

In any case, I have a feeling that rules for container runtimes should probably live somewhere else.

thesayyn commented 8 months ago

Example of incremental loader for testcontainers-py https://github.com/aspect-build/bazel-examples/blob/9e96e14f321f5e15231d8d2d0c7c4a18f1e311e6/oci_python_image/hello_world/app_test.py

gzm0 commented 8 months ago

Here is the version we use: https://github.com/datahouse/bazel_buildlib/blob/oss/buildlib/private/docker/src/loadImageToDocker.ts (someone requested it privately so I published it).