feat: CR controller checks for host overload scenario and resolves via reservation re-placements - #1125
Open
mblos wants to merge 10 commits into
Open
feat: CR controller checks for host overload scenario and resolves via reservation re-placements#1125mblos wants to merge 10 commits into
mblos wants to merge 10 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 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 Sequence Diagram(s)sequenceDiagram
participant Hypervisor
participant CommitmentReservationController
participant ReservationControllerMonitor
participant Reservation
Hypervisor->>CommitmentReservationController: Capacity update event
CommitmentReservationController->>CommitmentReservationController: Calculate free capacity
CommitmentReservationController->>ReservationControllerMonitor: Record oversubscription
CommitmentReservationController->>Reservation: Unplace selected reservation after grace period
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
Signed-off-by: Marcel <156897072+mblos@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/scheduling/reservations/commitments/integration_test.go (1)
447-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not duplicate the Reservation host index extractor in the fake client setup.
The closure repeats
reservations.IndexReservationByHost’s rule. Keep the production registration and export the extractor separately, or add a shared test helper/extractor. Otherwise controller and fake cache index rules can drift.🤖 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/integration_test.go` around lines 447 - 464, The fake client setup should reuse the existing Reservation host index extractor instead of duplicating its logic. Update the WithIndex registration for IdxReservationByHost to reference or share reservations.IndexReservationByHost, exporting or introducing a shared helper if necessary, while preserving the production and fake cache indexing behavior.
🤖 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/integration_test.go`:
- Around line 1342-1348: Remove the contradictory comment claiming
reconciliation always uses slot-10 as the trigger, while preserving the
following rotation explanation and the existing implementation that rotates
through unallocated slots.
- Around line 1277-1280: Update the capacity figures in the reservation fixture
comment near the slot-total calculation to use 200 GiB and 200 cores, matching
the hypervisor configuration and the existing 200 GiB reference; leave the slot
allocation and eviction descriptions unchanged.
---
Nitpick comments:
In `@internal/scheduling/reservations/commitments/integration_test.go`:
- Around line 447-464: The fake client setup should reuse the existing
Reservation host index extractor instead of duplicating its logic. Update the
WithIndex registration for IdxReservationByHost to reference or share
reservations.IndexReservationByHost, exporting or introducing a shared helper if
necessary, while preserving the production and fake cache indexing behavior.
🪄 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: f8559631-131a-44b6-a6b3-016f3f0f589a
📒 Files selected for processing (6)
helm/bundles/cortex-nova/templates/alerts.yamlhelm/bundles/cortex-nova/values.yamlinternal/scheduling/reservations/commitments/config.gointernal/scheduling/reservations/commitments/integration_test.gointernal/scheduling/reservations/commitments/reservation_controller.gointernal/scheduling/reservations/commitments/reservation_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- helm/bundles/cortex-nova/values.yaml
- helm/bundles/cortex-nova/templates/alerts.yaml
- internal/scheduling/reservations/commitments/reservation_controller.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/scheduling/reservations/commitments/reservation_controller_test.go (1)
1584-1587: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
RequeueIntervalActiveno longer drives the rate limit.
runOversubscriptionChecknow readsOversubscriptionMinCheckIntervaland defaults it to 30s. The test passes because of that default, not because of the 30-minute value set here. SetOversubscriptionMinCheckIntervalexplicitly so the test states the interval it depends on.♻️ Proposed change
Conf: ReservationControllerConfig{ - RequeueIntervalActive: metav1.Duration{Duration: 30 * time.Minute}, - EnableOversubscriptionCheck: true, + OversubscriptionMinCheckInterval: metav1.Duration{Duration: 30 * time.Second}, + EnableOversubscriptionCheck: true, },🤖 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_test.go` around lines 1584 - 1587, Update the ReservationControllerConfig setup for the test using EnableOversubscriptionCheck to set OversubscriptionMinCheckInterval explicitly to the intended interval, rather than relying on the default; retain RequeueIntervalActive only if the test independently requires it.internal/scheduling/reservations/commitments/reservation_controller.go (1)
381-401: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe no-hosts path now returns an error instead of a retry interval.
The analogous branch at Line 334 returns
RequeueAfter: r.Conf.RequeueIntervalRetry.Durationfor the same class of condition. This branch returns an error, so controller-runtime applies exponential backoff and logs a reconcile error on every occurrence. A host shortage is an expected steady state, not a failure of the reconcile.Consider returning
ctrl.Result{RequeueAfter: r.Conf.RequeueIntervalRetry.Duration}, nilto keep both no-host paths consistent and to avoid error-log noise.♻️ Proposed change
- return ctrl.Result{}, fmt.Errorf("no hosts found for reservation %s (flavor %s)", res.Name, resourceName) + return ctrl.Result{RequeueAfter: r.Conf.RequeueIntervalRetry.Duration}, nilNote:
internal/scheduling/reservations/commitments/payg_rollback_test.gonow matches on the"no hosts found"error string, so this change requires a test update.🤖 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 381 - 401, Update the no-hosts branch in the reservation reconcile flow after the status patch to return ctrl.Result{RequeueAfter: r.Conf.RequeueIntervalRetry.Duration}, nil instead of an error, matching the analogous no-host path. Update payg_rollback_test.go to stop expecting the no-hosts error and assert the retry result instead.internal/scheduling/reservations/commitments/integration_test.go (2)
482-486: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin the oversubscription timing in the integration environment.
EnableOversubscriptionCheckis enabled, but the two oversubscription timing fields are omitted. The remediation loop then backdatesoversubscriptionFirstSeenat Line 1354, so it does not verify the two-minute grace-period boundary. Set both fields explicitly and add an assertion that detection does not evict before the grace period.The configuration contract is defined in
internal/scheduling/reservations/commitments/config.goat Lines 53-94. The deployment values are defined at Lines 188-193.Suggested test configuration
RequeueIntervalActive: metav1.Duration{Duration: 5 * time.Minute}, + OversubscriptionGracePeriod: metav1.Duration{Duration: 2 * time.Minute}, + OversubscriptionMinCheckInterval: metav1.Duration{Duration: 30 * time.Second}, EnableOversubscriptionCheck: true,Also applies to: 1346-1365
🤖 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/integration_test.go` around lines 482 - 486, Update the integration test’s ReservationControllerConfig to explicitly set both oversubscription timing fields to the intended two-minute grace-period values, using the configuration contract symbols in commitments/config.go. In the remediation-loop test around the oversubscriptionFirstSeen backdating, assert that detection does not evict before the two-minute grace period, then preserve the existing post-grace-period eviction assertion.
1388-1395: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the smallest-first eviction order.
The test only verifies that allocated slots remain. It does not verify which unallocated slot the controller evicts. A controller that evicts
slot-19beforeslot-10still passes because the final capacity becomes non-negative. Record the evicted slot after each remediation cycle and assert the expected order:slot-10,slot-11, thenslot-12.The fixture comments at Lines 1266-1286 define the smallest-first expectation.
🤖 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/integration_test.go` around lines 1388 - 1395, Extend the remediation-cycle assertions in the integration test to record which unallocated slot is evicted after each cycle, then assert the eviction order is slot-10, slot-11, and slot-12. Preserve the existing check that allocated slots with running VMs are never evicted, and use the fixture’s smallest-first expectation when identifying the evicted slot.
🤖 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/reservation_controller.go`:
- Around line 1145-1155: Correct the comment above the fallback in the
reservation eviction-selection logic to state that the selected last element is
the largest unallocated slot, matching the ascending sort order and the existing
log message. Do not change the selection or logging behavior.
---
Nitpick comments:
In `@internal/scheduling/reservations/commitments/integration_test.go`:
- Around line 482-486: Update the integration test’s ReservationControllerConfig
to explicitly set both oversubscription timing fields to the intended two-minute
grace-period values, using the configuration contract symbols in
commitments/config.go. In the remediation-loop test around the
oversubscriptionFirstSeen backdating, assert that detection does not evict
before the two-minute grace period, then preserve the existing post-grace-period
eviction assertion.
- Around line 1388-1395: Extend the remediation-cycle assertions in the
integration test to record which unallocated slot is evicted after each cycle,
then assert the eviction order is slot-10, slot-11, and slot-12. Preserve the
existing check that allocated slots with running VMs are never evicted, and use
the fixture’s smallest-first expectation when identifying the evicted slot.
In `@internal/scheduling/reservations/commitments/reservation_controller_test.go`:
- Around line 1584-1587: Update the ReservationControllerConfig setup for the
test using EnableOversubscriptionCheck to set OversubscriptionMinCheckInterval
explicitly to the intended interval, rather than relying on the default; retain
RequeueIntervalActive only if the test independently requires it.
In `@internal/scheduling/reservations/commitments/reservation_controller.go`:
- Around line 381-401: Update the no-hosts branch in the reservation reconcile
flow after the status patch to return ctrl.Result{RequeueAfter:
r.Conf.RequeueIntervalRetry.Duration}, nil instead of an error, matching the
analogous no-host path. Update payg_rollback_test.go to stop expecting the
no-hosts error and assert the retry result instead.
🪄 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: 47340899-3ac7-498a-a443-88481ee509ef
📒 Files selected for processing (17)
helm/bundles/cortex-nova/values.yamlinternal/scheduling/lib/filter_monitor_test.gointernal/scheduling/lib/filter_validation_test.gointernal/scheduling/lib/filter_weigher_pipeline_step_test.gointernal/scheduling/lib/weigher_monitor_test.gointernal/scheduling/lib/weigher_validation_test.gointernal/scheduling/nova/filter_weigher_pipeline_controller_test.gointernal/scheduling/pods/plugins/filters/filter_node_available_test.gointernal/scheduling/reservations/commitments/api/report_capacity_test.gointernal/scheduling/reservations/commitments/api/usage_test.gointernal/scheduling/reservations/commitments/committed_resource_controller_test.gointernal/scheduling/reservations/commitments/config.gointernal/scheduling/reservations/commitments/integration_test.gointernal/scheduling/reservations/commitments/payg_rollback_test.gointernal/scheduling/reservations/commitments/reservation_controller.gointernal/scheduling/reservations/commitments/reservation_controller_test.gointernal/scheduling/reservations/commitments/syncer_monitor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/scheduling/reservations/commitments/committed_resource_controller_test.go
Comment on lines
+1145
to
+1155
| // Fall back to the last (smallest) unallocated if none covers all violations. | ||
| if len(unallocated) > 0 { | ||
| res := unallocated[len(unallocated)-1] | ||
| logger.Info("eviction target selected (largest unallocated slot, partial coverage)", | ||
| "reservation", res.Name, | ||
| "total unallocated slots", len(unallocated), | ||
| "slot resources", res.Spec.Resources, | ||
| "violations", formatQuantityMap(violations), | ||
| ) | ||
| return res | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The comment contradicts the sort order and the log message.
The slice is sorted ascending by memory at Line 1114. unallocated[len(unallocated)-1] is therefore the largest slot, not the smallest. The log message at Line 1148 already says "largest unallocated slot". Correct the comment.
🔤 Proposed comment fix
- // Fall back to the last (smallest) unallocated if none covers all violations.
+ // Fall back to the largest unallocated slot if none covers all violations.📝 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
| // Fall back to the last (smallest) unallocated if none covers all violations. | |
| if len(unallocated) > 0 { | |
| res := unallocated[len(unallocated)-1] | |
| logger.Info("eviction target selected (largest unallocated slot, partial coverage)", | |
| "reservation", res.Name, | |
| "total unallocated slots", len(unallocated), | |
| "slot resources", res.Spec.Resources, | |
| "violations", formatQuantityMap(violations), | |
| ) | |
| return res | |
| } | |
| // Fall back to the largest unallocated slot if none covers all violations. | |
| if len(unallocated) > 0 { | |
| res := unallocated[len(unallocated)-1] | |
| logger.Info("eviction target selected (largest unallocated slot, partial coverage)", | |
| "reservation", res.Name, | |
| "total unallocated slots", len(unallocated), | |
| "slot resources", res.Spec.Resources, | |
| "violations", formatQuantityMap(violations), | |
| ) | |
| return res | |
| } |
🤖 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 1145 - 1155, Correct the comment above the fallback in the
reservation eviction-selection logic to state that the selected last element is
the largest unallocated slot, matching the ascending sort order and the existing
log message. Do not change the selection or logging behavior.
Contributor
Test Coverage ReportTest Coverage 📊: 70.8% |
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.