GCE for cloud-controller-manager by lbb · Pull Request #45313 · kubernetes/kubernetes

lbb · 2017-05-03T23:16:59Z

What this PR does / why we need it:
This implements the NodeAddressesByProviderIDand InstanceTypeByProviderID methods used by the cloud-controller-manager to the GCE provider.

Release note:

NONE

k8s-ci-robot · 2017-05-03T23:17:02Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

Details

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. I understand the commands that are listed here.

k8s-reviewable · 2017-05-03T23:17:05Z

This change is

k8s-ci-robot · 2017-05-03T23:17:07Z

Hi @realfake. 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 @k8s-bot 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.

Details

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. I understand the commands that are listed here.

wlan0 · 2017-05-04T01:24:58Z

pkg/cloudprovider/providers/gce/gce_instances.go

 // methods cannot be used here
 func (gce *GCECloud) InstanceTypeByProviderID(providerID string) (string, error) {
-	return "", errors.New("unimplemented")
+	project, zone, name, err := splitProviderID(providerID)


What happens if err is not nil here?

What happens if zone is unknown (empty)?

The provider id is a concatinated string "gce://" + gce.InstanceID(). gce.InstanceID() returns an ID which is constructed like projectID + "/" + zone + "/" + instanceName. The zone for this is retrieved by the API and must not be nil.

please return the error and fail fast if err != nil here

wlan0 · 2017-05-04T01:26:56Z

Just curious, What's the providerID format for GCE?

wlan0 · 2017-05-04T01:27:42Z

Thanks for the PR @realfake. Looks good! Some minor concerns.

wlan0 · 2017-05-04T01:33:16Z

@luxas @thockin, @realfake has been working with me to implement the .*ByProviderID methods for cloudproviders. These methods are called by the CCM, and are required to fetch node specific information from a remote controller, instead of from within the node.

This is his first PR for GCE. PTAL.

@k8s-bot ok to test

k8s-ci-robot · 2017-05-04T01:33:36Z

@wlan0: you can't request testing unless you are a kubernetes member.

Details

In response to this:

@luxas @thockin, @realfake has been working with me to implement the .*ByProviderID methods for cloudproviders. These methods are called by the CCM, and are required to fetch node specific information from a remote controller, instead of from within the node.

This is his first PR for GCE. PTAL.

@k8s-bot ok to test

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. I understand the commands that are listed here.

luxas

Thanks! Looks pretty good to me but I want more tests, some clarifications and maybe some minor changes :)

@roberthbailey PTAL for more context...

luxas · 2017-05-07T06:59:53Z

pkg/cloudprovider/providers/gce/gce_util.go

+// A providerID is build out of '${ProviderName}://${project-id}/${zone}/${instance-name}'
+// See cloudprovider.GetInstanceProviderID. 
+func splitProviderID(providerID string) (project, zone, instance string, err error) {
+	if !strings.HasPrefix(providerID, ProviderName+"://") {


Is it possible to use url.Parse() here?
I think that would be more robust...

This function needs many unit tests so we are sure that it's working as intended, please make some tests

luxas · 2017-05-07T07:02:04Z

pkg/cloudprovider/providers/gce/gce_instances.go

 // methods cannot be used here
 func (gce *GCECloud) InstanceTypeByProviderID(providerID string) (string, error) {
-	return "", errors.New("unimplemented")
+	project, zone, name, err := splitProviderID(providerID)


please return the error and fail fast if err != nil here

luxas · 2017-05-07T07:03:29Z

pkg/cloudprovider/providers/gce/gce_instances.go

+func (gce *GCECloud) getInstanceFromProjectInZoneByName(project, zone, name string) (*gceInstance, error) {
+	name = canonicalizeInstanceName(name)
+	res, err := gce.service.Instances.Get(project, zone, name).Do()
+


no need for this line, remove

luxas · 2017-05-07T07:03:43Z

pkg/cloudprovider/providers/gce/gce_instances.go

+	res, err := gce.service.Instances.Get(project, zone, name).Do()
+
+	if err != nil {
+		glog.Errorf("getInstanceFromZoneByName: failed to get instance %s; err: %v", name, err)


getInstanceFromProjectInZoneByName...

luxas · 2017-05-07T07:05:59Z

pkg/cloudprovider/providers/gce/gce_instances.go

+	if len(instance.NetworkInterfaces) < 1 {
+		return []v1.NodeAddress{}, fmt.Errorf("could not find network interfaces for providerID %q", providerID)
+	}
+	networkInterface := instance.NetworkInterfaces[0]


is there a chance that there are multiple internal IPs?
In that case you should loop through instance.NetworkInterfaces here

The documentation states: An array of configurations for this interface. [...]. Only one interface is supported per instance.

https://cloud.google.com/compute/docs/reference/latest/instances#networkInterfaces

luxas · 2017-05-07T07:39:20Z

BTW; after we have this and #44258 @wlan0, will the cloud-controller-manager automatically set Node.Spec.ProviderID based on the .NodeAddresses or is it still expected that each node must provide the --provider-id flag?

I think it would be great if the ccm set the ProviderID automatically for the clouds that support it, WDYT?

@realfake This needs a rebase, please fix that and I'll take a second look 👍

luxas

Cool, just one Q. Thanks!

@jlowdermilk @roberthbailey @thockin @mikedanese PTAL and approve if it looks ok

luxas · 2017-05-09T13:33:47Z

pkg/cloudprovider/providers/gce/gce_util.go

+// splitProviderID splits a provider's id into core components.
+// A providerID is build out of '${ProviderName}://${project-id}/${zone}/${instance-name}'
+// See cloudprovider.GetInstanceProviderID.
+func splitProviderID(providerID string) (project, zone, instance string, err error) {


did you check if it was doable with url.Parse BTW?

It is doable with url.Parse() but it still would be an equal amount of complexity.
You couldn't tell apart empty parameters for example in gce:///foo/bar.
You however can use it to get the Scheme of the URI, but afterwards you still have to split the path and access URL.Host.

luxas · 2017-05-09T13:36:30Z

@k8s-bot ok to test

/lgtm

k8s-github-robot · 2017-05-09T13:36:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: luxas, realfake
We suggest the following additional approver: dchen1107

Assign the PR to them by writing /assign @dchen1107 in a comment when ready.

Details

Needs approval from an approver in each of these OWNERS Files:

pkg/cloudprovider/providers/gce/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

lbb · 2017-05-16T17:21:07Z

Ping @luxas

roberthbailey · 2017-05-17T07:30:51Z

pkg/cloudprovider/providers/gce/gce_util.go

+// splitProviderID splits a provider's id into core components.
+// A providerID is build out of '${ProviderName}://${project-id}/${zone}/${instance-name}'
+// See cloudprovider.GetInstanceProviderID.
+func splitProviderID(providerID string) (project, zone, instance string, err error) {


Why not use a regular expression rather than a series of string splits?

Personally I use regexp only for non trivial things to parse. But a simple regex could easily enhance the readability of this function. I'd go with something like this^gce://(.+?)/(.+?)/(.+)$.

luxas · 2017-05-18T15:27:04Z

@roberthbailey looks good to you now with the regexp?

@realfake please squash your commits, it makes sense for the changes in this PR to be an atomic unit (the default way)

roberthbailey · 2017-05-18T23:19:25Z

@luxas - yes, the RE looks much nicer to me. +1 on squashing.

*Add splitProviderID helper function *Add getInstanceFromProjectInZoneByName function *Implement gce InstanceTypeByProviderID *Implement gce NodeAddressesByProviderID

luxas · 2017-05-19T10:50:09Z

@k8s-bot verify test this

luxas · 2017-05-19T11:44:35Z

Interpreted @roberthbailey's last comment as an approval
Reapplying the lgtm

k8s-github-robot · 2017-05-19T13:18:38Z

Automatic merge from submit-queue

wlan0 · 2017-06-09T18:21:29Z

Addresses #47257

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 3, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 3, 2017

k8s-github-robot assigned dchen1107 and piosz May 3, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 3, 2017

lbb changed the title ~~Cloudprovider gce metadata~~ GCE for cloud-controller-manager May 3, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 3, 2017

wlan0 reviewed May 4, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 4, 2017

wlan0 mentioned this pull request May 4, 2017

cloud initialize node in external cloud controller #44258

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2017

luxas reviewed May 7, 2017

View reviewed changes

luxas assigned luxas, thockin and roberthbailey and unassigned piosz May 7, 2017

lbb force-pushed the cloudprovider-gce-metadata branch from 9561eb1 to 1599798 Compare May 7, 2017 13:04

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2017

lbb force-pushed the cloudprovider-gce-metadata branch from 1599798 to 53e4fef Compare May 8, 2017 22:02

k8s-github-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 8, 2017

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 8, 2017

luxas approved these changes May 9, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2017

roberthbailey reviewed May 17, 2017

View reviewed changes

lbb force-pushed the cloudprovider-gce-metadata branch from 53e4fef to a0fc386 Compare May 18, 2017 06:46

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2017

lbb force-pushed the cloudprovider-gce-metadata branch 2 times, most recently from d620f57 to d57923c Compare May 19, 2017 06:30

Implement providerID node functions for gce

250b229

*Add splitProviderID helper function *Add getInstanceFromProjectInZoneByName function *Implement gce InstanceTypeByProviderID *Implement gce NodeAddressesByProviderID

lbb force-pushed the cloudprovider-gce-metadata branch from d57923c to 250b229 Compare May 19, 2017 06:42

luxas added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 19, 2017

k8s-github-robot merged commit 07dac20 into kubernetes:master May 19, 2017

wlan0 mentioned this pull request Jun 9, 2017

Fill in *ByID methods for the various clouds in Kubernetes #47257

Closed

Conversation

lbb commented May 3, 2017

Uh oh!

k8s-ci-robot commented May 3, 2017

Uh oh!

k8s-reviewable commented May 3, 2017

Uh oh!

k8s-ci-robot commented May 3, 2017

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!

wlan0 commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wlan0 commented May 4, 2017

Uh oh!

wlan0 commented May 4, 2017

Uh oh!

k8s-ci-robot commented May 4, 2017

Uh oh!

luxas 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!

luxas commented May 7, 2017

Uh oh!

luxas 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!

luxas commented May 9, 2017

Uh oh!

k8s-github-robot commented May 9, 2017

Uh oh!

lbb commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxas commented May 18, 2017

Uh oh!

roberthbailey commented May 18, 2017

Uh oh!

luxas commented May 19, 2017

Uh oh!

luxas commented May 19, 2017

Uh oh!

k8s-github-robot commented May 19, 2017

Uh oh!

wlan0 commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

wlan0 commented May 4, 2017 •

edited

Loading