[GCE Discovery] Automatically set project-id and zone #33721

vladimirdolzhenko · 2018-09-14T15:36:46Z

Settings cloud.gce.project_id, cloud.gce.zone are not mandatory - they could be automatically set from a gce metadata server. Picking up the default zone is useful for cases like a single region.

Details on impl differences with GCE:
com.google.cloud.ServiceOptions#getDefaultProjectId tries to resolve project based on

system property GOOGLE_CLOUD_PROJECT (or legacy system property GCLOUD_PROJECT)
environment variable GOOGLE_CLOUD_PROJECT or GCLOUD_PROJECT
from http://metadata.google.internal/computeMetadata/v1/project/project-id

This PR allows without settings to fallback to retrieve info from gce metadata server

Closes #13618

Closes elastic#13618

elasticmachine · 2018-09-14T15:36:47Z

Pinging @elastic/es-distributed

vladimirdolzhenko · 2018-09-15T10:48:51Z

@elasticmachine test this please

bleskes · 2018-09-15T13:46:24Z

@vladimirdolzhenko thanks for picking this up! do you mind updating the PR title to indicate it relates to GCE discovery and maybe add a sentence or two to the descriptiont telling people what zone and project id are used for and why this is a good default?

vladimirdolzhenko · 2018-09-18T06:52:52Z

@elasticmachine test this please

tlrx

I left some minor comments. As a follow up I think we should upgrade the SDK here. And do you think that we could have a fixture based integration test (QA) for this plugin?

plugins/discovery-gce/src/main/java/org/elasticsearch/cloud/gce/GceInstancesServiceImpl.java

docs/plugins/discovery-gce.asciidoc

…ables

…s from metadata service

tlrx

LGTM - Did you test it on GCE?

vladimirdolzhenko · 2018-09-19T11:24:18Z

tested on GCE:
✅ project-id works
❌ google-compute-default-zone is not there

dadoonet · 2018-09-19T11:40:57Z

May be that changed since I opened the original issue. :)

vladimirdolzhenko · 2018-09-21T14:08:08Z

google-compute-default-zone metaattribute appeared after specifying default zone for the project :

AccessControlException happens on testing in a real GCE, to fix it QA test was added (two settings - cloud.gce.host and cloud.gce.root_url are exposed for that).

vladimirdolzhenko · 2018-09-24T07:14:40Z

@elasticmachine test this please

vladimirdolzhenko · 2018-09-26T07:54:26Z

@tlrx I'd like to ask you to have another look into this PR as I've spotted an issue on a real GCE and therefore added QA tests (and performed manual tests after fix).

One think I don't feel quite comfortable is to exposing two new settings - it is not possible to run QA cluster w/o them.

tlrx

It looks good, I left some minor comments. That's a very good thing that the QA test spotted an issue during manual tests, well done!

One think I don't feel quite comfortable is to exposing two new settings - it is not possible to run QA cluster w/o them

I agree... Maybe we could use a system property here (something similar to es.allow_insecure_settings) that would register the test settings only when the system property is set? In regular test classes you could still extend the GceDiscoveryPlugin to register the extra settings.

@ywelsch Can you give an opinion? And look at the PR too? Thanks :)

tlrx · 2018-09-28T07:21:18Z

plugins/discovery-gce/qa/gce/build.gradle

+    testCompile project(path: ':plugins:discovery-gce', configuration: 'runtime')
+}
+
+/** A task to start the GCEFixture which emulates an GCE service **/


nit: an -> a

done: ba608f7

tlrx · 2018-09-28T07:22:23Z

plugins/discovery-gce/qa/gce/src/test/java/org/elasticsearch/cloud/gce/GCEFixture.java

+import static java.nio.charset.StandardCharsets.UTF_8;
+
+/**
+ * {@link GCEFixture} is a fixture that emulates an GCE service.


as well in ba608f7

tlrx · 2018-09-28T07:24:14Z

plugins/discovery-gce/qa/gce/src/test/java/org/elasticsearch/cloud/gce/GCEFixture.java

+                commonHeaderConsumer.accept(headers);
+
+                final StringBuilder builder = new StringBuilder();
+                builder.append("{\"id\": \"test-instances\",\"items\":[");


Could you have used a jsonBuilder() instead?

sure: 67f6b22

tlrx · 2018-09-28T07:36:34Z

plugins/discovery-gce/src/test/java/org/elasticsearch/cloud/gce/GceInstancesServiceTests.java

+
+@SuppressForbidden(reason = "use http server")
+public class GceInstancesServiceTests extends ESTestCase {
+    private static HttpServer httpServer;


Now we have a QA test do we really need this one?

agree, unit test has no additional value with qa test, dropped in cf4f744

tlrx · 2018-09-28T07:41:16Z

...lasticsearch/discovery/gce/compute/v1/projects/metadataserver/zones/europe-west1-b/instances

@@ -0,0 +1,36 @@
+{


I think that we should replace those JSON files by simple code in the test class (not in this PR). I'm not even sure that they are all used.

they are used in GceDiscoveryTests#testMetadataServerValues

https://github.com/elastic/elasticsearch/pull/33721/files#diff-9a2669df7c9d3ea8a5a5100ee4f78951R274

tlrx · 2018-09-28T07:42:31Z

plugins/discovery-gce/qa/gce/build.gradle

+    numNodes = gceNumberOfNodes
+    plugin ':plugins:discovery-gce'
+    setting 'discovery.zen.hosts_provider', 'gce'
+    setting 'cloud.gce.host', "http://${-> gceFixture.addressAndPort}"


Can you add a small comment about what is each setting here?

done aa7c93b

…ile unit tests don't spot all issues

…se GCE-specific settings: `cloud.gce.host` and `cloud.gce.root_url`

vladimirdolzhenko · 2018-09-28T11:56:09Z

@tlrx in 1c2a66b I've added a system property es.allow_reroute_gce_settings to expose for the test purposes those two GCE-specific settings, so they are not exposed by default

tlrx

LGTM, thanks @vladimirdolzhenko

...s/discovery-gce/src/main/java/org/elasticsearch/plugin/discovery/gce/GceDiscoveryPlugin.java

docs/plugins/discovery-gce.asciidoc

ywelsch · 2018-10-01T08:30:14Z

docs/plugins/discovery-gce.asciidoc

-     https://developers.google.com/compute/docs/zones#available[GCE supported zones].
+     helps to retrieve instances running in a given zone.
+     It should be one of the https://developers.google.com/compute/docs/zones#available[GCE supported zones].
+     If the value is not specified, default zone is picked up from a metadata server.


Same here:

By default the zone will be derived from the instance metadata

thanks, this and other doc related comments are applied in 1159638

ywelsch · 2018-10-01T08:40:04Z

plugins/discovery-gce/src/main/java/org/elasticsearch/cloud/gce/GceInstancesServiceImpl.java

@@ -180,6 +234,16 @@ public synchronized Compute client() {
        return this.client;
    }

+    @Override
+    public String project() {


call this method projectId?

addressed in 38cdcc0

vladimirdolzhenko · 2018-10-03T09:36:43Z

thanks @tlrx and @ywelsch

Fetch default values for project-id and zone from metadata server Closes #13618 (cherry picked from commit a7f62ee)

Fetch default values for project-id and zone from metadata server Closes #13618

Automatically set project-id and zone

c79ba8c

Closes elastic#13618

vladimirdolzhenko added >enhancement :Distributed Coordination/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure v7.0.0 labels Sep 14, 2018

vladimirdolzhenko requested a review from tlrx September 14, 2018 15:37

adjust doc: project id and zone are not mandatory

151207e

vladimirdolzhenko changed the title ~~Automatically set project-id and zone~~ [GCE Discovery] Automatically set project-id and zone Sep 15, 2018

tlrx reviewed Sep 18, 2018

View reviewed changes

Vladimir Dolzhenko added 7 commits September 18, 2018 10:49

add the reason of copy-n-paste from GCE SDK

9cb840c

headerContainsMetadataFlavor test is added

aff5104

note that it doesn't try to use any system properties and/or env vari…

6191a7d

…ables

imports!

977343a

headerContainsMetadataFlavor test is added

5d62b0c

use diff GceMetadataService.GCE_HOST to avoid resolving default value…

be5a6db

…s from metadata service

add test with a real http server for testMetadataServerValues

68bd1bc

vladimirdolzhenko requested a review from tlrx September 18, 2018 12:39

tlrx approved these changes Sep 19, 2018

View reviewed changes

Added QA test for discovery-gce

db9fa8e

some fixture polish

f2e98d5

Merge remote-tracking branch 'remotes/origin/master' into 13618

6ed4957

vladimirdolzhenko requested a review from tlrx September 26, 2018 07:52

tlrx reviewed Sep 28, 2018

View reviewed changes

Vladimir Dolzhenko added 5 commits September 28, 2018 10:10

drop GceInstancesServiceTests as QA test covers same functionality wh…

cf4f744

…ile unit tests don't spot all issues

grammar fix: a gce service

ba608f7

use jsonBuilder to generate json responses

67f6b22

explain gce specific settings rerouted to fixture

aa7c93b

system property es.allow_reroute_gce_settings could be used to expo…

1c2a66b

…se GCE-specific settings: `cloud.gce.host` and `cloud.gce.root_url`

Merge remote-tracking branch 'remotes/origin/master' into 13618

66404ca

tlrx approved these changes Oct 1, 2018

View reviewed changes

...s/discovery-gce/src/main/java/org/elasticsearch/plugin/discovery/gce/GceDiscoveryPlugin.java Outdated Show resolved Hide resolved

ywelsch approved these changes Oct 1, 2018

View reviewed changes

Vladimir Dolzhenko added 3 commits October 1, 2018 12:09

add the test purposes only note to ALLOW_REROUTE_GCE_SETTINGS

cb5ae5f

polished doc wording

1159638

renamed project() -> projectId()

38cdcc0

vladimirdolzhenko merged commit a7f62ee into elastic:master Oct 3, 2018

vladimirdolzhenko deleted the 13618 branch October 3, 2018 09:38

vladimirdolzhenko added a commit that referenced this pull request Oct 3, 2018

[GCE Discovery] Automatically set project-id and zone (#33721)

68cdb8b

Fetch default values for project-id and zone from metadata server Closes #13618 (cherry picked from commit a7f62ee)

vladimirdolzhenko added the v6.5.0 label Oct 3, 2018

kcm pushed a commit that referenced this pull request Oct 30, 2018

[GCE Discovery] Automatically set project-id and zone (#33721)

dc41c46

Fetch default values for project-id and zone from metadata server Closes #13618

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GCE Discovery] Automatically set project-id and zone #33721

[GCE Discovery] Automatically set project-id and zone #33721

vladimirdolzhenko commented Sep 14, 2018 •

edited

Loading

elasticmachine commented Sep 14, 2018

vladimirdolzhenko commented Sep 15, 2018

bleskes commented Sep 15, 2018

vladimirdolzhenko commented Sep 18, 2018

tlrx left a comment

tlrx left a comment

vladimirdolzhenko commented Sep 19, 2018 •

edited

Loading

dadoonet commented Sep 19, 2018 •

edited

Loading

vladimirdolzhenko commented Sep 21, 2018

vladimirdolzhenko commented Sep 24, 2018

vladimirdolzhenko commented Sep 26, 2018

tlrx left a comment

tlrx Sep 28, 2018

vladimirdolzhenko Sep 28, 2018

tlrx Sep 28, 2018

vladimirdolzhenko Sep 28, 2018

tlrx Sep 28, 2018

vladimirdolzhenko Sep 28, 2018

tlrx Sep 28, 2018

vladimirdolzhenko Sep 28, 2018

tlrx Sep 28, 2018

vladimirdolzhenko Sep 28, 2018

tlrx Sep 28, 2018

vladimirdolzhenko Sep 28, 2018

vladimirdolzhenko commented Sep 28, 2018

tlrx left a comment

ywelsch Oct 1, 2018

vladimirdolzhenko Oct 1, 2018 •

edited

Loading

ywelsch Oct 1, 2018

vladimirdolzhenko Oct 1, 2018

vladimirdolzhenko commented Oct 3, 2018

[GCE Discovery] Automatically set project-id and zone #33721

[GCE Discovery] Automatically set project-id and zone #33721

Conversation

vladimirdolzhenko commented Sep 14, 2018 • edited Loading

elasticmachine commented Sep 14, 2018

vladimirdolzhenko commented Sep 15, 2018

bleskes commented Sep 15, 2018

vladimirdolzhenko commented Sep 18, 2018

tlrx left a comment

Choose a reason for hiding this comment

tlrx left a comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Sep 19, 2018 • edited Loading

dadoonet commented Sep 19, 2018 • edited Loading

vladimirdolzhenko commented Sep 21, 2018

vladimirdolzhenko commented Sep 24, 2018

vladimirdolzhenko commented Sep 26, 2018

tlrx 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

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

vladimirdolzhenko commented Sep 28, 2018

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirdolzhenko Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Oct 3, 2018

vladimirdolzhenko commented Sep 14, 2018 •

edited

Loading

vladimirdolzhenko commented Sep 19, 2018 •

edited

Loading

dadoonet commented Sep 19, 2018 •

edited

Loading

vladimirdolzhenko Oct 1, 2018 •

edited

Loading