From 625d06cd766b9f61bba1bbc60ef0e9ba41eda101 Mon Sep 17 00:00:00 2001 From: xuzha Date: Tue, 29 Sep 2015 10:26:12 -0700 Subject: [PATCH] cloud-gce plugin should check `discovery.type` GCE plugin tries to start immediately gce discovery even if we don't set discovery.type. This commmit adds check `discovery.type` and other required parameters before loading gce plugin. closes #13614 --- .../cloud/gce/GceComputeService.java | 1 - .../discovery/gce/GceDiscovery.java | 4 +- .../gce/GceUnicastHostsProvider.java | 16 ---- .../plugin/cloud/gce/CloudGcePlugin.java | 57 +++++++++++++- .../gce/GceDiscoverySettingsTests.java | 78 +++++++++++++++++++ 5 files changed, 133 insertions(+), 23 deletions(-) create mode 100644 plugins/cloud-gce/src/test/java/org/elasticsearch/discovery/gce/GceDiscoverySettingsTests.java diff --git a/plugins/cloud-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeService.java b/plugins/cloud-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeService.java index f23ba0cdc3e..6ba857db17a 100644 --- a/plugins/cloud-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeService.java +++ b/plugins/cloud-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeService.java @@ -20,7 +20,6 @@ package org.elasticsearch.cloud.gce; import com.google.api.services.compute.model.Instance; -import org.elasticsearch.common.component.AbstractLifecycleComponent; import org.elasticsearch.common.component.LifecycleComponent; import java.util.Collection; diff --git a/plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceDiscovery.java b/plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceDiscovery.java index bad3b204310..f20d1c74f83 100755 --- a/plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceDiscovery.java +++ b/plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceDiscovery.java @@ -21,8 +21,6 @@ package org.elasticsearch.discovery.gce; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterService; -import org.elasticsearch.cluster.settings.ClusterDynamicSettings; -import org.elasticsearch.cluster.settings.DynamicSettings; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.discovery.DiscoverySettings; @@ -38,6 +36,8 @@ import org.elasticsearch.transport.TransportService; */ public class GceDiscovery extends ZenDiscovery { + public static final String GCE = "gce"; + @Inject public GceDiscovery(Settings settings, ClusterName clusterName, ThreadPool threadPool, TransportService transportService, ClusterService clusterService, NodeSettingsService nodeSettingsService, ZenPingService pingService, diff --git a/plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceUnicastHostsProvider.java b/plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceUnicastHostsProvider.java index 79cd11666ad..8feb9b8697c 100644 --- a/plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceUnicastHostsProvider.java +++ b/plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceUnicastHostsProvider.java @@ -83,10 +83,6 @@ public class GceUnicastHostsProvider extends AbstractComponent implements Unicas this.project = settings.get(Fields.PROJECT); this.zones = settings.getAsArray(Fields.ZONE); - // Check that we have all needed properties - checkProperty(Fields.PROJECT, project); - checkProperty(Fields.ZONE, zones); - this.tags = settings.getAsArray(Fields.TAGS); if (logger.isDebugEnabled()) { logger.debug("using tags {}", Arrays.asList(this.tags)); @@ -250,16 +246,4 @@ public class GceUnicastHostsProvider extends AbstractComponent implements Unicas return cachedDiscoNodes; } - - private void checkProperty(String name, String value) { - if (!Strings.hasText(value)) { - logger.warn("{} is not set.", name); - } - } - - private void checkProperty(String name, String[] values) { - if (values == null || values.length == 0) { - logger.warn("{} is not set.", name); - } - } } diff --git a/plugins/cloud-gce/src/main/java/org/elasticsearch/plugin/cloud/gce/CloudGcePlugin.java b/plugins/cloud-gce/src/main/java/org/elasticsearch/plugin/cloud/gce/CloudGcePlugin.java index 5384f2c2599..0ff8d4ef6b7 100644 --- a/plugins/cloud-gce/src/main/java/org/elasticsearch/plugin/cloud/gce/CloudGcePlugin.java +++ b/plugins/cloud-gce/src/main/java/org/elasticsearch/plugin/cloud/gce/CloudGcePlugin.java @@ -19,9 +19,13 @@ package org.elasticsearch.plugin.cloud.gce; +import org.elasticsearch.cloud.gce.GceComputeService; import org.elasticsearch.cloud.gce.GceModule; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.component.LifecycleComponent; import org.elasticsearch.common.inject.Module; +import org.elasticsearch.common.logging.ESLogger; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.discovery.DiscoveryModule; import org.elasticsearch.discovery.gce.GceDiscovery; @@ -38,6 +42,7 @@ import java.util.List; public class CloudGcePlugin extends Plugin { private final Settings settings; + protected final ESLogger logger = Loggers.getLogger(CloudGcePlugin.class); public CloudGcePlugin(Settings settings) { this.settings = settings; @@ -56,7 +61,7 @@ public class CloudGcePlugin extends Plugin { @Override public Collection nodeModules() { List modules = new ArrayList<>(); - if (settings.getAsBoolean("cloud.enabled", true)) { + if (isDiscoveryAlive(settings, logger)) { modules.add(new GceModule()); } return modules; @@ -65,15 +70,59 @@ public class CloudGcePlugin extends Plugin { @Override public Collection> nodeServices() { Collection> services = new ArrayList<>(); - if (settings.getAsBoolean("cloud.enabled", true)) { + if (isDiscoveryAlive(settings, logger)) { services.add(GceModule.getComputeServiceImpl()); } return services; } public void onModule(DiscoveryModule discoveryModule) { - discoveryModule.addDiscoveryType("gce", GceDiscovery.class); - discoveryModule.addUnicastHostProvider(GceUnicastHostsProvider.class); + if (isDiscoveryAlive(settings, logger)) { + discoveryModule.addDiscoveryType("gce", GceDiscovery.class); + discoveryModule.addUnicastHostProvider(GceUnicastHostsProvider.class); + } + } + + /** + * Check if discovery is meant to start + * + * @return true if we can start gce discovery features + */ + public static boolean isDiscoveryAlive(Settings settings, ESLogger logger) { + // User set discovery.type: gce + if (GceDiscovery.GCE.equalsIgnoreCase(settings.get("discovery.type")) == false) { + logger.debug("discovery.type not set to {}", GceDiscovery.GCE); + return false; + } + + if (checkProperty(GceComputeService.Fields.PROJECT, settings.get(GceComputeService.Fields.PROJECT), logger) == false || + checkProperty(GceComputeService.Fields.ZONE, settings.getAsArray(GceComputeService.Fields.ZONE), logger) == false) { + logger.debug("one or more gce discovery settings are missing. " + + "Check elasticsearch.yml file. Should have [{}] and [{}].", + GceComputeService.Fields.PROJECT, + GceComputeService.Fields.ZONE); + return false; + } + + logger.trace("all required properties for gce discovery are set!"); + + return true; + } + + private static boolean checkProperty(String name, String value, ESLogger logger) { + if (!Strings.hasText(value)) { + logger.warn("{} is not set.", name); + return false; + } + return true; + } + + private static boolean checkProperty(String name, String[] values, ESLogger logger) { + if (values == null || values.length == 0) { + logger.warn("{} is not set.", name); + return false; + } + return true; } } diff --git a/plugins/cloud-gce/src/test/java/org/elasticsearch/discovery/gce/GceDiscoverySettingsTests.java b/plugins/cloud-gce/src/test/java/org/elasticsearch/discovery/gce/GceDiscoverySettingsTests.java new file mode 100644 index 00000000000..90b331dd8dc --- /dev/null +++ b/plugins/cloud-gce/src/test/java/org/elasticsearch/discovery/gce/GceDiscoverySettingsTests.java @@ -0,0 +1,78 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.discovery.gce; + +import org.elasticsearch.cloud.gce.GceModule; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.plugin.cloud.gce.CloudGcePlugin; +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.is; + +public class GceDiscoverySettingsTests extends ESTestCase { + public void testDiscoveryReady() { + Settings settings = Settings.builder() + .put("discovery.type", "gce") + .put("cloud.gce.project_id", "gce_id") + .putArray("cloud.gce.zone", "gce_zones_1", "gce_zones_2") + .build(); + + boolean discoveryReady = CloudGcePlugin.isDiscoveryAlive(settings, logger); + assertThat(discoveryReady, is(true)); + } + + public void testDiscoveryNotReady() { + Settings settings = Settings.EMPTY; + boolean discoveryReady = CloudGcePlugin.isDiscoveryAlive(settings, logger); + assertThat(discoveryReady, is(false)); + + settings = Settings.builder() + .put("discovery.type", "gce") + .build(); + + discoveryReady = CloudGcePlugin.isDiscoveryAlive(settings, logger); + assertThat(discoveryReady, is(false)); + + settings = Settings.builder() + .put("discovery.type", "gce") + .put("cloud.gce.project_id", "gce_id") + .build(); + + discoveryReady = CloudGcePlugin.isDiscoveryAlive(settings, logger); + assertThat(discoveryReady, is(false)); + + + settings = Settings.builder() + .put("discovery.type", "gce") + .putArray("cloud.gce.zone", "gce_zones_1", "gce_zones_2") + .build(); + + discoveryReady = CloudGcePlugin.isDiscoveryAlive(settings, logger); + assertThat(discoveryReady, is(false)); + + settings = Settings.builder() + .put("cloud.gce.project_id", "gce_id") + .putArray("cloud.gce.zone", "gce_zones_1", "gce_zones_2") + .build(); + + discoveryReady = CloudGcePlugin.isDiscoveryAlive(settings, logger); + assertThat(discoveryReady, is(false)); + } +}