Committed reservations allocations consider enter/leave events of VMs#644
Draft
Committed reservations allocations consider enter/leave events of VMs#644
Conversation
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
📝 WalkthroughWalkthroughAdds allocation-grace configuration and updates the commitments controller to verify new allocations via Nova API and existing allocations via the Hypervisor CRD (Nova fallback), uses grace-aware requeue intervals, and patches Spec before Status when removing stale allocations. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
actor Controller
end
rect rgba(200,255,200,0.5)
participant K8s as Kubernetes Client
participant Nova as Nova API
participant Hypervisor as Hypervisor CRD
end
Controller->>K8s: Fetch Reservation with allocations
alt Allocation within grace period
Controller->>Nova: Get VM (real-time)
alt VM found with placement
Nova-->>Controller: Placement info
Controller->>K8s: Patch Status.Allocations
else VM not visible
Nova-->>Controller: Not found
Controller->>Controller: Defer (skip from status)
end
else Older allocation
Controller->>Hypervisor: Lookup instance on host
alt Found on Hypervisor CRD
Hypervisor-->>Controller: Instance confirmed
Controller->>K8s: Patch Status.Allocations
else Not on Hypervisor CRD
Controller->>Nova: Fallback verify
alt Found in Nova
Nova-->>Controller: Placement info
Controller->>K8s: Patch Status.Allocations
else Not found
Controller->>K8s: Remove from Spec.Allocations
Controller->>K8s: Patch Spec (then refetch)
Controller->>K8s: Patch Status
end
end
end
Controller->>Controller: Compute requeue interval (GracePeriod vs Active)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/bundles/cortex-nova/alerts/nova.alerts.yaml (1)
489-506:⚠️ Potential issue | 🟡 MinorThe disabled-feature explanation does not match how this metric is emitted.
cortex_committed_resource_syncer_runs_totalis registered unconditionally ininternal/scheduling/reservations/commitments/syncer_monitor.go:37-85, soabsent(...)only tells us the service is not exposing metrics. A cluster withcommitments-sync-taskdisabled will still alert viaincrease(...)=0, which makes the new “metric is missing / feature is not enabled” guidance misleading for on-call. Please gate this rule on a real task-enabled signal, or drop the disabled-feature explanation from the annotation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/bundles/cortex-nova/alerts/nova.alerts.yaml` around lines 489 - 506, The annotation's guidance is incorrect because cortex_committed_resource_syncer_runs_total is always registered, so absent(...) means metrics aren't being scraped, not that the feature is disabled; either remove the “or the feature is not enabled” language from the description/summary in nova.alerts.yaml (and leave the existing expr using cortex_committed_resource_syncer_runs_total) or gate the alert by a real task-enabled signal (e.g., add a condition that requires a boolean/metric that indicates the commitments-sync-task is enabled such as commitments_sync_task_enabled == 1) so the alert only fires when the task is actually enabled; update the description to match the chosen approach and reference cortex_committed_resource_syncer_runs_total and the alert block in nova.alerts.yaml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/bundles/cortex-nova/alerts/nova.alerts.yaml`:
- Around line 525-529: The alert divides a CounterVec
(cortex_committed_resource_syncer_commitments_skipped_total with label reason)
by a Counter (cortex_committed_resource_syncer_commitments_total) causing label
mismatch and empty results; fix by aggregating away the reason label before the
division, e.g. replace
rate(cortex_committed_resource_syncer_commitments_skipped_total{service="cortex-nova-metrics",
reason="..."}[1h]) with sum by
(service)(rate(cortex_committed_resource_syncer_commitments_skipped_total{service="cortex-nova-metrics"}[1h]))
so the numerator and denominator share the same labels, and apply the same
change for the second occurrence around lines 548-552.
---
Outside diff comments:
In `@helm/bundles/cortex-nova/alerts/nova.alerts.yaml`:
- Around line 489-506: The annotation's guidance is incorrect because
cortex_committed_resource_syncer_runs_total is always registered, so absent(...)
means metrics aren't being scraped, not that the feature is disabled; either
remove the “or the feature is not enabled” language from the description/summary
in nova.alerts.yaml (and leave the existing expr using
cortex_committed_resource_syncer_runs_total) or gate the alert by a real
task-enabled signal (e.g., add a condition that requires a boolean/metric that
indicates the commitments-sync-task is enabled such as
commitments_sync_task_enabled == 1) so the alert only fires when the task is
actually enabled; update the description to match the chosen approach and
reference cortex_committed_resource_syncer_runs_total and the alert block in
nova.alerts.yaml.
🪄 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: cf03d3d5-e2e5-4518-9789-4d639071c0bc
📒 Files selected for processing (1)
helm/bundles/cortex-nova/alerts/nova.alerts.yaml
This reverts commit b8f55a1.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/scheduling/nova/plugins/filters/filter_committed_resource_bookkeeping.go (1)
72-85: Consider indexed lookup instead of full reservation scan in the scheduling hot path.Listing and iterating all reservations on every filter run is O(n) per request; this can become costly at scale. Consider cache indexers/field selectors (projectID, resourceGroup, type, readiness) to shrink scan cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_committed_resource_bookkeeping.go` around lines 72 - 85, The code currently calls s.Client.List into reservations and then filters with s.isMatchingCRReservation for every scheduling request, causing O(n) scans; replace this with an indexed/selector-based lookup: add a field indexer on Reservation for keys you filter by (e.g., projectID, resourceGroup, type, readiness) and query via the cache/indexer or use client.List with client.MatchingFields/client.MatchingLabels or a field selector to only return matching Reservation items instead of listing all; update the code paths that populate reservations (the variable reservations and the call site using s.Client.List) to use the indexer (e.g., reservationIndexer.ByIndex or a cached lister) so you avoid the full reservation scan while still applying s.isMatchingCRReservation for any remaining checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@internal/scheduling/nova/plugins/filters/filter_committed_resource_bookkeeping.go`:
- Around line 223-226: The code currently sets reservationHost from
reservation.Spec.TargetHost falling back to reservation.Status.Host; change the
resolution to prefer reservation.Status.Host first and only use
reservation.Spec.TargetHost if Status.Host is empty so ready reservations use
the effective placement. Update the logic around reservationHost (variables:
reservationHost, reservation.Spec.TargetHost, reservation.Status.Host) to check
Status.Host first and then Spec.TargetHost as the fallback.
- Around line 256-262: The update currently logs and skips on any error from
s.Client.Update when writing reservationCopy, which can lose VM allocation
updates under concurrent conflicts; change this to a conflict-safe retry loop:
fetch the latest reservation (use s.Client.Get or the same client used
elsewhere), apply the allocation mutation to the fresh object, and attempt
s.Client.Update again, retrying a few times only on API conflict errors (detect
via apierrors.IsConflict(err)) and breaking on success or non-conflict errors;
preserve the existing traceLog.Warn/traceLog.Info messages but include the
retry/failure path for reservation.Name and instanceUUID so allocations aren't
silently dropped.
---
Nitpick comments:
In
`@internal/scheduling/nova/plugins/filters/filter_committed_resource_bookkeeping.go`:
- Around line 72-85: The code currently calls s.Client.List into reservations
and then filters with s.isMatchingCRReservation for every scheduling request,
causing O(n) scans; replace this with an indexed/selector-based lookup: add a
field indexer on Reservation for keys you filter by (e.g., projectID,
resourceGroup, type, readiness) and query via the cache/indexer or use
client.List with client.MatchingFields/client.MatchingLabels or a field selector
to only return matching Reservation items instead of listing all; update the
code paths that populate reservations (the variable reservations and the call
site using s.Client.List) to use the indexer (e.g., reservationIndexer.ByIndex
or a cached lister) so you avoid the full reservation scan while still applying
s.isMatchingCRReservation for any remaining checks.
🪄 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: 6cc970b2-763b-4a86-9023-189ff5f8d929
📒 Files selected for processing (4)
helm/bundles/cortex-nova/alerts/nova.alerts.yamlhelm/bundles/cortex-nova/templates/pipelines_kvm.yamlinternal/scheduling/nova/plugins/filters/filter_committed_resource_bookkeeping.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go
✅ Files skipped from review due to trivial changes (1)
- internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/bundles/cortex-nova/alerts/nova.alerts.yaml
Comment on lines
+223
to
+226
| reservationHost := reservation.Spec.TargetHost | ||
| if reservationHost == "" { | ||
| reservationHost = reservation.Status.Host | ||
| } |
There was a problem hiding this comment.
Prefer Status.Host over Spec.TargetHost when resolving reservation host.
For ready reservations, Status.Host represents the effective placement; using Spec.TargetHost first can misclassify candidate membership if desired and actual host diverge.
Proposed host resolution order
- reservationHost := reservation.Spec.TargetHost
- if reservationHost == "" {
- reservationHost = reservation.Status.Host
- }
+ reservationHost := reservation.Status.Host
+ if reservationHost == "" {
+ reservationHost = reservation.Spec.TargetHost
+ }📝 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
| reservationHost := reservation.Spec.TargetHost | |
| if reservationHost == "" { | |
| reservationHost = reservation.Status.Host | |
| } | |
| reservationHost := reservation.Status.Host | |
| if reservationHost == "" { | |
| reservationHost = reservation.Spec.TargetHost | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@internal/scheduling/nova/plugins/filters/filter_committed_resource_bookkeeping.go`
around lines 223 - 226, The code currently sets reservationHost from
reservation.Spec.TargetHost falling back to reservation.Status.Host; change the
resolution to prefer reservation.Status.Host first and only use
reservation.Spec.TargetHost if Status.Host is empty so ready reservations use
the effective placement. Update the logic around reservationHost (variables:
reservationHost, reservation.Spec.TargetHost, reservation.Status.Host) to check
Status.Host first and then Spec.TargetHost as the fallback.
Comment on lines
+256
to
+262
| if err := s.Client.Update(context.Background(), reservationCopy); err != nil { | ||
| traceLog.Warn("failed to update reservation with VM allocation", | ||
| "reservation", reservation.Name, | ||
| "instanceUUID", instanceUUID, | ||
| "error", err) | ||
| // Continue with other reservations - this is best-effort | ||
| } else { |
There was a problem hiding this comment.
Handle update conflicts to avoid silently dropping allocations.
Concurrent schedulings can race on the same reservation; on conflict, this code currently logs and skips, which can permanently miss VM allocation bookkeeping for that request.
Suggested conflict-safe update pattern
+import (
+ "k8s.io/apimachinery/pkg/types"
+ "k8s.io/client-go/util/retry"
+)
...
- if err := s.Client.Update(context.Background(), reservationCopy); err != nil {
+ err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+ latest := &v1alpha1.Reservation{}
+ if getErr := s.Client.Get(
+ context.Background(),
+ types.NamespacedName{Name: reservation.Name, Namespace: reservation.Namespace},
+ latest,
+ ); getErr != nil {
+ return getErr
+ }
+
+ if latest.Spec.CommittedResourceReservation == nil {
+ return nil
+ }
+ if latest.Spec.CommittedResourceReservation.Allocations == nil {
+ latest.Spec.CommittedResourceReservation.Allocations = make(map[string]v1alpha1.CommittedResourceAllocation)
+ }
+ if _, exists := latest.Spec.CommittedResourceReservation.Allocations[instanceUUID]; exists {
+ return nil
+ }
+
+ latest.Spec.CommittedResourceReservation.Allocations[instanceUUID] = v1alpha1.CommittedResourceAllocation{
+ CreationTimestamp: now,
+ Resources: vmResources,
+ }
+ return s.Client.Update(context.Background(), latest)
+ })
+ if err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@internal/scheduling/nova/plugins/filters/filter_committed_resource_bookkeeping.go`
around lines 256 - 262, The update currently logs and skips on any error from
s.Client.Update when writing reservationCopy, which can lose VM allocation
updates under concurrent conflicts; change this to a conflict-safe retry loop:
fetch the latest reservation (use s.Client.Get or the same client used
elsewhere), apply the allocation mutation to the fresh object, and attempt
s.Client.Update again, retrying a few times only on API conflict errors (detect
via apierrors.IsConflict(err)) and breaking on success or non-conflict errors;
preserve the existing traceLog.Warn/traceLog.Info messages but include the
retry/failure path for reservation.Name and instanceUUID so allocations aren't
silently dropped.
Contributor
Test Coverage ReportTest Coverage 📊: 68.3% |
Collaborator
Author
|
I will refactor w/o using nova client.. |
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.