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

RFC: Re-order executions in waiting queue #324

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rjalander
Copy link
Contributor

RFC proposal for implementing Re-order executions in waiting queue,
spinnaker/spinnaker#6735

Copy link
Member

@jasonmcintosh jasonmcintosh left a comment

Choose a reason for hiding this comment

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

SO overall, no reason COULDN'T do this - mostly I wonder about the interest in this feature? E.g. how common or how many people really want this - would love to hear from others on these kinds of things ;). AND a few big caution flags on re-ordering pipelines mostly around different times of artifact handling issues.

_New Orca API endpoint will be created to access the same way_
>>:8083/pipelines/<execution_id>/reorder?reorderAction=<UP/Down/Top/Bottom>

_The main business logic of the feature lies in Spin Orca service, where the **CompoundExecutionOperator** will update the Redis repo using the existing class **RedisExecutionRepository** and update the pending executions to the user selected position in the Redis repo using the Class **RedisPendingExecutionService**._
Copy link
Member

Choose a reason for hiding this comment

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

Note, would need to handle SQL as well if this moved forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing this, Jason. Do we need this to be implemented for DualPendingExecutionService and InMemoryPendingExecutionService too.

## Drawbacks

_Currently there is no Re-order functionality of an execution_
_Why should we **not** do this?_
Copy link
Member

Choose a reason for hiding this comment

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

Some things that OFTEN hit:

  • How do you know what the order SHOULD be - by doing it in FIFO you handle SHA's which may not have a particular order, but you REALLY want the latest kinda thing.
  • E.g. Git triggers - it's possible that re-ordering would cause incorrect sha pulls on artifacts. SO there IS a question in my mind of whether this should by default be visible? It seems... rather specific to a very specific set of cases. Docker triggers would be similar where unintentionally you could deploy older docker images.

Copy link

Choose a reason for hiding this comment

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

The pipeline design will have significant bearing on usage of triggers in the delivery workflow. If the Git commit id from the trigger is used in the pipeline and not head of the branch and uses the state of pipeline context then it is possible to have two pipeline executions deliver correctly even if order is changed. However, it makes sense that one would want the latest.

There are couple of possibilities for reordering executions of the same pipeline

  • The pipeline is used by triggers from multiple branches and build from main branch needs to be prioritized in the queue while others need to execute as well
  • The pipeline is triggered by commits in same branch and multiple commits in the queue while the latest needs to be prioritized while others can be discarded

The first should use a different pipeline design and the second one should discard older executions in the queue as execution in different order can cause problems with identifying latest. This could then be an option of one execution at a time while discarding older execution in queue that has not yet started.

Copy link

Choose a reason for hiding this comment

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

Hi,
Our Spinnaker users have similar request to re-order the waiting executions. But I think one of the most important reason is the FIFO is not working as expected in certain situations. I used to submit a ticket for this issue spinnaker/spinnaker#6373.

Can we consider to fix it first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with FIFO is not working is fixed with this PR spinnaker/orca#4356 and pending for review.

There are some users still they need to re-order the waiting executions even the FIFO is working as expected, for some release purpose or pipeline urgencies.

Choose a reason for hiding this comment

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

Thanks @rjalander . Then it would be nice to also include this re-order feature . Our users will be happy to have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One another scenario where Spinnaker users want to Re-order the waiting execution of same pipelines is,

  • When the Pipeline has waiting executions from different source of triggers and If we want to prioritize the specific source of trigger to validate on priority for any business needs.

Copy link
Member

Choose a reason for hiding this comment

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

DEFINITELY want the FIFO fix in first, then... more discussion on impacts/changes required for this, which aren't in my mind exactly "small" at the moment. And this is MOSTLY tied to trigger impacts on executions and what data is pulled/used for triggers...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FIFO defect is fixed

And we have done initial changes to bring this feature in and demoed to our internal Spinnaker users.

The changes are in this private branch - spinnaker/orca@master...rjalander:orca:spin-issue-6735, to reorder the executions with RedisPendingExecutionService.kt implementation.

And the CLI changes are in private branch - spinnaker/spin@master...rjalander:spin:spin-issue-6735-reorder, to initiate the reorder using CLI command.

Can we discuss more on impacts of this feature, to get this going.

_Currently there is no Re-order functionality of an execution_
_Why should we **not** do this?_

## Prior Art and Alternatives
Copy link

Choose a reason for hiding this comment

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

Have you looked at the orca-qos API? There may be a way to extend that to implement this functionality.

@xibz
Copy link
Contributor

xibz commented Feb 24, 2023

I want to make sure I am understanding this right, @rjalander. So please fill me in any missed details.

So reading spinnaker/spinnaker#6735, you have n parent pipelines that funnel into this single pipeline, but need to be re-prioritized. To do this today, you'd need to either cancel all other executions or pause all other running executions. Wouldn't it make more sense to have the priority be sent from the parent pipeline as the caller? My concern with a UI up and down arrow approach is mostly around multiple users potentially clicking. WIth this approach a parent pipeline can send a priority which then can move the execution up or down the queue, eg a max/min-heap instead of a fifo queue. Only concern here is starvation, but we can take a look at that more deeply once we way the options and this is even considered

@rjalander
Copy link
Contributor Author

Yes as per spinnaker/spinnaker#6735, we have "n" parent pipelines that are configured as Automated triggers in another Pipeline to run when each parent pipeline is successful.

And this Pipeline with triggers is currently running with FIFO and all other executions are waiting in queue.
At this stage user wants to re-order the waiting executions as per needs, but not sure If Parent pipeline can decide on which priority the waiting execution should run at this stage.
Please correct me If I am wrong.

@rjalander
Copy link
Contributor Author

@jasonmcintosh @xibz wanted to bring this discussion up, can we have a look at this feature again and bring RFC to the next stage.

@jimmycasey
Copy link

Yes as per spinnaker/spinnaker#6735, we have "n" parent pipelines that are configured as Automated triggers in another Pipeline to run when each parent pipeline is successful.

And this Pipeline with triggers is currently running with FIFO and all other executions are waiting in queue. At this stage user wants to re-order the waiting executions as per needs, but not sure If Parent pipeline can decide on which priority the waiting execution should run at this stage. Please correct me If I am wrong.

This WoW make sense for us also where we know priority of item from the start and when this high priority item arrives, we want it to be executed next (put to top of queue).

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

Successfully merging this pull request may close these issues.

7 participants