feat(spur-cli): runtime partition CRUD via Raft WAL with scontrol reconfigure - #425
Conversation
yansun1996
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
- Coverage 76.03% 75.74% -0.29%
==========================================
Files 166 166
Lines 63002 64526 +1524
==========================================
+ Hits 47898 48871 +973
- Misses 15104 15655 +551 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds runtime partition CRUD and scontrol reconfigure support to Spur, persisting partition state through the Raft WAL/snapshots and extending partition ACL enforcement (accounts + QoS) so behavior matches Slurm semantics more closely.
Changes:
- Adds gRPC + CLI (
scontrol) support for create/update/delete partition andreconfigure, with Raft WAL-backed persistence and snapshot/tombstone handling. - Extends partition model and config/proto to support
deny_qos, and fixesAllowAccounts/DenyAccountsenforcement to work without a Postgres association cache. - Adds comprehensive native-host E2E coverage for partition lifecycle, persistence across restart, Slurm key=value syntax, and
reconfigurebehavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/native_host/e2e/test_partitions.py | New E2E suite validating partition CRUD, persistence, reconfigure semantics, and ACL enforcement. |
| proto/slurm.proto | Adds partition management RPCs/messages and new deny_qos fields. |
| crates/spurctld/src/server.rs | Implements partition CRUD + reconfigure RPC handlers and proto conversions. |
| crates/spurctld/src/cluster.rs | Adds partition CRUD/reconfigure logic, WAL apply handling, snapshot/tombstone support, and QoS enforcement. |
| crates/spurctld/src/main.rs | Wires controller startup to retain config path for runtime reconfigure. |
| crates/spur-cli/src/scontrol.rs | Adds new scontrol subcommands and Slurm-compatible key=value parsing for partitions. |
| crates/spur-core/src/wal.rs | Introduces WAL operation variants for partition create/update/delete. |
| crates/spur-core/src/config.rs | Extends partition config/build to carry deny_qos and related fields. |
| crates/spurctld/src/raft.rs | Extends Raft client response to report partition creation outcome. |
| crates/spurctld/src/rpc_middleware.rs | Updates test fixtures for new proto partition field. |
| crates/spurctld/src/metrics_server.rs | Updates test fixtures for new proto partition field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6afc925 to
34d4d15
Compare
34d4d15 to
7ff9f23
Compare
62d4994 to
e8426fb
Compare
There was a problem hiding this comment.
Few comments on the reconfigure changes:
-
scontrol reconfigure never contacts a compute node. No file under crates/spurd/ is in the diff and SlurmAgent has no Reconfigure RPC, so node-side hooks, the device registry, and memlock are permanently restart-only. docs/deployment/partitioning.rst:100 lists [hooks] under "Applied live" without distinguishing controller hooks from node hooks — so an operator who edits a root-executed node prolog is told their change took effect when it did not. Real Slurm forwards to every slurmd, so this is a compatibility gap too.
-
The warnings mechanism does not exist in any form. proto/slurm.proto:336 is rpc Reconfigure(Empty) returns (Empty), and there is no old-versus-new config comparison anywhere in reconfigure(). Because the RPC returns Empty, this can't be added later without a proto change. There's a nasty concrete case: admission.mode is live-reloadable while jwt_key is startup-pinned, so an operator who enables token admission and sets a secret key in the same edit gets the mode flipped live and the key ignored — the controller then signs node tokens with the hardcoded "spur-default-key" fallback, and the startup warning that would have caught it never fires.
-
Leader forwarding was inverted, and it's load-bearing. Because reconfigure now proposes partition ops to the Raft WAL, it must run on the leader — that part is forced arithmetic, not a choice. But only partitions replicate; licenses, hooks, admission mode and the rest stay leader-local, so the system sits in a partially-replicated state neither design produces cleanly. On bare metal, editing the conf on a follower and running scontrol reconfigure reports success while the leader re-reads its own unchanged file.
-
Scheduler tunables are now more pinned than before. interval_secs, max_jobs_per_cycle, and topology were all made restart-only to fix a real drift bug where the preemption hold read the cadence live while the loop didn't. But that drift came from two readers disagreeing, and making both read live fixes it equally well. max_jobs_per_cycle and topology weren't involved in the drift at all and have no obstacle to live reload — BackfillScheduler carries no cross-cycle state. The CLI help still advertises "scheduler tunables" as reloadable when three of five aren't.
-
Config-seeded partitions silently override the WAL on restart. new_with_config_path seeds partitions from build_partitions() before Raft replay, and PartitionCreate is first-writer-wins (cluster.rs:4223-4227 logs "duplicate partition create in WAL apply, ignoring"). So if someone later codifies a runtime-created partition into spur.conf with different values and restarts, the WAL entry is discarded and the runtime state silently reverts. Two controllers with differing conf files replay the same log into different partition tables. This breaks the PR's own "WAL is authoritative" premise.
-
Hot-reloading admission.mode into Token mode takes down the cluster. heartbeat reads it live (server.rs:1056) and rejects tokenless agents with unauthenticated, but agents only get a token at registration — and spurd's should_reregister deliberately returns false for unauthenticated, with an explicit test asserting that (reporter.rs:538). So every running agent heartbeats into the same rejection forever, nodes get marked down, and all running jobs are evicted.
-
Snapshot restore conflates "old format" with "zero partitions." cluster.rs:4571 uses snap.partitions.is_empty() as the legacy-snapshot signal, but zero partitions is a reachable state (reconfigure deletes all of them when the conf has none). A follower receiving such a snapshot reseeds from its own local conf and diverges from the leader immediately after InstallSnapshot. Fix is Option<Vec> with #[serde(default)].
…onfigure Squashed for rebase onto main (PR #425).
…der QoS default before partition ACL Address review findings on the partition CRUD / reconfigure changes: - submit_job resolved the partition QoS ACL before applying the default QoS, so a user with no explicit --qos whose association/cluster default is in the partition's allow_qos was wrongly rejected with "a QoS is required". Resolve the default first, then validate. - PartitionCreate apply was first-writer-wins, so a config-seeded partition blocked a replayed runtime partition of the same name on restart: a runtime edit later codified into spur.conf with different values silently reverted, and two controllers with differing confs replayed the same log into divergent tables. Track config-seeded names and let a replayed WAL entry overwrite the seed (WAL authoritative); a genuine duplicate create stays first-writer-wins. - Snapshot restore used partitions.is_empty() as the legacy-snapshot signal, conflating a pre-partition snapshot with an authoritative empty set (reconfigure can delete them all). A follower then reseeded from its own config and diverged after InstallSnapshot. Make the field Option<Vec<Partition>>: None (field absent) falls back to the config baseline, Some(_) installs verbatim. - Docs/CLI: clarify that reconfigure applies controller-side hooks live but does not reach compute nodes (node-side prolog/epilog, device registry, memlock stay restart-only), and that the scheduler loop cadence is restart-only while only complete_wait_secs/resv_overrun_minutes reload live.
e8426fb to
19246af
Compare
|
Thanks for the thorough pass on the reconfigure changes — went through all seven. Summary of what changed and what's deferred: Fixed in this PR:
Deferred to a follow-up (tracked in [1]):
Not changed:
[1] #573 |
Summary
Adds runtime partition management to Spur — create/update/delete partitions and
scontrol reconfigure— with all state persisted through the Raft WAL so it survives restarts and replicates across HA peers. Brings partition ACL enforcement (accounts + QoS) in line with Slurm.scontrol:create-partition,update-partition,delete-partitionin both Spur's native flag syntax and Slurm-compatible key=value syntax (scontrol create PartitionName=foo Nodes=bar). key=value entity detection scans all params forPartitionName=/ReservationName=, so argument order doesn't matter (matching Slurm) and the ambiguous both-present case is rejected.PartitionCreate/PartitionUpdate/PartitionDelete) carry the full diff; mutations survive controller restart and replicate to peers. A persisted tombstone set stops conf-seeded partitions that were runtime-deleted from re-appearing on restart.scontrol reconfigure: re-readsspur.confon the leader and applies it live — partitions (create/update/delete, skipping deletes of partitions with active jobs),[[nodes]]policy/membership,[licenses],[burst_buffer], hooks/notifications, and scheduler tunables. Restart-only sections (listen ports, accounting DB, raft peers,jwt_key, scheduler cadence) are left untouched and called out explicitly in the CLI help, success message, and controller log. Followers converge on restart.spur.confwriteback:slurmctldnever rewritesslurm.confat runtime, so our writeback was a divergence.spur.confis now a read-only boot-time input (plusreconfigurere-read); the WAL is the authoritative runtime state.AllowAccounts/DenyAccountswere gated behindassociation_cache.is_loaded(), making them a silent no-op on clusters with no Postgres backend. The check is pure string matching against the requested account and needs no cache. Adds partitionAllowQos/DenyQos(newdeny_qosproto/config field) with a clear "a QoS is required" error when a partition setsAllowQosbut the job resolves to none.PreemptMode: resolved at schedule time — a QoS-level override wins, otherwise the most aggressive matched partition mode applies.Design choices
spur.confis read only at boot and onreconfigure. This removes a whole class of write-ordering / partial-write bugs and keeps HA peers consistent through the log.reconfigureapplies conf live rather than being partition-only. Making it a broad, predictable "conf wins" reconcile (with the restart-only exceptions stated loudly) is more useful than a partition-only reload, and safer than a partial live-swap of some sections but not others.jwt_keyand scheduler cadence (interval_secs) are pinned at startup and deliberately NOT live-swapped byreconfigure. Hot-swappingjwt_keywould instantly invalidate every outstanding node token; a unit test asserts a token minted with the startup key still verifies after ajwt_keyedit + reconfigure.reconfigureis also scoped to the leader.max_nodesclear semantics: both--clear-max-nodesand a literal--max-nodes 0(and inlineMaxNodes=0) mean "no limit", matching the proto's documented0 = clear limit. Neither can express a real zero-node cap. The controller collapses these into one derived flag so--max-nodes 0actually clears instead of no-opping.update_partitionthreads the proto request struct through the CLI rather than a long positional argument list, dropping the pairedset_*bools and thetoo_many_argumentsallow.config_pathis retained inClusterManagerbecausereconfigureneeds to know where to re-read conf;ClusterManager::new(no config path) is#[cfg(test)]-only since production always usesnew_with_config_path.Testing
cargo test -p spurctld -p spur-cli --lockedpasses;cargo clippy --workspace --exclude spur-ffi --all-targets --lockedclean;cargo fmt --all --checkclean.max_nodesclear intents, and live-reload of scheduler tunables / hooks / license pool /max_batch_requeueviareconfigure(plus thejwt_key-pinning security test).tests/native_host/e2e/test_partitions.py) covers the create/update/delete lifecycle, Slurm-compatible syntax, the conf read-only invariant, persistence across restart, andreconfiguresemantics.--max-nodes 0/--clear-max-nodes/ inlineMaxNodes=0clearing, combined max-nodes + ACL updates, ACL clear-to-empty, and the unknown-partition fail-closed path.