hooks: improve logging for hooks (#2208)#2373
hooks: improve logging for hooks (#2208)#2373kumarsgoyal wants to merge 2 commits intoRamenDR:mainfrom
Conversation
- Add contextual errors for invalid nameSelector and labelSelector - Enhance ScaleHook logging for discovery and processing failures - Log successful scale operations with resource counts Fixes: RamenDR#2208 Signed-off-by: kumarsgoyal <kumar.sgoyal@gmail.com>
kumarsgoyal
force-pushed
the
issue-2208
branch
from
d1b753a to
3c673e5
Compare
| "failed listing resources using valid nameSelector: %w, hook=%q, namespace=%q, selector=%q", | ||
| err, hook.Name, hook.Namespace, hook.NameSelector, | ||
| ) | ||
| } |
There was a problem hiding this comment.
In this file you are using another pattern to return error:
3c673e5#diff-7f508f09c25a8296dcf66bb31da145a6acb98b492bd51f9907ba6de12532b6fdR79
I mean after "using nameSelector" you are printing hook nameselector and after a colon - real error.
Maybe it's better create the same pattern for all errors?
| ) error { | ||
| if len(hook.LabelSelector.MatchLabels) == 0 && len(hook.LabelSelector.MatchExpressions) == 0 { | ||
| return fmt.Errorf("labelSelector is empty in hook %q", hook.Name) | ||
| } |
There was a problem hiding this comment.
Sometimes the error messages include the hook namespace, and sometimes they don’t. It would be better to follow a consistent standard.
| if err != nil { | ||
| return fmt.Errorf("error listing resources using labelSelector: %w", err) | ||
| return fmt.Errorf("error listing resources using labelSelector: %w, hook=%q namespace=%q", | ||
| err, hook.Name, hook.Namespace) |
There was a problem hiding this comment.
Better to use format:
fmt.Errorf( "list resources using labelSelector failed (hook=%q namespace=%q): %w", hook.Name, hook.Namespace, err, )
so that error is added in the last and not in the middle. Applicable for all the other error returns/logs.
- Standardize error messages to wrap the underlying error last using %w - Add context to all name/label selector errors Fixes: RamenDR#2208 Signed-off-by: kumarsgoyal <kumar.sgoyal@gmail.com>
kumarsgoyal
force-pushed
the
issue-2208
branch
from
db56952 to
357f42c
Compare
| objs, err := getObjectsUsingValidK8sName(r, hook, objList) | ||
| if err != nil { | ||
| return ValidNameSelector, filteredObjs, fmt.Errorf( | ||
| "list resources using valid nameSelector=%q failed (hook=%q namespace=%q): %w", |
There was a problem hiding this comment.
error listing resources.... would be good.
| if err != nil { | ||
| return RegexNameSelector, filteredObjs, err | ||
| return RegexNameSelector, filteredObjs, fmt.Errorf( | ||
| "list resources using regex nameSelector=%q failed (hook=%q namespace=%q): %w", |
There was a problem hiding this comment.
error listing resources... would be good.
| if err != nil { | ||
| return nil, fmt.Errorf("error listing resources using nameSelector: %w", err) | ||
| return nil, fmt.Errorf( | ||
| "list resources using nameSelector=%q failed (hook=%q namespace=%q): %w", |
There was a problem hiding this comment.
error listing resources... would be good.
| ) error { | ||
| if len(hook.LabelSelector.MatchLabels) == 0 && len(hook.LabelSelector.MatchExpressions) == 0 { | ||
| return fmt.Errorf( | ||
| "list resources using labelSelector failed: empty MatchLabels and MatchExpressions "+ |
There was a problem hiding this comment.
error listing resources... would be good.
| selector, err := metav1.LabelSelectorAsSelector(hook.LabelSelector) | ||
| if err != nil { | ||
| return fmt.Errorf("error converting labelSelector to selector") | ||
| return fmt.Errorf("convert labelSelector to selector failed (hook=%q namespace=%q): %w", |
There was a problem hiding this comment.
return fmt.Errorf("error converting labelSelector to selector (hook=%q namespace=%q): %w",
hook.Name, hook.Namespace, err)
would be good
| err = r.List(context.Background(), objList, listOps) | ||
| if err != nil { | ||
| return fmt.Errorf("error listing resources using labelSelector: %w", err) | ||
| return fmt.Errorf("list resources using labelSelector failed (hook=%q namespace=%q): %w", |
There was a problem hiding this comment.
error listing resources... would be good.
3 participants
Fixes #2208