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

deads2k · 2017-08-09T16:38:53Z

This updates the TSB test to run during the conformance run and make use of the example install template that cluster-up uses as well.

@bparees @jim-minter if this works, we can remove the extended template test, right?

openshift-merge-robot · 2017-08-09T16:38:58Z

[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.

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

~~examples/OWNERS~~ [deads2k]
~~hack/OWNERS~~ [deads2k]
~~pkg/OWNERS~~ [deads2k]
~~test/extended/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

bparees · 2017-08-09T16:44:33Z

hack/update-generated-bindata.sh

@@ -51,7 +51,8 @@ pushd "${OS_ROOT}" > /dev/null
    examples/sample-app \
    examples/prometheus/... \
    examples/hello-openshift \
-    examples/jenkins/...
+    examples/jenkins/... \
+    examples/templateservicebroker/...


if this is fundamental to the product, even "examples" doesn't seem like the right location. Do we have any other templates being used in this way? where do they live?

if this is fundamental to the product, even "examples" doesn't seem like the right location. Do we have any other templates being used in this way? where do they live?

I asked @sdodson about where to put them. He asked to have it here.

bparees · 2017-08-09T16:45:52Z

test/extended/templates/helpers.go

+		// this is weird, but we have to ignore this error in case some of this already exists (race between tests and the like)
+		//o.Expect(err).NotTo(o.HaveOccurred())
+		err = WaitForDaemonSetStatus(tsbOC.AdminKubeClient(), &extensions.DaemonSet{ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: tsbNS}})
+		o.Expect(err).NotTo(o.HaveOccurred())


can any part of this be shared w/ what cluster up is doing? i'm worried we're going to end up with 3 different "install the TSB" codepaths:

cluster up

extended tests

ansible

can any part of this be shared w/ what cluster up is doing? i'm worried we're going to end up with 3 different "install the TSB" codepaths:

cluster up
extended tests
ansible

Basing them on the same template resolves most problems. Obviously you have at least two paths, since ansible is written in python.

Then you're just talking about test code versus cmd line code. Given the differences in how errors are detected and handled, along with the differences in client accessibility, I'm guessing I could roughly double the total size of code from both halves to try to abstract out the difference and create a new level of abstraction to eliminate the duplication.

I recommend keeping it like this. Share a template, run the logic in a way that is "normal" in its domain.

[grumbling acquiescence goes here]

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

test/extended/templates/helpers.go

+		o.Expect(err).NotTo(o.HaveOccurred())
+	}
+
+	// we're trying to test the TSB, not the service.  We're outside all the normal networks.  Run a portforward to a particular


@bparees @jim-minter hopefully you never have to mess with this, but you should be aware that this is portforward and why.

still wondering if we should be biting the bullet and putting a route in the template for when someone inevitably wants to run the SC off-cluster anyway. Is there a compelling reason not to?

still wondering if we should be biting the bullet and putting a route in the template for when someone inevitably wants to run the SC off-cluster anyway. Is there a compelling reason not to?

I'm unclear on the off cluster case. It's possible, but not easy, to run an aggregated api server off cluster. You really think its likely that someone will want to do this? Even if I had a router, I'm not sure our e2e tests set up dns in a way that will make the route dns name work here.

I'd rather see that scoped as a different piece work and I'd suggest trying limit the exposure of your server as much as possible. I would have gone in the other direction and tried to set up network ingress rules

I can envision someone running the SC in one cluster, talking to brokers in a different cluster. Admittedly there are some auth issues to deal with (what identify is provided by the SC when it calls the TSB?) but it's certainly part of the conceptual architecture of the SC/SB model.

Anyway yes it can wait, I just figured it would remove this port forwarding complexity at the same time, thus a net win.

Anyway yes it can wait, I just figured it would remove this port forwarding complexity at the same time, thus a net win.

Are you familiar with how the routers are built in our e2e envs? I'm not, so I'd prefer a followup.

bparees · 2017-08-10T03:07:45Z

test/extended/templates/helpers.go

+	// wait to get back healthy from the tsb
+	healthResponse := ""
+	err = wait.Poll(e2e.Poll, 2*time.Minute, func() (bool, error) {
+		resp, err := tsbclient.Client().Get("https://" + svc.Spec.ClusterIP + "/healthz")


do daemonsets have readiness checks? this seems like it should be a readiness check in the template.

do daemonsets have readiness checks? this seems like it should be a readiness check in the template.

See #15681

do daemonsets have readiness checks? this seems like it should be a readiness check in the template.
See #15681

A more detailed response: I'd like to have a test that checks the proper functioning of the template before I muck around it any more than I have to.

jim-minter · 2017-08-10T13:30:53Z

test/extended/templates/helpers.go

+
+	if !exists {
+		e2e.Logf("Installing TSB onto the cluster for testing")
+		_, _, err := tsbOC.AsAdmin().WithoutNamespace().Run("create", "namespace").Args(tsbNS).Outputs()


do we run into trouble here if multiple tests run in parallel and run EnsureTSB at the same time?

worth considering instantiating the tsb into the current / a separate throwaway namespace?

hmm, maybe I'm just saying that the o.Expect(err).NotTo(o.HaveOccurred()) in 94 should be commented (or something) for the same reason as at 100

hmm, maybe I'm just saying that the o.Expect(err).NotTo(o.HaveOccurred()) in 94 should be commented (or something) for the same reason as at 100

Yeah. This is all shim code until the installer sets this up for us.

jim-minter · 2017-08-10T15:09:01Z

test/extended/templates/helpers.go

+	if pod == nil {
+		e2e.Failf("no ready pod found")
+	}
+	port, err := intframework.FindFreeLocalPort()


there's a race here between finding the port and binding to it

there's a race here between finding the port and binding to it

Yes there is. In practice it's uncommon.

jim-minter · 2017-08-10T15:09:41Z

test/extended/templates/helpers.go

+	}
+	port, err := intframework.FindFreeLocalPort()
+	o.Expect(err).NotTo(o.HaveOccurred())
+	portForwardCmd, _, _, err := tsbOC.AsAdmin().WithoutNamespace().Run("port-forward").Args("-n="+tsbNS, pod.Name, fmt.Sprintf("%d:8443", port)).Background()


the Background() will require the caller to defer a cleanup - not currently done, and suggest documenting this function to indicate the requirement as well

oh, sorry, I see it's in the AfterEach. Doc would be good though

h, sorry, I see it's in the AfterEach. Doc would be good though

doc'ed

deads2k · 2017-08-10T16:11:01Z

/retest

deads2k · 2017-08-10T16:57:56Z

I have rebased on #15677 since that is going to merge shortly.

I have added a commit which disables front proxy auth and client cert auth on the TSB server until openshift/openshift-ansible#5056 is fixed. Aggregated api servers fail if they can't be proxied to (that's why this was failing), but the TSB can work without it for a little while. I will open a revert of that commit to be merged when the aggregation values are present.

deads2k · 2017-08-11T00:01:27Z

Let's see if images propagated.

/retest

deads2k · 2017-08-14T18:11:06Z

The authentication changes were lgtm'd separately. Tagging this (finally).

deads2k · 2017-08-14T19:47:04Z

test/extended/templates/templateservicebroker_e2e.go

@@ -340,6 +342,7 @@ var _ = g.Describe("[templates] templateservicebroker end-to-end test", func() {
 	}

 	g.It("should pass an end-to-end test", func() {
+		framework.SkipIfProviderIs("gce")


@bparees did I somehow do this wrong?

deads2k · 2017-08-14T21:18:56Z

/test end_to_end

deads2k · 2017-08-15T02:57:30Z

/retest

openshift-bot · 2017-08-15T09:08:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

deads2k · 2017-08-15T11:40:18Z

/retest

deads2k · 2017-08-15T11:52:08Z

this test is still going to be valid when it finishes (nothing in queue, nothing merged).

deads2k · 2017-08-15T13:49:14Z

/retest

deads2k · 2017-08-15T16:49:02Z

#13731

/retest

deads2k · 2017-08-15T17:06:50Z

/test verify

deads2k · 2017-08-15T17:15:19Z

/test all

deads2k · 2017-08-15T18:36:27Z

/retest

deads2k · 2017-08-15T22:12:45Z

/retest

openshift-merge-robot · 2017-08-16T01:18:48Z

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

openshift-merge-robot · 2017-08-16T03:10:55Z

Automatic merge from submit-queue

deads2k assigned jim-minter and bparees Aug 9, 2017

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2017

bparees reviewed 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 force-pushed the tsb-13-e2e branch from 191546d to 5354651 Compare August 9, 2017 19:54

deads2k commented Aug 9, 2017

View reviewed changes

bparees reviewed Aug 10, 2017

View reviewed changes

deads2k force-pushed the tsb-13-e2e branch 2 times, most recently from 1fd0a4e to 77a8843 Compare August 10, 2017 12:11

deads2k mentioned this pull request Aug 10, 2017

add template for TSB #15673

Merged

jim-minter reviewed Aug 10, 2017

View reviewed changes

deads2k force-pushed the tsb-13-e2e branch from 77a8843 to 0550dcd Compare August 10, 2017 14:47

jim-minter reviewed Aug 10, 2017

View reviewed changes

deads2k force-pushed the tsb-13-e2e branch from 0550dcd to c94bd12 Compare August 10, 2017 16:56

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

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

deads2k force-pushed the tsb-13-e2e branch from c94bd12 to 982b9e1 Compare August 10, 2017 20:35

openshift-merge-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 10, 2017

deads2k force-pushed the tsb-13-e2e branch from 982b9e1 to c7cf3bf Compare August 11, 2017 11:42

deads2k force-pushed the tsb-13-e2e branch from ec0295f to e21fe44 Compare August 14, 2017 18:10

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

deads2k commented Aug 14, 2017

View reviewed changes

deads2k added 3 commits August 14, 2017 15:51

deeper specification of authentication chain in TSB

cb2f816

run TSB test as part of the normal conformance bucket

18599bb

skip tsb tests on GCE until new images are published

3e9c66c

deads2k force-pushed the tsb-13-e2e branch from e21fe44 to 3e9c66c Compare August 14, 2017 19:54

deads2k added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Aug 14, 2017

deads2k added retest-not-required and removed retest-not-required labels Aug 15, 2017

UPSTREAM: <drop>: skip TSB namespace when checking for scheduled pods

e060970

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2017

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

openshift-merge-robot merged commit a2b1043 into openshift:master Aug 16, 2017

deads2k deleted the tsb-13-e2e branch January 24, 2018 14:32

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

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

Uh oh!

Conversation

deads2k commented Aug 9, 2017

Uh oh!

openshift-merge-robot 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!

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 10, 2017

Uh oh!

deads2k commented Aug 10, 2017

Uh oh!

deads2k commented Aug 11, 2017

Uh oh!

deads2k commented Aug 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Aug 14, 2017

Uh oh!

deads2k commented Aug 15, 2017

Uh oh!

openshift-bot commented Aug 15, 2017

Uh oh!

deads2k commented Aug 15, 2017

Uh oh!

deads2k commented Aug 15, 2017

Uh oh!

deads2k commented Aug 15, 2017

Uh oh!

deads2k commented Aug 15, 2017

Uh oh!

deads2k commented Aug 15, 2017

Uh oh!