Add preempt and cancel jobs on node for specified executor#4612
Add preempt and cancel jobs on node for specified executor#4612nikola-jokic merged 6 commits intomasterfrom
Conversation
b5a798b to
b084b17
Compare
39147de to
eda1819
Compare
| @@ -0,0 +1,117 @@ | |||
| package node | |||
There was a problem hiding this comment.
Ideally there would be some tests on this - as its our API
| PriorityClasses []string | ||
| } | ||
|
|
||
| type PreemptOnNode struct { |
There was a problem hiding this comment.
Can we double check there are no sensible tests we can add for these
Signed-off-by: Nikola Jokic <jokicnikola07@gmail.com>
e24f0af to
e524318
Compare
Signed-off-by: Nikola Jokic <jokicnikola07@gmail.com>
| WHERE jr.node = @node | ||
| AND jr.executor = @executor | ||
| AND j.queue = ANY(@queues::text[]) | ||
| AND jr.succeeded = false AND jr.failed = false AND jr.cancelled = false AND jr.preempted = false; |
There was a problem hiding this comment.
Instead of AND jr.succeeded = false AND jr.failed = false AND jr.cancelled = false AND jr.preempted = false we could add a terminated field which would be autogenerated from succeeded OR failed OR cancelled OR preempted. That way, an index will need to update only for 1 additional field instead of 4 additional fields and produce less index bloat.
Here is an example how we implemented it for the jobs table - https://github.com/armadaproject/armada/blob/master/internal/scheduler/database/migrations/027_add_terminated_column.sql
cc @masipauskas
There was a problem hiding this comment.
@JamesMurkin If we go with that, should we update other queries?
It makes sense to me to go with these changes, and the next PR would address jr.queue and termination.
There was a problem hiding this comment.
actually I require this new field for some other work so I'll create a PR which will add it in a migration
| ON jr.job_id = j.job_id | ||
| WHERE jr.node = @node | ||
| AND jr.executor = @executor | ||
| AND j.queue = ANY(@queues::text[]) |
There was a problem hiding this comment.
Also can you use jr.queue = ANY(@queues::text[]) so the filter can be done before the join, as that will speed up the query?
There was a problem hiding this comment.
Should we update the SelectJobsByExecutorAndQueues as well then? I based this query on that one
There was a problem hiding this comment.
I'd say just update the one you added, I can prepare a PR with the migration and for the other queries
Signed-off-by: Nikola Jokic <jokicnikola07@gmail.com>
<!-- Thanks for sending a pull request! Here are some tips for you: --> #### What type of PR is this? Enhancement #### What this PR does / why we need it jr.queue filtering should be faster: #4612 (comment) Signed-off-by: Nikola Jokic <jokicnikola07@gmail.com>
…ject#4612) <!-- Thanks for sending a pull request! Here are some tips for you: --> #### What type of PR is this? Enhancement #### What this PR does / why we need it Jobs may need to be preempted or canceled on a node. The server and `armadactl` should support these use cases natively. --------- Signed-off-by: Nikola Jokic <jokicnikola07@gmail.com> Signed-off-by: Sigele Nickerson-Adams <sigele.nickerson-adams@nmc2.ai>
<!-- Thanks for sending a pull request! Here are some tips for you: --> #### What type of PR is this? Enhancement #### What this PR does / why we need it jr.queue filtering should be faster: armadaproject#4612 (comment) Signed-off-by: Nikola Jokic <jokicnikola07@gmail.com> Signed-off-by: Sigele Nickerson-Adams <sigele.nickerson-adams@nmc2.ai>
What type of PR is this?
Enhancement
What this PR does / why we need it
Jobs may need to be preempted or canceled on a node. The server and
armadactlshould support these use cases natively.