Add InstanceExistsByProviderID to cloud provider interface for CCM #51087

prydie · 2017-08-22T08:20:56Z

What this PR does / why we need it:

Currently, MonitorNode() in the node controller checks with the CCM if a node still exists by calling ExternalID(nodeName). ExternalID is supposed to return the provider id of a node which is not supported on every cloud. This means that any clouds who cannot infer the provider id by the node name from a remote location will never remove nodes that no longer exist.

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

Special notes for your reviewer:

We'll want to create a subsequent issue to track the implementation of these two new methods in the cloud providers.

Release note:

Adds `InstanceExists` and `InstanceExistsByProviderID` to cloud provider interface for the cloud controller manager

/cc @wlan0 @thockin @andrewsykim @luxas @jhorwit2

/area cloudprovider
/sig cluster-lifecycle

k8s-ci-robot · 2017-08-22T08:20:58Z

@prydie: GitHub didn't allow me to request PR reviews from the following users: jhorwit2.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it:

Currently, MonitorNode() in the node controller checks with the CCM if a node still exists by calling ExternalID(nodeName). ExternalID is supposed to return the provider id of a node which is not supported on every cloud. This means that any clouds who cannot infer the provider id by the node name from a remote location will never remove nodes that no longer exist.

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

Special notes for your reviewer:

We'll want to create a subsequent issue to track the implementation of these two new methods in the cloud providers.

Release note:
Adds `InstanceExists` and `InstanceExistsByProviderID` to cloud provider interface for the cloud controller manager
/cc @wlan0 @thockin @andrewsykim @luxas @jhorwit2

/area cloudprovider
/sig cluster-lifecycle

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.

k8s-ci-robot · 2017-08-22T08:21:03Z

Hi @prydie. 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 /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.

I understand the commands that are listed here.

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 · 2017-08-22T12:16:49Z

pkg/cloudprovider/cloud.go

+	InstanceExists(nodeName types.NodeName) (bool, error)
+	// InstanceExistsByProviderID returns true if the instance for the given provider id still is running.
+	// If false is returned with no error, the instance will be immediately deleted.
+	// Note: The `cloudprovider.InstanceNotFound` error should not be returned here to notify


should not? Please clarify the comment here, it's not obvious what you mean

@luxas do you think I should just omit that? I added it originally since people are use to using that error to denote the instance is gone, so I wanted to call out to not use that. However, calling it out in hindsight seems more confusing. Thoughts on just removing that note?

luxas · 2017-08-22T12:18:57Z

pkg/controller/cloud/node_controller.go

@@ -372,6 +382,18 @@ func excludeTaintFromList(taints []v1.Taint, toExclude v1.Taint) []v1.Taint {
 	return newTaints
 }

+func ensureNodeExistsByProviderIDOrName(instances cloudprovider.Instances, node *v1.Node) (bool, error) {


add a comment and/or unit test here please

luxas · 2017-08-22T12:20:43Z

/ok-to-test
/assign @thockin @wlan0

jhorwit2 · 2017-08-22T13:12:39Z

/retest

jhorwit2 · 2017-08-22T13:15:33Z

Oh whoops my changes aren't being picked up... weird.

wlan0 · 2017-08-22T14:13:12Z

@prydie @jhorwit2

Thanks for the PR. I reviewed it. The problem with this PR is the ByNameOrID pattern has been followed, but without a reason. The ByName method does not add any value.

Aren't we all moving to addressing nodes by ProviderID? Woudln't it suffice to just add the ByProviderID method?

Additionally, this is breaking existing functionality. Have you tested this change?

Thirdly, We had the ByNameOrProviderID pattern to allow CCM to work without breaking existing functionality. If I merge this, CCM will stop working.

jhorwit2 · 2017-08-22T14:16:53Z

@wlan0 This is the same pattern followed in the zones PR that you gave your stamp of approval on. Should we opt to remove ByName in both to keep things consistent in the CCM? It also broke existing functionality with the CCM by removing the Zones() call.

The only existing functionality that would be broken is for the CCM, correct? That's the only place the CloudNodeController is created.

wlan0 · 2017-08-22T14:29:06Z

@jhorwit2 The Zones PR required both methods. The GetZone() method did not take any parameter because it was always called from kubelet/always from the node which was querying the zone. There was no existing method for getting zones for a node from a controller either ByName or byID, and since providerID was not always supported, it made sense to have both methods there. That is why it got my stamp of approval.

In this case, externalID is already a method that already exists for finding node existence by Name. Therefore it is unnecessary to add another one.

wlan0 · 2017-08-22T14:31:12Z

The only existing functionality that would be broken is for the CCM, correct? That's the only place the CloudNodeController is created.

@jhorwit2 Yes. This PR would break the functionality of the CCM.

andrewsykim · 2017-08-22T14:34:51Z

For what it's worth, this would definitely break DigitalOcean's CCM if I did a vendor update, but it would be acceptable since CCM is technically alpha still (right?).

Another thing to consider though is that we'll be making it more difficult for existing cloud providers to migrate since we're adding more friction to integrate CCM (having to implement InstanceExists* methods). On the plus side, adding this change now will mean we can deprecate ExternalID faster since there will be less users of it in the future.

jhorwit2 · 2017-08-22T14:37:34Z

Zones technically breaks existing functionality as well just in a different way. Any existing clusters managing by the CCM will have nodes with a random failure zone/region value set by the current CCM implementations i've seen. The CCM has no way to correct that now with the updated Zone logic.

I'm all for changing to only using provider id and moving to one method. I guess the point I'm trying to make is why support continue to ExternalID in the CCM if it's a deprecated method?

If you want I'll change the current check to something like

func ensureNodeExistsByProviderIDOrName(instances cloudprovider.Instances, node *v1.Node) (bool, error) {
	exists, err := instances.InstanceExistsByProviderID(node.Spec.ProviderID)
	if err != nil {
		providerIDErr := err
		_, err = instances.ExternalID(types.NodeName(node.Name))
		if err == cloudprovider.InstanceNotFound {
			return false, nil
		}
		if err != nil {
			return false, fmt.Errorf("InstanceExists: Error fetching by providerID: %v Error fetching by NodeName: %v", providerIDErr, err)
		}
		return true, nil
	}
	
	return exists, nil
}

so we keep using ExternalID although I don't really agree with keeping it around in the CCM. Any cloud providers that move to the CCM will already have to implement new methods (zones for example), so why not also implement this to have the desired state up front and drop support for the ExternalID call?

wlan0 · 2017-08-22T14:42:40Z

Zones technically breaks existing functionality as well just in a different way. Any existing clusters managing by the CCM will have nodes with a random failure zone/region value set by the current CCM implementations i've seen. The CCM has no way to correct that now with the updated Zone logic.

Yes, this needs to be fixed. The zones API does not add this bug though. Actually, It gets one step closer to fixing this.

Also, its not a random zone/region that gets assigned. It is the zone in which the CCM is running.

I'm all for changing to only using provider id and moving to one method. I guess the point I'm trying to make is why support continue to ExternalID in the CCM if it's a deprecated method?

It is not deprecated. Where did you get this information?

wlan0 · 2017-08-22T14:44:39Z

For what it's worth, this would definitely break DigitalOcean's CCM if I did a vendor update, but it would be acceptable since CCM is technically alpha still (right?).

Yes, and others. It is not acceptable to introduce breaking changes especially since we don't have all the cloudvendors ready to fill the voids that fix the breaks. It is always advisable to make non-breaking changes.

Another thing to consider though is that we'll be making it more difficult for existing cloud providers to migrate since we're adding more friction to integrate CCM (having to implement InstanceExists* methods). On the plus side, adding this change now will mean we can deprecate ExternalID faster since there will be less users of it in the future.

yes that is correct. Also, deprecation of ExternalID is not something that has been decided. Why are we all working under that assumption?

andrewsykim · 2017-08-25T19:23:59Z

pkg/cloudprovider/providers/aws/aws.go

@@ -1101,6 +1101,11 @@ func (c *Cloud) ExternalID(nodeName types.NodeName) (string, error) {
 	return orEmpty(instance.InstanceId), nil
 }

+// InstanceExistsByProviderID returns true if the instance with the given provider id still exists and is running.


Don't think it hurts to add comments from the cloud interface here for visibility

If false is returned with no error, the instance will be immediately deleted.

andrewsykim · 2017-08-25T19:31:30Z

pkg/controller/cloud/node_controller_test.go

+			instances, _ := fc.Instances()
+			exists, err := ensureNodeExistsByProviderIDOrName(instances, tc.node)
+			if err != nil {
+				t.Fatal(err)


Generally, t.Fatal is for when you fail to setup a test properly, t.Error for when a test check fails

andrewsykim · 2017-08-25T19:33:02Z

pkg/controller/cloud/node_controller_test.go

+				t.Fatalf("expected exist by provider id to be `%t` but got `%t`", tc.existsByProviderID, exists)
+			}
+
+			if tc.existsByNodeName && tc.existsByNodeName != exists {


Just tc.existsByNodeName != exists is fine?

andrewsykim · 2017-08-25T19:34:28Z

pkg/controller/cloud/node_controller_test.go

+				t.Fatalf("expected cloud provider methods `%v` to be called but `%v` was called ", tc.expectedCalls, fc.Calls)
+			}
+
+			if tc.existsByProviderID && tc.existsByProviderID != exists {


Just tc.existsByProviderID != exists should be fine here?

That won't work since you have two things to check to determine existence.

Test Case: if exists should be true because it exists by node name and not provider it, this test would fail.

I didn't see a previous way people were testing these "by provider id or node name" methods, so if you have a better idea i'm all ears :)

andrewsykim · 2017-08-25T19:35:44Z

pkg/controller/cloud/node_controller_test.go

+				t.Fatalf("expected exist by node name to be `%t` but got `%t`", tc.existsByNodeName, exists)
+			}
+
+			if !tc.existsByNodeName && !tc.existsByProviderID && exists {


This check seems unesscary, if tc.existsByNodeName and tc.existsByProviderID are properly set then the previous checks should have failed the test?

luxas · 2017-08-25T20:20:16Z

ping me when all other reviewers' comments are addressed please

jhorwit2 · 2017-08-25T20:27:32Z

@luxas i've addressed all the comments.

thockin · 2017-08-25T21:14:42Z

Assign me AFTER it is lgtm'ed :)

luxas

Some small-ish comments and clarifications
On a high level, LGTM though

luxas · 2017-08-25T21:16:45Z

pkg/cloudprovider/providers/aws/aws.go

@@ -1101,6 +1101,12 @@ func (c *Cloud) ExternalID(nodeName types.NodeName) (string, error) {
 	return orEmpty(instance.InstanceId), nil
 }

+// InstanceExistsByProviderID returns true if the instance with the given provider id still exists and is running.
+// If false is returned with no error, the instance will be immediately deleted.


... by the cloud controller manager?

luxas · 2017-08-25T21:17:35Z

pkg/controller/cloud/node_controller.go

-						}(node.Name)
-					}
-					glog.Errorf("Error getting node data from cloud: %v", err)
+				exists, err := ensureNodeExistsByProviderIDOrExternalID(instances, node)


I thought we deprecated the ExternalID name everywhere, why not ensureNodeExistsByProviderIDOrNodeName?

:) that's what I had before, but changed it based on @wlan0's feedback.

luxas · 2017-08-25T21:19:23Z

pkg/controller/cloud/node_controller.go

+	exists, err := instances.InstanceExistsByProviderID(node.Spec.ProviderID)
+	if err != nil {
+		providerIDErr := err
+		_, err = instances.ExternalID(types.NodeName(node.Name))


why not instances.NodeName like we talked about?
Isn't that implemented yet? (Did it stall out in a PR somewhere?)

I don't recall us discussing that. Mind filling me in??

I thought we landed on keeping the ExistsByProviderID & ExternalID checks. Then in the future we can add a ExistsByNodeName if we want to, but avoid adding that method now since it's a duplicate of ExternalID.

hah, never mind 😄
That was the case indeed, recalled incorrectly

luxas

LGTM
/assign @thockin

luxas · 2017-08-25T23:04:45Z

/retest

jhorwit2 · 2017-08-25T23:49:31Z

bazel flake: #51379

thockin · 2017-08-25T23:56:09Z

/lgtm
/approve

k8s-github-robot · 2017-08-25T23:56:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: prydie, thockin

Associated issue: 50985

The full list of commands accepted by this bot can be found here.

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

~~pkg/cloudprovider/OWNERS~~ [thockin]
~~pkg/controller/cloud/OWNERS~~ [thockin]
~~pkg/volume/cinder/OWNERS~~ [thockin]

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

jhorwit2 · 2017-08-26T00:03:16Z

/test pull-kubernetes-e2e-gce-bazel

jhorwit2 · 2017-08-26T01:15:37Z

/test pull-kubernetes-e2e-gce-etcd3

…

On Aug 25, 2017, at 8:16 PM, k8s-ci-robot ***@***.***> wrote: @prydie: The following tests failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-bazel cf75c49 link /test pull-kubernetes-e2e-gce-bazel pull-kubernetes-e2e-gce-etcd3 cf75c49 link /test pull-kubernetes-e2e-gce-etcd3 Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

k8s-github-robot · 2017-08-26T13:43:29Z

Automatic merge from submit-queue (batch tested with PRs 51174, 51363, 51087, 51382, 51388)

@wlan0

…cm-instance-exists Automatic merge from submit-queue (batch tested with PRs 51174, 51363, 51087, 51382, 51388) Add InstanceExistsByProviderID to cloud provider interface for CCM **What this PR does / why we need it**: Currently, [`MonitorNode()`](https://github.com/kubernetes/kubernetes/blob/02b520f0a40be2056d91fc0661c2b4fdb2694c30/pkg/controller/cloud/nodecontroller.go#L240) in the node controller checks with the CCM if a node still exists by calling `ExternalID(nodeName)`. `ExternalID` is supposed to return the provider id of a node which is not supported on every cloud. This means that any clouds who cannot infer the provider id by the node name from a remote location will never remove nodes that no longer exist. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes kubernetes#50985 **Special notes for your reviewer**: We'll want to create a subsequent issue to track the implementation of these two new methods in the cloud providers. **Release note**: ```release-note Adds `InstanceExists` and `InstanceExistsByProviderID` to cloud provider interface for the cloud controller manager ``` /cc @wlan0 @thockin @andrewsykim @luxas @jhorwit2 /area cloudprovider /sig cluster-lifecycle

k8s-ci-robot requested review from wlan0 and thockin August 22, 2017 08:20

k8s-ci-robot added the area/cloudprovider label Aug 22, 2017

k8s-ci-robot requested a review from andrewsykim August 22, 2017 08:20

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 22, 2017

k8s-ci-robot requested a review from luxas August 22, 2017 08:20

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 22, 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 Aug 22, 2017

k8s-github-robot assigned caesarxuchao and mikedanese Aug 22, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 22, 2017

luxas reviewed Aug 22, 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 Aug 22, 2017

k8s-ci-robot assigned thockin and wlan0 Aug 22, 2017

jhorwit2 force-pushed the for/upstream/master/ccm-instance-exists branch from a620a66 to 8fc4a7c Compare August 22, 2017 12:44

k8s-ci-robot assigned thockin Aug 25, 2017

prydie changed the title ~~Add InstanceExists* methods to cloud provider interface for CCM~~ Add InstanceExistsByProviderID to cloud provider interface for CCM Aug 25, 2017

caesarxuchao removed their assignment Aug 25, 2017

andrewsykim reviewed Aug 25, 2017

View reviewed changes

address test & doc comments

3528ceb

refactor method name as per comments

82a69b2

thockin assigned thockin and unassigned thockin Aug 25, 2017

luxas reviewed Aug 25, 2017

View reviewed changes

change godoc based on feedback from luxas

cf75c49

luxas reviewed Aug 25, 2017

View reviewed changes

luxas approved these changes Aug 25, 2017

View reviewed changes

luxas added this to the v1.8 milestone Aug 25, 2017

k8s-ci-robot assigned thockin Aug 25, 2017

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2017

k8s-github-robot merged commit 27fbb68 into kubernetes:master Aug 26, 2017

FengyunPan mentioned this pull request Aug 26, 2017

Implement InstanceExistsByProviderID() for all the cloud providers #51406

Closed

Add InstanceExistsByProviderID to cloud provider interface for CCM #51087

Add InstanceExistsByProviderID to cloud provider interface for CCM #51087

Conversation

prydie commented Aug 22, 2017

k8s-ci-robot commented Aug 22, 2017

k8s-ci-robot commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Aug 22, 2017

jhorwit2 commented Aug 22, 2017

jhorwit2 commented Aug 22, 2017

wlan0 commented Aug 22, 2017 • edited Loading

jhorwit2 commented Aug 22, 2017 • edited Loading

wlan0 commented Aug 22, 2017 • edited Loading

wlan0 commented Aug 22, 2017

andrewsykim commented Aug 22, 2017

jhorwit2 commented Aug 22, 2017

wlan0 commented Aug 22, 2017

wlan0 commented Aug 22, 2017

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

Choose a reason for hiding this comment

luxas commented Aug 25, 2017 • edited Loading

jhorwit2 commented Aug 25, 2017

thockin commented Aug 25, 2017

luxas 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

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

luxas commented Aug 25, 2017

jhorwit2 commented Aug 25, 2017

thockin commented Aug 25, 2017

k8s-github-robot commented Aug 25, 2017

jhorwit2 commented Aug 26, 2017

jhorwit2 commented Aug 26, 2017 via email

k8s-github-robot commented Aug 26, 2017

wlan0 commented Aug 22, 2017 •

edited

Loading

jhorwit2 commented Aug 22, 2017 •

edited

Loading

wlan0 commented Aug 22, 2017 •

edited

Loading

luxas commented Aug 25, 2017 •

edited

Loading