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

chore: adopt buildx bake when building container images #1145

Closed
wants to merge 1 commit into from
Closed

chore: adopt buildx bake when building container images #1145

wants to merge 1 commit into from

Conversation

MartinForReal
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

adopt buildx bake to boost image building process

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

May introduce regression to e2e tests.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MartinForReal

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2022
@coveralls
Copy link

coveralls commented Feb 15, 2022

Coverage Status

Coverage remained the same at 80.182% when pulling 81a75d0 on MartinForReal:chores/adopt_buildx_bake into 10a510e on kubernetes-sigs:master.

@MartinForReal MartinForReal changed the title chore: adopt buildx bake when build container images chore: adopt buildx bake when building container images Feb 15, 2022
@MartinForReal
Copy link
Contributor Author

/assign @nilo19

@MartinForReal
Copy link
Contributor Author

/cc @lodrem

@k8s-ci-robot
Copy link
Contributor

@MartinForReal: GitHub didn't allow me to request PR reviews from the following users: lodrem.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @lodrem

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/test-infra repository.

buildx-setup:
docker buildx inspect img-builder > /dev/null || docker buildx create --name img-builder --use
# enable qemu for arm64 build
# https://github.com/docker/buildx/issues/464#issuecomment-741507760
docker run --privileged --rm tonistiigi/binfmt --uninstall qemu-aarch64
docker run --rm --privileged tonistiigi/binfmt --install all

.PHONY: build-ccm-image
build-ccm-image: buildx-setup docker-pull-prerequisites ## Build controller-manager image.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we keep this or refine it to build for the specified arch/platform? The internal pipeline may depend on this (build and export to tar).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build-ccm-image-% target is added. e.g make build-ccm-image-amd64

Copy link
Contributor

Choose a reason for hiding this comment

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

same case for cnm, and we already use the following syntax to build cnm to mcr.
OUTPUT_TYPE="docker,dest=$mydir/azure-cloud-node-manager.tar" ARCH=$(arch) make build-node-image-linux

buildx-bake.hcl Outdated
}

group "default" {
targets = ["ccm-amd64","ccm-arm64","ccm-arm","cnm-amd64","cnm-arm64","cnm-arm"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this also support the Windows targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@MartinForReal
Copy link
Contributor Author

/cc @AbelHu

@k8s-ci-robot
Copy link
Contributor

@MartinForReal: GitHub didn't allow me to request PR reviews from the following users: AbelHu.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @AbelHu

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/test-infra repository.

@MartinForReal
Copy link
Contributor Author

/assign @feiskyer

@MartinForReal
Copy link
Contributor Author

MartinForReal commented Feb 16, 2022

/cc @mainred

Copy link
Contributor

@mainred mainred left a comment

Choose a reason for hiding this comment

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

Thanks for introducing buildx baker, left several comments.

buildx-setup:
docker buildx inspect img-builder > /dev/null || docker buildx create --name img-builder --use
# enable qemu for arm64 build
# https://github.com/docker/buildx/issues/464#issuecomment-741507760
docker run --privileged --rm tonistiigi/binfmt --uninstall qemu-aarch64
docker run --rm --privileged tonistiigi/binfmt --install all

.PHONY: build-ccm-image
build-ccm-image: buildx-setup docker-pull-prerequisites ## Build controller-manager image.
Copy link
Contributor

Choose a reason for hiding this comment

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

same case for cnm, and we already use the following syntax to build cnm to mcr.
OUTPUT_TYPE="docker,dest=$mydir/azure-cloud-node-manager.tar" ARCH=$(arch) make build-node-image-linux

Makefile Outdated

build-ccm-image-%:
ENABLE_GIT_COMMAND="$(ENABLE_GIT_COMMAND)" VERSION="$(VERSION)" OUTPUT=registry docker buildx bake --pull --progress plain -f buildx-bake.hcl ccm-$*
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto and same for the build-cnm-image-% you are going to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works.

@MartinForReal
Copy link
Contributor Author

/retest

1 similar comment
@MartinForReal
Copy link
Contributor Author

/retest


.PHONY: build-node-image-linux
build-node-image-linux: buildx-setup
Copy link
Contributor

Choose a reason for hiding this comment

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

OUTPUT_TYPE="docker,dest=$(image.release.dir)/linux/$(arch)/azure-cloud-node-manager.tar" ARCH=$(arch) make build-node-image-linux is used to build cnm with specific arch, can we support this?

https://dev.azure.com/AzureContainerUpstream/Kubernetes/_git/ado-build-definitions?path=/pipelines/cloud-provider.azure.build.yml&version=GBmaster&line=53&lineEnd=54&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMAGE_REGISTRY=martindlut OUTPUT=registry make build-node-image-linux can be used to build image for all platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to build arm64 cnm image alone? Through the buildx baker target you provided "cnm-linux-amd64","cnm-linux-arm64","cnm-linux-arm",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try build-node-image-linux

@MartinForReal
Copy link
Contributor Author

/retest


.PHONY: build-node-image-linux
build-node-image-linux: buildx-setup
ENABLE_GIT_COMMAND="$(ENABLE_GIT_COMMAND)" VERSION="$(VERSION)" OUTPUT="$(OUTPUT_TYPE)" IMAGE_TAG="$(IMAGE_TAG)" docker buildx bake --pull --progress plain -f buildx-bake.hcl cnm-linux-amd64
Copy link
Contributor

Choose a reason for hiding this comment

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

docker buildx bake --pull --progress plain -f buildx-bake.hcl cnm-linux-amd64, -amd64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It is amd64 only

tags = [
"${IMAGE_REGISTRY}/azure-cloud-node-manager:${IMAGE_TAG}-windows-ltsc2022-amd64"
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: an empty line here.

@MartinForReal
Copy link
Contributor Author

/test pull-cloud-provider-azure-e2e-ccm-capz

1 similar comment
@MartinForReal
Copy link
Contributor Author

/test pull-cloud-provider-azure-e2e-ccm-capz

JoelSpeed pushed a commit to JoelSpeed/cloud-provider-azure that referenced this pull request Feb 18, 2025
🐛 Handle pointer to alias in DeepCopy
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants