POC: Canceling concurrent PipelineRuns #5501
Conversation
This commit adds a proof of concept for "concurrency primitives" in Pipelines that will allow higher-level controllers to implement concurrency controls. It adds a new CRD, "ConcurrencyControl". PipelineRuns that should have their concurrency controlled should be created with a label referencing the ConcurrencyControl. The PipelineRun controller will create another label on the PipelineRun specifying its "concurrency key". Any PipelineRuns with the same concurrency key belong to the same concurrency group and use the same concurrency strategy. This solution doesn't require PipelineRuns to be created as Pending. The reason for this is that knative does not allow a reconciler to edit the spec of an object being reconciled. In other words, the PipelineRun controller cannot "start" Pending PipelineRuns after canceling other PipelineRuns in the same concurrency group. Instead, concurrency controls are just handled before execution starts. One drawback of this solution is that users can get around concurrency controls by editing the labels of the PipelineRun.
|
Skipping CI for Draft Pull Request. |
|
@lbernick: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
tekton-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
size/XL
The ReconcileKind interface does not update changes to the spec, but one can always make a call to update the status from within the ReconcileKind function. |
for sure! I mention this because I didn't want to "run" a pending pipelinerun by editing its status, because I thought it would be confusing/introduce bugs to have spec status = Pending but then have the PR actually executing. so I made this not dependent on creating pipelineruns as pending. |
| } | ||
| key := resources.ApplyParametersToConcurrency(ctx, cc, pr) | ||
| pr.Labels["tekton.dev/concurrency-key"] = key | ||
| if cc.Spec.Strategy != "Cancel" { |
There was a problem hiding this comment.
I'm assuming we'd create constants for supported strategies?
|
This feels like it's on the right track! |
tekton-robot
added
the
needs-rebase
|
@lbernick: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
I do like that the ConcurrencyControl CRD holds the strategy and "how" the key is computed.
What is "slightly" weird though, is the implicitness of how it is computed. With default value, we can probably get away with it, but it might lead to weird behavior from time to time, if, for example, a parameter is renamed and thus, doesn't map to the ConcurrencyControl anymore.
| params: | ||
| - name: param-1 | ||
| - name: param-2 |
There was a problem hiding this comment.
Those would be "populated" by the PipelineRun params ?
What happens if there is a mismatch between PipelineRun's params and that ConcurrencyControl params?
There was a problem hiding this comment.
Those would be "populated" by the
PipelineRunparams ?
yup!
What happens if there is a mismatch between
PipelineRun's params and thatConcurrencyControlparams?
We would probably fail the PipelineRun (not sure how a separate controller outside of pipelines would do that though) or cancel it.
I agree! Do you have any suggestions for what might work better? We could remove "key" and "params" entirely, but I think this will be challenging if we want to sort things into concurrency groups based on parameters from a trigger or workflow (which I think we'll want to do). |
|
Closing in favor of tektoncd/experimental#895 |
Changes
Alternative to tektoncd/triggers#1446.
This commit adds a proof of concept for "concurrency primitives" in Pipelines that will allow higher-level controllers to implement concurrency controls. It adds a new CRD, "ConcurrencyControl". PipelineRuns that should have their concurrency controlled should be created with a label referencing the ConcurrencyControl. The PipelineRun controller will create another label on the PipelineRun specifying its "concurrency key". Any PipelineRuns with the same concurrency key belong to the same concurrency group and use the same concurrency strategy.
This solution doesn't require PipelineRuns to be created as Pending. The reason for this is that knative does not allow a reconciler to edit the spec of an object being reconciled. In other words, the PipelineRun controller cannot "start" Pending PipelineRuns after canceling other PipelineRuns in the same concurrency group. Instead, concurrency controls are just handled before execution starts.
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes