refactor(reservations): extract shared ResourcesToBlock to replace duplicated reservation blocking logic#896
Merged
Conversation
juliusclausnitzer
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
mblos and
umswmayj
as code owners
|
Warning Review limit reached
More reviews will be available in 58 minutes and 48 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 (2)
📝 WalkthroughWalkthroughReservation blocking is centralized into a new accounting function UnusedReservationCapacity; the filter delegates blocked-resource computation to it, and the reservations capacity controller precomputes per-host blocked memory and subtracts those blocks during placeable-capacity probing. ChangesCentralize reservation capacity blocking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
|
Actionable comments posted: 0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go (1)
202-210: 💤 Low valueOptional: trim the duplicated blocking-math comment now that it lives in the helper.
The delegation is correct, but Lines 202-209 re-derive the exact confirmed/spec-only/clamping algorithm that's already documented on
UnusedReservationCapacity. Keeping the derivation in two places is the divergence trap this refactor is meant to remove; a short pointer keeps a single source of truth.♻️ Proposed comment trim
- // For CR reservations with allocations, compute the effective block: - // confirmed = sum of resources for VMs present in both Spec and Status allocations - // specOnly = sum of resources for VMs present in Spec but not yet in Status - // remaining = max(0, Spec.Resources - confirmed) [clamped: never negative] - // block = max(remaining, specOnly) [spec-only VM must be fully covered] - // - // Clamping: if confirmed VMs exceed slot size (e.g. after resize), block = 0. - // Oversize spec-only: if a pending VM is larger than the remaining slot, block its full size. - resourcesToBlock := resv.UnusedReservationCapacity(&reservation, s.Options.IgnoreAllocations) + // Per-resource blocking math is centralized in reservations.UnusedReservationCapacity + // (confirmed vs spec-only, clamping, migration/ignoreAllocations handling). + resourcesToBlock := resv.UnusedReservationCapacity(&reservation, s.Options.IgnoreAllocations)🤖 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/nova/plugins/filters/filter_has_enough_capacity.go` around lines 202 - 210, The long duplicated algorithm comment above the call to UnusedReservationCapacity should be removed and replaced with a short pointer to the helper; delete the multi-line derivation (confirmed/specOnly/remaining/block/clamping) and leave a one-line note like "Compute resources to block using UnusedReservationCapacity(reservation, s.Options.IgnoreAllocations)" so the single source of truth stays in the UnusedReservationCapacity implementation referenced by resourcesToBlock.
🤖 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.
Nitpick comments:
In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go`:
- Around line 202-210: The long duplicated algorithm comment above the call to
UnusedReservationCapacity should be removed and replaced with a short pointer to
the helper; delete the multi-line derivation
(confirmed/specOnly/remaining/block/clamping) and leave a one-line note like
"Compute resources to block using UnusedReservationCapacity(reservation,
s.Options.IgnoreAllocations)" so the single source of truth stays in the
UnusedReservationCapacity implementation referenced by resourcesToBlock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6f4baf9-61e1-4982-aad6-443e2ebe5f23
📒 Files selected for processing (4)
internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.gointernal/scheduling/reservations/capacity/controller.gointernal/scheduling/reservations/capacity_accounting.gointernal/scheduling/reservations/capacity_accounting_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/scheduling/reservations/capacity/controller.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/scheduling/reservations/capacity_accounting.go (1)
71-75: ⚡ Quick wininternal/scheduling/reservations/capacity_accounting.go:71-75 — drop the redundant
elseand broaden the bypass comment
golangci-lintonly enablesgocriticwith the specific checks listed in.golangci.yaml(none coverelse-after-return/unnecessaryElse), so this is purely readability/clarity: the inline comment should cover every bypass path that returnsres.Spec.Resources, not just failover.♻️ Proposed restructure
return result - } else { - // FailoverReservations: protected VMs run on their primary host, not the backup host, - // so they do not appear in hv.Status.Allocation on the backup — block the full slot. - return res.Spec.Resources } + // All remaining cases block the full slot on every host: failover reservations, + // ignoreAllocations, mid-migration (TargetHost != Status.Host), or CRs without + // committed allocations. (e.g. a failover's protected VMs run on the primary host, + // not the backup, so they never appear in hv.Status.Allocation there.) + return res.Spec.Resources }🤖 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/capacity_accounting.go` around lines 71 - 75, Remove the redundant else after the earlier return and move/broaden the inline comment so it precedes the return of res.Spec.Resources and documents all bypass cases that should block the slot (e.g., FailoverReservations / protected VMs running on their primary host and any other conditions that cause a full-slot bypass); specifically edit the block that currently returns res.Spec.Resources (referencing res.Spec.Resources) to eliminate the unnecessary "else" and expand the comment to explain every bypass path that justifies returning the full resource spec.
🤖 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.
Nitpick comments:
In `@internal/scheduling/reservations/capacity_accounting.go`:
- Around line 71-75: Remove the redundant else after the earlier return and
move/broaden the inline comment so it precedes the return of res.Spec.Resources
and documents all bypass cases that should block the slot (e.g.,
FailoverReservations / protected VMs running on their primary host and any other
conditions that cause a full-slot bypass); specifically edit the block that
currently returns res.Spec.Resources (referencing res.Spec.Resources) to
eliminate the unnecessary "else" and expand the comment to explain every bypass
path that justifies returning the full resource spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a3d2dcc-3505-4849-9957-9e6bc3750faa
📒 Files selected for processing (1)
internal/scheduling/reservations/capacity_accounting.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.6% |
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.
Refactor: Extract shared
ResourcesToBlockto eliminate duplicationThe resource-blocking logic for Reservations was duplicated between
filter_has_enough_capacityand theFlavorGroupCapacitycapacitycontroller. Any future change to blocking semantics would need to be
applied in both places with no enforcement of consistency.
This PR extracts the logic into
reservations.ResourcesToBlock(res, ignoreAllocations)as the single source of truth, and updates bothconsumers to use it.