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-0044]: Refocus on Execution Options #610

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

lbernick
Copy link
Member

This commit renames TEP-0044 to "More Execution Options for Pipelines" and re-focuses it on giving
users more control over how their Pipelines are executed. There are two reasons for this change:

  • The problems identified with how Tasks are executed (pod overhead and difficulty passing data between Tasks)
    are relevant only when executing multiple Tasks together, not when a single Task is executed in a TaskRun.
  • The title "Decoupling Scheduling from Execution" implies that we must choose a runtime-based configuration
    for Pipeline execution. This commit clarifies that while execution options must be configurable at runtime,
    we may choose to also make them configurable at authoring time.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2022
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 26, 2022
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2022
@jerop
Copy link
Member

jerop commented Jan 26, 2022

/assign

Copy link
Member

@jerop jerop 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 updating the problem statement @lbernick, much clearer now! 🎉

@lbernick lbernick marked this pull request as ready for review January 26, 2022 16:29
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2022
@lbernick
Copy link
Member Author

/assign bobcatfish

Copy link
Member

@afrittoli afrittoli 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 the update on this @lbernick.

The new TEP name seems fine to me, however the summary suggests that the execution mode must be controlled by the user that executes the pipeline.
I'm not convinced that that must be the case though. I agree we might give that control to users, but I would like to consider options where the Tekton controller makes some smart choices based about how execute a pipeline, rather than leaving it to the user by default.

@lbernick
Copy link
Member Author

The new TEP name seems fine to me, however the summary suggests that the execution mode must be controlled by the user that executes the pipeline. I'm not convinced that that must be the case though. I agree we might give that control to users, but I would like to consider options where the Tekton controller makes some smart choices based about how execute a pipeline, rather than leaving it to the user by default.

Thanks for the feedback! I've updated the summary to simply say that we want more options for how to execute pipelines, and tweaked the requirements wording.

Copy link
Member

@afrittoli afrittoli 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 the updates!
/approve

@vdemeester
Copy link
Member

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Jan 27, 2022
@bobcatfish
Copy link
Contributor

I have 2 pieces of feedback but don't need to necessarily block this PR:

  1. The new TEP name feels like it's broadening the scope, maybe more than we want to (could interpret it to mean a lot of things) <-- this is the only feedback that's really directly relevant to this PR
  2. Re. @afrittoli 's feedback on pipelines making choices on how to execute the pipeline vs the user being explicit, i agree that we should list this as an option however I have my doubts whether the work required to pull that off is worth it, when contrasted with (at least initially) making it possible for the user to be explicit (and laying the ground work for the user to be explicit about how to combine Tasks into pods could make it easier to add this smart grouping later on)

Anyway hoping we can talk some of this through in the API WG on Monday and I'm expecting I'll do an approve shortly after that!

This commit renames TEP-0044 to "Data Locality and Pod Overhead in Pipelines" and re-focuses it on giving
users more control over how their Pipelines are executed. There are two reasons for this change:

- The problems identified with how Tasks are executed (pod overhead and difficulty passing data between Tasks)
are relevant only when executing multiple Tasks together, not when a single Task is executed in a TaskRun.
- The title "Decoupling Scheduling from Execution" implies that we must choose a runtime-based configuration
for Pipeline execution. This commit clarifies that while execution options must be configurable at runtime,
we may choose to also make them configurable at authoring time in the future.
@lbernick
Copy link
Member Author

lbernick commented Feb 1, 2022

  1. The new TEP name feels like it's broadening the scope, maybe more than we want to (could interpret it to mean a lot of things) <-- this is the only feedback that's really directly relevant to this PR

I've updated the title to "Data Locality and Pod Overhead in Pipelines". A bit long, but this is the closest description I could come up with to the problem we are trying to solve. Makes it clear that the problem is with Pipelines rather than Tasks, doesn't imply runtime vs authoring time configuration, and doesn't expand the scope as much as the previous title.

  1. Re. @afrittoli 's feedback on pipelines making choices on how to execute the pipeline vs the user being explicit, i agree that we should list this as an option however I have my doubts whether the work required to pull that off is worth it, when contrasted with (at least initially) making it possible for the user to be explicit (and laying the ground work for the user to be explicit about how to combine Tasks into pods could make it easier to add this smart grouping later on)

I don't think this was completely resolved at the WG, but I also don't think this PR should be blocked on resolving this question.

@bobcatfish
Copy link
Contributor

Thanks @lbernick ! :D For me that's an:

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@jerop
Copy link
Member

jerop commented Feb 2, 2022

/lgtm

approved to be merged outside of api wg in slack thread

thanks @lbernick!

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2022
@tekton-robot tekton-robot merged commit b50e8f5 into tektoncd:main Feb 2, 2022
@lbernick lbernick deleted the pipeline-options branch February 4, 2022 18:33
@lbernick lbernick restored the pipeline-options branch August 15, 2022 13:44
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants