Expose the remote driver API structs publicly. by erikh · Pull Request #249 · moby/libnetwork

erikh · 2015-06-03T04:54:59Z

The diff sucks. Sorry in advance.

This exposes the message types formerly in drivers/remote/messages.go and moves it into drivers/remote/api. I have taken some care to pass golint but the comments are lacking, happy to fix this up more if that's desired, just need to know what to say.

The other big thing is that I moved maybeError, an interface that api.Response conforms to, into the other driver file and made getError public. This way they can split between.

squaremo · 2015-06-03T08:30:24Z

Nice! Thanks Erik. Looks good at a scan, and I'll try it out today.

squaremo · 2015-06-03T10:46:08Z

Works fine with the weave plugin, and a definite improvement, so 👍

mapuri · 2015-06-03T16:47:43Z

LGTM!

Not sure if it's a good idea but just wondering if the file drivers/remote/api/api.go should be part of the drivers/driverapi package, so that all driver implementations (including the native remote driver and the external drivers) just need to import and use the driverapi package from libnetwork? It might also make it easier if we decide to share these structs with the arguments and return values to the methods exposed by driverapi.Driver interface, tahn having to keep parity between the two everytime we add a feature to the driverapi.

squaremo · 2015-06-04T11:38:17Z

It might also make it easier if we decide to share these structs with the arguments and return values to the methods exposed by driverapi.Driver interface

The driverapi.Driver interface doesn't map straight-forwardly to RPCs, because of the EndpointInfo and JoinInfo arguments. So the structs in remote/api really are particular to the remote driver implementation.

Having said that, it would be nicely to enforce parity of driverapi.Driver and remote/api somehow.

squaremo · 2015-06-04T11:44:50Z

@erikh Looks like you were beaten to the punch by #241 and #240, which make changes to drivers/remote/messages. Should be an easy update though.

mavenugo · 2015-06-04T12:26:27Z

drivers/remote/api/api.go

Can you pls move this under netutils package ?

Yep I can!

Michael, I will rebase later today.

-Erik

On Jun 4, 2015, at 5:26 AM, Madhu Venugopal notifications@github.com wrote:

In drivers/remote/api/api.go #249 (comment):

HostsPath string

ResolvConfPath string
+}

+// LeaveRequest describes the API for detaching an endpoint from a sandbox.
+type LeaveRequest struct {

NetworkID string

EndpointID string
+}

+// LeaveResponse is the answer to LeaveRequest.
+type LeaveResponse struct {

Response
+}

+func toAddr(ipAddr string) (*net.IPNet, error) {
Can you pls move this under netutils package ?

—
Reply to this email directly or view it on GitHub https://github.com/docker/libnetwork/pull/249/files#r31715215.

mavenugo · 2015-06-09T02:06:37Z

@erikh any luck with this ?

erikh · 2015-06-09T08:23:40Z

Working on this now.

On Jun 8, 2015, at 7:06 PM, Madhu Venugopal notifications@github.com wrote:

@erikh https://github.com/erikh any luck with this ?

—
Reply to this email directly or view it on GitHub #249 (comment).

erikh · 2015-06-09T08:33:57Z

This should be ready for review. Note that I fudged the merge a little, sorry about that and I hope it's not too much of an issue.

mrjana · 2015-07-07T17:14:08Z

@erikh Would it be more useful to also make the entire remote driver protocol as a package rather than just exposing the structs for JSON encoding? That way we hide all of plugin management and such from the client code and client code can just send events or react to events defined in terms of interfaces

erikh · 2015-07-07T18:09:42Z

@mrjana I could make an attempt, but that's outside the scope of what I wanted to do here. If you want to give me a more detailed example of what you're looking for, I can attempt a plugin harness.

Additionally, I need to deserialize the structs in a versioned way. I don't think an evented system will provide that without a lot of contortion.

dave-tucker · 2015-07-15T02:13:30Z

I say we create another issue re: making the remote driver protocol a package. It shouldn't prevent this being merged...
@erikh would you be able to rebase please? would love to get this included in the 0.4 release

erikh · 2015-07-15T05:18:47Z

Guys, I don’t even know what happened here, but the rebase was pretty clean and it’s still broken.

I’ll see if I can figure it out tomorrow, but I can’t spend too much time on this.

On Jul 14, 2015, at 7:13 PM, Dave Tucker notifications@github.com wrote:

I say we create another issue re: making the remote driver protocol a package. It shouldn't prevent this being merged...
@erikh https://github.com/erikh would you be able to rebase please? would love to get this included in the 0.4 release

—
Reply to this email directly or view it on GitHub #249 (comment).

erikh · 2015-07-16T06:17:40Z

Got it sorted. PTAL. Note that I moved both the public functions in api into remote as private functions that receive the response structs as the first argument instead of making them the method receiver. This is IMO a little cleaner and that way API carries no code, only structs.

dave-tucker · 2015-07-16T13:24:04Z

Thanks erikh 🍰
/cc @mavenugo @aboch @mrjana

erikh · 2015-07-27T16:10:42Z

Anything I can do to move this along? We're already depending on these structs in our netplugin.

mavenugo · 2015-07-29T15:43:58Z

@erikh apologies on the delay. LGTM.
@squaremo, @tomdee , @spikecurtis can you please confirm if this works for you too ?

mrjana · 2015-07-29T15:47:40Z

LGTM

Expose the remote driver API structs publicly.

squaremo · 2015-07-29T16:08:53Z

drivers/remote/api/api.go

How come this is here rather than in driver.go? Alternatively: how come this is here, but the convenience functions for converting back and forth to string are not?

Oversight, sorry.

Well it doesnae hurt ... let's fix it up in post

squaremo mentioned this pull request Jun 3, 2015

Update remote driver to use destination prefix. #241

Merged

mavenugo reviewed Jun 4, 2015
View reviewed changes

dave-tucker added this to the 0.4 milestone Jul 15, 2015

dave-tucker added the status/2-needs-code-review label Jul 15, 2015

Expose the remote driver API structs publicly.

100f13f

erikh mentioned this pull request Jul 27, 2015

First revision of dockcontivnet contiv/netplugin#117

Merged

mrjana added a commit that referenced this pull request Jul 29, 2015

Merge pull request #249 from erikh/expose-api

4d6e9ae

Expose the remote driver API structs publicly.

mrjana merged commit 4d6e9ae into moby:master Jul 29, 2015

squaremo reviewed Jul 29, 2015
View reviewed changes

Conversation

erikh commented Jun 3, 2015

Uh oh!

squaremo commented Jun 3, 2015

Uh oh!

squaremo commented Jun 3, 2015

Uh oh!

mapuri commented Jun 3, 2015

Uh oh!

squaremo commented Jun 4, 2015

Uh oh!

squaremo commented Jun 4, 2015

Uh oh!

mavenugo Jun 4, 2015

Choose a reason for hiding this comment

Uh oh!

erikh Jun 4, 2015

Choose a reason for hiding this comment

Uh oh!

mavenugo commented Jun 9, 2015

Uh oh!

erikh commented Jun 9, 2015

Uh oh!

erikh commented Jun 9, 2015

Uh oh!

mrjana commented Jul 7, 2015

Uh oh!

erikh commented Jul 7, 2015

Uh oh!

dave-tucker commented Jul 15, 2015

Uh oh!

erikh commented Jul 15, 2015

Uh oh!

erikh commented Jul 16, 2015

Uh oh!

dave-tucker commented Jul 16, 2015

Uh oh!

erikh commented Jul 27, 2015

Uh oh!

mavenugo commented Jul 29, 2015

Uh oh!

mrjana commented Jul 29, 2015

Uh oh!

squaremo Jul 29, 2015

Choose a reason for hiding this comment

Uh oh!

erikh Jul 29, 2015

Choose a reason for hiding this comment

Uh oh!

squaremo Jul 29, 2015

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants