From 652cd6cd5e60879cac3e74088930de1fc603cdda Mon Sep 17 00:00:00 2001 From: Jesse Rusak Date: Sat, 4 Apr 2015 16:37:14 -0400 Subject: [PATCH] Fix --force-handlers, and allow it in plays and ansible.cfg The --force-handlers command line argument was not correctly running handlers on hosts which had tasks that later failed. This corrects that, and also allows you to specify force_handlers in ansible.cfg or in a play. --- bin/ansible-playbook | 3 +- docsite/rst/intro_configuration.rst | 14 ++++++++++ docsite/rst/playbooks_error_handling.rst | 20 +++++++++++++ lib/ansible/constants.py | 2 ++ lib/ansible/playbook/__init__.py | 17 +++++------ lib/ansible/playbook/play.py | 8 ++++-- test/integration/Makefile | 14 ++++++++++ .../test_force_handlers/handlers/main.yml | 2 ++ .../roles/test_force_handlers/tasks/main.yml | 26 +++++++++++++++++ test/integration/test_force_handlers.yml | 28 +++++++++++++++++++ test/units/TestPlayVarsFiles.py | 1 + 11 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 test/integration/roles/test_force_handlers/handlers/main.yml create mode 100644 test/integration/roles/test_force_handlers/tasks/main.yml create mode 100644 test/integration/test_force_handlers.yml diff --git a/bin/ansible-playbook b/bin/ansible-playbook index 118a0198e42..3d6e1f9f402 100755 --- a/bin/ansible-playbook +++ b/bin/ansible-playbook @@ -97,7 +97,8 @@ def main(args): help="one-step-at-a-time: confirm each task before running") parser.add_option('--start-at-task', dest='start_at', help="start the playbook at the task matching this name") - parser.add_option('--force-handlers', dest='force_handlers', action='store_true', + parser.add_option('--force-handlers', dest='force_handlers', + default=C.DEFAULT_FORCE_HANDLERS, action='store_true', help="run handlers even if a task fails") parser.add_option('--flush-cache', dest='flush_cache', action='store_true', help="clear the fact cache") diff --git a/docsite/rst/intro_configuration.rst b/docsite/rst/intro_configuration.rst index 4cb1f359948..a13f6c6ecd9 100644 --- a/docsite/rst/intro_configuration.rst +++ b/docsite/rst/intro_configuration.rst @@ -252,6 +252,20 @@ This options forces color mode even when running without a TTY:: force_color = 1 +.. _force_handlers: + +force_handlers +============== + +.. versionadded:: 1.9.1 + +This option causes notified handlers to run on a host even if a failure occurs on that host:: + + force_handlers = True + +The default is False, meaning that handlers will not run if a failure has occurred on a host. +This can also be set per play or on the command line. See :doc:`_handlers_and_failure` for more details. + .. _forks: forks diff --git a/docsite/rst/playbooks_error_handling.rst b/docsite/rst/playbooks_error_handling.rst index 98ffb2860f3..ac573d86ba6 100644 --- a/docsite/rst/playbooks_error_handling.rst +++ b/docsite/rst/playbooks_error_handling.rst @@ -29,6 +29,26 @@ write a task that looks like this:: Note that the above system only governs the failure of the particular task, so if you have an undefined variable used, it will still raise an error that users will need to address. +.. _handlers_and_failure: + +Handlers and Failure +```````````````````` + +.. versionadded:: 1.9.1 + +When a task fails on a host, handlers which were previously notified +will *not* be run on that host. This can lead to cases where an unrelated failure +can leave a host in an unexpected state. For example, a task could update +a configuration file and notify a handler to restart some service. If a +task later on in the same play fails, the service will not be restarted despite +the configuration change. + +You can change this behavior with the ``--force-handlers`` command-line option, +or by including ``force_handlers: True`` in a play, or ``force_handlers = True`` +in ansible.cfg. When handlers are forced, they will run when notified even +if a task fails on that host. (Note that certain errors could still prevent +the handler from running, such as a host becoming unreachable.) + .. _controlling_what_defines_failure: Controlling What Defines Failure diff --git a/lib/ansible/constants.py b/lib/ansible/constants.py index 71efefdbc38..089de5b7c5b 100644 --- a/lib/ansible/constants.py +++ b/lib/ansible/constants.py @@ -173,6 +173,8 @@ DEPRECATION_WARNINGS = get_config(p, DEFAULTS, 'deprecation_warnings', DEFAULT_CALLABLE_WHITELIST = get_config(p, DEFAULTS, 'callable_whitelist', 'ANSIBLE_CALLABLE_WHITELIST', [], islist=True) COMMAND_WARNINGS = get_config(p, DEFAULTS, 'command_warnings', 'ANSIBLE_COMMAND_WARNINGS', False, boolean=True) DEFAULT_LOAD_CALLBACK_PLUGINS = get_config(p, DEFAULTS, 'bin_ansible_callbacks', 'ANSIBLE_LOAD_CALLBACK_PLUGINS', False, boolean=True) +DEFAULT_FORCE_HANDLERS = get_config(p, DEFAULTS, 'force_handlers', 'ANSIBLE_FORCE_HANDLERS', False, boolean=True) + RETRY_FILES_ENABLED = get_config(p, DEFAULTS, 'retry_files_enabled', 'ANSIBLE_RETRY_FILES_ENABLED', True, boolean=True) RETRY_FILES_SAVE_PATH = get_config(p, DEFAULTS, 'retry_files_save_path', 'ANSIBLE_RETRY_FILES_SAVE_PATH', '~/') diff --git a/lib/ansible/playbook/__init__.py b/lib/ansible/playbook/__init__.py index d58657012c6..93804d123c8 100644 --- a/lib/ansible/playbook/__init__.py +++ b/lib/ansible/playbook/__init__.py @@ -375,17 +375,17 @@ class PlayBook(object): # ***************************************************** - def _trim_unavailable_hosts(self, hostlist=[]): + def _trim_unavailable_hosts(self, hostlist=[], keep_failed=False): ''' returns a list of hosts that haven't failed and aren't dark ''' - return [ h for h in hostlist if (h not in self.stats.failures) and (h not in self.stats.dark)] + return [ h for h in hostlist if (keep_failed or h not in self.stats.failures) and (h not in self.stats.dark)] # ***************************************************** - def _run_task_internal(self, task): + def _run_task_internal(self, task, include_failed=False): ''' run a particular module step in a playbook ''' - hosts = self._trim_unavailable_hosts(self.inventory.list_hosts(task.play._play_hosts)) + hosts = self._trim_unavailable_hosts(self.inventory.list_hosts(task.play._play_hosts), keep_failed=include_failed) self.inventory.restrict_to(hosts) runner = ansible.runner.Runner( @@ -493,7 +493,8 @@ class PlayBook(object): task.ignore_errors = utils.check_conditional(cond, play.basedir, task.module_vars, fail_on_undefined=C.DEFAULT_UNDEFINED_VAR_BEHAVIOR) # load up an appropriate ansible runner to run the task in parallel - results = self._run_task_internal(task) + include_failed = is_handler and play.force_handlers + results = self._run_task_internal(task, include_failed=include_failed) # if no hosts are matched, carry on hosts_remaining = True @@ -811,7 +812,7 @@ class PlayBook(object): # if no hosts remain, drop out if not host_list: - if self.force_handlers: + if play.force_handlers: task_errors = True break else: @@ -821,7 +822,7 @@ class PlayBook(object): # lift restrictions after each play finishes self.inventory.lift_also_restriction() - if task_errors and not self.force_handlers: + if task_errors and not play.force_handlers: # if there were failed tasks and handler execution # is not forced, quit the play with an error return False @@ -856,7 +857,7 @@ class PlayBook(object): play.max_fail_pct = 0 if (hosts_count - len(host_list)) > int((play.max_fail_pct)/100.0 * hosts_count): host_list = None - if not host_list and not self.force_handlers: + if not host_list and not play.force_handlers: self.callbacks.on_no_hosts_remaining() return False diff --git a/lib/ansible/playbook/play.py b/lib/ansible/playbook/play.py index 78f2f6d9ba8..9fd8a86f4e4 100644 --- a/lib/ansible/playbook/play.py +++ b/lib/ansible/playbook/play.py @@ -34,9 +34,10 @@ class Play(object): _pb_common = [ 'accelerate', 'accelerate_ipv6', 'accelerate_port', 'any_errors_fatal', 'become', - 'become_method', 'become_user', 'environment', 'gather_facts', 'handlers', 'hosts', - 'name', 'no_log', 'remote_user', 'roles', 'serial', 'su', 'su_user', 'sudo', - 'sudo_user', 'tags', 'vars', 'vars_files', 'vars_prompt', 'vault_password', + 'become_method', 'become_user', 'environment', 'force_handlers', 'gather_facts', + 'handlers', 'hosts', 'name', 'no_log', 'remote_user', 'roles', 'serial', 'su', + 'su_user', 'sudo', 'sudo_user', 'tags', 'vars', 'vars_files', 'vars_prompt', + 'vault_password', ] __slots__ = _pb_common + [ @@ -153,6 +154,7 @@ class Play(object): self.accelerate_ipv6 = ds.get('accelerate_ipv6', False) self.max_fail_pct = int(ds.get('max_fail_percentage', 100)) self.no_log = utils.boolean(ds.get('no_log', 'false')) + self.force_handlers = utils.boolean(ds.get('force_handlers', self.playbook.force_handlers)) # Fail out if user specifies conflicting privelege escalations if (ds.get('become') or ds.get('become_user')) and (ds.get('sudo') or ds.get('sudo_user')): diff --git a/test/integration/Makefile b/test/integration/Makefile index ac526cf752e..6e2acec341d 100644 --- a/test/integration/Makefile +++ b/test/integration/Makefile @@ -56,6 +56,20 @@ test_group_by: test_handlers: ansible-playbook test_handlers.yml -i inventory.handlers -e @$(VARS_FILE) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) + # Not forcing, should only run on successful host + [ "$$(ansible-playbook test_force_handlers.yml --tags normal -i inventory.handlers -e @$(VARS_FILE) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) | egrep -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_B" ] + # Forcing from command line + [ "$$(ansible-playbook test_force_handlers.yml --tags normal -i inventory.handlers --force-handlers -e @$(VARS_FILE) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) | egrep -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ] + # Forcing from command line, should only run later tasks on unfailed hosts + [ "$$(ansible-playbook test_force_handlers.yml --tags normal -i inventory.handlers --force-handlers -e @$(VARS_FILE) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) | egrep -o CALLED_TASK_. | sort | uniq | xargs)" = "CALLED_TASK_B CALLED_TASK_D CALLED_TASK_E" ] + # Forcing from command line, should call handlers even if all hosts fail + [ "$$(ansible-playbook test_force_handlers.yml --tags normal -i inventory.handlers --force-handlers -e @$(VARS_FILE) $(CREDENTIALS_ARG) -v -e fail_all=yes $(TEST_FLAGS) | egrep -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ] + # Forcing from ansible.cfg + [ "$$(ANSIBLE_FORCE_HANDLERS=true ansible-playbook --tags normal test_force_handlers.yml -i inventory.handlers -e @$(VARS_FILE) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) | egrep -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ] + # Forcing true in play + [ "$$(ansible-playbook test_force_handlers.yml --tags force_true_in_play -i inventory.handlers -e @$(VARS_FILE) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) | egrep -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_A CALLED_HANDLER_B" ] + # Forcing false in play, which overrides command line + [ "$$(ansible-playbook test_force_handlers.yml --force-handlers --tags force_false_in_play -i inventory.handlers -e @$(VARS_FILE) $(CREDENTIALS_ARG) -v $(TEST_FLAGS) | egrep -o CALLED_HANDLER_. | sort | uniq | xargs)" = "CALLED_HANDLER_B" ] test_hash: ANSIBLE_HASH_BEHAVIOUR=replace ansible-playbook test_hash.yml -i $(INVENTORY) $(CREDENTIALS_ARG) -v -e '{"test_hash":{"extra_args":"this is an extra arg"}}' diff --git a/test/integration/roles/test_force_handlers/handlers/main.yml b/test/integration/roles/test_force_handlers/handlers/main.yml new file mode 100644 index 00000000000..2cfb1ef7109 --- /dev/null +++ b/test/integration/roles/test_force_handlers/handlers/main.yml @@ -0,0 +1,2 @@ +- name: echoing handler + command: echo CALLED_HANDLER_{{ inventory_hostname }} \ No newline at end of file diff --git a/test/integration/roles/test_force_handlers/tasks/main.yml b/test/integration/roles/test_force_handlers/tasks/main.yml new file mode 100644 index 00000000000..a3948756d71 --- /dev/null +++ b/test/integration/roles/test_force_handlers/tasks/main.yml @@ -0,0 +1,26 @@ +--- + +# We notify for A and B, and hosts B and C fail. +# When forcing, we expect A and B to run handlers +# When not forcing, we expect only B to run handlers + +- name: notify the handler for host A and B + shell: echo + notify: + - echoing handler + when: inventory_hostname == 'A' or inventory_hostname == 'B' + +- name: fail task for all + fail: msg="Fail All" + when: fail_all is defined and fail_all + +- name: fail task for A + fail: msg="Fail A" + when: inventory_hostname == 'A' + +- name: fail task for C + fail: msg="Fail C" + when: inventory_hostname == 'C' + +- name: echo after A and C have failed + command: echo CALLED_TASK_{{ inventory_hostname }} \ No newline at end of file diff --git a/test/integration/test_force_handlers.yml b/test/integration/test_force_handlers.yml new file mode 100644 index 00000000000..a700da08f0b --- /dev/null +++ b/test/integration/test_force_handlers.yml @@ -0,0 +1,28 @@ +--- + +- name: test force handlers (default) + tags: normal + hosts: testgroup + gather_facts: False + connection: local + roles: + - { role: test_force_handlers } + +- name: test force handlers (set to true) + tags: force_true_in_play + hosts: testgroup + gather_facts: False + connection: local + force_handlers: True + roles: + - { role: test_force_handlers } + + +- name: test force handlers (set to false) + tags: force_false_in_play + hosts: testgroup + gather_facts: False + connection: local + force_handlers: False + roles: + - { role: test_force_handlers } diff --git a/test/units/TestPlayVarsFiles.py b/test/units/TestPlayVarsFiles.py index 497c3112ede..9d42b73e8b6 100644 --- a/test/units/TestPlayVarsFiles.py +++ b/test/units/TestPlayVarsFiles.py @@ -47,6 +47,7 @@ class FakePlayBook(object): self.transport = None self.only_tags = None self.skip_tags = None + self.force_handlers = None self.VARS_CACHE = {} self.SETUP_CACHE = {} self.inventory = FakeInventory()