Change map convention (prefix to name instead of suffix to section) + Update to latest Cilium #103
Conversation
This file contains 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
This removes the issue with having non standard section names for the maps
…to its sectionname
|
Waiting for approval from someone in the solo-io org to start testing. |
burak-ok
force-pushed
the
burak/update_cilium
branch
from
25c20eb to
860d293
Compare
|
Hello @burak-ok! Thanks so much for the PR! As this is a breaking change, it will be important to message this in the release notes when we release this. We will do it as 0.1.x to make it as clear as possible. |
burak-ok
force-pushed
the
burak/update_cilium
branch
from
860d293 to
e657773
Compare
EItanya
reviewed
May 4, 2023
There was a problem hiding this comment.
These changes LGTM, would like to give @lgadban and @krisztianfekete a chance to review as well!
9 tasks
SirNexus
reviewed
May 23, 2023
The old way of saving the (non public) BTF struct is not needed anymore Co-authored-by: Alban Crequy <albancrequy@linux.microsoft.com>
burak-ok
force-pushed
the
burak/update_cilium
branch
from
e657773 to
441b2e6
Compare
There was a problem hiding this comment.
LGTM, thanks for the PR, @burak-ok! My only "concern" is that this way the names of the Prometheus metrics will sound a bit unusual due to including the metric type explicitly, but don't really have a better idea.
SirNexus
approved these changes
Jun 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
BREAKING CHANGE
This PR has 2 parts
1. Change scheme for output maps
Instead of specifying the output type in the section name (
events SEC(".maps.print");) this PR relies on the name of the map itself (print_events SEC(".maps");). This keep the section names aligned with alleBPFprograms and the magic is now about the prefix of the name.This also has the advantage that one doesn't have to make changes to
cilium/ebpfA disadvantage is that changing the output type of a map is now more annoying. One has to change the name and all of its occurrences. Before that the change only involved the section name.
2. Update
cilium/ebpfto latestSome further changes are needed to update
cilium/ebpfto its latest version. BTF information are now available. (In the used version one had to move the packages to a public one)Generics are now required and therefore Go was update to
1.18Misc
I tried to separate the commits into logical chunks. I think this way it is easier to review. Rebasing and squashing can be done of course before merging.
Regarding using a prefix/suffix for the name: I prefer having a suffix to instantly see what that map is about