[v1.13] Author backport of "Address ipcache startup perfomance regression" #25185
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
[ upstream commit 6651e89 ] This patch adds a benchmark for ipcache.Upsert, with the intention of exercising the logic for named ports. This exposes a performance regression when upserting many pods, as is the case on agent startup. The startup regression was due to losing an optimization in 93cd67f, namely that the named ports bookkeeping is done only once named ports are present in policies. Without the lazy bookkeeping, we have accidentally quadratic behavior, as we look at every pod whenever a new pod is added. Follow up commits will address the issue, this is merely a convenient reproduction of the regression. Suggested-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit 39cf09b ] In preparation of introducing reference counting for named ports, change the NamedPortMultiMap from a concrete type to an interface. This produces a fair bit of churn in the test code (as it assumes the type is a map). There are no functional changes, however. The benefit of having this type be an interface is that we can switch it's implementing type to use reference counting with a smaller patch. That, in turn, will allow us to avoid looking at all other pods when a new pod is added or deleted, just for the named port bookkeeping. Suggested-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit 33079de ] This commit introduces reference counting for named ports. Using the reference counting, we know when to add or remove named ports in our bookkeeping, without having to look through all other pods. This leads to a significant speedup when inserting many pods, as is the case on agent startup. Note that this patch does _not_ restore the original optimization of not keeping track of named ports until used in a policy. We did not find a good way of doing so while also avoiding re-introducing the deadlock fixed in the commit referenced below. Instead, we always do the bookkeeping, but in a more efficient manner. The reference counting requires the use of a RWLock, since read and write accesses to a map must be protected. Thus, there is potential for introducing yet another deadlock. However, we argue that this is not the case: We do _not_ acquire any other lock while holding our mutex M. A wait cycle/deadlock involving M must include holding M when acquiring some other lock, however. Therefore this patch cannot introduce a deadlock. Fixes: 93cd67f (ipcache: fix potential deadlock in GetNamedPorts) Fixes: cilium#24987 Suggested-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit 191b672 ] With the reference counting in place, we can simplify the involved code paths in the ipcache. There is now little point to conditionally perform the update, as the update itself is about as expensive as checking whether it is necessary. Therefore, opt for simpler code and always call into NamedPortMultiMapUpdater.Update. Indeed, this even gives a mild speedup: goos: linux goarch: arm64 pkg: github.com/cilium/cilium/pkg/ipcache │ fix.txt │ cleanup.txt │ │ sec/op │ sec/op vs base │ IPCacheUpsert10-10 48.84µ ± 5% 47.59µ ± 5% ~ (p=0.165 n=10) IPCacheUpsert100-10 508.3µ ± 5% 485.0µ ± 2% -4.58% (p=0.005 n=10) IPCacheUpsert1000-10 5.282m ± 5% 5.056m ± 2% -4.28% (p=0.011 n=10) IPCacheUpsert10000-10 48.63m ± 5% 44.87m ± 5% -7.74% (p=0.009 n=10) geomean 1.589m 1.513m -4.81% Suggested-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
bimmlerd
added
kind/backports
bimmlerd
changed the title
|
/test-backport-1.13 |
bimmlerd
changed the title
|
/test-1.16-4.19 lots of jenkins failures with |
|
/test-1.17-4.19 |
|
/test-1.19-4.19 |
|
/test-1.20-4.19 |
|
/test-1.22-4.19 |
|
/ci-gke-1.13 failed with #24251 |
|
/test-runtime Runtime tests were failing with Probably something going on on Jenkins side. |
joestringer
approved these changes
May 2, 2023
maintainer-s-little-helper
bot
added
the
ready-to-merge
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.
Once this PR is merged, you can update the PR labels via:
or with