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] Update Alternatives #586

Merged
merged 1 commit into from
Feb 7, 2022
Merged

Conversation

lbernick
Copy link
Member

@lbernick lbernick commented Dec 20, 2021

This commit describes design constraints related to running multiple Tasks in one pod in more detail.
It also updates the alternative solutions based on the clarified problem statement.

Changes:

  • Add 3 options: pipeline in a pod + pipelines in pipelines, pipeline -> taskrun, and task pre and post steps
  • Modify and add more detail to the "TaskGroup" proposal
  • Remove "focus on workspaces" option: this adds new syntax but not new functionality
  • Remove "custom pipeline" solution due to similarity with grouping CRD solution
  • Remove "custom scheduler" option: This is a solution for scheduling parallel Tasks that share a PVC
    on the same node, but does not address the problems of pod overhead and the need
    to share data between tasks in the first place.

Co-authored-by: Christie Wilson christiewilson@google.com @bobcatfish

/kind tep
/assign vdemeester
/assign afrittoli

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

dibyom commented Dec 20, 2021

/assign @dibyom
/assign @jerop

@lbernick lbernick force-pushed the tep-44 branch 2 times, most recently from 457136c to 1ccff71 Compare January 11, 2022 14:21
@lbernick
Copy link
Member Author

@dibyom @jerop @afrittoli @vdemeester have you had the chance to look at this yet? This is just updating alternatives, not proposing one of them, so I think this can be merged if the pros/cons adequately reflect each alternative. I'd like to be able to move forward with proposing a design.
Would it be helpful for me to separate the "design consideration" section into a separate PR?

@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
@bobcatfish
Copy link
Contributor

@lbernick drive by comment (and good chance we already discussed this so my apologies!!) but is it possible to add a bit more detail to "Remove "custom scheduler" option: this is a solution to a different problem" - it doesnt address the multiple pod issue but i think it is a way of trying to address the data sharing option in that at least if you can schedule pods that access a volume on the same node, they get faster access to that volume

(i dont think this is a solution we should go with because i dont think it addresses all of the requirements well but im not seeing it as addressing a totally different problem?)

@lbernick
Copy link
Member Author

@lbernick drive by comment (and good chance we already discussed this so my apologies!!) but is it possible to add a bit more detail to "Remove "custom scheduler" option: this is a solution to a different problem" - it doesnt address the multiple pod issue but i think it is a way of trying to address the data sharing option in that at least if you can schedule pods that access a volume on the same node, they get faster access to that volume

Thanks for the feedback! I added some more detail to the commit message to elaborate. If you'd prefer I keep this alternative in there because there is overlap between what that is solving and the scope of this TEP, I'm happy to add it back.

@lbernick
Copy link
Member Author

/hold

needs some updates in response to #610

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2022
@bobcatfish
Copy link
Contributor

I added some more detail to the commit message to elaborate. If you'd prefer I keep this alternative in there because there is overlap between what that is solving and the scope of this TEP, I'm happy to add it back.

Thanks for updating the commit message - i think I see what you're saying that it does make it faster to share a PVC but doesn't remove the need for one. sgtm to keep it out of scope!

@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 2, 2022
@lbernick
Copy link
Member Author

lbernick commented Feb 2, 2022

/hold cancel

Rebased onto #610. The main changes I've made as a result are 1. rewriting the "task grouping" option so that the desired syntax can be specified on either the pipeline or pipelinerun and 2. Making the pros/cons of each alternative more clear with regards to whether they allow specifying runtime and/or authoring time user configuration.

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2022
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

The proposed options make sense. Just had one question around specifying this in a pipeline vs a run

- Any configuration for the execution of a Pipeline must be modifiable at runtime.
- This is to support use cases such as uploading test results, even if the test Task failed
- This requirement is being included because we could choose a solution that doesn't address the above use case.
1. Any configuration for the execution of a Pipeline must be modifiable at runtime.
Copy link
Member

Choose a reason for hiding this comment

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

I'm missing some background here - what's the use case for modifying the execution config in a PipelineRun vs a Pipeline?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's very likely we will want a configuration option here on the Pipeline; I think this is probably something we should implement. The reason to allow people to specify this on a PipelineRun is generally because the choice of how to execute a Pipeline is a runtime level concern. Specifically, I can imagine situations where you might have different approaches to PVCs in different clusters but want to reuse the same Pipeline definition from a repo, or maybe some PipelineRuns will use different amounts of data than other PipelineRuns for the same Pipeline and you want to sometimes distribute the work to different pods and sometimes not. Basically-- I think we shouldn't lock ourselves into a solution that can only be specified at one of runtime or authoring time. (Perhaps this should read "must be modifiable at both runtime and authoring time, but the initial implementation will be runtime based"? This is the question I tried to address with #610.)

Copy link
Member

@dibyom dibyom Feb 4, 2022

Choose a reason for hiding this comment

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

That helps, thanks. I agree that we shouldn't lock ourselves into a solution that cannot be extended.
The reason I was asking about having the config in a pipeline is that pipeline resources are specified in a pipeline/task spec. So, for the specific use case of replacing pipeline resources though (e.g. a git resource in a TaskRun), the decision of using a single pod/no PVCs was made by the task/pipeline author.
It seems like the scope of this TEP is greater than just replacing the pipeline resources use case so it might make sense to do this at the Run

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 all the updates.
There may be more pros and cons one some of the options, but this looks really good.

Once this is merged, we could start building a table with various features against multiple options to help us visualise this better.

/approve

teps/0044-decouple-task-composition-from-scheduling.md Outdated Show resolved Hide resolved
teps/0044-decouple-task-composition-from-scheduling.md Outdated Show resolved Hide resolved
Comment on lines 153 to 154
- This is to support use cases such as uploading test results, even if the test Task failed
- This requirement is being included because we could choose a solution that doesn't address the above use case.
Copy link
Member

Choose a reason for hiding this comment

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

Nice. This is not addressed by finally today because using finally would require the data to be on a PVC, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not completely sure what you mean by this. This requirement is currently addressed in pipelines with finally, but the required use of a PVC causes us to run into the problems described in the problem statement. If we chose a solution where you could run the pipeline's tasks in a pod but not its finally tasks, that wouldn't address this requirement also due to the required PVC.

Copy link
Member

Choose a reason for hiding this comment

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

@lbernick - I think what @afrittoli is referring to is that Results from failed Tasks are not available to any other Tasks, so the test "results" have to be in a PVC (they are not Results as defined in Tekton Pipelines) - see related issue in tektoncd/pipeline#3749

Copy link
Member Author

Choose a reason for hiding this comment

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

oh I see, thanks!

Comment on lines 160 to 161
- Hermetic Pipelines and Tasks will be run in their own network namespaces with no networking enabled.
The solution must provide a way for some colocated Tasks to run in isolated network namespaces, and some to have network access.
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this is requirement for cases like: clone / hermetic build / push where the first and last need network access, the middle one doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup! there's more detail on use cases for different levels of hermeticity in a pipeline in TEP 25.

teps/0044-data-locality-and-pod-overhead-in-pipelines.md Outdated Show resolved Hide resolved
teps/0044-data-locality-and-pod-overhead-in-pipelines.md Outdated Show resolved Hide resolved

Cons:
* Will need to update our entrypoint logic to allow for steps running in parallel
* Doesn't give as much flexibility as being explicit
* This functionality might not even be desirable for folks who want to make use of multiple nodes
* We could mitigate this by adding more configuration, e.g. opt in or out at a Pipeline level, but could get
complicated if people want more control (e.g. opting in for one workspace but not another)
* Removes ability to run Tasks on separate pods if data is shared between them.
Copy link
Member

Choose a reason for hiding this comment

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

If the reason for separate pods is security, using different service accounts would force separate pods anyways.
If the reason for separate pods is resources, having high resource requirements could enforce sequential execution of the use of separate Pods.
Alternatively this could be overcome by some kind of "anti-affinity" between PipelineTasks.
This gets scarily close to the scheduling domain though - which means there may be more complexity behind this than it appears on the surface.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great point. I think the reason is resources, not security, and so choosing this option would essentially be trading one resource-related problem for another.

teps/0044-data-locality-and-pod-overhead-in-pipelines.md Outdated Show resolved Hide resolved
teps/0044-data-locality-and-pod-overhead-in-pipelines.md Outdated Show resolved Hide resolved
@lbernick lbernick force-pushed the tep-44 branch 2 times, most recently from 14d6096 to 2c5f36a Compare February 4, 2022 16:40
@lbernick
Copy link
Member Author

lbernick commented Feb 4, 2022

FYI @dibyom @afrittoli @vdemeester since you've already approved: I decided to remove the ability to specify different levels of hermeticity as a requirement for this TEP. I looked into it a little more and it's not clear that it's feasible or that users would want it. We can continue to explore this but I'm not ready to make it a requirement yet, and want to leave out of scope for this PR.

@dibyom
Copy link
Member

dibyom commented Feb 4, 2022

the hermetic mode might be needed for chains/hermekton. @wlynch do you think its sufficient to have hermeticity only specified on tasks running in separate pods (at least to start?)

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.

looks great, thanks @lbernick!

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

@wlynch
Copy link
Member

wlynch commented Feb 4, 2022

the hermetic mode might be needed for chains/hermekton. @wlynch do you think its sufficient to have hermeticity only specified on tasks running in separate pods (at least to start?)

Still need to dig into this, but at a glance it seems odd to me if we promise different capabilities for the same type depending on the scheduling mode. I think we'll want a gameplan for how to support hermetic builds for Tasks scheduled on the same pod, even if it doesn't land right away.

@mattmoor might have some ideas here, since there's probably a fair amount of overlap with hermetic steps.

@lbernick
Copy link
Member Author

lbernick commented Feb 7, 2022

the hermetic mode might be needed for chains/hermekton. @wlynch do you think its sufficient to have hermeticity only specified on tasks running in separate pods (at least to start?)

Still need to dig into this, but at a glance it seems odd to me if we promise different capabilities for the same type depending on the scheduling mode. I think we'll want a gameplan for how to support hermetic builds for Tasks scheduled on the same pod, even if it doesn't land right away.

@mattmoor might have some ideas here, since there's probably a fair amount of overlap with hermetic steps.

I'm hoping to keep hermekton discussion out of scope for this PR; once it's merged I'll open a new one specifically to discuss hermekton and loop in everyone on this thread!

This commit describes design constraints related to running multiple Tasks in one pod in more detail.
It also updates the alternative solutions based on the clarified problem statement.

Changes:
- Add 3 options: pipeline in a pod + pipelines in pipelines, pipeline -> taskrun, and task pre and post steps
- Modify and add more detail to the "TaskGroup" proposal
- Remove "focus on workspaces" option: this adds new syntax but not new functionality
- Remove "custom pipeline" solution due to similarity with grouping CRD solution
- Remove "custom scheduler" option: This is a solution for scheduling parallel Tasks that share a PVC
  on the same node, but does not address the problems of pod overhead and the need
  to share data between tasks in the first place

Co-authored-by: Christie Wilson christiewilson@google.com
@dibyom
Copy link
Member

dibyom commented Feb 7, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2022
@tekton-robot tekton-robot merged commit 834e178 into tektoncd:main Feb 7, 2022
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
Status: UnAssigned
Development

Successfully merging this pull request may close these issues.

8 participants