API: add a new `Stream` field to `PodLogOptions` #127360

knight42 · 2024-09-14T15:28:07Z

What type of PR is this?

/kind api-change
/kind feature

What this PR does / why we need it:

This PR adds the feature gate and API for kubernetes/enhancements#3288 as well as validation rules to prevent inconsistent values. Only the changes about API, kube-apiserver and kubelet is included in this PR.

I plan to update the kubectl after this PR is merged in case the volume of this PR is too large.

Special notes for your reviewer:

We have decided not to support the combination of a specific Stream and TailLines. Please refer to kubernetes/enhancements#4879 for rationale.

Does this PR introduce a user-facing change?

Add a `Stream` field to `PodLogOptions`, which allows clients to request certain log stream(stdout or stderr) of the container.
Please also note that the combination of a specific `Stream` and `TailLines` is not supported.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/3288-separate-stdout-from-stderr

k8s-ci-robot · 2024-09-14T15:28:09Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

knight42 · 2024-09-16T08:42:26Z

/test all

staging/src/k8s.io/component-base/version/base.go

k8s-ci-robot · 2024-11-07T16:49:39Z

@knight42: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-cos-alpha-features	`a137b08`	link	false	`/test pull-kubernetes-e2e-gce-cos-alpha-features`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

SergeyKanzhelev

/lgtm
/approve

k8s-ci-robot · 2024-11-07T18:21:01Z

LGTM label has been added.

Git tree hash: 663b4ced2a837955877de6722163df121ff455ec

k8s-ci-robot · 2024-11-07T18:21:09Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~api/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [SergeyKanzhelev,thockin]
~~pkg/generated/openapi/OWNERS~~ [thockin]
~~pkg/kubelet/OWNERS~~ [SergeyKanzhelev,thockin]
~~pkg/registry/core/pod/OWNERS~~ [SergeyKanzhelev,thockin]
~~staging/publishing/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/apiserver/pkg/endpoints/OWNERS~~ [thockin]
~~staging/src/k8s.io/cri-client/OWNERS~~ [SergeyKanzhelev,thockin]
~~test/e2e/feature/OWNERS~~ [SergeyKanzhelev,thockin]
~~test/e2e/node/OWNERS~~ [SergeyKanzhelev,thockin]
~~test/featuregates_linter/test_data/OWNERS~~ [SergeyKanzhelev,thockin]

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

SergeyKanzhelev · 2024-11-07T18:21:09Z

/unhold
/retest-required

knight42 · 2024-11-08T15:40:13Z

Finally... my first KEP implementation PR gets merged! 🎉 Thank you all for your patience and professional advice!

thockin · 2024-11-08T15:41:51Z

Congratulations. This was a hard one because URL params are not as well documented or exercised as other paths.

kannon92 · 2024-11-08T15:44:36Z

Nice job @knight42!

I see that we had a kubectl PR up for this but that seems to not be ready. Is this a blocker for this feature?

I guess one can use crictl or other ways to test this feature but we may want to target the kubectl feature for 1.33?

knight42 · 2024-11-08T16:08:30Z

I see that we had a kubectl PR up for this but that seems to not be ready. Is this a blocker for this feature?

I am working on it. This might be a blocker, as users cannot request a specific stream seamlessly without a updated kubectl. While they can use kubectl get --raw as e2e tests do, I think that would be a bit inconvinient.

I guess one can use crictl or other ways to test this feature

Actually, crictl needs to be updated as well to set the newly introduced stream parameter.

but we may want to target the kubectl feature for 1.33?

I am okay with that.

kannon92 · 2024-11-08T16:12:12Z

If you want to aim for 1.32, you will need to request an extension for that PR.

Since this is alpha, I don't think we need everything but we should maybe consider that a blocker for beta. @thockin WDYT?

thockin · 2024-11-08T16:32:55Z

Without a CLI we will not get any meaningful feedback, I think

…

On Fri, Nov 8, 2024 at 8:12 AM Kevin Hannon ***@***.***> wrote: If you want to aim for 1.32, you will need an extension. Since this is alpha, I don't think we need everything but we should maybe consider that a blocker for beta. @thockin <https://github.com/thockin> WDYT? — Reply to this email directly, view it on GitHub <#127360 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVA6IL4RASKUNU3GDVTZ7TPHLAVCNFSM6AAAAABOG6CLG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRVGE2DOMZSGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kannon92 · 2024-11-08T16:36:52Z

True but we also don't get meaningful feedback for an alpha feature..

kannon92 · 2024-11-08T16:41:10Z

So our option right now is:

We leave this PR in for 1.32.
When crictl takes 1.32 APIS, we can implement a feature for crictl with this feature.
Once 1.33 opens up, we can get the kubectl PR in.
Verify the feature works.

Or we aim to get kubectl in and if not, we revert this feature since there is not really a possible way to test this at the moment?

Given that it is alpha, I think our plan is fine but it does mean that @knight42 has a bit more work in 1.33 to get this ready for beta.

kannon92 · 2024-11-08T16:42:16Z

Why I am pressing this: kubernetes/enhancements#3288 (comment).

I'm trying to understand what we should do with this feature for 1.32.

SergeyKanzhelev · 2024-11-08T16:56:43Z

Why I am pressing this: kubernetes/enhancements#3288 (comment).

I'm trying to understand what we should do with this feature for 1.32.

I wonder if anybody who can review CLI is around to tell how close it is. It looks complete for me - enough to ask for an exception.

knight42 · 2024-11-08T17:14:51Z

When crictl takes 1.32 APIS, we can implement a feature for crictl with this feature.

I can update crictl once 1.32 is released.

thockin · 2024-11-08T18:06:39Z

I'm not arguing to revert this, but I am arguing to try to get the CLI part of this in (and in future to not merge half of a feature)

kannon92 · 2024-11-08T18:48:20Z

Well it looks like there is something up with this feature anyway.

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/128680/pull-e2e-gci-gce-alpha-enabled-default/1854616736976867328

We may have to revert anyway.

liggitt · 2024-11-08T20:22:36Z

Well it looks like there is something up with this feature anyway.

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/128680/pull-e2e-gci-gce-alpha-enabled-default/1854616736976867328

We may have to revert anyway.

it looks like this validation code is requiring the new Stream option be populated if the feature is enabled:

	if allowStreamSelection {
		if opts.Stream == nil {
			allErrs = append(allErrs, field.Required(field.NewPath("stream"), "must be specified"))
		} else {

isn't that breaking? elsewhere, we write:

	// opts.Stream can be nil because defaulting might not apply if no URL params are provided.
	if opts.Stream != nil {

kannon92 · 2024-11-08T20:43:50Z

pkg/registry/core/pod/rest/log.go

+	if !utilfeature.DefaultFeatureGate.Enabled(features.PodLogsQuerySplitStreams) {
+		logOpts.Stream = nil
+	}
+	if errs := validation.ValidatePodLogOptions(logOpts, utilfeature.DefaultFeatureGate.Enabled(features.PodLogsQuerySplitStreams)); len(errs) > 0 {


This seems to break the tests.

not just the tests, I think it breaks existing pod log API clients when the feature is enabled

kannon92 · 2024-11-08T21:48:26Z

I opened up #128712.

We can also revert this if that is the preferred method.

…-stdout-stderr-server-side" This reverts commit 9660e5c, reversing changes made to 9c571ab.

k8s-ci-robot requested review from andrewsykim and dchen1107 September 14, 2024 15:28

knight42 mentioned this pull request Sep 14, 2024

Split stdout and stderr log stream of container kubernetes/enhancements#3288

Open

10 tasks

knight42 force-pushed the feat/split-stdout-stderr-server-side branch from c1b5916 to dd48818 Compare September 16, 2024 08:41

k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/release Categorizes an issue or PR as relevant to SIG Release. labels Sep 16, 2024

knight42 commented Sep 16, 2024

View reviewed changes

staging/src/k8s.io/component-base/version/base.go Outdated Show resolved Hide resolved

knight42 changed the title ~~[WIP] API: Allow Stream to be set in PodLogOptions~~ API: Allow Stream to be set in PodLogOptions Sep 16, 2024

knight42 marked this pull request as ready for review September 16, 2024 15:07

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2024

k8s-ci-robot requested review from apelisse and caesarxuchao September 16, 2024 15:07

SergeyKanzhelev approved these changes Nov 7, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2024

k8s-ci-robot merged commit 9660e5c into kubernetes:master Nov 7, 2024
16 of 17 checks passed

k8s-ci-robot added this to the v1.32 milestone Nov 7, 2024

knight42 deleted the feat/split-stdout-stderr-server-side branch November 8, 2024 02:35

kannon92 reviewed Nov 8, 2024

View reviewed changes

kannon92 mentioned this pull request Nov 8, 2024

If one enables PodLogsQuerySplitStreams and aims to access logs without stream, a validation error occurs #128709

Closed

liggitt mentioned this pull request Nov 8, 2024

fix PodLogsQuerySplitStream if feature is enabled and using defaults #128712

Merged

bertinatto added a commit to bertinatto/kubernetes that referenced this pull request Dec 3, 2024

Revert "Merge pull request kubernetes#127360 from knight42/feat/split…

e485ce0

…-stdout-stderr-server-side" This reverts commit 9660e5c, reversing changes made to 9c571ab.

knight42 mentioned this pull request Dec 14, 2024

feat: retrieve specific log stream of a container kubernetes-sigs/cri-tools#1724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: add a new `Stream` field to `PodLogOptions` #127360

API: add a new `Stream` field to `PodLogOptions` #127360

knight42 commented Sep 14, 2024 •

edited

Loading

k8s-ci-robot commented Sep 14, 2024

knight42 commented Sep 16, 2024

k8s-ci-robot commented Nov 7, 2024

SergeyKanzhelev left a comment

k8s-ci-robot commented Nov 7, 2024

k8s-ci-robot commented Nov 7, 2024

SergeyKanzhelev commented Nov 7, 2024

knight42 commented Nov 8, 2024

thockin commented Nov 8, 2024

kannon92 commented Nov 8, 2024

knight42 commented Nov 8, 2024

kannon92 commented Nov 8, 2024 •

edited

Loading

thockin commented Nov 8, 2024 via email

kannon92 commented Nov 8, 2024

kannon92 commented Nov 8, 2024

kannon92 commented Nov 8, 2024

SergeyKanzhelev commented Nov 8, 2024

knight42 commented Nov 8, 2024

thockin commented Nov 8, 2024

kannon92 commented Nov 8, 2024

liggitt commented Nov 8, 2024

kannon92 Nov 8, 2024

liggitt Nov 8, 2024

kannon92 commented Nov 8, 2024

API: add a new Stream field to PodLogOptions #127360

API: add a new Stream field to PodLogOptions #127360

Conversation

knight42 commented Sep 14, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 14, 2024

knight42 commented Sep 16, 2024

k8s-ci-robot commented Nov 7, 2024

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 7, 2024

k8s-ci-robot commented Nov 7, 2024

SergeyKanzhelev commented Nov 7, 2024

knight42 commented Nov 8, 2024

thockin commented Nov 8, 2024

kannon92 commented Nov 8, 2024

knight42 commented Nov 8, 2024

kannon92 commented Nov 8, 2024 • edited Loading

thockin commented Nov 8, 2024 via email

kannon92 commented Nov 8, 2024

kannon92 commented Nov 8, 2024

kannon92 commented Nov 8, 2024

SergeyKanzhelev commented Nov 8, 2024

knight42 commented Nov 8, 2024

thockin commented Nov 8, 2024

kannon92 commented Nov 8, 2024

liggitt commented Nov 8, 2024

kannon92 Nov 8, 2024

Choose a reason for hiding this comment

liggitt Nov 8, 2024

Choose a reason for hiding this comment

kannon92 commented Nov 8, 2024

API: add a new `Stream` field to `PodLogOptions` #127360

API: add a new `Stream` field to `PodLogOptions` #127360

knight42 commented Sep 14, 2024 •

edited

Loading

kannon92 commented Nov 8, 2024 •

edited

Loading