Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move KEP-2845 to implementable #2912

Merged
merged 7 commits into from
Sep 9, 2021
Merged

Conversation

serathius
Copy link
Contributor

Sending PR to move KEP-2845 to implementable to start discussion as there is not a lot of time before code freeze.
Proposal: https://github.com/kubernetes/enhancements/blob/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components/README.md

Assigning people that already volunteered to review the KEP. Thanks for offering help!
/assign @ehashman @pohly

Still looking for approver from SIG-Arch side:

@dims, @thockin, @liggitt, @johnbelamaric
Could one of you become KEP approver?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 30, 2021
@k8s-ci-robot 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. labels Aug 30, 2021
@pohly
Copy link
Contributor

pohly commented Aug 30, 2021

Looking at the proposal again from top to bottom again, I have one concern: -vmodule is a feature that will only be supported by some logging backends. klog has it, but current logr backends don't. That means we can keep the flag or something like it, but should define it in such a way that it is clear that its effect depends on the selected backend. For JSON output via zapr, implementing the feature would be doable.

Also, can you clarify what "follow standard component configuration mechanism" is? Make it a link perhaps?

@dims
Copy link
Member

dims commented Aug 30, 2021

/assign @johnbelamaric @liggitt

(for approval)

@liggitt
Copy link
Member

liggitt commented Aug 30, 2021

for klog, I'll defer to @dims or @thockin

@liggitt liggitt assigned dims and unassigned liggitt Aug 30, 2021
@dims
Copy link
Member

dims commented Aug 30, 2021

thanks @liggitt i will definitely look. will assign tim as well

/assign @thockin

@ehashman
Copy link
Member

ehashman commented Sep 1, 2021

/sig architecture

@k8s-ci-robot k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Sep 1, 2021
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serathius you have added me as PRR approver but the PRR questionnaire is not complete. Please fill it out, including the version skew sections (explain why they are not applicable).

@thockin
Copy link
Member

thockin commented Sep 2, 2021

I like @pohly comment, but otherwise I am happy to see this proceed.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 2, 2021
@serathius
Copy link
Contributor Author

@ehashman Sorry for that, verify complained that I need PRR file, so I added it forgetting the questionnaire.
PTAL

@serathius
Copy link
Contributor Author

@pohly For vmodule I'm still not sure what's the best solution. I see options:

  • Implement it in logr to make it a standard. logr already implements verbosity handling, it should not be a big stretch to also support vmodule to encapsulate verbosity handling in logr. Upsides: simple and clear solution from K8s perspective. Downsides: We don't have usage data to confirm that it's really needed (we got one issue kubelet config flag using "--logging-format" and "--vmodule" raises a FATAL error kubernetes#96314) also what if we open flood gates for users to request more verbosity controls?
  • Implement it as a logr wrapper in K8s. No impact on logr, but introducing wrappers is also not a preferred approach as we want to minimize logging code on K8s side.
  • Make it a feature that backend can implement. This feature is generic enough that it doesn't make sense to have each backend implement it by itself. Downsides: it might lead to multiple reimplementations of the feature.
  • Drop the feature. The only reason to keep the feature is fact that it impacts log volume (part of verbosity checks), so it's a escape hatch for contributors making mistakes when picking verbosity. Problem could be mitigated by improving how we pick verbosity. For example give better guidelines, measure log volume based on verbosity to make adjustments etc.

My preferred option would be first as it makes it clearly delegates verbosity to logr. I don't think that any backend would ever have any verbosity specific logic. Only case I would backend would want to play with verbosity is when someone would want to write to two backends on different verbosity conditions. But I don't think loger composition is something we planned for.

I'm not very involved with logr community, so I would depend on @thockin @pohly opinion for best action here.

@serathius
Copy link
Contributor Author

@pohly as for example of standard K8s flag configuration you can take a look here https://github.com/kubernetes/kubernetes/blob/4556873abf8aa88ce2ab40476267b0e9d4253366/staging/src/k8s.io/component-base/logs/options.go#L43
This example is a little outdated, but it represents whole idea in one file.

  • All configuration should be stored in dedicated structs.
  • Those structs should have methods: New, AddFlags, Validate, Apply
  • Components can use those structs to build hierarchical configuration, where there is no global state.

Current file differs a little as idea was expanded a little since. Instead of Options object having fileds directly we use Configuration object. The difference is that Configuration is an API object, meaning it can be passed via an API request.

@pohly
Copy link
Contributor

pohly commented Sep 2, 2021

Implement it in logr to make it a standard. logr already implements verbosity handling, it should not be a big stretch to also support vmodule to encapsulate verbosity handling in logr.

That's not how logr is currently designed. All logr.LogSink implementations provide their own verbosity checks, with only tracking of the message's verbosity level handled by logr.Logger. In other words, -v is a parameter for the underlying backend. And so is -vmodule.

My preferred option would be first as it makes it clearly delegates verbosity to logr. I don't think that any backend would ever have any verbosity specific logic.

I don't think we should change the logr design. We can provide support code for implementing -vmodule in logr and then LogSink implementations can share the same code to implement it if they want to.

My preferred solution is to keep only "-v" as a mandatory option and implement "-vmodule" for output via klog and zapr. That means in practice it will remain available in Kubernetes.

@serathius
Copy link
Contributor Author

Updated KEP to not introduce breaking change or --logtostdout flag. I still think that would be a better long term decision, but we don't have quorum of reviewers to make it (OOO).

Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better to me now. One nit about vmodule and a question around what we want to include now for split routing, but not a blocker IMHO.

temporary flags that will be removed at the same time as other klog flags.
(kube-apiserver, kube-scheduler, kube-controller-manager, kubelet) and leave
them with defaults. From klog flags we would remove all flags besides "-v"
and "-vmodule".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add:

Support for "-vmodule" will depend on the selected log format and be supported at least for the traditional "text" format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in section below

@@ -172,41 +170,12 @@ directly impacting log volume and component performance.

With removal of configuration alternatives we need to make sure that defaults
make sense. List of logging features implemented by klog and proposed actions:
* Routing logs based on type/verbosity - Should be reconsidered.
* Routing logs based on type/verbosity - Feature removed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could promise to implement that for JSON output. But I prefer to keep that out of the KEP for now if that helps to get it merged.

If we want to include it, then here we could replace "Feature removed" with "Move into JSON backend" and below under "beta graduation" add "configuration of JSON backend implemented, providing at least message routing based on type and verbosity".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to Alpha stage as I want to implement it in next release to unblock JSON graduation to Beta.

Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2021
@ehashman
Copy link
Member

ehashman commented Sep 8, 2021

I need to do a final review, but having skimmed the discussion, so long as log stream splitting is opt-in rather than the default, I think this is okay from a PRR perspective.

As this was originally written, that became the new default, with single-stream logging being entirely deprecated, which was a blocker. Will complete my review today.

@johnbelamaric
Copy link
Member

/unassign

Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small comments on PRR. Just waiting on a review/:+1: from SIG Arch.

Looks good from Instrumentation perspective.


###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?

Yes, there is no impact on cluster state. Logs should also be unaffected as logs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question says "Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?" and this doesn't answer it. It should specify how to enable/disable.

In this case, there is no way to enable/disable as the feature is deprecating flags.

@dims
Copy link
Member

dims commented Sep 8, 2021

i support this effort and approve on behalf of sig-arch. @thockin if you get a chance please take a pass at the latest updates as well.

/approve

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2021
@ehashman
Copy link
Member

ehashman commented Sep 9, 2021

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 9, 2021
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

for both SIG Instrumentation and PRR

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8568805 into kubernetes:master Sep 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 9, 2021
ravisantoshgudimetla pushed a commit to ravisantoshgudimetla/enhancements that referenced this pull request Sep 9, 2021
* Move KEP-2845 to implementable

* Add PRR for kubernetes#2845

* Remove mention of --logtostdout flag in kubernetes#2845

* KEP kubernetes#2485: Require splitting stdout/stderr to Json format

* Add pohly as reviewer for KEP 2845

* kubernetes#2845 Update PRR

* kubernetes#2845 Add dims as SIG arch approver
rikatz pushed a commit to rikatz/enhancements that referenced this pull request Feb 1, 2022
* Move KEP-2845 to implementable

* Add PRR for kubernetes#2845

* Remove mention of --logtostdout flag in kubernetes#2845

* KEP kubernetes#2485: Require splitting stdout/stderr to Json format

* Add pohly as reviewer for KEP 2845

* kubernetes#2845 Update PRR

* kubernetes#2845 Add dims as SIG arch approver
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. 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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants