For GCE, compute the external IP by using the local metadata server #8899

roberthbailey · 2015-05-27T23:53:30Z

This is in many ways a revert of #7530 but after auditing the code I found that this function is now only used to determine an address of the node where it is currently running.

roberthbailey · 2015-05-27T23:53:37Z

Still running e2e tests...

roberthbailey · 2015-05-27T23:55:49Z

@mbforbes FYI

cjcullen · 2015-05-28T00:02:26Z

pkg/cloudprovider/gce/gce.go

@@ -466,10 +468,10 @@ func (gce *GCECloud) getInstanceByName(name string) (*compute.Instance, error) {
 }

 // NodeAddresses is an implementation of Instances.NodeAddresses.
-func (gce *GCECloud) NodeAddresses(instance string) ([]api.NodeAddress, error) {
-	externalIP, err := gce.getExternalIP(instance)
+func (gce *GCECloud) NodeAddresses(_ string) ([]api.NodeAddress, error) {


Can you add a TODO to rename/redefine this function across the implementations of the interface to make it clear that it is getting the current instance's Addresses.

I've added the TODO to the cloud provider interface file rather than here.

server. This is in many ways a revert of kubernetes#7530 but after auditing the code I found that this function is now only used to determine an address of the node where it is currently running.

ghost · 2015-05-28T03:37:49Z

I'm pretty uncomfortable merging this as is, as the function NodeAddresses(instance string) just throws away it's argument and doesn't actually do what the interface definition says it should. Is there a good reason to defer fixing that until later? I'd feel better removing the parameter from the interface definition, and fixing whatever breakage that results in. I don't want to be pedantic, but I guess I've come across too many TODO comments at the heart of system bugs recently. I could be convinced otherwise if the TODO comment was replaced by a P1 v1.0 issue.

roberthbailey · 2015-05-28T04:27:01Z

@quinton-hoole that's a perfectly fair response. I didn't have time to do a more proper fix -- this was meant to stop the bleeding in our jenkins project. I'm happy to open a P1 issue to update/change the API definition if you can find an owner to carry it through to completion.

ghost · 2015-05-28T05:00:38Z

Lets leave the scalability test disabled to save GCE QPS quota until we can
fix this PR properly (hopefully within a day or two), unless that
significantly inconveniences someone.

@piosz @wojtek @fgrzadkowski any objections?

Until a fixed version of this PR is merged, the scalability tests are causing us to exceed our GCE QPS quota in the project in which we run continuous integration (Jenkins) e2e tests. So if we can disable the scalability tests in Jenkins while we fix this, it would help.

roberthbailey · 2015-05-28T06:05:42Z

e2e test result:

Summarizing 5 Failures:

[Fail] Density [AfterEach] [Performance suite] should allow starting 30 pods per node 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/density.go:84

[Fail] PD [It] should schedule a pod w/ a RW PD, remove it, then schedule it on another host 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/pd.go:105

[Timeout...] Networking [It] should function for intra-pod communication 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/networking.go:244

[Panic!] Services [BeforeEach] should be able to change the type and nodeport settings of a service 
/usr/src/go/src/runtime/asm_amd64.s:401

[Fail] Density [AfterEach] [Performance suite] should allow starting 3 pods per node 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/density.go:84

Ran 52 of 60 Specs in 3513.927 seconds
FAIL! -- 47 Passed | 5 Failed | 1 Pending | 7 Skipped --- FAIL: TestE2E (3581.17s)
FAIL

ghost · 2015-05-28T06:18:54Z

Those are all flaky tests, consistent with the kubernetes-e2e-gce-flaky job in Jenkins.

Tomorrow I will mark them as such in the test definitions (as opposed to just in our Jenkins continuous integration config), so that everyone is consistently aware of what's currently flaky and what's not.

wojtek-t · 2015-05-28T08:28:10Z

@quinton-hoole
I completely agree that we should have scalability tests switched off until this PR is merged.

On the other hand, I have a very strong feeling that switching them on should be task of a very high priority - they are (at least partially) driving our optimizations efforts and can uncover possible regressions.
So I think that merging this PR as is (since your comment is not about correctness of this change but about maintainability/readability/...) and creating a v1.0 issue to fix it is what we really should do here.

@fgrzadkowski - what do you think?

fgrzadkowski · 2015-05-28T14:46:41Z

@roberthbailey One suggestion to address Quinton's suggestion is to check if the node name matches name of the local machine (is it possible using metadata server or something else which is lightweight?) and return error if it's not. That would not break interface (in some sense) and should be easy to fix.

I agree with @wojtek-t that re-enabling scalability tests is very important for us and if my suggestion doesn't make sense, we should submit this PR and submit issue to address TODO.

ghost · 2015-05-28T15:29:26Z

OK, opened #8919 to fix this. Merging this PR in the mean time to stop the bleeding.

For GCE, compute the external IP by using the local metadata server

wojtek-t · 2015-05-28T15:30:45Z

Thanks @quinton-hoole - can we now enable scalability tests on Jenkins?

ghost · 2015-05-28T15:40:47Z

@wojtek-t Yes, I'll do that now. Thanks for reminding me 👍

mbforbes · 2015-05-28T17:52:24Z

Out of curiosity, how did this break? NatIP no longer correct?

ghost · 2015-05-28T18:16:44Z

Pre this PR, it just chewed up all of our API QPS quota on GCE, due to the large number of reads per node.

roberthbailey assigned ghost May 27, 2015

googlebot added the cla: yes label May 27, 2015

cjcullen reviewed May 28, 2015
View reviewed changes

For GCE, compute the external IP by using the local metadata

1dfaa93

server. This is in many ways a revert of kubernetes#7530 but after auditing the code I found that this function is now only used to determine an address of the node where it is currently running.

roberthbailey force-pushed the ip-from-metadata branch from b788a75 to 1dfaa93 Compare May 28, 2015 00:42

wojtek-t mentioned this pull request May 28, 2015

Failing performance tests on Jenkins #7561

Closed

ghost mentioned this pull request May 28, 2015

Fix #8899 #8919

Closed

ghost pushed a commit that referenced this pull request May 28, 2015

Merge pull request #8899 from roberthbailey/ip-from-metadata

9b67435

For GCE, compute the external IP by using the local metadata server

ghost merged commit 9b67435 into kubernetes:master May 28, 2015

roberthbailey unassigned ghost Aug 12, 2015

roberthbailey deleted the ip-from-metadata branch August 20, 2015 21:36

jiahuif mentioned this pull request May 4, 2020

remove obsolete TODO item of NodeAddresses #90742

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For GCE, compute the external IP by using the local metadata server #8899

For GCE, compute the external IP by using the local metadata server #8899

roberthbailey commented May 27, 2015

roberthbailey commented May 27, 2015

roberthbailey commented May 27, 2015

cjcullen May 28, 2015

roberthbailey May 28, 2015

ghost commented May 28, 2015

roberthbailey commented May 28, 2015

ghost commented May 28, 2015

roberthbailey commented May 28, 2015

ghost commented May 28, 2015

wojtek-t commented May 28, 2015

fgrzadkowski commented May 28, 2015

ghost commented May 28, 2015

wojtek-t commented May 28, 2015

ghost commented May 28, 2015

mbforbes commented May 28, 2015

ghost commented May 28, 2015

For GCE, compute the external IP by using the local metadata server #8899

For GCE, compute the external IP by using the local metadata server #8899

Conversation

roberthbailey commented May 27, 2015

roberthbailey commented May 27, 2015

roberthbailey commented May 27, 2015

cjcullen May 28, 2015

Choose a reason for hiding this comment

roberthbailey May 28, 2015

Choose a reason for hiding this comment

ghost commented May 28, 2015

roberthbailey commented May 28, 2015

ghost commented May 28, 2015

roberthbailey commented May 28, 2015

ghost commented May 28, 2015

wojtek-t commented May 28, 2015

fgrzadkowski commented May 28, 2015

ghost commented May 28, 2015

wojtek-t commented May 28, 2015

ghost commented May 28, 2015

mbforbes commented May 28, 2015

ghost commented May 28, 2015