Generate composite types #729

spencerhance · 2019-04-16T21:15:56Z

This PR adds generated composite types for many of the services from k8s-cloud-provider using the API spec json. This auto-generates the existing BackendService composite type as well.

The next step will to be to remove the first cloud-provider layer so that all types can be easily generated. That will also depend on: GoogleCloudPlatform/k8s-cloud-provider#9

k8s-ci-robot · 2019-04-16T21:16:05Z

Hi @spencerhance. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

rramkumar1 · 2019-04-17T17:15:45Z

/ok-to-test

rramkumar1

First round of comments: Nice usage of the JSON spec! I never thought of that as an approach but it seems a bit nicer than using reflection.

One general comment: Do you mind doing a diff of (HEAD, PR) of the composite.go and composite_test.go for just the BackendService? I want to make sure there is no difference so the possibility we introduce a subtle bug is as close to zero as possible.

rramkumar1 · 2019-04-17T20:07:51Z

pkg/backends/backends.go

@@ -74,7 +74,7 @@ func (b *Backends) Create(sp utils.ServicePort, hcLink string) (*composite.Backe
 		HealthChecks: []string{hcLink},
 	}
 	ensureDescription(be, &sp)
-	if err := composite.CreateBackendService(be, b.cloud); err != nil {
+	if err := composite.CreateBackendService(be, b.cloud, meta.GlobalKey(be.Name)); err != nil {


Because we wan't to silo your changes for L7 ILB as much as possible to keep the code base stable, I would suggest not introducing the key parameter in this PR. That way these function calls do not change and there is no real chance for a bug to be introduced.

I would leave a TODO in the composite generator code that states this should be added in a follow up.

Done. Also good call, the only major change this PR is introducing is bypassing the first cloud-provider layer and calling the CRUD functions directly
(e.g cloud.Compute().AlphaBackendServices().Insert(meta.GlobalKey()) instead of cloud.CreateAlphaGlobalBackendService().)

With this change, we would be losing metrics until some replacement is implemented.

The reason for that was to try to make it simpler to use global and regional resources with less switches (and easier code generation logic), but I'm definitely open to suggestions.

The only other change I see in the diff is making the TestToX() functions simpler since I don't use known Alpha/Beta fields anymore. It's possible to add those in by doing a diff in the generate code but it would definitely add a bunch more complexity.

pkg/composite/gen/main.go

rramkumar1 · 2019-04-17T20:21:59Z

pkg/composite/gen/main.go

+	gofmt = "gofmt"
+
+	// This assumes that alpha contains a superset of all struct fields
+	apiFilePath = "../../vendor/google.golang.org/api/compute/v0.alpha/compute-api.json"


This should be okay for now I think. Once we migrate to go modules, its possible we may have to do something different. My understanding of go modules is that they remove the need for vendor/ but you can still use it if you want. Anyways, we can cross that bridge when we get there.

hmm good point. Will look into modules.

pkg/composite/gen/main.go

rramkumar1 · 2019-04-17T20:46:35Z

pkg/composite/gen/main.go

+		return nil, fmt.Errorf("error unmarshalling {{$type.Name}} JSON to compute {{$lower}} type: %v", err)
+	}
+
+	{{- if eq $type.Name "BackendService"}}


We will have to think of a better solution for this. It may be fine to just add all fields to ForceSendFields but for now I think this is fine.

Agreed, do you remember why those ForceSendFields for BackendService were added originally?

They were needed because of some subtle behavior with how the GCE API's work. Specifically, if you there is a boolean field (like "IncludeHost") and you are trying to set the field to false, GCE will not recognize that false setting unless you add the field to the ForceSendFields list.

From GCE docs: "By default, fields with empty values are omitted from API requests." (https://godoc.org/google.golang.org/api/compute/v1).

I'm trying to remember why though I needed to add this here rather than have callers set those fields...

spencerhance · 2019-04-17T22:08:21Z

@rramkumar1 Thanks! There's also an additional approach which uses the parsing code found in k8s-cloud-provider, but that would have to be heavily modified or copy-pasted into ingress-gce which seemed worse than not having any dependencies.

Also ready for another round of review

pkg/composite/composite_test.go

pkg/composite/gen/main.go

pkg/composite/composite_test.go

rramkumar1 · 2019-04-18T17:13:09Z

pkg/composite/gen/main.go

+		return nil, fmt.Errorf("error unmarshalling {{$type.Name}} JSON to compute {{$lower}} type: %v", err)
+	}
+
+	{{- if eq $type.Name "BackendService"}}


They were needed because of some subtle behavior with how the GCE API's work. Specifically, if you there is a boolean field (like "IncludeHost") and you are trying to set the field to false, GCE will not recognize that false setting unless you add the field to the ForceSendFields list.

From GCE docs: "By default, fields with empty values are omitted from API requests." (https://godoc.org/google.golang.org/api/compute/v1).

I'm trying to remember why though I needed to add this here rather than have callers set those fields...

pkg/composite/gen/main.go

rramkumar1 · 2019-04-18T17:18:06Z

pkg/composite/composite.go

-			return err
-		}
-		klog.V(3).Infof("Creating alpha backend service %v", alpha.Name)
-		return cloud.CreateAlphaGlobalBackendService(alpha)


My feeling is that we should not remove the usage of the wrapper until we have a solution for keeping the metrics around.

So the two main reasons for removing the wrapper are that it doesn't follow a consistent formula like the generated wrapper, and it's in k/k so it would be potentially harder for me to change it for the PR. https://github.com/kubernetes/kubernetes/tree/master/pkg/cloudprovider/providers/gce.

That being said, I could just limit this PR to doing a drop-in replacement of BackendService and leave the other composite types for another PR once I have a replacement for metrics.

I don't mind that idea. That way, we don't have to block this necessarily on the metrics stuff.

side note, do you know offhand if anything uses those metrics?

spencerhance · 2019-04-18T19:44:37Z

@rramkumar1 Ready for another round

pkg/composite/gen/main.go

rramkumar1 · 2019-04-19T15:02:23Z

pkg/composite/gen/main.go

+// The other types that are discovered as dependencies will simply be wrapped with a composite struct
+// The format of the map is ServiceName -> k8s-cloud-provider wrapper name
+// TODO: (shance) Add the commented services and remove dependency on first cloud-provider layer
+var MainServices = map[string]string{


Can you extract this and all the other types and methods below into a separate package?

I'm starting to wonder if it would make sense for all of this stuff to be moved into k8s-cloud-provider 🙃 . Maybe not at first though.

Yeah I've thought about that. Reason I hesitate right now is that its pretty specific to us. If someone else finds a need for this, then definitely it should go there.

makes sense. It would allow us to refactor the parsing code a bit too which would be nice eventually.

rramkumar1 · 2019-04-19T15:04:25Z

pkg/composite/gen/main.go

+	}
+}
+
+func init() {


This init can also be moved to a separate package when you move the other stuff.

spencerhance · 2019-04-19T17:19:22Z

@rramkumar1 Ready for another review

rramkumar1

In general, LGTM. Would like @bowei to take a look as well. Also, please squash commits.

/assign @bowei

rramkumar1 · 2019-04-23T19:55:01Z

pkg/composite/gen/main.go

@@ -0,0 +1,476 @@
+/*
+Copyright 2019 The Kubernetes Authors.


Need the proper line breaks for the copyright. You can look at some other file for reference.

rramkumar1 · 2019-04-23T19:57:38Z

pkg/composite/gen/main.go

+	"fmt"
+	"io"
+	"io/ioutil"
+	compositemeta "k8s.io/ingress-gce/pkg/composite/meta"


Typically standard library imports (like bytes or strings) are first (and in alphabetical order) and the stuff imported from elsewhere is next (after a empty line). Example:

"bytes"
"fmt"
"io"
...

compositemeta "k8s.io...."

Can you make sure the imports for other files is fixed as well?

rramkumar1 · 2019-04-23T19:58:38Z

pkg/composite/gen/main.go

+)
+
+// gofmtContent runs "gofmt" on the given contents.
+// Duplicate of the function in k8s-cloud-provider


This function is pretty trivial so no need to mention that its a dup.

rramkumar1 · 2019-04-23T19:59:47Z

pkg/composite/gen/main.go

+	fmt.Fprintf(wr, "\n\n")
+}
+
+// genTypes() generates all of the struct definitions


Nit: genTypes() generates all of the composite structs.

rramkumar1 · 2019-04-23T20:00:11Z

pkg/composite/gen/main.go

+	}
+}
+
+// genFuncs() generates all of the struct methods


Nit: genFuncs() generates helper methods attached to composite structs.

rramkumar1 · 2019-04-23T20:01:06Z

pkg/composite/gen/main.go

+}
+
+// genTests() generates all of the tests
+// TODO: (shance) figure out a better way to test the toGA(), toAlpha(), and toBeta() functions


Can remove TODO since you addressed it I think.

rramkumar1 · 2019-04-23T20:02:07Z

pkg/composite/meta/meta.go

+	"GA":    "",
+}
+
+// ApiService is a struct to hold all of the relevant data for generating a composite service


Nit: ApiService holds relevant data for generating a composite type + helper methods for a single API service.

rramkumar1 · 2019-04-23T20:02:44Z

pkg/composite/meta/meta.go

+	json.Unmarshal([]byte(byteValue), &result)
+
+	// Queue of ApiService names for BFS
+	typesQueue := []string{}


Remove the extra spaces between here and line 136.

rramkumar1 · 2019-04-29T15:02:29Z

/lgtm
/approve

k8s-ci-robot · 2019-04-29T15:02:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rramkumar1, spencerhance

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:

~~OWNERS~~ [rramkumar1]

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 16, 2019

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

k8s-ci-robot requested review from freehan and thockin April 16, 2019 21:16

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 17, 2019

spencerhance mentioned this pull request Apr 17, 2019

Add more service versions to the generate list GoogleCloudPlatform/k8s-cloud-provider#9

Merged

spencerhance force-pushed the new-composite-types branch from 3a05935 to fc9b446 Compare April 17, 2019 18:15

spencerhance changed the title ~~[WIP] Generate composite types~~ Generate composite types Apr 17, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 17, 2019

spencerhance force-pushed the new-composite-types branch from 6e63d4c to c815bb0 Compare April 17, 2019 18:20

rramkumar1 self-assigned this Apr 17, 2019

spencerhance mentioned this pull request Apr 17, 2019

L7-ILB Support Tracking #731

Closed

10 tasks

rramkumar1 reviewed Apr 17, 2019

View reviewed changes

spencerhance force-pushed the new-composite-types branch from c815bb0 to f4c706f Compare April 17, 2019 21:41

rramkumar1 reviewed Apr 18, 2019

View reviewed changes

rramkumar1 reviewed Apr 19, 2019

View reviewed changes

k8s-ci-robot assigned bowei Apr 23, 2019

rramkumar1 reviewed Apr 23, 2019

View reviewed changes

Add generator for composite types and generate BackendService

7cf766f

spencerhance force-pushed the new-composite-types branch from 52f071e to 7cf766f Compare April 23, 2019 20:50

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

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

k8s-ci-robot merged commit 4918eb2 into kubernetes:master Apr 29, 2019

spencerhance deleted the new-composite-types branch June 5, 2019 02:10

Generate composite types #729

Generate composite types #729

Conversation

spencerhance commented Apr 16, 2019 • edited Loading

k8s-ci-robot commented Apr 16, 2019

rramkumar1 commented Apr 17, 2019

rramkumar1 left a 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

spencerhance Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerhance commented Apr 17, 2019

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

spencerhance Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

spencerhance commented Apr 18, 2019

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

spencerhance commented Apr 19, 2019

rramkumar1 left a comment • edited Loading

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

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

rramkumar1 commented Apr 29, 2019

k8s-ci-robot commented Apr 29, 2019

spencerhance commented Apr 16, 2019 •

edited

Loading

spencerhance Apr 17, 2019 •

edited

Loading

spencerhance Apr 18, 2019 •

edited

Loading

rramkumar1 left a comment •

edited

Loading