feat(log): don't emit "Failed to auto-balance volume in ..." for detached volumes #3589
Conversation
WalkthroughThis change refines the error handling logic in the volume controller. In the Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
…ched volumes Longhorn 10302 Signed-off-by: Derek Su <derek.su@suse.com>
c6ef6fc to
af83a2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controller/volume_controller.go (2)
2397-2401: Avoid unnecessary warning logs for detached volumesThe warning message about failing to auto-balance volume is logged even for detached volumes, which can cause log spam.
- if v.Status.State != longhorn.VolumeStateDetached { - log.Warnf("Failed to auto-balance volume in %s state", v.Status.Robustness) - } + if v.Status.State != longhorn.VolumeStateDetached && v.Status.State != longhorn.VolumeStateDetaching { + log.Warnf("Failed to auto-balance volume in %s robustness and %s state", v.Status.Robustness, v.Status.State) + }
4996-5003: Improve error handling for V2 data engine settingThe error handling for the V2 data engine setting could be improved. Currently, it silently defaults to false when there's an error getting the setting.
V2DataEngineFastReplicaRebuilding, err := c.ds.GetSettingAsBool(types.SettingNameV2DataEngineFastReplicaRebuilding) if err != nil { - log.WithError(err).Warnf("Failed to get the setting %v, will consider it as false", types.SettingDefinitionV2DataEngineFastReplicaRebuilding) - V2DataEngineFastReplicaRebuilding = false + return false, errors.Wrapf(err, "failed to get the setting %v", types.SettingDefinitionV2DataEngineFastReplicaRebuilding) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/volume_controller.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (1)
controller/volume_controller.go (1)
4473-4500: LGTM! Well-structured function for checking replica availabilityThe function has comprehensive checks for replica availability including node status and disk readiness, with proper error handling.
|
@mergify backport v1.8.x |
✅ Backports have been created
|
| @@ -2395,7 +2395,9 @@ func (c *VolumeController) getReplicaCountForAutoBalanceLeastEffort(v *longhorn. | |||
| } | |||
|
|
|||
| if v.Status.Robustness != longhorn.VolumeRobustnessHealthy { | |||
| log.Warnf("Failed to auto-balance volume in %s state", v.Status.Robustness) | |||
| if v.Status.State != longhorn.VolumeStateDetached { | |||
There was a problem hiding this comment.
This is good to prevent any confusion.
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10302
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context