Grpc change for network options in service spec #2176

abhi · 2017-05-10T21:54:18Z

Service command is being updated to accept network
specfic options like name,alias and driver options.The
commit contains changes to grpc service api and adopting
the same in task attachment

Signed-off-by: Abhinandan Prativadi [email protected]

aaronlehmann · 2017-05-10T22:07:56Z

api/types.proto

 	// Addresses specifies a list of ipv4 and ipv6 addresses
 	// preferred. If these addresses are not available then the
 	// attachment might fail.
 	repeated string addresses = 3;
+
+	//Driver options is a map of driver specific options for the network target


// DriverOpts is a map of driver-specific options for the network target.

The added lines above have trailing whitespace; please remove the tab characters.

Isn't there already a type for this?

I'd prefer that we avoid using the map types here, as their order is undefined.

@aaronlehmann I added spaces that were non existent only for this message in the file to make it consistent.
@stevvooe I don't see a type. And I believe we do pack messages in map. Is there an alternative or any known issues with this approach ?

@stevvooe: Are you talking about Driver?

@stevvooe: I'm not sure I understand the question, but this is the definition of Driver:

// Driver is a generic driver type to be used throughout the API. For now, a // driver is simply a name and set of options. The field contents depend on the // target use case and driver application. For example, a network driver may // have different rules than a volume driver. message Driver { string name = 1; map <string, string> options = 2; }

So, are these options propagated to the target then? How are they different from the existing service spec options?

I must be missing a detail here.

@stevvooe: Which existing service spec options are you talking about? I don't see any on the network attachment level, but I may also be missing something.

@abhinandanpb as a best practice always add the dependent PR links and that will help the reviewers to map the usage.

@stevvooe @aaronlehmann I think the usage details as expressed in moby/moby#33130 and docker/cli#62 will help explain this new option. Essentially, these are some of the user-specified service options that are passed to the network drivers. Labels are NOT sent to the drivers and in other docker objects we have introduced driver-opts to pass on such operator specific params to the drivers (volumes and network). Service object was missing it and via this PR @abhinandanpb is introducing it.

Can you please update the comment as requested above: https://github.com/docker/swarmkit/pull/2176/files#r115864626

This will make it consistent with the godoc-style comments elsewhere in this message that refer to the camelcased versions of the names.

aaronlehmann · 2017-05-10T22:08:50Z

I see that some unrelated generated .pb.go files are being changed. Are you running Go 1.7.x? I think other versions of go generate these files differently.

abhi · 2017-05-10T23:04:01Z

@aaronlehmann I believe the changes are due to the ripple effect due to the changes in the NetworkAttachmentConfig since its being used in other messages directly or embedded via another message ? I am using go1.8

aaronlehmann · 2017-05-10T23:05:44Z

We're using Go 1.7 right now, but we'll probably switch to Go 1.8 within a day or two when Docker switches over. If the PR can wait until then, the problem with inconsistent protobuf generation should go away.

codecov · 2017-05-11T06:50:52Z

Codecov Report

Merging #2176 into master will increase coverage by 0.25%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2176      +/-   ##
==========================================
+ Coverage   59.99%   60.24%   +0.25%     
==========================================
  Files         119      119              
  Lines       19784    19785       +1     
==========================================
+ Hits        11869    11920      +51     
+ Misses       6564     6534      -30     
+ Partials     1351     1331      -20

abhi · 2017-05-11T07:04:43Z

@aaronlehmann generated from a vm with go1.7. PTAL

aaronlehmann · 2017-05-11T17:46:42Z

@abhinandanpb: Thanks.

mavenugo · 2017-05-12T11:50:14Z

@aaronlehmann @abhinandanpb moby/moby is updated to go1.8.1. Can you please get this PR reviewed ? This PR is the base dependency on moby/moby#33130 and docker/cli#62

mavenugo

LGTM (NAM)

aaronlehmann · 2017-05-12T23:33:13Z

LGTM

I was hoping to get "Driver options" in the comment changed to DriverOpts to be consistent with the other comments, and to remove the tabs on empty lines, but I don't want to hold this up over that.

Service command is being updated to accept network specfic options like name,alias and driver options.The commit contains changes to grpc service api and adopting the same in task attachment Signed-off-by: Abhinandan Prativadi <[email protected]> temp

abhi · 2017-05-12T23:57:25Z

@aaronlehmann addressed all comments. PTAL.

aaronlehmann · 2017-05-13T00:01:52Z

Still LGTM, thanks

thaJeztah · 2017-05-15T12:25:55Z

ping @aluzzardi @stevvooe @aboch PTAL

aluzzardi · 2017-05-15T20:38:34Z

LGTM

stevvooe · 2017-05-15T21:03:15Z

How will consumers of this field no which driver these options are valid for?

stevvooe · 2017-05-15T21:13:44Z

Effectively, I think that the DriverOpts need to be namespaced by driver or included with a driver structure.

It also needs to be clarified that this is parameterizing the attachment to a network and not the network itself.

mavenugo · 2017-05-15T21:21:58Z

@stevvooe this field is used in conjunction with network (docker/cli#62) and is not an independent field. Each network is backed by one and only driver and hence the driver-opt for a given service is that network/driver specific.
am not sure if that answers your question. Please let me know.

stevvooe · 2017-05-15T21:44:27Z

@mavenugo Are they parameterizing the creation of a network or the attachment to a network? Right now, from the field documentation on the protobuf, this is not clear in the way these are intended to be used.

mavenugo · 2017-05-15T21:57:12Z

okay. I found the documentation okay because // Map of all the driver options for this network was written inside of NetworkAttachment object and hence contextually it can make sense. But if you think we should better comment it, I will let @abhinandanpb make it more descriptive.

stevvooe · 2017-05-15T22:02:28Z

@mavenugo That comment is misleading. They are the options for attaching to the network. The network is already created and the options are already specified when it was created. In that sense, it doesn't really make sense contextually, as these cannot parameterize a driver because it has already been created.

Should these be called DriverAttachmentOpts or something like that?

mavenugo · 2017-05-15T22:09:43Z

@stevvooe yes. the DriverAttachmentOpts makes it clear.
@abhinandanpb wdyt ?

Addressing comment from stevvooe on moby#2176 Signed-off-by: Abhinandan Prativadi <[email protected]>

aaronlehmann reviewed May 10, 2017

View reviewed changes

abhi force-pushed the network-opt branch 2 times, most recently from 5ec0573 to 4fd6c58 Compare May 11, 2017 06:43

abhi force-pushed the network-opt branch from 4fd6c58 to 8b364f7 Compare May 11, 2017 07:03

mavenugo approved these changes May 12, 2017

View reviewed changes

abhi mentioned this pull request May 12, 2017

Adding network specific option in csv format for service create/update docker/cli#62

Merged

abhi force-pushed the network-opt branch 2 times, most recently from d50d87a to 9c0ee6d Compare May 12, 2017 22:28

abhi force-pushed the network-opt branch from 9c0ee6d to bb1cdce Compare May 12, 2017 23:50

abhi force-pushed the network-opt branch from bb1cdce to 950b3d0 Compare May 12, 2017 23:55

aluzzardi merged commit 993a222 into moby:master May 15, 2017

abhi added a commit to abhi/swarmkit that referenced this pull request May 15, 2017

Changing networkAttachment to address PR comment

1378e76

Addressing comment from stevvooe on moby#2176 Signed-off-by: Abhinandan Prativadi <[email protected]>

abhi added a commit to abhi/swarmkit that referenced this pull request May 15, 2017

Changing networkAttachment to address PR comment

d811f93

Addressing comment from stevvooe on moby#2176 Signed-off-by: Abhinandan Prativadi <[email protected]>

abhi mentioned this pull request May 15, 2017

Changing networkAttachment to address PR comment #2183

Merged

abhi added a commit to abhi/swarmkit that referenced this pull request May 16, 2017

Changing networkAttachment to address PR comment

498315d

Addressing comment from stevvooe on moby#2176 Signed-off-by: Abhinandan Prativadi <[email protected]>

abhi mentioned this pull request May 17, 2017

Adding network specific options to service create/update moby/moby#33130

Merged

abhi mentioned this pull request Jun 20, 2017

Add template support for network aliases #2250

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grpc change for network options in service spec #2176

Grpc change for network options in service spec #2176

abhi commented May 10, 2017

aaronlehmann May 10, 2017

stevvooe May 10, 2017

stevvooe May 10, 2017

abhi May 10, 2017

aaronlehmann May 10, 2017

aaronlehmann May 10, 2017

stevvooe May 10, 2017

aaronlehmann May 11, 2017

mavenugo May 12, 2017

aaronlehmann May 12, 2017

aaronlehmann commented May 10, 2017

abhi commented May 10, 2017

aaronlehmann commented May 10, 2017

codecov bot commented May 11, 2017 •

edited

Loading

abhi commented May 11, 2017

aaronlehmann commented May 11, 2017

mavenugo commented May 12, 2017

mavenugo left a comment

aaronlehmann commented May 12, 2017

abhi commented May 12, 2017

aaronlehmann commented May 13, 2017

thaJeztah commented May 15, 2017

aluzzardi commented May 15, 2017

stevvooe commented May 15, 2017

stevvooe commented May 15, 2017

mavenugo commented May 15, 2017

stevvooe commented May 15, 2017

mavenugo commented May 15, 2017

stevvooe commented May 15, 2017

mavenugo commented May 15, 2017

Grpc change for network options in service spec #2176

Grpc change for network options in service spec #2176

Conversation

abhi commented May 10, 2017

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

aaronlehmann commented May 10, 2017

abhi commented May 10, 2017

aaronlehmann commented May 10, 2017

codecov bot commented May 11, 2017 • edited Loading

Codecov Report

abhi commented May 11, 2017

aaronlehmann commented May 11, 2017

mavenugo commented May 12, 2017

mavenugo left a comment

Choose a reason for hiding this comment

aaronlehmann commented May 12, 2017

abhi commented May 12, 2017

aaronlehmann commented May 13, 2017

thaJeztah commented May 15, 2017

aluzzardi commented May 15, 2017

stevvooe commented May 15, 2017

stevvooe commented May 15, 2017

mavenugo commented May 15, 2017

stevvooe commented May 15, 2017

mavenugo commented May 15, 2017

stevvooe commented May 15, 2017

mavenugo commented May 15, 2017

codecov bot commented May 11, 2017 •

edited

Loading