Add --enable=automation-service-broker #19409

jmontleon · 2018-04-18T14:50:56Z

No description provided.

jmontleon · 2018-04-18T16:38:36Z

soltysh · 2018-04-19T08:53:44Z

pkg/oc/bootstrap/clusteradd/components/ansible-service-broker/ansible_service_broker.go

+		InstallTemplate: bootstrap.MustAsset("install/ansibleservicebroker/deploy-automation-broker-apb.yaml"),
+	}
+
+	err := component.MakeReady(


Just

return component.MakeReady(...

soltysh · 2018-04-19T08:58:15Z

install/ansibleservicebroker/deploy-automation-broker-apb.yaml

+    namespace: "${NAMESPACE}"
+
+- apiVersion: batch/v1
+  kind: Job


Are you sure this is Job, looking at the .spec this should rather be DeploymentConfig.

This container will run once and exit. It launches the broker in the ansible-service-broker namespace.

You end up with:

NAMESPACE NAME READY STATUS RESTARTS AGE ansible-service-broker ansible-service-broker-1-kldc6 1/1 Running 0 22h automation-broker-apb automation-broker-apb-8hr8r 0/1 Completed 1 22h

Doesn't a job make more sense for this?

Job is fine, what I'm saying is that the definition you have in the yaml is not a valid job. See https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/ for an example job, what you have in here is a mixture of a job and deployment.

Thank you. I misunderstood. I'll clean it up.

jmontleon · 2018-04-19T20:22:16Z

/test gcp

soltysh

This lgtm, please squash your changes and you're good to go.

jmontleon · 2018-04-25T16:47:59Z

/test extended_conformance_install

djzager · 2018-04-25T19:57:32Z

install/automationservicebroker/deploy-automation-broker-apb.yaml

+        containers:
+        - image: docker.io/automationbroker/automation-broker-apb:latest
+          name: automation-broker-apb
+          command: [ "entrypoint.sh", "provision", "-e", "broker_name=automation-service-broker", "-e", "create_broker_namespace=true" ]


you could use args: instead of command so you don't have to specify entrypoint.sh.

Is there a reason why we are going with automation-service-broker instead of automation-broker (default)?

If you are taking the $NAMESPACE as an argument you may want to add -e broker_namespace=${NAMESPACE}

ACK on first.
Second, that's what I was told to set it to to be similar to template-service-broker.
Third, as mentioned above the NAMESPACE is where the apb is running not the broker.

djzager · 2018-04-25T19:59:59Z

install/automationservicebroker/deploy-automation-broker-apb.yaml

+- apiVersion: batch/v1
+  kind: Job
+  metadata:
+    name: automation-broker-apb


You may want to specifically run this job in the ${NAMESPACE} where the automation-broker-apb service account will live.

It runs in the namespace set here:
https://github.com/openshift/origin/pull/19409/files#diff-0b8cd438a5f3db747099c64b502fe091R12
https://github.com/openshift/origin/pull/19409/files#diff-0b8cd438a5f3db747099c64b502fe091R24

You may want to specifically run this job in the ${NAMESPACE} where the automation-broker-apb service account will live.

I second this. I think it's best to be very specific.

jmontleon · 2018-04-26T12:40:00Z

/test extended_conformance_install

jmontleon · 2018-04-26T15:41:16Z

/test extended_conformance_install

jmontleon · 2018-04-27T12:43:46Z

/test extended_conformance_install

jmontleon · 2018-04-27T14:33:55Z

@soltysh sorry for the delay. The changes have been squashed and are passing again. Thanks!

soltysh

/lgtm
/approve

jmontleon · 2018-05-01T15:43:54Z

In testing I found the deadline was too short and the job would sometimes be ended before had a chance to complete. I've upped the time a bit to compensate.

jmontleon · 2018-05-01T17:21:02Z

/test gcp

jmontleon · 2018-05-01T19:06:20Z

/test extended_conformance_install

jmontleon · 2018-05-01T20:22:27Z

/test extended_conformance_install

jmontleon · 2018-05-02T15:00:36Z

/test extended_conformance_install

jmontleon · 2018-05-03T14:23:54Z

/test gcp

jmontleon · 2018-05-03T15:25:45Z

/test gcp

soltysh

/lgtm

jwmatthews · 2018-05-11T12:59:14Z

@deads2k @bparees @smarterclayton @derekwaynecarr would one of you have a few mins to review/approve this PR to help us get deployment of automation broker integrated with 'oc'?

deads2k · 2018-05-14T19:03:33Z

install/automationservicebroker/deploy-automation-broker-apb.yaml

+- description: Namespace of the project that is being deploy
+  displayname: broker client cert key
+  name: NAMESPACE
+  value: "automation-broker-apb"


We reserve openshift-***. You should use a namespace under there.

@openshift/api-review this is our first external-ish component. How shall we reserve names?

deads2k · 2018-05-14T19:05:03Z

install/automationservicebroker/deploy-automation-broker-apb.yaml

+  metadata:
+    name: automation-broker-apb
+  roleRef:
+    name: cluster-admin


Thank you for making this very clear. Do you have a link to where this was litigated before?

@sdodson do you install this with ansible? Do you grant this permission?

It looks like the current implementation creates a clusterrole. I'm not sure how closely this new work aligns with the older implementation in openshift-ansible.

https://github.com/openshift/openshift-ansible/blob/master/roles/ansible_service_broker/tasks/install.yml#L30-L90

deads2k · 2018-05-14T19:05:13Z

install/automationservicebroker/deploy-automation-broker-apb.yaml

+    name: automation-broker-apb
+    namespace: "${NAMESPACE}"
+
+- apiVersion: rbac.authorization.k8s.io/v1beta1


use v1 if you please

deads2k · 2018-05-14T19:06:06Z

install/automationservicebroker/deploy-automation-broker-apb.yaml

+    namespace: "${NAMESPACE}"
+
+- apiVersion: rbac.authorization.k8s.io/v1beta1
+  kind: ClusterRoleBinding


This should be split into install-rbac.yaml since bindings are reconciled. The rest should be in install.yaml for consistency.

deads2k · 2018-05-14T19:06:33Z

install/automationservicebroker/deploy-automation-broker-apb.yaml

+        containers:
+        - image: docker.io/automationbroker/automation-broker-apb:latest
+          name: automation-broker-apb
+          args: [ "provision", "-e", "broker_name=automation-service-broker", "-e", "create_broker_namespace=true" ]


"create_broker_namespace=true" looks weird. What does that do?

deads2k · 2018-05-14T19:08:23Z

pkg/oc/bootstrap/clusteradd/components/automation-service-broker/automation_service_broker.go

+	}
+	glog.V(2).Infof("instantiating automation service broker template with parameters %v", params)
+
+	component := componentinstall.Template{


I think we're going to require the WaitCondition value.

@mfojtik probably worth updating to enforce. I can't think of a reason not to.

deads2k · 2018-05-14T19:10:54Z

I'm not familiar with what it does, but it appears to fit nicely. On the authorship side, how was it?

@jmontleon @jwmatthews To make the boundary clear, we'll keep it compiling, but if there are functional breaks over time you'll be responsible for your component.

jwmatthews · 2018-05-15T16:18:43Z

@deads2k thank you for the review.

As to owning functional breakage, sounds good to me, our team will own the functional issues that pop up.

jmontleon · 2018-05-15T17:27:58Z

@deads2k Thank you for the review. I have:

moved the namespace to openshift-automation-service-broker
used v1 for the clusterrolebinding resource
renamed the template to install.yaml
split the clusterrolebinding into a separate install-rbac.yaml template
removed "create_broker_namespace=true" as we don't actually need it (it will create the namespace for the broker if it doesn't exist)
add a wait condition for the job completion.

I think this addresses all your comments except possibly, "Do you have a link to where this was litigated before?" Do we need to follow up with someone to permit the clusterrolebinding to be created? The job actually runs a containerized ansible role[1] that we created install the automation broker on an openshift cluster. The broker itself does require a clusterrolebinding [2], and it's my understanding that we need this permission in order to create this and (possibly other resources) using the container.

We also fully expected and understand that we will be on the hook to maintain this going forward.

Re: authorship I found it pretty straight forward to look at the existing options (service-catalog, template-service-broker, etc.) and work from there. I think I had it at least functioning within a day.

deads2k · 2018-05-15T17:29:41Z

Re: authorship I found it pretty straight forward to look at the existing options (service-catalog, template-service-broker, etc.) and work from there. I think I had it at least functioning within a day.

https://github.com/automationbroker/automation-broker-apb
https://github.com/automationbroker/automation-broker-apb/blob/master/templates/broker-auth.clusterrolebinding.yaml

A cluster up record! :)

deads2k · 2018-05-15T17:35:56Z

install/automationservicebroker/install-rbac.yaml

+  metadata:
+    name: automation-broker-apb
+  roleRef:
+    name: cluster-admin


Please make these permissions match https://github.com/openshift/openshift-ansible/blob/master/roles/ansible_service_broker/tasks/install.yml#L30-L90 .

hmmm... perhaps I misunderstood what's happening. This is basically an installer pod?

deads2k · 2018-05-15T18:11:28Z

Alright, I cleared up my confusion with @jwmatthews . This gets cluster-admin because the job is actually installing the components. A few details to note that are not bugs with cluster-up

Components can be installed in any order. There is no concept of a "prereq", so you may be called in any order. Including in parallel with or before the service catalog. Going forward, we're expecting operators to manage the coming and going of prereqs.
Components must be idempotent. We are likely to call you multiple times.
Components are likely to be asked for a "remove" at some point. It isn't a requirement today.

This looks like a good starting point.

/lgtm

openshift-ci-robot · 2018-05-15T18:11:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jmontleon, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jmontleon · 2018-05-15T18:34:42Z

/test cmd

jmontleon · 2018-05-15T18:52:29Z

@deads2k is remove possible now? We have a deprovision playbook in the container that will remove the broker and other resources it creates as well so it might be pretty trivial for us to implement it today if so.

jmontleon · 2018-05-15T19:03:56Z

/test unit

deads2k · 2018-05-15T21:30:46Z

@deads2k is remove possible now? We have a deprovision playbook in the container that will remove the broker and other resources it creates as well so it might be pretty trivial for us to implement it today if so.

It won't be possible in v3.10. It may be possible in 3.11. I mentioned it so that you wouldn't be surprised if we contacted all component owners and asked them for one.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 18, 2018

openshift-ci-robot requested review from coreydaley and soltysh April 18, 2018 14:51

openshift-ci-robot assigned derekwaynecarr Apr 18, 2018

soltysh suggested changes Apr 19, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2018

jmontleon changed the title ~~Add --enable=ansible-service-broker~~ Add --enable=automation-service-broker Apr 19, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2018

soltysh approved these changes Apr 24, 2018

View reviewed changes

jmontleon mentioned this pull request Apr 25, 2018

Use --enable=automation-service-broker for v3.10 fusor/catasb#242

Merged

djzager reviewed Apr 25, 2018

View reviewed changes

openshift-ci-robot assigned soltysh Apr 30, 2018

soltysh approved these changes Apr 30, 2018

View reviewed changes

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Apr 30, 2018

soltysh approved these changes May 8, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2018

deads2k reviewed May 14, 2018

View reviewed changes

add --enable=automation-service-broker

1b656c1

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 15, 2018

deads2k reviewed May 15, 2018

View reviewed changes

openshift-ci-robot assigned deads2k May 15, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2018

openshift-merge-robot merged commit 9dae233 into openshift:master May 16, 2018

Add --enable=automation-service-broker #19409

Add --enable=automation-service-broker #19409

Conversation

jmontleon commented Apr 18, 2018

jmontleon commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmontleon commented Apr 19, 2018

soltysh left a comment

Choose a reason for hiding this comment

jmontleon commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmontleon commented Apr 26, 2018

jmontleon commented Apr 26, 2018

jmontleon commented Apr 27, 2018

jmontleon commented Apr 27, 2018

soltysh left a comment

Choose a reason for hiding this comment

jmontleon commented May 1, 2018

jmontleon commented May 1, 2018

jmontleon commented May 1, 2018

jmontleon commented May 1, 2018

jmontleon commented May 2, 2018

jmontleon commented May 3, 2018

jmontleon commented May 3, 2018

soltysh left a comment

Choose a reason for hiding this comment

jwmatthews commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 14, 2018

jwmatthews commented May 15, 2018

jmontleon commented May 15, 2018

deads2k commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 15, 2018

openshift-ci-robot commented May 15, 2018

jmontleon commented May 15, 2018

jmontleon commented May 15, 2018

jmontleon commented May 15, 2018

deads2k commented May 15, 2018