KEP-2845: Initial draft #2846

serathius · 2021-07-30T13:57:14Z

One-line PR description:

Issue link: Remove klog specific command line arguments from Kubernetes components #2845

/assign @ehashman
Elana, can we merge it quickly as provisional as we confirmed support from SIG-Arch and SIG-Instrumentation?

serathius · 2021-07-30T13:58:13Z

/cc @shubheksha

serathius · 2021-07-30T13:58:20Z

/wg structured-logging

…ents

serathius · 2021-07-30T14:00:22Z

/milestone v1.23

serathius · 2021-07-30T14:01:14Z

/sig architecture

yuzhiquan · 2021-08-02T02:44:31Z

/cc

ehashman

Please add me as the SIG Instrumentation approver.

I added some initial comments, let's also get feedback from SIG Architecture, the other sponsor, before we merge as provisional.

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

ehashman · 2021-08-05T17:13:17Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+
+#### Users don't want to use go-runner as replacement.
+
+TODO


Use systemd or redirect from shell.

ehashman · 2021-08-05T17:13:54Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+
+#### Log processing in parent process causes performance problems
+
+TODO


We should actually test this out but I don't think a plan will be required until KEP goes to "beta". Which is weird because this is a deprecation, so... maybe within 1 release.

ehashman · 2021-08-05T17:15:13Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+- Klog can be configured without registering flags
+- Kubernetes logging configuration drops global state
+- Go-runner is feature complementary to klog flags planned for deprecation
+- Projects in Kubernetes Org are migrated to go-runner


Or systemd-run, etc.

I think we can propose alternatives like systemd-run to documentation, but k8s projects should pick a tool written in golang just for ease of maintenance.

ehashman · 2021-08-05T17:16:04Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+- Go-runner project is well maintained and documented 
+- Documentation on switching go-runner is publicly available


This concerns me a little as I don't think adding maintenance of a new component helps reduce scope... tools exist out there, let's not put all our eggs in the go-runner basket 🐣

ehashman · 2021-08-05T17:16:27Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+
+### Release klog 3.0 with removed features 
+Removal of those features cannot be done without whole k8s community instead of
+just k8s core components


Maybe we can do this as a follow-up eventually...

Based on problems with klog 2.0, I would prefer to accelerate migration to logr as it would make more sense strategically. We still would need some wrapper code to configure external library but not something worth our own library.

pohly · 2021-08-19T10:30:24Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+#### Alpha
+
+- Klog can be configured without registering flags
+- Kubernetes logging configuration drops global state


If there is no global logging state anymore, how will the current log calls like klog.InfoS be handled? That currently falls back to the globally configured klog.

I fully agree that we should get rid of that global state. I'd like to see a logger instance be passed into all functions which do logging, but there's no consensus on how to do that (attach to context vs. explicit parameter) and this KEP doesn't address this.

FWIW, I prefer the approach via context. Adding another parameter implies API breaks (client-go...) and other components already started using the context (logr, operator SDK), so if Kubernetes does the same, different code bases will become interoperable.

I agree about using context to replace klog global state, but for now we want to start with removing configuration (flags etc), . This point is about configuration, basically removing global klog flags and passing through config struct to register/validate and initialize logging (same as how other components are configured). At some point this configuration will be applied to global klog state, but at least we clean up configuration.

serathius · 2021-08-23T11:21:09Z

/cc @thockin @dims
Last version of proposal split log streams into error and info. This change is would impact both klog and logr implementations like zapr. PTAL

pohly · 2021-08-23T12:46:03Z

zap already supports different streams for info and error messages. That should work also with go-logr/zapr which uses different message levels (https://github.com/go-logr/zapr/blob/566ac59c7d87b6450256fc47749549306638d348/zapr.go#L187-L197).

What concerns me more is configuring the logging backend:

Is the plan that "standard" Kubernetes command line flags get mapped to the corresponding config options in the logging backend? Or will there always be some wrapper around the backend which implements common features, like level checking (i.e. how level checking is currently implemented in klog)? IMHO, Kubernetes should just configure the backends and then leave logging directly to the various go-logr implementations (less code to maintain, better performance).
There are always going to be options that are only supported by some backends. Kubernetes should support configuring those, too, not just the common ones.

serathius · 2021-08-24T08:37:28Z

Is the plan that "standard" Kubernetes command line flags get mapped to the corresponding config options in the logging backend? Or will there always be some wrapper around the backend which implements common features, like level checking (i.e. how level checking is currently implemented in klog)? IMHO, Kubernetes should just configure the backends and then leave logging directly to the various go-logr implementations (less code to maintain, better performance).

We should have small set of common flags supported by all backend. Those flags should be based on features exposed by the logr interface itself, mainly about verbosity control. I don't think we should have any wrappers and agree that we should delegate the problem to backend implementations.

There are always going to be options that are only supported by some backends. Kubernetes should support configuring those, too, not just the common ones.

After giving it a though I agree, at some point we definetly will need to have a pass some configuration to backends. Main challenge here is to avoid version pinning problems that we had in klog. Kubernetes backward policy is too restrictive for most logging libraries (backward compatible for almost a year). I think we would want to introduce logging format versioning to avoid negatively impacting other projects. This problem would require a separate discussion and a KEP, but I think we should stop using configuration flags to maintain backward compatibility and instead start versioning log format. For example if we decide to change format of timestamp in json log format we should introduce "json-v2" format instead of adding flag "--json-timestamp-format=string". Old "json" format would be marked as deprecated and removed after 3 releases. This way we can make changes to format, and maintain backward compatibility.

To avoid this KEP being too big, I would propose to tackle log format versioning as a separate problem that will address when we have specific use cases. For now we should be ok to release klog and json formats with only one configuration based on good defaults.

pohly · 2021-08-24T12:20:27Z

I don't think we should have any wrappers and agree that we should delegate the problem to backend implementations.

Makes sense to me. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/logs/registry.go then needs to be changed from "register a logr.Logger" to "register a constructor of a logr.Logger".

For example if we decide to change format of timestamp in json log format we should introduce "json-v2" format instead of adding flag "--json-timestamp-format=string". Old "json" format would be marked as deprecated and removed after 3 releases. This way we can make changes to format, and maintain backward compatibility.

What I would do is to say "json" is selecting zap as backend with certain default options for the underlying zapr logger and then have a configuration format that is specific for that backend. That format then can be versioned separately. I wouldn't add command line flags for each backend, just a generic "--log-config-file <file name>" or "--log-config <data>".

To avoid this KEP being too big, I would propose to tackle log format versioning as a separate problem that will address when we have specific use cases. For now we should be ok to release klog and json formats with only one configuration based on good defaults.

Agreed.

serathius · 2021-08-25T08:31:59Z

ping @thockin

logicalhan · 2021-08-26T11:31:52Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+## Proposal
+
+I propose that Kubernetes core components (kube-apiserver, kube-scheduler,
+kube-controller-manager, kubelet) should drop flags that extend


When and on what sort of timeline?

Deprecate in 1.23 and remove in 1.26 to follow 3 release deprecation window.

logicalhan · 2021-08-26T11:32:29Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+logging over events streams or klog specific features. This change should be
+scoped to only those components and not affect broader klog community.
+
+With removal of output stream manipulation flags we need to provide a set of sane


For backwards compatibility, should we support a double set of flags during an overlapping period?

There sill not be a double set of flags as we are just removing flags, not replacing them with alternatives.

logicalhan · 2021-08-26T11:34:08Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+* --log-backtrace-at - an legacy glog feature.
+  Motivation: No trace of anyone using this feature.
+
+Flag deprecation should comply with standard k8s policy and require 3 releases before removal.


Ah here we are.

logicalhan · 2021-08-26T11:35:06Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+Technically K8s already doesn't support klog flags. Klog flags are renamed to
+comply with K8s flag naming convention (underscores are replaced with hyphens).
+Full klog support was never promised to users and removal of those flags should
+be treated as removal of any other flag.


That seems fair.

logicalhan · 2021-08-26T11:36:16Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+defaults and convention that will prevent logging formats implementations to
+diverge and reintroduce their own flags. As logs should be treated as event
+streams I would propose that we separate two main streams "info" and "error"
+based on log method called. As error logs should usually be treated with higher


Is this backwards compatible?

No, that's why I'm proposing a multi release plan to make this change.

logicalhan · 2021-08-26T11:37:05Z

keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

+
+Splitting stdout from stderr would be a breaking change in both klog and
+kubernetes components. To avoid that I propose to introduce new logging flag
+`--logtostdout` in klog that will allow writing info logs to stdout. This flag


All logs? What about error logs?

This flag is meant to split stdout and stderr. It's name is complementary to "--logtostderr". If both are enabled, info will be send to stdout, error to stderr.

logicalhan

discussed offline, @serathius helpfully answered my questions.

/lgtm

logicalhan

/approve

(elana is on vacation)

k8s-ci-robot · 2021-08-26T14:35:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logicalhan, serathius

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:

~~keps/sig-instrumentation/OWNERS~~ [logicalhan]

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

logicalhan

/lgtm

k8s-ci-robot assigned ehashman Jul 30, 2021

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 30, 2021

k8s-ci-robot requested review from brancz and logicalhan July 30, 2021 13:57

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 30, 2021

serathius mentioned this pull request Jul 30, 2021

Remove klog specific command line arguments from Kubernetes components #2845

Closed

20 tasks

k8s-ci-robot requested a review from shubheksha July 30, 2021 13:58

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

Initial draft of KEP-2845 Deprecate klog specific flags in k8s compon…

a5c4c07

…ents

serathius force-pushed the klog branch from 643c41e to a5c4c07 Compare July 30, 2021 13:59

k8s-ci-robot added this to the v1.23 milestone Jul 30, 2021

k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Jul 30, 2021

k8s-ci-robot requested a review from yuzhiquan August 2, 2021 02:44

ehashman reviewed Aug 5, 2021

View reviewed changes

pohly reviewed Aug 19, 2021

View reviewed changes

Apply feedback from review and introduce --logtostdout flag

0c389f9

k8s-ci-robot requested review from dims and thockin August 23, 2021 12:54

logicalhan reviewed Aug 26, 2021

View reviewed changes

k8s-ci-robot assigned logicalhan Aug 26, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2021

Add tldr sections to proposal

0b15e37

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2021

Add information about the impact of splitting stdout from stderr

ce9d9e7

logicalhan approved these changes Aug 26, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2021

Update toc

036a2b1

serathius force-pushed the klog branch from ead39d0 to 036a2b1 Compare August 26, 2021 14:38

logicalhan reviewed Aug 26, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2021

k8s-ci-robot merged commit 43d81d8 into kubernetes:master Aug 26, 2021


		#### Log processing in parent process causes performance problems

		TODO

		- Go-runner project is well maintained and documented
		- Documentation on switching go-runner is publicly available

KEP-2845: Initial draft #2846

KEP-2845: Initial draft #2846

Conversation

serathius commented Jul 30, 2021

serathius commented Jul 30, 2021

serathius commented Jul 30, 2021

serathius commented Jul 30, 2021

serathius commented Jul 30, 2021

yuzhiquan commented Aug 2, 2021

ehashman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius commented Aug 23, 2021 • edited Loading

pohly commented Aug 23, 2021

serathius commented Aug 24, 2021

pohly commented Aug 24, 2021 • edited Loading

serathius commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logicalhan left a comment

Choose a reason for hiding this comment

logicalhan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 26, 2021

logicalhan left a comment

Choose a reason for hiding this comment

serathius commented Aug 23, 2021 •

edited

Loading

pohly commented Aug 24, 2021 •

edited

Loading