glog/klog init issue

[tamalsaha]

My understanding is that glog->klog migration was done to solve the issue that glog adds flags inside a init() function. But it seems that this issue is not completely solved.

k8s.io/apiserver imports the logs util package which adds flags inside init() function.

https://github.com/kubernetes/kubernetes/blob/c9e14c85f5b826493a2376ab4743499ac834b8ee/staging/src/k8s.io/apiserver/pkg/server/config.go#L569

https://github.com/kubernetes/component-base/blob/ebca9cef46a2513b205fffed7d60d53fc50979bb/logs/logs.go#L35

As things stand, I am getting flag redefined: log_dir when used alongside glog.

/tmp/go-build285405043/b001/exe/main flag redefined: log_dir
panic: /tmp/go-build285405043/b001/exe/main flag redefined: log_dir

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc0000d6180, 0x1edd100, 0x2bf53d0, 0x152d0fb, 0x7, 0x1560b27, 0x2f)
    /usr/local/go/src/flag/flag.go:805 +0x529
flag.(*FlagSet).StringVar(0xc0000d6180, 0x2bf53d0, 0x152d0fb, 0x7, 0x0, 0x0, 0x1560b27, 0x2f)
    /usr/local/go/src/flag/flag.go:708 +0x8a
github.com/appscode/stash/vendor/k8s.io/klog.InitFlags(0xc0000d6180)
    /home/tamal/go/src/github.com/appscode/stash/vendor/k8s.io/klog/klog.go:411 +0x7b
github.com/appscode/stash/vendor/k8s.io/apiserver/pkg/util/logs.init.0()
    /home/tamal/go/src/github.com/appscode/stash/vendor/k8s.io/apiserver/pkg/util/logs/logs.go:36 +0x2d
exit status 2

One solution will be to move the init() function to its own pkg and ensuring that it is only called from main package in other binaries.

[tamalsaha]

cc: @dims

[tamalsaha]

/sig api-machinery /kind bug

[roycaihw]

/assign

[ash2k]

One solution will be to move the init() function to its own pkg and ensuring that it is only called from main package in other binaries.

Or just call klog.InitFlags() directly from main() where it's needed. i.e. there is no need for it to be called from an init(), right? Commenting because we've run into this issue too.

[nilebox]

Agree with @ash2k. We have to explicitly invoke klog.InitFlags() for CRD controllers, but if we do the same in a custom API server, we get panic (see https://github.com/kubernetes-incubator/service-catalog/pull/2528#issuecomment-455372801).

AFAICT, the only reason the main Kubernetes itself doesn't have this problem is because API servers and controller-managers share the main code via hyperkube, and as a result controller-manager imports k8s.io/apiserver's util package.

[tamalsaha]

Or just call klog.InitFlags() directly from main() where it's needed. i.e. there is no need for it to be called from an init(), right?

@ash2k, yes to what you said. It is ironic that glog was forked due to init, then init was reinvented in this util logs package.

[fejta-bot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

[devdattakulkarni]

@tamalsaha How did you solve the problem?

@nilebox I am still getting the error even after calling klog.InitFlags() in my main().

[devdattakulkarni]

Update: Pinning component-base in go.mod to following version solved it for me.

replace k8s.io/component-base => k8s.io/component-base v0.0.0-20190612130303-4062e14deebe

[nilebox]

/remove-lifecycle stale

[yue9944882]

got burnt by this bunch of times. this is driving me crazy.. 🤯

[roycaihw]

I agree having a package (k8s.io/component-base/logs in this case) define its command line flags in init() is problematic. This issue was also reported in https://github.com/kubernetes/kubernetes/issues/75403

ways to fix:

1. as @ash2k suggested, let's remove klog.InitFlags() from init() in k8s.io/component-base/logs, and properly call klog.InitFlags() in any main() that needs it.

Cons:

• We can make the change safely within k/k, but some other projects that depend on k8s.io/component-base/logs might lose their command line flag definition when they upgrade k8s.io/component-base/logs. I'm not sure there are such cases. If so we need to have a release note.
• There might be a lot of files need to be changed. Below are files importing k8s.io/component-base/logs directly, there can be more indirect imports:

$ grep -r "k8s.io\/component-base\/logs" | grep -v "BUILD" | grep -v "Binary file"
pkg/kubelet/server/server.go:   "k8s.io/component-base/logs"
test/e2e/e2e.go:    "k8s.io/component-base/logs"
test/images/agnhost/no-snat-test-proxy/main.go: "k8s.io/component-base/logs"
test/images/agnhost/inclusterclient/main.go:    "k8s.io/component-base/logs"
test/images/agnhost/no-snat-test/main.go:   "k8s.io/component-base/logs"
vendor/modules.txt:k8s.io/component-base/logs
.make/all_go_dirs.mk:vendor/k8s.io/component-base/logs
staging/src/k8s.io/sample-apiserver/main.go:    "k8s.io/component-base/logs"
staging/src/k8s.io/kube-aggregator/main.go: "k8s.io/component-base/logs"
staging/src/k8s.io/apiserver/pkg/server/config.go:  "k8s.io/component-base/logs"
staging/src/k8s.io/component-base/cli/globalflag/globalflags.go:    "k8s.io/component-base/logs"
staging/src/k8s.io/apiextensions-apiserver/main.go: "k8s.io/component-base/logs"
cmd/kubemark/hollow-node.go:    "k8s.io/component-base/logs"
cmd/cloud-controller-manager/controller-manager.go: "k8s.io/component-base/logs"
cmd/kube-proxy/proxy.go:    "k8s.io/component-base/logs"
cmd/hyperkube/main.go:  "k8s.io/component-base/logs"
cmd/kube-apiserver/apiserver.go:    "k8s.io/component-base/logs"
cmd/kubelet/app/options/globalflags.go: "k8s.io/component-base/logs"
cmd/kubelet/kubelet.go: "k8s.io/component-base/logs"
cmd/kube-scheduler/scheduler.go:    "k8s.io/component-base/logs"
cmd/kube-controller-manager/controller-manager.go:  "k8s.io/component-base/logs"

2. Make klog check flag existence and skip registration if flag exists already. This helps if klog.InitFlags() is called twice somehow.

Cons:

• This doesn't work with glog / manual flag registration that comes later

One solution will be to move the init() function to its own pkg and ensuring that it is only called from main package in other binaries.

@tamalsaha Could you elaborate more on "move the init() function to its own pkg"?

[fejta-bot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

[nilebox]

/remove-lifecycle stale

[roycaihw]

/unassign

I don't have the cycle to work on it, so I will let someone else to take a stab.

[fejta-bot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

[ash2k]

/remove-lifecycle stale /lifecycle frozen