Cleanup the use of ExternalID as it is deprecated #60692

adnavare · 2018-03-02T07:13:24Z

The patch removes ExternalID usage from node_controller
and node_lifecycle_oontroller. The code instead uses InstanceID
which returns the cloud provider ID as well.

fixes #60466

k8s-ci-robot · 2018-03-02T07:13:25Z

@adnavare: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

adnavare · 2018-03-02T07:16:01Z

Hi Folks, I have changed the code to remove ExternalID from node_controller and node_lifecycle_controller. But as ExternalID is deprecated and if not needed, I can go ahead to remove the implementation of ExternalID() from each provider and also delete the field from instances type defined in cloud.go. I wanted to know the feedback from you guys, thanks.

adnavare · 2018-03-02T07:17:25Z

Can someone please approve the PR to be tested? Thanks

jdumars · 2018-03-02T15:54:34Z

/ok-to-test

deads2k · 2018-03-05T15:57:07Z

/assign @jhorwit2
/assign @aveshagarwal

I think these reviewers are closer than me.

thockin · 2018-03-05T15:57:16Z

This change is

adnavare · 2018-03-05T17:27:57Z

Thanks, I see that unit test failed. I will analyze the gate failure and update the code.

jhorwit2

Thanks for the PR @adnavare! Sadly, I don't think we can move from ExternalID to InstanceID this easily since the interface contract is different. ExternalID explicitly says

// Note that if the instance does not exist or is no longer running, we must return ("", cloudprovider.InstanceNotFound)

and the InstanceID method does not. From what I can tell quickly looking through some cloud provider implementations this change would cause nodes to not be deleted since some clouds only return the error mentioned above in ExternalID.

I'd suggest writing a brief proposal and sending it to the working groups email group to get opinions/feedback from as many interested parties as possible.

jhorwit2 · 2018-03-06T01:20:21Z

An interesting point about the deprecation... it seems that the ExternalIDmethod on the Instances interface does not mention it's deprecated; however, the implementations of specific clouds do mention it is deprecated in their method doc.

adnavare · 2018-03-06T01:21:12Z

Hi @jhorwit2 : Thanks for the review. Yes, true but so far I found only two places where the error returned is used to check future condition and that is in node_controller.go, controller_utils.go and in two unit tests - photon_test.go and vsphere_test.go.
In those cases what can be done is check if the ID returned is nil or not, as InstanceID() returns ID or nil.
But yes, I did not change in the code right now because was waiting for feedback from folks.

adnavare · 2018-03-06T01:23:30Z

@jhorwit2: Yes indeed, i noticed that, deprecation note about ExternalID is not present on "Instances" interface

adnavare · 2018-03-06T18:51:35Z

Yes, due to InstanceID not returning bool,error instead returning bool,nil, I had to change a unit test. I will update based upon the feedback from folks later. I will send out an email to mailing list today asking for feedback whether to replace ExternalID entirely. Thank you.

wlan0 · 2018-03-07T04:21:49Z

This change sounds valid to me. If the implicit functionality of this API (checking Instance existence) is also satisfied by the new API, then it's fine by me.

On another note, we seem to be using InstanceID in lieu of ExternalID. There's also a ProviderID - It seems to me like they overlap to some extent. Should we consolidate these and make it one? We have three different IDs to address a node.

thoughts @jhorwit2 @cheftako?

@adnavare Please join us at the next wg-cloud-provider online meetup. Will share details on slack

wlan0 · 2018-03-08T19:50:18Z

/retest

adnavare · 2018-03-09T22:02:15Z

@jhorwit2 @wlan0: Can you get your eye over the cod, thanks!

jhorwit2 · 2018-03-09T22:38:22Z

@adnavare there is more code that uses the ExternalID method like the kubelet. If we're OK with completely removing the calls then we should also remove it from the Instances cloud provider interface.

adnavare · 2018-03-09T22:43:00Z

@jhorwit2: Yes i agree. As from this week's discussion it is decided in the next week's meeting, representative for each cloud provider will give an idea whether they want to remove ExternalID completely. If so, then I will go ahead and remove the ExternalID method from Instances cloud provider interface.

mikedanese · 2018-04-04T17:10:35Z

/approve

mikedanese · 2018-04-04T17:12:23Z

pkg/cloudprovider/providers/aws/aws.go

@@ -1318,24 +1318,6 @@ func (c *Cloud) NodeAddressesByProviderID(ctx context.Context, providerID string
 	return extractNodeAddresses(instance)
 }

-// ExternalID returns the cloud provider ID of the node with the specified nodeName (deprecated).
-func (c *Cloud) ExternalID(ctx context.Context, nodeName types.NodeName) (string, error) {


cc @justinsb

wlan0 · 2018-04-04T17:13:01Z

/approve

mikedanese · 2018-04-04T17:32:31Z

ExternalID aliased to InstanceID

azure
cloudstack
vsphere

ovirt InstanceID is ExternalID with a leading /

photon InstanceID and ExternalID are copies of the same function

gce InstanceID is <project>/<zone>/<instance> and ExternalID is <instance>

aws InstanceID is /<zone>/<instance> and ExternalID is <instance>

mikedanese · 2018-04-04T17:33:19Z

The kubelet change is safe, and we plan on removing that field entirely before 1.11. @tallclair for kubelet approval:

/assign @tallclair

tallclair · 2018-04-07T00:23:26Z

/approve

mikedanese · 2018-04-07T15:35:32Z

/lgtm

k8s-ci-robot · 2018-04-07T15:35:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adnavare, mikedanese, tallclair, wlan0

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:

~~pkg/cloudprovider/OWNERS~~ [mikedanese,wlan0]
~~pkg/controller/OWNERS~~ [mikedanese]
~~pkg/kubelet/OWNERS~~ [tallclair]

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

mikedanese · 2018-04-09T17:50:09Z

release note none. we can roll the release note into #61877

adnavare · 2018-04-09T18:12:57Z

Please let me know if any AR is needed from my side on this PR to get it going. Thank you all for reviews and feedback.

adnavare · 2018-04-09T18:13:27Z

/retest

k8s-github-robot · 2018-04-09T18:58:12Z

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

Automatic merge from submit-queue (batch tested with PRs 62590, 62818, 63015, 62922, 63000). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use BootID instead of ExternalID to check for new instance PR #60692 changed the way that ExternalID is reported on GCE. Its value is no longer the GCE instance ID. It is the instance name. So it cannot be used to determine VM uniqueness across time. Instead, upgrade will check that the boot ID changed. **What this PR does / why we need it**: Node upgrades stall out because the external ID remains the same across upgrades now. **Which issue(s) this PR fixes**: Fixes #62713 **Release note**: ```release-note NONE ```

This got marked as deprecated four years ago [1], and finally removed last year [2]. This is not present at all any more in Kubernetes 1.12. This has been identical to `metadata.name` since [1], so just use the latter directly. In the nodes overview list, don't show the name/ID again in the Address column, as that's just redundant: The first column already shows that. So show the actual address instead. [1] https://github.com/kubernetes/kubernetes/pull/7775/files#diff-e58c0c0709cf6f8a3b00aef6069b66b7 [2] kubernetes/kubernetes#60692

This got marked as deprecated four years ago [1], and finally removed last year [2]. This is not present at all any more in Kubernetes 1.12. This has been identical to `metadata.name` since [1], so just use the latter directly. In the nodes overview list, don't show the name/ID again in the Address column, as that's just redundant: The first column already shows that. So show the actual address instead. [1] https://github.com/kubernetes/kubernetes/pull/7775/files#diff-e58c0c0709cf6f8a3b00aef6069b66b7 [2] kubernetes/kubernetes#60692 Closes #11385

k8s-ci-robot requested review from deads2k and thockin March 2, 2018 07:13

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 Mar 2, 2018

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 Mar 2, 2018

k8s-ci-robot assigned aveshagarwal and jhorwit2 Mar 5, 2018

jhorwit2 suggested changes Mar 6, 2018

View reviewed changes

adnavare force-pushed the bug/60466 branch from c8ed898 to e0cd73c Compare March 6, 2018 18:49

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 6, 2018

adnavare force-pushed the bug/60466 branch from e0cd73c to ed67459 Compare March 8, 2018 21:37

adnavare force-pushed the bug/60466 branch from 67429f6 to 1335e6e Compare April 2, 2018 18:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2018

mikedanese reviewed Apr 4, 2018

View reviewed changes

k8s-ci-robot assigned tallclair Apr 4, 2018

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

k8s-ci-robot assigned mikedanese Apr 7, 2018

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

mikedanese added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 9, 2018

k8s-github-robot merged commit 09ec7bf into kubernetes:master Apr 9, 2018

yujuhong mentioned this pull request Apr 17, 2018

Failing Tests : Timeout and UpgradeTest in gce-1.10-master-upgrade-cluster, gce-1.10-master-upgrade-cluster-new and gce-1.10-master-upgrade-cluster-parallel #62713

Closed

krousey mentioned this pull request Apr 23, 2018

Use BootID instead of ExternalID to check for new instance #62922

Merged

This was referenced May 9, 2018

SkewTest failing gce-1.10-master-upgrade-cluster-new job in sig-release-master-upgrade #63506

Closed

[Failing Tests] : BeforeSuite,Up and Upgrade tests failing on multiple Downgrade cluster jobs in sig-release-master-upgrade #63505

Closed

liggitt mentioned this pull request Jun 5, 2018

From 1.11, an extended resource reported by a device plugin can be left on a node after node upgrade even though its device plugin never registers back #64632

Closed

martinpitt mentioned this pull request Mar 13, 2019

kubernetes: Remove deprecated spec.externalID cockpit-project/cockpit#11385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup the use of ExternalID as it is deprecated #60692

Cleanup the use of ExternalID as it is deprecated #60692

adnavare commented Mar 2, 2018

k8s-ci-robot commented Mar 2, 2018

adnavare commented Mar 2, 2018

adnavare commented Mar 2, 2018

jdumars commented Mar 2, 2018

deads2k commented Mar 5, 2018

thockin commented Mar 5, 2018

adnavare commented Mar 5, 2018

jhorwit2 left a comment •

edited

jhorwit2 commented Mar 6, 2018

adnavare commented Mar 6, 2018

adnavare commented Mar 6, 2018

adnavare commented Mar 6, 2018

wlan0 commented Mar 7, 2018 •

edited

wlan0 commented Mar 8, 2018

adnavare commented Mar 9, 2018

jhorwit2 commented Mar 9, 2018

adnavare commented Mar 9, 2018

mikedanese commented Apr 4, 2018

mikedanese Apr 4, 2018

wlan0 commented Apr 4, 2018

mikedanese commented Apr 4, 2018

mikedanese commented Apr 4, 2018

tallclair commented Apr 7, 2018

mikedanese commented Apr 7, 2018

k8s-ci-robot commented Apr 7, 2018

mikedanese commented Apr 9, 2018

adnavare commented Apr 9, 2018

adnavare commented Apr 9, 2018

k8s-github-robot commented Apr 9, 2018

Cleanup the use of ExternalID as it is deprecated #60692

Cleanup the use of ExternalID as it is deprecated #60692

Conversation

adnavare commented Mar 2, 2018

k8s-ci-robot commented Mar 2, 2018

adnavare commented Mar 2, 2018

adnavare commented Mar 2, 2018

jdumars commented Mar 2, 2018

deads2k commented Mar 5, 2018

thockin commented Mar 5, 2018

adnavare commented Mar 5, 2018

jhorwit2 left a comment • edited

Choose a reason for hiding this comment

jhorwit2 commented Mar 6, 2018

adnavare commented Mar 6, 2018

adnavare commented Mar 6, 2018

adnavare commented Mar 6, 2018

wlan0 commented Mar 7, 2018 • edited

wlan0 commented Mar 8, 2018

adnavare commented Mar 9, 2018

jhorwit2 commented Mar 9, 2018

adnavare commented Mar 9, 2018

mikedanese commented Apr 4, 2018

mikedanese Apr 4, 2018

Choose a reason for hiding this comment

wlan0 commented Apr 4, 2018

mikedanese commented Apr 4, 2018

mikedanese commented Apr 4, 2018

tallclair commented Apr 7, 2018

mikedanese commented Apr 7, 2018

k8s-ci-robot commented Apr 7, 2018

mikedanese commented Apr 9, 2018

adnavare commented Apr 9, 2018

adnavare commented Apr 9, 2018

k8s-github-robot commented Apr 9, 2018

jhorwit2 left a comment •

edited

wlan0 commented Mar 7, 2018 •

edited