feat(scheduler): add projected least-loaded placement strategy - #27
feat(scheduler): add projected least-loaded placement strategy#27catyans wants to merge 12 commits into
Conversation
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
|
Latest head |
424aa04 to
0679b78
Compare
LSX-s-Software
left a comment
There was a problem hiding this comment.
Thanks for the work on this. The overall direction looks good: using normalized projected CPU/memory pressure is a meaningful improvement over round-robin for heterogeneous clusters, and serializing selection with reservation addresses the stale-heartbeat concurrency problem.
However, I think there are still two correctness issues that should be addressed before merging.
1. Reservation reconciliation may consume the wrong reservation
The current implementation reconciles pending reservations using the increase in the aggregate sandbox count:
observedSandboxes := starting + running + paused
state.consume(observedSandboxes - state.observedSandboxes)consume then removes reservations in FIFO order. Sandbox creation order is not guaranteed to match scheduling order.
For example:
- reserve sandbox A with 8 CPUs;
- reserve sandbox B with 1 CPU;
- B enters
Creatingfirst; - the next heartbeat reports one additional sandbox and one additional allocated CPU.
The current code removes A's 8-CPU reservation rather than B's 1-CPU reservation. The scheduler would then account for only 2 CPUs instead of the actual/projected 9 CPUs and could continue placing work on an overloaded node.
Aggregate count growth may also come from unrelated activity, another scheduler instance, resume, fork, or requests that bypass this reservation state.
Please reconcile reservations by placement/assignment identity if possible. If changing the protocol is out of scope, a conservative resource-delta-based approach would be safer than count-only FIFO consumption. An unconfirmed reservation should preferably remain until its TTL rather than releasing the wrong resource reservation.
2. Non-placement and failed requests create reservations
LeastLoadedStrategy.Select currently always calls reserve, even when the request has no placement hint and the requested CPU/memory are both zero.
The Gateway calls Schedule for every request without a sandbox ID, not only:
POST /sandboxes;POST /sandboxes-cold.
This also includes list/read requests and other routes. Such requests create zero-resource reservation entries that are still counted as projected starting sandboxes:
load.starting += reserved.countAs a result, ordinary GET requests, unauthenticated requests, invalid requests, and failed upstream operations can artificially penalize a node for 30 seconds.
Please create reservations only for requests that are known to create a sandbox. Ideally, the Gateway should also explicitly confirm or cancel a reservation after the upstream operation succeeds or fails, rather than relying exclusively on heartbeat reconciliation and TTL expiry.
3. Please move this strategy into a separate file
LeastLoadedStrategy contains substantial state, reconciliation logic, scoring, and reservation management. Keeping all of it in strategy.go makes the generic strategy code unnecessarily large.
I suggest organizing it as:
scheduler/internal/strategy.go
Strategy interface
RoundRobinStrategy
RandomStrategy
scheduler/internal/least_loaded.go
LeastLoadedStrategy
reservation state and reconciliation
projected load calculation and comparison helpers
scheduler/internal/least_loaded_test.go
least-loaded-specific tests and benchmarks
This will make the implementation easier to review and maintain, especially if placement acknowledgement or resource-delta reconciliation is added later.
One smaller limitation worth documenting or addressing is that a cold-sandbox request without explicit cpuCount or memoryMB is projected as zero resources, while the AgentENV node applies its configured default CPU and memory values.
|
Thanks for the detailed review. I addressed the requested changes in
I also documented the omitted-resource limitation: per-node defaults are not currently exposed in Cluster verification passes the least-loaded tests 20 consecutive times, |
| if isSandboxCreation(hint) { | ||
| s.reserve(node, requestedCPU, requestedMemoryBytes, now) | ||
| } | ||
| return node, nil |
There was a problem hiding this comment.
[bug · medium]
The reservation is committed when the scheduler merely returns a node, but there is no cancellation API if the caller fails before the selected node receives/processes the creation request. Such a reservation cannot produce heartbeat acknowledgement and remains charged for the full default 10-minute TTL, skewing placement after transient dispatch/client failures. Add an explicit reservation lifecycle (token plus confirm/cancel), or use a substantially shorter lease that is renewed/confirmed once dispatch succeeds.
There was a problem hiding this comment.
Agreed; this cannot be made reliable as a local strategy-only change. The correct lifecycle needs a reservation token returned by Schedule, plus Gateway confirm/cancel calls on proxy success, upstream failure, client cancellation, and dispatch errors. That requires protobuf/API and reverse-proxy lifecycle changes across services. I have not marked this thread resolved. The current patch exposes a configurable stale lease as a bounded fallback, but I propose the token lifecycle as a follow-up RFC/PR unless maintainers want that protocol expansion in this PR.
There was a problem hiding this comment.
| if snapshot.GetCreateSuccesses() > state.observedCreateSuccesses { | ||
| state.acknowledgeCounts( | ||
| uint32(min( | ||
| snapshot.GetCreateSuccesses()-state.observedCreateSuccesses, | ||
| uint64(^uint32(0)), | ||
| )), | ||
| ) | ||
| } |
There was a problem hiding this comment.
[bug · medium]
These are node-wide cumulative counters, while pendingByNode is local to one strategy instance. A success generated by another scheduler replica or by unrelated node traffic can therefore acknowledge this instance's reservation; conversely, concurrent deletion can hide the corresponding positive allocation delta. This makes reconciliation attribute activity to the wrong request and under- or over-estimate pressure until TTL. Reconciliation needs a request/reservation identifier reported by the node (or another shared ownership mechanism), rather than inferring ownership from aggregate deltas.
There was a problem hiding this comment.
Agreed; aggregate node counters cannot provide ownership correlation across scheduler replicas or unrelated node traffic. A complete solution should bind a scheduler-issued reservation token to the resulting sandbox/assignment ID and reconcile through shared ownership state (or have the node report that identity). This is the same cross-service protocol extension as explicit confirm/cancel, so I have not marked this thread resolved and propose handling both together in a follow-up RFC/PR rather than claiming the aggregate heuristic is fully authoritative.
There was a problem hiding this comment.
| if isSandboxCreation(hint) { | ||
| s.reserve(node, requestedCPU, requestedMemoryBytes, now) | ||
| } | ||
| return node, nil |
There was a problem hiding this comment.
[bug · high]
This reservation is committed before the gateway has validated the selected endpoint or successfully dispatched the create request, and the strategy API provides no rollback path. A malformed endpoint, proxy/transport failure, client cancellation, or node-side rejection that occurs before the node increments create_fails leaves phantom CPU/memory charged until the configured TTL (10 minutes by default). Repeated failures can make every node fail fitsProjectedCapacity and cause a prolonged ErrNoNodes outage. Return a reservation token that the caller can cancel when dispatch fails, or move reservation commit/rollback into a lifecycle that observes the downstream outcome.
There was a problem hiding this comment.
| cpuDelta := positiveUint32Delta( | ||
| snapshot.GetAllocatedCpu(), | ||
| state.observedAllocatedCPU, | ||
| ) | ||
| memoryDelta := positiveUint64Delta( | ||
| snapshot.GetAllocatedMemoryBytes(), | ||
| state.observedAllocatedMemory, | ||
| ) | ||
| state.acknowledgeResources(float64(cpuDelta), float64(memoryDelta)) |
There was a problem hiding this comment.
[bug · medium]
Allocated CPU and memory are gauges, so a successful create does not necessarily produce a positive net delta: a concurrent deletion can offset the allocation, and a node restart can reset the gauges/counters. In either case create_successes may acknowledge the count while these resource reservations remain charged until TTL, causing false capacity exhaustion for up to 10 minutes. Gauge deltas cannot reliably correlate resources to creates; use per-request outcome/allocation data, or reconcile against a representation that tracks pending allocations explicitly and handles counter epochs/resets.
There was a problem hiding this comment.
|
Follow-up update:
The two directly fixed OCR threads are resolved. The protocol/ownership threads remain open intentionally for maintainer scope confirmation. Services CI is green; the final Integration and OpenCodeReview runs are still in progress/queued. |
| if target == 0 { | ||
| if isSandboxCreation(hint) { | ||
| s.reserve(node, requestedCPU, requestedMemoryBytes, now) | ||
| } | ||
| return node, nil | ||
| } |
There was a problem hiding this comment.
[bug · high]
This commits the reservation when scheduling succeeds, but the reachable Service.Schedule path only returns the node and exposes no completion/cancellation callback to this strategy. If the gateway request is canceled, proxying fails, or the selected node rejects sandbox creation, no heartbeat success can acknowledge the reservation, so it remains charged for the configured TTL (10 minutes by default). Repeated failed attempts can exhaust every node's projected capacity and cause ErrNoNodes despite no resources actually being allocated. Return a reservation handle and release it on downstream failure/cancellation, or add explicit outcome feedback with a much shorter fallback TTL.
| cpuDelta := positiveUint32Delta( | ||
| snapshot.GetAllocatedCpu(), | ||
| state.observedAllocatedCPU, | ||
| ) | ||
| memoryDelta := positiveUint64Delta( | ||
| snapshot.GetAllocatedMemoryBytes(), | ||
| state.observedAllocatedMemory, | ||
| ) | ||
| state.acknowledgeResources(float64(cpuDelta), float64(memoryDelta)) |
There was a problem hiding this comment.
[bug · medium]
These fields are aggregate gauges of the current active set, not per-creation counters. Their positive net delta cannot reliably reconcile a successful reservation: a concurrent deletion can offset the new allocation, and acknowledgeResources discards any credit that is smaller than an individual reservation at the end of each snapshot. In either case create_successes may acknowledge the count while CPU/memory remains double-counted until TTL, potentially rejecting valid placements. Reconciliation needs an operation/reservation identifier (preferred), or another cumulative per-resource signal; net gauge deltas are insufficient.
|
@LSX-s-Software To resolve the remaining correctness concern without hiding it behind TTL heuristics, I propose narrowing this PR to heartbeat-snapshot least-loaded scoring and removing the speculative reservation/reconciliation layer from the current merge scope. RFC #90 would then own the reservation token, explicit confirm/cancel lifecycle, replica-shared ownership, and identity-based reconciliation as a separate cross-service change. Would that split satisfy your requested changes for this PR? If you prefer the lifecycle to land atomically here, I will keep #27 blocked and implement #90 first. |
|
Thanks for the direction. I am closing this PR to keep the active review queue aligned with current priorities. The implementation branch, tests, and Lingjun benchmark data remain available if resource-aware scheduling is revisited; a future version should start from an explicitly approved scope rather than extending the current heuristic reservation layer. |
Summary
least_loadedscheduler strategy;Motivation
The scheduler already passes
RichNodeheartbeat snapshots and structuredScheduleRequestHintvalues to every strategy, but the current built-instrategies (
round_robinandrandom) do not use either signal. Onheterogeneous clusters, round-robin assigns the same number of sandboxes to
small and large nodes and can create severe normalized load skew.
This PR provides a general load-aware fallback that can also be reused by the
cache-locality work proposed in #15.
Selection model
For each eligible node:
The request projection is available for
/sandboxes-cold; other request typesuse the current allocation snapshot.
Candidates are ordered by:
Equal candidates are selected round-robin. For requests carrying a
sandbox-creation hint, selection and reservation happen under one strategy
mutex. Subsequent requests include pending CPU, memory, and sandbox-count
reservations in their projected load. Ordinary read/list routing does not
reserve capacity. Creation candidates whose active+paused allocation plus the
request and pending reservations exceeds advertised capacity are excluded; if
all candidates are full, scheduling returns no nodes. Partial capacity
snapshots retain sandbox counts for fallback ranking. Newer heartbeats are
detected with a scheduler-local receipt
generation rather than the node wall clock. The monotonic
create_successescounter reconciles projected sandbox count independently from compatible
running CPU/memory allocation deltas, so a sandbox entering
startingdoes notrelease its reservation early. Resource deltas apply only to reservations that
predate that heartbeat and already own a create-success acknowledgement;
acknowledgements are never transferred between differently sized reservations.
CPU and memory acknowledgement are tracked independently, so eventually
consistent fields can reconcile across heartbeats without carrying generic
credit into later reservations. Unmatched delta is discarded instead of
leaking into later selections. Aggregate
create_failschanges do not releasereservations because they cannot identify which request failed; without a
correlated outcome, reservations remain conservative until a compatible
allocation update or TTL. Reconciliation is linear in pending reservations.
Requests with no observed outcome use the configurable
scheduler.placement_reservation_ttl, which defaults to 10 minutes sooperators can cover their maximum queueing, creation, and heartbeat
acknowledgement delay.
When a cold-sandbox request omits CPU or memory, the node applies its locally
configured default. That default is not currently reported in
NodeSnapshot,so the scheduler reserves the sandbox count but cannot project the omitted
resource dimension. Template-based
NewSandboxrequests likewise lack resolvedCPU/memory hints; they are conservatively rejected when either known node
dimension is already full.
An explicit request-level reservation lifecycle, including confirm/cancel,
shared replica ownership, and template-resource resolution, is proposed in
#90.
Cluster validation
Validated on three 192-logical-CPU Intel Xeon Platinum 8575C nodes:
On lingjun-099, the final reservation-aware implementation selects among 1000
candidates in approximately
54.3 µs/opwith145–146 B/op, 0 allocs/op. The originalround-robin baseline is
7.926 ns/op.In a replay with 16/64/192-CPU nodes and 300 requests of 2 CPU / 4 GiB:
The maximum normalized pressure decreased by 82.25%.
Verification
Scope
This PR intentionally does not:
Those can be discussed separately under #15 and #90.