Breakup all the Broker Clients into a clients pkg #222

rthallisey · 2017-06-22T19:32:09Z

Refactor the broker clients into their own package.

Right now, we have clients scattered around in different packages. This is causing the clients to be initialized on each action (bind, unbind, etc...) as well as other places because the clients aren't available for use.

Let's initialize the clients one time and distribute those clients to the places that need it. This patch set creates a global struct 'Clients' that gets initialized by app.go before the broker object is created. Any function that runs after the broker object creation, will have access to all the clients.

Fixes: #213

rthallisey · 2017-06-22T19:39:22Z

Testing: Run the normal broker tests.

eriknelson

I'm going to try to apply my review comments to the PR that originally introduced the change since there are what look like 3 different ones. Will try to keep higher level discussion to this one.

I like the clients pkg a lot, decouples things in a way that makes changes a lot less brittle (client init code isn't just thrown about where it's used; it's got a home). Also going to make it real easy and natural to add an Origin() client.

[this is where you use clients] - [this is where you get clients (and how we create them)]

I think we should push any remaining client code unrelated to actually running an apb into the clients package, then we should reconsider renaming apb/client.go to something that more accurately describes what it's doing. Semantically, it's the part of the apb package that's responsible for actually executing apb methods. Could almost be apb/apb.go? Curious what you guys think. EDIT: Realizing this is directly related to my next bullet. NewClient simply initializes the various clients and sticks them onto a struct. All of that is effectively being replaced by the clients package and the clients.<instance>() variants.
We should get rid of this struct if its purpose is to just aggregate references to clients. We've basically turned it into a package, so caller code should import the clients package, and then actually get references by clients.Origin(). Really nice interface that lets us change code behind the instance accessors without breaking callers.
Speaking of clients.() methods, do we want any of these to be singletons? I.E., first time that method is called, check internal _instance field, if it's nil, construct and return and cache instance, then on subsequent calls, return cached instance.
I think I would have preferred for the PRs to have come in as one unit. Made it difficult to review the set since it looks like C builds on B builds on A, and they all introduce new features on top of the others.

eriknelson · 2017-06-23T18:57:40Z

pkg/clients/etcd.go

+	"github.com/coreos/etcd/client"
+)
+
+type EtcdConfig struct {


I think this should remain dao.Config, which is composed of EtcdConfig. Although it's only etcd currently, leaves dao configuration flexible for change.

Speaking of clients.() methods, do we want any of these to be singletons?

singleton object I think that we just create them during initialization of application with the configuration of the clients. Allows us to error early if something is going to go wrong imo.

I think I would have preferred for the PRs to have come in as one unit. Made it difficult to review the set since it looks like C builds on B builds on A, and they all introduce new features on top of the others.

wish that at least on comments that told us the order, which could be added.

We should get rid of this struct

👍

I like the clients pkg a lot, decouples things in a way that makes changes a lot less brittle (client init code isn't just thrown about where it's used; it's got a home). Also going to make it real easy and natural to add an Origin() client.

👍

Semantically, it's the part of the apb package that's responsible for actually executing apb methods. Could almost be apb/apb.go? Curious what you guys think.

To me, it looks like a lot of the ABP code is in their own files. and client.go should all be in the new package. I am wondering what methods you were thinking should be left in apb?

@eriknelson +1 to EtcdConfig remaining Config. I just added that as a comment to my review.

To me, it looks like a lot of the ABP code is in their own files. and client.go should all be in the new package. I am wondering what methods you were thinking should be left in apb?

I don't think code that's responsible for the execution of an apb belongs in the clients pkg, it's a natural fit for the apb pkg. In my mind, the clients package encapsulates the various gateways to other systems and APIs, along with their setup and validation.

RE: the logic that belongs in the apb pkg, it's effectively ExecuteApb(method, parameters), and we should hide the guts of how that actually gets done behind that function (in this case, it's running the pod). ExecuteApb IMO is a better name for RunImage, it's a higher level of abstraction. RunImage bleeds implementation details into the name. It suggests "run this apb method with these parameters, I don't care how you do that" vs "run this image because that's how we happen to run apbs".

Speaking of clients.() methods, do we want any of these to be singletons?

This sounds ok, we need to make sure we test multiple users of the SAME client at the SAME time. Will that work ok? IS it really a 'single' connection? The old way we created a new connection so if there were multiple provisions going on each worker would have their own connection.

We should get rid of this struct

I also concur with this 👍

eriknelson · 2017-06-23T19:11:23Z

pkg/apb/client.go

-	}
-	return config, nil
-}
-
 func NewClient(log *logging.Logger) (*Client, error) {


Can we push this into the clients package? I think it makes more sense there. Related to my overall comment on apb/client.go

@eriknelson I agree, but this is a much larger change since Client is a class. So things like RunImage need to be distributed out of client.go. For now, I'm going to keep this in place, but adding a TODO in the final patch for this series and we can return to it later.

@rthallisey since I'm requesting the change, mind if I take a crack at it and submit for your review? Think I could probably just explain myself better with code anyways. I think it's totally fine to accept this PR without it and add the TODO for a follow up PR.

@eriknelson That would be fantastic 👍

eriknelson · 2017-06-23T19:34:19Z

Is this PR a strict superset of the docker and k8s ones? Should all the changes just get make and addressed here and we can close the others, pulling it all in from this?

rthallisey · 2017-06-23T19:45:58Z

@eriknelson They are each a super set of the previous patch. Personally, I'd like for us to discuss in each patch the code that each one touches. I think it's much easier to review because this entire patch set is large and I have one more on the way ;). Also, I was careful not to have overlap, so anything in the docker patch only touches docker client code. The large patch that's coming last will touch a bit of everything.

If we want to talk about overall structure, then the final patch at the end will make sense since it will have the final picture.

jmrodri · 2017-06-23T19:47:58Z

@rthallisey the individual patches actually make it harder to see on github. Since the diff of the last one is ALL of them. It's hard to see what was just for the last patch vs the first one. And while looking at the first one, it's hard to see where you are going with it in the final implementation. 😄

shawn-hurley · 2017-06-23T20:06:02Z

I am wondering if we are even using the docker client. Pull Image appears to be the only thing that uses the dockerClient and that is not being called anywhere. I think we should consider removing this.

I also would like to note that we should add doc comments on the new exported structs/functions/constants et all.

Lastly, and this is just an idea/thing I want to throw out there. using individual clients should be in their own package. example client/kubernetes/client.go the client object could import this and do the above workflow but just a better speration of the clients.

jmrodri

This one boils down to 1 major change which is keep dao.Config with embedded EtcdConfig

jmrodri · 2017-06-23T20:01:15Z

pkg/app/config.go

 )

 type Config struct {
 	Registry   apb.RegistryConfig
-	Dao        dao.Config
+	Dao        clients.EtcdConfig


This will mean that if we ever want to add some configuration to the dao that is not etcd related we'd have to redo this. dao.Config was easier so we could configure all of the dao. I see how you arrived here since the only configuration items on the dao were etcd related, but it paints in a corner of only being etcd.

jmrodri · 2017-06-23T20:03:03Z

pkg/clients/etcd.go

+	"github.com/coreos/etcd/client"
+)
+
+type EtcdConfig struct {


@eriknelson +1 to EtcdConfig remaining Config. I just added that as a comment to my review.

jmrodri · 2017-06-23T20:04:59Z

pkg/clients/etcd.go

+	"github.com/coreos/etcd/client"
+)
+
+type EtcdConfig struct {


Speaking of clients.() methods, do we want any of these to be singletons?

This sounds ok, we need to make sure we test multiple users of the SAME client at the SAME time. Will that work ok? IS it really a 'single' connection? The old way we created a new connection so if there were multiple provisions going on each worker would have their own connection.

jmrodri · 2017-06-23T20:05:53Z

pkg/clients/etcd.go

+	"github.com/coreos/etcd/client"
+)
+
+type EtcdConfig struct {


We should get rid of this struct

I also concur with this 👍

jmrodri · 2017-06-23T20:06:52Z

pkg/dao/dao.go

-	endpoints []string
-	client    client.Client
-	kapi      client.KeysAPI // Used to interact with kvp API over HTTP
+	config clients.EtcdConfig


should be dao.Config

rthallisey · 2017-06-23T20:17:18Z

@jmrodri wfm. One more on the way

eriknelson · 2017-06-23T20:25:13Z

@shawn-hurley

singleton object I think that we just create them during initialization of application with the configuration of the clients. Allows us to error early if something is going to go wrong imo.

Good argument over lazy singleton, fail fast and early.

I am wondering if we are even using the docker client. Pull Image appears to be the only thing that uses the dockerClient and that is not being called anywhere. I think we should consider removing this.

I think you're right. It's a vestige from when we pulled images before running then with oc run. I don't remember why we needed the pull in the first place, it's possible we never did. It's a use case entirely covered by the Pod pullPolicy. Huge bonus points if it means we can shed a dependency on docker entirely. We should definitely look into this, probably appropriate for an issue and follow up PR. Edit: #227

I also would like to note that we should add doc comments on the new exported structs/functions/constants et all.
👍 , this a style thing for the linter? Obviously a good habit.

Lastly, and this is just an idea/thing I want to throw out there. using individual clients should be in their own package. example client/kubernetes/client.go the client object could import this and do the above workflow but just a better speration of the clients.

What's the benefit of having a package for every client? I understand it would separate things more, just don't see a case for something I can't do with them all in the same package. It also feels like we're blowing up our package count, I'm not familiar enough with go conventions to know if this is frowned upon or it's normal matter of course, but it feels excessive.

shawn-hurley · 2017-06-23T20:32:44Z

I think the major benefit is the code breaking up into their own little place. because of the way go does packages just be in their own file doesn't break it up for the caller. So, for instance, Origin client has a struct that we want to call ServiceRoleBinding but so does Kubernetes and they are different. Rather than having to name them OriginServiceRoleBinding and KuberneetesServiceRoleBinding you name it ServiceRoleBinding and use the package to tell which one to use. I think it makes for cleaner code. If this is not a use case we are worried about then yes I agree it is not worth the extra packages.

eriknelson · 2017-06-23T21:13:54Z

@shawn-hurley I'd propose we start with one package and if we start to see naming collisions, we can break things out.

eriknelson

Going to try to summarize acceptance criteria so @rthallisey can knock stuff out. Please add/dispute anything if I miss something.

Requested Changes:

Doc comments for anything exported so we don't introduce additional linter debt.
dao.Config should remain dao.Config, and it should be composed of EtcdConfig to allow for future expansion/changes.

Issues outside the scope of this PR (should get followed up):

Singleton clients and testing ~~Needs Issue~~ Consider singleton clients #229
Removal of DockerClient, PullImage, and docker dependency Investigate removing dockerClient and dependency #227
I will follow up with a PR to remove the Client struct and constructor from client.go, refactor the file/RunImage to better reflect what we're doing ~~Needs Issue~~ Remove apb/client.go Client struct and related refactoring. #231
Keep the various clients in the clients package, if we start to see naming collisions, let's talk about separating into packages.

rthallisey · 2017-06-23T21:23:32Z

git didn't like the branch rename :). We'll leave it as is

Make a seperate directory for clients so they are not apart of the APB code. This will make the clients easy to initialize and create new ones.

The Etcd Client is a little tricker because it requires a config file. Later when we unify all the clients we will use individual functions just like etcd.

Pull together all the client split work by using a global variable to pass around the clients.

rthallisey · 2017-06-23T21:44:35Z

Sorry for the confusion on the PRs :). That experiment didn't work

rthallisey · 2017-06-24T16:42:46Z

I think the major benefit is the code breaking up into their own little place. because of the way go does packages just be in their own file doesn't break it up for the caller. So, for instance, Origin client has a struct that we want to call ServiceRoleBinding but so does Kubernetes and they are different. Rather than having to name them OriginServiceRoleBinding and KuberneetesServiceRoleBinding you name it ServiceRoleBinding and use the package to tell which one to use. I think it makes for cleaner code. If this is not a use case we are > worried about then yes I agree it is not worth the extra packages.

I think that the clients should stay in the clients package because they're just objects. They aren't classes. We aren't building a kubernetes or docker client, we're creating a variable to call the clients efficiently.

eriknelson · 2017-06-26T19:37:34Z

Going to test this now in an integrated environment to make sure things are still running smoothly, but the current thinking after speak with the guys offline is we're ready to merge this into master. Doc strings can come in with a follow up PR (there's one out there to become golint compliant), and I will be following this up with a change that already has the DaoConfig/EtcdConfig requested change.

eriknelson · 2017-06-26T20:02:42Z

ACK, tested and was able to provision things fine with this on top of master. Merging.

* Break apart the Kubernetes Client Make a seperate directory for clients so they are not apart of the APB code. This will make the clients easy to initialize and create new ones. * Break apart the Docker Client * Break apart the Etcd client The Etcd Client is a little tricker because it requires a config file. Later when we unify all the clients we will use individual functions just like etcd. * Create a global client variable Pull together all the client split work by using a global variable to pass around the clients.

rthallisey added the post-tech-preview label Jun 22, 2017

jmrodri requested review from jmrodri and eriknelson June 23, 2017 11:39

rthallisey force-pushed the breakup-dao-client branch from 9b96a73 to fe5a7c1 Compare June 23, 2017 12:29

eriknelson reviewed Jun 23, 2017

View reviewed changes

jmrodri suggested changes Jun 23, 2017

View reviewed changes

eriknelson mentioned this pull request Jun 23, 2017

Investigate removing dockerClient and dependency #227

Closed

eriknelson mentioned this pull request Jun 23, 2017

Single client object #228

Closed

rthallisey removed the post-tech-preview label Jun 23, 2017

This was referenced Jun 23, 2017

Break apart the Kubernetes Client #214

Closed

Pull out the docker client #215

Closed

rthallisey changed the title ~~Breakup dao client~~ Breakup all the Broker Clients into a clients pkg Jun 23, 2017

eriknelson suggested changes Jun 23, 2017

View reviewed changes

rthallisey closed this Jun 23, 2017

rthallisey deleted the breakup-dao-client branch June 23, 2017 21:22

rthallisey restored the breakup-dao-client branch June 23, 2017 21:22

rthallisey reopened this Jun 23, 2017

Ryan Hallisey added 3 commits June 23, 2017 17:34

Break apart the Kubernetes Client

e0fadf4

Make a seperate directory for clients so they are not apart of the APB code. This will make the clients easy to initialize and create new ones.

Break apart the Docker Client

8a69c7b

Break apart the Etcd client

309a497

The Etcd Client is a little tricker because it requires a config file. Later when we unify all the clients we will use individual functions just like etcd.

Create a global client variable

c8ddfb5

Pull together all the client split work by using a global variable to pass around the clients.

rthallisey force-pushed the breakup-dao-client branch from 173cd5b to c8ddfb5 Compare June 23, 2017 21:43

This was referenced Jun 23, 2017

Consider singleton clients #229

Closed

Remove apb/client.go Client struct and related refactoring. #231

Closed

This was referenced Jun 26, 2017

Proposal: Support both Openshift and Kubernetes #233

Closed

With a newer Etcd, we can use the GetVersion function #223

Merged

jmrodri approved these changes Jun 26, 2017

View reviewed changes

eriknelson approved these changes Jun 26, 2017

View reviewed changes

eriknelson merged commit b2809dc into openshift:master Jun 26, 2017

rthallisey deleted the breakup-dao-client branch July 27, 2017 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breakup all the Broker Clients into a clients pkg #222

Breakup all the Broker Clients into a clients pkg #222

rthallisey commented Jun 22, 2017 •

edited

rthallisey commented Jun 22, 2017

eriknelson left a comment •

edited

eriknelson Jun 23, 2017

shawn-hurley Jun 23, 2017 •

edited

jmrodri Jun 23, 2017

eriknelson Jun 23, 2017 •

edited

jmrodri Jun 23, 2017

jmrodri Jun 23, 2017

eriknelson Jun 23, 2017

rthallisey Jun 23, 2017

eriknelson Jun 23, 2017 •

edited

rthallisey Jun 23, 2017

eriknelson commented Jun 23, 2017

rthallisey commented Jun 23, 2017

jmrodri commented Jun 23, 2017

shawn-hurley commented Jun 23, 2017

jmrodri left a comment

jmrodri Jun 23, 2017

jmrodri Jun 23, 2017

jmrodri Jun 23, 2017

jmrodri Jun 23, 2017

jmrodri Jun 23, 2017

rthallisey commented Jun 23, 2017

eriknelson commented Jun 23, 2017 •

edited

shawn-hurley commented Jun 23, 2017

eriknelson commented Jun 23, 2017

eriknelson left a comment •

edited

rthallisey commented Jun 23, 2017

rthallisey commented Jun 23, 2017

rthallisey commented Jun 24, 2017

eriknelson commented Jun 26, 2017

eriknelson commented Jun 26, 2017

Breakup all the Broker Clients into a clients pkg #222

Breakup all the Broker Clients into a clients pkg #222

Conversation

rthallisey commented Jun 22, 2017 • edited

rthallisey commented Jun 22, 2017

eriknelson left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson Jun 23, 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

eriknelson Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson commented Jun 23, 2017

rthallisey commented Jun 23, 2017

jmrodri commented Jun 23, 2017

shawn-hurley commented Jun 23, 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

rthallisey commented Jun 23, 2017

eriknelson commented Jun 23, 2017 • edited

shawn-hurley commented Jun 23, 2017

eriknelson commented Jun 23, 2017

eriknelson left a comment • edited

Choose a reason for hiding this comment

rthallisey commented Jun 23, 2017

rthallisey commented Jun 23, 2017

rthallisey commented Jun 24, 2017

eriknelson commented Jun 26, 2017

eriknelson commented Jun 26, 2017

rthallisey commented Jun 22, 2017 •

edited

eriknelson left a comment •

edited

shawn-hurley Jun 23, 2017 •

edited

eriknelson Jun 23, 2017 •

edited

eriknelson Jun 23, 2017 •

edited

eriknelson commented Jun 23, 2017 •

edited

eriknelson left a comment •

edited