crosperf: fix the skylab launch tests mode

Crosperf provides a mode to use `skylab create-test` to launch tests
when user specify 'skylab: true' in experiment file.

Since Skylab has been updated in these months, the command line itself
changes. This patch fixes it:

1) bb mode becomes deprecated, do not use it.
2) Task id becomes a recipe id, we need to parse it instead of swarming.
3) Real result location can be found in the json output of `wait-task`
from key "child-result".

TEST=Passed unittest, tested with simple experiment file.
BUG=chromium:984790

Change-Id: Iee114b1e6c623bb64a0ca784df9c2ba6d95e3d07
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/1916542
Reviewed-by: Zhizhou Yang <zhizhouy@google.com>
Reviewed-by: Caroline Tice <cmtice@chromium.org>
Tested-by: Zhizhou Yang <zhizhouy@google.com>
Auto-Submit: Zhizhou Yang <zhizhouy@google.com>

3 files changed, 66 insertions, 26 deletions

diff --git a/crosperf/experiment_runner.py b/crosperf/experiment_runner.py
index cb6e9785..a536f161 100644
--- a/crosperf/experiment_runner.py
+++ b/crosperf/experiment_runner.py
@@ -148,7 +148,7 @@ class ExperimentRunner(object):
       experiment.locked_machines = self.locked_machines
       self._UpdateMachineList(self.locked_machines)
       experiment.machine_manager.RemoveNonLockedMachines(self.locked_machines)
-      if len(self.locked_machines) == 0:
+      if not self.locked_machines:
         raise RuntimeError('Unable to lock any machines.')
 
   def _ClearCacheEntries(self, experiment):
@@ -168,7 +168,13 @@ class ExperimentRunner(object):
 
   def _Run(self, experiment):
     try:
-      self._LockAllMachines(experiment)
+      # We should not lease machines if tests are launched via `skylab
+      # create-test`. This is because leasing DUT in skylab will create a
+      # dummy task on the DUT and new test created will be hanging there.
+      # TODO(zhizhouy): Need to check whether machine is ready or not before
+      # assigning a test to it.
+      if not experiment.skylab:
+        self._LockAllMachines(experiment)
       # Calculate all checksums of avaiable/locked machines, to ensure same
       # label has same machines for testing
       experiment.SetCheckSums(forceSameImage=True)
diff --git a/crosperf/suite_runner.py b/crosperf/suite_runner.py
index 2311a728..ddca65df 100644
--- a/crosperf/suite_runner.py
+++ b/crosperf/suite_runner.py
@@ -8,6 +8,7 @@
 from __future__ import division
 from __future__ import print_function
 
+import json
 import os
 import shlex
 import time
@@ -212,9 +213,10 @@ class SuiteRunner(object):
     cp_command = '%s -mq cp -r %s %s' % (gsutil_cmd, result_dir, download_path)
 
     # Server sometimes will not be able to generate the result directory right
-    # after the test. Will try to access this gs location every 60s for 5 mins.
+    # after the test. Will try to access this gs location every 60s for
+    # RETRY_LIMIT mins.
     t = 0
-    RETRY_LIMIT = 5
+    RETRY_LIMIT = 10
     while t < RETRY_LIMIT:
       t += 1
       status = self._ce.RunCommand(ls_command, print_to_console=False)
@@ -234,23 +236,20 @@ class SuiteRunner(object):
     status = self._ce.RunCommand(cp_command)
     if status != 0:
       self.logger.LogOutput('Cannot download results from task %s' % task_id)
+    else:
+      self.logger.LogOutput('Result downloaded for task %s' % task_id)
     return status
 
   def Skylab_Run(self, label, benchmark, test_args, profiler_args):
     """Run the test via skylab.."""
-    # Skylab by default uses cros_test_platform to start test.
-    # We don't use it for now since we want to directly interact with dut.
-    options = '-bb=false'
-
-    if benchmark.suite != 'telemetry_Crosperf':
-      options += ' -client-test'
+    options = ''
     if label.board:
-      options += ' -board=%s' % label.board
+      options += '-board=%s' % label.board
     if label.build:
       options += ' -image=%s' % label.build
-    # TODO: now only put quota pool here, user need to be able to specify which
-    # pool to use. Need to request feature to not use this option at all.
-    options += ' -pool=DUT_POOL_QUOTA'
+    # TODO: now only put toolchain pool here, user need to be able to specify
+    # which pool to use. Need to request feature to not use this option at all.
+    options += ' -pool=toolchain'
     if benchmark.suite == 'telemetry_Crosperf':
       if test_args:
         # Strip double quotes off args (so we can wrap them in single
@@ -273,7 +272,7 @@ class SuiteRunner(object):
     for dut in label.remote:
       dimensions += ' -dim dut_name:%s' % dut.rstrip('.cros')
 
-    command = (('%s create-test %s %s %s') % \
+    command = (('%s create-test%s %s %s') % \
               (SKYLAB_PATH, dimensions, options, benchmark.test_name))
 
     if self.log_level != 'verbose':
@@ -286,17 +285,40 @@ class SuiteRunner(object):
       return ret_tup
 
     # Std output of the command will look like:
-    # Created Swarming task https://chromeos-swarming.appspot.com/task?id=12345
-    # We want to parse it and get the id number of the task.
-    task_id = ret_tup[1].strip().split('id=')[1]
+    # Created request at https://ci.chromium.org/../cros_test_platform/b12345
+    # We want to parse it and get the id number of the task, which is the
+    # number in the very end of the link address.
+    task_id = ret_tup[1].strip().split('b')[-1]
 
-    command = ('skylab wait-task -bb=false %s' % (task_id))
+    command = ('skylab wait-task %s' % task_id)
     if self.log_level != 'verbose':
       self.logger.LogOutput('Waiting for skylab test to finish.')
       self.logger.LogOutput('CMD: %s' % command)
 
     ret_tup = self._ce.RunCommandWOutput(command, command_terminator=self._ct)
-    if '"success":true' in ret_tup[1]:
+
+    # The output of `wait-task` command will be a combination of verbose and a
+    # json format result in the end. The json result looks like this:
+    # {"task-result":
+    #   {"name":"Test Platform Invocation",
+    #    "state":"", "failure":false, "success":true,
+    #    "task-run-id":"12345",
+    #    "task-run-url":"https://ci.chromium.org/.../cros_test_platform/b12345",
+    #    "task-logs-url":""
+    #    },
+    #  "stdout":"",
+    #  "child-results":
+    #    [{"name":"graphics_WebGLAquarium",
+    #      "state":"", "failure":false, "success":true, "task-run-id":"",
+    #      "task-run-url":"https://chromeos-swarming.appspot.com/task?id=1234",
+    #      "task-logs-url":"https://stainless.corp.google.com/1234/"}
+    #    ]
+    # }
+    # We need the task id of the child-results to download result.
+    output = json.loads(ret_tup[1].split('\n')[-1])
+    output = output['child-results'][0]
+    if output['success']:
+      task_id = output['task-run-url'].split('=')[-1]
       if self.DownloadResult(label, task_id) == 0:
         result_dir = '\nResults placed in tmp/swarming-%s\n' % task_id
         return (ret_tup[0], result_dir, ret_tup[2])
diff --git a/crosperf/suite_runner_unittest.py b/crosperf/suite_runner_unittest.py
index edde46a7..42b2392d 100755
--- a/crosperf/suite_runner_unittest.py
+++ b/crosperf/suite_runner_unittest.py
@@ -9,6 +9,7 @@
 
 from __future__ import print_function
 
+import json
 import os.path
 import time
 
@@ -30,6 +31,7 @@ class SuiteRunnerTest(unittest.TestCase):
   """Class of SuiteRunner test."""
   real_logger = logger.GetLogger()
 
+  mock_json = mock.Mock(spec=json)
   mock_cmd_exec = mock.Mock(spec=command_executer.CommandExecuter)
   mock_cmd_term = mock.Mock(spec=command_executer.CommandTerminator)
   mock_logger = mock.Mock(spec=logger.Logger)
@@ -324,7 +326,8 @@ class SuiteRunnerTest(unittest.TestCase):
     self.assertEqual(len(args_dict), 2)
 
   @mock.patch.object(command_executer.CommandExecuter, 'RunCommandWOutput')
-  def test_skylab_run(self, mock_runcmd):
+  @mock.patch.object(json, 'loads')
+  def test_skylab_run(self, mock_json_loads, mock_runcmd):
 
     def FakeDownloadResult(l, task_id):
       if l and task_id:
@@ -333,9 +336,18 @@ class SuiteRunnerTest(unittest.TestCase):
 
     mock_runcmd.return_value = \
       (0,
-       '"success":true\nCreated Swarming task https://swarming/task?id=12345',
+       'Created Swarming task https://swarming/task/b12345',
        '')
     self.mock_cmd_exec.RunCommandWOutput = mock_runcmd
+
+    mock_json_loads.return_value = {
+        'child-results': [{
+            'success': True,
+            'task-run-url': 'https://swarming/task?id=12345'
+        }]
+    }
+    self.mock_json.loads = mock_json_loads
+
     self.mock_label.skylab = True
     self.runner.DownloadResult = FakeDownloadResult
     res = self.runner.Skylab_Run(self.mock_label, self.test_that_bench, '', '')
@@ -346,14 +358,14 @@ class SuiteRunnerTest(unittest.TestCase):
     args_list = mock_runcmd.call_args_list[0][0]
     args_dict = mock_runcmd.call_args_list[0][1]
     self.assertEqual(args_list[0],
-                     ('/usr/local/bin/skylab create-test  '
+                     ('/usr/local/bin/skylab create-test '
                       '-dim dut_name:lumpy1 -dim dut_name:lumpy.cros2 '
-                      '-bb=false -client-test -board=lumpy -image=build '
-                      '-pool=DUT_POOL_QUOTA octane'))
+                      '-board=lumpy -image=build '
+                      '-pool=toolchain octane'))
     self.assertEqual(args_dict['command_terminator'], self.mock_cmd_term)
 
     args_list = mock_runcmd.call_args_list[1][0]
-    self.assertEqual(args_list[0], ('skylab wait-task -bb=false 12345'))
+    self.assertEqual(args_list[0], ('skylab wait-task 12345'))
     self.assertEqual(args_dict['command_terminator'], self.mock_cmd_term)
 
   @mock.patch.object(time, 'sleep')