Calculate effective capacity from overcommit and capacity#70
Open
PhilippMatthes wants to merge 5 commits intomainfrom
Open
Calculate effective capacity from overcommit and capacity#70PhilippMatthes wants to merge 5 commits intomainfrom
PhilippMatthes wants to merge 5 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds typed resource-key maps and a new effective-capacity processor that applies overcommit ratios (floored) to hypervisor and per-cell capacities; updates go.mod and adds unit tests covering overcommit and per-cell scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
📝 Coding Plan
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 (1)
internal/libvirt/libvirt.go (1)
560-562: Align docstring with implementation.Line 560 states "capacity and allocation", but the method uses only capacity. Update wording to avoid misleading future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/libvirt/libvirt.go` around lines 560 - 562, The docstring above the method that calculates the effective allocation capacity currently says "capacity and allocation" but the implementation only uses physical capacity; update that comment (the block starting "The effective allocation capacity is calculated...") to state that it uses physical capacity multiplied by the applied overcommit ratio (default 1.0), and note fractional results are floored—remove the misleading "and allocation" phrasing so the docstring matches the implementation.
🤖 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/libvirt/libvirt_test.go`:
- Around line 1747-1812: The tests
TestAddEffectiveAllocationCapacity_FractionalValuesFloored and
TestAddEffectiveAllocationCapacity_FractionalValuesFlooredDown don't actually
produce fractional products (10*1.5 and 10*1.9 are integers), so change the
hypervisor Capacity values or Overcommit factors to produce non-integer results
(e.g., set Capacity CPU to a value like 11 or 3 so 11*1.5=16.5 or 3*1.9=5.7) and
assert the EffectiveCapacity from addEffectiveAllocationCapacity is floored (16
and 5 respectively); update the expected resource.Quantity values accordingly
and keep references to the same tests and addEffectiveAllocationCapacity
function.
In `@internal/libvirt/libvirt.go`:
- Around line 565-593: The EffectiveCapacity maps are only initialized when nil
which leaves stale keys from prior status; instead, always recreate the maps
before populating: replace the nil-checks for newHv.Status.EffectiveCapacity and
cell.EffectiveCapacity in the function that computes capacities so you
unconditionally set newHv.Status.EffectiveCapacity =
make(map[v1.ResourceName]resource.Quantity) and cell.EffectiveCapacity =
make(map[v1.ResourceName]resource.Quantity) respectively, then populate them
from newHv.Status.Capacity and cell.Capacity using the existing overcommit logic
(refer to newHv.Status.EffectiveCapacity, newHv.Spec.Overcommit, and the loop
over newHv.Status.Cells/cell.Capacity) so stale entries are removed.
---
Nitpick comments:
In `@internal/libvirt/libvirt.go`:
- Around line 560-562: The docstring above the method that calculates the
effective allocation capacity currently says "capacity and allocation" but the
implementation only uses physical capacity; update that comment (the block
starting "The effective allocation capacity is calculated...") to state that it
uses physical capacity multiplied by the applied overcommit ratio (default 1.0),
and note fractional results are floored—remove the misleading "and allocation"
phrasing so the docstring matches the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6319af8-b68c-4884-9d15-3ee4d6959f17
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/libvirt/libvirt.gointernal/libvirt/libvirt_test.go
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
PhilippMatthes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/libvirt/libvirt_test.go (1)
1566-1575: Add a regression assertion for stale key removal.Given the stale-entry fix in production code, it’s worth pre-populating
EffectiveCapacitywith an extra key and asserting it is removed after recomputation (hypervisor + cell map).Also applies to: 1601-1614
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/libvirt/libvirt_test.go` around lines 1566 - 1575, Pre-populate the EffectiveCapacity map with an extra stale key (use v1.ResourceName to create a unique key) on the test Cell and on the hypervisor setup (the EffectiveCapacity fields you already construct) before you run the recomputation step; then invoke the same hypervisor+cell map recompute used in this test and add an assertion that the stale key is no longer present in the Cell.EffectiveCapacity and the hypervisor EffectiveCapacity maps. Reference the EffectiveCapacity and Cells->Cell{CellID, Capacity, EffectiveCapacity} structures when locating where to insert the stale key and where to assert its removal after the recompute.
🤖 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/libvirt/libvirt.go`:
- Around line 568-573: The code uses newHv.Spec.Overcommit[resourceName]
directly to compute effectiveCapacity (variables: overcommit, capacity,
flooredValue, effectiveCapacity) and must validate the ratio before
multiplication; update the logic that reads overcommit to reject NaN, ±Inf, and
non-positive values (<=0) and fall back to a safe default (e.g., 1.0) or clamp
to a sensible minimum, then compute flooredValue =
int64(float64(capacity.Value()) * validOvercommit) and ensure the resulting
flooredValue is not negative before creating resource.NewQuantity; apply the
same validation wherever overcommit is read (the other block around the later
use) so invalid ratios cannot produce undefined or negative capacities.
---
Nitpick comments:
In `@internal/libvirt/libvirt_test.go`:
- Around line 1566-1575: Pre-populate the EffectiveCapacity map with an extra
stale key (use v1.ResourceName to create a unique key) on the test Cell and on
the hypervisor setup (the EffectiveCapacity fields you already construct) before
you run the recomputation step; then invoke the same hypervisor+cell map
recompute used in this test and add an assertion that the stale key is no longer
present in the Cell.EffectiveCapacity and the hypervisor EffectiveCapacity maps.
Reference the EffectiveCapacity and Cells->Cell{CellID, Capacity,
EffectiveCapacity} structures when locating where to insert the stale key and
where to assert its removal after the recompute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 404fa40e-d442-46ce-a0c9-8ea3c3eac652
📒 Files selected for processing (2)
internal/libvirt/libvirt.gointernal/libvirt/libvirt_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/libvirt/libvirt.go (2)
563-591: Factor the effective-capacity transform into one helper.The hypervisor-level and per-cell loops duplicate the same overcommit-to-quantity calculation. Pulling that into a small helper reduces drift the next time this logic changes.
♻️ Suggested refactor
func (l *LibVirt) addEffectiveCapacity(old v1.Hypervisor) (v1.Hypervisor, error) { newHv := *old.DeepCopy() - // Always recreate the EffectiveCapacity map to remove stale entries - newHv.Status.EffectiveCapacity = make(map[v1.ResourceName]resource.Quantity) - for resourceName, capacity := range newHv.Status.Capacity { - overcommit, ok := newHv.Spec.Overcommit[resourceName] - if !ok { - overcommit = 1.0 - } - flooredValue := int64(float64(capacity.Value()) * overcommit) - effectiveCapacity := resource.NewQuantity(flooredValue, capacity.Format) - newHv.Status.EffectiveCapacity[resourceName] = *effectiveCapacity - } + calculateEffectiveCapacity := func( + capacities map[v1.ResourceName]resource.Quantity, + ) map[v1.ResourceName]resource.Quantity { + effective := make(map[v1.ResourceName]resource.Quantity, len(capacities)) + for resourceName, capacity := range capacities { + overcommit := 1.0 + if configured, ok := newHv.Spec.Overcommit[resourceName]; ok { + overcommit = configured + } + effective[resourceName] = *resource.NewQuantity( + int64(float64(capacity.Value())*overcommit), + capacity.Format, + ) + } + return effective + } + + newHv.Status.EffectiveCapacity = calculateEffectiveCapacity(newHv.Status.Capacity) // Also apply this to each cell. for i, cell := range newHv.Status.Cells { - // Always recreate the cell's EffectiveCapacity map to remove stale entries - cell.EffectiveCapacity = make(map[v1.ResourceName]resource.Quantity) - for resourceName, capacity := range cell.Capacity { - overcommit, ok := newHv.Spec.Overcommit[resourceName] - if !ok { - overcommit = 1.0 - } - flooredValue := int64(float64(capacity.Value()) * overcommit) - effectiveCapacity := resource.NewQuantity(flooredValue, capacity.Format) - cell.EffectiveCapacity[resourceName] = *effectiveCapacity - } + cell.EffectiveCapacity = calculateEffectiveCapacity(cell.Capacity) newHv.Status.Cells[i] = cell } return newHv, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/libvirt/libvirt.go` around lines 563 - 591, The addEffectiveCapacity function duplicates the overcommit-to-quantity calculation for the hypervisor and for each cell; extract that logic into a small helper (e.g., computeEffectiveCapacity or applyOvercommitToCapacity) that accepts a source map[v1.ResourceName]resource.Quantity and the overcommit map (newHv.Spec.Overcommit) and returns a map[v1.ResourceName]resource.Quantity of floored effective quantities (preserving each capacity.Format and using resource.NewQuantity(int64(float64(cap.Value())*overcommit), cap.Format) with default overcommit 1.0). Replace the inline loops in addEffectiveCapacity with calls to this helper for newHv.Status.Capacity and for each cell.Capacity, assign the returned maps to newHv.Status.EffectiveCapacity and cell.EffectiveCapacity respectively, and keep the existing newHv.Status.Cells[i] = cell update.
543-554: SortStatus.Cellsbefore persisting it.Line 544 builds the slice from a map, so cell order can flip across reconciles. That makes status updates noisy and multi-cell assertions order-dependent.
💡 Suggested change
import ( "context" "errors" "fmt" "os" "reflect" + "sort" "sync" "time"cellsAsSlice := []v1.Cell{} for _, cell := range cellsById { cellsAsSlice = append(cellsAsSlice, cell) } + sort.Slice(cellsAsSlice, func(i, j int) bool { + return cellsAsSlice[i].CellID < cellsAsSlice[j].CellID + }) newHv.Status.Capacity = make(map[v1.ResourceName]resource.Quantity)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/libvirt/libvirt.go` around lines 543 - 554, The slice built from cellsById can vary order across reconciles; sort cellsAsSlice before assigning to newHv.Status.Cells to ensure deterministic status updates. After constructing cellsAsSlice (from cellsById) call sort.SliceStable on it using a stable unique key (e.g. cell.ID or cell.Name) to compare elements, then assign the sorted slice to newHv.Status.Cells; also ensure the sort package is imported. This affects the code around cellsAsSlice and newHv.Status.Cells in internal/libvirt/libvirt.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/libvirt/libvirt.go`:
- Around line 563-591: The addEffectiveCapacity function duplicates the
overcommit-to-quantity calculation for the hypervisor and for each cell; extract
that logic into a small helper (e.g., computeEffectiveCapacity or
applyOvercommitToCapacity) that accepts a source
map[v1.ResourceName]resource.Quantity and the overcommit map
(newHv.Spec.Overcommit) and returns a map[v1.ResourceName]resource.Quantity of
floored effective quantities (preserving each capacity.Format and using
resource.NewQuantity(int64(float64(cap.Value())*overcommit), cap.Format) with
default overcommit 1.0). Replace the inline loops in addEffectiveCapacity with
calls to this helper for newHv.Status.Capacity and for each cell.Capacity,
assign the returned maps to newHv.Status.EffectiveCapacity and
cell.EffectiveCapacity respectively, and keep the existing newHv.Status.Cells[i]
= cell update.
- Around line 543-554: The slice built from cellsById can vary order across
reconciles; sort cellsAsSlice before assigning to newHv.Status.Cells to ensure
deterministic status updates. After constructing cellsAsSlice (from cellsById)
call sort.SliceStable on it using a stable unique key (e.g. cell.ID or
cell.Name) to compare elements, then assign the sorted slice to
newHv.Status.Cells; also ensure the sort package is imported. This affects the
code around cellsAsSlice and newHv.Status.Cells in internal/libvirt/libvirt.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61a03da2-6469-48a4-b747-47fe3a4db5c7
📒 Files selected for processing (2)
internal/libvirt/libvirt.gointernal/libvirt/libvirt_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/libvirt/libvirt_test.go
|
Caution Docstrings generation - FAILED No docstrings were generated. |
notandy
approved these changes
Mar 13, 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
2 participants
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.
In cobaltcore-dev/openstack-hypervisor-operator#257 we add a spec field that can be used to specify the desired overcommit ratio for each resource name. This change applies the overcommit ratio to the hypervisor to its status, by calculating the effective capacity and exposing it through the hypervisor status.
Summary by CodeRabbit
New Features
Chores
Tests