add template for TSB #15673

deads2k · 2017-08-08T15:40:31Z

Separate pull that uses a daemonset in a template to run the TSB.

openshift-merge-robot · 2017-08-08T15:40:39Z

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

~~hack/OWNERS~~ [deads2k]
~~pkg/oc/bootstrap/OWNERS~~ [deads2k]
~~test/testdata/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

deads2k · 2017-08-08T16:38:46Z

/test integration

deads2k · 2017-08-08T17:01:46Z

/test extended_conformance_gce

deads2k · 2017-08-08T17:04:01Z

@bparees @jim-minter rather than try to make this template perfect now, I'd like to start with "not offensive" and work out from there. I'm happy to explain any args if they look weird.

jim-minter · 2017-08-08T17:14:08Z

ah, ok - I ended up commenting on the template bits at #15677 before reading this.

deads2k · 2017-08-08T17:14:54Z

ah, ok - I ended up commenting on the template bits at #15677 before reading this.

no problem. The comments are fair (I responded there). I can open issues if you're willing to take it as a valid starting point.

jim-minter · 2017-08-08T17:16:30Z

think my only question is around location (test/testdata or examples) which can be changed easily, lgtm

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

Opened #15681 and #15682 to address the functional problems in the template. I'll work on those in parallel as we work out our ansible story.

deads2k · 2017-08-08T17:20:17Z

/test extended_conformance_install_update

openshift-merge-robot · 2017-08-08T19:31:10Z

Automatic merge from submit-queue

bparees · 2017-08-08T20:07:20Z

hack/update-generated-bindata.sh

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


is testdata really the final home for this? is this just for testing? I was under the impression it was more fundamental.

is testdata really the final home for this? is this just for testing? I was under the impression it was more fundamental.

I'm still working it out with @sdodson. The first use-case is test/initial cluster-up cases.

bparees · 2017-08-08T20:10:11Z

test/testdata/templateservicebroker/templateservicebroker-template.yaml

+objects:
+
+# to create the tsb server
+- apiVersion: extensions/v1beta1


do we have other GA product features built on top of beta apis? are we comfortable w/ the migration implications?

do we have other GA product features built on top of beta apis? are we comfortable w/ the migration implications?

I'm trying to get up with @smarterclayton about why service catalog is a daemonset. Seems wrong and @pmorie isn't sure. I'd choose a deploymentconfig.

bparees · 2017-08-08T20:38:00Z

test/testdata/templateservicebroker/templateservicebroker-template.yaml

+      spec:
+        serviceAccountName: apiserver
+        containers:
+        - name: c


surely we can name this better :)

surely we can name this better :)

If you wish.

bparees · 2017-08-08T20:42:30Z

test/testdata/templateservicebroker/templateservicebroker-template.yaml

+        containers:
+        - name: c
+          image: ${IMAGE}
+          imagePullPolicy: Never


IfNotPresent?

IfNotPresent?

once we have an image, I can switch it. For now, doing that breaks my local images.

why/how? if you have a local origin image you've built it should just use that, right?

why/how? if you have a local origin image you've built it should just use that, right?

If you use a tag of :latest, the pull policy is forced to always when running with some admission plugins. Since we now have images in latest that can work, I've updated it in #15707 for you.

I thought that pull policy was only forced if it wasn't explicitly declared otherwise. Are you telling me it will only force it to always if the value is not set to "never"? that's insane.

bparees · 2017-08-08T20:44:22Z

test/testdata/templateservicebroker/templateservicebroker-template.yaml

+  kind: ServiceAccount
+  metadata:
+    namespace: ${NAMESPACE}
+    name: apiserver


is there no ordering issue w/ creating the daemonset referencing the SA before the SA exists/has the right permissions?

is there no ordering issue w/ creating the daemonset referencing the SA before the SA exists/has the right permissions?

correct.

bparees · 2017-08-08T20:47:30Z

test/testdata/templateservicebroker/templateservicebroker-template.yaml

+    namespace: ${NAMESPACE}
+    name: apiserver
+    annotations:
+      service.alpha.openshift.io/serving-cert-secret-name: apiserver-serving-cert


relying on alpha features makes me even less happy than relying on beta features. sounds like a migration headache in the making. (unless this really is for testing purposes only, but i was under the impression this was the template cluster up and ansible will use to deploy the TSB)

Using the service serving cert signer really seems like the right thing to do here. @smarterclayton how would you like to generate our serving cert for the TSB service?

i don't disagree that it looks like the right solution for the immediate problem, but i'm trying to head off migration pain in the future so i want to make sure we have a (not horrible) path.

i don't disagree that it looks like the right solution for the immediate problem, but i'm trying to head off migration pain in the future so i want to make sure we have a (not horrible) path.

Imagine the migration path. We introduce the beta annotation. Because this API appears largely right (there's just not that many ways to do it), the secret with the exact same name is generated. A later apply places both annotations on the service, the secret doesn't change in meaning (might get regenerated) and the pods never know the difference.

add template for TSB

6cbbfa9

deads2k assigned jim-minter and bparees Aug 8, 2017

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

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

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

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

openshift-merge-robot merged commit 80dd9e6 into openshift:master Aug 8, 2017

bparees reviewed Aug 8, 2017

View reviewed changes

deads2k deleted the tsb-template branch January 24, 2018 14:31

add template for TSB #15673

add template for TSB #15673

Uh oh!

Conversation

deads2k commented Aug 8, 2017

Uh oh!

openshift-merge-robot commented Aug 8, 2017

Uh oh!

deads2k commented Aug 8, 2017

Uh oh!

deads2k commented Aug 8, 2017

Uh oh!

deads2k commented Aug 8, 2017

Uh oh!

jim-minter commented Aug 8, 2017

Uh oh!

deads2k commented Aug 8, 2017

Uh oh!

jim-minter commented Aug 8, 2017

Uh oh!

deads2k commented Aug 8, 2017

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants