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

fix(scheduler): restore slots-based global limit enforcement (bug-1790 regression)

Browse source 
The limit-plugin-ownership refactor (fa34b038) broke gitea#46's exact
original scenario. calc_wanted_job_total_resval() scans the job's
rusages by name, but "slots" is never a declared rusage entry --
seqjob_prepare_allocation()/parjob_prepare_allocation() derive its want
directly from the job's own task count via their own unconditional
key_find("slots", ...) + store, entirely outside the rusages loop. The
refactor's on-demand lookup therefore always missed for "slots",
silently leaving the computed want at 0 -- reintroducing bug-1789's
exact original symptom (matching worked, but caps never bound).

Live-confirmed against a fresh deploy of gitea#46's own repro (9 plain
sleep jobs, user1, queue normal, no -R flag): limit1/ug1_slots/
ug1_normal_slots all matched correctly but zero unsatisfy_ lines
appeared and all 9 jobs dispatched uncapped. The refactor's own new
unit test had masked this -- it artificially added a "slots" rusages
entry that doesn't reflect production reality.

Fix: sched_fill_limit_requests() now falls back to `nslots` directly
(the same value both allocators already store as "slots"'s own
per-host want) whenever the rusages lookup misses and the resource is
named "slots" -- the one documented exception to the rusages-lookup
rule, matching this codebase's existing "slots" special-casing
convention.

New test test_fill_limit_requests_global_limit_on_slots_uses_nslots
proves the fallback with no "slots" entry in rusages at all. The
original test reworked to use "mem" instead, so it now genuinely
exercises the general on-demand path.

Full xce-build.sh all asan --incremental: 94/94 Ok. Live re-verified
against both the original gitea#46 scenario (byte-identical to the
pre-refactor trace) and the ug1_global_mem scenario -- both correct.

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 10:59:16 +00:00
parent e8aa761ca7
commit 5b18f14ab4
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
2 changed files with 129 additions and 24 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -967,8 +967,30 @@ 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) {
calc_wanted_job_total_resval(rusages, PER_TASK, nhosts, nslots,
vres->res->name, &val);
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;
}
} else {
xvec_idval_t* src
= src_is_global ? &(alloc_req->shared) : &(alloc_req->host);

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

@ -830,24 +830,27 @@ static void test_ctx_uncommit_host_reverses_provisional_debit(void)
* (scope, underlying-resource) combinations read straight out of alloc_req
* (already populated by whichever allocator plugin ran). This test proves
* the fourth, different-in-kind combination (bug-1789's actual reported
* case): a GLOBAL limit (write target alloc_req->shared) on a per-host
* underlying resource like "slots" (limits.yaml's ug1_normal_slots,
* summing slots across the whole cluster) -- alloc_req->host never holds
* a job-WIDE total for this (only per-host/per-bucket-specific data for
* the real per-host capacity check), so the want is computed directly
* from the job's own rusages instead, via calc_wanted_job_total_resval()
* (resreq.c) -- PER_TASK: value * nslots, using nslots=3 here specifically
* to prove the job's WHOLE total is used, not any one host's/bucket's
* share (this is the limit-evaluation layer, allocator-agnostic -- no
* seqjob/parjob-specific test needed for this any more).
* case, generalized in bug-1790): a GLOBAL limit (write target
* alloc_req->shared) on a per-host underlying resource that IS a normal
* declared rusage -- e.g. "mem" (a future global mem cap, bug-1790's own
* regression coverage) -- alloc_req->host never holds a job-WIDE total for
* this (only per-host/per-bucket-specific data for the real per-host
* capacity check), so the want is computed directly from the job's own
* rusages instead, via calc_wanted_job_total_resval() (resreq.c) --
* PER_TASK: value * nslots, using nslots=3 here specifically to prove the
* job's WHOLE total is used, not any one host's/bucket's share (this is
* the limit-evaluation layer, allocator-agnostic -- no seqjob/parjob-
* specific test needed for this any more). "slots" is deliberately NOT
* used as the underlying resource here -- see the sibling test below for
* why it needs its own, separate coverage.
*/
static void
test_fill_limit_requests_global_limit_on_host_resource_reads_host(void)
{
xce_scheduler_t sched;
xce_resource_t slots_res_in = { 0 };
xce_resource_t mem_res_in = { 0 };
xce_resource_t limit_res_in = { 0 };
xce_resource_t* slots_res;
xce_resource_t* mem_res;
xce_resource_t* limit_res;
xce_sched_alloc_req_t alloc_req;
xlist_t limits;
@ -859,23 +862,26 @@ test_fill_limit_requests_global_limit_on_host_resource_reads_host(void)
memset(&sched, 0, sizeof(sched));
TEST_ASSERT_EQUAL_INT(0, xce_sched_core_init(&(sched.core)));
/* "slots" -- deliberately NOT XCE_RESF_GLOBAL, matching the real
/* "mem" -- deliberately NOT XCE_RESF_GLOBAL, matching the real
* makeResource() call every host-capacity resource in this codebase
* goes through. */
slots_res_in.name = strdup("slots");
slots_res = makeResource(sched.core.resmgr, &slots_res_in);
TEST_ASSERT_NOT_NULL(slots_res);
* goes through, AND (unlike "slots") a resource that genuinely
* appears as a normal declared rusage entry when requested via
* -R "rusage[mem=...]" -- the real production shape this test's
* on-demand rusages lookup needs to match. */
mem_res_in.name = strdup(SCHED_MEM_RESOURCE_NAME);
mem_res = makeResource(sched.core.resmgr, &mem_res_in);
TEST_ASSERT_NOT_NULL(mem_res);
/* the LIMIT's own virtual resource -- GLOBAL, exactly as
* instantiate_limit()->add_vres() would flag it for a limit like
* ug1_normal_slots (is_host_based_limit() == 0). */
limit_res_in.name = strdup("limit=ug1_normal_slots");
* ug1_global_mem (is_host_based_limit() == 0). */
limit_res_in.name = strdup("limit=ug1_global_mem");
limit_res_in.flags = XCE_RESF_GLOBAL;
limit_res = makeResource(sched.core.resmgr, &limit_res_in);
TEST_ASSERT_NOT_NULL(limit_res);
memset(&vres, 0, sizeof(vres));
vres.res = slots_res;
vres.res = mem_res;
vres.vres = limit_res;
rc = xlist_init(&limits, 4);
@ -883,12 +889,12 @@ test_fill_limit_requests_global_limit_on_host_resource_reads_host(void)
rc = xlist_store(&limits, &vres);
TEST_ASSERT_TRUE(rc >= 0);
/* the job's own declared "slots" rusage -- 2.0/task, 3 tasks total
/* the job's own declared "mem" rusage -- 2.0/task, 3 tasks total
* (nslots below), for an expected job-wide total of 6.0. */
rc = xlist_init(&rusages, 4);
TEST_ASSERT_EQUAL_INT(0, rc);
memset(&rv, 0, sizeof(rv));
rv.res = strdup("slots");
rv.res = strdup(SCHED_MEM_RESOURCE_NAME);
rv.value = 2.0f;
rv.per_flag = PER_TASK;
rc = xlist_store(&rusages, &rv);
@ -914,6 +920,82 @@ test_fill_limit_requests_global_limit_on_host_resource_reads_host(void)
xlist_fini(&limits);
}
/*
* bug-1790 regression (found live, gitea#46): "slots" is 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; (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).
*/
static void test_fill_limit_requests_global_limit_on_slots_uses_nslots(void)
{
xce_scheduler_t sched;
xce_resource_t slots_res_in = { 0 };
xce_resource_t limit_res_in = { 0 };
xce_resource_t* slots_res;
xce_resource_t* limit_res;
xce_sched_alloc_req_t alloc_req;
xlist_t limits;
xce_vres_t vres;
xlist_t rusages;
int rc;
memset(&sched, 0, sizeof(sched));
TEST_ASSERT_EQUAL_INT(0, xce_sched_core_init(&(sched.core)));
slots_res_in.name = strdup("slots");
slots_res = makeResource(sched.core.resmgr, &slots_res_in);
TEST_ASSERT_NOT_NULL(slots_res);
limit_res_in.name = strdup("limit=ug1_normal_slots");
limit_res_in.flags = XCE_RESF_GLOBAL;
limit_res = makeResource(sched.core.resmgr, &limit_res_in);
TEST_ASSERT_NOT_NULL(limit_res);
memset(&vres, 0, sizeof(vres));
vres.res = slots_res;
vres.vres = limit_res;
rc = xlist_init(&limits, 4);
TEST_ASSERT_EQUAL_INT(0, rc);
rc = xlist_store(&limits, &vres);
TEST_ASSERT_TRUE(rc >= 0);
/* deliberately EMPTY -- "slots" never appears here in real production
* either. */
rc = xlist_init(&rusages, 4);
TEST_ASSERT_EQUAL_INT(0, rc);
rc = xvec_idval_init(&(alloc_req.shared), 8);
TEST_ASSERT_EQUAL_INT(0, rc);
rc = xvec_idval_init(&(alloc_req.host), 8);
TEST_ASSERT_EQUAL_INT(0, rc);
rc = sched_fill_limit_requests(&alloc_req, &limits, &rusages, 1, 4);
TEST_ASSERT_EQUAL_INT(0, rc);
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]);
xlist_fini(&rusages);
xlist_fini(&limits);
}
/*
* bug-1789 (already-working sibling case, unaffected by the fix): a
* HOST-scoped limit (its own vres NOT GLOBAL-flagged, so its write
@ -1062,6 +1144,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_host_limit_on_host_resource_reads_host);
RUN_TEST(test_fill_limit_requests_host_limit_on_global_resource_reads_shared);
return UNITY_END();