Risk: Medium
Visibility: Changes the test keyval interface, deprecating the existing
test.write_keyval method in favour of a pair of write_*_keyval methods
for writing test & iteration keyvals. The deprecated method will still
work as it did before, but will generate a warning.

Adds a new iteration_attributes table to the database for storing
generic string attributes on a per-iteration basis, in the same way
that test_attributes allows generic string attributes on a per-test
basis.

This also adds new methods to the test class for writing these keyvals
so that tests can write out attributes by calling
self.write_test_keyval (or self.write_iteration_keyval). The iteration
method accepts parameters for both generic attributes and performance
data.

In order to store both performance and non-performance data in the
iteration keyvals, the format of the line has been extended to look
like "key{blah}=value", with no {blah} being interpreted as equvalent
to "{perf}", for backwards compatiblity.

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



git-svn-id: http://test.kernel.org/svn/autotest/trunk@1535 592f7852-d20e-0410-864c-8624ca9c26a4
diff --git a/client/common_lib/test.py b/client/common_lib/test.py
index 313d350..5dba155 100644
--- a/client/common_lib/test.py
+++ b/client/common_lib/test.py
@@ -16,7 +16,7 @@
 #	src		eg. tests/<test>/src
 #	tmpdir		eg. tmp/<testname.tag>
 
-import os, sys, re, fcntl, shutil, tarfile
+import os, sys, re, fcntl, shutil, tarfile, warnings
 
 from autotest_lib.client.common_lib import error, utils
 
@@ -78,8 +78,40 @@
 			raise error.TestError(msg)
 
 
+	def write_test_keyval(self, attr_dict):
+		utils.write_keyval(self.outputdir, attr_dict)
+
+
+	@staticmethod
+	def _append_type_to_keys(dictionary, typename):
+		new_dict = {}
+		for key, value in dictionary.iteritems():
+			new_key = "%s{%s}" % (key, typename)
+			new_dict[new_key] = value
+		return new_dict
+
+
+	def write_iteration_keyval(self, attr_dict, perf_dict):
+		attr_dict = self._append_type_to_keys(attr_dict, "attr")
+		perf_dict = self._append_type_to_keys(perf_dict, "perf")
+
+		utils.write_keyval(self.resultsdir, attr_dict,
+				   type_tag="attr")
+		utils.write_keyval(self.resultsdir, perf_dict,
+				   type_tag="perf")
+
+		keyval_path = os.path.join(self.resultsdir, "keyval")
+		print >> open(keyval_path, "a"), ""
+
+
+	# TODO: deprecate, remove from code in favour of
+	# the write_*_keyval methods
         def write_keyval(self, dictionary):
-		utils.write_keyval(self.resultsdir, dictionary)
+		warnings.warn("test.write_keyval is deprecated, use "
+			      "test.write_test_keyval or "
+			      "test.write_iteration_keyval instead",
+			      DeprecationWarning)
+		self.write_iteration_keyval({}, dictionary)
 
 
 	def initialize(self):
@@ -103,9 +135,8 @@
 
 			try:
 				os.chdir(self.outputdir)
-				version_keyval = {'version': self.version}
-				utils.write_keyval(self.outputdir,
-						   version_keyval)
+				version_dict = {'version': self.version}
+				self.write_test_keyval(version_dict)
 				self.execute(*args, **dargs)
 			finally:
 				self.cleanup()
diff --git a/client/common_lib/utils.py b/client/common_lib/utils.py
index d2fb9d3..c6ec525 100644
--- a/client/common_lib/utils.py
+++ b/client/common_lib/utils.py
@@ -37,13 +37,31 @@
 	return keyval
 
 
-def write_keyval(path, dictionary):
+def write_keyval(path, dictionary, type_tag=None):
+	"""
+	Write a key-value pair format file out to a file. This uses append
+	mode to open the file, so existing text will not be overwritten or
+	reparsed.
+
+	If type_tag is None, then the key must be composed of alphanumeric
+	characters (or dashes+underscores). However, if type-tag is not
+	null then the keys must also have "{type_tag}" as a suffix. At
+	the moment the only valid values of type_tag are "attr" and "perf".
+	"""
 	if os.path.isdir(path):
 		path = os.path.join(path, 'keyval')
 	keyval = open(path, 'a')
+
+	if type_tag is None:
+		key_regex = re.compile(r'^[-\w]+$')
+	else:
+		if type_tag not in ('attr', 'perf'):
+			raise ValueError('Invalid type tag: %s' % type_tag)
+		escaped_tag = re.escape(type_tag)
+		key_regex = re.compile(r'^[-\w]+\{%s\}$' % escaped_tag)
 	try:
 		for key, value in dictionary.iteritems():
-			if re.search(r'[^-\w]', key):
+			if not key_regex.search(key):
 				raise ValueError('Invalid key: %s' % key)
 			keyval.write('%s=%s\n' % (key, value))
 	finally:
diff --git a/tko/db.py b/tko/db.py
index 2d992ac..c197ba5 100644
--- a/tko/db.py
+++ b/tko/db.py
@@ -320,15 +320,20 @@
 
 		for i in test.iterations:
 			data['iteration'] = i.index
-			for key in i.keyval:
+			for key, value in i.attr_keyval.iteritems():
 				data['attribute'] = key
-				data['value'] = i.keyval[key]
-				self.insert('iteration_result',
-                                            data,
-                                            commit=commit)
+				data['value'] = value
+				self.insert('iteration_attributes', data,
+					    commit=commit)
+			for key, value in i.perf_keyval.iteritems():
+				data['attribute'] = key
+				data['value'] = value
+				self.insert('iteration_result', data,
+					    commit=commit)
 
 		for key, value in test.attributes.iteritems():
-			data = {'test_idx': test_idx, 'attribute': key, 'value': value}
+			data = {'test_idx': test_idx, 'attribute': key,
+				'value': value}
 			self.insert('test_attributes', data, commit=commit)
 
 
diff --git a/tko/migrations/008_add_iteration_attributes.py b/tko/migrations/008_add_iteration_attributes.py
new file mode 100644
index 0000000..4ef176e
--- /dev/null
+++ b/tko/migrations/008_add_iteration_attributes.py
@@ -0,0 +1,22 @@
+def migrate_up(manager):
+	manager.execute_script(CREATE_TABLE_SQL)
+
+def migrate_down(manager):
+	manager.execute_script(DROP_TABLE_SQL)
+
+
+CREATE_TABLE_SQL = """
+-- test iteration attributes (key value pairs at an iteration level)
+CREATE TABLE iteration_attributes (
+test_idx int(10) unsigned NOT NULL,	-- ref to test table
+FOREIGN KEY (test_idx) REFERENCES tests(test_idx) ON DELETE CASCADE,
+iteration INTEGER,			-- integer
+attribute VARCHAR(30),			-- attribute name (e.g. 'run_id')
+value VARCHAR(100),			-- attribute value
+KEY `test_idx` (`test_idx`)
+) TYPE=InnoDB;
+"""
+
+DROP_TABLE_SQL = """
+DROP TABLE iteration_attributes;
+"""
diff --git a/tko/models.py b/tko/models.py
index d6c1c1e..cc9d5ab 100644
--- a/tko/models.py
+++ b/tko/models.py
@@ -1,4 +1,7 @@
-import md5
+import os, md5
+
+from autotest_lib.client.common_lib import utils
+from autotest_lib.tko import utils as tko_utils
 
 
 class job(object):
@@ -44,6 +47,51 @@
 		self.attributes = attributes
 
 
+	@staticmethod
+	def load_iterations(keyval_path):
+		"""Abstract method to load a list of iterations from a keyval
+		file."""
+		raise NotImplemented
+
+
+	@classmethod
+	def parse_test(cls, job, subdir, testname, status, reason, test_kernel,
+		       started_time, finished_time):
+		"""Given a job and the basic metadata about the test that
+		can be extracted from the status logs, parse the test
+		keyval files and use it to construct a complete test
+		instance."""
+		tko_utils.dprint("parsing test %s %s" % (subdir, testname))
+
+		if subdir:
+			# grab iterations from the results keyval
+			iteration_keyval = os.path.join(job.dir, subdir,
+							"results", "keyval")
+			iterations = cls.load_iterations(iteration_keyval)
+			iterations = iteration.load_from_keyval(
+			    iteration_keyval)
+
+			# grab test attributes from the subdir keyval
+			test_keyval = os.path.join(job.dir, subdir, "keyval")
+			attributes = test.load_attributes(test_keyval)
+		else:
+			iterations = []
+			attributes = {}
+
+		return cls(subdir, testname, status, reason, test_kernel,
+			   job.machine, started_time, finished_time,
+			   iterations, attributes)
+
+
+	@staticmethod
+	def load_attributes(keyval_path):
+		"""Load the test attributes into a dictionary from a test
+		keyval path. Does not assume that the path actually exists."""
+		if not os.path.exists(keyval_path):
+			return {}
+		return utils.read_keyval(keyval_path)
+
+
 class patch(object):
 	def __init__(self, spec, reference, hash):
 		self.spec = spec
@@ -52,6 +100,42 @@
 
 
 class iteration(object):
-	def __init__(self, index, keyval):
+	def __init__(self, index, attr_keyval, perf_keyval):
 		self.index = index
-		self.keyval = keyval
+		self.attr_keyval = attr_keyval
+		self.perf_keyval = perf_keyval
+
+
+	@staticmethod
+	def parse_line_into_dicts(line, attr_dict, perf_dict):
+		"""Abstract method to parse a keyval line and insert it into
+		the appropriate dictionary.
+			attr_dict: generic iteration attributes
+			perf_dict: iteration performance results
+		"""
+		raise NotImplemented
+
+
+	@classmethod
+	def load_from_keyval(cls, keyval_path):
+		"""Load a list of iterations from an iteration keyval file.
+		Keyval data from separate iterations is separated by blank
+		lines. Makes use of the parse_line_into_dicts method to
+		actually parse the individual lines."""
+		if not os.path.exists(keyval_path):
+			return []
+
+		iterations = []
+		index = 1
+		attr, perf = {}, {}
+		for line in file(path):
+			line = line.strip()
+			if line:
+				cls.parse_line_into_dicts(line, attr, perf)
+			else:
+				iterations.append(cls(index, attr, perf))
+				index += 1
+				attr, perf = {}, {}
+		if attr or perf:
+			iterations.append(cls(index, attr, perf))
+		return iterations
diff --git a/tko/parsers/version_0.py b/tko/parsers/version_0.py
index d3f4fd9..7f167d3 100644
--- a/tko/parsers/version_0.py
+++ b/tko/parsers/version_0.py
@@ -123,51 +123,27 @@
 
 
 class test(models.test):
-	def __init__(self, job, subdir, testname, status, reason, test_kernel,
-		     started_time, finished_time):
-		tko_utils.dprint("parsing test %s %s" % (subdir, testname))
-
-		if subdir:
-			# grab iterations from the results keyval
-			keyval = os.path.join(job.dir, subdir, "results",
-					      "keyval")
-			iterations = iteration.load_from_keyval(keyval)
-
-			# grab version from the subdir keyval
-			keyval = os.path.join(job.dir, subdir, "keyval")
-			attributes = test.load_attributes(keyval)
-			# for backwards compatibility
-			if "version" in attributes:
-				v = attributes["version"]
-				v = "%s\n" % v
-				attributes["version"] = v
-			else:
-				attributes["version"] = None
+	def __init__(self, subdir, testname, status, reason, test_kernel,
+		     machine, started_time, finished_time, iterations,
+		     attributes):
+		# for backwards compatibility with the original parser
+		# implementation, if there is no test version we need a NULL
+		# value to be used; also, if there is a version it should
+		# be terminated by a newline
+		if "version" in attributes:
+			attributes["version"] += "\n"
 		else:
-			iterations = []
-			attributes = {"version": None}
+			attributes["version"] = None
 
 		super(test, self).__init__(subdir, testname, status, reason,
-					   test_kernel, job.machine,
-					   started_time, finished_time,
-					   iterations, attributes)
+					   test_kernel, machine, started_time,
+					   finished_time, iterations,
+					   attributes)
 
 
 	@staticmethod
-	def load_version(path):
-		if not os.path.exists(path):
-			return None
-		for line in file(path):
-			match = re.search(r"^version=(.*)$", line)
-			if match:
-				return match.group(1)
-
-
-	@staticmethod
-	def load_attributes(path):
-		if not os.path.exists(path):
-			return {}
-		return common_utils.read_keyval(path)
+	def load_iterations(keyval_path):
+		return iteration.load_from_keyval(keyval_path)
 
 
 class patch(models.patch):
@@ -180,30 +156,10 @@
 
 
 class iteration(models.iteration):
-	def __init__(self, index, lines):
-		keyval = dict(line.split("=", 1) for line in lines)
-		super(iteration, self).__init__(index, keyval)
-
-
-	@classmethod
-	def load_from_keyval(cls, path):
-		if not os.path.exists(path):
-			return []
-		# pull any values we can from the keyval file
-		iterations = []
-		index = 1
-		lines = []
-		for line in file(path):
-			line = line.strip()
-			if line:
-				lines.append(line)
-			else:
-				iterations.append(cls(index, lines))
-				index += 1
-				lines = []
-		if lines:
-			iterations.append(cls(index, lines))
-		return iterations
+	@staticmethod
+	def parse_line_into_dicts(line, attr_dict, perf_dict):
+		key, value = line.split("=", 1)
+		perf_dict[key] = value
 
 
 class status_line(object):
@@ -418,10 +374,12 @@
 					 "%s\nSubdir:%s\nTestname:%s\n%s" %
 					 (final_status, line.subdir,
 					  line.testname, line.reason))
-			new_test = test(self.job, line.subdir, line.testname,
-					final_status, line.reason,
-					current_kernel, started_time,
-					finished_time)
+			new_test = test.parse_test(self.job, line.subdir,
+						   line.testname,
+						   final_status, line.reason,
+						   current_kernel,
+						   started_time,
+						   finished_time)
 			started_time = None
 			new_tests.append(new_test)
 
@@ -432,7 +390,8 @@
 			tko_utils.dprint(("Adding: ABORT\nSubdir:----\n"
 					  "Testname:%s\n%s")
 					 % (testname, reason))
-			new_test = test(self.job, None, testname, "ABORT",
-					reason, current_kernel, None, None)
+			new_test = test.parse_test(self.job, None, testname,
+						   "ABORT", reason,
+						   current_kernel, None, None)
 			new_tests.append(new_test)
 		yield new_tests
diff --git a/tko/parsers/version_1.py b/tko/parsers/version_1.py
index d9ad82e..b13e272 100644
--- a/tko/parsers/version_1.py
+++ b/tko/parsers/version_1.py
@@ -1,3 +1,5 @@
+import os, re
+
 from autotest_lib.tko import models, status_lib, utils as tko_utils
 from autotest_lib.tko.parsers import base, version_0
 
@@ -15,6 +17,41 @@
 		super(kernel, self).__init__(base, patches, kernel_hash)
 
 
+class test(models.test):
+	@staticmethod
+	def load_iterations(keyval_path):
+		return iteration.load_from_keyval(keyval_path)
+
+
+class iteration(models.iteration):
+	@staticmethod
+	def parse_line_into_dicts(line, attr_dict, perf_dict):
+		typed_match = re.search("^([^=]*)\{(\w*)\}=(.*)$", line)
+		if typed_match:
+			key, val_type, value = typed_match.groups()
+			if val_type == "attr":
+				attr_dict[key] = value
+			elif val_type == "perf":
+				perf_dict[key] = value
+			else:
+				msg = ("WARNING: line '%s' found in test "
+				       "iteration keyval could not be parsed")
+				msg %= line
+				tko_utils.dprint(msg)
+				return # skip the line
+		else:
+			# old-fashioned untyped match, assume perf
+			untyped_match = re.search("^([^=]*)=(.*)$", line)
+			if not untyped_match:
+				msg = ("WARNING: line '%s' found in test "
+				       "iteration keyval could not be parsed")
+				msg %= line
+				tko_utils.dprint(msg)
+				return # skip this line
+			key, value = untyped_match.groups()
+			perf_dict[key] = value
+
+
 class status_line(version_0.status_line):
 	def is_successful_reboot(self, current_status):
 		# make sure this is a reboot line
@@ -51,9 +88,7 @@
 
 # the default implementations from version 0 will do for now
 job = version_0.job
-test = version_0.test
 patch = version_0.patch
-iteration = version_0.iteration
 
 
 class parser(base.parser):
@@ -165,10 +200,14 @@
 				if line.testname is None:
 					line.testname = "JOB"
 
-				new_test = test(self.job, line.subdir,
-						line.testname, current_status,
-						line.reason, current_kernel,
-						started_time, finished_time)
+				new_test = test.parse_test(self.job,
+							   line.subdir,
+							   line.testname,
+							   current_status,
+							   line.reason,
+							   current_kernel,
+							   started_time,
+							   finished_time)
 				msg = "ADD: %s\nSubdir: %s\nTestname: %s\n%s"
 				msg %= (new_test.status, new_test.subdir,
 					new_test.testname, new_test.reason)