Skip to content

feat(scheduler): add projected least-loaded placement strategy - #27

Closed
catyans wants to merge 12 commits into
kvcache-ai:mainfrom
catyans:feat/least-loaded-scheduler
Closed

feat(scheduler): add projected least-loaded placement strategy#27
catyans wants to merge 12 commits into
kvcache-ai:mainfrom
catyans:feat/least-loaded-scheduler

Conversation

@catyans

@catyans catyans commented Jul 28, 2026

Copy link
Copy Markdown

Summary

  • add a built-in least_loaded scheduler strategy;
  • rank nodes by projected CPU/memory allocation pressure using heartbeat snapshots and cold-sandbox resource hints;
  • use starting, running, and paused sandbox counts as tie-breakers;
  • round-robin equally loaded candidates to avoid request concentration between heartbeats;
  • reserve projected placements atomically so concurrent requests observe in-flight selections;
  • order heartbeats with a scheduler-local generation, reconcile successful creates and running resource allocations independently, and provide a configurable conservative expiry for unconfirmed placements;
  • reserve only requests carrying a sandbox-creation hint;
  • expose the strategy through scheduler config and Prometheus labels;
  • document fallback behavior for nodes without usable heartbeat capacity.

Motivation

The scheduler already passes RichNode heartbeat snapshots and structured
ScheduleRequestHint values to every strategy, but the current built-in
strategies (round_robin and random) do not use either signal. On
heterogeneous 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:

cpu_pressure = (allocated_cpu + paused_allocated_cpu + requested_cpu) / cpu_capacity
memory_pressure = (allocated_memory + paused_allocated_memory + requested_memory) / memory_capacity
pressure = max(cpu_pressure, memory_pressure)

The request projection is available for /sandboxes-cold; other request types
use the current allocation snapshot.

Candidates are ordered by:

  1. usable observed capacity before unknown capacity;
  2. lower projected pressure;
  3. fewer starting sandboxes;
  4. fewer running sandboxes;
  5. fewer paused sandboxes.

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_successes
counter reconciles projected sandbox count independently from compatible
running CPU/memory allocation deltas, so a sandbox entering starting does not
release 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_fails changes do not release
reservations 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 so
operators 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 NewSandbox requests likewise lack resolved
CPU/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/op with 145–146 B/op, 0 allocs/op. The original
round-robin baseline is 7.926 ns/op.

In a replay with 16/64/192-CPU nodes and 300 requests of 2 CPU / 4 GiB:

peak allocation pressure: round_robin=12.5000 least_loaded=2.2188

The maximum normalized pressure decreased by 82.25%.

Verification

go test ./...
go test -race ./scheduler/internal
go vet ./...
git diff --check

Scope

This PR intentionally does not:

Those can be discussed separately under #15 and #90.

@catyans
catyans marked this pull request as ready for review July 28, 2026 03:16
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 2 issue(s) in this PR.

  • ✅ Successfully posted inline: 2 comment(s)

Comment thread services/scheduler/internal/strategy.go Outdated
Comment thread services/scheduler/internal/strategy.go Outdated
@catyans

catyans commented Jul 28, 2026

Copy link
Copy Markdown
Author

Latest head 94ac9d2 is mergeable and rebased on current main. Fork Services CI is green; final-head Integration Tests are running. The immediately preceding head passed the complete integration workflow, including docker-compose E2E. Upstream Services/Integration workflows still require maintainer approval for fork-originated runs.

@catyans
catyans force-pushed the feat/least-loaded-scheduler branch from 424aa04 to 0679b78 Compare July 28, 2026 09:49
Comment thread services/scheduler/internal/strategy.go Outdated

@LSX-s-Software LSX-s-Software left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. reserve sandbox A with 8 CPUs;
  2. reserve sandbox B with 1 CPU;
  3. B enters Creating first;
  4. 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.count

As 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.

@catyans

catyans commented Jul 30, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed review. I addressed the requested changes in 148a982 and 5d0addf:

  1. Reservation reconciliation now tracks sandbox-count and running CPU/memory acknowledgements independently. A starting sandbox clears only projected count; resources remain pending until compatible allocation deltas are observed. Differently sized reservations are matched by resource shape rather than FIFO, including when count and allocation arrive in separate heartbeats.
  2. Reservations are now created only for structured new_sandbox / new_cold_sandbox creation hints. Requests without a creation hint still use least-loaded routing but do not mutate reservation state. Added coverage that 64 non-placement selections leave pendingByNode empty, plus a count-only reservation test for new_sandbox.
  3. The implementation and its tests/benchmarks have been split into least_loaded.go and least_loaded_test.go; strategy.go now contains only the generic interface, round-robin, random, and strategy construction.

I also documented the omitted-resource limitation: per-node defaults are not currently exposed in NodeSnapshot, so an omitted cold-sandbox dimension cannot be projected yet, although the creation still reserves sandbox count. I agree an explicit success/cancel acknowledgement would be stronger than TTL for failed creates; the current scheduler protocol has no reservation identity, so this patch keeps the 30-second expiry as the failure fallback while eliminating reservations for all non-creation traffic.

Cluster verification passes the least-loaded tests 20 consecutive times, go test ./..., go test -race ./scheduler/internal, go vet ./..., and git diff --check. The 1000-node benchmark remains allocation-free at approximately 55.3 µs/op.

Comment thread services/scheduler/internal/least_loaded.go Outdated
Comment thread services/scheduler/internal/least_loaded.go Outdated
Comment thread services/scheduler/internal/least_loaded.go Outdated
Comment thread services/scheduler/internal/least_loaded.go Outdated
Comment thread services/scheduler/internal/least_loaded.go Outdated
Comment thread services/scheduler/internal/least_loaded.go
Comment thread services/shared/config/config.go Outdated
Comment thread services/scheduler/internal/least_loaded.go
Comment thread services/scheduler/internal/least_loaded.go
Comment thread services/scheduler/internal/least_loaded.go
Comment on lines +119 to +122
if isSandboxCreation(hint) {
s.reserve(node, requestedCPU, requestedMemoryBytes, now)
}
return node, nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formal follow-up RFC created as #90: #90. It specifies the reservation token, idempotent confirm/cancel operations, Gateway failure handling, leases, compatibility rollout, and identity-based acceptance tests. I am leaving this thread open for maintainer scope confirmation.

Comment on lines +149 to +156
if snapshot.GetCreateSuccesses() > state.observedCreateSuccesses {
state.acknowledgeCounts(
uint32(min(
snapshot.GetCreateSuccesses()-state.observedCreateSuccesses,
uint64(^uint32(0)),
)),
)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formal follow-up RFC created as #90: #90. It covers shared reservation ownership, atomic state transitions, binding to sandbox/assignment identity, replica races, and removal of aggregate attribution after rollout. I am leaving this thread open for maintainer scope confirmation.

Comment thread services/scheduler/internal/least_loaded.go
Comment on lines +134 to +137
if isSandboxCreation(hint) {
s.reserve(node, requestedCPU, requestedMemoryBytes, now)
}
return node, nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This dispatch gap is now tracked by the explicit lifecycle RFC in #90: #90. It assigns cancel responsibility to the Gateway for endpoint validation, proxy/transport failure, client cancellation, and node rejection, with TTL retained only as crash recovery.

Comment on lines +172 to +180
cpuDelta := positiveUint32Delta(
snapshot.GetAllocatedCpu(),
state.observedAllocatedCPU,
)
memoryDelta := positiveUint64Delta(
snapshot.GetAllocatedMemoryBytes(),
state.observedAllocatedMemory,
)
state.acknowledgeResources(float64(cpuDelta), float64(memoryDelta))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Gauge deltas remain only a conservative compatibility heuristic in this PR; they cannot be authoritative. RFC #90 binds confirmation to a reservation token plus sandbox/assignment identity and explicitly covers concurrent deletion and node restart/counter reset tests: #90.

Comment thread services/scheduler/internal/least_loaded.go Outdated
Comment thread services/scheduler/internal/least_loaded.go
@catyans

catyans commented Jul 31, 2026

Copy link
Copy Markdown
Author

Follow-up update:

  • Created RFC RFC: explicit placement reservation lifecycle and shared ownership #90 for the cross-service reservation token, explicit confirm/cancel lifecycle, shared replica ownership, identity binding, and template-resource resolution: RFC: explicit placement reservation lifecycle and shared ownership #90
  • Pushed 4fb1ac2: aggregate create_fails no longer releases an uncorrelated reservation, and template-based creates are rejected when an unknown resource dimension is already at known capacity.
  • Added regressions for out-of-order differently sized reservations and CPU-/memory-full template creates.
  • Cluster verification passed: go test ./scheduler/internal -count=50, go test ./..., go test -race ./scheduler/internal, and go vet ./....

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.

Comment on lines +143 to +148
if target == 0 {
if isSandboxCreation(hint) {
s.reserve(node, requestedCPU, requestedMemoryBytes, now)
}
return node, nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment on lines +174 to +182
cpuDelta := positiveUint32Delta(
snapshot.GetAllocatedCpu(),
state.observedAllocatedCPU,
)
memoryDelta := positiveUint64Delta(
snapshot.GetAllocatedMemoryBytes(),
state.observedAllocatedMemory,
)
state.acknowledgeResources(float64(cpuDelta), float64(memoryDelta))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@catyans

catyans commented Jul 31, 2026

Copy link
Copy Markdown
Author

@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.

@catyans

catyans commented Aug 5, 2026

Copy link
Copy Markdown
Author

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.

@catyans catyans closed this Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants