use kubernetes informer framework #30
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
|
Do we see an immediate use case for the informers that should finally land soon? (kubernetes/client-go#4) If so, we might want to hold off so we don't constantly revendor. |
|
That makes sense. This says the informer framework should be in the client once this is merged, which it is already, just waiting for submit queue according to the GitHub statuses. So I will hold back on this once the informer framework is included. Then we vendor it in this PR and then switch to it in an additional PR. |
|
Although it's merged now and available, it seems that there is still a problem with it, however, it looks like once kubernetes/kubernetes#33632 is fixed it will work. So holding off just a bit longer until then. |
|
As kubernetes/kubernetes#33632 is merged we're only waiting for submit queue again. 🎉 |
brancz
force-pushed
the
clientgo15
branch
8 times, most recently
from
b741c02
to
cced33e
Compare
brancz
force-pushed
the
clientgo15
branch
3 times, most recently
from
b6b28c9
to
05b1a39
Compare
|
Alright now implemented it with the k8s informer framework. @fabxc |
| func (l NodeLister) List() (v1.NodeList, error) { | ||
| return l() | ||
| } | ||
|
|
||
| // initializeMetrics creates a new controller from the given config. | ||
| func initializeMetrics(kubeClient clientset.Interface) { |
There was a problem hiding this comment.
Function name and comment are far outdated by now I think. Maybe just setup() or something.
| @@ -70,7 +69,7 @@ var ( | |||
| ) | |||
|
|
|||
| type deploymentStore interface { | |||
| List() (deployments []v1beta1.Deployment, err error) | |||
| List() []interface{} | |||
| } | |||
There was a problem hiding this comment.
List() []interface{} is super generic now. Not really necessary to keep one type per resource type anymore.
Maybe just:
type lister interface {
List() []interface{}
}There was a problem hiding this comment.
Or keep them as they were and add a wrapper around the cache's List method doing the typecast there. Saves us from dragging interface{} through the actual application logic layers and we can keep working with more idiomatic go.
| prometheus.MustRegister(&deploymentCollector{store: dplLister}) | ||
| prometheus.MustRegister(&podCollector{store: podLister}) | ||
| prometheus.MustRegister(&nodeCollector{store: nodeLister}) | ||
| go dinf.Run(context.TODO().Done()) |
There was a problem hiding this comment.
We can just use background here as this is basically top-level. From my understanding, TODO is intended for when you want to connect a context chain later on but can't yet because the call chain doesn't fully support it yet.
|
@fabxc Undid the interface weirdness and applied the other comments. |
|
👍 |
use kubernetes informer framework
BUG 1859956: Openshift 1.9.7 bump
Fixes #29 .
@fabxc @ghodss