From e2f3df268dbcd8fd0b0e7209a237545e6761f681 Mon Sep 17 00:00:00 2001 From: Steven Hardy Date: Tue, 22 May 2018 14:47:43 +0100 Subject: [PATCH] Expose error when a hook script raises an exception Currently this can result in errors being ignored so it's not clear to the user that something failed. Change-Id: Idf6badecbfa72150f3506a485eed9ae2cb5858f7 Story: 2002084 Task: 19753 --- .../configure.d/55-heat-config | 9 ++- tests/hook-fake-raises.py | 41 +++++++++++++ tests/test_heat_config.py | 57 ++++++++++++++----- 3 files changed, 90 insertions(+), 17 deletions(-) create mode 100755 tests/hook-fake-raises.py diff --git a/heat-config/os-refresh-config/configure.d/55-heat-config b/heat-config/os-refresh-config/configure.d/55-heat-config index 2ed1975..37452a6 100755 --- a/heat-config/os-refresh-config/configure.d/55-heat-config +++ b/heat-config/os-refresh-config/configure.d/55-heat-config @@ -155,6 +155,11 @@ def invoke_hook(c, log): if subproc.returncode: log.error("Error running %s. [%s]\n" % ( hook_path, subproc.returncode)) + signal_data = { + 'deploy_stdout': stdout.decode("utf-8", "replace"), + 'deploy_stderr': stderr.decode("utf-8", "replace"), + 'deploy_status_code': subproc.returncode, + } else: log.info('Completed %s' % hook_path) @@ -163,8 +168,8 @@ def invoke_hook(c, log): signal_data = json.loads(stdout.decode('utf-8', 'replace')) except ValueError: signal_data = { - 'deploy_stdout': stdout, - 'deploy_stderr': stderr, + 'deploy_stdout': stdout.decode("utf-8", "replace"), + 'deploy_stderr': stderr.decode("utf-8", "replace"), 'deploy_status_code': subproc.returncode, } diff --git a/tests/hook-fake-raises.py b/tests/hook-fake-raises.py new file mode 100755 index 0000000..c5f5d0d --- /dev/null +++ b/tests/hook-fake-raises.py @@ -0,0 +1,41 @@ +#!/usr/bin/env python +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +''' +A fake heat-config hook for unit testing the 55-heat-config +os-refresh-config script, this version raises an error. +''' + +import json +import os +import sys + + +def main(argv=sys.argv): + c = json.load(sys.stdin) + + inputs = {} + for input in c['inputs']: + inputs[input['name']] = input.get('value', '') + + # write out stdin json for test asserts + stdin_path = os.path.join( + os.path.dirname(os.path.realpath(__file__)), '%s.stdin' % c['group']) + + with open(stdin_path, 'w') as f: + json.dump(c, f) + f.flush() + raise OSError("Something bad happened!") + +if __name__ == '__main__': + sys.exit(main(sys.argv)) diff --git a/tests/test_heat_config.py b/tests/test_heat_config.py index eabb13c..852259d 100644 --- a/tests/test_heat_config.py +++ b/tests/test_heat_config.py @@ -15,6 +15,7 @@ import copy import json import os import shutil +import six import tempfile import fixtures @@ -26,7 +27,7 @@ from tests import common class HeatConfigTest(common.RunScriptTest): fake_hooks = ['cfn-init', 'chef', 'puppet', 'salt', 'script', - 'apply-config', 'hiera', 'json-file'] + 'apply-config', 'hiera', 'json-file', 'hook-raises'] data = [ { @@ -85,6 +86,11 @@ class HeatConfigTest(common.RunScriptTest): 'group': 'no-such-hook', 'inputs': [], 'config': 'nine' + }, { + 'id': '0123', + 'group': 'hook-raises', + 'inputs': [], + 'config': 'ten' }] outputs = { @@ -128,6 +134,11 @@ class HeatConfigTest(common.RunScriptTest): 'deploy_status_code': '0', 'deploy_stderr': 'stderr', 'deploy_stdout': 'stdout' + }, + 'hook-raises': { + 'deploy_status_code': '1', + 'deploy_stderr': 'OSError: Something bad happened!', + 'deploy_stdout': '' } } @@ -135,7 +146,8 @@ class HeatConfigTest(common.RunScriptTest): super(HeatConfigTest, self).setUp() self.fake_hook_path = self.relative_path(__file__, 'hook-fake.py') - + self.fake_hook_raises_path = self.relative_path(__file__, + 'hook-fake-raises.py') self.heat_config_path = self.relative_path( __file__, '..', @@ -147,11 +159,17 @@ class HeatConfigTest(common.RunScriptTest): with open(self.fake_hook_path) as f: fake_hook = f.read() + with open(self.fake_hook_raises_path) as f: + fake_hook_raises = f.read() + for hook in self.fake_hooks: hook_name = self.hooks_dir.join(hook) with open(hook_name, 'w') as f: os.utime(hook_name, None) - f.write(fake_hook) + if hook == 'hook-raises': + f.write(fake_hook_raises) + else: + f.write(fake_hook) f.flush() os.chmod(hook_name, 0o755) self.env = os.environ.copy() @@ -170,10 +188,7 @@ class HeatConfigTest(common.RunScriptTest): 'HEAT_CONFIG_DEPLOYED': self.deployed_dir.join(), 'HEAT_SHELL_CONFIG': config_file.name }) - returncode, stdout, stderr = self.run_cmd( - [self.heat_config_path], self.env) - - self.assertEqual(0, returncode, stderr) + return self.run_cmd([self.heat_config_path], self.env) def test_hooks_exist(self): self.assertThat( @@ -186,13 +201,15 @@ class HeatConfigTest(common.RunScriptTest): def test_run_heat_config(self): - self.run_heat_config(self.data) + returncode, stdout, stderr = self.run_heat_config(self.data) for config in self.data: hook = config['group'] stdin_path = self.hooks_dir.join('%s.stdin' % hook) stdout_path = self.hooks_dir.join('%s.stdout' % hook) deployed_file = self.deployed_dir.join('%s.json' % config['id']) + notify_file = self.deployed_dir.join('%s.notify.json' % + config['id']) if hook == 'no-such-hook': self.assertThat( @@ -201,10 +218,8 @@ class HeatConfigTest(common.RunScriptTest): stdout_path, matchers.Not(matchers.FileExists())) continue - self.assertThat(stdin_path, matchers.FileExists()) - self.assertThat(stdout_path, matchers.FileExists()) - # parsed stdin should match the config item + self.assertThat(stdin_path, matchers.FileExists()) self.assertEqual(config, self.json_from_file(stdin_path)) @@ -212,12 +227,24 @@ class HeatConfigTest(common.RunScriptTest): self.assertEqual(config, self.json_from_file(deployed_file)) - self.assertEqual(self.outputs[hook], - self.json_from_file(stdout_path)) + if hook != 'hook-raises': + self.assertEqual(self.outputs[hook], + self.json_from_file(notify_file)) + self.assertEqual(self.outputs[hook], + self.json_from_file(stdout_path)) + self.assertThat(stdout_path, matchers.FileExists()) + os.remove(stdout_path) + else: + notify_data = self.json_from_file(notify_file) + self.assertEqual( + self.outputs[hook]['deploy_status_code'], + six.text_type(notify_data['deploy_status_code'])) + self.assertIn( + self.outputs[hook]['deploy_stderr'], + notify_data['deploy_stderr']) # clean up files in preparation for second run os.remove(stdin_path) - os.remove(stdout_path) # run again with no changes, assert no new files self.run_heat_config(self.data) @@ -261,7 +288,7 @@ class HeatConfigTest(common.RunScriptTest): self.run_heat_config(data) for config in self.data: hook = config['group'] - if hook == 'no-such-hook': + if hook in ('no-such-hook', 'hook-raises'): continue deployed_file = self.deployed_dir.join('%s.json' % config['id']) old_deployed_file = old_deployed_dir.join('%s.json' % config['id'])