Open
Conversation
SoWieMarkus
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
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 54 minutes and 14 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 (1)
📝 WalkthroughWalkthroughAdds a new VMware commitments KPI: a Prometheus-backed KPI plugin that queries limes commitments, Nova flavors, and Nova servers to compute unused HANA instance commitments and exposes CPU/RAM/Disk gauges; includes a Kubernetes KPI manifest and registration. Changes
Sequence Diagram(s)sequenceDiagram
participant Collector as KPI Collector
participant KPI as VMwareResourceCommitmentsKPI
participant DB as Database
participant Prometheus as Prometheus
Collector->>KPI: Collect()
KPI->>DB: Query limes commitments (resource_name LIKE 'instances_%')
DB-->>KPI: Commitment rows
KPI->>DB: Query Nova flavors (vCPUs, RAM, Disk)
DB-->>KPI: Flavor records
KPI->>DB: Query Nova servers (flavor_name LIKE 'hana_%', exclude DELETED/ERROR)
DB-->>KPI: Server records (status, flavor_name, project_id, availability_zone)
KPI->>KPI: Derive cpu_architecture from flavor_name
KPI->>KPI: Aggregate committed vs running by (project, flavor, AZ, arch)
KPI->>KPI: Compute unused = max(0, committed - running)
KPI->>KPI: Convert unused → cpu/ram/disk using flavor capacities
KPI->>Prometheus: Emit gauge metrics (cortex_vmware_hana_unused_instance_commitments)
Prometheus-->>Collector: Metrics recorded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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: 2
🧹 Nitpick comments (2)
internal/knowledge/kpis/supported_kpis.go (1)
25-25: Align the registry key with the implementation name.
VMwareResourceCommitmentsKPI.GetName()returnsvmware_hana_commitments_kpi, but this registry exposes the same plugin asvmware_commitments_kpi. Keeping two public names for one KPI makes manifests and dashboards harder to reason about and is easy to break later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/kpis/supported_kpis.go` at line 25, The registry key for the VMware commitments KPI is inconsistent with the implementation: update the key in the supported_kpis map to match VMwareResourceCommitmentsKPI.GetName() (change "vmware_commitments_kpi" to "vmware_hana_commitments_kpi") so the public registry name and the plugin's GetName() are identical; locate the entry referencing &compute.VMwareResourceCommitmentsKPI{} in supported_kpis and replace the map key accordingly to avoid dual public names.helm/bundles/cortex-nova/templates/kpis.yaml (1)
191-199: Make the manifest HANA-specific.The implementation skips every non-
hana_commitment and exports acortex_vmware_hana_*metric, sovmware-commitmentsreads broader than what this KPI actually covers. Narrowing the resource name/description now will avoid misleading dashboards and alerts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/bundles/cortex-nova/templates/kpis.yaml` around lines 191 - 199, The manifest currently names the KPI "vmware-commitments" but the implementation (impl: vmware_commitments_kpi) only processes hana_ commitments and emits cortex_vmware_hana_* metrics, so rename and reword this resource to be HANA-specific: change the resource name (vmware-commitments) to something like vmware-hana-commitments and update the description to explicitly state it only tracks VMware HANA commitments and unused HANA-specific reservations; leave impl (vmware_commitments_kpi) and dependencies (limes-project-commitments) intact so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/bundles/cortex-nova/templates/kpis.yaml`:
- Around line 195-197: The KPI manifest only depends on
limes-project-commitments but the code in
internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go also
queries Nova flavors and servers, so update the kpis.yaml dependencies to
include the Nova datasources (add entries for the datasource names used by Nova:
"nova-flavors" and "nova-servers") alongside "limes-project-commitments" so the
KPI waits for those tables to be populated before running; make sure the
datasource names exactly match the datasource resources registered for flavors
and servers.
In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go`:
- Around line 82-88: The query using k.DB.Select against
nova.Server{}.TableName() currently treats every non-DELETED/ERROR server as
consuming HANA capacity; change the WHERE clause to only include real running
states (e.g., WHERE status IN ('ACTIVE','RESCUED') — add other cluster-specific
running states as needed) so SHUTOFF/SHELVED_OFFLOADED/BUILD do not reduce
unused-commitment; then add a regression test that uses the same selection logic
to assert that SHUTOFF and SHELVED_OFFLOADED servers are not counted toward used
commitments (name the test to reference stopped/shelved regression).
---
Nitpick comments:
In `@helm/bundles/cortex-nova/templates/kpis.yaml`:
- Around line 191-199: The manifest currently names the KPI "vmware-commitments"
but the implementation (impl: vmware_commitments_kpi) only processes hana_
commitments and emits cortex_vmware_hana_* metrics, so rename and reword this
resource to be HANA-specific: change the resource name (vmware-commitments) to
something like vmware-hana-commitments and update the description to explicitly
state it only tracks VMware HANA commitments and unused HANA-specific
reservations; leave impl (vmware_commitments_kpi) and dependencies
(limes-project-commitments) intact so behavior is unchanged.
In `@internal/knowledge/kpis/supported_kpis.go`:
- Line 25: The registry key for the VMware commitments KPI is inconsistent with
the implementation: update the key in the supported_kpis map to match
VMwareResourceCommitmentsKPI.GetName() (change "vmware_commitments_kpi" to
"vmware_hana_commitments_kpi") so the public registry name and the plugin's
GetName() are identical; locate the entry referencing
&compute.VMwareResourceCommitmentsKPI{} in supported_kpis and replace the map
key accordingly to avoid dual public names.
🪄 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: 653be8de-2b01-4620-94b8-271366ad909d
📒 Files selected for processing (4)
helm/bundles/cortex-nova/templates/kpis.yamlinternal/knowledge/kpis/plugins/compute/resource_commitments_vmware.gointernal/knowledge/kpis/plugins/compute/resource_commitments_vmware_test.gointernal/knowledge/kpis/supported_kpis.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go (1)
82-88:⚠️ Potential issue | 🟠 MajorRestrict the server filter to states that actually burn HANA commitments.
This query still counts stopped/shelved/transitional VMs as "used", so the KPI underreports unused commitments whenever a HANA server exists but is not actually consuming reserved capacity. Please switch Line 87 to an allowlist of commitment-consuming states and add a stopped/shelved regression case.
Suggested fix
if _, err := k.DB.Select(&servers, ` SELECT * FROM `+nova.Server{}.TableName()+` WHERE flavor_name LIKE 'hana_%' - AND status NOT IN ('DELETED', 'ERROR') + AND status IN ('ACTIVE', 'RESCUED') `); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go` around lines 82 - 88, The SQL filter currently treats any non-DELETED/ERROR server as "running" and inflates used HANA commitments; update the WHERE clause in the k.DB.Select call that queries nova.Server{}.TableName() (the variable servers) to use an allowlist of states that actually consume commitments (e.g., only include states like 'ACTIVE' or the cloud-specific running state(s) rather than NOT IN ('DELETED','ERROR')), and add a regression test case asserting that stopped/shelved VMs do not count as used commitments (covering stopped/shelved transitional states).
🤖 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/knowledge/kpis/plugins/compute/resource_commitments_vmware.go`:
- Around line 124-127: The subtraction of two uint64s to compute unused (unused
:= total - runningCount[sk]) can underflow when runningCount[sk] > total; change
the logic in the loop over committed to guard the subtraction by first comparing
totals: if runningCount[sk] >= total then set unused = 0 (or skip exporting)
otherwise compute unused = total - runningCount[sk]; update the handling around
serverKey, committed and runningCount to use that guarded path so no uint64
underflow occurs.
---
Duplicate comments:
In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go`:
- Around line 82-88: The SQL filter currently treats any non-DELETED/ERROR
server as "running" and inflates used HANA commitments; update the WHERE clause
in the k.DB.Select call that queries nova.Server{}.TableName() (the variable
servers) to use an allowlist of states that actually consume commitments (e.g.,
only include states like 'ACTIVE' or the cloud-specific running state(s) rather
than NOT IN ('DELETED','ERROR')), and add a regression test case asserting that
stopped/shelved VMs do not count as used commitments (covering stopped/shelved
transitional states).
🪄 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: af2452c8-f295-4ac3-8c44-ead4af6162b7
📒 Files selected for processing (2)
helm/bundles/cortex-nova/templates/kpis.yamlinternal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/bundles/cortex-nova/templates/kpis.yaml
internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go (1)
82-88:⚠️ Potential issue | 🟠 MajorOnly count states that actually consume HANA commitment capacity.
Line 87 still treats every non-
DELETED/ERRORserver as used, which includesSHUTOFF,SHELVED_OFFLOADED,BUILD, etc., and underreports unused commitments. Use a positive allowlist of running states instead (e.g.,ACTIVE,RESCUED, plus environment-specific running states), and add a stopped/shelved regression test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go` around lines 82 - 88, The query in resource_commitments_vmware.go currently treats any server not in ('DELETED','ERROR') as consuming HANA commitment capacity; change the WHERE clause in the k.DB.Select call that loads []nova.Server (using nova.Server{}.TableName()) to use an explicit allowlist of running states (e.g., WHERE flavor_name LIKE 'hana_%' AND status IN ('ACTIVE','RESCUED' /* add any env-specific running states here */)) instead of NOT IN ('DELETED','ERROR'), and add a regression test that creates HANA-flavored servers in stopped/shelved states (e.g., SHUTOFF, SHELVED_OFFLOADED) to assert they are not counted as used.
🧹 Nitpick comments (1)
internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go (1)
108-110: Reduce scrape-path log volume for skipped KVM commitments.Line 109 logs at
Infofor each skipped commitment on every collection cycle; this can create noisy/high-cardinality logs. ConsiderDebuglevel or aggregate one summary log per collect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go` around lines 108 - 110, The loop that checks flavors using strings.HasPrefix(flavorName, "hana_k_") currently calls slog.Info("unused_commitments: skipping hana kvm commitment", ...) for every skipped commitment which creates noisy/high-cardinality logs; change that call to slog.Debug(...) to reduce log volume (or alternatively accumulate a counter and emit a single Info summary per collection cycle) — update the slog.Info invocation referencing flavorName and c.ProjectID in resource_commitments_vmware.go accordingly so skipped hana_k_ KVM commitments are logged at Debug (or aggregated) instead of Info.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go`:
- Around line 82-88: The query in resource_commitments_vmware.go currently
treats any server not in ('DELETED','ERROR') as consuming HANA commitment
capacity; change the WHERE clause in the k.DB.Select call that loads
[]nova.Server (using nova.Server{}.TableName()) to use an explicit allowlist of
running states (e.g., WHERE flavor_name LIKE 'hana_%' AND status IN
('ACTIVE','RESCUED' /* add any env-specific running states here */)) instead of
NOT IN ('DELETED','ERROR'), and add a regression test that creates HANA-flavored
servers in stopped/shelved states (e.g., SHUTOFF, SHELVED_OFFLOADED) to assert
they are not counted as used.
---
Nitpick comments:
In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go`:
- Around line 108-110: The loop that checks flavors using
strings.HasPrefix(flavorName, "hana_k_") currently calls
slog.Info("unused_commitments: skipping hana kvm commitment", ...) for every
skipped commitment which creates noisy/high-cardinality logs; change that call
to slog.Debug(...) to reduce log volume (or alternatively accumulate a counter
and emit a single Info summary per collection cycle) — update the slog.Info
invocation referencing flavorName and c.ProjectID in
resource_commitments_vmware.go accordingly so skipped hana_k_ KVM commitments
are logged at Debug (or aggregated) instead of Info.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f366f417-b138-454d-b7a1-275e7cf92711
📒 Files selected for processing (1)
internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go (1)
84-88:⚠️ Potential issue | 🟠 MajorCount only capacity-consuming server states in this query.
At Line 87,
status NOT IN ('DELETED', 'ERROR')still treats stopped/shelved/transient servers as commitment-consuming, which underreports unused capacity. Please restrict this filter to states that should burn HANA commitments (e.g.,ACTIVEplus any environment-specific running equivalents), and add a regression assertion for stopped/shelved states.Suggested fix
if _, err := k.DB.Select(&servers, ` SELECT * FROM `+nova.Server{}.TableName()+` WHERE flavor_name LIKE 'hana_%' - AND status NOT IN ('DELETED', 'ERROR') + AND status IN ('ACTIVE', 'RESCUED') `); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go` around lines 84 - 88, The SQL in the k.DB.Select call that queries nova.Server{}.TableName() currently treats any server not DELETED/ERROR as consuming HANA commitments; change the WHERE clause to only include capacity-consuming states (e.g., status IN ('ACTIVE') or the project's canonical running states) instead of status NOT IN ('DELETED','ERROR') so stopped/shelved/transient servers are excluded; update the query used in the function where k.DB.Select(&servers, ...) is invoked and add a regression test asserting that servers with STOPPED/SHELVED (and other non-running states) are not counted toward HANA commitments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.go`:
- Around line 84-88: The SQL in the k.DB.Select call that queries
nova.Server{}.TableName() currently treats any server not DELETED/ERROR as
consuming HANA commitments; change the WHERE clause to only include
capacity-consuming states (e.g., status IN ('ACTIVE') or the project's canonical
running states) instead of status NOT IN ('DELETED','ERROR') so
stopped/shelved/transient servers are excluded; update the query used in the
function where k.DB.Select(&servers, ...) is invoked and add a regression test
asserting that servers with STOPPED/SHELVED (and other non-running states) are
not counted toward HANA commitments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d78d40b-3a9b-4581-bcbe-987565127873
📒 Files selected for processing (2)
internal/knowledge/kpis/plugins/compute/resource_commitments_vmware.gointernal/knowledge/kpis/plugins/compute/resource_commitments_vmware_test.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.2% |
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