bpftool: Only compile for the relevant platform#339
Merged
Conversation
48fb19d to
74fe4e8
Compare
ti-mo
reviewed
Jul 1, 2025
Contributor
ti-mo
left a comment
There was a problem hiding this comment.
Thanks! One nit and a question.
74fe4e8 to
3e0ff56
Compare
ti-mo
approved these changes
Jul 1, 2025
Contributor
ti-mo
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback! One more nit.
Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
3e0ff56 to
e4609e3
Compare
HadrienPatte
added a commit
to cilium/cilium
that referenced
this pull request
Aug 23, 2025
With #41230, renovate will now handle updating those images, but since they haven't been updated in a while, there's some manual adjustments that are required before renovate can handle those. With cilium/image-tools#339 and cilium/image-tools#343, these two images no longer include their test files. This PR removes the test stage from the `runtime` image as it was just rerunning those tests and there are no `runtime` image tests. This PR also fixes the tests for the `builder` image. It turns out that those tests have been broken since #32767 because they haven't been running in CI for years. Those tests aren't running since we switched from the legacy docker engine to buildkit, see [details](https://docs.docker.com/build/building/multi-stage/#differences-between-legacy-builder-and-buildkit): > The legacy Docker Engine builder processes all stages of a Dockerfile leading up to the selected --target. It will build a stage even if the selected target doesn't depend on that stage. > > BuildKit only builds the stages that the target stage depends on. Future followups: * Does it make sense to have a test expect a specific version of `libprotoc` when this dependency is regularly automatically updated by renovate? With the current setup, this test will break everytime renovate updates `libprotoc`. I'd argue that the version string should be removed from the expected output so we only test that `protoc --version` runs sucessfully without expecting a given version string. * Consider either: * Update the CI to ensure those container structure tests are run on PRs * Remove container structure tests as after this PR, only the `builder` image will have [some](https://github.com/cilium/cilium/blob/a7de0143835a080750dbbde7285be37ab8599883/images/builder/test/spec.yaml) and all they test is that `protoc` is installed in the image Note: the release note for this PR is more focused on the user-visible change related to the update of the `bpftools` and `llvm` images. ```release images: Update `bpftools` and `llvm` images to reduce the size of the `cilium` image by 39MB (`amd64`) / 35MB (arm64) ``` Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
HadrienPatte
added a commit
to cilium/cilium
that referenced
this pull request
Aug 23, 2025
With #41230, renovate will now handle updating those images, but since they haven't been updated in a while, there's some manual adjustments that are required before renovate can handle those. With cilium/image-tools#339 and cilium/image-tools#343, these two images no longer include their test files. This PR removes the test stage from the `runtime` image as it was just rerunning those tests and there are no `runtime` image tests. This PR also fixes the tests for the `builder` image. It turns out that those tests have been broken since #32767 because they haven't been running in CI for years. Those tests aren't running since we switched from the legacy docker engine to buildkit, see [details](https://docs.docker.com/build/building/multi-stage/#differences-between-legacy-builder-and-buildkit): > The legacy Docker Engine builder processes all stages of a Dockerfile leading up to the selected --target. It will build a stage even if the selected target doesn't depend on that stage. > > BuildKit only builds the stages that the target stage depends on. Future followups: * Does it make sense to have a test expect a specific version of `libprotoc` when this dependency is regularly automatically updated by renovate? With the current setup, this test will break everytime renovate updates `libprotoc`. I'd argue that the version string should be removed from the expected output so we only test that `protoc --version` runs successfully without expecting a given version string. * Consider either: * Update the CI to ensure those container structure tests are run on PRs * Remove container structure tests as after this PR, only the `builder` image will have [some](https://github.com/cilium/cilium/blob/a7de0143835a080750dbbde7285be37ab8599883/images/builder/test/spec.yaml) and all they test is that `protoc` is installed in the image Note: the release note for this PR is more focused on the user-visible change related to the update of the `bpftools` and `llvm` images. ```release images: Update `bpftools` and `llvm` images to reduce the size of the `cilium` image by 39MB (`amd64`) / 35MB (arm64) ``` Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
HadrienPatte
added a commit
to cilium/cilium
that referenced
this pull request
Sep 3, 2025
With #41230, renovate will now handle updating those images, but since they haven't been updated in a while, there's some manual adjustments that are required before renovate can handle those. With cilium/image-tools#339 and cilium/image-tools#343, these two images no longer include their test files. This PR removes the test stage from the `runtime` image as it was just rerunning those tests and there are no `runtime` image tests. This PR also fixes the tests for the `builder` image. It turns out that those tests have been broken since #32767 because they haven't been running in CI for years. Those tests aren't running since we switched from the legacy docker engine to buildkit, see [details](https://docs.docker.com/build/building/multi-stage/#differences-between-legacy-builder-and-buildkit): > The legacy Docker Engine builder processes all stages of a Dockerfile leading up to the selected --target. It will build a stage even if the selected target doesn't depend on that stage. > > BuildKit only builds the stages that the target stage depends on. Future followups: * Does it make sense to have a test expect a specific version of `libprotoc` when this dependency is regularly automatically updated by renovate? With the current setup, this test will break everytime renovate updates `libprotoc`. I'd argue that the version string should be removed from the expected output so we only test that `protoc --version` runs successfully without expecting a given version string. * Consider either: * Update the CI to ensure those container structure tests are run on PRs * Remove container structure tests as after this PR, only the `builder` image will have [some](https://github.com/cilium/cilium/blob/a7de0143835a080750dbbde7285be37ab8599883/images/builder/test/spec.yaml) and all they test is that `protoc` is installed in the image Note: the release note for this PR is more focused on the user-visible change related to the update of the `bpftools` and `llvm` images. ```release images: Update `bpftools` and `llvm` images to reduce the size of the `cilium` image by 39MB (`amd64`) / 35MB (arm64) ``` Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
HadrienPatte
added a commit
to cilium/cilium
that referenced
this pull request
Sep 4, 2025
With #41230, renovate will now handle updating those images, but since they haven't been updated in a while, there's some manual adjustments that are required before renovate can handle those. With cilium/image-tools#339 and cilium/image-tools#343, these two images no longer include their test files. This PR removes the test stage from the `runtime` image as it was just rerunning those tests and there are no `runtime` image tests. This PR also fixes the tests for the `builder` image. It turns out that those tests have been broken since #32767 because they haven't been running in CI for years. Those tests aren't running since we switched from the legacy docker engine to buildkit, see [details](https://docs.docker.com/build/building/multi-stage/#differences-between-legacy-builder-and-buildkit): > The legacy Docker Engine builder processes all stages of a Dockerfile leading up to the selected --target. It will build a stage even if the selected target doesn't depend on that stage. > > BuildKit only builds the stages that the target stage depends on. Future followups: * Does it make sense to have a test expect a specific version of `libprotoc` when this dependency is regularly automatically updated by renovate? With the current setup, this test will break everytime renovate updates `libprotoc`. I'd argue that the version string should be removed from the expected output so we only test that `protoc --version` runs successfully without expecting a given version string. * Consider either: * Update the CI to ensure those container structure tests are run on PRs * Remove container structure tests as after this PR, only the `builder` image will have [some](https://github.com/cilium/cilium/blob/a7de0143835a080750dbbde7285be37ab8599883/images/builder/test/spec.yaml) and all they test is that `protoc` is installed in the image Note: the release note for this PR is more focused on the user-visible change related to the update of the `bpftools` and `llvm` images. ```release images: Update `bpftools` and `llvm` images to reduce the size of the `cilium` image by 39MB (`amd64`) / 35MB (arm64) ``` Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
HadrienPatte
added a commit
to DataDog/cilium
that referenced
this pull request
Sep 8, 2025
With cilium#41230, renovate will now handle updating those images, but since they haven't been updated in a while, there's some manual adjustments that are required before renovate can handle those. With cilium/image-tools#339 and cilium/image-tools#343, these two images no longer include their test files. This PR removes the test stage from the `runtime` image as it was just rerunning those tests and there are no `runtime` image tests. This PR also fixes the tests for the `builder` image. It turns out that those tests have been broken since cilium#32767 because they haven't been running in CI for years. Those tests aren't running since we switched from the legacy docker engine to buildkit, see [details](https://docs.docker.com/build/building/multi-stage/#differences-between-legacy-builder-and-buildkit): > The legacy Docker Engine builder processes all stages of a Dockerfile leading up to the selected --target. It will build a stage even if the selected target doesn't depend on that stage. > > BuildKit only builds the stages that the target stage depends on. Future followups: * Does it make sense to have a test expect a specific version of `libprotoc` when this dependency is regularly automatically updated by renovate? With the current setup, this test will break everytime renovate updates `libprotoc`. I'd argue that the version string should be removed from the expected output so we only test that `protoc --version` runs successfully without expecting a given version string. * Consider either: * Update the CI to ensure those container structure tests are run on PRs * Remove container structure tests as after this PR, only the `builder` image will have [some](https://github.com/cilium/cilium/blob/a7de0143835a080750dbbde7285be37ab8599883/images/builder/test/spec.yaml) and all they test is that `protoc` is installed in the image Note: the release note for this PR is more focused on the user-visible change related to the update of the `bpftools` and `llvm` images. ```release images: Update `bpftools` and `llvm` images to reduce the size of the `cilium` image by 39MB (`amd64`) / 35MB (arm64) ``` Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
github-merge-queue bot
pushed a commit
to cilium/cilium
that referenced
this pull request
Sep 9, 2025
With #41230, renovate will now handle updating those images, but since they haven't been updated in a while, there's some manual adjustments that are required before renovate can handle those. With cilium/image-tools#339 and cilium/image-tools#343, these two images no longer include their test files. This PR removes the test stage from the `runtime` image as it was just rerunning those tests and there are no `runtime` image tests. This PR also fixes the tests for the `builder` image. It turns out that those tests have been broken since #32767 because they haven't been running in CI for years. Those tests aren't running since we switched from the legacy docker engine to buildkit, see [details](https://docs.docker.com/build/building/multi-stage/#differences-between-legacy-builder-and-buildkit): > The legacy Docker Engine builder processes all stages of a Dockerfile leading up to the selected --target. It will build a stage even if the selected target doesn't depend on that stage. > > BuildKit only builds the stages that the target stage depends on. Future followups: * Does it make sense to have a test expect a specific version of `libprotoc` when this dependency is regularly automatically updated by renovate? With the current setup, this test will break everytime renovate updates `libprotoc`. I'd argue that the version string should be removed from the expected output so we only test that `protoc --version` runs successfully without expecting a given version string. * Consider either: * Update the CI to ensure those container structure tests are run on PRs * Remove container structure tests as after this PR, only the `builder` image will have [some](https://github.com/cilium/cilium/blob/a7de0143835a080750dbbde7285be37ab8599883/images/builder/test/spec.yaml) and all they test is that `protoc` is installed in the image Note: the release note for this PR is more focused on the user-visible change related to the update of the `bpftools` and `llvm` images. ```release images: Update `bpftools` and `llvm` images to reduce the size of the `cilium` image by 39MB (`amd64`) / 35MB (arm64) ``` Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
jarrodb
pushed a commit
to 46labs/cilium
that referenced
this pull request
Sep 10, 2025
With cilium#41230, renovate will now handle updating those images, but since they haven't been updated in a while, there's some manual adjustments that are required before renovate can handle those. With cilium/image-tools#339 and cilium/image-tools#343, these two images no longer include their test files. This PR removes the test stage from the `runtime` image as it was just rerunning those tests and there are no `runtime` image tests. This PR also fixes the tests for the `builder` image. It turns out that those tests have been broken since cilium#32767 because they haven't been running in CI for years. Those tests aren't running since we switched from the legacy docker engine to buildkit, see [details](https://docs.docker.com/build/building/multi-stage/#differences-between-legacy-builder-and-buildkit): > The legacy Docker Engine builder processes all stages of a Dockerfile leading up to the selected --target. It will build a stage even if the selected target doesn't depend on that stage. > > BuildKit only builds the stages that the target stage depends on. Future followups: * Does it make sense to have a test expect a specific version of `libprotoc` when this dependency is regularly automatically updated by renovate? With the current setup, this test will break everytime renovate updates `libprotoc`. I'd argue that the version string should be removed from the expected output so we only test that `protoc --version` runs successfully without expecting a given version string. * Consider either: * Update the CI to ensure those container structure tests are run on PRs * Remove container structure tests as after this PR, only the `builder` image will have [some](https://github.com/cilium/cilium/blob/a7de0143835a080750dbbde7285be37ab8599883/images/builder/test/spec.yaml) and all they test is that `protoc` is installed in the image Note: the release note for this PR is more focused on the user-visible change related to the update of the `bpftools` and `llvm` images. ```release images: Update `bpftools` and `llvm` images to reduce the size of the `cilium` image by 39MB (`amd64`) / 35MB (arm64) ``` Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
NihaNallappagari
pushed a commit
to NihaNallappagari/cilium
that referenced
this pull request
Sep 17, 2025
With cilium#41230, renovate will now handle updating those images, but since they haven't been updated in a while, there's some manual adjustments that are required before renovate can handle those. With cilium/image-tools#339 and cilium/image-tools#343, these two images no longer include their test files. This PR removes the test stage from the `runtime` image as it was just rerunning those tests and there are no `runtime` image tests. This PR also fixes the tests for the `builder` image. It turns out that those tests have been broken since cilium#32767 because they haven't been running in CI for years. Those tests aren't running since we switched from the legacy docker engine to buildkit, see [details](https://docs.docker.com/build/building/multi-stage/#differences-between-legacy-builder-and-buildkit): > The legacy Docker Engine builder processes all stages of a Dockerfile leading up to the selected --target. It will build a stage even if the selected target doesn't depend on that stage. > > BuildKit only builds the stages that the target stage depends on. Future followups: * Does it make sense to have a test expect a specific version of `libprotoc` when this dependency is regularly automatically updated by renovate? With the current setup, this test will break everytime renovate updates `libprotoc`. I'd argue that the version string should be removed from the expected output so we only test that `protoc --version` runs successfully without expecting a given version string. * Consider either: * Update the CI to ensure those container structure tests are run on PRs * Remove container structure tests as after this PR, only the `builder` image will have [some](https://github.com/cilium/cilium/blob/a7de0143835a080750dbbde7285be37ab8599883/images/builder/test/spec.yaml) and all they test is that `protoc` is installed in the image Note: the release note for this PR is more focused on the user-visible change related to the update of the `bpftools` and `llvm` images. ```release images: Update `bpftools` and `llvm` images to reduce the size of the `cilium` image by 39MB (`amd64`) / 35MB (arm64) ``` Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently the
bpftoolimage compilesbpftoolfor both the "native" arch and crosscompiles for the "aarch64" arch, to then only keep the binary matching theTARGETPLATFORM.With this PR we only build one version of
bpftoolbased onTARGETPLATFORM.Additional changes:
FromAsCasingandLegacyKeyValueFormatviolationsCOMPILERS_IMAGEref as the one used was a single archadm64one, the new one is a proper multi-platform manifest:crane manifest quay.io/cilium/image-compilers:1732033829-330cbaf@sha256:5c54f614fb8ee7939492aa4b7d74b37922d98199f5993f6d957a1637ce30eb9e { "schemaVersion": 2, "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json", "manifests": [ { "mediaType": "application/vnd.docker.distribution.manifest.v2+json", "digest": "sha256:302fc3b1a13cd7ecf28178f52dd7cdb1757353e3f9468aab7f6200a7912ea999", "size": 504, "platform": { "architecture": "amd64", "os": "linux" } }, { "mediaType": "application/vnd.docker.distribution.manifest.v2+json", "digest": "sha256:7ce45a14c376d51b5fa901436f863770d8f60624e4e35b74ff34fd598f5a50e2", "size": 504, "platform": { "architecture": "arm64", "os": "linux" } } ] }Test of the new image: