Cancel running jobs by event rather than previous commit SHA#3076
Cancel running jobs by event rather than previous commit SHA#3076nforro wants to merge 1 commit intopackit:mainfrom
Conversation
Some triggers such as comment commands have no relation to a commit SHA, so canceling doesn't work for them. We can use project event instead to identify running jobs that should be canceled, but event IDs are reused and we need to exclude current jobs, i.e. those that have been started by our trigger. To make that possible, before starting any tasks for a job record datetime of the most recent pipeline of the triggering event and use that as a cutoff time when filtering running jobs to cancel. Signed-off-by: Nikola Forró <nforro@redhat.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a cancel_cutoff_time mechanism to prevent accidental cancellation of newly triggered jobs within the same event batch. This involves adding cancel_cutoff_time to Event and EventData classes, and modifying get_running methods across various build and test models (Copr, Koji, Testing Farm, Log Detective) to filter jobs based on this new timestamp and project_event_type/event_id instead of commit_sha. A new get_latest_datetime_for_event method was added to PipelineModel to determine this cutoff time. Review comments highlight the need for robust checks to ensure cancel_cutoff_time and db_project_event are not None to prevent unintended job cancellations and to improve docstring clarity for the created_before parameter in get_running methods.
| yield from CoprBuildGroupModel.get_running( | ||
| project_event_type=self.db_project_event.type, | ||
| event_id=self.db_project_event.event_id, | ||
| created_before=self.metadata.cancel_cutoff_time, | ||
| ) |
There was a problem hiding this comment.
It's important to ensure that self.db_project_event and self.metadata.cancel_cutoff_time are always properly populated. If these values are None, the query will return all running jobs for the given project event, potentially canceling unrelated jobs. Add a check to ensure these values are not None and log a warning if they are.
| @classmethod | ||
| def get_running(cls, commit_sha: str) -> Iterable["CoprBuildTargetModel"]: | ||
| """Get list of currently running Copr builds matching the passed | ||
| arguments. | ||
| def get_running( | ||
| cls, | ||
| project_event_type: ProjectEventModelType, | ||
| event_id: int, | ||
| created_before: Optional[datetime] = None, | ||
| ) -> Iterable["CoprBuildTargetModel"]: |
There was a problem hiding this comment.
The docstring should mention that created_before is inclusive, i.e., 'at or before'. This clarifies the behavior for users of this method.
| if self.event.db_project_event: | ||
| self.event.cancel_cutoff_time = PipelineModel.get_latest_datetime_for_event( | ||
| project_event_type=self.event.db_project_event.type, | ||
| event_id=self.event.db_project_event.event_id, | ||
| ) |
There was a problem hiding this comment.
Consider adding a check to ensure that self.event.db_project_event is not None before attempting to retrieve the cancel_cutoff_time. If it's None, it might indicate an issue with the event processing, and logging a warning would be beneficial.
|
✔️ pre-commit SUCCESS in 1m 56s |
2 participants
Some triggers such as comment commands have no relation to a commit SHA, so canceling doesn't work for them. We can use project event instead to identify running jobs that should be canceled, but event IDs are reused and we need to exclude current jobs, i.e. those that have been started by our trigger. To make that possible, before starting any tasks for a job record datetime of the most recent pipeline of the triggering event and use that as a cutoff time when filtering running jobs to cancel.
TODO:
Related to #3005.
RELEASE NOTES BEGIN
Packit now cancels running jobs even after manual retriggering.
RELEASE NOTES END