kclient: use internal indexes instead of external #51576
Conversation
Before: we register an informer handler which builds our own index on top of the underlying store. After: we register an index directly into the informer, which handles things entirely. This was not done originally because Kubernetes only added the ability to add indexes dynamically recently (I added this). The benefits here are: * Deterministic ordering. The index is ALWAYS updated before any other handlers are called. This allows us to guarantee the index is up to date in our handlers. Without this, we had the crazy Delegate hack. * Minor performance benefit from not needing to cut through as many abstractions or locking. K8s indexes are actually a pain to use, so kclient provides a higher level API over it; the same as what we had before. One quirk is they only allow string keys. To get around this, we allow any key, but it must compile down to a unique string.
howardjohn
added
the
release-notes-none
istio-testing
added
the
size/L
|
I am very concerned about frequently changing the funamental framework we are using. Transiting from k8s client to kclient, there are many bugs met. This is also the most important reason that i am opposed to migrate all of our components to krt. |
| res, err := i.indexer.ByIndex(i.key, key) | ||
| if err != nil { | ||
| // This should only happen if the key does not exist which should be impossible | ||
| log.Fatalf("index lookup failed: %v", err) |
There was a problem hiding this comment.
why i.indexer.ByIndex cannot return not exist error, where do you protect it
There was a problem hiding this comment.
The only time it returns an error is if the index doesn't exist. We know not must exist because we were the ones that created it
I am not sure what to do with this feedback. This particular PR was always the intended path forward (and had Todos in the code, etc) but was blocked by client-go limitations which I fixed upstream. Overall kclient has, IMO been a huge improvement in development and fixed a ton of bugs. While it has introduced a few minor bugs, it has overall been a net positive imo. I am not suggesting we move everything to krt, and tbh I wasn't even the reason it was - there was an overwhelming majority of contributors who wanted to see it used for ambient and we haven't agreed yet to use it more broadly. Overall I feel I made a change that somplifies our code, improves performance, and fixes bugs. It's hard to react to a comment that we shouldn't make such a change without more concrete feedback for how to improve it or what the issue is |
pkg/kube/kclient/client.go
Outdated
| func (i internalIndex) Lookup(key string) []interface{} { | ||
| res, err := i.indexer.ByIndex(i.key, key) | ||
| if err != nil { | ||
| // This should only happen if the key does not exist which should be impossible |
There was a problem hiding this comment.
Could you clarify which key you are referring to? The key for the internal index or the passed key
There was a problem hiding this comment.
The internal index. Ill update the comment
howardjohn
force-pushed
the
kclient/raw-index
branch
from
4859373
to
42d8208
Compare
Before: we register an informer handler which builds our own index on
top of the underlying store.
After: we register an index directly into the informer, which handles
things entirely.
This was not done originally because Kubernetes only added the ability
to add indexes dynamically recently (I added this).
The benefits here are:
handlers are called. This allows us to guarantee the index is up to
date in our handlers. Without this, we had the crazy Delegate hack.
abstractions or locking.
K8s indexes are actually a pain to use, so kclient provides a higher
level API over it; the same as what we had before. One quirk is they
only allow string keys. To get around this, we allow any key, but it
must compile down to a unique string.