allocator: Fix kvstore identity leak #34893
Merged
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
|
/test |
odinuge
commented
Sep 16, 2024
odinuge
commented
Sep 16, 2024
odinuge
commented
Sep 16, 2024
|
This pull request has been automatically marked as stale because it |
|
Ahh. I'll rebase this and fix the conflict as well as remove the extra code in #34893 (comment) tomorrow morning. |
9d17929 to
7a2c1f5
Compare
|
Got stuck with other work, but did the rebase now. Will read through on Monday and then remove the |
odinuge
commented
Oct 21, 2024
odinuge
commented
Oct 21, 2024
7a2c1f5 to
b1dd783
Compare
|
After running the reproduction script in #35451 with this code, with a The following while the script creating and deleting pods is running; $ kubectl exec -n kube-system daemonset/cilium -- cilium kvstore get --recursive "cilium/state/identities/v1/value/" 2>&1 |egrep "fix-"
cilium/state/identities/v1/value/k8s:io.cilium.k8s.namespace.labels.kubernetes.io/metadata.name=default;k8s:io.cilium.k8s.policy.cluster=default;k8s:io.cilium.k8s.policy.serviceaccount=default;k8s:io.kubernetes.pod.namespace=default;k8s:run=fix-745;/172.18.0.2 => 1377After stopping, and all pods are deleted; $ kubectl exec -n kube-system daemonset/cilium -- cilium kvstore get --recursive "cilium/state/identities/v1/value/" 2>&1 |egrep "fix-"
<no output> |
youngnick
reviewed
Oct 24, 2024
giorio94
reviewed
Oct 25, 2024
b1dd783 to
9b1782b
Compare
8 tasks
9b1782b to
e5dbba8
Compare
|
Thanks for looking at this @giorio94! Pushed a new version now. Been running in a loop locally and seems to work as expected! Running locally I see some of these (with a tiny kvstore-sync value); so it all seems to work as intended; and after all pods are gone, all the "slave keys" are released correctly and we have no leaks. |
e5dbba8 to
16a8184
Compare
giorio94
approved these changes
Nov 4, 2024
|
/test |
The syncLocalKeys call will with some small probability re-create slave keys for identities that are no longer used on the node. This often happens immidiately after the last reference on the node is cleaned up, and the slave key is deleted during the Release portion of the allocator. This results in the slave key being present until the etcd lease expires - and that usually doesn't happen until the agent restarts. This is not an issue for the master keys - as that is not using a per-node lease, and is cleaned up by the operator at an interval. In large clusters with a lot of identity and pod churn, we see this happening multiple times an hour. It can be investigated by looking at the "Re-created missing slave key" logline. This often leads to multiple thousand extra identities in the clusters, that are unused and not cleaned up. This function will now ensure that as best as it can it won't upsert the key if its no longer in use. And, if its unused after the upsert is done, it grabs the lock to double check - and if its no longer in use, it will release it to ensure its cleared up. Signed-off-by: Odin Ugedal <ougedal@palantir.com> Signed-off-by: Odin Ugedal <odin@uged.al>
Signed-off-by: Odin Ugedal <odin@uged.al> Signed-off-by: Odin Ugedal <ougedal@palantir.com>
16a8184 to
c1ad50a
Compare
|
/test |
youngnick
approved these changes
Nov 18, 2024
14 tasks
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.
This fixes an identity leak in the kvstore implementation that has been here since the beginning. If the kvstore-refresh sync dumps the loadKeys immediately before a key is released, it will try to update that key. In large clusters with a lot of identity and pod churn, we see this
happening multiple times an hour. It can be investigated by looking at
the "Re-created missing slave key" logline. This often leads to multiple
thousand extra identities in the clusters, that are unused and not
cleaned up.
This can also be easily mitigated by always grabbing the
slaveKeysMutex- but that is most likely going to cause a lot of slowness to identity allocation on the agents, so that is probably not a good idea.The term
slaveandmasterkey is also a bit confusing, and sometimes its explained asIDandreference(eg. via acquire reference) - so getting a consistent and better naming scheme would be useful for the readability of the code. We can probably save that for another PR.See the commits for a more detailed explanation.
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Fixes: #35451