Fix notification timing issue by sending in the latter of 2 events #12110
Conversation
AlanCoding
changed the title
AlanCoding
changed the title
awx/main/dispatch/worker/callback.py
Outdated
| EVENT_MAP = { | ||
| 'job_id': JobEvent, | ||
| 'ad_hoc_command_id': AdHocCommandEvent, | ||
| 'project_update_id': ProjectUpdateEvent, | ||
| 'inventory_update_id': InventoryUpdateEvent, | ||
| 'system_job_id': SystemJobEvent, | ||
| } |
There was a problem hiding this comment.
If you inverted this map could you get rid of your new for loop on 185-189?
awx/main/dispatch/worker/callback.py
Outdated
| if key in body: | ||
| job_identifier = body[key] | ||
| break | ||
|
|
||
| self.last_event = f'\n\t- {cls.__name__} for #{job_identifier} ({body.get("event", "")} {body.get("uuid", "")})' # noqa | ||
|
|
||
| trigger_event_type = 'playbook_on_stats' if cls in (JobEvent, ProjectUpdateEvent, AdHocCommandEvent) else 'EOF' |
There was a problem hiding this comment.
(assuming you can invert EVENT_MAP from previous comment)
What about changing event map to include a field for this and then just doing:
trigger_event_map = EVENT_MAP[cls]['event_type']
There was a problem hiding this comment.
I can dig that. Still need to consider the original loop that used event_map - if we made the values a dictionary, then that could look like
for cls, props in EVENT_MAP.items():
if props['job_ref'] in body:
job_identifier = body[props['job_ref']]
breakThere's a little bit of indirection going on there, but this data structure could be reused more broadly than what I have now. I would probably move it into another module.
There was a problem hiding this comment.
Pushed a commit addressing this.
There was a problem hiding this comment.
But I still missed this exact line, pushed another commit which should wrap it up.
|
All test results are looking good for this, so I'm pretty serious about it at this point. |
| # events are persisted _before_ the UJ.status | ||
| # changes from running -> successful | ||
| retries += 1 | ||
| time.sleep(1) |
|
Tried to write up some details if someone wants to read. https://gist.github.com/AlanCoding/f7ae3d10ef04692c254c17507ef16b59 |
awx/main/dispatch/worker/callback.py
Outdated
| @@ -113,6 +113,32 @@ def work_loop(self, *args, **kw): | |||
| signal.signal(signal.SIGUSR1, self.toggle_profiling) | |||
| return super(CallbackBrokerWorker, self).work_loop(*args, **kw) | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
I only see you calling this as a method on self, can we drop this decorator?
There was a problem hiding this comment.
Pushed a commit to try and address this comment. I get what you're saying, I was calling self.job_stats_wrapup which isn't great.
This reverts commit 3b8ae35.
AlanCoding
force-pushed
the
host_status_counts
branch
from
432dc25
to
04f86d6
Compare
…r of 2 events (ansible#12110)" This reverts some of commit 29d6084. The reversion concern the addition of the host status counts field that field is removed here and migrated away, replaced with different field
…r of 2 events (ansible#12110)" This reverts some of commit 29d6084. The reversion concern the addition of the host status counts field that field is removed here and migrated away, replaced with different field
…r of 2 events (ansible#12110)" This reverts some of commit 29d6084. The reversion concern the addition of the host status counts field that field is removed here and migrated away, replaced with different field
SUMMARY
Connect #11422 as the primary issue for this.
As a secondary issue, I would suggest we close #8771 if we merge this. This PR adds the basic conflict management between event processing and status change. This does not attempt to assure that all events are processed, only that the "stats" event in processed, which is the "playbook_on_stats" event for playbooks, and EOF for other types.
See prior work at https://github.com/ansible/awx/compare/devel...chrismeyersfsu:job_done_done?expand=1 --> this tried to tackle the problem of a "job done" hook in conjunction with making the events queue durable. I'm not attacking the problem of event queue durability here.
I first implemented this fix in a different way here --> https://github.com/ansible/awx/compare/devel...AlanCoding:send_notifications?expand=1, which added a
notifications_processedfield, but that was probably poorly named. You could do what I'm doing here with a boolean likestats_event_processed, but that feels like it has no connection to other use cases, and would be useless for any other than this very narrow problem.ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
WIP upon opening because: