Adding error handling and logging #694
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fiona-Waters
force-pushed
the
error-handling
branch
from
9481eff to
0869334
Compare
| @@ -42,7 +43,7 @@ func buildConfig(master, kubeconfig string) (*rest.Config, error) { | |||
| func Run(opt *options.ServerOption) error { | |||
| restConfig, err := buildConfig(opt.Master, opt.Kubeconfig) | |||
| if err != nil { | |||
| return err | |||
| return fmt.Errorf("[Run] unable to build server config, - error: %w", err) | |||
There was a problem hiding this comment.
errors should not start with Uppercase.
There was a problem hiding this comment.
Hi @z103cb, are you referring here to the function name - [Run] or to the string following it? Throughout the mcad codebase the log lines are very inconsistent in terms of starting with upper and lowercase, but a lot of them start with the function name which I have done here. Please clarify, thank you.
There was a problem hiding this comment.
I noticed a point in Google's style guide for Go that suggests not capitalizing error strings, as they typically appear within other contexts before being presented to the user:
https://google.github.io/styleguide/go/decisions.html#error-strings
There was a problem hiding this comment.
Hi @ronensc thank you, yes I understand this. I can either remove the [Function] part altogether or make it lowercase [function]? There are a large number of pre-existing error logs in the code base that include the function name which starts with an uppercase letter. So perhaps all of these need to be updated - we need to decide how to proceed in this case.
There was a problem hiding this comment.
@Fiona-Waters I'm with you. I think for this PR, we can keep it as is, and uniquely format the error messages in a future PR once a decision is made.
There was a problem hiding this comment.
It's ok if we fix this later wholesale.
| @@ -71,7 +72,8 @@ func Run(opt *options.ServerOption) error { | |||
| // This call is blocking (unless an error occurs) which equates to <-neverStop | |||
| err = listenHealthProbe(opt) | |||
| if err != nil { | |||
| return err | |||
| return fmt.Errorf("[Run] unable to start health probe listener, - error: %w", err) | |||
There was a problem hiding this comment.
Very awesome job @Fiona-Waters ! I left a few nitpicks and the comment below
I think this file should be addressed as well:
| @@ -2077,9 +2113,9 @@ func (cc *XController) manageQueueJob(ctx context.Context, qj *arbv1.AppWrapper, | |||
| _, err00 := cc.genericresources.SyncQueueJob(qj, &ar) | |||
| if err00 != nil { | |||
| if apierrors.IsInvalid(err00) { | |||
| klog.Warningf("[manageQueueJob] Invalid generic item sent for dispatching by app wrapper='%s/%s' err=%v", qj.Namespace, qj.Name, err00) | |||
| klog.Warningf("[manageQueueJob] Invalid generic item sent for dispatching by app wrapper='%s/%s' - error: %v", qj.Namespace, qj.Name, err00) | |||
There was a problem hiding this comment.
This should probably be an Error log.
There was a problem hiding this comment.
Here I just updated the formatting so I'm not sure about changing this. @z103cb added this a few months ago. Maybe there were reasons for using klog.Warningf here rather than Errorf.
There was a problem hiding this comment.
If I recall correctly, this was done to not generate a spurious error message when there's a bad definition for the generic resource. As they say beauty in in the eye of the beholder, if you will if this warning should be an error I would not object.
Thanks for the review. This issue wasn't fully completed but as we are moving to mcad v2 Anish suggested I create a PR with what was completed so far, but not to do any more. Therefore all files haven't been covered. |
|
/LGTM |
|
@z103cb: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Issue link
Closes #613
What changes have been made
Added error handling and logging. This issue has not been fully completed as we will now be working on V2 of MCAD. Hoping to merge the work that was done before this decision was made.
Verification steps
Code review.
Checks