refactor(reservations): move VMSource to shared package, unify VM data layer#930
Open
juliusclausnitzer wants to merge 12 commits into
Open
refactor(reservations): move VMSource to shared package, unify VM data layer#930juliusclausnitzer wants to merge 12 commits into
juliusclausnitzer wants to merge 12 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy 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 VM data access across the scheduling system by introducing a shared ChangesVM source abstraction and controller adoption
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
juliusclausnitzer
changed the title
juliusclausnitzer
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
mblos and
umswmayj
as code owners
![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/scheduling/reservations/vm_source.go (1)
99-108: 💤 Low valueConsider extracting the duplicated
flavorDatastruct.The
flavorDatastruct is defined identically in bothListVMs(lines 99-108) andListVMsByProject(lines 169-178). Consider extracting it to a package-level unexported type for consistency.Also applies to: 169-178
🤖 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/scheduling/reservations/vm_source.go` around lines 99 - 108, The file defines an identical local struct flavorData in both ListVMs and ListVMsByProject; extract that duplicate into a single package-level unexported type (e.g., type flavorData struct { VCPUs uint64; RAM uint64; Disk uint64; ExtraSpecs string }) and update both functions to use the shared flavorData type and existing maps like flavorByName to remove the repeated definition while preserving current field names and usage.internal/scheduling/reservations/quota/controller_test.go (1)
587-589: ⚡ Quick winMake
ListVMsByProjectconfigurable like the other mock methods.Right now this always returns
nil, nil, which can hide accidental call paths in tests. Adding an injectable function keeps failures explicit and consistent with the existing mock design.💡 Suggested patch
type mockVMSource struct { listVMs func(ctx context.Context) ([]failover.VM, error) + listVMsByProject func(ctx context.Context, projectID string) ([]failover.VM, error) getVM func(ctx context.Context, vmUUID string) (*failover.VM, error) isServerActive func(ctx context.Context, vmUUID string) (bool, error) getDeletedVM func(ctx context.Context, vmUUID string) (*failover.DeletedVMInfo, error) } func (m *mockVMSource) ListVMsByProject(_ context.Context, _ string) ([]failover.VM, error) { + if m.listVMsByProject != nil { + return m.listVMsByProject(ctx, projectID) + } return nil, nil }🤖 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/scheduling/reservations/quota/controller_test.go` around lines 587 - 589, The mock method ListVMsByProject should be made configurable like the other mocks: add a field on mockVMSource (e.g. ListVMsByProjectFunc or ListVMsByProjectFn of type func(context.Context, string) ([]failover.VM, error)) and change the ListVMsByProject method to call that function when non-nil; if the func is nil, fail fast (panic or return a clear error) so tests don't silently hide unexpected calls. Update any test setups to set mockVMSource.ListVMsByProjectFunc as needed.
🤖 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/scheduling/reservations/commitments/api/report_usage_test.go`:
- Around line 629-630: The two explicit int64 conversions around
vm.Flavor.MemoryMB and vm.Flavor.VCPUs are redundant and causing the unconvert
check to fail; edit the resource.NewQuantity calls in report_usage_test.go (the
lines constructing the "memory" and "vcpus" quantities) to remove the int64(...)
casts and pass vm.Flavor.MemoryMB and vm.Flavor.VCPUs directly (keeping the same
multipliers and resource.BinarySI/resource.DecimalSI arguments) so the types
match without unnecessary conversion.
In `@internal/scheduling/reservations/commitments/usage.go`:
- Around line 247-249: The error messages using "VM" / "VMs" need to be
lowercased to satisfy the repository lint rule; locate the checks that return
errors when vmSource is nil (reference the vmSource variable in usage.go) and
replace the error strings so they use lowercase "vm" / "vms" (e.g. change the
message returned by errors.New(...) where vmSource is validated and the similar
message at the other occurrence) ensuring punctuation and wording remain
otherwise identical.
---
Nitpick comments:
In `@internal/scheduling/reservations/quota/controller_test.go`:
- Around line 587-589: The mock method ListVMsByProject should be made
configurable like the other mocks: add a field on mockVMSource (e.g.
ListVMsByProjectFunc or ListVMsByProjectFn of type func(context.Context, string)
([]failover.VM, error)) and change the ListVMsByProject method to call that
function when non-nil; if the func is nil, fail fast (panic or return a clear
error) so tests don't silently hide unexpected calls. Update any test setups to
set mockVMSource.ListVMsByProjectFunc as needed.
In `@internal/scheduling/reservations/vm_source.go`:
- Around line 99-108: The file defines an identical local struct flavorData in
both ListVMs and ListVMsByProject; extract that duplicate into a single
package-level unexported type (e.g., type flavorData struct { VCPUs uint64; RAM
uint64; Disk uint64; ExtraSpecs string }) and update both functions to use the
shared flavorData type and existing maps like flavorByName to remove the
repeated definition while preserving current field names and usage.
🪄 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: 7d4e2b92-44c5-46d5-b20b-ee145378beba
📒 Files selected for processing (15)
cmd/manager/main.gointernal/scheduling/external/nova.gointernal/scheduling/reservations/commitments/api/handler.gointernal/scheduling/reservations/commitments/api/report_usage_test.gointernal/scheduling/reservations/commitments/api/usage_test.gointernal/scheduling/reservations/commitments/usage.gointernal/scheduling/reservations/commitments/usage_reconciler.gointernal/scheduling/reservations/failover/controller_test.gointernal/scheduling/reservations/failover/integration_test.gointernal/scheduling/reservations/failover/vm_source.gointernal/scheduling/reservations/hypervisor_diff_handler.gointernal/scheduling/reservations/quota/controller.gointernal/scheduling/reservations/quota/controller_test.gointernal/scheduling/reservations/vm_source.gointernal/scheduling/reservations/vm_source_test.go
💤 Files with no reviewable changes (1)
- internal/scheduling/reservations/failover/controller_test.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.3% |
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/scheduling/reservations/vm_source.go (1)
488-493:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIndividual VM warnings may flood logs in production.
This logs at
Infolevel for every VM on a hypervisor that isn't found inListVMs. If there are many such VMs (e.g., during a data sync delay), this could generate excessive log output. The summary at lines 506-513 already captures the count atV(1).Consider raising the per-VM log to
V(1)orV(2)to match the summary logging level:Suggested change
if inst.Active && !vmUUIDs[inst.ID] { - vmSourceLog.Info("WARNING: VM on hypervisor not found in ListVMs - possible data sync issue", + vmSourceLog.V(1).Info("VM on hypervisor not found in ListVMs - possible data sync issue", "vmUUID", inst.ID, "vmName", inst.Name, "hypervisor", hv.Name)🤖 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/scheduling/reservations/vm_source.go` around lines 488 - 493, The per-VM log currently emitted by vmSourceLog.Info when an active instance (inst.Active && !vmUUIDs[inst.ID]) is missing from ListVMs can flood production logs; change the call to a verbosity-level log such as vmSourceLog.V(1).Info (or V(2) if quieter) so individual VM warnings are emitted only at the appropriate debug verbosity while retaining the same message fields ("vmUUID", inst.ID, "vmName", inst.Name, "hypervisor", hv.Name) and leaving the summary counter vmsOnHypervisorsNotInListVMs and the summary V(1) log unchanged.
🤖 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/scheduling/reservations/vm_source.go`:
- Around line 488-493: The per-VM log currently emitted by vmSourceLog.Info when
an active instance (inst.Active && !vmUUIDs[inst.ID]) is missing from ListVMs
can flood production logs; change the call to a verbosity-level log such as
vmSourceLog.V(1).Info (or V(2) if quieter) so individual VM warnings are emitted
only at the appropriate debug verbosity while retaining the same message fields
("vmUUID", inst.ID, "vmName", inst.Name, "hypervisor", hv.Name) and leaving the
summary counter vmsOnHypervisorsNotInListVMs and the summary V(1) log unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1e37428-a96c-4857-b550-1303d355a459
📒 Files selected for processing (14)
cmd/manager/main.gointernal/scheduling/reservations/commitments/api/report_usage_test.gointernal/scheduling/reservations/commitments/usage.gointernal/scheduling/reservations/failover/controller.gointernal/scheduling/reservations/failover/controller_test.gointernal/scheduling/reservations/failover/helpers.gointernal/scheduling/reservations/failover/integration_test.gointernal/scheduling/reservations/failover/reservation_eligibility.gointernal/scheduling/reservations/failover/reservation_eligibility_test.gointernal/scheduling/reservations/failover/reservation_scheduling.gointernal/scheduling/reservations/failover/reservation_scheduling_test.gointernal/scheduling/reservations/quota/controller_test.gointernal/scheduling/reservations/quota/integration_test.gointernal/scheduling/reservations/vm_source.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/manager/main.go
- internal/scheduling/reservations/commitments/usage.go
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.
No description provided.