Use command's error reason and message for the request level ones.

PiperOrigin-RevId: 701875368
google · Dec 2, 2024 · 2647960 · 2647960
1 parent 80ff6a3
commit 2647960
Show file tree

Hide file tree

Showing 10 changed files with 223 additions and 37 deletions.
diff --git a/src/devtools/mobileharness/infra/ats/server/proto/service.proto b/src/devtools/mobileharness/infra/ats/server/proto/service.proto
@@ -188,6 +188,7 @@ enum ErrorReason {
   INVALID_RESOURCE = 4;
   TOO_MANY_LOST_DEVICES = 5;
   RESULT_PROCESSING_ERROR = 6;
+  TRADEFED_INVOCATION_ERROR = 7;
 }
 
 // TODO: No longer useful. Remove this proto.

diff --git a/src/java/com/google/devtools/mobileharness/infra/ats/common/BUILD b/src/java/com/google/devtools/mobileharness/infra/ats/common/BUILD
@@ -228,6 +228,7 @@ java_library(
         "//src/java/com/google/devtools/mobileharness/platform/android/xts/plugin:__pkg__",
         "//src/javatests/com/google/devtools/mobileharness/infra/ats/common/jobcreator:__pkg__",
         "//src/javatests/com/google/devtools/mobileharness/infra/ats/console/controller/sessionplugin:__pkg__",
+        "//src/javatests/com/google/devtools/mobileharness/infra/ats/server/sessionplugin:__pkg__",
         "//src/javatests/com/google/devtools/mobileharness/platform/android/xts/plugin:__pkg__",
     ],
     deps = ["//src/java/com/google/wireless/qa/mobileharness/shared/constant:property"],

diff --git a/src/java/com/google/devtools/mobileharness/infra/ats/common/SessionResultHandlerUtil.java b/src/java/com/google/devtools/mobileharness/infra/ats/common/SessionResultHandlerUtil.java
@@ -835,6 +835,12 @@ private Optional<String> getLabGenFileDir(TestInfo test) {
     }
   }
 
+  public Path getTradefedInvocationLogDir(TestInfo tradefedTestInfo, Path logRootDir)
+      throws MobileHarnessException {
+    Path invocationDir = prepareTradefedInvocationDir(tradefedTestInfo, logRootDir);
+    return prepareLogOrResultDirForTest(tradefedTestInfo, invocationDir);
+  }
+
   private Path prepareTradefedInvocationDir(TestInfo tradefedTestInfo, Path logRootDir) {
     Path invocationDir;
     if (tradefedTestInfo.properties().has(XtsConstants.TRADEFED_INVOCATION_DIR_NAME)) {

diff --git a/src/java/com/google/devtools/mobileharness/infra/ats/server/sessionplugin/BUILD b/src/java/com/google/devtools/mobileharness/infra/ats/server/sessionplugin/BUILD
@@ -88,6 +88,7 @@ java_library(
         "//src/java/com/google/devtools/mobileharness/infra/client/longrunningservice/model:session_info",
         "//src/java/com/google/devtools/mobileharness/infra/lab/common/dir",
         "//src/java/com/google/devtools/mobileharness/platform/android/xts/common/util:xts_constants",
+        "//src/java/com/google/devtools/mobileharness/platform/android/xts/runtime:xts_tradefed_runtime_info_file_util",
         "//src/java/com/google/devtools/mobileharness/shared/util/auto:auto_value",
         "//src/java/com/google/devtools/mobileharness/shared/util/command",
         "//src/java/com/google/devtools/mobileharness/shared/util/error:more_throwables",

diff --git a/.../devtools/mobileharness/infra/ats/server/sessionplugin/NewMultiCommandRequestHandler.java b/.../devtools/mobileharness/infra/ats/server/sessionplugin/NewMultiCommandRequestHandler.java
@@ -16,9 +16,11 @@
 
 package com.google.devtools.mobileharness.infra.ats.server.sessionplugin;
 
+import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.common.base.Throwables.getStackTraceAsString;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.Iterables.getLast;
 import static com.google.devtools.mobileharness.shared.util.error.MoreThrowables.shortDebugString;
 import static com.google.devtools.mobileharness.shared.util.time.TimeUtils.toJavaDuration;
 import static java.nio.charset.StandardCharsets.UTF_8;
@@ -44,6 +46,7 @@
 import com.google.devtools.mobileharness.infra.ats.common.SessionRequestInfo;
 import com.google.devtools.mobileharness.infra.ats.common.SessionResultHandlerUtil;
 import com.google.devtools.mobileharness.infra.ats.common.XtsPropertyName;
+import com.google.devtools.mobileharness.infra.ats.common.XtsPropertyName.Job;
 import com.google.devtools.mobileharness.infra.ats.common.XtsTypeLoader;
 import com.google.devtools.mobileharness.infra.ats.common.jobcreator.XtsJobCreator;
 import com.google.devtools.mobileharness.infra.ats.common.proto.XtsCommonProto.ShardingMode;
@@ -62,6 +65,8 @@
 import com.google.devtools.mobileharness.infra.client.longrunningservice.model.SessionInfo;
 import com.google.devtools.mobileharness.infra.lab.common.dir.DirUtil;
 import com.google.devtools.mobileharness.platform.android.xts.common.util.XtsConstants;
+import com.google.devtools.mobileharness.platform.android.xts.runtime.XtsTradefedRuntimeInfoFileUtil;
+import com.google.devtools.mobileharness.platform.android.xts.runtime.XtsTradefedRuntimeInfoFileUtil.XtsTradefedRuntimeInfoFileDetail;
 import com.google.devtools.mobileharness.shared.util.command.Command;
 import com.google.devtools.mobileharness.shared.util.command.CommandExecutor;
 import com.google.devtools.mobileharness.shared.util.file.local.LocalFileUtil;
@@ -70,6 +75,7 @@
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.protobuf.util.Timestamps;
 import com.google.wireless.qa.mobileharness.shared.model.job.JobInfo;
+import com.google.wireless.qa.mobileharness.shared.model.job.TestInfo;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
@@ -111,13 +117,18 @@ final class NewMultiCommandRequestHandler {
       Pattern.compile("android-[a-z]+\\.zip");
   @VisibleForTesting static final String XTS_TF_JOB_PROP = "xts-tradefed-job";
 
+  @VisibleForTesting
+  static final String REQUEST_ERROR_MESSAGE_FOR_TRADEFED_INVOCATION_ERROR =
+      "Tradefed invocation had an error.";
+
   private static final ImmutableSet<ErrorId> INVALID_RESOURCE_ERROR_IDS =
       ImmutableSet.of(
           BasicErrorId.LOCAL_MOUNT_ZIP_TO_DIR_ERROR, InfraErrorId.ATS_SERVER_INVALID_TEST_RESOURCE);
 
   private final SessionRequestHandlerUtil sessionRequestHandlerUtil;
   private final SessionResultHandlerUtil sessionResultHandlerUtil;
   private final LocalFileUtil localFileUtil;
+  private final XtsTradefedRuntimeInfoFileUtil xtsTradefedRuntimeInfoFileUtil;
   private final CommandExecutor commandExecutor;
   private final Clock clock;
   private final XtsTypeLoader xtsTypeLoader;
@@ -136,13 +147,15 @@ final class NewMultiCommandRequestHandler {
       SessionRequestHandlerUtil sessionRequestHandlerUtil,
       SessionResultHandlerUtil sessionResultHandlerUtil,
       LocalFileUtil localFileUtil,
+      XtsTradefedRuntimeInfoFileUtil xtsTradefedRuntimeInfoFileUtil,
       CommandExecutor commandExecutor,
       Clock clock,
       XtsTypeLoader xtsTypeLoader,
       XtsJobCreator xtsJobCreator) {
     this.sessionRequestHandlerUtil = sessionRequestHandlerUtil;
     this.sessionResultHandlerUtil = sessionResultHandlerUtil;
     this.localFileUtil = localFileUtil;
+    this.xtsTradefedRuntimeInfoFileUtil = xtsTradefedRuntimeInfoFileUtil;
     this.commandExecutor = commandExecutor;
     this.clock = clock;
     this.xtsTypeLoader = xtsTypeLoader;
@@ -620,29 +633,31 @@ private HandleResultProcessingResult handleResultProcessingInternal(
           ImmutableMap.of());
     }
 
-    ImmutableMap.Builder<String, TestContext> testContextMap = ImmutableMap.builder();
-    ImmutableMap.Builder<String, CommandDetail> commandDetailMap = ImmutableMap.builder();
+    ImmutableMap.Builder<String, TestContext> testContextMapBuilder = ImmutableMap.builder();
+    ImmutableMap.Builder<String, CommandDetail> commandDetailMapBuilder = ImmutableMap.builder();
     for (CommandDetail commandDetail : commandDetails) {
       CommandDetail.Builder commandDetailBuilder = commandDetail.toBuilder();
       String commandId = commandDetail.getId();
+      Path outputDirPath =
+          Path.of(outputUrl.getPath()).resolve(sessionInfo.getSessionId()).resolve(commandId);
+      String resultDirectoryName =
+          TIMESTAMP_DIR_NAME_FORMATTER.format(Instant.now()) + "_" + getRandom4Digits();
+      Path resultDir = outputDirPath.resolve(resultDirectoryName);
+      Path logDir = outputDirPath.resolve("logs");
+      ImmutableList<JobInfo> jobs =
+          commandToJobsMap.get(commandId).stream()
+              .map(jobIdToJobMap::get)
+              .collect(toImmutableList());
       try {
         SessionRequestInfo sessionRequestInfo =
             getSessionRequestInfo(request, commandDetail.getOriginalCommandInfo(), sessionInfo);
-        Path outputDirPath =
-            Path.of(outputUrl.getPath()).resolve(sessionInfo.getSessionId()).resolve(commandId);
-        String resultDirectoryName =
-            TIMESTAMP_DIR_NAME_FORMATTER.format(Instant.now()) + "_" + getRandom4Digits();
-        Path resultDir = outputDirPath.resolve(resultDirectoryName);
-        Path logDir = outputDirPath.resolve("logs");
         Optional<Result> processResult =
             sessionResultHandlerUtil.processResult(
                 resultDir,
                 logDir,
                 /* latestResultLink= */ null,
                 /* latestLogLink= */ null,
-                commandToJobsMap.get(commandId).stream()
-                    .map(jobIdToJobMap::get)
-                    .collect(toImmutableList()),
+                jobs,
                 sessionRequestInfo);
         if (processResult.isPresent() && processResult.get().hasSummary()) {
           long failedModuleCount =
@@ -677,7 +692,7 @@ private HandleResultProcessingResult handleResultProcessingInternal(
                           .build())
                   .build();
           // TODO: filter context files.
-          testContextMap.put(commandId, testContext);
+          testContextMapBuilder.put(commandId, testContext);
         }
 
         if (localFileUtil.isDirExist(resultDir)) {
@@ -711,12 +726,12 @@ private HandleResultProcessingResult handleResultProcessingInternal(
               ErrorReason.RESULT_PROCESSING_ERROR,
               "No valid test cases found in the result.");
         }
-        // TODO: Collect state and error message from TF agent if exists.
+        checkTradefedInvocationError(commandDetailBuilder, jobs, logDir);
       }
       commandDetailBuilder
           .setEndTime(Timestamps.fromMillis(clock.millis()))
           .setUpdateTime(Timestamps.fromMillis(clock.millis()));
-      commandDetailMap.put(commandId, commandDetailBuilder.build());
+      commandDetailMapBuilder.put(commandId, commandDetailBuilder.build());
     }
 
     // Record OLC server session logs if no command recorded it due to empty command list or result
@@ -742,12 +757,34 @@ private HandleResultProcessingResult handleResultProcessingInternal(
             "Failed to create server session logs dir for session: %s", sessionInfo.getSessionId());
       }
     }
+
+    // Determine the error reason and error message for the request level, based on error reason and
+    // error message from commands.
+    ImmutableMap<String, CommandDetail> commandDetailsMap =
+        commandDetailMapBuilder.buildKeepingLast();
+    ErrorReason errorReason;
+    String errorMessage;
+    Optional<CommandDetail> commandDetailWithError =
+        commandDetailsMap.values().stream()
+            .filter(commandDetail -> commandDetail.getState() == CommandState.ERROR)
+            .findFirst();
+    if (commandDetailWithError.isPresent()) {
+      errorReason = commandDetailWithError.get().getErrorReason();
+      errorMessage =
+          errorReason == ErrorReason.TRADEFED_INVOCATION_ERROR
+              ? REQUEST_ERROR_MESSAGE_FOR_TRADEFED_INVOCATION_ERROR
+              : commandDetailWithError.get().getErrorMessage();
+    } else {
+      errorReason = null;
+      errorMessage = null;
+    }
+
     return HandleResultProcessingResult.of(
         RequestState.RUNNING,
-        /* errorReason= */ null,
-        /* errorMessage= */ null,
-        commandDetailMap.buildKeepingLast(),
-        testContextMap.buildKeepingLast());
+        errorReason,
+        errorMessage,
+        commandDetailMapBuilder.buildKeepingLast(),
+        testContextMapBuilder.buildKeepingLast());
   }
 
   // Clean up temporary files and directories in session and jobs.
@@ -770,6 +807,67 @@ void cleanup(SessionInfo sessionInfo) throws InterruptedException {
     }
   }
 
+  /**
+   * Reads the Tradefed runtime info file and checks for the Tradefed invocation error message, and
+   * set it in the command detail proto if present.
+   */
+  private void checkTradefedInvocationError(
+      CommandDetail.Builder commandDetailBuilder, ImmutableList<JobInfo> jobs, Path logDir) {
+    ImmutableList<TestInfo> tradefedTestInfos =
+        jobs.stream()
+            .filter(jobInfo -> jobInfo.properties().getBoolean(Job.IS_XTS_TF_JOB).orElse(false))
+            .flatMap(jobInfo -> jobInfo.tests().getAll().values().stream())
+            .collect(toImmutableList());
+
+    tradefedTestInfos.forEach(
+        tradefedTestInfo -> {
+          Path testLogDir;
+          try {
+            testLogDir =
+                sessionResultHandlerUtil.getTradefedInvocationLogDir(tradefedTestInfo, logDir);
+          } catch (MobileHarnessException e) {
+            logger.atWarning().withCause(e).log(
+                "Failed to get Tradefed invocation log dir for test %s",
+                tradefedTestInfo.locator().getId());
+            return;
+          }
+
+          Path runtimeInfoFilePath =
+              testLogDir.resolve(XtsConstants.TRADEFED_RUNTIME_INFO_FILE_NAME);
+          if (!localFileUtil.isFileExist(runtimeInfoFilePath)) {
+            return;
+          }
+
+          Optional<XtsTradefedRuntimeInfoFileDetail> fileDetailOptional;
+          try {
+            fileDetailOptional =
+                xtsTradefedRuntimeInfoFileUtil.readInfo(
+                    runtimeInfoFilePath, /* lastModifiedTime= */ null);
+          } catch (IOException | RuntimeException | Error e) {
+            logger.atWarning().withCause(e).log(
+                "Failed to read Tradefed runtime info of test %s from file %s",
+                tradefedTestInfo.locator().getId(), runtimeInfoFilePath);
+            return;
+          }
+
+          if (fileDetailOptional.isEmpty()) {
+            return;
+          }
+
+          String errorMessage =
+              getLast(fileDetailOptional.get().runtimeInfo().invocations()).errorMessage();
+
+          if (!isNullOrEmpty(errorMessage)) {
+            setCommandError(
+                commandDetailBuilder,
+                ErrorReason.TRADEFED_INVOCATION_ERROR,
+                commandDetailBuilder.getErrorMessage().isEmpty()
+                    ? errorMessage
+                    : commandDetailBuilder.getErrorMessage() + "\n" + errorMessage);
+          }
+        });
+  }
+
   private URL getTestResourceUrl(TestResource testResource) throws MobileHarnessException {
     try {
       return URI.create(testResource.getUrl()).toURL();

diff --git a/...java/com/google/devtools/mobileharness/platform/android/xts/common/util/XtsConstants.java b/...java/com/google/devtools/mobileharness/platform/android/xts/common/util/XtsConstants.java
@@ -43,6 +43,8 @@ public class XtsConstants {
    */
   public static final String TRADEFED_RUNTIME_INFO_FILE_PATH = "tf_runtime_info_file_path";
 
+  public static final String TRADEFED_RUNTIME_INFO_FILE_NAME = "tf_runtime_info";
+
   public static final String INVOCATION_SUMMARY_FILE_NAME = "invocation_summary.txt";
 
   /** A MH job property key to indicate whether xTS dynamic download is enabled. */

diff --git a/src/java/com/google/devtools/mobileharness/platform/android/xts/runtime/BUILD b/src/java/com/google/devtools/mobileharness/platform/android/xts/runtime/BUILD
@@ -22,6 +22,8 @@ java_library(
     srcs = ["XtsTradefedRuntimeInfoFileUtil.java"],
     visibility = [
         "//src/java/com/google/devtools/mobileharness/infra/ats/console/controller/sessionplugin:__pkg__",
+        "//src/java/com/google/devtools/mobileharness/infra/ats/server/sessionplugin:__pkg__",
+        "//src/javatests/com/google/devtools/mobileharness/infra/ats/server/sessionplugin:__pkg__",
         "//src/javatests/com/google/devtools/mobileharness/platform/android/xts/agent:__pkg__",
     ],
     deps = [
@@ -35,6 +37,7 @@ java_library(
     srcs = ["XtsTradefedRuntimeInfo.java"],
     visibility = [
         "//src/java/com/google/devtools/mobileharness/infra/ats/console/controller/sessionplugin:__pkg__",
+        "//src/javatests/com/google/devtools/mobileharness/infra/ats/server/sessionplugin:__pkg__",
         "//src/javatests/com/google/devtools/mobileharness/platform/android/xts/agent:__pkg__",
         "//src/javatests/com/google/devtools/mobileharness/platform/android/xts/runtime:__pkg__",
     ],

diff --git a/src/java/com/google/wireless/qa/mobileharness/shared/api/driver/XtsTradefedTest.java b/src/java/com/google/wireless/qa/mobileharness/shared/api/driver/XtsTradefedTest.java
@@ -357,7 +357,8 @@ private Optional<Integer> runXtsCommand(
         getEnvironmentToTradefedConsole(tmpXtsRootDir, xtsType, spec);
 
     // Create runtime info file path.
-    Path runtimeInfoFilePath = Path.of(testInfo.getGenFileDir()).resolve("tf_runtime_info");
+    Path runtimeInfoFilePath =
+        Path.of(testInfo.getGenFileDir()).resolve(XtsConstants.TRADEFED_RUNTIME_INFO_FILE_NAME);
     testInfo
         .properties()
         .add(XtsConstants.TRADEFED_RUNTIME_INFO_FILE_PATH, runtimeInfoFilePath.toString());

diff --git a/src/javatests/com/google/devtools/mobileharness/infra/ats/server/sessionplugin/BUILD b/src/javatests/com/google/devtools/mobileharness/infra/ats/server/sessionplugin/BUILD
@@ -47,6 +47,7 @@ java_library(
         "//src/java/com/google/devtools/mobileharness/infra/ats/common:session_request_handler_util",
         "//src/java/com/google/devtools/mobileharness/infra/ats/common:session_request_info",
         "//src/java/com/google/devtools/mobileharness/infra/ats/common:session_result_handler_util",
+        "//src/java/com/google/devtools/mobileharness/infra/ats/common:xts_property_name",
         "//src/java/com/google/devtools/mobileharness/infra/ats/common:xts_type_loader",
         "//src/java/com/google/devtools/mobileharness/infra/ats/common/jobcreator:xts_job_creator",
         "//src/java/com/google/devtools/mobileharness/infra/ats/server/sessionplugin:ats_server_session_plugin_lib",
@@ -60,6 +61,8 @@ java_library(
         "//src/java/com/google/devtools/mobileharness/infra/controller/scheduler/model/job/in:device_requirements",
         "//src/java/com/google/devtools/mobileharness/infra/lab/common/dir",
         "//src/java/com/google/devtools/mobileharness/platform/android/xts/common/util:xts_constants",
+        "//src/java/com/google/devtools/mobileharness/platform/android/xts/runtime:xts_tradefed_runtime_info",
+        "//src/java/com/google/devtools/mobileharness/platform/android/xts/runtime:xts_tradefed_runtime_info_file_util",
         "//src/java/com/google/devtools/mobileharness/shared/util/command",
         "//src/java/com/google/devtools/mobileharness/shared/util/file/local",
         "//src/java/com/google/devtools/mobileharness/shared/util/flags",