[TEP-0120] Add proposal for concurrency controls

[lbernick]

This commit adds design details for canceling concurrent PipelineRuns.

/kind tep

[lbernick]

I really tried to get this solution working but I struggled to do so in a way that wasn't super complex-- this is my best attempt at explaining why.

[lbernick]

@dibyom @vdemeester @williamlfish @RafaeLeal @jerop PTAL

[lbernick]

Proof of concept: tektoncd/triggers#1446

[vdemeester]

I think there is a "slight" difference of approach in what I had and yours @lbernick.
My initial thought on concurrency was to provides the concurrency control primitives to tektoncd/pipelne so that other tools (be it upstream or elsewhere) could use them (hence having it on PipelineRun or at least in tektoncd/pipeline).

What is proposed here, however, is an "end-user" proposal, for Triggers. The two approaches are not antinomic, but one might use the 2nd. I do think the proposal on trigger probably make sense for triggers. But I do think the "primitives for concurrency control" shouldn't be in triggers (Triggers "description" is "… that allows you to create Kubernetes resources based on information it extracts from event payloads.").

As proposed here, any tool that wants to implement concurrency control and that doesn't rely on triggers, will need to "copy" what's proposed (or come up with its own approach) — which was already the case today.

[vdemeester]

From my point of view, the only "job" of triggers should be to create that label and most likely create the PipelineRun as pending, and have something else handle that label (aka not the triggers controller).

[vdemeester]

Because the PipelineRun controller
(or ConcurrencyControl controller) is not responsible for creating PipelineRuns, it has to rely on other components starting PipelineRuns as pending, and matching
these PipelineRuns to the ConcurrencyControl via some strategy such as labels, ownerReferences, or modifying PipelineRuns themselves

Althought I slightly agree, the PipelineRun reconciler has all the possibilities to do whatever it wants with the newly created PipelineRun. We could decide that, if a "conccurency" label is set on a PipelineRun, we make it pending. This is a choice we can make (or not).

[vdemeester]

Which "is" kind of true, as this solution adds it in Triggers.

[abayer]

Yeah, I agree - Triggers is upstream of Pipelines, and isn't necessarily the only place users would want to be able to control concurrency.

[lbernick]

@vdemeester could you elaborate on what a "concurrency primitive" might look like?
I like the idea of building something that higher-level systems can use for concurrency controls, but I think concurrency controls belong at a higher level than Pipelines. It doesn't make sense to talk about concurrency controls for a single PipelineRun, so I'm not sure what a primitive in Pipelines would look like-- do you have an example in mind?

[lbernick]

/assign @vdemeester @abayer

[lbernick]

@abayer @vdemeester @chmouel @dibyom I've opened an alternative POC in the Pipelines repo for "concurrency primitives". PTAL and let me know if this is in line with what you are thinking: tektoncd/pipeline#5501

[dibyom]

So, I agree with @vdemeester 's comment about the primitive based approach. To me, that means that the solution has to be loosely coupled i.e. we should not require changes to pipelines/triggers code and ideally no changes to the existing crds either. This does not mean we cannot add those changes in later if we decide this is a very core component of say a trigger. Also, this does not mean we cannot provide a more opinionated approach in another higher level crd such as workflows. But the most basic implementation should be flexible and loosely coupled.

Here is roughly what I had in mind (let me know if some of this is infeasible)

1. A way for a user to specify the Concurrency configuration which contains 1. the concurrency key, 2. the strategy and 3. a way to match this concurrency config to some subset of pipelineruns - I think we can start we a label selector here and optimize later
   (e.g. for the triggers PoC scenario - since each run created by a Triggers gets a label with the name of the trigger, the label selector can just match against that label)

2. On Run creation, modify the run to use PipelineRunPending. (This can happen in a mutating webhook so that we don't need to change code in Pipeline or Triggers)

3. For all runs in a pending state, a new reconciler first sees if a valid concurrency label is present. If not, it matches it against all label selectors for all relevant concurrency configs. If nothing matches, remove the pipelinerun pending state. If there is a match, add a new concurrency key label to the object, run the concurrency strategy (cancel any relevant runs) and then remove the pipelinerun pending state.

Some caveats/drawbacks of this approach I can think of

1. The matching might get complex/slow - maybe a higher level thing can set the concurrencyLabel directly
2. Label manipulation - Ideally the selection and the concurrency key should be immutable fields; not a easily changeable label
3. Multiple additional components - For workflows, since we generate the trigger template, we can always set the status to pending at that point instead of relying on a webhook to do so.

[lbernick]

thanks @dibyom for the feedback! I am experimenting with a solution in a separate reconciler, similar to what you've described. The one thing I might not end up implementing is the reconciler changing the pipelinerun status from pending -> running, partially because it's a bit of a code smell for the reconciler to edit the spec of the pipelinerun it's reconciling and partially because I haven't figured out yet how to get this to actually work 😅 edit: dibyo and I figured this out!

Would be great to hear others' thoughts as well!

[lbernick]

There's one other issue with the mutating admission webhook strategy that @RafaeLeal mentioned to me. Labels and annotations aren't available during the create request (they're patched after creation), so we'd have to mark the pipelinerun as pending during an update request. However I'm not sure how to know during an update request whether it's appropriate to mark a pipelinerun as pending, and our webhook doesn't currently allow running pipelineruns to be changed to pending (although ofc this is something we can change)

[lbernick]

POC number 3 as requested! I haven't yet implemented a mutating admission webhook but that's next on the list.

[lbernick]

@ all -- I've updated the TEP to list out alternatives, and the proposed solution to basically state that we're going to experiment with a separate reconciler and use it in dogfooding, and we'll design a higher level api for concurrency in workflows once the workflows api exists and after getting experience testing out the experimental project. Also added links to existing POCs. Hoping this can be merged without major changes, please lmk what you think.

[lbernick]

@abayer @vdemeester PTAL, hoping to merge during API WG on Monday

[abayer]

/approve

Looks like a good start to me - looking forward to seeing where we end up going with this!

[vdemeester]

I think the default should be "graceful cancellation", aka cancel but run finally tasks.

[lbernick]

sounds good-- the initial version I'm working on uses regular cancelation as the default but this should be super easy to change.

[vdemeester]

I think so yes. A user can change labels on a Pod from a TaskRun and provoke a weird behavior for example. This is very similar here, we cannot completely stop the user from shooting its foot, and that's fine 🙃

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• teps/OWNERS [abayer,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[afrittoli]

LGTM in the working group
/lgtm