Merge ~ltrager/maas:pass_script_parameters into maas:master

[Lee Trager]

Pass parameters to scripts.

Paramaters are now passed to commissioning and testing scripts. If the
parameter is a storage type maas_run_remote_scripts will attempt to lookup
the storage device by model and serial to get the block device path.

[Данило Шеган (danilo)]

Looks good, a few minor nits inline (mostly typos and test improvement suggestions), so approving.

review: Approve

[Данило Шеган (danilo)]

You've also got a typo in your commit message ("ParamAters" :)

[Lee Trager (ltrager)]

Thanks for the review and catching those late night spelling errors ;) All corrections have been made.

[Andres Rodriguez (andreserl)]

This branch has been reverted as it broke testing in MAAS. http://<maas-ci>:8080/job/maas-xenial-master-git/142/ . I've re-opened this MP so that when it gets fixed it can be re-loanded.

diff --git a/src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py b/src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py
index b0237a5..d938174 100644
--- a/src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py
+++ b/src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py
@@ -26,7 +26,9 @@ from io import BytesIO
 import json
 import os
 import re
+import shlex
 from subprocess import (
+    check_output,
     PIPE,
     Popen,
     TimeoutExpired,
@@ -210,6 +212,58 @@ def install_dependencies(
     return True
 
 
+# Cache the block devices so we only have to query once.
+_block_devices = None
+
+
+def get_block_devices():
+    """If needed, query lsblk for all known block devices and store."""
+    global _block_devices
+    if _block_devices is None:
+        _block_devices = []
+        block_list = check_output([
+            'lsblk', '--exclude', '1,2,7', '-d', '-P', '-o',
+            'NAME,MODEL,SERIAL']).decode('utf-8')
+        for blockdev in block_list.splitlines():
+            tokens = shlex.split(blockdev)
+            current_block = {}
+            for token in tokens:
+                k, v = token.split("=", 1)
+                current_block[k] = v.strip()
+            _block_devices.append(current_block)
+
+    return _block_devices
+
+
+def parse_parameters(script, scripts_dir):
+    """Return a list containg script path and parameters to be passed to it."""
+    ret = [os.path.join(scripts_dir, script['path'])]
+    for param in script.get('parameters', {}).values():
+        param_type = param.get('type')
+        if param_type == 'runtime':
+            argument_format = param.get('argument_format', '--runtime={input}')
+            ret += argument_format.format(input=param['value']).split()
+        elif param_type == 'storage':
+            value = param['value']
+            if not (value.get('model') and value.get('serial')):
+                # If no model or serial were included trust that id_path
+                # is correct. This is needed for VirtIO devices.
+                value['path'] = value['input'] = value['id_path']
+            else:
+                # Map the current path of the device to what it currently is
+                # for the device model and serial. This is needed as the
+                # the device name may have changed since commissioning.
+                for blockdev in get_block_devices():
+                    if (value['model'] == blockdev['MODEL'] and
+                            value['serial'] == blockdev['SERIAL']):
+                        value['path'] = value['input'] = "/dev/%s" % blockdev[
+                            'NAME']
+            argument_format = param.get(
+                'argument_format', '--storage={path}')
+            ret += argument_format.format(**value).split()
+    return ret
+
+
 def run_scripts(url, creds, scripts_dir, out_dir, scripts):
     """Run and report results for the given scripts."""
     total_scripts = len(scripts)
@@ -227,15 +281,23 @@ def run_scripts(url, creds, scripts_dir, out_dir, scripts):
         if script_version_id is not None:
             args['script_version_id'] = script_version_id
         timeout_seconds = script.get('timeout_seconds')
-
-        script_path = os.path.join(scripts_dir, script['path'])
-        combined_path = os.path.join(out_dir, script['name'])
+        for param in script.get('parameters', {}).values():
+            if param.get('type') == 'runtime':
+                timeout_seconds = param['value']
+                break
+
+        # Create a seperate output directory for each script being run as
+        # multiple scripts with the same name may be run.
+        script_out_dir = os.path.join(out_dir, '%s.%s' % (
+            script['name'], script['script_result_id']))
+        os.makedirs(script_out_dir, exist_ok=True)
+        combined_path = os.path.join(script_out_dir, script['name'])
         stdout_name = '%s.out' % script['name']
-        stdout_path = os.path.join(out_dir, stdout_name)
+        stdout_path = os.path.join(script_out_dir, stdout_name)
         stderr_name = '%s.err' % script['name']
-        stderr_path = os.path.join(out_dir, stderr_name)
+        stderr_path = os.path.join(script_out_dir, stderr_name)
         result_name = '%s.yaml' % script['name']
-        result_path = os.path.join(out_dir, result_name)
+        result_path = os.path.join(script_out_dir, result_name)
         download_path = os.path.join(scripts_dir, 'downloads', script['name'])
 
         if not install_dependencies(
@@ -257,6 +319,21 @@ def run_scripts(url, creds, scripts_dir, out_dir, scripts):
         env['DOWNLOAD_PATH'] = download_path
 
         try:
+            script_arguments = parse_parameters(script, scripts_dir)
+        except KeyError:
+            # 2 is the return code bash gives when it can't execute.
+            args['exit_status'] = 2
+            args['files'] = {
+                script['name']: b'Unable to map parameters',
+                stderr_name: b'Unable to map parameters',
+            }
+            signal_wrapper(
+                error='Failed to execute %s [%d/%d]: %d' % (
+                    script['name'], i, total_scripts, args['exit_status']),
+                **args)
+            continue
+
+        try:
             # This script sets its own niceness value to the highest(-20) below
             # to help ensure the heartbeat keeps running. When launching the
             # script we need to lower the nice value as a child process
@@ -266,7 +343,7 @@ def run_scripts(url, creds, scripts_dir, out_dir, scripts):
             # to the provided value. Since the runner uses a nice value of -20
             # setting it to 40 gives the actual nice value of 20.
             proc = Popen(
-                script_path, stdout=PIPE, stderr=PIPE, env=env,
+                script_arguments, stdout=PIPE, stderr=PIPE, env=env,
                 preexec_fn=lambda: os.nice(40))
             capture_script_output(
                 proc, combined_path, stdout_path, stderr_path, timeout_seconds)
diff --git a/src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py b/src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py
index 0e25501..90c858a 100644
--- a/src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py
+++ b/src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py
@@ -28,7 +28,9 @@ from maastesting.testcase import MAASTestCase
 from snippets import maas_run_remote_scripts
 from snippets.maas_run_remote_scripts import (
     download_and_extract_tar,
+    get_block_devices,
     install_dependencies,
+    parse_parameters,
     run_and_check,
     run_scripts,
     run_scripts_from_metadata,
@@ -52,19 +54,23 @@ class TestMaasRunRemoteScripts(MAASTestCase):
 
     def make_script_output(self, scripts, scripts_dir, with_result=False):
         for script in scripts:
+            script_out_dir = os.path.join(scripts_dir, '%s.%s' % (
+                script['name'], script['script_result_id']))
+            os.makedirs(script_out_dir)
             output = factory.make_string()
             stdout = factory.make_string()
             stderr = factory.make_string()
             script['output'] = output.encode()
             script['stdout'] = stdout.encode()
             script['stderr'] = stderr.encode()
-            script['output_path'] = os.path.join(scripts_dir, script['name'])
+            script['output_path'] = os.path.join(
+                script_out_dir, script['name'])
             script['stdout_path'] = os.path.join(
-                scripts_dir, '%s.out' % script['name'])
+                script_out_dir, '%s.out' % script['name'])
             script['stderr_path'] = os.path.join(
-                scripts_dir, '%s.err' % script['name'])
+                script_out_dir, '%s.err' % script['name'])
             script['download_path'] = os.path.join(
-                scripts_dir, 'downloads', script['name'])
+                script_out_dir, 'downloads', script['name'])
             os.makedirs(script['download_path'], exist_ok=True)
             open(script['output_path'], 'w').write(output)
             open(script['stdout_path'], 'w').write(stdout)
@@ -73,7 +79,7 @@ class TestMaasRunRemoteScripts(MAASTestCase):
                 result = factory.make_string()
                 script['result'] = result.encode()
                 script['result_path'] = os.path.join(
-                    scripts_dir, '%s.yaml' % script['name'])
+                    script_out_dir, '%s.yaml' % script['name'])
                 open(script['result_path'], 'w').write(result)
 
     def make_index_json(
@@ -732,6 +738,152 @@ class TestMaasRunRemoteScripts(MAASTestCase):
         args['error'] = 'Finished %s [2/2]: 1' % scripts[1]['name']
         self.assertThat(mock_signal, MockAnyCall(**args))
 
+    def test_get_block_devices(self):
+        expected_blockdevs = [
+            {
+                'NAME': factory.make_name('NAME'),
+                'MODEL': factory.make_name('MODEL'),
+                'SERIAL': factory.make_name('SERIAL'),
+            } for _ in range(3)
+        ]
+        mock_check_output = self.patch(maas_run_remote_scripts, 'check_output')
+        mock_check_output.return_value = ''.join([
+            'NAME="{NAME}" MODEL="{MODEL}" SERIAL="{SERIAL}"\n'.format(
+                **blockdev) for blockdev in expected_blockdevs]).encode()
+        maas_run_remote_scripts._block_devices = None
+
+        self.assertItemsEqual(expected_blockdevs, get_block_devices())
+
+    def test_get_block_devices_cached(self):
+        block_devices = factory.make_name('block_devices')
+        mock_check_output = self.patch(maas_run_remote_scripts, 'check_output')
+        maas_run_remote_scripts._block_devices = block_devices
+
+        self.assertItemsEqual(block_devices, get_block_devices())
+        self.assertThat(mock_check_output, MockNotCalled())
+
+    def test_parse_parameters(self):
+        scripts_dir = factory.make_name('scripts_dir')
+        script = {
+            'path': os.path.join('path_to', factory.make_name('script_name')),
+            'parameters': {
+                'runtime': {
+                    'type': 'runtime',
+                    'value': random.randint(0, 1000),
+                },
+                'storage_virtio': {
+                    'type': 'storage',
+                    'value': {
+                        'name': factory.make_name('name'),
+                        'model': '',
+                        'serial': '',
+                        'id_path': '/dev/%s' % factory.make_name('id_path'),
+                    },
+                },
+                'storage': {
+                    'type': 'storage',
+                    'value': {
+                        'name': factory.make_name('name'),
+                        'model': factory.make_name('model'),
+                        'serial': factory.make_name('serial'),
+                        'id_path': '/dev/%s' % factory.make_name('id_path'),
+                    },
+                },
+            },
+        }
+        mock_check_output = self.patch(maas_run_remote_scripts, 'check_output')
+        mock_check_output.return_value = ''.join([
+            'NAME="{name}" MODEL="{model}" SERIAL="{serial}"\n'.format(
+                **param['value'])
+            for param_name, param in script['parameters'].items()
+            if 'storage' in param_name]).encode()
+        maas_run_remote_scripts._block_devices = None
+
+        self.assertItemsEqual(
+            [
+                os.path.join(scripts_dir, script['path']),
+                '--runtime=%s' % script['parameters']['runtime']['value'],
+                '--storage=%s' % script['parameters']['storage_virtio'][
+                    'value']['id_path'],
+                '--storage=/dev/%s' % script['parameters']['storage']['value'][
+                    'name'],
+            ], parse_parameters(script, scripts_dir))
+
+    def test_parse_parameters_argument_format(self):
+        scripts_dir = factory.make_name('scripts_dir')
+        script = {
+            'path': os.path.join('path_to', factory.make_name('script_name')),
+            'parameters': {
+                'runtime': {
+                    'type': 'runtime',
+                    'value': random.randint(0, 1000),
+                    'argument_format': '--foo --timeout {input}',
+                },
+                'storage': {
+                    'type': 'storage',
+                    'value': {
+                        'name': factory.make_name('name'),
+                        'model': factory.make_name('model'),
+                        'serial': factory.make_name('serial'),
+                        'id_path': '/dev/%s' % factory.make_name('id_path'),
+                    },
+                    'argument_format': (
+                        '--bar {name} {model} {serial} {path} {input}'),
+                },
+            },
+        }
+        mock_check_output = self.patch(maas_run_remote_scripts, 'check_output')
+        mock_check_output.return_value = ''.join([
+            'NAME="{name}" MODEL="{model}" SERIAL="{serial}"\n'.format(
+                **param['value'])
+            for param_name, param in script['parameters'].items()
+            if 'storage' in param_name]).encode()
+        maas_run_remote_scripts._block_devices = None
+
+        self.assertItemsEqual(
+            [
+                os.path.join(scripts_dir, script['path']),
+                '--foo', '--timeout',
+                str(script['parameters']['runtime']['value']),
+                '--bar', script['parameters']['storage']['value']['name'],
+                script['parameters']['storage']['value']['model'],
+                script['parameters']['storage']['value']['serial'],
+                '/dev/%s' % script['parameters']['storage']['value']['name'],
+                '/dev/%s' % script['parameters']['storage']['value']['name'],
+            ], parse_parameters(script, scripts_dir))
+
+    def test_run_scripts_errors_with_bad_param(self):
+        scripts_dir = self.useFixture(TempDirectory()).path
+        mock_signal = self.patch(maas_run_remote_scripts, 'signal')
+        mock_popen = self.patch(maas_run_remote_scripts, 'Popen')
+        self.patch(maas_run_remote_scripts, 'install_dependencies')
+        scripts = self.make_scripts()
+        scripts[0]['parameters'] = {'storage': {
+            'type': 'storage',
+            'argument_format': '{bad}',
+        }}
+        self.make_script_output(scripts, scripts_dir, with_result=True)
+
+        self.assertEquals(
+            0, run_scripts(None, None, scripts_dir, scripts_dir, scripts))
+
+        args = {
+            'url': None, 'creds': None, 'status': 'WORKING',
+            'script_result_id': scripts[0]['script_result_id'],
+            'script_version_id': scripts[0]['script_version_id'],
+            'error': 'Starting %s [1/1]' % scripts[0]['name'],
+        }
+        self.assertThat(mock_signal, MockAnyCall(**args))
+        self.assertThat(mock_popen, MockNotCalled())
+        # This is a MagicMock
+        args['exit_status'] = 2
+        args['files'] = {
+            scripts[0]['name']: b'Unable to map parameters',
+            '%s.err' % scripts[0]['name']: b'Unable to map parameters',
+        }
+        args['error'] = 'Failed to execute %s [1/1]: 2' % scripts[0]['name']
+        self.assertThat(mock_signal, MockAnyCall(**args))
+
     def test_run_scripts_sends_result_when_available(self):
         scripts_dir = self.useFixture(TempDirectory()).path
         mock_signal = self.patch(maas_run_remote_scripts, 'signal')
@@ -788,7 +940,10 @@ class TestMaasRunRemoteScripts(MAASTestCase):
             1, run_scripts(None, None, scripts_dir, scripts_dir, scripts))
 
         env = mock_popen.call_args[1]['env']
-        base = os.path.join(scripts_dir, scripts[0]['name'])
+        base = os.path.join(
+            scripts_dir, '%s.%s' % (
+                scripts[0]['name'], scripts[0]['script_result_id']),
+            scripts[0]['name'])
         self.assertEquals(base, env['OUTPUT_COMBINED_PATH'])
         self.assertEquals('%s.out' % base, env['OUTPUT_STDOUT_PATH'])
         self.assertEquals('%s.err' % base, env['OUTPUT_STDERR_PATH'])