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-0127: Larger results using sidecar logs: Validation, documentation and examples #5695

Merged

Conversation

chitrangpatel
Copy link
Contributor

@chitrangpatel chitrangpatel commented Oct 27, 2022

Prior to this, we were extracting results from tasks via the termination messages which had a limit of only 4 KB per pod. If users had many results then the results would need to become smaller to obey the upper limit of 4 KB.

We now run a dedicated sidecar that has access to the results of all the steps. This sidecar prints out the result and its content to stdout. The logs of the sidecar are parsed by the taskrun controller and the results updated instead of termination logs. We set an upper limit on the results to 4KB by default (configurable) and users can have as many such results as needed.

This PR is the last one on issue #5851 which deals with validation of sidecar name, documentation and examples.

We validate that the user does not accidentally provide the reserved sidecar name as the name of their sidecar while the results-from feature has been set to sidecar-logs.

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs included if any changes are user facing
  • Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Admission webhook validation to ensure that the reserved sidecar name is not used by the user, user documentation and larger results TaskRun and PipelineRun examples.

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 27, 2022
@chitrangpatel
Copy link
Contributor Author

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 27, 2022
@chitrangpatel
Copy link
Contributor Author

/assign @jerop

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 78.3% 77.4% -0.9
pkg/pod/entrypoint.go 89.5% 89.8% 0.2
pkg/pod/pod.go 89.5% 91.0% 1.5
pkg/pod/status.go 90.9% 85.4% -5.6
pkg/reconciler/taskrun/taskrun.go 81.3% 80.7% -0.5

@chitrangpatel
Copy link
Contributor Author

/retest

docs/install.md Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/taskrun_types.go Outdated Show resolved Hide resolved
pkg/entrypoint/entrypointer.go Outdated Show resolved Hide resolved
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 6890644 to 06bbc1d Compare October 28, 2022 19:51
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 78.3% 77.4% -0.9
pkg/pod/entrypoint.go 89.5% 89.8% 0.2
pkg/pod/pod.go 89.5% 91.0% 1.5
pkg/pod/status.go 90.9% 85.4% -5.6
pkg/reconciler/taskrun/taskrun.go 81.3% 80.7% -0.5

@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 06bbc1d to 4b7d862 Compare October 28, 2022 21:48
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 78.3% 77.4% -0.9
pkg/pod/entrypoint.go 89.5% 89.8% 0.2
pkg/pod/pod.go 89.5% 91.0% 1.5
pkg/pod/status.go 90.9% 85.4% -5.6
pkg/reconciler/taskrun/taskrun.go 81.3% 80.7% -0.5

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2022
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch 2 times, most recently from a9484cf to 38fa068 Compare October 29, 2022 03:53
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2022
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 78.3% 77.4% -0.9
pkg/pod/entrypoint.go 89.5% 89.8% 0.2
pkg/pod/pod.go 89.5% 91.0% 1.5
pkg/pod/status.go 90.9% 85.4% -5.5
pkg/reconciler/taskrun/taskrun.go 81.3% 80.7% -0.5

@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 38fa068 to 5368fd1 Compare October 29, 2022 13:34
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 78.3% 77.4% -0.9
pkg/pod/entrypoint.go 89.5% 89.8% 0.2
pkg/pod/pod.go 89.5% 91.0% 1.5
pkg/pod/status.go 90.9% 85.4% -5.5
pkg/reconciler/taskrun/taskrun.go 81.3% 80.7% -0.5

cmd/entrypoint/main.go Outdated Show resolved Hide resolved
pkg/apis/config/feature_flags.go Outdated Show resolved Hide resolved
pkg/pod/pod.go Outdated Show resolved Hide resolved
pkg/pod/entrypoint.go Outdated Show resolved Hide resolved
docs/install.md Show resolved Hide resolved
pkg/reconciler/taskrun/taskrun.go Outdated Show resolved Hide resolved
pkg/pod/status.go Outdated Show resolved Hide resolved
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 5368fd1 to 6f7b4a0 Compare November 2, 2022 21:30
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2022
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 6f7b4a0 to 27e6a47 Compare November 2, 2022 21:34
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2022
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 81.2% 80.3% -0.9
pkg/pod/pod.go 89.5% 90.0% 0.5
pkg/pod/status.go 90.9% 82.1% -8.8
pkg/reconciler/taskrun/taskrun.go 81.4% 80.9% -0.5
pkg/sidecarlogresults/sidecarlogresults.go Do not exist 95.0%

@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 27e6a47 to 2c61ea8 Compare November 3, 2022 02:48
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 57b0cc3 to 2e39083 Compare December 13, 2022 16:50
@chitrangpatel chitrangpatel changed the title TEP-0127: Larger results using sidecar logs: Validation and documentation TEP-0127: Larger results using sidecar logs: Validation, documentation and wrap up Dec 13, 2022
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch 3 times, most recently from c0173a8 to e8c0eea Compare December 14, 2022 17:01
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 97.0% 97.0% 0.1
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 97.2% 0.1

@chitrangpatel chitrangpatel changed the title TEP-0127: Larger results using sidecar logs: Validation, documentation and wrap up TEP-0127: Larger results using sidecar logs: Validation, documentation and examples Dec 14, 2022
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from e8c0eea to 335ee18 Compare December 14, 2022 19:45
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 335ee18 to 35a6a2e Compare December 14, 2022 19:47
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 97.0% 97.0% 0.1
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 97.2% 0.1

…n and example tests

Prior to this, we were extracting results from tasks via the termination messages which had a limit of only 4 KB per pod. If users had many results then the results would need to become smaller to obey the upper limit of 4 KB.

We now run a dedicated sidecar that has access to the results of all the steps. This sidecar prints out the result and its content to stdout. The logs of the sidecar are parsed by the taskrun controller and the results updated instead of termination logs. We set an upper limit on the results to 4KB by default (configurable) and users can have as many such results as needed.

This PR is the last one on issue tektoncd#5851 which deals with validation of sidecar name, documentation and examples.

We validate that the user does not accidentally provide the reserved sidecar name as the name of their sidecar while the `results-from` feature has been set to `sidecar-logs`.
@chitrangpatel chitrangpatel force-pushed the larger-results-using-sidecar-logs branch from 35a6a2e to f91575f Compare December 14, 2022 19:53
@chitrangpatel
Copy link
Contributor Author

chitrangpatel commented Dec 14, 2022

/hold cancel

All PRs prior to this have been merged. This is the last of one.

@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 Dec 14, 2022
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 97.0% 97.0% 0.1
pkg/apis/pipeline/v1beta1/task_validation.go 97.1% 97.2% 0.1

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.

thank you @chitrangpatel!

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bendory, jerop

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 Dec 15, 2022
@jerop
Copy link
Member

jerop commented Dec 15, 2022

@abayer @dibyom please take a look

@abayer
Copy link
Contributor

abayer commented Dec 15, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2022
@dibyom
Copy link
Member

dibyom commented Dec 15, 2022

@chitrangpatel - Can we modify the release notes with a line that describes the feature from an end user standpoint? And also mention that sidecars cannot be named the reserved sidecar name?

@chitrangpatel
Copy link
Contributor Author

@chitrangpatel - Can we modify the release notes with a line that describes the feature from an end user standpoint? And also mention that sidecars cannot be named the reserved sidecar name?

Sure!

@tekton-robot tekton-robot merged commit 427f7b8 into tektoncd:main Dec 15, 2022
JeromeJu pushed a commit to JeromeJu/pipeline that referenced this pull request Dec 19, 2022
This PR implements a part of TEP-0127 - entrypoint and sidecar binary (see tektoncd#5695).

The sidecar binary relies on the Postfiles generated by the steps in /tekton/run/<step> to determine when a step has finished running. Once all the steps are done or if one of the step fails, the sidecar binary logs results to stdout.
JeromeJu pushed a commit to JeromeJu/pipeline that referenced this pull request Jan 3, 2023
This PR implements a part of TEP-0127 - entrypoint and sidecar binary (see tektoncd#5695).

The sidecar binary relies on the Postfiles generated by the steps in /tekton/run/<step> to determine when a step has finished running. Once all the steps are done or if one of the step fails, the sidecar binary logs results to stdout.
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/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.