Versioned proto and golang package structure #195

jdef · 2018-02-16T12:12:31Z

GetSupportedVersions RPC moves from Identity to a new Versioned gRPC service
fixes Consider adding vMAJOR version (e.g. csi.v1.) to either the API bindings or package names #193
- spec major version name is incorporated into the generated proto package
generated protobuf files are as follows:
- csi.proto: contains versioned gRPC services and protobuf messages
- versioned.proto: contains Versioned service and related proto messages. SHOULD never change across CSI revisions. MUST never change in ways that break backward/forward across any releases of CSI
generated Go bindings are packaged in a csi/v1 subdirectory
- importing code may still refer to bindings using csi package prefix by default

alternative to #194

akutz · 2018-02-16T14:01:36Z

Maybe I’m confused, but why submit your own PR instead of supplying feedback to the one I already submitted?

jdef · 2018-02-16T14:05:04Z

I wrote my changes before seeing yours. It was less effort for me to simply push an alternate PR for the code I had already written vs. rewrite my PR as a list of suggestions. Feel free to incorporate changes from this PR to maintain authorship.

…

On Fri, Feb 16, 2018 at 9:01 AM, Schley Andrew Kutz < ***@***.***> wrote: Maybe I’m confused, but why submit your own PR instead of supplying feedback to the one I already submitted? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLCuNEmOcMtdfd_1uNvrpgjg8Gw4tks5tVYpAgaJpZM4SIP5D> .

akutz · 2018-02-16T16:41:27Z

Hi @jdef,

Let's just go with yours :)

julian-hj

I am a bit troubled by the interaction between this change and the GetSupportedVersions RPC. To me it seems very awkward to have to call v1.GetSupportedVersions to find out whether or not v1 is supported.

I imagine normally if you are going to have some mechanism to fetch the supported versions, you would put that RPC outside of the versioned namespace and make some guarantee that it won't change, but I think in our case, we have loaded enough other calls into the Identity service that just leaving it out of versioning is not practical.

So...do we still actually need the GetSupportedVersions call if we make each major version into its own package? If so, can we clarify the interaction?

akutz

Hi @jdef,

As I addressed in #194, I am not sure that placing the generated language bindings in a versioned package path is the best solution.

Maintaining all previous versions at the HEAD of the repository would result in a convoluted generation process as well as questions about whether lib/go/csi/v2 is v2.0, v2.1.5, v2.3, etc. One might assert that the Git tag could be used to access the specific version, but if that's the case I see no value in providing a versioned path structure.

People that wish to access older versions of the specification could do so by using git or GitHub's UI to fetch a specific tag or commit and manually copying the protobuf or language binding locally.

Additionally, I'm not sure that the following options are necessary:

option go_package = "csi";
option java_package="csi";
option csharp_namespace="csi";

It's not difficult to import a package with an alias in Go, Java, or C#, so I'm not sure it's necessary to bother worrying about stripping the version from the package of the language bindings.

Thanks for doing this!

akutz · 2018-02-16T18:54:39Z

csi.proto

-package csi;
+package csi.v1;
+
+option go_package = "csi";


Hi @jdef,

Please consider adding the following after option go_package:

option java_package="csi"; option csharp_namespace="csi";

As mentioned later in this thread, I think this only makes sense to do once we're generating bindings within the spec repo for these langs, otherwise it may be difficult to predict the full impact of changes on bindings generated from the spec.

akutz · 2018-02-16T18:57:22Z

lib/go/Makefile

@@ -76,10 +76,11 @@ export PATH := $(shell pwd):$(PATH)
 ########################################################################
 ##                              BUILD                                 ##
 ########################################################################
-CSI_GO := csi/csi.pb.go
-CSI_A := csi.a
 CSI_PROTO := ../../csi.proto


Hi @jdef,

If the language binding's path/package structure is going to be versioned then shouldn't csi.proto as well?

I believe this is necessary for C++ to version the .proto file.

akutz · 2018-02-16T19:05:44Z

Hi @julian-hj,

You make a good point regarding the GetSupportedVersions call. My suggestion? Create two protos and generate the Identity service without a versioned component and ensure that all RPCs and messages remain as static as possible. It could be overkill, but it's an idea. If nothing else, I see no reason that it would be overly difficult to answer to multiple versions of a GetSupportedVersions call on the server side. A piece of gRPC middleware could easily intercept all incoming requests, scan the request message's name with a pattern csi\.(?<version>[^\.]+)\.GetSupportedVersions and then return the appropriate response message based on the request's API version.

jdef · 2018-02-21T17:54:28Z

I am a bit troubled by the interaction between this change and the GetSupportedVersions RPC. To me it seems very awkward to have to call v1.GetSupportedVersions to find out whether or not v1 is supported.

AFAIK the intent is that GetSupportedVersions SHALL NOT ever change in a way that's breaking/incompatible with prior or future versions of the spec. This allows a CO to query any plugin for its supported versions, regardless of the spec version that the plugin has implemented. If needed, we can break out GetSupportedVersions into a separate service but I'm not convinced that we're there yet, and I don't see the harm in leaving it where it is for the moment.

That said, other message types are certainly expected to break across major spec revisions and namespacing the messages by major version gives us the freedom to both eliminate deprecated/obsolete things as well as restructure the RPCs to better fit the needs of the time.

As I addressed in #194, I am not sure that placing the generated language bindings in a versioned package path is the best solution.

I decided to place the generated bindings in a versioned package path because it allows CO and SP implementations to easily support multiple variants of CSI at the same time. Relying upon ONLY GH-versioning/repo-tagging for this leads to wacky solutions that don't necessarily scale well over time.

Currently thinking about the best way to version the .proto and spec.md files: if we need to generate all versions with every make then we'll need something more flexible than the scheme currently in place.

julian-hj · 2018-02-21T18:15:07Z

Maybe I am missing something, but to me, we lose the benefit of not changing GetSupportedVersions once we put it into a versioned namespace.

Say theoretically, we have a Plugin that supports V2, and a CO that can consume either V1 or V2. The CO can't just call GetSupportedVersions anymore to find out which version of the client to use. It instead has to try to invoke "/csi.v1.Identity/GetSupportedVersions", and when that fails, it has to try to use the V2 service version. So most of the version support information will be determined by the success or failure of the RPC.

I suppose a plugin author could choose to implement multiple identity service versions, just to be callable from incompatible COs, but I doubt any plugin author would bother.

Does this make sense, or am I working from wrong assumptions?

jdef · 2018-02-21T18:46:16Z

If GetSupportedVersions is wire compatible for prior and future versions of the spec, what stops a v1 GetSupportedVersions call from working against a server that was implemented against a v2 spec?

…

On Wed, Feb 21, 2018 at 1:15 PM, Julian Hjortshoj ***@***.***> wrote: Maybe I am missing something, but to me, we lose the benefit of not changing GetSupportedVersions once we put it into a versioned namespace. Say theoretically, we have a Plugin that supports V2, and a CO that can consume either V1 or V2. The CO can't just call GetSupportedVersions anymore to find out which version of the client to use. It instead has to try to invoke "/csi.v1.Identity/GetSupportedVersions", and when that fails, it has to try to use the V2 service version. So *most* of the version support information will be determined by the success or failure of the RPC. I suppose a plugin author could choose to implement multiple identity service versions, just to be callable from incompatible COs, but I doubt any plugin author would bother. Does this make sense, or am I working from wrong assumptions? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLLhPQBkaodRXoI8kbVg0W7QaoSyCks5tXF0rgaJpZM4SIP5D> .

julian-hj · 2018-02-21T21:34:01Z

@jdef Isn't the whole point of namespacing that we will create a separate set of RPCs? My read is that it wouldn't work because it is a different RPC.

See the following diff in the grpc client code:
https://github.com/container-storage-interface/spec/pull/195/files#diff-9f98c96192effffd9a3d33831d159befR1984

cpuguy83 · 2018-02-22T01:21:51Z

Couldn't we leave GetSupportedVersions in v1 and use it to determine what features are available from v 1.x? Then when v2 comes around we can do the same for v2.

saad-ali · 2018-02-22T06:44:55Z

@jdef Isn't the whole point of namespacing that we will create a separate set of RPCs? My read is that it wouldn't work because it is a different RPC.

I believe that since option go_package = "csi"; is specified the go package will still be csi not csi.v1. +1 to @akutz comment on doing the same for other languages. The code you linked to is just registering the proto.

That said, I do have questions about this:

I decided to place the generated bindings in a versioned package path because it allows CO and SP implementations to easily support multiple variants of CSI at the same time. Relying upon ONLY GH-versioning/repo-tagging for this leads to wacky solutions that don't necessarily scale well over time.

Currently thinking about the best way to version the .proto and spec.md files: if we need to generate all versions with every make then we'll need something more flexible than the scheme currently in place.

In order for this to work HEAD needs to be to maintain all previous versions of the generated bindings in separate folders. What is the plan for managing that? Would HEAD generate to lib/go/csi/v1/csi.pb.go until v2 was cut? And then HEAD generates to lib/go/csi/v2/csi.pb.go? If so how do we update v1? Manually?

Also should the current version be v1 or v02?

* Versioned service presents RPC to discover CSI version. - GetSupportedVersions is now an unversioned RPC of Versioned. - Version is now an unversioned message. * Identity is no longer responsible for version negotiation. * Versioned is a mandatory service, like Identity.

jdef · 2018-02-22T16:43:59Z

Isn't the whole point of namespacing that we will create a separate set of RPCs? My read is that it wouldn't work because it is a different RPC.

I think you're right @julian-hj - it looks like gRPC IDL service names incorporate the protobuf package name: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#appendix-a---grpc-for-protobuf

... so perhaps it makes sense to extract GetSupportedVersions into an unversioned service? I tried to do that with my latest commits.

In order for this to work HEAD needs to be to maintain all previous versions of the generated bindings in separate folders. What is the plan for managing that?

Current thinking is that HEAD would maintain (somewhere) a copy of the prior, backward-incompatible specs and would be able to generate all versions of all prior specs every time you run make. I don't have a solid plan yet for a directory structure. Initial thinking was that we could copy/paste/rename the spec.md file for each major revision. Since we don't have any major revisions yet, I haven't solved this problem yet.

That said, if we want this mechanism to be more granular, as suggested by @akutz (for example, to support breaking spec variants prior to releasing v1) then we should attempt to solve this sooner than later.

jdef · 2018-02-22T16:45:51Z

I believe that since option go_package = "csi"; is specified the go package will still be csi not csi.v1. +1 to @akutz comment on doing the same for other languages. The code you linked to is just registering the proto.

I added the option for Go package generation because we include Go bindings in this repo. C++ doesn't have an option like this, instead the C++ namespace is derived from the protobuf package name. No other languages have build files in this repo. Does it make sense to include protoc options for langs that we don't have a way to test/build in CI?

jdef · 2018-02-22T16:48:16Z

Couldn't we leave GetSupportedVersions in v1 and use it to determine what features are available from v 1.x? Then when v2 comes around we can do the same for v2.

We could but that kind of defeats the purpose of GetSupportedVersions as mentioned by @julian-hj . The original intent of the RPC was to support version discovery/negotiation across breaking versions of the spec. My thinking is that we should keep it that way.

julian-hj · 2018-02-22T17:40:08Z

The new Versioned service answers my main objection to this PR, so I am OK with it now as-is, provided that others are also OK with it. But I am sort of starting to think we might be better off just removing the GetSupportedVersions rpc altogether. Between this call and the Probe call and the GetCapabilities calls, we are building up a fair amount of cruft that's not really related to the actual work the plugin has to do, and could be reasonably relegated to documentation.

cpuguy83 · 2018-02-22T19:44:11Z

I would prefer to not take version negotiation out of the spec, otherwise this will be adding to what the SP needs to figure out for each CO.

saad-ali · 2018-02-22T19:49:49Z

I would prefer to not take version negotiation out of the spec, otherwise this will be adding to what the SP needs to figure out for each CO.

Having a SP expose a single version per endpoint does simplify things, but it also limits SP flexibility.

julian-hj · 2018-02-22T20:56:43Z

I would prefer to not take version negotiation out of the spec, otherwise this will be adding to what the SP needs to figure out for each CO.

I wasn't suggesting removal of version negotiation entirely. I was merely questioning whether or not GetSupportedVersions offers the CO any useful insights about version support that it cannot get by just calling Probe() with the version it will actually use. From the SP side, I don't think removing GetSupportedVersions() would really change anything except to reduce the number of RPCs (and now services) they have to implement by 1. Anyway, I hope we can all discuss it in a few minutes.

cpuguy83 · 2018-02-22T21:15:16Z

versioned.proto

+message GetSupportedVersionsResponse {
+  // All the CSI versions that the Plugin supports. This field is
+  // REQUIRED.
+  repeated Version supported_versions = 1;


Since we're changing this, we've found that we need to know what the min and max supported version so that this can be negotiated.

jieyu · 2018-02-23T23:46:08Z

Resolved by #204

James DeFelice added 3 commits February 16, 2018 12:04

spec: proto package incorporates major version

b5697ba

lib/go: generate bindings into versioned package structure

4e6c341

git: ignore .build for the whole project

ef3f61c

jdef requested review from jieyu and saad-ali February 16, 2018 12:12

akutz mentioned this pull request Feb 16, 2018

Version the CSI package name #194

Closed

julian-hj reviewed Feb 16, 2018

View reviewed changes

akutz suggested changes Feb 16, 2018

View reviewed changes

saad-ali mentioned this pull request Feb 21, 2018

Clarify Identity Service vs Probe request #171

Closed

James DeFelice added 3 commits February 22, 2018 16:11

git: ignore all .tmp files

06856c9

make: generate and build multiple protos from a single spec

a26766e

jdef mentioned this pull request Feb 22, 2018

Consider adding vMAJOR version (e.g. csi.v1.) to either the API bindings or package names #193

Closed

cpuguy83 reviewed Feb 22, 2018

View reviewed changes

jdef mentioned this pull request Feb 22, 2018

Drop GetSupportedVersions in favor of versioned proto package #202

Closed

jieyu mentioned this pull request Feb 23, 2018

Drop GetSupportedVersions in favor of versioned proto package #204

Merged

jieyu closed this Feb 23, 2018

Versioned proto and golang package structure #195

Versioned proto and golang package structure #195

Uh oh!

Conversation

jdef commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akutz commented Feb 16, 2018

Uh oh!

jdef commented Feb 16, 2018 via email

Uh oh!

akutz commented Feb 16, 2018

Uh oh!

julian-hj left a comment

Choose a reason for hiding this comment

Uh oh!

akutz left a comment

Choose a reason for hiding this comment

Uh oh!

akutz Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

jdef Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

akutz Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

jieyu Feb 17, 2018

Choose a reason for hiding this comment

Uh oh!

akutz commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdef commented Feb 21, 2018

Uh oh!

julian-hj commented Feb 21, 2018

Uh oh!

jdef commented Feb 21, 2018 via email

Uh oh!

julian-hj commented Feb 21, 2018

Uh oh!

cpuguy83 commented Feb 22, 2018

Uh oh!

saad-ali commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdef commented Feb 22, 2018

Uh oh!

jdef commented Feb 22, 2018

Uh oh!

jdef commented Feb 22, 2018

Uh oh!

julian-hj commented Feb 22, 2018

Uh oh!

cpuguy83 commented Feb 22, 2018

Uh oh!

saad-ali commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julian-hj commented Feb 22, 2018

Uh oh!

cpuguy83 Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

jieyu commented Feb 23, 2018

Uh oh!

Uh oh!

jdef commented Feb 16, 2018 •

edited

Loading

akutz commented Feb 16, 2018 •

edited

Loading

saad-ali commented Feb 22, 2018 •

edited

Loading

saad-ali commented Feb 22, 2018 •

edited

Loading