test(scheduler-dispatch-commit): bug-1624 multi-round regression coverage (task 5.1)
Extends task 2.1's test_scheduler_dispatch_commit.c with two tests proving resource-in-use accounting survives more than a single round boundary between dispatch and worker confirmation, per task 5.1's "several rounds" / "matching a same-round confirmation" language: - test_dispatch_commit_survives_several_rounds_before_confirmation: loops 5 round-boundary cycles, asserting resInUse after EVERY round, not just once. - test_dispatch_commit_multi_round_gap_matches_same_round_confirmation: compares resInUse after 1 round vs after 5 rounds and asserts equality -- a same-round confirmation is modeled as nrounds==1 since design.md documents re-applying JOB_STAT_RUN to an already-RUN job as a confirmed no-op, so resInUse depends only on job->state, never on confirmation timing. Test-only; scheduler.c is untouched. RED phase independently verified by both implementer and reviewer via an actual revert-rebuild-rerun cycle (4/5 tests fail without the task 2.1 fix). Independently reviewed and approved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cxa1PmqAB5PyXfJaNvj8TN
This commit is contained in:
parent
7b3dfed56e
commit
01c8306694
2 changed files with 188 additions and 1 deletions
|
|
@ -71,7 +71,7 @@
|
|||
- _Depends: 3.1, 3.2, 3.3_
|
||||
|
||||
- [ ] 5. Validation: end-to-end behavior and regression
|
||||
- [ ] 5.1 End-to-end test: resource-in-use accounting survives a multi-round gap between dispatch and confirmation
|
||||
- [x] 5.1 End-to-end test: resource-in-use accounting survives a multi-round gap between dispatch and confirmation
|
||||
- Reproduce bug-1624 directly: dispatch a job, let several rounds pass before the worker confirms it started, and confirm resource-in-use totals reflect the job continuously from the round it was dispatched, matching what a same-round confirmation would have produced
|
||||
- Observable completion: the regression scenario passes, and fails against the pre-change code path if temporarily reverted
|
||||
- _Requirements: 1.1, 1.2, 1.3, 2.1, 2.3_
|
||||
|
|
@ -114,3 +114,5 @@
|
|||
- Task 3.3: the bound constant is `XCE_DISPATCH_FAIL_RETRY_BOUND = 3` (design.md's stated default), defined once near job.c's top and used at all four retryable sites — no magic numbers. 3 consecutive failures still retry; the 4th resolves terminal.
|
||||
- Task 4.1: **`test_commit_barrier_reply.c`'s `test_jobdispatch_receipt_ack_no_wal_append` is now stale for a SECOND, independent reason**, on top of task 3.1's already-documented WAL-append-count staleness: its 2-job batch (one real-worker success, one `nhosts==0` terminal failure — no retryable job) now produces ZERO `notify_dispatch()` calls total post-task-4.1 (the success path never called it even before task 4.1, per task 3.1's design; the terminal path resolves via `mark_job_fail()`'s `schedFailJob` hook chain, not `notify_dispatch()`/`jobDispatchFail`, per task 3.2's design) — breaking its pinned `g_notify_count == 2` / both-`jobDispatchFail`-event assertions (lines ~1991-2003). **Task 5.4 must update BOTH the WAL-count assertion (task 3.1's finding) and the notify-count/event assertions (this finding) in the same test** before claiming full-suite regression-clean; the test's own scenario (1 success + 1 terminal, zero retryable jobs) may need to change too since it can no longer meaningfully exercise `notify_dispatch()` at all with today's cast of outcomes — consider adding a retryable-classified job to the batch if the intent is still to prove `notify_dispatch()`'s wiring through `handle_jobdispatch()`.
|
||||
- Task 4.1: confirmed `handle_jobdispatch()`'s only caller (`cluster.c:471`, inside `myreq_handler`) uses its return value only as the generic RPC-handler `rc`, and `handle_jobdispatch()` itself always returns 0 unconditionally regardless of any job's outcome — nothing depends on the removed per-job notification behavior at the caller level.
|
||||
- Task 5.1: a "same-round confirmation" scenario for resource-in-use accounting is legitimately modeled as `nrounds==1` rather than by simulating a real `schedStartJob` notify event — design.md documents (and the reviewer independently re-verified against `core_events.c`) that re-applying `JOB_STAT_RUN` to an already-RUN job via the confirmation path is a confirmed no-op, so `resInUse` depends solely on `JOB_IS_RUN(job->state)`, never on a separate confirmation flag. Reviewer's own finding: the resulting same-round-vs-multi-round comparison test is honest and correctly fails on revert, but adds little independent detection power beyond a per-round-asserting loop test (which is the one genuinely load-bearing proof) — worth knowing if a future task considers deduplicating these two tests, but not something to act on now.
|
||||
- Task 5.1: no controller-side test was added for this task — Requirements 2.1/2.3 (controller transitions to RUN independent of confirmation) are already covered by task 3.1's `test_dc_success_commits_alloc_state_and_resets_counter`. No live scheduler+controller wire-protocol integration harness exists anywhere in this codebase; building one was judged disproportionate to what this task verifies, and the reviewer agreed.
|
||||
|
|
|
|||
|
|
@ -19,6 +19,24 @@
|
|||
* threads), so it needs none of their `-Wl,--wrap=xcecli_*` interposition:
|
||||
* sched_commit_decision() itself only touches sched->core and the (empty,
|
||||
* in this fixture) plugin-hook chain, never sched->apictx.
|
||||
*
|
||||
* Extended for task 5.1 (Req 1.1, 1.2, 1.3, 2.1, 2.3 -- bug-1624's
|
||||
* end-to-end regression test): task 2.1's own single-round-boundary test
|
||||
* above proves the core mechanism; the two tests added at the bottom of
|
||||
* this file extend that proof to (a) GAP_ROUNDS=5 consecutive round
|
||||
* boundaries rather than just one, and (b) an explicit value comparison
|
||||
* between a "confirmed same round" scenario and a "confirmed after several
|
||||
* rounds" scenario, rather than only comparing each to a hardcoded
|
||||
* constant -- see run_dispatch_then_n_rounds()'s own comment for why no
|
||||
* live controller connection or simulated schedStartJob notify event is
|
||||
* needed to make that comparison meaningful: the controller's own role in
|
||||
* this flow (transitioning to RUN, notifying) is already independently
|
||||
* covered by tasks 3.1-4.1's controller-side suites
|
||||
* (test_dispatch_commit.c, test_dispatch_fail_terminal.c,
|
||||
* test_dispatch_fail_retry.c, test_jobdispatch_no_double_notify.c); nothing
|
||||
* scheduler-side that determines resInUse ever reads a confirmation event
|
||||
* at all, only job->state, which task 2.1's fix already sets synchronously
|
||||
* at dispatch-commit.
|
||||
*/
|
||||
#include <errno.h>
|
||||
#include <stdlib.h>
|
||||
|
|
@ -34,6 +52,7 @@
|
|||
#define JOB_ID 4242ULL
|
||||
#define RES_NAME "slots"
|
||||
#define WANT_VALUE 4.0f
|
||||
#define GAP_ROUNDS 5
|
||||
|
||||
static xce_sched_worker_t* add_worker(xce_sched_core_t* core, char* name)
|
||||
{
|
||||
|
|
@ -253,11 +272,177 @@ static void test_pend_decision_does_not_touch_job_state(void)
|
|||
TEST_ASSERT_NULL(job->accounts);
|
||||
}
|
||||
|
||||
/*
|
||||
* Task 5.1 (bug-1624's own regression test, "several rounds" language):
|
||||
* extends the single-round-boundary proof above
|
||||
* (test_dispatch_commit_survives_round_boundary_before_confirmation, task
|
||||
* 2.1) to GAP_ROUNDS consecutive round boundaries -- zeroing resInUse and
|
||||
* calling sched_account_jobs() GAP_ROUNDS times in a row, exactly as
|
||||
* GAP_ROUNDS real driver rounds would via xce_core_begin_session(), with no
|
||||
* worker-confirmation event injected anywhere in the loop -- and asserts
|
||||
* resInUse is restored to WANT_VALUE after EVERY one of them, not merely
|
||||
* that it eventually comes back after one round. Before task 2.1/3.1's fix,
|
||||
* job->state would still read JOB_STAT_PEND at commit time, so this would
|
||||
* already fail at round 1 (the same way the pre-existing single-round test
|
||||
* would); this test additionally proves the fix holds continuously across
|
||||
* a realistic multi-round confirmation gap, not just a single round
|
||||
* boundary.
|
||||
*/
|
||||
static void test_dispatch_commit_survives_several_rounds_before_confirmation(void)
|
||||
{
|
||||
xce_scheduler_t sched;
|
||||
xce_sched_worker_t* w;
|
||||
xce_sched_job_t* job;
|
||||
xce_job_alloc_t* alloc;
|
||||
xce_decision_t* decision;
|
||||
xce_resource_t* vres = NULL;
|
||||
int rc;
|
||||
int residx;
|
||||
float val;
|
||||
float zero[1] = { 0.0f };
|
||||
int round;
|
||||
|
||||
memset(&sched, 0, sizeof(sched));
|
||||
rc = xce_sched_core_init(&(sched.core));
|
||||
TEST_ASSERT_EQUAL_INT(0, rc);
|
||||
|
||||
w = add_worker(&(sched.core), WORKER_NAME);
|
||||
job = add_pending_job(&(sched.core), JOB_ID);
|
||||
|
||||
residx = add_vres(&vres, sched.core.resmgr, RES_NAME, 0);
|
||||
TEST_ASSERT_TRUE(residx >= 0);
|
||||
|
||||
alloc = make_dispatch_alloc(JOB_ID, WORKER_NAME, WANT_VALUE);
|
||||
decision = make_dispatch_decision(alloc);
|
||||
TEST_ASSERT_NOT_NULL(decision);
|
||||
|
||||
rc = sched_commit_decision(&sched, decision);
|
||||
TEST_ASSERT_TRUE(rc >= 0);
|
||||
TEST_ASSERT_EQUAL_UINT(JOB_STAT_RUN, job->state);
|
||||
|
||||
get_resvals(&(w->resInUse), 1, &residx, &val);
|
||||
TEST_ASSERT_EQUAL_FLOAT(WANT_VALUE, val);
|
||||
|
||||
for (round = 0; round < GAP_ROUNDS; round++) {
|
||||
/* still no schedStartJob/confirmation event anywhere in this loop
|
||||
* -- job->accounts/job->state were set once, at dispatch commit,
|
||||
* and nothing else re-arms them between rounds. */
|
||||
put_resvals(&(w->resInUse), 1, &residx, zero);
|
||||
get_resvals(&(w->resInUse), 1, &residx, &val);
|
||||
TEST_ASSERT_EQUAL_FLOAT(0.0f, val);
|
||||
|
||||
rc = sched_account_jobs(&(sched.core));
|
||||
TEST_ASSERT_EQUAL_INT(0, rc);
|
||||
|
||||
get_resvals(&(w->resInUse), 1, &residx, &val);
|
||||
TEST_ASSERT_EQUAL_FLOAT(WANT_VALUE, val);
|
||||
}
|
||||
}
|
||||
|
||||
/*
|
||||
* Shared fixture for the comparison test below: builds a fresh core +
|
||||
* worker + dispatched job (same shape as the tests above), then simulates
|
||||
* `nrounds` successive round boundaries -- each zeroing resInUse exactly as
|
||||
* xce_core_begin_session() does every round, then re-deriving it via
|
||||
* sched_account_jobs() -- with NO worker-confirmation event injected at any
|
||||
* point, and returns the worker's final resInUse value for RES_NAME.
|
||||
*
|
||||
* `nrounds == 1` stands in for "the worker's confirmation would have
|
||||
* arrived the very same round dispatch was committed": since resInUse for
|
||||
* a RUN job is fully determined by sched_account_jobs()'s JOB_IS_RUN()
|
||||
* gate (unmodified by this feature) and job->state is already RUN
|
||||
* synchronously at dispatch-commit (Requirement 1.1), a same-round
|
||||
* confirmation event applying JOB_STAT_RUN again is a confirmed no-op
|
||||
* against an already-RUN job (design.md, "Scheduler Dispatch Commit"
|
||||
* Responsibilities & Constraints) -- it changes nothing that
|
||||
* sched_account_jobs() reads. So the very next round's accounting pass
|
||||
* already reflects exactly what a same-round confirmation would have
|
||||
* produced; there is nothing further for an explicit confirmation event to
|
||||
* contribute to this value, which is why simulating that event is not
|
||||
* needed here to make the comparison meaningful.
|
||||
*
|
||||
* `nrounds > 1` simulates however many additional rounds elapse with still
|
||||
* no confirmation in between. Per Requirement 1.2, the two must be
|
||||
* identical -- asserted explicitly by
|
||||
* test_dispatch_commit_multi_round_gap_matches_same_round_confirmation
|
||||
* below, not merely by separately comparing each against a hardcoded
|
||||
* constant.
|
||||
*/
|
||||
static float run_dispatch_then_n_rounds(int nrounds)
|
||||
{
|
||||
xce_scheduler_t sched;
|
||||
xce_sched_worker_t* w;
|
||||
xce_sched_job_t* job;
|
||||
xce_job_alloc_t* alloc;
|
||||
xce_decision_t* decision;
|
||||
xce_resource_t* vres = NULL;
|
||||
int rc;
|
||||
int residx;
|
||||
float val;
|
||||
float zero[1] = { 0.0f };
|
||||
int i;
|
||||
|
||||
memset(&sched, 0, sizeof(sched));
|
||||
rc = xce_sched_core_init(&(sched.core));
|
||||
TEST_ASSERT_EQUAL_INT(0, rc);
|
||||
|
||||
w = add_worker(&(sched.core), WORKER_NAME);
|
||||
job = add_pending_job(&(sched.core), JOB_ID);
|
||||
|
||||
residx = add_vres(&vres, sched.core.resmgr, RES_NAME, 0);
|
||||
TEST_ASSERT_TRUE(residx >= 0);
|
||||
|
||||
alloc = make_dispatch_alloc(JOB_ID, WORKER_NAME, WANT_VALUE);
|
||||
decision = make_dispatch_decision(alloc);
|
||||
TEST_ASSERT_NOT_NULL(decision);
|
||||
|
||||
rc = sched_commit_decision(&sched, decision);
|
||||
TEST_ASSERT_TRUE(rc >= 0);
|
||||
TEST_ASSERT_EQUAL_UINT(JOB_STAT_RUN, job->state);
|
||||
|
||||
for (i = 0; i < nrounds; i++) {
|
||||
put_resvals(&(w->resInUse), 1, &residx, zero);
|
||||
rc = sched_account_jobs(&(sched.core));
|
||||
TEST_ASSERT_EQUAL_INT(0, rc);
|
||||
}
|
||||
|
||||
get_resvals(&(w->resInUse), 1, &residx, &val);
|
||||
return val;
|
||||
}
|
||||
|
||||
/*
|
||||
* Task 5.1's explicit "matching what a same-round confirmation would have
|
||||
* produced" language: directly compares the resInUse value produced after
|
||||
* only ONE round boundary since dispatch (run_dispatch_then_n_rounds(1) --
|
||||
* see its own comment for why this stands in for "confirmed the same
|
||||
* round") against the value produced after GAP_ROUNDS round boundaries
|
||||
* with still no confirmation in between. Per Requirement 1.2, a job's
|
||||
* contribution to resInUse must not depend on how many rounds elapse
|
||||
* before confirmation -- both values must be identical, and both must
|
||||
* equal WANT_VALUE. This is deliberately a value comparison against a
|
||||
* second, independently-computed scenario, not merely two separate
|
||||
* comparisons against the same hardcoded constant.
|
||||
*/
|
||||
static void test_dispatch_commit_multi_round_gap_matches_same_round_confirmation(void)
|
||||
{
|
||||
float same_round_val;
|
||||
float multi_round_gap_val;
|
||||
|
||||
same_round_val = run_dispatch_then_n_rounds(1);
|
||||
multi_round_gap_val = run_dispatch_then_n_rounds(GAP_ROUNDS);
|
||||
|
||||
TEST_ASSERT_EQUAL_FLOAT(WANT_VALUE, same_round_val);
|
||||
TEST_ASSERT_EQUAL_FLOAT(WANT_VALUE, multi_round_gap_val);
|
||||
TEST_ASSERT_EQUAL_FLOAT(same_round_val, multi_round_gap_val);
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
UNITY_BEGIN();
|
||||
RUN_TEST(test_dispatch_commit_transitions_job_to_run);
|
||||
RUN_TEST(test_dispatch_commit_survives_round_boundary_before_confirmation);
|
||||
RUN_TEST(test_dispatch_commit_survives_several_rounds_before_confirmation);
|
||||
RUN_TEST(test_dispatch_commit_multi_round_gap_matches_same_round_confirmation);
|
||||
RUN_TEST(test_pend_decision_does_not_touch_job_state);
|
||||
return UNITY_END();
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue