lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactor(scheduler): read slots' job total from alloc_req instead of name-checking

Browse source 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GLfB5uWRdmx2WiZSFC7Fm7
This commit is contained in:
Rongsong Shen 2026-09-08 11:14:55 +00:00
parent 604373b48a
commit a6d96705d9
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
2 changed files with 61 additions and 48 deletions
Download patch file Download diff file Expand all files Collapse all files

63
src/scheduler/src/resource.c
View file

@ -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

46
src/scheduler/testsuite/test_resource_accounting.c
View file

@ -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();