Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding the skeleton for versioned APIs, both servers and clients #7

Merged
merged 3 commits into from Sep 20, 2019

Conversation

@wk8
Copy link
Contributor

commented Sep 11, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:

This patch proposes a generic framework to be able to easily define, maintain
and extend versioned API groups going forward.

The whole idea is documented in api/README.md; reviewers should read that
file before reviewing the rest of the PR.

Please note that all *_generated.go files in this PR are meant to be
auto-generated by a tool based on gengo, but that tool will only be written
after this patch's approach is accepted and this PR merged; in particular
*_generated.go files in this PR should be reviewed.

With integration tests with a dummy API group, across 3 versions, with breaking
changes in conversions from one version to the next.

Also some unit tests where relevant.

Special notes for your reviewer:

Please make sure to read api/README.md first (https://github.com/wk8/csi-proxy/blob/wk8/server_scaffolding/api/README.md)

Does this PR introduce a user-facing change?:

NONE
@k8s-ci-robot k8s-ci-robot requested review from ddebroy and msau42 Sep 11, 2019
@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch from 47c7a9a to 1b8723e Sep 11, 2019
Copy link
Contributor

left a comment

The overall API versioning and dispatching approach (inspired by the Kubernetes API approach) makes a lot of sense and LGTM. A couple of comments around removal/deprecation.

api/README.md Outdated Show resolved Hide resolved
api/README.md Outdated Show resolved Hide resolved
@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch from 1b8723e to 7c43a66 Sep 11, 2019
@ddebroy

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

/assign @jingxu97 @msau42 @peterhornyack

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@ddebroy: GitHub didn't allow me to assign the following users: peterhornyack.

Note that only kubernetes-csi members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @jingxu97 @msau42 @peterhornyack

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch from 7c43a66 to 1509289 Sep 11, 2019
Copy link

left a comment

I appreciate the thorough test coverage, thanks!

* create clients to talk to that API group & versioned
is then generated automatically using [gengo](https://github.com/kubernetes/gengo).

The only caveat is that when conversions cannot be made trivially (e.g. when fields from internal and versioned `struct`s have different types), API devs need to define conversion functions. They can do that by creating an (otherwise optional) `internal/server/<api_group_name>/internal/<version>/conversion.go` file, containing functions of the form `func convert_pb_<Type>_To_internal_<Type>(in *pb.<Type>, out *internal.<Type>) error` or `func convert_internal_<Type>_To_pb_<Type>(in *internal.<Type>, out *pb.<Type>) error`; for example, in our `dummy` example above, we need to define a conversion function to account for the different fields in requests and responses from `v1alpha1` to `v1`; so `internal/server/dummy/internal/v1alpha1/conversion.go` could look like:

This comment has been minimized.

Copy link
@pjh

pjh Sep 12, 2019

Everything above was intuitive to me until I got here. Is there an alternative to the converting scheme described below?

Could there just be a ComputeDouble_v1alpha1 func and a ComputeDouble_v1 func and the RPC would get directed to the right one depending on which version the client is using? I'm guessing there's a reason this is not possible or ideal but it's not clear to me.

Is it because this would require a separate server for each version of the API group? Would that be a big deal?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 12, 2019

Author Contributor

@pjh : the whole idea is indeed to have only one generic server per API group that handles all the versions for that API group, as the main reason to ever create a new version for a given API group is expected to say remove a field, or change its type; so having a single place where to handle all versions for a given API call makes re-using code across versions easy.

Besides, that's the same general idea that k8s' API uses, so mimicking that here makes it easier for people used to working on k8s' API to onboard on this repo.

│      ├── conversion_generated.go
│   └── server_generated.go
└── server.go
```

This comment has been minimized.

Copy link
@pjh

pjh Sep 12, 2019

Thanks for including these detailed explanations!

}
}

// Starts starts one GRPC server per API version; it is a blocking call, that returns

This comment has been minimized.

Copy link
@pjh

pjh Sep 12, 2019

"Starts starts" needs rephrasing :)

This comment has been minimized.

Copy link
@wk8

wk8 Sep 12, 2019

Author Contributor

To Starts starts starts? ;) changed, good catch!

Copy link

left a comment

Do you expect to merge this PR or #1 first, or does it not matter either way?

This patch proposes a generic framework to be able to easily define, maintain
and extend versioned API groups going forward.

The whole idea is documented in `api/README.md`; reviewers should read that
file before reviewing the rest of the PR.

Please note that all `*_generated.go` files in this PR are _meant_ to be
auto-generated by a tool based on gengo, but that tool will only be written
after this patch's approach is accepted and this PR merged; in particular
`*_generated.go` files in this PR should be reviewed.

With integration tests with a dummy API group, across 3 versions, with breaking
changes in conversions from one version to the next.

Also some unit tests where relevant.

Signed-off-by: Jean Rouge <rougej+github@gmail.com>
@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch from 1509289 to 37b62bf Sep 12, 2019
@wk8

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@pjh thanks for the review, replied/amended.

Do you expect to merge this PR or #1 first, or does it not matter either way?

I'd like to get this one in first, so that I can then re-factor #1 to abide by the rules this PR sets forth for API proto files.

@pjh

This comment has been minimized.

Copy link

commented Sep 13, 2019

LGTM but will let others give official lgtm/approval.

@ddebroy ddebroy added this to In progress in Windows: Alpha Sep 16, 2019

Simply create a new `api/<api_group_name>/<version>/api.proto` file, defining your new service; then generate the Go protobuf code, and run the CSI-proxy generator to generate all the boilerplate code.

FIXME: add more details on which commands to run, and which files to edit when done generating.

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Sep 17, 2019

you plan to add this after PR is get approved?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Yes indeed. That will depend on the shape of the generator, and I'd rather get this approved before getting too far on writing it.


### How to make changes to an existing API group & version

Changes to existing APIs are only acceptable as long as they are backward compatible; where compatibility is understood here much [as k8s API guidelines define it](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility):

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Contributor

I know K8s APIs allow things like adding new fields without bumping the version, but that ends up adding one more version dependency, ie "I support 1.15 version of v1 API". I had a conversation with liggitt a while ago about CRD versioning, and he recommended that we always bump the version when we make any change to the API to remove any ambiguity.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

That is indeed a fair point, thank you for bringing it up! Changing.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Thinking a bit more about this, that would also mean bumping versions at an alarming rate. How about we simply choose to make stable versions (ie alpha and beta) immutable once they're promoted, but alpha and beta versions simply have to stay backward compatible with themselves (ie adding fields to a non-stable version doesn't require bumping to a new version altogether)?

Otherwise, every time there is any change to an API, that would require creating a new version for it, and keeping it around for quite a while (as per the deprecation policy), and we'd fast end up with a lot of versions. And although the overhead to having each version is rather low, it still adds up: one named pipe per version, plus the need to add conversion functions for all lower versions when making a breaking change that requires a conversion.

If you agree with that, I will simply change the beginning of this section to read:

Stable API versions (i.e. not alpha nor beta) are considered immutable, and cannot be changed.

Changes to existing alpha or beta API versions are only acceptable [...]

This comment has been minimized.

Copy link
@msau42

msau42 Sep 19, 2019

Contributor

I would expect most of the changes to happen during alpha, and much fewer once beta or GA. And for alpha it doesn't make a difference either way since it can change release to release. I think it's much more simpler/obvious from a client's perspective to be explicit about the api version and proxy version compatibility ("If I use v1alpha3, I know it's supported by proxy version X, Y", instead of "If I use this function/field from v1alpha1, I need to use it with proxy version X")

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

I think it's much more simpler/obvious from a client's perspective to be explicit about the api version and proxy version compatibility ("If I use v1alpha3, I know it's supported by proxy version X, Y", instead of "If I use this function/field from v1alpha1, I need to use it with proxy version X")

Absolutely, totally agree. My main concern here is simply to ensure that we don't end up maintaining an unreasonable number of API versions at any one time.

And for alpha it doesn't make a difference either way since it can change release to release.

So are you proposing that alpha APIs offer no guarantees whatsoever, not even backward compatibility with themselves, but beta and GA APIs are immutable? If so, we could even get rid of beta versions altogether, since they don't really have much purpose?

This comment has been minimized.

Copy link
@msau42

msau42 Sep 19, 2019

Contributor

I'm thinking it's simpler to say they are all immutable, but whether or not the proxy needs to carry support for it depends on the designation:

  • alpha: 0 releases
  • beta: 9 months or 3 releases
  • ga: 12 months or 3 releases

What this implies is that alpha is where most of the iteration and testing should go, and we should only promote to beta once we have confidence in the api. Even once an api is promoted to beta or GA, you can add more features with a new alpha version and have that go through beta, GA cycles again.

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 19, 2019

Contributor

Agree with the above suggestion. During alpha, as the API churns, the API will go through vXalpha1 -> vXalpha2 -> ... -> vXalphaN. Once vXbeta1 gets cut, churn will be much less and we should have at most vXbeta1/vXbeta2 and finally the stable vX.

All the Alpha named pipes + API versions can thus be dropped frequently at every release and prevent an increasing build-up. The limited number of beta pipes + API versions will be carried for a limited amount of time and so on.

Once a new Alpha for vX+1 is started, unpromoted APIs will need to be carried over manually by maintainers from vXalphaN if they were not ready to be promoted to vXbeta1.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Changed the wording :)

> 1. 12 months or 3 releases (whichever is longer) if the API member is part of a Stable/vN version.
> 2. 9 months or 3 releases (whichever is longer) if the API member is part of a Beta/vNbeta1 version.
> 3. 0 releases if the API member is part of an Alpha/vNalpha1 version.
> To continue running CSI Node Plugins that depend on an old version of csi-proxy.exe, vN, some of whose members have been removed, Kubernetes administrators will be required to run the latest version of the csi-proxy.exe (that will be used by CSI Node Plugins that use versions of CSIProxyService more recent than vN) along with an old version of csi-proxy.exe that does support vN.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Contributor

Can you clarify here are we talking about the version of csi-proxy.exe, or the csi proxy API? My understanding here is that a single binary csi-proxy.exe will need to support multiple API versions, including deprecated ones until we reach the end of the deprecation period.

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 18, 2019

Contributor

In the vast majority of scenarios, we expect a single version of csi-proxy.exe to be running that supports the necessary API versions for the CSI node plugins running on a node. We expect this to be the case for the first few releases.

The above bit about multiple versions of csi-proxy.exe is geared towards a remote scenario in the future where an operator for some reason wants to keep running a really old CSI driver (that requires an API version that is no longer supported by the current version of csi-proxy.exe at that point in time) along with a modern CSI driver (that requires an API version that is only supported by current version of csi-proxy.exe at that point in time).

We can explain the above in some more details and emphasize that such a situation should be very rare.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 18, 2019

Contributor

Yeah I think distinguishing between the two would be good: what the supported case is vs unsupported but if you really want to this is what you could do

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Good point, clarifying.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

(also note for later: running 2 csi-proxy.exe at the same time will require small tweaks on named pipes paths)

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 19, 2019

Contributor

In the KEP we proposed a probe and back-off mechanism to enable side-by-side execution (when necessary):

For every version of the API supported, csi-proxy.exe will first probe for and then expose a \\.\pipe... If during the initial
probe phase, csi-proxy.exe determines that another process is already listening on a \\.\pipe\csi-proxy-v[N] named pipe, it will not try to create and listen on that named pipe. This allows multiple versions of csi-proxy.exe to run side-by-side if absolutely necessary to support multiple CSI Node Plugins that require widely different versions of CSIProxyService that no single version of csi-proxy.exe can support.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Oh, missed that part. As of right now the server will crash if it can't create all the pipes it needs.

I'm not sure I love this though, because that has the potential to hide problems from admins. You want to know when your service can't listen on what it's supposed to be listening on.

How about having a flag for the server --enabled-apis <group>-<version>,<group>-<version>,... instead?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

Sounds good. More deterministic that way. Only the old/unsupported versions will use the above parameters, correct?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

It'll be an optional flag that you can pass to csi-proxy.exe. By default, all API groups/versions will be enabled.

Created #9


#### A field in an API object

Mark it as deprecated in the `proto` file, e.g.:

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Contributor

To clarify, all of these types of deprecation results in deprecating the entire api version right? Because in order to actual remove a field or proc, you need to serve a new api version, and remove the current one.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

I don't think that deprecating a single field, or a single procedure, necessarily means deprecating the entire API version. There's no strong reason to deprecate API versions as fast as possible, and adding a new one shouldn't necessarily mean deprecating all the previous versions for that group. And deprecation doesn't mean immediate removal either.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 19, 2019

Contributor

I guess what I'm wondering about is to actually remove support for a field or function entirely, we would have to deprecate and eventually remove the entire API version (and replace it with a new api version that doesn't have it)

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Absolutely; deprecating a single field or procedure is simply meant to let users know that this field or procedure will be removed in a subsequent version. It doesn't change anything to the behaviour of the version it's applied to.


## Detailed breakdown of generated files

This section details how `csi-proxy-gen` works, and what files it generates; `csi-proxy-gen` is built on top of [gengo](https://github.com/kubernetes/gengo), and re-uses part of [k8s' code-generator](https://github.com/kubernetes/code-generator), notably to generate conversion functions.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Contributor

Do you see any risk of depending on these tools that were mainly designed for K8s APIs? If changes are made in those tools that break our generation, will we need to fork them?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

That is indeed a very valid concern. The generator will mainly be based on gengo, whose purpose is well defined, and quite separate from k8s, so I think we're in the clear there.

As to code-generator, the only part that we're interested in is from https://github.com/kubernetes/code-generator/blob/master/cmd/conversion-gen/generators/conversion.go, namely comparing 2 structs, finding the fields with the same names and types in there, and generating efficient conversion code from one to the other. The plan is to make a PR on code-generator to extract the relevant pieces of code from conversion.go into a new package, and with extensive unit tests, and then use only that package in our own converter. Since that new package would have a well defined purpose, it should hopefully stay stable enough that we can rely on it without too much hassle going forward.


Deprecate and remove all its versions as explained in the previous version; then remove the entire `api/<api_group_name>` directory, and run `csi-proxy-gen`, it will remove all references to the removed API group.

## Detailed breakdown of generated files

This comment has been minimized.

Copy link
@msau42

msau42 Sep 17, 2019

Contributor

Can you also add a section on exporting client files? One problem we have with the snapshot APIs + dep, is that users that want the client libraries need to import the external-snapshotter repo, which also contains the server side dependencies, which often include k/k libraries. For the user, dependency management is difficult because now they have dependencies on kubernetes and have to potentially deal with conflicting libraries that may also have transient dependencies on kubernetes.

I'm not sure if go modules can solve this problem, ie exporting the client library as a module. Can we investigate this? Otherwise, we may want to go a similar route with how k/k does it by having a staging directory and then mirroring it to another repo.

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 18, 2019

Contributor

Good point. In a quick prototype of csi-proxy, I placed the API Go files under a separate package: https://github.com/ddebroy/csi-proxy/tree/master/pkg from the server installation.

The dependency solver was able to import in just the API pkg (v1alpha1) from the repo when trying it out with gce-pd: ddebroy/gcp-compute-persistent-disk-csi-driver@e129ffd#diff-bd247e83efc3c45ae9e8c47233249f18R52

This allowed the vendor repo to be pretty clean on the client repo (gce-pd in this case): https://github.com/ddebroy/gcp-compute-persistent-disk-csi-driver/tree/win-csi1/vendor/github.com/ddebroy/csi-proxy

@wk8 if the above just works with the .proto + generated pb.go files corresponding to the APIs, we can call it out here.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 18, 2019

Contributor

Took a quick look at your client api files, it looks like it doesn't depend on any kubernetes imports so maybe it's fine.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Digging more into this, it does seem that this might indeed be a problem. Importing any package from a module does import all of that module's dependencies, even if there are not used for the one package you import.

I'm not so worried about k8s imports, as this repo should never import anything from k8s, but it could indeed be a problem anyway: it already has a number of dependencies, there will most likely be many more to come, and ideally we'd like clients to be able to import what they need without getting all the deps with it.

Going to try having 3 separate modules: one under /api, one under /client, and one for the rest; clients would just need to import the 1st two afterwards.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Added commit 5d2a22b in which everything that clients need to import is broken into its own go module.

Namely one containing everything needed for clients to import into
repositories using this one; and then another go module for all the
rest.

See more at #7 (comment)

Signed-off-by: Jean Rouge <rougej+github@gmail.com>
@wk8

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@msau42 @ddebroy : amended/replied, thanks for the reviews! :)

client/api/README.md Show resolved Hide resolved

FIXME: add more details on which commands to run, and which files to edit when done generating.

### How to make changes to an existing API group & version

Changes to existing APIs are only acceptable as long as they are backward compatible; where compatibility is understood here much [as k8s API guidelines define it](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility):
Beta and GA (stable) APIs are immutables. Changes to alpha APIs are only acceptable as long as they are backward compatible; where compatibility is understood here much [as k8s API guidelines define it](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility):

This comment has been minimized.

Copy link
@msau42

msau42 Sep 19, 2019

Contributor

typo: immutable.

I would actually make alpha apis immutable too. So we will be revving alpha versions very quickly, but the proxy only needs to support one version. It makes it much more explicit which version of the proxy your driver is compatible with.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

👍

@@ -178,6 +178,8 @@ From the CSI [proxy KEP](https://github.com/kubernetes/enhancements/blob/master/
> 3. 0 releases if the API member is part of an Alpha/vNalpha1 version.
> To continue running CSI Node Plugins that depend on an old version of csi-proxy.exe, vN, some of whose members have been removed, Kubernetes administrators will be required to run the latest version of the csi-proxy.exe (that will be used by CSI Node Plugins that use versions of CSIProxyService more recent than vN) along with an old version of csi-proxy.exe that does support vN.
A given version of csi-proxy.exe will support multiple API versions, according to the deprecation schedule above. Running more than one version of csi-proxy.exe at one time is not supported yet.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 19, 2019

Contributor

This conflicts with the sentence above, which talks about running multiple versions of the proxy

This comment has been minimized.

Copy link
@wk8

wk8 Sep 19, 2019

Author Contributor

Indeed... Removed!

@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch from 7c1b463 to ae8d814 Sep 19, 2019
@msau42

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

Did you upload a new version?

@wk8

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

Yes :) ?


Existing API versions are immutable.

Beta and GA (stable) APIs are immutable. Changes to alpha APIs are only acceptable as long as they are backward compatible; where compatibility is understood here much [as k8s API guidelines define it](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#on-compatibility):

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Contributor

I think alpha should be immutable too. Then we can remove the rest of this section. @ddebroy seems to agree too

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

I'm sorry, I amended the first sentence, and then forgot to remove this paragraph :/

> 3. 0 releases if the API member is part of an Alpha/vNalpha1 version.
> To continue running CSI Node Plugins that depend on an old version of csi-proxy.exe, vN, some of whose members have been removed, Kubernetes administrators will be required to run the latest version of the csi-proxy.exe (that will be used by CSI Node Plugins that use versions of CSIProxyService more recent than vN) along with an old version of csi-proxy.exe that does support vN.
A given version of csi-proxy.exe will support multiple API versions, according to the deprecation schedule above.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Contributor

I think this still conflicts with the sentence above (I think we should modify or clarify the above sentence). Previous sentence says "you may need to run multiple versions of the proxy", and then this sentence says "1 proxy may support multiple versions"

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

Previous sentence says "you may need to run multiple versions of the proxy"

If you need old API versions

"1 proxy may support multiple versions"

It will, the last k versions, as required by the deprecation schedule.

Tried to reword it to make it clearer, hopefully that's better.

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Contributor

Maybe we should not even mention the case of running older versions of csi-proxy. It's not something we want to support anyway.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

I'd think that we do want to support that, when really needed (#7 (comment)), so it can't hurt to at least brush on it?

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Contributor

I'm actually not sure if we really want to officially support things that are out of our deprecation/support policy. It adds a lot of burden on us to validate and test. @ddebroy wdyt?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

I think we do not need to officially support old versions of csi-proxy.exe. So I am okay with removing the text above from the README covering multiple versions/instances of csi-proxy.exe side-by-side as a supported option. I agree this allows for a simpler validation and test matrix.

The approach described in #7 (comment) allows someone who wishes to run an old, unsupported version of csi-proxy for some reason to do so (side-by-side with a supported version of csi-proxy). As long as that ability is not explicitly blocked in the implementation, I am fine.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

Removed

@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch 2 times, most recently from f78691a to 76e59a9 Sep 20, 2019
Copy link
Contributor

left a comment

Looking pretty good. A bunch of README nits to improve the readability a bit. Two questions on the auto generated converters for Doubles in FileSystem.


This separate go module is the one intended to be imported by clients that want to use the CSI-proxy.

It should strive to keep as many dependencies as possible, to make it easy to import in other repositories.

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

nit: I think you mean "as few dependencies" and not "as many dependencies", right?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

... :(

@@ -0,0 +1,5 @@
# Client go module

This separate go module is the one intended to be imported by clients that want to use the CSI-proxy.

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

nit: maybe you can remove "separate" and "the one" from the sentence - "This go module is intended to be imported by clients ..."

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

👍


APIs are defined by protobuf files; each API group should live in its own directory under `client/api/<api_group_name>` in this repo's root (e.g. `client/api/iscsi`), and then define each of its version in `client/api/<api_group_name>/<version>/api.proto` files (e.g. `client/api/iscsi/v1/api.proto`). Each `proto` file should define exactly one RPC service.

Internally, there is only one internal server `struct` per API group, that handles all the versions for that API group. That server is defined in this repo's `internal/server/<api_group_name>` (e.g. `internal/server/iscsi`) go package. This go package should follow the following pattern:

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

How about removing "internal" - "Internally, there is only one server struct per API group ..."

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

👍

* convert versioned requests to internal representations
* and then internal responses back to versioned responses
* create clients to talk to that API group & versioned
is then generated automatically using [gengo](https://github.com/kubernetes/gengo).

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

nit: remove "then" => "is generated automatically ..."

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

👍

All the boilerplate code to:
* add a named pipe to the server for each version of the API group, listening for its version's requests, and replying with its version's responses
* convert versioned requests to internal representations
* and then internal responses back to versioned responses

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

nit: how about "convert internal responses back to versioned responses" instead of "and then internal ..."

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

👍

}
```
All the boilerplate code to:
* add a named pipe to the server for each version of the API group, listening for its version's requests, and replying with its version's responses

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

how about: "listening for its version's requests" => "listen for each version's requests"
and: "replying with it's version's responses" => "reply with each version's responses"

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

👍


FIXME: add more details on which commands to run, and which files to edit when done generating.

### No changes to existing API group & version

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

how about moving this bullet to a sentence under "## How to change the API" ?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

👍


### How to add a new version to an existing API group

Any change that cannot be done in a compatible way requires creating a new API version.

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

"compatible" is slightly unclear. May be word it as "Any changes to the API of an existing API group requires creating a new API version"?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

👍

"github.com/kubernetes-csi/csi-proxy/internal/server/filesystem/internal"
)

func autoconvert_pb_ComputeDoubleRequest_To_internal_ComputeDoubleRequest(in *pb.PathExistsRequest, out *internal.PathExistsRequest) error {

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

Is this expected? The ComputeDouble stuff is from the dummy API group. They should not show up under the filesystem API group, correct?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

And that's why humans are bad at "auto-generating" code ;) good catch!

return autoconvert_pb_ComputeDoubleRequest_To_internal_ComputeDoubleRequest(in, out)
}

func autoconvert_internal_ComputeDoubleResponse_To_pb_ComputeDoubleResponse(in *internal.PathExistsResponse, out *pb.PathExistsResponse) error {

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

Is this expected? The ComputeDouble stuff is from the dummy API group. They should not show up under the filesystem API group, correct?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

Same :)

@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch from 76e59a9 to ee504a4 Sep 20, 2019
@wk8

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

@ddebroy @msau42 : thanks for the thorough reviews, addressed your comments.

Copy link
Contributor

left a comment

/approve

I only reviewed the README.md. Will let @ddebroy and @jingxu97 review the rest. Thanks so much, this is looking great!

@@ -0,0 +1,5 @@
# Client go module

This comment has been minimized.

Copy link
@msau42

msau42 Sep 20, 2019

Contributor

A top level README.md with instructions to users on how to import this go module would be great too! Can be done as a separate PR once there's actually something to import.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, wk8

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

```go
type Server struct{}
func (s *Server) ComputeDouble(ctx context.Context, request *internal.ComputeDoubleRequest, version apiversion.Version) (*internal.ComputeDoubleResponse, error) {

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Sep 20, 2019

The parameter of "version" is not used in the function?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

Yes. That's required to fulfill the auto-generated interface that the generated versioned servers expect (https://github.com/kubernetes-csi/csi-proxy/pull/7/files#diff-fed01b95f0f43874e57816bfe316b1f7R18).

The idea here is that API devs can use that to make adjustments based on the API version the request was made against. The preferred way to handle that would be through custom conversion functions though.

i := in.Response
if i > math.MaxInt32 || i < math.MinInt64 {
// overflow
out.Overflow = true

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Sep 20, 2019

so for the response, overflow is included?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

Good catch, it shouldn't. Sorry. Fixed. Returns an error now, as it should.

@@ -0,0 +1,26 @@
cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Sep 20, 2019

this file is automatically generated?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

Yes. It's the go mod's lock file, automatically generated when using GO111MODULE=on

@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch from ee504a4 to dbed47b Sep 20, 2019
Copy link
Contributor

left a comment

Couple more nits. Otherwise LGTM at this point.

1. define it its own new `api.proto` file
2. generate the Go protobuf code
3. update the API group's internal representations (in its `types.go` file) to be able to represent all of the group's version (the new and the old ones)
3. add any needed conversion functions for all existing versions of this API group to account for the changes made at the previous step

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

Check the bullet numbering - 3 is repeated.

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

👍

connection *grpc.ClientConn
}

// NewClient returns a client to make calls to the dummy API group version v1.

This comment has been minimized.

Copy link
@ddebroy

ddebroy Sep 20, 2019

Contributor

comment nits:
"dummy API group" => "Filesystem API group"
"version v1" => "version v1alpha1"

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

Manual auto generation is hard ;)


qualifier := stable
var qualifierVersion uint
if len(matches) >= 4 && matches[2] != "" {

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Sep 20, 2019

is it possible len(matches) > 4?

This comment has been minimized.

Copy link
@wk8

wk8 Sep 20, 2019

Author Contributor

Nope. This could be == 4.

Signed-off-by: Jean Rouge <rougej+github@gmail.com>
@wk8 wk8 force-pushed the wk8:wk8/server_scaffolding branch from dbed47b to b9eac70 Sep 20, 2019
@ddebroy

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

/lgtm

@ddebroy

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

/hold

@k8s-ci-robot k8s-ci-robot merged commit 15613db into kubernetes-csi:master Sep 20, 2019
2 of 3 checks passed
2 of 3 checks passed
tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation wk8 authorized
Details
pull-kubernetes-csi-csi-proxy Job succeeded.
Details
Windows: Alpha automation moved this from In progress to Done Sep 20, 2019
@ddebroy

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

tried to hold to request a squash of the "review comments" PR but that was not quick enough :-( oh well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.