Skip to content

Commit 4ed288a

Browse files
committed
feat(spurd): survive agent restart without interrupting running jobs
spurd's SIGTERM handler unconditionally deregistered the node, force-evicting every running job — so any graceful restart (e.g. a binary upgrade) failed in-flight jobs even though the job process could survive it. This makes a graceful restart transparent to running jobs, with the controller's heartbeat timeout as the backstop if the restart takes too long. - SIGTERM deregisters only when the agent has no active jobs (none running and none mid-launch); a reservation-only node still deregisters so the controller reclaims it. - Per-job manifest split into identity (pid, start time, cgroup, exit sentinel) and an obligation ledger (epilog, completion report), each cleared as it is discharged. Reconcile drives the ledger to empty, persisting progress so a lost completion report retried on the next restart resends only the report, never re-running a non-idempotent epilog; the resolved exit is recorded so it survives the sentinel/rootfs being cleaned up. - Completion detection without waitpid: tri-state cgroup liveness with a /proc fallback when cgroup.events is unreadable (non-v2 host, or a process never moved into the cgroup), so an adopted job is not misreported as complete. - Resource restore is all-or-nothing, so a restarted agent refuses to adopt a job whose manifest names a GPU another adopted job already holds. - Exit sentinel opened O_NONBLOCK|O_NOFOLLOW with a regular-file/nlink check and a bounded read, so a job-planted FIFO or oversized file can't block or bloat startup; the wrapper uses a top-level trap on EXIT (not a subshell) so a later syntax error still runs the lines above it. - cgroups and spool dirs are keyed by (job_id, run_attempt); the spool dir stays root-owned with a sticky bit so a co-located user can't replace the manifest. - spurctld warns at startup if heartbeat_timeout_secs is below 30s.
1 parent 4fb4a38 commit 4ed288a

7 files changed

Lines changed: 2417 additions & 135 deletions

File tree

crates/spur-sched/src/cons_tres.rs

Lines changed: 173 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,11 @@ impl NodeAllocation {
6666
}
6767
}
6868

69+
/// Whether any job is mid-launch (reserved, not yet committed to `running`).
70+
pub fn has_launching(&self) -> bool {
71+
!self.launching.is_empty()
72+
}
73+
6974
/// Available (unallocated) CPU count.
7075
pub fn free_cpus(&self) -> u32 {
7176
self.allocated_cpus.iter().filter(|&&a| !a).count() as u32
@@ -184,7 +189,7 @@ impl NodeAllocation {
184189
}
185190
}
186191

187-
self.allocated_memory_mb += memory_mb;
192+
self.allocated_memory_mb = self.allocated_memory_mb.saturating_add(memory_mb);
188193
for &idx in &gpu_indices {
189194
self.gpu_allocated[idx] = true;
190195
}
@@ -208,6 +213,49 @@ impl NodeAllocation {
208213
self.owners.contains_key(&job_id)
209214
}
210215

216+
/// Re-adopt an already-committed allocation (e.g. after an agent restart),
217+
/// pinning the exact ids rather than re-picking via first-fit. Idempotent:
218+
/// a second call for the same job_id is a no-op, so a duplicate manifest
219+
/// can't double-count memory (the cpu/gpu bitmaps are naturally
220+
/// idempotent, but the memory counter isn't).
221+
///
222+
/// GPUs are validated all-or-nothing before any mutation: a manifest naming
223+
/// a device already held by another job is rejected with `GpusUnavailable`
224+
/// so the caller can refuse the adoption rather than double-book the device
225+
/// (releasing one owner would then free a GPU the other still holds).
226+
pub fn restore_committed(
227+
&mut self,
228+
job_id: u32,
229+
alloc: AllocationResult,
230+
) -> Result<(), AllocError> {
231+
if self.owners.contains_key(&job_id) {
232+
return Ok(());
233+
}
234+
let mut gpu_indices = Vec::with_capacity(alloc.gpu_ids.len());
235+
for &device_id in &alloc.gpu_ids {
236+
let idx = self
237+
.gpus
238+
.iter()
239+
.position(|g| g.device_id == device_id)
240+
.ok_or(AllocError::GpusUnavailable)?;
241+
if self.gpu_allocated[idx] || gpu_indices.contains(&idx) {
242+
return Err(AllocError::GpusUnavailable);
243+
}
244+
gpu_indices.push(idx);
245+
}
246+
for &cpu in &alloc.cpu_ids {
247+
if let Some(a) = self.allocated_cpus.get_mut(cpu as usize) {
248+
*a = true;
249+
}
250+
}
251+
self.allocated_memory_mb = self.allocated_memory_mb.saturating_add(alloc.memory_mb);
252+
for idx in gpu_indices {
253+
self.gpu_allocated[idx] = true;
254+
}
255+
self.owners.insert(job_id, alloc);
256+
Ok(())
257+
}
258+
211259
/// Release a job's allocation by id. Idempotent: releasing an unknown or
212260
/// already-released job is a no-op returning false.
213261
pub fn release_job(&mut self, job_id: u32) -> bool {
@@ -556,6 +604,130 @@ mod tests {
556604
);
557605
}
558606

607+
#[test]
608+
fn test_restore_committed_pins_exact_ids_not_first_fit() {
609+
let mut node = make_node_with_ids(4, 8_000, vec![0, 1], "mi300x");
610+
// Simulate a spurd restart: nothing allocated yet in this fresh
611+
// NodeAllocation, but a manifested job actually holds cpu 3 and gpu 1
612+
// (not the first-fit picks allocate_for_job would make).
613+
let alloc = AllocationResult {
614+
cpu_ids: vec![3],
615+
gpu_ids: vec![1],
616+
memory_mb: 2_000,
617+
};
618+
node.restore_committed(42, alloc.clone()).unwrap();
619+
620+
assert_eq!(node.free_cpus(), 3);
621+
assert_eq!(node.free_gpus(None), 1);
622+
assert_eq!(node.free_memory_mb(), 6_000);
623+
assert_eq!(node.allocated_gpu_ids(), vec![1]);
624+
625+
// A subsequent allocation must not double-book cpu 3 / gpu 1.
626+
let next = node.allocate_for_job(43, 1, 0, &[0]).unwrap();
627+
assert_eq!(next.cpu_ids, vec![0]);
628+
assert!(!next.cpu_ids.contains(&3));
629+
630+
// Committed (not launching), so reconcile treats it like any other
631+
// live job rather than sparing it as mid-launch.
632+
let live: HashSet<u32> = [42, 43].into_iter().collect();
633+
assert!(node
634+
.reconcile(&live, Instant::now(), Duration::from_secs(120))
635+
.is_empty());
636+
assert!(node.release_job(42));
637+
assert_eq!(node.free_memory_mb(), 8_000);
638+
}
639+
640+
#[test]
641+
fn test_has_launching_tracks_in_flight_launch() {
642+
let mut node = make_node_with_ids(4, 8_000, vec![0], "mi300x");
643+
assert!(!node.has_launching());
644+
node.allocate_for_job(1, 1, 0, &[0]).unwrap();
645+
assert!(
646+
node.has_launching(),
647+
"reserved-but-not-committed job is launching"
648+
);
649+
node.commit_job(1);
650+
assert!(
651+
!node.has_launching(),
652+
"committed job is no longer launching"
653+
);
654+
}
655+
656+
#[test]
657+
fn test_restore_committed_memory_saturates_on_overflow() {
658+
// A corrupt/tampered manifest with a huge memory_mb must not wrap the
659+
// accounting counter (which would silently corrupt free-memory math).
660+
let mut node = make_node_with_ids(4, 8_000, vec![0], "mi300x");
661+
node.restore_committed(
662+
1,
663+
AllocationResult {
664+
cpu_ids: vec![],
665+
gpu_ids: vec![],
666+
memory_mb: u64::MAX,
667+
},
668+
)
669+
.unwrap();
670+
node.restore_committed(
671+
2,
672+
AllocationResult {
673+
cpu_ids: vec![],
674+
gpu_ids: vec![],
675+
memory_mb: u64::MAX,
676+
},
677+
)
678+
.unwrap();
679+
// Saturated rather than wrapped; free memory floors at 0.
680+
assert_eq!(node.free_memory_mb(), 0);
681+
}
682+
683+
#[test]
684+
fn test_restore_committed_rejects_gpu_double_book() {
685+
// Two manifests naming the same device: the second adoption must be
686+
// rejected, not silently double-book the GPU — otherwise releasing the
687+
// first frees a device the second still holds.
688+
let mut node = make_node_with_ids(4, 8_000, vec![0, 1], "mi300x");
689+
node.restore_committed(
690+
1,
691+
AllocationResult {
692+
cpu_ids: vec![0],
693+
gpu_ids: vec![0],
694+
memory_mb: 1_000,
695+
},
696+
)
697+
.unwrap();
698+
let conflict = node.restore_committed(
699+
2,
700+
AllocationResult {
701+
cpu_ids: vec![1],
702+
gpu_ids: vec![0],
703+
memory_mb: 1_000,
704+
},
705+
);
706+
assert_eq!(conflict, Err(AllocError::GpusUnavailable));
707+
// The rejected adoption left nothing behind: gpu 0 still belongs only to
708+
// job 1, and its cpu/memory were not partially applied.
709+
assert_eq!(node.allocated_gpu_ids(), vec![0]);
710+
assert_eq!(node.free_memory_mb(), 7_000);
711+
assert!(!node.release_job(2));
712+
}
713+
714+
#[test]
715+
fn test_restore_committed_rejects_unknown_gpu() {
716+
// A manifest naming a device this node doesn't have is rejected rather
717+
// than silently dropped, so the caller can refuse the adoption.
718+
let mut node = make_node_with_ids(4, 8_000, vec![0], "mi300x");
719+
let bad = node.restore_committed(
720+
1,
721+
AllocationResult {
722+
cpu_ids: vec![0],
723+
gpu_ids: vec![99],
724+
memory_mb: 1_000,
725+
},
726+
);
727+
assert_eq!(bad, Err(AllocError::GpusUnavailable));
728+
assert_eq!(node.free_memory_mb(), 8_000);
729+
}
730+
559731
#[test]
560732
fn test_memory_released_symmetrically_with_zero_cpus() {
561733
// A job with 0 cpus must still have its memory reserved and released

crates/spurctld/src/main.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ use std::path::PathBuf;
2222
use std::sync::Arc;
2323

2424
use clap::Parser;
25-
use tracing::info;
25+
use tracing::{info, warn};
2626

2727
use cluster::ClusterManager;
2828
use rpc_stats::RpcStatsCollector;
@@ -250,6 +250,14 @@ async fn main() -> anyhow::Result<()> {
250250

251251
// Start node health checker (only on leader).
252252
let hb_timeout = config.controller.heartbeat_timeout_secs.unwrap_or(90);
253+
if hb_timeout < 30 {
254+
warn!(
255+
hb_timeout,
256+
"heartbeat_timeout_secs is below 30s; a graceful spurd restart slower than this \
257+
will fail its running jobs — this is now the only safety net for a restart, \
258+
not just crash detection"
259+
);
260+
}
253261
let health_cluster = cluster.clone();
254262
let health_raft = raft_handle.clone();
255263
tokio::spawn(async move {

0 commit comments

Comments
 (0)