Support index manifest to push multi-arch container images #2087

lbcjbb · 2022-05-14T12:46:07Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

The rule container_push is able to push a single container to a registry but isn't able to push a multi-arch containers, aka index or fat manifest.

The specification of an index manifest can be found here:

Issue Number: #1599

What is the new behavior?

I add a new rule named container_push_index to push a multi-arch containers.

This new rule can be used like that:

load( "@io_bazel_rules_docker//container:container.bzl", "container_push_index")

container_push_index(
    name = "push_all",
    format = "Docker",
    images = {
        "//:my_image_amd64": "linux/amd64",
        "//:my_image_arm64": "linux/arm64/v8",
        "//:my_image_ppc64le": "linux/ppc64le",
    },
    registry = "localhost:5000",
    repository = "docker/test",
    tag = "{VERSION}",
)

Does this PR introduce a breaking change?

Yes
No

The container_push rule is not impacted and still works fine as always.

lbcjbb · 2022-05-17T06:11:22Z

We could improve this new rule container_push_index to use a transition to avoid to explicitly specify the list of image targets to push, like this:

load( "@io_bazel_rules_docker//container:container.bzl", "container_push_index")

container_push_index(
    name = "push_all",
    format = "Docker",
    image = "//:my_image",
    platforms: [
        "linux/amd64",
        "linux/arm64/v8",
        "linux/ppc64le",
    ],
    registry = "localhost:5000",
    repository = "docker/test",
    tag = "{VERSION}",
)

Where image attribute is a target created by container_image.

But this improvement requires before several modifications as suggested in #2073.

aw185176 · 2022-05-17T19:07:42Z

Regarding using transitions: one benefit of not doing so, and allowing explicitly wiring up multiple images is that it enables non-traditional use cases such as storing things like K8s manifests (or binaries, or WASM artifacts, etc). I am not sure using transitions is worth losing the flexibility for other use cases.

lbcjbb · 2022-05-17T20:48:23Z

As I said on Slack, we can keep the images attribute to keep the possibility to explicitly give the list of images to push.

We're able to keep the two differents ways:

The images attribute to explicitly give the list of images to push.
The image + platforms attributes to use a transition to define the list of images to push.

And:

images and image will be mutually exclusive.
platforms will be mandatory if image is given.
One of these two attributes betwwen image or images must be defined.

nadavwe · 2022-09-21T08:46:02Z

hey @lbcjbb, this is super useful!
Why is that hanging since May? anything holding it back?

lbcjbb · 2022-09-21T11:32:11Z

Hi. This project needs new maintainers as mentionened in this discussion #2038

nadavwe · 2022-09-21T11:44:35Z

oh... but I see @uhthomas and @linzhp volunteered to be maintainers now...

drewcsillagdd · 2022-10-17T19:00:34Z

So does this actually have maintainers now?

lbcjbb · 2022-10-18T07:16:43Z

samruthr · 2022-11-18T20:27:39Z

This is a very useful addition to the rule. How can we get attention to this PR?

lbcjbb · 2022-11-18T20:49:44Z

I don't know :/

uhthomas · 2022-11-18T21:59:27Z

I can take a look soon :)

samruthr · 2022-12-12T17:11:41Z

Hi @uhthomas, just checking in, have you had an opportunity to take a look?

uhthomas

So, I think this looks mostly fine. Just a few code comments. With respect to the code itself, is it possible to unit test some of this to avoid regressions?

I also had another topic I want to discuss. How does this fit into transitions? Is this compatible with a single target which could transition to different target platforms?

Sorry for the delay.

container/go/cmd/digester_index/digester_index.go

container/go/pkg/compat/image_index.go

These components will also be used by the 'index' version of the pusher binary. - The function to check and initialize the docker client configuration. - The function to compute the list of options to give to remote.Write*() functions. We now use three new options: - remote.WithContext() to make the push cancellable. - remote.WithUserAgent() to help the registry to known the origin of requests. - remote.WithJobs() to define the number of parralel jobs to use. We give the value returned by runtime.NumCPU() to override the hardcoded value in the library go-containerregistry which is 4.

The main goal of this refactoring is to make available several functions. These functions will be used by the 'index' version of the `container_push` rule. Unless I am mistaken, the code is exactly the same.

lbcjbb · 2023-01-14T11:37:42Z

Thank you very much for the review @uhthomas ❤️.

For all comments related to the Go errors 1.13 (the use of %w), it's maybe better to make this change in another pull request, to migrate all the Go code of this repository.

All this new code is already covered by E2E tests, but I take a look to also add several unit tests. ✅

I also had another topic I want to discuss. How does this fit into transitions? Is this compatible with a single target which could transition to different target platforms?

If I recall, i don't think. But we will be able to easily add this feature.

loeffel-io · 2023-01-27T10:25:23Z

@uhthomas please 🙏

Topher-the-Geek

There's some issue in digester_index.

On Mac OS Ventura 13.1, Intel chip: I give up waiting after 10 minutes. digester_index is the top CPU consumer that whole time.

On Ubuntu 20.04 in a container run on K8S, x86_64: digester_index is killed after about 3 minutes.

To reproduce, I put together a simple build that merely repackages the NGINX container.

BUILD

load("@io_bazel_rules_docker//container:container.bzl", "container_push_index")

container_push_index(
    name = "push-all",
    format = "Docker",
    images = {
        "@nginx-linux-amd64//image": "linux/amd64",
        "@nginx-linux-arm64v8//image": "linux/arm64/v8",
    },
    registry = "<my private repo>.amazonaws.com",
    repository = "test",
    stamp = "@io_bazel_rules_docker//stamp:always",
    tag = "latest"
)

pr2087.patch

diff --git a/container/push.bzl b/container/push.bzl
index 9be8d89..83b8b96 100644
--- a/container/push.bzl
+++ b/container/push.bzl
@@ -282,6 +282,7 @@ def _container_push_index_impl(ctx):
             stamp = ctx.attr.stamp[StampSettingInfo].value,
             format = ctx.attr.format,
             skip_unchanged_digest = ctx.attr.skip_unchanged_digest,
+            insecure_repository = ctx.attr.insecure_repository
         ),
     )

(That final blank line is important)

WORKSPACE

workspace(
    name = "pr2087",
)

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "bazel_skylib",
    sha256 = "74d544d96f4a5bb630d465ca8bbcfe231e3594e5aae57e1edbf17a6eb3ca2506",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.3.0/bazel-skylib-1.3.0.tar.gz",
        "https://github.com/bazelbuild/bazel-skylib/releases/download/1.3.0/bazel-skylib-1.3.0.tar.gz",
    ],
)

load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")

bazel_skylib_workspace()

RULES_DOCKER_VERSION = "c079c0ddad54d34205c0a94fbb7cf9c6e7a6f48b"

http_archive(
    name = "io_bazel_rules_docker",
    patch_args = ["-p1"],
    patches = [
        "//:pr2087.patch",
    ],
    strip_prefix = "bazel-rules_docker-%s" % RULES_DOCKER_VERSION,
    type = "zip",
    url = "https://github.com/leboncoin/bazel-rules_docker/archive/%s.zip" % RULES_DOCKER_VERSION,
)

load("@io_bazel_rules_docker//repositories:repositories.bzl", container_repositories = "repositories")

container_repositories()

load("@io_bazel_rules_docker//repositories:deps.bzl", container_deps = "deps")

container_deps()

load("@io_bazel_rules_docker//container:container.bzl", "container_pull")

# https://hub.docker.com/_/nginx
# 1.23.3-alpine

container_pull(
    name = "nginx-linux-amd64",
    digest = "sha256:c1b9fe3c0c015486cf1e4a0ecabe78d05864475e279638e9713eb55f013f907f",  
    registry = "index.docker.io",
    repository = "library/nginx",
)

container_pull(
    name = "nginx-linux-arm64v8",
    digest = "sha256:5fbf2392cd98bf322ff24e5de9a74e732ebeb4873c06f29ed801b2bed1b11019",  
    registry = "index.docker.io",
    repository = "library/nginx",
)

container/push.bzl

Topher-the-Geek · 2023-01-29T16:56:14Z

I forgot: That's with Bazel 6.0.0

I add the new rule `container_push_index` to push a docker image multi platforms. We can use this new rule like this: container_push_index( name = "push_index", format = "Docker", images = { "//:my_image_amd64": "linux/amd64", "//:my_image_arm64": "linux/arm/v6", "//:my_image_ppc64le": "ppc64le", }, registry = "localhost:5000", repository = "docker/myimage", tag = "{STABLE_VERSION}", )

lbcjbb · 2023-01-30T09:27:58Z

@Topher-the-Geek It's now fixed. Sorry for this stupid mistake. You can apply this patch in your test workspace:

diff --git a/WORKSPACE b/WORKSPACE
index 2e1ba82..91b5101 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -17,14 +17,10 @@ load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")
 
 bazel_skylib_workspace()
 
-RULES_DOCKER_VERSION = "c079c0ddad54d34205c0a94fbb7cf9c6e7a6f48b"
+RULES_DOCKER_VERSION = "19d30777c526fe9986e1a19b49324beb34537ce0"
 
 http_archive(
     name = "io_bazel_rules_docker",
-    patch_args = ["-p1"],
-    patches = [
-        "//:pr2087.patch",
-    ],
     strip_prefix = "bazel-rules_docker-%s" % RULES_DOCKER_VERSION,
     type = "zip",
     url = "https://github.com/leboncoin/bazel-rules_docker/archive/%s.zip" % RULES_DOCKER_VERSION,

Topher-the-Geek · 2023-01-30T12:37:56Z

Thanks for the quick fix @lbcjbb. For me, this is working as advertised. I'm not an owner of this repo tho, so we still need @uhthomas

tgeng · 2023-02-15T17:50:31Z

What does it take to get this merged? @uhthomas can you give an update?

lbcjbb · 2023-02-15T18:25:45Z

What does it take to get this merged?

I don't known. Maybe just some time to review, or find new maintainers of this repository (#2038).

lbcjbb requested review from gravypod, linzhp, pcj, smukherj1 and uhthomas as code owners May 14, 2022 12:46

lbcjbb mentioned this pull request Aug 21, 2022

Add Support for Multi-Arch and Multi-Platform Images #1599

Open

uhthomas suggested changes Jan 14, 2023

View reviewed changes

lbcjbb added 2 commits January 14, 2023 11:27

Refactor the rule container_push to split all shareable functions

6ebf90c

The main goal of this refactoring is to make available several functions. These functions will be used by the 'index' version of the `container_push` rule. Unless I am mistaken, the code is exactly the same.

lbcjbb requested review from uhthomas and removed request for pcj, linzhp, gravypod and smukherj1 January 14, 2023 21:10

Topher-the-Geek reviewed Jan 29, 2023

View reviewed changes

container/push.bzl Show resolved Hide resolved

lbcjbb added 3 commits January 30, 2023 10:19

Add tests for the new container_push_index rule

533e78e

Add docs for the new container_push_index rule

19d3077

yapishu mentioned this pull request Feb 15, 2023

Docker: Multi-Arch Support urbit/vere#226

Open

lbcjbb closed this Jul 5, 2023

lbcjbb deleted the feature/multi-arch-container-push branch July 5, 2023 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support index manifest to push multi-arch container images #2087

Support index manifest to push multi-arch container images #2087

lbcjbb commented May 14, 2022

lbcjbb commented May 17, 2022

aw185176 commented May 17, 2022

lbcjbb commented May 17, 2022 •

edited

Loading

nadavwe commented Sep 21, 2022

lbcjbb commented Sep 21, 2022

nadavwe commented Sep 21, 2022

drewcsillagdd commented Oct 17, 2022

lbcjbb commented Oct 18, 2022

samruthr commented Nov 18, 2022

lbcjbb commented Nov 18, 2022

uhthomas commented Nov 18, 2022

samruthr commented Dec 12, 2022

uhthomas left a comment •

edited

Loading

lbcjbb commented Jan 14, 2023 •

edited

Loading

loeffel-io commented Jan 27, 2023 •

edited

Loading

Topher-the-Geek left a comment

Topher-the-Geek commented Jan 29, 2023

lbcjbb commented Jan 30, 2023

Topher-the-Geek commented Jan 30, 2023

tgeng commented Feb 15, 2023

lbcjbb commented Feb 15, 2023

Support index manifest to push multi-arch container images #2087

Support index manifest to push multi-arch container images #2087

Conversation

lbcjbb commented May 14, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

lbcjbb commented May 17, 2022

aw185176 commented May 17, 2022

lbcjbb commented May 17, 2022 • edited Loading

nadavwe commented Sep 21, 2022

lbcjbb commented Sep 21, 2022

nadavwe commented Sep 21, 2022

drewcsillagdd commented Oct 17, 2022

lbcjbb commented Oct 18, 2022

samruthr commented Nov 18, 2022

lbcjbb commented Nov 18, 2022

uhthomas commented Nov 18, 2022

samruthr commented Dec 12, 2022

uhthomas left a comment • edited Loading

Choose a reason for hiding this comment

lbcjbb commented Jan 14, 2023 • edited Loading

loeffel-io commented Jan 27, 2023 • edited Loading

Topher-the-Geek left a comment

Choose a reason for hiding this comment

Topher-the-Geek commented Jan 29, 2023

lbcjbb commented Jan 30, 2023

Topher-the-Geek commented Jan 30, 2023

tgeng commented Feb 15, 2023

lbcjbb commented Feb 15, 2023

lbcjbb commented May 17, 2022 •

edited

Loading

uhthomas left a comment •

edited

Loading

lbcjbb commented Jan 14, 2023 •

edited

Loading

loeffel-io commented Jan 27, 2023 •

edited

Loading