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

Fixes broken migration to klog #188

Merged
merged 2 commits into from
Feb 12, 2019
Merged

Fixes broken migration to klog #188

merged 2 commits into from
Feb 12, 2019

Conversation

gman0
Copy link
Contributor

@gman0 gman0 commented Feb 9, 2019

Fixes #187

Bug introduced in #150 (quite insufficient testing - plugins would panic immediately, #168) by not playing nicely with vendor'd packages that still use glog (github.com/kubernetes-csi/drivers/pkg/csi-common).

This then continued on in #169 which fixed the panic, but skipping klog.InitFlags() means logtostderr cmd flag doesn't get propagated to klog.

This PR reintroduces klog.InitFlags() in a hopefully less conflicting manner with glog.

@gman0
Copy link
Contributor Author

gman0 commented Feb 10, 2019

cc @rootfs @humblec

@@ -42,8 +35,27 @@ var (
)

func main() {
if err := flag.Set("logtostderr", "true"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

make a util function for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm getting

E0212 15:15:23.815436    5850 log.go:38] failed to set log_backtrace_at flag: syntax error: expect file.go:234

if I put it in the ceph-csi/pkg/util package - I'm not 100% sure why...i guess because it's in another package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, should be working now...I had to ignore error checking when copying args from glog to klog though.

@rootfs
Copy link
Member

rootfs commented Feb 11, 2019

thanks @gman0

@humblec
Copy link
Collaborator

humblec commented Feb 11, 2019

@gman0 Thanks.. One question here is, the klogInitFlags() has to happen before flags.set https://github.com/ceph/ceph-csi/pull/188/files#diff-48c9752907803b1df4780beae292b547R38, otherwise it will be NO OP , Isnt it ?

@gman0
Copy link
Contributor Author

gman0 commented Feb 11, 2019

@humblec I don't think it makes a difference in this case, because glog already initializes the cmd flags:

First, glog initializes the global command-line FlagSet in its init() function:

func init() {
flag.BoolVar(&logging.toStderr, "logtostderr", false, "log to standard error instead of files")
flag.BoolVar(&logging.alsoToStderr, "alsologtostderr", false, "log to standard error as well as files")
flag.Var(&logging.verbosity, "v", "log level for V logs")
flag.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr")
flag.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging")
flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")

...which means the flag.Set() in this PR actually sets the logtostderr flag for glog, not klog => it's not a NOOP.

After the values are parsed by flag.Parse(), a new FlagSet (a separate one, for klog only) is created and klog stores its cmd args here (that's why it doesn't panic anymore, it uses a separate flagset - no duplicate flags):

func InitFlags(flagset *flag.FlagSet) {
if flagset == nil {
flagset = flag.CommandLine
}
flagset.StringVar(&logging.logDir, "log_dir", "", "If non-empty, write log files in this directory")
flagset.StringVar(&logging.logFile, "log_file", "", "If non-empty, use this log file")
flagset.BoolVar(&logging.toStderr, "logtostderr", false, "log to standard error instead of files")
flagset.BoolVar(&logging.alsoToStderr, "alsologtostderr", false, "log to standard error as well as files")
flagset.Var(&logging.verbosity, "v", "log level for V logs")
flagset.BoolVar(&logging.skipHeaders, "skip_headers", false, "If true, avoid header prefixes in the log messages")
flagset.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr")
flagset.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging")
flagset.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")

Lastly, the values from glog flagset are copied over to the values of klog flagset. Granted, this is hardly ideal because there are some flags that klog has and glog hasn't, and those cannot be set (because glog manages the global cmd flagset) but AFAIK there's no way around this, unless csi-common moves to klog or we move away from csi-common.

@humblec
Copy link
Collaborator

humblec commented Feb 12, 2019

@gman0 The csi-common migration to klog is under review at the same repo by PR #144 , may be getting it done first would be the right choice. I have to do some changes in that PR and I am on it.

@gman0
Copy link
Contributor Author

gman0 commented Feb 12, 2019

PTAL @rootfs @humblec

CI is failing on an unrelated file:

$ scripts/lint-text.sh --require-all
mdl --style scripts/mdl-style.rb ./README.md 
./README.md:27: MD012 Multiple consecutive blank lines

may be fixed in another PR

@rootfs rootfs merged commit 5ee0751 into ceph:csi-v1.0 Feb 12, 2019
@rootfs
Copy link
Member

rootfs commented Feb 12, 2019

rerun CI and it passed. megify failed to merge, will figure out that later. thanks @gman0

@gman0 gman0 mentioned this pull request Feb 12, 2019
wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019
Fixes broken migration to klog
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/ceph-csi that referenced this pull request Oct 11, 2023
Sync the upstream changes from `ceph/ceph-csi:devel` into the `devel` branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging is broken
3 participants