More composite types #742

spencerhance · 2019-04-30T18:27:36Z

Blocked by: GoogleCloudPlatform/k8s-cloud-provider#12 GoogleCloudPlatform/k8s-cloud-provider#10

Add composite types for:
- ForwardingRule
- HealthChecks
- UrlMap
- TargetHttpProxy
- TargetHttpsProxy
Adds support for Regional resources for Alpha types that support it
- The type (e.g. global/regional) is interacted with via meta.Key objects
- Services also contain a "ResourceType" object so that downstream users can keep track of the type
Removes dependency on k/k cloud provider layer for basic CRUD operations
Copies in metrics from vendored version of k/k cloud provider (still needs to be verified)
- Renames prometheus metric prefix from cloudprovider to gce_api

k8s-ci-robot · 2019-04-30T18:27:44Z

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.

bowei · 2019-05-06T21:53:27Z

/ok-to-test

rramkumar1 · 2019-05-09T19:24:06Z

pkg/composite/metrics/metrics.go

@@ -0,0 +1,105 @@
+/*


What's the plan for this? Are we going to move forward with tracking the metrics here or is this a stopgap?

I filed an issue (GoogleCloudPlatform/k8s-cloud-provider#4) a while back to track these metrics in k8s-cloud-provider since it makes more sense in that context.

I was thinking of it as a stopgap because I wasn't sure of the best way to fix the metrics long-term. In the case of that issue, wouldn't the metrics still live in ingress-gce but then just be passed along to the k8s-cloud-provider layer?

rramkumar1 · 2019-05-09T19:24:29Z

pkg/composite/gen/main.go

@@ -153,6 +155,9 @@ func genTypes(wr io.Writer) {
 			// Note that the compute API's do not contain this field. It is for our
 			// own bookkeeping purposes.
 			Version meta.Version {{$backtick}}json:"-"{{$backtick}}
+			// ResourceType keeps track of the intended type of the service (e.g. Global)


Do you have an example of how this is used?

Not currently, I added preemptively since I think we'll need a way of keeping track of this when integrating the composite types. Open to removing it for now though.

rramkumar1 · 2019-05-09T19:24:51Z

pkg/composite/composite.go

+	Version meta.Version `json:"-"`
+	// ResourceType keeps track of the intended type of the service (e.g. Global)
+	// This is also an internal field purely for bookkeeping purposes
+	ResourceType             meta.KeyType      `json:"-"`


I would add a space after ResourceType

you probably mean \n

Sorry yes I meant newline

bowei

I am doing another pass to look at the generator code

bowei · 2019-05-09T19:54:40Z

pkg/backends/backends.go

 		return err
 	}
 	return nil
 }

 // Get implements Pool.
 func (b *Backends) Get(name string, version meta.Version) (*composite.BackendService, error) {
-	be, err := composite.GetBackendService(name, version, b.cloud)
+	be, err := composite.GetBackendService(name, version, b.cloud, meta.GlobalKey(name))


this seems wrong, why does this take a name AND and a key?

The key is just being used as a way of distinguishing if it should be global or regional.

right, so the name arg is redundnat

ah I see, yeah that makes sense

bowei · 2019-05-09T19:55:09Z

pkg/composite/composite.go

+	Version meta.Version `json:"-"`
+	// ResourceType keeps track of the intended type of the service (e.g. Global)
+	// This is also an internal field purely for bookkeeping purposes
+	ResourceType             meta.KeyType      `json:"-"`


you probably mean \n

bowei · 2019-05-09T19:55:50Z

pkg/composite/composite.go

+	Type                     string            `json:"type,omitempty"`
+	UdpHealthCheck           *UDPHealthCheck   `json:"udpHealthCheck,omitempty"`
+	UnhealthyThreshold       int64             `json:"unhealthyThreshold,omitempty"`
+	googleapi.ServerResponse `json:"-"`


what is this?

It's included in the generated api code with this comment:

// ServerResponse contains the HTTP response code and headers from the // server.

now that I think about it, I can probably just parse the descriptions too so that the struct field comments are also retained.

Added that in latest commit

bowei · 2019-05-09T19:58:09Z

pkg/composite/composite.go

+// ToAlpha converts our composite type into an alpha type.
+// This alpha type can be used in GCE API calls.
+func (forwardingRule *ForwardingRule) ToAlpha() (*computealpha.ForwardingRule, error) {
+	bytes, err := json.Marshal(forwardingRule)


this can be generic and I think there is already copyViaJSON util function with a signature like

func CopyViaJSON(dest, src interface{}) error

hmm good point, that seems a lot cleaner

I refactored the conversion functions to use copyViaJSON(). Is that what you were thinking?

bowei · 2019-05-09T19:58:45Z

pkg/composite/gen/main.go

@@ -191,80 +196,240 @@ func genFuncs(wr io.Writer) {

 {{range $type := $All}}
 {{if .IsMainService}}
-	func Create{{.Name}}({{.VarName}} *{{.Name}}, cloud *gce.Cloud) error {
+


extra spaces here

bowei · 2019-05-09T19:59:03Z

pkg/composite/gen/main.go

+
+
+	{{if .IsDefaultRegionalService}}
+			func Create{{.Name}}({{.VarName}} *{{.Name}}, cloud *gce.Cloud, key *meta.Key) error {


please make sure the whitespace makes sense

Gave it another pass, should be more consistent now

spencerhance · 2019-05-18T00:11:11Z

@bowei FYI I added the changes we discussed offline

/assign bowei

rramkumar1

/lgtm

rramkumar1 · 2019-05-29T14:57:07Z

pkg/composite/metrics/metrics_test.go

@@ -0,0 +1,28 @@
+/*
+Copyright 2017 The Kubernetes Authors.


@spencerhance Can you fix this? I'll re-lgtm after that.

@rramkumar1 My bad. should be fixed now

rramkumar1 · 2019-06-04T21:56:57Z

/lgtm

k8s-ci-robot · 2019-06-04T21:57:13Z

[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 30, 2019

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

k8s-ci-robot requested review from MrHohn and thockin April 30, 2019 18:28

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 May 6, 2019

spencerhance changed the title ~~[WIP] More composite types~~ More composite types May 6, 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 May 6, 2019

spencerhance force-pushed the more-composite-types branch from b7ed973 to f64ba6a Compare May 6, 2019 22:27

spencerhance mentioned this pull request May 6, 2019

1.4.0 Release GoogleCloudPlatform/k8s-cloud-provider#10

Closed

rramkumar1 reviewed May 9, 2019

View reviewed changes

bowei reviewed May 9, 2019

View reviewed changes

spencerhance force-pushed the more-composite-types branch from 415c0ef to 59c0010 Compare May 10, 2019 01:49

spencerhance mentioned this pull request May 13, 2019

L7-ILB Support Tracking #731

Closed

10 tasks

spencerhance force-pushed the more-composite-types branch 3 times, most recently from a858803 to 4585528 Compare May 18, 2019 00:09

k8s-ci-robot assigned bowei May 18, 2019

rramkumar1 reviewed May 29, 2019

View reviewed changes

k8s-ci-robot assigned rramkumar1 May 29, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 29, 2019

spencerhance force-pushed the more-composite-types branch from b86c174 to 3fb325d Compare June 4, 2019 21:46

Add more composite types

ae73efe

Update k8s-cloud-provider to 1.4.0

180df87

spencerhance force-pushed the more-composite-types branch from 3fb325d to 180df87 Compare June 4, 2019 21:54

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

k8s-ci-robot merged commit 5bd3a2a into kubernetes:master Jun 4, 2019

spencerhance deleted the more-composite-types branch June 4, 2019 22:00



		{{if .IsDefaultRegionalService}}
		func Create{{.Name}}({{.VarName}} {{.Name}}, cloud gce.Cloud, key *meta.Key) error {

More composite types #742

More composite types #742

Conversation

spencerhance commented Apr 30, 2019 • edited Loading

k8s-ci-robot commented Apr 30, 2019

bowei commented May 6, 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

Choose a reason for hiding this comment

bowei 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

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

spencerhance commented May 18, 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

rramkumar1 commented Jun 4, 2019

k8s-ci-robot commented Jun 4, 2019

spencerhance commented Apr 30, 2019 •

edited

Loading