Reuse the authorization and template shared informers for GC #14391
Conversation
smarterclayton
force-pushed
the
reuse_informers
branch
from
32a3f30 to
92ea314
Compare
|
@liggitt I am confused why the preferred group versions are changing |
|
I'm not sure either |
|
[test] |
smarterclayton
force-pushed
the
reuse_informers
branch
from
92ea314 to
3218b05
Compare
|
It's because the generated informers require their API group to be registered. |
|
Bolt db segmentation fault [test]
|
|
Fixed, review.
…On Sat, Jun 3, 2017 at 10:00 PM, OpenShift Bot ***@***.***> wrote:
continuous-integration/openshift-jenkins/test SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1920/)
(Base Commit: 90592c5
<90592c5>
)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14391 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pwg0LhQxEwmHoYp69bFuf4vV8pVtks5sAg-ngaJpZM4NorhY>
.
|
|
@enj also for review |
pkg/cmd/server/start/start_master.go
Outdated
| ForResource(resource schema.GroupVersionResource) (kinformers.GenericInformer, error) | ||
| } | ||
|
|
||
| type genericInformers struct { |
There was a problem hiding this comment.
This should be in a separate package. I agree that combining informers is useful, but I suspect we'll need it more generally.
There was a problem hiding this comment.
This was just the adapter interface for GC, if you want to start a brand new controller factory i want to do that in a separate PR where the rest of them get converted.
pkg/cmd/server/start/start_master.go
Outdated
| } | ||
|
|
||
| type genericInformers struct { | ||
| kinformers.SharedInformerFactory |
There was a problem hiding this comment.
Why must this be anonymous?
There was a problem hiding this comment.
this is passing through all the kubernetes informer methods
pkg/cmd/server/start/start_master.go
Outdated
| ClientBuilder: saClientBuilder, | ||
| InformerFactory: genericInformers{ | ||
| SharedInformerFactory: oc.Informers.KubernetesInformers(), | ||
| generic: []GenericResourceInformer{oc.Informers}, |
There was a problem hiding this comment.
Isn't this wiring in the hand coded informers? I'm against including them in new places.
There was a problem hiding this comment.
We need those to reuse existing informers
| @@ -81,13 +87,26 @@ type sharedInformerFactory struct { | |||
| customListerWatchers ListerWatcherOverrides | |||
| defaultResync time.Duration | |||
|
|
|||
| templateInformers templateinformers.SharedInformerFactory | |||
There was a problem hiding this comment.
I don't like this. If we must have a new struct, we should be building out a new struct that doesn't carry the legacy of hand-coded informers with it.
There was a problem hiding this comment.
What do you want the tsruct to look like? The upstream shared struct? This is the generated type
|
Will remove Register so we carve registries off.
…On Tue, Jun 6, 2017 at 10:25 AM, David Eads ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/controller/shared/shared_informer.go
<#14391 (comment)>:
> @@ -81,13 +87,26 @@ type sharedInformerFactory struct {
customListerWatchers ListerWatcherOverrides
defaultResync time.Duration
+ templateInformers templateinformers.SharedInformerFactory
I don't like this. If we must have a new struct, we should be building out
a new struct that doesn't carry the legacy of hand-coded informers with it.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14391 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pyyhPC3cbuEi9y_91BfY2WL49wpZks5sBWFbgaJpZM4NorhY>
.
|
smarterclayton
force-pushed
the
reuse_informers
branch
from
3218b05 to
9e43c92
Compare
|
Refactored to be clearer, removed Register, and excluded a lot more resources that I honestly don't care about right now (mostly ones without reflectors). |
smarterclayton
force-pushed
the
reuse_informers
branch
from
9e43c92 to
7b2124a
Compare
|
[merge] based on feedback from previous
…On Fri, Jun 9, 2017 at 3:06 PM, OpenShift Bot ***@***.***> wrote:
continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2066/)
(Base Commit: 9279133
<9279133>
)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14391 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p6ZbehPkf25Bttuz7ZslNsrAie7jks5sCZelgaJpZM4NorhY>
.
|
Allow them to be loaded via ForResource() as a stepping stone to transforming their behavior. Set projects and tokens as GC excluded resources (virtual, and security related).
smarterclayton
force-pushed
the
reuse_informers
branch
from
7b2124a to
37ebaf7
Compare
|
Evaluated for origin test up to 37ebaf7 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2089/) (Base Commit: c9a1d10) |
|
Evaluated for origin merge up to 37ebaf7 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/966/) (Base Commit: 52661e3) (Image: devenv-rhel7_6338) |
Allow them to be loaded via ForResource() as a stepping stone to
transforming their behavior.
For #8229
[test]