Implement in-flight reservations controller#957
Open
PhilippMatthes wants to merge 9 commits into
Open
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an inflight reservation controller that reconciles ChangesInflight Reservation Controller and KPI
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
PhilippMatthes
commented
Jul 6, 2026
PhilippMatthes
force-pushed
the
in-flight-reservations-controller
branch
from
07669c6 to
29afa24
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/scheduling/reservations/inflight/controller.go (2)
144-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnnecessary blank lines after function literal signatures.
Each
handler.CreateFunc/UpdateFunc/DeleteFunc(Lines 147-153, 154-160, 161-167, 191-193, 218-223, 224-229, 230-235) has a blank line right after the signature before the first statement. As per coding guidelines, "avoid unnecessary empty lines between code segments."🤖 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/inflight/controller.go` around lines 144 - 237, The event-handler function literals in handleReservations and handleHypervisors have unnecessary empty lines immediately after the CreateFunc/UpdateFunc/DeleteFunc signatures. Remove those blank lines so the first statement follows the signature directly in each handler.Funcs assignment, keeping the formatting consistent with the coding guidelines.Source: Coding guidelines
117-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent status-write semantics:
Status().UpdatevsStatus().Patch.The
UnexpectedTypeandMissingSpecbranches useclient.MergeFrom(orig)patches, but this branch callsc.Status().Update(ctx, obj)on the object fetched at the top of Reconcile.Updaterequires an exactresourceVersionmatch and will fail with a conflict if anything modified the object between the initialGetand this call, whereas the merge patch used elsewhere does not. UsingPatchconsistently here would align concurrency handling across all three status-write paths.♻️ Proposed fix for consistency
+ orig := obj.DeepCopy() meta.SetStatusCondition(&obj.Status.Conditions, metav1.Condition{ Type: v1alpha1.ReservationConditionReady, Status: metav1.ConditionUnknown, Reason: "InstanceNotFound", Message: "The instance has not spawned on any hypervisor yet", }) - if err := c.Status().Update(ctx, obj); err != nil { + if err := c.Status().Patch(ctx, obj, client.MergeFrom(orig)); err != nil { log.Error(err, "Failed to update reservation status") return ctrl.Result{}, err }🤖 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/inflight/controller.go` around lines 117 - 133, The status write in the not-found branch of the Reconcile flow is inconsistent with the other status paths because it uses Status().Update instead of the merge-patch approach used by the UnexpectedType and MissingSpec branches. Change the reservation status write in the controller’s Reconcile logic to use Status().Patch with client.MergeFrom(orig) so it matches the existing concurrency-safe pattern and avoids resourceVersion conflicts after the initial Get.
🤖 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/knowledge/kpis/plugins/deployment/reservation_state.go`:
- Around line 60-66: The ReservationStateKPI.Collect method currently uses an
unbounded context and silently returns on list errors, which can hang scrapes
and hide failures. Update Collect to create a short context.WithTimeout for the
k.Client.List call, and ensure any list failure is logged before returning so
reservation metric collection issues are visible.
In `@internal/scheduling/reservations/inflight/controller_test.go`:
- Around line 39-52: The test client setup duplicates the reservation
target-host index logic instead of reusing the shared callback. Update
newTestClient to use idxReservationByTargetHostFn in the WithIndex call for
v1alpha1.Reservation so the test and controller stay aligned if the indexing
behavior changes. This keeps the indexing behavior defined in one place and
avoids silent drift between controller.go and controller_test.go.
In `@internal/scheduling/reservations/inflight/controller.go`:
- Around line 188-245: `handleHypervisors` is currently enqueueing reservations
on every Hypervisor update because `predicateHypervisors` only filters by type
and `UpdateFunc` always calls `enqueueCorrespondingReservations`. Update the
hypervisor event filtering so `handleHypervisors` only reacts when the relevant
instance list changes, using the `predicateHypervisors` and `UpdateFunc` path to
compare the old and new `hv1.Hypervisor` state and skip enqueuing when
`Status.Instances` is unchanged. Keep the existing create/delete behavior, but
gate update-triggered reconciliation to the specific instance-list change that
`enqueueCorrespondingReservations` depends on.
---
Nitpick comments:
In `@internal/scheduling/reservations/inflight/controller.go`:
- Around line 144-237: The event-handler function literals in handleReservations
and handleHypervisors have unnecessary empty lines immediately after the
CreateFunc/UpdateFunc/DeleteFunc signatures. Remove those blank lines so the
first statement follows the signature directly in each handler.Funcs assignment,
keeping the formatting consistent with the coding guidelines.
- Around line 117-133: The status write in the not-found branch of the Reconcile
flow is inconsistent with the other status paths because it uses Status().Update
instead of the merge-patch approach used by the UnexpectedType and MissingSpec
branches. Change the reservation status write in the controller’s Reconcile
logic to use Status().Patch with client.MergeFrom(orig) so it matches the
existing concurrency-safe pattern and avoids resourceVersion conflicts after the
initial Get.
🪄 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: c4364f02-180b-42a2-bbb4-927892609121
📒 Files selected for processing (9)
cmd/manager/main.gohelm/bundles/cortex-nova/templates/alerts.yamlhelm/bundles/cortex-nova/templates/kpis.yamlhelm/bundles/cortex-nova/values.yamlinternal/knowledge/kpis/plugins/deployment/reservation_state.gointernal/knowledge/kpis/plugins/deployment/reservation_state_test.gointernal/knowledge/kpis/supported_kpis.gointernal/scheduling/reservations/inflight/controller.gointernal/scheduling/reservations/inflight/controller_test.go
- Bound ReservationStateKPI.Collect with a 10s context timeout and log list failures so scrape hangs and errors are visible. - Reuse idxReservationByTargetHostFn in the test fake client so the index logic stays defined in a single place. - Filter Hypervisor Update events in predicateHypervisors so we only re-enqueue reservations when Status.Instances actually changed; unrelated status churn no longer triggers a list + enqueue. - Use Status().Patch(ctx, obj, client.MergeFrom(orig)) in the InstanceNotFound branch to match the other status writes in Reconcile. - Drop unnecessary blank lines after handler function-literal signatures.
Member
Author
|
@coderabbitai the three actionable inline comments and both nitpicks from your review have been addressed in d869070:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5
git show d869070b --stat 2>/dev/null | head -50Length of output: 1660 Confirmed — commit d869070 includes all five fixes as described (timeout/logging in |
Member
Author
|
@claude please resolve the linting issues (run |
Contributor
|
Claude finished @PhilippMatthes's task in 3m 12s —— View job Resolving Linting Issues
All 7 whitespace lint issues in |
Add required blank lines after multi-line function signatures to satisfy the golangci-lint whitespace checker. Co-authored-by: Philipp Matthes <PhilippMatthes@users.noreply.github.com>
mblos
reviewed
Jul 6, 2026
mblos
left a comment
mblos
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed, but it seems we delete in-flight reservations of a VM as soon as the vm is found on any host, right?
What about other events like migrate or resize that are also relfected using in-flight reservations?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/scheduling/reservations/inflight/vm_client.go (1)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the Nova microversion comment
This client only needs microversion 2.47+ for the embeddedflavor.ram/flavor.vcpusfields in/servers/{id}. The hypervisor/placement UUID rationale belongs to the other Nova client.🤖 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/inflight/vm_client.go` around lines 70 - 73, The Nova client comment in the microversion setup is wrong for this path: it mentions hypervisor/placement UUIDs, but this client only relies on the embedded flavor fields available from microversion 2.47+. Update the comment near the Microversion assignment in the Nova client constructor/build logic to describe the /servers/{id} flavor.ram and flavor.vcpus requirement, and keep the UUID rationale only in the other Nova client that uses it.
🤖 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/inflight/controller.go`:
- Around line 146-183: The VM size validation in controller.go is running too
early in the reconciliation flow and affects stale reservations before the
target-host check in the reconciliation path. Move or guard the resize/rebuild
block in the controller’s reconcile logic so `VMClient.GetCurrentVMSize` and the
`VMSizeMismatch` status patch only happen when `hypervisorName` matches
`obj.Spec.TargetHost`; otherwise let the stale-reservation path continue
unchanged.
- Around line 146-178: The VM size comparison in the in-flight reservation
controller is using reflect.DeepEqual, which can treat equivalent
resource.Quantity values as different and keep resize/rebuild reservations in
VMSizeMismatch. Update the check in the controller logic around
obj.Spec.InFlightReservation.Intent to use semantic quantity comparison instead,
such as equality.Semantic.DeepEqual or per-quantity Equal/Cmp checks against
obj.Spec.Resources and the VMClient.GetCurrentVMSize result. Make sure the
reservation status/requeue path still behaves the same, but only when the
numeric sizes truly differ.
---
Nitpick comments:
In `@internal/scheduling/reservations/inflight/vm_client.go`:
- Around line 70-73: The Nova client comment in the microversion setup is wrong
for this path: it mentions hypervisor/placement UUIDs, but this client only
relies on the embedded flavor fields available from microversion 2.47+. Update
the comment near the Microversion assignment in the Nova client
constructor/build logic to describe the /servers/{id} flavor.ram and
flavor.vcpus requirement, and keep the UUID rationale only in the other Nova
client that uses it.
🪄 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: 424c788b-1e31-4688-a53c-346e176fa8b8
📒 Files selected for processing (5)
cmd/manager/main.gointernal/scheduling/reservations/inflight/controller.gointernal/scheduling/reservations/inflight/controller_test.gointernal/scheduling/reservations/inflight/vm_client.gointernal/scheduling/reservations/inflight/vm_client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/manager/main.go
- controller.go: replace reflect.DeepEqual on the reservation resource map with a per-key resource.Quantity Cmp check. reflect.DeepEqual on Quantity can return false for numerically-equal values because Quantity's unexported cached string/format state differs between values from API decoding and values freshly constructed by the vm client — that would leave resize/rebuild reservations stuck in VMSizeMismatch forever. - controller.go: gate the resize/rebuild size-check on the instance landing on the reservation's TargetHost. For a stale reservation (instance found on a different host) the previous code fired an unnecessary VMClient call and marked the reservation VMSizeMismatch instead of falling through to the awaiting-deletion no-op. - controller_test.go: add regression tests covering both fixes.
Contributor
Test Coverage ReportTest Coverage 📊: 70.1% |
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce a controller that owns the lifecycle of in-flight reservations, which are placement decisions that have been committed but whose VM has not yet appeared on a hypervisor.
Assisted-by: Claude Code:claude-opus-4-8 [Bash] [Read]