Feat: Add logging config option to helm#699
Merged
Merged
Conversation
umswmayj
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
mblos
as code owners
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds Renovate rules for PostgreSQL, bumps postgres image and chart versions, introduces slog-based logging with LOG_LEVEL handling, exposes Helm log settings, adjusts logging verbosity across scheduling/knowledge code, and adds a 60s recency to a Knowledge resource. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go (1)
265-270:⚠️ Potential issue | 🟠 MajorRemove the host before continuing on invalid CPU capacity.
This branch logs and
continues, but it never deleteshostfromresult.Activations. Because Line 258 already marked the host as encountered, it survives the filter and can still be returned even though its CPU capacity is invalid.Proposed fix
freeCPU, ok := free["cpu"] if !ok || freeCPU.Value() < 0 { traceLog.Warn( "host with invalid CPU capacity", "host", host, "freeCPU", freeCPU.String(), ) + delete(result.Activations, host) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go` around lines 265 - 270, In the branch inside filterHasEnoughCapacity that checks "if !ok || freeCPU.Value() < 0" (the one that calls traceLog.Warn for "host with invalid CPU capacity"), remove the host from result.Activations before the continue so the invalid host cannot survive the filter; locate the result.Activations entry for the current host and delete it using the appropriate removal operation for that collection (map delete or slice/remove method) right before the continue, keeping the existing log call intact.
🧹 Nitpick comments (1)
helm/library/cortex/templates/manager/manager.yaml (1)
65-75: Guardenv:emission so the block only renders when non-empty.Current structure always emits
env:. It’s safer and cleaner to render it only when either autoLOG_LEVELor user env entries exist.♻️ Suggested template refinement
- env: - {{- if and .Values.controllerManager.container.logLevel (not (and .Values.controllerManager.container.env (hasKey .Values.controllerManager.container.env "LOG_LEVEL"))) }} + {{- $injectLogLevel := and .Values.controllerManager.container.logLevel (not (and .Values.controllerManager.container.env (hasKey .Values.controllerManager.container.env "LOG_LEVEL"))) -}} + {{- if or $injectLogLevel .Values.controllerManager.container.env }} + env: + {{- if $injectLogLevel }} - name: LOG_LEVEL value: {{ .Values.controllerManager.container.logLevel | quote }} {{- end }} {{- if .Values.controllerManager.container.env }} {{- range $key, $value := .Values.controllerManager.container.env }} - name: {{ $key }} value: {{ $value }} {{- end }} {{- end }} + {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/library/cortex/templates/manager/manager.yaml` around lines 65 - 75, The template always emits the environment block; change it so `env:` only renders when there will be entries by wrapping the whole block in a conditional that checks for either the auto LOG_LEVEL case or user env entries (i.e., check .Values.controllerManager.container.logLevel (and that it's not overridden via .Values.controllerManager.container.env with key "LOG_LEVEL") OR that .Values.controllerManager.container.env exists and is non-empty). Update the template around the ENV block in manager.yaml (references: .Values.controllerManager.container.logLevel, .Values.controllerManager.container.env, hasKey) so the `env:` line and its children are omitted entirely when no env vars will be emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/renovate.json:
- Around line 65-75: The generic Renovate rule that matches /.*/? is currently
capturing updates for the postgres package and overriding the dedicated rule
with matchPackageNames "postgres" and groupName "postgres Dockerfile"; update
the generic rule (the rule that matches /.*/?) to exclude "postgres" (or add an
explicit exclude for matchPackageNames "postgres") so the dedicated postgres
rule (groupName "postgres Dockerfile") always applies for postgres updates.
In `@cmd/main.go`:
- Around line 160-170: The switch over strings.ToLower(os.Getenv("LOG_LEVEL"))
currently ignores invalid values; add a default branch that surfaces
misconfiguration by logging a warning or exiting (e.g., using
slog.Warn/slog.Error or log.Fatalf) and include the invalid value plus the list
of accepted values ("debug","info","warn","warning","error"); update the code
around slogLevel.Set calls so when the value is unrecognized you clearly notify
the operator (or return non-zero) instead of silently keeping the info level.
In `@cortex.secrets.example.yaml`:
- Around line 27-39: Update the example header text to match the actual
examples: replace the phrase "e.g. for cortex-nova" with wording that references
the shown sub-chart names (e.g. "for cortex-scheduling-controllers and
cortex-knowledge-controllers") so the sentence aligns with the example blocks
for cortex-scheduling-controllers and cortex-knowledge-controllers.
In `@internal/scheduling/nova/plugins/filters/filter_capabilities.go`:
- Around line 96-101: hvToNovaCapabilities returning an error currently causes
you to continue and never populate hvCaps[hv.Name], which later causes the host
to be dropped even for unrelated flavor fields; instead of skipping the host on
error, log the warning (traceLog.Warn) but still set a safe/default entry in
hvCaps for hv.Name (e.g., an empty/default novaCapabilities value or
placeholder) so the host remains in the map; keep using hvToNovaCapabilities but
when it errors assign a non-nil default to hvCaps[hv.Name] rather than
continuing.
In `@postgres/Dockerfile`:
- Line 71: The README line stating the current Postgres version is out of sync
with the Dockerfile's PG_VERSION; update the version string in
postgres/README.md to match PG_VERSION ("17.9-1.pgdg13+1") or at minimum the
same major.minor (v17.9) so the docs reflect the Dockerfile; search for the
README entry "Current postgres version" and replace the old v17.6 text
accordingly.
---
Outside diff comments:
In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go`:
- Around line 265-270: In the branch inside filterHasEnoughCapacity that checks
"if !ok || freeCPU.Value() < 0" (the one that calls traceLog.Warn for "host with
invalid CPU capacity"), remove the host from result.Activations before the
continue so the invalid host cannot survive the filter; locate the
result.Activations entry for the current host and delete it using the
appropriate removal operation for that collection (map delete or slice/remove
method) right before the continue, keeping the existing log call intact.
---
Nitpick comments:
In `@helm/library/cortex/templates/manager/manager.yaml`:
- Around line 65-75: The template always emits the environment block; change it
so `env:` only renders when there will be entries by wrapping the whole block in
a conditional that checks for either the auto LOG_LEVEL case or user env entries
(i.e., check .Values.controllerManager.container.logLevel (and that it's not
overridden via .Values.controllerManager.container.env with key "LOG_LEVEL") OR
that .Values.controllerManager.container.env exists and is non-empty). Update
the template around the ENV block in manager.yaml (references:
.Values.controllerManager.container.logLevel,
.Values.controllerManager.container.env, hasKey) so the `env:` line and its
children are omitted entirely when no env vars will be emitted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 488a8007-cca5-423d-9db4-a2d372e9e23a
📒 Files selected for processing (20)
.github/renovate.jsoncmd/main.gocortex.secrets.example.yamlhelm/bundles/cortex-cinder/Chart.yamlhelm/bundles/cortex-manila/Chart.yamlhelm/bundles/cortex-nova/Chart.yamlhelm/bundles/cortex-nova/alerts/nova.alerts.yamlhelm/bundles/cortex-nova/templates/knowledges_kvm.yamlhelm/library/cortex-postgres/Chart.yamlhelm/library/cortex/templates/_helpers.tplhelm/library/cortex/templates/manager/manager.yamlhelm/library/cortex/values.yamlinternal/knowledge/extractor/controller.gointernal/scheduling/lib/filter_weigher_pipeline.gointernal/scheduling/nova/external_scheduler_api.gointernal/scheduling/nova/hypervisor_overcommit_controller.gointernal/scheduling/nova/plugins/filters/filter_capabilities.gointernal/scheduling/nova/plugins/filters/filter_external_customer.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity.gopostgres/Dockerfile
SoWieMarkus
approved these changes
Apr 13, 2026
Collaborator
SoWieMarkus
left a comment
SoWieMarkus
left a comment
There was a problem hiding this comment.
See coderabbit comments
# Conflicts: # cmd/manager/main.go # internal/scheduling/nova/plugins/filters/filter_capabilities.go # postgres/Dockerfile
Contributor
Test Coverage ReportTest Coverage 📊: 69.1% |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.