Restore support for log specific flags in Kubernetes Components

[yangjunmyfm192085]

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #933
as #933 discussed, We need to restore support for the following flags

• --log-dir, --log-file, --log-flush-frequency - responsible for writing to files and syncs to disk. Motivation: Not critical as there are easy to set up alternatives like: shell redirection, systemd service management or docker log driver. Removing them reduces complexity and allows development of non-text loggers like one writing to journal.
• --logtostderr, --alsologtostderr, --one-output, --stderrthreshold - responsible enabling/disabling writing to stderr (vs file). Motivation: Routing logs can be easily implemented by any log processors like: Fluentd, Fluentbit, Logstash.
• --log-file-max-size, --skip-log-headers - responsible configuration of file rotation. Motivation: Not needed if writing to files is removed.
• --add-dir-header, --skip-headers - klog format specific flags . Motivation: don't apply to other log formats
• --log-backtrace-at - A legacy glog feature. Motivation: No trace of anyone using this feature.

[yangjunmyfm192085]

/cc @serathius @dgrisonnet

[serathius]

I don't think we should revert the dependency update. We just need to properly register klog flags.

[yangjunmyfm192085]

Yeah, I dealed with too complicated, I'm not sure if the current way is right.
Could you take a look @serathius ? thanks.

[serathius]

cc @pohly

[pohly]

Yeah, I dealed with too complicated, I'm not sure if the current way is right.

Ideally you follow what Kubernetes is doing: register the flags, but mark them as deprecated. Then in a future version you can remove them.

The k8s.io/component-base/logs/config package does all of that for you. See https://github.com/kubernetes/component-base/blob/master/logs/example/cmd/logger.go for an example.

[yangjunmyfm192085]

Hi, @pohly, thanks for your suggestion, but according to the template in the link, the flags will still be deprecated, or there are other setting modes?

[pohly]

So you want to support these flags forever, even after Kubernetes is done with the deprecation?

Then you are on your own, you will have to use old code that might not get any security fixes anymore. Are you sure you want to do that?

[yangjunmyfm192085]

So you want to support these flags forever, even after Kubernetes is done with the deprecation?

Then you are on your own, you will have to use old code that might not get any security fixes anymore. Are you sure you want to do that?

No. We hope to drop these flags in the next release. Only the current version supports it. I mean, I use the way by example, those flags have been deprecation, where am i using it wrong?

[pohly]

The current component-base still supports them. They are just marked as deprecated.

[yangjunmyfm192085]

The current component-base still supports them. They are just marked as deprecated.

With the example given, In the following function, the deprecated flags are not loaded as before.
https://github.com/kubernetes/component-base/blob/f57281d7c18ff3a9d1c2404bf97f4ca70bdcb655/logs/config.go#L61

[yangjunmyfm192085]

I used another function AddGoFlags from k8s.io/component-base/logs/ and it works, The output is as follows:

Logging flags:

      --add_dir_header                                                                                                                               
                If true, adds the file directory to the header of the log messages (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --alsologtostderr                                                                                                                              
                log to standard error as well as files (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-flush-frequency duration                                                                                                                 
                Maximum number of seconds between log flushes (default 5s)
      --log_backtrace_at traceLocation                                                                                                               
                when logging hits line file:N, emit a stack trace (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
                (default :0)
      --log_dir string                                                                                                                               
                If non-empty, write log files in this directory (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log_file string                                                                                                                              
                If non-empty, use this log file (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log_file_max_size uint                                                                                                                       
                Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (DEPRECATED:
                will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
                (default 1800)
      --logtostderr                                                                                                                                  
                log to standard error instead of files (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
                (default true)
      --one_output                                                                                                                                   
                If true, only write logs to their native severity level (vs also writing to each lower severity level) (DEPRECATED: will be removed in a
                future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --skip_headers                                                                                                                                 
                If true, avoid header prefixes in the log messages (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --skip_log_headers                                                                                                                             
                If true, avoid headers when opening log files (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --stderrthreshold severity                                                                                                                     
                logs at or above this threshold go to stderr (DEPRECATED: will be removed in a future release, see
                https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
                (default 2)
  -v, --v Level                                                                                                                                      
                number for the log level verbosity
      --vmodule moduleSpec                                                                                                                           
                comma-separated list of pattern=N settings for file-filtered logging

anyway, thanks @pohly .
/cc @pohly @serathius

[yangjunmyfm192085]

/retest

[serathius]

Can you confirm that passing one of the flags results in changes in output?

[yangjunmyfm192085]

Can you confirm that passing one of the flags results in changes in output?

let me confirm it. Whether the modification will take effect?

[pohly]

I used another function AddGoFlags from k8s.io/component-base/logs/ and it works

Ah, sorry - my comment about component-base/logs/config led you in the wrong direction. That's for the flags that will remain after deprecation. As you noticed, k8s.io/component-base/logs/ is for the legacy ones.

Out of curiosity, why did the 0.6.0 release of metrics-server not have those flags anymore?

[yangjunmyfm192085]

I used another function AddGoFlags from k8s.io/component-base/logs/ and it works

Ah, sorry - my comment about component-base/logs/config led you in the wrong direction. That's for the flags that will remain after deprecation. As you noticed, k8s.io/component-base/logs/ is for the legacy ones.

Out of curiosity, why did the 0.6.0 release of metrics-server not have those flags anymore?

You're welcome. We may not communicate clearly. Our needs are as described in the issue #933,

klog specific flags was deprecated in v1.23, K8s version that Metrics Server v0.6.0 bind it's dependencies. Removing those flags however was not intended, this happened due to changes in how flags are registered by component-base/logs package.

Overall I'm for removing klog flags, however only with proper previous communication. I think we should restore klog flags in v0.6.1 and announce their deprecation. Their removal should only happen in next release.

Now my question is, how to correctly implement it, can you help analyze it, thank you

[yangjunmyfm192085]

/cc @pohly

[yangjunmyfm192085]

Can you confirm that passing one of the flags results in changes in output?

Hi @serathius When I set the flag like below

- --log_file=/path/metrics-server.klog
- --logtostderr=false

logs output to file

[pohly]

Out of curiosity, why did the 0.6.0 release of metrics-server not have those flags anymore?

So it seems it was the removal of unconditional klog flag registration in kubernetes/kubernetes@21d1bcd#diff-343bdf6b972d2cacf3d2be418fc4d36f9ece7b726ff8b23af8c623425284c439

@serathius Is there anything we can and/or should do to notify users of that package that updating to 1.23.x needs additional changes?

[serathius]

/lgtm
/approve

Out of curiosity, why did the 0.6.0 release of metrics-server not have those flags anymore?

So it seems it was the removal of unconditional klog flag registration in kubernetes/kubernetes@21d1bcd#diff-343bdf6b972d2cacf3d2be418fc4d36f9ece7b726ff8b23af8c623425284c439

@serathius Is there anything we can and/or should do to notify users of that package that updating to 1.23.x needs additional changes?

There is no stability guarantees for staging repos, that said we should notify consumers on how they can re-enable klog, to follow K8s deprecation. I'm not sure about best communication channel.

One way would be to use code search to find kubernetes sigs projects that might be affected and file an issue there.
Sending an email to [email protected] might also be a good alternative.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [serathius]

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

[serathius]

/hold

I remembered that this change is needed for v0.6.0 and not the next release.
@yangjunmyfm192085 could you maybe change the PR so it targets release-0.6 branch instead of master?

[serathius]

Ok, #933 (comment) reported that even --v flag doesn't work. This means that this fix is also needed on master branch.

[serathius]

/unhold

[serathius]

/retest

[serathius]

/retest

[yangjunmyfm192085]

/hold

I remembered that this change is needed for v0.6.0 and not the next release. @yangjunmyfm192085 could you maybe change the PR so it targets release-0.6 branch instead of master?

ok, Let me cherry pick into the v0.6.0 branch too.