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

enable template service broker by default #15641

Closed
wants to merge 1 commit into from

Conversation

jim-minter
Copy link
Contributor

fixes #15638

@openshift-merge-robot openshift-merge-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 4, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jim-minter
We suggest the following additional approver: bparees

Assign the PR to them by writing /assign @bparees in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@@ -102,9 +102,6 @@ function os::test::extended::setup () {
cp "${SERVER_CONFIG_DIR}/master/master-config.yaml" "${SERVER_CONFIG_DIR}/master/master-config.orig2.yaml"
openshift ex config patch "${SERVER_CONFIG_DIR}/master/master-config.orig2.yaml" --patch="{\"auditConfig\": {\"enabled\": true}}" > "${SERVER_CONFIG_DIR}/master/master-config.yaml"

cp "${SERVER_CONFIG_DIR}/master/master-config.yaml" "${SERVER_CONFIG_DIR}/master/master-config.orig2.yaml"
openshift ex config patch "${SERVER_CONFIG_DIR}/master/master-config.orig2.yaml" --patch="{\"templateServiceBrokerConfig\": {\"templateNamespaces\": [\"openshift\"]}}" > "${SERVER_CONFIG_DIR}/master/master-config.yaml"

Copy link
Contributor

Choose a reason for hiding this comment

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

woo :)

@bparees
Copy link
Contributor

bparees commented Aug 4, 2017

lgtm (intentionally not tagging since it needs api review)

@jim-minter
Copy link
Contributor Author

/test integration

@openshift-merge-robot openshift-merge-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 4, 2017
@deads2k
Copy link
Contributor

deads2k commented Aug 4, 2017

In 3.7, we built the infrastructure to install a server as a service via a daemonset. In 3.8, we created a separate server hosting the template service broker (already done) and just chained it in.

Why not take advantage of the things we built already and take the last step to split this out before it's too late? @liggitt

@bparees
Copy link
Contributor

bparees commented Aug 4, 2017

In 3.7, we built the infrastructure to install a server as a service via a daemonset. In 3.8, we created a separate server hosting the template service broker (already done) and just chained it in.
Why not take advantage of the things we built already and take the last step to split this out before it's too late? @liggitt

@deads2k by 3.7 and 3.8 i assume you mean k8s 1.7 and 1.8 since openshift 3.7+3.8 don't exist yet?

questions:

  1. does that daemonset server still have seamless access to the cluster etcd for resource storage?
  2. what are the advantages of running it as a daemonset instead of where it is currently running?
  3. is api aggregation seamlessly handled?
  4. Of all the various pod controller mechanisms, why a daemonset?
  5. why would it be "too late" to do it later?

@deads2k
Copy link
Contributor

deads2k commented Aug 4, 2017

Sorry, 3.6 and 3.7. we did the install bits for service catalog and we did the separate server but last week.

It does have access to the cluster etcd (see service catalog). Daemonset to take advantage or servicecatalog infrastructure. The API aggregator is always on in 3.7 (already done).

It's too late to do later because nothing ever moves out of the server after it's on by default and my memory of the template service broker is that it's a one off API (not standard). It's home should be outside the main API server before it's out of alpha or it will never move.

@openshift-ci-robot
Copy link

@jim-minter: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_gce 5e5a23d link /test extended_conformance_gce
ci/openshift-jenkins/extended_templates 5e5a23d link /test extended_templates
ci/openshift-jenkins/integration 5e5a23d link /test integration
ci/openshift-jenkins/extended_conformance_install_update 5e5a23d link /test extended_conformance_install_update

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@bparees
Copy link
Contributor

bparees commented Aug 4, 2017

It does have access to the cluster etcd (see service catalog). Daemonset to take advantage or servicecatalog infrastructure. The API aggregator is always on in 3.7 (already done).

the aggregator being on is not the same as seamless. There is additional effort involved in registering with it, no?

It's too late to do later because nothing ever moves out of the server after it's on by default and my memory of the template service broker is that it's a one off API (not standard). It's home should be outside the main API server before it's out of alpha or it will never move.

The TSB apis (the part that would actually move, since the resource apis would presumably not be moved) are called by the SC, after the TSB tells the SC where it is/how to call it. I don't really see how moving it later is any more difficult than moving it now. While historically nothing may have ever moved out of the server once it's on, is there an actual technical reason for that? Or is it simply a matter of "it became tech debt and people stopped caring?"

@bparees
Copy link
Contributor

bparees commented Aug 4, 2017

Also how is configuration handled when it is launched as a daemonset? For the SC, in cluster up at least, we're hand configuring the deployment that runs it. I guess the ansible install does the same. So that's another piece of net new work that needs to be done to move this out, unless there is a seamless way that these daemonsets are given a copy of the master-config automatically.

@liggitt
Copy link
Contributor

liggitt commented Aug 5, 2017

The master config doesn't seem like the right place to configure an unrelated API server. We should be reducing the number of entangled components, not increasing them.

Even the logic of the template service broker startup indicates it doesn't belong in the same process as the apiserver, since it requires both the apiserver and controllers before it can run.

Once it is enabled in process, it becomes extremely difficult to migrate out of process seamlessly across releases.

We're already seeing the benefit of separate components with the service catalog, which we can update at a different cadence, largely independent of origin and kube versions. As the server intended to be a target of the service catalog, it only makes sense that we'd want the same ability to update independent of origin and kube.

The tech debt argument is also a strong one, given the pain of the existing things we keep in repo, and the inertia that keeps them in their current state

@deads2k
Copy link
Contributor

deads2k commented Aug 7, 2017

Opened #15655 to make openshift start template-service-broker as a starting point.

@deads2k
Copy link
Contributor

deads2k commented Aug 8, 2017

#15672 allows the TSB to run separately.

@openshift-merge-robot
Copy link
Contributor

@jim-minter PR needs rebase

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2017
@jim-minter jim-minter closed this Aug 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-api-review needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable TSB by default in 3.7
6 participants