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

Build hybrid image #8476

Merged
merged 7 commits into from
Dec 10, 2024
Merged

Conversation

Lyndon-Li
Copy link
Contributor

Fix issue #8415, implement multi-arch build and Windows build

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@github-actions github-actions bot added the Area/Design Design Documents label Dec 3, 2024
@Lyndon-Li Lyndon-Li force-pushed the build-hybrid-image branch 2 times, most recently from b30674c to 4c1f3a7 Compare December 3, 2024 06:36
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.02%. Comparing base (04d6c79) to head (bcba234).
Report is 15 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8476   +/-   ##
=======================================
  Coverage   59.02%   59.02%           
=======================================
  Files         369      369           
  Lines       39056    39056           
=======================================
  Hits        23052    23052           
  Misses      14542    14542           
  Partials     1462     1462           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Lyndon-Li Lyndon-Li force-pushed the build-hybrid-image branch 9 times, most recently from 98fac9a to 5d32882 Compare December 3, 2024 08:04
@@ -42,8 +42,8 @@ jobs:
- name: Build Velero Image
if: steps.image-cache.outputs.cache-hit != 'true'
run: |
IMAGE=velero VERSION=pr-test make container
docker save velero:pr-test -o ./velero.tar
IMAGE=velero VERSION=pr-test BUILD_OUTPUT_TYPE=docker make container
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At present, CI E2E supports Linux only, opened a separate issue #8477 for requirements of running it in Windows.

@Lyndon-Li Lyndon-Li marked this pull request as ready for review December 3, 2024 08:29
@Lyndon-Li Lyndon-Li requested a review from ywk253100 December 3, 2024 08:29
Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

I had to use following to run

BUILDX_OSVERSION=ltsc2025 BUILDX_ARCH=amd64 make container-windows

We should add overridable ?= default to Makefile.

Also I thought we are also moving away from BUILDX_ to BUILD_? per last comment in design?

Dockerfile-Windows Outdated Show resolved Hide resolved
@Lyndon-Li
Copy link
Contributor Author

I had to use following to run

BUILDX_OSVERSION=ltsc2025 BUILDX_ARCH=amd64 make container-windows

We should add overridable ?= default to Makefile.

No, if you want to build Windows image only, the command is BUILD_OUTPUT_TYPE=registry REGISTRY=xxx BUILD_OS=windows make container.
And we already have default values for all the input parameters, merely, the default values are not for building a Windows image. Check the Sample section in the design for details.

Also I thought we are also moving away from BUILDX_ to BUILD_? per last comment in design?

The latest code in this PR has changed to BUILD_, please double check

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li Lyndon-Li requested a review from kaovilai December 4, 2024 02:30
Comment on lines +223 to +224
-docker buildx rm velero-builder || true
@docker buildx create --use --name=velero-builder
Copy link
Member

Choose a reason for hiding this comment

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

This overrides my current buildx builder settings. ie. I already have a kubernetes docker builder I intended to use on a more powerful system.

Can we do it in currently available builders that our docker is already set to use?

I think this can be doc as a pre-step. It is also not compatible with podman for example.

~/git/velero build-hybrid-image 1m 44s
❯ podman buildx ls
Error: unrecognized command `podman buildx ls`
Try 'podman buildx --help' for more information

~/git/velero build-hybrid-image
❯ podman buildx rm
Error: unrecognized command `podman buildx rm`
Try 'podman buildx --help' for more information

~/git/velero build-hybrid-image
❯ podman buildx create
Error: unrecognized command `podman buildx create`
Try 'podman buildx --help' for more information

Copy link
Member

Choose a reason for hiding this comment

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

BUILD_OUTPUT_TYPE=registry REGISTRY=xxx BUILD_OS=windows make container

other than that this did build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified the code to accept specified buildx instance by BUILDX_INSTANCE input parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the podman part, as far as my understanding, there is no conclusions/actions to support podman emulated docker purposefully. Looks like the current Makefile works, it may be a coincidence.
Anyway, with BUILDX_INSTANCE input parameter set, we don't add any new docker buildx call, hopefully this could ensure podman still work as is.
However, supporting podman is not something must-have with/without this PR. If podman is required after our evaluation, there should be separate issue and PR for the necessary changes based on this PR.

Copy link
Contributor Author

@Lyndon-Li Lyndon-Li Dec 6, 2024

Choose a reason for hiding this comment

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

For the doc part, there is a separate issue for build-from-source doc changes related to Windows support #8475, so there will be a separate PR for the same which will include the changes mentioned here.

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li Lyndon-Li requested a review from kaovilai December 6, 2024 07:54
@Lyndon-Li Lyndon-Li merged commit ff6ea15 into vmware-tanzu:main Dec 10, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants