KEP 3288: Split stdout and stderr log stream #3289

knight42 · 2022-05-01T14:08:38Z

Signed-off-by: Jian Zeng anonymousknight96@gmail.com

One-line PR description: Split stdout and stderr log stream of container

Issue link: Split stdout and stderr log stream of container #3288

Other comments:

Rendered

keps/sig-node/3288-separate-stdout-from-stderr/README.md

knight42 · 2022-05-12T02:58:54Z

/cc @mrunalp

Hi! Please take a look when you are available 🚀

keps/sig-node/3288-separate-stdout-from-stderr/README.md

sftim · 2022-05-24T15:31:23Z

keps/sig-node/3288-separate-stdout-from-stderr/README.md

+<!--
+What other approaches did you consider, and why did you rule them out? These do
+not need to be as detailed as the proposal, but should include enough
+information to express the idea and why it was not acceptable.
+-->


I presume “just one multiplexed log stream” is the only alternative we could consider here?

Did you mean we return the combined stdout and stderr and let the client somehow filter out the unwanted stream?

Another option could be kubectl log directing these 2 streams to stdout/stderr. The downside of that approach would be distinguishing the errors from kubectl log vs. what's coming from the container process.

kubectl log directing these 2 streams to stdout/stderr

That actually sounds like something that'd be useful to offer, and doesn't conflict with this KEP.

return the combined stdout and stderr and let the client somehow filter out the unwanted stream?

If an API client got a stream of CRI-format logs, the demultiplexing could be client side.

To be clear, I wouldn't implement that. We should list the alternative and then briefly explain why the approach we've selected is better.

+1 on listing alternative explicitly

I have updated the Alternatives section to describe this alternative approach.

keps/sig-node/3288-separate-stdout-from-stderr/README.md

dchen1107 · 2022-05-31T17:41:46Z

Thanks for making the proposal. Thanks @mrunalp to bring this up to today's SIG Node. We quickly discussed it at the meeting earlier, and will track it with 1.25 release. Thanks!

SergeyKanzhelev · 2022-05-31T20:28:13Z

keps/sig-node/3288-separate-stdout-from-stderr/README.md

+...
+    // If set, return the given log stream of the container.
+	// Otherwise, the combined stdout and stderr from the container is returned.
+	// Available values are: "stdout", "stderr", "" (empty string).


I'd suggest to have "all" as an available value so it can be set explicitly to all - it will be more user friendly than explicit empty string or omitted parameter.

I am up for that and I have added "all" as available value.

SergeyKanzhelev · 2022-05-31T20:31:32Z

keps/sig-node/3288-separate-stdout-from-stderr/README.md

+    // If set, return the given log stream of the container.
+	// Otherwise, the combined stdout and stderr from the container is returned.
+	// Available values are: "stdout", "stderr", "" (empty string).
+    Stream string


is it a string for implementation simplicity or expect any other values to be added later? Perhaps a custom type with three constants instead?

Perhaps a custom type with three constants instead?

Yeah I think this is better, I have updated the proposal.

SergeyKanzhelev · 2022-05-31T21:07:26Z

keps/sig-node/3288-separate-stdout-from-stderr/README.md

+}
+```
+
+### Changes of kubelet


will there be any issues with TailLines log option? What will be the behavior of this log option when only one stream was specified? (I haven't looked deep in the code)

will there be any issues with TailLines log option?

If TailLines is set, only the last N lines in the log file whose stream match the one user specified would be returned.

What will be the behavior of this log option when only one stream was specified?

Then only that log stream from the beginning would be returned.

I haven't looked into code, I just anticipate possible additional changes needed to ensure that the counting of TailLines happens properly and only logs from a specified stream were counted. Definitely add this to the test plan section

I just anticipate possible additional changes needed to ensure that the counting of TailLines happens properly and only logs from a specified stream were counted.

Actually my original thought is slightly different, I expected kubelet still examine the last TailLines of the log file, but only returns lines that matches the stream user specified.

I also understand my original thought is easy to implement, though it might confuse users. I agree that the most intutive way is only counting logs from a specified stream, and it would require extra work to be done in kubelet.

+1 on counting logs from a specified stream.

SergeyKanzhelev

/lgtm

this feels small enough to include into 1.25. It's important to ensure that the test coverage is there

deads2k · 2022-06-09T20:35:26Z

keps/sig-node/3288-separate-stdout-from-stderr/README.md

+
+Yes.
+
+###### What happens if we reenable the feature if it was previously rolled back?


requires an answer, but since this is not persisted, I don't think there's a concern here.

deads2k · 2022-06-09T20:35:44Z

keps/sig-node/3288-separate-stdout-from-stderr/README.md

+
+###### What happens if we reenable the feature if it was previously rolled back?
+
+###### Are there any tests for feature enablement/disablement?


requires an answer

deads2k · 2022-06-09T20:40:20Z

minor PRR comments and then PRR lgtm

knight42 · 2022-06-10T03:06:35Z

The failed tests seems to be caused by a recent merged PR #3281

deads2k · 2022-06-10T15:29:30Z

PRR lgtm. The sig still owns review of the overall feature.

/approve

knight42 · 2022-06-10T16:28:58Z

@deads2k Thanks a lot!

Gently ping @dchen1107 @derekwaynecarr for final approval 🚀 . It would be great if we could ship it before enhancements freeze.

SergeyKanzhelev

/lgtm

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

SergeyKanzhelev

/lgtm
/assign @dchen1107

dchen1107 · 2022-06-15T00:11:07Z

/lgtm
/approve

k8s-ci-robot · 2022-06-15T00:11:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, deads2k, knight42, SergeyKanzhelev

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:

~~keps/prod-readiness/OWNERS~~ [deads2k]
~~keps/sig-node/OWNERS~~ [dchen1107]

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

cathchu · 2022-07-27T16:46:26Z

Hi @knight42

1.25 Release Docs Shadow here. Does this enhancement work planned for 1.25 require any new docs or modification to existing docs?
If so, please follows the steps here to open a PR against dev-1.25 branch in the k/website repo. This PR can be just a placeholder at this time and must be created before August 4.
Also, take a look at Documenting for a release to get yourself familiarize with the docs requirement for the release.
Thank you!

k8s-ci-robot requested review from dchen1107 and derekwaynecarr May 1, 2022 14:08

knight42 force-pushed the feat/split-stdout-stderr branch from ac18b66 to 0d466c6 Compare May 1, 2022 14:09

This was referenced May 1, 2022

Split stdout and stderr log stream of container #3288

Open

kubectl logs should allow to discriminate between pod's stderr and stdout kubernetes/kubernetes#28167

Open

knight42 force-pushed the feat/split-stdout-stderr branch from 0d466c6 to ec99926 Compare May 8, 2022 16:20

pohly reviewed May 9, 2022

View reviewed changes

keps/sig-node/3288-separate-stdout-from-stderr/README.md Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from mrunalp May 12, 2022 02:58

kikisdeliveryservice reviewed May 13, 2022

View reviewed changes

keps/sig-node/3288-separate-stdout-from-stderr/README.md Show resolved Hide resolved

sftim reviewed May 24, 2022

View reviewed changes

mrunalp reviewed May 31, 2022

View reviewed changes

keps/sig-node/3288-separate-stdout-from-stderr/README.md Outdated Show resolved Hide resolved

mrunalp reviewed May 31, 2022

View reviewed changes

keps/sig-node/3288-separate-stdout-from-stderr/README.md Outdated Show resolved Hide resolved

SergeyKanzhelev reviewed May 31, 2022

View reviewed changes

knight42 force-pushed the feat/split-stdout-stderr branch 3 times, most recently from 328368c to 3059d92 Compare June 1, 2022 17:20

SergeyKanzhelev approved these changes Jun 3, 2022

View reviewed changes

k8s-ci-robot assigned SergeyKanzhelev Jun 3, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2022

deads2k reviewed Jun 9, 2022

View reviewed changes

dchen1107 added this to the v1.25 milestone Jun 9, 2022

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2022

SergeyKanzhelev approved these changes Jun 10, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2022

knight42 added 6 commits June 11, 2022 21:57

KEP 3288: Split stdout and stderr log stream

33ff07a

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

Update test plan

a6e88b3

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

address comments

a29aa12

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

Supplement the test plan

78cda8a

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

Add PRR

6d5bcaa

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

Polish "Feature Enablement and Rollback" section

fabdb56

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>

knight42 force-pushed the feat/split-stdout-stderr branch from d17de17 to fabdb56 Compare June 11, 2022 13:57

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2022

SergeyKanzhelev approved these changes Jun 11, 2022

View reviewed changes

k8s-ci-robot assigned dchen1107 Jun 11, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2022

k8s-ci-robot merged commit ce61e37 into kubernetes:master Jun 15, 2022

knight42 deleted the feat/split-stdout-stderr branch June 15, 2022 01:22

saiqulhaq-hh mentioned this pull request Nov 6, 2023

Ability to get separated stdout and stderr jwendell/fnkube#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP 3288: Split stdout and stderr log stream #3289

KEP 3288: Split stdout and stderr log stream #3289

knight42 commented May 1, 2022 •

edited

Loading

knight42 commented May 12, 2022

sftim May 24, 2022

knight42 May 26, 2022

mrunalp May 31, 2022

sftim May 31, 2022

SergeyKanzhelev May 31, 2022

knight42 Jun 1, 2022

dchen1107 commented May 31, 2022

SergeyKanzhelev May 31, 2022

knight42 Jun 1, 2022

SergeyKanzhelev May 31, 2022

knight42 Jun 1, 2022

SergeyKanzhelev May 31, 2022

knight42 Jun 1, 2022

SergeyKanzhelev Jun 3, 2022

knight42 Jun 7, 2022

dchen1107 Jun 15, 2022

SergeyKanzhelev left a comment

deads2k Jun 9, 2022

knight42 Jun 10, 2022

deads2k Jun 9, 2022

knight42 Jun 10, 2022

deads2k commented Jun 9, 2022

knight42 commented Jun 10, 2022

deads2k commented Jun 10, 2022

knight42 commented Jun 10, 2022

SergeyKanzhelev left a comment

SergeyKanzhelev left a comment

dchen1107 commented Jun 15, 2022

k8s-ci-robot commented Jun 15, 2022

cathchu commented Jul 27, 2022


		Yes.

		###### What happens if we reenable the feature if it was previously rolled back?


		###### What happens if we reenable the feature if it was previously rolled back?

		###### Are there any tests for feature enablement/disablement?

KEP 3288: Split stdout and stderr log stream #3289

KEP 3288: Split stdout and stderr log stream #3289

Conversation

knight42 commented May 1, 2022 • edited Loading

knight42 commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented May 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 9, 2022

knight42 commented Jun 10, 2022

deads2k commented Jun 10, 2022

knight42 commented Jun 10, 2022

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

dchen1107 commented Jun 15, 2022

k8s-ci-robot commented Jun 15, 2022

cathchu commented Jul 27, 2022

knight42 commented May 1, 2022 •

edited

Loading