Planner scheduler and simulator - #2489
Open
akheffache wants to merge 28 commits into
Open
Conversation
…tery A tick-driven, in-process scheduler (scheduler.enabled) that plans placements by E-PVM opportunity cost instead of relying on the report-driven dispatcher: - E-PVM placement scored on cores/memory/GPU stranding, with a same-host locality bonus so a completing proc's cores tend to refill from the same layer. - Whole-host reservations + EASY backfill so wide jobs never strand behind narrow work; dispatcher.frame_cores_max raises the per-frame core clamp for whole-host jobs. - Priority-weighted lottery (Efraimidis-Spirakis) for booking order, so a sustained high-priority backlog cannot starve lower-priority work. Priority becomes a rate, not a rank (see Scheduler.md section 3.5). - License limits and folder/group core ceilings honored in-tick. The candidate query skips layers whose limit (limit_record.int_max_value) is full or whose folder (folder_resource.int_max_cores) is at its ceiling, so no doomed bookings are planned. Limits stay exact through the existing downstream frame query; the folder ceiling is held exactly by a pre-commit pass that trims any planned frame which would push a capped folder past its cap (planHost has no folder clause, so the batch must be trimmed before it commits). - Per-frame OOM memory handling: bump the offending frame and escalate the whole layer only after repeated OOMs, instead of ratcheting the layer on every kill. - Batch-commit robustness: skip a full host instead of aborting the whole tick, plus reserved/backfilled-core instrumentation on the scheduler stat line. Gated behind scheduler.enabled: with the scheduler off and booking on, the legacy dispatcher path is unchanged. The standalone-scheduler handoff mode (scheduler off + dispatcher.turn_off_booking=true) additionally suppresses FrameCompleteHandler's reactive rebook paths, so cuebot only reconciles RQD reports while an external planner owns booking.
A one-command simulator that runs a real cuebot + Postgres against a fake RQD
and a synthetic farm (up to ~1553 hosts) to exercise the scheduler under load:
- Workload feeders: steady fill, priority streams, and wide-job strand tests;
a per-layer memory model (baseline + jitter) so a layer's frames cluster
realistically instead of drawing memory independently.
- Live stats and end-of-run graphs (utilization, throughput, cores-vs-memory,
reservation subsystem, DB load), each stamped with the simulate.py command
line so every graph is traceable to the config that produced it.
- --strand-cores for whole-host (128-core) reservation tests.
--verify is the recommended way to run it: one command that exercises the
scheduler end to end. It runs six scenarios back-to-back, each a fresh, fully
torn-down sim that writes its own graphs, then prints a PASS/FAIL summary
(nonzero exit if any scenario fails):
OOM -- memory failures bump the layer's memory per-frame,
no legacy ratchet, and frames retry
PRIORITY -- completion share is ordered by priority across 10
classes (Spearman rho)
PRIORITY_STARVING -- a low-priority stream survives a high-priority flood
(stays above a 3% floor)
RESERVATIONS -- stranded wide jobs are rescued by reservations +
backfill and actually run
LIMIT -- a global license cap (limit_record.int_max_value) holds
concurrent running frames at the cap under a deep backlog
FOLDER -- a folder/group core ceiling (folder_resource.int_max_cores)
holds the folder's running cores at the cap under a deep
backlog
Run it exactly as `simulate.py --verify`, with no other flags. Each scenario is
tuned (farm size, oversubscription, frame length) so its verdict is meaningful;
changing the options on a --verify run is unsupported and easily misleading
(e.g. PRIORITY only shows proportional shares on a small, heavily oversubscribed
farm -- on the full farm priority looks absent though the scheduler is correct).
Only SIM_VERIFY_SECONDS (per-scenario length) is meant to be adjusted.
The simulator's what/how, the --verify guidance, and the full flag reference
live in cuebot/scheduler-sim/README.md.
akheffache
requested review from
DiegoTavares,
lithorus and
ramonfigueiredo
as code owners
July 21, 2026 00:47
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a whole-farm scheduler with batched planning, resource accounting, reservations, backfill, completion handling, and license gating. Adds a reproducible scheduler simulation with workload injectors, fake services, monitoring, benchmarking, graphing, and verification scenarios. ChangesScheduler simulation and workload harness
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java (1)
1086-1100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
plannedByHostis only cleared after the plan tasks are built, so it can carry stale placements across an aborted tick.If
doTickthrows anywhere in the group placement loop aftersubmitCommithas populatedplannedByHostbut before Line 1100,runTick'scatch (RuntimeException | SQLException)swallows it and the entries survive. The next tick appends to them and only then clears, so it replans(host, layer)pairs chosen under a snapshot that was never committed.planHostre-reads current frames and host capacity is re-verified, so this won't over-book, but it does book layers on hosts they were not scored for this tick. Consider resetting at the top ofdoTick(alongside the other per-tick resets) or in afinally.♻️ Reset at tick start
private int doTick() { long tStart = System.currentTimeMillis(); // Reset per-tick stat outputs before any early return, so a host-less // tick contributes zero to the window rather than last tick's values. + plannedByHost.clear(); tickPlanned = 0;🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java` around lines 1086 - 1100, Reset plannedByHost at the beginning of doTick alongside the other per-tick state resets, before any placement or submitCommit work occurs. Ensure each tick starts with an empty host-to-layer plan, including ticks following a RuntimeException or SQLException, and retain the existing clear after task construction if it remains useful.cuebot/scheduler-sim/stats.py (1)
18-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor Python style nits from static analysis.
perat Line 120 is a lambda assigned to a name (Ruff E731 — preferdef); many lines use;-chained statements (E702); Line 148's f-string has no placeholders (F541).🤖 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 `@cuebot/scheduler-sim/stats.py` around lines 18 - 153, Resolve the static-analysis style issues in the stats script: replace the `per` lambda in the DB load reporting block with a named function, split semicolon-chained statements throughout the visible helpers and sampling/reporting flow into separate statements, and remove the unnecessary f-string prefix from the fixed “running-frame size mix” output.Source: Linters/SAST tools
cuebot/scheduler-sim/BUILD.md (1)
16-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFenced code blocks missing a language hint.
markdownlint flags several fenced blocks (Lines 16, 25, 30, 47, 65, 78) without a language identifier (MD040).
🤖 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 `@cuebot/scheduler-sim/BUILD.md` around lines 16 - 78, Update the fenced code blocks in the scheduler simulation documentation to include appropriate language identifiers, satisfying MD040. Add shell-specific hints to command blocks and use the appropriate plain-text hint for non-shell content, while preserving all commands and documentation unchanged.Source: Linters/SAST tools
cuebot/scheduler-sim/fake_rqd.py (1)
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
osis already imported at line 31, so this re-import is redundant and can be dropped.🤖 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 `@cuebot/scheduler-sim/fake_rqd.py` around lines 83 - 85, Remove the redundant os import near the _REPORTER_THREADS definition, preserving the existing os import and the current environment-variable fallback behavior.cuebot/scheduler-sim/rqd_complete.py (1)
69-111: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
_seenonly ever grows (entries are removed just on RPC failure). Over a long dry-run the set accumulates every bookedpk_procfor the process lifetime. Fine at sim scale, but for extended runs you may want to bound it or clear it periodically.🤖 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 `@cuebot/scheduler-sim/rqd_complete.py` around lines 69 - 111, Bound the lifetime of _seen in poll_new so it does not retain every booked pk_proc indefinitely during extended runs. Remove entries once their scheduled simulation has completed or otherwise implement periodic cleanup, while preserving the duplicate-scheduling guard and ensuring active heap entries cannot be rediscovered prematurely.cuebot/scheduler-sim/simulate.py (1)
687-833: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
start_cuebotandstart_extra_cuebotduplicate a large block (the-Djdk.net.hosts.file/nonProxyHostscomment,java_tool_optsconstruction, and theSCHEDULER_*env map). Extracting a shared_cuebot_env(...)helper would keep the two launch paths from drifting. Optional given this is sim tooling.🤖 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 `@cuebot/scheduler-sim/simulate.py` around lines 687 - 833, Extract the duplicated environment setup from start_cuebot and start_extra_cuebot into a shared _cuebot_env(...) helper, including JAVA_TOOL_OPTIONS construction, database settings, scheduler configuration, and common optional overrides. Have both launch functions call the helper while preserving their distinct listener ports and booking behavior, preventing the two paths from drifting.cuebot/scheduler-sim/register_hosts.py (1)
24-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
bootstraphelper
__main__only callsregister_hosts(), and base entities are seeded viasim_seed.sql. Delete this path if it’s obsolete; otherwise add a brief note on when it should be used.🤖 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 `@cuebot/scheduler-sim/register_hosts.py` around lines 24 - 62, Remove the unused bootstrap function and its facility, allocation, show, subscription, and gRPC setup logic from register_hosts.py, since __main__ only invokes register_hosts() and sim_seed.sql seeds the base entities. Do not add replacement behavior or documentation.cuebot/scheduler-sim/analysis/db_sampler.py (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHonor
SIM_PG_BINfor the psql path (consistency withutil_sampler.py).util_sampler.pydeliberately resolves a full psql path viaSIM_PG_BINbecause a non-root sim user may have a minimalPATHlacking the postgres bin dir;db_sampler.pyis started alongside it bysimulate.pybut invokes bare"psql", so it can fail to launch under that same environment.♻️ Align psql resolution with util_sampler.py
_PORT = os.environ.get("SIM_PG_PORT", "5433") _HOST = os.environ.get("SIM_PG_HOST", "127.0.0.1") -PSQL = ["psql","-tA","-h",_HOST,"-p",_PORT,"-U","cue","-d","cuebot","-c"] +_BIN = os.environ.get("SIM_PG_BIN", "/usr/lib/postgresql/16/bin") +PSQL = [f"{_BIN}/psql","-tA","-h",_HOST,"-p",_PORT,"-U","cue","-d","cuebot","-c"]🤖 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 `@cuebot/scheduler-sim/analysis/db_sampler.py` around lines 3 - 6, Update the psql command construction near PSQL and q to resolve the executable through the SIM_PG_BIN environment variable, matching util_sampler.py’s behavior, while preserving the existing host, port, user, database, and SQL arguments.
🤖 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 `@cuebot/scheduler-sim/analysis/make_graphs.py`:
- Around line 53-54: The positional dbstat readers use incorrect column indexes
for active and lockwait. In cuebot/scheduler-sim/analysis/make_graphs.py lines
53-54, widen the row slice to include all 12 post-timestamp columns, guard rows
shorter than 13 fields, map lockwait to b[11] and active to b[10], and update
the related comment. Apply the same changes in
cuebot/scheduler-sim/analysis/make_graphs_ba.py lines 56-57. In
cuebot/scheduler-sim/analysis/analyze_sweep.py line 62, change the lockwait
median index to 11 and correct the comment to document active=10 and
lockwait=11.
In `@cuebot/scheduler-sim/drain_test.py`:
- Around line 25-43: Rename the local XML string variable assigned in main()
before the LaunchSpec loop so it no longer shadows the imported spec module used
by spec.GRPC. Update all subsequent references in the range parsing, request
construction, and retry call to use the renamed variable.
In `@cuebot/scheduler-sim/gen_jobs.py`:
- Around line 94-96: Replace the hardcoded 57248 farm-size value in the
demand-ratio print with the dynamically computed result from
farm_spec.total_cores(), so the reported comparison reflects the configured host
types and SIM_HOST_COUNTS. Keep the existing demand and ratio formatting
unchanged.
In `@cuebot/scheduler-sim/metrics.py`:
- Around line 49-59: Replace the hardcoded 1553 host-count denominator in the
live metrics and final summary output with the configured host count used by the
simulator, preserving the existing formatting and small-farm behavior. Update
the relevant print statements in the metrics reporting flow and reuse the shared
configuration symbol rather than duplicating the value.
In `@cuebot/scheduler-sim/stats.py`:
- Around line 104-105: Update the honest-peak output in stats.py to replace the
hardcoded 1553 denominator in the busyHosts display with the host-count value
selected for the active farm mode, including small-farm mode. Reuse the existing
shared configuration or variable used by the scheduler simulation rather than
introducing a separate constant.
In `@cuebot/scheduler-sim/status_pinger.py`:
- Around line 25-33: Update render_host in cuebot/scheduler-sim/status_pinger.py
and the corresponding render_host implementation in
cuebot/scheduler-sim/status_pinger_fast.py to set tags using
spec.host_tags(name) instead of the fixed spec.TAG value, preserving
host-specific capability tags when SIM_NTAGS is at least 2.
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.java`:
- Around line 440-491: Remove the pickupStrandedCores(host) call and related
early break from planHost’s planning loop. Defer stranded-core pickup to the
post-commit path, invoking it only after the corresponding FrameBooking batch
has been successfully committed so dropped bookings do not consume stranded-core
state.
---
Nitpick comments:
In `@cuebot/scheduler-sim/analysis/db_sampler.py`:
- Around line 3-6: Update the psql command construction near PSQL and q to
resolve the executable through the SIM_PG_BIN environment variable, matching
util_sampler.py’s behavior, while preserving the existing host, port, user,
database, and SQL arguments.
In `@cuebot/scheduler-sim/BUILD.md`:
- Around line 16-78: Update the fenced code blocks in the scheduler simulation
documentation to include appropriate language identifiers, satisfying MD040. Add
shell-specific hints to command blocks and use the appropriate plain-text hint
for non-shell content, while preserving all commands and documentation
unchanged.
In `@cuebot/scheduler-sim/fake_rqd.py`:
- Around line 83-85: Remove the redundant os import near the _REPORTER_THREADS
definition, preserving the existing os import and the current
environment-variable fallback behavior.
In `@cuebot/scheduler-sim/register_hosts.py`:
- Around line 24-62: Remove the unused bootstrap function and its facility,
allocation, show, subscription, and gRPC setup logic from register_hosts.py,
since __main__ only invokes register_hosts() and sim_seed.sql seeds the base
entities. Do not add replacement behavior or documentation.
In `@cuebot/scheduler-sim/rqd_complete.py`:
- Around line 69-111: Bound the lifetime of _seen in poll_new so it does not
retain every booked pk_proc indefinitely during extended runs. Remove entries
once their scheduled simulation has completed or otherwise implement periodic
cleanup, while preserving the duplicate-scheduling guard and ensuring active
heap entries cannot be rediscovered prematurely.
In `@cuebot/scheduler-sim/simulate.py`:
- Around line 687-833: Extract the duplicated environment setup from
start_cuebot and start_extra_cuebot into a shared _cuebot_env(...) helper,
including JAVA_TOOL_OPTIONS construction, database settings, scheduler
configuration, and common optional overrides. Have both launch functions call
the helper while preserving their distinct listener ports and booking behavior,
preventing the two paths from drifting.
In `@cuebot/scheduler-sim/stats.py`:
- Around line 18-153: Resolve the static-analysis style issues in the stats
script: replace the `per` lambda in the DB load reporting block with a named
function, split semicolon-chained statements throughout the visible helpers and
sampling/reporting flow into separate statements, and remove the unnecessary
f-string prefix from the fixed “running-frame size mix” output.
In `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java`:
- Around line 1086-1100: Reset plannedByHost at the beginning of doTick
alongside the other per-tick state resets, before any placement or submitCommit
work occurs. Ensure each tick starts with an empty host-to-layer plan, including
ticks following a RuntimeException or SQLException, and retain the existing
clear after task construction if it remains useful.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd1d5b1e-3259-49ed-8f18-bb5a1f0b49e3
📒 Files selected for processing (68)
cuebot/scheduler-sim/.gitignorecuebot/scheduler-sim/BUILD.mdcuebot/scheduler-sim/README.mdcuebot/scheduler-sim/analysis/.gitignorecuebot/scheduler-sim/analysis/README.mdcuebot/scheduler-sim/analysis/analyze_sweep.pycuebot/scheduler-sim/analysis/cpu_sampler.pycuebot/scheduler-sim/analysis/db_sampler.pycuebot/scheduler-sim/analysis/make_graphs.pycuebot/scheduler-sim/analysis/make_graphs_ba.pycuebot/scheduler-sim/analysis/plot_run.pycuebot/scheduler-sim/analysis/util_sampler.pycuebot/scheduler-sim/drain_test.pycuebot/scheduler-sim/fake_rqd.pycuebot/scheduler-sim/farm_spec.pycuebot/scheduler-sim/feed.pycuebot/scheduler-sim/folder_watch.pycuebot/scheduler-sim/gen_jobs.pycuebot/scheduler-sim/inject_big.pycuebot/scheduler-sim/inject_folder.pycuebot/scheduler-sim/inject_limit.pycuebot/scheduler-sim/inject_priority_spread.pycuebot/scheduler-sim/inject_priority_starve.pycuebot/scheduler-sim/kill_all_jobs.pycuebot/scheduler-sim/limit_watch.pycuebot/scheduler-sim/live_stats.pycuebot/scheduler-sim/metrics.pycuebot/scheduler-sim/priority_spread_watch.pycuebot/scheduler-sim/priority_starve_watch.pycuebot/scheduler-sim/register_hosts.pycuebot/scheduler-sim/resolve_local.ccuebot/scheduler-sim/rqd_complete.pycuebot/scheduler-sim/rqd_report.pycuebot/scheduler-sim/setup.shcuebot/scheduler-sim/sim_mem.pycuebot/scheduler-sim/sim_metrics.pycuebot/scheduler-sim/sim_model.pycuebot/scheduler-sim/sim_seed.sqlcuebot/scheduler-sim/simulate.pycuebot/scheduler-sim/stats.pycuebot/scheduler-sim/status_pinger.pycuebot/scheduler-sim/status_pinger_fast.pycuebot/scheduler-sim/strand_dur_watch.pycuebot/scheduler-sim/strand_watch.pycuebot/scheduler-sim/util_test.pycuebot/src/main/java/com/imageworks/spcue/VirtualProc.javacuebot/src/main/java/com/imageworks/spcue/dao/FrameDao.javacuebot/src/main/java/com/imageworks/spcue/dao/ProcDao.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatcherDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/Dispatcher.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameBooking.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/HostReportHandler.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/LocalDispatcher.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/OomMemoryTracker.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.mdcuebot/src/main/java/com/imageworks/spcue/dispatcher/SchedulerMode.javacuebot/src/main/java/com/imageworks/spcue/rqd/RqdClientGrpc.javacuebot/src/main/java/com/imageworks/spcue/service/JobSpec.javacuebot/src/main/resources/conf/spring/applicationContext-service.xmlcuebot/src/main/resources/opencue.propertiescuebot/src/test/java/com/imageworks/spcue/dispatcher/SchedulerTests.java
The new scheduler files and touched shared files were written wrapped at ~80 columns; the project's spotless config (eclipse JDT, jdtls.xml) wraps javadoc and code at 100. Formatting only -- no behavior change.
…res branch - doTick: clear plannedByHost with the other per-tick resets. It is normally drained every tick, but a tick that throws mid-placement left stale (host, layer) pairings that the next tick would plan against a fresh snapshot. - planHost: remove the stranded-cores block copied from dispatchHost. The plan phase must stay read-only, and the branch was unreachable anyway: the only strandCores() producer is the legacy completion path (gated off by bookingOff in facility mode), and it sets the field on its own DispatchHost instance while the planner works from freshly DAO-loaded hosts that never carry it. The tick planner also replans every tick, which subsumes the legacy refill-freed-cores-fast behaviour stranded cores existed for.
… tags Address review comments on the scheduler-sim harness: * drain_test.py: the per-job local `spec` shadowed `import farm_spec as spec`, so `spec.GRPC` on the first line of main() raised UnboundLocalError -- the drain test crashed on startup. Rename the local to `job_spec`. * make_graphs.py, make_graphs_ba.py, analyze_sweep.py: the DB-sampler CSV carries blks_read/blks_hit before active/lockwait, but the readers still mapped active/lockwait to indices 8/9 -- plotting blks_read and blks_hit instead. Remap to 10/11 and fix the column-legend comment. * status_pinger.py, status_pinger_fast.py: hosts were registered with tags=[TAG], dropping the per-host capability tag, so the SIM_NTAGS>=2 fragmentation model silently did nothing. Use host_tags(name). * gen_jobs.py, metrics.py, stats.py: replace hardcoded farm sizes (57248 cores, /1553 hosts) with farm_spec.total_cores()/total_hosts() so they track SIM_HOST_COUNTS small-farm mode. * db_sampler.py: resolve psql via SIM_PG_BIN instead of a bare "psql". * BUILD.md: add language hints to fenced code blocks and drop a stray trailing code fence.
…rify stage
Wire up limit_record.b_host_limit (schema-only since V2, never read by any
code path): when true, the limit's int_max_value counts DISTINCT HOSTS
("seats") instead of running frames. Every frame of the limit's layers on a
seated host shares that host's one seat -- the classic per-machine floating
license. At the seat cap, seated hosts may take more of the limit's frames;
only NEW hosts are blocked.
Scheduler (the E-PVM planner):
- Candidate query projects lim.b_host_limit and exempts host limits from the
frame-count efficiency filter (their frame count is unbounded by design).
- readLimitHostSeats(): per-tick limitId->seated-hosts map, derived from proc
(a seat exists while >=1 proc of the limit's layers runs on the host, so
seats need no bookkeeping or release logic). Kept separate from the
locality-gated affinity read: near-free when no host limits exist.
- Seat gate in the scoring loop: at the cap, only seated hosts are eligible.
The map is DB-seeded then accumulated in-tick, so one tick can never open
more seats than remain; a single planner runs per tick (advisory lock).
- Seat bonus (scheduler.host_limit_seat_bonus, default 16.0): seated hosts
score above the max E-PVM spread (~7*(e-1)), packing licensed work onto the
fewest hosts. Stacks with the per-layer locality bonus.
- Host limits skip the frame-headroom clamp and the frame-count capped test;
a layer at its seat cap that cannot place is treated like capped (no
reservation debt -- reserving new hosts could never help it).
Planner-only SQL variants (the legacy dispatcher's statements are untouched,
byte-identical to before; host limits are supported ONLY under the planner):
- FIND_DISPATCH_FRAME_BY_LAYER_AND_HOST_PLANNER (used only by planHost, via
findNextDispatchFramesPlanner): host-limit arm passes if the candidate
host already holds a seat (EXISTS on proc, correlated to the already-
joined host) or a seat is still free (COUNT(DISTINCT pk_host)). Without
this arm the frame-count gate would return zero frames the moment running
frames exceed the numeric cap, making seat sharing impossible.
- UPDATE_FRAME_STARTED_PLANNER (used only by the planner's batch commit):
host limits bypass the frame-count gate (their cap is enforced by the
planner + the plan-read arm above).
Sim harness, new LIMIT_HOST --verify scenario (the suite is now 7 scenarios):
- inject_limit.py/limit_watch.py: SIM_LIMIT_HOST=1 flips the injected limit
to b_host_limit=true and the watcher to distinct-host sampling (from proc,
the same view the enforcement uses).
- simulate.py: --limit-host-test SECS (cap defaults to SIM_LIMIT_MAX=5
seats), LIMIT_HOST verify scenario + verdict: PASS only if peak distinct
hosts <= cap AND running frames peak >= 10x cap (proof seats are shared)
under a deep backlog.
Verified on the sim: the full 7-scenario --verify suite passes (OOM,
PRIORITY, PRIORITY_STARVING, RESERVATIONS, LIMIT, LIMIT_HOST, FOLDER), plus
a full-farm sustained-load run (1553 hosts, --compress 8: steady-state util
86%, ~178 done/s) with the feature code in place. Key targeted results
(17-host farm, deep 1-core flood):
- LIMIT regression: peak concurrent running exactly 50 vs cap 50 (the
rewritten gates change nothing for frame limits).
- LIMIT_HOST: peak distinct hosts 5 vs cap 5, running peaked at 448 (~90x
the cap; the 3 large + 2 medium seated hosts' full core capacity), 8.4k
frames waiting. Seat grants logged 1..5/5, none after.
The legacy dispatcher, redirects and local dispatch keep the original
statements and treat a host limit as an over-strict frame cap (safe,
self-healing: the planner re-books such frames next tick); cueweb/API
surfacing of b_host_limit is deferred.
… 10) LOCALITY (full farm): measures REFILL AFFINITY -- of newly booked procs whose layer already ran somewhere, the fraction landing on a host already running that layer, i.e. exactly the decision scheduler.locality_bonus biases. Empirically calibrated: bonus-ON measures ~29-36% on the full farm, bonus-OFF ~1.3% (accidental rate); the PASS floor is 15% (SIM_LOCALITY_MIN_HIT). locality_watch.py; control runs via SIM_LOCALITY_ENABLED=false, which start_cuebot now forwards as SCHEDULER_LOCALITY_ENABLED. DEPENDS (full farm): asserts the invariant that no frame is ever RUNNING with unsatisfied depends (frame.int_depend_count > 0), sampled continuously, plus coverage floors proving the machinery cycled (depends satisfied, previously gated frames ran). Full farm on purpose: parents must complete within the watch window for children to unblock; on a mini farm coverage stays zero. depend_watch.py. FAILOVER (full farm): SIGKILLs the leader cuebot (instance 0, which holds the advisory lock) at half-time and asserts FULL-SERVICE failover: the standby must book new frames (>= SIM_FAILOVER_MIN_STARTED, default 100) AND accept new job submissions (>= SIM_FAILOVER_MIN_JOBS, default 3, counted as the job-table delta -- job.ts_started is not submission time). Every sim client re-dials the survivor via SIM_CUEBOT_GRPC_FALLBACKS, like a real farm's multi-cuebot client config: fake_rqd fails its completion stub over (frame timers live in-process, so running frames survive the kill), rqd_report rotates when a whole round fails, and feed.py rotates on launch failure. A post-kill probe feeder (open backlog gate) guarantees live submission traffic even when the long feeder is legitimately idle at its target. Cuebot launches now record PID files (/tmp/sim-cuebot-<n>.pid) so the test can target the leader. Independent jobs (--dep-tree-depth 1): with dep trees the feeder's DEPEND-backlog cap silences it and the submission assertion is unexercisable. Measured on this box: FAILOVER standby booked 21k frames and accepted 52 submissions after the kill on the full farm, with DB commit rate carrying over to the survivor (15.8k -> 18.6k commits/s). Scenario farm sizes are deliberate: LOCALITY/DEPENDS/FAILOVER (like OOM and PRIORITY_STARVING) run the FULL 1553-host farm for realism; the cap tests (PRIORITY, RESERVATIONS, LIMIT, LIMIT_HOST, FOLDER) keep their tuned mini farms where the cap or scarcity must bind for the verdict to mean anything.
…uite now 11) One MIXED run on the FULL farm where the two fragmentations INTERSECT: 8 random capability tags scatter across the hosts (--tags 8) AND 25% of hosts/ layers are GPU (--gpu 0.25), so a GPU layer tagged capN fits only hosts that are BOTH GPU-capable and tagged capN, while plain tagged CPU work competes on the same machines. tag_gpu_watch.py samples continuously and the verdict asserts: - zero tag-placement violations (the same host.str_tags ~* layer.str_tags regex predicate cuebot's dispatch query uses); - zero procs of GPU layers on GPU-less hosts; - no host oversubscribed on GPU units or GPU memory (SUM(proc.int_gpus_reserved) <= host.int_gpus, same for gpu_mem), and no negative int_gpus_idle / int_gpu_mem_idle; - coverage floors so the verdict cannot pass vacuously: peak GPU procs >= SIM_TAGGPU_MIN_GPU (default 50) and ALL N tag pools ran work; - GPU utilization floor: peak GPU-unit utilization >= SIM_TAGGPU_MIN_UTIL (default 40%), computed over GPU-CAPABLE HOSTS ONLY -- most of the farm has no GPUs and a farm-wide percentage would be meaningless. GPU utilization is also GRAPHED: the watcher writes run_taggpu.csv and analysis/plot_run.py renders a dedicated <tag>_gpu.png panel (GPU-unit util% + GPU-memory util% over GPU hosts, GPU procs on the twin axis). Tag demand is forced uniform for this scenario (SIM_TAG_SKEW=1.0): farm_spec's steep default skew (0.3) is a deliberate starvation profile for stranding studies, under which cold pools get ~zero jobs and the every-pool coverage floor can never be met. Fragmentation still binds -- each job stays confined to its ~1/N slice. Measured on this box (full farm, 180s): PASS with zero violations, peak 1361 GPU procs = 72.5% of GPU units (30.5% of GPU memory -- the mix is unit-bound, not mem-bound, and the new panel shows exactly that), all 8 pools active.
Contributor
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 (2)
cuebot/scheduler-sim/simulate.py (1)
809-825: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
SCHEDULER_LOCALITY_ENABLEDis not propagated to extra cuebot instances.
start_cuebotsets it (line 730) but this env block omits it. With the default--cuebots 2, instance 1 wins the advisory lock on roughly half the ticks and plans with cuebot's default locality bonus regardless ofSIM_LOCALITY_ENABLED, so the bonus-OFF control run the 0.15 floor was calibrated against is contaminated.🔧 Proposed fix
"SCHEDULER_BACKFILL_ENABLED": "true" if backfill else "false", + "SCHEDULER_LOCALITY_ENABLED": os.environ.get("SIM_LOCALITY_ENABLED", "true"), "SCHEDULER_STAT_INTERVAL_SECONDS": os.environ.get("SIM_STAT_INTERVAL_SECONDS", "30"),🤖 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 `@cuebot/scheduler-sim/simulate.py` around lines 809 - 825, Update the environment mapping in start_cuebot so extra cuebot instances also receive SCHEDULER_LOCALITY_ENABLED, using the same value derived from SIM_LOCALITY_ENABLED as the primary instance. Preserve the existing locality configuration and ensure all instances honor the simulation setting.cuebot/scheduler-sim/gen_jobs.py (1)
83-97: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
speclocal shadows thefarm_specmodule —spec.total_cores()raisesAttributeError.Line 83 rebinds
specto the job XML string for every iteration, so by line 94specis astrandspec.total_cores()fails wheneverNUM_JOBS >= 1.drain_test.pyalready names the same valuejob_spec; do the same here.🐛 Proposed fix
for i in range(NUM_JOBS): random.seed(1000 + i) - spec = SPEC_HEAD + make_job(i) + "</spec>\n" - resp = stub.LaunchSpec(job_pb2.JobLaunchSpecRequest(spec=spec)) + job_spec = SPEC_HEAD + make_job(i) + "</spec>\n" + resp = stub.LaunchSpec(job_pb2.JobLaunchSpecRequest(spec=job_spec)) # tally demand from the spec we just built - for line in spec.splitlines(): + for line in job_spec.splitlines(): line = line.strip() if line.startswith("<cores>"): cp = int(line[len("<cores>"):-len("</cores>")]) total_core_demand += cp if line.startswith("<range>1-"): total_frames += int(line[len("<range>1-"):-len("</range>")]) total_layers += 1 farm_cores = spec.total_cores()🤖 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 `@cuebot/scheduler-sim/gen_jobs.py` around lines 83 - 97, Rename the per-job XML string local in the job submission loop from spec to job_spec, updating its construction, LaunchSpec request, and parsing references. Preserve the farm_spec module reference so the later total_cores() call resolves correctly.
🧹 Nitpick comments (2)
cuebot/scheduler-sim/analysis/analyze_sweep.py (1)
62-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the semicolon-chained assignments.
Ruff reports E702 here. Put each metric assignment on its own line to keep the mapping readable and lint-clean.
Proposed fix
- reads=rd(); writes=wr(); rb=med_csv(pre,"dbstat",1,LO,HI,True); lw=med_csv(pre,"dbstat",11,LO,HI,False) + reads = rd() + writes = wr() + rb = med_csv(pre, "dbstat", 1, LO, HI, True) + lw = med_csv(pre, "dbstat", 11, LO, HI, False)🤖 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 `@cuebot/scheduler-sim/analysis/analyze_sweep.py` at line 62, Split the chained assignments in the sweep analysis flow into separate lines: assign reads via rd(), writes via wr(), rb via med_csv(...), and lw via med_csv(...) independently, preserving their existing order and arguments.Source: Linters/SAST tools
cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.java (1)
194-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the imported
Listfor consistency.Every other declaration in this interface uses the unqualified
List;java.util.Listis already imported.♻️ Proposed tweak
- public java.util.List<FrameBooking> startFramesAndProcsBatch( - java.util.List<FrameBooking> bookings); + public List<FrameBooking> startFramesAndProcsBatch(List<FrameBooking> bookings);🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.java` around lines 194 - 195, Update the startFramesAndProcsBatch method declaration to use the imported List type instead of the fully qualified java.util.List, matching the other interface declarations.
🤖 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 `@cuebot/scheduler-sim/simulate.py`:
- Around line 1998-2052: Track successful leader termination in the failover
block around the os.kill call, setting the state only after the kill completes
and leaving it false when pid-file reading or termination raises. Require that
state in the final PASS condition and report a failure verdict when the leader
was not killed, while preserving the existing booking, submission, and
running-count floor checks.
---
Outside diff comments:
In `@cuebot/scheduler-sim/gen_jobs.py`:
- Around line 83-97: Rename the per-job XML string local in the job submission
loop from spec to job_spec, updating its construction, LaunchSpec request, and
parsing references. Preserve the farm_spec module reference so the later
total_cores() call resolves correctly.
In `@cuebot/scheduler-sim/simulate.py`:
- Around line 809-825: Update the environment mapping in start_cuebot so extra
cuebot instances also receive SCHEDULER_LOCALITY_ENABLED, using the same value
derived from SIM_LOCALITY_ENABLED as the primary instance. Preserve the existing
locality configuration and ensure all instances honor the simulation setting.
---
Nitpick comments:
In `@cuebot/scheduler-sim/analysis/analyze_sweep.py`:
- Line 62: Split the chained assignments in the sweep analysis flow into
separate lines: assign reads via rd(), writes via wr(), rb via med_csv(...), and
lw via med_csv(...) independently, preserving their existing order and
arguments.
In `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.java`:
- Around line 194-195: Update the startFramesAndProcsBatch method declaration to
use the imported List type instead of the fully qualified java.util.List,
matching the other interface declarations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e67d20b8-def0-4714-b27c-76c86b251b24
📒 Files selected for processing (30)
cuebot/scheduler-sim/BUILD.mdcuebot/scheduler-sim/README.mdcuebot/scheduler-sim/analysis/analyze_sweep.pycuebot/scheduler-sim/analysis/db_sampler.pycuebot/scheduler-sim/analysis/make_graphs.pycuebot/scheduler-sim/analysis/make_graphs_ba.pycuebot/scheduler-sim/analysis/plot_run.pycuebot/scheduler-sim/depend_watch.pycuebot/scheduler-sim/drain_test.pycuebot/scheduler-sim/fake_rqd.pycuebot/scheduler-sim/feed.pycuebot/scheduler-sim/gen_jobs.pycuebot/scheduler-sim/inject_limit.pycuebot/scheduler-sim/limit_watch.pycuebot/scheduler-sim/locality_watch.pycuebot/scheduler-sim/metrics.pycuebot/scheduler-sim/rqd_report.pycuebot/scheduler-sim/simulate.pycuebot/scheduler-sim/stats.pycuebot/scheduler-sim/status_pinger.pycuebot/scheduler-sim/status_pinger_fast.pycuebot/scheduler-sim/tag_gpu_watch.pycuebot/src/main/java/com/imageworks/spcue/dao/DispatcherDao.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatcherDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java
🚧 Files skipped from review as they are similar to previous changes (17)
- cuebot/scheduler-sim/drain_test.py
- cuebot/scheduler-sim/BUILD.md
- cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java
- cuebot/scheduler-sim/metrics.py
- cuebot/scheduler-sim/status_pinger.py
- cuebot/scheduler-sim/rqd_report.py
- cuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.java
- cuebot/scheduler-sim/analysis/make_graphs.py
- cuebot/scheduler-sim/README.md
- cuebot/scheduler-sim/analysis/plot_run.py
- cuebot/scheduler-sim/status_pinger_fast.py
- cuebot/scheduler-sim/stats.py
- cuebot/scheduler-sim/fake_rqd.py
- cuebot/scheduler-sim/analysis/make_graphs_ba.py
- cuebot/scheduler-sim/feed.py
- cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java
- cuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.java
Comment on lines
+1998
to
+2052
| try: | ||
| pid = int(open("/tmp/sim-cuebot-0.pid").read().strip()) | ||
| os.kill(pid, signal.SIGKILL) | ||
| log(f"KILLED leader cuebot (instance 0, pid {pid}); watching the " | ||
| f"standby (:{GRPC_PORT + 10}) take over for {D - half}s ...") | ||
| except Exception as e: | ||
| log(f"FAILOVER: could not kill leader: {e}") | ||
| kill_ts = psql("SELECT now();").stdout.strip() | ||
| # Submission-failover probe: the long-running feeder may legitimately | ||
| # be idle at the kill (backlog target reached), so it would never dial | ||
| # cuebot again and prove nothing. Launch a fresh SHORT feeder with an | ||
| # effectively-unbounded target -- "an artist submitting during the | ||
| # outage": it dials the DEAD primary first, must fail over | ||
| # (SIM_CUEBOT_GRPC_FALLBACKS) and land submissions on the survivor. | ||
| # Submissions are counted as the job-table delta from the kill moment | ||
| # (job.ts_started is NOT submission time -- it mutates when the job | ||
| # starts running). | ||
| jobs0 = int(psql("SELECT count(*) FROM job;").stdout.strip() or 0) | ||
| probe_dur = max(20, D - half - 15) | ||
| spawn(["feed.py", str(probe_dur), "9999999"], f"{FARM}/feed_probe.log", | ||
| env_extra={"SIM_DEP_TREE_DEPTH": "1"}) | ||
| log(f" [failover] submission probe feeder launched ({probe_dur}s)") | ||
| started_after = 0 | ||
| running_end = 0 | ||
| jobs_after = 0 | ||
| t1 = time.time() | ||
| while time.time() - t1 < (D - half): | ||
| out = psql(f"SELECT count(*) FROM frame WHERE ts_started > '{kill_ts}';") | ||
| started_after = int(out.stdout.strip() or 0) | ||
| out = psql("SELECT count(*) FROM frame WHERE str_state='RUNNING';") | ||
| running_end = int(out.stdout.strip() or 0) | ||
| out = psql("SELECT count(*) FROM job;") | ||
| jobs_after = int(out.stdout.strip() or 0) - jobs0 | ||
| log(f" [failover] frames started since kill: {started_after} " | ||
| f"running now: {running_end} jobs submitted since kill: {jobs_after}") | ||
| time.sleep(5) | ||
| floor = int(os.environ.get("SIM_FAILOVER_MIN_STARTED", "100")) | ||
| jfloor = int(os.environ.get("SIM_FAILOVER_MIN_JOBS", "3")) | ||
| print("\n==== FAILOVER VERDICT ====", flush=True) | ||
| print(f"frames started after leader kill={started_after} " | ||
| f"jobs submitted after leader kill={jobs_after} " | ||
| f"running at end={running_end} (floors {floor}/{jfloor})", flush=True) | ||
| if started_after >= floor and jobs_after >= jfloor and running_end > 0: | ||
| print(f"PASS: after the leader was killed the standby booked " | ||
| f"{started_after} new frames (>= {floor}) AND accepted " | ||
| f"{jobs_after} new job submissions (>= {jfloor}) -- full-service " | ||
| f"leader failover works.", flush=True) | ||
| elif started_after >= floor: | ||
| print(f"FAIL: bookings failed over ({started_after} frames) but only " | ||
| f"{jobs_after} jobs were submitted after the kill (< {jfloor}) -- " | ||
| f"the submission path did not fail over.", flush=True) | ||
| else: | ||
| print(f"FAIL: only {started_after} frames started after the leader " | ||
| f"kill (floor {floor}, running at end {running_end}) -- the " | ||
| f"standby did not take over booking.", flush=True) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A failed leader kill still produces a PASS verdict.
If os.kill at line 2000 throws (stale or missing pid file, wrong pid, EPERM), the handler at 2003 only logs and execution continues into the measurement loop with the leader still alive and booking. started_after then clears the floor trivially and the probe feeder reaches the live primary, so the scenario prints PASS for a failover that was never exercised — the vacuous pass the other watchers guard against with coverage floors.
Track whether the kill actually happened and force a non-PASS verdict when it did not.
🔧 Proposed fix
D = args.failover_test
half = D // 2
log(f"FAILOVER: load for {half}s, then killing the leader cuebot ...")
subprocess.run([VENV_PY, "live_stats.py", str(half), "5"], cwd=FARM)
+ killed = False
try:
pid = int(open("/tmp/sim-cuebot-0.pid").read().strip())
os.kill(pid, signal.SIGKILL)
+ killed = True
log(f"KILLED leader cuebot (instance 0, pid {pid}); watching the "
f"standby (:{GRPC_PORT + 10}) take over for {D - half}s ...")
except Exception as e:
log(f"FAILOVER: could not kill leader: {e}")
@@
print("\n==== FAILOVER VERDICT ====", flush=True)
print(f"frames started after leader kill={started_after} "
f"jobs submitted after leader kill={jobs_after} "
f"running at end={running_end} (floors {floor}/{jfloor})", flush=True)
- if started_after >= floor and jobs_after >= jfloor and running_end > 0:
+ if not killed:
+ print("INCONCLUSIVE: the leader was never killed, so nothing failed "
+ "over -- these numbers come from the still-live primary.",
+ flush=True)
+ elif started_after >= floor and jobs_after >= jfloor and running_end > 0:
print(f"PASS: after the leader was killed the standby booked "📝 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
| try: | |
| pid = int(open("/tmp/sim-cuebot-0.pid").read().strip()) | |
| os.kill(pid, signal.SIGKILL) | |
| log(f"KILLED leader cuebot (instance 0, pid {pid}); watching the " | |
| f"standby (:{GRPC_PORT + 10}) take over for {D - half}s ...") | |
| except Exception as e: | |
| log(f"FAILOVER: could not kill leader: {e}") | |
| kill_ts = psql("SELECT now();").stdout.strip() | |
| # Submission-failover probe: the long-running feeder may legitimately | |
| # be idle at the kill (backlog target reached), so it would never dial | |
| # cuebot again and prove nothing. Launch a fresh SHORT feeder with an | |
| # effectively-unbounded target -- "an artist submitting during the | |
| # outage": it dials the DEAD primary first, must fail over | |
| # (SIM_CUEBOT_GRPC_FALLBACKS) and land submissions on the survivor. | |
| # Submissions are counted as the job-table delta from the kill moment | |
| # (job.ts_started is NOT submission time -- it mutates when the job | |
| # starts running). | |
| jobs0 = int(psql("SELECT count(*) FROM job;").stdout.strip() or 0) | |
| probe_dur = max(20, D - half - 15) | |
| spawn(["feed.py", str(probe_dur), "9999999"], f"{FARM}/feed_probe.log", | |
| env_extra={"SIM_DEP_TREE_DEPTH": "1"}) | |
| log(f" [failover] submission probe feeder launched ({probe_dur}s)") | |
| started_after = 0 | |
| running_end = 0 | |
| jobs_after = 0 | |
| t1 = time.time() | |
| while time.time() - t1 < (D - half): | |
| out = psql(f"SELECT count(*) FROM frame WHERE ts_started > '{kill_ts}';") | |
| started_after = int(out.stdout.strip() or 0) | |
| out = psql("SELECT count(*) FROM frame WHERE str_state='RUNNING';") | |
| running_end = int(out.stdout.strip() or 0) | |
| out = psql("SELECT count(*) FROM job;") | |
| jobs_after = int(out.stdout.strip() or 0) - jobs0 | |
| log(f" [failover] frames started since kill: {started_after} " | |
| f"running now: {running_end} jobs submitted since kill: {jobs_after}") | |
| time.sleep(5) | |
| floor = int(os.environ.get("SIM_FAILOVER_MIN_STARTED", "100")) | |
| jfloor = int(os.environ.get("SIM_FAILOVER_MIN_JOBS", "3")) | |
| print("\n==== FAILOVER VERDICT ====", flush=True) | |
| print(f"frames started after leader kill={started_after} " | |
| f"jobs submitted after leader kill={jobs_after} " | |
| f"running at end={running_end} (floors {floor}/{jfloor})", flush=True) | |
| if started_after >= floor and jobs_after >= jfloor and running_end > 0: | |
| print(f"PASS: after the leader was killed the standby booked " | |
| f"{started_after} new frames (>= {floor}) AND accepted " | |
| f"{jobs_after} new job submissions (>= {jfloor}) -- full-service " | |
| f"leader failover works.", flush=True) | |
| elif started_after >= floor: | |
| print(f"FAIL: bookings failed over ({started_after} frames) but only " | |
| f"{jobs_after} jobs were submitted after the kill (< {jfloor}) -- " | |
| f"the submission path did not fail over.", flush=True) | |
| else: | |
| print(f"FAIL: only {started_after} frames started after the leader " | |
| f"kill (floor {floor}, running at end {running_end}) -- the " | |
| f"standby did not take over booking.", flush=True) | |
| subprocess.run([VENV_PY, "live_stats.py", str(half), "5"], cwd=FARM) | |
| killed = False | |
| try: | |
| pid = int(open("/tmp/sim-cuebot-0.pid").read().strip()) | |
| os.kill(pid, signal.SIGKILL) | |
| killed = True | |
| log(f"KILLED leader cuebot (instance 0, pid {pid}); watching the " | |
| f"standby (:{GRPC_PORT + 10}) take over for {D - half}s ...") | |
| except Exception as e: | |
| log(f"FAILOVER: could not kill leader: {e}") | |
| kill_ts = psql("SELECT now();").stdout.strip() | |
| # Submission-failover probe: the long-running feeder may legitimately | |
| # be idle at the kill (backlog target reached), so it would never dial | |
| # cuebot again and prove nothing. Launch a fresh SHORT feeder with an | |
| # effectively-unbounded target -- "an artist submitting during the | |
| # outage": it dials the DEAD primary first, must fail over | |
| # (SIM_CUEBOT_GRPC_FALLBACKS) and land submissions on the survivor. | |
| # Submissions are counted as the job-table delta from the kill moment | |
| # (job.ts_started is NOT submission time -- it mutates when the job | |
| # starts running). | |
| jobs0 = int(psql("SELECT count(*) FROM job;").stdout.strip() or 0) | |
| probe_dur = max(20, D - half - 15) | |
| spawn(["feed.py", str(probe_dur), "9999999"], f"{FARM}/feed_probe.log", | |
| env_extra={"SIM_DEP_TREE_DEPTH": "1"}) | |
| log(f" [failover] submission probe feeder launched ({probe_dur}s)") | |
| started_after = 0 | |
| running_end = 0 | |
| jobs_after = 0 | |
| t1 = time.time() | |
| while time.time() - t1 < (D - half): | |
| out = psql(f"SELECT count(*) FROM frame WHERE ts_started > '{kill_ts}';") | |
| started_after = int(out.stdout.strip() or 0) | |
| out = psql("SELECT count(*) FROM frame WHERE str_state='RUNNING';") | |
| running_end = int(out.stdout.strip() or 0) | |
| out = psql("SELECT count(*) FROM job;") | |
| jobs_after = int(out.stdout.strip() or 0) - jobs0 | |
| log(f" [failover] frames started since kill: {started_after} " | |
| f"running now: {running_end} jobs submitted since kill: {jobs_after}") | |
| time.sleep(5) | |
| floor = int(os.environ.get("SIM_FAILOVER_MIN_STARTED", "100")) | |
| jfloor = int(os.environ.get("SIM_FAILOVER_MIN_JOBS", "3")) | |
| print("\n==== FAILOVER VERDICT ====", flush=True) | |
| print(f"frames started after leader kill={started_after} " | |
| f"jobs submitted after leader kill={jobs_after} " | |
| f"running at end={running_end} (floors {floor}/{jfloor})", flush=True) | |
| if not killed: | |
| print("INCONCLUSIVE: the leader was never killed, so nothing failed " | |
| "over -- these numbers come from the still-live primary.", | |
| flush=True) | |
| elif started_after >= floor and jobs_after >= jfloor and running_end > 0: | |
| print(f"PASS: after the leader was killed the standby booked " | |
| f"{started_after} new frames (>= {floor}) AND accepted " | |
| f"{jobs_after} new job submissions (>= {jfloor}) -- full-service " | |
| f"leader failover works.", flush=True) | |
| elif started_after >= floor: | |
| print(f"FAIL: bookings failed over ({started_after} frames) but only " | |
| f"{jobs_after} jobs were submitted after the kill (< {jfloor}) -- " | |
| f"the submission path did not fail over.", flush=True) | |
| else: | |
| print(f"FAIL: only {started_after} frames started after the leader " | |
| f"kill (floor {floor}, running at end {running_end}) -- the " | |
| f"standby did not take over booking.", flush=True) |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 1998-1998: Do not hardcode temporary file or directory names
Context: "/tmp/sim-cuebot-0.pid"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 Ruff (0.15.21)
[error] 1999-1999: Probable insecure usage of temporary file or directory: "/tmp/sim-cuebot-0.pid"
(S108)
[warning] 2003-2003: Do not catch blind exception: Exception
(BLE001)
[error] 2025-2025: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@cuebot/scheduler-sim/simulate.py` around lines 1998 - 2052, Track successful
leader termination in the failover block around the os.kill call, setting the
state only after the kill completes and leaving it false when pid-file reading
or termination raises. Require that state in the final PASS condition and report
a failure verdict when the leader was not killed, while preserving the existing
booking, submission, and running-count floor checks.
| logger.debug("Reserving all cores minus " + proc.coresReserved); | ||
| proc.coresReserved = host.cores + proc.coresReserved; | ||
| } else if (proc.coresReserved >= 100) { | ||
| } else if (proc.coresReserved >= 100 && expandThreadable) { |
Collaborator
There was a problem hiding this comment.
By skipping this section, expandThreadable=False skips both Host.ThreadMode and SelfishService checks. Some hosts are configured to always give away all idle cores, if the intent is to change this behavior, this needs to be properly documented to make sure users are not caught unprepared. The SelfishService feature should be respected in all cases. This features allows configuring services that cannot share resources to claim all remaining cores when allocated to a host. When combined with allocation configurations, frames belonging to selfish services can run by their own without competing resources.
Comment on lines
+171
to
+174
| * Batch variant of {@link #updateFrameStarted}: marks many frames RUNNING in one round-trip | ||
| * with the same per-row optimistic version+state guard. No SELECT ... FOR UPDATE is taken; a | ||
| * frame whose state/version changed since planning simply updates zero rows and is reported as | ||
| * a loser. |
Collaborator
There was a problem hiding this comment.
This doc is partially accurate. No SELECT FOR UPDATE is taken for the frame table, but there's two new select for updated clauses for layer_stat and job_stat
Comment on lines
+440
to
+443
| // Bind order must match the query: outer layer.pk_layer=?, then | ||
| // the tag subquery's h.str_name=?, then its l.pk_layer=?. The host | ||
| // name and the second layer id were transposed, so the tag | ||
| // subquery matched no host/layer and the query returned no frames. |
Collaborator
There was a problem hiding this comment.
Great finding. Looking back to the repository history, this bug was introduced by 9e4fb1c — "Add basic limits functionality (#414)", Greg Denton, Oct 2 2019.
I'm creating a new issue reporting this bug since it also uncovered another but on sibling queries introduced in the same era. Until your change, both queries were dead-code so this issue never really materialized in production (read the issue for more information about the flow and why it was never reachable).
For this PR, I suggest removing the comment as it only makes sense in the context of the bug. The comment doesn't help to understand the code itself, but it tells a history that has a better chance of being read as part of the commit message.
Comment on lines
+238
to
+245
| /** | ||
| * Pre-acquire, in a deterministic global order, the layer_stat and job_stat counter rows that | ||
| * the batch's frame-start triggers will update. Locks all distinct layer_stat rows first | ||
| * (ordered by pk_layer), then all distinct job_stat rows (ordered by pk_job), the same | ||
| * "layer-before-job" order every single-frame transaction follows via the trigger, so this | ||
| * batch can never deadlock against a concurrent frame completion. SELECT ... FOR UPDATE inside | ||
| * the batch's transaction; rows are released at commit. | ||
| */ |
Collaborator
There was a problem hiding this comment.
The only caller is DispatchSupportService.startFramesAndProcsBatch (DispatchSupportService.java:245), which is @transactional(REQUIRED), so the SELECT ... FOR UPDATE locks survive until commit. (The DAO silently depends on that annotation. If this method is ever called without an enclosing transaction, the locks evaporate per-statement and the whole scheme is a no-op. Worth one line in the javadoc.)
Comment on lines
+256
to
+258
| getJdbcTemplate().query("SELECT pk_layer FROM layer_stat WHERE pk_layer IN (" + in | ||
| + ") " + "ORDER BY pk_layer FOR UPDATE", rs -> { | ||
| }, layerIds.toArray()); |
Collaborator
There was a problem hiding this comment.
The only concern I have for holding many locks over the batch process period is the blocking impact it can have on other flows that also have to update it. The flow I'm more concerned about measuring the impact is FrameCompleteReport, which calls stopFrame and triggers a layer_stat and job_stat update. Delays on this flow leave completed frames RUNNING without clearing their dependencies. If the batch process runs fast enough, this won't be an issue, but it is definitelly something we need to measure.
Author
There was a problem hiding this comment.
The batch completions commit (dafb38b) removed most (99%) of this: completions for scheduler-owned shows go through the drain and are the batch itself, so this flow can't be blocked by its own locks, and legacy shows touch disjoint stat rows. What remains is a small wait (one batch transaction, tens of ms) for manual operations (on the same show (like if someone is retrying maybe?). I will add a batch-duration metric alongside your Prometheus suggections.
This dafb38b also applies to the other Frame Completion review. Basically, I made sure that all this is bached ion the same tick and it removed a whole class of problems. Unless I am missing something major here.
Suggested by Mr. Matt Chambers at that meeting in L.A and really helped me a lot of extract good performance, in the sumlator at least. Let's see in real life !!! :)
| // (facility, or a 'managed' show); legacy shows and Rust | ||
| // (dispatcher.turn_off_booking) stay on the async path. | ||
| boolean schedulerOwnsShow = SchedulerMode.schedules(env, showDao, proc.getShowId()); | ||
| if (dispatcher.isTestMode() || schedulerOwnsShow) { |
Collaborator
There was a problem hiding this comment.
I don't think removing this call from the dispatchQueue is a good idea. Running it serially means running on the same thread responding to RQD's request and handlePostFrameCompleteOperations is not lightweight:
handlePostFrameCompleteOperations is genuinely blocking. On the Scheduler-owned path it still does, inline:
- publishFrameCompleteEvent → Kafka publish (network)
- updateUsageCounters → DB write
- satisfyWhatDependsOn (frame + possibly layer) → potentially heavy DB, wrapped in satisfyDependsWithRetry that Thread.sleeps up to 100+200+400 = 700ms on transient failure
- isLayerComplete, getExecutionSummary, getLayerDetail, layer Kafka event
- isJobComplete → DB
- OOM branch: whiteboardDao.getServiceOverride/findService → DB
- unbookProc → DB write
The assumption on the comment "the legacy async .. exists only to defer to rebook-or-release decision.." is misleading. The rebook branch (DispatchNextFrame/DispatchBookHost) is indeed dead in Scheduler mode, so the inline path is shorter than the legacy worst case. But the depend-satisfaction, usage-counter, job-completion, Kafka, and unbook work all still run and those were also being offloaded and throttled by the async hop.
The change is defensible for the Scheduler-owned path; the limbo/zombie problem it fixes is real (a pk_frame=NULL proc holding cores until a backed-up dispatchQueue task runs), and the dangerous rebook branch is gone. But the justification undersells the blocking: you're moving a DB-heavy, occasionally-700ms workload onto RQD-facing threads and removing the concurrency cap that protected a 20-connection pool.
If you want to experiment with this on the live environment, I suggest creating a new configuration property to turn it on and off, completelly disconnected from the Planner toggle. This way we will be able to turn it On/Off using environment variables and not have to deactivate the entire Planner solution in case this goes sideways.
Author
There was a problem hiding this comment.
Shortly after yiour review I redid this part completely on the advice of Matt Chmabers. The completion-drain commits superseded it:
commit dafb38b
Report threads only resolve+enqueue, planner-critical writes are batched in the tick, and the heavy post ops run on a dedicated bounded worker, so RQD threads and the connection pool are better protected than on the legacy path. dispatchQueue was rejected deliberately because its load-shedding can drop depend satisfaction.
This really made a big difference in the simualtor. Frame throughput increased by 20% with some weird bugs completely gone. Note that the simulator can write an insane amount of data (beyond the capabilities of our render farm).
Let me know what you think of that rewrite.
| // deliberately left on the async path here -- out of scope for this change. | ||
| // Inline post-complete only for shows the in-process Scheduler owns | ||
| // (facility, or a 'managed' show); legacy shows and Rust | ||
| // (dispatcher.turn_off_booking) stay on the async path. |
Collaborator
There was a problem hiding this comment.
Please review and reformat this comment.
- Remove multiple
--charts - reformat line-breaks
- Reorganize thoughts to simplify understanding. I had to read this 3 times to understand what it is trying to tell me.
| logger.debug("Scheduler: another Cuebot holds the planning lock"); | ||
| summaryLockLost++; | ||
| return; | ||
| } |
Collaborator
There was a problem hiding this comment.
I'm a great fan of advisory locks on postgres and they are a good candidate for leader election, but they cost one mostly idle connection.
There's a module on Cuebot called MaintenanceTask that uses a postgres table for locking per task. It is currently used for tasks that can't run in more than one host at a time. I suggest taking a look to confirm if it isn't better suited for this mechanism.
Author
There was a problem hiding this comment.
I had a problem with the timeout timer being too slow in simulated crashes (this is the FALLOVER scenario simulated ). Note that the implementation started with your proposed solution but I had to use this for performance reasons. Let me know if you absolutely want this and I will try again.
| score(h) = sum_D W_D * ( e^(after_D/total_D) - e^(before_D/total_D) ) | ||
| before_D = total_D - idle_D (currently reserved) | ||
| after_D = before_D + layer.min_D (with this frame added) | ||
| ``` |
Collaborator
There was a problem hiding this comment.
Please provide a legend for the variables on this formula. I was able to understand the logic by looking at the code, but it would be nice to be able to put it together entirely from this document.
| @@ -0,0 +1,559 @@ | |||
| # Scheduler (Planner) | |||
Collaborator
There was a problem hiding this comment.
Please move this file to ./docs/_docs/developer-guide/planner.md
DiegoTavares
left a comment
DiegoTavares
left a comment
Collaborator
There was a problem hiding this comment.
Unfortunately I wasn't able to complete the entire review.
- All files on Cuebot except Scheduler.java have been completely reviewed.
- Scheduler.java has been 40% reviewed. I'm going to work on the rest when I'm back from my vacation, next Monday.
- I still didn't have the chance to experiment heavily with the scheduler-sim, but I'm looking forward to it.
Thanks again for the great work.
Comment on lines
+115
to
+118
| private static final double W_CORES = 1.0; | ||
| private static final double W_MEM = 1.0; | ||
| private static final double W_GPUS = 4.0; | ||
| private static final double W_GPU_MEM = 1.0; |
Collaborator
There was a problem hiding this comment.
Is there a change we will want to tune this on the fly? If that's the case, let's source this from opencue.properties+env-vars. This was it can be tuned without requiring a new build.
Comment on lines
+433
to
+438
| private static final String SELECT_ALL_HOSTS = "SELECT " + " h.pk_host, " + " h.str_name, " | ||
| + " h.pk_alloc, " + " h.int_cores, " + " h.int_cores_idle, " + " h.int_mem, " | ||
| + " h.int_mem_idle, " + " h.int_gpus, " + " h.int_gpus_idle, " + " h.int_gpu_mem, " | ||
| + " h.int_gpu_mem_idle, " + " h.int_procs, " + " h.str_tags, " + " hs.str_os " | ||
| + "FROM host h, host_stat hs " + "WHERE h.pk_host = hs.pk_host " | ||
| + " AND hs.str_state = 'UP' " + " AND h.str_lock_state = 'OPEN' "; |
Collaborator
There was a problem hiding this comment.
Please sorround query with spotless:off and format it for better reading.
Comment on lines
+451
to
+564
| + " COALESCE(lim.int_max_value, 0) AS limit_max, " | ||
| + " COALESCE(lu2.int_sum_running, 0) AS limit_running, " | ||
| + " COALESCE(lim.b_host_limit, false) AS limit_host, " | ||
| // Folder (group/dept) core cap: the job's folder, its ceiling, and the | ||
| // folder's current running cores (ground truth = SUM of the folder's jobs). | ||
| + " j.pk_folder AS folder_id, " + " COALESCE(fr.int_max_cores, -1) AS folder_max, " | ||
| + " COALESCE(fu.folder_cores, 0) AS folder_running " + "FROM layer l " | ||
| + "JOIN job j ON j.pk_job = l.pk_job " | ||
| + "JOIN job_resource jr ON jr.pk_job = j.pk_job " | ||
| + "JOIN show sh ON sh.pk_show = j.pk_show " | ||
| + "JOIN subscription sub ON sub.pk_show = j.pk_show AND sub.pk_alloc = ? " | ||
| + "LEFT JOIN layer_usage lu ON lu.pk_layer = l.pk_layer " | ||
| + "LEFT JOIN layer_stat ls ON ls.pk_layer = l.pk_layer " | ||
| // The layer's most-constraining limit (smallest cap), one row per layer. | ||
| + "LEFT JOIN LATERAL (" | ||
| + " SELECT ll.pk_limit_record, lr.int_max_value, lr.b_host_limit " | ||
| + " FROM layer_limit ll " | ||
| + " JOIN limit_record lr ON lr.pk_limit_record = ll.pk_limit_record " | ||
| + " WHERE ll.pk_layer = l.pk_layer " | ||
| + " ORDER BY lr.int_max_value LIMIT 1) lim ON true " | ||
| // Farm-wide running count per limit (computed once, not per row). | ||
| + "LEFT JOIN (" | ||
| + " SELECT ll2.pk_limit_record, SUM(ls2.int_running_count) AS int_sum_running " | ||
| + " FROM layer_limit ll2 " | ||
| + " JOIN layer_stat ls2 ON ls2.pk_layer = ll2.pk_layer " | ||
| + " GROUP BY ll2.pk_limit_record) lu2 " | ||
| + " ON lu2.pk_limit_record = lim.pk_limit_record " | ||
| // Folder core ceiling + the folder's current running cores. Derived from | ||
| // layer_stat.int_running_count (running frames x per-frame cores) -- the | ||
| // same trigger-maintained counter the limit cap uses. It is robust to frame | ||
| // completion (int_running_count drops automatically) and, at a tick | ||
| // boundary, equals SUM(job_resource.int_cores) (one proc per running | ||
| // frame), the figure the folder cap is measured against. Computed once, | ||
| // not per row. | ||
| + "LEFT JOIN folder_resource fr ON fr.pk_folder = j.pk_folder " + "LEFT JOIN (" | ||
| + " SELECT j2.pk_folder, " | ||
| + " SUM(ls2.int_running_count * l2.int_cores_min) AS folder_cores " | ||
| + " FROM job j2 " | ||
| // Only aggregate CAPPED folders (int_max_cores <> -1). Every job has a | ||
| // folder but almost none are capped, so without this join the subquery | ||
| // would sum layer_stat across the whole farm every candidate query; this | ||
| // keeps it empty (free) when no folder has a ceiling. | ||
| + " JOIN folder_resource fr2 ON fr2.pk_folder = j2.pk_folder " | ||
| + " AND fr2.int_max_cores <> -1 " | ||
| + " JOIN layer l2 ON l2.pk_job = j2.pk_job " | ||
| + " JOIN layer_stat ls2 ON ls2.pk_layer = l2.pk_layer " | ||
| + " WHERE j2.str_state = 'PENDING' " | ||
| + " GROUP BY j2.pk_folder) fu ON fu.pk_folder = j.pk_folder " | ||
| + "WHERE j.str_state = 'PENDING' " + " AND j.b_paused = false " | ||
| + " AND (j.str_os IS NULL OR j.str_os = '' OR j.str_os = ?) " | ||
| + " AND ? ~* ('(?x)' || l.str_tags || '\\y') " | ||
| + " AND jr.int_cores < jr.int_max_cores " + " AND sub.int_cores < sub.int_burst " | ||
| + " AND l.int_cores_min <= ? " | ||
| // Dispatchable-frame test AND waiting_frame_count both come from | ||
| // layer_stat.int_waiting_count (maintained by core trigger | ||
| // trigger__update_frame_status_counts; WAITING frames are depend-resolved, | ||
| // DEPEND is a separate state). Backed by the partial index | ||
| // idx_layer_stat_waiting (V44). Replaces a correlated COUNT(*) + EXISTS | ||
| // over frame that scanned every frame of each candidate layer per tick. | ||
| + " AND COALESCE(ls.int_waiting_count, 0) > 0 " | ||
| // Skip layers whose limit (license cap) is already full farm-wide. Their | ||
| // frames get filtered out downstream by findNextDispatchFrames anyway, so | ||
| // scoring a host + running the plan read for them only burns a cycle that | ||
| // returns nothing (it surfaces as raceLost). A limit-less layer (NULL) | ||
| // always passes. Not a correctness gate -- the downstream query still | ||
| // enforces the cap -- purely an efficiency filter. HOST limits | ||
| // (b_host_limit) always pass: their cap counts distinct hosts, not | ||
| // frames, and at the seat cap the layer may still book onto hosts | ||
| // that already hold a seat -- the planner enforces that per host. | ||
| + " AND (lim.pk_limit_record IS NULL " | ||
| + " OR COALESCE(lim.b_host_limit, false) = true " | ||
| + " OR COALESCE(lu2.int_sum_running, 0) < lim.int_max_value) " | ||
| // Skip jobs whose FOLDER (group/dept) core ceiling is already reached -- | ||
| // folder_resource.int_max_cores, another core cap the legacy dispatcher | ||
| // enforces. -1 = unlimited. Same rationale as the limit filter: purely an | ||
| // efficiency gate (don't plan bookings a full folder can't take). The exact | ||
| // ceiling is enforced by the post-plan folder trim in doTick, which -- | ||
| // unlike this filter -- also binds the frames planHost books in the tick | ||
| // that crosses the cap. | ||
| + " AND (COALESCE(fr.int_max_cores, -1) = -1 " | ||
| + " OR COALESCE(fu.folder_cores, 0) + l.int_cores_min <= fr.int_max_cores) " | ||
| // Progressive rollout: in 'managed' mode only shows flagged | ||
| // b_scheduler_managed are planned here (the legacy dispatch query excludes | ||
| // exactly those, so the two partition); in 'facility' mode the bound flag | ||
| // is true and this short-circuits to plan every show. | ||
| + " AND (? OR sh.b_scheduler_managed = true) " | ||
| // Priority-WEIGHTED LOTTERY, not a strict priority sort. Each eligible | ||
| // layer gets a random key random()^(1/priority) -- Efraimidis-Spirakis | ||
| // weighted reservoir sampling -- and we take the top-LIMIT by that key. | ||
| // ORDER BY ranks the WHOLE eligible set before LIMIT (sort-then-limit), | ||
| // so a low-priority layer always keeps a real, smaller chance of being | ||
| // selected: its expected share is proportional to its priority, so it is | ||
| // never starved by a sustained higher-priority stream. The old strict | ||
| // "int_priority DESC" starved it outright -- pri-100 work never ran while | ||
| // a pri-300 backlog kept the farm saturated. GREATEST(...,1) floors the | ||
| // weight so priority 0/negative still gets the minimum (nonzero) share | ||
| // rather than divide-by-zero or starvation. Reservation GRANTING stays | ||
| // strict priority-first (the requests are re-sorted by priority below), | ||
| // so wide-job rescue is unaffected by this booking-order change. | ||
| + "ORDER BY power(random(), 1.0 / GREATEST(jr.int_priority, 1)) DESC " + "LIMIT ? "; |
Collaborator
There was a problem hiding this comment.
Please sorround query with spotless:off and format it for better reading
Comment on lines
+427
to
+619
| // trigger__update_frame_status_counts; WAITING frames are depend-resolved, | ||
| // DEPEND is a separate state). Backed by the partial index | ||
| // idx_layer_stat_waiting (V44). Replaces a correlated COUNT(*) + EXISTS | ||
| // over frame that scanned every frame of each candidate layer per tick. | ||
| + " AND COALESCE(ls.int_waiting_count, 0) > 0 " | ||
| // Skip layers whose limit (license cap) is already full farm-wide. Their | ||
| // frames get filtered out downstream by findNextDispatchFrames anyway, so | ||
| // scoring a host + running the plan read for them only burns a cycle that | ||
| // returns nothing (it surfaces as raceLost). A limit-less layer (NULL) | ||
| // always passes. Not a correctness gate -- the downstream query still | ||
| // enforces the cap -- purely an efficiency filter. HOST limits | ||
| // (b_host_limit) always pass: their cap counts distinct hosts, not | ||
| // frames, and at the seat cap the layer may still book onto hosts | ||
| // that already hold a seat -- the planner enforces that per host. | ||
| + " AND (lim.pk_limit_record IS NULL " | ||
| + " OR COALESCE(lim.b_host_limit, false) = true " | ||
| + " OR COALESCE(lu2.int_sum_running, 0) < lim.int_max_value) " | ||
| // Skip jobs whose FOLDER (group/dept) core ceiling is already reached -- | ||
| // folder_resource.int_max_cores, another core cap the legacy dispatcher | ||
| // enforces. -1 = unlimited. Same rationale as the limit filter: purely an | ||
| // efficiency gate (don't plan bookings a full folder can't take). The exact | ||
| // ceiling is enforced by the post-plan folder trim in doTick, which -- | ||
| // unlike this filter -- also binds the frames planHost books in the tick | ||
| // that crosses the cap. | ||
| + " AND (COALESCE(fr.int_max_cores, -1) = -1 " | ||
| + " OR COALESCE(fu.folder_cores, 0) + l.int_cores_min <= fr.int_max_cores) " | ||
| // Progressive rollout: in 'managed' mode only shows flagged | ||
| // b_scheduler_managed are planned here (the legacy dispatch query excludes | ||
| // exactly those, so the two partition); in 'facility' mode the bound flag | ||
| // is true and this short-circuits to plan every show. | ||
| + " AND (? OR sh.b_scheduler_managed = true) " | ||
| // Priority-WEIGHTED LOTTERY, not a strict priority sort. Each eligible | ||
| // layer gets a random key random()^(1/priority) -- Efraimidis-Spirakis | ||
| // weighted reservoir sampling -- and we take the top-LIMIT by that key. | ||
| // ORDER BY ranks the WHOLE eligible set before LIMIT (sort-then-limit), | ||
| // so a low-priority layer always keeps a real, smaller chance of being | ||
| // selected: its expected share is proportional to its priority, so it is | ||
| // never starved by a sustained higher-priority stream. The old strict | ||
| // "int_priority DESC" starved it outright -- pri-100 work never ran while | ||
| // a pri-300 backlog kept the farm saturated. GREATEST(...,1) floors the | ||
| // weight so priority 0/negative still gets the minimum (nonzero) share | ||
| // rather than divide-by-zero or starvation. Reservation GRANTING stays | ||
| // strict priority-first (the requests are re-sorted by priority below), | ||
| // so wide-job rescue is unaffected by this booking-order change. | ||
| + "ORDER BY power(random(), 1.0 / GREATEST(jr.int_priority, 1)) DESC " + "LIMIT ? "; | ||
|
|
||
| // ---- row mappers ------------------------------------------------------ | ||
|
|
||
| private static final RowMapper<BookableHost> HOST_MAPPER = new RowMapper<BookableHost>() { | ||
| public BookableHost mapRow(ResultSet rs, int i) throws SQLException { | ||
| BookableHost h = new BookableHost(); | ||
| h.hostId = rs.getString("pk_host"); | ||
| h.hostName = rs.getString("str_name"); | ||
| h.pkAlloc = rs.getString("pk_alloc"); | ||
| h.coresTotal = rs.getInt("int_cores"); | ||
| h.coresIdle = rs.getInt("int_cores_idle"); | ||
| h.memTotal = rs.getLong("int_mem"); | ||
| h.memIdle = rs.getLong("int_mem_idle"); | ||
| h.gpusTotal = rs.getInt("int_gpus"); | ||
| h.gpusIdle = rs.getInt("int_gpus_idle"); | ||
| h.gpuMemTotal = rs.getLong("int_gpu_mem"); | ||
| h.gpuMemIdle = rs.getLong("int_gpu_mem_idle"); | ||
| h.runningProcs = rs.getInt("int_procs"); | ||
| h.tagsRaw = rs.getString("str_tags"); | ||
| h.os = rs.getString("str_os"); | ||
| return h; | ||
| } | ||
| }; | ||
|
|
||
| private static final RowMapper<LayerCandidate> CANDIDATE_MAPPER = | ||
| new RowMapper<LayerCandidate>() { | ||
| public LayerCandidate mapRow(ResultSet rs, int i) throws SQLException { | ||
| LayerCandidate c = new LayerCandidate(); | ||
| c.layerId = rs.getString("pk_layer"); | ||
| c.jobId = rs.getString("pk_job"); | ||
| c.showId = rs.getString("pk_show"); | ||
| c.layerCoresMin = rs.getInt("int_cores_min"); | ||
| c.layerMemMin = rs.getLong("int_mem_min"); | ||
| c.layerGpusMin = rs.getInt("int_gpus_min"); | ||
| c.layerGpuMemMin = rs.getLong("int_gpu_mem_min"); | ||
| c.priority = rs.getInt("int_priority"); | ||
| c.jobCoresInUse = rs.getInt("job_cores_in_use"); | ||
| c.jobMaxCores = rs.getInt("job_max_cores"); | ||
| c.showCoresInUse = rs.getInt("show_cores_in_use"); | ||
| c.showBurstCores = rs.getInt("show_burst"); | ||
| c.waitingFrameCount = rs.getInt("waiting_frame_count"); | ||
| c.clockTimeHighSec = rs.getInt("clock_time_high"); | ||
| c.frameSuccessCount = rs.getInt("frame_success_count"); | ||
| c.limitId = rs.getString("limit_id"); // null when no limit | ||
| c.limitMax = rs.getInt("limit_max"); | ||
| c.limitRunning = rs.getInt("limit_running"); | ||
| c.limitHostBased = rs.getBoolean("limit_host"); | ||
| c.folderId = rs.getString("folder_id"); | ||
| c.folderMax = rs.getInt("folder_max"); // -1 = unlimited | ||
| c.folderRunning = rs.getInt("folder_running"); // core-points | ||
| return c; | ||
| } | ||
| }; | ||
|
|
||
| // ---- tick ------------------------------------------------------------- |
Collaborator
There was a problem hiding this comment.
As an effort to reduce the size of this file and keep the same structure of other modules, I suggest moving the queries and row mappers to a new class SchedulerDaoJdbc.java.
Author
There was a problem hiding this comment.
OK. Almost all these calls are generated by Claude. I will need to spend some time to review all of them again after pulling them out. So might take me a couple days.
Comment on lines
+653
to
+654
| logger.debug("Scheduler tick: dispatched " + dispatched + " procs, " + ms | ||
| + " ms, reservations=" + reservations.size()); |
Collaborator
There was a problem hiding this comment.
A great portion of the work on the rust scheduler was putting metrics in place to understand its performance, which was very useful when things don't go to plan on the production environment. Here's a good candidate for a place to start. Take a look at PrometheusMetricCollector.java - bookingDurationMillisMetric for a reference.
I suggest creating metrics for:
- schedulerDurations, with the stage name as the label.
- Successfull vs Failed count
- Group sizes
Comment on lines
+942
to
+945
| List<LayerCandidate> candidates = | ||
| readLayerCandidatesForGroup(spec, maxCoresTotalInGroup); | ||
| if (candidates.isEmpty()) | ||
| continue; |
Collaborator
There was a problem hiding this comment.
One of the surprises when running the rust-scheduler on the production environment was that a lot of cycles were wasted on groups that produced no work. It is normal to have several allocations/tags that are not very active most of the time, but are active when needed, which justifies their existence.
My solution for this was a sleep backoff algorithm. Groups that didn't produce work would be dormant for a few cycles. I'm not sure if the cost wasted by computing a group that has no work is worth this effort here, maybe not. But It is worth preemptively adding a metric to count groups that produced no work in prometheus.
Author
There was a problem hiding this comment.
Interesting. I didn't think of this. Let me work on this a bit this week. I think under massive load this could be a problem inded. At the very least, there will be some timings.
Comment on lines
+1339
to
+1343
| // Cuebot auto-adds each host's own name as a tag. Drop it | ||
| // from the grouping key, otherwise every host falls into a | ||
| // group of one and the per-group candidate query runs once | ||
| // per host instead of once per real spec. | ||
| normalizeTags(h.tagsRaw, h.hostName), h.os, |
Collaborator
There was a problem hiding this comment.
If I'm reading this correctly, this means tagging a layer with a hostname will no longer dispatch its frames on the requested host. Although I agree using a host own name here is necessary to prevent a single host per group, we need to find a way to accomodate targeted booking as it is heavily used by cue managers to allow manually allocating resources when needed.
Author
There was a problem hiding this comment.
Yes this was a mistake. I am fixing it.
The planner re-acquired the Postgres advisory lock every tick and released it at tick end, so leadership could bounce between cuebots and each new leader re-planned the whole farm from a cold snapshot. That is redundant work with no payoff. The scheduler is single-writer by design: one cuebot plans the whole farm each tick. Running more cuebots cannot make planning faster, because only the lock holder plans and the others cannot help with the same tick. Additional cuebots are therefore backups, not extra capacity. Spreading the work across them would only duplicate the farm-wide planning and waste resources. So hold the lock once, on a dedicated (non-pooled) connection, for the leader's lifetime. Standbys stay idle and only take over when the holder's session dies (Postgres releases the session-scoped lock on connection loss), which is real failover, not load sharing. The lock connection is a raw DriverManager connection so HikariCP idle-reaping and leak-detection cannot silently release it.
Layers declare licenses in their environment (CUE_LICENSES=hengine,katana); the
planner books them only while the license server reports free seats. Seats are
per frame (floating) or per machine (host_based, shared by all frames on the
machine, packed onto as few machines as possible).
LicenseSource polls an http endpoint or a script wrapping the vendor CLI:
scheduler.license.provider=http://lic-reporter:9101/licenses
scheduler.license.provider=script:/site/bin/cue_licenses.sh
Either returns the same JSON (hosts is optional; available is server truth,
net of every consumer):
{"queried_at": <epoch s>,
"licenses": [{"name": "hengine", "total": 800, "available": 794,
"host_based": false,
"hosts": [{"host": "wolf1018", "count": 1}]}]}
Tuning, all under scheduler.license.*: poll_seconds, timeout_seconds,
stale_seconds, inflight_pad_seconds, headroom.<name> (seats withheld per
license, e.g. headroom.hengine=5), env_key (default CUE_LICENSES), and
denied_exit_statuses (vendor exit codes meaning "no license").
The planner corrects the sample with DB-derived in-flight bookings (consistent
across failover) and per-license headroom for interactive users. host_based
caps are bounded via `available`, so a provider that reports no host list
(sesictrl only reports per user) still cannot be oversubscribed. Stale sample,
unknown license or no provider: the work is held, not run blind. A configured
license-denied exit status requeues the frame without spending a retry.
Licensing CONSTRAINS, it never prioritizes. Having few licenses is not a claim
to the farm: licensed layers go through the same priority lottery as everything
else, and a full license pool simply holds the layer, costing nobody anything.
The only placement bias a license adds is where that layer's OWN frames land
(packing onto already-seated machines, to burn fewer seats); other jobs never
pay for it beyond the marginal stranding delta on those hosts. That packing
bonus is deliberately stronger than the ordinary cache-warmth locality bonus
(16 per license pool vs 8, dominant over the E-PVM score spread): a seat is
scarcer than a warm cache.
No schema change, no legacy dispatcher contact, no enable flag: the layer's
declaration is the switch.
Sim: fake license server (counts real farm usage plus artist holds), scenarios
LICENSE and LICENSE_NO_HOSTS, licensed load in FAILOVER, and every scenario is
now gated on completed frames. Standby cuebots poll through the script: flavour
of the provider, so FAILOVER proves both transports.
Also removes the static per-host limit (b_host_limit): unreachable by users (no
proto/API/GUI) and blind to seats held outside the cue. Restores the upstream
plan-read and frame-start statements. Frame-count Limits unchanged; the seat
bonus keeps its property name (scheduler.host_limit_seat_bonus).
A proc row left behind by a failed completion (or any crash) wedges the planner permanently: every batch commit hits c_proc_uk, rolls back whole, and the next tick replans the same frame. Reproduced deterministically: plant terminal orphan procs on never-dispatched WAITING frames at the head of the dispatch order; on current code booking stops (13 failed ticks in a row, orphans untouched). PASS requires eviction + uninterrupted booking, so this is the gate for the coming fix. SIM_POISON_MODE=stall is the organic variant (Aghiles): SIGSTOP cuebot so completion acks time out and RQD re-sends, SIGCONT into the duplicate flood, the mechanism that manufactured the orphans in the real incident.
Frame completions were processed one at a time on the gRPC threads that received them: every completed frame was its own transaction, dozens of threads contending over the same frame, proc, host and stat rows. At farm scale that contention is the completion path's ceiling, and the suite shows it: batching the completions into the scheduler tick lifts every high-churn scenario (OOM 62 to 88 frames/s, DEPENDS 58 to 91, LOCALITY 60 to 100, TAGS_GPU 56 to 90, FAILOVER 53 to 209, and the priority flood runs at 165 frames/s while keeping the low-priority stream fed). How it works now: - The report thread only acks, resolves (pure reads) and queues the completion. Every cuebot drains its own queue at the start of its tick, leader and standby alike; the database stays the shared truth. - The drain applies completions in chunks, each chunk one transaction: frames stopped (state+version guarded, stat counters pre-locked in sorted order), procs deleted with their live reserved values, host and accounting resources refunded. The planner then plans against a snapshot where every freed core is already visible. - Lock order is procs, then hosts, then stats. Procs first because single-proc writers (the OOM memory bump trigger) take proc then host; hosts before stats matches the booking commit. - Follow-up work per completed frame (depends, layer/job completion, usage counters) runs on one dedicated non-droppable thread, so the tick stays a few batched statements regardless of completion rate. Batching also kills a whole failure class. Concurrent inline completions could race each other, a job shutdown and the batch commit into leaving an ORPHANED proc behind (frame back to WAITING, proc row alive); that one corpse collides with c_proc_uk on replan, rolls back the entire batch commit, and poisons every tick after it. The whole farm stops booking over one row. With completions applied by one writer in tick order that interleaving no longer exists, and two layers of armor clean up corpses from any other source: the batch commit evicts stale procs sitting on frames it just won, and a janitor sweep deletes procs whose frame has not been RUNNING for 10s. The POISON scenario committed previously reproduces the wedge deterministically and now passes in both modes (planted corpses and a SIGSTOP'd cuebot): orphans evicted, zero failed ticks, booking uninterrupted. Drain-at-tick-start is also how Plow, the schema author's scheduler at WETA, applies completions: the scheduler consumes the completion queue first on every pass, rolling the accounting into one transaction, and runs with near zero contention. That design is confirmed here. Crash semantics are deliberate: a queued completion lost with the process leaves its frame RUNNING, and host-report reconciliation requeues it. A few seconds of redone work beats at-least-once machinery. All 13 verify scenarios pass.
The locality bonus only sees LIVE procs: the moment a host loses its
last proc of a layer, the layer's pull on that host vanishes, even
though its locally cached data (texture caches, NFS client caches) is
still on the machine. Measured over 88 minutes and 555k frame starts:
77.5% of starts landed live-warm, but once a host went dark nothing
brought the layer back, natural revisits died within a minute, and
40.7% of starts ran cold.
Placement now keeps a decayed pull on vacated (host, layer) pairs:
bonus = locality_bonus * (1 - foreignFrames / window)
Age is displacement, not wall clock. What invalidates a local cache is
other layers' frames writing their data over yours, so age is counted
in frames of other work booked onto the host since the layer left it.
The scheduler keeps a per-host booking odometer; each warmth entry
stores the reading at the layer's last completion there. A busy host
cools one step per foreign frame; an idle host's odometer never moves,
so its entries stay fully warm no matter how long it idles, because
nothing displaced them.
The map is fed by the completion drain and read only on the planner
thread (no locking), expires by the same odometer comparison (self
cleaning, bounded by hosts x co-resident layers), and adds per tick
only work linear in completions, bookings and map size plus one hash
lookup per scored (host, candidate) pair. Live bonus always outranks
warm, and fit, reservations, tags and licenses are filtered before any
bonus, so warmth only breaks ties among hosts that could all take the
work. scheduler.locality_window_frames (default 64, 0 disables) is the
cache size over a typical frame's cache footprint, a site property.
Same-feed A/B in the sim: cold starts fell from 40.7% to 19.4%, the
live-warm rate rose from 80% to 90%, refill affinity reached 90% over
40k refills, and POISON plus the throughput gates were unaffected.
Scheduler.md 3.7 documents the model with a worked example; the sim's
locality watcher gains a frames mode with a warmth-age histogram, the
measurement behind these numbers.
This was referenced Aug 3, 2026
…ss facilities
Two eligibility gates existed in the legacy dispatcher but not in the
scheduler's candidate query, so jobs the old path renders were silently
starved (or misplaced) by the new one. This is the same "old books it,
new doesn't" class as the host-name tag gap, found by auditing every
legacy clause against the planner's:
* OS: a host may advertise SEVERAL OSes, comma-separated in
host_stat.str_os ("rhel7,rhel9" on mid-migration boxes). The legacy
dispatcher expands that into str_os IN ('rhel7','rhel9'); the
scheduler compared j.str_os to the raw string, exactly, so every
os-pinned job was invisible on such hosts, forever. Match any
advertised value with str_os = ANY(string_to_array(?, ',')).
* Facility: every legacy job-finding query binds job.pk_facility to the
host's facility (jobs render next to their assets; a job must never
cross sites). The candidate query had no facility clause at all, and
the frame-level plan read does not re-check it, so the planner booked
jobs onto other facilities' hosts. Carry the alloc's facility through
the host snapshot into the group key and bind it in the candidate
query.
Both are proven by a new PARITY verify scenario, the drift detector for
this whole class: inject_parity.py submits one job per eligibility
archetype (plain, os-pinned on multi-OS hosts, dual-tag alternation,
other-facility) and records which of them ever book; run_verify runs the
battery once under "--mode old" (PARITY_OLD gates the legacy baseline:
everything books except the other-facility job) and once under
"--mode new" (PARITY_NEW fails on ANY difference between the booked
sets, in either direction). On the unfixed scheduler the diff was
old-only=[parity_os], new-only=[parity_facother]; with this change the
sets are identical. Hosts advertise the multi-OS string via a new
SIM_HOST_OS knob (farm_spec.os_attrs feeding the RenderHost SP_OS
attribute, sent by the registrar, both pingers and the report loop).
The verify suite grows to 16 scenarios. SchedulerTests cover the new
facility field in the host-spec key.
…-zero streak) Reporting only, no scheduling decision changes. Three traces so one DEBUG tick tells the whole story when jobs sit WAITING with idle hosts, plus one always-on detector: * The per-group summary line now logs BEFORE the empty-continue, so every tick records candidates=N for every group. A group with idle hosts and ZERO candidates (the incident signature) was previously invisible: the line only printed when candidates existed. * Scheduler explain: when a group yields zero candidates, the candidate query's WHERE clauses are recast as per-layer boolean columns (tagOk osOk facOk hasSub underBurst underJobCap fitsCores hasWaiting limitOk folderOk managedOk) for the top 20 pending layers by priority. The first false column names the excluding gate. The subscription join is LEFT here precisely so a missing subscription shows as hasSub=false instead of an invisible row; each column mirrors its production clause (os = ANY of the host's comma-separated list, facility bind, smallest-cap limit). * Scheduler unplaced: one DEBUG line per candidate that wanted work but placed nothing, naming the binding constraint in cap-check precedence (job cap, show burst, limit, folder ceiling, license held, no floating seats, or no fitting idle host). * planZeroStreak, at WARN with no DEBUG needed: counts consecutive ticks a layer was PLANNED onto a host while planHost's commit-time read returned zero frames. That combination is the silent-starvation signature of a commit-side gate the planner does not model (thread mode, a limit filling mid-tick, local assignments): the layer burns its one commit per tick forever while looking schedulable. Warns every scheduler.plan_zero_warn_ticks consecutive strikes (default 40) with the layer and last host; counters reset on any successful plan and are dropped for layers no longer planned, so only live pathologies are tracked. To read the DEBUG traces, add the case-preserving JVM property -Dlogging.level.com.imageworks.spcue.dispatcher.Scheduler=DEBUG to cuebot's JAVA_OPTS. The environment-variable form silently misses this logger (relaxed binding lowercases the class name); if an env var is the only option, target the package instead: LOGGING_LEVEL_COM_IMAGEWORKS_SPCUE_DISPATCHER=DEBUG.
Layer tags are regex fragments typed by users, and the candidate query
compiles every pending layer's tags in one statement: one malformed tag
("comp(") aborted the whole tick, every tick, farm-wide. The legacy
dispatcher runs the same regex per show, so a bad tag only poisons that
show. Match that blast radius: catch the failure, skip that group for
the tick with a throttled WARN carrying the database error, and keep
planning the other groups.
Same fixture, host and values as DispatcherDaoTests; the only intended delta is scheduler.enabled=facility. Asserts the scheduler's candidate query finds exactly what the legacy job query finds: the fixture job on the fixture host, an os-pinned job on a multi-OS host, and refusal of a cross-facility job. CI-resident sibling of the simulator's PARITY scenario; it would have caught the multi-OS and facility drifts at commit time. Two Scheduler read methods widen to package-private as test seams.
ThreadMode.ALL hosts (NIMBY workstations by default) run only threadable layers. The planner was blind to the attribute: it parked non-threadable layers on ALL hosts (idle workstations score best), planHost's legacy re-check found zero frames, and the layer burned its one commit per tick forever while legacy booked it elsewhere. Carry int_thread_mode through the host snapshot into the group key (one bit: legacy normalizes every mode but ALL to AUTO) and bind the legacy threadability clause in the candidate query and the DEBUG explain. Parity tests cover refusal and booking on ALL hosts; the refusal test was written first and failed against the unfixed scheduler.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.java (1)
184-213: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe
startFramesAndProcsBatchjavadoc is detached from its method.The javadoc at lines 184-193 documents
startFramesAndProcsBatch, including its@param bookingsand@return. A second javadoc block starts at line 194 before any declaration, so the first block attaches to nothing andstartFramesAndProcsBatchat line 211 ends up undocumented. Move the block down to the declaration.Also use the imported
Listinstead ofjava.util.Listat lines 209 and 211-212, to match the rest of the interface.🔧 Proposed change
- /** - * Batch variant of {`@link` `#startFrameAndProc`}: commits many planned bookings in one transaction - * with batched statements, version-guarded frame RUNNING transition, proc INSERT, and host idle - * decrement, instead of one transaction and ~6 round-trips per frame. The - * subscription/layer/job/ folder/point counters are NOT written here; the Scheduler batches - * those separately. Frames that lost their optimistic version race are dropped. - * - * `@param` bookings the planned (frame, proc) pairs from the planning phase - * `@return` the subset of bookings that were actually committed (winners) - */ /** * The janitor sweep: delete every proc whose frame is no longer RUNNING (older than the given * age) and refund its host resources. Catches orphans on frames that never get planned again * (job finished or killed), which the commit-time eviction cannot reach. Returns how many were * swept. */ int sweepOrphanedProcs(int olderThanSeconds); /** * Commit a chunk of queued frame completions as ONE transaction: host rows pre-locked (sorted, * the same global order as the booking commit), every frame stopped with the state+version * guard in one batch (stat triggers fire on pre-locked counter rows), winners' max-RSS marks * coalesced per layer/job, and winners' procs batch-deleted with all release-side resource * credits applied. Returns the winner mask aligned to the input. */ - boolean[] stopFramesBatch(java.util.List<QueuedFrameCompletion> completions); + boolean[] stopFramesBatch(List<QueuedFrameCompletion> completions); - public java.util.List<FrameBooking> startFramesAndProcsBatch( - java.util.List<FrameBooking> bookings); + /** + * Batch variant of {`@link` `#startFrameAndProc`}: commits many planned bookings in one transaction + * with batched statements, version-guarded frame RUNNING transition, proc INSERT, and host idle + * decrement, instead of one transaction and ~6 round-trips per frame. The + * subscription/layer/job/folder/point counters are NOT written here; the Scheduler batches + * those separately. Frames that lost their optimistic version race are dropped. + * + * `@param` bookings the planned (frame, proc) pairs from the planning phase + * `@return` the subset of bookings that were actually committed (winners) + */ + List<FrameBooking> startFramesAndProcsBatch(List<FrameBooking> bookings);🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.java` around lines 184 - 213, Move the batch booking Javadoc, including its `@param` and `@return` tags, so it directly precedes startFramesAndProcsBatch; keep the orphan-sweep Javadoc attached to sweepOrphanedProcs. Update startFramesAndProcsBatch and stopFramesBatch to use the imported List type instead of fully qualified java.util.List.cuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.java (1)
480-483: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the booking counters to the commit path.
planHostincrementsbookedProcs,bookedCoresandbookedGpuswhile it plans. The planner does not persist anything here;startFramesAndProcsBatchlater drops any booking that loses the frame version race, andSchedulerdrops bookings whose host reservation failed. The counters therefore report more bookings than the farm actually made, and the legacydispatchHostpath increments the same counters only after a successful dispatch. Increment them from the committed winners instead.🔧 Proposed change
bookings.add(new FrameBooking(frame, proc)); - DispatchSupport.bookedProcs.getAndIncrement(); - DispatchSupport.bookedCores.addAndGet(proc.coresReserved); - DispatchSupport.bookedGpus.addAndGet(proc.gpusReserved);Then add the equivalent increments where the Scheduler processes the winners returned by
startFramesAndProcsBatch.🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.java` around lines 480 - 483, Remove the bookedProcs, bookedCores, and bookedGpus increments from the planning logic around planHost, leaving only the booking creation there. Add equivalent counter updates in the committed-winner path after startFramesAndProcsBatch results pass frame-version and host-reservation checks, matching the successful-dispatch behavior of dispatchHost.cuebot/scheduler-sim/simulate.py (5)
1546-1553: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake license failover verification fail closed.
When
license_watchimport fails,licensedbecomesFalse, so--with-licensesskips the license assertion. Whenserver_state()fails, it returns{}; no pools are checked andlic_okremains true. The five-second sampling loop can also miss a short oversubscription.Keep licensing mandatory for
--with-licenses. Treat missing or invalid state as failure or inconclusive. Use a server-side peak or violation counter instead of only current samples.Also applies to: 2462-2477, 2486-2507, 2516-2536
🤖 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 `@cuebot/scheduler-sim/simulate.py` around lines 1546 - 1553, Make license failover verification fail closed across the with-licenses test flow, including the sampling and result-reporting logic near server_state() and the final assertion. Require successful license_watch import and valid server state, treating missing, malformed, or inconclusive data as failure rather than allowing lic_ok to remain true; replace short-lived current-sample checks with a server-side peak or violation counter so transient oversubscription is detected.
1466-1495: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate old verification artifacts and check child exit status.
read_sets()reads fixed files under/tmp/scheduler-sim.run_verify()reuses eachgdirand ignoressubprocess.run()failure. A failed injector or simulation can leave old parity files orrun_util.csv, which can produce a false PASS.Remove or namespace artifacts for each verification run. Require a zero child exit code and fresh output markers before running the checks.
Also applies to: 1713-1721
🤖 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 `@cuebot/scheduler-sim/simulate.py` around lines 1466 - 1495, Update run_verify() and the parity read_sets() flow to isolate artifacts per verification run or remove existing parity result files and run_util.csv before launching child processes. Capture subprocess.run() results and fail verification when any injector or simulation exits nonzero; require fresh output markers generated by the current run before evaluating parity booked sets or CSV results, preventing stale artifacts from producing a PASS.
1896-1906: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign
--poison-testhelp with the implementation.The help says the test flips RUNNING frames to WAITING. The implementation inserts proc rows for WAITING, never-dispatched frames. Describe the terminal orphan insertion instead.
🤖 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 `@cuebot/scheduler-sim/simulate.py` around lines 1896 - 1906, Update the --poison-test argument help text to describe inserting terminal orphan proc rows for WAITING, never-dispatched frames, rather than flipping RUNNING frames back to WAITING. Keep the existing explanation of the orphaned-proc wedge, stale-proc eviction, and --feed pairing aligned with the implementation.
2296-2424: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail the POISON scenario when injection fails.
In
stallmode,SIGSTOPandSIGCONTerrors are logged and ignored. In plant mode, the SQL return code and inserted frame count are not checked. A failed insert leavespoisoned=[], so the test can report PASS after evicting zero orphans.Require successful signal transitions and a nonzero, expected injection count. Reject
SIM_POISON_COUNT <= 0before entering the observation loop.🤖 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 `@cuebot/scheduler-sim/simulate.py` around lines 2296 - 2424, Validate SIM_POISON_COUNT immediately after parsing it and fail the POISON scenario when it is nonpositive. In the stall branch of the poison scenario, require SIGSTOP and SIGCONT to succeed instead of logging and continuing after exceptions; in plant mode, check the psql INSERT result and verify the returned poisoned frame count is nonzero and matches the requested injection count. Abort before the observation loop whenever signal transitions or orphan injection validation fails.
1113-1120: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWait for license-server readiness before starting Cuebot.
spawn()returns beforefake_license.pybinds its HTTP server. Poll/licenseswith a bounded timeout and require a valid initial sample before starting Cuebot.🤖 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 `@cuebot/scheduler-sim/simulate.py` around lines 1113 - 1120, Update start_license_server to wait for fake_license.py readiness after spawn returns: poll the configured server’s /licenses endpoint with a bounded timeout, requiring a successful response containing a valid initial license sample before allowing Cuebot to start. Fail clearly if readiness is not reached within the timeout.
🧹 Nitpick comments (12)
cuebot/scheduler-sim/license_watch.py (1)
288-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the backlog-pressure floor configurable.
Every other gate in this verdict reads an environment variable (
SIM_LIC_SHARE_FACTOR,SIM_LIC_MIN_DONE,SIM_LIC_MAX_RETRIES). The backlog floor is the literal1000. A shorter or smaller run then reports INCONCLUSIVE with no way to tune it. Move the value to an environment variable next to the other thresholds.🤖 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 `@cuebot/scheduler-sim/license_watch.py` around lines 288 - 290, Make the backlog-pressure threshold in the verdict logic configurable by reading it from an environment variable alongside SIM_LIC_SHARE_FACTOR, SIM_LIC_MIN_DONE, and SIM_LIC_MAX_RETRIES, with 1000 as the default. Use the parsed threshold in both the comparison and the diagnostic message within the peak_backlog branch.cuebot/scheduler-sim/inject_license.py (1)
131-140: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm the
breakonRpcErroris intended to abandon the whole wave.
submit_waveincrementsseqbefore the launch attempt. WhenLaunchSpecraises, the function sleeps and breaks, so the consumedseqvalue maps to no submitted job. The finalsubmitted={seq}counter then overstates the number of launched jobs. Move the increment after a successfulLaunchSpec, or track a separate success counter.♻️ Proposed counter fix
def submit_wave(stub, prefix, seq): for _ in range(WAVE): - seq += 1 - xml = SPEC_HEAD + make_job(f"{prefix}-{seq:05d}", random.Random(seq * 13)) + "</spec>\n" + nxt = seq + 1 + xml = SPEC_HEAD + make_job(f"{prefix}-{nxt:05d}", random.Random(nxt * 13)) + "</spec>\n" try: stub.LaunchSpec(job_pb2.JobLaunchSpecRequest(spec=xml)) + seq = nxt except grpc.RpcError: time.sleep(2.0) # launch queue full -> back off, retry next tick break return seq🤖 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 `@cuebot/scheduler-sim/inject_license.py` around lines 131 - 140, Update submit_wave so seq advances only after LaunchSpec succeeds, or otherwise track successful submissions separately; ensure the returned seq and submitted counter count only launched jobs while preserving the existing RpcError backoff and wave-abandoning break behavior.cuebot/scheduler-sim/locality_watch.py (2)
104-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
warm_agesaccumulates without being read.
warm_agescollects one float for every non-live start but no later code reports it. The final report at Lines 190-212 useswarm_bucketsonly. On a long run this list grows without bound and returns no value. Either remove it, or report a median and a 90th percentile from it, which would add real information the buckets cannot show.♻️ Option: report percentiles instead of discarding
nb = sum(warm_buckets.values()) if nb: + if warm_ages: + s = sorted(warm_ages) + p50 = s[len(s) // 2] + p90 = s[min(len(s) - 1, int(len(s) * 0.9))] + print(f"non-live start warmth age: median {p50:.0f}s p90 {p90:.0f}s", + flush=True)Also applies to: 139-140
🤖 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 `@cuebot/scheduler-sim/locality_watch.py` at line 104, Remove the unused warm_ages accumulation from the locality simulation, including its declaration and append sites near the non-live start handling, since the final report uses warm_buckets and does not consume this list.Source: Linters/SAST tools
167-176: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the checkpoint path unique per run.
The checkpoint path is the fixed literal
/tmp/locality_warmth_checkpoint.txt. Two concurrent scenarios, or a comparison run of--mode oldand--mode new, overwrite each other's checkpoint. Derive the name fromMODEor from theCSVsetting, or place it beside the CSV output.🤖 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 `@cuebot/scheduler-sim/locality_watch.py` around lines 167 - 176, Update the checkpoint file handling in the sampling loop to derive a unique path from the current MODE or CSV setting instead of the fixed /tmp/locality_warmth_checkpoint.txt literal, so concurrent and old/new runs write to separate checkpoint files while preserving the existing checkpoint contents and error handling.Source: Linters/SAST tools
cuebot/scheduler-sim/farm_spec.py (1)
42-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize
SIM_HOST_OSbefore reporting it.The value passes through unchanged. If an operator sets
SIM_HOST_OS="rhel7, rhel9", the space is included instr_os. The legacy expansion then compares against' rhel9'and the PARITY scenario reports an asymmetry that is caused by the input, not by the scheduler. Strip whitespace around each element.♻️ Proposed normalization
-HOST_OS = os.environ.get("SIM_HOST_OS", "") +HOST_OS = ",".join( + p.strip() for p in os.environ.get("SIM_HOST_OS", "").split(",") if p.strip())🤖 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 `@cuebot/scheduler-sim/farm_spec.py` around lines 42 - 47, Normalize SIM_HOST_OS in the HOST_OS/os_attrs flow by trimming whitespace from each comma-separated operating-system value before returning SP_OS. Preserve the existing empty-value behavior and ensure inputs such as “rhel7, rhel9” report “rhel7,rhel9” without changing other scheduler logic.cuebot/src/test/java/com/imageworks/spcue/dispatcher/LicenseSourceTests.java (1)
56-67: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the
Files.walkstream.
Files.walkreturns aStreamthat holds an open directory handle. The handle stays open until the stream is closed. Wrap the walk in try-with-resources.♻️ Proposed refactor
`@After` public void tearDown() throws IOException { if (dir != null) { - Files.walk(dir).sorted(java.util.Comparator.reverseOrder()).forEach(p -> { - try { - Files.delete(p); - } catch (IOException e) { - // best effort - } - }); + try (java.util.stream.Stream<Path> paths = Files.walk(dir)) { + paths.sorted(java.util.Comparator.reverseOrder()).forEach(p -> { + try { + Files.delete(p); + } catch (IOException e) { + // best effort + } + }); + } } }🤖 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 `@cuebot/src/test/java/com/imageworks/spcue/dispatcher/LicenseSourceTests.java` around lines 56 - 67, Update LicenseSourceTests.tearDown to manage the Stream returned by Files.walk with try-with-resources, while preserving the existing reverse-order deletion and best-effort IOException handling.cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.md (1)
295-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConvert the indented code blocks to fenced blocks.
markdownlint reports MD046 at Lines 300, 311, 316, and 360. The rest of the document uses fenced blocks, so these four indented blocks are inconsistent.
🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.md` around lines 295 - 320, In Scheduler.md, update the four indented code blocks identified by the LicenseSource documentation and the reported MD046 locations to fenced Markdown blocks. Preserve each block’s existing content and language-neutral formatting, matching the fenced-block style used elsewhere in the document.Source: Linters/SAST tools
cuebot/src/test/java/com/imageworks/spcue/dispatcher/SchedulerCandidateParityTests.java (1)
120-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the negative tests against a vacuous pass.
candidates()returns an empty list when no host-spec group containsHOSTNAME. In that casecandidatesContainJobreturnsfalsefor any reason, including a host-snapshot regression unrelated to the property under test. The twoassertFalsetests (nonThreadableLayerOnAllModeHostIsRefusedByBothPaths,crossFacilityJobIsRefusedByBothPaths) then pass without exercising the scheduler filter.Fail fast when the group is missing, so a negative assertion always follows a real candidate query.
♻️ Proposed refactor
private List<Scheduler.LayerCandidate> candidates() { Map<Scheduler.HostSpecKey, List<Scheduler.BookableHost>> groups = Scheduler.groupByHostSpec(scheduler.readAllHosts()); for (Map.Entry<Scheduler.HostSpecKey, List<Scheduler.BookableHost>> e : groups.entrySet()) { int maxCores = 0; boolean mine = false; for (Scheduler.BookableHost h : e.getValue()) { if (h.coresTotal > maxCores) maxCores = h.coresTotal; if (HOSTNAME.equals(h.hostName)) mine = true; } if (mine) return scheduler.readLayerCandidatesForGroup(e.getKey(), maxCores); } - return Collections.emptyList(); + throw new AssertionError( + "host " + HOSTNAME + " is missing from the scheduler host snapshot"); }🤖 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 `@cuebot/src/test/java/com/imageworks/spcue/dispatcher/SchedulerCandidateParityTests.java` around lines 120 - 145, Update candidates() to fail immediately when no host-spec group contains HOSTNAME, instead of returning Collections.emptyList(). Preserve the existing candidate lookup for the matching group so candidatesContainJob and the negative tests nonThreadableLayerOnAllModeHostIsRefusedByBothPaths and crossFacilityJobIsRefusedByBothPaths always exercise a real scheduler query.cuebot/src/main/java/com/imageworks/spcue/dispatcher/LicenseSource.java (1)
602-616: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReading
outafter a timed-outjoinis a data race.
reader.join(2s)can return while the reader thread is still running. The code then callsout.toString()on aStringBuilderthat the reader thread may still append to.StringBuilderis not thread-safe, so the result can be truncated or the call can throw. The same applies toerrinerrExcerpt.Treat a still-alive reader as a failed poll.
♻️ Proposed refactor
reader.join(TimeUnit.SECONDS.toMillis(2)); errReader.join(TimeUnit.SECONDS.toMillis(2)); + if (reader.isAlive()) { + p.destroyForcibly(); + throw new IOException("script stdout reader did not finish; rejecting sample"); + } if (readError[0] != null) throw readError[0];🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/LicenseSource.java` around lines 602 - 616, Update the process-output cleanup in the surrounding script-execution method so each reader thread is verified stopped after its timed join, including the timeout path. Treat either still-alive reader as a failed poll and throw an IOException before accessing out, err, or errExcerpt(err); retain the existing readError and nonzero-exit handling when both readers have terminated.cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java (2)
880-887: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the license-denied branch state its real purpose.
Line 880 already sets
newState = FrameState.WAITING, so the assignment at line 886 changes nothing. The branch is load-bearing only because it short-circuits the laterelse ifchain, in particular theframe.retries >= job.maxRetriestest that would otherwise mark the frameDEAD. A reader who removes the seemingly redundant branch would silently reintroduce that path. Return directly and say so.🔧 Proposed change
- FrameState newState = FrameState.WAITING; - if (isLicenseDenied(report.getExitStatus())) { - // No license free: a contended resource, not a broken frame. - // Requeue WAITING without burning a retry. This catches the - // race the planner's gate cannot: an artist taking the last - // seat between the sample and the checkout. - newState = FrameState.WAITING; - } else if (report.getExitStatus() == FrameExitStatus.SKIP_RETRY_VALUE + if (isLicenseDenied(report.getExitStatus())) { + // No license free: a contended resource, not a broken frame. + // Return WAITING before the retry-limit and timeout checks below, + // so a busy license pool can never march a layer to DEAD. + return FrameState.WAITING; + } + + FrameState newState = FrameState.WAITING; + if (report.getExitStatus() == FrameExitStatus.SKIP_RETRY_VALUE || (job.maxRetries != 0 && report.getExitSignal() == 119)) {🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java` around lines 880 - 887, Update the license-denied branch in the frame completion state handling to return directly with the existing WAITING outcome, making clear that it bypasses the subsequent retry and DEAD-state checks. Remove the redundant newState assignment while preserving the later else-if handling for non-license-denied statuses.
137-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA static field written from an instance constructor couples all instances.
licenseDeniedStatusesisstatic volatilebut assigned in the constructor at line 153. Any secondFrameCompleteHandlerbuilt with a differentEnvironment, which happens in tests and in any context that creates more than one bean, overwrites the value for every instance. The javadoc explains the choice, and the only caller is the staticdetermineFrameState, so this works today. Pass the set intodetermineFrameStateas a parameter, or make the field an instance field and have the caller resolve it. That removes the hidden global.🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java` around lines 137 - 159, Remove the static global state for licenseDeniedStatuses in FrameCompleteHandler. Make the parsed statuses instance-scoped and update determineFrameState and its callers to use the appropriate instance value, or pass the set explicitly as a parameter, so separate handlers retain their own Environment-specific configuration.cuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.java (1)
456-468: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
continueand log the build failure
VirtualProc.build(DispatchHost, DispatchFrame, boolean, String[])has the expected parameter order. If one candidate fails, usecontinueinstead ofbreakso later candidates remain eligible. Log the exception before continuing.🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.java` around lines 456 - 468, Update the VirtualProc.build call in the host-candidate planning loop to retain its expected argument order, and change the RuntimeException handler to log the build failure before continuing to the next candidate. Replace break with continue so one failed candidate does not stop evaluation of later hosts.
🤖 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 `@cuebot/scheduler-sim/fake_rqd.py`:
- Around line 146-149: Update the `_stats` initializer to add a `lic_denied`
counter, then adjust the statistics classification branch to increment it when
`exit_status` is `_EXIT_LICENSE_DENIED` instead of counting the frame as
completed. Include the new counter in the periodic statistics output so
license-denial volume is reported.
In `@cuebot/scheduler-sim/inject_parity.py`:
- Around line 116-125: Guard the row parsing in the query-results loop so only
lines whose split on "|" yields exactly three fields are unpacked; skip
malformed rows and continue processing valid results. Preserve the existing
booking logic for valid rows and ensure main() can still complete and write
parity_booked_<mode>.txt.
In `@cuebot/scheduler-sim/license_watch.py`:
- Around line 244-245: Update the sampling logic around the rows.append call to
capture each license’s instantaneous use in the sample loop and append those
values instead of peak_use[n]. Keep peak_use for peak tracking, while ensuring
the per-license CSV columns reflect usage at the current time.
In `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.java`:
- Around line 328-343: The batched release paths omit required credit
decrements. In ProcDaoJdbc.batchDeleteVirtualProcs, reject or filter procs where
isLocalDispatch is true because this path does not refund host_local or
job_resource.int_local_cores. In ProcDaoJdbc lines 457-501, update
deleteStaleProcsByFrames and deleteOrphanedProcs to return pk_show, pk_alloc,
pk_layer, and pk_job, then route deleted procs through the same coalesced
decrement logic used by batchDeleteVirtualProcs so all subscription,
layer_resource, job_resource, folder_resource, and point credits are released.
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java`:
- Around line 330-344: Both sweepOrphanedProcs and deleteStaleProcsByFrames
credit only host resources, leaving subscription, layer_resource, job_resource,
folder_resource, and point counters over-counted. Add one shared
accounting-credit step, reusing the existing batch-delete accounting behavior,
and invoke it for the procs returned by both deletion paths; update
cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java
lines 330-344 and 417-434, with both sites requiring the same accounting credit
in addition to host refunds.
- Around line 303-325: Update the batched release flow around
procDao.batchDeleteVirtualProcs to capture the VirtualProc objects returned by
the DAO, then publish one PROC_UNBOOKED event for each returned proc. Do not
publish events for requested procs omitted by the DAO, and preserve the existing
unbooked marking and local-dispatch handling.
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`:
- Around line 262-291: Replace postCompleteExecutor with an explicitly
configured ThreadPoolExecutor using a bounded work queue and CallerRunsPolicy,
preserving ordered single-worker processing while applying back-pressure. Update
shutdown() to call executor shutdown and await termination using
healthy_threadpool.shutdown_drain_ms. Add the post-complete queue depth to the
existing Scheduler stat: output.
- Around line 232-260: Update the drain branch in handleFrameCompleteReport and
resolveForDrain so transient resolution failures are caught and wrapped in
RqdRetryReportException, matching processReportNow’s retry behavior. Preserve
the existing null return for EmptyResultDataAccessException stale or duplicate
reports, while ensuring other resolve exceptions do not propagate raw through
gRPC.
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/QueuedFrameCompletion.java`:
- Around line 26-33: Update the Javadoc in QueuedFrameCompletion to replace the
nonexistent FrameCompleteHandler#flushCompletionBatch reference with the actual
DispatchSupport#stopFramesBatch(java.util.List) link invoked for the completion
batch.
---
Outside diff comments:
In `@cuebot/scheduler-sim/simulate.py`:
- Around line 1546-1553: Make license failover verification fail closed across
the with-licenses test flow, including the sampling and result-reporting logic
near server_state() and the final assertion. Require successful license_watch
import and valid server state, treating missing, malformed, or inconclusive data
as failure rather than allowing lic_ok to remain true; replace short-lived
current-sample checks with a server-side peak or violation counter so transient
oversubscription is detected.
- Around line 1466-1495: Update run_verify() and the parity read_sets() flow to
isolate artifacts per verification run or remove existing parity result files
and run_util.csv before launching child processes. Capture subprocess.run()
results and fail verification when any injector or simulation exits nonzero;
require fresh output markers generated by the current run before evaluating
parity booked sets or CSV results, preventing stale artifacts from producing a
PASS.
- Around line 1896-1906: Update the --poison-test argument help text to describe
inserting terminal orphan proc rows for WAITING, never-dispatched frames, rather
than flipping RUNNING frames back to WAITING. Keep the existing explanation of
the orphaned-proc wedge, stale-proc eviction, and --feed pairing aligned with
the implementation.
- Around line 2296-2424: Validate SIM_POISON_COUNT immediately after parsing it
and fail the POISON scenario when it is nonpositive. In the stall branch of the
poison scenario, require SIGSTOP and SIGCONT to succeed instead of logging and
continuing after exceptions; in plant mode, check the psql INSERT result and
verify the returned poisoned frame count is nonzero and matches the requested
injection count. Abort before the observation loop whenever signal transitions
or orphan injection validation fails.
- Around line 1113-1120: Update start_license_server to wait for fake_license.py
readiness after spawn returns: poll the configured server’s /licenses endpoint
with a bounded timeout, requiring a successful response containing a valid
initial license sample before allowing Cuebot to start. Fail clearly if
readiness is not reached within the timeout.
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.java`:
- Around line 480-483: Remove the bookedProcs, bookedCores, and bookedGpus
increments from the planning logic around planHost, leaving only the booking
creation there. Add equivalent counter updates in the committed-winner path
after startFramesAndProcsBatch results pass frame-version and host-reservation
checks, matching the successful-dispatch behavior of dispatchHost.
In `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.java`:
- Around line 184-213: Move the batch booking Javadoc, including its `@param` and
`@return` tags, so it directly precedes startFramesAndProcsBatch; keep the
orphan-sweep Javadoc attached to sweepOrphanedProcs. Update
startFramesAndProcsBatch and stopFramesBatch to use the imported List type
instead of fully qualified java.util.List.
---
Nitpick comments:
In `@cuebot/scheduler-sim/farm_spec.py`:
- Around line 42-47: Normalize SIM_HOST_OS in the HOST_OS/os_attrs flow by
trimming whitespace from each comma-separated operating-system value before
returning SP_OS. Preserve the existing empty-value behavior and ensure inputs
such as “rhel7, rhel9” report “rhel7,rhel9” without changing other scheduler
logic.
In `@cuebot/scheduler-sim/inject_license.py`:
- Around line 131-140: Update submit_wave so seq advances only after LaunchSpec
succeeds, or otherwise track successful submissions separately; ensure the
returned seq and submitted counter count only launched jobs while preserving the
existing RpcError backoff and wave-abandoning break behavior.
In `@cuebot/scheduler-sim/license_watch.py`:
- Around line 288-290: Make the backlog-pressure threshold in the verdict logic
configurable by reading it from an environment variable alongside
SIM_LIC_SHARE_FACTOR, SIM_LIC_MIN_DONE, and SIM_LIC_MAX_RETRIES, with 1000 as
the default. Use the parsed threshold in both the comparison and the diagnostic
message within the peak_backlog branch.
In `@cuebot/scheduler-sim/locality_watch.py`:
- Line 104: Remove the unused warm_ages accumulation from the locality
simulation, including its declaration and append sites near the non-live start
handling, since the final report uses warm_buckets and does not consume this
list.
- Around line 167-176: Update the checkpoint file handling in the sampling loop
to derive a unique path from the current MODE or CSV setting instead of the
fixed /tmp/locality_warmth_checkpoint.txt literal, so concurrent and old/new
runs write to separate checkpoint files while preserving the existing checkpoint
contents and error handling.
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.java`:
- Around line 456-468: Update the VirtualProc.build call in the host-candidate
planning loop to retain its expected argument order, and change the
RuntimeException handler to log the build failure before continuing to the next
candidate. Replace break with continue so one failed candidate does not stop
evaluation of later hosts.
In
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`:
- Around line 880-887: Update the license-denied branch in the frame completion
state handling to return directly with the existing WAITING outcome, making
clear that it bypasses the subsequent retry and DEAD-state checks. Remove the
redundant newState assignment while preserving the later else-if handling for
non-license-denied statuses.
- Around line 137-159: Remove the static global state for licenseDeniedStatuses
in FrameCompleteHandler. Make the parsed statuses instance-scoped and update
determineFrameState and its callers to use the appropriate instance value, or
pass the set explicitly as a parameter, so separate handlers retain their own
Environment-specific configuration.
In `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/LicenseSource.java`:
- Around line 602-616: Update the process-output cleanup in the surrounding
script-execution method so each reader thread is verified stopped after its
timed join, including the timeout path. Treat either still-alive reader as a
failed poll and throw an IOException before accessing out, err, or
errExcerpt(err); retain the existing readError and nonzero-exit handling when
both readers have terminated.
In `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.md`:
- Around line 295-320: In Scheduler.md, update the four indented code blocks
identified by the LicenseSource documentation and the reported MD046 locations
to fenced Markdown blocks. Preserve each block’s existing content and
language-neutral formatting, matching the fenced-block style used elsewhere in
the document.
In
`@cuebot/src/test/java/com/imageworks/spcue/dispatcher/LicenseSourceTests.java`:
- Around line 56-67: Update LicenseSourceTests.tearDown to manage the Stream
returned by Files.walk with try-with-resources, while preserving the existing
reverse-order deletion and best-effort IOException handling.
In
`@cuebot/src/test/java/com/imageworks/spcue/dispatcher/SchedulerCandidateParityTests.java`:
- Around line 120-145: Update candidates() to fail immediately when no host-spec
group contains HOSTNAME, instead of returning Collections.emptyList(). Preserve
the existing candidate lookup for the matching group so candidatesContainJob and
the negative tests nonThreadableLayerOnAllModeHostIsRefusedByBothPaths and
crossFacilityJobIsRefusedByBothPaths always exercise a real scheduler query.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b9bbc6e-c548-4432-8c45-595ef35b8415
📒 Files selected for processing (34)
cuebot/scheduler-sim/README.mdcuebot/scheduler-sim/fake_license.pycuebot/scheduler-sim/fake_rqd.pycuebot/scheduler-sim/farm_spec.pycuebot/scheduler-sim/inject_license.pycuebot/scheduler-sim/inject_limit.pycuebot/scheduler-sim/inject_parity.pycuebot/scheduler-sim/license_watch.pycuebot/scheduler-sim/limit_watch.pycuebot/scheduler-sim/locality_watch.pycuebot/scheduler-sim/register_hosts.pycuebot/scheduler-sim/rqd_report.pycuebot/scheduler-sim/simulate.pycuebot/scheduler-sim/status_pinger.pycuebot/scheduler-sim/status_pinger_fast.pycuebot/src/main/java/com/imageworks/spcue/dao/FrameDao.javacuebot/src/main/java/com/imageworks/spcue/dao/ProcDao.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatcherDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/CoreUnitDispatcher.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/LicenseSource.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/QueuedFrameCompletion.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.mdcuebot/src/main/java/com/imageworks/spcue/dispatcher/SchedulerCompletionQueue.javacuebot/src/main/resources/conf/spring/applicationContext-service.xmlcuebot/src/main/resources/opencue.propertiescuebot/src/test/java/com/imageworks/spcue/dispatcher/LicenseSourceTests.javacuebot/src/test/java/com/imageworks/spcue/dispatcher/SchedulerCandidateParityTests.javacuebot/src/test/java/com/imageworks/spcue/dispatcher/SchedulerTests.java
💤 Files with no reviewable changes (1)
- cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatcherDaoJdbc.java
🚧 Files skipped from review as they are similar to previous changes (6)
- cuebot/scheduler-sim/status_pinger.py
- cuebot/src/test/java/com/imageworks/spcue/dispatcher/SchedulerTests.java
- cuebot/src/main/resources/conf/spring/applicationContext-service.xml
- cuebot/scheduler-sim/status_pinger_fast.py
- cuebot/scheduler-sim/rqd_report.py
- cuebot/scheduler-sim/README.md
Comment on lines
+146
to
+149
| # A license denial is not a memory failure, so it only applies to frames that | ||
| # were not already failing for memory. | ||
| if not mem_fail and _LIC_DENY_RATE > 0 and random.random() < _LIC_DENY_RATE: | ||
| exit_status = _EXIT_LICENSE_DENIED |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
License denials are counted as completions in the statistics.
The new status is set at Line 149, but the statistics branch at Lines 162-167 classifies only killed and mem_fail. A license-denied frame therefore falls into the else branch and increments _stats["completed"]. The frame did not complete; cuebot requeues it. When SIM_LIC_DENY_RATE > 0, the completed counter and any throughput derived from it are inflated, and the periodic statistics line has no way to show the denial volume.
Add a dedicated counter and report it.
🐛 Proposed fix
if not mem_fail and _LIC_DENY_RATE > 0 and random.random() < _LIC_DENY_RATE:
exit_status = _EXIT_LICENSE_DENIED
+ lic_denied = exit_status == _EXIT_LICENSE_DENIED if killed:
_stats["oom_killed"] += 1
elif mem_fail:
_stats["mem_failed"] += 1
+ elif lic_denied:
+ _stats["lic_denied"] += 1
else:
_stats["completed"] += 1Add "lic_denied": 0 to the _stats initializer and include it in the periodic statistics print.
🧰 Tools
🪛 ast-grep (0.45.1)
[info] 147-147: use secrets package over random package
Context: random.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(avoid-random-python)
🪛 Ruff (0.16.1)
[error] 148-148: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
🤖 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 `@cuebot/scheduler-sim/fake_rqd.py` around lines 146 - 149, Update the `_stats`
initializer to add a `lic_denied` counter, then adjust the statistics
classification branch to increment it when `exit_status` is
`_EXIT_LICENSE_DENIED` instead of counting the frame as completed. Include the
new counter in the periodic statistics output so license-denial volume is
reported.
Comment on lines
+116
to
+125
| rows = q("SELECT j.str_name," | ||
| " COALESCE((SELECT count(*) FROM proc p WHERE p.pk_job=j.pk_job),0)," | ||
| " COALESCE((SELECT count(*) FROM frame f WHERE f.pk_job=j.pk_job" | ||
| " AND f.str_state='SUCCEEDED'),0)" | ||
| " FROM job j WHERE j.str_name LIKE '%parity%';") | ||
| for r in [x for x in rows.split("\n") if x]: | ||
| jname, procs, done = r.split("|") | ||
| for name in booked: | ||
| if name in jname and (int(procs) > 0 or int(done) > 0): | ||
| booked[name] = True |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the row parse so one malformed line does not abort the run.
r.split("|") must return exactly three fields. If q() returns any other line, the unpack raises ValueError, main() exits, and no parity_booked_<mode>.txt file is written. The verify step then has no result to read. Every other query helper in this directory tolerates a bad read and returns a default.
🛡️ Proposed guard
for r in [x for x in rows.split("\n") if x]:
- jname, procs, done = r.split("|")
+ parts = r.split("|")
+ if len(parts) != 3:
+ continue
+ jname, procs, done = parts
for name in booked:
- if name in jname and (int(procs) > 0 or int(done) > 0):
- booked[name] = True
+ if name in jname and (procs.strip().isdigit() and int(procs) > 0
+ or done.strip().isdigit() and int(done) > 0):
+ booked[name] = True📝 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
| rows = q("SELECT j.str_name," | |
| " COALESCE((SELECT count(*) FROM proc p WHERE p.pk_job=j.pk_job),0)," | |
| " COALESCE((SELECT count(*) FROM frame f WHERE f.pk_job=j.pk_job" | |
| " AND f.str_state='SUCCEEDED'),0)" | |
| " FROM job j WHERE j.str_name LIKE '%parity%';") | |
| for r in [x for x in rows.split("\n") if x]: | |
| jname, procs, done = r.split("|") | |
| for name in booked: | |
| if name in jname and (int(procs) > 0 or int(done) > 0): | |
| booked[name] = True | |
| rows = q("SELECT j.str_name," | |
| " COALESCE((SELECT count(*) FROM proc p WHERE p.pk_job=j.pk_job),0)," | |
| " COALESCE((SELECT count(*) FROM frame f WHERE f.pk_job=j.pk_job" | |
| " AND f.str_state='SUCCEEDED'),0)" | |
| " FROM job j WHERE j.str_name LIKE '%parity%';") | |
| for r in [x for x in rows.split("\n") if x]: | |
| parts = r.split("|") | |
| if len(parts) != 3: | |
| continue | |
| jname, procs, done = parts | |
| for name in booked: | |
| if name in jname and (procs.strip().isdigit() and int(procs) > 0 | |
| or done.strip().isdigit() and int(done) > 0): | |
| booked[name] = 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 `@cuebot/scheduler-sim/inject_parity.py` around lines 116 - 125, Guard the row
parsing in the query-results loop so only lines whose split on "|" yields
exactly three fields are unpacked; skip malformed rows and continue processing
valid results. Preserve the existing booking logic for valid rows and ensure
main() can still complete and write parity_booked_<mode>.txt.
Comment on lines
+244
to
+245
| rows.append((time.time() - t0, util, backlog, unlic, | ||
| [peak_use[n] for n in names])) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Record the instantaneous per-license usage in the CSV, not the running peak.
Each row appends peak_use[n], which is monotonic. The CSV header at Line 258 names one column per license, so a reader expects the usage at time t. As written, every license column is a non-decreasing curve and the shape of real usage over the run is lost. Capture use per license in the sample loop and append that.
🐛 Proposed fix
line = []
+ use_now = {}
for n in names:
@@
use = len(hosts.get(n, [])) if host_based.get(n) else nframes
+ use_now[n] = use
peak_use[n] = max(peak_use[n], use)
@@
rows.append((time.time() - t0, util, backlog, unlic,
- [peak_use[n] for n in names]))
+ [use_now[n] for n in names]))📝 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
| rows.append((time.time() - t0, util, backlog, unlic, | |
| [peak_use[n] for n in names])) | |
| rows.append((time.time() - t0, util, backlog, unlic, | |
| [use_now[n] for n in names])) |
🤖 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 `@cuebot/scheduler-sim/license_watch.py` around lines 244 - 245, Update the
sampling logic around the rows.append call to capture each license’s
instantaneous use in the sample loop and append those values instead of
peak_use[n]. Keep peak_use for peak tracking, while ensuring the per-license CSV
columns reflect usage at the current time.
Comment on lines
+328
to
+343
| @Override | ||
| public List<VirtualProc> batchDeleteVirtualProcs(List<VirtualProc> procs) { | ||
| if (procs == null || procs.isEmpty()) { | ||
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| // 1. One DELETE for the whole batch. RETURNING carries the reserved | ||
| // amounts as of deletion time (a concurrent memory bump must be | ||
| // refunded at its final value, not the cached one). Procs someone | ||
| // else already deleted don't come back and get no refunds. | ||
| Map<String, VirtualProc> byId = new HashMap<String, VirtualProc>(procs.size() * 2); | ||
| List<Object> ids = new ArrayList<Object>(procs.size()); | ||
| for (VirtualProc proc : procs) { | ||
| byId.put(proc.getProcId(), proc); | ||
| ids.add(proc.getProcId()); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The batched release paths apply only part of the credits that procDestroyed applies. procDestroyed credits host idle, the five accounting tables, and the local-dispatch tables. The new batched paths each drop a different subset of those credits, and both omissions are enforced by javadoc rather than by code, so a violation leaks counters silently instead of failing.
cuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.java#L328-L343: reject or filterproc.isLocalDispatchinbatchDeleteVirtualProcs, because this method never creditshost_localorjob_resource.int_local_cores.cuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.java#L457-L501: addpk_show,pk_alloc,pk_layerandpk_jobto theRETURNINGclauses ofdeleteStaleProcsByFramesanddeleteOrphanedProcs, then route the release through the same coalesced decrementsbatchDeleteVirtualProcsperforms, so swept procs do not ratchetsubscription,layer_resource,job_resource,folder_resourceandpoint.
📍 Affects 1 file
cuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.java#L328-L343(this comment)cuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.java#L457-L501
🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.java`
around lines 328 - 343, The batched release paths omit required credit
decrements. In ProcDaoJdbc.batchDeleteVirtualProcs, reject or filter procs where
isLocalDispatch is true because this path does not refund host_local or
job_resource.int_local_cores. In ProcDaoJdbc lines 457-501, update
deleteStaleProcsByFrames and deleteOrphanedProcs to return pk_show, pk_alloc,
pk_layer, and pk_job, then route deleted procs through the same coalesced
decrement logic used by batchDeleteVirtualProcs so all subscription,
layer_resource, job_resource, folder_resource, and point credits are released.
Comment on lines
+303
to
+325
| // 3. Release the winners' procs in this same transaction, so freed | ||
| // capacity is visible to the plan that runs right after this flush. | ||
| // proc.unbooked=true makes the later post-complete operations treat | ||
| // the proc as already released (their unbook is a no-op). Local | ||
| // dispatches keep the per-proc path (different credit tables). | ||
| List<VirtualProc> releasable = new ArrayList<VirtualProc>(winners.size()); | ||
| List<DispatchFrame> localFrames = new ArrayList<DispatchFrame>(); | ||
| for (QueuedFrameCompletion c : winners) { | ||
| if (c.proc.isLocalDispatch) { | ||
| localFrames.add(c.frame); | ||
| } else { | ||
| releasable.add(c.proc); | ||
| } | ||
| } | ||
| if (!releasable.isEmpty()) { | ||
| procDao.batchDeleteVirtualProcs(releasable); | ||
| for (VirtualProc proc : releasable) { | ||
| proc.unbooked = true; | ||
| } | ||
| } | ||
| if (!localFrames.isEmpty()) { | ||
| procDao.batchClearVirtualProcAssignments(localFrames); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any proc event is published for the batched delete path.
rg -n -C5 'PROC_UNBOOKED|publishProcEvent' cuebot/src/main/java
ast-grep run --pattern 'procDao.batchDeleteVirtualProcs($$$)' --lang java cuebot/src/main/javaRepository: AcademySoftwareFoundation/OpenCue
Length of output: 6205
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- batch delete implementation and callers ---'
rg -n -C12 'batchDeleteVirtualProcs|batchClearVirtualProcAssignments' cuebot/src/main/java cuebot/src/test || true
echo '--- release and booking event call sites ---'
rg -n -C8 'PROC_(BOOKED|UNBOOKED)|unbookProc|deleteVirtualProc' cuebot/src/main/java cuebot/src/test || true
echo '--- DAO structure ---'
ast-grep outline cuebot/src/main/java/com/imageworks/spcue/dao/ProcDaoJdbc.java --match 'batchDeleteVirtualProcs' --view expanded || trueRepository: AcademySoftwareFoundation/OpenCue
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- batch delete body ---'
sed -n '329,520p' cuebot/src/main/java/com/imageworks/spcue/dao/postgres/ProcDaoJdbc.java
echo '--- event types and monitoring consumers ---'
rg -n -C5 'enum EventType|PROC_UNBOOKED|PROC_BOOKED|ProcEvent' \
cuebot/src/main/java cuebot/src/test \
-g '*.java' | head -n 500
echo '--- completion flush contract and tests ---'
rg -n -C8 'flush|QueuedFrameCompletion|batchDeleteVirtualProcs|PROC_UNBOOKED' \
cuebot/src/test cuebot/src/main/java/com/imageworks/spcue/dispatcher \
-g '*.java' | head -n 500Repository: AcademySoftwareFoundation/OpenCue
Length of output: 50389
Publish PROC_UNBOOKED for batched deletions.
batchDeleteVirtualProcs deletes procs and updates accounting, but it does not publish proc events. Publish one event for each proc returned by the DAO, not each requested proc, because already-deleted procs are excluded.
🤖 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
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java`
around lines 303 - 325, Update the batched release flow around
procDao.batchDeleteVirtualProcs to capture the VirtualProc objects returned by
the DAO, then publish one PROC_UNBOOKED event for each returned proc. Do not
publish events for requested procs omitted by the DAO, and preserve the existing
unbooked marking and local-dispatch handling.
Comment on lines
+330
to
+344
| @Override | ||
| @Transactional(propagation = Propagation.REQUIRED) | ||
| public int sweepOrphanedProcs(int olderThanSeconds) { | ||
| List<VirtualProc> orphans = procDao.deleteOrphanedProcs(olderThanSeconds); | ||
| if (!orphans.isEmpty()) { | ||
| procDao.refundHostResourcesBatch(orphans); | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (VirtualProc p : orphans) { | ||
| sb.append(' ').append(p.frameId); | ||
| } | ||
| logger.warn("janitor swept " + orphans.size() + " orphaned proc(s) whose frames are" | ||
| + " no longer RUNNING (crash or failed completion left them); frames:" + sb); | ||
| } | ||
| return orphans.size(); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Both proc-deletion paths credit only the host row. procDao.refundHostResourcesBatch updates host counters. Proc booking also increments subscription, layer_resource, job_resource, folder_resource, and point, and only batchDeleteVirtualProcs decrements them. Every proc removed through these two paths leaves those counters permanently over-counted, which shrinks burst and folder/job core ceilings over time. Add one shared accounting-credit step and call it from both sites.
cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java#L330-L344: credit the accounting tables for the procs returned bydeleteOrphanedProcs, not only the host rows.cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java#L417-L434: credit the accounting tables for the procs returned bydeleteStaleProcsByFrames, not only the host rows.
📍 Affects 1 file
cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java#L330-L344(this comment)cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java#L417-L434
🤖 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
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java`
around lines 330 - 344, Both sweepOrphanedProcs and deleteStaleProcsByFrames
credit only host resources, leaving subscription, layer_resource, job_resource,
folder_resource, and point counters over-counted. Add one shared
accounting-credit step, reusing the existing batch-delete accounting behavior,
and invoke it for the procs returned by both deletion paths; update
cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupportService.java
lines 330-344 and 417-434, with both sites requiring the same accounting credit
in addition to host refunds.
Comment on lines
+232
to
+260
| public QueuedFrameCompletion resolveForDrain(FrameCompleteReport report) { | ||
| try { | ||
| final VirtualProc proc = hostManager.getVirtualProc(report.getFrame().getResourceId()); | ||
| final DispatchJob job = jobManager.getDispatchJob(proc.getJobId()); | ||
| final LayerDetail layer = jobManager.getLayerDetail(report.getFrame().getLayerId()); | ||
| final FrameDetail frameDetail = | ||
| jobManager.getFrameDetail(report.getFrame().getFrameId()); | ||
| final DispatchFrame frame = jobManager.getDispatchFrame(report.getFrame().getFrameId()); | ||
| final FrameState newFrameState = determineFrameState(job, layer, frame, report); | ||
| int exitStatus = report.getExitStatus(); | ||
| if (frameDetail.exitStatus == Dispatcher.EXIT_STATUS_MEMORY_FAILURE) { | ||
| exitStatus = frameDetail.exitStatus; | ||
| } | ||
| if (isLicenseDenied(exitStatus)) { | ||
| logger.info("frame " + frame.getName() + " could not get a license (exit " | ||
| + exitStatus + "); requeueing without spending a retry"); | ||
| exitStatus = FrameExitStatus.SKIP_RETRY_VALUE; | ||
| } | ||
| return new QueuedFrameCompletion(report, proc, job, layer, frameDetail, frame, | ||
| newFrameState, exitStatus); | ||
| } catch (EmptyResultDataAccessException e) { | ||
| // Duplicate or stale report: the proc (or frame) is already gone. | ||
| // The single-threaded drain makes this the ONLY way a duplicate | ||
| // shows up -- there is no concurrent twin to race. | ||
| logger.debug("drain: stale/duplicate completion report for frame " | ||
| + report.getFrame().getFrameName() + ": " + e.getMessage()); | ||
| return null; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
resolveForDrain changes the error contract seen by RQD.
resolveForDrain catches only EmptyResultDataAccessException. Every other exception propagates out of handleFrameCompleteReport unwrapped, because the drain branch at lines 213-222 has no try. processReportNow wraps the same class of failure in RqdRetryReportException (lines 444-455) so RQD retries the report. On the drain path RQD instead receives a raw runtime exception over gRPC.
Wrap the drain branch so a transient resolve failure produces the same retry signal.
🔧 Proposed change
if (SchedulerMode.enabled(env)) {
- QueuedFrameCompletion resolved = resolveForDrain(report);
+ QueuedFrameCompletion resolved;
+ try {
+ resolved = resolveForDrain(report);
+ } catch (Exception e) {
+ throw new RqdRetryReportException("error resolving the frame complete "
+ + "report for the scheduler drain, sending retry message to RQD " + e, e);
+ }
if (resolved == null) {
return;
}🤖 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
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`
around lines 232 - 260, Update the drain branch in handleFrameCompleteReport and
resolveForDrain so transient resolution failures are caught and wrapped in
RqdRetryReportException, matching processReportNow’s retry behavior. Preserve
the existing null return for EmptyResultDataAccessException stale or duplicate
reports, while ensuring other resolve exceptions do not propagate raw through
gRPC.
Comment on lines
+262
to
+291
| /** | ||
| * The post-complete worker for the drain: one dedicated, NON-droppable thread. The drain's | ||
| * batched stop already did every write the planner depends on (frame stopped, proc deleted, | ||
| * resources refunded), so the follow-up work per frame (depend satisfaction, layer/job | ||
| * completion checks, usage counters) can lag a little without hurting anyone; running it inside | ||
| * the tick would multiply the tick time by the completion rate, and putting it on dispatchQueue | ||
| * would let load-shedding silently drop depend satisfaction (a job then hangs forever). An | ||
| * unbounded single-thread queue drops nothing and stays ordered. | ||
| */ | ||
| private final ExecutorService postCompleteExecutor = Executors.newSingleThreadExecutor(r -> { | ||
| Thread t = new Thread(r, "CompletionPostOps"); | ||
| t.setDaemon(true); | ||
| return t; | ||
| }); | ||
|
|
||
| /** | ||
| * Queue a drained (already stopped) completion's follow-up work on the post-complete worker. | ||
| * Called by the Scheduler's drain for every frame its batched stop won. | ||
| */ | ||
| public void queuePostOps(final QueuedFrameCompletion c) { | ||
| postCompleteExecutor.execute(() -> { | ||
| try { | ||
| handlePostFrameCompleteOperations(c.proc, c.report, c.job, c.frame, c.newFrameState, | ||
| c.frameDetail); | ||
| } catch (RuntimeException e) { | ||
| logger.warn("post-complete operations for frame " + c.frame.getName() + " failed: " | ||
| + CueExceptionUtil.getStackTrace(e)); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the post-complete queue and shut the executor down.
postCompleteExecutor uses Executors.newSingleThreadExecutor, which is backed by an unbounded LinkedBlockingQueue. Two consequences:
- If the follow-up work slows down (depend satisfaction retries sleep up to 700 ms each, and
satisfyWhatDependsOnis DB-heavy), the queue grows without limit while completions keep arriving. There is no size metric and no back-pressure, so the failure mode is heap growth rather than a visible drop. shutdown()sets theshutdownflag but never callspostCompleteExecutor.shutdown(). The thread is a daemon, so the JVM exits with queued depend satisfactions unexecuted. Downstream frames then stay inDEPENDuntil a maintenance sweep recovers them.
Use an explicitly sized ThreadPoolExecutor with a bounded queue and a caller-runs policy, and drain the executor in shutdown() with a timeout aligned to healthy_threadpool.shutdown_drain_ms. Expose the queue depth on the existing Scheduler stat: line.
🤖 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
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java`
around lines 262 - 291, Replace postCompleteExecutor with an explicitly
configured ThreadPoolExecutor using a bounded work queue and CallerRunsPolicy,
preserving ordered single-worker processing while applying back-pressure. Update
shutdown() to call executor shutdown and await termination using
healthy_threadpool.shutdown_drain_ms. Add the post-complete queue depth to the
existing Scheduler stat: output.
Comment on lines
+26
to
+33
| /** | ||
| * A frame completion report, fully resolved on the RQD report thread and queued for the scheduler's | ||
| * batched flush at the next tick start (see {@link FrameCompleteHandler#flushCompletionBatch}). | ||
| * Everything the flush needs is captured here so the batch never re-reads what the report thread | ||
| * already loaded; in particular {@code frame.getVersion()} is the version observed at arrival, | ||
| * which is what makes the batched stop update lose cleanly (0 rows) if a kill, eat or retry beat | ||
| * the flush to the frame. | ||
| */ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether flushCompletionBatch exists anywhere.
rg -nP -C 3 '\bflushCompletionBatch\b' cuebot/src/main/java
ast-grep outline cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java --items all --type functionRepository: AcademySoftwareFoundation/OpenCue
Length of output: 1340
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- QueuedFrameCompletion.java ---'
cat -n cuebot/src/main/java/com/imageworks/spcue/dispatcher/QueuedFrameCompletion.java | sed -n '1,90p'
printf '%s\n' '--- Relevant symbols and call sites ---'
rg -n -P -C 4 'flushCompletionBatch|stopFramesBatch|resolveForDrain|queuePostOps|handleStaleCompletion|processReportNow' cuebot/src/main/java
printf '%s\n' '--- Scheduler and handler structure ---'
ast-grep outline cuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.java --items all --type function
ast-grep outline cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java --items all --type function
ast-grep outline cuebot/src/main/java/com/imageworks/spcue/dispatcher/DispatchSupport.java --items all --type functionRepository: AcademySoftwareFoundation/OpenCue
Length of output: 21006
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Scheduler drain declaration and implementation ---'
rg -n -P -C 8 'void .*drain|drain.*Completion|stopFramesBatch' cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java
printf '%s\n' '--- Scheduler class and nearby methods ---'
cat -n cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java | sed -n '700,815p'
printf '%s\n' '--- Cross-references to the drain method ---'
rg -n -P '\b(drain[A-Za-z0-9_]*|stopFramesBatch)\b' cuebot/src/main/java/com/imageworks/spcue/dispatcherRepository: AcademySoftwareFoundation/OpenCue
Length of output: 18558
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Scheduler drain declaration and implementation ---'
rg -n -P -C 8 'void .*drain|drain.*Completion|stopFramesBatch' cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java
printf '%s\n' '--- Scheduler class and nearby methods ---'
cat -n cuebot/src/main/java/com/imageworks/spcue/dispatcher/Scheduler.java | sed -n '700,815p'
printf '%s\n' '--- Cross-references to drain methods ---'
rg -n -P '\b(drain[A-Za-z0-9_]*|stopFramesBatch)\b' cuebot/src/main/java/com/imageworks/spcue/dispatcherRepository: AcademySoftwareFoundation/OpenCue
Length of output: 18555
Point the Javadoc link to DispatchSupport#stopFramesBatch(java.util.List). FrameCompleteHandler#flushCompletionBatch does not exist. Scheduler#runTick() invokes DispatchSupport#stopFramesBatch(java.util.List) for the completion batch.
🤖 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
`@cuebot/src/main/java/com/imageworks/spcue/dispatcher/QueuedFrameCompletion.java`
around lines 26 - 33, Update the Javadoc in QueuedFrameCompletion to replace the
nonexistent FrameCompleteHandler#flushCompletionBatch reference with the actual
DispatchSupport#stopFramesBatch(java.util.List) link invoked for the completion
batch.
Entire-Checkpoint: f68a943135a3
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.java (1)
957-982: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the proc-query bind order with the SQL placeholders.
The non-local proc caller in
DispatcherDaoJdbc.java, Lines 405-426, passes(layerId, layerId, hostName, limit)after the resource arguments. This query consumes(layerId, hostName, layerId, limit)at Lines 957, 963, 977. The caller therefore binds the layer ID toh.str_nameand the host name tol.pk_layer. PostgreSQL can reject the comparison or return no frames, so non-local proc dispatch fails.Reorder the caller arguments and add a regression test.
Proposed fix
frames = getJdbcTemplate().query(FIND_DISPATCH_FRAME_BY_LAYER_AND_PROC, FrameDaoJdbc.DISPATCH_FRAME_MAPPER, proc.coresReserved, proc.memoryReserved, proc.gpusReserved, proc.gpuMemoryReserved, layer.getLayerId(), - layer.getLayerId(), proc.hostName, limit); + proc.hostName, layer.getLayerId(), limit);🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.java` around lines 957 - 982, Update the non-local proc query invocation in DispatcherDaoJdbc.java to bind arguments in the SQL placeholder order: layerId, hostName, layerId, then limit after the resource arguments. Add a regression test covering non-local proc dispatch that verifies the host name and layer ID are bound to the correct placeholders.
🤖 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.
Outside diff comments:
In `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.java`:
- Around line 957-982: Update the non-local proc query invocation in
DispatcherDaoJdbc.java to bind arguments in the SQL placeholder order: layerId,
hostName, layerId, then limit after the resource arguments. Add a regression
test covering non-local proc dispatch that verifies the host name and layer ID
are bound to the correct placeholders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ca667cf-d1df-49de-bdf7-e61bff8b8f89
📒 Files selected for processing (1)
cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.java
Fix the batch-start locking javadoc, note the transaction requirement on the stat pre-locks, add a legend to the score formula, explain the advisory lock choice, and clean comment punctuation. Comments and docs only.
Expose the scheduler's behaviour as Prometheus metrics and read them from a Grafana dashboard. Metrics cover tick duration, frames dispatched per show, group pass reasons and totals, per-show farm cores, fragmentation by reason, and active vs inactive host-spec groups, under a human-readable label vocabulary. The dashboard draws per-show throughput and farm share as stacked bars, so each show's contribution and the farm-wide total read at a glance. On the simulator side, add the fragmentation scenario tooling, a multi-show feeder (sim1 to sim5) with per-show priority, and multi-tag hosts, and start each cuebot with the Prometheus collector on so the stack can be scraped.
A permissive layer (loose tags, or plain 'general') is a candidate in every host-spec group it fits. The within-group break already caps it to one host per group per tick, but nothing stopped it being planned again in the next group: each group re-planned it onto its own host, the parallel per-host plan reads then pulled the same waiting frames, and every copy but one lost the frame.int_version race at commit. The waste was real: candidate scans, reads and VirtualProc construction, plus idle cores stolen from siblings that booked nothing. Add a tick-wide placedLayerIds set (cleared with plannedByHost). submitCommit records the placed layer, and the candidate loop skips a layer already placed in an earlier group this tick. The skip sits after seenLayerIds.add, so reservation sweeping still sees the layer, and before any host/cap mutation, so a duplicate consumes no simulated resources. It keys on placement, not candidacy, so a layer that could not fit an earlier group is still tried in later ones. Measured on the simulator under a 120-tag, 30% run-anywhere farm (about 120 host-spec groups, 1553 hosts): raceLost fell from about 97% of planned to 0 (planned now equals committed). Utilisation is unchanged: at this tag count the farm is fragmentation-limited, not planning-limited. sim: guard the fix with a TAGMAX scenario in the --verify battery. tagmax_watch reads the Scheduler's per-window stat line and fails if raceLost exceeds a small fraction of planned (default 0.10) across the fragmented farm, with planned and host-spec-group floors so it cannot pass on an idle run. Add the --tagmax-test flag, its wiring and a README row, plus SIM_GENERAL_FRAC in farm_spec: the fraction of layers that carry no capability tag (run-anywhere 'general' work, a candidate in every group). 0 by default; the scenario uses 120 tags and 0.3.
Break the 527-line doTick into small, single-purpose methods so the tick reads as its phases: the completion drain, the leadership gate, then 1. snapshot, 2. group, 3. plan, 4. commit. The extracted methods (planGroup, planBookings, recordCommitted, stampWarmthAndLaunch, snapshotFarmFill, grantReservations, trimOverFolderCeiling, trimOverLicensePools, clearTickScratch, drainResolvedCompletions, expireDisplacedWarmth, and friends) each carry a plain prose header. runTick stays a thin Quartz harness that times the pass and rolls the leader counters into the window summary; doTick returns the procs dispatched, or -1 for a standby that did not plan.
There is no logic change in here. This should make the scheduler much more readable. Scheduler.md documentation has been updated as well.
A blocked wide layer could reserve a host and drain it, but two gaps let it starve anyway: - The reservation was not firm. A running frame or a higher-priority reserver could seize the host mid-drain, so the wide job never assembled its block and stranded forever. - Grants were ordered strictly by priority, so a low-priority wide job was starved of the scarce reservation budget by any steady higher-priority stream. Fix both: - Make reservations firm. reservationAllows is owner-only and pickReservationTarget only ever claims a free host, so once a layer holds a host nothing takes it away, not a running frame and not another reserver. Higher-priority work still borrows the draining host's spare cores through EASY backfill (never owning), so the owner is never delayed; the host drains and the wide job runs. - Grant by a priority-weighted lottery, the same one the dispatcher uses (key = random()^(1/priority), Efraimidis-Spirakis), so a low-priority wide job keeps a proportional share of the budget instead of being starved. The RESERVATIONS --verify scenario asserts the stranded wide jobs reserve, drain, and actually run, with a farm-wide throughput floor so a dead farm cannot pass.
Stats only, no scheduling change. Each tick puts every waiting frame from the candidate layers into one of six buckets: flowing, capacity, no fit, limit, no license, held. The tally goes to the Prometheus gauge cue_scheduler_waiting_frames. A live booking ledger feeds cue_scheduler_running_frames, the denominator, so the board shows each cause as a percent of all frames. No SQL is used for stats. The fragmentation metric is removed. The board gains waitlist and utilisation panels. The verify battery asserts each bucket fires.
A legacy trigger rejects any plus that lands over a cap. When a user lowers a job's max cores, or an admin shrinks a subscription burst, below live usage, the batched flush aborts. The pluses wedge in the retry buffer, completions keep subtracting, and the mirror goes negative (seen in production as a job at -14 cores). The flush now writes each guarded table as a cap-neutral pair of updates that the trigger skips; the cap is net unchanged. The planner still enforces caps at plan time. The CAPDROP scenario drops both caps under load and fails on divergence, a negative mirror, or a rejected flush.
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.
Related Issues
Related to #1001
Summarize your change
This PR adds an in-process E-PVM ("opportunity-cost") scheduler for Cuebot, plus a DB-backed simulator to validate it end-to-end. It is disabled by default and fully additive. the existing report-driven dispatcher is untouched unless the new scheduler is explicitly turned on.
What it does
A new planning-based scheduler (
Scheduler.java,scheduler.enabled=facility|managed). Instead of reacting to each host report, it plans placements farm-wide on a tick:limit_record) and folder/group core ceilings (folder_resource.int_max_cores) are enforced exactly, so e.g. a comp group can be capped to bound concurrent network traffic.A DB-backed simulator (
cuebot/scheduler-sim/) that drives the real Cuebot scheduler against a throwaway Postgres and a fake RQD, with a one-command self-test:It runs six scenarios back-to-back: OOM, priority spread, priority starvation, wide-job reservations, license limits, and folder ceilings each on a fresh farm, and prints a PASS/FAIL summary with graphs.
Why
The reactive dispatcher makes good local decisions but has no farm-wide view, so packing, priority proportionality, and hard constraints (licenses, group caps) are only loosely honored. A planning scheduler that scores opportunity cost and enforces those constraints at plan time improves utilization and makes behavior predictable. The simulator gives a reproducible, hermetic way to validate all of this against real Cuebot + Postgres, rather than by hand on a live farm.
Backward compatibility
scheduler.enabled=no) : the new scheduler never runs unless enabled.SchedulerMode, and the legacy dispatch SQL (DispatchQuery.java) is not modified. New scheduler code is additive (new files / new method overloads).findNextDispatchFrames(layer, host)(upstream had transposed bind params, so that query returned no frames, this also repairs the legacy local layer-partition path), and a guard so completed procs aren't unbooked-for-rebook whenturn_off_booking=true(a no-op for the default config).Testing
simulate.py --verify— all six scenarios PASS on this branch.SchedulerTests.java.Branch is rebased onto current
master.LLM usage disclosure
Version: Open 4.7 - 4.8 when available.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation