Optimize debian-base image size #2113
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
size/S
|
hum, so maybe I need one PR for the base image and other for the iptables one? I'll then keep this only for the iptables one (which is our bigger consumer) and then we can deal with the Debian-base :D |
k8s-ci-robot
added
area/release-eng
|
does it not make sense to get debian-base merged and re-built in post-submit first to maximize savings in debian-iptables? |
|
@BenTheElder you mean, focusing this PR only in Debian-base and then opening a new one only for debian-iptables? I'm not sure what is the post submit job here :) |
|
but yeah, I can do this! |
I'd have to double check but I'm pretty sure we automatically run the image builds to push to staging whenever a PR merges to the image(s), so we'd want that to run for debian base and then for debian-iptables (so we need seperate merges, and we need to do a promotion PR for debian base inbetween) |
| @@ -16,7 +16,6 @@ ARG BASEIMAGE | |||
| FROM $BASEIMAGE | |||
|
|
|||
| ARG ARCH | |||
| COPY qemu-$ARCH-static /usr/bin/ | |||
There was a problem hiding this comment.
eventually tracked this back to https://github.com/kubernetes/kubernetes/blame/0f413e5940e18c947bf8e94e9f6c35a574de1baf/build/debian-base/Dockerfile.build#L20 (bit of a pain with the repo move and all),
still not clear why that was done even then. it looks like it was copied from debian-iptables back then kubernetes/kubernetes#41915
There was a problem hiding this comment.
kubernetes/kubernetes@560268e / kubernetes/kubernetes#21617
looks like this just predates the idea of running multiarch qemu setup seperately from the image build, these never needed to be in any of the component base images, as suspected ...
There was a problem hiding this comment.
so about this I'm also not sure why this is necessary (or if this is still necessary for cross builds).
I have a RPI3 here, maybe will try later to build the images on amd64 and check if they still work properly on arm64
There was a problem hiding this comment.
I don't believe this was ever necessary versus doing it in some process external to the image, which we definitely do now.
There was a problem hiding this comment.
This is safe to remove, I just wanted to understand how it ever got there in case we missed something, because it doesn't make much sense today 🙃
You can see the history above.
There was a problem hiding this comment.
@BenTheElder that was required before buildx to inject the qemu-user-static binary for the platform
https://github.com/kubernetes/kubernetes/blob/0f413e5940e18c947bf8e94e9f6c35a574de1baf/build/debian-base/Makefile#L83-L86
There was a problem hiding this comment.
@rikatz if you remove the binary, then you can also drop
release/images/build/debian-base/Makefile
Lines 81 to 83 in ae29abf
There was a problem hiding this comment.
I don't think it was required even before buildx, we do that by seperately running a qemu setup outside of the base image.
There was a problem hiding this comment.
@aledbf glad to see you here!!! :D
Yes, probably then this can be dropped in a lot of places around the repos :)
| @@ -18,15 +18,12 @@ FROM ${BASEIMAGE} | |||
|
|
|||
| # Install iptables and ebtables packages from buster-backports | |||
| ARG IPTABLES_VERSION | |||
| RUN ln -s /bin/rm /usr/sbin/rm \ | |||
There was a problem hiding this comment.
is dropping the symlink here intentional?
There was a problem hiding this comment.
Yes, in new Debian-base images the symlink already exists. Keeping it breaks the build. But as we are moving to create first a new Debian-base image and then the debian-iptables I'll revert everything here but the Debian-base things
|
@BenTheElder @aledbf Thanks for reviewing this. I'm gonna revert the iptables changes and leave the proper debian-base images changes later today, and once this gets merged and promoted do the same for debian-iptables image |
rikatz
changed the title
Signed-off-by: Ricardo Katz <rkatz@vmware.com>
rikatz
force-pushed
the
kproxy-img-optimization
branch
from
b86d2ae to
9d1b494
Compare
|
@BenTheElder as promised, debian-base only change right now :) |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, justaugustus, rikatz 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 |
k8s-ci-robot
added
the
approved
|
Image promotion PR: kubernetes/k8s.io#2185 |
|
Thanks Stephen! I'm on it and will remove this from my queue right now! :) |
Signed-off-by: Ricardo Katz rkatz@vmware.com
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
As discussed in kubernetes/kubernetes#102493 and per wg-k8s-infra as well, we can see the mostly used image is Kube-proxy.
This PR introduces some simple optimizations in debian-base image generation, removing the qemu-static file from it and reducing in 9Mb its size.
Next PR will be for Kube-proxy generation using the image promoted from thijs PR, and that changes can reduce its image in 30% of size
Which issue(s) this PR fixes:
Partially kubernetes/kubernetes#102493
Special notes for your reviewer:
Need to verify if the change of tags is also necessary, as this will replace the current images once promoted.
If this is the case later we can promote the images once both are generated
Does this PR introduce a user-facing change?