WINC-1859: Implement log file rotation for WICD#4177
Conversation
this commit introduces a new package to load the configuration for log files from environment and generalizes the previous implementation.
this commit introduces the logFileMaxSizeMb param in the WICD controller to allow configuration for log file rotation
openshift-ci-robot
added
the
jira/valid-reference
|
@jrvaldes: This pull request references WINC-1859 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis PR consolidates log file configuration management across the Windows Machine Config Operator by extracting a duplicated initialization pattern from 🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jrvaldes The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/logconfig/logconfig_test.go (1)
282-421: 💤 Low valueSolid coverage — minor: test name drifted from the function under test.
TestGetLogRunnerForCmdexercisesGenerateKubeLogRunnerCmd(the oldgetLogRunnerForCmdis gone). Renaming toTestGenerateKubeLogRunnerCmdkeepsgo test -runand grep aligned with the actual symbol. The ordering/contains assertions themselves are well done.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/logconfig/logconfig_test.go` around lines 282 - 421, The test function name no longer matches the symbol under test; rename the test from TestGetLogRunnerForCmd to TestGenerateKubeLogRunnerCmd so it reflects and maps to GenerateKubeLogRunnerCmd; update the function declaration only (leave all test body assertions and references to GenerateKubeLogRunnerCmd unchanged) to keep go test filtering and grep consistent.pkg/logconfig/logconfig.go (1)
25-40: 💤 Low valueHarden the overflow/rounding guard for
ScaledValueinGetLogFileSizeMegabytes.
resource.Quantity.ScaledValuedrops the internalexact/oksignal from the scaling path; as a result,if mb < 0can miss cases where the scaled conversion doesn’t cleanly fit into anint64(and thenuint64(mb)would proceed). Prefer usingmb, ok := q.AsScaledInt64(resource.Mega)(orq.Value()with explicit bounds checks) and return0whenokis false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/logconfig/logconfig.go` around lines 25 - 40, GetLogFileSizeMegabytes currently uses q.ScaledValue(resource.Mega) which can drop the exact/ok signal and let an out-of-range value slip through; update GetLogFileSizeMegabytes to use q.AsScaledInt64(resource.Mega) (or q.Value() with explicit bounds checks) so you get (mb, ok) and return 0 when ok is false or mb is negative, then safely convert mb to uint64; reference the GetLogFileSizeMegabytes function and the resource.Quantity methods AsScaledInt64 / Value when making this change.pkg/services/services.go (1)
314-351: ⚡ Quick winRemove dead env parsing helpers from pkg/services/services.go
pkg/logconfignow defines and uses its owngetEnvQuantityString/getEnvDurationString(pkg/logconfig/init.go), while the versions inpkg/services/services.goappear to have no remaining call sites (only the function definitions). Drop the dead helpers frompkg/services/services.goor reuse thepkg/logconfigimplementations to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/services.go` around lines 314 - 351, The two dead helper functions getEnvQuantityString and getEnvDurationString in services.go are duplicated and unused; remove these definitions from services.go, and if any remaining code later needs them, replace references to those functions with the implementations in pkg/logconfig (importing that package and calling logconfig.GetEnvQuantityString/GetEnvDurationString or the exact exported names used there); otherwise simply delete the getEnvQuantityString and getEnvDurationString functions to eliminate the dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/daemon/controller.go`:
- Around line 76-82: fs.Set("log_file_max_size", logFileMaxSizeString) currently
ignores the returned error; update the controller initialization (around the
block that sets log_file_max_size in controller.go) to capture the error from
fs.Set and handle it (e.g., log via klog.Errorf/klog.Fatalf or return the error
from the enclosing function) so a typo or invalid value doesn't silently leave
logging misconfigured; preserve the existing special-case message that converts
"0" to "unlimited" when composing the klog.Infof message.
In `@pkg/logconfig/init.go`:
- Around line 7-25: The init() currently swallows env parse errors (overwriting
logFileSize/logFileAge/flushInterval with empty strings), which can silently
disable log rotation and prevents GenerateKubeLogRunnerCmd from emitting flags;
instead move the parsing out of package init into a validation function (e.g.,
ValidateLogConfig) that uses getEnvQuantityString/getEnvDurationString to set
the globals and returns an error on parse failure, then call ValidateLogConfig
after ctrl.SetLogger in cmd/operator/main.go so failures can be logged by the
real logger (or cause process exit); ensure you do not overwrite the globals on
error—return/exit with a non-zero status or log.Fatalf so invalid env values are
surfaced and rotation flags are not silently omitted.
---
Nitpick comments:
In `@pkg/logconfig/logconfig_test.go`:
- Around line 282-421: The test function name no longer matches the symbol under
test; rename the test from TestGetLogRunnerForCmd to
TestGenerateKubeLogRunnerCmd so it reflects and maps to
GenerateKubeLogRunnerCmd; update the function declaration only (leave all test
body assertions and references to GenerateKubeLogRunnerCmd unchanged) to keep go
test filtering and grep consistent.
In `@pkg/logconfig/logconfig.go`:
- Around line 25-40: GetLogFileSizeMegabytes currently uses
q.ScaledValue(resource.Mega) which can drop the exact/ok signal and let an
out-of-range value slip through; update GetLogFileSizeMegabytes to use
q.AsScaledInt64(resource.Mega) (or q.Value() with explicit bounds checks) so you
get (mb, ok) and return 0 when ok is false or mb is negative, then safely
convert mb to uint64; reference the GetLogFileSizeMegabytes function and the
resource.Quantity methods AsScaledInt64 / Value when making this change.
In `@pkg/services/services.go`:
- Around line 314-351: The two dead helper functions getEnvQuantityString and
getEnvDurationString in services.go are duplicated and unused; remove these
definitions from services.go, and if any remaining code later needs them,
replace references to those functions with the implementations in pkg/logconfig
(importing that package and calling
logconfig.GetEnvQuantityString/GetEnvDurationString or the exact exported names
used there); otherwise simply delete the getEnvQuantityString and
getEnvDurationString functions to eliminate the dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bb45bcec-f745-439d-b25f-c93f7ec9a90e
📒 Files selected for processing (8)
cmd/daemon/controller.gopkg/logconfig/init.gopkg/logconfig/logconfig.gopkg/logconfig/logconfig_test.gopkg/services/init.gopkg/services/services.gopkg/services/services_test.gopkg/windows/windows.go
💤 Files with no reviewable changes (2)
- pkg/services/init.go
- pkg/services/services_test.go
|
/test ? |
|
/test lint |
|
/test unit |
|
/test vsphere-e2e-operator |
|
/test lint |
|
/test vsphere-e2e-operator |
|
@jrvaldes: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| // kubeProxyConfiguration returns the Service definition for kube-proxy | ||
| func kubeProxyConfiguration(debug bool) servicescm.Service { | ||
| cmd := getLogRunnerForCmd(windows.KubeProxyPath, windows.KubeProxyLog) | ||
| cmd := logconfig.GenerateKubeLogRunnerCmd(windows.KubeLogRunnerPath, windows.KubeProxyPath, windows.KubeProxyLog) |
There was a problem hiding this comment.
since this is generating and returning the logrunnercmd, it can be GetKubeLogRunnerCmd?
| fs.Set("log_file_max_size", logFileMaxSizeString) | ||
|
|
||
| if logFileMaxSizeString == "0" { | ||
| logFileMaxSizeString = "unlimited" |
There was a problem hiding this comment.
shouldn't this be default unlimited and then if set to a value other than 0, then set to that value?
| func GetLogFileSize() string { | ||
| return logFileSize | ||
| // GetLogFileSizeMegabytes returns the configured log file size resource value in megabytes | ||
| func GetLogFileSizeMegabytes() uint64 { |
There was a problem hiding this comment.
We can still keep this as GetLogFileSize if there isn't going to be another function that will need to return size not in megabytes. Function doc can state that the value will be in megabytes.
3 participants
This pull request introduces a new CLI flag for maximum log file size and ensures log file size is consistently set for WICD service configuration, also refactors and centralizes the log configuration logic moving environment variable parsing and log runner command construction into a new dedicated
logconfigpackage.Summary by CodeRabbit
New Features
--log-file-max-sizeflag to controller daemon.Refactor
Tests