-
Notifications
You must be signed in to change notification settings - Fork 3.4k
ipcache: Consolidate prefixes from various resources #39176
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
Merged
christarazi
merged 3 commits into
cilium:main
from
christarazi:pr/christarazi/ipcache-consolidate-resources
May 6, 2025
Merged
ipcache: Consolidate prefixes from various resources #39176
christarazi
merged 3 commits into
cilium:main
from
christarazi:pr/christarazi/ipcache-consolidate-resources
May 6, 2025
Conversation
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
daca1c3
to
70b9bcb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
70b9bcb
to
8415740
Compare
/test |
8415740
to
d972446
Compare
/test |
viktor-kurchenko
approved these changes
Apr 30, 2025
bimmlerd
approved these changes
Apr 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of feedback, but nothing hard blocking - I'll be on PTO starting tomorrow, so approving in good faith (but adding do-not-merge/discussion so triager doesn't auto-merge)
🎩 - removed ready-to-merge to remove from triager queue. |
d972446
to
39ff4d8
Compare
/test |
AllocateCIDRs() and ReleaseCIDRIdentitiesByCIDR() have both been removed from the codebase already, so remove the leftovers from this mock ipcache. Signed-off-by: Chris Tarazi <chris@isovalent.com>
Benchmark removing and re-adding CIDR prefixes as well using the batch metadata API, to simulate the usage from the policy importer. Signed-off-by: Chris Tarazi <chris@isovalent.com>
When many CIDRs are inserted into the ipcache via the metadata API (i.e. UpsertMetadataBatch() / UpsertMetadata()) and many of those CIDRs are present across many various resources such as CNPs (aka "duplicated" CIDRs across many policies), the ipcache ends up performing many redundant label merging operations via ToLabels() inside resolveIdentity(). See BenchmarkManyCIDREntries() for more context. To optimize the ipcache metadata structure and avoid storing duplicate instances of metadata (particularly CIDR prefixes from policies), we handle CIDR prefixes outside the cluster as a special case. Instead of tracking each CIDR by its originating policy, we consolidate them by treating each CIDR as originating from its own dedicated resource. This approach reduces duplication. The above is done by a refcount of each CIDR prefix that is destined for outside of the cluster. Benchmark results: ``` $ go test -v ./pkg/ipcache -test.run '^$' -test.bench 'BenchmarkManyCIDREntries' -count 10 > old $ go test -v ./pkg/ipcache -test.run '^$' -test.bench 'BenchmarkManyCIDREntries' -count 10 > new $ benchstat old new name old time/op new time/op delta ManyCIDREntries-16 35.4ms ± 4% 0.2ms ± 1% -99.33% (p=0.000 n=10+10) name old alloc/op new alloc/op delta ManyCIDREntries-16 4.94MB ± 1% 0.22MB ± 0% -95.52% (p=0.000 n=10+10) name old allocs/op new allocs/op delta ManyCIDREntries-16 33.4k ± 1% 4.0k ± 0% -88.11% (p=0.000 n=10+10) ``` Suggested-by: Casey Callendrello <cdc@isovalent.com> Suggested-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>
39ff4d8
to
048a5b2
Compare
/test Edit: #38574 |
bimmlerd
approved these changes
May 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
affects/v1.15
This issue affects v1.15 branch
affects/v1.16
This issue affects v1.16 branch
affects/v1.17
This issue affects v1.17 branch
kind/performance
There is a performance impact of this.
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.
sig/policy
Impacts whether traffic is allowed or denied based on user-defined policies.
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.
Depends on #38851.
testutils/ipcache: Remove nonexistent methods
AllocateCIDRs() and ReleaseCIDRIdentitiesByCIDR() have both been removed
from the codebase already, so remove the leftovers from this mock
ipcache.
ipcache: Extend benchmark to test CIDR churn
Benchmark removing and re-adding CIDR prefixes as well using the batch
metadata API, to simulate the usage from the policy importer.
ipcache: Consolidate prefixes from various resources
When many CIDRs are inserted into the ipcache via the metadata API (i.e.
UpsertMetadataBatch() / UpsertMetadata()) and many of those CIDRs are
present across many various resources such as CNPs (aka "duplicated"
CIDRs across many policies), the ipcache ends up performing many
redundant label merging operations via ToLabels() inside
resolveIdentity(). See BenchmarkManyCIDREntries() for more context.
To optimize the ipcache metadata structure and avoid storing duplicate
instances of metadata (particularly CIDR prefixes from policies), we
handle CIDR prefixes outside the cluster as a special case. Instead of
tracking each CIDR by its originating policy, we consolidate them by
treating each CIDR as originating from its own dedicated resource. This
approach reduces duplication.
The above is done by a refcount of each CIDR prefix that is destined for
outside of the cluster.
Benchmark results:
Suggested-by: Casey Callendrello cdc@isovalent.com
Suggested-by: Sebastian Wicki sebastian@isovalent.com