Add error dictionary entries for operator reconciler #26311
Conversation
pkg/errs-dictionary/errdict.go
Outdated
| OperatorFailedToRemoveFinalizer = &structured.Error{ | ||
| MoreInfo: "This error occurred because the finalizer set by the operator controller could not be removed " + | ||
| "when the IstioOperator CR was deleted.", | ||
| Impact: "The error indicates that the IstioOperator CR can not be removed by the operator controller.", |
There was a problem hiding this comment.
meta comment: do we want to be so verbose/wordy? ie do we want
impact=IstioOperator CR can not be removed by the operator controller
or
impact=The error indicates that the IstioOperator CR can not be removed by the operator controller.
IMO if we want to use plain english proper sentences we should have an arbitrary blob (ie old log.Errorf but write good messages). If we are going to use structured logging lets make use of the fields instead
There was a problem hiding this comment.
good point, yes on reflection i think the wordiness doesn't help much with the actual error and just adds more text. the same can probably be said of go comment style and i've probably been brainwashed by writing too much go. let's make the guideline to be comprehensible but minimal.
pkg/errs-dictionary/errdict.go
Outdated
| ActionIfErrPersistsContactSupport = "If this error persists, " + ActionContactSupport | ||
| ActionIfErrSureCorrectConfigContactSupport = "If you are sure your configuration is correct, " + ActionContactSupport | ||
| ActionContactSupport = "contact support if using managed Istio. Otherwise check if the issue is already fixed in " + | ||
| "another version of Istio at https://github.com/istio/istio/issues, raise an issue, or ask for help in " + |
There was a problem hiding this comment.
nit: do we want to just like to some like like https://istio.io/latest/about/bugs/ so this isn't so verbose?
There was a problem hiding this comment.
good suggestion, thanks, done.
| @@ -380,7 +384,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| log.Info("Controller added") | |||
| scope.Info("Controller added.") | |||
There was a problem hiding this comment.
by some poor grep, I think that <1% of logs are ending with a period today, nor are kubernetes or envoy logs. Should we be consistent?
There was a problem hiding this comment.
where things get weird is when we have multiple sentences. period after the first but not the last?
i agree about consistency but this one may be a bit of a grey area that i'd be happy to leave to individual discretion.
| var jwtPolicy util.JWTPolicy | ||
| if jwtPolicy, err = util.DetectSupportedJWTPolicy(r.config); err != nil { | ||
| log.Warnf("Failed to detect third-party JWT support: %v", err) | ||
| // TODO: add to dictionary. {status=New, assignee=} |
There was a problem hiding this comment.
What is {status=New, assignee=}?
There was a problem hiding this comment.
assignee=howardjohn :-)
i want to have some annotation to keep track of log entries that need fixing but also want to capture WontFix type of resolution if the assignee things the status quo is fine. how about this instead:
TODO(): add to dictionary. When resolved, replace this sentence with Done or WontFix.
| return reconcile.Result{}, nil | ||
| } | ||
| log.Errorf("Failed to update IstioOperator with finalizer, %v", err) | ||
| scope.Errorf(errdict.OperatorFailedToAddFinalizer, "Failed to add finalizer to IstioOperator CR %s: %s", iopName, err) |
There was a problem hiding this comment.
So this will log:
Failed to add finalizer to IstioOperator CR foobar: some error message moreInfo=This error occurred because a finalizer could not be added to the IstioOperator CR. The controller uses the finalizer to temporarily prevent the CR from being deleted while the Istio control plane is being deleted. impact=The error indicates that when the IstioOperator CR is deleted, the Istio control plane may not be fully removed. action=Delete and re-add the IstioOperator CR. If this error persists, contact support if using managed Istio. Otherwise check if the issue is already fixed in another version of Istio at https://github.com/istio/istio/issues, raise an issue, or ask for help in discuss.istio.io. likelyCause=a problem with the k8s API server
Did we need 112 words to express this? It seems a bit repetative
There was a problem hiding this comment.
yeah i agree, per your other comments i'm trimming things back. but keep in mind that this is an error which should be an exceptional condition. if it's spamming repeatedly, that should be fixed. otherwise i haven't ever heard users complaining that an error message was too verbose :-)
|
thanks for the detailed review @howardjohn. addressed comments, ptal. |
| var jwtPolicy util.JWTPolicy | ||
| if jwtPolicy, err = util.DetectSupportedJWTPolicy(r.config); err != nil { | ||
| log.Warnf("Failed to detect third-party JWT support: %v", err) | ||
| // TODO(howardjohn): add to dictionary. When resolved, replace this sentence with Done or WontFix - if WontFix, add reason. |
There was a problem hiding this comment.
I don't think this is a good idea.. do we expect every single log to have a comment with Done or WontFix?
There was a problem hiding this comment.
only temporarily, once we've scrubbed/fixed the logs i'll remove all these.
i couldn't think of a better way to track which messages have been cleaned up but i'm open to suggestions.
|
@ostromart: The following test failed, say
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. I understand the commands that are listed here. |
First batch of errors restructured to use the error dictionary where appropriate.