TEP-0086: Changing the way result parameters are stored #521
Conversation
tekton-robot
added
the
do-not-merge/work-in-progress
|
|
tekton-robot
added
the
size/L
vdemeester
added
the
kind/tep
|
/assign @afrittoli |
|
/assign @vdemeester |
|
/assign |
tekton-robot
removed
the
do-not-merge/work-in-progress
|
Submitting PR based on Alternate API WG discussion with 0086 in 'proposed' status |
There was a problem hiding this comment.
Thank you for starting this!
I think having a flexible mechanism for to share results between tasks and pipelines is an important feature, and being able to store larger results would be super-nice.
Even if binary artefacts are mentioned as a non-goal, I think this goes in the direction of artefacts anyways, and I wonder if should provide a way for users to plug-in their preferred storage mechanism, whether it's etcd (ConfigMap, CRD, status or so), a database or some kind of artefact storage.
|
|
||
| 1. N Configmaps: Per TaskRun or Per pipeline with Patch Merges (concern on parallelism even with queued Patch Merges) | ||
|
|
||
| - Suppose hypothetically that the pipelines controller were to create N ConfigMaps** for each of N results, it could also grant the workload access to write to these results using one of these focused Roles: |
There was a problem hiding this comment.
In case of a pipeline, it would be one ConfigMap for each result defined in each of the Tasks, plus one for each result defined at pipeline level? What is the reason for having so many different config maps?
There was a problem hiding this comment.
I had assumed it would be one ConfigMap for all Results for a specific Task. The idea behind this was security i.e. a write once at end of TaskRun only by that approved Task.
But I am sure the others from the issue can also add additional detail here.
| - Concern around having to create a new ConfigMap**, Role and RoleBinding per TaskRun, when at the end of the day we don't actually care about updating that ConfigMap**, but the TaskRun's results. | ||
| - This option won't 'scale fail' which would happen with self definition update when the definition itself starts to get too big in unrelated areas (aggregate limit). | ||
|
|
||
| 2. CRD |
There was a problem hiding this comment.
Does this mean creating a dedicated CRD to store results?
|
|
||
| There is the current alternative of storing result parameters as data in a workspace, however Workspaces | ||
| - require there has to be a storage mechanism in the cluster that can be shared between Tasks. That can be complex, or have performance issues in itself if using an As A Service that orders the storage at spin-up time. Or forces Tasks to all run on the same node. etc. Storage is a big complex adoption hurdle. | ||
| - changes the way end users refer to the result parameter or pass between containers |
There was a problem hiding this comment.
I 100% we should not change the interface for retrieving results.
For producing results, if we decide to provide a new interface, we should maintain backward compatibility anyways.
Since tasks can be written in any programming language, it feels to me that the current file-system based interface works well across languages, and it can be used without the need of extra dependencies.
I wonder if it would make sense to provide a pluggable interface on Tekton side?
We could keep both read and write interfaces from Tasks as they are, provide a default storage implementation as Tekton and let users plug-in other kind of storage based on their requirements (size, speed).
| - Could run as part of the controller | ||
| - Could be a separate HTTP server(s) which write to TaskRuns (or even config maps); task pods connect to this server to submit results, this server records the results (means result size would be limited to what can be stored in the CRD but there probably needs to be an upper bound on result size anyway) | ||
|
|
||
| 4. Self-update / mutate the TaskRun via admission controller (with the various controls i.e. first write, subsequent read only) |
There was a problem hiding this comment.
im not quite sure how this would work - how would the admission controller know what result to write?
There was a problem hiding this comment.
good question? i just grabbed it from the issue. Should I change or remove it?
tekton-robot
added
needs-rebase
|
Updated based on a number of the review comments. |
|
@tlawrie Something went wrong in the PR rebase perhaps? It now contains 27 commits, most unrelated to this TEP. |
tekton-robot
added
the
do-not-merge/hold
I noticed this TEP is not yet accepted and merged. This TEP is stuck in the review process. Dropping the proposal and design details based on the recent discussion in the API WG. This TEP instead now contains a list of alternatives. If we can merge this without proposal, it opens up an opportunity for the community to create a PoC and evaluate which solution fits best for the set of the requirements. Moving the proposal and design details under the first alternative, making it more like a TEP within TEP. One major change here is the requirement section in which task results as part of the taskRun status is listed as a requirement with the flexible design for the use cases which would like to avoid it. Updated the toc.
There was a problem hiding this comment.
@tlawrie @pritidesai @imjasonh - thank you for the hard work here!
Given that the possible solutions are all listed as alternatives and we have contributors interested in experimenting to help us figure out a way forward, I suggest that we approve this TEP in its current proposed state to indicate this is a direction we want to go and unblock the contributors to work on the experiments. What do you think @tektoncd/core-maintainers?
|
/test .* |
|
We have approval from @vdemeester @bobcatfish and @jerop. @ScrapCodes has a PoC created in the pipeline repo in the form of a WIP PR: tektoncd/pipeline#4838 Please help merge this PR so that we can move forward with the implementation. |
|
This was on hold until commits were sorted out which is the case now :) |
tekton-robot
removed
the
do-not-merge/hold
|
/test pull-community-teps-lint |
|
/test pull-community-org-validation |
There was a problem hiding this comment.
I think we can refer to tektoncd/pipeline#4808 and I would also like to see the idea of a "built-in" CSI volume driver as a possible alternative to explore.
But that can be done in follow-ups PR on this. We need to fix results in some way.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
/lgtm |
|
/test pull-community-org-validation |
Co-authored-by: Jerop Kipruto <jerop@google.com>
Based on feedback by @bobcatfish
@jerop I agree. Merging it in proposed lgtm. Overall, I have a vision that hopefully will be achieved with a pluggable interface with results stored byRef on the TaskRun but I know people are eager to start experimenting! |
|
Thanks to everyone for all the feedback & conversation & improvements that this generated and how it evolved. This is the first TEP I have ever worked on and I wanted to give big kudos to @bobcatfish (and now @jerop @imjasonh @mattmoor for the time they have put into guiding and designing with me. I can't believe its been 11 months of discussion and work and growth to get to this point. I first raised an issue w.r.t. this on 8 June 2021! |
|
API WG - two pending comments to address in the follow up PR, ready to merge in proposed state. /lgtm |
Initial TEP PR for issue tektoncd/pipeline#4012