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.

Author: alpar-t

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

@@ -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();
 

test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

@@ -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(']');

test/framework/src/main/java/org/elasticsearch/test/transport/MockTransportService.java

@@ -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) {

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 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 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)));
     }
 }

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());
+    }
+
+}