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

Update klog to 0.3.0 #76474

Merged
merged 1 commit into from Apr 18, 2019

Conversation

@vincepri
Copy link
Member

commented Apr 11, 2019

Signed-off-by: Vince Prignano vincepri@vmware.com

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR updates klog dependency to the latest release: 0.3.0.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Updated klog to 0.3.0
@dims

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

/approve
/assign @liggitt @cblecker

@dims

dims approved these changes Apr 11, 2019

@vincepri vincepri force-pushed the vincepri:update-klog-030 branch from 90977ca to 40290d8 Apr 11, 2019

@@ -105,6 +105,7 @@ func addGlogFlags(fs *pflag.FlagSet) {
register(global, local, "log_backtrace_at")
register(global, local, "log_dir")
register(global, local, "log_file")
register(global, local, "log_file_max_size")

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 12, 2019

Member

is it our intent to keep adding global flags like this? I expected us to manage flags in a better way going forward, especially if we control the log library

This comment has been minimized.

Copy link
@vincepri

vincepri Apr 12, 2019

Author Member

Honestly I'm not sure. I updated the code where the codebase is referenced, I can take this one out I think, but the other change in components-base is required otherwise some tests fail.

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 12, 2019

Member

To be clear, the issue is with the change made in klog, but this change here is a good example of the further pollution of the global flag space it is causing. Before we pull it into kubernetes, we should resolve the future direction of klog flag handling.

This comment has been minimized.

Copy link
@liggitt

This comment has been minimized.

Copy link
@dims

dims Apr 12, 2019

Member

@liggitt please see kubernetes/klog#56 and kubernetes/klog#55 we can continue the conversation there

@vincepri vincepri force-pushed the vincepri:update-klog-030 branch 4 times, most recently from 861b6c4 to 2d63c92 Apr 18, 2019

Update klog to 0.3.0
Signed-off-by: Vince Prignano <vincepri@vmware.com>

@vincepri vincepri force-pushed the vincepri:update-klog-030 branch from 2d63c92 to 3f55226 Apr 18, 2019

@vincepri

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

/retest

@vincepri

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@dims @liggitt All tests passed 🎉, ptal

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 18, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 17fe18b into kubernetes:master Apr 18, 2019

20 checks passed

cla/linuxfoundation vincepri authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@mtaufen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

FYI it looks like this may have broken Kubelet's log to file behavior: #77416

@yujuhong mentioned it now writes to the log file up to the point where it tries to load the Kubelet config file, then starts logging to stderr afterwards. This makes me wonder if there is some nefarious interaction between how klog registers its flags and the way we currently enforce flag precedence.

@mtaufen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@liggitt @vincepri

I think the issue is that this PR changes the global FlagSet construction logic to call klog.InitFlags every time a new FlagSet is constructed. This has the side effect of resetting the values of all klog flags to defaults. The old logic constructed a FlagSet that referenced the global flags without changing their values.

Constructing the global flag set should not have such a side-effect, because the Kubelet currently needs to re-parse the full command line to enforce precedence while explicitly not calling Set for global flag values a second time, or else accumulator flags could end up with duplicate entries during the re-parse.

@dims

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@mtaufen also see #77305 could that be the same problem you are describing above?

// addKlogFlags adds flags from k8s.io/klog
func addKlogFlags(fs *pflag.FlagSet) {
local := flag.NewFlagSet(os.Args[0], flag.ExitOnError)
klog.InitFlags(local)

This comment has been minimized.

Copy link
@mtaufen

mtaufen May 7, 2019

Contributor

This has the side effect of resetting the values in klog to defaults every time a new flagset is constructed.

@mtaufen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@dims I believe the problem I described only happens in the Kubelet; I don't think any other components have copied the flag precedence approach in the Kubelet yet (hence the legacyflag work). That said, if other components wanted to do something similar, a similar call to InitFlags in the globalflag package in component-base will cause the same issue.

I think we should be able to fix by just reverting the Kubelet code to register each klog flag individually, and avoid the side-effect.

@mtaufen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Alternatively, we could revert this PR, then fix klog so that InitFlags itself is free of global side-effects.

@mtaufen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Former fix: #77562

@vincepri

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

I'll defer to @liggitt and @dims on path forward, happy to put in some time for a fix if needed

@dims

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@mtaufen thanks!

@vincepri looks like @mtaufen has filed the fix for "reverting the Kubelet code", can you please do the other one? " register each klog flag individually " Thanks!

@mtaufen

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@vincepri FYI reverting the Kubelet code == registering each klog flag individually. The other one was "fix klog so that InitFlags itself is free of global side-effects."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.