API: add a new `Stream` field to `PodLogOptions` by knight42 · Pull Request #127360 · kubernetes/kubernetes

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

Signed-off-by: Jian Zeng <[email protected]>

The `k8s.io/utils/ptr` should be allowed for import within ./pkg/apis/core Signed-off-by: Jian Zeng <[email protected]>

knight42 · 2024-11-07T14:23:39Z

/test pull-kubernetes-e2e-gce-cos-alpha-features

@SergeyKanzhelev @thockin I've updated the release note to reflect that a specific Stream and TailLines are mutually exclusive. Please feel free to make any additional updates if needed.

thockin · 2024-11-07T16:44:13Z

There's no CLI interface for this yet, right?

knight42 · 2024-11-07T16:49:05Z

There's no CLI interface for this yet, right?

Not yet, the kubectl side changes #128159 depends on this PR.

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.

Details

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.

Details

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

Details

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

+	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.

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

Closed

11 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

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

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

knight42 added 2 commits November 7, 2024 13:52

test: add e2e test

1a2c557

Signed-off-by: Jian Zeng <[email protected]>

chore: update import-restrictions.yaml

a137b08

The `k8s.io/utils/ptr` should be allowed for import within ./pkg/apis/core Signed-off-by: Jian Zeng <[email protected]>

SergeyKanzhelev approved these changes Nov 7, 2024

View reviewed changes

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

knight42 mentioned this pull request Dec 14, 2024

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

Merged

BenTheElder mentioned this pull request Jul 17, 2025

get logs started failing on GKE v1.32: PodLogOptions XXX is invalid: stream: Forbidden: may not be specified #133005

Closed

Uh oh!

Conversation

knight42 commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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.:

Uh oh!

k8s-ci-robot commented Sep 14, 2024

Uh oh!

knight42 commented Sep 16, 2024

Uh oh!

Uh oh!

knight42 commented Nov 7, 2024

Uh oh!

thockin commented Nov 7, 2024

Uh oh!

knight42 commented Nov 7, 2024

Uh oh!

k8s-ci-robot commented Nov 7, 2024

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 7, 2024

Uh oh!

k8s-ci-robot commented Nov 7, 2024

Uh oh!

SergeyKanzhelev commented Nov 7, 2024

Uh oh!

knight42 commented Nov 8, 2024

Uh oh!

thockin commented Nov 8, 2024

Uh oh!

kannon92 commented Nov 8, 2024

Uh oh!

knight42 commented Nov 8, 2024

Uh oh!

kannon92 commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thockin commented Nov 8, 2024 via email

Uh oh!

kannon92 commented Nov 8, 2024

Uh oh!

kannon92 commented Nov 8, 2024

Uh oh!

kannon92 commented Nov 8, 2024

Uh oh!

SergeyKanzhelev commented Nov 8, 2024

Uh oh!

knight42 commented Nov 8, 2024

Uh oh!

thockin commented Nov 8, 2024

Uh oh!

kannon92 commented Nov 8, 2024

Uh oh!

liggitt commented Nov 8, 2024

Uh oh!

kannon92 Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

liggitt Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

kannon92 commented Nov 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

knight42 commented Sep 14, 2024 •

edited

Loading

kannon92 commented Nov 8, 2024 •

edited

Loading