Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

load handler only one time #53644

Closed
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@goneri
Copy link
Contributor

goneri commented Mar 11, 2019

Ensure we attach only one instance the handler per playbook.

SUMMARY

If we call include_role in a loop, _play.handler can end up with several entries of the same handler. As a result, the handler will be triggered more than just one time.
Ensure we don't stack more s

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

include_role

@@ -290,6 +290,10 @@ def get_vars_files(self):
return [self.vars_files]
return self.vars_files

def add_handler(self, handler):
if handler.name not in [h.name for h in self.handlers]:
self.handlers.append(handler)

This comment has been minimized.

@sivel

sivel Mar 11, 2019

Member

Technically speaking this is a change in functionality. We have always allowed duplicate handler names.

See #49249

This also would not allow 2 roles that have the same handler name, but can be addressed using the role name as a namespace:

role_name : handler name

Maybe it's simple enough to use .get_name() instead of .name to resolve this. I'm not sure yet.

See

def search_handler_blocks_by_name(handler_name, handler_blocks):
for where we do this lookup, and allow the get_name() version which supports namespacing.

This comment has been minimized.

@goneri

goneri Mar 11, 2019

Author Contributor

Thanks! I was indeed checking the wrong name. I just push a new version that should be better. Regarding the order, I don't think it's a problem since this is just the triggers. Not the list of handlers.

@goneri goneri force-pushed the goneri:issue/53518 branch 2 times, most recently from 234176a to 904d36b Mar 11, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 11, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/executor/task_queue_manager.py:191:33: undefined-variable Undefined variable 'hander'

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Mar 11, 2019

@goneri goneri force-pushed the goneri:issue/53518 branch from 904d36b to 2d624a6 Mar 12, 2019

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented Mar 12, 2019

not sure this is correct, you have many scenarios in which you rely on the behaviour, i.e dynamic roles with same name for handlers but they are actually executing different actions.

I suspect the original ticket is more of an issue of lack of documentation than actual bad behaviour

@goneri goneri force-pushed the goneri:issue/53518 branch from fab67e8 to a9c9e91 Mar 12, 2019

@goneri

This comment has been minimized.

Copy link
Contributor Author

goneri commented Mar 12, 2019

@bcoca, do you have an example of such scenario? Even if #53518 still has the need_triage_flag, #49371 is flagged as bug.

This patch does not change the way the handlers are triggered at the end of the play, it just ensures the "notify" will be recorded just one time (and so, the handler will be trigger just one time too).

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 12, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/executor/test_play_iterator.py:469:5: E303 too many blank lines (2)

click here for bot help

@goneri goneri force-pushed the goneri:issue/53518 branch from a9c9e91 to 309d065 Mar 12, 2019

@ansibot ansibot added core_review and removed needs_revision labels Mar 12, 2019

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented Mar 12, 2019

This code changes adding handlers to play, not the notification, which is already 'unique' each notify in a section only happens once per host. Though name handlers only execute one, but 'listen' handlers can execute many, in both cases, only one notification happens.

@goneri

This comment has been minimized.

Copy link
Contributor Author

goneri commented Mar 12, 2019

Thanks for the update.

This code changes adding handlers to play, not the notification, which is already 'unique' each notify in a section only happens once per host.

From what I can see, It's indeed unique for all the cases but for the import_role:

  1. For every import_role in the loop, Ansible calls get_block_list(), https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/strategy/free.py#L223-L227
  2. at the end of get_block_list() Ansible injects the handlers in the .handlers list.
    https://github.com/ansible/ansible/blob/devel/lib/ansible/playbook/role_include.py#L104-L109

With my test sample, I've got 2 handlers and I loop 3 times -> I end up with 6 entries in the handlers list.

An alternative fix:

  • is to adjust get_block_list() to be sure it does not actually modify the state of the play
  • push the new handlers after the get_block_list() (in strategy plugins)

Though name handlers only execute one, but 'listen' handlers can execute many, in both cases, only one notification happens.

Yes, I agree on this. This behaviour is preserved by the patch.

@@ -187,7 +187,8 @@ def run(self, play):

new_play = play.copy()
new_play.post_validate(templar)
new_play.handlers = new_play.compile_roles_handlers() + new_play.handlers
for handler in new_play.compile_roles_handlers():
new_play.add_handler(handler)

This comment has been minimized.

@mkrizek

mkrizek Mar 13, 2019

Contributor

Isn't this changing handlers order? Role handlers are currently prepended to the play handlers. This appends them, no?

This comment has been minimized.

@goneri

goneri Mar 13, 2019

Author Contributor

I think this line is actually useless: compile_roles_handlers() calls get_handler_blocks() 1, and get_handler_blockers() already refresh the handlers list 2.

So new_play.add_handler(handler) here loops on a list of handlers that are already in new_play.handlers.

@goneri goneri force-pushed the goneri:issue/53518 branch from e8ab2fc to 0c42380 Mar 15, 2019

@ansibot ansibot added the new_plugin label Mar 15, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 15, 2019

The test ansible-test sanity --test no-underscore-variable [explain] failed with 5 errors:

test/units/plugins/strategy/test_notify.py:106:15: use `dummy` instead of `_` for a variable name
test/units/plugins/strategy/test_notify.py:124:15: use `dummy` instead of `_` for a variable name
test/units/plugins/strategy/test_notify.py:150:15: use `dummy` instead of `_` for a variable name
test/units/plugins/strategy/test_notify.py:169:15: use `dummy` instead of `_` for a variable name
test/units/plugins/strategy/test_notify.py:188:15: use `dummy` instead of `_` for a variable name

The test ansible-test sanity --test pep8 [explain] failed with 5 errors:

test/units/plugins/strategy/test_notify.py:70:5: E306 expected 1 blank line before a nested definition, found 0
test/units/plugins/strategy/test_notify.py:83:9: E126 continuation line over-indented for hanging indent
test/units/plugins/strategy/test_notify.py:99:5: E121 continuation line under-indented for hanging indent
test/units/plugins/strategy/test_notify.py:101:1: E302 expected 2 blank lines, found 1
test/units/plugins/strategy/test_notify.py:155:1: E303 too many blank lines (3)

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Mar 15, 2019

@goneri goneri force-pushed the goneri:issue/53518 branch from 0c42380 to 7eae84f Mar 15, 2019

playbook_file = setup_test_env(test_case)
handlers, results = run_playbook(playbook_file, strategy_class, monkeypatch)
assert handlers == ['with_handler : myhandler']
assert results[-1]['msg'] == 'first win'

This comment has been minimized.

@mkrizek

mkrizek Mar 18, 2019

Contributor

This is inconsistent, in general we have "last loaded wins" but in role's handlers we do the opposite?

This comment has been minimized.

@goneri

goneri Mar 18, 2019

Author Contributor

@mkczuk , my patch didn't change anything. This test passes with the devel branch and I just reproduced the behaviour with the v2.7.9 version.

This comment has been minimized.

@mkrizek

mkrizek Mar 18, 2019

Contributor

I know, was just wondering if it's intended that it works that way.

This comment has been minimized.

@bcoca

bcoca Mar 18, 2019

Member

last loaded wins, but iirc, we reverse search on handlers list

This comment has been minimized.

@goneri

goneri Mar 18, 2019

Author Contributor

So:

  • if I've several handler files, the last comes first.
  • in an handler file, the handler comes first.

I just added this test. The "listen" handlers come first.
354c38a

@goneri goneri force-pushed the goneri:issue/53518 branch from 7eae84f to 39392f5 Mar 18, 2019

@ansibot ansibot added core_review and removed needs_revision labels Mar 18, 2019

@goneri

This comment has been minimized.

Copy link
Contributor Author

goneri commented Mar 18, 2019

@sivel, @bcoca, @mkrizek is this PR good for you or do you need some extra changes?

@bcoca
Copy link
Member

bcoca left a comment

@goneri i still think this PR is wrong and its not handling the 'single notify' issue it claims to do, it tries to avoid adding handlers .. which I don't think is correct as we want any mutated handler to override it's previous incarnation

@ansibot ansibot added needs_revision and removed core_review labels Mar 18, 2019

@goneri

This comment has been minimized.

Copy link
Contributor Author

goneri commented Mar 18, 2019

What is a mutated handler?

@sivel

This comment has been minimized.

Copy link
Member

sivel commented Mar 18, 2019

This PR is attempting to de-dupe handlers by name alone when registering. This doesn't actually fix #53518

The reason is that the handlers name is the same, but there are actually 2 handlers, with the same name, but a different item variable context.

The change in this PR only allows test : h to be registered one time, with an item variable value of a.

The OP in #53518 wants to have both registered, but to execute only 1 based on that item context.

The solution here should not concern itself with how handlers are registered to the Play, but how StrategyBase._process_pending_results notifies handlers.

@goneri

This comment has been minimized.

Copy link
Contributor Author

goneri commented Mar 18, 2019

Thanks! I was indeed on a wrong path.

@goneri

This comment has been minimized.

Copy link
Contributor Author

goneri commented Mar 18, 2019

Thank you both, now I understand @bcoca concerns.

  • Some of the variables of the context change for every loop.

  • A handler may or may not makes use of them.

  • As a result, we don't have any reliable way to know if we can skip a handler task.

I will refresh my patch to only keep the unit-test.

test coverage for handlers
Include various tests for the task handlers.

@goneri goneri force-pushed the goneri:issue/53518 branch from 354c38a to 2f1f09c Mar 18, 2019

@goneri

This comment has been minimized.

Copy link
Contributor Author

goneri commented Mar 19, 2019

I close this PR. I created a new one with just the unit-test: #54031

@goneri goneri closed this Mar 19, 2019

@goneri goneri deleted the goneri:issue/53518 branch Mar 19, 2019

@sivel sivel removed the needs_triage label Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.