diff --git a/src/scheduler/src/sched_election.c b/src/scheduler/src/sched_election.c index 98d40767..0a426168 100644 --- a/src/scheduler/src/sched_election.c +++ b/src/scheduler/src/sched_election.c @@ -230,6 +230,16 @@ void* sched_allocator(void* arg) continue; } + /* bug-1897: election and allocation both work on job->schedobj; a + * job without one (its resreq failed to parse) cannot be scheduled. + * Skip it like a job that cannot be found. */ + if (job->schedobj == NULL) { + fmt_jobid(jobidString, sizeof(jobidString), jobid); + ERROR("%s job %s has no scheduling object, skipped", __func__, + jobidString); + continue; + } + /* scheduler-sim-live-allocation spec, task 1.2: plugin_job_election() * is core-typed (mirrors plugin_begin_session(core) in this same * codebase's driver files) -- pass core directly. */ diff --git a/src/scheduler/src/sim_picker.c b/src/scheduler/src/sim_picker.c index a02fc36b..3c68abbd 100644 --- a/src/scheduler/src/sim_picker.c +++ b/src/scheduler/src/sim_picker.c @@ -114,6 +114,11 @@ void sim_bootstrap_picker(xce_sched_sim_driver_t* sim) if ((rc < 0) || (rc == RTTI_ITER_NO_DATA)) { break; } + /* bug-1897: same skip as the load itself -- a job + * without a schedobj is never a picker candidate. */ + if (job->schedobj == NULL) { + continue; + } plugin->picker_iface->on_job_push(plugin, job); } iter_destroy(&it); diff --git a/src/scheduler/src/snapshot_load.c b/src/scheduler/src/snapshot_load.c index 7f89dd8c..287e10df 100644 --- a/src/scheduler/src/snapshot_load.c +++ b/src/scheduler/src/snapshot_load.c @@ -366,10 +366,9 @@ int xce_sched_core_load_snapshot(xce_sched_core_t* core, /* scheduler-snapshot-job-allocation task 1.2 (Req 2 AC 1-4): * construct job->schedobj for every restored job whose resreq * parsed successfully -- guarded on job->resreq != NULL so the - * pre-existing, unrelated resreq-parse-failure path above - * (which already leaves job->resreq NULL) is tolerated, not - * fixed, here (design.md "Load-Side Job Reconstruction", Known - * limitation). */ + * resreq-parse-failure path above (which leaves job->resreq + * NULL) is tolerated here; such a job is kept out of the + * picker below (bug-1897). */ if (job->resreq != NULL) { job->schedobj = (xce_sched_obj_t*)calloc(1, sizeof(*(job->schedobj))); @@ -403,7 +402,17 @@ int xce_sched_core_load_snapshot(xce_sched_core_t* core, sched_add_job(core, job); - if (picker != NULL) { + /* bug-1897: a job without a schedobj (resreq parse failure + * above, or its calloc failing) stays in the core so its + * restored allocation is still charged and later events still + * find it, but must never become a picker candidate -- + * sched_allocator() runs plugin_job_election() and + * sched_make_allocation() on job->schedobj. */ + if (job->schedobj == NULL) { + ERROR("%s jobid %llu has no scheduling object; it will " + "not be scheduled", + __func__, (unsigned long long)wj->jobid); + } else if (picker != NULL) { picker->picker_iface->on_job_push(picker, job); } } diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index c8c0c51d..1247007b 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -2216,6 +2216,27 @@ test('scheduler election sim fixture test', sched_election_sim_fixture_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +# bug-1897: sched_allocator() must skip a picked job whose schedobj is NULL +# (resreq failed to parse on snapshot load) instead of dereferencing it. +# Same real-scheduler_srcs link technique as the fixture test above. +sched_election_null_schedobj_test_srcs = ['test_sched_election_null_schedobj.c'] +foreach f : scheduler_srcs + if f != 'src/main.c' + sched_election_null_schedobj_test_srcs += ['../' + f] + endif +endforeach + +sched_election_null_schedobj_test = executable( + 'sched_election_null_schedobj_test', + sched_election_null_schedobj_test_srcs, + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + +test('scheduler election null schedobj test', + sched_election_null_schedobj_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) + # scheduler-sim-live-allocation spec, task 4.1 (Req 1.1, 1.2, 3.3, 4.2, 4.3, # 5.1, boundary: Unified Picker/Allocator Threads, Sim Driver): proves # sched_sim_driver_run() (sim_loop.c) itself -- not just the relocated diff --git a/src/scheduler/testsuite/test_sched_election_null_schedobj.c b/src/scheduler/testsuite/test_sched_election_null_schedobj.c new file mode 100644 index 00000000..a77181c1 --- /dev/null +++ b/src/scheduler/testsuite/test_sched_election_null_schedobj.c @@ -0,0 +1,115 @@ +/* + * test_sched_election_null_schedobj.c - bug-1897. + * + * sched_allocator() (sched_election.c) runs plugin_job_election() and + * sched_make_allocation() on job->schedobj. A job whose resreq could not be + * parsed has no schedobj (snapshot_load.c keeps such a job so a running + * job's allocation stays charged), and before the fix the allocator thread + * dereferenced the NULL schedobj (limit_job_election(), limit.c:336, in a + * replay; sched_make_allocation()'s obj->tag here, with no limit plugin). + * + * The allocator must skip such a job -- no decision for it -- and carry on + * with the session: the only decisions-cbuf entry is the end-of-session + * NULL marker. + * + * Fixture as in test_sched_election_sim_fixture.c (`wrapper` is a real + * xce_scheduler_t used only for plugin registration; the threads get the + * bare core). One RUN_TEST() only: the threads are never joined and + * reference stack-local state. + */ +#include +#include +#include +#include +#include + +#include + +#include "decision.h" +#include "plugin.h" +#include "sched/types.h" +#include "scheduler.h" +#include "xstring.h" + +#define TEST_JOBID ((uint64_t)189701ULL) +#define QUEUE_NAME "normal" + +void setUp(void) { return; } +void tearDown(void) { return; } + +static void test_sched_allocator_skips_job_without_schedobj(void) +{ + xce_scheduler_t wrapper; + xce_sched_core_t* core; + xce_sched_plugin_t* fcfs; + xce_sched_queue_t* queue; + xce_sched_job_t* job; + pthread_t picker_tid; + pthread_t allocator_tid; + xce_decision_t* decision = (xce_decision_t*)0x1; + int rc; + + memset(&wrapper, 0, sizeof(wrapper)); + rc = xce_sched_core_init(&(wrapper.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + core = &(wrapper.core); + + fcfs = builtin_fcfs_new_plugin(); + TEST_ASSERT_NOT_NULL(fcfs); + rc = plugin_register(&wrapper, fcfs); + TEST_ASSERT_TRUE(rc >= 0); + core->job_pick_plugin = (char*)"builtin_fcfs"; + + rc = register_seqjob_allocator(&wrapper); + TEST_ASSERT_TRUE(rc >= 0); + rc = register_parjob_allocator(&wrapper); + TEST_ASSERT_TRUE(rc >= 0); + + queue = (xce_sched_queue_t*)calloc(1, sizeof(*queue)); + TEST_ASSERT_NOT_NULL(queue); + queue->name = makeString(QUEUE_NAME); + rc = sched_add_queue(core, queue); + TEST_ASSERT_TRUE(rc >= 0); + fcfs->picker_iface->on_queue_update(fcfs, queue); + + /* The shape snapshot_load.c leaves for a job whose resreq failed to + * parse: resreq and schedobj both NULL. Pushed to the picker directly, + * so this test covers the allocator guard on its own, independent of + * the loader no longer pushing such a job. */ + job = (xce_sched_job_t*)calloc(1, sizeof(*job)); + TEST_ASSERT_NOT_NULL(job); + job->jobid = TEST_JOBID; + job->state = JOB_STAT_PEND; + job->queue = queue; + job->user = sched_get_user(core, "alice"); + + rc = sched_add_job(core, job); + TEST_ASSERT_TRUE(rc >= 0); + fcfs->picker_iface->on_job_push(fcfs, job); + + rc = pthread_create(&picker_tid, NULL, sched_job_picker, core); + TEST_ASSERT_EQUAL_INT(0, rc); + rc = pthread_create(&allocator_tid, NULL, sched_allocator, core); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = xce_core_go(core); + TEST_ASSERT_EQUAL_INT(0, rc); + + cbuf_get(&(core->decisions), &decision); + TEST_ASSERT_NULL_MESSAGE(decision, + "a job without a schedobj must be skipped: the " + "only entry is the end-of-session NULL marker"); + + rc = xce_core_end_session(core); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_EQUAL_UINT(JOB_STAT_PEND, job->state); + TEST_ASSERT_NULL(job->allocs); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_sched_allocator_skips_job_without_schedobj); + return UNITY_END(); +} diff --git a/src/scheduler/testsuite/test_sim_event_loop.c b/src/scheduler/testsuite/test_sim_event_loop.c index 43b42a2b..05f53097 100644 --- a/src/scheduler/testsuite/test_sim_event_loop.c +++ b/src/scheduler/testsuite/test_sim_event_loop.c @@ -899,11 +899,13 @@ test_run_deactivates_tlog_virtual_clock_logging_on_completion(void) * already white-box #included above) BEFORE calling sched_record_start() -- * so xce_sched_core_save_snapshot() (snapshot.c) captures both into the * INITIAL snapshot itself, not as a later ReplayEvent. */ -static FILE* write_header_and_snapshot_with_pending_job(char* out_path, - size_t out_path_len, - const char* suffix, - const char* queue_name, - uint64_t jobid) +static FILE* +write_header_and_snapshot_with_pending_job_resreq(char* out_path, + size_t out_path_len, + const char* suffix, + const char* queue_name, + uint64_t jobid, + const char* saved_resreq) { char record_dir[PATH_MAX]; xce_scheduler_t sched; @@ -945,6 +947,13 @@ static FILE* write_header_and_snapshot_with_pending_job(char* out_path, TEST_ASSERT_NOT_NULL(job); TEST_ASSERT_GREATER_OR_EQUAL(0, sched_add_job(&(sched.core), job)); + /* bug-1897: the snapshot saves resreq_str verbatim, so overwriting it + * here yields a snapshot whose resreq the loader cannot parse. */ + if (saved_resreq != NULL) { + FREEUP(job->resreq_str); + job->resreq_str = strdup(saved_resreq); + } + rc = sched_record_start(&sched, "fixture.replay"); TEST_ASSERT_EQUAL_INT(0, rc); TEST_ASSERT_TRUE(strlen(sched.record_file_path) < out_path_len); @@ -953,6 +962,16 @@ static FILE* write_header_and_snapshot_with_pending_job(char* out_path, return sched.record_file; } +static FILE* write_header_and_snapshot_with_pending_job(char* out_path, + size_t out_path_len, + const char* suffix, + const char* queue_name, + uint64_t jobid) +{ + return write_header_and_snapshot_with_pending_job_resreq( + out_path, out_path_len, suffix, queue_name, jobid, NULL); +} + /* Same dup()/dup2() STDOUT_FILENO capture technique test_sim_verify.c's own * run_sim_and_capture_stdout() establishes (this file's white-box TU does * not #include that file, so a plain copy is needed here rather than a @@ -1065,6 +1084,44 @@ test_job_present_only_in_initial_snapshot_still_reaches_the_picker(void) "picker: PASS\n"); } +/* + * bug-1897: a replay whose INITIAL snapshot holds a pending job with a + * resreq the loader cannot parse (e.g. bug-1888's old debug-dump text). + * The loader keeps the job without a schedobj; sim_bootstrap_picker()'s + * picker rebuild used to push it anyway, and the allocator thread then + * dereferenced the NULL schedobj (SEGV in limit_job_election()). The run + * must complete, with no decision for that job. + */ +static void test_snapshot_job_with_unparsable_resreq_is_not_scheduled(void) +{ + char path[PATH_MAX]; + char marker[80]; + char jobid_buf[64]; + FILE* f; + int rc; + char* captured; + + f = write_header_and_snapshot_with_pending_job_resreq( + path, sizeof(path), "_badresreq", "normal", 4343ULL, "{"); + TEST_ASSERT_NOT_NULL(f); + + append_replay_session(f, 1000ULL); + append_end_of_stream_marker(f); + fclose(f); + + captured = run_sim_and_capture_stdout(path, &rc); + + fmt_jobid(jobid_buf, sizeof(jobid_buf), 4343ULL); + snprintf(marker, sizeof(marker), "jobid=%s", jobid_buf); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NULL_MESSAGE(strstr(captured, marker), + "a job whose snapshot resreq cannot be parsed " + "must get no decision (bug-1897)"); + + free(captured); +} + int main(void) { UNITY_BEGIN(); @@ -1076,6 +1133,7 @@ int main(void) RUN_TEST(test_run_deactivates_tlog_virtual_clock_logging_on_completion); RUN_TEST( test_job_present_only_in_initial_snapshot_still_reaches_the_picker); + RUN_TEST(test_snapshot_job_with_unparsable_resreq_is_not_scheduled); return UNITY_END(); } diff --git a/src/scheduler/testsuite/test_snapshot_save_load.c b/src/scheduler/testsuite/test_snapshot_save_load.c index 619d30c4..f6bdee25 100644 --- a/src/scheduler/testsuite/test_snapshot_save_load.c +++ b/src/scheduler/testsuite/test_snapshot_save_load.c @@ -1313,11 +1313,11 @@ test_snapshot_load_computes_resreq_fingerprint_for_restored_primary(void) 0, restored->schedobj->u.primary->resreq_fp.select_node_id); } -/* Regression-safety test (NOT a new feature): a job whose resreq fails to - * parse on load is a pre-existing, out-of-boundary gap (design.md "Known - * limitation") that task 1.2 must NOT try to fix -- it must only make sure - * schedobj construction is skipped rather than crashing on a NULL resreq. - * Proves job->schedobj stays NULL, identical to today's behavior. */ +/* Regression-safety test: a job whose resreq fails to parse on load keeps + * job->resreq and job->schedobj NULL (schedobj construction is skipped + * rather than crashing on a NULL resreq, task 1.2), stays in the core, and + * -- bug-1897 -- is never pushed to the picker, so the allocator thread + * never elects a job with no schedobj. */ static void test_snapshot_load_leaves_schedobj_null_when_resreq_fails_to_parse(void) { @@ -1364,6 +1364,12 @@ test_snapshot_load_leaves_schedobj_null_when_resreq_fails_to_parse(void) /* THIS feature's guard: schedobj construction is skipped, not crashed * into, when resreq is NULL. */ TEST_ASSERT_NULL(restored->schedobj); + /* bug-1897: a job with no schedobj must never become a picker + * candidate -- the allocator thread runs plugin_job_election() on + * job->schedobj (limit_job_election() dereferenced it as NULL). */ + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, g_stub_state_a.ncandidates, + "a job whose resreq failed to parse must not be pushed to the picker"); } /* ---------------------------------------------------------------------- */