-
Notifications
You must be signed in to change notification settings - Fork 561
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
Conversation
cmd/cephfs/main.go
Outdated
@@ -42,8 +35,27 @@ var ( | |||
) | |||
|
|||
func main() { | |||
if err := flag.Set("logtostderr", "true"); err != nil { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
thanks @gman0 |
@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 ? |
@humblec I don't think it makes a difference in this case, because glog already initializes the cmd flags: First, ceph-csi/vendor/github.com/golang/glog/glog.go Lines 398 to 404 in 1748629
...which means the After the values are parsed by ceph-csi/vendor/k8s.io/klog/klog.go Lines 407 to 419 in 1748629
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 |
rerun CI and it passed. megify failed to merge, will figure out that later. thanks @gman0 |
Fixes broken migration to klog
Sync the upstream changes from `ceph/ceph-csi:devel` into the `devel` branch.
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()
meanslogtostderr
cmd flag doesn't get propagated to klog.This PR reintroduces
klog.InitFlags()
in a hopefully less conflicting manner with glog.