GCE: support Cloud TPU API in cloud provider #58029

yguo0905 · 2018-01-10T01:02:48Z

What this PR does / why we need it:

This PR adds the support for Cloud TPU API in GCE cloud provider.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

GCE: support Cloud TPU API in cloud provider

/assign @vishh
/assign @cheftako

cheftako

Given that the new k8s API's are not implementing cloud provider but do end up calling objects with Contexts, can we have them have a Context object as their first parameter? Also can you file an issue on the tpu package for not adhering to best practices on Context in go? ("Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:" from https://golang.org/pkg/context/)

cheftako · 2018-01-19T23:45:08Z

pkg/cloudprovider/providers/gce/gce_tpu.go

+	defer mc.Observe(err)
+
+	var op *tpuapi.Operation
+	name = fmt.Sprintf("projects/%s/locations/%s/nodes/%s", gce.projectID, zone, name)


Can we abstract fmt.Sprintf("projects/%s/locations/%s/nodes/%s" and fmt.Sprintf("projects/%s/locations/%s" into helper methods? That way we don't have to worry about things like typos.

cheftako · 2018-01-19T23:46:09Z

pkg/cloudprovider/providers/gce/gce_tpu.go

+// occurs.
+func (gce *GCECloud) waitForTPUOp(interval, timeout time.Duration, op *tpuapi.Operation) (*tpuapi.Operation, error) {
+	if err := wait.PollImmediate(interval, timeout, func() (bool, error) {
+		glog.V(2).Infof("Waiting for operation %q to complete...", op.Name)


This seems like it might be a bit spammy for log level 2

cheftako · 2018-01-19T23:47:05Z

pkg/cloudprovider/providers/gce/gce_tpu.go

+			return true, err
+		}
+		if op.Done {
+			glog.V(2).Infof("Operation %q has completed", op.Name)


Again with a bit spammy for 2

yguo0905 · 2018-01-23T01:16:09Z

Given that the new k8s API's are not implementing cloud provider but do end up calling objects with Contexts, can we have them have a Context object as their first parameter? Also can you file an issue on the tpu package for not adhering to best practices on Context in go? ("Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:" from https://golang.org/pkg/context/)

The APIs (including TPU APIs) in https://github.com/google/google-api-go-client are generated in a legacy way - they have the context issue as you described. The APIs with the new style are in https://github.com/GoogleCloudPlatform/google-cloud-go but they do not yet include TPU APIs. I think when the API owners migrate them to the new style, the context issue should be automatically fixed.

yguo0905 · 2018-01-23T19:14:07Z

/test pull-kubernetes-e2e-kops-aws

yguo0905 · 2018-01-27T01:01:12Z

/test pull-kubernetes-e2e-kops-aws

yguo0905 · 2018-02-14T02:09:47Z

@vishh, please take a look at this PR.

vishh · 2018-02-14T23:34:58Z

pkg/cloudprovider/providers/gce/gce_tpu.go

+// newTPUMetricContext returns a new metricContext used for recording metrics
+// of Cloud TPU API calls.
+func newTPUMetricContext(request, zone string) *metricContext {
+	return newGenericMetricContext("tpus", request, unusedMetricLabel, zone, computeAlphaVersion)


What's computeAlphaVersion? Are we using alpha APIs?

It's a string constant alpha. This is just a label passed into the metrics. Since we are using alpha TPU API, I set this to "alpha" explicitly.

vishh · 2018-02-14T23:35:49Z

Godeps/Godeps.json

@@ -3043,6 +3043,10 @@
 			"ImportPath": "google.golang.org/api/pubsub/v1",
 			"Rev": "c0dae069ee96c9261a04c81efd9e0f1e55f565ac"
 		},
+		{
+			"ImportPath": "google.golang.org/api/tpu/v1alpha1",


Let's switch to v1beta1

We don't have generated clients for v1beta1 at this point. I asked the TPU team to generate and publish them on https://github.com/google/google-api-go-client.

vishh · 2018-02-14T23:36:19Z

Switch to v1beta1 and then this PR is good to go.

yguo0905 · 2018-02-23T01:33:42Z

/retest

vishh · 2018-02-27T01:08:36Z

/lgtm
/approve

yguo0905 · 2018-02-27T01:15:25Z

/assign @thockin

ixdy · 2018-02-27T01:57:35Z

/retest

thockin · 2018-02-27T04:37:28Z

/approve
/approve no-issue

vishh · 2018-02-27T19:20:59Z

/lgtm

k8s-ci-robot · 2018-02-27T19:21:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, vishh, yguo0905

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:

~~Godeps/OWNERS~~ [thockin]
~~pkg/cloudprovider/providers/gce/OWNERS~~ [thockin]
~~vendor/OWNERS~~ [thockin]

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

k8s-github-robot · 2018-02-27T22:57:11Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-27T23:58:12Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot assigned cheftako and vishh Jan 10, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 10, 2018

cheftako reviewed Jan 20, 2018

View reviewed changes

yguo0905 force-pushed the tpu-api branch from 3bb13d6 to 20a7d97 Compare January 23, 2018 01:02

vishh reviewed Feb 14, 2018

View reviewed changes

yguo0905 force-pushed the tpu-api branch from 20a7d97 to 1550a50 Compare February 22, 2018 22:03

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

vishh added this to the v1.10 milestone Feb 27, 2018

vishh added status/approved-for-milestone priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. area/hw-accelerators labels Feb 27, 2018

k8s-ci-robot assigned thockin Feb 27, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2018

yguo0905 added 2 commits February 26, 2018 22:09

GCE: support Cloud TPU API in cloud provider

3aca71e

Add Cloud TPU v1alpha1 API dependency

a8ac330

yguo0905 force-pushed the tpu-api branch from 1550a50 to a8ac330 Compare February 27, 2018 06:57

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

k8s-github-robot merged commit e30554b into kubernetes:master Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCE: support Cloud TPU API in cloud provider #58029

GCE: support Cloud TPU API in cloud provider #58029

yguo0905 commented Jan 10, 2018

cheftako left a comment

cheftako Jan 19, 2018

yguo0905 Jan 23, 2018

cheftako Jan 19, 2018

yguo0905 Jan 23, 2018

cheftako Jan 19, 2018

yguo0905 Jan 23, 2018

yguo0905 commented Jan 23, 2018

yguo0905 commented Jan 23, 2018

yguo0905 commented Jan 27, 2018

yguo0905 commented Feb 14, 2018

vishh Feb 14, 2018

yguo0905 Feb 22, 2018

vishh Feb 14, 2018

yguo0905 Feb 22, 2018

vishh commented Feb 14, 2018

yguo0905 commented Feb 23, 2018

vishh commented Feb 27, 2018

yguo0905 commented Feb 27, 2018

ixdy commented Feb 27, 2018

thockin commented Feb 27, 2018

vishh commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018

GCE: support Cloud TPU API in cloud provider #58029

GCE: support Cloud TPU API in cloud provider #58029

Conversation

yguo0905 commented Jan 10, 2018

cheftako 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

yguo0905 commented Jan 23, 2018

yguo0905 commented Jan 23, 2018

yguo0905 commented Jan 27, 2018

yguo0905 commented Feb 14, 2018

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

vishh commented Feb 14, 2018

yguo0905 commented Feb 23, 2018

vishh commented Feb 27, 2018

yguo0905 commented Feb 27, 2018

ixdy commented Feb 27, 2018

thockin commented Feb 27, 2018

vishh commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018