klog v2.60.1 #108725

pohly · 2022-03-16T08:14:23Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The new release supports FlushAndExit and contextual logging.

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2022-03-16T08:14:30Z

@pohly: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

dims · 2022-03-16T10:46:47Z

/retest

dims · 2022-03-16T10:47:21Z

/assign

pohly · 2022-03-16T11:18:17Z

/hold

Let's also include kubernetes/klog#306 and https://github.com/kubernetes/kubernetes/compare/master...pohly:klog-flush?expand=1 in this PR.

liggitt · 2022-03-21T14:22:55Z

vendor/k8s.io/klog/v2/contextual.go

+//
+// This must be called during initialization before goroutines are started.
+func EnableContextualLogging(enabled bool) {
+	contextualLoggingEnabled = enabled


This must be called during initialization before goroutines are started.

if there any enforcement or protection if someone doesn't follow this rule? the places we'll be calling this will require parsing feature gates from args/config files, which means we can't call this from func init() methods which are easier to reason about goroutines not having started.

This will get called in component-base/logs InitLogs which was recently (1.23) moved to the beginning of a program's startup:
https://github.com/kubernetes/component-base/blob/c5dae0cde26ef1c4fc6cfe6384bed8f36148f13f/cli/run.go#L122-L144

kubernetes/cmd/kubelet/app/server.go

Lines 226 to 232 in 637394c

// Config and flags parsed, now we can initialize logging.

logs.InitLogs()

logOption := &logs.Options{Config: kubeletConfig.Logging}

if err := logOption.ValidateAndApply(); err != nil {

klog.ErrorS(err, "Failed to initialize logging")

os.Exit(1)

}

There's no enforcement to prevent data races or detect misuse. We could use an atomic Int32 access, but I don't think that this is needed. Even if it gets misused and the race goes wrong, the impact will probably be low.

FWIW, the decision to not do locking also applies to SetLogger which has been around for much longer.

For a while SetLogger used mutex locking, but the corresponding read accesses don't, which rendered the mutex a bit mute (no pun intended), for example in https://github.com/kubernetes/klog/blob/f8e668dbaa5f6f0e6a5c24ffd7667263840d79ae/klog.go#L1387.

I brought this up when I noticed it and @serathius explained that relying on sequential execution is good enough.

non-threadsafe globally writeable package vars seems like an anti-pattern we wouldn't want to extend.

I'm really not trying to make this a proxy review of changes that have gone into klog, but I'm concerned that the library is becoming harder to use safely within k/k

IMHO it's as easy (or hard) as before: some functions have to be called early. It's true that this PR adds one more of those, but it doesn't change how klog is used in k/k because EnableContextualLogging will be called at the same time or even earlier than the others (like SetLogger).

vendor/k8s.io/klog/v2/klog.go

The new release supports FlushAndExit and contextual logging.

Not all code knows that it needs to flush through component-base/logs.FlushLogs when the JSON logger is used. By registering the flush callback together with the logger, klog.Flush and klog.FlushAndExit are sufficient for flushing all data.

The advantage is that klog properly handles restarting of the daemon with a new interval and the daemon can be stopped. Stopping the daemon solves a data race that the tests had when modifying the Logger's flush function while goroutines from previous tests were still running.

pohly · 2022-03-21T16:34:14Z

/retest

One unit test flake is getting fixed in #108553

The other is #105589

fedebongio · 2022-03-22T20:06:30Z

/remove-sig api-machinery

liggitt · 2022-03-23T19:16:07Z

/approve

liggitt · 2022-03-23T19:16:28Z

dependency mechanics lgtm

k8s-ci-robot · 2022-03-23T19:20:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, pohly

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~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/legacy-cloud-providers/OWNERS~~ [dims,liggitt]

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

dims · 2022-03-23T22:06:56Z

/lgtm

pohly · 2022-03-24T06:49:36Z

/hold cancel

k8s-triage-robot · 2022-03-24T07:40:46Z

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.

k8s-ci-robot requested review from ahg-g, andrewsykim and a team March 16, 2022 08:15

pohly mentioned this pull request Mar 16, 2022

Fixes exited messages when leaderelection lost #107724

Merged

k8s-ci-robot assigned dims Mar 16, 2022

liggitt reviewed Mar 21, 2022

View reviewed changes

vendor/k8s.io/klog/v2/klog.go Show resolved Hide resolved

liggitt reviewed Mar 21, 2022

View reviewed changes

vendor/k8s.io/klog/v2/klog.go Show resolved Hide resolved

enj added this to Needs Triage in SIG Auth Old Mar 21, 2022

pohly added 3 commits March 21, 2022 16:23

klog v2.60.1

09aa107

The new release supports FlushAndExit and contextual logging.

logs: flush Logger through klog

f8bb67c

Not all code knows that it needs to flush through component-base/logs.FlushLogs when the JSON logger is used. By registering the flush callback together with the logger, klog.Flush and klog.FlushAndExit are sufficient for flushing all data.

pohly mentioned this pull request Mar 21, 2022

StopFlushDaemon: document flushing on shutdown kubernetes/klog#314

Merged

pohly force-pushed the klog-flush-and-exit branch from 002d139 to 0b7d303 Compare March 21, 2022 15:46

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 21, 2022

ehashman added this to Triage in SIG Node PR Triage Mar 21, 2022

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 22, 2022

pohly mentioned this pull request Mar 23, 2022

contextual logging kubernetes/enhancements#3077

Open

12 tasks

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

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

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

k8s-ci-robot merged commit 56f45c5 into kubernetes:master Mar 24, 2022

SIG Auth Old automation moved this from Needs Triage to Closed / Done Mar 24, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 24, 2022

pohly mentioned this pull request Mar 24, 2022

component-base: make LoggingConfiguration a single-version API #105797

Merged

liggitt mentioned this pull request Mar 26, 2022

kubelet: Race condition in nodeshutdown unit test #108040

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

klog v2.60.1 #108725

klog v2.60.1 #108725

pohly commented Mar 16, 2022

k8s-ci-robot commented Mar 16, 2022

dims commented Mar 16, 2022

dims commented Mar 16, 2022

pohly commented Mar 16, 2022

liggitt Mar 21, 2022

pohly Mar 21, 2022

pohly Mar 21, 2022

pohly Mar 21, 2022 •

edited

liggitt Mar 21, 2022

pohly Mar 21, 2022

pohly commented Mar 21, 2022 •

edited

fedebongio commented Mar 22, 2022

liggitt commented Mar 23, 2022

liggitt commented Mar 23, 2022

k8s-ci-robot commented Mar 23, 2022

dims commented Mar 23, 2022

pohly commented Mar 24, 2022

k8s-triage-robot commented Mar 24, 2022

	// Config and flags parsed, now we can initialize logging.
	logs.InitLogs()
	logOption := &logs.Options{Config: kubeletConfig.Logging}
	if err := logOption.ValidateAndApply(); err != nil {
	klog.ErrorS(err, "Failed to initialize logging")
	os.Exit(1)
	}

klog v2.60.1 #108725

klog v2.60.1 #108725

Conversation

pohly commented Mar 16, 2022

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 16, 2022

dims commented Mar 16, 2022

dims commented Mar 16, 2022

pohly commented Mar 16, 2022

liggitt Mar 21, 2022

Choose a reason for hiding this comment

pohly Mar 21, 2022

Choose a reason for hiding this comment

pohly Mar 21, 2022

Choose a reason for hiding this comment

pohly Mar 21, 2022 • edited

Choose a reason for hiding this comment

liggitt Mar 21, 2022

Choose a reason for hiding this comment

pohly Mar 21, 2022

Choose a reason for hiding this comment

pohly commented Mar 21, 2022 • edited

fedebongio commented Mar 22, 2022

liggitt commented Mar 23, 2022

liggitt commented Mar 23, 2022

k8s-ci-robot commented Mar 23, 2022

dims commented Mar 23, 2022

pohly commented Mar 24, 2022

k8s-triage-robot commented Mar 24, 2022

pohly Mar 21, 2022 •

edited

pohly commented Mar 21, 2022 •

edited