-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GCE for cloud-controller-manager #45313
GCE for cloud-controller-manager #45313
Conversation
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.
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. |
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 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. |
} | ||
|
||
// InstanceTypeByProviderID returns the cloudprovider instance type of the node | ||
// with the specified unique providerID This method will not be called from the | ||
// node that is requesting this ID. i.e. metadata service and other local | ||
// methods cannot be used here | ||
func (gce *GCECloud) InstanceTypeByProviderID(providerID string) (string, error) { | ||
return "", errors.New("unimplemented") | ||
project, zone, name, err := splitProviderID(providerID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if err is not nil here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if zone is unknown (empty)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please return the error and fail fast if err != nil
here
Just curious, What's the providerID format for GCE? |
Thanks for the PR @realfake. Looks good! Some minor concerns. |
@luxas @thockin, @realfake has been working with me to implement the This is his first PR for GCE. PTAL. @k8s-bot ok to test |
@wlan0: you can't request testing unless you are a kubernetes member. In response to this:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks pretty good to me but I want more tests, some clarifications and maybe some minor changes :)
@roberthbailey PTAL for more context...
// 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+"://") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
|
||
// InstanceTypeByProviderID returns the cloudprovider instance type of the node | ||
// with the specified unique providerID This method will not be called from the | ||
// node that is requesting this ID. i.e. metadata service and other local | ||
// methods cannot be used here | ||
func (gce *GCECloud) InstanceTypeByProviderID(providerID string) (string, error) { | ||
return "", errors.New("unimplemented") | ||
project, zone, name, err := splitProviderID(providerID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please return the error and fail fast if err != nil
here
func (gce *GCECloud) getInstanceFromProjectInZoneByName(project, zone, name string) (*gceInstance, error) { | ||
name = canonicalizeInstanceName(name) | ||
res, err := gce.service.Instances.Get(project, zone, name).Do() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this line, remove
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getInstanceFromProjectInZoneByName...
if len(instance.NetworkInterfaces) < 1 { | ||
return []v1.NodeAddress{}, fmt.Errorf("could not find network interfaces for providerID %q", providerID) | ||
} | ||
networkInterface := instance.NetworkInterfaces[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a chance that there are multiple internal IPs?
In that case you should loop through instance.NetworkInterfaces
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states: An array of configurations for this interface. [...]. Only one interface is supported per instance.
BTW; after we have this and #44258 @wlan0, will the cloud-controller-manager automatically set 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 👍 |
9561eb1
to
1599798
Compare
1599798
to
53e4fef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, just one Q. Thanks!
@jlowdermilk @roberthbailey @thockin @mikedanese PTAL and approve if it looks ok
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you check if it was doable with url.Parse BTW?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@k8s-bot ok to test /lgtm |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: luxas, realfake Assign the PR to them by writing
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Ping @luxas |
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a regular expression rather than a series of string splits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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://(.+?)/(.+?)/(.+)$
.
53e4fef
to
a0fc386
Compare
@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) |
@luxas - yes, the RE looks much nicer to me. +1 on squashing. |
d620f57
to
d57923c
Compare
*Add splitProviderID helper function *Add getInstanceFromProjectInZoneByName function *Implement gce InstanceTypeByProviderID *Implement gce NodeAddressesByProviderID
d57923c
to
250b229
Compare
@k8s-bot verify test this |
Interpreted @roberthbailey's last comment as an approval |
Automatic merge from submit-queue |
Addresses #47257 |
What this PR does / why we need it:
This implements the
NodeAddressesByProviderID
andInstanceTypeByProviderID
methods used by the cloud-controller-manager to the GCE provider.Release note: