refactor some callback receiver code #8236
Conversation
|
Build failed.
|
| for e in events: | ||
| try: | ||
| if ( | ||
| isinstance(exc, IntegrityError) and | ||
| getattr(e, 'host_id', '') |
There was a problem hiding this comment.
This foreign key constraint just doesn't exist anymore at all (go ahead and go check 😄 \d main_jobevent).
| def _send_notifications(): | ||
| handle_success_and_failure_notifications.apply_async([self.job.id]) | ||
| connection.on_commit(_send_notifications) | ||
| try: |
There was a problem hiding this comment.
The job_id constraint doesn't exist either, so it's totally possible to insert a playbook_on_stats for a job that no longer exists. When that's the case, this code will blow up. This is probably a little difficult to hit in practice, because the API actually prevents you from deleting a job that's still processing events.
There was a problem hiding this comment.
"the API actually prevents you from deleting a job that's still processing events, until 2 minutes after the job is finished" But yeah, it's still hard to hit.
There was a problem hiding this comment.
Yea, it can happen. Kind of hard to get here, but better to have this work properly 👍
| @@ -497,7 +503,11 @@ def _hostnames(self): | |||
|
|
|||
| def _update_host_summary_from_stats(self, hostnames): | |||
| with ignore_inventory_computed_fields(): | |||
| if not self.job or not self.job.inventory: | |||
| try: | |||
There was a problem hiding this comment.
This code will also blow up in the scenario described above.
|
recheck |
|
Build failed.
|
the bigint migration removed the foreign key constraints for: - host_id - job_id (and projectupdate_id, etc...) because of this, we don't really need to check explicitly for a host_id IntegrityError anymore (because it won't occur) additionally, while it's possible to insert an event with a mismatched job_id now (for example, you can totally start a long-running job, and delete the job record in the background using the ORM or psql), doing so results in DoesNotExist errors in the code that handles the playbook_on_stats events
ryanpetrello
force-pushed
the
more-callback-cleanup
branch
from
8e217b1
to
baad765
Compare
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
the bigint migration (#6032) removed the foreign key constraints for:
I'd like to say this was intentional, but I actually think is was an unexpected
side effect of my work that happened to work out in our favor.
because of this, we don't really need to check explicitly for a host_id
IntegrityError anymore (because it won't occur)
additionally, while it's possible to insert an event with a mismatched
job_id now (for example, you can totally start a long-running job, and
delete the job record in the background using the ORM or psql), doing
so results in DoesNotExist errors in the code that handles the
playbook_on_stats events
cc @gamuniz