Skip to content

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

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Apr 25, 2025

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:

    $ 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

@christarazi christarazi added kind/performance There is a performance impact of this. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. labels Apr 25, 2025
@christarazi christarazi force-pushed the pr/christarazi/ipcache-consolidate-resources branch from daca1c3 to 70b9bcb Compare April 25, 2025 22:49
@christarazi christarazi changed the title pr/christarazi/ipcache consolidate resources ipcache: Consolidate prefixes from various resources Apr 25, 2025
@christarazi

This comment was marked as outdated.

@christarazi christarazi force-pushed the pr/christarazi/ipcache-consolidate-resources branch from 70b9bcb to 8415740 Compare April 25, 2025 22:51
@christarazi
Copy link
Member Author

/test

@christarazi christarazi force-pushed the pr/christarazi/ipcache-consolidate-resources branch from 8415740 to d972446 Compare April 28, 2025 23:19
@christarazi
Copy link
Member Author

/test

@christarazi christarazi marked this pull request as ready for review April 29, 2025 15:14
@christarazi christarazi requested review from a team as code owners April 29, 2025 15:14
Copy link
Member

@bimmlerd bimmlerd left a 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)

@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 Apr 30, 2025
@bimmlerd bimmlerd added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Apr 30, 2025
@ldelossa ldelossa removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 30, 2025
@ldelossa
Copy link
Contributor

🎩 - removed ready-to-merge to remove from triager queue.

@christarazi christarazi force-pushed the pr/christarazi/ipcache-consolidate-resources branch from d972446 to 39ff4d8 Compare May 1, 2025 18:19
@christarazi
Copy link
Member Author

/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>
@christarazi christarazi force-pushed the pr/christarazi/ipcache-consolidate-resources branch from 39ff4d8 to 048a5b2 Compare May 5, 2025 18:14
@christarazi
Copy link
Member Author

christarazi commented May 5, 2025

/test

Edit: #38574

@bimmlerd bimmlerd removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label May 6, 2025
@christarazi christarazi added this pull request to the merge queue May 6, 2025
Merged via the queue into cilium:main with commit a3bd1a6 May 6, 2025
66 checks passed
@christarazi christarazi deleted the pr/christarazi/ipcache-consolidate-resources branch May 6, 2025 19:34
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels May 6, 2025
@christarazi christarazi added 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 labels 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants