SRIOV persistent logging enhancement - #1057
Conversation
Sriov presistent logging enhancement - host-based log storage with rotation for the sriov-network-config-daemon
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the SR-IOV network configuration daemon by implementing persistent, host-based logging with rotation. This change addresses a critical debugging gap where daemon logs were lost after container restarts or node reboots, making it difficult to diagnose issues or understand the reasons behind system reconfigurations. By persisting logs to the host filesystem and providing configurable rotation, this feature ensures a reliable audit trail and improves the overall observability and troubleshooting experience for operators. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a design document for adding persistent logging to the sriov-network-config-daemon. This is a valuable enhancement for debuggability. The design is mostly well-thought-out, covering motivation, use cases, API changes, and testing. However, I've identified a few areas that need clarification and correction to ensure a robust implementation. My main concerns are around the handling of chroot, the logic for enabling logging by default, and the log level for file-based logging. Please see the detailed comments.
| if operatorConfig.Spec.LogConfig != nil && operatorConfig.Spec.LogConfig.Enabled { | ||
| snolog.InitLogWithFile( | ||
| filepath.Join("/host/var/log/sriov-network-config-daemon", "config-daemon.log"), | ||
| operatorConfig.Spec.LogConfig.MaxSizeMB, | ||
| operatorConfig.Spec.LogConfig.MaxFiles, | ||
| operatorConfig.Spec.LogConfig.MaxAgeDays, | ||
| operatorConfig.Spec.LogConfig.Compress, | ||
| ) | ||
| } |
There was a problem hiding this comment.
There's an inconsistency between the described default behavior and the proposed implementation logic for enabling logging.
The "Upgrade & Downgrade considerations" section on line 237 states: "On upgrade, if LogConfig is not set, persistent logging defaults to enabled with default values." This implies an opt-out model where logging is on by default.
However, the code snippet for runStartCmd shows an opt-in logic:
if operatorConfig.Spec.LogConfig != nil && operatorConfig.Spec.LogConfig.Enabled {
// ... initialize file logging
}This code has two issues:
- It will not enable logging if
LogConfigisnil, which contradicts the "enabled by default" requirement. - The condition
operatorConfig.Spec.LogConfig.Enabledis incorrect becauseEnabledis a*bool. It should be dereferenced, e.g.,*operatorConfig.Spec.LogConfig.Enabled, and thenilcase for the pointer must be handled.
The logic should be adjusted to correctly handle the pointer and reflect the intended "enabled by default" behavior. For example:
logCfg := operatorConfig.Spec.LogConfig
if logCfg == nil || logCfg.Enabled == nil || *logCfg.Enabled {
// ... initialize file logging with defaults if needed
}Please update the design to reflect the correct, consistent logic.
| // Create a zap core for file output | ||
| fileEncoder := zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()) | ||
| fileCore := zapcore.NewCore(fileEncoder, zapcore.AddSync(fileWriter), zapcore.DebugLevel) |
There was a problem hiding this comment.
The proposed InitLogWithFile function hardcodes the log level for the file core to zapcore.DebugLevel:
// Create a zap core for file output
fileEncoder := zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig())
fileCore := zapcore.NewCore(fileEncoder, zapcore.AddSync(fileWriter), zapcore.DebugLevel)This might not be the desired behavior. The file logs should likely adhere to the same log level as the console logs, which is configurable via the SriovOperatorConfig. The design should specify that the file logger will use the same dynamic log level as the console logger.
There was a problem hiding this comment.
Thanks for the design doc. This feature will be helpful for debugging.
I am missing any reference to systemd mode where the config daemon is running directly on the host, which means that writing to /host path will probably fail.
Currently in this mode, the logs are available in journactl but can be flushed.
The systemd service doesn't talk to the Kubernetes API. It reads config from /etc/sriov-operator/sriov-interface-config.yaml (the SriovConfig struct).
The LogConfig settings would need to be included in the SriovConfig/hosttypes.SriovConfig struct and written to disk by WriteConfFile(), or the systemd service needs hardcoded/flag-based log config.
|
+1 we need to also handle the systemd mode |
in systemd mode, journactl allows to display logs from previous boots, so this case is covered already by the system logs. |
SchSeba
left a comment
SchSeba
left a comment
There was a problem hiding this comment.
LGTM just a critical comment we must be sure
| - **Option B**: Write the log file to a path that resolves correctly in both contexts. Since `/host` maps to `/` on the host, the log path inside chroot would be `/var/log/sriov-network-config-daemon/config-daemon.log`, which is valid. We can open the file using this host-relative path before chroot, and lumberjack rotation will work because chroot sections are short-lived and the file is re-opened at the original path when chroot exits. However, this relies on timing assumptions and could fail if rotation occurs during chroot. | ||
| - **Option C**: Write to a tmpfs or emptyDir mount that is also bind-mounted to the host. However, this does not survive pod restarts. | ||
|
|
||
| **Recommended approach**: Option A — Use a dedicated logging mechanism that is isolated from chroot operations. This provides a robust solution that avoids race conditions entirely. The zap logger will buffer writes in memory, and the underlying file writer (lumberjack) will operate in the main namespace, never entering the chrooted environment. This eliminates the risk of path resolution failures during log rotation, regardless of when rotation occurs relative to chroot operations. |
There was a problem hiding this comment.
please check is this is working as I remember that for cni's in general we had to configured https://github.com/k8snetworkplumbingwg/sriov-cni/blob/master/cmd/sriov/main.go#L16
I am not sure if that is still the case with newer golang versions and the additional go routine will be isolated from the chroot command
3 participants
SRIOV persistent logging enhancement -
host-based log storage with rotation for the sriov-network-config-daemon