Refactor the results collection code to run out of a separate class
that we can reuse in different places, and change the mkfifo code
to use a filename created on the fly, rather than hardcoding a fixed
filename.

Risk: Medium
Visibility: Mostly just a refactoring.

Signed-off-by: John Admanski <jadmanski@google.com>



git-svn-id: http://test.kernel.org/svn/autotest/trunk@2159 592f7852-d20e-0410-864c-8624ca9c26a4
diff --git a/server/autotest.py b/server/autotest.py
index 984492b..f80b5f7 100644
--- a/server/autotest.py
+++ b/server/autotest.py
@@ -198,59 +198,6 @@
         return host
 
 
-    def prepare_for_copying_logs(self, src, dest, host):
-        keyval_path = ''
-        if not os.path.exists(os.path.join(dest, 'keyval')):
-            # Client-side keyval file can be copied directly
-            return keyval_path
-        # Copy client-side keyval to temporary location
-        try:
-            try:
-                # Create temp file
-                fd, keyval_path = tempfile.mkstemp('.keyval_%s' % host.hostname)
-                host.get_file(os.path.join(src, 'keyval'), keyval_path)
-            finally:
-                # We will squirrel away the client side keyval
-                # away and move it back when we are done
-                self.temp_keyval_path = tempfile.mktemp()
-                host.run('mv %s %s' %
-                         (os.path.join(src, 'keyval'), self.temp_keyval_path))
-        except (error.AutoservRunError, error.AutoservSSHTimeout):
-            print "Prepare for copying logs failed"
-        return keyval_path
-
-
-    def process_copied_logs(self, dest, host, keyval_path):
-        if not os.path.exists(os.path.join(dest, 'keyval')):
-            # Client-side keyval file was copied directly
-            return
-        # Append contents of keyval_<host> file to keyval file
-        try:
-            # Read in new and old keyval files
-            new_keyval = utils.read_keyval(keyval_path)
-            old_keyval = utils.read_keyval(dest)
-            # 'Delete' from new keyval entries that are in both
-            tmp_keyval = {}
-            for key, val in new_keyval.iteritems():
-                if key not in old_keyval:
-                    tmp_keyval[key] = val
-            # Append new info to keyval file
-            utils.write_keyval(dest, tmp_keyval)
-            # Delete keyval_<host> file
-            os.remove(keyval_path)
-        except IOError:
-            print "Process copied logs failed"
-
-
-    def postprocess_copied_logs(self, src, host):
-        # we can now put our keyval file back
-        try:
-            host.run('mv %s %s' % (self.temp_keyval_path,
-                     os.path.join(src, 'keyval')))
-        except:
-            pass
-
-
     def _do_run(self, control_file, results_dir, host, atrun, timeout):
         try:
             atrun.verify_machine()
@@ -298,36 +245,11 @@
         if os.path.abspath(tmppath) != os.path.abspath(control_file):
             os.remove(tmppath)
 
-        collect_func = lambda: self._collect_results(atrun, host, results_dir)
-        host.job.collect_client_job_results = collect_func
         try:
             atrun.execute_control(timeout=timeout)
         finally:
-            del host.job.collect_client_job_results
-            collect_func()
-
-
-    def _collect_results(self, atrun, host, results_dir):
-        # make an effort to wait for the machine to come up
-        try:
-            host.wait_up(timeout=30)
-        except error.AutoservError:
-            # don't worry about any errors, we'll try and
-            # get the results anyway
-            pass
-
-        # get the results
-        if not atrun.tag:
-            results = os.path.join(atrun.autodir, 'results', 'default')
-        else:
-            results = os.path.join(atrun.autodir, 'results', atrun.tag)
-
-        # Copy all dirs in default to results_dir
-        keyval_path = self.prepare_for_copying_logs(results, results_dir,
-                                                    host)
-        host.get_file(results + '/', results_dir)
-        self.process_copied_logs(results_dir, host, keyval_path)
-        self.postprocess_copied_logs(results, host)
+            collector = server_job.log_collector(host, atrun.tag, results_dir)
+            collector.collect_client_job_results()
 
 
     def run_timed_test(self, test_name, results_dir='.', host=None,
@@ -414,7 +336,8 @@
 
         full_cmd = self.get_full_cmd(section)
         client_log = self.get_client_log(section)
-        redirector = server_job.client_logger(self.host)
+        redirector = server_job.client_logger(self.host, self.tag,
+                                              self.results_dir)
 
         try:
             old_resultdir = self.host.job.resultdir
diff --git a/server/autotest_unittest.py b/server/autotest_unittest.py
index ca53d4c..a0f44c6 100644
--- a/server/autotest_unittest.py
+++ b/server/autotest_unittest.py
@@ -40,6 +40,7 @@
         self.god.stub_function(autotest.global_config.global_config,
                                "get_config_value")
         self.god.stub_class(autotest, "_Run")
+        self.god.stub_class(server_job, "log_collector")
 
 
     def tearDown(self):
@@ -116,9 +117,6 @@
 
         # stub out install
         self.god.stub_function(self.base_autotest, "install")
-        self.god.stub_function(self.base_autotest, "prepare_for_copying_logs")
-        self.god.stub_function(self.base_autotest, "process_copied_logs")
-        self.god.stub_function(self.base_autotest, "postprocess_copied_logs")
 
         # record
         self.base_autotest.install.expect_call(self.host)
@@ -165,69 +163,13 @@
             'autodir/control.None.state')
         os.remove.expect_call("temp")
         run_obj.execute_control.expect_call(timeout=30)
-        self.host.wait_up.expect_call(timeout=30)
-
-        run_obj.autodir = 'autodir'
-        results = os.path.join(run_obj.autodir,
-                               'results', 'default')
-        self.base_autotest.prepare_for_copying_logs.expect_call(
-            'autodir/results/default', '.', self.host).and_return('keyval_path')
-        self.host.get_file.expect_call('autodir/results/default/', '.')
-        self.base_autotest.process_copied_logs.expect_call('.',self.host,
-            'keyval_path')
-        self.base_autotest.postprocess_copied_logs.expect_call(results,
-            self.host)
+        collector = server_job.log_collector.expect_new(self.host, tag, '.')
+        collector.collect_client_job_results.expect_call()
 
         # run and check output
         self.base_autotest.run(control, timeout=30)
         self.god.check_playback()
 
 
-    def test_prepare_for_copying_logs(self):
-        self.construct()
-
-        # record
-        src = "src"
-        dest = "dest"
-        keyval_path = ''
-        os.path.exists.expect_call(os.path.join(dest,
-                                                'keyval')).and_return(True)
-        tempfile.mkstemp.expect_call(
-            '.keyval_%s' % self.host.hostname).and_return((None, keyval_path))
-        self.host.get_file.expect_call(os.path.join(src, 'keyval'), keyval_path)
-        tempfile.mktemp.expect_call().and_return("temp_keyval")
-        self.host.run.expect_call('mv %s temp_keyval' %
-                                   os.path.join(src, 'keyval'))
-
-        # run and check
-        self.base_autotest.prepare_for_copying_logs(src, dest, self.host)
-        self.god.check_playback()
-
-
-    def test_process_copied_logs(self):
-        self.construct()
-
-        # record
-        dest = "dest"
-        keyval_path = "keyval_path"
-        os.path.exists.expect_call(os.path.join(dest,
-                                                'keyval')).and_return(True)
-        old_keyval = {"version": 1, "author": "me"}
-        new_keyval = {"version": 1, "data": "foo"}
-        utils.read_keyval.expect_call(
-            keyval_path).and_return(new_keyval)
-        utils.read_keyval.expect_call(dest).and_return(old_keyval)
-        tmp_keyval = {}
-        for key, val in new_keyval.iteritems():
-            if key not in old_keyval:
-                tmp_keyval[key] = val
-        utils.write_keyval.expect_call(dest, tmp_keyval)
-        os.remove.expect_call(keyval_path)
-
-        # run check
-        self.base_autotest.process_copied_logs(dest, self.host, keyval_path)
-        self.god.check_playback()
-
-
 if __name__ == "__main__":
     unittest.main()
diff --git a/server/server_job.py b/server/server_job.py
index ebf0bdd..b1e6296 100755
--- a/server/server_job.py
+++ b/server/server_job.py
@@ -701,12 +701,95 @@
 
 
 
+class log_collector(object):
+    def __init__(self, host, client_tag, results_dir):
+        self.host = host
+        if not client_tag:
+            client_tag = "default"
+        self.client_results_dir = os.path.join(host.get_autodir(), "results",
+                                               client_tag)
+        self.server_results_dir = results_dir
+
+
     def collect_client_job_results(self):
         """ A method that collects all the current results of a running
         client job into the results dir. By default does nothing as no
         client job is running, but when running a client job you can override
         this with something that will actually do something. """
-        pass
+
+        # make an effort to wait for the machine to come up
+        try:
+            self.host.wait_up(timeout=30)
+        except error.AutoservError:
+            # don't worry about any errors, we'll try and
+            # get the results anyway
+            pass
+
+
+        # Copy all dirs in default to results_dir
+        keyval_path = self._prepare_for_copying_logs()
+        self.host.get_file(self.client_results_dir + '/',
+                           self.server_results_dir)
+        self._process_copied_logs(keyval_path)
+        self._postprocess_copied_logs()
+
+
+    def _prepare_for_copying_logs(self):
+        server_keyval = os.path.join(self.server_results_dir, 'keyval')
+        if not os.path.exists(server_keyval):
+            # Client-side keyval file can be copied directly
+            return
+
+        # Copy client-side keyval to temporary location
+        suffix = '.keyval_%s' % self.host.hostname
+        fd, keyval_path = tempfile.mkstemp(suffix)
+        os.close(fd)
+        try:
+            client_keyval = os.path.join(self.client_results_dir, 'keyval')
+            try:
+                self.host.get_file(client_keyval, keyval_path)
+            finally:
+                # We will squirrel away the client side keyval
+                # away and move it back when we are done
+                remote_temp_dir = self.host.get_tmp_dir()
+                self.temp_keyval_path = os.path.join(remote_temp_dir, "keyval")
+                self.host.run('mv %s %s' % (client_keyval,
+                                            self.temp_keyval_path))
+        except (error.AutoservRunError, error.AutoservSSHTimeout):
+            print "Prepare for copying logs failed"
+        return keyval_path
+
+
+    def _process_copied_logs(self, keyval_path):
+        if not keyval_path:
+            # Client-side keyval file was copied directly
+            return
+
+        # Append contents of keyval_<host> file to keyval file
+        try:
+            # Read in new and old keyval files
+            new_keyval = utils.read_keyval(keyval_path)
+            old_keyval = utils.read_keyval(self.server_results_dir)
+            # 'Delete' from new keyval entries that are in both
+            tmp_keyval = {}
+            for key, val in new_keyval.iteritems():
+                if key not in old_keyval:
+                    tmp_keyval[key] = val
+            # Append new info to keyval file
+            utils.write_keyval(self.server_results_dir, tmp_keyval)
+            # Delete keyval_<host> file
+            os.remove(keyval_path)
+        except IOError:
+            print "Process copied logs failed"
+
+
+    def _postprocess_copied_logs(self):
+        # we can now put our keyval file back
+        client_keyval = os.path.join(self.client_results_dir, 'keyval')
+        try:
+            self.host.run('mv %s %s' % (self.temp_keyval_path, client_keyval))
+        except Exception:
+            pass
 
 
 # a file-like object for catching stderr from an autotest client and
@@ -716,13 +799,14 @@
     the status log file.  We only implement those methods
     utils.run() actually calls.
     """
-    parser = re.compile(r"^AUTOTEST_STATUS:([^:]*):(.*)$")
-    test_complete = re.compile(r"^AUTOTEST_TEST_COMPLETE$")
+    status_parser = re.compile(r"^AUTOTEST_STATUS:([^:]*):(.*)$")
+    test_complete_parser = re.compile(r"^AUTOTEST_TEST_COMPLETE:(.*)$")
     extract_indent = re.compile(r"^(\t*).*$")
 
-    def __init__(self, host):
+    def __init__(self, host, tag, server_results_dir):
         self.host = host
         self.job = host.job
+        self.log_collector = log_collector(host, tag, server_results_dir)
         self.leftover = ""
         self.last_line = ""
         self.logs = {}
@@ -780,14 +864,15 @@
         lines sent by autotest will be prepended with
         "AUTOTEST_STATUS", and all other lines are ssh error
         messages."""
-        match = self.parser.search(line)
-        if match:
-            tag, line = match.groups()
+        status_match = self.status_parser.search(line)
+        test_complete_match = self.test_complete_parser.search(line)
+        if status_match:
+            tag, line = status_match.groups()
             self._process_quoted_line(tag, line)
-        elif self.test_complete.search(line):
-            self.job.collect_client_job_results()
-            fifo = os.path.join(self.host.get_autodir(), "autoserv.fifo")
-            self.host.run("echo A > %s" % fifo)
+        elif test_complete_match:
+            fifo_path, = test_complete_match.groups()
+            self.log_collector.collect_client_job_results()
+            self.host.run("echo A > %s" % fifo_path)
         else:
             print line
 
diff --git a/server/server_job_unittest.py b/server/server_job_unittest.py
index f17d52f..3aea277 100644
--- a/server/server_job_unittest.py
+++ b/server/server_job_unittest.py
@@ -1,8 +1,8 @@
 #!/usr/bin/python
 
-import unittest, os, time
+import unittest, os, time, tempfile
 import common
-from autotest_lib.server import server_job, test, subcommand
+from autotest_lib.server import server_job, test, subcommand, hosts
 from autotest_lib.client.common_lib import utils, error, host_protections
 from autotest_lib.client.common_lib import packages
 from autotest_lib.tko import db as tko_db, status_lib, utils as tko_utils
@@ -364,5 +364,63 @@
         self.god.check_playback()
 
 
+class BaseServerJobTest(unittest.TestCase):
+    def setUp(self):
+        self.god = mock.mock_god()
+
+        self.host = self.god.create_mock_class(hosts.RemoteHost, "host")
+        self.host.hostname = "testhost"
+
+        self.god.stub_function(os.path, "exists")
+        self.god.stub_function(os, "close")
+        self.god.stub_function(os, "remove")
+        self.god.stub_function(tempfile, "mkstemp")
+        self.god.stub_function(utils, "read_keyval")
+        self.god.stub_function(utils, "write_keyval")
+
+
+    def tearDown(self):
+        self.god.unstub_all()
+
+
+    def test_prepare_for_copying_logs(self):
+        self.host.get_autodir.expect_call().and_return("/autodir")
+        collector = server_job.log_collector(self.host, None, "/resultsdir")
+        self.god.check_playback()
+
+        os.path.exists.expect_call("/resultsdir/keyval").and_return(True)
+        tempfile.mkstemp.expect_call(".keyval_testhost").and_return(
+            (10, "tmp.keyval_testhost"))
+        os.close.expect_call(10)
+        self.host.get_file.expect_call("/autodir/results/default/keyval",
+                                       "tmp.keyval_testhost")
+        self.host.get_tmp_dir.expect_call().and_return("/autotmp")
+        self.host.run.expect_call(
+            "mv /autodir/results/default/keyval /autotmp/keyval")
+
+        # run and check
+        keyval = collector._prepare_for_copying_logs()
+        self.assertEquals(keyval, "tmp.keyval_testhost")
+        self.god.check_playback()
+
+
+    def test_process_copied_logs(self):
+        self.host.get_autodir.expect_call().and_return("/autodir")
+        collector = server_job.log_collector(self.host, None, "/resultsdir")
+        self.god.check_playback()
+
+        utils.read_keyval.expect_call("tmp.keyval_testhost").and_return(
+            {"field1": "new thing", "field3": "other new thing"})
+        utils.read_keyval.expect_call("/resultsdir").and_return(
+            {"field1": "thing", "field2": "otherthing"})
+        utils.write_keyval.expect_call("/resultsdir",
+                                       {"field3": "other new thing"})
+        os.remove.expect_call("tmp.keyval_testhost")
+
+        # run and check
+        collector._process_copied_logs("tmp.keyval_testhost")
+        self.god.check_playback()
+
+
 if __name__ == "__main__":
     unittest.main()