Skip to content

Conversation

vyasgun
Copy link
Member

@vyasgun vyasgun commented May 9, 2025

Builds separate images for amd64 and arm64, creates a manifest of the images, and pushes the multiarch
manifest to the registry

Copy link
Contributor

openshift-ci bot commented May 9, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vyasgun
Once this PR has been reviewed and has the lgtm label, please assign evidolob for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vyasgun vyasgun force-pushed the pr/gvforwarder-multiarch branch 3 times, most recently from 6b2e714 to 3423a91 Compare May 9, 2025 14:53
Builds separate images for amd64 and arm64, creates a
manifest of the images, and pushes the multiarch
manifest to the registry

Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>
@vyasgun vyasgun force-pushed the pr/gvforwarder-multiarch branch from 9ca1b2d to ecac20c Compare May 9, 2025 14:58
@gbraad
Copy link
Member

gbraad commented May 12, 2025

This relies on qemu-user-static to provide an emulated container build. Is this approach we want? On other crc projects we use native runners for the container assets. This is why images/Dockerfile does a plain make build, as that means the arch specification controls the target. runs-on with an upload and aggregation job, can solve this: https://gist.github.com/gbraad/f8a46c5958d158707d18a333a414a900

@@ -50,11 +52,19 @@ lint: $(TOOLS_BINDIR)/golangci-lint

.PHONY: image
image:
${CONTAINER_RUNTIME} build -t quay.io/crcont/gvisor-tap-vsock:$(TAG) -f images/Dockerfile .
${CONTAINER_RUNTIME} build --arch amd64 -t $(IMAGE)-amd64 -f images/Dockerfile .
${CONTAINER_RUNTIME} build --platform linux/arm64 -t $(IMAGE)-arm64 -f images/Dockerfile .
Copy link
Member

@gbraad gbraad May 12, 2025

Choose a reason for hiding this comment

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

Why is this not:

	${CONTAINER_RUNTIME} build [--arch amd64|--platform linux/amd64] -t $(IMAGE)-amd64 -f images/Dockerfile .
	${CONTAINER_RUNTIME} build [--arch arm64|--platform linux/arm64] -t $(IMAGE)-arm64 -f images/Dockerfile .

to be more consistent in the way this is written. I think you might experience issues between steps that the image downloaded is architecture specific, and therefore needs to be removed to be correctly handled.

As this seems to assume, it was build from an Arm-based machine.

Note: this will also build for one of the architectures using emulation, as the images/Dockerfile performs a RUN make. This can be slow and not according to what we normally do for 'secondary architectures' (only allow native builds or cross compilation).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe try if a make cross and create for --arch --platform and a cp -a would work? This might be a way to actually prevent it to 'run' the image under virtualization/emulation.

Copy link
Member

Choose a reason for hiding this comment

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

I still believe this is the wrong approach. This could work for development, but not for release.

For a release, the process can run on a native runner; perform a native build with the Dockerfile, aggregate files in a job and publish the manifest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally we would be building on native runners/… May not be that hard using https://github.com/gbraad-dotfiles/upstream/blob/main/.github/workflows/reusable-build-and-push-containers.yml#L43-L76

However, the container image we use at the moment hasn’t been updated in 2 years, so (in my opinion) any automation is an improvement, even if it involves qemu-user-aarch64.

We can also do this in multiple steps, do something that works, and then remove the use of the emulation code. Or delay this by a few days, as if we can finish crc-org/snc#1052 then the container image is no longer that useful.

@gbraad gbraad mentioned this pull request May 13, 2025
Copy link
Member

@gbraad gbraad left a comment

Choose a reason for hiding this comment

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

Since this pushes to a public endpoint, this is a release process.
This solution does not get my preference as it involves emulation.

@gbraad
Copy link
Member

gbraad commented May 13, 2025

For now; let's focus on crc-org/snc#1052 as that can remove the need for this container image. There is still a podman cp that pulls the executable out of this image; that has to become an RPM or so.

Note: quay.io/crcont/gvisor-tap-vsock:${TAG} is very specific for the CRC/SNC usecase and should not belong in the upstream repo.

@gbraad gbraad requested a review from Copilot May 13, 2025 12:49
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces multiarch support by building separate images for amd64 and arm64, creating a manifest for these images, and pushing the manifest to the registry.

  • Adds an IMAGE variable and new build commands for both architectures.
  • Implements manifest creation, cleanup, and push steps.

@cfergeau
Copy link
Collaborator

With crc-org/snc#1052 merged, we no longer need this.

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.

None yet

3 participants