runtime log verbosity level changes #114609

pohly · 2022-12-20T15:12:28Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This enables changing the verbosity level of JSON output at runtime.

Does this PR introduce a user-facing change?

kube-proxy, kube-scheduler and kubelet have HTTP APIs for changing the logging verbosity at runtime. This now also works for JSON output.

pohly · 2022-12-20T19:24:51Z

staging/src/k8s.io/component-base/logs/api/v1/registry.go

-	// If provided, the caller must ensure that it is called
-	// periodically (if desired) and at program exit.
-	Create(c LoggingConfiguration) (log logr.Logger, flush func())
+	Create(c LoggingConfiguration) (logr.Logger, RuntimeControl)


It's unfortunate that Go API changes inside k8s.io/component-base/logs are needed, but the effect is limited to code which provides its own logging format, which might be none at all outside of k/k.

The "v1" refers to the user-facing API (configuration file, command line flags) and that doesn't change.

I looked (again) at whether some of this code could be moved out of the "v1" package, but that quickly fails due to circular dependencies.

k8s-triage-robot · 2022-12-20T19:53:11Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

pohly · 2022-12-21T16:08:42Z

As this PR will need an API review due to the changes in api/v1, let me add another commit with a change in that directory: its an enabler for writing tests which need to redirect output instead of using os.Stdout and os.Stderr.

fedebongio · 2023-01-03T21:09:32Z

/assign @logicalhan @serathius
/triage accepted

serathius · 2023-01-09T10:49:40Z

staging/src/k8s.io/component-base/logs/internal/setverbositylevel/setverbositylevel.go

+// k8s.io/component-base/logs (uses the callbacks) to
+// k8s.io/component-base/logs/api/v1 (adds them). Not all users of the logs
+// package also use the API.
+package setverbositylevel


Interface of this package is a little too open and doesn't follow golang naming scheme (https://go.dev/doc/effective_go#package-names).

Could we expose AddCallback and SetVerbosity functions instead of Mutex and Callbacks to avoid misuse?

Instead naming package setverbositylevel could we rename it to just verbosity? This would allow external packages to call function verbosity.Set.

doesn't follow golang naming scheme (https://go.dev/doc/effective_go#package-names).

setverbositylevel.Mutex and setverbositylevel.Callbacks sounds okay to me.

to avoid misuse?

This is an internal package - nothing outside of k8s.io/component-base/logs has access to it. Therefore I wasn't worried about preventing misuse and instead went for simplicity. Does that make sense?

Instead naming package setverbositylevel could we rename it to just verbosity?

I can, but is verbosity.Callbacks really a better name?

I don't think it's worth blocking, however I still think verbosity is a better package name.

I can, but is verbosity.Callbacks really a better name?

Maybe verbosity.ChangeCallbacks. Usually you would prefer verbosity.RegisterCallback.

Let's continue with the simpler approach of not adding functions to the package. The package only exists to avoid circular package dependencies, not to protect variables against undesired access.

serathius · 2023-01-17T08:34:08Z

/lgtm
/approve

k8s-ci-robot · 2023-01-17T08:34:14Z

LGTM label has been added.

Git tree hash: ee3a14e58afc72d99f80a7f36923cac8fbf1e6ff

Having the sub-package using the same name as the parent package makes no sense. This seems to be a cut-and-paste error.

The GlogSetter method is used by three components to change verbosity at runtime through HTTP APIs. This used to work only for text output with klog calls, but not for text output through the klog logger or for JSON output. Now loggers can also provide a callback for changing their verbosity at runtime. Implementing that implies that the Create factory method has to be extended, which is an API break for the Go package, but not an API break for the configuration file and command line flags, which is what matters for the "api/v1" component API.

This is useful for tests which need to discard or capture the output.

serathius · 2023-01-17T09:31:57Z

/lgtm

k8s-ci-robot · 2023-01-17T09:32:03Z

LGTM label has been added.

Git tree hash: 9743d8b20a34e285079ecdf08368ff2df4b527e1

pohly · 2023-01-17T11:16:04Z

/retest

pohly · 2023-01-17T13:20:49Z

/retest

thockin · 2023-01-20T22:15:47Z

/approve

k8s-ci-robot · 2023-01-20T22:15:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, serathius, 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:

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [thockin]
~~staging/src/k8s.io/apiserver/OWNERS~~ [thockin]
~~staging/src/k8s.io/cloud-provider/OWNERS~~ [thockin]
~~staging/src/k8s.io/component-base/logs/OWNERS~~ [pohly,serathius,thockin]
~~staging/src/k8s.io/component-base/logs/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/controller-manager/OWNERS~~ [thockin]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [thockin]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [thockin]
~~test/integration/logs/OWNERS~~ [pohly,serathius,thockin]
~~vendor/OWNERS~~ [thockin]

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

k8s-triage-robot · 2023-01-21T02:01:59Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

pohly force-pushed the log-runtime-verbosity-level branch from 9155296 to 0693e89 Compare December 20, 2022 15:13

k8s-ci-robot added the wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. label Dec 20, 2022

k8s-ci-robot requested review from deads2k and serathius December 20, 2022 15:13

pohly changed the title ~~WIP: runtime log verbosity level changes~~ runtime log verbosity level changes Dec 20, 2022

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 Dec 20, 2022

pohly force-pushed the log-runtime-verbosity-level branch from 0693e89 to e5cb77c Compare December 20, 2022 18:51

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Dec 20, 2022

pohly force-pushed the log-runtime-verbosity-level branch from e5cb77c to 5029d39 Compare December 20, 2022 19:19

pohly commented Dec 20, 2022

View reviewed changes

pohly force-pushed the log-runtime-verbosity-level branch from 5029d39 to d63e633 Compare December 20, 2022 19:26

pohly force-pushed the log-runtime-verbosity-level branch 3 times, most recently from a681fc7 to 679c666 Compare December 21, 2022 07:56

pohly force-pushed the log-runtime-verbosity-level branch from ee7c14e to d148b63 Compare December 21, 2022 16:46

pohly mentioned this pull request Dec 23, 2022

log: split streams also for text output #114672

Merged

3 tasks

k8s-ci-robot assigned logicalhan and serathius Jan 3, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 3, 2023

serathius reviewed Jan 9, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2023

pohly added 3 commits January 17, 2023 10:20

k8s.io/component-base/logs/json: fix package name

8881b71

Having the sub-package using the same name as the parent package makes no sense. This seems to be a cut-and-paste error.

k8s.io/component-base/logs: allow overriding os.Stdout and os.Stderr

a41424d

This is useful for tests which need to discard or capture the output.

pohly force-pushed the log-runtime-verbosity-level branch from 3fe5b39 to a41424d Compare January 17, 2023 09:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2023

k8s-ci-robot requested review from logicalhan and serathius January 17, 2023 09:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2023

k8s-ci-robot merged commit 92f0818 into kubernetes:master Jan 21, 2023

k8s-ci-robot added this to the v1.27 milestone Jan 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime log verbosity level changes #114609

runtime log verbosity level changes #114609

pohly commented Dec 20, 2022 •

edited

pohly Dec 20, 2022

k8s-triage-robot commented Dec 20, 2022

pohly commented Dec 21, 2022

fedebongio commented Jan 3, 2023

serathius Jan 9, 2023

pohly Jan 9, 2023

serathius Jan 17, 2023

pohly Jan 17, 2023

serathius commented Jan 17, 2023

k8s-ci-robot commented Jan 17, 2023

serathius commented Jan 17, 2023

k8s-ci-robot commented Jan 17, 2023

pohly commented Jan 17, 2023

pohly commented Jan 17, 2023

thockin commented Jan 20, 2023

k8s-ci-robot commented Jan 20, 2023

k8s-triage-robot commented Jan 21, 2023

runtime log verbosity level changes #114609

runtime log verbosity level changes #114609

Conversation

pohly commented Dec 20, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

pohly Dec 20, 2022

Choose a reason for hiding this comment

k8s-triage-robot commented Dec 20, 2022

pohly commented Dec 21, 2022

fedebongio commented Jan 3, 2023

serathius Jan 9, 2023

Choose a reason for hiding this comment

pohly Jan 9, 2023

Choose a reason for hiding this comment

serathius Jan 17, 2023

Choose a reason for hiding this comment

pohly Jan 17, 2023

Choose a reason for hiding this comment

serathius commented Jan 17, 2023

k8s-ci-robot commented Jan 17, 2023

serathius commented Jan 17, 2023

k8s-ci-robot commented Jan 17, 2023

pohly commented Jan 17, 2023

pohly commented Jan 17, 2023

thockin commented Jan 20, 2023

k8s-ci-robot commented Jan 20, 2023

k8s-triage-robot commented Jan 21, 2023

pohly commented Dec 20, 2022 •

edited