-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
identity: stop double-update of selector cache and regenerate when a local identity is allocated #29865
identity: stop double-update of selector cache and regenerate when a local identity is allocated #29865
Conversation
f5908e7
to
d45c917
Compare
This may also fix #29846, which is an error message due to slow FQDN policy updates. Regeneration blocks incremental policy updates, which causes delays, which causes that error message. |
/test |
d45c917
to
f76a756
Compare
/test |
Given that this is a follow-up to a refactor which (to my knowledge) landed in v1.15, should we also backport this? |
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.
Looks good for IPCache (I mainly focused if the new usage is consistent with how we do things in the async path), thanks. The overall reasoning makes sense, but I'll defer the identity allocator review to other reviewers.
f76a756
to
e8362ff
Compare
Definitely, yes, this should be backported. It is causing CI flakes and annoying lock contention. It's also not actually directly related to the FQDN refactor. This is a long-standing bug. However, we have the confidence to fix this now that there are only a very limited set of users of the synchronous APIs. |
/test |
With the newer, asynchronous APIs, the ipcache takes on the responsibility of updating the policy engine (i.e. the SelectorCache) whenever it allocates an identity. However, the legacy APIs don't do that. This commit changes that. By centralizing this in the ipcache, we can (in a subsequent commit) stop regenerating all endpoints whenever an identity is allocated. This is wasteful, since local identities can only ever be allocated by the ipcache now. Signed-off-by: Casey Callendrello <cdc@isovalent.com>
AllocateIdentity() takes a parameter, `notifyOwner`, that when true, tells the allocator to propagate those updates to the policy subsystem. However, the local (i.e. CIDR) identity allocator ignores this field, and *always* propagates identity updates to the policy subsystem as well as **always regenerating all endpoints**. This is needless, since the ipcache always updates the policy system as well. This change is safe since the ipcache is the only caller that passes `notifyOwner = false`, and the ipcache updates the policy system itself. All other callers to `AllocateIdentity()` set `notifyOwner = true`. Fixes: cilium#29681 Signed-off-by: Casey Callendrello <cdc@isovalent.com>
This should be fixed now. Fixes: cilium#29681 Signed-off-by: Casey Callendrello <cdc@isovalent.com>
8206a2c
to
31339e2
Compare
/test |
All green! |
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.
Approving for ci-structure.
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.
I believe this is correct, but I raised a couple of points below for additional due diligence. The one conclusion I can come to here is that we can delete quite a lot of complicated logic once we finally migrate the k8sServiceHandler
over to the new ipcache model 🎉
I also took a look over the callers who are passing the notifyOwner
flag to the identity subsystem and I agree with your assessment that this should be safe / only removing a double-trigger.
I suspect that there's also a subsequent improvement to re-evaluate the role of notifications in the identity subsystem and potentially even remove them. Maybe we could prepare a short CFP to describe the design impacts of that change and float it around for discussion to confirm that's the right path forward.
As I was looking in to some issues, I discovered that we are still regenerating all endpoints whenever we allocated a new local identity. This is not necessary, since we already have an incremental policy update engine.
This has been the case for some time, but it is a lot more relevant since #29036, which moved FQDN policy over to this code-path.
Background:
The
AllocateIdentity()
function takes a parameter,notifyOwner
. When this istrue
, any newly allocated identities are passed to the policy engine by the identity allocator and an endpoint regeneration is triggered. However, there is a funny inconsistency: if the identity is local (i.e. a CIDR identity), thennotifyOwner
is ignored and the policy engine is always updated. Incidentally, as part of updating the policy engine, all endpoints are regenerated.However, the only source of local (CIDR) identities now is the IPCache, which already has its own policy update mechanism. There is one missed case -- fixed in this PR -- so there really is no need to ignore
notifyOwner
. This is, also, the only caller whereAllocateIdentity(notifyOwner = false)
.The fix:
AllocateCIDRs()
/releaseCIDRIdentities()
calls too. These methods are only have one caller: the ServiceHandler that translatestoServices:
policy rules to a CIDR selector.notifyOwner
, and don't pass policy updates when it isfalse
.What's changed?
It's useful to look at the flow, just to understand the full context of what's changed
Before:
ipcache.UpsertMetadata(prefix, ...
)AllocateIdentity(labels, notifyOwner = false)
SelectorCache.UpdateIdentities(...)
andRegenerateAllEndpoints()
SelectorCache.UpdateIdentities()
andUpdatePolicyMaps()
After:
ipcache.UpsertMetadata(prefix, ...
)AllocateIdentity(labels, notifyOwner = false)
SelectorCache.UpdateIdentities()
andUpdatePolicyMaps()
So, this change is safe, in that it is not possible for a new identity to be missed.
Fixes: #29681
Fixes: #29846