From a3132e5dd6acc526ce575f6db134169c7090f72d Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Fri, 11 Aug 2017 17:50:49 -0700 Subject: [PATCH] Optimize template (#28044) * Optimize template * In fixing template to handle diff correctly #24477, I introduced more round trips to the remote end which slowed things down The new code now uses one fewer round trips than the old code. * Reimplement a large part of template by calling the copy action plugin instead of doing it in template's code. This reduces the code in template and gives us one place to fix bugs and optimize. * Add a follow parameter to template that mirrors the follow parameters for file and copy. * Fix copy's diff handling (probably broken in my rewrite for in 2.4 development) * Adjusted when copy creates tmp dirs to rduce round trips in copy and template. Fixes #27956 --- CHANGELOG.md | 3 + lib/ansible/modules/files/template.py | 7 ++ lib/ansible/plugins/action/copy.py | 21 ++--- lib/ansible/plugins/action/template.py | 106 +++++++------------------ 4 files changed, 51 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c7baf056c36..dc59ba11f6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,6 +149,9 @@ Ansible Changes By Release absolute path. - The archive module has a new parameter exclude_path which lists paths to exclude from the archive - The yum module has a new parameter security which limits state=latest to security updates +- The template module gained a follow parameter to match with copy and file. + Like those modules, template defaults this parameter to False. Previously, + template hardcoded this to true. ### New Modules - aci diff --git a/lib/ansible/modules/files/template.py b/lib/ansible/modules/files/template.py index 5fce2f037d2..2d876907ba3 100644 --- a/lib/ansible/modules/files/template.py +++ b/lib/ansible/modules/files/template.py @@ -81,6 +81,13 @@ options: if the destination does not exist. type: bool default: 'yes' + follow: + description: + - This flag indicates that filesystem links in the destination, if they exist, should be followed. + - Previous to Ansible 2.4, this was hardcoded as C(yes). + type: bool + default: 'no' + version_added: "2.4" notes: - For Windows you can use M(win_template) which uses '\r\n' as C(newline_sequence). - Including a string that uses a date in the template will result in the template being marked 'changed' each time diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py index 55aba4dc179..14820cb3f95 100644 --- a/lib/ansible/plugins/action/copy.py +++ b/lib/ansible/plugins/action/copy.py @@ -219,6 +219,12 @@ class ActionModule(ActionBase): else: dest_file = self._connection._shell.join_path(dest) + # Create a tmp path if missing only if this is not recursive. + # If this is recursive we already have a tmp path. + if delete_remote_tmp: + if tmp is None or "-tmp-" not in tmp: + tmp = self._make_tmp_path() + # Attempt to get remote file info dest_status = self._execute_remote_stat(dest_file, all_vars=task_vars, follow=follow, tmp=tmp, checksum=force) @@ -246,19 +252,13 @@ class ActionModule(ActionBase): if local_checksum != dest_status['checksum']: # The checksums don't match and we will change or error out. - # Create a tmp path if missing only if this is not recursive. - # If this is recursive we already have a tmp path. - if delete_remote_tmp: - if tmp is None or "-tmp-" not in tmp: - tmp = self._make_tmp_path() - if self._play_context.diff and not raw: result['diff'].append(self._get_diff_data(dest_file, source_full, task_vars)) if self._play_context.check_mode: self._remove_tempfile_if_content_defined(content, content_tempfile) - module_return = dict(changed=True) - return module_return + result['changed'] = True + return result # Define a remote directory that we will copy the file to. tmp_src = self._connection._shell.join_path(tmp, 'source') @@ -321,7 +321,7 @@ class ActionModule(ActionBase): # If checksums match, and follow = True, find out if 'dest' is a link. If so, # change it to point to the source of the link. if follow: - dest_status_nofollow = self._execute_remote_stat(dest_file, all_vars=task_vars, follow=False) + dest_status_nofollow = self._execute_remote_stat(dest_file, all_vars=task_vars, tmp=tmp, follow=False) if dest_status_nofollow['islnk'] and 'lnk_source' in dest_status_nofollow.keys(): dest = dest_status_nofollow['lnk_source'] @@ -345,7 +345,8 @@ class ActionModule(ActionBase): if not module_return.get('checksum'): module_return['checksum'] = local_checksum - return module_return + result.update(module_return) + return result def _get_file_args(self): new_module_args = {'recurse': False} diff --git a/lib/ansible/plugins/action/template.py b/lib/ansible/plugins/action/template.py index 33aa580dceb..e0674b1569e 100644 --- a/lib/ansible/plugins/action/template.py +++ b/lib/ansible/plugins/action/template.py @@ -18,10 +18,12 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type import os +import shutil +import tempfile from ansible import constants as C from ansible.errors import AnsibleError, AnsibleFileNotFound -from ansible.module_utils._text import to_text +from ansible.module_utils._text import to_bytes, to_text from ansible.module_utils.parsing.convert_bool import boolean from ansible.plugins.action import ActionBase from ansible.template import generate_ansible_template_vars @@ -58,6 +60,7 @@ class ActionModule(ActionBase): source = self._task.args.get('src', None) dest = self._task.args.get('dest', None) force = boolean(self._task.args.get('force', True), strict=False) + follow = boolean(self._task.args.get('follow', False), strict=False) state = self._task.args.get('state', None) newline_sequence = self._task.args.get('newline_sequence', self.DEFAULT_NEWLINE_SEQUENCE) variable_start_string = self._task.args.get('variable_start_string', None) @@ -92,20 +95,6 @@ class ActionModule(ActionBase): if 'failed' in result: return result - # Expand any user home dir specification - dest = self._remote_expand_user(dest) - - directory_prepended = False - if dest.endswith(os.sep): - # Optimization. trailing slash means we know it's a directory - directory_prepended = True - dest = self._connection._shell.join_path(dest, os.path.basename(source)) - else: - # Find out if it's a directory - dest_stat = self._execute_remote_stat(dest, task_vars, True, tmp=tmp) - if dest_stat['exists'] and dest_stat['isdir']: - dest = self._connection._shell.join_path(dest, os.path.basename(source)) - # Get vault decrypted tmp file try: tmp_source = self._loader.get_real_file(source) @@ -160,70 +149,35 @@ class ActionModule(ActionBase): finally: self._loader.cleanup_tmp_file(tmp_source) - if not tmp: - tmp = self._make_tmp_path() - - local_checksum = checksum_s(resultant) - remote_checksum = self.get_checksum(dest, task_vars, not directory_prepended, source=source, tmp=tmp) - if isinstance(remote_checksum, dict): - # Error from remote_checksum is a dict. Valid return is a str - result.update(remote_checksum) - return result - - diff = {} - new_module_args = self._task.args.copy() - - # remove newline_sequence from standard arguments - new_module_args.pop('newline_sequence', None) - new_module_args.pop('block_start_string', None) - new_module_args.pop('block_end_string', None) - new_module_args.pop('variable_start_string', None) - new_module_args.pop('variable_end_string', None) - new_module_args.pop('trim_blocks', None) - - if (remote_checksum == '1') or (force and local_checksum != remote_checksum): - - result['changed'] = True - # if showing diffs, we need to get the remote value - if self._play_context.diff: - diff = self._get_diff_data(dest, resultant, task_vars, source_file=False) - - if not self._play_context.check_mode: # do actual work through copy - xfered = self._transfer_data(self._connection._shell.join_path(tmp, 'source'), resultant) - - # fix file permissions when the copy is done as a different user - self._fixup_perms2((tmp, xfered)) - - # run the copy module - new_module_args.update( - dict( - src=xfered, - dest=dest, - original_basename=os.path.basename(source), - follow=True, - ), - ) - result.update(self._execute_module(module_name='copy', module_args=new_module_args, task_vars=task_vars, tmp=tmp, delete_remote_tmp=False)) - - if result.get('changed', False) and self._play_context.diff: - result['diff'] = diff + new_task = self._task.copy() + new_task.args.pop('newline_sequence', None) + new_task.args.pop('block_start_string', None) + new_task.args.pop('block_end_string', None) + new_task.args.pop('variable_start_string', None) + new_task.args.pop('variable_end_string', None) + new_task.args.pop('trim_blocks', None) + try: + tempdir = tempfile.mkdtemp() + result_file = os.path.join(tempdir, os.path.basename(source)) + with open(result_file, 'wb') as f: + f.write(to_bytes(resultant, errors='surrogate_or_strict')) - else: - # when running the file module based on the template data, we do - # not want the source filename (the name of the template) to be used, - # since this would mess up links, so we clear the src param and tell - # the module to follow links. When doing that, we have to set - # original_basename to the template just in case the dest is - # a directory. - new_module_args.update( + new_task.args.update( dict( - src=None, - original_basename=os.path.basename(source), - follow=True, + src=result_file, + dest=dest, + follow=follow, ), ) - result.update(self._execute_module(module_name='file', module_args=new_module_args, task_vars=task_vars, tmp=tmp, delete_remote_tmp=False)) - - self._remove_tmp_path(tmp) + copy_action = self._shared_loader_obj.action_loader.get('copy', + task=new_task, + connection=self._connection, + play_context=self._play_context, + loader=self._loader, + templar=self._templar, + shared_loader_obj=self._shared_loader_obj) + result.update(copy_action.run(task_vars=task_vars)) + finally: + shutil.rmtree(tempdir) return result