[kudu] 02/03: dist-test: support test result reporting

[ad...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit 3707cf33ccbcb015130de356e4e82d1f54636c6e
Author: Adar Dembo <ad...@cloudera.com>
AuthorDate: Thu Apr 18 15:40:25 2019 -0700

    dist-test: support test result reporting
    
    The goal is to be able to use dist-test in any run of build-and-test.sh,
    even those that power the flaky test dashboard.
    
    Because dist-test runs just one test per invocation, it isn't safe to disown
    the reporting process; the slave might disappear mid-report. There wasn't an
    obvious way to know that we're using dist-test from within run-test.sh, so I
    opted to stop disowning altogether. The Java result reporting is synchronous
    (and per-test rather than per-class) so this didn't seem like a big deal.
    
    I was concerned that the additional parallelism might lead to "thundering
    herd" behavior in the test result server (i.e. if all tests finished and
    reported at the same time) so I ran a couple reporting-enabled dist-tests
    looking for load spikes on the server. I didn't see any, likely because:
    - Most tests' running time varies.
    - Tests only start when dist-test slaves become available, not all at once.
    
    RUN_FLAKY_ONLY will be addressed in a follow-up.
    
    Change-Id: Ie5072e2395376e455f63dac6d3debc88526aed07
    Reviewed-on: http://gerrit.cloudera.org:8080/13068
    Tested-by: Adar Dembo <ad...@cloudera.com>
    Reviewed-by: Grant Henke <gr...@apache.org>
---
 build-support/dist_test.py                         | 24 +++++++++++++++
 build-support/run-test.sh                          | 12 +-------
 .../org/apache/kudu/gradle/DistTestTask.java       | 36 ++++++++++++++++++++--
 3 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/build-support/dist_test.py b/build-support/dist_test.py
index 402b6c6..b26ebc9 100755
--- a/build-support/dist_test.py
+++ b/build-support/dist_test.py
@@ -83,6 +83,7 @@ TEST_SHARD_RE = re.compile("\.\d+$")
 
 DEPS_FOR_ALL = \
     ["build-support/stacktrace_addr2line.pl",
+     "build-support/report-test.sh",
      "build-support/run-test.sh",
      "build-support/run_dist_test.py",
      "build-support/java-home-candidates.txt",
@@ -273,6 +274,20 @@ def copy_system_library(lib):
     shutil.copy2(rel_to_abs(lib), dst)
   return dst
 
+def forward_env_var(command_list, var_name, is_required=True):
+  """
+  Extends 'command_list' with the name and value of the environment variable
+  given by 'var_name'.
+
+  Does nothing if the environment variable isn't set or is empty, unless
+  'is_required' is True, in which case an exception is raised.
+  """
+  if not var_name in os.environ or not os.environ.get(var_name):
+    if is_required:
+      raise Exception("required env variable %s is missing" % (var_name,))
+    return
+  command_list.extend(["-e", "%s=%s" % (var_name, os.environ.get(var_name))])
+
 def create_archive_input(staging, execution, dep_extractor,
                          collect_tmpdir=False):
   """
@@ -324,6 +339,15 @@ def create_archive_input(staging, execution, dep_extractor,
       continue
     command.extend(['-e', '%s=%s' % (k, v)])
 
+  # If test result reporting was requested, forward all relevant environment
+  # variables into the test process so as to enable reporting.
+  if os.environ.get('KUDU_REPORT_TEST_RESULTS', 0):
+    forward_env_var(command, 'KUDU_REPORT_TEST_RESULTS')
+    forward_env_var(command, 'BUILD_CONFIG')
+    forward_env_var(command, 'BUILD_TAG')
+    forward_env_var(command, 'GIT_REVISION')
+    forward_env_var(command, 'TEST_RESULT_SERVER', is_required=False)
+
   if collect_tmpdir:
     command += ["--collect-tmpdir"]
   command.append('--')
diff --git a/build-support/run-test.sh b/build-support/run-test.sh
index c5b9ab8..21d09d0 100755
--- a/build-support/run-test.sh
+++ b/build-support/run-test.sh
@@ -215,17 +215,7 @@ for ATTEMPT_NUMBER in $(seq 1 $TEST_EXECUTION_ATTEMPTS) ; do
 
   if [ -n "$KUDU_REPORT_TEST_RESULTS" ] && [ "$KUDU_REPORT_TEST_RESULTS" -ne 0 ]; then
     echo Reporting results
-    $SOURCE_ROOT/build-support/report-test.sh "$ABS_TEST_PATH" "$LOGFILE" "$STATUS" &
-
-    # On success, we'll do "best effort" reporting, and disown the subprocess.
-    # On failure, we want to upload the failed test log. So, in that case,
-    # wait for the report-test.sh job to finish, lest we accidentally run
-    # a test retry and upload the wrong log.
-    if [ "$STATUS" -eq "0" ]; then
-      disown
-    else
-      wait
-    fi
+    $SOURCE_ROOT/build-support/report-test.sh "$ABS_TEST_PATH" "$LOGFILE" "$STATUS"
   fi
 
   if [ "$STATUS" -eq "0" ]; then
diff --git a/java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java b/java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
index 4d3f5db..9340b25 100644
--- a/java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
+++ b/java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
@@ -149,7 +149,7 @@ public class DistTestTask extends DefaultTask {
    *
    * Note: This currently fails OSX because dump_base_deps use ldd.
    */
-  List<String> getBaseDeps() throws IOException {
+  private List<String> getBaseDeps() throws IOException {
     Process proc = new ProcessBuilder(distTestBin,
         "internal",
         "dump_base_deps")
@@ -162,6 +162,35 @@ public class DistTestTask extends DefaultTask {
     }
   }
 
+  /**
+   * @return all test result reporting environment variables and their values,
+   *         in a format suitable for consumption by run_dist_test.py.
+   */
+  private List<String> getTestResultReportingEnvironmentVariables() {
+    ImmutableList.Builder<String> args = new ImmutableList.Builder<>();
+    String enabled = System.getenv("KUDU_REPORT_TEST_RESULTS");
+    if (enabled != null && Integer.parseInt(enabled) > 0) {
+      for (String ev : ImmutableList.of("KUDU_REPORT_TEST_RESULTS",
+                                        "BUILD_CONFIG",
+                                        "BUILD_TAG",
+                                        "GIT_REVISION",
+                                        "TEST_RESULT_SERVER")) {
+        String evValue = System.getenv(ev);
+        if (evValue == null || evValue.isEmpty()) {
+          if (ev.equals("TEST_RESULT_SERVER")) {
+            // This one is optional.
+            continue;
+          }
+          throw new RuntimeException(
+              String.format("Required env variable %s is missing", ev));
+        }
+        args.add("-e");
+        args.add(String.format("%s=%s", ev, evValue));
+      }
+    }
+    return args.build();
+  }
+
   private String genIsolate(Path isolateFileDir, Test test, String testClass,
                             List<String> baseDeps) throws IOException {
     Path rootDir = test.getProject().getRootDir().toPath();
@@ -200,8 +229,9 @@ public class DistTestTask extends DefaultTask {
     if (collectTmpDir) {
       cmd.add("--collect-tmpdir");
     }
-    cmd.add("--test-language=java",
-            "--",
+    cmd.add("--test-language=java");
+    cmd.addAll(getTestResultReportingEnvironmentVariables());
+    cmd.add("--",
             "-ea",
             "-cp",
             Joiner.on(":").join(classpath));