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.

11 files changed, 123 insertions, 12 deletions

diff --git a/bin/ansible-playbook b/bin/ansible-playbook
index 118a0198e4..3d6e1f9f40 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 4cb1f35994..a13f6c6ecd 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 98ffb2860f..ac573d86ba 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 71efefdbc3..089de5b7c5 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 d58657012c..93804d123c 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 78f2f6d9ba..9fd8a86f4e 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 ac526cf752..6e2acec341 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 0000000000..2cfb1ef710
--- /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 0000000000..a3948756d7
--- /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 0000000000..a700da08f0
--- /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 497c3112ed..9d42b73e8b 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()