Skip to content

Enable multi arch images - #400

Open
rakyll wants to merge 1 commit into
mainfrom
ko-target
Open

rakyll wants to merge 1 commit into
mainfrom
ko-target

Conversation

@rakyll

@rakyll rakyll commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Fixes #358.

Comment thread Makefile
TASK_RUNNER_REPO ?= $(AX_IMAGE_REPO)/ax-task-runner
CONTAINER_CLI ?= $(shell which podman 2>/dev/null || which docker 2>/dev/null)
KO_PLATFORM ?= linux/amd64,linux/arm64
TASK_RUNNER_PLATFORMS ?= $(KO_PLATFORM)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we keep TASK_RUNNER_PLATFORMS independent from KO_PLATFORM here?

These variables control two different build paths (ko for the Go services and the task-runner's Go/Docker build). Coupling them means changing KO_PLATFORM implicitly changes the task-runner build targets as well.

I think explicit defaults would make the interface clearer and avoid surprising behavior:

KO_PLATFORM ?= linux/amd64,linux/arm64
TASK_RUNNER_PLATFORMS ?= linux/amd64,linux/arm64

If the intention is to deliberately share this configuration, could we document that relationship?

Comment thread Makefile
GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o bin/linux_amd64/ax-task-runner ./cmd/ax-task-runner
@echo "==> Cross-compiling ax-task-runner for $(TASK_RUNNER_PLATFORMS)..."
@for p in $$(echo $(TASK_RUNNER_PLATFORMS) | tr ',' ' '); do \
arch=$$(basename $$p); \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we avoid deriving GOARCH with basename from the OCI platform string?

This assumes every supported platform maps directly to a Go architecture and can break for valid platform specifications containing additional components, e.g. linux/arm/v7 or linux/arm64/v8.

Since this value is ultimately passed to GOARCH, I'd prefer either explicitly maintaining the supported Go architectures (amd64, arm64) or adding validation/mapping from platform → GOARCH before invoking go build.

Comment thread Makefile
@if [ "$$(basename $(CONTAINER_CLI))" = "podman" ]; then \
$(CONTAINER_CLI) manifest push $(TASK_RUNNER_REPO):latest $(TASK_RUNNER_REPO):latest 2>/dev/null || $(CONTAINER_CLI) push $(TASK_RUNNER_REPO):latest; \
else \
$(CONTAINER_CLI) buildx build --platform $(TASK_RUNNER_PLATFORMS) -t $(TASK_RUNNER_REPO):latest -f Dockerfile.task-runner --push .; \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we clarify the intended output of this buildx build?

With multiple platforms, BuildKit cannot generally load the resulting multi-platform image into the classic local Docker image store as a normal image. This target also doesn't use --push or an explicit output.

push-task-runner subsequently performs another multi-platform build with --push, so make build-task-runner and make push-task-runner now have somewhat surprising semantics and the image may be built twice.

Could we make the build/publish flow explicit—for example, have the build target use an appropriate local/export output and have the push target be the single publishing path?

Comment thread Makefile
@echo "==> Pushing task runner image to $(TASK_RUNNER_REPO):latest..."
$(CONTAINER_CLI) push $(TASK_RUNNER_REPO):latest
@if [ "$$(basename $(CONTAINER_CLI))" = "podman" ]; then \
$(CONTAINER_CLI) manifest push $(TASK_RUNNER_REPO):latest $(TASK_RUNNER_REPO):latest 2>/dev/null || $(CONTAINER_CLI) push $(TASK_RUNNER_REPO):latest; \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we avoid suppressing the manifest-push error here?

2>/dev/null || ... can hide the actual reason the multi-platform manifest push failed and then fall back to a different push operation. For a release/deployment target, that can make an architecture-related publishing failure difficult to diagnose.

I'd prefer failing with the original error, or using an explicit Podman/Docker-specific publishing path with clear failure semantics.

@ashishsinghbora ashishsinghbora left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rakyll Overall, this is a good direction and it addresses the core problem in #358: the previous build/deploy path was hardcoded to linux/amd64, while this change adds amd64/arm64 support to the task runner and passes multiple platforms to ko.

The TARGETARCH change in Dockerfile.task-runner and the ko --platform changes are straightforward and aligned with the intended fix.

Before merging, I think the build/publish workflow needs some tightening:

  1. TASK_RUNNER_PLATFORMS is coupled to KO_PLATFORM even though they control separate build systems.
  2. basename is being used to convert OCI platform strings into GOARCH, which assumes a narrower platform format than the variable name suggests.
  3. The Docker Buildx multi-platform build has no explicit --push or --load, while push-task-runner builds the image again. This makes the target semantics unclear and can result in duplicate builds.
  4. The manifest push suppresses the underlying error, which makes release failures harder to diagnose.
  5. There is currently no automated verification that both architectures are actually produced and published correctly.

I would address these points and add a small CI/build verification for linux/amd64 and linux/arm64. With those changes, the multi-architecture support would be much easier to reason about and maintain.

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.

Makefile/ko build targets hardcode linux/amd64, breaking arm64 clusters

2 participants