WINC-1635, WINC-1592: enable log rotation for kubelet and kubeproxy services#3766
WINC-1635, WINC-1592: enable log rotation for kubelet and kubeproxy services#3766jrvaldes wants to merge 6 commits intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
approved
|
/retitle WINC-1635: enable log rotation for kubelet service |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jrvaldes: This pull request references WINC-1635 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. |
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
336fc21 to
ad7f5c8
Compare
|
/test ? |
|
/test lint |
|
/test unit |
|
/test images |
|
/test vsphere-e2e-operator |
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
4469104 to
f6a626f
Compare
|
/test vsphere-e2e-operator |
1 similar comment
|
/test vsphere-e2e-operator |
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
06d4c31 to
ca06837
Compare
|
/test vsphere-e2e-operator |
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
ca06837 to
f994aee
Compare
|
/test vsphere-e2e-operator |
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
f994aee to
547445b
Compare
|
/test vsphere-e2e-operator |
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
547445b to
abd7af6
Compare
|
/test vsphere-e2e-operator |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds kubelet Logging configuration (FlushFrequency 5s), promotes k8s.io/component-base to a direct go.mod require, introduces environment-driven log rotation for managed Windows services with helpers, tests and package init-time env parsing, and adds enable_debug_logging() in hack/common.sh to toggle WMCO debug logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
abd7af6 to
5abf765
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/services/services_test.go`:
- Around line 158-189: The test TestGetLogRunnerForCmd relies on package-level
defaults (logFileSize, logFileAge, flushInterval) which are initialized from
environment variables; make the test deterministic by setting those env vars at
test start (use t.Setenv for SERVICES_LOG_FILE_SIZE, SERVICES_LOG_FILE_AGE,
SERVICES_LOG_FLUSH_INTERVAL) to the expected default values ("100M", "168h",
"5s") before calling getLogRunnerForCmd so the output always matches the
hardcoded expected strings.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/services_test.go`: - Around line 158-189: The test TestGetLogRunnerForCmd relies on package-level defaults (logFileSize, logFileAge, flushInterval) which are initialized from environment variables; make the test deterministic by setting those env vars at test start (use t.Setenv for SERVICES_LOG_FILE_SIZE, SERVICES_LOG_FILE_AGE, SERVICES_LOG_FLUSH_INTERVAL) to the expected default values ("100M", "168h", "5s") before calling getLogRunnerForCmd so the output always matches the hardcoded expected strings.pkg/services/services_test.go (1)
158-189: Test relies on package-level defaults initialized at startup.The test hardcodes expected values (
100M,168h,5s) that match the default constants. However, sincelogFileSize,logFileAge, andflushIntervalare set ininit()from environment variables, if any of these env vars (SERVICES_LOG_FILE_SIZE,SERVICES_LOG_FILE_AGE,SERVICES_LOG_FLUSH_INTERVAL) are set in the test environment, this test will fail unexpectedly.Consider either:
- Explicitly setting the env vars in this test using
t.Setenvto ensure predictable behavior, or- Documenting that this test assumes default values
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/services_test.go` around lines 158 - 189, The test TestGetLogRunnerForCmd relies on package-level defaults (logFileSize, logFileAge, flushInterval) which are initialized from environment variables; make the test deterministic by setting those env vars at test start (use t.Setenv for SERVICES_LOG_FILE_SIZE, SERVICES_LOG_FILE_AGE, SERVICES_LOG_FLUSH_INTERVAL) to the expected default values ("100M", "168h", "5s") before calling getLogRunnerForCmd so the output always matches the hardcoded expected strings.
|
/test vsphere-e2e-operator |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/services/init.go`:
- Around line 12-24: The log.Error calls are using printf-style "%s" which won’t
be interpolated by controller-runtime's logr; update each call (the three uses
of log.Error in this snippet) to pass the variable name as a structured
key-value pair instead of a format string — e.g. replace log.Error(err, "cannot
load environment variable %s", logFileSizeEnvVar) with something like
log.Error(err, "cannot load environment variable", "env", logFileSizeEnvVar); do
the same for the calls around getEnvDuration and the variables logFileAgeEnvVar
and logFlushIntervalEnvVar so the variable names are logged as keys rather than
a literal format string.
In `@README.md`:
- Around line 271-278: The "Not yet supported" list contains a duplicated entry
"csi-proxy"; edit the README's list (the "Not yet supported" section) to remove
the duplicate so "csi-proxy" appears only once, ensuring the other entries
(containerd, windows_exporter, hybrid-overlay-node, azure-cloud-node-manager)
remain unchanged and the list ordering/format stays consistent.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/services/services_test.go (1)
486-494: Consider usingstrings.Indexfrom the standard library.This helper reimplements
strings.Index. The stdlib version would be more idiomatic.-// indexOf returns the index of the first occurrence of substr in s, or -1 if not found. -func indexOf(s, substr string) int { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return i - } - } - return -1 -} +import "strings" +// Then use strings.Index(s, substr) directly in the test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/services_test.go` around lines 486 - 494, The custom indexOf function duplicates stdlib behavior; replace its usage with strings.Index by removing or deprecating indexOf and updating callers to call strings.Index(s, substr) instead (and add an import "strings" where needed); alternatively, if you must keep a wrapper, implement indexOf to return strings.Index(s, substr) and add the strings import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/services/init.go`:
- Around line 11-24: The log.Error calls in init.go are using printf-style "%s"
which won't be interpolated by controller-runtime's logr; update each failing
call (the three calls that follow getEnvQuantity and getEnvDuration) to use
structured key/value args instead of "%s" so the error and the environment
variable name are recorded (e.g., call log.Error(err, "cannot load environment
variable", "env", logFileSizeEnvVar) and similarly for logFileAgeEnvVar and
logFlushIntervalEnvVar), keeping the same error variable and message context.
In `@README.md`:
- Around line 271-276: In the "Not yet supported" list titled "Not yet
supported" remove the duplicate csi-proxy entry so it appears only once; locate
the bullet list containing containerd, csi-proxy, windows_exporter,
hybrid-overlay-node and ensure only a single csi-proxy line remains (delete the
repeated csi-proxy bullet).
---
Nitpick comments:
In `@pkg/services/services_test.go`:
- Around line 486-494: The custom indexOf function duplicates stdlib behavior;
replace its usage with strings.Index by removing or deprecating indexOf and
updating callers to call strings.Index(s, substr) instead (and add an import
"strings" where needed); alternatively, if you must keep a wrapper, implement
indexOf to return strings.Index(s, substr) and add the strings import.
|
@jrvaldes: all tests passed! 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. |
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
b469a17 to
646672d
Compare
|
vsphere-e2e-operator passed, and you can see the log rotation configuration applied in the name: windows-services-10.22.0-b9b2652-dirty: |
mansikulkarni96
left a comment
mansikulkarni96
left a comment
There was a problem hiding this comment.
Mostly looks good, some questions.
|
|
||
| #### Patching the subscription using the CLI: | ||
| ```shell script | ||
| oc patch subscription <subscription_name> -n <namespace_name> \ |
There was a problem hiding this comment.
I assume this will need an operator restart?
There was a problem hiding this comment.
100%, a subs change with spin a new pod. I will add a line make it explicit
There was a problem hiding this comment.
|
|
||
| #### Patching the operator deployment using the CLI: | ||
| ```shell script | ||
| oc set env deployment/windows-machine-config-operator -n <namespace_name> -c manager \ |
There was a problem hiding this comment.
I may be missing this change, but have we removed setting default values for these env variables now?
There was a problem hiding this comment.
correct, that was the agreement, the feature is disabled by default. So no default values.
|
|
||
| For details on the log rotation naming convention, please refer to the [kube-log-runner documentation](https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/component-base/logs/kube-log-runner) | ||
|
|
||
| The log rotation functionality is disabled by default, meaning: |
this commit configures kubelet service to use kube-log-runner wrapper to enable optional log rotation on Windows nodes. This prevents unbounded log growth that could exhaust disk space. Log rotation parameters are configurable via environment variables: - SERVICES_LOG_FILE_SIZE: Size limit before rotation - SERVICES_LOG_FILE_AGE: Retention period for rotated logs - SERVICES_LOG_FLUSH_INTERVAL: Flush interval to disk This approach is necessary because Windows services don't have a native mechanism for output redirection, and Kubernetes has deprecated the --log-file flag for components.
ran: go mod tidy && go mod vendor
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
646672d to
8533948
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
README.md (2)
249-316: Consider adding guidance for disabling log rotation and handling configuration changes.The documentation clearly explains how to enable and configure log rotation, but could be enhanced with:
- Instructions for disabling log rotation (reverting to defaults)
- Guidance on what happens to existing logs when changing rotation settings
- Expected operator/node behavior during configuration changes
These additions would provide a more complete operational picture for cluster administrators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 249 - 316, The README adds only enable instructions for log rotation but lacks disable guidance and behavior notes; update the "Enabling log rotation for managed Windows services" section to include (1) how to disable rotation by removing or clearing the SERVICES_LOG_FILE_SIZE / SERVICES_LOG_FILE_AGE / SERVICES_LOG_FLUSH_INTERVAL env vars (or setting them to empty/0 where applicable) for both Subscription and operator Deployment examples, (2) a short paragraph describing what happens to existing log files when rotation settings change (existing rotated files are retained per previous SERVICES_LOG_FILE_AGE until expiry; new rotation rules apply going forward), and (3) a brief note on operator/node behavior during config changes (operator restart when env changes, node-by-node reconfiguration to apply new settings and no service downtime guarantee), referencing the env var names and the operator deployment/subscription examples already present so readers can find the locations to change.
314-316: Consider improving the formatting of parameter explanations.The "where:" section could be more readable using a markdown table or consistent formatting. For example:
**Parameters:** - `<namespace_name>`: The namespace where the operator is installed (e.g., `openshift-windows-machine-config-operator`) - `<subscription_name>`: The name of the subscription used to install the operator (e.g., `windows-machine-config-operator-subscription`)This improves scannability and consistency with other documentation sections.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 314 - 316, Update the "where:" parameter explanations to use consistent, more readable formatting: replace the current plain lines for <namespace_name> and <subscription_name> with either a Markdown bullet list or a small table, and include inline examples in backticks and parentheses (e.g., `<namespace_name>`: The namespace where the operator is installed (e.g., `openshift-windows-machine-config-operator`) and `<subscription_name>`: The name of the subscription used to install the operator (e.g., `windows-machine-config-operator-subscription`); ensure the same style is applied to both parameter lines for consistency with other docs sections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Around line 249-316: The README adds only enable instructions for log rotation
but lacks disable guidance and behavior notes; update the "Enabling log rotation
for managed Windows services" section to include (1) how to disable rotation by
removing or clearing the SERVICES_LOG_FILE_SIZE / SERVICES_LOG_FILE_AGE /
SERVICES_LOG_FLUSH_INTERVAL env vars (or setting them to empty/0 where
applicable) for both Subscription and operator Deployment examples, (2) a short
paragraph describing what happens to existing log files when rotation settings
change (existing rotated files are retained per previous SERVICES_LOG_FILE_AGE
until expiry; new rotation rules apply going forward), and (3) a brief note on
operator/node behavior during config changes (operator restart when env changes,
node-by-node reconfiguration to apply new settings and no service downtime
guarantee), referencing the env var names and the operator
deployment/subscription examples already present so readers can find the
locations to change.
- Around line 314-316: Update the "where:" parameter explanations to use
consistent, more readable formatting: replace the current plain lines for
<namespace_name> and <subscription_name> with either a Markdown bullet list or a
small table, and include inline examples in backticks and parentheses (e.g.,
`<namespace_name>`: The namespace where the operator is installed (e.g.,
`openshift-windows-machine-config-operator`) and `<subscription_name>`: The name
of the subscription used to install the operator (e.g.,
`windows-machine-config-operator-subscription`); ensure the same style is
applied to both parameter lines for consistency with other docs sections.
this commit documents the automatic log rotation for managed Windows services in the Enabled features section in the README.md
jrvaldes
force-pushed
the
log-rotation-kubelet
branch
from
8533948 to
98ddd01
Compare
|
/remove-approve |
openshift-ci
bot
removed
the
approved
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mansikulkarni96 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
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/common.sh (1)
393-406:⚠️ Potential issue | 🟡 MinorAvoid overwriting existing ARGS/env entries when enabling debug logging.
The OLMv0 merge patch replaces the env array, and the OLMv1
oc set envcall overwritesARGS, which can drop existing flags or env-based config. Consider merging instead of replacing, and append--debugLoggingonly if missing.🔧 Example adjustment for OLMv1 ARGS preservation
- # Add debug env variable and log file limit to the WMCO manager container - oc set env deployment/windows-machine-config-operator -n "$WMCO_DEPLOY_NAMESPACE" \ - ARGS="--debugLogging" \ - SERVICES_LOG_FILE_SIZE="1M" \ - -c manager + # Add debug env variable and log file limit to the WMCO manager container + current_args=$(oc get deployment/windows-machine-config-operator -n "$WMCO_DEPLOY_NAMESPACE" \ + -o jsonpath='{.spec.template.spec.containers[?(@.name=="manager")].env[?(@.name=="ARGS")].value}') + if [[ -z "$current_args" ]]; then + current_args="--debugLogging" + elif [[ "$current_args" != *"--debugLogging"* ]]; then + current_args="$current_args --debugLogging" + fi + oc set env deployment/windows-machine-config-operator -n "$WMCO_DEPLOY_NAMESPACE" \ + ARGS="$current_args" \ + SERVICES_LOG_FILE_SIZE="1M" \ + -c manager🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/common.sh` around lines 393 - 406, The current OLMv0 oc patch (using '{"spec":{"config":{"env":[...]}}}') and the OLMv1 oc set env call overwrite the existing env/ARGS and can drop flags; update the logic so we merge/update environment entries instead of replacing them: for the OLMv0 path, patch the subscription/deployment config by adding/updating only the ARGS and SERVICES_LOG_FILE_SIZE entries (preserve existing env array and other entries) and for the OLMv1 path read the current ARGS value from deployment/windows-machine-config-operator (container "manager"), append "--debugLogging" only if it is not already present, and set/patch SERVICES_LOG_FILE_SIZE without clobbering other env vars. Ensure you reference WMCO_SUB, ARGS, SERVICES_LOG_FILE_SIZE, oc patch and oc set env/deployment/windows-machine-config-operator when implementing the merge/update logic.
🧹 Nitpick comments (1)
README.md (1)
329-333: Minor wording tweak for concision.Consider replacing “in order to” with “to.”
📝 Suggested edit
- will cause the operator to restart in order to load the updated configuration. + will cause the operator to restart to load the updated configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 329 - 333, The sentence describing operator restart uses the phrase "in order to" which is wordy; edit the sentence that mentions SERVICES_LOG_FILE_SIZE, SERVICES_LOG_FILE_AGE, and SERVICES_LOG_FLUSH_INTERVAL so it reads "...will cause the operator to restart to load the updated configuration." — replace "in order to" with "to" in that sentence to improve concision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@hack/common.sh`:
- Around line 393-406: The current OLMv0 oc patch (using
'{"spec":{"config":{"env":[...]}}}') and the OLMv1 oc set env call overwrite the
existing env/ARGS and can drop flags; update the logic so we merge/update
environment entries instead of replacing them: for the OLMv0 path, patch the
subscription/deployment config by adding/updating only the ARGS and
SERVICES_LOG_FILE_SIZE entries (preserve existing env array and other entries)
and for the OLMv1 path read the current ARGS value from
deployment/windows-machine-config-operator (container "manager"), append
"--debugLogging" only if it is not already present, and set/patch
SERVICES_LOG_FILE_SIZE without clobbering other env vars. Ensure you reference
WMCO_SUB, ARGS, SERVICES_LOG_FILE_SIZE, oc patch and oc set
env/deployment/windows-machine-config-operator when implementing the
merge/update logic.
---
Nitpick comments:
In `@README.md`:
- Around line 329-333: The sentence describing operator restart uses the phrase
"in order to" which is wordy; edit the sentence that mentions
SERVICES_LOG_FILE_SIZE, SERVICES_LOG_FILE_AGE, and SERVICES_LOG_FLUSH_INTERVAL
so it reads "...will cause the operator to restart to load the updated
configuration." — replace "in order to" with "to" in that sentence to improve
concision.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
README.mdgo.modhack/common.shpkg/nodeconfig/nodeconfig.gopkg/nodeconfig/nodeconfig_test.gopkg/services/init.gopkg/services/services.gopkg/services/services_test.go
sebsoto
left a comment
sebsoto
left a comment
There was a problem hiding this comment.
Commits seem to be out of order, the commit that implements the env var parsing should be first shouldn't it?
| - no rotation, log file grows indefinitely | ||
| - no cleanup, old rotated files are never deleted | ||
| - no periodic flushing, relies on OS buffer flushing |
There was a problem hiding this comment.
This is overly verbose, essentially repeating the same thing over again. I recommend trimming to this:
The log rotation functionality is disabled by default, causing log files to grow indefinitely
| To enable and customize the log rotation behavior, add the following environment variables to the subscription | ||
| (OLMv0) or operator deployment (if using OLMv1 or installing manually). The operator will restart to load the newly | ||
| added environment variables and apply log rotation to the managed services. This will result in a reconfiguration of | ||
| the existing Windows nodes, one at a time, until all nodes have been handled, to minimize disruption. |
There was a problem hiding this comment.
While OLMv1 is in tech preview I recommend not mentioning it, or adding it as a separate section
| > **Warning:** The defaults **are not** recommended for production installations, logs will grow indefinitely and | ||
| eventually fill the disk. Unless you have a specific reason to disable log rotation, or have an alternative | ||
| log management solution in place, consider enabling log rotation as described below. |
There was a problem hiding this comment.
If this statement is believed to be true, then we should not be shipping it with defaulted off.
I recommend creating a new story for 10.23.0 (assuming this feature is in 10.22.0) to default to on.
| ### Disabling log rotation for managed Windows services | ||
|
|
||
| To disable log rotation, remove the `SERVICES_LOG_FILE_SIZE`, `SERVICES_LOG_FILE_AGE`, and `SERVICES_LOG_FLUSH_INTERVAL` | ||
| environment variables from the subscription or operator deployment. | ||
|
|
||
| ### Behavior when log rotation settings change | ||
|
|
||
| **Effect on existing log files:** When rotation settings are changed (enabled, disabled, or updated), any previously | ||
| rotated log files are retained according to the `SERVICES_LOG_FILE_AGE` value that was in effect when they were | ||
| created. Once that retention period expires, the files are cleaned up automatically. New log files and any future | ||
| rotated files will follow the updated rotation rules going forward. | ||
|
|
||
| **Operator and node behavior:** Any change to the `SERVICES_LOG_FILE_SIZE`, `SERVICES_LOG_FILE_AGE`, or | ||
| `SERVICES_LOG_FLUSH_INTERVAL` environment variables—whether in the subscription (OLMv0) or the operator deployment | ||
| (OLMv1 / manual installs)—will cause the operator to restart in order to load the updated configuration. After | ||
| restarting, the operator will reconfigure each Windows node one at a time to apply the new log rotation settings, | ||
| minimizing disruption. Note that service continuity during reconfiguration is not guaranteed; brief interruptions | ||
| to managed services (such as kubelet or kube-proxy) may occur on each node as it is reconfigured. |
There was a problem hiding this comment.
This is a lot of documentation for log rotation.
Consider moving to a separate file and linking from the README.
| Logging: logsapi.LoggingConfiguration{ | ||
| FlushFrequency: logsapi.TimeOrMetaDuration{ | ||
| Duration: meta.Duration{Duration: 5 * time.Second}, | ||
| SerializeAsString: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This is explicitly setting a default, which is fine, but the commit message is misleading as there is no functional change here.
| // not present | ||
| return "", nil | ||
| } | ||
| if strings.HasPrefix(value, "-") { |
There was a problem hiding this comment.
Don't parse the string, if you are already parsing this as a quantity.
https://pkg.go.dev/k8s.io/apimachinery/pkg/api/resource#Quantity.Sign
|
|
||
| // getEnvQuantity returns the value of the environment variable with the given key | ||
| // if it represents a valid and non-negative quantity, otherwise returns error | ||
| func getEnvQuantity(key string) (string, error) { |
There was a problem hiding this comment.
getEnvQuantity returns a string, not a quantity?
Also why does getEnvQuantity care if it is "non-negative". The caller should check that shouldn't they?
|
|
||
| // getEnvDuration returns the value of the environment variable with the given key | ||
| // if it represents a valid and non-negative duration, otherwise returns error | ||
| func getEnvDuration(key string) (string, error) { |
There was a problem hiding this comment.
Same as this 7722cda#r2850043734
But time.Duration rather than Quantity
| } | ||
| } | ||
|
|
||
| func TestGetEnvDuration(t *testing.T) { |
There was a problem hiding this comment.
If this much testing is required for parsing environment variables its a good sign something might need to be tweaked 😉
| } | ||
|
|
||
| // indexOf returns the index of the first occurrence of substr in s, or -1 if not found. | ||
| func indexOf(s, substr string) int { |
There was a problem hiding this comment.
5 participants
This pull request introduces configurable log rotation and flushing for Windows kubelet services, allowing log file size, retention, and flush interval to be set via environment variables. It also adds helper functions to safely parse and use these environment variables, and includes unit tests to ensure their correctness.
Log rotation and flush configuration:
SERVICES_LOG_FILE_SIZE), log file age (SERVICES_LOG_FILE_AGE), and log flush interval (SERVICES_LOG_FLUSH_INTERVAL), with sensible defaults, to control log runner behavior inpkg/services/services.go. [1] [2]getLogRunnerForCmd, which incorporates the log rotation and flush parameters. [1] [2]Loggingfield, using the newlogsapi.LoggingConfigurationstruct. [1] [2]Helper functions and environment parsing:
getEnvQuantityOrDefaultandgetEnvDurationOrDefaultfunctions to safely parse quantity and duration environment variables, falling back to defaults if invalid. [1] [2]Testing:
pkg/services/services_test.go, covering various scenarios and edge cases.Summary by CodeRabbit
New Features
Updates
Tests
Documentation
Chores