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.

Sign up for GitHub

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

Jump to bottom

Migrate to FilteredSharedInformer #169

Closed
BugRoger opened this issue Jan 13, 2018 · 1 comment
Closed

Migrate to FilteredSharedInformer #169

BugRoger opened this issue Jan 13, 2018 · 1 comment
Labels
operator

Comments

@BugRoger
Copy link
Contributor

With client-go v6.0.0 there's now a FilteredSharedInformer that can be scoped to a namespace. This allows us to refactor our own custom filter. There's then also potential for simplifying the controller initialisation.

As seen here: https://github.com/sapcc/kubernikus/blob/master/pkg/controller/operator.go#L216-L246

Production Example:
https://github.com/jetstack/cert-manager/blob/b978faa28c9f0fb0414b5d7293fab7bde65bde76/cmd/controller/app/controller.go#L123

See also: http://blog.kubernetes.io/2018/01/introducing-client-go-version-6.html
@databus23
Copy link
Member

databus23 commented Jan 13, 2018 •
edited
Loading

I started looking at this on friday. Its nice for the kubernikus CRD but its not working for our PodInformer because there way have a custom index and you still can't define those (other then we are doing it already with InformerFor)

I'm starting to think that maybe instead of struggling with the factories maybe we should just store and share the Informers directly. I might be missing something but I don't see much benefit in using these factories.

databus23 added a commit that referenced this issue Jan 15, 2018
@databus23
Use filtered informers scoped to given namespace
77ada1e 
To make this work we can’t use the custom “kluster” index for the pod informer anymore. We now use the standard PodLister with a label selector instead. This might incour a performance cost but I think its negligible and we only do this in state creating anyway.

Manually tested to work.

Fixes #169
@databus23 databus23 mentioned this issue Jan 15, 2018
Merged
BugRoger pushed a commit that referenced this issue Jan 15, 2018
@databus23 @BugRoger
Use filtered informers scoped to given namespace
baeac49 
To make this work we can’t use the custom “kluster” index for the pod informer anymore. We now use the standard PodLister with a label selector instead. This might incour a performance cost but I think its negligible and we only do this in state creating anyway.

Manually tested to work.

Fixes #169
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
operator
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@BugRoger @databus23