Rongsong Shen
dcf54387ae
libxce-alloc-preason-equality (requirements, design, tasks; not yet approved): - alloc_eq compares `from` and excludes only instid; the scheduler stamps `from` with the deciding session's start, the same clock reading that is recorded as beginRound, so a replay reproduces it. - every comparator takes an optional char** why: a malloc'd description of the first difference, owned and freed by the caller; --sim-verify mismatch lines print it (resolves bug-1881). - job_preason_eq is new full-match behavior, not a relocation (bug-1845 keeps the simulator main-reason-only); the three white-box test files switch to it and keep their sim_verify.c include. - alloc_eq stays separate from merge_job_alloc()'s same_alloc_slot() (bug-1863), which does match on instid. scheduler-sim-discrete-clock (initialized): remove --sim-speed so the simulator clock moves only at recorded timestamps and a replayed session_start equals the recorded beginRound exactly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
78 lines
8.4 KiB
Markdown
78 lines
8.4 KiB
Markdown
# Implementation Plan
|
||
|
||
## Task Format
|
||
|
||
Work is sequenced test-first (TDD: red → green → refactor). New library API gets its unit tests first; the `from` stamping change gets a failing scheduler test first; the scheduler-side migration is then verified by the existing and extended `--sim-verify` tests.
|
||
|
||
---
|
||
|
||
- [ ] 1. Add a shared NULL-safe string helper to libxce (`utils.c`/`utils.h`)
|
||
- Add `int str_eq(const char* a, const char* b)` beside the existing `float_equal` helper: returns non-zero iff both arguments are NULL, or both non-NULL and `strcmp(a, b) == 0`.
|
||
- Test it in `test_preason.c` or a `utils` test: NULL/NULL, NULL/non-NULL, equal, different.
|
||
- _Requirements: 1.5, 2.3 (supporting)_
|
||
- _Boundary: utils.c, utils.h_
|
||
|
||
- [ ] 2. Allocation equality API with first-difference description (`xcealloc.c`/`xcealloc.h`), test-first
|
||
- RED: in `test_xcealloc.c`, add tests for `alloc_eq`/`host_alloc_eq`/`job_alloc_eq`: match; mismatch per field (`resname`, `reserve`, `value`, `duration`, `from`) with the expected description in `*why` (freed by the test); `instid` difference still equal; host name, order and list-length sensitivity; `jobid`, ordered hosts, ordered shared, length mismatch; NULL at top level, nested entries and string fields; NULL `why`; `*why == NULL` on equal. In a dedicated test binary with `-Wl,--wrap=vasprintf` (bug-1843's `--wrap` idiom), a failed description allocation leaves `*why` NULL with the same result.
|
||
- GREEN: implement the three functions with signature `(a, b, char** why)`, comparing in the order given in design.md Data Models, `value` with exact `==` (not `float_equal`), strings via `str_eq`, descriptions via `vasprintf` in the design's path format (`jobid` via `fmt_jobid`, `value` with `%.17g`); a nested call's description is prefixed with the parent's path segment and the inner string freed.
|
||
- Declare them in `src/include/xcealloc.h` with a doc comment covering: "same allocation decision" equality, `from` compared and `instid` excluded (and why), the difference from `merge_job_alloc()`'s slot matching, NULL/order semantics, the malformed-record precondition, and the `why` ownership contract (caller frees with `free()`; `*why` NULL on equal or on allocation failure).
|
||
- Do not change `make_alloc()`, `merge_job_alloc()` or `same_alloc_slot()`; confirm existing `test_xcealloc.c` tests still pass.
|
||
- _Requirements: 1.1–1.7, 3.1–3.5, 6.4, 6.6_
|
||
- _Boundary: xcealloc.c, xcealloc.h, test_xcealloc.c_
|
||
- _Depends: 1_
|
||
|
||
- [ ] 3. Pending-reason equality API with description (`preason.c`/`preason.h`), test-first
|
||
- RED: in `test_preason.c`, add tests for `job_preason_eq`: match; main-reason mismatch; host-based list mismatch per field, order and length; resource-based list mismatch per field, order and length; NULL handling; each with the expected description (freed by the test); NULL `why`; `*why == NULL` on equal.
|
||
- GREEN: implement `job_preason_eq(a, b, char** why)` as a full structural match with the shared NULL convention.
|
||
- Declare it in `src/include/preason.h` with a doc comment covering the full-match contract (nothing excluded), NULL/order semantics, malformed-record precondition, the `why` ownership contract, and that the simulator deliberately uses a main-reason-only check instead (bug-1845).
|
||
- _Requirements: 2.1–2.3, 3.1–3.5, 6.4_
|
||
- _Boundary: preason.c, preason.h, test_preason.c_
|
||
- _Depends: 1_
|
||
|
||
- [ ] 4. Reproducible `from`: one session-start reading, stamped on every allocation, test-first
|
||
- RED: scheduler test(s) asserting (a) every allocation produced in one normal-mode session, including a COMPOUND request merged by `merge_job_alloc()`, has `from == session_start / 1000000000`; (b) with recording on, the recorded `beginRound == session_start`.
|
||
- GREEN:
|
||
- `driver.c`: pass `sched->core.session_start` to `sched_record_session_begin()` instead of a second `xce_core_now()` read.
|
||
- `scheduler.c:1893`: after `make_alloc()`, set `allocs[i]->from = sched->core.session_start / 1000000000ULL`.
|
||
- Check there is no other scheduler path that builds an `xce_alloc_t` for a decision outside a session; if one exists, stop and raise it (Revalidation Trigger).
|
||
- Simulation side (Requirement 4.3) is spec `scheduler-sim-discrete-clock`; once it lands, add a test that a replayed allocation's `from` equals the recorded one.
|
||
- _Requirements: 4.1, 4.2, 4.4, 4.5, 6.5_
|
||
- _Boundary: driver.c, scheduler.c, new/extended scheduler test_
|
||
|
||
- [ ] 5. Migrate the simulator to the public API and put the difference in the mismatch log (`sim_verify.c`, `sim_loop.c`)
|
||
- Delete the five `static` comparators (`sim_verify_str_eq`, `sim_verify_alloc_eq`, `sim_verify_host_alloc_eq`, `sim_verify_job_alloc_eq`, `sim_verify_job_preason_eq`) and their `from`-exclusion comments.
|
||
- Add a `static` simulator-local `sim_verify_pend_outcome_eq(a, b, char** why)` comparing `main_reason` only, documented with the bug-1845 rationale.
|
||
- Change `sim_decision_eq` to `(expected, actual, char** why)`: DISPATCH → `job_alloc_eq(..., why)`; PEND → jobid equality plus the local main-reason check, describing whichever differs; kind mismatch described too. Update `sim_loop.c`'s `extern` declaration and call.
|
||
- `sim_log_mismatch`: get the description from `sim_decision_eq`, append ` -- diff: <why>` to the existing line, then `free()` it (resolves bug-1881).
|
||
- Add `#include "xcealloc.h"`; keep the `describe_*` helpers.
|
||
- _Requirements: 5.1, 5.2, 5.3_
|
||
- _Boundary: sim_verify.c, sim_loop.c_
|
||
- _Depends: 2, 4_
|
||
|
||
- [ ] 6. Migrate the three white-box test consumers to the public API
|
||
- `test_sim_equivalence.c`, `test_repl_equivalence.c`, `test_repl_limit_table.c`: replace `sim_verify_job_alloc_eq` → `job_alloc_eq` and `sim_verify_job_preason_eq` → `job_preason_eq` (take the description, print it in the failure message and free it, or pass `NULL`); add the two headers.
|
||
- Keep each file's white-box `#include "../src/sim_verify.c"`: the files also include `sim_loop.c`, which calls `sim_decision_eq`/`sim_log_mismatch`.
|
||
- The switch to full-match `job_preason_eq` is valid because none of these tests registers `unsatisfiable_lookup_cb`, so both sides build detail lists by the same full per-host computation; add a short comment at each `job_preason_eq` call stating this. `from` is now compared too. If a test fails after either change, investigate it as a real divergence; do not loosen the comparison.
|
||
- Update comments that name `sim_verify_*_eq` or describe `from` as excluded in these files and in `test_sim_live_allocation.c` (~702, ~718) and `test_sim_reconcile_unaccounted.c` (~550).
|
||
- _Requirements: 6.1, 6.2, 6.3, 6.7_
|
||
- _Boundary: test_sim_equivalence.c, test_repl_equivalence.c, test_repl_limit_table.c, test_sim_live_allocation.c, test_sim_reconcile_unaccounted.c_
|
||
- _Depends: 2, 3, 4, 5_
|
||
|
||
- [ ] 7. Reconcile `test_sim_verify.c` with the relocated comparators
|
||
- Remove the DISPATCH alloc-level unit tests that only re-prove the allocation comparators — now owned by libxce tests from task 2. Replace the "`from` difference is ignored" test with one that shows same-session allocations verify cleanly and a changed `from` is reported as a mismatch naming `from`.
|
||
- Keep the PEND unit tests (identical-content match, jobid/main-reason mismatch, detail-ignored) — they pin `sim_decision_eq`'s simulator-local main-reason-only behavior.
|
||
- Add a test that `sim_log_mismatch`'s line contains the differing field and both values.
|
||
- Run the `--sim-verify` round-trip tests (zero-mismatch self-replay, altered replay, reconcile cases). Until `scheduler-sim-discrete-clock` lands, at `--sim-speed 0`.
|
||
- _Requirements: 5.2, 5.3, 5.4, 6.4_
|
||
- _Boundary: test_sim_verify.c_
|
||
- _Depends: 5_
|
||
|
||
- [ ] 8. Full build, test and live verification
|
||
- `/xce-build lib` (debug + ASAN + tests) green.
|
||
- `/xce-build scheduler --incremental` (debug + ASAN + tests) green, including `test_sim_verify.c`, `test_sim_equivalence.c`, `test_repl_equivalence.c`, `test_repl_limit_table.c`.
|
||
- `/xce-build controller --incremental` green (consumes `xcealloc.h`; confirms nothing else depended on the removed names).
|
||
- Live (after `scheduler-sim-discrete-clock`): record a session on a local deployment, replay it with `--sim-verify`: zero mismatches.
|
||
- `grep -rn 'sim_verify_\(host_\|job_\)\?\(alloc\|preason\)_eq' src` returns no matches.
|
||
- After merge: `buglog.py status bug-1881 fixed --commit <sha>` with the Verification section filled in.
|
||
- _Requirements: 5.4, 6.6, 6.7_
|
||
- _Depends: 2, 3, 4, 5, 6, 7_
|