From 5b18f14ab4fbbbc5cf94674d4000ae69d93f53ca Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 8 Sep 2026 10:59:16 +0000 Subject: [PATCH] fix(scheduler): restore slots-based global limit enforcement (bug-1790 regression) 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 Claude-Session: https://claude.ai/code/session_01GLfB5uWRdmx2WiZSFC7Fm7 --- src/scheduler/src/resource.c | 26 +++- .../testsuite/test_resource_accounting.c | 127 +++++++++++++++--- 2 files changed, 129 insertions(+), 24 deletions(-) diff --git a/src/scheduler/src/resource.c b/src/scheduler/src/resource.c index e74f5a64..665e48a7 100644 --- a/src/scheduler/src/resource.c +++ b/src/scheduler/src/resource.c @@ -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); diff --git a/src/scheduler/testsuite/test_resource_accounting.c b/src/scheduler/testsuite/test_resource_accounting.c index fc0c0ddf..9e9d69bb 100644 --- a/src/scheduler/testsuite/test_resource_accounting.c +++ b/src/scheduler/testsuite/test_resource_accounting.c @@ -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();