feat: CR controller checks for host overload scenario and resolves via reservation re-placements - #1125
Open
mblos wants to merge 2 commits into
Open
feat: CR controller checks for host overload scenario and resolves via reservation re-placements#1125mblos wants to merge 2 commits into
mblos wants to merge 2 commits into
Conversation
…a reservation re-placements Signed-off-by: Marcel <156897072+mblos@users.noreply.github.com>
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
August 7, 2026 14:32
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe committed-resource reservation controller now calculates host free capacity, detects and remediates host oversubscription, indexes reservations by host, exposes Prometheus metrics, and adds deployment configuration, alerting, and tests. ChangesHost reservation oversubscription
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 8
🧹 Nitpick comments (3)
internal/scheduling/reservations/commitments/committed_resource_controller_test.go (1)
141-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the host index function instead of copying it.
This closure duplicates the index logic in
internal/scheduling/reservations/field_index.go(lines 34-52). If the production indexer changes, this copy will not change and the tests will pass against different index semantics.Export the extractor from the
reservationspackage and use it in both places.♻️ Proposed refactor
In
internal/scheduling/reservations/field_index.go:// ReservationHostIndexValues returns the host keys for IdxReservationByHost. func ReservationHostIndexValues(obj client.Object) []string { res, ok := obj.(*v1alpha1.Reservation) if !ok { return nil } hosts := make([]string, 0, 2) if res.Spec.TargetHost != "" { hosts = append(hosts, res.Spec.TargetHost) } if res.Status.Host != "" && res.Status.Host != res.Spec.TargetHost { hosts = append(hosts, res.Status.Host) } return hosts }Then in this file:
- WithIndex(&v1alpha1.Reservation{}, reservations.IdxReservationByHost, func(obj client.Object) []string { - res, ok := obj.(*v1alpha1.Reservation) - if !ok { - return nil - } - hosts := make(map[string]struct{}) - if res.Spec.TargetHost != "" { - hosts[res.Spec.TargetHost] = struct{}{} - } - if res.Status.Host != "" { - hosts[res.Status.Host] = struct{}{} - } - result := make([]string, 0, len(hosts)) - for h := range hosts { - result = append(result, h) - } - return result - }). + WithIndex(&v1alpha1.Reservation{}, reservations.IdxReservationByHost, reservations.ReservationHostIndexValues).🤖 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/commitments/committed_resource_controller_test.go` around lines 141 - 158, Export the reservation host extractor from field_index.go as ReservationHostIndexValues, preserving the production behavior of indexing unique non-empty target and status hosts. Replace the duplicated WithIndex closure in the test with reservations.ReservationHostIndexValues so tests and production share the same index semantics.internal/scheduling/reservations/commitments/reservation_controller.go (2)
1097-1103: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftEviction picks the smallest slot, so convergence is slow.
Both candidate slices are sorted ascending by memory at lines 1075-1086, and line 1103 takes
candidates[0]. The controller therefore evicts the smallest slot first, which frees the least capacity. A host that is over-subscribed by 1 TiB and holds four 256 GiB slots needs four eviction rounds, each separated by a full grace period.Select the smallest slot that covers the violation, and fall back to the largest slot when no single slot covers it.
🤖 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/commitments/reservation_controller.go` around lines 1097 - 1103, Update the eviction selection logic in the reservation controller around candidates and target so it chooses the smallest reservation slot whose memory covers the current over-subscription, using the existing ascending ordering; if none covers the violation, select the largest candidate instead of candidates[0].
921-922: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftThe mutex is held across remote API calls.
The field comment at line 61 states that the mutex protects three maps. This
defered lock holds it for the whole function, which includes theGetat line 953, theListat line 958, andcheckHostOversubscription, which issues twoPatchcalls and oneGetinsideunplaceReservation.
MaxConcurrentReconcilesis 1 today, so there is no contention. If that value is ever raised, every reconcile serializes behind these network calls.Narrow the critical section to the map accesses.
🤖 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/commitments/reservation_controller.go` around lines 921 - 922, In the reservation reconciliation function, replace the function-wide oversubscriptionMu lock around the remote Get/List and checkHostOversubscription calls with short critical sections that lock only while reading or updating the protected maps. Ensure all map accesses remain synchronized, but release the mutex before any remote API calls or unplaceReservation network operations.
🤖 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 `@helm/bundles/cortex-nova/templates/alerts.yaml`:
- Around line 780-784: Update the alert description for the host reservation
capacity rule to use the resource-agnostic humanize formatter instead of
humanize1024 when rendering $value. Keep the existing compute_host, resource,
and capacity wording unchanged.
In `@internal/scheduling/reservations/commitments/reservation_controller_test.go`:
- Around line 1377-1384: Update the eviction assertion in the reservation test
instead of assuming slot-1 is selected from the equal-memory candidates.
Retrieve all three slots and assert that exactly one has an empty
Spec.TargetHost, while preserving the existing validation that the eviction
clears the target host.
In `@internal/scheduling/reservations/commitments/reservation_controller.go`:
- Around line 1136-1155: The unplaceReservation update order can leave a ready
reservation counted on its old host when the status patch fails. Apply the
status changes clearing Status.Host and setting Ready=False before clearing
Spec.TargetHost, preserving the existing rollback-safe behavior, and update
runOversubscriptionCheck to return the unplaceReservation error so
reconciliation retries instead of returning success.
- Around line 1031-1038: Update the violation-handling flow in the reservation
controller to call monitor.ClearHost(host, az) before processing violations on
every run, not only when len(violations) == 0. Then set gauges for the current
entries in violations and retain the existing return behavior for empty and
non-empty violation sets.
- Around line 926-950: Decouple the oversubscription rate limit in the
reservation controller from RequeueIntervalActive by introducing or reusing a
dedicated minimum check interval appropriate for the grace-period flow. Update
the timeSinceLastCheck branch to always return a positive requeue duration,
including when oversubscriptionPendingCheck[host] is already true, so
rate-limited hosts are reliably rechecked without dropping the trigger.
- Around line 57-59: Update the Monitor field documentation to accurately state
that a nil monitor disables the over-subscription check, detection, and
eviction, while retaining the existing nil guard in checkHostOversubscription
and its callers.
- Around line 836-853: Update hvCapacityChangePredicate’s UpdateFunc to compare
oldHV.Status.Capacity with newHV.Status.Capacity alongside Instances,
Allocation, and EffectiveCapacity, so capacity-only changes trigger
reconciliation.
- Around line 1062-1095: The fallback loop over allocatedReservations must not
select allocation-bearing committed-resource reservations for eviction, because
unplaceReservation clears required VM allocation mappings. Remove that fallback
selection and restrict eviction to unallocatedReservations; when no eligible
unallocated slot exists, report the memory over-subscription as unresolvable
through the existing monitor path.
---
Nitpick comments:
In
`@internal/scheduling/reservations/commitments/committed_resource_controller_test.go`:
- Around line 141-158: Export the reservation host extractor from field_index.go
as ReservationHostIndexValues, preserving the production behavior of indexing
unique non-empty target and status hosts. Replace the duplicated WithIndex
closure in the test with reservations.ReservationHostIndexValues so tests and
production share the same index semantics.
In `@internal/scheduling/reservations/commitments/reservation_controller.go`:
- Around line 1097-1103: Update the eviction selection logic in the reservation
controller around candidates and target so it chooses the smallest reservation
slot whose memory covers the current over-subscription, using the existing
ascending ordering; if none covers the violation, select the largest candidate
instead of candidates[0].
- Around line 921-922: In the reservation reconciliation function, replace the
function-wide oversubscriptionMu lock around the remote Get/List and
checkHostOversubscription calls with short critical sections that lock only
while reading or updating the protected maps. Ensure all map accesses remain
synchronized, but release the mutex before any remote API calls or
unplaceReservation network operations.
🪄 Autofix
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 Plus
Run ID: 52168470-26ed-42dc-856d-c2baae0f4b0a
📒 Files selected for processing (12)
cmd/manager/main.gohelm/bundles/cortex-nova/templates/alerts.yamlhelm/bundles/cortex-nova/values.yamlinternal/scheduling/reservations/capacity_accounting.gointernal/scheduling/reservations/capacity_accounting_test.gointernal/scheduling/reservations/commitments/committed_resource_controller_test.gointernal/scheduling/reservations/commitments/config.gointernal/scheduling/reservations/commitments/field_index.gointernal/scheduling/reservations/commitments/reservation_controller.gointernal/scheduling/reservations/commitments/reservation_controller_monitor.gointernal/scheduling/reservations/commitments/reservation_controller_test.gointernal/scheduling/reservations/field_index.go
💤 Files with no reviewable changes (1)
- internal/scheduling/reservations/commitments/field_index.go
Comment on lines
+780
to
+784
| summary: "Host {{ "{{" }} $labels.compute_host {{ "}}" }} reservation blocks exceed capacity for {{ "{{" }} $labels.resource {{ "}}" }}" | ||
| description: > | ||
| The total of running VM allocations and reservation blocks (committed resource + | ||
| failover) on host {{ "{{" }} $labels.compute_host {{ "}}" }} exceeds its effective | ||
| capacity for {{ "{{" }} $labels.resource {{ "}}" }} by {{ "{{" }} $value | humanize1024 {{ "}}" }}. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
humanize1024 is wrong for non-memory resources.
The alert fires per resource. For memory the binary formatting is correct. For cpu the value is a core count, and humanize1024 renders it with binary prefixes (for example 1.5Ki cores). Use humanize for a resource-agnostic annotation.
🔤 Proposed fix
- capacity for {{ "{{" }} $labels.resource {{ "}}" }} by {{ "{{" }} $value | humanize1024 {{ "}}" }}.
+ capacity for {{ "{{" }} $labels.resource {{ "}}" }} by {{ "{{" }} $value | humanize {{ "}}" }}.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| summary: "Host {{ "{{" }} $labels.compute_host {{ "}}" }} reservation blocks exceed capacity for {{ "{{" }} $labels.resource {{ "}}" }}" | |
| description: > | |
| The total of running VM allocations and reservation blocks (committed resource + | |
| failover) on host {{ "{{" }} $labels.compute_host {{ "}}" }} exceeds its effective | |
| capacity for {{ "{{" }} $labels.resource {{ "}}" }} by {{ "{{" }} $value | humanize1024 {{ "}}" }}. | |
| summary: "Host {{ "{{" }} $labels.compute_host {{ "}}" }} reservation blocks exceed capacity for {{ "{{" }} $labels.resource {{ "}}" }}" | |
| description: > | |
| The total of running VM allocations and reservation blocks (committed resource + | |
| failover) on host {{ "{{" }} $labels.compute_host {{ "}}" }} exceeds its effective | |
| capacity for {{ "{{" }} $labels.resource {{ "}}" }} by {{ "{{" }} $value | humanize {{ "}}" }}. |
🤖 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 `@helm/bundles/cortex-nova/templates/alerts.yaml` around lines 780 - 784,
Update the alert description for the host reservation capacity rule to use the
resource-agnostic humanize formatter instead of humanize1024 when rendering
$value. Keep the existing compute_host, resource, and capacity wording
unchanged.
Comment on lines
+57
to
+59
| // Monitor reports over-subscription violations as Prometheus metrics. | ||
| // Nil disables metric reporting (check still runs, only logging). | ||
| Monitor *ReservationControllerMonitor |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The doc comment contradicts the code.
The comment states that the check still runs when Monitor is nil. Line 917 returns early when r.Monitor == nil, so the whole over-subscription feature is disabled, including detection and eviction. checkHostOversubscription also dereferences monitor at lines 1032 and 1037, so the guard is required to avoid a nil-pointer panic.
Correct the comment, or make the monitor calls nil-safe so the documented behavior holds.
🔤 Proposed comment fix
- // Monitor reports over-subscription violations as Prometheus metrics.
- // Nil disables metric reporting (check still runs, only logging).
+ // Monitor reports over-subscription violations as Prometheus metrics.
+ // Nil disables the over-subscription check entirely.
Monitor *ReservationControllerMonitor📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Monitor reports over-subscription violations as Prometheus metrics. | |
| // Nil disables metric reporting (check still runs, only logging). | |
| Monitor *ReservationControllerMonitor | |
| // Monitor reports over-subscription violations as Prometheus metrics. | |
| // Nil disables the over-subscription check entirely. | |
| Monitor *ReservationControllerMonitor |
🤖 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/commitments/reservation_controller.go`
around lines 57 - 59, Update the Monitor field documentation to accurately state
that a nil monitor disables the over-subscription check, detection, and
eviction, while retaining the existing nil guard in checkHostOversubscription
and its callers.
Comment on lines
+836
to
+853
| // hvCapacityChangePredicate fires when Status.Instances, Status.Allocation, or | ||
| // Status.EffectiveCapacity changes on a Hypervisor. Instances covers VM presence | ||
| // (used by allocation verification); Allocation and EffectiveCapacity cover capacity | ||
| // accounting (used by the over-subscription check). | ||
| var hvCapacityChangePredicate = predicate.Funcs{ | ||
| CreateFunc: func(e event.CreateEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | ||
| DeleteFunc: func(e event.DeleteEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | ||
| GenericFunc: func(e event.GenericEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | ||
| UpdateFunc: func(e event.UpdateEvent) bool { | ||
| oldHV, ok1 := e.ObjectOld.(*hv1.Hypervisor) | ||
| newHV, ok2 := e.ObjectNew.(*hv1.Hypervisor) | ||
| if !ok1 || !ok2 { | ||
| return false | ||
| } | ||
| return !reflect.DeepEqual(oldHV.Status.Instances, newHV.Status.Instances) || | ||
| !reflect.DeepEqual(oldHV.Status.Allocation, newHV.Status.Allocation) || | ||
| !reflect.DeepEqual(oldHV.Status.EffectiveCapacity, newHV.Status.EffectiveCapacity) | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The predicate does not watch Status.Capacity.
reservations.HostFreeCapacity falls back to hv.Status.Capacity when Status.EffectiveCapacity is nil (see internal/scheduling/reservations/capacity_accounting.go lines 84-87). On a hypervisor that reports only Status.Capacity, a capacity change does not trigger this predicate, so the over-subscription check does not run until an unrelated event arrives.
Add Status.Capacity to the comparison.
🐛 Proposed fix
return !reflect.DeepEqual(oldHV.Status.Instances, newHV.Status.Instances) ||
!reflect.DeepEqual(oldHV.Status.Allocation, newHV.Status.Allocation) ||
- !reflect.DeepEqual(oldHV.Status.EffectiveCapacity, newHV.Status.EffectiveCapacity)
+ !reflect.DeepEqual(oldHV.Status.EffectiveCapacity, newHV.Status.EffectiveCapacity) ||
+ !reflect.DeepEqual(oldHV.Status.Capacity, newHV.Status.Capacity)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // hvCapacityChangePredicate fires when Status.Instances, Status.Allocation, or | |
| // Status.EffectiveCapacity changes on a Hypervisor. Instances covers VM presence | |
| // (used by allocation verification); Allocation and EffectiveCapacity cover capacity | |
| // accounting (used by the over-subscription check). | |
| var hvCapacityChangePredicate = predicate.Funcs{ | |
| CreateFunc: func(e event.CreateEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | |
| DeleteFunc: func(e event.DeleteEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | |
| GenericFunc: func(e event.GenericEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | |
| UpdateFunc: func(e event.UpdateEvent) bool { | |
| oldHV, ok1 := e.ObjectOld.(*hv1.Hypervisor) | |
| newHV, ok2 := e.ObjectNew.(*hv1.Hypervisor) | |
| if !ok1 || !ok2 { | |
| return false | |
| } | |
| return !reflect.DeepEqual(oldHV.Status.Instances, newHV.Status.Instances) || | |
| !reflect.DeepEqual(oldHV.Status.Allocation, newHV.Status.Allocation) || | |
| !reflect.DeepEqual(oldHV.Status.EffectiveCapacity, newHV.Status.EffectiveCapacity) | |
| }, | |
| // hvCapacityChangePredicate fires when Status.Instances, Status.Allocation, or | |
| // Status.EffectiveCapacity changes on a Hypervisor. Instances covers VM presence | |
| // (used by allocation verification); Allocation and EffectiveCapacity cover capacity | |
| // accounting (used by the over-subscription check). | |
| var hvCapacityChangePredicate = predicate.Funcs{ | |
| CreateFunc: func(e event.CreateEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | |
| DeleteFunc: func(e event.DeleteEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | |
| GenericFunc: func(e event.GenericEvent) bool { _, ok := e.Object.(*hv1.Hypervisor); return ok }, | |
| UpdateFunc: func(e event.UpdateEvent) bool { | |
| oldHV, ok1 := e.ObjectOld.(*hv1.Hypervisor) | |
| newHV, ok2 := e.ObjectNew.(*hv1.Hypervisor) | |
| if !ok1 || !ok2 { | |
| return false | |
| } | |
| return !reflect.DeepEqual(oldHV.Status.Instances, newHV.Status.Instances) || | |
| !reflect.DeepEqual(oldHV.Status.Allocation, newHV.Status.Allocation) || | |
| !reflect.DeepEqual(oldHV.Status.EffectiveCapacity, newHV.Status.EffectiveCapacity) || | |
| !reflect.DeepEqual(oldHV.Status.Capacity, newHV.Status.Capacity) | |
| }, |
🤖 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/commitments/reservation_controller.go`
around lines 836 - 853, Update hvCapacityChangePredicate’s UpdateFunc to compare
oldHV.Status.Capacity with newHV.Status.Capacity alongside Instances,
Allocation, and EffectiveCapacity, so capacity-only changes trigger
reconciliation.
Comment on lines
+926
to
+950
| gracePeriod := r.Conf.OversubscriptionGracePeriod.Duration | ||
| if gracePeriod == 0 { | ||
| gracePeriod = 2 * time.Minute | ||
| } | ||
| minCheckInterval := r.Conf.RequeueIntervalActive.Duration | ||
| if minCheckInterval == 0 { | ||
| minCheckInterval = 30 * time.Second | ||
| } | ||
|
|
||
| if r.oversubscriptionLastCheckedAt == nil { | ||
| r.oversubscriptionLastCheckedAt = make(map[string]time.Time) | ||
| r.oversubscriptionPendingCheck = make(map[string]bool) | ||
| r.oversubscriptionFirstSeen = make(map[string]time.Time) | ||
| } | ||
|
|
||
| // Rate limit: if checked recently and if pending flag marks already dirty | ||
| if timeSinceLastCheck := time.Since(r.oversubscriptionLastCheckedAt[host]); timeSinceLastCheck < minCheckInterval { | ||
| if !r.oversubscriptionPendingCheck[host] { | ||
| r.oversubscriptionPendingCheck[host] = true | ||
| return minCheckInterval - timeSinceLastCheck + time.Second | ||
| } else { | ||
| // already dirty, so someone else requeued already | ||
| return 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The rate limit defeats OversubscriptionGracePeriod.
minCheckInterval reuses RequeueIntervalActive. This PR raises that value from 5m to 30m in helm/bundles/cortex-nova/values.yaml line 180, while oversubscriptionGracePeriod is 2m.
Trace the sequence for an over-subscribed host:
- First check runs, sets
firstSeen, returnsgracePeriod(2m). - The reconcile arrives 2m later.
timeSinceLastCheckis 2m, which is below 30m, so line 942 short-circuits before the check runs. It returns~28m. - The reconcile arrives ~28m later and only then can evict.
The eviction that the 2m grace period is designed to trigger is delayed by roughly 30 minutes per slot. A host that needs several evictions takes hours to converge.
Line 946 makes this worse. If pendingCheck is already true, the function returns 0 and drops the requeue. If that requeue was the only pending trigger, the host is not re-checked until an unrelated event arrives.
Use a dedicated minimum check interval that is independent of RequeueIntervalActive, and always return a requeue duration on the rate-limited path.
🐛 Proposed fix
- minCheckInterval := r.Conf.RequeueIntervalActive.Duration
- if minCheckInterval == 0 {
- minCheckInterval = 30 * time.Second
- }
+ // The minimum interval between two checks for the same host. Keep it below
+ // gracePeriod so the grace period drives the eviction cadence.
+ minCheckInterval := gracePeriod / 4
+ if minCheckInterval < 30*time.Second {
+ minCheckInterval = 30 * time.Second
+ } if timeSinceLastCheck := time.Since(r.oversubscriptionLastCheckedAt[host]); timeSinceLastCheck < minCheckInterval {
- if !r.oversubscriptionPendingCheck[host] {
- r.oversubscriptionPendingCheck[host] = true
- return minCheckInterval - timeSinceLastCheck + time.Second
- } else {
- // already dirty, so someone else requeued already
- return 0
- }
+ r.oversubscriptionPendingCheck[host] = true
+ return minCheckInterval - timeSinceLastCheck + time.Second
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| gracePeriod := r.Conf.OversubscriptionGracePeriod.Duration | |
| if gracePeriod == 0 { | |
| gracePeriod = 2 * time.Minute | |
| } | |
| minCheckInterval := r.Conf.RequeueIntervalActive.Duration | |
| if minCheckInterval == 0 { | |
| minCheckInterval = 30 * time.Second | |
| } | |
| if r.oversubscriptionLastCheckedAt == nil { | |
| r.oversubscriptionLastCheckedAt = make(map[string]time.Time) | |
| r.oversubscriptionPendingCheck = make(map[string]bool) | |
| r.oversubscriptionFirstSeen = make(map[string]time.Time) | |
| } | |
| // Rate limit: if checked recently and if pending flag marks already dirty | |
| if timeSinceLastCheck := time.Since(r.oversubscriptionLastCheckedAt[host]); timeSinceLastCheck < minCheckInterval { | |
| if !r.oversubscriptionPendingCheck[host] { | |
| r.oversubscriptionPendingCheck[host] = true | |
| return minCheckInterval - timeSinceLastCheck + time.Second | |
| } else { | |
| // already dirty, so someone else requeued already | |
| return 0 | |
| } | |
| } | |
| gracePeriod := r.Conf.OversubscriptionGracePeriod.Duration | |
| if gracePeriod == 0 { | |
| gracePeriod = 2 * time.Minute | |
| } | |
| // The minimum interval between two checks for the same host. Keep it below | |
| // gracePeriod so the grace period drives the eviction cadence. | |
| minCheckInterval := gracePeriod / 4 | |
| if minCheckInterval < 30*time.Second { | |
| minCheckInterval = 30 * time.Second | |
| } | |
| if r.oversubscriptionLastCheckedAt == nil { | |
| r.oversubscriptionLastCheckedAt = make(map[string]time.Time) | |
| r.oversubscriptionPendingCheck = make(map[string]bool) | |
| r.oversubscriptionFirstSeen = make(map[string]time.Time) | |
| } | |
| // Rate limit: if checked recently and if pending flag marks already dirty | |
| if timeSinceLastCheck := time.Since(r.oversubscriptionLastCheckedAt[host]); timeSinceLastCheck < minCheckInterval { | |
| r.oversubscriptionPendingCheck[host] = true | |
| return minCheckInterval - timeSinceLastCheck + time.Second | |
| } |
🤖 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/commitments/reservation_controller.go`
around lines 926 - 950, Decouple the oversubscription rate limit in the
reservation controller from RequeueIntervalActive by introducing or reusing a
dedicated minimum check interval appropriate for the grace-period flow. Update
the timeSinceLastCheck branch to always return a positive requeue duration,
including when oversubscriptionPendingCheck[host] is already true, so
rate-limited hosts are reliably rechecked without dropping the trigger.
Comment on lines
+1031
to
+1038
| if len(violations) == 0 { | ||
| monitor.ClearHost(host, az) | ||
| return false, true, nil | ||
| } | ||
|
|
||
| for rn, excess := range violations { | ||
| monitor.SetOversubscribed(host, az, string(rn), float64(excess.Value())) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Per-resource gauges go stale.
ClearHost runs only when the violation set is empty. SetOversubscribed runs only for the resources that currently violate. If memory violates in one run and only CPU violates in the next, the memory gauge keeps its last non-zero value forever.
Clear the host gauges first, then set the current violations.
🐛 Proposed fix
if len(violations) == 0 {
monitor.ClearHost(host, az)
return false, true, nil
}
+ // Drop previous series so resources that no longer violate do not stay non-zero.
+ monitor.ClearHost(host, az)
for rn, excess := range violations {
monitor.SetOversubscribed(host, az, string(rn), float64(excess.Value()))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(violations) == 0 { | |
| monitor.ClearHost(host, az) | |
| return false, true, nil | |
| } | |
| for rn, excess := range violations { | |
| monitor.SetOversubscribed(host, az, string(rn), float64(excess.Value())) | |
| } | |
| if len(violations) == 0 { | |
| monitor.ClearHost(host, az) | |
| return false, true, nil | |
| } | |
| // Drop previous series so resources that no longer violate do not stay non-zero. | |
| monitor.ClearHost(host, az) | |
| for rn, excess := range violations { | |
| monitor.SetOversubscribed(host, az, string(rn), float64(excess.Value())) | |
| } |
🤖 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/commitments/reservation_controller.go`
around lines 1031 - 1038, Update the violation-handling flow in the reservation
controller to call monitor.ClearHost(host, az) before processing violations on
every run, not only when len(violations) == 0. Then set gauges for the current
entries in violations and retain the existing return behavior for empty and
non-empty violation sets.
Comment on lines
+1062
to
+1095
| var unallocatedReservations, allocatedReservations []*v1alpha1.Reservation | ||
| for i := range allReservations { | ||
| res := &allReservations[i] | ||
| if res.Spec.Type != v1alpha1.ReservationTypeCommittedResource { | ||
| continue | ||
| } | ||
| if res.Spec.CommittedResourceReservation == nil || | ||
| len(res.Spec.CommittedResourceReservation.Allocations) == 0 { | ||
| unallocatedReservations = append(unallocatedReservations, res) | ||
| } else { | ||
| allocatedReservations = append(allocatedReservations, res) | ||
| } | ||
| } | ||
| sort.Slice(unallocatedReservations, func(i, j int) bool { | ||
| mi := unallocatedReservations[i].Spec.Resources[hv1.ResourceMemory] | ||
| mj := unallocatedReservations[j].Spec.Resources[hv1.ResourceMemory] | ||
| return mi.Cmp(mj) < 0 | ||
| }) | ||
| sort.Slice(allocatedReservations, func(i, j int) bool { | ||
| ui := reservations.UnusedReservationCapacity(allocatedReservations[i], false) | ||
| uj := reservations.UnusedReservationCapacity(allocatedReservations[j], false) | ||
| mi := ui[hv1.ResourceMemory] | ||
| mj := uj[hv1.ResourceMemory] | ||
| return mi.Cmp(mj) < 0 | ||
| }) | ||
| memViolation := violations[hv1.ResourceMemory] | ||
| for _, res := range allocatedReservations { | ||
| unused := reservations.UnusedReservationCapacity(res, false) | ||
| unusedMem := unused[hv1.ResourceMemory] | ||
| if unusedMem.Cmp(memViolation) >= 0 { | ||
| allocatedReservations = []*v1alpha1.Reservation{res} | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find other consumers of CommittedResourceReservation.Allocations to assess the impact of clearing them.
set -euo pipefail
rg -n -C 6 'CommittedResourceReservation\.Allocations' --type=go -g '!**/*_test.go'Repository: cobaltcore-dev/cortex
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find Go files matching reservation_controller"
fd -a 'reservation_controller\.go$' . || true
echo
echo "Search all references to relevant API/data fields"
rg -n 'CommittedResourceReservation|Allocations|unplaceReservation|UsageReservationCapacity|UnusedReservationCapacity' --type=go -g '!**/*_test.go' .
echo
echo "Search V1alpha1 Reservation type definitions"
fd -a '.*\.go$' . | xargs rg -n 'type CommittedResourceReservation|Allocations map|struct CommittedResource|type Reservation' -g '*go' || trueRepository: cobaltcore-dev/cortex
Length of output: 49542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "reservation_controller.go relevant sections"
sed -n '1020,1165p' internal/scheduling/reservations/commitments/reservation_controller.go
echo
echo "resource_provider_allocations.go"
sed -n '1,120p' internal/shim/placement/handle_resource_provider_allocations.go
echo
echo "handle_allocations.go"
sed -n '1,90p' internal/shim/placement/handle_allocations.go
echo
echo "Capacity accounting relevant section"
sed -n '110,155p' internal/scheduling/reservations/capacity_accounting.go
echo
echo "nova capacity filter around committed_reservation block"
sed -n '215,245p' internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go
echo
echo "nova CRS recorder around spec allocation writes/status clear check"
sed -n '110,165p' internal/scheduling/nova/crs/recorder.go
echo
echo "reservation_controller_monitor.go outline and relevant references"
sed -n '1,240p' internal/scheduling/reservations/commitments/reservation_controller_monitor.go
echo
echo "change_commitments relevant section"
sed -n '680,730p' internal/scheduling/reservations/commitments/api/change_commitments.goRepository: cobaltcore-dev/cortex
Length of output: 18988
Do not evict allocation-bearing committed-resource reservations without moving/terminating VMs.
When no unallocated slot exists, the code falls back to allocatedReservations, then unplaceReservation clears Spec.CommittedResourceReservation.Allocations and Status.CommittedResourceReservation.Allocations. Those maps preserve the VM UUID to host mapping for already-placed VMs and are used by resource-provider allocation reads, availability filtering, and committed-resource capacity accounting. Clearing them without moving or stopping those VMs loses required allocation records. Restrict eviction to unallocated slots, or report the over-subscription as unresolvable through the monitor.
🤖 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/commitments/reservation_controller.go`
around lines 1062 - 1095, The fallback loop over allocatedReservations must not
select allocation-bearing committed-resource reservations for eviction, because
unplaceReservation clears required VM allocation mappings. Remove that fallback
selection and restrict eviction to unallocatedReservations; when no eligible
unallocated slot exists, report the memory over-subscription as unresolvable
through the existing monitor path.
Comment on lines
+1136
to
+1155
| if err := r.Patch(ctx, res, client.MergeFrom(old)); err != nil { | ||
| return nil, fmt.Errorf("failed to patch reservation %s: %w", res.Name, err) | ||
| } | ||
| if err := r.Get(ctx, client.ObjectKeyFromObject(res), res); err != nil { | ||
| return nil, fmt.Errorf("failed to re-fetch reservation %s: %w", res.Name, err) | ||
| } | ||
| old = res.DeepCopy() | ||
| meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ | ||
| Type: v1alpha1.ReservationConditionReady, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "OversubscriptionRemediation", | ||
| Message: fmt.Sprintf("evicted from %s due to host over-subscription", host), | ||
| }) | ||
| res.Status.Host = "" | ||
| if res.Status.CommittedResourceReservation != nil { | ||
| res.Status.CommittedResourceReservation.Allocations = nil | ||
| } | ||
| if err := r.Status().Patch(ctx, res, client.MergeFrom(old)); err != nil { | ||
| return nil, fmt.Errorf("failed to patch reservation %s status: %w", res.Name, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A failed status patch leaves the slot in a stuck state.
unplaceReservation writes in two phases. If the spec patch at line 1136 succeeds and the status patch at line 1153 fails, the reservation ends with Spec.TargetHost == "", Status.Host == host, and Ready == True.
That state does not self-repair:
Reconcilereturns early at line 130 becauseres.IsReady()is true, so the slot is never rescheduled.- The sync branch at line 205 requires
Spec.TargetHost != "", soStatus.Hostis never cleared. HostFreeCapacitystill counts the slot against the host becauseStatus.Hostmatches, so the host stays over-subscribed.
The caller compounds this. runOversubscriptionCheck logs the error at line 970 and returns 0, so no requeue is scheduled.
Clear Status.Host and set Ready=False before clearing Spec.TargetHost, so a failure between the two writes leaves a state that the reconcile loop can repair. Also return the error from runOversubscriptionCheck so the controller retries.
🤖 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/commitments/reservation_controller.go`
around lines 1136 - 1155, The unplaceReservation update order can leave a ready
reservation counted on its old host when the status patch fails. Apply the
status changes clearing Status.Host and setting Ready=False before clearing
Spec.TargetHost, preserving the existing rollback-safe behavior, and update
runOversubscriptionCheck to return the unplaceReservation error so
reconciliation retries instead of returning success.
Contributor
Test Coverage ReportTest Coverage 📊: 70.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.
No description provided.