fix(logging): make RPC call to instance-manager to set log level #3280
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
controller/instance_manager_controller.go (2)
534-547: Approve changes with suggestions for error handling improvementsThe implementation for V1 data engine log level synchronization looks good. However, the error handling could be more specific to help with debugging.
Consider enhancing the error handling by adding more context:
- err = client.LogSetLevel(longhorn.DataEngineTypeV1, "", setting.Value) - if err != nil { - return errors.Wrapf(err, "failed to set instance-manager log level from %v", settingName) - } + if err = client.LogSetLevel(longhorn.DataEngineTypeV1, "", setting.Value); err != nil { + return errors.Wrapf(err, "failed to set instance-manager log level to %v for data engine v1", setting.Value) + }
534-535: Move client.Close() defer statement upThe client.Close() defer statement should be placed immediately after client creation to ensure cleanup in all code paths.
Move the defer statement up:
if types.IsDataEngineV1(im.Spec.DataEngine) { + defer client.Close() settingName := types.SettingNameLogLevel
| if types.IsDataEngineV1(im.Spec.DataEngine) { | ||
| settingName := types.SettingNameLogLevel | ||
| setting, err := imc.ds.GetSettingWithAutoFillingRO(settingName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = client.LogSetLevel(longhorn.DataEngineTypeV1, "", setting.Value) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to set instance-manager log level from %v", settingName) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Why can this setting only be applied to v1 data engine? v2 data engine should be applied as well.
There was a problem hiding this comment.
Ah, thank you.
There was a problem hiding this comment.
Remedied and re-pushed.
james-munson
force-pushed
the
6702-set-log-level
branch
from
5215c8d
to
24d416d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
controller/instance_manager_controller.go (1)
546-558: LGTM with suggestions for error handlingThe implementation correctly handles log level synchronization for both V1 and V2 data engines. However, the error handling could be improved.
Consider consolidating the error handling to reduce duplication:
- if err != nil { - return errors.Wrapf(err, "failed to set instance-manager log level to %v, using setting %v", setting.Value, settingName) - } - case types.SettingNameV2DataEngineLogFlags: - err = client.LogSetFlags(longhorn.DataEngineTypeV2, "spdk_tgt", setting.Value) - if err != nil { - return errors.Wrapf(err, "failed to set spdk_tgt log flags to %v, using setting %v", setting.Value, settingName) - } + if err != nil { + return errors.Wrapf(err, "failed to set %v to %v for setting %v", + settingName == types.SettingNameLogLevel ? "log level" : "spdk_tgt log flags", + setting.Value, settingName) + } + case types.SettingNameV2DataEngineLogFlags: + if err := client.LogSetFlags(longhorn.DataEngineTypeV2, "spdk_tgt", setting.Value); err != nil { + return errors.Wrapf(err, "failed to set %v to %v for setting %v", + "spdk_tgt log flags", setting.Value, settingName) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
controller/instance_manager_controller.go(2 hunks)
🔇 Additional comments (1)
controller/instance_manager_controller.go (1)
535-535: LGTM: Generalized log level setting name
The change from engine-specific to a generic log level setting name improves the configurability across all Longhorn components.
james-munson
force-pushed
the
6702-set-log-level
branch
from
24d416d
to
29f95bf
Compare
| case types.SettingNameV2DataEngineLogFlags: | ||
| err = client.LogSetFlags(longhorn.DataEngineTypeV2, "spdk_tgt", setting.Value) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to set log flags for %v", settingName) | ||
| if types.IsDataEngineV2(im.Spec.DataEngine) { | ||
| err = client.LogSetFlags(longhorn.DataEngineTypeV2, "spdk_tgt", setting.Value) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to set spdk_tgt log flags to %v, using setting %v", setting.Value, settingName) | ||
| } |
There was a problem hiding this comment.
Are we deprecating this setting?
There was a problem hiding this comment.
Yes, that's the conclusion of the discussion yesterday. No need for granularity to set it separately for instance-manager and spdk_tgt. That will need to be updated in the docs.
There was a problem hiding this comment.
Oh, wait. We deprecated V2DataEngineLogLevel. LogFlags needs to remain. That is a separate feature of SPDK logging.
| case types.SettingNameLogLevel: | ||
| if types.IsDataEngineV1(im.Spec.DataEngine) { | ||
| err = client.LogSetLevel(longhorn.DataEngineTypeV1, "", setting.Value) | ||
| } else { | ||
| err = client.LogSetLevel(longhorn.DataEngineTypeV2, "", setting.Value) | ||
| } |
There was a problem hiding this comment.
What about other components like UI, csi plugin, csi side car pod as mentioned at longhorn/longhorn#6702 (comment) ? Will we handle them in this PR?
There was a problem hiding this comment.
That would be a separate PR, although less critical, since they don't log as heavily. It shouldn't gate this change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
controller/instance_manager_controller.go (1)
Line range hint
535-560: Consider adding validation for log level and flag values.The implementation correctly handles both v1 and v2 data engines, but could benefit from validating the setting values before applying them.
Consider adding validation:
case types.SettingNameLogLevel: + validLogLevels := []string{"info", "debug", "warn", "error"} + if !slices.Contains(validLogLevels, strings.ToLower(setting.Value)) { + return fmt.Errorf("invalid log level %v", setting.Value) + } if types.IsDataEngineV1(im.Spec.DataEngine) { err = client.LogSetLevel(longhorn.DataEngineTypeV1, "", setting.Value)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
controller/instance_manager_controller.go(2 hunks)
🔇 Additional comments (3)
controller/instance_manager_controller.go (3)
546-551: LGTM! Clean implementation of data engine specific log level setting.
The code correctly differentiates between v1 and v2 data engines when setting log levels.
556-560: LGTM! V2-specific log flags handling.
The code correctly applies log flags only for v2 data engine.
Line range hint 577-580: LGTM! Appropriate error handling for log settings sync.
The integration properly handles log setting sync errors by logging them without blocking pod management, which is the correct approach for non-critical operations.
Signed-off-by: James Munson <[email protected]>
james-munson
force-pushed
the
6702-set-log-level
branch
from
29f95bf
to
95f3152
Compare
| @@ -536,7 +532,7 @@ func (imc *InstanceManagerController) syncLogSettingsToInstanceManagerPod(im *lo | |||
| defer client.Close() | |||
|
|
|||
| settingNames := []types.SettingName{ | |||
| types.SettingNameV2DataEngineLogLevel, | |||
| types.SettingNameLogLevel, | |||
| types.SettingNameV2DataEngineLogFlags, | |||
There was a problem hiding this comment.
Rename SettingNameV2DataEngineLogFlags to SettingNameLogFlags. Make it generalized although v1 data engine doesn't need it.
Which issue(s) this PR fixes:
Issue longhorn/longhorn#6702
What this PR does / why we need it:
Sync the log level with instance-managers.
Special notes for your reviewer:
Additional documentation or context