feat: vmware project commitment kpi include domain_id#806
Merged
Conversation
SoWieMarkus
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (1)
📝 WalkthroughWalkthroughThis PR replaces a resource-centric VMware commitments KPI with a project-centric implementation: introduces VMwareProjectCommitmentsKPI, adds project↔domain lookups, extends metric labels (project_name, domain_id, domain_name), updates Helm KPI manifest and registry, and adapts tests to assert per-project/per-domain metrics. ChangesVMware Project Commitments KPI Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go (2)
70-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale
vmware_resource_commitments:prefix in log messages.All
slog.Error/slog.Warncalls in this file still use thevmware_resource_commitments:log key prefix even though the KPI has been renamed. This will make grepping logs and dashboards harder once the rename rolls out. Update the prefix to match the new identifier (e.g.,vmware_project_commitments:).Also applies to: 76-76, 169-169, 174-174, 188-188, 199-199, 227-227, 232-232, 263-263
🤖 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 `@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go` at line 70, Multiple slog.Error/slog.Warn calls in vmware_project_commitments.go still use the old "vmware_resource_commitments:" prefix; update each occurrence (e.g., the slog.Error in the flavors load, and other slog.Error/slog.Warn calls referenced around lines with symbols in this file) to use "vmware_project_commitments:" instead so logs use the new KPI identifier consistently (search for slog.Error and slog.Warn in this file and replace the "vmware_resource_commitments:" key with "vmware_project_commitments:").
206-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSilent fallback when a commitment's project is missing from the identity table.
projects[key.projectID]returns a zero-valueprojectWithDomainif the commitment'sProjectIDisn't in the projects map (e.g., because of an out-of-sync identity datasource or an orphaned commitment). The metric is still emitted with emptyproject_name,domain_id, anddomain_name, which makes the data integrity issue invisible in Prometheus. Consider logging a warning (and/or skipping the emission) so missing project↔domain context is observable.🔧 Proposed fix sketch
- project := projects[key.projectID] + project, ok := projects[key.projectID] + if !ok { + slog.Warn("vmware_project_commitments: project not found for commitment", "project_id", key.projectID) + continue + }Also applies to: 271-279
🤖 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 `@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go` around lines 206 - 219, Loop emits metrics using projects[key.projectID] which returns a zero-value projectWithDomain when missing; update the loop in the function that sends k.unusedGeneralPurposeCommitmentsPerProject (and the similar block for lines ~271-279) to check presence first (project, ok := projects[key.projectID]); if missing, log a warning including key.projectID, key.az, key.resource and the unused amount via the component logger (e.g., k.logger.Warnf or processLogger) and skip emitting the metric so orphaned commitments are visible instead of producing empty project/domain labels.
🧹 Nitpick comments (2)
internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments_test.go (2)
19-33: 💤 Low valueOptional: rename
setupResourceCommitmentsDBandcollectResourceCommitmentsMetricsfor consistency.The KPI was renamed to
VMwareProjectCommitmentsKPI, but these test helpers still carry theResourceCommitmentsname. Not a functional issue, but renaming them (e.g.,setupProjectCommitmentsDB,collectProjectCommitmentsMetrics) would keep terminology consistent across the rename.Also applies to: 38-38
🤖 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 `@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments_test.go` around lines 19 - 33, Rename the test helpers to match the KPI rename: change setupResourceCommitmentsDB to setupProjectCommitmentsDB and collectResourceCommitmentsMetrics to collectProjectCommitmentsMetrics, and update every reference to those functions (e.g., calls within vmware_project_commitments_test.go and any helper invocations) so the test helper names align with VMwareProjectCommitmentsKPI; ensure function signatures and return values remain unchanged when renaming.
86-303: ⚡ Quick winTest coverage looks thorough.
The added per-project/per-domain expectations, the inclusion of
identity.Project/identity.Domainrows, and the expanded edge cases (DELETED/ERROR servers, KVM filtering, guaranteed commitments,_v2arch suffix, multi-project aggregation) cover the new label surface well.One scenario worth adding: a commitment whose
ProjectIDhas no matching row inidentity.Project(i.e., orphaned/out-of-sync data). With the current implementation, the metric would still be emitted with emptyproject_name/domain_id/domain_name. A test would pin down the intended behavior — see the related comment on the source file about logging/skipping in that case.Also applies to: 351-557
🤖 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 `@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments_test.go` around lines 86 - 303, Add a test case to TestVMwareProjectCommitmentsKPI_Collect_GeneralPurpose that covers an orphaned commitment (a limes.Commitment whose ProjectID is not present in the provided projects slice) to assert the intended behavior; specifically, create a commitment with ProjectID "missing_project" and do NOT include a matching identity.Project/Domain, then assert whether no metric is emitted for gpKey(...) (recommended: expect no metric to ensure orphaned commitments are skipped). Update the tests slice in the same function and use gpKey(...) to build the expected map entry (or absence) to validate the behavior.
🤖 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 `@helm/bundles/cortex-nova/templates/kpis.yaml`:
- Around line 199-204: The dependencies list for this KPI is missing the
identity-projects datasource which causes getProjectsWithDomains (the function
that LEFT JOINs identity_projects with identity_domains) to run without the
projects table populated; update the dependencies block to include "- name:
identity-projects" alongside "- name: identity-domains" so both datasources are
declared and the KPI receives project_name/domain_id/domain_name labels
correctly.
In
`@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go`:
- Around line 289-304: The Helm manifest for this KPI is missing the
identity-projects datasource required by getProjectsWithDomains (which queries
identity.Project{}.TableName() for p.id, p.name, p.domain_id); update
helm/bundles/cortex-nova/templates/kpis.yaml for this KPI to include
identity-projects under dependencies.datasources (matching the peer
vmware-project-utilization entry) so the identity_projects table is scheduled
and available at collection time, and verify the chart deploys that datasource
so getProjectsWithDomains sees populated project_name/domain_id/domain_name
values.
- Around line 36-38: GetName() on VMwareProjectCommitmentsKPI currently returns
the stale identifier "vmware_resource_commitments_kpi"; update
VMwareProjectCommitmentsKPI.GetName() to return the canonical registry/Helm key
"vmware_project_commitments_kpi" so it matches supported_kpis.go and the CRD
impl field, ensuring logs/metrics that use GetName() correlate with the registry
identifier.
---
Outside diff comments:
In
`@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go`:
- Line 70: Multiple slog.Error/slog.Warn calls in vmware_project_commitments.go
still use the old "vmware_resource_commitments:" prefix; update each occurrence
(e.g., the slog.Error in the flavors load, and other slog.Error/slog.Warn calls
referenced around lines with symbols in this file) to use
"vmware_project_commitments:" instead so logs use the new KPI identifier
consistently (search for slog.Error and slog.Warn in this file and replace the
"vmware_resource_commitments:" key with "vmware_project_commitments:").
- Around line 206-219: Loop emits metrics using projects[key.projectID] which
returns a zero-value projectWithDomain when missing; update the loop in the
function that sends k.unusedGeneralPurposeCommitmentsPerProject (and the similar
block for lines ~271-279) to check presence first (project, ok :=
projects[key.projectID]); if missing, log a warning including key.projectID,
key.az, key.resource and the unused amount via the component logger (e.g.,
k.logger.Warnf or processLogger) and skip emitting the metric so orphaned
commitments are visible instead of producing empty project/domain labels.
---
Nitpick comments:
In
`@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments_test.go`:
- Around line 19-33: Rename the test helpers to match the KPI rename: change
setupResourceCommitmentsDB to setupProjectCommitmentsDB and
collectResourceCommitmentsMetrics to collectProjectCommitmentsMetrics, and
update every reference to those functions (e.g., calls within
vmware_project_commitments_test.go and any helper invocations) so the test
helper names align with VMwareProjectCommitmentsKPI; ensure function signatures
and return values remain unchanged when renaming.
- Around line 86-303: Add a test case to
TestVMwareProjectCommitmentsKPI_Collect_GeneralPurpose that covers an orphaned
commitment (a limes.Commitment whose ProjectID is not present in the provided
projects slice) to assert the intended behavior; specifically, create a
commitment with ProjectID "missing_project" and do NOT include a matching
identity.Project/Domain, then assert whether no metric is emitted for gpKey(...)
(recommended: expect no metric to ensure orphaned commitments are skipped).
Update the tests slice in the same function and use gpKey(...) to build the
expected map entry (or absence) to validate the behavior.
🪄 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: 2c2035bf-0426-48a6-9366-ae47eedef2ea
📒 Files selected for processing (4)
helm/bundles/cortex-nova/templates/kpis.yamlinternal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.gointernal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments_test.gointernal/knowledge/kpis/supported_kpis.go
…s for consistency
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)
internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go (1)
224-280:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale log prefix
vmware_resource_commitmentsin three places.Lines 227, 232, and 263 still emit
slogmessages prefixed withvmware_resource_commitments:while the rest of the file (e.g., lines 70, 76, 169, 174, 188, 199) usesvmware_project_commitments:. This breaks log filtering/correlation by KPI name.🔧 Proposed fix
commitments, err := k.getHanaInstanceCommitments() if err != nil { - slog.Error("vmware_resource_commitments: failed to load hana commitments", "err", err) + slog.Error("vmware_project_commitments: failed to load hana commitments", "err", err) return } servers, err := k.getRunningHanaServers() if err != nil { - slog.Error("vmware_resource_commitments: failed to load hana servers", "err", err) + slog.Error("vmware_project_commitments: failed to load hana servers", "err", err) return } @@ flavor, ok := flavorsByName[ck.flavorName] if !ok { - slog.Warn("vmware_resource_commitments: hana flavor not found", "flavor", ck.flavorName) + slog.Warn("vmware_project_commitments: hana flavor not found", "flavor", ck.flavorName) continue }🤖 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 `@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go` around lines 224 - 280, In collectHana, change the stale log message prefix "vmware_resource_commitments:" to "vmware_project_commitments:" for the three slog calls (the error after getHanaInstanceCommitments(), the error after getRunningHanaServers(), and the warn when a flavor is not found) so they match the rest of the file; update the strings in those slog.Error/slog.Warn invocations inside VMwareProjectCommitmentsKPI.collectHana accordingly.
🧹 Nitpick comments (1)
internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go (1)
289-304: ⚡ Quick winConsider falling back to
p.domain_idwhen the domain row is missing.The
LEFT JOINplusCOALESCE(d.id, '')will emit an emptydomain_idwhenever the matchingidentity_domainsrow hasn't been ingested yet, even though the project itself already carries a non-emptydomain_id. UsingCOALESCE(d.id, p.domain_id)preserves the known domain identifier and onlydomain_nameends up empty during transient ingestion gaps.🔧 Proposed fix
- SELECT p.id AS project_id, p.name AS project_name, COALESCE(d.id, '') AS domain_id, COALESCE(d.name, '') AS domain_name + SELECT p.id AS project_id, p.name AS project_name, COALESCE(d.id, p.domain_id) AS domain_id, COALESCE(d.name, '') AS domain_name FROM `+identity.Project{}.TableName()+` p LEFT JOIN `+identity.Domain{}.TableName()+` d ON p.domain_id = d.id🤖 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 `@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go` around lines 289 - 304, The SQL in VMwareProjectCommitmentsKPI.getProjectsWithDomains uses COALESCE(d.id, '') which returns an empty domain_id when the identity_domains row is missing; change the SELECT to fall back to the project's stored domain_id (use COALESCE(d.id, p.domain_id)) so projectWithDomain.ProjectID/DomainID preserve the known p.domain_id during transient ingestion gaps while keeping domain_name as COALESCE(d.name, '').
🤖 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.
Outside diff comments:
In
`@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go`:
- Around line 224-280: In collectHana, change the stale log message prefix
"vmware_resource_commitments:" to "vmware_project_commitments:" for the three
slog calls (the error after getHanaInstanceCommitments(), the error after
getRunningHanaServers(), and the warn when a flavor is not found) so they match
the rest of the file; update the strings in those slog.Error/slog.Warn
invocations inside VMwareProjectCommitmentsKPI.collectHana accordingly.
---
Nitpick comments:
In
`@internal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.go`:
- Around line 289-304: The SQL in
VMwareProjectCommitmentsKPI.getProjectsWithDomains uses COALESCE(d.id, '') which
returns an empty domain_id when the identity_domains row is missing; change the
SELECT to fall back to the project's stored domain_id (use COALESCE(d.id,
p.domain_id)) so projectWithDomain.ProjectID/DomainID preserve the known
p.domain_id during transient ingestion gaps while keeping domain_name as
COALESCE(d.name, '').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b8633cd-85a1-48da-abab-4996c71e55a1
📒 Files selected for processing (4)
helm/bundles/cortex-nova/templates/kpis.yamlinternal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments.gointernal/knowledge/kpis/plugins/infrastructure/vmware_project_commitments_test.gointernal/knowledge/kpis/supported_kpis.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.7% |
Merged
mblos
added a commit
that referenced
this pull request
May 7, 2026
### Release digest — 2026-05-07 — [#814](#814) #### cortex v0.0.47 (sha-7d1745d8) **New features:** - `ProjectQuota` CRD with per-resource, per-AZ quota breakdown and PAYG calculation ([#796](#796)) - `FlavorGroupCapacity` CRD + background capacity controller for pre-computed per-flavor VM slot capacity per (flavor group × AZ) ([#728](#728)) - Capacity reporting in `POST /commitments/v1/report-capacity` now uses real `FlavorGroupCapacity` CRD values (replaces placeholder zeros) - CommittedResource usage reconciler — moves usage calculation into CRD status, feeding both LIQUID API and quota controller ([#800](#800)) - KVM OS version label on host capacity metrics ([#810](#810)) - KVM project usage metrics (running VMs / resource usage per project/flavor) ([#803](#803)) - `domain_id` + name on vmware project capacity metrics ([#802](#802)) - `domain_id` in vmware project commitment KPI ([#806](#806)) - Weighing explainer for scheduling decisions ([#808](#808)) **Refactors:** - KVM host capacity metric moved to infrastructure plugins package ([#809](#809)) - Deprecated per-compute KPIs removed (`flavor_running_vms`, `host_running_vms`, `resource_capacity_kvm`) ([#807](#807)) - Bundle-specific RBAC templates moved from library chart into `cortex-ironcore` / `cortex-pods` bundles ([#797](#797)) - Webhook templates moved back into `cortex-nova` ([#805](#805)) - `testlib.Ptr` replaced with native `new()` ([#801](#801)) **Fixes:** - Remove `ignoreAllocations` from kvm-report-capacity pipeline to unblock older admission webhook ([#812](#812)) - Suppress nova scheduling alerts on transient `no such host` DNS errors - Add `identity-domains` as KPI dependency - Rename hypervisor `ClusterRoleBinding` to avoid `roleRef` conflict on redeploy ([#804](#804)) #### cortex-nova v0.0.60 (sha-7d1745d8) Includes cortex v0.0.47. Adds Prometheus datasources and KPI CRD templates for KVM project usage/utilization, and updated RBAC for `FlavorGroupCapacity` + `ProjectQuota` CRDs.
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.
Changes
Resource->Projectproject_name,domain_idanddomain_nameas labels to the metrics