-
Notifications
You must be signed in to change notification settings - Fork 2k
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
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. 🎉 |
b741c02
to
cced33e
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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