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

feat(scheduler-sim-live-allocation): make limit matching core-safe (task 2.3)

Browse source 
Migrates match_job_global_limit_()/limit_job_election()/
match_job_host_limit_() (plugins/limit.c) to read plugin->core (added
task 1.2) instead of deriving xce_scheduler_t* from plugin->sched,
which stays NULL for a plugin registered via plugin_register_core()
(simulation mode). Removes the dead ctx.sched write flagged during
task 2.1's review -- verified this ctx never has .sched read anywhere
downstream.

Fixes bug-1828 (found while writing this task's own TDD test for a
previously untested code path): match_job_host_limit_() had the same
key_find()-negative-rc-treated-as-fatal defect bug-1786 already fixed
in its global-limit sibling, but never applied here -- meaning
host-scoped resource limits were never actually enforced, in any
mode, until this fix (not merely a display gap; the limit's vres was
never added to obj->hlimits, so sched_fill_limit_requests() always
saw an empty list for it).

TDD: new tests drive the real hook-registered entry points against a
plugin_register_core()-registered plugin, asserting real usage
instances via limit_get_usage_instances() for both global and
host-scoped limits.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
Rongsong Shen 2026-09-18 16:21:25 +00:00
parent ecdea8bf55
commit fc23ea8d31
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
4 changed files with 347 additions and 27 deletions
Download patch file Download diff file Expand all files Collapse all files

85
src/scheduler/src/plugins/limit.c
View file

@ -203,7 +203,14 @@ static int match_job_global_limit_(xce_sched_plugin_t* plugin,
char* vresname = NULL;
xce_vres_t* vres = NULL;
limit_plugin_data_t* privdata = NULL;
xce_scheduler_t* sched = NULL;
/* scheduler-sim-live-allocation spec, task 2.3 (Limit Plugin Core-Safe
* Matching): reads plugin->core (task 1.2's honestly-typed reference,
* set by BOTH plugin_register() and plugin_register_core()) instead of
* plugin->sched (a bare void*, NULL for a plugin registered via
* plugin_register_core() -- i.e. every simulation-mode plugin). This is
* what makes global-limit matching work identically regardless of
* which driver registered the plugin. */
xce_sched_core_t* core = NULL;
if ((limit == NULL) || (plugin == NULL) || (ctx == NULL)) {
return -EINVAL;
@ -214,9 +221,9 @@ static int match_job_global_limit_(xce_sched_plugin_t* plugin,
return -EINVAL;
}
sched = plugin->sched;
core = plugin->core;
privdata = (limit_plugin_data_t*)plugin->data;
if ((privdata == NULL) || (sched == NULL)) {
if ((privdata == NULL) || (core == NULL)) {
return -EINVAL;
}
@ -224,17 +231,17 @@ static int match_job_global_limit_(xce_sched_plugin_t* plugin,
* 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
* its manager by value. Passing &(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) {
if (is_host_based_limit(core->resmgr, limit) > 0) {
return 0;
}
@ -273,7 +280,7 @@ static int match_job_global_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, core->resmgr, limit, obj->user,
obj->queue, obj->project);
if (rc < 0) {
return rc;
@ -288,7 +295,7 @@ static int match_job_global_limit_(xce_sched_plugin_t* plugin,
}
}
rc = update_limit_res_value(&(sched->core.sharedInst), vres);
rc = update_limit_res_value(&(core->sharedInst), vres);
if (rc >= 0) {
rc = xlist_store(&(obj->glimits), vres);
}
@ -300,7 +307,11 @@ static int limit_job_election(void* arg, xce_sched_job_t* job)
{
xce_sched_plugin_t* plugin = (xce_sched_plugin_t*)arg;
limit_plugin_data_t* privdata = (limit_plugin_data_t*)plugin->data;
xce_scheduler_t* sched = (xce_scheduler_t*)plugin->sched;
/* scheduler-sim-live-allocation spec, task 2.3: plugin->core (task 1.2),
* not plugin->sched -- see match_job_global_limit_()'s identical
* comment above for the full rationale. Already the right type
* (xce_sched_core_t*), no cast needed. */
xce_sched_core_t* core = plugin->core;
xce_sched_obj_t* obj = job->schedobj;
int rc = 0;
iterator_t it;
@ -310,13 +321,23 @@ static int limit_job_election(void* arg, xce_sched_job_t* job)
fmt_jobid(jobidString, sizeof(jobidString), job->jobid);
TRACE("%s job %s plugin %s", __func__, jobidString, plugin->name);
if (obj->configver >= sched->core.configver) {
if (obj->configver >= core->configver) {
return 0;
}
reset_sched_obj(obj);
ctx.sched = plugin->sched;
/* scheduler-sim-live-allocation spec, task 2.3: the dead, type-confused
* write this line used to be (`ctx.sched = plugin->sched;`, flagged
* during task 2.1's review) is removed rather than repointed at `core`.
* This `ctx` is a throwaway, function-local value used only to shuttle
* `schedobj` into match_job_global_limit_() below (never passed to
* sched_ctx_set_primary() or any other consumer of ctx->sched) --
* match_job_global_limit_() itself reads ctx->schedobj only and never
* touches ctx->sched, so there is nothing downstream of this ctx that
* would ever read the field either way. Leaving it unset (zero-
* initialized to NULL by `= { 0 }` above) is honest about that, rather
* than assigning a value for a reader that doesn't exist. */
ctx.worker = NULL;
ctx.schedobj = job->schedobj;
@ -345,7 +366,7 @@ static int limit_job_election(void* arg, xce_sched_job_t* job)
iter_destroy(&it);
obj->configver = sched->core.configver;
obj->configver = core->configver;
return rc;
}
@ -358,7 +379,10 @@ static int match_job_host_limit_(xce_sched_plugin_t* plugin,
char* vresname = NULL;
xce_vres_t* vres = NULL;
limit_plugin_data_t* privdata = NULL;
xce_scheduler_t* sched = NULL;
/* scheduler-sim-live-allocation spec, task 2.3: plugin->core, not
* plugin->sched -- see match_job_global_limit_()'s identical comment
* above for the full rationale. */
xce_sched_core_t* core = NULL;
xce_sched_worker_t* worker = NULL;
xlist_t* hlimits = NULL;
@ -371,11 +395,11 @@ static int match_job_host_limit_(xce_sched_plugin_t* plugin,
return -EINVAL;
}
sched = plugin->sched;
core = plugin->core;
privdata = (limit_plugin_data_t*)plugin->data;
worker = ctx->worker;
if ((sched == NULL) || (privdata == NULL)) {
if ((core == NULL) || (privdata == NULL)) {
return -EINVAL;
}
@ -383,7 +407,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(core->resmgr, limit) <= 0) {
return 0;
}
@ -412,17 +436,26 @@ static int match_job_host_limit_(xce_sched_plugin_t* plugin,
return rc;
}
/* bug-1828 (scheduler-sim-live-allocation spec, task 2.3, discovered
* while writing this task's own required TDD test -- the host-scoped
* sibling of bug-1786, never fixed alongside it): key_find() signals
* "not found" as a NEGATIVE rc (-ENOENT), not 0, on the FIRST-EVER
* lookup of a given vresname -- exactly the case here, since no vres
* has been instantiated for it yet. The `if (rc < 0) { return rc; }`
* this replaces unconditionally caught that expected miss and returned
* before the `if (vres == NULL)` instantiate_limit() block below could
* ever run, so no host-scoped limit's usage instance was ever created.
* `vres` (calloc'd NULL, only ever reassigned by a successful
* key_find) is already the correct, sufficient signal -- mirrors
* match_job_global_limit_()'s own identical fix (bug-1786) exactly. */
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, core->resmgr, limit, obj->user,
obj->queue, obj->project);
if (rc < 0) {
return rc;

242
src/scheduler/testsuite/test_repl_limit_table.c
View file

@ -2570,6 +2570,244 @@ test_sim_bootstrap_limit_plugin_registers_without_crash_when_config_null(
"config_null: PASS\n");
}
/* ---------------------------------------------------------------------- */
/* scheduler-sim-live-allocation spec, task 2.3 (Limit Plugin Core-Safe */
/* Matching, Req 2.1/2.2): the sim_bootstrap_limit_plugin() tests above */
/* prove REGISTRATION and CONFIG-SEEDING work against a plugin_register_ */
/* core()-registered plugin (plugin->sched left NULL by design). Neither */
/* proves MATCHING works -- limit_job_election()/match_job_global_limit_()*/
/* and limit_prepare_allocation()/match_job_host_limit_() have always */
/* derived their own `xce_scheduler_t* sched` from `plugin->sched`, so a */
/* sim-registered plugin (sched == NULL) either NULL-derefs (global path, */
/* limit_job_election()'s own `sched->core.configver` read, no prior NULL */
/* guard) or returns -EINVAL without matching anything (host-scoped path, */
/* match_job_host_limit_()'s own `sched == NULL` guard) -- both are the */
/* "genuinely never invoked/no-op in sim mode" gap bug-1826/this task's own*/
/* brief describe. These two tests drive the real hook-registered entry */
/* points directly (the same (void* arg, ...) shape plugin_job_election()/ */
/* plugin_prepare_allocation() -- both already core-safe, task 1.2/2.2 -- */
/* dispatch through in production) against a plugin registered ONLY via */
/* plugin_register_core(), proving matching now reads plugin->core (task */
/* 1.2's honest core-state reference) instead of plugin->sched. */
/* ---------------------------------------------------------------------- */
/*
* (1) Global limit match (Req 2.1): a limit configured on a plugin
* registered via plugin_register_core() must still match a real job and
* register its usage instance, verifiable through limit_get_usage_
* instances() -- the exact "Observable" this task's brief specifies.
* `obj.user`/`.queue`/`.project` are non-NULL empty strings (never NULL --
* match_limit()/is_in_group() both reject NULL outright), matching against
* the limit's default "all"-group fallback (add_configured_limit()'s own
* config shape, seeded here via limit_seed_config() through sim_bootstrap_
* limit_plugin() instead, since plugin->sched is NULL for this plugin).
*/
static void
test_limit_job_election_matches_global_limit_via_sim_registered_plugin(void)
{
xce_sched_sim_driver_t sim;
xce_sched_plugin_t* plugin = NULL;
xce_limit_config_t* limits[1];
xce_conf4sched_t config;
xce_sched_obj_t obj;
xce_sched_job_t job;
var instances_handle;
iterator_t it;
void* item = NULL;
int rc;
memset(&sim, 0, sizeof(sim));
rc = xce_sched_core_init(&(sim.core));
TEST_ASSERT_EQUAL_INT(0, rc);
/* instantiate_limit() (xcelimit.c) requires the limit's underlying
* resource (val.resname, "mem" -- make_limit_config()'s own hardcoded
* shape) to genuinely exist in core->resmgr (matching production, where
* sched_init_resources() registers it at config load) -- every other
* test in this file that drives real matching (not just registration/
* config-seeding) does the same via register_per_host_resource(). Its
* non-GLOBAL/non-SHARED flags are harmless here: this limit's own
* percent=0/per_host=0 shape (make_limit_config()) still resolves as a
* GLOBAL limit via is_host_based_limit()'s own fallback rule, matching
* instantiate_and_register_global_vres()'s identical comment above. */
register_per_host_resource(&sim, "mem");
limits[0] = make_limit_config("sim_global_election_limit");
memset(&config, 0, sizeof(config));
config.nlimits = 1;
config.limits = limits;
sim.core.config = &config;
/* Real production bootstrap: plugin_register_core() + limit_seed_
* config(), exactly as sched_sim_driver_run() calls it (sim_limit.c). */
sim_bootstrap_limit_plugin(&sim);
rc = key_find(sim.core.plugins, "xce_sched_limit", (void**)&plugin);
TEST_ASSERT_TRUE(rc >= 0);
TEST_ASSERT_NOT_NULL(plugin);
TEST_ASSERT_NULL_MESSAGE(
plugin->sched,
"sanity: plugin_register_core() must leave plugin->sched NULL -- "
"this test proves matching works WITHOUT it, via plugin->core");
TEST_ASSERT_EQUAL_PTR_MESSAGE(
&(sim.core), plugin->core,
"sanity: plugin->core (task 1.2) must reference this real sim "
"core");
/* limit_job_election() skips any schedobj whose own configver is
* already >= core->configver (the "already evaluated against this
* config" guard, core_events.c's own config-push bump being the
* production trigger) -- both default to 0 (xce_sched_core_init() /
* this memset()), so bump core->configver past the fresh obj's 0 here,
* exactly as a real CONFIG_PUSH event would before this job is ever
* offered to the picker. */
sim.core.configver = 1;
memset(&obj, 0, sizeof(obj));
obj.user = (char*)"";
obj.queue = (char*)"";
obj.project = (char*)"";
xlist_init(&(obj.glimits), 8);
zarray_init(&(obj.hlimits), 8, NULL);
memset(&job, 0, sizeof(job));
job.jobid = 1;
job.schedobj = &obj;
/* The real job_election_hook-registered entry point, called exactly
* the way the now-core-safe plugin_job_election() dispatcher (plugin.c,
* task 1.2) calls it in production -- `arg` is the plugin instance. */
rc = limit_job_election((void*)plugin, &job);
TEST_ASSERT_TRUE_MESSAGE(
rc >= 0,
"limit_job_election() must succeed for a plugin registered via "
"plugin_register_core(), reading plugin->core instead of the "
"always-NULL-here plugin->sched");
instances_handle = limit_get_usage_instances(&(sim.core));
TEST_ASSERT_NOT_NULL(instances_handle);
rc = iter_make(&it, instances_handle);
TEST_ASSERT_TRUE(rc >= 0);
rc = iter_next(&it, &item);
TEST_ASSERT_TRUE(rc >= 0);
TEST_ASSERT_NOT_EQUAL(RTTI_ITER_NO_DATA, rc);
TEST_ASSERT_NOT_NULL_MESSAGE(
item,
"a global limit match via the simulation-mode-registered plugin "
"must create a real usage instance, where today it silently never "
"matches at all (plugin->sched is NULL for a plugin_register_"
"core()-registered plugin)");
iter_destroy(&it);
free_limit_config(limits[0]);
printf(
"test_limit_job_election_matches_global_limit_via_sim_registered_"
"plugin: PASS\n");
}
/*
* (2) Host-scoped limit match (Req 2.2): same proof as (1), but for the
* prepare_allocation_hook path -- a percent-based limit on a real,
* non-GLOBAL/non-SHARED per-host resource (register_per_host_resource(),
* the exact shape add_configured_percent_limit() uses for the normal-mode
* equivalent of this scenario), matched via limit_prepare_allocation()
* against a real xce_sched_ctx_t/xce_sched_worker_t, on a plugin registered
* ONLY via plugin_register_core() (plugin->sched NULL).
*/
static void
test_limit_prepare_allocation_matches_host_limit_via_sim_registered_plugin(
void)
{
xce_sched_sim_driver_t sim;
xce_sched_plugin_t* plugin = NULL;
xce_limit_val_t val = { .resname = (char*)"sim_host_res",
.percent = 1,
.val = 50.0 };
xce_limit_config_t limit_cfg = { .name = (char*)"sim_host_limit",
.val = &val };
xce_limit_config_t* limits[1] = { &limit_cfg };
xce_conf4sched_t config;
xce_sched_obj_t obj;
xce_sched_worker_t worker;
xce_sched_ctx_t ctx;
var instances_handle;
iterator_t it;
void* item = NULL;
int rc;
memset(&sim, 0, sizeof(sim));
rc = xce_sched_core_init(&(sim.core));
TEST_ASSERT_EQUAL_INT(0, rc);
register_per_host_resource(&sim, "sim_host_res");
memset(&config, 0, sizeof(config));
config.nlimits = 1;
config.limits = limits;
sim.core.config = &config;
sim_bootstrap_limit_plugin(&sim);
rc = key_find(sim.core.plugins, "xce_sched_limit", (void**)&plugin);
TEST_ASSERT_TRUE(rc >= 0);
TEST_ASSERT_NOT_NULL(plugin);
TEST_ASSERT_NULL_MESSAGE(
plugin->sched,
"sanity: plugin_register_core() must leave plugin->sched NULL");
memset(&obj, 0, sizeof(obj));
obj.user = (char*)"";
obj.queue = (char*)"";
obj.project = (char*)"";
xlist_init(&(obj.glimits), 8);
zarray_init(&(obj.hlimits), 8, NULL);
memset(&worker, 0, sizeof(worker));
worker.name = (char*)"sim_host1";
worker.idx = 0;
memset(&ctx, 0, sizeof(ctx));
ctx.worker = &worker;
ctx.schedobj = &obj;
/* The real prepare_allocation_hook-registered entry point, called
* exactly the way the now-core-safe plugin_prepare_allocation()
* dispatcher (plugin.c, task 1.2) and sched_ctx_set_primary()
* (schedobj.c, task 2.1) call it in production. */
rc = limit_prepare_allocation((void*)plugin, &ctx);
TEST_ASSERT_TRUE_MESSAGE(
rc >= 0,
"limit_prepare_allocation() must succeed for a plugin registered "
"via plugin_register_core(), reading plugin->core instead of the "
"always-NULL-here plugin->sched");
instances_handle = limit_get_usage_instances(&(sim.core));
TEST_ASSERT_NOT_NULL(instances_handle);
rc = iter_make(&it, instances_handle);
TEST_ASSERT_TRUE(rc >= 0);
rc = iter_next(&it, &item);
TEST_ASSERT_TRUE(rc >= 0);
TEST_ASSERT_NOT_EQUAL(RTTI_ITER_NO_DATA, rc);
TEST_ASSERT_NOT_NULL_MESSAGE(
item,
"a host-scoped limit match via the simulation-mode-registered "
"plugin must create a real usage instance, where today it either "
"no-ops (-EINVAL from match_job_host_limit_()'s own sched==NULL "
"guard) or never matches at all");
iter_destroy(&it);
printf(
"test_limit_prepare_allocation_matches_host_limit_via_sim_"
"registered_plugin: PASS\n");
}
int main(void)
{
UNITY_BEGIN();
@ -2624,5 +2862,9 @@ int main(void)
test_sim_bootstrap_limit_plugin_seeds_real_config_limit_when_config_present);
RUN_TEST(
test_sim_bootstrap_limit_plugin_registers_without_crash_when_config_null);
RUN_TEST(
test_limit_job_election_matches_global_limit_via_sim_registered_plugin);
RUN_TEST(
test_limit_prepare_allocation_matches_host_limit_via_sim_registered_plugin);
return UNITY_END();
}