Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multi-platform support for macvtap cni image builds. #127

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

ashokpariya0
Copy link
Contributor

These changes enable building and pushing macvtap container images for multiple platforms (amd64, s390x, arm64) from a single Dockerfile. Enhanced multi-platform support in the build process by adding a PLATFORMS argument in the Makefile for amd64, s390x, and arm64 architectures. Multi-platform build support is provided for both Docker and Podman container runtimes.

What this PR does / why we need it:
The Macvtap CNI image, available at [quay.io/kubevirt/macvtap-cni](https://quay.io/repository/kubevirt/macvtap-cni?tab=tags&tag=latest), currently supports only the amd64 architecture.
This PR introduces the necessary changes for multi-platform container image building and pushes for the macvtap-cni project, including updates Makefile, and Dockerfile to support building and pushing the image for multiple architectures (amd64, arm64, s390x).

Special notes for your reviewer:
More details Build instruction and variable used is available at: https://gist.github.com/ashokpariya0/0424fb9022887c44b4da5b3f921f3df4

Release note:

None

These changes enable building and pushing macvtap container images for multiple
platforms (amd64, s390x, arm64) from a single Dockerfile.
Enhanced multi-platform support in the build process by adding a PLATFORMS
argument in the Makefile for amd64, s390x, and arm64 architectures.
Multi-platform build support is provided for both Docker and Podman container runtimes.

Signed-off-by: Ashok Pariya <[email protected]>
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 10, 2024
@kubevirt-bot kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 10, 2024
@kubevirt-bot
Copy link
Collaborator

Hi @ashokpariya0. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@ashokpariya0
Copy link
Contributor Author

cc @oshoval

@oshoval
Copy link
Contributor

oshoval commented Dec 10, 2024

lets combine it with #125 and remove please the git actions from there?

@ashokpariya0
Copy link
Contributor Author

lets combine it with #125 and remove please the git actions from there?

No, Whatever is needed is there in this PR, I plan to verify both the GitHub Actions and make command approaches for multi-platform support in parallel as a follow-up. Can we proceed with this PR for now?

@oshoval
Copy link
Contributor

oshoval commented Dec 10, 2024

lets combine it with #125 and remove please the git actions from there?

No, Whatever is needed is there in this PR, I plan to verify both the GitHub Actions and make command approaches for multi-platform support in parallel as a follow-up. Can we proceed with this PR for now?

I am against adding git actions atm, i don't have the capacity to review stuff related for that
duplicity isn't good to have as well, so it will require more changes which we don't need atm
prow is the one that release it right now, lets please stick to that

EDIT will focus on this PR now, understanding it is self contained for prow

Makefile Show resolved Hide resolved
@@ -11,6 +11,7 @@
source automation/check-patch.setup.sh
cd ${TMP_PROJECT_PATH}

export PLATFORMS=all
Copy link
Contributor

Choose a reason for hiding this comment

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

this is bit tricky, because it will run on post submit, so harder to test it
we can just hope for best and do manual tests before that / fix according needs

@oshoval
Copy link
Contributor

oshoval commented Dec 10, 2024

/lgtm

it was tested manually on host amd64, podman for "all" right ?
important because this what prow going to do on post submit iiuc

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2024
@ashokpariya0
Copy link
Contributor Author

it was tested manually on host amd64, podman for "all" right ? important because this what prow going to do on post submit iiuc

Yes, I have tested this on an amd64 machine, where it successfully built multi-architecture images for amd64, arm64, and s390x. Additionally, I have verified that the image builds and works correctly on a s390x machine as well.

@ashokpariya0
Copy link
Contributor Author

@maiqueb @oshoval All tests have passed for this PR. If everything looks good to you and no further changes are needed, could we please proceed with PR?

@maiqueb
Copy link
Collaborator

maiqueb commented Dec 12, 2024

/approve

@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashokpariya0, maiqueb

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

The pull request process is described 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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2024
@kubevirt-bot kubevirt-bot merged commit f115287 into kubevirt:main Dec 12, 2024
5 checks passed
@ashokpariya0
Copy link
Contributor Author

@maiqueb Thank you so much! The multi-platform supported macvtap-cni images are now available at this link. I quickly verified the image on s390x, and it’s working fine.
cc @oshoval @phoracek

@oshoval
Copy link
Contributor

oshoval commented Dec 12, 2024

Thanks for your effort

we would need a tag please so CNAO will consume it

@oshoval
Copy link
Contributor

oshoval commented Dec 12, 2024

seems you created a new tag v0.11.2 ?
if so it seems it didnt create a new release for that (according https://github.com/kubevirt/macvtap-cni/releases)

i just see latest here
https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/logs/publish-macvtap-cni/1867179080718225408
which belong to this PR, not to the tag

@maiqueb
Copy link
Collaborator

maiqueb commented Dec 12, 2024

Yes, I totally forgot how this repo worked.

I'll issue a new release now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants