Kill the store #32
Kill the store #32
Conversation
There was a problem hiding this comment.
I'm not familiar with the IndexerInformer API and am not sure what problem you are trying to solve. Do you have more info e.g. error messages, symptoms, etc. about the issues you've encountered?
In the end, this starts to diverge quite a bit from the kubernetes DNS implementation e.g. https://github.com/kubernetes/kubernetes/blob/master/pkg/dns/dns.go#L217 that this code is originally based on and might introduce side effects that I am unable to really grasp being unfamiliar with what the IndexerInformer API does.
| type k8s struct { | ||
| *client.Client | ||
| podStore kcache.Indexer |
There was a problem hiding this comment.
Technically, it's an Indexer not a store. Indexers contain store, so let's rename that to podIndexer.
| err = backoff.Retry(operation, backoff.NewExponentialBackOff()) | ||
| if err != nil { | ||
| return "", err | ||
| exponential := backoff.NewExponentialBackOff() |
There was a problem hiding this comment.
Good idea 👍
Wonder if we want to try for more than one second and if we should set the number of retries as well though.
| for i, pod := range pods { | ||
| podNames[i] = pod.(*api.Pod).ObjectMeta.Name | ||
| } | ||
| return nil, fmt.Errorf("%d pods (%v) with the ip %s indexed", len(pods), podNames, IP) |
There was a problem hiding this comment.
Not sure how indexer works exactly, but the code doesn't seem to do anything when a pod is removed so wouldn't there be a case old pods have been indexed at a specific IP and the IP has been recycled for a new pod? In that case, the code would return an error here.
There was a problem hiding this comment.
The indexer takes care of calling this on every add/update (on both new & old pods)/remove and keeping the internal mapping up-to-date.
|
Unfortunately, I was still seeing mismaps (i.e. pods getting credentials from other pods or pods not getting their own credentials) when using the manual add/remove code. Definitely agree that it would be awesome to keep this the same as kube-dns. Looking at the code that kube-dns uses, it's likely there are race conditions in that code as well. However, folks are unlikely to ever experience them since kube-dns only watches for
When implementing that exact scheme here, I found that |
|
Am I correct in my reading of the current |
No, on startup the Informer API will send all the pods that have been created. It works exactly the same as the current kube dns implementation. |
jtblin
force-pushed
the
master
branch
3 times, most recently
from
1581bdb
to
12fa75c
Compare
|
@mikekap I'm curious if you are still running this or if you have opted for a different solution. I would be willing to take a look at the branch conflicts and submit a PR if it would be considered by @jtblin but the credential caching for jobs in the |
|
@jrnt30 we're still running it. The |
|
@jrnt30 would be totally happy to consider an updated PR. I actually have that in my todos for a while but have been pretty busy with different things lately. Additional complexity is that there is now support for namespaces as well but moving to |
|
Thanks for the reply. I started working on this yesterday and actually found that as well. Knowing the PR will be considered is very helpful, I'll see if I can come up with something. |
|
Deprecated in favour of #92. Thanks! |
Unfortunately there are still bugs in the store w.r.t. ordering. I can't figure out how to track them down anymore :(.
Instead just get rid of the store and use NewIndexerInformer to create a custom pod ip -> pod index. Also only index pods that are "active" - i.e. not succeeded or failed.