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

TEP-0083: Scheduled and Polling runs in Tekton #517

Merged
merged 1 commit into from
Nov 17, 2021

Conversation

sm43
Copy link
Member

@sm43 sm43 commented Sep 13, 2021

This TEP introduces an idea for a feature in triggers which would allows user to

  • Schedule a pipelinerun/taskrun at a certain time/ at certain interval
  • Setup a poll which looks for changes on a repository and triggers pipelinerun/taskrun.

Signed-off-by: Shivam Mukhade [email protected]

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2021
@vdemeester vdemeester added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Sep 13, 2021
@afrittoli
Copy link
Member

/assign @sbwsg
/assign @savitaashture

Copy link
Contributor

@savitaashture savitaashture left a comment

Choose a reason for hiding this comment

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

This is a cool feature which adds more usecases to Triggers
Thank you @sm43 @vdemeester

teps/0083-scheduled-pipelinerun--tekton-polling.md Outdated Show resolved Hide resolved
teps/0083-scheduled-pipelinerun--tekton-polling.md Outdated Show resolved Hide resolved
teps/0083-scheduled-pipelinerun--tekton-polling.md Outdated Show resolved Hide resolved
teps/0083-scheduled-pipelinerun--tekton-polling.md Outdated Show resolved Hide resolved
template: pipeline-template
```

- This takes GitHub Repo URL and then check for changes at frequeny defined by user.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see right now it supports only Github
Is there a plan to support different scm like bitbucket, gitlab etc...?
If yes then can we add somewhere in TEP the supported and non supported one?

Copy link
Member

Choose a reason for hiding this comment

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

The repo happens to be GitHub, but there is nothing specific to GitHub. The PoC is limited to git though, but we could rework a bit the structure to allow more support for other VCS (like mercurial, …)

-->

## Design Details

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we have design details mentioning some points like

  1. what are the optional and required field in SyncRepo CRD
  2. does both polling and scheduled supported together
  3. If yes then describing which attributes for which functionality(polling, scheduling)

Copy link
Member

Choose a reason for hiding this comment

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

This initial TEP is mainly on the problem statement. the SyncRepo PoC is only based on the poll part of this TEP, but digging more, we might rework that CRD altogether, hence not getting in too much detail too quickly 😛

teps/0083-scheduled-pipelinerun--tekton-polling.md Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Sep 14, 2021

Thanks for opening the TEP, I understand the end goal but here are the questions I've come away with after reading:

  • We identify CronJobs as an existing solution for scheduling. Why aren't they good enough on their own?
  • We identify an alternative project that exists today for polling GitHub. Is there a strong reason to favor a new solution and if so what is it? Is there a strong reason to favor a project owned by Tekton and if so what is it?

It would be great to get a bit more detail in the problem statement covering these if possible.

@vdemeester
Copy link
Member

Thanks for opening the TEP, I understand the end goal but here are the questions I've come away with after reading:

  • We identify CronJobs as an existing solution for scheduling. Why aren't they good enough on their own?
  • We identify an alternative project that exists today for polling GitHub. Is there a strong reason to favor a new solution and if so what is it? Is there a strong reason to favor a project owned by Tekton and if so what is it?

It would be great to get a bit more detail in the problem statement covering these if possible.

Yes I think we need to answer those 2 questions in the TEP indeed 👼🏼

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2021
@afrittoli
Copy link
Member

Nice idea, thank you!
This reminds me of the concept of event source, commonly used in serverless architectures.

A couple of examples:

It seems to me that both cron and polling a git repo could be seen as event sources.

One option is to have the triggers controller able to reconcile an event source CRD.
Another solution could be to have a generic CRD - and have dedicated controllers, one for each kind of event source.
Those controllers would be responsible for setting up a dedicated trigger CRD and to send events to it over HTTP.
This would be similar approach I was thinking of using for the implementation of notifications in Tekton.

@pritidesai
Copy link
Member

/assign @afrittoli

@dibyom
Copy link
Member

dibyom commented Sep 21, 2021

/assign

@afrittoli
Copy link
Member

Nice idea, thank you!
This reminds me of the concept of event source, commonly used in serverless architectures.

A couple of examples:

It seems to me that both cron and polling a git repo could be seen as event sources.

One option is to have the triggers controller able to reconcile an event source CRD.
Another solution could be to have a generic CRD - and have dedicated controllers, one for each kind of event source.
Those controllers would be responsible for setting up a dedicated trigger CRD and to send events to it over HTTP.
This would be similar approach I was thinking of using for the implementation of notifications in Tekton.

My comments above ^^^ are more relevant for the design discussion - I agree with the goals of being able to schedule runs and polling to avoid exposing a public endpoint.

@dibyom
Copy link
Member

dibyom commented Sep 23, 2021

Problem statement/use cases sounds good!

Also agree with @afrittoli 's comment that there are a lot of projects in this space and we should consider how we can integrate with them or use them seamlessly vs inventing something of our own. (But's that a discussion for the design phase!)

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 23, 2021
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2021
@sm43
Copy link
Member Author

sm43 commented Sep 27, 2021

Thanks for opening the TEP, I understand the end goal but here are the questions I've come away with after reading:

* We identify CronJobs as an existing solution for scheduling. Why aren't they good enough on their own?

* We identify an alternative project that exists today for polling GitHub. Is there a strong reason to favor a new solution and if so what is it? Is there a strong reason to favor a project _owned by Tekton_ and if so what is it?

It would be great to get a bit more detail in the problem statement covering these if possible.

@sbwsg I have addressed the question in the TEP. ptal :)
cc @dibyom

@ghost
Copy link

ghost commented Sep 27, 2021

Thanks for providing responses to those questions. At this point you need an approve from a non-Google reviewer.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2021

```
apiVersion: triggers.tekton.dev/v1alpha1
kind: SyncRepo
Copy link

Choose a reason for hiding this comment

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

@sm43 @dibyom @chmouel It looks like something similar to a Repo CRD is a shared problem across this TEP, the Workflows project, pipelines as code, remote resource resolution

Could we potentially extract a common element here and deliver that on its own? I could see a few ways this could help. It might just be common vcs configuration (a name, the type of VCS, and the repo's URL?) or it could extend all the way to supporting polling, content fetching, content caching, rbac, etc etc with one or multiple controllers watching for RepoCRD objects and layering functionality on top of them.

Not sure, what do folks think?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this is definitely an idea worth exploring!!

Copy link

Choose a reason for hiding this comment

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

Great, I will put together some ideas for discussion at the next Workflows WG 👍

Comment on lines 220 to 221
- (sbwsg) We identify an alternative project that exists today for polling GitHub. Is there a strong reason to favor a new solution
and if so what is it? Is there a strong reason to favor a project owned by Tekton and if so what is it?
Copy link
Member

Choose a reason for hiding this comment

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

Link?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 125 to 129
- To allow users to schedule a pipelinerun/taskrun at a certain time or at certain interval

Ex. I want to run a pipeline every day at 8 am. Currently, I can do this by setting up a cronjob, but having as a part of Trigger could be a good idea. So, Triggers can start a pipelinerun at the time mentioned by me.

- To allow users to use triggers without need to setup a webhook. Triggers could have a feature which allow user to setup a polling feature for a repository which would look for changes in repository and trigger a pipelinerun or taskrun.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have a few words explaining why the two features are proposed together.
I assume the reason is that their implementation may rely on a shared new feature? If so should the TEP only be about that base feature and scheduled pipeline / polling be added as use cases?

Copy link
Member

Choose a reason for hiding this comment

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

Right, that might make sense.

Comment on lines 215 to 218
Cronjob do cover a use case which we are proposing which is triggering run at certain interval but to check it
something is actually changed to trigger a pipelinrun/taskrun then we will need a script to check the condition.
Everytime we setup this for a repository, the user will have to write a script. This logic can be abstracted into
triggers and an interface can be exposed to user which would be simple to configure.
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to capture the requirement of doing "conditional" scheduled runs in the summary or use cases or requirements. Abstracting the filtering logic to a Trigger can be done when using CronJobs too - the CronJob can send an event to an EventListener that can then decide - through the triggers - whether to run a pipeline or not. Recently added TriggerGroups also may help in making this setup easier to maintain.

The main downside for me in using CronJobs is that they are not well integrated with Tekton and it requires an event listener which could be avoided for an "internal" trigger. The HTTP POST needs to be scripted in the CronJob and the target URL needs to be derived from the event listener.

I would prefer an approach where we could define a Cron trigger more declaratively - see TriggerRuns and tektoncd/triggers#504 for further context.

@bobcatfish
Copy link
Contributor

Comments from API WG today: need to explore the alternatives a bit more in the proposal; in some setups you may not need explicit support for this (e.g. if you have knative)

teps/0083-scheduled-pipelinerun--tekton-polling.md Outdated Show resolved Hide resolved
teps/0083-scheduled-pipelinerun--tekton-polling.md Outdated Show resolved Hide resolved
Comment on lines 125 to 129
- To allow users to schedule a pipelinerun/taskrun at a certain time or at certain interval

Ex. I want to run a pipeline every day at 8 am. Currently, I can do this by setting up a cronjob, but having as a part of Trigger could be a good idea. So, Triggers can start a pipelinerun at the time mentioned by me.

- To allow users to use triggers without need to setup a webhook. Triggers could have a feature which allow user to setup a polling feature for a repository which would look for changes in repository and trigger a pipelinerun or taskrun.
Copy link
Member

Choose a reason for hiding this comment

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

Right, that might make sense.

@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021
@sm43 sm43 changed the title TEP-0083: Scheduled Pipelinerun & Tekton Polling TEP-0083: Scheduled and Polling runs in Tekton Oct 26, 2021
@sm43
Copy link
Member Author

sm43 commented Oct 27, 2021

Comments from API WG today: need to explore the alternatives a bit more in the proposal; in some setups you may not need explicit support for this (e.g. if you have knative)

@bobcatfish we created this PR to introduce the idea and use cases basically. we are exploring possible solutions and suggestion on the pr 😅
shall we merge this pr and follow up in a new with possible solutions and alternatives?

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2021
This TEP introduces an idea for a feature in triggers which would allows user to
- Schedule a pipelinerun/taskrun at a certain time/ at certain interval
- Setup a poll which looks for changes on a repository and triggers pipelinerun/taskrun.

Signed-off-by: Shivam Mukhade <[email protected]>
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2021
@pritidesai
Copy link
Member

API WG - waiting for more feedback ...

@dibyom
Copy link
Member

dibyom commented Nov 9, 2021

@savitaashture @afrittoli (also @khrm and @vdemeester ) - This problem statement still needs a non Google approver

@khrm
Copy link
Contributor

khrm commented Nov 17, 2021

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021
@tekton-robot tekton-robot merged commit 6810d57 into tektoncd:main Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Proposed
Development

Successfully merging this pull request may close these issues.

9 participants