Spec: Kubernetes and COE agnostic support #329

rthallisey · 2017-08-02T19:19:58Z

Spec outlining the work for supporting Kubernetes.

jmrodri

One thing that would be nice is if we could identify how we could do this over time. Can we spread it over a few PRs or sprints? Some minor change requests.

jmrodri · 2017-08-04T00:43:01Z

docs/specs/add-kubernetes-support.md

+
+## Introduction
+The ansible-service-broker and all of the tooling around it
+(apb-examples, catasb, adn ansible-playbook-bundles) specifically target


TYPO: adn -> and

jmrodri · 2017-08-04T00:55:47Z

docs/specs/add-kubernetes-support.md

+should be cluster agnostic.
+
+## Problem Description
+Issues having been coming up during testing because the broker does not target


Issues have been coming up...

jmrodri · 2017-08-04T01:08:58Z

docs/specs/add-kubernetes-support.md

+
+### Hierarchy Diagram
+This diagram represents if the current state of code were split into two
+files.


This table represents if the code were split into two files.

jmrodri · 2017-08-04T01:13:30Z

docs/specs/add-kubernetes-support.md

+The main issue with packaging is that folks using Kubernetes may only want the
+Kubernetes client. The package ```origin-clients``` installs both the OpenShift
+and Kubernetes clients.  If this the container size isn't an issue, then there
+may not be a problem using ```origin-clients```.


There seems to be two parts to this. 1) the code being compiled into the broker for both 2) the branches will require specific clients i.e. openshift.go would require oc client. We could put in code to only require the specific oc client IF openshift.go is being invoked.

There was a blog post Shawn shared with me about plugins in go programs. I haven't read it yet but if there was a way to make these pieces more "pluggable" that could help.

The plugin package is what @jmrodri is referring to.

Currently plugins only work on Linux.

jmrodri · 2017-08-04T01:18:36Z

docs/specs/add-kubernetes-support.md

+
+### apb-examples
+ - Add support for Kubernetes to ```oc-login.sh``` script. Also, it should
+   be renamed.


... it should be renamed to login.sh

jmrodri · 2017-08-04T01:19:08Z

docs/specs/add-kubernetes-support.md

+ - Create a Kubernetes solution for ClusterRoleBindings, ClusterRoles, and
+   Projects using the clients.
+ - Convert all the client commands to API [calls](https://github.com/openshift/ansible-service-broker/search?p=1&q=%22oc%22&type=&utf8=%E2%9C%93).
+ - Convert any broker tools to target either OpenShift or Kubernetes.


Can we get a first list of the tools that need to change?

shawn-hurley

I also would like to see a cluster.Client interface or something similar.

shawn-hurley · 2017-08-04T12:20:08Z

docs/specs/add-kubernetes-support.md

+The main issue with packaging is that folks using Kubernetes may only want the
+Kubernetes client. The package ```origin-clients``` installs both the OpenShift
+and Kubernetes clients.  If this the container size isn't an issue, then there
+may not be a problem using ```origin-clients```.


The plugin package is what @jmrodri is referring to.

shawn-hurley · 2017-08-04T12:29:03Z

docs/specs/add-kubernetes-support.md

+
+## Work Items
+### Ansible-service-broker
+ - Split out all Cluster logic into a ```pkg/cluster/openshift.go``` and


Do we want two clients to share the same package namespace? If this is too in weeds, please just this, but I am little confused on how separate the proposed clients are.
Options:
We have a kubernetes client that does 80% of the stuff. Then the openshift client has an embedded kubernetes type?
Or are they going to be completely separate objects that each contain their own logic for each action?

Do we want two clients to share the same package namespace

Yes.

are they going to be completely separate objects that each contain their own logic for each action?

What I'm thinking we do is create an abstraction where the broker has only once code path for things like serviceaccounts, rolebindings, ect... The broker makes a call to the client which handles whether the action needs run with either the openshift or kubernetes client/API. Kubernetes and openshift will be in different files in the same pkg namespace with different logic for the ~20% of overlapping calls. The result is: If I create a rolebinding and I'm on openshift, the client object will know we need to use the openshift api. If I create a pod in openshift, the client object will know to use the kubernetes api.

Good questions @shawn-hurley. Will add more detail to the doc.

shawn-hurley · 2017-08-07T13:00:08Z

docs/specs/add-kubernetes-support.md

+
+The Cluster object will recieve the request and call the correct API.
+```diff
+ func Create(c interface{}, ns string) {


Is there some method they both share that we could make an interface for? That would allow the compiler to make some guarantees.

Also, passing in the type of object, (either k8s or oc pod) to determine what to do, still requires logic in the caller to determine which type of object to use right?

I'm using c interface{} to accept any API type as a parameter so I don't think a normal interface will work. Another way I could do this is a struct to hold variables for all the API types.

Why don't we create methods to create each resource?

I guess I am wondering why we are making it so generic, I think that in this case, the caller would probably just prefer to create a pod and not care about the pod object itself or the implementation of how that pod is created.

I guess I am wondering why we are making it so generic, I think that in this case, the caller would
probably just prefer to create a pod and not care about the pod object itself or the implementation
of how that pod is created.

Create is very generic. I can make it more specific with Create<resource>. I agree, being more specific will be easier.

By being specific, we could do a few things. The caller can create a pod object and pass it to CreatePod or the caller can pass information to Create<resource>. I like the idea of abstracting the whole thing.

I am arguing that we take the values that we need to create a pod and create a function that will take those values and both create the pod struct as well as call the method to whichever client.

The client is responsible for dealing with

kube/oc specific errors making them client package errors

kube/oc specific structs making the callers unaware of anything specific about the cluster we are running in

Making a public(to our broker) API that will allow the broker to create the resources it needs. This is not a generic client that is being imported, it is very specific for just our broker IMO.

I updated it see the latest spec.

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

docs/specs/add-kubernetes-support.md

+Kubernetes client. The package ```origin-clients``` installs both the OpenShift
+and Kubernetes clients.  If this the container size isn't an issue, then there
+may not be a problem using ```origin-clients```.
+


How do we annotate that an APB requires OpenShift and can not run on Kubernetes. Say it creates a Route or uses a DeploymentConfig?

I'm assuming any APB that runs on Kubernetes will be able to run on OpenShift.
Yet if an APB truly requires OpenShift (Route, DeploymentConfig, etc) it must have OpenShift and cannot run on pure k8s.

Would we want to allow an APB to downgrade itself based on environment?
i.e., it's written to deploy to OpenShift if available and use a DeploymentConfig, but if on K8S it follows a different path in the APB?

Good questions @jwmatthews.

How do we annotate that an APB requires OpenShift and can not run on Kubernetes. Say it
creates a Route or uses a DeploymentConfig?

I think this is up to the user and we need to document 'Creating an APB' very well so it's clear where openshift and kubernetes differ. Looking at this as an APB writer, if I'm using Kubernetes, I'm going to write my apb with the resources I know Kubernetes uses. Ditto OpenShift. The case I'm concerned about is someone who didn't write the APB trying to use it with the wrong cluster. But I think we make it clear by having maybe a separate playbook directory, git repo, or top level directory for Kubernetes and OpenShift APBs.

I'm assuming any APB that runs on Kubernetes will be able to run on OpenShift.

Not 100% sure. What I'm aware of that could be problematic is the ClusterRoleBinding resource. But, that resource is mapped to the OpenShift security policy where as in Kubernetes it's Rbac. So I think it will work, but function different.

Either way, I think we should discourage this behaviour and treat the playbooks and the clusters as completely independent entities.

Would we want to allow an APB to downgrade itself based on environment?

No. Let's keep them separate. I think to have success here requires clean separation of the playbooks so we communicate to the user that didn't write the playbook what it's used for.

But, that resource is mapped to the OpenShift security policy where as in Kubernetes it's Rbac. So I think it will work, but function different.

From my testing, RBAC entities are entirely ignored by OpenShift. It's very deceptive, because I was able to create RBAC *RoleBindings that appeared to have created absolutely fine (because they did), so my expectation was that they were, well...doing what role bindings do, but they were not from an openshift perspective. I am very hesitant to say this will work.

Either way, I think we should discourage this behaviour and treat the playbooks and the clusters as completely independent entities.

+1. I think fundamentally we should encourage good design practice around this. Organize logic in such a way where shared logic can be passed around with things like roles, and encapsulate pieces that are specific to certain clusters.

WRT annotation, I would like to see something in the apb spec that tells the broker what COE a given APB supports. I think the broker should validate early, know what cluster it's able to deploy to and what APBs are possible to be deployed to that cluster. That way the broker can say if !clusterSupports(apb) { handleUnsupported() } else { continue }

WRT annotation, I would like to see something in the apb spec that tells the broker what
COE a given APB supports. I think the broker should validate early, know what cluster it's
able to deploy to and what APBs are possible to be deployed to that cluster. That way the
broker can say if !clusterSupports(apb) { handleUnsupported() } else { continue }

@eriknelson good idea.

What if we create a field in apb.yaml for Cluster: (OpenShift/Kubernetes).
From the broker, we read the specs and identify which specs will work on the
current cluster and ignore the specs that don't work.

jwmatthews · 2017-08-07T20:21:03Z

docs/specs/add-kubernetes-support.md

+
+### Phase One
+Infrastructure setup - Make sure there's a repeatable infrastracture setup
+and begin transitioning broker code.


+1 Want to be sure team has a shared common way to run a script and get a working environment to develop/debug/test in.

jwmatthews · 2017-08-07T20:25:05Z

docs/specs/add-kubernetes-support.md

+The community has two tools for CI: Jenkins and Travis. When adding
+Kubernetes support to the broker, the upstream Travis CI job should test the
+broker and catalog on top of the upstream COE, Kubernetes.  Jenkins will become
+the downstream CI testing on OpenShift.


Do we need to have Travis only test Kubernetes support?

If we could also do a sanity check with oc cluster up like we do now that'd be good feedback to have with a PR.

Essentially have the gate checking both K8S and OCP behavior.

Do we need to have Travis only test Kubernetes support?

We can have travis test both.

jwmatthews · 2017-08-07T20:27:36Z

docs/specs/add-kubernetes-support.md

+      if OpenShift {
+         OCcli.CoreV1().ClusterRoleBinding(ns).Create(roleBindingM)
+      } elif Kubernetes {
+         k8scli.CoreV1().ClusterRoleBinding(ns).Create(roleBindingM)


Is there a mechanism in Go where we can have the OCP Object reuse the K8S implementation and just override/implement the deltas?

Opposed to needing to redeclare, if OCP/if K8S workflows

Assuming OCP workflow can reuse k8scli except for those cases where there are deltas in behavior.

Is there a mechanism in Go where we can have the OCP Object reuse the K8s implementation
and just override/implement the deltas?

From the broker's perspective, that's essentially what I'm doing. The broker calls Cluster.CreatePod() and has no idea how the pod is created, but gets back the pod information. The cluster pkg handles the abstraction and figures out what cli/api to call.

Opposed to needing to redeclare, if OCP/if K8S workflows

I agree that the way we did it before wasn't good, but these checks aren't 'branching' the broker code like before. The extend of the 'branch' only encompasses one line, the client/API call. And the broker won't know this is happening because it's completely abstracted. The design is similar to a plugin, where the plugin code is abstracted away from the core broker code and does whatever it wants under the covers. Also different from before, OpenShift will come from the config file instead broker core code and we'll verify it when we start the broker.

Yes, albeit a bit differently than traditional virtual methods. Go chooses to use composition over inheritance. On a high level, it's all about sharing behavior and delegation of specifics to components that know better (also providing encapsulation). We've organized our registry code and adapters in a similar way, so there is precedence, and I think we can leverage a similar pattern.

Spitballing, we can have a parent COE object that exposes the set of methods the broker needs to run APB actions. I would take the RunPod abstraction even further to RunApbAction. The fact that APBs are actually run as pods is an implementation detail; also, it provides optionality to run an APB action in a completely different way, (in an environment where the concept of pods don't even exist). Broker operates here on a higher level as well, calling into coe.RunApbAction(ActionProvision, params).

Broker can build a coe runner via factory method, which is responsible for constructing a coe runner based on the current, concrete cluster the broker is configured to target: runner := NewCOERunner(ClusterTypeKubernetes). Inside this, we can construct a parent COE with a clusterDelegate ClusterDelegate field, where ClusterDelegate is an interface sitting in front of cluster specifics. We can implement a ClusterDelegate for kubernetes and for openshift, and this is where the specialized behavior belongs.

Pseudocode:

type ClusterDelegate interface { SetupPermissions() } func (r *COERunner) RunApbAction(...) { r.sharedBehavior(...) r.clusterDelegate.SetupPermissions() }

In this case, we know we need to SetupPermissions for all cluster types, they just need to be done in a different way, which the delegates define themselves. For similar cases, think about what's actually getting achieved where differences arise, abstract and declare that behavior (SetupPermissions) on the ClusterDelegate, and then allow the specific implementations to define what it actually means to SetupPermissions.

More food for thought, this very much reminds me of PIMPL from the c++ world, or the Bridge Pattern:
http://en.cppreference.com/w/cpp/language/pimpl
https://en.wikipedia.org/wiki/Bridge_pattern

jmrodri · 2017-08-08T17:30:55Z

@rthallisey @shawn-hurley @eriknelson I setup a meeting for 11am tomorrow to discuss this. I found it worked well with registries and filtering.

eriknelson

I think this is pretty much there, I would like to have a design conversation and strategy for how we're going to be organizing the logic. We did this for multiple registries and it worked out really well.

rthallisey · 2017-08-08T18:01:31Z

@jmrodri Sounds good. Can you post something on the ML about the discussion you scheduled?

eriknelson · 2017-08-09T17:20:07Z

jwmatthews · 2017-08-09T17:37:04Z

docs/specs/add-kubernetes-support.md

+
+```diff
+bindable: false
+ cluster: kubernetes


We will need to track the version of the cluster type as well in the APB
Assuming some APBs may only work with k8s 1.6, others 1.7+ etc

jmrodri · 2017-08-09T18:03:46Z

docs/specs/add-kubernetes-support.md

+### New APB Level Directory
+Add a new directory layer into each APB.
+
+mediawiki123-apb/


can you surround these directory listings in ```

eriknelson · 2017-08-09T18:18:18Z

Example of the runner pattern, important types introduced are the InputBundle, COERunner, COEDelegate, and the concrete delegate types.

https://gist.github.com/eriknelson/4c2c29ea107e6e197fb03ea7abe19a8d

Runnable: https://play.golang.org/p/Rv1ktceNEj

rthallisey · 2017-08-09T20:05:12Z

I think this is good to go since this covers the major design topics. If anything is left to add I think it will be minor and we can deal with it on the fly.

eriknelson

See request for Cluster -> Delegate.

eriknelson · 2017-08-09T20:35:30Z

docs/specs/add-kubernetes-support.md

+     DestroySandbox()
+}
+
+struct Cluster {


One question since this was discussed as a Delegate: Do we care about runtimes that are not kubernetes or openshift? For example: vanilla docker? IMO, the answer is "No, but we should design for the possibility" since we've already had interest expressed from outside the team. With that in mind, are these other runtimes always going to be "Clusters"? I don't think it would be appropriate to call docker that.

My request would be to keep this as the runner's Delegate, curious how other folks feel about that. Otherwise, looks great. Thanks for writing this all up @rthallisey.

@eriknelson that's true, Docker would not be a cluster. Something like Delegate wfm or maybe Runtime?

rthallisey force-pushed the multiple-coe-spec branch 7 times, most recently from 466eb30 to 36d229d Compare August 2, 2017 19:46

jmrodri requested review from jmrodri and eriknelson August 4, 2017 00:42

jmrodri suggested changes Aug 4, 2017

View reviewed changes

shawn-hurley reviewed Aug 4, 2017

View reviewed changes

rthallisey force-pushed the multiple-coe-spec branch from a49c641 to 2719e9c Compare August 4, 2017 22:35

jmrodri approved these changes Aug 5, 2017

View reviewed changes

rthallisey changed the title ~~Spec: Kubernetes and COE agnostic support~~ [Under Review] Spec: Kubernetes and COE agnostic support Aug 7, 2017

rthallisey requested a review from jwmatthews August 7, 2017 12:41

shawn-hurley reviewed Aug 7, 2017

View reviewed changes

rthallisey force-pushed the multiple-coe-spec branch from 2719e9c to c780fc3 Compare August 7, 2017 14:51

shawn-hurley approved these changes Aug 7, 2017

View reviewed changes

jwmatthews reviewed Aug 7, 2017

View reviewed changes

eriknelson suggested changes Aug 8, 2017

View reviewed changes

rthallisey force-pushed the multiple-coe-spec branch from c780fc3 to 53bff64 Compare August 8, 2017 19:40

jwmatthews reviewed Aug 9, 2017

View reviewed changes

jmrodri reviewed Aug 9, 2017

View reviewed changes

Ryan Hallisey added 3 commits August 9, 2017 15:03

Spec: Kubernetes and COE agnostic support

ab2f6f7

Add a lot more details and a phased plan

23134a7

Expand APB plan

1114052

rthallisey force-pushed the multiple-coe-spec branch from 53bff64 to e410df5 Compare August 9, 2017 20:03

Update after spec meeting

e410df5

rthallisey changed the title ~~[Under Review] Spec: Kubernetes and COE agnostic support~~ Spec: Kubernetes and COE agnostic support Aug 9, 2017

eriknelson approved these changes Aug 9, 2017

View reviewed changes

Change cluster to runtime in most places

acda8dc

eriknelson merged commit 0d59553 into openshift:master Aug 10, 2017

Spec: Kubernetes and COE agnostic support #329

Spec: Kubernetes and COE agnostic support #329

Conversation

rthallisey commented Aug 2, 2017

jmrodri left a 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

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

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley Aug 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley Aug 7, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey Aug 8, 2017 • edited

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

jmrodri commented Aug 8, 2017

eriknelson left a comment • edited

Choose a reason for hiding this comment

rthallisey commented Aug 8, 2017

eriknelson commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson commented Aug 9, 2017

rthallisey commented Aug 9, 2017

eriknelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley Aug 4, 2017 •

edited

shawn-hurley Aug 7, 2017 •

edited

rthallisey Aug 8, 2017 •

edited

eriknelson left a comment •

edited