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

docker images: cross build go binaries #3214

Merged
merged 3 commits into from
May 11, 2023

Conversation

BenTheElder
Copy link
Member

huge build speed-up for the arm64 build on amd64 hosts and vice versa

Notable change: Switched to debian base image from ubuntu. They have comparable enough packages but debian has much better cross platform package support.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 10, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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 requested review from aojea and neolit123 May 10, 2023 23:57
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 10, 2023
@stmcginnis
Copy link
Contributor

/lgtm

Nice!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023
@BenTheElder
Copy link
Member Author

Found and fixed bug in kindnetd and local-path-provisioner changes.

I've built all of these locally now with make -C images/$image which builds both platforms ...

@@ -90,15 +90,18 @@ RUN echo "Enabling kubelet and containerd services ... " \
RUN echo "Ensuring /etc/kubernetes/manifests" \
&& mkdir -p /etc/kubernetes/manifests

RUN echo "Adjusting systemd-tmpfiles timer" \
&& sed -i /usr/lib/systemd/system/systemd-tmpfiles-clean.timer -e 's#OnBootSec=.*#OnBootSec=1min#'
Copy link
Member Author

Choose a reason for hiding this comment

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

the timer value is in a different location and already reasonable in the debian image

@@ -20,7 +20,7 @@
# start from ubuntu, this image is reasonably small as a starting point
# for a kubernetes node image, it doesn't contain much we don't need
# this stage will install basic files and packages
ARG BASE_IMAGE=ubuntu:22.04
ARG BASE_IMAGE=debian:bullseye-slim
Copy link
Member Author

Choose a reason for hiding this comment

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

kubernetes's own debian-base image has broken held packages, I'm not sure it would actually make any more sense here. debian:.*-slim is already quite compact and seems well-maintained.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was pleasantly surprised to find that bullseye-slim even has the same iptables version as the existing image, and the build output looks reasonably sized :-)

If we find we need newer packages, we can switch to debian testing track or bullseye backports. It looks like everything in bullseye should be new enough though and this more closely aligns with k8s core images.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, this also will keep aligned kube-proxy and kindnetd iptables

Copy link
Member Author

Choose a reason for hiding this comment

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

The distro switch didn't quite work out smoothly, #3218

@aojea
Copy link
Contributor

aojea commented May 11, 2023

/lgtm

It took me a while to understand it, until I realized this is coming from https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/

There is a lot of magic here with the special environment variables, it is good to keep a reference to the docker document at least in the PR comments

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023
ARG CNI_PLUGINS_VERSION="v1.2.0"
ARG CNI_PLUGINS_CLONE_URL="https://github.com/containernetworking/plugins"
RUN git clone --filter=tree:0 "${CNI_PLUGINS_CLONE_URL}" /cni-plugins \
&& cd /cni-plugins \
&& git checkout "${CNI_PLUGINS_VERSION}" \
&& eval "$(gimme "${GO_VERSION}")" \
&& mkdir ./bin \
&& export GOARCH=$TARGETARCH && export CC=$(target-cc) && export CGO_ENABLED=1 \
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we actually want CGO for these.

We do for containerd, runc. Maybe not for the others.
Thought most of them can already control that in their makefile etc. This is the only set we're just go build ing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it will cause problems, anyhow, and it looks like the usual builds do not disable CGO.

CGO is disabled by default when attempting to cross compile with go by setting GOARCH though

https://github.com/containernetworking/plugins/blob/38f18d26ecfef550b8bac02656cc11103fd7cff1/build_linux.sh#L20

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this will impact only the libc binding and mostly dns resolution, since we control the image it should be relatively safe, I honestly can't say what is better, but the cni plugins and crictl are small programs

Copy link
Member Author

Choose a reason for hiding this comment

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

crictl we're using the makefile anyhow so if they typically release/test with static that should remain the case.

for this one where we're go build ourselves it's worth double checking if that build is comparable to the released binaries. AFAICT it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

since we control the image it should be relatively safe

yeah, that's a motivating factor in #3201 proceeding this, ran into some issues working on an image build and glibc symbol mismatch with external cgo binaries

@k8s-ci-robot k8s-ci-robot merged commit dc714da into kubernetes-sigs:main May 11, 2023
@BenTheElder
Copy link
Member Author

There is a lot of magic here with the special environment variables, it is good to keep a reference to the docker document at least in the PR comments

Yes, FWIW in most places I think I left a comment about "buildx sets this" or "makefile sets this" for the ARGs, but that link is an excellent reference for this sort of multi-stage cross compilation and all the built-in envs.

@BenTheElder BenTheElder deleted the xbuild branch May 11, 2023 06:56
@BenTheElder
Copy link
Member Author

base image built in only 5 minutes instead of hour+ timeout :-)

local-path-provisioner in 4 minutes

kindnetd in 3.5 minutes

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants