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 <kecarter@redhat.com>
This commit is contained in:
parent
037dd3342f
commit
aedc61466f
|
@ -48,6 +48,7 @@ class TestRunAnsiblePlaybook(TestCase):
|
||||||
self.addCleanup(self.unlink_patch.stop)
|
self.addCleanup(self.unlink_patch.stop)
|
||||||
self.unlink_patch.start()
|
self.unlink_patch.start()
|
||||||
self.mock_log = mock.Mock('logging.getLogger')
|
self.mock_log = mock.Mock('logging.getLogger')
|
||||||
|
self.mock_log.warning = mock.MagicMock()
|
||||||
python_version = sys.version_info[0]
|
python_version = sys.version_info[0]
|
||||||
self.ansible_playbook_cmd = "ansible-playbook-%s" % (python_version)
|
self.ansible_playbook_cmd = "ansible-playbook-%s" % (python_version)
|
||||||
|
|
||||||
|
@ -109,7 +110,7 @@ class TestRunAnsiblePlaybook(TestCase):
|
||||||
'-i', 'localhost,', '-v',
|
'-i', 'localhost,', '-v',
|
||||||
'-c', 'smart',
|
'-c', 'smart',
|
||||||
'/tmp/existing.yaml'],
|
'/tmp/existing.yaml'],
|
||||||
env=env, retcode_only=False)
|
env=env)
|
||||||
|
|
||||||
@mock.patch('os.path.isabs')
|
@mock.patch('os.path.isabs')
|
||||||
@mock.patch('os.path.exists', return_value=False)
|
@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('os.path.exists', return_value=True)
|
||||||
@mock.patch('tripleoclient.utils.run_command_and_log')
|
@mock.patch('tripleoclient.utils.run_command_and_log')
|
||||||
def test_run_success_default(self, mock_run, mock_exists, mock_mkstemp):
|
def test_run_success_default(self, mock_run, mock_exists, mock_mkstemp):
|
||||||
mock_process = mock.Mock()
|
mock_run.return_value = 0
|
||||||
mock_process.returncode = 0
|
|
||||||
mock_run.return_value = mock_process
|
|
||||||
|
|
||||||
retcode, output = utils.run_ansible_playbook(
|
utils.run_ansible_playbook(
|
||||||
self.mock_log, '/tmp', 'existing.yaml', 'localhost,')
|
self.mock_log, '/tmp', 'existing.yaml', 'localhost,')
|
||||||
self.assertEqual(retcode, 0)
|
|
||||||
mock_exists.assert_called_once_with('/tmp/existing.yaml')
|
mock_exists.assert_called_once_with('/tmp/existing.yaml')
|
||||||
|
|
||||||
env = os.environ.copy()
|
env = os.environ.copy()
|
||||||
|
@ -166,23 +164,20 @@ class TestRunAnsiblePlaybook(TestCase):
|
||||||
'-i', 'localhost,', '-v',
|
'-i', 'localhost,', '-v',
|
||||||
'-c', 'smart',
|
'-c', 'smart',
|
||||||
'/tmp/existing.yaml'],
|
'/tmp/existing.yaml'],
|
||||||
env=env, retcode_only=False)
|
env=env)
|
||||||
|
|
||||||
@mock.patch('os.path.isabs')
|
@mock.patch('os.path.isabs')
|
||||||
@mock.patch('os.path.exists', return_value=True)
|
@mock.patch('os.path.exists', return_value=True)
|
||||||
@mock.patch('tripleoclient.utils.run_command_and_log')
|
@mock.patch('tripleoclient.utils.run_command_and_log')
|
||||||
def test_run_success_ansible_cfg(self, mock_run, mock_exists, mock_isabs):
|
def test_run_success_ansible_cfg(self, mock_run, mock_exists, mock_isabs):
|
||||||
mock_process = mock.Mock()
|
mock_run.return_value = 0
|
||||||
mock_process.returncode = 0
|
|
||||||
mock_run.return_value = mock_process
|
|
||||||
|
|
||||||
retcode, output = utils.run_ansible_playbook(
|
utils.run_ansible_playbook(
|
||||||
self.mock_log,
|
self.mock_log,
|
||||||
'/tmp',
|
'/tmp',
|
||||||
'existing.yaml',
|
'existing.yaml',
|
||||||
'localhost,',
|
'localhost,',
|
||||||
ansible_config='/tmp/foo.cfg')
|
ansible_config='/tmp/foo.cfg')
|
||||||
self.assertEqual(retcode, 0)
|
|
||||||
|
|
||||||
mock_isabs.assert_called_once_with('/tmp/foo.cfg')
|
mock_isabs.assert_called_once_with('/tmp/foo.cfg')
|
||||||
|
|
||||||
|
@ -219,24 +214,21 @@ class TestRunAnsiblePlaybook(TestCase):
|
||||||
'-i', 'localhost,', '-v',
|
'-i', 'localhost,', '-v',
|
||||||
'-c', 'smart',
|
'-c', 'smart',
|
||||||
'/tmp/existing.yaml'],
|
'/tmp/existing.yaml'],
|
||||||
env=env, retcode_only=False)
|
env=env)
|
||||||
|
|
||||||
@mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg'))
|
@mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg'))
|
||||||
@mock.patch('os.path.exists', return_value=True)
|
@mock.patch('os.path.exists', return_value=True)
|
||||||
@mock.patch('tripleoclient.utils.run_command_and_log')
|
@mock.patch('tripleoclient.utils.run_command_and_log')
|
||||||
def test_run_success_connection_local(self, mock_run, mock_exists,
|
def test_run_success_connection_local(self, mock_run, mock_exists,
|
||||||
mok_mkstemp):
|
mok_mkstemp):
|
||||||
mock_process = mock.Mock()
|
mock_run.return_value = 0
|
||||||
mock_process.returncode = 0
|
|
||||||
mock_run.return_value = mock_process
|
|
||||||
|
|
||||||
retcode, output = utils.run_ansible_playbook(
|
utils.run_ansible_playbook(
|
||||||
self.mock_log,
|
self.mock_log,
|
||||||
'/tmp',
|
'/tmp',
|
||||||
'existing.yaml',
|
'existing.yaml',
|
||||||
'localhost,',
|
'localhost,',
|
||||||
connection='local')
|
connection='local')
|
||||||
self.assertEqual(retcode, 0)
|
|
||||||
mock_exists.assert_called_once_with('/tmp/existing.yaml')
|
mock_exists.assert_called_once_with('/tmp/existing.yaml')
|
||||||
env = os.environ.copy()
|
env = os.environ.copy()
|
||||||
env['ANSIBLE_LIBRARY'] = \
|
env['ANSIBLE_LIBRARY'] = \
|
||||||
|
@ -267,24 +259,21 @@ class TestRunAnsiblePlaybook(TestCase):
|
||||||
'-i', 'localhost,', '-v',
|
'-i', 'localhost,', '-v',
|
||||||
'-c', 'local',
|
'-c', 'local',
|
||||||
'/tmp/existing.yaml'],
|
'/tmp/existing.yaml'],
|
||||||
env=env, retcode_only=False)
|
env=env)
|
||||||
|
|
||||||
@mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg'))
|
@mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg'))
|
||||||
@mock.patch('os.path.exists', return_value=True)
|
@mock.patch('os.path.exists', return_value=True)
|
||||||
@mock.patch('tripleoclient.utils.run_command_and_log')
|
@mock.patch('tripleoclient.utils.run_command_and_log')
|
||||||
def test_run_success_gathering_policy(self, mock_run, mock_exists,
|
def test_run_success_gathering_policy(self, mock_run, mock_exists,
|
||||||
mok_mkstemp):
|
mok_mkstemp):
|
||||||
mock_process = mock.Mock()
|
mock_run.return_value = 0
|
||||||
mock_process.returncode = 0
|
|
||||||
mock_run.return_value = mock_process
|
|
||||||
|
|
||||||
retcode, output = utils.run_ansible_playbook(
|
utils.run_ansible_playbook(
|
||||||
self.mock_log,
|
self.mock_log,
|
||||||
'/tmp',
|
'/tmp',
|
||||||
'existing.yaml',
|
'existing.yaml',
|
||||||
'localhost,',
|
'localhost,',
|
||||||
gathering_policy='explicit')
|
gathering_policy='explicit')
|
||||||
self.assertEqual(retcode, 0)
|
|
||||||
mock_exists.assert_called_once_with('/tmp/existing.yaml')
|
mock_exists.assert_called_once_with('/tmp/existing.yaml')
|
||||||
env = os.environ.copy()
|
env = os.environ.copy()
|
||||||
env['ANSIBLE_LIBRARY'] = \
|
env['ANSIBLE_LIBRARY'] = \
|
||||||
|
@ -316,28 +305,25 @@ class TestRunAnsiblePlaybook(TestCase):
|
||||||
'-i', 'localhost,', '-v',
|
'-i', 'localhost,', '-v',
|
||||||
'-c', 'smart',
|
'-c', 'smart',
|
||||||
'/tmp/existing.yaml'],
|
'/tmp/existing.yaml'],
|
||||||
env=env, retcode_only=False)
|
env=env)
|
||||||
|
|
||||||
@mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg'))
|
@mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg'))
|
||||||
@mock.patch('os.path.exists', return_value=True)
|
@mock.patch('os.path.exists', return_value=True)
|
||||||
@mock.patch('tripleoclient.utils.run_command_and_log')
|
@mock.patch('tripleoclient.utils.run_command_and_log')
|
||||||
def test_run_success_extra_vars(self, mock_run, mock_exists, mock_mkstemp):
|
def test_run_success_extra_vars(self, mock_run, mock_exists, mock_mkstemp):
|
||||||
mock_process = mock.Mock()
|
mock_run.return_value = 0
|
||||||
mock_process.returncode = 0
|
|
||||||
mock_run.return_value = mock_process
|
|
||||||
|
|
||||||
arglist = {
|
arglist = {
|
||||||
'var_one': 'val_one',
|
'var_one': 'val_one',
|
||||||
}
|
}
|
||||||
|
|
||||||
retcode, output = utils.run_ansible_playbook(
|
utils.run_ansible_playbook(
|
||||||
self.mock_log,
|
self.mock_log,
|
||||||
'/tmp',
|
'/tmp',
|
||||||
'existing.yaml',
|
'existing.yaml',
|
||||||
'localhost,',
|
'localhost,',
|
||||||
extra_vars=arglist)
|
extra_vars=arglist)
|
||||||
|
|
||||||
self.assertEqual(retcode, 0)
|
|
||||||
mock_exists.assert_called_once_with('/tmp/existing.yaml')
|
mock_exists.assert_called_once_with('/tmp/existing.yaml')
|
||||||
env = os.environ.copy()
|
env = os.environ.copy()
|
||||||
env['ANSIBLE_LIBRARY'] = \
|
env['ANSIBLE_LIBRARY'] = \
|
||||||
|
@ -369,8 +355,7 @@ class TestRunAnsiblePlaybook(TestCase):
|
||||||
'--extra-vars', '%s' % arglist,
|
'--extra-vars', '%s' % arglist,
|
||||||
'-c', 'smart', '/tmp/existing.yaml'
|
'-c', 'smart', '/tmp/existing.yaml'
|
||||||
],
|
],
|
||||||
env=env,
|
env=env)
|
||||||
retcode_only=False)
|
|
||||||
|
|
||||||
|
|
||||||
class TestRunCommandAndLog(TestCase):
|
class TestRunCommandAndLog(TestCase):
|
||||||
|
|
|
@ -127,10 +127,6 @@ class TestDeleteOvercloud(fakes.TestDeployOvercloud):
|
||||||
self.cmd.log.debug.assert_any_call(
|
self.cmd.log.debug.assert_any_call(
|
||||||
"Removing temporary ansible configuration directory"
|
"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(
|
@mock.patch(
|
||||||
"tripleoclient.v1.overcloud_delete.DeleteOvercloud._cleanup_ipa",
|
"tripleoclient.v1.overcloud_delete.DeleteOvercloud._cleanup_ipa",
|
||||||
|
|
|
@ -211,11 +211,15 @@ def run_ansible_playbook(logger,
|
||||||
|
|
||||||
play = os.path.join(workdir, playbook)
|
play = os.path.join(workdir, playbook)
|
||||||
|
|
||||||
if os.path.exists(play):
|
try:
|
||||||
cmd = ["ansible-playbook-{}".format(sys.version_info[0]),
|
if not os.path.exists(play):
|
||||||
'-u', ssh_user,
|
raise RuntimeError('No such playbook: %s' % play)
|
||||||
'-i', inventory
|
|
||||||
]
|
cmd = [
|
||||||
|
"ansible-playbook-{}".format(sys.version_info[0]),
|
||||||
|
'-u', ssh_user,
|
||||||
|
'-i', inventory
|
||||||
|
]
|
||||||
|
|
||||||
if 0 < verbosity < 6:
|
if 0 < verbosity < 6:
|
||||||
cmd.extend(['-' + ('v' * verbosity)])
|
cmd.extend(['-' + ('v' * verbosity)])
|
||||||
|
@ -254,15 +258,15 @@ def run_ansible_playbook(logger,
|
||||||
|
|
||||||
cmd.extend(['-c', connection, play])
|
cmd.extend(['-c', connection, play])
|
||||||
|
|
||||||
proc = run_command_and_log(logger, cmd, env=env, retcode_only=False)
|
if run_command_and_log(logger, cmd, env=env) != 0:
|
||||||
proc.wait()
|
logger.warning(
|
||||||
|
"{} did not complete successfully.".format(play)
|
||||||
|
)
|
||||||
|
raise RuntimeError(
|
||||||
|
"Ansible playbook execution failed: {}.".format(cmd)
|
||||||
|
)
|
||||||
|
finally:
|
||||||
cleanup and os.unlink(tmp_config)
|
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):
|
def convert(data):
|
||||||
|
|
|
@ -126,18 +126,15 @@ class DeleteOvercloud(command.Command):
|
||||||
tmp_tripleoclient_dir, remote_user)
|
tmp_tripleoclient_dir, remote_user)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
rc, output = utils.run_ansible_playbook(
|
utils.run_ansible_playbook(
|
||||||
self.log,
|
self.log,
|
||||||
constants.ANSIBLE_TRIPLEO_PLAYBOOKS,
|
constants.ANSIBLE_TRIPLEO_PLAYBOOKS,
|
||||||
playbook,
|
playbook,
|
||||||
static_inventory,
|
static_inventory,
|
||||||
log_path_dir=pwd.getpwuid(os.getuid()).pw_dir,
|
log_path_dir=pwd.getpwuid(os.getuid()).pw_dir,
|
||||||
ansible_config=ansible_config,
|
ansible_config=ansible_config,
|
||||||
python_interpreter=python_interpreter)
|
python_interpreter=python_interpreter
|
||||||
if rc != 0:
|
)
|
||||||
self.log.warning(
|
|
||||||
"{} did not complete successfully.".format(playbook)
|
|
||||||
)
|
|
||||||
finally:
|
finally:
|
||||||
self.log.debug("Removing static tripleo ansible inventory file")
|
self.log.debug("Removing static tripleo ansible inventory file")
|
||||||
utils.cleanup_tripleo_ansible_inventory_file(static_inventory)
|
utils.cleanup_tripleo_ansible_inventory_file(static_inventory)
|
||||||
|
|
|
@ -423,7 +423,7 @@ class TripleOValidatorRun(command.Command):
|
||||||
def _run_ansible(self, logger, plan, workdir, log_path_dir, playbook,
|
def _run_ansible(self, logger, plan, workdir, log_path_dir, playbook,
|
||||||
inventory, retries, output_callback, extra_vars,
|
inventory, retries, output_callback, extra_vars,
|
||||||
python_interpreter, gathering_policy):
|
python_interpreter, gathering_policy):
|
||||||
rc, output = oooutils.run_ansible_playbook(
|
oooutils.run_ansible_playbook(
|
||||||
logger=logger,
|
logger=logger,
|
||||||
plan=plan,
|
plan=plan,
|
||||||
workdir=workdir,
|
workdir=workdir,
|
||||||
|
@ -435,7 +435,6 @@ class TripleOValidatorRun(command.Command):
|
||||||
extra_vars=extra_vars,
|
extra_vars=extra_vars,
|
||||||
python_interpreter=python_interpreter,
|
python_interpreter=python_interpreter,
|
||||||
gathering_policy=gathering_policy)
|
gathering_policy=gathering_policy)
|
||||||
return rc, output
|
|
||||||
|
|
||||||
def _run_validator_run(self, parsed_args):
|
def _run_validator_run(self, parsed_args):
|
||||||
LOG = logging.getLogger(__name__ + ".ValidationsRunAnsible")
|
LOG = logging.getLogger(__name__ + ".ValidationsRunAnsible")
|
||||||
|
@ -505,13 +504,13 @@ class TripleOValidatorRun(command.Command):
|
||||||
|
|
||||||
for tk, pl in six.iteritems(tasks_exec):
|
for tk, pl in six.iteritems(tasks_exec):
|
||||||
try:
|
try:
|
||||||
_rc, output = tk.result()
|
tk.result()
|
||||||
results.append({
|
results.append({
|
||||||
'validation': {
|
'validation': {
|
||||||
'validation_id': pl,
|
'validation_id': pl,
|
||||||
'logfile': None,
|
'logfile': None,
|
||||||
'status': 'PASSED',
|
'status': 'PASSED',
|
||||||
'output': output
|
'output': 'Ansible playbook execution complete.'
|
||||||
}})
|
}})
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
failed_val = True
|
failed_val = True
|
||||||
|
|
Loading…
Reference in New Issue