🌱 log to logger from context during drain #6170
Conversation
Signed-off-by: Stefan Büringer buringerst@vmware.com
k8s-ci-robot
added
cncf-cla: yes
| @@ -530,8 +529,10 @@ func (r *Reconciler) drainNode(ctx context.Context, cluster *clusterv1.Cluster, | |||
| log.Info(fmt.Sprintf("%s pod from Node", verbStr), | |||
| "pod", fmt.Sprintf("%s/%s", pod.Name, pod.Namespace)) | |||
| }, | |||
| Out: writer{klog.Info}, | |||
| ErrOut: writer{klog.Error}, | |||
| Out: writer{log.Info}, | |||
There was a problem hiding this comment.
I think log level 0 is too high for those logs. What should we use?
There was a problem hiding this comment.
Given that it deals with users' workloads I don't mind being verbose here.
But probably we will get back to this soon with more context as soon as we consolidate log improvements actions
|
/test pull-cluster-api-test-mink8s-main |
k8s-ci-robot
added
the
lgtm
| ErrOut: writer{klog.Error}, | ||
| Out: writer{log.Info}, | ||
| ErrOut: writer{func(msg string, keysAndValues ...interface{}) { | ||
| log.Error(nil, msg, keysAndValues) |
There was a problem hiding this comment.
Not sure if we should log the whole message as error or log it as I currently did.
There was a problem hiding this comment.
I think this is fine as you've done it here.
|
@fabriziopandini Do we want to merge it? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
k8s-ci-robot
added
the
approved
Signed-off-by: Stefan Büringer buringerst@vmware.com
What this PR does / why we need it:
Apart from main functions and test code we should never use the global klog logger.
By using the one from the context we can make sure that the log lines have the right k/v pairs.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #