diff --git a/.kiro/specs/scheduler-allocation/tasks.md b/.kiro/specs/scheduler-allocation/tasks.md index c49d8ee1..52ea48c9 100644 --- a/.kiro/specs/scheduler-allocation/tasks.md +++ b/.kiro/specs/scheduler-allocation/tasks.md @@ -16,7 +16,7 @@ - Add an allocation-mode field to the per-attempt allocation context, defaulting to normal-resources-only - Observable: the field exists, defaults correctly, and no existing allocation code path branches on any value other than the default - _Requirements: 9.5_ -- [ ] 1.4 Add the per-host resource-debit reversal primitive +- [x] 1.4 Add the per-host resource-debit reversal primitive - Add a helper that reverses exactly the provisional resource debit an earlier successful host-claim made, using the existing subtraction primitive - Write a unit test that debits a host's provisional usage, reverses it, and confirms the host's provisional usage returns to its pre-debit value - Observable: a passing test shows debit-then-reverse leaves provisional usage unchanged from baseline @@ -106,5 +106,6 @@ ## Implementation Notes +- (1.4) When a new scheduler test needs a real function from `schedobj.c`/`core.c` (e.g. `sched_ctx_get_uncomit_host()`), white-box `#include` the real source file rather than privately reimplementing its logic in the test — established precedent: `test_picker_config_fields.c`'s `#include "../src/core.c"` + `#include "../src/schedobj.c"` pairing. A private reimplementation may pass today but silently drifts from the real function with nothing to catch it later. - (1.1) `xce_host_iter_t.next()` returns `xce_sched_worker_t*` (`sched/types.h`, carries `resvals`/`resInUse`/`host_eval_ctx`) — never `xce_worker_t*` (`xce/types.h`, the wire/config type `make_sched_worker()` converts FROM). requirements.md/design.md briefly had this wrong; fixed in commit `d1a3e158`. Task 2.1 (`host_iter.c`'s real implementation) and any later task touching worker candidates must use `xce_sched_worker_t*` from the start. - (1.2) `resreq_fp`'s ingest-time population lives in `core_build_primary_schedobj()` (`src/scheduler/src/core_events.c`), right after `primary->resreq = resreq;` — confirmed wired (min_tasks/max_tasks/select_node_id/rusage_hash all set from the parsed `xce_primary_resreq_t`) as of task 1.2 round 2, regression-covered by `test_core_build_primary_schedobj_populates_resreq_fp_from_parsed_resreq` in `src/scheduler/testsuite/test_core_events.c`. Round 1 added the type/field/hash machinery but never wired this call site, leaving `resreq_fp` permanently zeroed — do not rediscover this gap in a later task. diff --git a/src/scheduler/include/sched/types.h b/src/scheduler/include/sched/types.h index 91934968..fe74cdb3 100644 --- a/src/scheduler/include/sched/types.h +++ b/src/scheduler/include/sched/types.h @@ -198,6 +198,7 @@ extern void sched_ctx_fini(xce_sched_ctx_t*); extern void sched_ctx_set_primary(xce_sched_ctx_t*, xce_primary_resreq_t*); extern int sched_ctx_get_uncomit_shared(xvec_t**, xce_sched_ctx_t*); extern int sched_ctx_get_uncomit_host(xvec_t**, xce_sched_ctx_t*, int); +extern int sched_ctx_uncommit_host(xce_sched_ctx_t*, int, xvec_idval_t*); extern void reset_sched_obj(xce_sched_obj_t*); #endif diff --git a/src/scheduler/src/resource.c b/src/scheduler/src/resource.c index f9c4fb87..aa13aeb6 100644 --- a/src/scheduler/src/resource.c +++ b/src/scheduler/src/resource.c @@ -557,6 +557,44 @@ int sched_try_alloc_on_hosts(xce_worker_group_t* g, va_list ap) return sched_try_alloc_on_host_(w, ap); } +/* + * sched_ctx_uncommit_host - reverses exactly one provisional per-host + * resource debit a prior, successful sched_try_alloc_on_host_() call made + * for host `idx`, undoing the XVEC_ADD_FLOAT commit + * (`xvec_idval(uncomit, &(ctx->alloc_req.host), XVEC_ADD_FLOAT)`, above) + * with the exact inverse operation against the SAME per-host uncommitted + * vector (`ctx->uncomitted.hosts[idx]`, via sched_ctx_get_uncomit_host() -- + * the same get-or-create accessor the commit path itself uses, so a + * reversal always lands on the identical xvec_t instance the debit did). + * + * `committed` must be the exact xvec_idval_t snapshot the earlier commit + * added (e.g. the `claimed[]` entry's saved `alloc_req.host`, per + * requirements.md's "Rollback scope" -- ctx->alloc_req is reset and + * repopulated for every subsequent host tried, so callers must retain + * their own copy rather than reading ctx->alloc_req.host after the fact). + * + * Scoped entirely to ctx->uncomitted (per-job-allocation-attempt scratch + * state) -- never touches the real, session-wide resInUse/sharedInUse + * books (see resource-accounting.md / requirements.md's "Rollback scope"). + * + * Returns the same contract as xvec_idval(): >=0 (count of entries + * applied) on success, negative errno on failure (e.g. -EINVAL if ctx/idx + * are invalid, propagated from sched_ctx_get_uncomit_host()). + */ +int sched_ctx_uncommit_host(xce_sched_ctx_t* ctx, int idx, + xvec_idval_t* committed) +{ + xvec_t* uncomit = NULL; + int rc; + + rc = sched_ctx_get_uncomit_host(&uncomit, ctx, idx); + if (rc < 0) { + return rc; + } + + return xvec_idval(uncomit, committed, XVEC_SUB_FLOAT); +} + static int sched_update_account(xce_sched_core_t* core, xce_job_account_t* account, int charge) { diff --git a/src/scheduler/testsuite/test_resource_accounting.c b/src/scheduler/testsuite/test_resource_accounting.c index 8e62d614..4fb39573 100644 --- a/src/scheduler/testsuite/test_resource_accounting.c +++ b/src/scheduler/testsuite/test_resource_accounting.c @@ -45,19 +45,31 @@ * * White-box #include of ../src/core.c (for the real * xce_sched_core_init()/sched_add_worker()/sched_get_worker() the fixture - * needs) and ../src/resource.c (the functions under test). Neither file - * needs core_clock.c/core_events.c/core_session.c/decision.c/plugin.c -- - * this test never calls xce_core_push_events()/xce_core_begin_session()/ - * anything decision- or plugin-registration-related, only the plain - * add-worker/charge-account entry points core.c/resource.c themselves - * define. Stubs below (kill_sched_* family, plugin_unregister) are the - * standard link-satisfying no-ops xce_sched_core_init()'s kvmgr datakillers - * reference (never actually invoked -- this fixture is never torn down, - * same convention as test_driver_session_ordering.c/ - * test_snapshot_save_load.c's identical stub set). - * sched_worker_release_host_ctx() is the one real (non-stub) symbol this - * pulls from resource.c itself, not from core_session.c -- unused here - * (this test never calls it), listed only because resource.c defines it. + * needs), ../src/resource.c (the functions under test), and + * ../src/schedobj.c (for the real sched_ctx_get_uncomit_host() that the + * new sched_ctx_uncommit_host() under test itself calls -- white-box + * #included here rather than reimplemented, the same convention + * test_picker_config_fields.c's own #include "../src/core.c" + + * #include "../src/schedobj.c" pairing already established in this + * directory "so it exercises the real functions, not a re-implementation + * of any of them"). None of the three needs core_clock.c/core_events.c/ + * core_session.c/decision.c/plugin.c -- this test never calls + * xce_core_push_events()/xce_core_begin_session()/anything decision- or + * plugin-registration-related, only the plain add-worker/charge-account/ + * uncommit-host entry points core.c/resource.c/schedobj.c themselves + * define. The kill_sched_* family (the kvmgr datakillers + * xce_sched_core_init() references) is now the REAL implementation + * pulled in from schedobj.c, not a stub -- only plugin_unregister remains + * a link-satisfying no-op (plugin.c is still not white-box #included). + * sched_worker_release_host_ctx() is a real (non-stub) symbol this pulls + * from resource.c itself, not from core_session.c -- unused here (this + * test never calls it), listed only because resource.c defines it. + * plugin_job_update()/plugin_queue_update()/plugin_job_rusage_update() + * below are new link-satisfying stubs for schedobj.c's OWN unrelated + * functions (sched_update_existing_queue()/sched_update_existing_job()/ + * sched_apply_job_rusage()) that reference them (plugin.c, still not + * included) -- never actually invoked, since this file never calls any + * of those three schedobj.c functions. */ #include #include @@ -70,28 +82,26 @@ #include "sim.h" /* Pull in the real xce_sched_core_init()/sched_add_worker()/ - * sched_get_worker() (core.c) and sched_charge_account()/ - * sched_uncharge_account()/sched_commit_job_alloc()/get_resvals() (resource.c, - * the functions under test) into this TU (white-box #include, same - * convention as every other file in this directory). */ + * sched_get_worker() (core.c), sched_charge_account()/ + * sched_uncharge_account()/sched_commit_job_alloc()/get_resvals()/ + * sched_ctx_uncommit_host() (resource.c, the functions under test), and + * sched_ctx_get_uncomit_host() (schedobj.c) into this TU (white-box + * #include, same convention as every other file in this directory -- + * see this file's own header comment for why schedobj.c specifically). */ #include "../src/core.c" #include "../src/resource.c" +#include "../src/schedobj.c" -void kill_sched_job(void* arg) { (void)arg; } -void kill_sched_queue(void* arg) { (void)arg; } -void kill_sched_worker(void* arg) { (void)arg; } -void kill_sched_user(void* arg) { (void)arg; } -void kill_sched_project(void* arg) { (void)arg; } -void kill_sched_resval(xce_val_t* val) { (void)val; } void plugin_unregister(void* arg) { (void)arg; } -/* Link-satisfying stubs for resource.c's sched_try_alloc_on_host_() (its - * OWN callees, plugin_prepare_allocation()/sched_ctx_get_uncomit_host(), - * live in plugin.c/schedobj.c -- neither white-box #included here). Never - * actually invoked: this file never calls sched_try_alloc_on_hosts()/ +/* Link-satisfying stub for resource.c's sched_try_alloc_on_host_() (its + * OWN callee plugin_prepare_allocation() lives in plugin.c, not white-box + * #included here; its other callee, sched_ctx_get_uncomit_host(), is now + * the real function pulled in from schedobj.c above). Never actually + * invoked: this file never calls sched_try_alloc_on_hosts()/ * sched_try_alloc_on_host_(), only sched_charge_account()/ - * sched_uncharge_account()/sched_commit_job_alloc(). Same two stubs - * test.c's own #include "../src/resource.c" already establishes needing. */ + * sched_uncharge_account()/sched_commit_job_alloc(). Same stub test.c's + * own #include "../src/resource.c" already establishes needing. */ int plugin_prepare_allocation(void* s, xce_sched_ctx_t* c) { (void)s; @@ -99,11 +109,19 @@ int plugin_prepare_allocation(void* s, xce_sched_ctx_t* c) return 0; } -int sched_ctx_get_uncomit_host(xvec_t** v, xce_sched_ctx_t* c, int i) +/* Link-satisfying stubs for schedobj.c's OWN unrelated functions + * (sched_update_existing_queue()/sched_update_existing_job()/ + * sched_apply_job_rusage()) that reference plugin_job_update()/ + * plugin_queue_update()/plugin_job_rusage_update() (plugin.c, not + * white-box #included here). Never actually invoked: this file never + * calls any of those three schedobj.c functions. */ +int plugin_job_update(void* s, xce_sched_job_t* j) { (void)s; (void)j; return 0; } +int plugin_queue_update(void* s, xce_sched_queue_t* q) { (void)s; (void)q; return 0; } +int plugin_job_rusage_update(void* s, xce_sched_job_t* j, xce_job_rusages_t* r) { - (void)v; - (void)c; - (void)i; + (void)s; + (void)j; + (void)r; return 0; } @@ -720,6 +738,84 @@ static void test_reconcile_job_resource_usage_null_args_return_einval(void) sched_reconcile_job_resource_usage(NULL, NULL, NULL)); } +/* + * test_ctx_uncommit_host_reverses_provisional_debit - Requirement 4.2 + * (scheduler-allocation spec, task 1.4): sched_ctx_uncommit_host() + * (resource.c, new) reverses exactly the provisional per-host resource + * debit an earlier successful host-claim made. + * + * Debits ctx->uncomitted.hosts[idx] directly via xvec_idval(..., + * XVEC_ADD_FLOAT) -- the exact call sched_try_alloc_on_host_() makes on a + * successful host claim (resource.c line ~544) -- rather than driving the + * full plugin_prepare_allocation()/sched_check_host_satisfy() chain, which + * needs a real worker/select-expression/resmgr fixture unrelated to what + * this rollback primitive itself does (this file's own convention: see + * make_test_account()'s residx-only shortcut above for the same rationale). + * Then calls the new sched_ctx_uncommit_host() and asserts the per-host + * uncommitted vector is back to its pre-debit (zero) baseline -- proving + * debit-then-reverse leaves provisional usage unchanged, this task's + * Observable. + * + * sched_ctx_get_uncomit_host() (the baseline/re-read get-or-create + * accessor called below) and host_uncommit_killer() (the zarray item + * killer passed to zarray_init() below) are both the REAL functions + * white-box #included from schedobj.c above, not reimplementations -- + * host_uncommit_killer() is `static` in schedobj.c, but the #include + * pastes it into this same translation unit, so it resolves normally. + * This is the same "exercise the real functions, not a re-implementation + * of any of them" precedent test_picker_config_fields.c already + * established for combining core.c + schedobj.c in one white-box test. + */ +#define UNCOMMIT_HOST_IDX 3 +#define UNCOMMIT_RESIDX 0 +#define UNCOMMIT_DEBIT 2.5f + +static void test_ctx_uncommit_host_reverses_provisional_debit(void) +{ + xce_sched_ctx_t ctx; + xvec_t* uncomit = NULL; + xvec_idval_t committed; + float* data; + int rc; + + memset(&ctx, 0, sizeof(ctx)); + rc = zarray_init(&(ctx.uncomitted.hosts), 8, + (zarray_item_killer_t)host_uncommit_killer); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = xvec_idval_init(&committed, 4); + TEST_ASSERT_EQUAL_INT(0, rc); + rc = xvec_idval_store(&committed, UNCOMMIT_RESIDX, UNCOMMIT_DEBIT); + TEST_ASSERT_TRUE(rc >= 0); + + /* Baseline: a freshly created per-host uncommitted vector reads 0. */ + rc = sched_ctx_get_uncomit_host(&uncomit, &ctx, UNCOMMIT_HOST_IDX); + TEST_ASSERT_EQUAL_INT(1, rc); + TEST_ASSERT_NOT_NULL(uncomit); + data = (float*)uncomit->data; + TEST_ASSERT_EQUAL_FLOAT(0.0f, data[UNCOMMIT_RESIDX]); + + /* Debit: the exact commit call sched_try_alloc_on_host_() makes on a + * successful host claim. */ + rc = xvec_idval(uncomit, &committed, XVEC_ADD_FLOAT); + TEST_ASSERT_EQUAL_INT(committed.count, rc); + TEST_ASSERT_EQUAL_FLOAT(UNCOMMIT_DEBIT, data[UNCOMMIT_RESIDX]); + + /* Reverse: the new primitive under test. */ + rc = sched_ctx_uncommit_host(&ctx, UNCOMMIT_HOST_IDX, &committed); + TEST_ASSERT_EQUAL_INT(committed.count, rc); + + /* Back to the pre-debit baseline -- same vector instance, re-read. */ + TEST_ASSERT_EQUAL_FLOAT(0.0f, data[UNCOMMIT_RESIDX]); + + /* Same per-host vector instance is returned again on a second lookup + * (get-or-create semantics unchanged by the reversal). */ + uncomit = NULL; + rc = sched_ctx_get_uncomit_host(&uncomit, &ctx, UNCOMMIT_HOST_IDX); + TEST_ASSERT_EQUAL_INT(1, rc); + TEST_ASSERT_EQUAL_FLOAT(0.0f, ((float*)uncomit->data)[UNCOMMIT_RESIDX]); +} + int main(void) { UNITY_BEGIN(); @@ -737,5 +833,6 @@ int main(void) RUN_TEST(test_reconcile_job_resource_usage_charges_unreserved_job_full_usage); RUN_TEST(test_reconcile_job_resource_usage_noop_when_mem_not_a_registered_resource); RUN_TEST(test_reconcile_job_resource_usage_null_args_return_einval); + RUN_TEST(test_ctx_uncommit_host_reverses_provisional_debit); return UNITY_END(); }