-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 support for building multi-arch images #3141
Conversation
Hi @tstapler. Thanks for your PR. I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
@@ -1,5 +1,7 @@ | |||
FROM ppc64le/alpine:3.15 | |||
MAINTAINER dashpole@google.com lysannef@us.ibm.com | |||
# Deprecated: the Dockerfile in this directory should support ppc64le |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to remove this file now. I added the deprecation message to be cautious about breaking people's workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, sounds good!
# ipmctl only supports Intel x86_64 processors. | ||
# https://github.com/intel/ipmctl/issues/163 | ||
RUN if [ "$(uname --machine)" = "x86_64" ]; then \ | ||
git clone -b v02.00.00.3885 https://github.com/intel/ipmctl/ && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to update from 3820 to 3885 to fix a compiler warning which caused errors intel/ipmctl#169
|
||
RUN apk --no-cache add libc6-compat device-mapper findutils zfs build-base linux-headers python3 bash git wget cmake pkgconfig ndctl-dev && \ | ||
# Install build depdencies for all supported arches | ||
RUN apk --no-cache add bash build-base cmake device-mapper findutils git \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependencies should be identical, I just alphabetically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for cleanup!
echo "Release info (copy to the release page):" | ||
echo | ||
echo "Docker Image: N/A" | ||
echo "gcr.io Image: $gcr_tag" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if I should build a similar release info page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great, to make it simpler to cut releases.
If we can just include the gcr tag, hash, and hashes of the cadvisor binaries, it's all we need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright the latest version should print out a summary. I updated the releasing.md file to reflect the changes.
/ok-to-test |
build/build.sh
Outdated
@@ -16,11 +16,15 @@ | |||
|
|||
set -e | |||
|
|||
declare -A arches=( ["amd64"]="x86_64" ["arm"]="armv7l" ["arm64"]="aarch64" ["ppc64le"]="ppc64le" ["s390x"]="s390x" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, let's just skip to amd64 armv7 and arm64 since these are the most popular ones and we can try to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this to only include the architectures listed above.
arch_specific_image="${image_name}-${arch}:${VERSION}" | ||
docker buildx build --platform "linux/${arch}" -f deploy/Dockerfile -t "$arch_specific_image" --progress plain --push . | ||
docker manifest create --amend "$final_image" "$arch_specific_image" | ||
docker manifest annotate --os=linux --arch="$arch" "$final_image" "$arch_specific_image" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, for my own understanding ,why do we need to use docker manifest annotate
here? Does docker buildx not do this automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker buildx build
will create a multi-arch manifest if you specify multiple platforms --platform linux/amd64,linux/arm64
and the --push
flag.
However, it does not tag intermediate containers like cadvisor-arm64
. It makes it a little harder when you want to test a specific architecture locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah gotcha, thanks for the explanation!
Thank you @tstapler for all your work on this! Please update https://github.com/google/cadvisor/blob/master/docs/development/releasing.md with the new details (since |
The |
dbb963e
to
8d763d1
Compare
ADD go.mod go.sum ./ | ||
RUN go mod download | ||
ADD cmd/go.mod cmd/go.sum ./cmd/ | ||
RUN cd cmd && go mod download |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added caching of the go deps so that if you want to repeatedly build the docker container locally it will be pretty quick.
unset GO_FLAGS | ||
build/build.sh | ||
docker buildx inspect cadvisor-builder > /dev/null \ | ||
|| docker buildx create --name cadvisor-builder --use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this builder persistent, it helps with local testing because the unchanged parts of the image are cached. Let me know if you want me to return to deleting the builder afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
fi | ||
fi | ||
|
||
read -p "Please confirm: $VERSION is the desired version (Type y/n to continue):" -n 1 -r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having the user confirm the version at the end, I added a prompt here.
docs/development/releasing.md
Outdated
``` | ||
|
||
## 5. Cut the release | ||
## 4. Cut the release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add running check_container.sh
as a step to confirm that you can start all of the containers that were generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea, let's add it as validation step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the validation step
declare -A arches=( ["amd64"]="x86_64" ["arm"]="arm" ["arm64"]="aarch64") | ||
|
||
for arch in "${arches[@]}"; do | ||
if ! hash "qemu-${arch}-static"; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a check to see if qemu was installed to prevent people being unable to release and not understanding why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this check, will help alot to debug
a30ca19
to
612866b
Compare
LGTM! |
Only install libipmctl for amd64 versions of the docker image. Updated ipmctl to v02.00.00.3885 to fix intel/ipmctl#169 Updated base image to alpine:1.16 from alpine:1.15 to add zfs support on "arm" architecture. Co-authored-by: David Porter <porterdavid@google.com> Signed-of-by: Tyler Stapler <tystapler@gmail.com>
I think I've addressed all comments now. |
612866b
to
56ac6ef
Compare
@tstapler: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
LGTM! |
Add support for building multi-arch images [#3141](google/cadvisor#3141) - cAdvisor v0.45.0 now has a multi arch image
I took a prototype by @bobbypage and added support for additional architectures, simplified the docker build, and wrote a script to test the images generated by release.sh.
Note: My GCR repository is private you'll have to use your own container registry that you have permissions to.
Starting each of the images and curling the healthz endpoint:
Co-authored-by: David Porter porterdavid@google.com
Signed-of-by: Tyler Stapler tystapler@gmail.com