Skip to content

Fix port range allocation with large worker IDs #44213

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

Merged
merged 6 commits into from
Jul 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,14 @@ public static void resetPortCounter() {
}

// Allows distinguishing between parallel test processes
public static final int TEST_WORKER_VM;
public static final String TEST_WORKER_VM_ID;

protected static final String TEST_WORKER_SYS_PROPERTY = "org.gradle.test.worker";
public static final String TEST_WORKER_SYS_PROPERTY = "org.gradle.test.worker";

public static final String DEFAULT_TEST_WORKER_ID = "--not-gradle--";

static {
// org.gradle.test.worker starts counting at 1, but we want to start counting at 0 here
// in case system property is not defined (e.g. when running test from IDE), just use 0
TEST_WORKER_VM = RandomizedTest.systemPropertyAsInt(TEST_WORKER_SYS_PROPERTY, 1) - 1;
TEST_WORKER_VM_ID = System.getProperty(TEST_WORKER_SYS_PROPERTY, DEFAULT_TEST_WORKER_ID);
setTestSysProps();
LogConfigurator.loadLog4jPlugins();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ private static Settings getRandomNodeSettings(long seed) {

public static String clusterName(String prefix, long clusterSeed) {
StringBuilder builder = new StringBuilder(prefix);
builder.append("-TEST_WORKER_VM=[").append(ESTestCase.TEST_WORKER_VM).append(']');
builder.append("-TEST_WORKER_VM=[").append(ESTestCase.TEST_WORKER_VM_ID).append(']');
builder.append("-CLUSTER_SEED=[").append(clusterSeed).append(']');
// if multiple maven task run on a single host we better have an identifier that doesn't rely on input params
builder.append("-HASH=[").append(SeedUtils.formatSeed(System.nanoTime())).append(']');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,27 +106,28 @@ public static MockTransportService createNewService(Settings settings, Version v
}

/**
* Some tests use MockTransportService to do network based testing. Yet, we run tests in multiple JVMs that means
* concurrent tests could claim port that another JVM just released and if that test tries to simulate a disconnect it might
* be smart enough to re-connect depending on what is tested. To reduce the risk, since this is very hard to debug we use
* a different default port range per JVM unless the incoming settings override it
* use a non-default base port otherwise some cluster in this JVM might reuse a port
*/
private static int getBasePort() {
final int basePort = 10300 + (ESTestCase.TEST_WORKER_VM * 100);
if (basePort < 10300 || basePort >= 65000) {
// to ensure we don't get illegal ports above 65536 in the getPortRange method
throw new AssertionError("Expected basePort to be between 10300 and 65000 but was " + basePort);
}
return basePort;
}

/**
* Returns a unique port range for this JVM starting from the computed base port (see {@link #getBasePort()})
* Returns a unique port range for this JVM starting from the computed base port
*/
public static String getPortRange() {
int basePort = getBasePort();
return basePort + "-" + (basePort + 99); // upper bound is inclusive
return getBasePort() + "-" + (getBasePort() + 99); // upper bound is inclusive
}

protected static int getBasePort() {
// some tests use MockTransportService to do network based testing. Yet, we run tests in multiple JVMs that means
// concurrent tests could claim port that another JVM just released and if that test tries to simulate a disconnect it might
// be smart enough to re-connect depending on what is tested. To reduce the risk, since this is very hard to debug we use
// a different default port range per JVM unless the incoming settings override it
// use a non-default base port otherwise some cluster in this JVM might reuse a port

// We rely on Gradle implementation details here, the worker IDs are long values incremented by one for the
// lifespan of the daemon this means that they can get larger than the allowed port range.
// Ephemeral ports on Linux start at 32768 so we modulo to make sure that we don't exceed that.
// This is safe as long as we have fewer than 224 Gradle workers running in parallel
// See also: https://github.com/elastic/elasticsearch/issues/44134
final String workerId = System.getProperty(ESTestCase.TEST_WORKER_SYS_PROPERTY);
final int startAt = workerId == null ? 0 : Math.floorMod(Long.valueOf(workerId), 223);
assert startAt >= 0 : "Unexpected test worker Id, resulting port range would be negative";
return 10300 + (startAt * 100);
}

public static MockNioTransport newMockTransport(Settings settings, Version version, ThreadPool threadPool) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package org.elasticsearch.test.test;

import com.carrotsearch.randomizedtesting.RandomizedTest;
import junit.framework.AssertionFailedError;

import org.elasticsearch.common.bytes.BytesReference;
Expand All @@ -43,6 +42,7 @@

import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.not;

public class ESTestCaseTests extends ESTestCase {

Expand Down Expand Up @@ -185,8 +185,7 @@ public void testRandomValueOtherThan() {

public void testWorkerSystemProperty() {
assumeTrue("requires running tests with Gradle", System.getProperty("tests.gradle") != null);
// org.gradle.test.worker starts counting at 1
assertThat(RandomizedTest.systemPropertyAsInt(TEST_WORKER_SYS_PROPERTY, -1), greaterThan(0));
assertEquals(RandomizedTest.systemPropertyAsInt(TEST_WORKER_SYS_PROPERTY, -1) - 1, TEST_WORKER_VM);

assertThat(ESTestCase.TEST_WORKER_VM_ID, not(equals(ESTestCase.DEFAULT_TEST_WORKER_ID)));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* 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.test.transport;

import org.elasticsearch.test.ESTestCase;

public class MockTransportServiceTests extends ESTestCase {

public void testBasePortGradle() {
assumeTrue("requires running tests with Gradle", System.getProperty("tests.gradle") != null);
// Gradle worker IDs are 1 based
assertNotEquals(10300, MockTransportService.getBasePort());
}

public void testBasePortIDE() {
assumeTrue("requires running tests without Gradle", System.getProperty("tests.gradle") == null);
assertEquals(10300, MockTransportService.getBasePort());
}

}