diff --git a/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md b/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md index 2bff2faa..c4da1877 100644 --- a/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md +++ b/.kiro/specs/scheduler-snapshot-job-allocation/tasks.md @@ -27,7 +27,7 @@ - _Boundary: snapshot save (allocation population)_ - _Note: not marked parallel with 2.2 — both tasks' tests land in the same test file (test_snapshot_save_load.c), a shared-file conflict even though the production code they touch is disjoint._ -- [ ] 2.2 Restore a job's resource allocation and accounting on the load side +- [x] 2.2 Restore a job's resource allocation and accounting on the load side - Write a failing test first (TDD red) asserting that loading a saved job that carries a resource allocation reconstructs both the job's allocation and its per-host/per-resource accounting. - Restore the allocation via the deep-copy primitive from 1.1 (never sharing the saved representation's own copy), then rebuild the job's accounting from the restored allocation using the existing accounting constructor. - Position this restoration immediately after the scheduling-representation construction from 1.2 within the same job-reconstruction step, before the job becomes visible to the rest of the scheduler. diff --git a/src/scheduler/src/snapshot_load.c b/src/scheduler/src/snapshot_load.c index 0b3efd2b..34a59fcd 100644 --- a/src/scheduler/src/snapshot_load.c +++ b/src/scheduler/src/snapshot_load.c @@ -8,6 +8,7 @@ #include "resobj.h" #include "sched/types.h" #include "utils.h" +#include "xcealloc.h" #include "xstring.h" /* @@ -461,6 +462,26 @@ int xce_sched_core_load_snapshot(xce_sched_core_t* core, } } + /* scheduler-snapshot-job-allocation task 2.2 (Req 1 AC 1-4, 3 + * AC 5): restore job->allocs via a genuine deep copy -- never + * aliasing wj->allocs, which belongs to the caller's `in` + * (const sched_snapshot_data_t*) and may be loaded into more + * than one core (research.md "Deep-copy vs. move semantics"). + * Independent of whether resreq parsed above -- an + * allocation has nothing to do with resreq parsing. Absent + * allocation data (old snapshot, or a job never dispatched) + * is the normal case: leave allocs/accounts NULL, no WARN. */ + if (wj->allocs != NULL) { + job->allocs = dup_job_alloc(wj->allocs); + if (job->allocs != NULL) { + job->accounts = make_job_account(job->allocs); + } else { + WARN("%s dup_job_alloc failed for jobid %llu; this " + "job's allocation will not be restored", + __func__, (unsigned long long)wj->jobid); + } + } + sched_add_job(core, job); if (picker != NULL) { diff --git a/src/scheduler/testsuite/test_snapshot_save_load.c b/src/scheduler/testsuite/test_snapshot_save_load.c index affffa97..75a695d7 100644 --- a/src/scheduler/testsuite/test_snapshot_save_load.c +++ b/src/scheduler/testsuite/test_snapshot_save_load.c @@ -1339,6 +1339,236 @@ test_snapshot_save_leaves_allocs_null_for_job_without_allocation(void) TEST_ASSERT_NULL(saved_job->allocs); } +/* ---------------------------------------------------------------------- */ +/* scheduler-snapshot-job-allocation task 2.2: xce_sched_core_load_snapshot() + * (load side) must restore job->allocs via a genuine deep copy + * (dup_job_alloc()) whenever the wire job carries allocation data (Req 1.1- + * 1.4), and rebuild job->accounts from that restored allocation via the + * existing make_job_account() constructor -- so worker/shared resource "used" + * accounting is correct immediately on load, with no further replay + * activity required (Req 1.2, 1.3). Drives the REAL, full cross-core round + * trip -- save core_a's real state, load into a completely independent + * core_b -- the same "genuinely cross-core, not a field-level shortcut" + * convention this file's header comment establishes for the top-level + * save/load equivalence test above, since Req 1.4's independence guarantee + * can only be genuinely proven across two separately-alive cores. + * ---------------------------------------------------------------------- */ + +static void +test_snapshot_load_restores_allocs_and_accounts_for_running_job_across_cores( + void) +{ + xce_sched_core_t core_a; + xce_sched_core_t core_b; + sched_snapshot_data_t snap; + xce_core_event_t events[2]; + xce_queue_t wire_queue; + xce_job_t* wire_job; + xce_sched_job_t* job_a; + xce_alloc_t* a0; + xce_alloc_t* a1; + xce_alloc_t* a2; + xce_host_alloc_t* h0; + xce_host_alloc_t* h1; + xce_host_alloc_t** hallocs; + xce_job_alloc_t* jalloc; + xce_job_t* saved_job; + xce_sched_job_t* restored; + int rc; + + memset(&g_stub_state_a, 0, sizeof(g_stub_state_a)); + memset(&g_stub_state_b, 0, sizeof(g_stub_state_b)); + + /* =================================================================== + * Phase 1: build core_a with a RUNNING job carrying a multi-host, + * multi-resource allocation -- hostA gets TWO resources (slots, mem), + * hostB gets ONE (slots), so both "multi-host" and "multi-resource" + * are genuinely exercised, not just one alloc per host. + * =================================================================== */ + fixture_init(&core_a, &g_stub_plugin_a); + + memset(&wire_queue, 0, sizeof(wire_queue)); + wire_queue.queue = "normal"; + wire_queue.priority = 1; + wire_queue.status = QUEUE_STAT_OPEN; + + wire_job = make_job(601, JOB_STAT_PEND, "normal", "alice", "proj1"); + + memset(events, 0, sizeof(events)); + events[0].type = XCE_CORE_EVENT_QUEUE_UPDATE; + events[0].u.queue_update.queue = &wire_queue; + events[1].type = XCE_CORE_EVENT_JOBS_PUSH; + events[1].u.jobs_push.njobs = 1; + events[1].u.jobs_push.jobs = &wire_job; + + rc = xce_core_push_events(&core_a, events, 2); + TEST_ASSERT_EQUAL_INT(0, rc); + + job_a = sched_get_job(&core_a, 601); + TEST_ASSERT_NOT_NULL(job_a); + + a0 = make_alloc((char*)"slots", NULL, 0, 2.0f, 0); + a1 = make_alloc((char*)"mem", NULL, 0, 100.0f, 0); + a2 = make_alloc((char*)"slots", NULL, 0, 3.0f, 0); + TEST_ASSERT_NOT_NULL(a0); + TEST_ASSERT_NOT_NULL(a1); + TEST_ASSERT_NOT_NULL(a2); + + h0 = make_host_alloc((char*)"hostA", 2, make_allocs(2, a0, a1)); + h1 = make_host_alloc((char*)"hostB", 1, make_allocs(1, a2)); + TEST_ASSERT_NOT_NULL(h0); + TEST_ASSERT_NOT_NULL(h1); + + hallocs = make_host_allocs(2, h0, h1); + TEST_ASSERT_NOT_NULL(hallocs); + + jalloc = make_job_alloc(601, 2, hallocs); + TEST_ASSERT_NOT_NULL(jalloc); + + job_a->state = JOB_STAT_RUN; + job_a->allocs = jalloc; + + /* =================================================================== + * Phase 2: save core_a's snapshot. + * =================================================================== */ + memset(&snap, 0, sizeof(snap)); + rc = xce_sched_core_save_snapshot(&core_a, &snap); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_NOT_NULL(snap.jobs); + TEST_ASSERT_EQUAL_INT(1, snap.jobs->count); + saved_job = snap.jobs->jobs[0]; + TEST_ASSERT_NOT_NULL(saved_job); + TEST_ASSERT_NOT_NULL(saved_job->allocs); + + /* =================================================================== + * Phase 3: load into core_b -- a completely separate, independently- + * alive instance. + * =================================================================== */ + fixture_init(&core_b, &g_stub_plugin_b); + + rc = xce_sched_core_load_snapshot(&core_b, &snap); + TEST_ASSERT_EQUAL_INT(0, rc); + + restored = sched_get_job(&core_b, 601); + TEST_ASSERT_NOT_NULL(restored); + TEST_ASSERT_EQUAL_UINT32(JOB_STAT_RUN, restored->state); + + /* =================================================================== + * Phase 4: job->allocs content matches the original, host-for-host, + * resource-for-resource. + * =================================================================== */ + TEST_ASSERT_NOT_NULL(restored->allocs); + TEST_ASSERT_EQUAL_UINT64(601, restored->allocs->jobid); + TEST_ASSERT_EQUAL_INT(2, restored->allocs->nhosts); + TEST_ASSERT_NOT_NULL(restored->allocs->allocs[0]); + TEST_ASSERT_NOT_NULL(restored->allocs->allocs[1]); + TEST_ASSERT_EQUAL_STRING("hostA", restored->allocs->allocs[0]->host); + TEST_ASSERT_EQUAL_STRING("hostB", restored->allocs->allocs[1]->host); + TEST_ASSERT_EQUAL_INT(2, restored->allocs->allocs[0]->nallocs); + TEST_ASSERT_EQUAL_INT(1, restored->allocs->allocs[1]->nallocs); + TEST_ASSERT_EQUAL_STRING("slots", + restored->allocs->allocs[0]->allocs[0]->resname); + TEST_ASSERT_EQUAL_FLOAT( + 2.0f, (float)restored->allocs->allocs[0]->allocs[0]->value); + TEST_ASSERT_EQUAL_STRING("mem", + restored->allocs->allocs[0]->allocs[1]->resname); + TEST_ASSERT_EQUAL_FLOAT( + 100.0f, (float)restored->allocs->allocs[0]->allocs[1]->value); + TEST_ASSERT_EQUAL_STRING("slots", + restored->allocs->allocs[1]->allocs[0]->resname); + TEST_ASSERT_EQUAL_FLOAT( + 3.0f, (float)restored->allocs->allocs[1]->allocs[0]->value); + + /* =================================================================== + * Phase 5: job->accounts correctly reconstructed from the restored + * allocation via make_job_account() -- nhosts matches, and per-resource + * allocated/reserved values reflect the allocation (mirrors + * make_res_account()'s own reserved=allocated=alloc->value, used=0.0 + * convention -- see xcealloc.c). + * =================================================================== */ + TEST_ASSERT_NOT_NULL(restored->accounts); + TEST_ASSERT_EQUAL_INT(2, restored->accounts->nhosts); + TEST_ASSERT_NOT_NULL(restored->accounts->accounts[0]); + TEST_ASSERT_NOT_NULL(restored->accounts->accounts[1]); + TEST_ASSERT_EQUAL_INT(2, restored->accounts->accounts[0]->naccount); + TEST_ASSERT_EQUAL_INT(1, restored->accounts->accounts[1]->naccount); + TEST_ASSERT_EQUAL_FLOAT( + 2.0f, (float)restored->accounts->accounts[0]->accounts[0]->allocated); + TEST_ASSERT_EQUAL_FLOAT( + 2.0f, (float)restored->accounts->accounts[0]->accounts[0]->reserved); + TEST_ASSERT_EQUAL_FLOAT( + 0.0f, (float)restored->accounts->accounts[0]->accounts[0]->used); + TEST_ASSERT_EQUAL_FLOAT( + 100.0f, (float)restored->accounts->accounts[0]->accounts[1]->allocated); + TEST_ASSERT_EQUAL_FLOAT( + 3.0f, (float)restored->accounts->accounts[1]->accounts[0]->allocated); + + /* =================================================================== + * Phase 6: Req 1.4's independence guarantee, proven end-to-end across + * the full save->load round trip -- THREE genuinely distinct pointers: + * core_b's restored copy, core_a's own live copy, and the wire job's + * own (already-independent, task 2.1) copy. Freeing/mutating any one + * must never be able to corrupt either of the others. + * =================================================================== */ + TEST_ASSERT_TRUE(restored->allocs != job_a->allocs); + TEST_ASSERT_TRUE(restored->allocs != saved_job->allocs); + TEST_ASSERT_TRUE(job_a->allocs != saved_job->allocs); +} + +/* Companion test (Req 3.5's "absent allocation is the normal case, not an + * error" -- load-side half): a wire job with no allocation data at all + * (old snapshot file, or a job that was pending/never dispatched at save + * time) must load cleanly with job->allocs/job->accounts left at their + * calloc()-zeroed NULL default -- no crash, no WARN. */ +static void +test_snapshot_load_leaves_allocs_and_accounts_null_for_job_without_allocation( + void) +{ + xce_sched_core_t core; + sched_snapshot_data_t in; + xce_jobs_data_t jobs_data; + xce_queue_t wire_queue; + xce_queue_t* queues[1]; + xce_job_t* wire_job; + xce_job_t* jobs[1]; + xce_sched_job_t* restored; + int rc; + + memset(&g_stub_state_a, 0, sizeof(g_stub_state_a)); + fixture_init(&core, &g_stub_plugin_a); + + memset(&wire_queue, 0, sizeof(wire_queue)); + wire_queue.queue = "normal"; + wire_queue.priority = 1; + wire_queue.status = QUEUE_STAT_OPEN; + queues[0] = &wire_queue; + + /* make_job() never sets ->allocs -- calloc()-zeroed NULL default, + * exactly the "old snapshot file" / "never dispatched" case Req 3.5 + * covers. */ + wire_job = make_job(602, JOB_STAT_PEND, "normal", "alice", "proj1"); + jobs[0] = wire_job; + + memset(&jobs_data, 0, sizeof(jobs_data)); + jobs_data.count = 1; + jobs_data.jobs = jobs; + + memset(&in, 0, sizeof(in)); + in.jobs = &jobs_data; + in.queues = queues; + in.n_queues = 1; + + rc = xce_sched_core_load_snapshot(&core, &in); + TEST_ASSERT_EQUAL_INT(0, rc); + + restored = sched_get_job(&core, 602); + TEST_ASSERT_NOT_NULL(restored); + + TEST_ASSERT_NULL(restored->allocs); + TEST_ASSERT_NULL(restored->accounts); +} + int main(void) { UNITY_BEGIN(); @@ -1355,5 +1585,9 @@ int main(void) RUN_TEST( test_snapshot_save_populates_allocs_for_running_job_with_multi_host_allocation); RUN_TEST(test_snapshot_save_leaves_allocs_null_for_job_without_allocation); + RUN_TEST( + test_snapshot_load_restores_allocs_and_accounts_for_running_job_across_cores); + RUN_TEST( + test_snapshot_load_leaves_allocs_and_accounts_null_for_job_without_allocation); return UNITY_END(); }