make TSB run separately in oc cluster up #15677

deads2k · 2017-08-08T15:54:52Z

This combines several other pulls together to make the TSB run separately in cluster-up. I tested it locally via the admin console.

openshift-merge-robot · 2017-08-08T15:55:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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

Details

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

~~OWNERS~~ [deads2k]

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

jim-minter · 2017-08-08T16:51:18Z

hack/update-generated-bindata.sh

    examples/service-catalog/... \
-    pkg/image/admission/imagepolicy/api/v1/...
+    pkg/image/admission/imagepolicy/api/v1/... \
+    test/testdata/templateservicebroker/...


wonder whether this should be examples/templateservicebroker

wonder whether this should be examples/templateservicebroker

We're going to need it to update the e2e tests to create based on the template.

hack/update-generated-bindata.sh already pulls from many locations in examples for extended tests, a new one would need to be added, that's all

jim-minter · 2017-08-08T16:55:11Z

pkg/oc/bootstrap/docker/openshift/templateservicebroker.go

+	}
+
+	// Wait for the apiserver endpoint to become available
+	err = wait.Poll(5*time.Second, 600*time.Second, func() (bool, error) {


@csrwng ptal - wonder if we should wait here or whether this should go in oc cluster status? Also whether a /healthz should be used instead, if it exists, that is

Also whether a /healthz should be used instead, if it exists, that is

You're not guaranteed to have access to the service network, right? If we get the healthcheck right (TODO in the template I guess), then this does what we want.

true; I think that for better or worse, in general oc cluster up doesn't wait for things to be ready before returning. I think the API aggregator may be the single exception

@jim-minter @deads2k if the web console is unusable before this starts then yes, I agree, it makes sense to wait here.

@jim-minter @deads2k if the web console is unusable before this starts then yes, I agree, it makes sense to wait here.

@csrwng it looks to be a pre-existing problem. oc cluster up doesn't wait for the broker.servicecatalog/status to be ready before finishing. For some reason, that takes quite a while (long after the TSB is running).

oc cluster up (today) waits for the SC api to be available because it has to register the TSB with the SC before it can proceed. If you're saying that the api becomes available before the service catalog actually considers itself ready (and that the SC isn't ready until significantly later) that's news to me, i've never seen an issue doing oc cluster up and then going straight to the web console.

If you're saying that the api becomes available before the service catalog actually considers itself ready (and that the SC isn't ready until significantly later) that's news to me, i've never seen an issue doing oc cluster up and then going straight to the web console.

Takes maybe 15 seconds. The service catalog is running, but the service catalog hasn't filled in any status (good or bad) on the broker resource.

by broker resource you mean the TSB broker resource in particular? ie we've created the TSB broker resource object (registered the TSB w/ the SC) but the SC hasn't updated the status to indicate it has successfully imported the TSB's catalog?

If so my guess is we're in the following flow:

SC has come up

TSB is registered but auth is not dropped yet

SC tries to call the TSB to get the catalog, can't (due to lack of auth)

SC enters some sort of backoff/retry loop for (3) (no idea what the interval is)

you drop auth

SC eventually retries and gets the catalog

If so, i guess that would imply a couple things:

once the SC supports auth the problem mostly goes away

the SC should be updating the broker resource w/ some sort of status while its failing to reach the TSB due to auth errors

If so my guess is we're in the following flow:

Sounds probable. I'm going to leave this as-is for now then.

@pmorie should be able to 1) confirm my suspicions and 2) clarify what the retry interval is and why the status isn't updated while it's retrying.

jim-minter · 2017-08-08T16:56:19Z

pkg/oc/bootstrap/docker/up.go


+	// Install template service broker
+	c.addTask(conditionalTask("Installing template service broker", c.InstallTemplateServiceBroker, func() bool {
+		return c.ShouldInstallServiceCatalog


typo? is useTemplateServiceBroker used?

typo? is useTemplateServiceBroker used?

I'll move it.

jim-minter · 2017-08-08T16:58:20Z

test/testdata/templateservicebroker/templateservicebroker-template.yaml

+parameters:
+- name: IMAGE
+  value: openshift/origin:latest
+- name: TSB_TEMPLATE_NAMESPACE


TSB_TEMPLATE_NAMESPACES plural?

TSB_TEMPLATE_NAMESPACES plural?

It's a weird thing. I don't know how to plumb this through with a template to the arg. I'm going to talk with @sdodson about how we want to handle config long term. Can I carry it as-is for now knowing it won't survive contact with ansible?

jim-minter · 2017-08-08T17:04:28Z

test/testdata/templateservicebroker/templateservicebroker-template.yaml

+  name: template-service-broker
+parameters:
+- name: IMAGE
+  value: openshift/origin:latest


worried about the how this gets productised (something I know nothing about) - is it down to ansible and oc cluster up to handle setting this according to the version and handling upgrade? presumably oc cluster up should be setting this then?

worried about the how this gets productised (something I know nothing about) - is it down to ansible and oc cluster up to handle setting this according to the version and handling upgrade? presumably oc cluster up should be setting this then?

Yes, but I have to work with @sdodson on whether and how to handle the image bits.

jim-minter · 2017-08-08T17:06:03Z

test/testdata/templateservicebroker/templateservicebroker-template.yaml

+        name: apiserver
+        labels:
+          apiserver: "true"
+      spec:


parameterised nodeselector needed?

parameterised nodeselector needed?

I think I'd leave it to be controlled by the namespace annotations to avoid being prescriptive about deployment topology.

jim-minter · 2017-08-08T17:15:13Z

pkg/oc/bootstrap/docker/openshift/templateservicebroker.go

+	}
+
+	// create the actual resources required
+	if err = instantiateTemplate(osClient, clientcmd.ResourceMapper(f), tsbNamespace, tsbTemplateName, tsbNamespace, nil, true); err != nil {


Think parameter IMAGE should be being set here

Think parameter IMAGE should be being set here

Ok. I spoke with @bparees and he's ok with versioning gating all of service-catalog on 3.7 (no longer allow 3.6), so I think I can do that.

deads2k · 2017-08-09T14:13:04Z

Comments addressed and rebased on the latest master.

@jim-minter ptal

deads2k · 2017-08-09T14:13:46Z

pkg/oc/bootstrap/docker/openshift/templateservicebroker.go

+	imageTemplate.Latest = false
+
+	if err = instantiateTemplate(osClient, clientcmd.ResourceMapper(f), tsbNamespace, tsbTemplateName, tsbNamespace, map[string]string{
+		"IMAGE": imageTemplate.ExpandOrDie(""),


image format used here.

deads2k · 2017-08-09T14:14:03Z

pkg/oc/bootstrap/docker/up.go

+
+	// service catalog requires the TSB, which requires 3.7
+	serverVersion, _ := c.OpenShiftHelper().ServerPrereleaseVersion()
+	if serverVersion.LT(openshiftVersion37) {


@bparees version gating here

Does this prevent someone instantiating a 3.6 server + SC using a latest origin oc client? In theory the idea is that this should be possible.

it does. After much discussion between @deads2k and I, this is the path of least resistance. It's not ideal but we're not going to lose too much sleep over it given the tech preview nature of the SC in 3.6.

If you're using the latest(3.7) oc client binary and you want to use the SC/TSB with cluster up, you will need to also use the latest (3.7) images.

deads2k · 2017-08-09T14:14:19Z

pkg/oc/bootstrap/docker/up.go

-	// special case for someone who is building a local image using commits based on 3.6.0.alpha2.  They most likely
-	// have the necessary changes.
-	if serverVersion.EQ(openshiftVersion36alpha2) && (serverVersion.String() != openshiftVersion36alpha2.String()) {
+	if c.ImageVersion == "latest" {


I special cased --version=latest since that should be latest.

we don't know what "openshift/origin:latest" is on their local machine so i think you still need to check the server version. passing "--version=latest" to oc cluster up does not enforce a pullalways behavior.

we don't know what "openshift/origin:latest" is on their local machine so i think you still need to check the server version. passing "--version=latest" to oc cluster up does not enforce a pullalways behavior.

That would block this pull on a new tag to be able to better handle cases where someone built the latest oc from source, did not build the images, did not pull new images, and ran with --service-catalog=true. Would you rather block on on that edge or merge this (which will work for most sane cases against head) and take a TODO?

deads2k · 2017-08-09T14:14:45Z

pkg/oc/bootstrap/docker/up.go

+	}
+	// TODO we want to use this eventually, but until we have our own image for TSB, we have to hardcode this origin
+	//return c.OpenShiftHelper().InstallTemplateServiceBroker(f, c.imageFormat())
+	return c.OpenShiftHelper().InstallTemplateServiceBroker(f, c.Image)


@jim-minter I had to hardcode the image format to be our base image. See the comment above.

makes sense

deads2k · 2017-08-09T17:12:23Z

/retest

deads2k · 2017-08-09T19:55:51Z

@bparees can you confirm lgtm-ness here

bparees · 2017-08-09T20:08:59Z

lgtm (i am not /lgtm'ing any of these PRs because i don't know what order you're trying to merge things in)

openshift-merge-robot · 2017-08-10T06:33:21Z

/test all [submit-queue is verifying that this PR is safe to merge]

deads2k · 2017-08-10T11:36:08Z

/retest

deads2k · 2017-08-10T12:08:00Z

/test end_to_end

deads2k · 2017-08-10T12:08:31Z

Rechecking the cluster up test. Nothing else I have conflicts. letting this in without retest.

deads2k · 2017-08-10T12:58:15Z

/test extended_conformance_install_update

deads2k · 2017-08-10T14:47:17Z

/retest

openshift-merge-robot · 2017-08-10T18:16:35Z

Automatic merge from submit-queue

deads2k assigned jim-minter and bparees Aug 8, 2017

openshift-merge-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 8, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 8, 2017

deads2k force-pushed the tsb-07-oc-up branch from b37b9a2 to 6278b05 Compare August 8, 2017 16:35

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017

jim-minter reviewed Aug 8, 2017

View reviewed changes

jim-minter mentioned this pull request Aug 8, 2017

add template for TSB #15673

Merged

jim-minter reviewed Aug 8, 2017

View reviewed changes

make cluster-up work with separate TSB

73da3e1

deads2k force-pushed the tsb-07-oc-up branch from 6278b05 to 73da3e1 Compare August 9, 2017 14:11

openshift-merge-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 9, 2017

deads2k commented Aug 9, 2017

View reviewed changes

deads2k mentioned this pull request Aug 9, 2017

stop running the TSB in the main apiserver #15684

Merged

deads2k added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2017

deads2k added the retest-not-required label Aug 10, 2017

jim-minter mentioned this pull request Aug 10, 2017

Stop allowing oc cluster up version X to instantiate a server version X-1 #15731

Closed

deads2k mentioned this pull request Aug 10, 2017

run TSB test as part of the normal conformance bucket #15707

Merged

openshift-merge-robot merged commit adc2cab into openshift:master Aug 10, 2017

deads2k deleted the tsb-07-oc-up branch January 24, 2018 14:31

make TSB run separately in oc cluster up #15677

make TSB run separately in oc cluster up #15677

Uh oh!

Conversation

deads2k commented Aug 8, 2017

Uh oh!

openshift-merge-robot commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Aug 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Aug 9, 2017

Uh oh!

deads2k commented Aug 9, 2017

Uh oh!

bparees commented Aug 9, 2017

Uh oh!