[infra] Support recursive configs in builder_name_schema

This fixes problems with Upload- tasks which just prefix other types of
task names.

Bug: skia:
Change-Id: Icdbcfc5a889e821c6923f635eae0744c3cb0133c
Reviewed-on: https://skia-review.googlesource.com/121786
Commit-Queue: Eric Boren <borenet@google.com>
Reviewed-by: Ravi Mistry <rmistry@google.com>

7 files changed, 336 insertions, 144 deletions

diff --git a/infra/bots/gen_tasks.go b/infra/bots/gen_tasks.go
index 4191ff4643..b3c6c48e27 100644
--- a/infra/bots/gen_tasks.go
+++ b/infra/bots/gen_tasks.go
@@ -1374,11 +1374,18 @@ func main() {
 // TODO(borenet): The below really belongs in its own file, probably next to the
 // builder_name_schema.json file.
 
+// schema is a sub-struct of JobNameSchema.
+type schema struct {
+	Keys         []string `json:"keys"`
+	OptionalKeys []string `json:"optional_keys"`
+	RecurseRoles []string `json:"recurse_roles"`
+}
+
 // JobNameSchema is a struct used for (de)constructing Job names in a
 // predictable format.
 type JobNameSchema struct {
-	Schema map[string][]string `json:"builder_name_schema"`
-	Sep    string              `json:"builder_name_sep"`
+	Schema map[string]*schema `json:"builder_name_schema"`
+	Sep    string             `json:"builder_name_sep"`
 }
 
 // NewJobNameSchema returns a JobNameSchema instance based on the given JSON
@@ -1399,56 +1406,143 @@ func NewJobNameSchema(jsonFile string) (*JobNameSchema, error) {
 // ParseJobName splits the given Job name into its component parts, according
 // to the schema.
 func (s *JobNameSchema) ParseJobName(n string) (map[string]string, error) {
+	popFront := func(items []string) (string, []string, error) {
+		if len(items) == 0 {
+			return "", nil, fmt.Errorf("Invalid job name: %s (not enough parts)", n)
+		}
+		return items[0], items[1:], nil
+	}
+
+	result := map[string]string{}
+
+	var parse func(int, string, []string) ([]string, error)
+	parse = func(depth int, role string, parts []string) ([]string, error) {
+		s, ok := s.Schema[role]
+		if !ok {
+			return nil, fmt.Errorf("Invalid job name; %q is not a valid role.", role)
+		}
+		if depth == 0 {
+			result["role"] = role
+		} else {
+			result[fmt.Sprintf("sub-role-%d", depth)] = role
+		}
+		var err error
+		for _, key := range s.Keys {
+			var value string
+			value, parts, err = popFront(parts)
+			if err != nil {
+				return nil, err
+			}
+			result[key] = value
+		}
+		for _, subRole := range s.RecurseRoles {
+			if len(parts) > 0 && parts[0] == subRole {
+				parts, err = parse(depth+1, parts[0], parts[1:])
+				if err != nil {
+					return nil, err
+				}
+			}
+		}
+		for _, key := range s.OptionalKeys {
+			if len(parts) > 0 {
+				var value string
+				value, parts, err = popFront(parts)
+				if err != nil {
+					return nil, err
+				}
+				result[key] = value
+			}
+		}
+		if len(parts) > 0 {
+			return nil, fmt.Errorf("Invalid job name: %s (too many parts)", n)
+		}
+		return parts, nil
+	}
+
 	split := strings.Split(n, s.Sep)
 	if len(split) < 2 {
-		return nil, fmt.Errorf("Invalid job name: %q", n)
+		return nil, fmt.Errorf("Invalid job name: %s (not enough parts)", n)
 	}
 	role := split[0]
 	split = split[1:]
-	keys, ok := s.Schema[role]
-	if !ok {
-		return nil, fmt.Errorf("Invalid job name; %q is not a valid role.", role)
-	}
-	extraConfig := ""
-	if len(split) == len(keys)+1 {
-		extraConfig = split[len(split)-1]
-		split = split[:len(split)-1]
-	}
-	if len(split) != len(keys) {
-		return nil, fmt.Errorf("Invalid job name; %q has incorrect number of parts.", n)
-	}
-	rv := make(map[string]string, len(keys)+2)
-	rv["role"] = role
-	if extraConfig != "" {
-		rv["extra_config"] = extraConfig
-	}
-	for i, k := range keys {
-		rv[k] = split[i]
-	}
-	return rv, nil
+	_, err := parse(0, role, split)
+	return result, err
 }
 
 // MakeJobName assembles the given parts of a Job name, according to the schema.
 func (s *JobNameSchema) MakeJobName(parts map[string]string) (string, error) {
-	role, ok := parts["role"]
-	if !ok {
-		return "", fmt.Errorf("Invalid job parts; jobs must have a role.")
-	}
-	keys, ok := s.Schema[role]
-	if !ok {
-		return "", fmt.Errorf("Invalid job parts; unknown role %q", role)
-	}
 	rvParts := make([]string, 0, len(parts))
-	rvParts = append(rvParts, role)
-	for _, k := range keys {
-		v, ok := parts[k]
+
+	var process func(int, map[string]string) (map[string]string, error)
+	process = func(depth int, parts map[string]string) (map[string]string, error) {
+		roleKey := "role"
+		if depth != 0 {
+			roleKey = fmt.Sprintf("sub-role-%d", depth)
+		}
+		role, ok := parts[roleKey]
 		if !ok {
-			return "", fmt.Errorf("Invalid job parts; missing %q", k)
+			return nil, fmt.Errorf("Invalid job parts; missing key %q", roleKey)
+		}
+
+		s, ok := s.Schema[role]
+		if !ok {
+			return nil, fmt.Errorf("Invalid job parts; unknown role %q", role)
+		}
+		rvParts = append(rvParts, role)
+		delete(parts, roleKey)
+
+		for _, key := range s.Keys {
+			value, ok := parts[key]
+			if !ok {
+				return nil, fmt.Errorf("Invalid job parts; missing %q", key)
+			}
+			rvParts = append(rvParts, value)
+			delete(parts, key)
 		}
-		rvParts = append(rvParts, v)
+
+		if len(s.RecurseRoles) > 0 {
+			subRoleKey := fmt.Sprintf("sub-role-%d", depth+1)
+			subRole, ok := parts[subRoleKey]
+			if !ok {
+				return nil, fmt.Errorf("Invalid job parts; missing %q", subRoleKey)
+			}
+			rvParts = append(rvParts, subRole)
+			delete(parts, subRoleKey)
+			found := false
+			for _, recurseRole := range s.RecurseRoles {
+				if recurseRole == subRole {
+					found = true
+					var err error
+					parts, err = process(depth+1, parts)
+					if err != nil {
+						return nil, err
+					}
+					break
+				}
+			}
+			if !found {
+				return nil, fmt.Errorf("Invalid job parts; unknown sub-role %q", subRole)
+			}
+		}
+		for _, key := range s.OptionalKeys {
+			if value, ok := parts[key]; ok {
+				rvParts = append(rvParts, value)
+				delete(parts, key)
+			}
+		}
+		if len(parts) > 0 {
+			return nil, fmt.Errorf("Invalid job parts: too many parts: %v", parts)
+		}
+		return parts, nil
+	}
+
+	// Copy the parts map, so that we can modify at will.
+	partsCpy := make(map[string]string, len(parts))
+	for k, v := range parts {
+		partsCpy[k] = v
 	}
-	if _, ok := parts["extra_config"]; ok {
-		rvParts = append(rvParts, parts["extra_config"])
+	if _, err := process(0, partsCpy); err != nil {
+		return "", err
 	}
 	return strings.Join(rvParts, s.Sep), nil
 }
diff --git a/infra/bots/recipe_modules/builder_name_schema/api.py b/infra/bots/recipe_modules/builder_name_schema/api.py
index be27a666b0..8686111c84 100644
--- a/infra/bots/recipe_modules/builder_name_schema/api.py
+++ b/infra/bots/recipe_modules/builder_name_schema/api.py
@@ -19,7 +19,6 @@ class BuilderNameSchemaApi(recipe_api.RecipeApi):
     self.BUILDER_NAME_SCHEMA = builder_name_schema.BUILDER_NAME_SCHEMA
     self.BUILDER_NAME_SEP = builder_name_schema.BUILDER_NAME_SEP
 
-    self.BUILDER_ROLE_CANARY = builder_name_schema.BUILDER_ROLE_CANARY
     self.BUILDER_ROLE_BUILD = builder_name_schema.BUILDER_ROLE_BUILD
     self.BUILDER_ROLE_HOUSEKEEPER = builder_name_schema.BUILDER_ROLE_HOUSEKEEPER
     self.BUILDER_ROLE_INFRA = builder_name_schema.BUILDER_ROLE_INFRA
@@ -28,8 +27,8 @@ class BuilderNameSchemaApi(recipe_api.RecipeApi):
     self.BUILDER_ROLE_CALMBENCH = builder_name_schema.BUILDER_ROLE_CALMBENCH
     self.BUILDER_ROLES = builder_name_schema.BUILDER_ROLES
 
-  def MakeBuilderName(self, *args, **kwargs):
-    return builder_name_schema.MakeBuilderName(*args, **kwargs)
+  def MakeBuilderName(self, **kwargs):
+    return builder_name_schema.MakeBuilderName(**kwargs)
 
   def DictForBuilderName(self, *args, **kwargs):
     return builder_name_schema.DictForBuilderName(*args, **kwargs)
diff --git a/infra/bots/recipe_modules/builder_name_schema/builder_name_schema.json b/infra/bots/recipe_modules/builder_name_schema/builder_name_schema.json
index 66bd3cbe4d..d8ea7dc33e 100644
--- a/infra/bots/recipe_modules/builder_name_schema/builder_name_schema.json
+++ b/infra/bots/recipe_modules/builder_name_schema/builder_name_schema.json
@@ -1,65 +1,85 @@
 {
   "builder_name_schema": {
-    "Test": [
-      "os",
-      "compiler",
-      "model",
-      "cpu_or_gpu",
-      "cpu_or_gpu_value",
-      "arch",
-      "configuration",
-      "test_filter"
-    ],
-    "Housekeeper": [
-      "frequency"
-    ],
-    "Infra": [
-      "frequency"
-    ],
-    "Build": [
-      "os",
-      "compiler",
-      "target_arch",
-      "configuration"
-    ],
-    "Perf": [
-      "os",
-      "compiler",
-      "model",
-      "cpu_or_gpu",
-      "cpu_or_gpu_value",
-      "arch",
-      "configuration",
-      "test_filter"
-    ],
-    "Canary": [
-      "project",
-      "os",
-      "compiler",
-      "target_arch",
-      "configuration"
-    ],
-    "Upload": [
-      "orig_role",
-      "os",
-      "compiler",
-      "model",
-      "cpu_or_gpu",
-      "cpu_or_gpu_value",
-      "arch",
-      "configuration",
-      "test_filter"
-    ],
-    "Calmbench": [
-      "os",
-      "compiler",
-      "model",
-      "cpu_or_gpu",
-      "cpu_or_gpu_value",
-      "arch",
-      "configuration",
-      "test_filter"
-    ]
+    "Build": {
+      "keys": [
+        "os",
+        "compiler",
+        "target_arch",
+        "configuration"
+      ],
+      "optional_keys": [
+        "extra_config"
+      ]
+    },
+    "Calmbench": {
+      "keys": [
+        "os",
+        "compiler",
+        "model",
+        "cpu_or_gpu",
+        "cpu_or_gpu_value",
+        "arch",
+        "configuration",
+        "test_filter"
+      ],
+      "optional_keys": [
+        "extra_config"
+      ]
+    },
+    "Housekeeper": {
+      "keys": [
+        "frequency"
+      ],
+      "optional_keys": [
+        "extra_config"
+      ]
+    },
+    "Infra": {
+      "keys": [
+        "frequency"
+      ],
+      "optional_keys": [
+        "extra_config"
+      ]
+    },
+    "Perf": {
+      "keys": [
+        "os",
+        "compiler",
+        "model",
+        "cpu_or_gpu",
+        "cpu_or_gpu_value",
+        "arch",
+        "configuration",
+        "test_filter"
+      ],
+      "optional_keys": [
+        "extra_config"
+      ]
+    },
+    "Test": {
+      "keys": [
+        "os",
+        "compiler",
+        "model",
+        "cpu_or_gpu",
+        "cpu_or_gpu_value",
+        "arch",
+        "configuration",
+        "test_filter"
+      ],
+      "optional_keys": [
+        "extra_config"
+      ]
+    },
+    "Upload": {
+      "recurse_roles": [
+        "Build",
+        "Calmbench",
+        "Perf",
+        "Test"
+      ]
+    }
   },
   "builder_name_sep": "-"
 }
diff --git a/infra/bots/recipe_modules/builder_name_schema/builder_name_schema.py b/infra/bots/recipe_modules/builder_name_schema/builder_name_schema.py
index bdd93ff0e6..cf605d03ee 100644
--- a/infra/bots/recipe_modules/builder_name_schema/builder_name_schema.py
+++ b/infra/bots/recipe_modules/builder_name_schema/builder_name_schema.py
@@ -20,7 +20,6 @@ BUILDER_NAME_SCHEMA = None
 BUILDER_NAME_SEP = None
 
 # Builder roles.
-BUILDER_ROLE_CANARY = 'Canary'
 BUILDER_ROLE_BUILD = 'Build'
 BUILDER_ROLE_HOUSEKEEPER = 'Housekeeper'
 BUILDER_ROLE_INFRA = 'Infra'
@@ -28,8 +27,7 @@ BUILDER_ROLE_PERF = 'Perf'
 BUILDER_ROLE_TEST = 'Test'
 BUILDER_ROLE_UPLOAD = 'Upload'
 BUILDER_ROLE_CALMBENCH = "Calmbench"
-BUILDER_ROLES = (BUILDER_ROLE_CANARY,
-                 BUILDER_ROLE_BUILD,
+BUILDER_ROLES = (BUILDER_ROLE_BUILD,
                  BUILDER_ROLE_HOUSEKEEPER,
                  BUILDER_ROLE_INFRA,
                  BUILDER_ROLE_PERF,
@@ -74,47 +72,99 @@ def _LoadSchema():
 _LoadSchema()
 
 
-def MakeBuilderName(role, extra_config=None, **kwargs):
-  schema = BUILDER_NAME_SCHEMA.get(role)
-  if not schema:
-    raise ValueError('%s is not a recognized role.' % role)
-  for k, v in kwargs.iteritems():
+def MakeBuilderName(**parts):
+  for v in parts.itervalues():
     if BUILDER_NAME_SEP in v:
-      raise ValueError('%s not allowed in %s.' % (BUILDER_NAME_SEP, v))
-    if not k in schema:
-      raise ValueError('Schema does not contain "%s": %s' %(k, schema))
-  if extra_config and BUILDER_NAME_SEP in extra_config:
-    raise ValueError('%s not allowed in %s.' % (BUILDER_NAME_SEP,
-                                                extra_config))
-  name_parts = [role]
-  name_parts.extend([kwargs[attribute] for attribute in schema])
-  if extra_config:
-    name_parts.append(extra_config)
-  return BUILDER_NAME_SEP.join(name_parts)
+      raise ValueError('Parts cannot contain "%s"' % BUILDER_NAME_SEP)
+
+  rv_parts = []
+
+  def process(depth, parts):
+    role_key = 'role'
+    if depth != 0:
+      role_key = 'sub-role-%d' % depth
+    role = parts.get(role_key)
+    if not role:
+      raise ValueError('Invalid parts; missing key %s' % role_key)
+    s = BUILDER_NAME_SCHEMA.get(role)
+    if not s:
+      raise ValueError('Invalid parts; unknown role %s' % role)
+    rv_parts.append(role)
+    del parts[role_key]
+
+    for key in s.get('keys', []):
+      value = parts.get(key)
+      if not value:
+        raise ValueError('Invalid parts; missing %s' % key)
+      rv_parts.append(value)
+      del parts[key]
+
+    recurse_roles = s.get('recurse_roles', [])
+    if len(recurse_roles) > 0:
+      sub_role_key = 'sub-role-%d' % (depth+1)
+      sub_role = parts.get(sub_role_key)
+      if not sub_role:
+        raise ValueError('Invalid parts; missing %s' % sub_role_key)
+
+      found = False
+      for recurse_role in recurse_roles:
+        if recurse_role == sub_role:
+          found = True
+          parts = process(depth+1, parts)
+          break
+      if not found:
+        raise ValueError('Invalid parts; unknown sub-role %s' % sub_role)
+
+    for key in s.get('optional_keys', []):
+      if parts.get(key):
+        rv_parts.append(parts[key])
+        del parts[key]
+
+    if len(parts) > 0:
+      raise ValueError('Invalid parts; too many parts: %s' % parts)
+
+    return parts
+
+  process(0, parts)
+
+  return BUILDER_NAME_SEP.join(rv_parts)
 
 
 def DictForBuilderName(builder_name):
   """Makes a dictionary containing details about the builder from its name."""
-  split_name = builder_name.split(BUILDER_NAME_SEP)
+  split = builder_name.split(BUILDER_NAME_SEP)
 
-  def pop_front():
+  def pop_front(items):
     try:
-      return split_name.pop(0)
+      return items.pop(0), items
     except:
-      raise ValueError('Invalid builder name: %s' % builder_name)
+      raise ValueError(
+          'Invalid builder name: %s (not enough parts)' % builder_name)
 
   result = {}
-  if split_name[0] in BUILDER_NAME_SCHEMA.keys():
-    key_list = BUILDER_NAME_SCHEMA[split_name[0]]
-    result['role'] = pop_front()
-    for key in key_list:
-      result[key] = pop_front()
-    if split_name:
-      result['extra_config'] = pop_front()
-    if split_name:
+
+  def _parse(depth, role, parts):
+    schema = BUILDER_NAME_SCHEMA.get(role)
+    if not schema:
       raise ValueError('Invalid builder name: %s' % builder_name)
-  else:
-    raise ValueError('Invalid builder name: %s' % builder_name)
-  return result
+    if depth == 0:
+      result['role'] = role
+    else:
+      result['sub-role-%d' % depth] = role
+    for key in schema.get('keys', []):
+      value, parts = pop_front(parts)
+      result[key] = value
+    for sub_role in schema.get('recurse_roles', []):
+      if len(parts) > 0 and sub_role == parts[0]:
+        parts = _parse(depth+1, parts[0], parts[1:])
+    for key in schema.get('optional_keys', []):
+      if parts:
+        value, parts = pop_front(parts)
+        result[key] = value
+    if parts:
+      raise ValueError('Invalid builder name: %s' % builder_name)
+    return parts
 
+  _parse(0, split[0], split[1:])
 
+  return result
diff --git a/infra/bots/recipe_modules/builder_name_schema/examples/full.py b/infra/bots/recipe_modules/builder_name_schema/examples/full.py
index 5e5f77ffcb..f8eade8bcb 100644
--- a/infra/bots/recipe_modules/builder_name_schema/examples/full.py
+++ b/infra/bots/recipe_modules/builder_name_schema/examples/full.py
@@ -9,20 +9,23 @@ DEPS = [
 
 
 def RunSteps(api):
-  name = 'Build-Debian9-Clang-x64-Release-Android'
-  d = api.builder_name_schema.DictForBuilderName(name)
-  got = api.builder_name_schema.MakeBuilderName(**d)
-  assert got == name
+  names = [
+    'Build-Debian9-Clang-x64-Release-Android',
+    'Upload-Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-Shard_12-Coverage',
+  ]
+  for name in names:
+    d = api.builder_name_schema.DictForBuilderName(name)
+    got = api.builder_name_schema.MakeBuilderName(**d)
+    assert got == name
 
   # Failures.
   try:
-    api.builder_name_schema.MakeBuilderName('nope')
+    api.builder_name_schema.MakeBuilderName(role='nope')
   except ValueError:
     pass
 
   try:
-    api.builder_name_schema.MakeBuilderName(
-        role='Build', os='a%sb' % api.builder_name_schema.BUILDER_NAME_SEP)
+    api.builder_name_schema.MakeBuilderName(compiler='Build', os='ab')
   except ValueError:
     pass
 
@@ -59,6 +62,32 @@ def RunSteps(api):
   except ValueError:
     pass
 
+  try:
+    api.builder_name_schema.MakeBuilderName(role='Upload')
+  except ValueError:
+    pass
+
+  try:
+    m = {
+      'role': 'Upload',
+      'sub-role-1': 'fake',
+    }
+    api.builder_name_schema.MakeBuilderName(**m)
+  except ValueError:
+    pass
+
+  try:
+    api.builder_name_schema.MakeBuilderName(
+        role='Build',
+        os='Debian9',
+        compiler='Clang',
+        target_arch='x64',
+        configuration='Release',
+        extra_config='Android',
+        extra_extra_config='Bogus',
+    )
+  except ValueError:
+    pass
 
 def GenTests(api):
   yield api.test('test')
diff --git a/infra/bots/recipe_modules/vars/examples/full.expected/Upload-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-Coverage-All.json b/infra/bots/recipe_modules/vars/examples/full.expected/Upload-Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-Coverage.json
index 4594f9e6b9..4594f9e6b9 100644
--- a/infra/bots/recipe_modules/vars/examples/full.expected/Upload-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-Coverage-All.json
+++ b/infra/bots/recipe_modules/vars/examples/full.expected/Upload-Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-Coverage.json
diff --git a/infra/bots/recipe_modules/vars/examples/full.py b/infra/bots/recipe_modules/vars/examples/full.py
index 0254b05d1a..5fca9df48d 100644
--- a/infra/bots/recipe_modules/vars/examples/full.py
+++ b/infra/bots/recipe_modules/vars/examples/full.py
@@ -33,7 +33,7 @@ TEST_BUILDERS = [
   'Perf-Chromecast-GCC-Chorizo-CPU-Cortex_A7-arm-Debug-All',
   'Perf-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-ASAN',
   'Perf-Ubuntu14-GCC-GCE-CPU-AVX2-x86_64-Release-All-CT_BENCH_1k_SKPs',
-  'Upload-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-Coverage-All',
+  'Upload-Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-Coverage',
   'Calmbench-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All',
   'Calmbench-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Release-All'
 ]