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

don't start template informer unless templateservicebroker is enabled #14579

Merged

Conversation

jim-minter
Copy link
Contributor

should resolve #8229 (comment), #14216 (review)

(also broken out TemplateInstanceControllerConfig into its own source file, didn't belong in apps.go)

@jim-minter
Copy link
Contributor Author

@smarterclayton can you confirm this was what you were looking for?

@@ -464,7 +464,9 @@ func (m *Master) Start() error {
openshiftConfig.AppInformers.Start(utilwait.NeverStop)
openshiftConfig.AuthorizationInformers.Start(utilwait.NeverStop)
openshiftConfig.ImageInformers.Start(utilwait.NeverStop)
openshiftConfig.TemplateInformers.Start(utilwait.NeverStop)
if openshiftConfig.Options.TemplateServiceBrokerConfig != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't right - you should be avoiding the call to TemplateInformers.Template(). Start is already guarded by "has anyone asked for this informer yet".

@jim-minter
Copy link
Contributor Author

tried and failed to be smarter than @smarterclayton - ptal and merge.

@jim-minter jim-minter force-pushed the guard_template_informer_start branch from a26680f to fc5e181 Compare June 13, 2017 16:53
@smarterclayton
Copy link
Contributor

Phenomenal lgtm [merge] thanks

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift openshift deleted a comment from openshift-bot Jun 14, 2017
@bparees
Copy link
Contributor

bparees commented Jun 14, 2017

flake #13108
[test]

@smarterclayton
Copy link
Contributor

[test]

@bparees
Copy link
Contributor

bparees commented Jun 15, 2017

[merge][severity:bug]

@jim-minter
Copy link
Contributor Author

@bparees @csrwng merge failed because FAIL github.com/openshift/origin/pkg/build/controller/build [build failed] - unit test failure. I'm confused about this - if I locally rebase the branch onto latest master and do go test ./pkg/build/controller/build it works fine. Any thoughts?

@bparees
Copy link
Contributor

bparees commented Jun 15, 2017

not sure how this tested success and then failed to merge, i guess it got conflicted by another PR:

# github.com/openshift/origin/pkg/build/controller/build
pkg/build/controller/build/build_controller_test.go:961: c.informers.StartCore undefined (type shared.InformerFactory has no field or method StartCore)

@bparees
Copy link
Contributor

bparees commented Jun 15, 2017

@jim-minter uhhh... that's extremely weird.
@smarterclayton do you know what might have happened here?

meanwhile.... [test]

@jim-minter
Copy link
Contributor Author

@bparees bbd8d9a

@jim-minter
Copy link
Contributor Author

@bparees think you can just re-hit merge

@bparees
Copy link
Contributor

bparees commented Jun 15, 2017

@jim-minter ah hah. you caught the window.
[merge][severity:bug]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to fc5e181

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 15, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1009/) (Base Commit: 84b8802) (Extended Tests: bug) (Image: devenv-rhel7_6362)

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to fc5e181

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2269/) (Base Commit: 84b8802)

@openshift-bot openshift-bot merged commit 98b2737 into openshift:master Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate caches in OpenShift master
4 participants