feat: Add KVM OS version as label to host metrics#810
Merged
SoWieMarkus merged 2 commits intomainfrom May 7, 2026
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 consolidates KVM host metric label generation by moving ChangesKVM Host Label Consolidation and OS Version Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 1
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/shared_test.go (1)
124-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
TestKVMHost_GetHostLabels: allwantslices are missing the newos_versionelement at index 9.
kvmHostLabelsnow has 10 entries, so the length check (len(got) != len(kvmHostLabels)) passes (10 == 10), but the assertion loop only iterates over the 9 elements intt.want, silently leavingos_versionunverified.Every
wantslice needs"unknown"appended (since no test case setsSpec.OperatingSystemVersion), and a new case should cover a non-emptyOperatingSystemVersion.🐛 Proposed fix (representative cases)
- want: []string{"node001-bb01", "unknown", "bb01", "cascade-lake", "general-purpose", "true", "false", "false", "false"}, + want: []string{"node001-bb01", "unknown", "bb01", "cascade-lake", "general-purpose", "true", "false", "false", "false", "unknown"},Apply the same
"unknown"append to everywantslice in the table, and add:{ name: "os version populated", host: kvmHost{hv1.Hypervisor{ ObjectMeta: metav1.ObjectMeta{Name: "node001-bb01"}, Spec: hv1.HypervisorSpec{OperatingSystemVersion: "22.04"}, }}, want: []string{"node001-bb01", "unknown", "bb01", "cascade-lake", "general-purpose", "true", "false", "false", "false", "22.04"}, },🤖 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/shared_test.go` around lines 124 - 204, TestKVMHost_GetHostLabels currently has each tt.want missing the new os_version element so the test never verifies it; update every want slice in the table to append "unknown" (since no Spec.OperatingSystemVersion is set) so they have 10 entries matching kvmHostLabels, and add a new test case named like "os version populated" where the host (kvmHost wrapping hv1.Hypervisor) sets Spec.OperatingSystemVersion (e.g., "22.04") and the corresponding want includes that string as the last element; ensure the test loop still iterates over all entries in kvmHostLabels so the os_version index is asserted.
🧹 Nitpick comments (1)
internal/knowledge/kpis/plugins/infrastructure/shared_test.go (1)
15-33: 💤 Low value
mockKVMHostLabelsis defined inshared_test.gobut is not used by any test in that file.It is only consumed by
kvm_host_capacity_test.go(andkvm_project_utilization_test.goper the PR summary). As per coding guidelines, helper functions should be kept in the same file as the tests that use them to avoid creating ad-hoc test libraries. Consider movingmockKVMHostLabelsintokvm_host_capacity_test.go(and duplicating/inlining inkvm_project_utilization_test.goas needed).As per coding guidelines: "Avoid creating testing libraries; keep helper functions in the same file as the tests that use them."
🤖 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/shared_test.go` around lines 15 - 33, The helper function mockKVMHostLabels in shared_test.go is unused there; move it into the test files that actually call it: cut mockKVMHostLabels and paste it into kvm_host_capacity_test.go (where it’s consumed) and duplicate/inline it into kvm_project_utilization_test.go as needed so each test file owns its helper; ensure the function signature and behavior remain identical and update any imports if required.
🤖 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 `@internal/knowledge/kpis/plugins/infrastructure/shared_test.go`:
- Line 31: The hardcoded "os_version": "1.1.1" in mockKVMHostLabels causes
mismatches because getHostLabels() returns "unknown" when
hv1.Hypervisor.Spec.OperatingSystemVersion is unset and
TestKVMResourceCapacityKPI_Collect uses reflect.DeepEqual; change the mocked
label value to "unknown" to match the default path, and if you need coverage for
the non-default path add a separate test that constructs an hv1.Hypervisor with
Spec.OperatingSystemVersion set and assert labels from getHostLabels() reflect
that value (see getHostLabels, mockKVMHostLabels, and
TestKVMResourceCapacityKPI_Collect / kvm_host_capacity_test.go for where to add
the dedicated case).
---
Outside diff comments:
In `@internal/knowledge/kpis/plugins/infrastructure/shared_test.go`:
- Around line 124-204: TestKVMHost_GetHostLabels currently has each tt.want
missing the new os_version element so the test never verifies it; update every
want slice in the table to append "unknown" (since no
Spec.OperatingSystemVersion is set) so they have 10 entries matching
kvmHostLabels, and add a new test case named like "os version populated" where
the host (kvmHost wrapping hv1.Hypervisor) sets Spec.OperatingSystemVersion
(e.g., "22.04") and the corresponding want includes that string as the last
element; ensure the test loop still iterates over all entries in kvmHostLabels
so the os_version index is asserted.
---
Nitpick comments:
In `@internal/knowledge/kpis/plugins/infrastructure/shared_test.go`:
- Around line 15-33: The helper function mockKVMHostLabels in shared_test.go is
unused there; move it into the test files that actually call it: cut
mockKVMHostLabels and paste it into kvm_host_capacity_test.go (where it’s
consumed) and duplicate/inline it into kvm_project_utilization_test.go as needed
so each test file owns its helper; ensure the function signature and behavior
remain identical and update any imports if required.
🪄 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: 739b10c9-2072-409b-b21c-019d5721a38a
📒 Files selected for processing (4)
internal/knowledge/kpis/plugins/infrastructure/kvm_host_capacity_test.gointernal/knowledge/kpis/plugins/infrastructure/kvm_project_utilization_test.gointernal/knowledge/kpis/plugins/infrastructure/shared.gointernal/knowledge/kpis/plugins/infrastructure/shared_test.go
💤 Files with no reviewable changes (1)
- internal/knowledge/kpis/plugins/infrastructure/kvm_project_utilization_test.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