Skip to content
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

Create dynamic informers and listers #64319

Closed
nikhita opened this issue May 25, 2018 · 16 comments
Closed

Create dynamic informers and listers #64319

nikhita opened this issue May 25, 2018 · 16 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@nikhita
Copy link
Member

nikhita commented May 25, 2018

From @deads2k's comments in #64310 (comment):

I think this is highlighting the need to have a dynamic shared informer factory. If I've reading this code correctly, we've just doubled the number of times we're caching resources and not created the capital to make this easy in general.

With a dynamic shared informer factory, it becomes possible wire the "get informer for resource" to delegate across multiple candidates as we do in openshift. All that helps the general cause and avoids unnecessary duplication.

Controllers like the GC controller and resource quota controller use informers and listers. Currently, we only have support for static, generic informers and listers.

To work with custom resources, the GC controller creates a new informer each time it comes across an unknown resource.

glog.V(5).Infof("create storage for resource %s", resource)
_, monitor := cache.NewInformer(
listWatcher(gb.dynamicClient, resource),
nil,
ResourceResyncTime,
// don't need to clone because it's not from shared cache
handlers,
)

If we try to do something similar for the RQ controller as well (creating a new informer and lister, see #64201), we end up increasing the number of times we cache resources. This also does not make it easy if we want to add a similar behaviour in the future.

To avoid this, we can tackle the problem by creating dynamic informers, then dynamic listers, then shared dynamic informers, then "reactive" shared dynamic informers (ones where it can figure out that the resource has been removed and recreated).

/sig api-machinery
/kind feature
/cc @deads2k @sttts @liggitt @lavalamp @mbohlool

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/feature Categorizes issue or PR as related to a new feature. labels May 25, 2018
@fedebongio
Copy link
Contributor

/cc @caesarxuchao @yliaog @roycaihw

@carolynvs
Copy link

This would let us completely redo how service catalog works, and make the UX really rock. I'd love to help make it happen or test it out with a prototype as soon as you've got it! 💯

@nikhita
Copy link
Member Author

nikhita commented Jun 29, 2018

I'm working on this and just discussed it with @sttts today. @enisoc already added something similar in metacontroller https://github.com/GoogleCloudPlatform/metacontroller/tree/master/dynamic/informer to make our lives easy developing it for kube. :)

@enisoc
Copy link
Member

enisoc commented Jun 29, 2018

I've been appropriately shamed by @deads2k for not doing that work upstream in the first place. In my defense, at the time the dynamic client hadn't seen much attention in a while; I thought I was the only one using it. :)

I look forward to rebasing on an official upstream library. Let me know if I can help with reviews or anything.

@caesarxuchao
Copy link
Member

cc @janetkuo I remember your original new GC design depends on this feature.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 27, 2018
@carolynvs
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 8, 2018
@carolynvs
Copy link

@nikhita Do you know if work has been begun on this already? If not, perhaps I could help implement it?

@nikhita
Copy link
Member Author

nikhita commented Oct 8, 2018

@p0lyn0mial has been working on it :)

@p0lyn0mial
Copy link
Contributor

yeah, I almost have it :)

/assign @p0lyn0mial

@p0lyn0mial
Copy link
Contributor

dynamic lister already merged, see: #68748

@p0lyn0mial
Copy link
Contributor

dynamic informer factory has just been merged #69308
Is there anything else we would like to implement or can we close this issue ?

@nikhita
Copy link
Member Author

nikhita commented Oct 12, 2018

Is there anything else we would like to implement or can we close this issue ?

I think we can close this. Thanks, @p0lyn0mial! 🎉

Closing the issue. Please reopen if needed.

/close

@k8s-ci-robot
Copy link
Contributor

@nikhita: Closing this issue.

In response to this:

Is there anything else we would like to implement or can we close this issue ?

I think we can close this. Thanks, @p0lyn0mial! 🎉

Closing the issue. Please reopen if needed.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@carolynvs
Copy link

O.M.G I can't wait to try it out! 🏃‍♀️ 💨

@krmayankk
Copy link

I know there is no requirement for documentation on internal enhancements, but if there is a scope for that , please add a blob about how to use it , for both core and controller writers . I have been looking for an internal deep dive on informers but haven’t found one and it keeps improving :-) @Nikitha @p0lyn0mial

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

9 participants