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-0100: Embedded TaskRuns and Runs Status in PipelineRuns [Proposal] #616

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

jerop
Copy link
Member

@jerop jerop commented Jan 31, 2022

In TEP-0100, we described the challenges caused by embedding
the complete TaskRun and Run status in PipelineRuns: performance
degradation, memory bloat, and lack of extensibility.

In this change we add the proposal to reduce the amount of information
stored about the status of TaskRuns and Runs to only the object
references in order to improve performance, reduce memory bloat
and improve extensibility. We also mark the TEP as implementable.

Co-authored-by: Lee Bernick leebernick@google.com

/kind tep

/cc @vdemeester @imjasonh @bobcatfish @afrittoli
(approvers of the problem statement added in #606)

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Jan 31, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 31, 2022
@bobcatfish
Copy link
Contributor

/assign

@jerop jerop force-pushed the tep-0100-proposal branch 3 times, most recently from 90e5de7 to 558b020 Compare February 1, 2022 15:43
@jerop jerop force-pushed the tep-0100-proposal branch 4 times, most recently from 95ed686 to 2cff979 Compare February 2, 2022 00:57
Copy link
Contributor

@bobcatfish bobcatfish 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 great!! Thanks for being so thorough

/approve

(need to get some non-google approvers on this as well)

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2022
@jerop
Copy link
Member Author

jerop commented Feb 2, 2022

@tektoncd/core-maintainers we're looking for non-googler reviewers here 🙏🏾

(thanks @bobcatfish and @wlynch for the reviews)

@pritidesai
Copy link
Member

pritidesai commented Feb 2, 2022

Hey @AlanGreene please help us review this proposal! Does this solution aligns with the way we have truncated pipelineRun status and accessing it when needed?

@jerop jerop force-pushed the tep-0100-proposal branch 2 times, most recently from 9f7ab6b to 95e7516 Compare February 2, 2022 23:25
@imjasonh
Copy link
Member

imjasonh commented Feb 3, 2022

/assign

Copy link
Member

@AlanGreene AlanGreene left a comment

Choose a reason for hiding this comment

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

This will be a welcome change in terms of the performance improvements to be gained for users and other consumers. I have a few small concerns about impact on the Dashboard, but hopefully we can work around most of them relatively easily. See the comments.

One other thing to point out is the potential impact on our own use of the Dashboard in the dogfooding cluster. We currently have scheduled cleanup of resources meaning that for a given PipelineRun its TaskRuns may no longer be available on the cluster. The Dashboard falls back on data in the PipelineRun.status.taskRuns today to attempt to fill in the gaps and display most of the relevant information. We would lose this ability with the removal of that field. I don't think this is a blocker but figured it was worth mentioning so it doesn't come as a surprise later.

@jerop jerop force-pushed the tep-0100-proposal branch 3 times, most recently from 1a562ce to 552b968 Compare February 3, 2022 22:29
@jerop jerop force-pushed the tep-0100-proposal branch 2 times, most recently from b7e2fa9 to f59e74f Compare February 7, 2022 20:06
@jerop jerop force-pushed the tep-0100-proposal branch 2 times, most recently from a2fd7d4 to 76fd6e2 Compare February 7, 2022 20:33
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.

Thank you for this TEP!

I'm definitely in support of the idea. My main comment would be go actually go further and do not store the status of child resources at all, only type and part of the meta (name and possibly namespace/cluster in future).
Since we need to change the API to make this happen, and we need to go through the 9 months deprecation cycle, I see no reason to only remove part of the duplicated data.

I would love to see v1 only having the essential information to identify child resources and associate them to the part of the Pipeline they belong to (i.e. PipelineTask).

@jerop jerop force-pushed the tep-0100-proposal branch 2 times, most recently from 26c1d10 to 08b3bdc Compare February 14, 2022 16:59
@dibyom
Copy link
Member

dibyom commented Feb 14, 2022

For the duckv1beta1.Status part, I would not include it, because it means again extra space, extra writes to the PipelineRun so extra reconcile cycles triggered - and I don't see the advantage for the controller of having that information locally in the PipelineRun resource.

@afrittoli I agree that this will save space. How will this impact current API consumers? Will they have to change their logic to make extra API calls or are they doing that already? (/cc @tektoncd/cli-maintainers @tektoncd/dashboard-maintainers )

@jerop jerop force-pushed the tep-0100-proposal branch 3 times, most recently from 24e2508 to 11e707d Compare February 15, 2022 16:03
@jerop
Copy link
Member Author

jerop commented Feb 15, 2022

@afrittoli @pritidesai @dibyom @wlynch @vdemeester @AlanGreene @piyush-garg @imjasonh @bobcatfish - thanks for the reviews! updated the TEP and it's ready for another look :)

In [TEP-0100][tep-0100], we described the challenges caused by embedding
the complete `TaskRun` and `Run` status in `PipelineRuns`: performance
degradation, memory bloat, and lack of extensibility.

In this change we add the proposal to reduce the amount of information
stored about the status of `TaskRuns` and `Runs` to only the object
references in order to improve performance, reduce memory bloat
and improve extensibility. We also mark the TEP as implementable.

Co-authored-by: Lee Bernick <leebernick@google.com>

[tep-0100]: https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

/lgtm

This looks great, thanks for pushing on this! 👍

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2022
@tekton-robot tekton-robot merged commit ac21d53 into tektoncd:main Feb 15, 2022
@jerop
Copy link
Member Author

jerop commented Feb 15, 2022

Thanks everyone for the reviews! We resolved the open questions at API working group yesterday, and the TEP was updated as discussed. This TEP was merged offline, with all assigned approvers having signed off. If anyone has any more comments or concerns, please let me know 😁

@jerop jerop deleted the tep-0100-proposal branch June 11, 2022 01:57
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.