-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(recurring-job): flooded source label log #2058
fix(recurring-job): flooded source label log #2058
Conversation
61ca07c
to
5920b42
Compare
controller/volume_controller.go
Outdated
@@ -3647,7 +3647,7 @@ func (c *VolumeController) syncPVCRecurringJobLabels(volume *longhorn.Volume) er | |||
} | |||
|
|||
if !hasSourceLabel { | |||
c.logger.Warnf("Ignoring recurring job labels on Volume %v PVC %v due to missing source label", volume.Name, pvc.Name) | |||
logrus.Tracef("Ignoring recurring job labels on Volume %v PVC %v due to missing source label", volume.Name, pvc.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using the existing c.logger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most log messages' levels are corrected. I think we use INFO as default log level. Then, use c.logger.Debugf
here. WDYT? @c3y1huang @innobead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we still use debug, confirm? @derekbit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c3y1huang do u know why it doesn't have trace? Duo to a different interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c3y1huang do u know why it doesn't have trace? Duo to a different interface?
Yes, upstream removed it due to backward compatibility concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we still use debug, confirm? @derekbit
How about changing it to INFO from v1.5.1?
I've adjusted most of log messages' levels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
ref: 6257 Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
5920b42
to
408fae8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
BTW, I adjust some logs in #2059. Please help review it if you have time. Thank you.
@Mergifyio backport v1.5.x |
✅ Backports have been created
|
@c3y1huang need to backport this to 1.4 and 1.3 with a different implementation since their default log level is debug. |
@Mergifyio backport v1.4.x v1.3.x |
✅ Backports have been created
|
I think we don't need to backport this to 1.4 and 1.3 since the source label is introduced in v1.5. |
@c3y1huang please go ahead to remove backup labels. Thanks. |
longhorn/longhorn#6257