Conversation
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
📝 WalkthroughWalkthroughRemoved the controller's persisted DB dependency and stopped initializing a DB in controller startup; commitments API and usage client wiring now accept an optional UsageDB backed by a datasource name, and the usage client lazily creates a Postgres reader via the Kubernetes client when first used. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as SchedulerClient
participant API as Commitments HTTP API
participant K8s as Kubernetes API
participant DB as PostgresReader
Client->>API: request (report usage / reconcile)
alt usageDB provided && reader cached
API->>DB: query usage
DB-->>API: usage rows
else usageDB provided && reader not cached
API->>K8s: request datasource secret/config
K8s-->>API: datasource info
API->>DB: New PostgresReader(datasource) (cached)
DB-->>API: connection OK / error
API->>DB: query usage
DB-->>API: usage rows
else usageDB not provided
API-->>Client: return error (usage not configured)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/scheduling/reservations/commitments/controller.go (1)
63-67: Consider clarifying why the returned connection is unused.The discarded return value works because
connectDBcaches the connection on the controller, but future readers may wonder why the result isn't used. A brief inline comment would help clarify intent.📝 Suggested clarification
// Ensure database connection is available, surfacing errors as reconcile errors // rather than crashing at startup when the secret may not yet exist. + // The connection is cached on the controller; we only need to ensure it can be established. if _, err := r.connectDB(ctx); err != nil { return ctrl.Result{}, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/controller.go` around lines 63 - 67, The call to r.connectDB(ctx) intentionally discards the returned connection because connectDB caches and stores the connection on the controller instance for later use; add a brief inline comment above the call in the reconcile path (the r.connectDB(ctx) invocation) stating that the returned value is intentionally ignored because connectDB caches the connection on the controller and the call is used only to ensure a usable DB and surface errors as reconcile errors rather than crashing at startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/scheduling/reservations/commitments/controller.go`:
- Around line 534-551: The connectDB method on CommitmentReservationController
currently acquires r.dbMu and caches the result in r.db but no callers ever read
r.db (callers discard the return and r.db is unused), so remove the misleading
caching: simplify connectDB to not store r.db or use r.dbMu and instead always
return a fresh *db.DB (remove r.db assignment and mutex usage), or alternatively
update all callers to accept and use the returned *db.DB and remove
discarded-call sites (e.g., change the call at the site that does `if _, err :=
r.connectDB(ctx); err != nil {` to capture and propagate the returned db) so the
cached r.db is actually used; pick one approach and make changes consistently
for connectDB, r.db, and r.dbMu and any call sites.
---
Nitpick comments:
In `@internal/scheduling/reservations/commitments/controller.go`:
- Around line 63-67: The call to r.connectDB(ctx) intentionally discards the
returned connection because connectDB caches and stores the connection on the
controller instance for later use; add a brief inline comment above the call in
the reconcile path (the r.connectDB(ctx) invocation) stating that the returned
value is intentionally ignored because connectDB caches the connection on the
controller and the call is used only to ensure a usable DB and surface errors as
reconcile errors rather than crashing at startup.
🪄 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: 57c691c2-d442-4945-ad1e-b30d99831a61
📒 Files selected for processing (1)
internal/scheduling/reservations/commitments/controller.go
mblos
changed the title
SoWieMarkus
requested changes
Apr 17, 2026
Collaborator
SoWieMarkus
left a comment
SoWieMarkus
left a comment
There was a problem hiding this comment.
Can you remove the db secrets reference in the values as well?
Collaborator
|
Sorry misclicked and accidentally requested the review of copilot |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes an unused knowledge DB connection from the commitment reservations controller, simplifying initialization and eliminating an unnecessary dependency.
Changes:
- Dropped the
dbpackage import and removed the controller’sDBfield. - Removed DB initialization logic from
CommitmentReservationController.Init().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -516,16 +513,6 @@ func (r *CommitmentReservationController) hypervisorToReservations(ctx context.C | |||
|
|
|||
| // Init initializes the reconciler with required clients and DB connection. | |||
There was a problem hiding this comment.
The comment above Init still says it initializes a DB connection, but the DB field/initialization has been removed. Please update the comment to reflect what Init actually does now (scheduler client only) to avoid misleading future changes.
Suggested change
| // Init initializes the reconciler with required clients and DB connection. | |
| // Init initializes the reconciler's scheduler client. |
| @@ -516,16 +513,6 @@ func (r *CommitmentReservationController) hypervisorToReservations(ctx context.C | |||
|
|
|||
| // Init initializes the reconciler with required clients and DB connection. | |||
| func (r *CommitmentReservationController) Init(ctx context.Context, client client.Client, conf Config) error { | |||
There was a problem hiding this comment.
Init no longer uses the client parameter after removing the DB connector logic. Since unparam is enabled in .golangci.yaml, this will likely be reported; consider removing the parameter from the method signature (and updating the call site), or renaming it to _ if you need to keep the signature for consistency.
Suggested change
| func (r *CommitmentReservationController) Init(ctx context.Context, client client.Client, conf Config) error { | |
| func (r *CommitmentReservationController) Init(ctx context.Context, _ client.Client, conf Config) error { |
Contributor
Test Coverage ReportTest Coverage 📊: 69.1% |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/scheduling/reservations/commitments/usage_db.go (1)
29-41: Lazy init under mutex — works, but consider double-checked pattern.
getReaderholdsc.mufor the duration ofexternal.NewPostgresReader, which performs a k8sGeton the Datasource CRD. That will serialize all first-time concurrent callers behind a single network/cache round-trip, and any transient error keepsc.reader == nilso the next call retries (good).Non-blocking suggestion: if contention becomes visible, switch to a
sync.Once-style or double-checked pattern (atomic load ofreaderwithout the mutex on the fast path). Current code is correct; this is purely an optimization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/usage_db.go` around lines 29 - 41, The current getReader method holds c.mu while calling external.NewPostgresReader which may block; to reduce contention implement a double-checked pattern: first read c.reader without locking (atomic/unsynchronized read) and return if non-nil, then acquire c.mu, check c.reader again and only call external.NewPostgresReader if still nil, set c.reader and return; refer to getReader, c.mu, c.reader and external.NewPostgresReader when making the change to ensure the fast path avoids the mutex and the slow path still serializes creation.internal/scheduling/reservations/commitments/config.go (1)
29-31: LGTM — consider validatingDatasourceNameat startup.Since an empty
DatasourceNamesilently disables usage reporting (the API ends up with a nilusageDBand every/report-usagecall errors out at request time), consider logging a warning incmd/manager/main.gowhencommitmentsConfig.DatasourceName == ""andEnableReportUsageAPIis true, so misconfiguration surfaces at boot rather than on the first request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/config.go` around lines 29 - 31, Add a startup-time validation that checks commitmentsConfig.DatasourceName and logs a warning when it's empty while EnableReportUsageAPI is true: in the manager startup (where EnableReportUsageAPI and commitmentsConfig are read), add an if block like `if EnableReportUsageAPI && commitmentsConfig.DatasourceName == "" { logger.Warn("report-usage enabled but commitmentsConfig.DatasourceName is empty; usage reporting will fail at request time") }` (use the existing logger/registry used in main) so misconfiguration is surfaced at boot rather than on first /report-usage.cmd/manager/main.go (1)
361-368: LGTM.Conditional wiring of
commitmentsUsageDBlooks correct, and threading a possibly-nil client throughNewAPIWithConfigis safe given the nil-guard inUsageCalculator.ListProjectVMs.Minor observation (pre-existing, not introduced here):
commitmentsConfigon line 362 is used to build the API without callingApplyDefaults(), while the controller block at line 537–538 fetches a fresh copy and does callApplyDefaults(). That means API-side timeouts/intervals (e.g.,ChangeAPIWatchReservationsTimeout,ChangeAPIWatchReservationsPollInterval) could be zero if not explicitly set in values. Worth fixing in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/manager/main.go` around lines 361 - 368, commitmentsConfig is used to construct the commitments API without calling ApplyDefaults(), which can leave timeouts/intervals zero; call commitmentsConfig.ApplyDefaults() before passing it into commitments.NewAPIWithConfig (i.e., ensure the same defaulting applied in the controller path is applied here) so NewAPIWithConfig and commitmentsAPI.Init receive a fully-defaulted commitments.Config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/manager/main.go`:
- Around line 361-368: commitmentsConfig is used to construct the commitments
API without calling ApplyDefaults(), which can leave timeouts/intervals zero;
call commitmentsConfig.ApplyDefaults() before passing it into
commitments.NewAPIWithConfig (i.e., ensure the same defaulting applied in the
controller path is applied here) so NewAPIWithConfig and commitmentsAPI.Init
receive a fully-defaulted commitments.Config.
In `@internal/scheduling/reservations/commitments/config.go`:
- Around line 29-31: Add a startup-time validation that checks
commitmentsConfig.DatasourceName and logs a warning when it's empty while
EnableReportUsageAPI is true: in the manager startup (where EnableReportUsageAPI
and commitmentsConfig are read), add an if block like `if EnableReportUsageAPI
&& commitmentsConfig.DatasourceName == "" { logger.Warn("report-usage enabled
but commitmentsConfig.DatasourceName is empty; usage reporting will fail at
request time") }` (use the existing logger/registry used in main) so
misconfiguration is surfaced at boot rather than on first /report-usage.
In `@internal/scheduling/reservations/commitments/usage_db.go`:
- Around line 29-41: The current getReader method holds c.mu while calling
external.NewPostgresReader which may block; to reduce contention implement a
double-checked pattern: first read c.reader without locking
(atomic/unsynchronized read) and return if non-nil, then acquire c.mu, check
c.reader again and only call external.NewPostgresReader if still nil, set
c.reader and return; refer to getReader, c.mu, c.reader and
external.NewPostgresReader when making the change to ensure the fast path avoids
the mutex and the slow path still serializes creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ae0ef5e-241c-4df8-b5d1-1414b1a3311e
📒 Files selected for processing (6)
cmd/manager/main.gohelm/bundles/cortex-nova/values.yamlinternal/scheduling/reservations/commitments/api.gointernal/scheduling/reservations/commitments/config.gointernal/scheduling/reservations/commitments/controller.gointernal/scheduling/reservations/commitments/usage_db.go
💤 Files with no reviewable changes (1)
- internal/scheduling/reservations/commitments/controller.go
SoWieMarkus
reviewed
Apr 17, 2026
Comment on lines
+364
to
+366
| if commitmentsConfig.DatasourceName != "" { | ||
| commitmentsUsageDB = commitments.NewDBUsageClient(multiclusterClient, commitmentsConfig.DatasourceName) | ||
| } |
Collaborator
There was a problem hiding this comment.
What happens if the datasource name is not set? Could this lead to a crash loop of the service if commitmentsUsageDB is nil?
Collaborator
Author
There was a problem hiding this comment.
should™️ be safe.. and API returns then http 500
SoWieMarkus
approved these changes
Apr 17, 2026
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
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.