Open
Conversation
umswmayj
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
mblos
as code owners
📝 WalkthroughWalkthroughThis PR updates PostgreSQL Docker image configuration with Renovate automation, implements structured logging (slog) in the main application, adds logging configuration to Helm values and templates, bumps the cortex-postgres Helm chart version across bundles, downgrades logging verbosity across scheduling and knowledge modules, and adds a Knowledge resource recency window. 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 docstrings
🧪 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 |
Contributor
Test Coverage ReportTest Coverage 📊: 68.8% |
![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
Comment on lines
+65
to
+75
| { | ||
| "matchPackageNames": [ | ||
| "postgres" | ||
| ], | ||
| "matchFileNames": [ | ||
| "postgres/Dockerfile" | ||
| ], | ||
| "allowedVersions": "17.x", | ||
| "automerge": true, | ||
| "groupName": "postgres Dockerfile" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Renovate, when multiple packageRules match the same dependency, do later rules override scalar fields like groupName from earlier rules?
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
cd .github && cat -n renovate.json | head -90Repository: cobaltcore-dev/cortex
Length of output: 3176
🏁 Script executed:
cd .github && cat -n renovate.json | sed -n '84,110p'Repository: cobaltcore-dev/cortex
Length of output: 694
🌐 Web query:
Renovate packageRules precedence multiple matching rules override groupName
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
# Search for any Renovate documentation or config examples in the repo that might indicate rule behavior
find . -type f \( -name "*.md" -o -name "*.json" \) -exec grep -l "packageRules" {} \; 2>/dev/null | head -5Repository: cobaltcore-dev/cortex
Length of output: 88
Rule at line 84 may override the postgres groupName for minor and patch updates.
The rule at lines 84-94 matches postgres for minor/patch updates (since postgres is not in the sapcc namespace and matches /.*/?), which would override the groupName from lines 65-75 for those update types.
Suggested fix
{
"matchPackageNames": [
"!/^github\\.com\\/sapcc\\/.*/",
+ "!postgres",
"/.*/"
],
"matchUpdateTypes": [
"minor",
"patch"Exclude postgres from this generic rule to ensure its dedicated grouping is always applied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/renovate.json around lines 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.
Comment on lines
+160
to
+170
| if lvl := os.Getenv("LOG_LEVEL"); lvl != "" { | ||
| switch strings.ToLower(lvl) { | ||
| case "debug": | ||
| slogLevel.Set(slog.LevelDebug) | ||
| case "info": | ||
| slogLevel.Set(slog.LevelInfo) | ||
| case "warn", "warning": | ||
| slogLevel.Set(slog.LevelWarn) | ||
| case "error": | ||
| slogLevel.Set(slog.LevelError) | ||
| } |
There was a problem hiding this comment.
Surface invalid LOG_LEVEL values instead of silently defaulting to info.
A typo here is ignored without any signal, so operators can think they changed verbosity while the process still runs at info. A default branch that warns or exits would make misconfiguration obvious.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/main.go` around lines 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.
Comment on lines
+27
to
+39
| # These apply per sub-chart, e.g. for cortex-nova: | ||
| # | ||
| # cortex-scheduling-controllers: | ||
| # controllerManager: | ||
| # container: | ||
| # logLevel: "debug" | ||
| # zapDevel: true | ||
| # | ||
| # cortex-knowledge-controllers: | ||
| # controllerManager: | ||
| # container: | ||
| # logLevel: "debug" | ||
| # zapDevel: true |
There was a problem hiding this comment.
Fix the example label mismatch in logging docs.
Line 27 says “e.g. for cortex-nova”, but the examples shown are cortex-scheduling-controllers and cortex-knowledge-controllers. Please align the sentence with the actual examples to avoid confusion.
✏️ Suggested wording tweak
-# These apply per sub-chart, e.g. for cortex-nova:
+# These apply per sub-chart, for example:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # These apply per sub-chart, e.g. for cortex-nova: | |
| # | |
| # cortex-scheduling-controllers: | |
| # controllerManager: | |
| # container: | |
| # logLevel: "debug" | |
| # zapDevel: true | |
| # | |
| # cortex-knowledge-controllers: | |
| # controllerManager: | |
| # container: | |
| # logLevel: "debug" | |
| # zapDevel: true | |
| # These apply per sub-chart, for example: | |
| # | |
| # cortex-scheduling-controllers: | |
| # controllerManager: | |
| # container: | |
| # logLevel: "debug" | |
| # zapDevel: true | |
| # | |
| # cortex-knowledge-controllers: | |
| # controllerManager: | |
| # container: | |
| # logLevel: "debug" | |
| # zapDevel: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cortex.secrets.example.yaml` around lines 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.
Comment on lines
+96
to
+101
| caps, err := hvToNovaCapabilities(hv) | ||
| if err != nil { | ||
| traceLog.Warn("skipping hypervisor with unknown capabilities", "host", hv.Name, "error", err) | ||
| continue | ||
| } | ||
| hvCaps[hv.Name] = caps |
There was a problem hiding this comment.
Don’t exclude the host on capability fields the flavor did not request.
hvToNovaCapabilities() fails the whole map on unknown hypervisor_type. With this continue, that host never lands in hvCaps and is later removed on Lines 107-111, even if the request only asked for something unrelated like capabilities:cpu_arch. This turns an unsupported optional field into a hard filter-out.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/scheduling/nova/plugins/filters/filter_capabilities.go` around lines
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.
| ENV PATH $PATH:/usr/lib/postgresql/$PG_MAJOR/bin | ||
|
|
||
| ENV PG_VERSION 17.8-1.pgdg13+1 | ||
| ENV PG_VERSION 17.9-1.pgdg13+1 |
There was a problem hiding this comment.
Update version in docs to match PG_VERSION.
postgres/README.md (Line 8 in the provided snippet) still says Current postgres version: v17.6 while this file now uses 17.9-1.pgdg13+1.
📝 Suggested docs sync
-Current postgres version: `v17.6`
+Current postgres version: `v17.9`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@postgres/Dockerfile` at 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.
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
1 participant
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.