Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use file-based discovery not MockUncasedHostsProvider #33554

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public DiscoveryModule(Settings settings, ThreadPool threadPool, TransportServic
if (discoverySupplier == null) {
throw new IllegalArgumentException("Unknown discovery type [" + discoveryType + "]");
}
Loggers.getLogger(getClass(), settings).info("using discovery type [{}]", discoveryType);
Loggers.getLogger(getClass(), settings).info("using discovery type [{}] and host providers {}", discoveryType, hostsProviderNames);
discovery = Objects.requireNonNull(discoverySupplier.get());
}

Expand Down
6 changes: 6 additions & 0 deletions server/src/main/java/org/elasticsearch/node/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,8 @@ public Node start() throws NodeValidationException {
assert localNodeFactory.getNode() != null;
assert transportService.getLocalNode().equals(localNodeFactory.getNode())
: "transportService has a different local node than the factory provided";
onTransportServiceStarted();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can avoid adding these changes here in Node (and MockNode), but instead after initializing the node (but before starting it), call node.injector().getInstance(TransportService.class) to get the TransportService and then register a lifecycle listener on that (addLifecycleListener) which implements afterStart.


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

final MetaData onDiskMetadata;
try {
// we load the global state here (the persistent part of the cluster state stored on disk) to
Expand Down Expand Up @@ -781,6 +783,10 @@ public void onTimeout(TimeValue timeout) {
return this;
}

// For notifying tests that discovery can be configured
protected void onTransportServiceStarted() {
}

private Node stop() {
if (!lifecycle.moveToStopped()) {
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
import java.util.Collection;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicBoolean;

import static java.util.Collections.emptySet;
Expand Down Expand Up @@ -113,6 +112,7 @@ public void blockActions(String... actions) {
@Override
protected Settings nodeSettings(int nodeOrdinal) {
return Settings.builder()
.put(super.nodeSettings(nodeOrdinal))
// manual collection or upon cluster forming.
.put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), 2)
.put(InternalClusterInfoService.INTERNAL_CLUSTER_INFO_TIMEOUT_SETTING.getKey(), "1s")
Expand All @@ -121,8 +121,7 @@ protected Settings nodeSettings(int nodeOrdinal) {

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return Arrays.asList(TestPlugin.class,
MockTransportService.TestPlugin.class);
return Arrays.asList(TestPlugin.class, MockTransportService.TestPlugin.class);
}

public void testClusterInfoServiceCollectsInformation() throws Exception {
Expand Down Expand Up @@ -172,7 +171,7 @@ public void testClusterInfoServiceCollectsInformation() throws Exception {
}
}

public void testClusterInfoServiceInformationClearOnError() throws InterruptedException, ExecutionException {
public void testClusterInfoServiceInformationClearOnError() {
internalCluster().startNodes(2,
// manually control publishing
Settings.builder().put(InternalClusterInfoService.INTERNAL_CLUSTER_INFO_UPDATE_INTERVAL_SETTING.getKey(), "60m").build());
Expand All @@ -186,7 +185,6 @@ public void testClusterInfoServiceInformationClearOnError() throws InterruptedEx
assertThat("some usages are populated", info.getNodeLeastAvailableDiskUsages().size(), Matchers.equalTo(2));
assertThat("some shard sizes are populated", info.shardSizes.size(), greaterThan(0));


MockTransportService mockTransportService = (MockTransportService) internalCluster().getInstance(TransportService.class, internalTestCluster.getMasterName());

final AtomicBoolean timeout = new AtomicBoolean(false);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* 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.zen;

import org.elasticsearch.action.admin.cluster.node.info.NodesInfoRequest;
import org.elasticsearch.action.admin.cluster.node.info.NodesInfoResponse;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.Settings.Builder;
import org.elasticsearch.test.ESIntegTestCase;
import org.junit.Before;

import java.util.function.Consumer;

import static org.elasticsearch.discovery.DiscoveryModule.DISCOVERY_HOSTS_PROVIDER_SETTING;
import static org.elasticsearch.discovery.zen.SettingsBasedHostsProvider.DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING;
import static org.elasticsearch.discovery.zen.SettingsBasedHostsProvider.LIMIT_LOCAL_PORTS_COUNT;
import static org.elasticsearch.transport.TcpTransport.PORT;

@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0, numClientNodes = 0)
public class SettingsBasedHostProviderIT extends ESIntegTestCase {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we were effectively testing the settings-based host provider via all the other integ tests, but not after this change, I thought it prudent to add this.


private Consumer<Builder> configureDiscovery;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels a little hacky to me. Can't you explicitly pass the extra settings to the nodes when you're starting them up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came about due to a need to remove a setting from the builder, but that's no longer necessary.


@Before
public void setDefaultDiscoveryConfiguration() {
configureDiscovery = b -> {
};
}

@Override
protected Settings nodeSettings(int nodeOrdinal) {
Settings.Builder builder = Settings.builder().put(super.nodeSettings(nodeOrdinal));

// super.nodeSettings enables file-based discovery, but here we disable it again so we can test the static list:
if (randomBoolean()) {
builder.putList(DISCOVERY_HOSTS_PROVIDER_SETTING.getKey());
} else {
builder.remove(DISCOVERY_HOSTS_PROVIDER_SETTING.getKey());
}

builder.remove(DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING.getKey());

configureDiscovery.accept(builder);
return builder.build();
}

public void testClusterFormsWithSingleSeedHostInSettings() {
final String seedNodeName = internalCluster().startNode();
final NodesInfoResponse nodesInfoResponse
= client(seedNodeName).admin().cluster().nodesInfo(new NodesInfoRequest("_local")).actionGet();
final String seedNodeAddress = nodesInfoResponse.getNodes().get(0).getTransport().getAddress().publishAddress().toString();
configureDiscovery = b -> b.putList(DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING.getKey(), seedNodeAddress);
logger.info("--> using seed node address {}", seedNodeAddress);

int extraNodes = randomIntBetween(1, 5);
internalCluster().startNodes(extraNodes);

ensureStableCluster(extraNodes + 1);
}

public void testClusterFormsByScanningPorts() {
final String seedNodeName = internalCluster().startNode();
final NodesInfoResponse nodesInfoResponse
= client(seedNodeName).admin().cluster().nodesInfo(new NodesInfoRequest("_local")).actionGet();
final int seedNodePort = nodesInfoResponse.getNodes().get(0).getTransport().getAddress().publishAddress().getPort();
final int minPort = randomIntBetween(seedNodePort - LIMIT_LOCAL_PORTS_COUNT + 1, seedNodePort - 1);
final String portSpec = minPort + "-" + seedNodePort;

logger.info("--> using port specification [{}]", portSpec);

configureDiscovery = b -> b.put(PORT.getKey(), portSpec);

internalCluster().startNode();
ensureStableCluster(2);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
protected Settings nodeSettings(int nodeOrdinal) {
boolean lowLevelCancellation = randomBoolean();
logger.info("Using lowLevelCancellation: {}", lowLevelCancellation);
return Settings.builder().put(SearchService.LOW_LEVEL_CANCELLATION_SETTING.getKey(), lowLevelCancellation).build();
return Settings.builder()
.put(super.nodeSettings(nodeOrdinal))
.put(SearchService.LOW_LEVEL_CANCELLATION_SETTING.getKey(), lowLevelCancellation)
.build();
}

private void indexTestData() {
Expand Down
18 changes: 14 additions & 4 deletions test/framework/src/main/java/org/elasticsearch/node/MockNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
public class MockNode extends Node {

private final Collection<Class<? extends Plugin>> classpathPlugins;
private final Runnable onTransportServiceStarted;

public MockNode(final Settings settings, final Collection<Class<? extends Plugin>> classpathPlugins) {
this(settings, classpathPlugins, true);
Expand All @@ -76,26 +77,30 @@ public MockNode(
final Settings settings,
final Collection<Class<? extends Plugin>> classpathPlugins,
final boolean forbidPrivateIndexSettings) {
this(settings, classpathPlugins, null, forbidPrivateIndexSettings);
this(settings, classpathPlugins, null, forbidPrivateIndexSettings, () -> {});
}

public MockNode(
final Settings settings,
final Collection<Class<? extends Plugin>> classpathPlugins,
final Path configPath,
final boolean forbidPrivateIndexSettings) {
final boolean forbidPrivateIndexSettings,
final Runnable onTransportServiceStarted) {
this(
InternalSettingsPreparer.prepareEnvironment(settings, Collections.emptyMap(), configPath),
classpathPlugins,
forbidPrivateIndexSettings);
forbidPrivateIndexSettings,
onTransportServiceStarted);
}

private MockNode(
final Environment environment,
final Collection<Class<? extends Plugin>> classpathPlugins,
final boolean forbidPrivateIndexSettings) {
final boolean forbidPrivateIndexSettings,
final Runnable onTransportServiceStarted) {
super(environment, classpathPlugins, forbidPrivateIndexSettings);
this.classpathPlugins = classpathPlugins;
this.onTransportServiceStarted = onTransportServiceStarted;
}

/**
Expand Down Expand Up @@ -179,4 +184,9 @@ protected HttpServerTransport newHttpTransport(NetworkModule networkModule) {
protected void registerDerivedNodeNameWithLogger(String nodeName) {
// Nothing to do because test uses the thread name
}

@Override
protected void onTransportServiceStarted() {
onTransportServiceStarted.run();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS;
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_SHARDS;
import static org.elasticsearch.common.util.CollectionUtils.eagerPartition;
import static org.elasticsearch.discovery.DiscoveryModule.DISCOVERY_HOSTS_PROVIDER_SETTING;
import static org.elasticsearch.discovery.zen.SettingsBasedHostsProvider.DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING;
import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
import static org.elasticsearch.test.XContentTestUtils.convertToMap;
import static org.elasticsearch.test.XContentTestUtils.differenceBetweenMapsIgnoringArrayOrder;
Expand Down Expand Up @@ -1806,7 +1808,9 @@ protected Settings nodeSettings(int nodeOrdinal) {
// wait short time for other active shards before actually deleting, default 30s not needed in tests
.put(IndicesStore.INDICES_STORE_DELETE_SHARD_TIMEOUT.getKey(), new TimeValue(1, TimeUnit.SECONDS))
// randomly enable low-level search cancellation to make sure it does not alter results
.put(SearchService.LOW_LEVEL_CANCELLATION_SETTING.getKey(), randomBoolean());
.put(SearchService.LOW_LEVEL_CANCELLATION_SETTING.getKey(), randomBoolean())
.putList(DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING.getKey()) // empty list disables a port scan for other nodes
.putList(DISCOVERY_HOSTS_PROVIDER_SETTING.getKey(), "file");
if (rarely()) {
// Sometimes adjust the minimum search thread pool size, causing
// QueueResizingEsThreadPoolExecutor to be used instead of a regular
Expand Down Expand Up @@ -1919,7 +1923,7 @@ protected NodeConfigurationSource getNodeConfigSource() {
networkSettings.put(NetworkModule.TRANSPORT_TYPE_KEY, getTestTransportType());
}

NodeConfigurationSource nodeConfigurationSource = new NodeConfigurationSource() {
return new NodeConfigurationSource() {
@Override
public Settings nodeSettings(int nodeOrdinal) {
return Settings.builder()
Expand Down Expand Up @@ -1953,7 +1957,6 @@ public Collection<Class<? extends Plugin>> transportClientPlugins() {
return Collections.unmodifiableCollection(plugins);
}
};
return nodeConfigurationSource;
}

/**
Expand Down Expand Up @@ -2027,7 +2030,7 @@ protected Collection<Class<? extends Plugin>> getMockPlugins() {
public static final class TestSeedPlugin extends Plugin {
@Override
public List<Setting<?>> getSettings() {
return Arrays.asList(INDEX_TEST_SEED_SETTING);
return Collections.singletonList(INDEX_TEST_SEED_SETTING);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import java.util.Collection;
import java.util.Collections;

import static org.elasticsearch.discovery.zen.SettingsBasedHostsProvider.DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.lessThanOrEqualTo;
Expand Down Expand Up @@ -197,6 +198,7 @@ private Node newNode() {
// turning on the real memory circuit breaker leads to spurious test failures. As have no full control over heap usage, we
// turn it off for these tests.
.put(HierarchyCircuitBreakerService.USE_REAL_MEMORY_USAGE_SETTING.getKey(), false)
.putList(DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING.getKey()) // empty list disables a port scan for other nodes
.put(nodeSettings()) // allow test cases to provide their own settings or override these
.build();
Collection<Class<? extends Plugin>> plugins = getPlugins();
Expand Down
Loading