From 22cfe61642912ac5f433eb97d1243f2263b3c187 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 7 Sep 2026 12:44:16 +0000 Subject: [PATCH] fix(scheduler): fix limit-plugin usergroup/scope/error-code cascade (gitea#46) While live-verifying gitea#46, found and fixed a cascade of 5 previously undetected bugs that together made every usergroup-scoped limit in limits.yaml silently non-functional: - Named usergroups/hostgroups (users.yaml/hosts.yaml) were never registered into the scheduler's group managers (groupmgr_add() had zero production callers), so any limit referencing one by name could never resolve real membership. - An unconfigured limit scope (e.g. no "projects:") arrived as an empty-but-present capnp group, not NULL, so build_consumer() built a real empty group (matching no one) instead of falling back to "all". - Three separate call sites conflated key_find()'s negative "not found" return with a real error, short-circuiting before ever reaching the "instantiate on first use" logic right below them -- the same defect class already fixed once before in this codebase (bug-1495). - Two call sites passed &(sched->core.resmgr) where a plain handle was expected, one level of indirection too many, causing RTTI's key_find() to reject it as an invalid handle. All five are live-verified: fresh cluster deploys now correctly match and instantiate usage tracking for all 7 limits in limits.yaml on every job submission, reproduced across multiple independent from-scratch redeploys. A 6th, architecturally distinct issue remains open (bug-1789, logged, not fixed): sched_fill_limit_requests() computes a job's consumption of a slots-scoped global limit by reading the wrong request vector, so the numeric cap itself still never binds. This needs a design decision on intended cross-scope semantics, not a mechanical fix, so it's left for a follow-up. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Hb6Edtj4KbS8CrgEWYdA2E --- .wolf/buglog.json | 170 ++++++++++++++++++++++++++++++ src/include/group.h | 1 + src/lib/group.c | 37 +++++++ src/lib/testsuite/test_group.c | 107 +++++++++++++++++++ src/lib/testsuite/test_limit.c | 32 ++++++ src/lib/xcelimit.c | 51 +++++++-- src/scheduler/src/plugins/limit.c | 42 ++++++-- src/scheduler/src/scheduler.c | 11 ++ 8 files changed, 432 insertions(+), 19 deletions(-) diff --git a/.wolf/buglog.json b/.wolf/buglog.json index a42368c5..625131d5 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -21259,6 +21259,176 @@ ], "occurrences": 1, "last_seen": "2026-09-07T08:40:00Z" + }, + { + "id": "bug-1784", + "timestamp": "2026-09-07T09:00:00Z", + "gitea_issue": 46, + "error_message": "Discovered while live-verifying gitea#46's Part 1 (dispatch-stops-at-8, previously hypothesized in bug-1743 to be limits.yaml's ug1_normal_slots cap=8 working as designed, never live-confirmed). Live test: submitted 10 single-slot jobs as user1 (a genuine ug1 member per users.yaml) on queue normal against a 22-slot worker -- all 10 ran, the supposed cap=8 usergroup limit never blocked anything.", + "file": "src/lib/group.c (groupmgr_load_configs, new), src/include/group.h, src/scheduler/src/scheduler.c (sched_init_config)", + "root_cause": "Named usergroups/hostgroups (users.yaml's top-level \"usergroups:\", hosts.yaml's \"hostgroups:\") are carried down to the scheduler via xce_conf4sched_t.ugroups/hgroups (src/controller/sched.c populates these from c->myconf->cluster_config->ugroups/hgroups), but nothing on the scheduler side ever registered them into a group manager by name: groupmgr_add() (src/lib/group.c) -- the obvious candidate -- had ZERO callers anywhere in production code (confirmed via full-tree grep). When a limit's own per-limit consumer group (plugins/limit.c's build_consumer()) references a named group like \"ug1\" as a member, make_g_member() (group.c) calls get_group(mgr, \"ug1\"), finds nothing (since it was never registered), and falls back to treating \"ug1\" as a literal, ungrouped member name that will never strcmp-match a real username via is_in_group(). Confirmed via source inspection (build_group(), a plausible alternative entry point declared in group.h, has NO implementation anywhere either -- a second dead prototype) and via live behavior (user1, a real ug1 member, ran unthrottled against a configured cap).", + "fix": "FIXED (2026-09-07). Added groupmgr_load_configs(var mgr, int count, xce_group_config_t** configs) to group.c/group.h: iterates the config list and registers each named entry into mgr via the existing, already-correct makeGroup(mgr, config, config->name). Wired into scheduler.c's sched_init_config(): groupmgr_load_configs() for both core.ugmgr/config->ugroups and core.hgmgr/config->hgroups, called BEFORE plugin_config_update() so any limit's own build_consumer() call (which runs inside plugin_config_update -> limit_update_config -> build_limit) can resolve a named-group reference at build time. TDD: 3 new tests in src/lib/testsuite/test_group.c -- test_load_configs_registers_each_group_by_name, test_load_configs_enables_group_name_as_member_reference (the exact regression: a limit-style unnamed consumer group referencing a named group by member, mirroring build_consumer()'s own call shape), test_load_configs_zero_count_is_a_noop. All pass (313 Tests 0 Failures under CGO... err, under ASAN_OPTIONS=detect_leaks=0 xcelib_test). Full scheduler suite: 76/77 (pre-existing unrelated bug-1762 test_core_events.c failure). LIVE-VERIFIED: fresh single-node deploy, user1 (ug1 member) submitting jobs on queue normal now correctly has match_limit() report a match (previously always rc=0/no-match) for ug1_slots and ug1_normal_slots -- confirmed via scheduler trace logs. (Full cap enforcement required two further fixes in the same session -- see bug-1786/1788 -- and one remaining gap not yet fixed, see bug-1789.)", + "tags": [ + "scheduler", + "limit-plugin", + "usergroup", + "hostgroup", + "group-manager", + "gitea-46", + "live-verified", + "fixed" + ], + "related_bugs": [ + "bug-1743", + "bug-1785", + "bug-1786", + "bug-1787", + "bug-1788", + "bug-1789" + ], + "occurrences": 1, + "last_seen": "2026-09-07T09:00:00Z" + }, + { + "id": "bug-1785", + "timestamp": "2026-09-07T09:15:00Z", + "gitea_issue": 46, + "error_message": "Found immediately after fixing bug-1784: with named-usergroup resolution working, a limit scoped to only SOME of the four dimensions (users.yaml's demo limits.yaml sets users+queues on ug1_normal_slots but never projects/hosts) still matched no one at all.", + "file": "src/lib/xcelimit.c (build_consumer)", + "root_cause": "build_consumer() (xcelimit.c) is documented to default an unconfigured scope to the builtin \"all\" group (matching everyone) when g_config is NULL. But capnp encodes LimitConfig's users/queues/projects/hosts as embedded GROUPS, not nullable pointers (xceadmin's mapping.py: _build_membership_group() no-ops on an absent YAML key, but the embedded capnp struct is still present on the wire as an empty-but-non-NULL group) -- so an unconfigured scope arrives here as a non-NULL xce_group_config_t with zero members/ex_members, never as NULL. build_consumer()'s g_config!=NULL branch then builds a real, empty, UNNAMED group via makeGroup() -- a group that (per is_in_group()'s own logic) matches no one, the exact OPPOSITE of the intended \"unrestricted\" default. Every limit in this repo's demo limits.yaml configures only a subset of the four scopes, so every one of them was silently rejecting every job on its unconfigured dimensions.", + "fix": "FIXED (2026-09-07). build_consumer() now normalizes an empty-but-present g_config (members.count==0 AND ex_members.count==0) to NULL before the existing g_config!=NULL/NULL branch runs, restoring the documented \"fall back to all\" behavior for a genuinely-unconfigured scope. An operator who wants \"no one\" for a scope still has the builtin \"empty\" group to reference by name explicitly, so this doesn't remove any real capability. TDD: new test_build_consumer_empty_config_defaults_to_all (src/lib/testsuite/test_limit.c) -- builds a zero-valued xce_group_config_t{0}, asserts build_consumer() falls back to a group where is_in_group() matches an arbitrary name. Passes (313 lib tests, 0 failures). Full scheduler suite: 76/77 (pre-existing bug-1762). LIVE-VERIFIED as part of the same trace as bug-1784/1786/1787/1788 -- combined, these let match_limit() and instantiate_limit() run to completion for all 7 limits in limits.yaml on every job submission (previously it silently rejected on the FIRST unconfigured scope it hit).", + "tags": [ + "scheduler", + "limit-plugin", + "capnp", + "group-config", + "default-semantics", + "gitea-46", + "fixed" + ], + "related_bugs": [ + "bug-1784", + "bug-1786", + "bug-1787", + "bug-1788", + "bug-1789" + ], + "occurrences": 1, + "last_seen": "2026-09-07T09:15:00Z" + }, + { + "id": "bug-1786", + "timestamp": "2026-09-07T09:30:00Z", + "gitea_issue": 46, + "error_message": "Found immediately after bug-1784/1785: with group resolution and default-scope semantics both fixed, match_limit() finally reported a genuine match (rc=1) for the FIRST time in this codebase's live history -- and the very next step (usage/threshold accounting) immediately broke: limit_job_election()'s loop aborted after the first matching limit, so no limit past it in iteration order was ever evaluated for that job.", + "file": "src/scheduler/src/plugins/limit.c (match_job_global_limit_)", + "root_cause": "match_job_global_limit_(), right after a successful match_limit(), does `rc = key_find(privdata->instances, vresname, &vres); if (rc <= 0) { return rc; }` before the `if (vres == NULL) { instantiate_limit(...) }` block below it. On the FIRST-EVER lookup of a given vresname (always true immediately after bug-1784/1785 made matching work for the first time), key_find() signals \"not found\" as a NEGATIVE rc (confirmed live: rc=-2/-ENOENT, not 0) -- this codebase's kvmgr convention elsewhere (get_group(), is_host_based_limit()) treats rc<=0 uniformly as \"not found\", so this specific early-return, written assuming rc==0 meant not-found and only negative meant a real error, unconditionally caught EVERY not-found case and returned before instantiate_limit() could ever run. limit_job_election()'s own loop breaks on any rc<0 from match_job_global_limit_, so this also silently aborted evaluation of every remaining limit for that job. Same defect class already fixed once before in this codebase, in a different pair of functions: bug-1495 (sched_get_user()/sched_get_project(), core.c) hit and fixed the identical rc<0-vs-not-found conflation.", + "fix": "FIXED (2026-09-07). Removed the erroneous `if (rc <= 0) { return rc; }` guard entirely; `vres` (calloc'd NULL, only ever reassigned by a successful key_find) is already the correct, sufficient signal for \"needs instantiation\" via the existing `if (vres == NULL)` block immediately below -- mirrors this file's own build_vres_name/key_add error-handling style, just without conflating a legitimate not-found miss with failure. No new unit test added for this specific control-flow fix (see bug-1788's fix note for why: exercising limit_job_election() end-to-end requires a fully-wired xce_sched_core_t fixture -- resmgr, sharedInst, a real xce_sched_obj_t with initialized xlist_t/zarray_t fields -- judged not worth the fragility for a single control-flow correction already covered by full live E2E verification). Full scheduler suite: 76/77 (pre-existing bug-1762). LIVE-VERIFIED: after this fix (combined with bug-1787/1788, needed to get all the way through), all 7 limits in limits.yaml now appear in match_job_global_limit_'s own trace log for every job submission, where previously only the first one in iteration order ever did.", + "tags": [ + "scheduler", + "limit-plugin", + "kvmgr", + "error-code-convention", + "not-found-vs-error", + "gitea-46", + "live-verified", + "fixed" + ], + "related_bugs": [ + "bug-1495", + "bug-1784", + "bug-1785", + "bug-1787", + "bug-1788", + "bug-1789" + ], + "occurrences": 1, + "last_seen": "2026-09-07T09:30:00Z" + }, + { + "id": "bug-1787", + "timestamp": "2026-09-07T09:45:00Z", + "gitea_issue": 46, + "error_message": "Found immediately after bug-1786: match_job_global_limit_'s own key_find rc=-2 masking fixed, but the FOLLOWING step -- instantiate_limit() -- now returned rc=-22/-EINVAL outright, still aborting the loop after the first limit.", + "file": "src/scheduler/src/plugins/limit.c (match_job_global_limit_, match_job_host_limit_)", + "root_cause": "is_host_based_limit() and instantiate_limit() (src/lib/xcelimit.c) both take a plain `var resmgr` parameter -- core.h declares core.resmgr itself as a plain `var` (a handle, not var*), matching how every other manager consumer in this codebase (get_group(), is_in_group(), key_find() itself) takes its manager by value. All four call sites in limit.c (2 in match_job_global_limit_, 2 in the host-scoped sibling match_job_host_limit_) instead passed &(sched->core.resmgr) -- the ADDRESS of the field, one level of indirection too many. RTTI's key_find() dispatch received what amounts to a var* misinterpreted as a var (a meta_obj_t* pointing at raw, non-RTTI memory) and rejected it: silently inside is_host_based_limit() (whose own rc<=0 guard turns ANY key_find failure, including this type-confusion, into a uniform -ENOENT -- see bug-1786's note on why that specific defensive pattern isn't itself wrong), but fatally inside instantiate_limit(), whose own key_find() call on the SAME mistyped resmgr returned -EINVAL and (before this session's other fixes) propagated straight out.", + "fix": "FIXED (2026-09-07). Removed the erroneous & at all 4 call sites (2 in match_job_global_limit_, 2 in match_job_host_limit_): `is_host_based_limit(sched->core.resmgr, limit)` / `instantiate_limit(&vres, sched->core.resmgr, limit, ...)`. Confirmed via live diagnostic instrumentation (temporary, removed before landing) that instantiate_limit()'s own top-of-function NULL-argument dump showed every argument non-NULL yet still failing -- proving the -EINVAL came from RTTI's own internal type/handle validation inside key_find(), not the function's explicit guard clause, which directly confirmed the type-confusion theory before touching production code. Full scheduler suite: 76/77 (pre-existing bug-1762). LIVE-VERIFIED as part of the combined bug-1784/1785/1786/1788 trace.", + "tags": [ + "scheduler", + "limit-plugin", + "rtti", + "pointer-indirection", + "type-confusion", + "gitea-46", + "live-verified", + "fixed" + ], + "related_bugs": [ + "bug-1784", + "bug-1785", + "bug-1786", + "bug-1788", + "bug-1789" + ], + "occurrences": 1, + "last_seen": "2026-09-07T09:45:00Z" + }, + { + "id": "bug-1788", + "timestamp": "2026-09-07T10:00:00Z", + "gitea_issue": 46, + "error_message": "Found immediately after bug-1787: with the resmgr type-confusion fixed, instantiate_limit() still returned rc=-2/-ENOENT, still aborting limit_job_election()'s loop after the first limit -- reproduced identically on a scheduler's FIRST-EVER join to a freshly-started controller, ruling out any rejoin/resync-specific explanation.", + "file": "src/lib/xcelimit.c (instantiate_limit)", + "root_cause": "The EXACT same defect class as bug-1786, recurring twice more, plus a related truthiness bug, all inside instantiate_limit() itself: (1) `rc = key_find(resmgr, vresname, &(vres->vres)); if (rc < 0) { return rc; } if (rc == 0) { add_vres(...); }` -- live-confirmed via targeted diagnostic instrumentation that this key_find's own \"not found\" signal is rc=-2/-ENOENT (not 0) for THIS resmgr-backed lookup too, so the `if (rc<0) return rc;` guard caught every first-ever vresname lookup before the add_vres()-creates-it branch could run. (2) The immediately following lookup of the limit's underlying resource (limit->val.resname, e.g. \"slots\") had the identical `if (rc==0) return -ENOENT;` pattern, vulnerable to the same rc convention mismatch (not hit in this specific trace only because \"slots\" was already registered by the time it was reached, but latent). (3) `if (is_host_based_limit(resmgr, limit)) { shared = 0; }` checked bare truthiness of a function that can return a negative errno -- a non-zero, therefore truthy, value -- so a transient is_host_based_limit() failure would have incorrectly flagged a limit as host-based.", + "fix": "FIXED (2026-09-07). All three corrected in instantiate_limit(): (1) `if (vres->vres == NULL) { add_vres(...); }` replaces the rc<0/rc==0 checks -- vres->vres is calloc'd NULL and only ever reassigned by a successful key_find, the same reliable-signal pattern as bug-1786's fix. (2) `if (vres->res == NULL) { return -ENOENT; }` replaces the equivalent rc<0/rc==0 pair for the resource-existence check. (3) `is_host_based_limit(resmgr, limit) > 0` replaces the bare truthiness check. No new unit test added (same reasoning as bug-1786: this function's correctness is now covered by full live E2E verification, and hand-building a resmgr+sched_core fixture for a unit test was judged not worth the fragility versus the live proof already obtained -- see bug-1786/1789 discussion). Full scheduler suite: 76/77 (pre-existing bug-1762). LIVE-VERIFIED: combined with bug-1784/1785/1786/1787, all 7 limits in limits.yaml (ug1_slots, ug2_slots_per_user, night_mem_pct, hg1_slots_per_host, ug1_normal_slots, limit1, limits2) now correctly appear in the trace and complete matching+instantiation for every job submission on a completely fresh, never-before-touched cluster deploy -- confirmed reproducible across 3 independent from-scratch redeploys, ruling out any deploy-specific or rejoin-specific artifact.", + "tags": [ + "scheduler", + "limit-plugin", + "kvmgr", + "error-code-convention", + "not-found-vs-error", + "truthiness-bug", + "gitea-46", + "live-verified", + "fixed" + ], + "related_bugs": [ + "bug-1495", + "bug-1784", + "bug-1785", + "bug-1786", + "bug-1787", + "bug-1789" + ], + "occurrences": 1, + "last_seen": "2026-09-07T10:00:00Z" + }, + { + "id": "bug-1789", + "timestamp": "2026-09-07T10:30:00Z", + "gitea_issue": 46, + "error_message": "After bug-1784 through bug-1788 all fixed and live-verified (all 7 limits in limits.yaml now correctly match and instantiate usage tracking for every job), the actual numeric cap still does not reject anyone: 10 single-slot jobs submitted as user1 on queue normal against ug1_normal_slots's configured value:8 cap all still reach RUN, none PEND.", + "file": "src/scheduler/src/resource.c (sched_fill_limit_requests)", + "root_cause": "NOT YET ROOT-CAUSED TO A FIX -- traced to a specific, likely-defective computation, but the correct intended semantics were not established this session (deliberately stopped here rather than guess at production logic). Chain traced and CONFIRMED CORRECT up to one point: match_job_global_limit_ populates obj->glimits (fixed, bug-1784-1788) -> plugin.c's plugin_prepare_allocation calls sched_fill_limit_requests(sched, &(ctx->alloc_req.shared), &(obj->glimits)) when ctx->worker==NULL (the global/non-host-scoped case, correct for ug1_normal_slots) -> seqjob_allocator.c/parjob_allocator.c both correctly call sched_check_shared_satisfy(sched, uncomit, &(ctx->alloc_req.shared), &(ctx->unmeet)) and correctly reject (PREASON_NO_RESOURCE, rc=0) when a shared resource's wanted amount would exceed its tracked total (itself correctly set to limit->val.val=8 by update_limit_res_value(), which DOES run correctly now). The break is inside sched_fill_limit_requests() itself (resource.c:912-931): for each matched limit, it computes the job's \"wanted\" quantity of the limit's virtual resource via `val = get_resval_by_id(vec, vres->res->idx)` where `vec` is `ctx->alloc_req.shared` ITSELF -- i.e. it looks up how many units of the limit's UNDERLYING resource (e.g. \"slots\") the job already requests, inside the SAME shared-resource-request vector it is about to write the limit's own synthetic entry into. But \"slots\" is inherently a per-host resource, requested via ctx->alloc_req.HOST (task/slot count per host), not ctx->alloc_req.shared -- so this lookup always misses, get_resval_by_id's own not-found fallback returns 0.0, and every job is recorded as requesting ZERO units of ug1_normal_slots regardless of how many slots it actually asks for. sched_check_shared_satisfy then trivially \"satisfies\" a wanted=0 request against any total, no matter how many jobs are already running -- the cap can never bind. NOT CONFIRMED whether this is a straightforward bug (should read from ctx->alloc_req.host or from resreq directly) or reflects a genuine, unaddressed design gap in how a GLOBAL (non-host-scoped) limit is meant to interact with a per-host-scoped underlying resource like slots -- the fix requires understanding what \"consumption count\" SHOULD mean for this combination, not just correcting an obvious return-code mistake like bug-1784-1788 all were.", + "fix": "NOT FIXED -- deliberately stopped here per explicit user instruction (\"land what's confirmed\") rather than guess at the intended design of cross-scope limit resource consumption. This is a 6th, architecturally distinct issue from the bug-1784-1788 cascade (which were all clear-cut, single-fix logic errors); this one needs either a design decision (what should a slots-resname'd global limit's per-job consumption be computed from?) or deeper investigation into whether an existing, unexamined mechanism already threads the job's task/slot count into a form sched_fill_limit_requests can read. NEXT STEPS for a follow-up session: (1) check whether ctx->alloc_req.host already carries a per-job \"slots wanted\" entry at the point plugin_prepare_allocation's ctx->worker==NULL branch runs (it may not yet, if host selection hasn't happened) -- if the global-limit-evaluation phase genuinely precedes host selection, the job's own resreq/mintasks (not a per-host allocation) is likely the correct source instead; (2) check test_scheduler_backpressure.c / any existing shared-resource-limit test fixture for a working end-to-end precedent that might already demonstrate correct semantics for a DIFFERENT (non-slots) global limit; (3) consider whether this NEEDS fixing at all for the ORIGINAL gitea#46 report, given the reporter's dispatch-stops-at-8 was against raw host MAX_SLOT capacity (confirmed working correctly, unaffected by any of bug-1784-1789) rather than a usergroup limit -- the usergroup-limit cascade was discovered via this session's OWN verification attempt, not the original report.", + "tags": [ + "scheduler", + "limit-plugin", + "resource-allocation", + "design-gap", + "not-fixed", + "gitea-46", + "needs-design-decision" + ], + "related_bugs": [ + "bug-1784", + "bug-1785", + "bug-1786", + "bug-1787", + "bug-1788" + ], + "occurrences": 1, + "last_seen": "2026-09-07T10:30:00Z" } ] } diff --git a/src/include/group.h b/src/include/group.h index f9ac5eda..aed918db 100644 --- a/src/include/group.h +++ b/src/include/group.h @@ -13,6 +13,7 @@ extern xce_group_t* makeGroup(var, xce_group_config_t*, char*); extern int groupmgr_init(var*, int, int); extern void groupmgr_fini(var); extern int groupmgr_add(var, xce_group_t*); +extern int groupmgr_load_configs(var, int, xce_group_config_t**); extern xce_group_t* groupmgr_lock(var, char*, int); extern void groupmgr_del(var, xce_group_t*); extern int groupmgr_iter(var, xlist_t*, void*); diff --git a/src/lib/group.c b/src/lib/group.c index fdcf5dfc..97a6c83b 100644 --- a/src/lib/group.c +++ b/src/lib/group.c @@ -303,6 +303,43 @@ int groupmgr_add(var mgr, xce_group_t* g) return rc; } +/* bug-1784: registers a cluster config's NAMED group list (e.g. + * xce_conf4sched_t.ugroups/hgroups, sourced from users.yaml's top-level + * "usergroups:"/hosts.yaml's "hostgroups:") into mgr by name, via the + * existing makeGroup(). Without this, a group config's own name is never + * added to mgr's kvtab under that name, so any OTHER group config that + * references it as a member (e.g. a limit's per-limit consumer group, + * plugins/limit.c's build_consumer()) can never resolve it: make_g_member() + * falls back to treating the name as a literal, ungrouped member that will + * never match a real username/hostname. Callers (e.g. scheduler.c's + * sched_init_config()) must call this BEFORE building anything that may + * reference these groups by name. */ +int groupmgr_load_configs(var mgr, int count, xce_group_config_t** configs) +{ + int i; + + if (mgr == NULL) { + return -EINVAL; + } + + for (i = 0; i < count; i++) { + xce_group_config_t* config = configs[i]; + xce_group_t* g; + + if ((config == NULL) || (config->name == NULL) + || (strlen(config->name) == 0)) { + continue; + } + + g = makeGroup(mgr, config, config->name); + if (g == NULL) { + return -errno; + } + } + + return 0; +} + xce_group_t* groupmgr_lock(var mgr, char* name, int op) { int rc; diff --git a/src/lib/testsuite/test_group.c b/src/lib/testsuite/test_group.c index 6e47e29d..f315b53e 100644 --- a/src/lib/testsuite/test_group.c +++ b/src/lib/testsuite/test_group.c @@ -255,6 +255,110 @@ static void test_nest2(void) destroy(mgr); } +/* bug-1784: xce_conf4sched_t's n_ugroups/ugroups (and n_hgroups/hgroups) + * carry the cluster's NAMED usergroups/hostgroups (e.g. users.yaml's + * top-level "usergroups: [{groupname: ug1, members: [user1, user2]}]") + * down to the scheduler, but nothing ever registered them into a group + * manager by name -- groupmgr_add() (the obvious candidate) has zero + * production callers. Left unregistered, any OTHER group config that + * references "ug1" as a member (e.g. a limit's own per-limit consumer + * group, built via build_consumer()/makeGroup() in plugins/limit.c) can + * never resolve it: make_g_member() calls get_group(mgr, "ug1"), finds + * nothing, and falls back to treating "ug1" as a literal, ungrouped member + * name that will never strcmp-match a real username -- so a limit scoped + * to a named usergroup silently never matches anyone. Live-confirmed via + * gitea#46 investigation: a real ug1 member ran 10 jobs against a + * configured cap of 8 with zero enforcement. + * + * groupmgr_load_configs() is the fix: a bulk loader for exactly the + * config-push shape (a count + xce_group_config_t** array) that registers + * each entry by name via the existing, already-correct makeGroup(), so + * later group configs (limits, or another named group) can resolve it via + * get_group() by name, the same way test_nest() above proves manual + * back-to-back makeGroup() calls already work. + */ +static void test_load_configs_registers_each_group_by_name(void) +{ + var mgr; + xce_group_config_t g1 = { + .name = "ug1", + .members = { .count = 2, .data = (char*[]) { "user1", "user2" } } + }; + xce_group_config_t g2 = { + .name = "ug2", + .members = { .count = 1, .data = (char*[]) { "user3" } } + }; + xce_group_config_t* configs[2] = { &g1, &g2 }; + int rc; + + groupmgr_init(&mgr, 16, 16); + + rc = groupmgr_load_configs(mgr, 2, configs); + TEST_ASSERT_EQUAL_INT(0, rc); + + TEST_ASSERT_TRUE(get_group(mgr, "ug1") != NULL); + TEST_ASSERT_TRUE(get_group(mgr, "ug2") != NULL); + TEST_ASSERT_TRUE(is_in_group(get_group(mgr, "ug1"), "user1") > 0); + TEST_ASSERT_TRUE(is_in_group(get_group(mgr, "ug1"), "user2") > 0); + TEST_ASSERT_TRUE(is_in_group(get_group(mgr, "ug1"), "user3") == 0); + + destroy(mgr); +} + +/* THE regression, reproduced directly at the group layer: a limit-style + * consumer group (empty name -- build_consumer() always leaves per-limit + * group configs unnamed, deriving a default name instead, config-design.md + * "combined scope" convention) that references a NAMED group ("ug1") as a + * member must resolve real membership through it, exactly as a real + * xce_limit_t's CONSUMER_USER group would when it references + * users.yaml's "ug1". Before the fix (no bulk loader called, "ug1" never + * registered), this fails: is_in_group() falls through to a literal + * strcmp("ug1", "user1") that never matches. + */ +static void test_load_configs_enables_group_name_as_member_reference(void) +{ + var mgr; + xce_group_config_t ug1 = { + .name = "ug1", + .members = { .count = 2, .data = (char*[]) { "user1", "user2" } } + }; + xce_group_config_t* configs[1] = { &ug1 }; + xce_group_config_t limit_group_cfg = { + .members = { .count = 1, .data = (char*[]) { "ug1" } } + }; + xce_group_t* limit_group; + int rc; + + groupmgr_init(&mgr, 16, 16); + + rc = groupmgr_load_configs(mgr, 1, configs); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* mirrors build_consumer()'s own call shape: no config->name, a + * caller-supplied default_g_name instead. */ + limit_group = makeGroup(mgr, &limit_group_cfg, "some_limit-ug-group"); + TEST_ASSERT_TRUE(limit_group != NULL); + + TEST_ASSERT_TRUE(is_in_group(limit_group, "user1") > 0); + TEST_ASSERT_TRUE(is_in_group(limit_group, "user2") > 0); + TEST_ASSERT_TRUE(is_in_group(limit_group, "user3") == 0); + + destroy(mgr); +} + +static void test_load_configs_zero_count_is_a_noop(void) +{ + var mgr; + int rc; + + groupmgr_init(&mgr, 16, 16); + + rc = groupmgr_load_configs(mgr, 0, NULL); + TEST_ASSERT_EQUAL_INT(0, rc); + + destroy(mgr); +} + static void test_reconfigure_same_group_members(void) { var mgr; @@ -311,6 +415,9 @@ void test_group(void) RUN_TEST(test_nest); RUN_TEST(test_nest2); RUN_TEST(test_reconfigure_same_group_members); + RUN_TEST(test_load_configs_registers_each_group_by_name); + RUN_TEST(test_load_configs_enables_group_name_as_member_reference); + RUN_TEST(test_load_configs_zero_count_is_a_noop); xstring_table_fini(); } diff --git a/src/lib/testsuite/test_limit.c b/src/lib/testsuite/test_limit.c index 0238f25a..3002c4d1 100644 --- a/src/lib/testsuite/test_limit.c +++ b/src/lib/testsuite/test_limit.c @@ -65,6 +65,37 @@ static void test_build_consumer2(void) return; } +/* bug-1785: capnp encodes LimitConfig's users/queues/projects/hosts as + * embedded GROUPS, not nullable pointers -- so a scope the YAML author + * never configured (e.g. no "projects:" block) still arrives here as a + * non-NULL xce_group_config_t with zero members/ex_members, not NULL. + * build_consumer() must treat that the same as a genuinely NULL g_config + * (fall back to the "all" group, matching everyone) rather than building + * a real, empty, unnamed group that matches no one -- the opposite of + * this function's own documented default, and the reason a limit scoped + * to only some of the four dimensions (e.g. users+queues but not + * projects+hosts, matching this repo's own limits.yaml) never matched + * anyone at all: its always-present, always-empty unconfigured scopes + * rejected every job. + */ +static void test_build_consumer_empty_config_defaults_to_all(void) +{ + xce_consumer_t* c = NULL; + var mgr; + xce_group_config_t gdata = { 0 }; + int rc = 0; + + groupmgr_init(&mgr, 4, 4); + rc = build_consumer(&c, mgr, CONSUMER_PROJECT, 0, &gdata, "some_limit"); + + TEST_ASSERT_TRUE(rc == 1); + TEST_ASSERT_NOT_NULL(c->group); + TEST_ASSERT_TRUE(is_in_group(c->group, "any_project_name") > 0); + + kill_consumer(c); + destroy(mgr); +} + static void test_build_limit_ok1(void) { var umgr; @@ -337,6 +368,7 @@ void test_limit(void) RUN_TEST(test_build_consumer1); RUN_TEST(test_build_consumer2); + RUN_TEST(test_build_consumer_empty_config_defaults_to_all); RUN_TEST(test_build_limit_ok1); RUN_TEST(test_match_limit_ok1); RUN_TEST(test_match_limit_ok2); diff --git a/src/lib/xcelimit.c b/src/lib/xcelimit.c index 7d8100ac..a336fb27 100644 --- a/src/lib/xcelimit.c +++ b/src/lib/xcelimit.c @@ -76,6 +76,26 @@ int build_consumer(xce_consumer_t** pconsumer, var mgr, c->per_consumer = (per_consumer != 0) ? 1 : 0; c->group = NULL; + /* bug-1785: a scope the limit's YAML never configured (e.g. no + * "projects:" block) still arrives here as a non-NULL g_config with + * zero members/ex_members -- capnp encodes LimitConfig's users/queues/ + * projects/hosts as embedded GROUPS, not nullable pointers, so they are + * always present on the wire even when the YAML author left them out. + * Treating that empty-but-present config the same as a genuinely + * user-authored empty group (which would make this consumer match NO + * ONE) silently defeats every unconfigured scope instead of leaving it + * unrestricted -- the opposite of this function's own documented + * "NULL means match everyone" fallback below, and the reason a limit + * scoped to only e.g. users+queues never matched anyone at all (its + * always-present, always-empty projects/hosts scopes rejected every + * job). An operator who genuinely wants "no one" already has the + * builtin "empty" group to reference explicitly. */ + if ((g_config != NULL) + && (g_config->members.count == 0) + && (g_config->ex_members.count == 0)) { + g_config = NULL; + } + if (g_config != NULL) { char* default_g_name = NULL; char consumer_group_name[2 * XCE_MAXNAME_LEN + 1]; @@ -437,7 +457,11 @@ int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit, vres->limit = limit; - if (is_host_based_limit(resmgr, limit)) { + /* bug-1788: is_host_based_limit() can return a negative errno (e.g. a + * transient key_find() miss); checking bare truthiness treated ANY + * such error as "yes, host-based" (a negative int is non-zero), same + * mistake class as this function's own two key_find() calls above. */ + if (is_host_based_limit(resmgr, limit) > 0) { shared = 0; } @@ -446,15 +470,19 @@ int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit, return rc; } + /* bug-1788: same class of defect as bug-1786 (match_job_global_limit_'s + * own vres lookup) -- key_find() on resmgr signals "not found" as a + * negative rc (-ENOENT), not 0, so the preceding `if (rc<0) return rc;` + * always caught the FIRST-ever lookup of a given vresname before this + * code could ever reach add_vres()'s "not found, create it" branch + * below. `vres->vres` is the reliable signal instead: key_find() only + * ever assigns it on an actual find, and it starts NULL (vres was + * calloc'd above), so a failed/not-found lookup leaves it untouched. */ lock(resmgr, RTTI_LOCK_READ); rc = key_find(resmgr, (void*)vresname, (void**)&(vres->vres)); unlock(resmgr); - if (rc < 0) { - return rc; - } - - if (rc == 0) { + if (vres->vres == NULL) { rc = add_vres(&(vres->vres), resmgr, vresname, shared); if (rc < 0) { FREEUP(vresname); @@ -464,15 +492,16 @@ int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit, FREEUP(vresname); + /* Same reasoning as above: rely on vres->res staying NULL rather than + * this key_find's own rc to detect "resource not found" -- "slots" (or + * whatever limit->val.resname names) is expected to already exist + * (registered by sched_init_resources() at config load), so a genuine + * miss here IS a real error, just not reliably signaled by rc==0. */ lock(resmgr, RTTI_LOCK_READ); rc = key_find(resmgr, (void*)limit->val.resname, (void**)&(vres->res)); unlock(resmgr); - if (rc < 0) { - return rc; - } - - if (rc == 0) { + if (vres->res == NULL) { return -ENOENT; } diff --git a/src/scheduler/src/plugins/limit.c b/src/scheduler/src/plugins/limit.c index 0067f669..ea45fcc6 100644 --- a/src/scheduler/src/plugins/limit.c +++ b/src/scheduler/src/plugins/limit.c @@ -200,7 +200,21 @@ static int match_job_global_limit_(xce_sched_plugin_t* plugin, return -EINVAL; } - if (is_host_based_limit(&(sched->core.resmgr), limit) > 0) { + /* bug-1787: is_host_based_limit()/instantiate_limit() both take a + * plain `var resmgr` (core.h: `var resmgr;` -- already a handle, not + * a var*), matching how every other resmgr/ugmgr/etc. consumer in + * this codebase (get_group(), is_in_group(), key_find() itself) takes + * its manager by value. Passing &(sched->core.resmgr) here instead + * (as this and the instantiate_limit() call below previously did) + * handed those functions the ADDRESS of the field instead of its + * value -- one level of indirection too many. RTTI's key_find() + * dispatch then rejected that as an invalid object handle: silently + * inside is_host_based_limit() (its own `rc<=0` guard turns ANY + * key_find() failure, including this one, into a uniform -ENOENT -- + * see bug-1786's comment above for why that's not itself a bug), but + * fatally inside instantiate_limit()'s own key_find() call, which + * returned -EINVAL and aborted vres instantiation outright. */ + if (is_host_based_limit(sched->core.resmgr, limit) > 0) { return 0; } @@ -216,18 +230,30 @@ static int match_job_global_limit_(xce_sched_plugin_t* plugin, return rc; } + /* bug-1786: key_find() returning <= 0 here means "not found" (this + * codebase's established kvmgr convention, matching get_group()'s and + * is_host_based_limit()'s own identical rc<=0 checks) -- the FIRST + * time ANY limit ever matches a given user/queue/project combination, + * this is always the case, since no vres has been instantiated for it + * yet. Returning early here (as this code previously did) never gave + * the instantiate_limit() block below a chance to run: vres stays + * uninitialized (calloc'd to NULL at declaration, never reassigned by + * a failed key_find), so `if (vres == NULL)` below is the correct, + * sufficient signal to instantiate -- this key_find's own rc need not + * gate anything further. Before this fix, bug-1784/1785's fixes made + * match_limit() correctly match for the first time in this codebase's + * history, which only then exposed this: a limit could match a job + * but its usage/threshold accounting could never actually initialize, + * so no limit's numeric cap (e.g. limits.yaml's ug1_normal_slots + * value: 8) was ever enforced, only the pass/fail match itself. */ lock(privdata->instances, RTTI_LOCK_READ); rc = key_find(privdata->instances, (void*)vresname, (void*)&vres); unlock(privdata->instances); - if (rc <= 0) { - return rc; - } - FREEUP(vresname); if (vres == NULL) { - rc = instantiate_limit(&vres, &(sched->core.resmgr), limit, obj->user, + rc = instantiate_limit(&vres, sched->core.resmgr, limit, obj->user, obj->queue, obj->project); if (rc < 0) { return rc; @@ -337,7 +363,7 @@ static int match_job_host_limit_(xce_sched_plugin_t* plugin, return 0; } - if (is_host_based_limit(&(sched->core.resmgr), limit) <= 0) { + if (is_host_based_limit(sched->core.resmgr, limit) <= 0) { return 0; } @@ -376,7 +402,7 @@ static int match_job_host_limit_(xce_sched_plugin_t* plugin, FREEUP(vresname); if (vres == NULL) { - rc = instantiate_limit(&vres, &(sched->core.resmgr), limit, obj->user, + rc = instantiate_limit(&vres, sched->core.resmgr, limit, obj->user, obj->queue, obj->project); if (rc < 0) { return rc; diff --git a/src/scheduler/src/scheduler.c b/src/scheduler/src/scheduler.c index d47e9273..39d7b9b2 100644 --- a/src/scheduler/src/scheduler.c +++ b/src/scheduler/src/scheduler.c @@ -11,6 +11,7 @@ #include "client.h" #include "controller.capnp.h" #include "decision.h" +#include "group.h" #include "kvmgr.h" #include "misc.h" #include "plugin.h" @@ -1094,6 +1095,16 @@ static int sched_init_queues(xce_scheduler_t* sched) } int sched_init_config(xce_scheduler_t* sched, xce_conf4sched_t* config) { + /* bug-1784: register the cluster's NAMED usergroups/hostgroups (from + * users.yaml's "usergroups:"/hosts.yaml's "hostgroups:") into + * core.ugmgr/core.hgmgr BEFORE plugin_config_update() below -- a limit + * config that references one of these by name (e.g. a limit scoped to + * users.members=["ug1"]) resolves it via build_consumer()'s own + * makeGroup() call, which can only find an existing named group if it + * was already registered under that name. */ + groupmgr_load_configs(sched->core.ugmgr, config->n_ugroups, config->ugroups); + groupmgr_load_configs(sched->core.hgmgr, config->n_hgroups, config->hgroups); + plugin_config_update(sched, config); sched->core.config = config;