Fix no logs in stdout/stderr if uses stdoutConfig #6162

chengjoey · 2023-02-14T15:17:09Z

Changes

fix #6136
used to only collect stdout and err to a file if the std{out/err}Path is specified, now add both os.std{out/err} and the std{out/err}Path to multiwriter to collect logs.

there is a full test to verify the stdoutConfig, in addition, this pr adds the verification of whether to print the log to standard output in the TestRealRunnerStdoutAndStderrPaths test

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

users can still view the output through the Pod log API if stdoutConfig.path or stderrConfig.path is specified

chengjoey · 2023-02-14T15:19:57Z

/kind bug

chengjoey · 2023-02-14T15:22:50Z

/assign @wlynch
could you please take a review?🙏

tekton-robot · 2023-02-14T15:23:16Z

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
cmd/entrypoint/runner.go	82.8%	83.3%	0.5

tekton-robot · 2023-02-14T15:25:06Z

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

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	82.8%	85.2%	2.4

vdemeester

This will probably need a release-note entry 👼🏼

chengjoey · 2023-02-15T01:34:26Z

This will probably need a release-note entry 👼🏼

thanks @vdemeester , i added the release-note and marked the release-note checklist

chengjoey · 2023-02-15T01:34:59Z

/test pull-tekton-pipeline-integration-tests

afrittoli · 2023-02-16T13:37:04Z

@chengjoey thanks for this fix.

As you pointed out, there is a test for this, which was already succeeding, which unfortunately means there is full coverage for this change - it could be inadvertently broken again and the tests would not catch it.
Would it be possible to add test coverage to make sure we do not lose pod logs when using stdoutConfig?

afrittoli · 2023-02-16T13:37:54Z

@wlynch it would be nice if you could review this one since you changed this code last 🙏

used to only collect stdout and err to a file if the std{out/err}Path is specified, now add both os.std{out/err} and the std{out/err}Path to multiwriter to collect logs

tekton-robot · 2023-02-16T15:44:02Z

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
cmd/entrypoint/runner.go	84.4%	83.3%	-1.0

tekton-robot · 2023-02-16T15:44:26Z

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

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	84.4%	85.2%	0.8

chengjoey · 2023-02-16T15:44:58Z

@chengjoey thanks for this fix.

As you pointed out, there is a test for this, which was already succeeding, which unfortunately means there is full coverage for this change - it could be inadvertently broken again and the tests would not catch it. Would it be possible to add test coverage to make sure we do not lose pod logs when using stdoutConfig?

Of course, i added verification in TestRealRunnerStdoutAndStderrPaths that whether realRunner print the log to std{out/err}. Thanks for the reminder, your advice is great!

wlynch

One thing, otherwise LGTM!

wlynch · 2023-02-16T19:04:54Z

cmd/entrypoint/runner_test.go

+	oldStdout := os.Stdout // keep backup of the real stdout
+	outReader, outWriter, _ := os.Pipe()
+	os.Stdout = outWriter
+
+	oldStderr := os.Stderr
+	errReader, errWriter, _ := os.Pipe()
+	os.Stderr = errWriter


This seems brittle since it modifies global state.

How about we:

Modify realRunner to take in io.Writers

Have main.go initialize the multiwriters if the flag is set.

If the writers are nil, default to os.Stdout/Stderr

This way in tests we can just use a buffer and remove a bunch of the path manipulation?

The test here is to verify that logs can be printed to standard output. If you modify realRunner to accept io.Writers, you need to pass in os.stdout, which is the same as the current situation.

chengjoey · 2023-02-17T05:37:19Z

/test pull-tekton-pipeline-alpha-integration-tests

Yongxuanzhang · 2023-03-22T17:56:22Z

/assign

Yongxuanzhang · 2023-03-27T20:31:53Z

cmd/entrypoint/runner.go

-		return nil, fmt.Errorf("error creating pipe: %w", err)
-	}
-
+// be used for multiple streams if desired. note that close after use


Is this docstring missing after "note that close after use"?

It's not missing, I just modified the note here and added it to where it was used.
notes related to teaReader are no longer needed here

tekton-robot · 2023-03-27T20:50:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester, Yongxuanzhang

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:

~~OWNERS~~ [vdemeester]

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

Yongxuanzhang · 2023-03-28T14:37:23Z

/lgtm

vdemeester · 2023-04-06T08:35:30Z

@chengjoey @pritidesai @jerop should this be cherry-picked in the latest LTS ? (0.41 and 0.44)

chengjoey · 2023-04-06T14:39:41Z

/cherry-pick release-v0.41.x
/cherry-pick release-v0.42.x
/cherry-pick release-v0.43.x
/cherry-pick release-v0.44.x

tekton-robot · 2023-04-06T14:40:32Z

@chengjoey: #6162 failed to apply on top of branch "release-v0.41.x":

Applying: Fix no logs in stdout/stderr if uses stdoutConfig
Using index info to reconstruct a base tree...
M	cmd/entrypoint/runner.go
M	cmd/entrypoint/runner_test.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/entrypoint/runner_test.go
CONFLICT (content): Merge conflict in cmd/entrypoint/runner_test.go
Auto-merging cmd/entrypoint/runner.go
CONFLICT (content): Merge conflict in cmd/entrypoint/runner.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix no logs in stdout/stderr if uses stdoutConfig
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.41.x
/cherry-pick release-v0.42.x
/cherry-pick release-v0.43.x
/cherry-pick release-v0.44.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…ektoncd#6162) used to only collect stdout and err to a file if the std{out/err}Path is specified, now add both os.std{out/err} and the std{out/err}Path to multiwriter to collect logs io/ioutil has been deprecaed in Go 1.16 Signed-off-by: chengjoey <zchengjoey@gmail.com>

chengjoey · 2023-04-06T15:14:01Z

@chengjoey @pritidesai @jerop should this be cherry-picked in the latest LTS ? (0.41 and 0.44)

when i try to cherry-pick to 0.41, there are some conflicts, I manually submitted a hotfix pr, is this the correct hotfix process

…6162) used to only collect stdout and err to a file if the std{out/err}Path is specified, now add both os.std{out/err} and the std{out/err}Path to multiwriter to collect logs io/ioutil has been deprecaed in Go 1.16 Signed-off-by: chengjoey <zchengjoey@gmail.com>

chengjoey · 2023-04-07T02:12:19Z

/cherry-pick release-v0.44.x

tekton-robot · 2023-04-07T02:12:58Z

@chengjoey: new pull request created: #6506

In response to this:

/cherry-pick release-v0.44.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Feb 14, 2023

tekton-robot requested review from dibyom and imjasonh February 14, 2023 15:17

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 14, 2023

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 14, 2023

tekton-robot assigned wlynch Feb 14, 2023

vdemeester approved these changes Feb 14, 2023

View reviewed changes

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Feb 14, 2023

Fix no logs in stdout/stderr if uses stdoutConfig

4587a7f

used to only collect stdout and err to a file if the std{out/err}Path is specified, now add both os.std{out/err} and the std{out/err}Path to multiwriter to collect logs

chengjoey force-pushed the fix/stdout-log branch from 2efb66d to 4587a7f Compare February 16, 2023 15:37

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 16, 2023

wlynch reviewed Feb 16, 2023

View reviewed changes

tekton-robot assigned Yongxuanzhang Mar 22, 2023

Yongxuanzhang approved these changes Mar 27, 2023

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2023

tekton-robot merged commit 1689636 into tektoncd:main Mar 28, 2023

lbernick mentioned this pull request Apr 4, 2023

increase timer for TestRealRunnerTimeout #6409

Closed

7 tasks

chengjoey mentioned this pull request Apr 6, 2023

hotfix 0.41.x no logs in stdout/stderr if uses stdoutConfig #6503

Merged

7 tasks

tekton-robot mentioned this pull request Apr 7, 2023

[release-v0.44.x] Fix no logs in stdout/stderr if uses stdoutConfig #6506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix no logs in stdout/stderr if uses stdoutConfig #6162

Fix no logs in stdout/stderr if uses stdoutConfig #6162

chengjoey commented Feb 14, 2023 •

edited

Loading

chengjoey commented Feb 14, 2023

chengjoey commented Feb 14, 2023

tekton-robot commented Feb 14, 2023

tekton-robot commented Feb 14, 2023

vdemeester left a comment

chengjoey commented Feb 15, 2023

chengjoey commented Feb 15, 2023

afrittoli commented Feb 16, 2023

afrittoli commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

chengjoey commented Feb 16, 2023

wlynch left a comment

wlynch Feb 16, 2023

chengjoey Feb 17, 2023

chengjoey commented Feb 17, 2023

Yongxuanzhang commented Mar 22, 2023

Yongxuanzhang Mar 27, 2023

chengjoey Mar 28, 2023

tekton-robot commented Mar 27, 2023

Yongxuanzhang commented Mar 28, 2023

vdemeester commented Apr 6, 2023

chengjoey commented Apr 6, 2023

tekton-robot commented Apr 6, 2023

chengjoey commented Apr 6, 2023

chengjoey commented Apr 7, 2023

tekton-robot commented Apr 7, 2023

Fix no logs in stdout/stderr if uses stdoutConfig #6162

Fix no logs in stdout/stderr if uses stdoutConfig #6162

Conversation

chengjoey commented Feb 14, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

chengjoey commented Feb 14, 2023

chengjoey commented Feb 14, 2023

tekton-robot commented Feb 14, 2023

tekton-robot commented Feb 14, 2023

vdemeester left a comment

Choose a reason for hiding this comment

chengjoey commented Feb 15, 2023

chengjoey commented Feb 15, 2023

afrittoli commented Feb 16, 2023

afrittoli commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

chengjoey commented Feb 16, 2023

wlynch left a comment

Choose a reason for hiding this comment

wlynch Feb 16, 2023

Choose a reason for hiding this comment

chengjoey Feb 17, 2023

Choose a reason for hiding this comment

chengjoey commented Feb 17, 2023

Yongxuanzhang commented Mar 22, 2023

Yongxuanzhang Mar 27, 2023

Choose a reason for hiding this comment

chengjoey Mar 28, 2023

Choose a reason for hiding this comment

tekton-robot commented Mar 27, 2023

Yongxuanzhang commented Mar 28, 2023

vdemeester commented Apr 6, 2023

chengjoey commented Apr 6, 2023

tekton-robot commented Apr 6, 2023

chengjoey commented Apr 6, 2023

chengjoey commented Apr 7, 2023

tekton-robot commented Apr 7, 2023

chengjoey commented Feb 14, 2023 •

edited

Loading