cloud-gce plugin should check `discovery.type` #13809

xuzha · 2015-09-25T23:43:11Z

closes #13614
This commmit adds check discovery.type and other required parameters before
loading gce plugin.

This change make gce plugin behave like Azure:

if user doesn't set discovery type to gce, ES would not load gce modules.
if user does set discovery type to gce but without all parameters we need, es would fail to start.

dadoonet · 2015-09-26T02:42:49Z

plugins/cloud-gce/src/main/java/org/elasticsearch/plugin/cloud/gce/CloudGcePlugin.java

@@ -56,7 +59,7 @@ public String description() {
    @Override
    public Collection<Module> nodeModules() {
        List<Module> modules = new ArrayList<>();
-        if (settings.getAsBoolean("cloud.enabled", true)) {
+        if (settings.getAsBoolean("cloud.enabled", true) && GceModule.isDiscoveryReady(settings, logger)) {


Not really related to this change but we can remove now the check for cloud.enabled

dadoonet · 2015-09-26T02:47:12Z

Thanks for doing this!
Left some comments. It looks very good to me.
Actually I should do the same test for discovery.type in azure and AWS now projects are split.

rjernst · 2015-09-26T03:55:04Z

Is this really necessary? I don't like plugins checking a setting that is not theirs. The gce plugin registers itself as a discovery type. It is the discovery modules job to decide which discovery type to use.

xuzha · 2015-09-26T04:42:10Z

Thanks @dadoonet and @rjernst. I don't have strong opinion.

But I think we could remove checking discovery.type here. If gce have all required paras, then we could go ahead and register. What do you think @dadoonet.

dadoonet · 2015-09-26T06:27:46Z

Don't have a strong opinion as well.

Just thinking of what would be the consequences when we will remove the need for setting any cloud.gce property. Not a problem I guess.

Let's apply Ryan suggestion.

dadoonet · 2015-09-26T08:47:49Z

Actually, I like the fact we don't bind unnecessary modules when discovery is not set to gce.

That's what we do today for azure: https://github.com/elastic/elasticsearch/blob/master/plugins/discovery-azure/src/main/java/org/elasticsearch/cloud/azure/AzureDiscoveryModule.java#L79-L83

I think it's cleaner.

If we don't do that, in Azure, we will automatically start AzureComputeServiceImpl which will complain that some settings are not set. But may be in that case AzureComputeServiceImpl should detect that discovery.type is not set to azure and just don't log any warning in that case.

bleskes · 2015-09-26T17:02:39Z

@rjernst I agree with your sentiment. Do you see any way of allowing the plugin to bind extra services if the DiscoveryModule activated it ? there are things like network address resolution, adding unicast host providers etc.

@dadoonet long term it will be awesome if we can decouple things such that we won't need guice. The network part of the plugin can be registered all the time (people need to active it using gce in their setting) and maybe the unicast host provider can be create in GCEDiscovery on start (GCEDiscovery is now kinda empty)

rjernst · 2015-09-26T17:33:37Z

@bleskes I'm not sure we need extra services? I have two thoughts:

Can the unicast hosts list be provided through subclassing zen discovery?
Does zen discovery cover to many "things"? Should we have separate pluggable apis for master election (election.type?), ping service (ping.type?), and host discovery? Could this split be done now or are there tight couplings right now blocking a split like that?

bleskes · 2015-09-26T17:40:46Z

@rjernst re splitting discovery - yeah and I'm still thinking on what's the right split. these things are tied together. Breaking them up are not trivial at all. Note though that GCE only supplies configuration (i.e., list of hosts to ping), which makes it creating it from the discovery implementation an option (see note to david from earlier :))

rjernst · 2015-09-28T05:52:17Z

I'm fine for now with moving the hosts list to a protected method that plugins override on their subclass of zen (I think that's what you are suggesting)?

bleskes · 2015-09-28T06:58:48Z

I looked at the code and I think it will be a biggish change to move the unicast host providers from Guice into a method zen discovery. It will also imply that UnicastZenPing needs to move ZenDiscovery and ZenPingService need to be either moved away from Guice as well or have an extra method for late addition of ping types.

For the sake of moving forward, I suggest we proceed with the current approach where we check settings for now and try to work towards a better way of doing this in ZenDiscovery/DiscoveryModule.

On 28 Sep 2015, at 07:52, Ryan Ernst notifications@github.com wrote:

I'm fine for now with moving the hosts list to a protected method that plugins override on their subclass of zen (I think that's what you are suggesting)?

—
Reply to this email directly or view it on GitHub.

xuzha · 2015-09-28T08:26:48Z

Thanks a lot guys.
Updated and addressed some concerns @dadoonet.

dadoonet · 2015-09-29T13:27:23Z

It looks good to me

dadoonet · 2015-09-29T17:31:13Z

@xuzha I think we need it as well in 2.x and 2.0 branches. See https://github.com/elastic/elasticsearch/blob/2.0/plugins/cloud-gce/src/main/java/org/elasticsearch/plugin/cloud/gce/CloudGcePlugin.java#L74

We hit a similar issue with ec2 today on 2.0 branch. Could you check it and cherry pick this commit if needed to 2.0, 2.x branches? And add 2.0.0 and 2.1.0 labels to this PR?

xuzha · 2015-09-29T17:33:22Z

@dadoonet sure, I saw your message in hipchat, I think we should do it.

dadoonet · 2015-09-29T17:34:32Z

thanks for confirming. I added labels to this PR then.

xuzha · 2015-09-29T17:55:12Z

In 2.x and 2.0 now,

2.0 21092d9
2.x 9654e08

As done in elastic#13809 and in Azure, we should check that `discovery.type` is set to `ec2` before starting services. Closes elastic#13581.

As done in #13809 and in Azure, we should check that `discovery.type` is set to `ec2` before starting services. Closes #13581. (cherry picked from commit 314f074) (cherry picked from commit 466bac6)

As done in #13809 and in Azure, we should check that `discovery.type` is set to `ec2` before starting services. Closes #13581. (cherry picked from commit 314f074)

As done in #13809 and in Azure, we should check that `discovery.type` is set to `ec2` before starting services. Closes #13581. (cherry picked from commit 314f074) (cherry picked from commit 466bac6)

xuzha added >enhancement review labels Sep 25, 2015

dadoonet reviewed Sep 26, 2015
View reviewed changes

dadoonet mentioned this pull request Sep 26, 2015

Discovery-ec2 plugin should check discovery.type #13814

Merged

xuzha force-pushed the gce_logging branch from c9c7ee0 to fac7a16 Compare September 28, 2015 08:25

xuzha force-pushed the gce_logging branch 2 times, most recently from 415ce48 to 74fdb60 Compare September 28, 2015 22:32

xuzha force-pushed the gce_logging branch from 74fdb60 to af30c14 Compare September 29, 2015 17:25

dadoonet added the v5.0.0-alpha1 label Sep 29, 2015

dadoonet removed the review label Sep 29, 2015

dadoonet added v2.1.0 v2.0.0 labels Sep 29, 2015

xuzha mentioned this pull request Sep 29, 2015

[cloud-gce] warn message is displayed when not using gce discovery #13614

Closed

xuzha closed this Sep 29, 2015

xuzha force-pushed the gce_logging branch from af30c14 to 625d06c Compare September 29, 2015 17:51

xuzha deleted the gce_logging branch September 29, 2015 17:55

xuzha added v2.2.0 and removed v2.2.0 labels Oct 1, 2015

dadoonet added a commit that referenced this pull request Oct 2, 2015

discovery-ec2 plugin should check discovery.type

2b44dab

As done in #13809 and in Azure, we should check that `discovery.type` is set to `ec2` before starting services. Closes #13581. (cherry picked from commit 314f074)

clintongormley added v2.0.0-rc1 and removed v2.0.0 labels Oct 7, 2015

clintongormley removed the v2.0.0-rc1 label Nov 22, 2015

clintongormley added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Cloud GCE labels Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloud-gce plugin should check `discovery.type` #13809

cloud-gce plugin should check `discovery.type` #13809

xuzha commented Sep 25, 2015

dadoonet Sep 26, 2015

dadoonet commented Sep 26, 2015

rjernst commented Sep 26, 2015

xuzha commented Sep 26, 2015

dadoonet commented Sep 26, 2015

dadoonet commented Sep 26, 2015

bleskes commented Sep 26, 2015

rjernst commented Sep 26, 2015

bleskes commented Sep 26, 2015

rjernst commented Sep 28, 2015

bleskes commented Sep 28, 2015

xuzha commented Sep 28, 2015

dadoonet commented Sep 29, 2015

dadoonet commented Sep 29, 2015

xuzha commented Sep 29, 2015

dadoonet commented Sep 29, 2015

xuzha commented Sep 29, 2015

cloud-gce plugin should check discovery.type #13809

cloud-gce plugin should check discovery.type #13809

Conversation

xuzha commented Sep 25, 2015

dadoonet Sep 26, 2015

Choose a reason for hiding this comment

dadoonet commented Sep 26, 2015

rjernst commented Sep 26, 2015

xuzha commented Sep 26, 2015

dadoonet commented Sep 26, 2015

dadoonet commented Sep 26, 2015

bleskes commented Sep 26, 2015

rjernst commented Sep 26, 2015

bleskes commented Sep 26, 2015

rjernst commented Sep 28, 2015

bleskes commented Sep 28, 2015

xuzha commented Sep 28, 2015

dadoonet commented Sep 29, 2015

dadoonet commented Sep 29, 2015

xuzha commented Sep 29, 2015

dadoonet commented Sep 29, 2015

xuzha commented Sep 29, 2015

cloud-gce plugin should check `discovery.type` #13809

cloud-gce plugin should check `discovery.type` #13809