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

cilium, docker: switch to {clang,llvm}-10.0 and externalize build deps #11308

Merged
merged 9 commits into from May 5, 2020

Conversation

borkmann
Copy link
Member

@borkmann borkmann commented May 4, 2020

Needs #11199 to be merged first.

Still wip, I'll also merge #10675 into here.

@maintainer-s-little-helper
Copy link

Commit 6d95a92 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note labels May 4, 2020
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 4, 2020
@borkmann borkmann added the release-note/misc This PR makes changes that have no direct user impact. label May 4, 2020
@borkmann borkmann added area/docker Impacts the integration with Docker. dont-merge/needs-release-note labels May 4, 2020
@coveralls
Copy link

coveralls commented May 4, 2020

Coverage Status

Coverage decreased (-0.02%) to 44.429% when pulling 2ad88b1 on pr/docker-rework into 9f32546 on master.

Lets not generate unnecessary layers in the docker image since they
all add up space. Noticed via dive.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 5, 2020
@borkmann borkmann marked this pull request as ready for review May 5, 2020 11:28
@borkmann
Copy link
Member Author

borkmann commented May 5, 2020

(still need to wait for quay's llvm build before ci testing)

Move both into a single layer instead of two to avoid bloating the
cilium runtime image size.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
They have been moved to https://github.com/cilium/packaging/ repo.
I would have much rather liked that we ship them as well under
Cilium's contrib/packaging/docker/ to have everything in one place
but this would be hitting a Github limitation seen on quay.io where
it cannot link the build trigger with an error "Cannot active build
trigger [...] The "push" event cannot have more than 20 hooks".

Anyway, from quay.io side we have 3 new scratch images with the built
binaries as follows:

  - quay.io/cilium/cilium-llvm:yyyy-mm-dd
    containing: /bin/clang /bin/llc
  - quay.io/cilium/cilium-bpftool:yyyy-mm-dd
    containing: /bin/bpftool
  - quay.io/cilium/cilium-iproute2:yyyy-mm-dd
    containing: /bin/tc /bin/ip

This also updates LLVM to the official 10.0 release.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
This allows to consolidate all COPY to /bin to use the temporary tools
image as source and therefore reduces Docker layers.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Move runtime over to 2020-05-05 for pulling in all base image changes.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
There is no need to keep its symbols around.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann borkmann requested a review from a team May 5, 2020 12:38
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

One question and a suggestion, but feel free to ignore. The changes look good, thank you!

make \
pkg-config \
python \
rsync \
unzip \
wget \
Copy link
Member

Choose a reason for hiding this comment

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

Could we replace wget with curl where it's needed and get rid of it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, the rsync/unzip/curl/wget/zip combo is not quite clear to me yet where it's needed, I'll see to reduce it further.

Comment on lines +23 to +24
CLANG ?= clang
LLC ?= llc
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

Suggested change
CLANG ?= clang
LLC ?= llc
CLANG ?= clang$LLVM_VERSION
LLC ?= llc$LLVM_VERSION

(Personal preference: When I have to append a version number for clang, I usually have to append it for llc as well. Then I can run LLVM_VERSION=-11 make instead of CLANG=clang-11 LLC=llc-11 make.)

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 changing that part, but your suggestion makes sense. I think others like init.sh and the golang loader would be needed as well. Alternatively, one could mount a docker volume to /bin/llc and /bin/clang in the cilium runtime image to test different versions.

And then don't install {clang,llvm}-7, iproute2 and ca-certificates
as not needed given runtime image has them as base as well. This also
ensures we don't need to update dependencies twice and we always use
the very same underlying tooling when building e.g. BPF programs.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Dust off things we do not need for building the Cilium image.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Move builder over to 2020-05-05 for pulling in all base image changes.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
Copy link
Member Author

borkmann commented May 5, 2020

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 5, 2020
@borkmann borkmann merged commit aaf12c0 into master May 5, 2020
1.8.0 automation moved this from In progress to Merged May 5, 2020
@borkmann borkmann deleted the pr/docker-rework branch May 5, 2020 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docker Impacts the integration with Docker. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants