kitchen_run: initial CL

scripts/slave/kitchen_run.py

Index: scripts/slave/kitchen_run.py
diff --git a/scripts/slave/kitchen_run.py b/scripts/slave/kitchen_run.py
new file mode 100644
index 0000000000000000000000000000000000000000..da57e1567806a7d679248e034ec33dcd926afe77
--- /dev/null
+++ b/scripts/slave/kitchen_run.py
@@ -0,0 +1,154 @@
+#!/usr/bin/env python
+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import argparse
+import collections
+import json
+import logging
+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+
+
+# Install Infra build environment.
+BUILD_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(
+                             os.path.abspath(__file__))))
+sys.path.insert(0, os.path.join(BUILD_ROOT, 'scripts'))
+
+from common import chromium_utils
+from common import env
+from slave import infra_platform
+from slave import update_scripts
+
+
+LOGGER = logging.getLogger('kitchen_run')
+
+
+# A CIPD binary description, including the package name and version, and relative

[iannucci, 2016/04/11 18:07:28]

hm... why copy+paste this but not copy+paste update scripts? Maybe factor this
out into a cipd.py?

[dnj, 2016/04/11 18:17:15]

+1 make these first-level cipd.py constructs.

[Paweł Hajdan Jr., 2016/04/11 20:44:39]

Well let me know what you mean by that. Our interface to cipd.py is currently
invoking it as a separate process. How would we pass named tuples?

[dnj, 2016/04/12 21:22:21]

We'd define them in "cipd.py", then "import cipd" here and use it.

"cipd.py" would be both a library and an executable.

+# path of the binary within the package.
+CipdBinary = collections.namedtuple('CipdBinary',
+    ('package', 'version', 'relpath'))
+
+
+CIPD_BINARIES = {
+  ('linux', 64): CipdBinary(
+      'infra/tools/luci/kitchen/linux-amd64', 'latest', 'kitchen'),
+}
+
+
+def _run_command(cmd, **kwargs):
+  if kwargs.pop('dry_run', False):

[dnj, 2016/04/11 18:17:15]

You don't actually use this option.

[Paweł Hajdan Jr., 2016/04/11 20:44:40]

Good point - will remove.

+    LOGGER.info('(Dry Run) Would have executed command: %s', cmd)
+    return 0, ''
+
+  LOGGER.debug('Executing command: %s', cmd)
+  kwargs.setdefault('stderr', subprocess.STDOUT)
+  proc = subprocess.Popen(cmd, **kwargs)
+  stdout, _ = proc.communicate()
+
+  LOGGER.debug('Process [%s] returned [%d] with output:\n%s',
+               cmd, proc.returncode, stdout)
+  return proc.returncode, stdout
+
+
+def _check_command(cmd, **kwargs):
+  rv, stdout = _run_command(cmd, **kwargs)
+  if rv != 0:
+    raise subprocess.CalledProcessError(rv, cmd, output=stdout)
+  return stdout
+
+
+def _install_cipd(path, *packages):
+  """Returns (list): The paths to the binaries in each of the packages.
+
+  This method bootstraps CIPD in "path", installing the packages specified
+  by "packages" and returning the paths to their binaries.
+
+  Args:
+    path (str): The CIPD installation root.
+    packages (CipdBinary): The set of CIPD binary packages to install.
+  """
+  verbosity = 0
+  level = logging.getLogger().level
+  if level <= logging.INFO:

[nodir, 2016/04/12 01:04:55]

I'd use simpler == and `verbose = 1` and `verbose = 2`
it would be more explicit and same amount of code

+    verbosity += 1
+  if level <= logging.DEBUG:
+    verbosity += 1
+
+  packages_path = os.path.join(path, 'packages.json')
+  pmap = {}
+  cmd = [
+      sys.executable,
+      os.path.join(env.Build, 'scripts', 'slave', 'cipd.py'),
+      '--dest-directory', path,
+      '--json-output', packages_path,
+  ] + (['--verbose'] * verbosity)
+  for p in packages:
+    cmd += ['-P', '%s@%s' % (p.package, p.version)]
+    pmap[p.package] = os.path.join(path, p.relpath)
+
+  try:
+    _check_command(cmd)
+  except subprocess.CalledProcessError:
+    LOGGER.exception('Failed to install LogDog CIPD packages.')

[dnj, 2016/04/11 18:17:15]

nit: "Kitchen".

[Paweł Hajdan Jr., 2016/04/11 20:44:40]

Will do.

+    raise LogDogBootstrapError()

[iannucci, 2016/04/11 18:07:28]

especially since this is clearly raising copy/pasted exceptions.

+
+  # Resolve installed packages.
+  return tuple(pmap[p.package] for p in packages)

[nodir, 2016/04/12 01:04:55]

You could compute path here, no need for pmap var

+
+
+def _bootstrap_kitchen(basedir):
+  cipd_path = os.path.join(basedir, '.kitchen_cipd')
+  #(kitchen,) = _install_cipd(cipd_path, CIPD_BINARIES[infra_platform.get()])
+  return '/usr/local/google/home/phajdan/luci-go/kitchen'

[iannucci, 2016/04/11 18:07:28]

erm... what?

[Paweł Hajdan Jr., 2016/04/11 20:44:39]

See "As you see, it has some local hacks I've used for testing. They'll be
removed before final review and landing." in original message. This was needed
before https://codereview.chromium.org/1868553006 and
https://codereview.chromium.org/1862163004 landed.

+  return kitchen
+
+
+def main(argv):
+  print argv
+  parser = argparse.ArgumentParser()
+  parser.add_argument('--repository')
+  parser.add_argument('--revision')
+  parser.add_argument('--recipe')
+  parser.add_argument('--build-properties-gz', dest='build_properties',
+      type=chromium_utils.convert_gz_json_type, default={},
+      help='build properties in b64 gz JSON format')
+  args = parser.parse_args(argv)
+
+  basedir = os.getcwd()
+  kitchen = _bootstrap_kitchen(basedir)
+
+  rv = 1
+
+  tempdir = tempfile.mkdtemp(prefix='kitchen_run')

[dnj, 2016/04/11 18:17:15]

I would like you to use a scheme like I did with LogDog. You can borrow it from
Runtime.tempdir. This should offer cleanup without accidentally having one run
taint another.

[Paweł Hajdan Jr., 2016/04/11 20:44:39]

Could you explain more? How would one run taint another? Even though prefix is
the same, each run gets a different temporary directory.

+  try:
+    rv, _ = _run_command([
+        kitchen, 'cook',
+        '-repository', args.repository,
+        '-revision', args.revision,
+        '-recipe', args.recipe,
+        '-properties', json.dumps(args.build_properties),

[dnj, 2016/04/11 18:17:15]

Can we have this get passed to Kitchen as JSON blob? Windows bots have been
known to hit command-line length limitations.

[Paweł Hajdan Jr., 2016/04/11 20:44:39]

Good point. Could you explain more? Pass path to a file containing JSON instead?
Pass compressed JSON like we do for build properties?

[dnj, 2016/04/12 21:22:21]

Yep.

1) Create JSON blob as an array (or object w/ "args" array field, whatever).
2) Write blob to disk.
3) Run "kitchen", passing it that file.

The important difference here, and the reason we zipped properties above, is b/c
it did exceed the command-line length on Windows. You're decompressing them,
then passing the uncompressed data to kitchen, which revives the problem.

+    ], cwd=tempdir)

[dnj, 2016/04/11 18:17:15]

Do we actually want to run Kitchen out of a temporary directory?

[Paweł Hajdan Jr., 2016/04/11 20:44:39]

I think so. That's what we do with swarmbucket. Why not?

[dnj, 2016/04/12 21:22:21]

CWD means magic things for some processes. No specific reason, as I'm not sure
what assumptions Kitchen or its subprocesses make about working directory. Just
a note.

+  finally:
+    shutil.rmtree(tempdir)

[iannucci, 2016/04/11 18:07:28]

finally clauses like this are unreliable. tempfolder cleanup must happen at the
beginning of this script (to clean up previous crashes). Doing it in finally
will add another source of out-of-disk errors.

It may make sense to build this folder management into the kitchen binary
instead? I'm not sure.

[Paweł Hajdan Jr., 2016/04/11 20:44:39]

Good point. I'll find some way to implement the cleanup.

+
+  return rv
+
+
+def shell_main(argv):
+  # TODO(phajdan.jr): re-enable update_scripts.
+  if False and update_scripts.update_scripts():

[dnj, 2016/04/11 18:17:15]

Want to remove "False", no?

[Paweł Hajdan Jr., 2016/04/11 20:44:40]

"As you see, it has some local hacks I've used for testing. They'll be removed
before final review and landing."

[nodir, 2016/04/12 01:04:55]

I guess it would be easier for reviewers (4) if the uploaded patchset does not
include local hacks

[dnj, 2016/04/12 21:22:21]

Understood that you have local hacks, but when you don't note them it's
difficult to identify which are hacks and which are oversights. Also, it's good
to comment regardless in case you forget to remove a specific hack.

+    # Re-execute with the updated kitchen_run.py.
+    rv, _ = _run_command([sys.executable] + argv)
+    return rv
+  else:
+    return main(argv[1:])
+
+
+if __name__ == '__main__':
+  logging.basicConfig(level=logging.INFO)
+  sys.exit(shell_main(sys.argv))