[ocm-2.2] ensure controller runtime uses logrus for logging #1265
Conversation
controller runtime currently defaults to NullLogger [1] which doesn't log anything. This is bad because any error returned from Reconcile function is therefore not logged anywhere which is same as just eating the error. To make sure the controller runtime errors are also logged we would need an adapter for logrus as this project uses that for logging. Therefore, this implements a wrapper around lorgus that satisfies the logr.Logger interface and confure the hive-controller and hive-operator managers with it. The wrapper uses logrus.DebugLevel for all info messages and logrus.ErrorLevel for all error messages from controller-runtime regardless of level (V) selected by caller in controller-runtime. This makes sure all error messages from controller-runtime are always visible and the rest are only visible when in debug mode. [1]: https://github.com/kubernetes-sigs/controller-runtime/blob/9e78e653228851684b6b9cdabe5aae8559fe3722/pkg/log/null.go#L28
|
Can one of the admins verify this patch? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: openshift-cherrypick-robot, suhanime The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/hold @chrisahl or you ok with this logging fix cherry-pick to 2.2? |
openshift-ci-robot
added
the
do-not-merge/hold
|
@gregsheremeta What is the risk and benefit of this fix? |
|
This fix eliminates the possibility of an error getting silently dropped because the hive code explicitly forgot to log it, something we think might be going on with the cluster pool bug reported by ACM, and I believe this was the driver for backporting in the hopes we could get some more info. https://issues.redhat.com/browse/HIVE-1343 Risk should be exceedingly low. |
|
/retest |
|
Sounds like it should go in. |
|
/unhold |
openshift-ci-robot
removed
the
do-not-merge/hold
9 participants
This is an automated cherry-pick of #1263
/assign abhinavdahiya