Switch image quota to shared informers #12088
Conversation
|
Note to myself, try looking into how to solve the problem of "use this cache if available". |
openshift-bot
added
the
needs-rebase
soltysh
changed the title
soltysh
removed
the
needs-rebase
|
@deads2k this is ready for review, if there's something more that should be added to fulfill the attached card lemme know |
| @@ -221,7 +221,7 @@ func BuildMasterConfig(options configapi.MasterConfig) (*MasterConfig, error) { | |||
|
|
|||
| kubeletClientConfig := configapi.GetKubeletClientConfig(options) | |||
|
|
|||
| quotaRegistry := quota.NewAllResourceQuotaRegistry(privilegedLoopbackOpenShiftClient, privilegedLoopbackKubeClientset, kubeInformerFactory) | |||
| quotaRegistry := quota.NewAllResourceQuotaRegistry(informerFactory, privilegedLoopbackOpenShiftClient, privilegedLoopbackKubeClientset) | |||
There was a problem hiding this comment.
Is this one used for admission? The admission one probably doesn't need the informers.
There was a problem hiding this comment.
This one already accepted informers, but just k8s ones, but I needed both k8s and origin ones. I've also moved this parameter from the end of this function call as the first argument, to make it similar to other methods. And since quota.NewAllResourceQuotaRegistry under covers creates evaluators which are now switched to informers, it's needed this way.
|
yeah, this is what I meant. |
|
[test] |
| results = append(results, &itemList.Items[i]) | ||
| results := make([]runtime.Object, 0, len(list)) | ||
| for _, is := range list { | ||
| objCopy, err := kapi.Scheme.DeepCopy(is) |
There was a problem hiding this comment.
Is it necessary to deep-copy here?
There was a problem hiding this comment.
If I don't I might end up modifying cache, and that's not desired, right?
There was a problem hiding this comment.
You should deep-copy when you are about to mutate but I am not sure if we gain much. I guess it depends on how much we mutate below this point.
There was a problem hiding this comment.
Evaluator should not mutate, by design, so you might be right. I'll wait for @deads2k review and his advice on that, if you don't mind.
There was a problem hiding this comment.
If I don't I might end up modifying cache, and that's not desired, right?
Quota counts won't do mutation. In general, we've tried to avoid unnecessary deep copies, so I'm used to seeing a pattern in controllers where you check whether you are likely to have changes against your shared cache reference. After you've eliminated all the obvious pre-checks, you can make a copy in one spot to make the code easier to reason about, but it should be minority of cases.
Doing this copy here would be too early for a normal controller and we happen to know in this case we don't need it, so I would skip the copies.
| fakeClient.AddReactor("list", "imagestreams", imagetest.GetFakeImageStreamListHandler(t, tc.iss...)) | ||
|
|
||
| evaluator := NewImageStreamEvaluator(fakeClient) | ||
| isInformer := cache.NewSharedIndexInformer( |
There was a problem hiding this comment.
This is just a straight counting evaluator, right? If so, I'm not sure you need a test at all. The counting is checked upstream.
There was a problem hiding this comment.
You know I like more then less testing 😉
|
just the deep copy, then lgtm. |
|
Comments addressed, will merge on green tests. |
|
Evaluated for origin test up to e2dbd12 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12839/) (Base Commit: 8a41d6f) |
|
Merging on previous approval. [merge] |
|
Evaluated for origin merge up to e2dbd12 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12875/) (Base Commit: 46bdbfa) (Image: devenv-rhel7_5677) |
This is addressing this card: https://trello.com/c/Gc7NkZoe/765-5-quota-performance-improvements
@deads2k is that good direction, I prefer asking rather than going to deep with it?