CONSOLE-1523: Switch logging to klog

[zherman0]

In order to become more consistent, I have remove the use of capnslog in favor of the k8s.io klog. Also, to handle the transition of log level values from the console operator, I have created a convert helper function that handles both klog and capnslog values.

See CONSOLE-1523

[zherman0]

/assign @spadgett

[spadgett]

Hm, I didn't realize this was going to pull in so many additional dependencies like API machinery.

Build is failing:

 --> RUN ./build.sh
cmd/bridge/main.go:10:2: cannot find package "k8s.io/component-base/logs" in any of:
	/go/src/github.com/openshift/console/vendor/k8s.io/component-base/logs (vendor tree)
	/usr/local/go/src/k8s.io/component-base/logs (from $GOROOT)
	/go/src/k8s.io/component-base/logs (from $GOPATH)
pkg/auth/auth.go:19:2: cannot find package "k8s.io/klog" in any of:
	/go/src/github.com/openshift/console/vendor/k8s.io/klog (vendor tree)
	/usr/local/go/src/k8s.io/klog (from $GOROOT)
	/go/src/k8s.io/klog (from $GOPATH)
error: build error: running './build.sh' failed with exit code 1 

[spadgett]

I'd expect a V(<some-number>) for most info logging.

[zherman0]

I'd expect a V(<some-number>) for most info logging.

@spadgett - I went ahead and set the level for all the info log stmts.

[spadgett]

+46,092 −5,195

I'd like to evaluate this since it's bringing in so many new dependencies. Holding for the moment.

/hold

[zherman0]

I'd like to evaluate this since it's bringing in so many new dependencies. Holding for the moment.

That seems very reasonable.

[benjaminapetersen]

Are 2 dependency commits needed, or can that be squashed into 1 commit?

[zherman0]

@benjaminapetersen - I am just leaving this one alone until @spadgett decides if he wants to add it. I can squash things whenever we decide to take the hold off.

[benjaminapetersen]

Is k8s.io/component-base needed? https://github.com/kubernetes/klog/blob/master/go.mod

[benjaminapetersen]

Guessing this is why the blow up.

[zherman0]

On further investigation, the import component-base that is the main cause for this blowing up in size had 2 functions I call for initialization. Neither function appears to call anything deeper.

// InitLogs initializes logs the way we want for kubernetes.
func InitLogs() {
	log.SetOutput(KlogWriter{})
	log.SetFlags(0)
	// The default glog flush interval is 5 seconds.
	go wait.Forever(klog.Flush, *logFlushFreq)
}
// GlogSetter is a setter to set glog level.
func GlogSetter(val string) (string, error) {
	var level klog.Level
	if err := level.Set(val); err != nil {
		return "", fmt.Errorf("failed set klog.logging.verbosity %s: %v", val, err)
	}
	return fmt.Sprintf("successfully set klog.logging.verbosity to %s", val), nil
}

I can try removing the dependency to component-base and seeing if we reduce the changes and if we still do not lose functionality. However, the apimachinery package is used for wait. I also still need to include the log package.

[spadgett]

@zherman0 @jhadvig Does this require openshift/console-operator#250 ?

[zherman0]

@spadgett - This change is de-coupled from the console operator change (PR250). The PR 250 gets rid of the log level flag from being sent over in capnlog style, but we handle that flag still and just show a message saying it has been deprecated.

[jhadvig]

Does this require openshift/console-operator#250

yes, it should get merged afterwards. It doesn't matter what will be the merge sequence

[spadgett]

/hold cancel

[spadgett]

@jhadvig please review, thanks!

[jhadvig]

@zherman0 don't think this function is needed, since it's not used anywhere. Do you recall whats the purpose by any chance, so we dont overlook anything.

[zherman0]

This function is only for anywhere that people might still be using the the log level so we can informed them it is deprecated. It is not needed but I don't think it hurts to tell people of the change. If we leave it, then we should probably remove it in a release or 2.

[jhadvig]

/lgtm
/approve

[spadgett]

/assign @yapei @ahardin-rh @sferich888
for QE/docs/CEE review

This PR switches internally for the capnslog library to klog for logging in the console backend.

@zherman0 @jhadvig Are there any changes to the operator config for this PR that might impact docs?

[jhadvig]

Don't think will impact docs, since the operator config is not going to change.
We need to merge openshift/console-operator#250 though.

ccing @zherman0

[yapei]

@zherman0 For changes from log to klog, when viewing console logs, how can we differentiate them?
On a 4.6 cluster, I see console log is showing:

2020-10-19T23:47:06Z cmd/main: Flag inactivity-timeout is set to less then 300 seconds and will be ignored!

While on a cluster with changes in this PR, it shows:

sh-4.4$ /opt/bridge/bin/bridge --log-level 5
W1020 05:55:37.303295      25 main.go:211] Flag inactivity-timeout is set to less then 300 seconds and will be ignored!
W1020 05:55:37.303578      25 main.go:254] DEPRECATED: --log-level is now deprecated, use verbosity flag --v=Level instead

How can I know Flag inactivity-timeout is set to less then 300 seconds and will be ignored! is printed with klog?

[jhadvig]

@yapei not sure why we need to differentiate between them?

How can I know Flag inactivity-timeout is set to less then 300 seconds and will be ignored! is printed with klog?

Since we will be using just klog for the backend logging we dont need to differentiate between anything.

[zherman0]

Agreed, not sure why you need to tell the difference, but the new klog stuff shows the file name and line number main.go:211 if that helps.

[yapei]

Yeah, the line number tells the truth @zherman0
When verifying the bug Logging is broken due to mix of k8s.io/klog v1 and v2 we need to confirm whether klog v2 is used. So I thought for this PR we may have same verification process which means I need to confirm klog is used instead of old logging. That's' my thought but maybe I'm wrong @jhadvig @zherman0

[jhadvig]

@yapei @spadgett @zherman0 just one thing to notice here. The console backend will be using the klog/v1 since we need to update helm pkg if we want to use the klog/v2. On the other hand the console-operator moved by this PR to using the klog/v2. This should not do any harm since those are two different binaries.
Just want to point out the difference here and also think out loud about the upgrading to klog/v2 for the console backend binary.

The issue with the helm is that the release we are using (and the latest as well) is using k8s 1.18.0 pkgs version. In order not to have import collisions they need to be on the 1.19.2., which their master is on currently , they just need to release it.

[ahardin-rh]

👍 from the docs side

[yapei]

/label qe-approved

[spadgett]

👍 from the docs side

/label docs-approved

[sferich888]

/label px-approved

[spadgett]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, spadgett, zherman0

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

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