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

docs: Add Tekton backend design doc #65

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

FogDong
Copy link

@FogDong FogDong commented Sep 28, 2020

What changes were proposed in this pull request?

Chore(docs): add tekton backend design doc
Ref: #44

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

@CLAassistant
Copy link

CLAassistant commented Sep 28, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@merlintang merlintang left a comment

Choose a reason for hiding this comment

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

thanks for you to start this process. this would be useful to tekton community.

I also want to know, how can you define the workflow for the tekton? do we follow the explicitly or implicitly way of couler ?

```

A `whenExpression` is made of `Input`, `Operator` and `Values`. The `Input` can be static inputs or variables (Parameters or Results in Tekton). The `Operator` can be either `in` or `notin`.
As we can see, there is no `else_op` in Tekton, so we need to convert the `else` in the Tekton backend, which is pretty simple since there is only `in` and `notin` operator, we can simply change the operator in the "else" task like:
Copy link
Member

Choose a reason for hiding this comment

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

Good to see this. Also note that else_op should be optional. I'll add a note in Couler APIs design on this.


#### `while_loop(cond, func, *args, **kwargs)`

There is no native support for loops in Tekton for now.
Copy link
Member

Choose a reason for hiding this comment

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

Is tektoncd/pipeline#2050 a good issue to track this feature in Tekton?


The `get_status`, `get_logs` remains the same as the Argo design.

* `submit(config=workflow_config(schedule="* * * * 1"))`: There is no native support for cron in Tekton, see this [issue](https://github.com/tektoncd/triggers/issues/69) for more information. The community recommend cronjob in kubernetes, see [example](https://github.com/tektoncd/triggers/blob/master/examples/cron/README.md) here.
Copy link
Member

Choose a reason for hiding this comment

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

Argo Workflow's CronWorkflow is based on k8s CronJob anyways. Would it be easy to wrap CronJob in a Tekton workflow so that it's easy for users to start a scheduled workflow by simply specifying a schedule argument?

Copy link
Author

Choose a reason for hiding this comment

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

Great idea, we can wrap cron in the couler's workflow and let backend to handle the work

The `get_status`, `get_logs` remains the same as the Argo design.

* `submit(config=workflow_config(schedule="* * * * 1"))`: There is no native support for cron in Tekton, see this [issue](https://github.com/tektoncd/triggers/issues/69) for more information. The community recommend cronjob in kubernetes, see [example](https://github.com/tektoncd/triggers/blob/master/examples/cron/README.md) here.
* `delete_workflow(workflow_name)`: To be discussed: should we delete the tasks when delete the pipeline?
Copy link
Member

Choose a reason for hiding this comment

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

Yes we should but we should probably make this configurable for users to decide.

Copy link
Author

Choose a reason for hiding this comment

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

Something like delete_workflow(workflow_name, all) and delete_workflow(workflow_name, only)?

Copy link
Member

@terrytangyuan terrytangyuan Sep 30, 2020

Choose a reason for hiding this comment

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

I am thinking something like delete_workflow(workflow_name, dependencies=True) where dependencies indicates whether to delete this workflow's dependencies/resources it owns. Just my two cents and open to other ideas as well.


* `submit(config=workflow_config(schedule="* * * * 1"))`: There is no native support for cron in Tekton, see this [issue](https://github.com/tektoncd/triggers/issues/69) for more information. The community recommend cronjob in kubernetes, see [example](https://github.com/tektoncd/triggers/blob/master/examples/cron/README.md) here.
* `delete_workflow(workflow_name)`: To be discussed: should we delete the tasks when delete the pipeline?
* `list_workflow_records(workflow_name)`: After submitting the workflow(create PipelineRun in Tekton), there will be numbers of workflow records. A list function is necessary for users to show all the records.
Copy link
Member

Choose a reason for hiding this comment

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

Can the name be simplified to list_workflows()? We can probably add this in core Couler APIs as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also do we have API for dealing with workflow runs? For argo is that a feature only accessible via argo serve API?

Copy link
Member

Choose a reason for hiding this comment

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

@binarycrayon Could you clarify what you meant by "dealing with workflow runs"?

Copy link
Author

@FogDong FogDong Sep 30, 2020

Choose a reason for hiding this comment

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

By workflow records, I mean workflow runs, which is pipeline run in tekton. Should we change the name to list_workflow_runs and add another api like list_workflows?

Copy link
Member

@terrytangyuan terrytangyuan Sep 30, 2020

Choose a reason for hiding this comment

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

PipelineRun is just an instantiation of a Tekton Pipeline. In Argo, there's no such additional concept. Should we abstract such backend specific details away so that users will only focus on defining Workflow that will be submitted and won't be confused with these two concepts?

In other words, we only provide one API list_workflows() in Couler. What do you think? If really necessary, we can perhaps consider exposing list_workflow_runs at Tekton backend level.

Copy link
Contributor

Choose a reason for hiding this comment

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

@terrytangyuan We could, I can add argo server API support as part of argo-client-python

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 we can keep both list_workflows and list_workflow_runs. Though this is less applicable to Argo, it's quite common for other backends - they have the concept of pipeline/workflow to represent the execution logic and pipeline/workflow runs to represent the result and state for each run either triggered by cron or on-demand.

For Argo, I think CronWorkflow also has the idea of (though not explicit) the workflow vs run.

Copy link
Member

@terrytangyuan terrytangyuan Sep 30, 2020

Choose a reason for hiding this comment

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

Actually for Tekton users, my understanding is that they may not need to get a list of Pipelines at all. Instead, they want to get a list of PipelineRuns, which is actually "equivalent" to Argo's Workflow. Am I right?

If so, we actually only need one API which can be named either list_workflow_runs or list_workflows where list_workflows might be more generalized and concise. Correct me if I am wrong though.


### Other Tekton Utilities

* [Finally](https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md#adding-finally-to-the-pipeline)
Copy link
Member

Choose a reason for hiding this comment

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

Good to see this. The current Argo backend supports exit handlers in Argo Workflows as well. We can consider offer this in core Couler APIs. Could you add a function signature on how this API will look like?

@merlintang
Copy link
Member

Meanwhile, can you give an example of the workflow for Tekton based on Couler in this doc? This would help others to follow and understand.

@terrytangyuan terrytangyuan changed the title [RFC]Chore(docs): add tekton backend design doc docs: Add Tekton backend design doc Oct 7, 2020
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Merging this for now but feel free to continue the discussions here. Thanks! @FogDong

If anyone would like to help out with the implementation, please let us know!

@terrytangyuan terrytangyuan merged commit ce533cb into couler-proj:master Oct 13, 2020
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.

6 participants