Broker client dao #795

shawn-hurley · 2018-02-26T15:47:31Z

Describe what this PR does and why we need it:
Implements Broker client dao.
Changes proposed in this pull request

Adds a config value for CRD dao
Adds implementation for conversion and dao from CRD to apb package structures

Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on
~~#775~~
~~#794~~

eriknelson · 2018-02-28T16:30:55Z

@shawn-hurley merged both blocker PRs, so this should be good for a rebase.

jmrodri

ACK

jmrodri · 2018-02-28T16:45:46Z

pkg/broker/broker.go

@@ -152,7 +151,7 @@ func NewAnsibleBroker(dao dao.Dao,
 func (a AnsibleBroker) GetServiceInstance(instanceUUID uuid.UUID) (apb.ServiceInstance, error) {
 	instance, err := a.dao.GetServiceInstance(instanceUUID.String())
 	if err != nil {
-		if client.IsKeyNotFound(err) {
+		if a.dao.IsNotFoundError(err) {


+1 makes sense.

jmrodri · 2018-02-28T16:47:20Z

pkg/app/app.go

+		etcdClient, err := clients.Etcd()
+		if err != nil {
+			return err
+		}


👍 took a while to figure out what really changed here, but it was the if statement that caused the indentation of the block of code. looks good.

jmrodri · 2018-02-28T16:49:42Z

pkg/app/app.go

-	if err != nil {
-		return err
-	}
+	if strings.ToLower(c.GetString("dao.type")) != "crd" {


What happens if it isn't crd? What is the other valid value?

All other values will mean we use etcd. This is so every old config should still work as expected with etcd as it's DAO.

jmrodri · 2018-02-28T16:53:34Z

templates/k8s-ansible-service-broker.yaml.j2

+            bindingIds:
+              type: array
+              items:
+                type: string


jmrodri · 2018-02-28T16:54:20Z

templates/deploy-ansible-service-broker.template.yaml

+  - apiGroups: ["automationbroker.io"]
+    attributeRestrictions: null
+    resources: ["bundles", "jobstates", "servicebindings", "serviceinstances"]
+    verbs: ["*"]


jmrodri · 2018-02-28T16:54:34Z

templates/deploy-ansible-service-broker.template.yaml

@@ -402,6 +406,242 @@ objects:
      ${{BROKER_AUTH}}
    caBundle: ${BROKER_CA_CERT}

+# CRDs for the broker.
+- apiVersion: apiextensions.k8s.io/v1beta1


eriknelson

Minor spelling nit and a few questions. We also need to file an issue over the mix between automation broker and ansible service broker. I also think we should add to the developer documentation some of the basics about the different storage backends, the basics of how to tweak CRDS (like if I want to add or remove a field), and how to get the code generated and into the broker's client go.

Overall looks great though.

We're also going to need to update CI to turn on the CRD backend, but only for PRs coming into master, is that possible? I'm running through regression tests with this now.

eriknelson · 2018-02-28T19:35:40Z

templates/deploy-ansible-service-broker.template.yaml

+    name: bundles.automationbroker.io
+  spec:
+    group: automationbroker.io
+    version: v1


Is there any reason to consider these v1alpha or v1beta?

it is not possible to have more than 1 version for an API in CRDs so it is not like we could bump up and start using v1 and still have v1beta1 around. I think because of that, just make it easy and call them v1.

eriknelson · 2018-02-28T19:36:39Z

pkg/dao/dao.go

 	etcd "github.com/openshift/ansible-service-broker/pkg/dao/etcd"
 	logutil "github.com/openshift/ansible-service-broker/pkg/util/logging"
 )

 var log = logutil.NewLog()

 // NewDao - Create a new Dao object
-func NewDao() (Dao, error) {
+func NewDao(c *config.Config) (Dao, error) {


This is great. 👍

eriknelson · 2018-02-28T19:39:13Z

pkg/dao/crd/dao.go

+import (
+	"fmt"
+
+	automationbrokerv1 "github.com/automationbroker/broker-client-go/client/clientset/versioned/typed/automationbroker.io/v1"


I'm concerned that we're starting to mix automationbroker and ansible-service-broker language now in the codebase. automationbroker is the correct choice IMO, but I think we need to get this issue captured and start to move forward with the rebrand.

eriknelson · 2018-02-28T19:41:23Z

pkg/dao/crd/dao.go

+		log.Errorf("unable to get bundle from k8s api - %v", err)
+		return nil, err
+	}
+	return bundleToSpec(s.Spec, s.GetName()), nil


So it's expected that the dao layer will need to transform between the ASB types and their CRD counterparts for most of these, so the rest of the codebase is getting objects they expect?

Yes, that is the plan and what I was hoping to do here.

eriknelson · 2018-02-28T19:42:34Z

pkg/dao/crd/dao.go

+	log.Debugf("set service instance: %v", id)
+	spec := convertServiceInstanceToCRD(serviceInstance)
+	if si, err := d.client.ServiceInstances(d.namespace).Get(id, metav1.GetOptions{}); err == nil {
+		log.Debugf("updateing service instance: %v", id)


updateing -> updating

eriknelson · 2018-02-28T19:46:11Z

pkg/dao/crd/dao.go

+	j := convertJobStateToCRD(&state)
+	if js, err := d.client.JobStates(d.namespace).Get(state.Token, metav1.GetOptions{}); err == nil {
+		js.Spec = j
+		js.ObjectMeta.Labels[jobStateLabel] = fmt.Sprintf("%v", convertStateToCRD(state.State))


Why are we setting the labels here? Selector filtering? That's actually pretty slick.

eriknelson · 2018-02-28T19:49:27Z

pkg/dao/etcd/dao.go

@@ -333,6 +333,11 @@ func (d *Dao) GetStateByKey(key string) (apb.JobState, error) {
 	return state, nil
 }

+// IsNotFoundError - Will determine if an error is a key is not found error.
+func (d *Dao) IsNotFoundError(err error) bool {


Nice this got pushed into the dao.

shawn-hurley · 2018-02-28T21:35:03Z

@eriknelson as far as testing against CRDs on master branch. I am slightly worried about switching over to CRDs right now. Mostly, this has worked in my testing of the actions but I am gun shy. If someone else's tests and thinks it is good to go, then I say we change the default template to use crds.

maleck13 · 2018-02-27T09:04:22Z

pkg/dao/crd/conversion.go

+	b, err := json.Marshal(spec.Metadata)
+	if err != nil {
+		log.Errorf("unable to marshal the metadata for spec to a json byte array - %v", err)
+		return v1.BundleSpec{}


this makes me a bit nervous that instead of failing on an error you may save just an empty BundleSpec without realizing it?

maleck13 · 2018-02-27T09:11:03Z

pkg/dao/crd/dao.go

+// BatchDeleteSpecs - set specs based on SpecManifest in the kvp API.
+func (d *Dao) BatchDeleteSpecs(specs []*apb.Spec) error {
+	for _, spec := range specs {
+		err := d.DeleteSpec(spec.ID)


Should we perhaps push errors into an array here to allow for other specs to delete. Then check the len of the array and return a new error with any errors that occurred. Or perhaps consider using something like multi error https://github.com/hashicorp/go-multierror

maleck13 · 2018-02-27T09:13:48Z

pkg/dao/crd/dao.go

+		Spec: spec,
+	}
+	_, err := d.client.ServiceInstances(d.namespace).Create(&s)
+	if err != nil {


this is really just a nit or point of discussion but should we consider just returning the error here and allowing the caller decide what to do with it, rather than logging it here, as what can happen is it gets logged here, then the caller logs it, then the callers caller logs it etc causing very noisy logs.
Some places in this file we do this and others we log the error before returning.

eriknelson

Sorry @shawn-hurley I ack'd a little early, I just want to test it as well. Working on that tonight.

eriknelson · 2018-03-01T01:22:34Z

I've been banging on this for the last hour, make ci is good, I ran through a number of scenarios I know were potential trouble cases, it's handling them fine. I can also read the resources with oc get, which is actually making this much more friendly for broker developers debugging compared to raw etcd. Big +1, nice work @shawn-hurley.

We should start to think about when we want this to graduate to the default storage layer, and make sure when that happens, we've updated our various supported deployment efforts.

jmrodri · 2018-03-01T02:18:52Z

@shawn-hurley Tested with my crash simulation patch (i.e. random adapter failing). I see that the bundles all get deleted and slowly populate as they get parsed. The final state was right, not sure if this is just being picky or not.

eriknelson · 2018-03-01T02:20:38Z

I have scenarios I run knowing they were existing bugs. We should get these captured in make ci somehow so they're always checked.

…

On Feb 28, 2018 9:18 PM, "Jesus Rodriguez" ***@***.***> wrote: @shawn-hurley <https://github.com/shawn-hurley> Tested with my crash simulation patch (i.e. random adapter failing). I see that the bundles all get deleted and slowly populate as they get parsed. The final state was right, not sure if this is just being picky or not. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#795 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZFNfEsoclsal66S17kvx520jjKCgg-ks5tZ1qNgaJpZM4STdBj> .

maleck13

These changes look great!

* initial commit * adding DAO implementation for CRDs * adding CRDs to deploy template * adding CRDs to k8s templates. * working CRD dao implementation. * Implement error checking for IsNotFound error int he dao impl * implement errors returns for conversion methods

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 26, 2018

shawn-hurley added the 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 label Feb 26, 2018

shawn-hurley force-pushed the broker-client-dao branch 2 times, most recently from 989c992 to 7193d52 Compare February 28, 2018 14:35

shawn-hurley changed the title ~~[WIP] - Broker client dao~~ Broker client dao Feb 28, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2018

shawn-hurley added needs-upstream needs-review labels Feb 28, 2018

shawn-hurley added 7 commits February 28, 2018 11:35

initial commit

e6509fd

adding DAO implementation for CRDs

96850bc

adding CRDs to deploy template

edcf2af

adding CRDs to k8s templates.

43f1786

working CRD dao implementation.

9865293

fixup

72a4e4f

Implement error checking for IsNotFound error int he dao impl

c979e27

shawn-hurley force-pushed the broker-client-dao branch from 7193d52 to c979e27 Compare February 28, 2018 16:40

remove testing log statements

4b24837

jmrodri requested review from jmrodri and eriknelson February 28, 2018 16:42

jmrodri approved these changes Feb 28, 2018

View reviewed changes

shawn-hurley removed the needs-upstream label Feb 28, 2018

eriknelson suggested changes Feb 28, 2018

View reviewed changes

maleck13 suggested changes Feb 28, 2018

View reviewed changes

eriknelson approved these changes Feb 28, 2018

View reviewed changes

eriknelson suggested changes Mar 1, 2018

View reviewed changes

eriknelson approved these changes Mar 1, 2018

View reviewed changes

implement errors returns for conversion methods

a6d3bf4

maleck13 approved these changes Mar 1, 2018

View reviewed changes

shawn-hurley merged commit 6b5cf4b into openshift:master Mar 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broker client dao #795

Broker client dao #795

shawn-hurley commented Feb 26, 2018 •

edited

Loading

eriknelson commented Feb 28, 2018

jmrodri left a comment

jmrodri Feb 28, 2018

jmrodri Feb 28, 2018

jmrodri Feb 28, 2018

shawn-hurley Feb 28, 2018

jmrodri Feb 28, 2018

jmrodri Feb 28, 2018

jmrodri Feb 28, 2018

eriknelson left a comment

eriknelson Feb 28, 2018

shawn-hurley Feb 28, 2018

eriknelson Feb 28, 2018

eriknelson Feb 28, 2018

eriknelson Feb 28, 2018

shawn-hurley Feb 28, 2018

eriknelson Feb 28, 2018

eriknelson Feb 28, 2018

eriknelson Feb 28, 2018

shawn-hurley commented Feb 28, 2018

maleck13 Feb 27, 2018 •

edited

Loading

maleck13 Feb 27, 2018

maleck13 Feb 27, 2018

eriknelson left a comment

eriknelson commented Mar 1, 2018 •

edited

Loading

jmrodri commented Mar 1, 2018

eriknelson commented Mar 1, 2018 via email

maleck13 left a comment

Broker client dao #795

Broker client dao #795

Conversation

shawn-hurley commented Feb 26, 2018 • edited Loading

eriknelson commented Feb 28, 2018

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

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

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 commented Feb 28, 2018

maleck13 Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson left a comment

Choose a reason for hiding this comment

eriknelson commented Mar 1, 2018 • edited Loading

jmrodri commented Mar 1, 2018

eriknelson commented Mar 1, 2018 via email

maleck13 left a comment

Choose a reason for hiding this comment

shawn-hurley commented Feb 26, 2018 •

edited

Loading

maleck13 Feb 27, 2018 •

edited

Loading

eriknelson commented Mar 1, 2018 •

edited

Loading