From a6d96705d9ece5a442b6d8e957466f82a7872705 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 8 Sep 2026 11:14:55 +0000 Subject: [PATCH] refactor(scheduler): read slots' job total from alloc_req instead of name-checking The previous fix for the "slots" regression (5b18f14a) worked but was an unneeded special case: strcmp(vres->res->name, "slots") to fall back to nslots directly. alloc_req->host already holds exactly that value during the global phase -- both allocators' own unconditional "add slots" step stages it there regardless of phase, and plugin_prepare_allocation() never resets .host during the global phase, so by the time sched_fill_ limit_requests() runs (right after, same invocation), .host contains exactly "slots"'s job-wide total and nothing else. Read alloc_req->host FIRST for this combination and fall back to the rusages-based on-demand computation only when nothing is staged there (get_resval_by_id()'s "not found" == 0.0, safe here since "slots" is never legitimately 0 -- seqjob only runs for maxTasks==1, parjob only for maxTasks>1). Data-driven instead of name-based, matching bug-1789's own earlier flag-based-dispatch philosophy, and generalizes for free to any future resource an allocator plugin similarly pre-stages outside its normal rusages loop. test_fill_limit_requests_global_limit_on_slots_reads_host now pre-stages alloc_req->host exactly as the allocators do, rusages left empty, matching production exactly. Full xce-build.sh all asan --incremental: 94/94 Ok. Live re-verified against both the original gitea#46 scenario and the ug1_global_mem scenario: byte-identical scheduler traces to every prior verification round. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GLfB5uWRdmx2WiZSFC7Fm7 --- src/scheduler/src/resource.c | 63 ++++++++++--------- .../testsuite/test_resource_accounting.c | 46 +++++++++----- 2 files changed, 61 insertions(+), 48 deletions(-) diff --git a/src/scheduler/src/resource.c b/src/scheduler/src/resource.c index 665e48a7..cbb60525 100644 --- a/src/scheduler/src/resource.c +++ b/src/scheduler/src/resource.c @@ -936,14 +936,34 @@ static float get_resval_by_id(xvec_idval_t* vec, int idx) * read: alloc_req->host only ever holds per-host/per-bucket-specific data * for the REAL per-host capacity check (a different question, with a * different answer, computed fresh per candidate host), never a job-WIDE - * total. For that one combination, `rusages`/`nhosts`/`nslots` (the job's - * own declared resource requests, and its whole-job totals -- not any one - * host's share) are used to compute the aggregate directly, on demand, via - * calc_wanted_job_total_resval() (resreq.c) -- deliberately kept as this - * function's own responsibility rather than the allocator plugins': what a - * LIMIT needs to see is this function's concern, not seqjob/parjob's, which - * should only ever compute what the job itself wants for capacity-satisfy - * purposes. + * total -- EXCEPT for "slots", which seqjob_prepare_allocation()/ + * parjob_prepare_allocation() both unconditionally stage into alloc_req + * ->host regardless of phase (their own "add slots" step, outside the + * normal rusages-driven computation) -- so for "slots" specifically, + * alloc_req->host during the global phase already holds exactly the + * job-wide total this combination needs, the same way it would for the + * general per-host-phase case. Reading alloc_req->host FIRST therefore + * naturally gets "slots" right with no name-based special case at all + * (bug-1790 refactor regression, live-confirmed against gitea#46's + * original ug1_slots/ug1_normal_slots/limit1 scenario: an earlier version + * of this fix instead special-cased strcmp(vres->res->name, "slots") to + * fall back to `nslots` directly -- functionally equivalent, since + * alloc_req->host already holds exactly that value for "slots", but a + * needless string-name special case where reading the data that's already + * there does the same job). Any OTHER per-host resource genuinely has + * nothing staged in alloc_req->host during the global phase (calc_wanted_ + * host_resvals(), the thing that WOULD stage it, only ever runs in the + * per-host branch), so the read comes back 0 (get_resval_by_id()'s "not + * found" value -- indistinguishable from a genuine want of 0, but "slots" + * is never legitimately 0 here: seqjob only ever runs for maxTasks==1, + * parjob only for maxTasks>1, so nslots is always >=1) and `rusages`/ + * `nhosts`/`nslots` (the job's own declared resource requests, and its + * whole-job totals -- not any one host's share) are used to compute the + * aggregate directly, on demand, via calc_wanted_job_total_resval() + * (resreq.c) instead -- deliberately kept as this function's own + * responsibility rather than the allocator plugins': what a LIMIT needs to + * see is this function's concern, not seqjob/parjob's, which should only + * ever compute what the job itself wants for capacity-satisfy purposes. */ int sched_fill_limit_requests(xce_sched_alloc_req_t* alloc_req, xlist_t* limits, xlist_t* rusages, int nhosts, @@ -967,29 +987,10 @@ int sched_fill_limit_requests(xce_sched_alloc_req_t* alloc_req, dst = dst_is_global ? &(alloc_req->shared) : &(alloc_req->host); if (dst_is_global && !src_is_global) { - int found = calc_wanted_job_total_resval( - rusages, PER_TASK, nhosts, nslots, vres->res->name, &val); - - /* "slots" is synthetic, never a declared rusage entry: seqjob_ - * prepare_allocation()/parjob_prepare_allocation() derive its - * want directly from the job's own task count (1.0 for - * seqjob -- always maxTasks==1 -- or (float)ctx->nslots for - * parjob), via their own unconditional key_find("slots", ...) - * + store, entirely outside the normal rusages loop -- so the - * lookup above always misses for it (bug-1790 refactor - * regression, live-confirmed against gitea#46's original - * ug1_slots/ug1_normal_slots/limit1 scenario: matching worked, - * but the cap never bound, because "wanted" silently stayed - * 0). Its job-wide total is exactly `nslots` itself -- the - * SAME value those allocators already store as its per-host - * want -- mirrored here as the one documented exception to - * the rusages-lookup rule, matching this codebase's own - * existing "slots" special-casing convention (both allocators - * already look it up by literal name, outside their normal - * rusages-driven computation). */ - if (!found && (vres->res->name != NULL) - && (strcmp(vres->res->name, "slots") == 0)) { - val = (float)nslots; + val = get_resval_by_id(&(alloc_req->host), vres->res->idx); + if (val == 0.0) { + calc_wanted_job_total_resval(rusages, PER_TASK, nhosts, + nslots, vres->res->name, &val); } } else { xvec_idval_t* src diff --git a/src/scheduler/testsuite/test_resource_accounting.c b/src/scheduler/testsuite/test_resource_accounting.c index 9e9d69bb..edae4385 100644 --- a/src/scheduler/testsuite/test_resource_accounting.c +++ b/src/scheduler/testsuite/test_resource_accounting.c @@ -925,23 +925,28 @@ test_fill_limit_requests_global_limit_on_host_resource_reads_host(void) * rusage entry -- seqjob_prepare_allocation()/parjob_prepare_allocation() * derive its want directly from the job's own task count (1.0 for seqjob, * always maxTasks==1; (float)ctx->nslots for parjob), via their own - * unconditional key_find("slots", ...) + store, entirely outside the - * normal rusages loop. The general on-demand path above (calc_wanted_job_ - * total_resval(), keyed by scanning rusages for a matching entry) always - * misses for "slots" as a result -- confirmed live against a fresh deploy - * of limits.yaml's actual ug1_slots/ug1_normal_slots/limit1 (all global, - * slots-resname'd): matching worked, but every job's "wanted" silently - * stayed 0, so the caps never bound at all, reintroducing bug-1789's - * exact original symptom. sched_fill_limit_requests() (resource.c) now - * falls back to `nslots` directly (the SAME value those allocators - * already store as "slots"'s own per-host want) whenever the rusages - * lookup misses AND the resource is named "slots" -- this test proves - * that fallback with NO "slots" entry in rusages at all, matching the - * real production shape exactly (unlike this test's own first version, - * which artificially added a "slots" rusages entry and masked this exact - * gap). + * unconditional key_find("slots", ...) + store into alloc_req->host, + * regardless of phase, entirely outside the normal rusages loop. The + * general on-demand path (calc_wanted_job_total_resval(), keyed by + * scanning rusages for a matching entry) always misses for "slots" as a + * result -- confirmed live against a fresh deploy of limits.yaml's actual + * ug1_slots/ug1_normal_slots/limit1 (all global, slots-resname'd): + * matching worked, but every job's "wanted" silently stayed 0, so the caps + * never bound at all, reintroducing bug-1789's exact original symptom. + * + * sched_fill_limit_requests() (resource.c) now reads alloc_req->host FIRST + * for this combination -- since the allocator's own unconditional "add + * slots" step already puts exactly the job-wide total there, no name-based + * special case is needed, just reading the data that's already staged -- + * and only falls back to the rusages-based computation when nothing is + * staged (any OTHER per-host resource). This test proves the "slots" path + * by pre-staging alloc_req->host exactly as the allocator plugins already + * do (value == the job's whole nslots, matching parjob's own (float)ctx-> + * nslots store) and leaving rusages EMPTY -- the real production shape, + * unlike this test's own first version, which artificially added a + * "slots" rusages entry and masked this exact gap. */ -static void test_fill_limit_requests_global_limit_on_slots_uses_nslots(void) +static void test_fill_limit_requests_global_limit_on_slots_reads_host(void) { xce_scheduler_t sched; xce_resource_t slots_res_in = { 0 }; @@ -984,6 +989,10 @@ static void test_fill_limit_requests_global_limit_on_slots_uses_nslots(void) TEST_ASSERT_EQUAL_INT(0, rc); rc = xvec_idval_init(&(alloc_req.host), 8); TEST_ASSERT_EQUAL_INT(0, rc); + /* pre-staged into .host, exactly as seqjob/parjob_prepare_allocation() + * always do for "slots" regardless of phase. */ + rc = xvec_idval_store(&(alloc_req.host), slots_res->idx, 4.0f); + TEST_ASSERT_TRUE(rc >= 0); rc = sched_fill_limit_requests(&alloc_req, &limits, &rusages, 1, 4); TEST_ASSERT_EQUAL_INT(0, rc); @@ -991,6 +1000,9 @@ static void test_fill_limit_requests_global_limit_on_slots_uses_nslots(void) TEST_ASSERT_EQUAL_INT(1, alloc_req.shared.count); TEST_ASSERT_EQUAL_INT(limit_res->idx, ((int*)alloc_req.shared.idxs.data)[0]); TEST_ASSERT_EQUAL_FLOAT(4.0f, ((float*)alloc_req.shared.vals.data)[0]); + /* .host stays exactly as pre-staged -- the limit's own vres entry went + * to .shared, nothing new was written to .host. */ + TEST_ASSERT_EQUAL_INT(1, alloc_req.host.count); xlist_fini(&rusages); xlist_fini(&limits); @@ -1144,7 +1156,7 @@ int main(void) RUN_TEST(test_reconcile_job_resource_usage_null_args_return_einval); RUN_TEST(test_ctx_uncommit_host_reverses_provisional_debit); RUN_TEST(test_fill_limit_requests_global_limit_on_host_resource_reads_host); - RUN_TEST(test_fill_limit_requests_global_limit_on_slots_uses_nslots); + RUN_TEST(test_fill_limit_requests_global_limit_on_slots_reads_host); RUN_TEST(test_fill_limit_requests_host_limit_on_host_resource_reads_host); RUN_TEST(test_fill_limit_requests_host_limit_on_global_resource_reads_shared); return UNITY_END();