Revert default changes

[mtaufen]

What this PR does / why we need it:
Reverts recent, potentially breaking changes to flag defaults. These block vendoring the latest version of klog into upstream Kubernetes, because they could break user deployments of core Kubernetes components. If these changes are eventually included, they need to be included in a way that respects the upstream deprecation policy (see below).

This PR reverts three other PRs:

1. Use info in default stderrThreshold #42, which changes the stderrthreshold default to INFO
   • This could cause significant, unexpected increases in logging verbosity.
2. Default logtostderr to true #39, which changes the logtostderr default to true
   • This, especially, could break existing logging configurations because when set, it disables logs from being sent to files, even if --log_file is set. EDIT: See below conversation, we decided to keep Default logtostderr to true #39 due to pre-existing overrides in k/k, but still revert the other two.
3. stderrthreshold is useless if logtostderr is set #31, which conditions tostderr output on stderrthreshold
   • Without the addition of Use info in default stderrThreshold #42, this could have resulted in default behavior that silently drops (Info) logs sent to stderr when logtostderr is true. With the revert of Use info in default stderrThreshold #42, this should also be reverted.

Adding these changes back in can be revisited and discussed during code slush, but I would like to back them out in order to get the O_APPEND fix into Kubernetes 1.14.

If we decide to include these changes in upstream Kubernetes, we need to follow the official policy of announcing deprecation of old defaults and waiting the appropriate amount of time before changing them: https://kubernetes.io/docs/reference/using-api/deprecation-policy/. In the future, we need to figure out how to more carefully coordinate changes to defaults in this repo with deprecation policies of other Kubernetes projects.

Release note:

Recent commits that changed --stderrthreshold default from ERROR to INFO and conditioned --tostderr output on --stderrthreshold have been reverted.

[mtaufen]

/kind bug
/priority critical-urgent
/assign @liggitt @dims
/cc @DirectXMan12 @vincepri @shsjshentao @justinsb

[k8s-ci-robot]

@mtaufen: GitHub didn't allow me to request PR reviews from the following users: shsjshentao.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/kind bug
/assign @liggitt @dims
/cc @DirectXMan12 @vincepri @shsjshentao

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.

[neolit123]

cc @vincepri

[neolit123]

if we consider klog usage as GA and user facing -> GA: 12 months or 2 releases (whichever is longer)
https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-flag-or-cli

[DirectXMan12]

@mtaufen we already default to logtostderr in k/k proper and anything that pulls in k/apiserver (which is a lot of stuff, by accident) in previous versions, and now anything that pulls in k/component-base (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/logs/logs.go#L37)

[DirectXMan12]

#31 I've seen actively confuse plenty of people before, so it's not clear to me that people were actually expecting it to work as it did before #31 merged (which is not the best reason for breaking compatibility, but it's some justification). Practically, it's not clear to me that #42 actually has any impact on anyone's logging. We explicitly state that info log level 0 is message that everyone should see, and currently logtostderr causes info to go to stderr anyway (unless I'm misunderstanding here).

That being said, I'm not super-against reverting #31 and #42 until we can have a later discussion.

[vincepri]

The main reason behind the changes proposed in #39 and #41 is to clarify the development experience and the true default for users. The logtostderr flag is hardcoded to true in init or main functions across the codebase, which results in an incorrect default to be printed in help usage sections and the impossibility for end users to overwrite this flag.

Sidenote: #42 was needed because of #31.

[DirectXMan12]

We talked about this on the component-wg meeting. The upshot is that #39 is fine to deep (since it's defaulted across k/k and k/component-base anyway), but #42 could cause you to receive extra INFO-level logs on stderr if you're trying to just use files and don't expect much to be going to stderr (since stderrthreshold comes into effect whether or not logtostderr or alsologtostderr are set:

klog/klog.go

Lines 741 to 748 in ad78c01

	if l.toStderr {
	if s >= l.stderrThreshold.get() {
	os.Stderr.Write(data)
	}
	} else {
	if alsoToStderr || l.alsoToStderr || s >= l.stderrThreshold.get() {
	os.Stderr.Write(data)
	}

SInce #42 and #31 are tied together, it's probably best to revert them together to avoid behavior changes.

As a broader effort, we should figure out the semantics that we want for a logging library around things like verbosity, etc, and propose that, figuring out specific deprecation paths for the older stuff. Doing this piecemeal is probably going to be harder.

[DirectXMan12]

@mtaufen if you drop the revert of #39, I'm LGTM on this PR.

[vincepri]

+1 on reverting #31 and #42 but keep #39.

[mtaufen]

Thanks everyone!
@DirectXMan12 updated to drop the revert of #39.

[DirectXMan12]

/lgtm

We seem to have some amount of concensus among maintainers here (+1 from me, vincepri) and lack of anyone vociferously complaining.

I'll give it till tomorrow morning, and then give the approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, mtaufen

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:

• OWNERS [DirectXMan12]

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