From 8d35583c433d5de240dd1d26e7973d83726530e2 Mon Sep 17 00:00:00 2001 From: Alpar Torok Date: Fri, 12 Jul 2019 10:53:33 +0300 Subject: [PATCH] Fix port range allocation with large worker IDs (#44213) * Fix port range allocation with large worker IDs Relates to #43983 The IDs gradle uses are incremented for the lifetime of the daemon which can result in port ranges that are outside the valid range. This change implements a modulo based formula to wrap the port ranges when the IDs get too large. Adresses #44134 but #44157 is also required to be able to close it. --- .../org/elasticsearch/test/ESTestCase.java | 10 ++--- .../test/InternalTestCluster.java | 2 +- .../test/transport/MockTransportService.java | 39 ++++++++++--------- .../test/test/ESTestCaseTests.java | 7 ++-- .../transport/MockTransportServiceTests.java | 36 +++++++++++++++++ 5 files changed, 65 insertions(+), 29 deletions(-) create mode 100644 test/framework/src/test/java/org/elasticsearch/test/transport/MockTransportServiceTests.java diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index b97d9f2b429..0f7310c4452 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -187,14 +187,14 @@ public abstract class ESTestCase extends LuceneTestCase { } // 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(); diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index 510c4be28e8..00c837feb1b 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -525,7 +525,7 @@ public final class InternalTestCluster extends TestCluster { 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(']'); diff --git a/test/framework/src/main/java/org/elasticsearch/test/transport/MockTransportService.java b/test/framework/src/main/java/org/elasticsearch/test/transport/MockTransportService.java index 18caa0edd96..053861f1ef8 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/transport/MockTransportService.java +++ b/test/framework/src/main/java/org/elasticsearch/test/transport/MockTransportService.java @@ -106,27 +106,28 @@ public final class MockTransportService extends TransportService { } /** - * 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 : (int) 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) { diff --git a/test/framework/src/test/java/org/elasticsearch/test/test/ESTestCaseTests.java b/test/framework/src/test/java/org/elasticsearch/test/test/ESTestCaseTests.java index af8df361d4c..2450b5658be 100644 --- a/test/framework/src/test/java/org/elasticsearch/test/test/ESTestCaseTests.java +++ b/test/framework/src/test/java/org/elasticsearch/test/test/ESTestCaseTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.test.test; -import com.carrotsearch.randomizedtesting.RandomizedTest; import junit.framework.AssertionFailedError; import org.elasticsearch.common.bytes.BytesReference; @@ -43,6 +42,7 @@ import java.util.function.Supplier; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.not; public class ESTestCaseTests extends ESTestCase { @@ -185,8 +185,7 @@ public class ESTestCaseTests extends ESTestCase { 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))); } } diff --git a/test/framework/src/test/java/org/elasticsearch/test/transport/MockTransportServiceTests.java b/test/framework/src/test/java/org/elasticsearch/test/transport/MockTransportServiceTests.java new file mode 100644 index 00000000000..5bb074e4b59 --- /dev/null +++ b/test/framework/src/test/java/org/elasticsearch/test/transport/MockTransportServiceTests.java @@ -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()); + } + +}