Merged
Conversation
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates resource quantity maps throughout the codebase from string-keyed ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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.
🧹 Nitpick comments (7)
internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go (1)
42-49: Consider usinghv1.ResourceCPUandhv1.ResourceMemoryconstants for consistency.The map type is correctly updated to
map[hv1.ResourceName]resource.Quantity, but string literals"cpu"and"memory"are used as keys. Other files in this PR (e.g.,kvm_binpack.go,vmware_binpack_test.go) usehv1.ResourceCPUandhv1.ResourceMemoryconstants, which would be more consistent and type-safe.♻️ Suggested improvement
Status: hv1.HypervisorStatus{ Capacity: map[hv1.ResourceName]resource.Quantity{ - "cpu": resource.MustParse(cpuCap), - "memory": resource.MustParse(memCap), + hv1.ResourceCPU: resource.MustParse(cpuCap), + hv1.ResourceMemory: resource.MustParse(memCap), }, Allocation: map[hv1.ResourceName]resource.Quantity{ - "cpu": resource.MustParse(cpuAlloc), - "memory": resource.MustParse(memAlloc), + hv1.ResourceCPU: resource.MustParse(cpuAlloc), + hv1.ResourceMemory: resource.MustParse(memAlloc), }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go` around lines 42 - 49, The test uses string literals "cpu" and "memory" as keys in the Capacity and Allocation maps even though the map type is map[hv1.ResourceName]resource.Quantity; replace those string literals with the typed constants hv1.ResourceCPU and hv1.ResourceMemory in both Capacity and Allocation entries (in filter_has_enough_capacity_test.go) to match other files like kvm_binpack.go and vmware_binpack_test.go and ensure type-safety and consistency.internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go (2)
186-216: Mixed string andhv1.ResourceNamekey usage for resource maps.The
freeResourcesByHostmap useshv1.ResourceNamekeys, but this block accesses it with string literals ("cpu","memory") becauseresourcesToBlockcomes fromreservation.Spec.Resourceswhich still usesmap[string]resource.Quantity. This works due to implicit type conversion but creates an inconsistency.Consider defining constants or adding a comment explaining why string literals are used here (the Reservation API uses string keys while HypervisorStatus uses
hv1.ResourceNamekeys).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go` around lines 186 - 216, The code mixes string keys ("cpu", "memory") with hv1.ResourceName keys in freeResourcesByHost; normalize keys by converting reservation.Spec.Resources (resourcesToBlock) from map[string]resource.Quantity into a map[hv1.ResourceName]resource.Quantity before the blocking loop in the function containing the shown block (use hv1.ResourceCPU / hv1.ResourceMemory or a small mapping helper), or alternatively replace the string literals with the corresponding hv1.ResourceName constants when reading from freeResourcesByHost; include a brief comment noting that reservation.Spec.Resources uses string keys so a conversion/mapping is performed to keep map key types consistent.
227-227: Same string literal usage forfreeResourcesByHostaccess.Line 227 uses
free["cpu"]and line 253 usesfree["memory"]to access themap[hv1.ResourceName]resource.Quantity. This works but is inconsistent with the typed approach used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go` at line 227, Replace string-literal map access with the typed resource name constants: in the filter_has_enough_capacity logic where you read free["cpu"] and free["memory"], use the hv1.ResourceCPU and hv1.ResourceMemory constants (e.g., free[hv1.ResourceCPU], free[hv1.ResourceMemory]) so accesses on the map[hv1.ResourceName]resource.Quantity are consistent; update both occurrences (the variables freeCPU and freeMemory) in the function handling freeResourcesByHost.internal/scheduling/nova/integration_test.go (1)
51-58: Same consistency suggestion as in other test files.The
newHypervisorhelper uses typedmap[hv1.ResourceName]resource.Quantitybut with string literal keys. Consider usinghv1.ResourceCPUandhv1.ResourceMemoryconstants for consistency.♻️ Suggested refactor
Status: hv1.HypervisorStatus{ Capacity: map[hv1.ResourceName]resource.Quantity{ - "cpu": resource.MustParse(cpuCap), - "memory": resource.MustParse(memCap), + hv1.ResourceCPU: resource.MustParse(cpuCap), + hv1.ResourceMemory: resource.MustParse(memCap), }, Allocation: map[hv1.ResourceName]resource.Quantity{ - "cpu": resource.MustParse(cpuAlloc), - "memory": resource.MustParse(memAlloc), + hv1.ResourceCPU: resource.MustParse(cpuAlloc), + hv1.ResourceMemory: resource.MustParse(memAlloc), }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/integration_test.go` around lines 51 - 58, The map literal in the newHypervisor helper uses string keys while the map type is typed as map[hv1.ResourceName]resource.Quantity; update the keys to use the hv1 constants (hv1.ResourceCPU and hv1.ResourceMemory) in both Capacity and Allocation maps so the keys are consistent and type-safe with the map declaration, e.g., replace the string keys "cpu"/"memory" with hv1.ResourceCPU/hv1.ResourceMemory wherever newHypervisor constructs these maps.internal/scheduling/nova/plugins/weighers/kvm_binpack_test.go (2)
25-32: Consider usinghv1.ResourceCPUandhv1.ResourceMemoryconstants for consistency.The map is now typed as
map[hv1.ResourceName]resource.Quantity, but you're using string literals"cpu"and"memory"as keys. While this works due to Go's underlying type conversion, using the typed constantshv1.ResourceCPUandhv1.ResourceMemorywould be more consistent with how they're used elsewhere in the tests (e.g., lines 84-85, 529, 539).♻️ Suggested refactor for consistency
Status: hv1.HypervisorStatus{ Capacity: map[hv1.ResourceName]resource.Quantity{ - "cpu": resource.MustParse(capacityCPU), - "memory": resource.MustParse(capacityMem), + hv1.ResourceCPU: resource.MustParse(capacityCPU), + hv1.ResourceMemory: resource.MustParse(capacityMem), }, Allocation: map[hv1.ResourceName]resource.Quantity{ - "cpu": resource.MustParse(allocationCPU), - "memory": resource.MustParse(allocationMem), + hv1.ResourceCPU: resource.MustParse(allocationCPU), + hv1.ResourceMemory: resource.MustParse(allocationMem), }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/weighers/kvm_binpack_test.go` around lines 25 - 32, The Capacity and Allocation maps use string keys "cpu" and "memory" but the map type is map[hv1.ResourceName]resource.Quantity; replace those string literals with the typed constants hv1.ResourceCPU and hv1.ResourceMemory in both the Capacity and Allocation initializers (the blocks constructing the maps in the test) to match other usages (e.g., where hv1.ResourceCPU/hv1.ResourceMemory are used) and keep key types consistent.
346-353: Same consistency suggestion applies to inline hypervisor constructions.These inline
CapacityandAllocationmaps also use string literals instead of the typed constants. Consider usinghv1.ResourceCPUandhv1.ResourceMemoryfor consistency with the rest of the test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/weighers/kvm_binpack_test.go` around lines 346 - 353, Replace the string literal resource keys in the inline Capacity and Allocation maps with the typed constants to match the rest of the test: use hv1.ResourceCPU and hv1.ResourceMemory instead of "cpu" and "memory" in the Capacity and Allocation maps found in the kvm_binpack_test.go inline hypervisor constructions; update any affected map literal keys so they compile and remain consistent with other tests.internal/scheduling/nova/plugins/weighers/kvm_prefer_smaller_hosts_test.go (1)
25-28: Same consistency suggestion fornewHypervisorWithCapacityhelper.Consider using
hv1.ResourceCPUandhv1.ResourceMemoryconstants instead of string literals for map keys.♻️ Suggested refactor
Status: hv1.HypervisorStatus{ Capacity: map[hv1.ResourceName]resource.Quantity{ - "cpu": resource.MustParse(capacityCPU), - "memory": resource.MustParse(capacityMem), + hv1.ResourceCPU: resource.MustParse(capacityCPU), + hv1.ResourceMemory: resource.MustParse(capacityMem), }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/weighers/kvm_prefer_smaller_hosts_test.go` around lines 25 - 28, The test helper newHypervisorWithCapacity uses string literals "cpu" and "memory" as keys in the Capacity map; change those keys to the package constants hv1.ResourceCPU and hv1.ResourceMemory to keep consistency and avoid typos. Locate the Capacity: map[hv1.ResourceName]resource.Quantity block inside newHypervisorWithCapacity and replace the string keys with hv1.ResourceCPU and hv1.ResourceMemory respectively, ensuring imports remain correct and types still match.
🤖 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/scheduling/nova/integration_test.go`:
- Around line 51-58: The map literal in the newHypervisor helper uses string
keys while the map type is typed as map[hv1.ResourceName]resource.Quantity;
update the keys to use the hv1 constants (hv1.ResourceCPU and
hv1.ResourceMemory) in both Capacity and Allocation maps so the keys are
consistent and type-safe with the map declaration, e.g., replace the string keys
"cpu"/"memory" with hv1.ResourceCPU/hv1.ResourceMemory wherever newHypervisor
constructs these maps.
In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go`:
- Around line 42-49: The test uses string literals "cpu" and "memory" as keys in
the Capacity and Allocation maps even though the map type is
map[hv1.ResourceName]resource.Quantity; replace those string literals with the
typed constants hv1.ResourceCPU and hv1.ResourceMemory in both Capacity and
Allocation entries (in filter_has_enough_capacity_test.go) to match other files
like kvm_binpack.go and vmware_binpack_test.go and ensure type-safety and
consistency.
In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go`:
- Around line 186-216: The code mixes string keys ("cpu", "memory") with
hv1.ResourceName keys in freeResourcesByHost; normalize keys by converting
reservation.Spec.Resources (resourcesToBlock) from map[string]resource.Quantity
into a map[hv1.ResourceName]resource.Quantity before the blocking loop in the
function containing the shown block (use hv1.ResourceCPU / hv1.ResourceMemory or
a small mapping helper), or alternatively replace the string literals with the
corresponding hv1.ResourceName constants when reading from freeResourcesByHost;
include a brief comment noting that reservation.Spec.Resources uses string keys
so a conversion/mapping is performed to keep map key types consistent.
- Line 227: Replace string-literal map access with the typed resource name
constants: in the filter_has_enough_capacity logic where you read free["cpu"]
and free["memory"], use the hv1.ResourceCPU and hv1.ResourceMemory constants
(e.g., free[hv1.ResourceCPU], free[hv1.ResourceMemory]) so accesses on the
map[hv1.ResourceName]resource.Quantity are consistent; update both occurrences
(the variables freeCPU and freeMemory) in the function handling
freeResourcesByHost.
In `@internal/scheduling/nova/plugins/weighers/kvm_binpack_test.go`:
- Around line 25-32: The Capacity and Allocation maps use string keys "cpu" and
"memory" but the map type is map[hv1.ResourceName]resource.Quantity; replace
those string literals with the typed constants hv1.ResourceCPU and
hv1.ResourceMemory in both the Capacity and Allocation initializers (the blocks
constructing the maps in the test) to match other usages (e.g., where
hv1.ResourceCPU/hv1.ResourceMemory are used) and keep key types consistent.
- Around line 346-353: Replace the string literal resource keys in the inline
Capacity and Allocation maps with the typed constants to match the rest of the
test: use hv1.ResourceCPU and hv1.ResourceMemory instead of "cpu" and "memory"
in the Capacity and Allocation maps found in the kvm_binpack_test.go inline
hypervisor constructions; update any affected map literal keys so they compile
and remain consistent with other tests.
In `@internal/scheduling/nova/plugins/weighers/kvm_prefer_smaller_hosts_test.go`:
- Around line 25-28: The test helper newHypervisorWithCapacity uses string
literals "cpu" and "memory" as keys in the Capacity map; change those keys to
the package constants hv1.ResourceCPU and hv1.ResourceMemory to keep consistency
and avoid typos. Locate the Capacity: map[hv1.ResourceName]resource.Quantity
block inside newHypervisorWithCapacity and replace the string keys with
hv1.ResourceCPU and hv1.ResourceMemory respectively, ensuring imports remain
correct and types still match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3edd4829-851e-4a8d-b1a2-fb781fe7a6fa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.modinternal/knowledge/kpis/plugins/compute/resource_capacity_kvm_test.gointernal/scheduling/nova/integration_test.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.gointernal/scheduling/nova/plugins/weighers/kvm_binpack.gointernal/scheduling/nova/plugins/weighers/kvm_binpack_test.gointernal/scheduling/nova/plugins/weighers/kvm_prefer_smaller_hosts.gointernal/scheduling/nova/plugins/weighers/kvm_prefer_smaller_hosts_test.gointernal/scheduling/nova/plugins/weighers/vmware_binpack.gointernal/scheduling/nova/plugins/weighers/vmware_binpack_test.go
SoWieMarkus
approved these changes
Mar 16, 2026
Member
Author
|
I will cleanup some more parts of this.
|
There was a problem hiding this comment.
🧹 Nitpick comments (8)
internal/scheduling/reservations/commitments/syncer_test.go (1)
297-304: Usehv1.ResourceNameconstants for map access consistency.The test assertions access
res.Spec.Resourcesusing string literals"memory"and"cpu", while the rest of the codebase useshv1.ResourceMemoryandhv1.ResourceCPU. Although this compiles (sincehv1.ResourceNameis a string alias), it's inconsistent with the migration pattern.🔧 Suggested fix for consistency
// Check resource values - should be sized for the flavor that fits // With 2048MB total capacity, we can fit 2x 1024MB flavors expectedMemory := resource.MustParse("1073741824") // 1024MB in bytes - if !res.Spec.Resources["memory"].Equal(expectedMemory) { - t.Errorf("Expected memory %v, got %v", expectedMemory, res.Spec.Resources["memory"]) + if !res.Spec.Resources[hv1.ResourceMemory].Equal(expectedMemory) { + t.Errorf("Expected memory %v, got %v", expectedMemory, res.Spec.Resources[hv1.ResourceMemory]) } expectedVCPUs := resource.MustParse("2") - if !res.Spec.Resources["cpu"].Equal(expectedVCPUs) { - t.Errorf("Expected vCPUs %v, got %v", expectedVCPUs, res.Spec.Resources["cpu"]) + if !res.Spec.Resources[hv1.ResourceCPU].Equal(expectedVCPUs) { + t.Errorf("Expected vCPUs %v, got %v", expectedVCPUs, res.Spec.Resources[hv1.ResourceCPU]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/syncer_test.go` around lines 297 - 304, The test uses string literals "memory" and "cpu" to index res.Spec.Resources which is inconsistent with the rest of the codebase; replace those map accesses with the hv1.ResourceName constants hv1.ResourceMemory and hv1.ResourceCPU respectively (e.g., change res.Spec.Resources["memory"] -> res.Spec.Resources[hv1.ResourceMemory] and res.Spec.Resources["cpu"] -> res.Spec.Resources[hv1.ResourceCPU]) so the assertions compare expectedMemory and expectedVCPUs against the canonical keys.internal/scheduling/reservations/commitments/reservation_manager_test.go (3)
68-69: Inconsistent map key access: usehv1.ResourceMemoryinstead of string literal.The test setup uses
hv1.ResourceMemoryas the map key (lines 93-95), but the assertion accesses the map with the string literal"memory". For consistency with the PR objective of usinghv1.ResourceNamethroughout, update the access to use the typed constant.♻️ Suggested fix
- memQuantity := res.Spec.Resources["memory"] + memQuantity := res.Spec.Resources[hv1.ResourceMemory]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/reservation_manager_test.go` around lines 68 - 69, Update the map access in the test so it uses the typed resource name constant rather than a string literal: replace the `"memory"` key lookup on res.Spec.Resources with hv1.ResourceMemory (keeping use of memQuantity and totalMemory accumulation unchanged) so the test consistently uses hv1.ResourceName constants for resource map access.
534-535: Usehv1.ResourceCPUfor map access instead of string literal.♻️ Suggested fix
- cpuQuantity := reservation.Spec.Resources["cpu"] + cpuQuantity := reservation.Spec.Resources[hv1.ResourceCPU]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/reservation_manager_test.go` around lines 534 - 535, Replace the string literal key "cpu" when accessing the resource map with the constant hv1.ResourceCPU to avoid hardcoded keys; update the access in reservation.Spec.Resources (where cpuQuantity is assigned) to use hv1.ResourceCPU and keep the subsequent comparison to tt.expectedCores unchanged so the test reads cpuQuantity := reservation.Spec.Resources[hv1.ResourceCPU] followed by the existing if cpuQuantity.Value() != tt.expectedCores check.
172-173: Same inconsistency: usehv1.ResourceMemoryfor map access.♻️ Suggested fix
- memQuantity := res.Spec.Resources["memory"] + memQuantity := res.Spec.Resources[hv1.ResourceMemory]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/reservation_manager_test.go` around lines 172 - 173, The test is indexing the resource map with the literal string "memory"; change that to use the constant hv1.ResourceMemory when accessing res.Spec.Resources (i.e., replace the map key "memory" with hv1.ResourceMemory) so memQuantity is retrieved consistently; ensure the hv1 package is imported/aliased in the test file if not already and keep the subsequent totalMemory += memQuantity.Value() logic unchanged.internal/scheduling/reservations/commitments/reservation_manager.go (4)
157-158: Usehv1.ResourceMemoryfor map access.♻️ Suggested fix
- memValue := reservation.Spec.Resources["memory"] + memValue := reservation.Spec.Resources[hv1.ResourceMemory]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/reservation_manager.go` around lines 157 - 158, The code is accessing reservation.Spec.Resources with the literal key "memory"; change that to use the hv1.ResourceMemory constant (e.g., memValue := reservation.Spec.Resources[hv1.ResourceMemory]) and update imports if needed so hv1 is available; keep the subsequent use (deltaMemoryBytes -= memValue.Value()) the same.
108-109: Usehv1.ResourceMemoryfor map access.♻️ Suggested fix
- memValue := res.Spec.Resources["memory"] + memValue := res.Spec.Resources[hv1.ResourceMemory]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/reservation_manager.go` around lines 108 - 109, The code is using the string key "memory" to access res.Spec.Resources; change it to use the typed constant hv1.ResourceMemory when reading the resource map (replace res.Spec.Resources["memory"] with res.Spec.Resources[hv1.ResourceMemory]) and ensure the hv1 package is imported; keep the rest of the logic updating deltaMemoryBytes with memValue.Value() unchanged (variables: res.Spec.Resources, memValue, deltaMemoryBytes).
81-82: Inconsistent map key access: usehv1.ResourceMemoryinstead of string literal.The newReservation function (lines 269-278) now uses
hv1.ResourceMemoryto create the Resources map, but this read access still uses the string literal"memory". Update for consistency with the PR objective.♻️ Suggested fix
- memoryQuantity := res.Spec.Resources["memory"] + memoryQuantity := res.Spec.Resources[hv1.ResourceMemory]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/reservation_manager.go` around lines 81 - 82, Replace the string literal map key "memory" with the hv1.ResourceMemory constant when reading from res.Spec.Resources (the lines that set memoryQuantity and adjust deltaMemoryBytes). Specifically, change the access to use res.Spec.Resources[hv1.ResourceMemory] so it matches how newReservation builds the Resources map; ensure hv1 is imported/available in this file and that memoryQuantity and deltaMemoryBytes usage remains unchanged.
136-137: Usehv1.ResourceMemoryfor map access.♻️ Suggested fix
- memValue := reservationToDelete.Spec.Resources["memory"] + memValue := reservationToDelete.Spec.Resources[hv1.ResourceMemory]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/reservation_manager.go` around lines 136 - 137, Replace the string key "memory" with the typed constant hv1.ResourceMemory when accessing reservationToDelete.Spec.Resources (i.e., use reservationToDelete.Spec.Resources[hv1.ResourceMemory]); ensure you check the map lookup result (ok, memValue := reservationToDelete.Spec.Resources[hv1.ResourceMemory]; if ok { deltaMemoryBytes += memValue.Value() }) to avoid nil/zero-value panics and keep the update to deltaMemoryBytes using the retrieved resource.Quantity; reference: reservationToDelete.Spec.Resources, hv1.ResourceMemory, memValue, deltaMemoryBytes.
🤖 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/scheduling/reservations/commitments/reservation_manager_test.go`:
- Around line 68-69: Update the map access in the test so it uses the typed
resource name constant rather than a string literal: replace the `"memory"` key
lookup on res.Spec.Resources with hv1.ResourceMemory (keeping use of memQuantity
and totalMemory accumulation unchanged) so the test consistently uses
hv1.ResourceName constants for resource map access.
- Around line 534-535: Replace the string literal key "cpu" when accessing the
resource map with the constant hv1.ResourceCPU to avoid hardcoded keys; update
the access in reservation.Spec.Resources (where cpuQuantity is assigned) to use
hv1.ResourceCPU and keep the subsequent comparison to tt.expectedCores unchanged
so the test reads cpuQuantity := reservation.Spec.Resources[hv1.ResourceCPU]
followed by the existing if cpuQuantity.Value() != tt.expectedCores check.
- Around line 172-173: The test is indexing the resource map with the literal
string "memory"; change that to use the constant hv1.ResourceMemory when
accessing res.Spec.Resources (i.e., replace the map key "memory" with
hv1.ResourceMemory) so memQuantity is retrieved consistently; ensure the hv1
package is imported/aliased in the test file if not already and keep the
subsequent totalMemory += memQuantity.Value() logic unchanged.
In `@internal/scheduling/reservations/commitments/reservation_manager.go`:
- Around line 157-158: The code is accessing reservation.Spec.Resources with the
literal key "memory"; change that to use the hv1.ResourceMemory constant (e.g.,
memValue := reservation.Spec.Resources[hv1.ResourceMemory]) and update imports
if needed so hv1 is available; keep the subsequent use (deltaMemoryBytes -=
memValue.Value()) the same.
- Around line 108-109: The code is using the string key "memory" to access
res.Spec.Resources; change it to use the typed constant hv1.ResourceMemory when
reading the resource map (replace res.Spec.Resources["memory"] with
res.Spec.Resources[hv1.ResourceMemory]) and ensure the hv1 package is imported;
keep the rest of the logic updating deltaMemoryBytes with memValue.Value()
unchanged (variables: res.Spec.Resources, memValue, deltaMemoryBytes).
- Around line 81-82: Replace the string literal map key "memory" with the
hv1.ResourceMemory constant when reading from res.Spec.Resources (the lines that
set memoryQuantity and adjust deltaMemoryBytes). Specifically, change the access
to use res.Spec.Resources[hv1.ResourceMemory] so it matches how newReservation
builds the Resources map; ensure hv1 is imported/available in this file and that
memoryQuantity and deltaMemoryBytes usage remains unchanged.
- Around line 136-137: Replace the string key "memory" with the typed constant
hv1.ResourceMemory when accessing reservationToDelete.Spec.Resources (i.e., use
reservationToDelete.Spec.Resources[hv1.ResourceMemory]); ensure you check the
map lookup result (ok, memValue :=
reservationToDelete.Spec.Resources[hv1.ResourceMemory]; if ok { deltaMemoryBytes
+= memValue.Value() }) to avoid nil/zero-value panics and keep the update to
deltaMemoryBytes using the retrieved resource.Quantity; reference:
reservationToDelete.Spec.Resources, hv1.ResourceMemory, memValue,
deltaMemoryBytes.
In `@internal/scheduling/reservations/commitments/syncer_test.go`:
- Around line 297-304: The test uses string literals "memory" and "cpu" to index
res.Spec.Resources which is inconsistent with the rest of the codebase; replace
those map accesses with the hv1.ResourceName constants hv1.ResourceMemory and
hv1.ResourceCPU respectively (e.g., change res.Spec.Resources["memory"] ->
res.Spec.Resources[hv1.ResourceMemory] and res.Spec.Resources["cpu"] ->
res.Spec.Resources[hv1.ResourceCPU]) so the assertions compare expectedMemory
and expectedVCPUs against the canonical keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 493676ed-5991-4638-8bff-da3564407af2
📒 Files selected for processing (16)
api/v1alpha1/reservation_types.goapi/v1alpha1/zz_generated.deepcopy.gointernal/knowledge/kpis/plugins/compute/resource_capacity_kvm_test.gointernal/scheduling/nova/integration_test.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.gointernal/scheduling/nova/plugins/weighers/kvm_binpack_test.gointernal/scheduling/nova/plugins/weighers/kvm_failover_evacuation_test.gointernal/scheduling/nova/plugins/weighers/kvm_prefer_smaller_hosts_test.gointernal/scheduling/reservations/commitments/reservation_manager.gointernal/scheduling/reservations/commitments/reservation_manager_test.gointernal/scheduling/reservations/commitments/state_test.gointernal/scheduling/reservations/commitments/syncer_test.gointernal/scheduling/reservations/controller/controller_test.gointernal/scheduling/reservations/controller/monitor.gointernal/scheduling/reservations/controller/monitor_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/knowledge/kpis/plugins/compute/resource_capacity_kvm_test.go
- internal/scheduling/nova/integration_test.go
- internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go
Contributor
Test Coverage ReportTest Coverage 📊: 67.9% |
umswmayj
approved these changes
Mar 16, 2026
This was referenced Mar 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
3 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.
This upgrade is necessary, because we changed the allocation and capacity types from a generic
map[string]resource.Quantitytomap[hv1.ResourceName]resource.Quantity. In addition, this upgrade also makes use of thehv1.ResourceNameapi inside our Reservation CRD.See: cobaltcore-dev/openstack-hypervisor-operator#257
Summary by CodeRabbit
Refactor
Chores