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

Make sure to fire off failure notifications on error #11384

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

jbradberry
Copy link
Contributor

@jbradberry jbradberry commented Nov 23, 2021

SUMMARY

where the error is unrelated to Ansible, thus is not caught by the
usual methods.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • API
AWX VERSION

ADDITIONAL INFORMATION

where the error is unrelated to Ansible, thus is not caught by the
usual methods.
@jbradberry
Copy link
Contributor Author

@chrismeyersfsu The one thing I'm worried about in this PR is if there are circumstances where an error result can cause the notification templates fire off by this path and the more usual path. Any thoughts?

@chrismeyersfsu
Copy link
Member

That's a good thing to be worried about @jbradberry . Do we call handle_success_and_failure_notifications anywhere else?

@sarabrajsingh
Copy link
Contributor

That's a good thing to be worried about @jbradberry . Do we call handle_success_and_failure_notifications anywhere else?

@chrismeyersfsu the other calls to handle_success_and_failure_notifications are done in callback.py and events.py but aren't directly involved in firing off notifications when jobs fail. this is done at the tasks.py level

@AlanCoding
Copy link
Member

I didn't understand #8771 before, but I might be seeing it now.

Either way, this seems fairly correct on its own. The speculated problem here seems to be the mutual exclusivity of the error status and the EOF message. While it could possibly happen, that doesn't sound highly credible to me.

@jbradberry jbradberry merged commit b195f9d into ansible:devel Dec 15, 2021
@jbradberry jbradberry deleted the failure-notification-on-error branch December 15, 2021 18:47
self.instance.job_explanation = "Job terminated due to timeout"
status = 'failed'
if status in ('timeout', 'error'):
self.instance.job_explanation = f"Job terminated due to {status}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that we don't want job_explanation to read

Job terminated due to error

This is particularly bad in the cases where an error occurred and job_explanation was set to something in response to that. This code would overwrite that useful message with this vague one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants