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

Propose queue changeTaskRunPriority #190

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lotas
Copy link
Contributor

@lotas lotas commented Apr 3, 2024

@lotas lotas requested a review from a team as a code owner April 3, 2024 11:11
@lotas lotas requested review from petemoore and matt-boris and removed request for a team April 3, 2024 11:11
Comment on lines 37 to 38
* `queue:change-task-run-priority-in-queue:<taskQueueId>`
* `queue:change-task-run-priority:<taskId>`
Copy link

Choose a reason for hiding this comment

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

I was wondering if there's a case to be made for the source and target priorities being included in the scopes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!
source <> target would be 8 to 8 permutations, maybe too much?

We can, however, add more generic ones like lowering or raising the priority, so two separate scopes?
queue:lower-task-run-priority:<taskId>
queue:raise-task-run-priority:<taskId>

@Archaeopteryx
Copy link

Have there been considerations to set this at the task graph level?

  • This should prioritize the whole graph and ensure its timely execution else potentially backlogged tasks need to be identified and configured manually.
  • A release graph can dependent on the tasks in a different one, e.g. the tasks scheduled automatically for a push can be dependencies of release graph created by the hook (fired by cron job or manually) to actually do a release (which has additional tasks).

@jcristau
Copy link

jcristau commented Apr 3, 2024

Have there been considerations to set this at the task graph level?

* This should prioritize the whole graph and ensure its timely execution else potentially backlogged tasks need to be identified and configured manually.

* A release graph can dependent on the tasks in a different one, e.g. the tasks scheduled automatically for a push can be dependencies of release graph created by the hook (fired by cron job or manually) to actually do a release (which has additional tasks).

I would argue that task (or run) is the right granularity for the queue service. If we need to do this for a whole task group that can likely be built on top of the fine-grained API.

@lotas lotas closed this Apr 3, 2024
@matt-boris matt-boris reopened this Apr 3, 2024
@lotas
Copy link
Contributor Author

lotas commented Apr 3, 2024

(sorry, pr was closed by accident)

Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

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

Writing down a few things we talked about synchronously:

  • If the run is where the adjusted priority is to be stored, we should do something to make sure reruns pick up the priority of the previous run (to make sure bumped priorities aren't lost on rerun).
  • Here are a few concrete use cases for this:
    • We have a high priority patch (eg: a potential fix for a chemspill bug) running tasks on Try. We want to bump the priority of all tasks in that task group.
    • We have a chemspill release or high priority nightly to get out. We want to bump the priority of all tasks in one or more task groups.
    • We are waiting for a specific task in a low resource pool (eg: mac hardware). We want to bump the priority of that specific task.

rfcs/0190-queue-change-task-run-priority.md Outdated Show resolved Hide resolved
rfcs/0190-queue-change-task-run-priority.md Outdated Show resolved Hide resolved
@lotas
Copy link
Contributor Author

lotas commented May 28, 2024

thanks @bhearsum for the summary

* If the `run` is where the adjusted priority is to be stored, we should do something to make sure reruns pick up the priority of the previous run (to make sure bumped priorities aren't lost on rerun).

indeed, priority on reruns should be "inherited"

* Here are a few concrete use cases for this:
  * We have a high priority patch (eg: a potential fix for a chemspill bug) running tasks on Try. We want to bump the priority of all tasks in that task group.
  * We have a chemspill release or high priority nightly to get out. We want to bump the priority of all tasks in one or more task groups.
  * We are waiting for a specific task in a low resource pool (eg: mac hardware). We want to bump the priority of that specific task.

So for the first two scenarios I wonder, why not simply create whole task group with the highest priority already, since it is known in advance this is very important? Or try pushes with raised priority is not possible?

For the third example, it's worth adding that if there are multiple tasks pending with same priority, we would need to lower the priority for the "not so important" ones, to let important one go sooner

@bhearsum
Copy link
Contributor

* Here are a few concrete use cases for this:
  * We have a high priority patch (eg: a potential fix for a chemspill bug) running tasks on Try. We want to bump the priority of all tasks in that task group.
  * We have a chemspill release or high priority nightly to get out. We want to bump the priority of all tasks in one or more task groups.
  * We are waiting for a specific task in a low resource pool (eg: mac hardware). We want to bump the priority of that specific task.

So for the first two scenarios I wonder, why not simply create whole task group with the highest priority already, since it is known in advance this is very important? Or try pushes with raised priority is not possible?

That's a good idea actually, yeah. We'd have to figure out how to wire that into UIs & CLIs, but that's a very tractable problem.

@lotas lotas force-pushed the feat/0190-change-task-run-priority branch from 914d497 to 24e74fa Compare May 28, 2024 13:17
@lotas
Copy link
Contributor Author

lotas commented May 28, 2024

@bhearsum @Archaeopteryx @jcristau I have reworked this document a bit, since it is clear that operating on the task group level is very important.
It was also missed out by me initially that tasks runs could fail and new runs wouldn't necessarily pick up same priority.

I'm changing this to work on both task and task group levels, and it would do "best effort" to change priority of scheduled and not yet scheduled tasks

rfcs/0190-queue-change-task-run-priority.md Outdated Show resolved Hide resolved
* `queue:change-task-priority-in-queue:<taskQueueId>`
* `queue:change-task-priority:<taskId>`
* `queue:lower-task-priority:<taskId>` to only allow lowering the priority
* `queue:raise-task-priority:<taskId>` to only allow raising the priority
Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to be certain if we'll use them, but I like this break out of raising/lowering - nice addition!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that I think about it.. maybe it's too dangerous. If someone can lower the priority of a task that means that high-priority tasks might be de-prioritized.

@jcristau suggested to include existing and new priority in the scope as well, maybe we should do that instead

queue:change-task-priority:<newPriority>:<existingPriority>:<taskId> // to allow easier wildcard scopes
queue:change-task-group-priority:<newPriority>/<taskGroupId>

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think either are fine, but it may affect who we grant these scopes to depending on the implementation.

rfcs/0190-queue-change-task-run-priority.md Outdated Show resolved Hide resolved
rfcs/0190-queue-change-task-run-priority.md Outdated Show resolved Hide resolved
@lotas lotas force-pushed the feat/0190-change-task-run-priority branch from d2d7718 to 34535c4 Compare June 4, 2024 09:35
@lotas
Copy link
Contributor Author

lotas commented Jun 4, 2024

I'm leaving audit side of this out of scope, since we don't have anything similar on the platform yet.

However, it would probably be necessary to leave traces in the form of publishing events.
I would probably add two new exchanges to notify whoever is interested in such changes


Following events would be emitted:

* `task-priority-changed` with `taskId`, `existingPriority` and `newPriority`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know other events do not include this, but maybe we should attach actor/user to those events?

Copy link
Member

@petemoore petemoore left a comment

Choose a reason for hiding this comment

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

Regarding scopes, if we can reuse existing scopes, to avoid privilege escalation, that would be good. In other words, in general, if your action has the effect of scheduling a task with a given priority, but you would normally not have the scopes required to execute a task in that queue with that same priority, this could be considered a form of privilege escalation. If execution with a given priority always requires a particular scope, they should be no way to subvert the system via a different path.

So e.g. how about in order to change priority of a task to priority <p> you need to have either scope (anyOf):

queue:create-task:<priority>:<taskQueueId>
queue:change-priority:<priority>:<taskQueueId>
queue:change-task-group-priority:<priority>:<schedulerId>/<taskGroupId>

I'm not too fond of a scope that just allows increasing or decreasing priority since there is no way to limit the priority to a particular value.

@bhearsum
Copy link
Contributor

Regarding scopes, if we can reuse existing scopes, to avoid privilege escalation, that would be good. In other words, in general, if your action has the effect of scheduling a task with a given priority, but you would normally not have the scopes required to execute a task in that queue with that same priority, this could be considered a form of privilege escalation. If execution with a given priority always requires a particular scope, they should be no way to subvert the system via a different path.

I see what you're getting at, but "privilege escalation" seems a bit extreme. With just these new scopes you cannot affect what runs, just when it runs. I suppose it's theoretically possible that this could be exploited, but it seems rather farfetched.

So e.g. how about in order to change priority of a task to priority <p> you need to have either scope (anyOf):

queue:create-task:<priority>:<taskQueueId> queue:change-priority:<priority>:<taskQueueId> queue:change-task-group-priority:<priority>:<schedulerId>/<taskGroupId>

I think is probably fine. The new change-priority scopes allow for addressing the use case of granting someone just the ability to change priorities if it comes up.

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.

6 participants