-
Notifications
You must be signed in to change notification settings - Fork 23.8k
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
include_role handlers bug fix #26335
Conversation
The test
|
I can confirm that this fixes the test case #18411 |
include_role needs to ensure that any handlers included with the role are added to the _notified_handler and _listening_handler lists of the TaskQueueManager, otherwise it fails when trying to run the handler. Additionally, the handler needs to be added to the PlayIterator's `_uuid_cache` or it fails after running the handler Add more uuid debug statements - this code was hard to debug with existing debug statements, so add more uuid information at little additional output cost. Fixes ansible#18411
1f34ae6
to
c6f2455
Compare
Closing and reopening, don't understand the shippable failure (from run 27349) at all, suspect nothing to do with this change. |
Another shippable timeout (27356). Closing and reopening |
are these timeouts normal? maybe that should be fixed, instead of closing and reopening? |
@smemsh it's completely unrelated to this code (the two timeouts were in completely different systems). I could look into the timeouts, but I really spent far more time on this issue than I would have liked already. |
!needs_revision |
Candidate for cherry-picking onto stable-2.3 at the very least (there is one conflict but it's a fairly minor one) |
LGTM |
Merged. |
@willthames if we cherry-pick this to stable-2.3, it will need to go in the holding branch. I'm actually overdue for releasing 2.3.3 and need to do it this week, so this won't have time to burn in there. |
* Ensure that include_role properly fires handlers include_role needs to ensure that any handlers included with the role are added to the _notified_handler and _listening_handler lists of the TaskQueueManager, otherwise it fails when trying to run the handler. Additionally, the handler needs to be added to the PlayIterator's `_uuid_cache` or it fails after running the handler Add more uuid debug statements - this code was hard to debug with existing debug statements, so add more uuid information at little additional output cost. Fixes #18411 * Add tests for include_role handlers Tests for #18411 (cherry picked from commit ef8c979)
SUMMARY
Ensure handlers fire correctly when using include_role
When including roles, the handlers need to be added to the TaskQueueManager
_notified_handlers
andlistening_handlers
lists. They also need to be added to the uuid cacheA separate commit adds tests for this that fail before the change and succeed after the change.
Fixes #18411
ISSUE TYPE
COMPONENT NAME
include_role
ANSIBLE VERSION
ADDITIONAL INFORMATION
In addition to the KeyError issue associated with #18411, if you fix that by updating
_notified_handlers
and_listening_handlers
, and add some of the extra uuid debug statements in this PR, you get this instead:The above is because the handler that just ran (9cb6d012-9901-7bcf-cf94-000000000024) is not in the uuid cache.