GEODE-9176: Automatically pass properties to benchmark JVMs (#149)

Converting all of the system properties we use in the benchmarks to start with the benchmark
prefix. Changing the gradle build to copy all benchmark.* properties as system
properties in the tst.

Getting rid of problematic org.json dependency, and also making
sure we capture *all* system properties that might effect the behavior of the
test.

Adding a way to automatically set system properties in test JVMs. Just add a property
with the prefix benchmark.system.ROLE, where ROLE is the role of jvms to target. Eg

benchmark.system.server.gemfire.disablePartitionedRegionBucketAck=true would set
gemfire.disablePartitionedRegionBucketAck=true  in the server JVMs.

Logging all benchmark properties during test run

geode-benchmarks/build.gradle

@@ -73,6 +73,10 @@ test{
     logger.quiet "Executing test ${desc.className}.${desc.name} with result: ${result.resultType}"
   }
   useJUnitPlatform()
+  systemProperty 'org.slf4j.simpleLogger.showDateTime', 'true'
+  systemProperty 'org.slf4j.simpleLogger.dateTimeFormat', 'yyyy-MM-dd HH:mm:ss.SSS'
+  systemProperty 'org.slf4j.simpleLogger.showThreadNam', 'false'
+  systemProperty 'org.slf4j.simpleLogger.showShortLogName', 'true'
 }
 
 task benchmark(type: Test) {
@@ -83,49 +87,69 @@ task benchmark(type: Test) {
   testClassesDirs = project.sourceSets.main.output.classesDirs
   classpath = project.sourceSets.main.runtimeClasspath
   useJUnitPlatform()
-  testLogging { exceptionFormat = 'full' }
+  testLogging {
+    exceptionFormat = 'full'
+    showStandardStreams = true
+  }
 
   exclude "**/NoopBenchmark.class"
   exclude "**/P2pPartitionedPutLongBenchmark.class"
 
   forkEvery 1
   failFast = true
 
+  systemProperty 'org.slf4j.simpleLogger.showDateTime', 'true'
+  systemProperty 'org.slf4j.simpleLogger.dateTimeFormat', 'yyyy-MM-dd HH:mm:ss.SSS'
+  systemProperty 'org.slf4j.simpleLogger.showThreadName', 'false'
+  systemProperty 'org.slf4j.simpleLogger.showShortLogName', 'true'
   systemProperty 'TEST_HOSTS', project.findProperty('hosts')
   systemProperty 'TEST_METADATA', project.findProperty('metadata')
   systemProperty 'OUTPUT_DIR', outputDir
+
+  //Set all project properties starting with "benchmark." as system properties in the test
+  //JVM
+  project.properties.findAll {
+    it.key.startsWith("benchmark.")
+  }.each {
+    systemProperty(it.getKey(), it.getValue())
+  }
+
+  //------------------------------------------------------------
+  //Legacy properties - these properties were added before the benchmark.
+  //prefix convention. They will be passed on to the JVM for now to not break
+  //CI or peoples scripts. Remove these soon!
+  //------------------------------------------------------------
   if (project.hasProperty('withGc')) {
-    systemProperty 'withGc', project.findProperty('withGc')
+    systemProperty 'benchmark.withGc', project.findProperty('withGc')
   }
   if (project.hasProperty('withHeap')) {
-    systemProperty 'withHeap', project.findProperty('withHeap')
+    systemProperty 'benchmark.withHeap', project.findProperty('withHeap')
   }
   if (project.hasProperty('withThreads')) {
-    systemProperty 'withThreads', project.findProperty('withThreads')
+    systemProperty 'benchmark.withThreads', project.findProperty('withThreads')
   }
   if (project.hasProperty('withWarmup')) {
-    systemProperty 'withWarmup', project.findProperty('withWarmup')
+    systemProperty 'benchmark.withWarmup', project.findProperty('withWarmup')
   }
   if (project.hasProperty('withDuration')) {
-    systemProperty 'withDuration', project.findProperty('withDuration')
+    systemProperty 'benchmark.withDuration', project.findProperty('withDuration')
   }
 
-  systemProperty 'withSsl', project.hasProperty('withSsl')
-  systemProperty 'withSslProtocols', project.findProperty('withSslProtocols')
-  systemProperty 'withSslCiphers', project.findProperty('withSslCiphers')
+  systemProperty 'benchmark.withSsl', project.hasProperty('withSsl')
+  systemProperty 'benchmark.withSslProtocols', project.findProperty('withSslProtocols')
+  systemProperty 'benchmark.withSslCiphers', project.findProperty('withSslCiphers')
 
   if (project.hasProperty('withSniProxy')) {
-    systemProperty 'withSniProxy', project.findProperty('withSniProxy')
+    systemProperty 'benchmark.withSniProxy', project.findProperty('withSniProxy')
   }
-  systemProperty 'withSniProxyImage', project.findProperty('withSniProxyImage')
+  systemProperty 'benchmark.withSniProxyImage', project.findProperty('withSniProxyImage')
 
   if (project.hasProperty('withRouter')) {
-    systemProperty 'withRouter', project.findProperty('withRouter')
+    systemProperty 'benchmark.withRouter', project.findProperty('withRouter')
   }
-  systemProperty 'withRouterImage', project.findProperty('withRouterImage')
+  systemProperty 'benchmark.withRouterImage', project.findProperty('withRouterImage')
 
-  systemProperty 'withSecurityManager', project.hasProperty('withSecurityManager')
-  systemProperty 'benchmark.profiler.argument', project.findProperty('benchmark.profiler.argument')
+  systemProperty 'benchmark.withSecurityManager', project.hasProperty('withSecurityManager')
 
 
   doFirst {

geode-benchmarks/src/main/java/org/apache/geode/benchmark/parameters/GcParameters.java

@@ -28,7 +28,7 @@ public class GcParameters {
 
   public static void configure(final TestConfig testConfig) {
     final GcImplementation gcImplementation =
-        GcImplementation.valueOf(System.getProperty("withGc", "CMS"));
+        GcImplementation.valueOf(System.getProperty("benchmark.withGc", "CMS"));
     logger.info("Configuring {} GC.", gcImplementation);
     switch (gcImplementation) {
       case CMS:

geode-benchmarks/src/main/java/org/apache/geode/benchmark/parameters/HeapParameters.java

@@ -26,7 +26,7 @@ public class HeapParameters {
   private static final Logger logger = LoggerFactory.getLogger(HeapParameters.class);
 
   public static void configure(final TestConfig testConfig) {
-    final String heap = System.getProperty("withHeap", "8g");
+    final String heap = System.getProperty("benchmark.withHeap", "8g");
     logger.info("Configuring heap parameters {}.", heap);
     configureGeodeProductJvms(testConfig, "-Xmx" + heap, "-Xms" + heap);
   }

geode-benchmarks/src/main/java/org/apache/geode/benchmark/topology/ClientServerTopologyWithRouterAndSniProxy.java

@@ -35,8 +35,8 @@
 import org.apache.geode.perftest.TestConfig;
 
 public class ClientServerTopologyWithRouterAndSniProxy extends ClientServerTopologyWithSniProxy {
-  public static final String WITH_ROUTER_PROPERTY = "withRouter";
-  public static final String WITH_ROUTER_IMAGE_PROPERTY = "withRouterImage";
+  public static final String WITH_ROUTER_PROPERTY = "benchmark.withRouter";
+  public static final String WITH_ROUTER_IMAGE_PROPERTY = "benchmark.withRouterImage";
 
   private static final int NUM_LOCATORS = 1;
   private static final int NUM_SERVERS = 2;

geode-benchmarks/src/main/java/org/apache/geode/benchmark/topology/ClientServerTopologyWithSniProxy.java

@@ -40,8 +40,8 @@
 import org.apache.geode.perftest.TestConfig;
 
 public class ClientServerTopologyWithSniProxy extends Topology {
-  public static final String WITH_SNI_PROXY_PROPERTY = "withSniProxy";
-  public static final String WITH_SNI_PROXY_IMAGE_PROPERTY = "withSniProxyImage";
+  public static final String WITH_SNI_PROXY_PROPERTY = "benchmark.withSniProxy";
+  public static final String WITH_SNI_PROXY_IMAGE_PROPERTY = "benchmark.withSniProxyImage";
 
   private static final int NUM_LOCATORS = 1;
   private static final int NUM_SERVERS = 2;

geode-benchmarks/src/main/java/org/apache/geode/benchmark/topology/Topology.java

@@ -25,14 +25,14 @@
 import org.apache.geode.perftest.TestConfig;
 
 public abstract class Topology {
-  public static final String WITH_SSL_PROPERTY = "withSsl";
-  static final String WITH_SSL_ARGUMENT = "-DwithSsl=true";
+  public static final String WITH_SSL_PROPERTY = "benchmark.withSsl";
+  static final String WITH_SSL_ARGUMENT = "-Dbenchmark.withSsl=true";
 
-  public static final String WITH_SSL_PROTOCOLS_PROPERTY = "withSslProtocols";
-  public static final String WITH_SSL_CIPHERS_PROPERTY = "withSslCiphers";
+  public static final String WITH_SSL_PROTOCOLS_PROPERTY = "benchmark.withSslProtocols";
+  public static final String WITH_SSL_CIPHERS_PROPERTY = "benchmark.withSslCiphers";
 
-  public static final String WITH_SECURITY_MANAGER_PROPERTY = "withSecurityManager";
-  static final String WITH_SECURITY_MANAGER_ARGUMENT = "-DwithSecurityManager=true";
+  public static final String WITH_SECURITY_MANAGER_PROPERTY = "benchmark.withSecurityManager";
+  static final String WITH_SECURITY_MANAGER_ARGUMENT = "-Dbenchmark.withSecurityManager=true";
 
   static void configureCommon(TestConfig config) {
     JvmParameters.configure(config);

geode-benchmarks/src/test/java/org/apache/geode/benchmark/parameters/GcParametersTest.java

@@ -30,7 +30,7 @@
 import org.apache.geode.perftest.TestConfig;
 
 class GcParametersTest {
-  private static final String WITH_GC = "withGc";
+  private static final String WITH_GC = "benchmark.withGc";
   private static final String JAVA_RUNTIME_VERSION = "java.runtime.version";
   private static final String XX_USE_ZGC = "-XX:+UseZGC";
   private static final String XX_USE_G_1_GC = "-XX:+UseG1GC";

geode-benchmarks/src/test/java/org/apache/geode/benchmark/parameters/HeapParametersTest.java

@@ -30,7 +30,7 @@
 
 class HeapParametersTest {
 
-  private static final String WITH_HEAP = "withHeap";
+  private static final String WITH_HEAP = "benchmark.withHeap";
 
   private Properties systemProperties;
 

geode-benchmarks/src/test/java/org/apache/geode/benchmark/tests/ClientServerBenchmarkTest.java

@@ -56,40 +56,40 @@ public void beforeEach() {
 
   @AfterAll
   public static void afterAll() {
-    System.clearProperty("withSniProxy");
+    System.clearProperty("benchmark.withSniProxy");
   }
 
   @Test
   public void withoutSniProxy() {
-    System.clearProperty("withSniProxy");
+    System.clearProperty("benchmark.withSniProxy");
     config = ClientServerBenchmark.createConfig();
     assertThat(config.getBefore()).doesNotContain(startHAProxyStep, startEnvoyStep);
   }
 
   @Test
   public void withSniProxyInvalid() {
-    System.setProperty("withSniProxy", "invalid");
+    System.setProperty("benchmark.withSniProxy", "invalid");
     assertThatThrownBy(() -> ClientServerBenchmark.createConfig())
         .isInstanceOf(IllegalArgumentException.class);
   }
 
   @Test
   public void withSniProxyDefault() {
-    System.setProperty("withSniProxy", "");
+    System.setProperty("benchmark.withSniProxy", "");
     config = ClientServerBenchmark.createConfig();
     assertThat(config.getBefore()).contains(startHAProxyStep).doesNotContain(startEnvoyStep);
   }
 
   @Test
   public void withSniProxyHAProxy() {
-    System.setProperty("withSniProxy", "HAProxy");
+    System.setProperty("benchmark.withSniProxy", "HAProxy");
     config = ClientServerBenchmark.createConfig();
     assertThat(config.getBefore()).contains(startHAProxyStep);
   }
 
   @Test
   public void withSniProxyEnvoy() {
-    System.setProperty("withSniProxy", "Envoy");
+    System.setProperty("benchmark.withSniProxy", "Envoy");
     config = ClientServerBenchmark.createConfig();
     assertThat(config.getBefore()).contains(startEnvoyStep);
   }

geode-benchmarks/src/test/java/org/apache/geode/benchmark/topology/ClientServerTopologyTest.java

@@ -43,45 +43,48 @@ public void afterEach() {
 
   @Test
   public void configWithSsl() {
-    System.setProperty("withSsl", "true");
+    System.setProperty("benchmark.withSsl", "true");
     TestConfig testConfig = new TestConfig();
     ClientServerTopology.configure(testConfig);
-    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).contains("-DwithSsl=true");
+    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).contains("-Dbenchmark.withSsl=true");
   }
 
   @Test
   public void configWithNoSsl() {
     TestConfig testConfig = new TestConfig();
     ClientServerTopology.configure(testConfig);
-    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).doesNotContain("-DwithSsl=true");
+    assertThat(testConfig.getJvmArgs().get(CLIENT.name()))
+        .doesNotContain("-Dbenchmark.withSsl=true");
   }
 
   @Test
   public void configWithoutSecurityManager() {
     TestConfig testConfig = new TestConfig();
     ClientServerTopology.configure(testConfig);
     assertThat(testConfig.getJvmArgs().get(CLIENT.name()))
-        .doesNotContain("-DwithSecurityManager=true");
+        .doesNotContain("-Dbenchmark.withSecurityManager=true");
   }
 
   @Test
   public void configWithSecurityManager() {
-    System.setProperty("withSecurityManager", "true");
+    System.setProperty("benchmark.withSecurityManager", "true");
     TestConfig testConfig = new TestConfig();
     ClientServerTopology.configure(testConfig);
-    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).contains("-DwithSecurityManager=true");
+    assertThat(testConfig.getJvmArgs().get(CLIENT.name()))
+        .contains("-Dbenchmark.withSecurityManager=true");
   }
 
   @Test
   public void configWithSecurityManagerAndSslAndJava11() {
-    System.setProperty("withSecurityManager", "true");
+    System.setProperty("benchmark.withSecurityManager", "true");
     System.setProperty("java.runtime.version", "11.0.4+11");
-    System.setProperty("withSsl", "true");
+    System.setProperty("benchmark.withSsl", "true");
     TestConfig testConfig = new TestConfig();
 
     ClientServerTopology.configure(testConfig);
 
-    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).contains("-DwithSecurityManager=true");
-    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).contains("-DwithSsl=true");
+    assertThat(testConfig.getJvmArgs().get(CLIENT.name()))
+        .contains("-Dbenchmark.withSecurityManager=true");
+    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).contains("-Dbenchmark.withSsl=true");
   }
 }

geode-benchmarks/src/test/java/org/apache/geode/benchmark/topology/ClientServerTopologyWithSniProxyTest.java

@@ -51,7 +51,7 @@ public void configWithNoSsl(final SniProxyImplementation sniProxyImplementation)
     System.setProperty(WITH_SNI_PROXY_PROPERTY, sniProxyImplementation.name());
     final TestConfig testConfig = new TestConfig();
     ClientServerTopologyWithSniProxy.configure(testConfig);
-    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).contains("-DwithSsl=true");
+    assertThat(testConfig.getJvmArgs().get(CLIENT.name())).contains("-Dbenchmark.withSsl=true");
   }
 
 }

harness/build.gradle

@@ -55,7 +55,6 @@ dependencies {
   compile(group: 'commons-io', name: 'commons-io', version: project.'commons-io.version')
   compile(group: 'org.yardstickframework', name: 'yardstick', version: project.'yardstick.version')
   compile(group: 'org.hdrhistogram', name: 'HdrHistogram', version: project.'HdrHistogram.version')
-  compile(group: 'org.json', name: 'json', version: project.'JSON.version')
   compile(group: 'org.apache.geode', name: 'geode-core', version: geodeVersion)
   testCompile(group: 'org.mockito', name: 'mockito-all', version: project.'mockito-all.version')
   testCompile(group: 'org.awaitility', name: 'awaitility', version: project.'awaitility.version')

harness/src/main/java/org/apache/geode/perftest/BenchmarkProperties.java

@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.geode.perftest;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class BenchmarkProperties {
+
+  /**
+   * Read the current system properties and find any system properties that start
+   * with "benchmark.system.ROLE" Returns a map of ROLE-> system properties for that
+   * role.
+   */
+  public static Map<String, List<String>> getDefaultJVMArgs() {
+    Map<String, List<String>> results = new HashMap<>();
+    System.getProperties().stringPropertyNames().stream()
+        .filter(name -> name.startsWith("benchmark.system."))
+        .forEach(name -> {
+          String shortName = name.replace("benchmark.system.", "");
+          String[] roleAndProperty = shortName.split("\\.", 2);
+          String role = roleAndProperty[0];
+          String property = roleAndProperty[1];
+          String value = System.getProperty(name);
+          List<String> roleProperties = results.computeIfAbsent(role, key -> new ArrayList<>());
+          roleProperties.add("-D" + property + "=" + value);
+        });
+    return results;
+  }
+}

harness/src/main/java/org/apache/geode/perftest/TestConfig.java

@@ -21,7 +21,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -39,7 +38,7 @@ public class TestConfig implements Serializable {
 
   private final WorkloadConfig workloadConfig = new WorkloadConfig();
   private Map<String, Integer> roles = new LinkedHashMap<>();
-  private Map<String, List<String>> jvmArgs = new HashMap<>();
+  private Map<String, List<String>> jvmArgs = BenchmarkProperties.getDefaultJVMArgs();
   private List<TestStep> before = new ArrayList<>();
   private List<TestStep> workload = new ArrayList<>();
   private List<TestStep> after = new ArrayList<>();

harness/src/main/java/org/apache/geode/perftest/TestRunners.java

@@ -38,8 +38,8 @@
  */
 public class TestRunners {
 
-  public static final String TEST_HOSTS = "TEST_HOSTS";
-  public static final String OUTPUT_DIR = "OUTPUT_DIR";
+  public static final String TEST_HOSTS = "benchmark.TEST_HOSTS";
+  public static final String OUTPUT_DIR = "benchmark.OUTPUT_DIR";
 
   public static final String[] JVM_ARGS_SMALL_SIZE = new String[] {
       "-XX:CMSInitiatingOccupancyFraction=60",
@@ -91,7 +91,7 @@ public static TestRunner defaultRunner() {
   static TestRunner defaultRunner(String testHosts, File outputDir) {
     if (testHosts == null) {
       throw new IllegalStateException(
-          "You must set the TEST_HOSTS system property to a comma separated list of hosts to run the benchmarks on.");
+          "You must set the benchmark.TEST_HOSTS system property to a comma separated list of hosts to run the benchmarks on.");
     }
 
     String userName = System.getProperty("user.name");