APIs for conditionals

[dibyom]

Changes

The commit adds the basic APIs for conditionals in Tekton:

1. Condition CRD defines a condition how a condition is evaluated i.e.
   the container spec and any input parameters.
2. The Conditions field in PipelineTask references Condition
   resources that have to pass before the task is executed.
3. The ConditionChecks field in PipelineRun.Status.TaskRuns surfaces
   the status of conditions that were evaluated for that particular task.

Part of #1019

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Adds support for conditional execution to pipelines.

[ghost]

Most of my review comments here are stylistic / nits. Overall approach lgtm! Will review again when it's moved out of WIP.

[linux-foundation-easycla]

Thank you. Unfortunately, your account is not authorized under a signed CLA. Please click here to proceed.

• ❌ Dibyo Mukherjee (e7919b6, e2b5f96, af090f0, 6c7accb, c7c5f5d, 623eeda, cd2262f, b414016, b2575f2, dc750ff, b939af1, 118662b, 1282b56, 3110571, 7bc7791, fcaa33d, 00c4ced, 2900022, c0168f1, 41472de, 4bbde87)

[dibyom]

/test pull-tekton-pipeline-integration-tests

[dibyom]

/test pull-tekton-pipeline-integration-tests

[dibyom]

/test pull-tekton-pipeline-integration-tests

[dibyom]

/retest

[ghost]

/lgtm

[vdemeester]

Looking good, few remarks.

The second commit adds an initial implementation.
In this implementation, condition evaluations aka ConditionChecks are
backed by TaskRuns. All conditionChecks associated with a PipelineTask
have to succeed before the task is executed. If a ConditionCheck fails,
the PipelineTask's associated TaskRun is marked failed i.e. its
Status.ConditionSucceeded is False. However, the PipelineRun itself
is not marked as failed.

So, we are using TaskRun for running the condition, which is nice I think — because we can re-use our tooling. Looking quickly at the implementation, the TaskRun created will be attached to the PipelineRun (aka created the same way as normal PipelineTask -> TaskRun). I wonder how current tools (dashboard, the cli, …) will handle that, or how should they handle that – like how tkn taskrun list will show TaskRun vs Condition (I am thinking outloud).

I need a bit more time to dig into it (and test) but initial review, looks good !

[dibyom]

I wonder how current tools (dashboard, the cli, …) will handle that, or how should they handle that – like how tkn taskrun list will show TaskRun vs Condition (I am thinking outloud).

Does the CLI/dashboard use the labels (i.e. tekton.dev/pipelineRun or tekton.dev/pipelineTask) or the pipelinerun status field? If it is using the status field, we should be ok, the condition check taskrun statuses are embedded under the taskrun that they are gating.

If they are using the labels, then the conditionCheck taskruns also add the same labels as a regular taskrun. However, in addition, they also have the tekton.dev/pipelineConditionCheck label set. The CLI/dashboard could use that to filter the conditionals out. Though I guess this is a breaking change -- we could also only set the conditionCheck label and not the pipelineRun or taskRun label.

[afrittoli]

Nice work, thank you for this! This feature is really handy and its implementation using CRD + TaskRuns is consistent with the rest of Tekton.

One question I have is how you envision the lifecycle of this new CRD.
Conditions used in pipelines but they are independent, so they must be provisioned separately, and they stay in the system after the PipelineRun is deleted. Are we going to create and delete condition all the time, or are they meant to be provisioned once and stay around and be used by all pipelineruns that needs them?
It might make sense to have a few common conditions added to the catalog project.

I left a few minor comments. They only bit where I'd like to see a change before this is one YAML test case defined in the example folders, so we can run one e2e test with that.

[afrittoli]

NIT: It looks as if it may be possible to factor up TestUpdateTaskRunState_WithPassingConditionChecks and TestUpdateTaskRunState_WithFailingConditionChecks into a single func that runs the test twice with different inputs.

[bobcatfish]

I am very excited about this! My initial reaction is pretty similar to the discussion @afrittoli and I are having in #837 - maybe we could add this over a couple PRs? e.g. we could start with a PR that adds the types and the validation maybe? Would allow us to focus on reviewing the interface before reviewing the implementation.

Just let me know if that would be too disruptive to what you are doing @dibyom , this is feedback I should have given a couple weeks ago instead of suddenly now :( If needed I can review as a whole!

So, we are using TaskRun for running the condition, which is nice I think — because we can re-use our tooling. Looking quickly at the implementation, the TaskRun created will be attached to the PipelineRun (aka created the same way as normal PipelineTask -> TaskRun). I wonder how current tools (dashboard, the cli, …) will handle that, or how should they handle that – like how tkn taskrun list will show TaskRun vs Condition (I am thinking outloud).

I am wondering about this - esp. since I happen to know that TaskRuns have some specific behaviour in them when they are owned by a PipelineRun 😅 which you probably dont want (see #1068 😅 ) (not that this should be the case! dont let that alone stop you from using TaskRuns).

I would almost rather see the useful part of TaskRuns factored out to be more easily called by something else vs. actually creating a TaskRun 🤔 (I also wonder if we're going to end up wanting to reuse that logic more as we go, e.g. via. Notfications #49, and in event triggering we're probably gonna want to create Conditions too ...) Feel free to disagree with me here tho, I've only glanced at the implementation and you're way more familiar with it 🙇‍♀

[dibyom]

Just let me know if that would be too disruptive to what you are doing @dibyom

Not at all, this PR is already broken into commits that first adds the types and then the implementation. Gonna drop the later commits and put them in a separate PR.

As far as using TaskRuns goes, I think using them makes some features easier e.g. adding in resource support, status updates etc. That being said, I'm going to dig a bit deeper into how much effort it'll be if we want to refactor the taskrun logic into something more reusable.

Also, @afrittoli thanks a lot for the review. Will add the YAML tests. You are right that conditions are created once and then reused in pipelines (much like tasks are used in pipelines). The plan is to add basic conditions to Tekton (the catalog is a nice idea, did not consider that!)

[dibyom]

Dropped the other commits and this PR is now just the initial API. PTAL @bobcatfish !

[afrittoli]

Thanks you for all the updates and for splitting the PR in multiple parts!

[m1093782566]

/cc

[tekton-robot]

@m1093782566: GitHub didn't allow me to request PR reviews from the following users: m1093782566.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc

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.

[bobcatfish]

I wonder if there is a way that we can still reuse a lot of the TaskRun logic, but also narrow the amount of functionality we are exposing for conditionals? One idea: still use TaskRuns underneath (i.e. create a TaskRun in order to fulfill a Condition) but make a new ConditionStatus that only contains fields that make sense for Conditions, i.e. we can treat the fact that TaskRuns are being used under the surface as an implementation detail and not expose it to the user at all - that way we can approach refactoring it later without changing the user experience.

[bobcatfish]

I think you said you're already digging a bit into refactoring this so that we don't reuse Task runs but here's some more food for thought! Looking at what we've got in TaskRunStatus we've got these fields:

    Results *Results "json:\"results,omitempty\"" # Oh boy what even is this? Should probably delete this... 😅 

    # These seems like they make sense for a Condition
    Status "json:\",inline\""
    PodName string "json:\"podName\""
    StartTime *Time "json:\"startTime,omitempty\""
    CompletionTime *Time "json:\"completionTime,omitempty\""

    # Since we only have one step ever it's kinda odd that we have a "steps" field with multiple steps in it
    Steps []StepState "json:\"steps,omitempty\""

    # I'm doubtful these make sense for a condition
    RetriesStatus []TaskRunStatus "json:\"retriesStatus,omitempty\""
    ResourcesResult []PipelineResourceResult "json:\"resourcesResult,omitempty\""

   # Would it make sense to have something explicit like "condition result" to say "pass" or "fail" or similar?
   # (Just ignore if it's not worth it)

My inclination here would be to add as little functionality as we can possibly get away with - it's always easier to add more later than to remove it!

[dibyom]

Totally agree. I think reusing the TaskRunStatus ties us to the TaskRun implementation which is not something we want!. Will have this updated soon!

[dibyom]

Removed the Results, RetriesResults, ResourceResult, and Steps fields. Added a Check field that is of type ContainerState.

Also, from the comment for Results, it seems like it points to location logs have been uploaded to though it seems like its not fully implemented (cc @sbwsg )

[bobcatfish]

Also, from the comment for Results, it seems like it points to location logs have been uploaded to though it seems like its not fully implemented (cc @sbwsg )

No worries leaving that as-is - almost definitely leftover from our first attempt at the API 😅

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_types.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha1/condition_validation.go	Do not exist	87.5%

[bobcatfish]

Nice! Thanks for making the status change so quickly @dibyom looking good to me!

/lgtm
/approve
/meow space

[tekton-robot]

@bobcatfish:

In response to this:

Nice! Thanks for making the status change so quickly @dibyom looking good to me!

/lgtm
/approve
/meow space

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, bobcatfish, dibyom

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:

• OWNERS [bobcatfish]

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