Use ProviderID to address nodes in the cloudprovider #42604
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @wlan0. 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. |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED The following people have approved this PR: wlan0 Needs approval from an approver in each of these OWNERS Files: We suggest the following people: |
k8s-github-robot
added
size/L
|
This change is |
|
@k8s-bot ok to test |
thockin
added
release-note
thockin
reviewed
Mar 7, 2017
pkg/cloudprovider/cloud.go
Outdated
| // ProviderID is a unique identifier of the node. This will not be called | ||
| // from the node whose nodeaddresses are being queried. i.e. local metadata | ||
| // services cannot be used in this method to obtain nodeaddresses | ||
| NodeAddressesV2(providerId string) ([]v1.NodeAddress, error) |
There was a problem hiding this comment.
NodeAddressesByProviderID, please?
pkg/cloudprovider/cloud.go
Outdated
| // ExternalID returns the cloud provider ID of the node with the specified NodeName. | ||
| // Note that if the instance does not exist or is no longer running, we must return ("", cloudprovider.InstanceNotFound) | ||
| ExternalID(nodeName types.NodeName) (string, error) | ||
| // InstanceID returns the cloud provider ID of the node with the specified NodeName. | ||
| InstanceID(nodeName types.NodeName) (string, error) | ||
| // InstanceType returns the type of the specified instance. | ||
| InstanceType(name types.NodeName) (string, error) | ||
| // InstanceTypeV2 returns the type of the specified instance. | ||
| InstanceTypeV2(providerID string) (string, error) |
|
I'm wary of panic here - do we do that for other not-implemented methods? |
|
@wlan0 this targets v1.7, right? |
|
yes @luxas |
wlan0
force-pushed
the
provider-id
branch
2 times, most recently
from
e7fd243
to
a44a30c
Compare
|
This will actually be very helpful on AWS. We've also debated pushing in the whole Node object in the past, because it also includes other information like the zone, which saves a lookup on GCE. cc @jingxu97 And we do actually push the whole object for LoadBalancers, and this is used on OpenStack. One suggestion: define a strong type like types.NodeName instead of using a string. You'll avoid a lot of bugs :-) |
wlan0
force-pushed
the
provider-id
branch
2 times, most recently
from
aa24908
to
041a3f0
Compare
thockin
added
the
lgtm
|
I heartily endorse the idea of typedefs for things like this. |
|
It's a flake error - @k8s-bot cvm gce e2e test this |
k8s-github-robot
added
the
needs-rebase
k8s-github-robot
removed
the
lgtm
k8s-github-robot
removed
the
needs-rebase
wlan0
force-pushed
the
provider-id
branch
2 times, most recently
from
35d14fe
to
43239ed
Compare
|
Flake! @k8s-bot verify test this |
The cloudprovider is being refactored out of kubernetes core. This is being done by moving all the cloud-specific calls from kube-apiserver, kubelet and kube-controller-manager into a separately maintained binary(by vendors) called cloud-controller-manager. The Kubelet relies on the cloudprovider to detect information about the node that it is running on. Some of the cloudproviders worked by querying local information to obtain this information. In the new world of things, local information cannot be relied on, since cloud-controller-manager will not run on every node. Only one active instance of it will be run in the cluster. Today, all calls to the cloudprovider are based on the nodename. Nodenames are unqiue within the kubernetes cluster, but generally not unique within the cloud. This model of addressing nodes by nodename will not work in the future because local services cannot be queried to uniquely identify a node in the cloud. Therefore, I propose that we perform all cloudprovider calls based on ProviderID. This ID is a unique identifier for identifying a node on an external database (such as the instanceID in aws cloud).
|
@k8s-bot <https://github.com/k8s-bot> verify test this
…On Mar 27, 2017 8:22 PM, "k8s-ci-robot" ***@***.***> wrote:
@wlan0 <https://github.com/wlan0>: The following test(s) *failed*:
Test name Commit Details Rerun command
Jenkins verification 43239ed
<43239ed>
link
<https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/42604/pull-kubernetes-verify/23488/> @k8s-bot
verify test this
Full PR test history <https://k8s-gubernator.appspot.com/pr/42604>. Your
PR dashboard <https://k8s-gubernator.appspot.com/pr/wlan0>. Please help
us cut down on flakes by linking to an open issue
<https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when
you hit one in your PR.
Instructions for interacting with me using PR comments are available here
<https://github.com/kubernetes/community/blob/master/contributors/devel/pull-request-commands.md>.
If you have questions or suggestions related to my behavior, please file an
issue against the kubernetes/test-infra
<https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:>
repository. I understand the commands that are listed here
<https://github.com/kubernetes/test-infra/blob/master/prow/commands.md>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#42604 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVLJqrAo6Em7290XiY0_FcdamOCtKks5rqHzdgaJpZM4MU3Sn>
.
|
|
Builds green, re-applying lgtm |
luxas
added
the
lgtm
|
Automatic merge from submit-queue |
|
This is great! just one more PR to go, before external cloud kubelet! |
|
the gce code should really go into gce_instances.go... |
dims
pushed a commit
to dims/kubernetes
that referenced
this pull request
Feb 8, 2018
Automatic merge from submit-queue Use ProviderID to address nodes in the cloudprovider The cloudprovider is being refactored out of kubernetes core. This is being done by moving all the cloud-specific calls from kube-apiserver, kubelet and kube-controller-manager into a separately maintained binary(by vendors) called cloud-controller-manager. The Kubelet relies on the cloudprovider to detect information about the node that it is running on. Some of the cloudproviders worked by querying local information to obtain this information. In the new world of things, local information cannot be relied on, since cloud-controller-manager will not run on every node. Only one active instance of it will be run in the cluster. Today, all calls to the cloudprovider are based on the nodename. Nodenames are unqiue within the kubernetes cluster, but generally not unique within the cloud. This model of addressing nodes by nodename will not work in the future because local services cannot be queried to uniquely identify a node in the cloud. Therefore, I propose that we perform some(to start off with) of the cloudprovider calls based on ProviderID. This ID is a unique identifier for identifying a node on an external database (such as the instanceID in aws cloud). In the next PR, i'll add support to initialize nodes from the cloud-controller-manager instead of the kubelet using this API. @thockin @keontang @joonas @luxas @justinsb ```release-note ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The cloudprovider is being refactored out of kubernetes core. This is being
done by moving all the cloud-specific calls from kube-apiserver, kubelet and
kube-controller-manager into a separately maintained binary(by vendors) called
cloud-controller-manager. The Kubelet relies on the cloudprovider to detect information
about the node that it is running on. Some of the cloudproviders worked by
querying local information to obtain this information. In the new world of things,
local information cannot be relied on, since cloud-controller-manager will not
run on every node. Only one active instance of it will be run in the cluster.
Today, all calls to the cloudprovider are based on the nodename. Nodenames are
unqiue within the kubernetes cluster, but generally not unique within the cloud.
This model of addressing nodes by nodename will not work in the future because
local services cannot be queried to uniquely identify a node in the cloud. Therefore,
I propose that we perform some(to start off with) of the cloudprovider calls based on
ProviderID. This ID is a unique identifier for identifying a node on an external database (such as
the instanceID in aws cloud).
In the next PR, i'll add support to initialize nodes from the cloud-controller-manager instead of the kubelet using this API.
@thockin @keontang @joonas @luxas @justinsb