Adding the API generator #12

wk8 · 2019-10-08T01:37:03Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This patch adds the API generator that generates all the boilerplate files the API needs, as described in
https://github.com/kubernetes-csi/csi-proxy/tree/master/client/api#detailed-breakdown-of-generated-files

Added a few tests, though the main test will be checking that auto-generated files are in sync with the rest of the code & proto files; that will be done in automated builds in a future patch (very soon).

Which issue(s) this PR fixes:

Fixes #8

Does this PR introduce a user-facing change?:

Added a code generator to generate boilerplate files for the API

ddebroy

This is looking great! Just a few comments.

ddebroy · 2019-10-10T06:02:40Z

client/api/README.md

 CSI-proxy's API is a GRPC, versioned API.

-The server exposes a number of API groups, all independent of each other. Additionally, each API group has one or more versions. Each version in each group listens for GRPC messages on a Windows named pipe of the form `\\.\\pipe\\csi-proxy-<api_group_name>-<version>` (e.g. `\\.\\pipe\\csi-proxy-filesystem-v2alpha1` or `\\.\\pipe\\csi-proxy-iscsi-v1`).
+The server exposes a number of API groups, all independent of each other. Additionally, each API group has one or more versions. Each version in each group listens for GRPC messages on a Windows named pipe of the form `\\.\\pipe\\csi-proxy-<api_group_name>-<version>` (e.g. `\\.\\pipe\\csi-proxy-file_system-v2alpha1` or `\\.\\pipe\\csi-proxy-iscsi-v1`).


How about keeping the API groups single word as much as possible - like filesystem, volume, disk, smb and iscsi?

FileSystem seemed cleaner than Filesystem in the places where we need camel case, so that's why I changed this - but happy to change back if you prefer.

My preference would be filesystem.

ddebroy · 2019-10-10T06:10:19Z

client/utils.go

 	// CsiProxyNamedPipePrefix is the prefix for the named pipes the proxy creates.
 	// The suffix will be the API group and version,
-	// e.g. "\\.\\pipe\\csi-proxy-iscsi-v1", "\\.\\pipe\\csi-proxy-filesystem-v2alpha1", etc.
+	// e.g. "\\.\\pipe\\csi-proxy-iscsi-v1", "\\.\\pipe\\csi-proxy-file_system-v2alpha1", etc.


Let's keep this filesystem

ddebroy · 2019-10-10T06:12:51Z

cmd/csi-api-gen/generators/api.go

+
+const (
+	// csiProxyRootPath is the CSI-proxy go library's root path.
+	csiProxyRootPath = "github.com/kubernetes-csi/csi-proxy"


Will it be possible to make csiProxyRootPath configurable through a parameter? Can be done in a separate PR later.

Sure, though I'm not sure what the use case would be?

I was thinking it will make name changes easier to handle in the future. Looks like k/k hardcodes this as well in their generators: https://github.com/kubernetes/kubernetes/blob/a3ccea9d8743f2ff82e41b6c2af6dc2c41dc7b10/staging/src/k8s.io/code-generator/cmd/conversion-gen/generators/conversion.go#L439-L440. So fine for now.

But even then, that parameter presumably would have to be hardcoded somewhere in this repo? :)

Keeping as is for now anyway, as agreed.

ddebroy · 2019-10-10T06:27:05Z

cmd/csi-api-gen/generators/api.go

+	tagName   = "csi-proxy-gen"
+
+	// headerComment is the comment on the first line of every generated file.
+	headerComment = "// Code generated by csi-api-gen. DO NOT EDIT.\n\n"


How about calling the utility csi-proxy-api-gen? It is not generating CSI APIs, so csi-api-gen does not sound right.

Fair enough, changing :)

ddebroy · 2019-10-10T06:28:47Z

cmd/csi-api-gen/generators/api.go

+	// * +csi-proxy-gen=clientBasePkg:<pkg_path> to set the base output directory
+	//		for generated client files - defaults to github.com/kubernetes-csi/csi-proxy/client/groups
+	tagMarker = "+"
+	tagName   = "csi-proxy-gen"


Based on comment below, I think this (as well as the comments above) should read csi-proxy-api-gen.

ddebroy · 2019-10-10T06:49:21Z

integrationtests/apigroups/server/dummy/internal/v1/conversion_generated.go

 	unsafe "unsafe"

-	pb "github.com/kubernetes-csi/csi-proxy/integrationtests/apigroups/api/dummy/v1"
+	v1 "github.com/kubernetes-csi/csi-proxy/integrationtests/apigroups/api/dummy/v1"


The v1 in the alias seems unnecessary given the structure of the imports.

It is, but that's what what gengo spits out - not sure why. Would need to go look why it does that, and change it; or just leave it as it - generated code does tend to have some quirks?

I think it is okay for now.

ddebroy · 2019-10-10T06:54:28Z

integrationtests/apigroups/server/dummy/internal/v1/conversion_generated.go

+	return nil
+}
+
+// Convert_internal_ComputeDoubleRequest_To_v1_ComputeDoubleRequest is an autogenerated conversion function.


Is Convert_internal_*Request_To_vX_*Request here mainly for completeness? I assume this conversion path will never be invoked i.e. in the Request path, Convert_vX_*Request_To_internal_*Request is what is mainly needed.

You're absolutely right.

However, I couldn't think of a great way to tell the generic conversion generator which conversion functions you actually cared about. Best I could think of would be registering a callback with it to check if a given conversion pair is of interest - but it does add a decent amount of complexity for not much again.

If you feel strongly against having unused funcs though, can work it into code-generator.

I think their presence is fine. I was wondering if they will be invoked in any scenario.

ddebroy · 2019-10-10T06:59:59Z

integrationtests/apigroups/server/dummy/internal/v1/conversion_generated.go

-func convert_internal_ComputeDoubleResponse_To_pb_ComputeDoubleResponse(in *internal.ComputeDoubleResponse, out *pb.ComputeDoubleResponse) error {
-	return autoconvert_internal_ComputeDoubleResponse_To_pb_ComputeDoubleResponse(in, out)
+// Convert_v1_ComputeDoubleResponse_To_internal_ComputeDoubleResponse is an autogenerated conversion function.
+func Convert_v1_ComputeDoubleResponse_To_internal_ComputeDoubleResponse(in *v1.ComputeDoubleResponse, out *internal.ComputeDoubleResponse) error {


Is Convert_vX_*Response_To_internal_*Response here mainly for completeness? I assume this conversion path will never be invoked i.e. in the Response path, Convert_internal_*Response_To_vX_*Response is what is mainly needed.

Same answer :)

ddebroy · 2019-10-11T22:17:35Z

cc @jingxu97 @peterhornyak

jingxu97 · 2019-10-11T23:08:02Z

cmd/csi-api-gen/generators/api_group_generated_gen.go

@@ -0,0 +1,74 @@
+package generators


is this file auto generated?

No. It contains the generator that generates api_group_generated.go files, hence the name.

wk8 · 2019-10-14T23:48:17Z

@ddebroy : amended as requested, PTAL, thanks! :)

jingxu97 · 2019-10-15T19:00:22Z

cmd/csi-api-gen/generators/groups_and_versions.go

+	}
+}
+
+// isError returns true iff type t is the built-in type "error"


maybe spell out iff to if and only if

jingxu97 · 2019-10-15T19:01:25Z

cmd/csi-api-gen/generators/groups_and_versions.go

+
+// isError returns true iff type t is the built-in type "error"
+func isError(t *types.Type) bool {
+	return t.Kind == types.Interface && t.Name.Name == "error" && t.Name.Package == ""


The t.Name.Package must be empty string? What does it mean if it is not empty?

It means that the type belongs to a package; that's not the case for built-in types.

does it make sense to name it IsBuildinError to be more specific?

Absolutely, changed, thanks :)

jingxu97 · 2019-10-15T19:16:42Z

cmd/csi-api-gen/generators/api.go

+
+// buildAPIGroupDefinitionFromDocComment looks for a +csi-proxy-api-gen comment in the package's
+// doc.go file, and if it finds one build the corresponding API definition.
+func buildAPIGroupDefinitionFromDocComment(pkgPath string, pkg *types.Package, groups map[string]*groupDefinition) bool {


the passing pkgPath is just for logging purpose in the function?

Just for logging.

jingxu97 · 2019-10-15T19:18:21Z

cmd/csi-api-gen/generators/api.go

+		}
+	}
+
+	klog.V(5).Infof("Found API group %q", definition.name)


Here we assume we always find one api group? Is that possible that the returned commentTags are all just empty string? Is that possible that name or value is empty string? Is that valid?

If there is a csi-proxy-api-gen tag in a package's doc.go file, then it must be an API group, regardless of which additional custom options it has (or doesn't have). So yes, past the

if len(commentTags) == 0 { return false }

check on line 160, we know we've found an API group.

jingxu97 · 2019-10-15T19:21:36Z

cmd/csi-api-gen/generators/api.go

+			continue
+		}
+
+		if buildAPIGroupDefinitionFromDocComment(pkgPath, pkg, groups) {


so it is not possible to have group definition in both subdirectories of client/api and doc.go file ?

It's absolutely possible. If a group definition is in the canonical client/api/ location and has a doc.go file with csi-proxy-api-gen tags, then it's processed according to the tags' options (and if there are no custom options then the tag is redundant with it being in the canonical location)

I am trying to check the continue logic here means if buildAPIGroupDefinitionFromDocComment returns true, the rest will be skipped?

Yes, it will, since the groupDefinition for it will already have been built.

jingxu97 · 2019-10-15T19:29:44Z

cmd/csi-api-gen/generators/api.go

+func packages(context *generator.Context, arguments *args.GeneratorArgs) (pkgs generator.Packages) {
+	// find API group definitions
+	groups := findAPIGroupDefinitions(context)
+	klog.V(5).Infof("Found API groups: %v", groups)


all your log level is 5 or above?

It's all debugging logging.

jingxu97 · 2019-10-15T19:31:35Z

cmd/csi-api-gen/generators/api.go

+	for _, filePath := range toRemove {
+		filePath = path.Join(outputBase, filePath)
+		if err := os.RemoveAll(filePath); err != nil {
+			klog.Fatalf("unable to remove %q: %v", filePath, err)


do we want to retry or just fail immediately?

I'm not sure what error modes could justify retrying later?

Also, the general philosophy here (and also for all of k8s' code generation, as far as I can tell) is that since this is not a service, and is just meant to be run manually (or in tests) from time to time, it's okay to fail early.

jingxu97 · 2019-10-15T19:34:30Z

cmd/csi-api-gen/generators/api.go

+		removeGeneratedFiles(group, arguments.OutputBase)
+		pkgs = append(pkgs, packagesForGroup(group, arguments.OutputBase)...)
+	}
+	return


So it just returns nothing?

Nope, it's returning pkgs

jingxu97 · 2019-10-15T22:24:29Z

integrationtests/csi_api_gen_test.go

+// are up-to-date (i.e. consistent with the current generator).
+
+// TestNewAPIGroup tests that bootstraping a new group works as intended.
+func TestNewAPIGroup(t *testing.T) {


This test case is having a doc.go? Maybe it is not that important, just want to check is there other test cases that are interesting to us?

Yes it does have one. Testing API groups from canonical locations will be done by re-generating all auto-generated files as part of all builds.

ddebroy

Can we name the cmd tool directory (cmd/csi-api-gen/) csi-proxy-api-gen as well please? Thanks for updating the name elsewhere in the code/comments.

ddebroy

Just a few nits. Otherwise looking good.

ddebroy · 2019-10-16T00:51:00Z

cmd/csi-api-gen/main.go

@@ -0,0 +1,11 @@
+package main


Can we name the cmd tool directory (cmd/csi-api-gen/ above) csi-proxy-api-gen as well please? Thanks for updating the name elsewhere in the code/comments.

ddebroy · 2019-10-16T01:03:13Z

cmd/csi-api-gen/generators/api.go

+						if err := os.MkdirAll(path.Dir(docFilePath), os.ModePerm); err != nil {
+							klog.Fatalf("Unable to create directory %q: %v", path.Dir(docFilePath), err)
+						}
+						contents := fmt.Sprintf("// Temporary code generated by conversion-gen. DO NOT COMMIT.\n// Should be removed automatically, but please remove manually otherwise.\n\npackage %s\n", vsn.Name)


Should this say conversion-gen or csi-proxy-api-gen?

ddebroy · 2019-10-16T01:11:51Z

cmd/csi-api-gen/generators/groups_and_versions.go

+// validateServerCallback checks that server callbacks have the expected shape, i.e.:
+// * all versioned (i.e. in the same package) parameter should be pointers
+// * return values should all be pointers, except for the last one, that must be an error
+// These assumptions are


nit: check the comment sentence

But they... are!

Changed, thanks :)

The API generator generates all the boilerplate files the API needs, as described in https://github.com/kubernetes-csi/csi-proxy/tree/master/client/api#detailed-breakdown-of-generated-files Added a few tests, though the main test will be checking that auto-generated files are in sync with the rest of the code & proto files; that will be done in automated builds in a future patch (very soon). Signed-off-by: Jean Rouge <rougej+github@gmail.com>

ddebroy

/lgtm
/approve

k8s-ci-robot · 2019-10-16T23:57:31Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ddebroy]

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

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 8, 2019

k8s-ci-robot requested review from ddebroy and jingxu97 October 8, 2019 01:37

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 8, 2019

ddebroy reviewed Oct 10, 2019

View reviewed changes

jingxu97 reviewed Oct 11, 2019

View reviewed changes

wk8 force-pushed the wk8/generator branch from 54f5bae to fa50d47 Compare October 14, 2019 23:47

jingxu97 reviewed Oct 15, 2019

View reviewed changes

wk8 force-pushed the wk8/generator branch 3 times, most recently from 7cd45af to c54f852 Compare October 15, 2019 19:57

jingxu97 reviewed Oct 15, 2019

View reviewed changes

ddebroy reviewed Oct 16, 2019

View reviewed changes

wk8 force-pushed the wk8/generator branch from c54f852 to 9beec53 Compare October 16, 2019 23:34

ddebroy reviewed Oct 16, 2019

View reviewed changes

k8s-ci-robot assigned ddebroy Oct 16, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2019

k8s-ci-robot merged commit 28e6078 into kubernetes-csi:master Oct 16, 2019

Adding the API generator #12

Adding the API generator #12

Uh oh!

Conversation

wk8 commented Oct 8, 2019

Uh oh!

ddebroy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddebroy commented Oct 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wk8 commented Oct 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment