From aedc61466f7f535e051d1dc6670faa1be52b28e4 Mon Sep 17 00:00:00 2001 From: Kevin Carter Date: Wed, 22 Jul 2020 11:39:50 -0500 Subject: [PATCH] Correct ansible playbook execution handler This change modifies the `run_ansible_playbook` method so that its correctly handling output. When a playbook has a lot output the method will hang in an S+ state waiting for stdout to be read and the FD closed. To correct this issue the method has been modified to return only the rc information and allow the `run_command_and_log` to handle stdout, which ensures this subprocess invocation is handled in the same way as all other invocations. > All uses of `run_ansible_playbook` have been updated to ensure we're handling the process correctly throughout the client. > The method `run_ansible_playbook` will now execute and return None. In the event of a failure, the method will still raise a "Runtime" exception and log a warning. > This change only impacts stable/train, this issue has been resolved in future releases through the use of Ansible-Runner. > Tests have been updated to support the changes within the `run_ansible_playbook` method. Change-Id: Id60bf618f6cd778970c5ff3aee19bcc40d294c3b Signed-off-by: Kevin Carter --- tripleoclient/tests/test_utils.py | 49 +++++++------------ .../overcloud_delete/test_overcloud_delete.py | 4 -- tripleoclient/utils.py | 30 +++++++----- tripleoclient/v1/overcloud_delete.py | 9 ++-- tripleoclient/v1/tripleo_validator.py | 7 ++- 5 files changed, 40 insertions(+), 59 deletions(-) diff --git a/tripleoclient/tests/test_utils.py b/tripleoclient/tests/test_utils.py index 79e6e1926..e41b1493e 100644 --- a/tripleoclient/tests/test_utils.py +++ b/tripleoclient/tests/test_utils.py @@ -48,6 +48,7 @@ class TestRunAnsiblePlaybook(TestCase): self.addCleanup(self.unlink_patch.stop) self.unlink_patch.start() self.mock_log = mock.Mock('logging.getLogger') + self.mock_log.warning = mock.MagicMock() python_version = sys.version_info[0] self.ansible_playbook_cmd = "ansible-playbook-%s" % (python_version) @@ -109,7 +110,7 @@ class TestRunAnsiblePlaybook(TestCase): '-i', 'localhost,', '-v', '-c', 'smart', '/tmp/existing.yaml'], - env=env, retcode_only=False) + env=env) @mock.patch('os.path.isabs') @mock.patch('os.path.exists', return_value=False) @@ -128,13 +129,10 @@ class TestRunAnsiblePlaybook(TestCase): @mock.patch('os.path.exists', return_value=True) @mock.patch('tripleoclient.utils.run_command_and_log') def test_run_success_default(self, mock_run, mock_exists, mock_mkstemp): - mock_process = mock.Mock() - mock_process.returncode = 0 - mock_run.return_value = mock_process + mock_run.return_value = 0 - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( self.mock_log, '/tmp', 'existing.yaml', 'localhost,') - self.assertEqual(retcode, 0) mock_exists.assert_called_once_with('/tmp/existing.yaml') env = os.environ.copy() @@ -166,23 +164,20 @@ class TestRunAnsiblePlaybook(TestCase): '-i', 'localhost,', '-v', '-c', 'smart', '/tmp/existing.yaml'], - env=env, retcode_only=False) + env=env) @mock.patch('os.path.isabs') @mock.patch('os.path.exists', return_value=True) @mock.patch('tripleoclient.utils.run_command_and_log') def test_run_success_ansible_cfg(self, mock_run, mock_exists, mock_isabs): - mock_process = mock.Mock() - mock_process.returncode = 0 - mock_run.return_value = mock_process + mock_run.return_value = 0 - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( self.mock_log, '/tmp', 'existing.yaml', 'localhost,', ansible_config='/tmp/foo.cfg') - self.assertEqual(retcode, 0) mock_isabs.assert_called_once_with('/tmp/foo.cfg') @@ -219,24 +214,21 @@ class TestRunAnsiblePlaybook(TestCase): '-i', 'localhost,', '-v', '-c', 'smart', '/tmp/existing.yaml'], - env=env, retcode_only=False) + env=env) @mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg')) @mock.patch('os.path.exists', return_value=True) @mock.patch('tripleoclient.utils.run_command_and_log') def test_run_success_connection_local(self, mock_run, mock_exists, mok_mkstemp): - mock_process = mock.Mock() - mock_process.returncode = 0 - mock_run.return_value = mock_process + mock_run.return_value = 0 - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( self.mock_log, '/tmp', 'existing.yaml', 'localhost,', connection='local') - self.assertEqual(retcode, 0) mock_exists.assert_called_once_with('/tmp/existing.yaml') env = os.environ.copy() env['ANSIBLE_LIBRARY'] = \ @@ -267,24 +259,21 @@ class TestRunAnsiblePlaybook(TestCase): '-i', 'localhost,', '-v', '-c', 'local', '/tmp/existing.yaml'], - env=env, retcode_only=False) + env=env) @mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg')) @mock.patch('os.path.exists', return_value=True) @mock.patch('tripleoclient.utils.run_command_and_log') def test_run_success_gathering_policy(self, mock_run, mock_exists, mok_mkstemp): - mock_process = mock.Mock() - mock_process.returncode = 0 - mock_run.return_value = mock_process + mock_run.return_value = 0 - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( self.mock_log, '/tmp', 'existing.yaml', 'localhost,', gathering_policy='explicit') - self.assertEqual(retcode, 0) mock_exists.assert_called_once_with('/tmp/existing.yaml') env = os.environ.copy() env['ANSIBLE_LIBRARY'] = \ @@ -316,28 +305,25 @@ class TestRunAnsiblePlaybook(TestCase): '-i', 'localhost,', '-v', '-c', 'smart', '/tmp/existing.yaml'], - env=env, retcode_only=False) + env=env) @mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg')) @mock.patch('os.path.exists', return_value=True) @mock.patch('tripleoclient.utils.run_command_and_log') def test_run_success_extra_vars(self, mock_run, mock_exists, mock_mkstemp): - mock_process = mock.Mock() - mock_process.returncode = 0 - mock_run.return_value = mock_process + mock_run.return_value = 0 arglist = { 'var_one': 'val_one', } - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( self.mock_log, '/tmp', 'existing.yaml', 'localhost,', extra_vars=arglist) - self.assertEqual(retcode, 0) mock_exists.assert_called_once_with('/tmp/existing.yaml') env = os.environ.copy() env['ANSIBLE_LIBRARY'] = \ @@ -369,8 +355,7 @@ class TestRunAnsiblePlaybook(TestCase): '--extra-vars', '%s' % arglist, '-c', 'smart', '/tmp/existing.yaml' ], - env=env, - retcode_only=False) + env=env) class TestRunCommandAndLog(TestCase): diff --git a/tripleoclient/tests/v1/overcloud_delete/test_overcloud_delete.py b/tripleoclient/tests/v1/overcloud_delete/test_overcloud_delete.py index 760446201..5670832c6 100644 --- a/tripleoclient/tests/v1/overcloud_delete/test_overcloud_delete.py +++ b/tripleoclient/tests/v1/overcloud_delete/test_overcloud_delete.py @@ -127,10 +127,6 @@ class TestDeleteOvercloud(fakes.TestDeployOvercloud): self.cmd.log.debug.assert_any_call( "Removing temporary ansible configuration directory" ) - self.cmd.log.warning.assert_any_call( - "/usr/share/ansible/tripleo-playbooks/cli-cleanup-ipa.yml " - "did not complete successfully." - ) @mock.patch( "tripleoclient.v1.overcloud_delete.DeleteOvercloud._cleanup_ipa", diff --git a/tripleoclient/utils.py b/tripleoclient/utils.py index b82fd2fd1..b92e55313 100644 --- a/tripleoclient/utils.py +++ b/tripleoclient/utils.py @@ -211,11 +211,15 @@ def run_ansible_playbook(logger, play = os.path.join(workdir, playbook) - if os.path.exists(play): - cmd = ["ansible-playbook-{}".format(sys.version_info[0]), - '-u', ssh_user, - '-i', inventory - ] + try: + if not os.path.exists(play): + raise RuntimeError('No such playbook: %s' % play) + + cmd = [ + "ansible-playbook-{}".format(sys.version_info[0]), + '-u', ssh_user, + '-i', inventory + ] if 0 < verbosity < 6: cmd.extend(['-' + ('v' * verbosity)]) @@ -254,15 +258,15 @@ def run_ansible_playbook(logger, cmd.extend(['-c', connection, play]) - proc = run_command_and_log(logger, cmd, env=env, retcode_only=False) - proc.wait() + if run_command_and_log(logger, cmd, env=env) != 0: + logger.warning( + "{} did not complete successfully.".format(play) + ) + raise RuntimeError( + "Ansible playbook execution failed: {}.".format(cmd) + ) + finally: cleanup and os.unlink(tmp_config) - if proc.returncode != 0: - raise RuntimeError(proc.stdout.read()) - return proc.returncode, proc.stdout.read() - else: - cleanup and os.unlink(tmp_config) - raise RuntimeError('No such playbook: %s' % play) def convert(data): diff --git a/tripleoclient/v1/overcloud_delete.py b/tripleoclient/v1/overcloud_delete.py index e3c81bc89..40627bea5 100644 --- a/tripleoclient/v1/overcloud_delete.py +++ b/tripleoclient/v1/overcloud_delete.py @@ -126,18 +126,15 @@ class DeleteOvercloud(command.Command): tmp_tripleoclient_dir, remote_user) try: - rc, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( self.log, constants.ANSIBLE_TRIPLEO_PLAYBOOKS, playbook, static_inventory, log_path_dir=pwd.getpwuid(os.getuid()).pw_dir, ansible_config=ansible_config, - python_interpreter=python_interpreter) - if rc != 0: - self.log.warning( - "{} did not complete successfully.".format(playbook) - ) + python_interpreter=python_interpreter + ) finally: self.log.debug("Removing static tripleo ansible inventory file") utils.cleanup_tripleo_ansible_inventory_file(static_inventory) diff --git a/tripleoclient/v1/tripleo_validator.py b/tripleoclient/v1/tripleo_validator.py index 564d9c65b..15d537129 100644 --- a/tripleoclient/v1/tripleo_validator.py +++ b/tripleoclient/v1/tripleo_validator.py @@ -423,7 +423,7 @@ class TripleOValidatorRun(command.Command): def _run_ansible(self, logger, plan, workdir, log_path_dir, playbook, inventory, retries, output_callback, extra_vars, python_interpreter, gathering_policy): - rc, output = oooutils.run_ansible_playbook( + oooutils.run_ansible_playbook( logger=logger, plan=plan, workdir=workdir, @@ -435,7 +435,6 @@ class TripleOValidatorRun(command.Command): extra_vars=extra_vars, python_interpreter=python_interpreter, gathering_policy=gathering_policy) - return rc, output def _run_validator_run(self, parsed_args): LOG = logging.getLogger(__name__ + ".ValidationsRunAnsible") @@ -505,13 +504,13 @@ class TripleOValidatorRun(command.Command): for tk, pl in six.iteritems(tasks_exec): try: - _rc, output = tk.result() + tk.result() results.append({ 'validation': { 'validation_id': pl, 'logfile': None, 'status': 'PASSED', - 'output': output + 'output': 'Ansible playbook execution complete.' }}) except Exception as e: failed_val = True