fix(scheduler-sim-diagnostics): correct usage display's resource classification (task 9)
repl_print_limit_usage()/repl_print_limit_usage_per_host() (repl_info.c) passed vres->vres->name (a synthesized compound scope-instance name, e.g. "limits2/u=rshen/q=xce/p=") to the byte-value classifier instead of the limit's actual constrained resource name -- so a byte-valued limit's usage amounts never got recognized as byte-valued, regardless of the underlying formatting machinery already working correctly. Fixed both call sites to pass the limit's own resname (limit->val.resname / vres->limit->val.resname) instead. Added tests proving human-readable rendering for both the cluster-wide/GLOBAL path and a percentage-based per-host limit (Requirement 6 AC 6.4: a percentage-of-byte-resource limit's resolved absolute amount is human-formatted too). Discovered and confirmed (implementer + independent reviewer): the fix landed with zero pre-existing test failures, not the handful design.md predicted -- those assertions used whole-number fixture values with bare strstr() substring checks, which still match the new, longer human-augmented output by coincidence of formatting. Documented in tasks.md; task 10.1 will strengthen those assertions to genuinely exercise the fix rather than pass by accident. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
parent
8477f677e4
commit
701901079e
3 changed files with 180 additions and 5 deletions
|
|
@ -91,7 +91,7 @@
|
|||
- _Depends: 8.1_
|
||||
- _Boundary: REPL Limit Info Command_
|
||||
|
||||
- [ ] 9. Core: Correct live usage display's resource classification
|
||||
- [x] 9. Core: Correct live usage display's resource classification
|
||||
- The live usage display currently classifies a limit's amounts using an internal per-instance identifier rather than the limit's own constrained resource, so a byte-valued limit's usage amounts never actually get evaluated for human-readable rendering even though the surrounding display logic (already retyped onto the shared composer by task 7) already supports it
|
||||
- Classify a limit's live usage amounts by the limit's own constrained resource instead, for both the cluster-wide and the per-host usage display paths
|
||||
- Observable: a byte-valued limit's accounted and capacity amounts in its live usage display render in human-readable form, matching the same format its configured-setting display already uses; a limit expressed as a percentage of a byte-valued resource shows its resolved absolute amount in human-readable form too
|
||||
|
|
@ -101,8 +101,9 @@
|
|||
|
||||
- [ ] 10. Validation: Confirm correctness, consistency, and no regression
|
||||
- [ ] 10.1 Re-verify existing test coverage against the corrected usage display
|
||||
- Existing test coverage for the live usage display was passing only because the resource-classification defect prevented it from ever being evaluated correctly — identify every such assertion, and confirm each one would now fail against the pre-fix behavior before replacing it with an assertion on the corrected, human-readable output
|
||||
- Observable: each updated assertion demonstrably fails when checked against the prior (defective) behavior and passes against the corrected behavior — not merely a new assertion added alongside an untouched old one
|
||||
- Task 9 found that fixing the resource-classification defect did NOT break any pre-existing assertion outright — instead, several existing assertions on the live usage display's amounts turned out to be loose substring checks against whole-number fixture values, which happen to still match the corrected, human-augmented output by coincidence of formatting (the old expected text is a literal prefix of the new one) rather than by genuinely exercising the fix
|
||||
- Identify every such loose, coincidentally-still-passing assertion, and strengthen each one to assert the actual corrected output precisely enough that it would fail if the resource-classification fix were reverted
|
||||
- Observable: each strengthened assertion demonstrably fails when checked against the prior (defective) behavior and passes against the corrected behavior — not merely a new assertion added alongside an untouched, still-coincidentally-passing old one
|
||||
- _Requirements: 6.1, 6.3, 6.4_
|
||||
- _Depends: 9_
|
||||
|
||||
|
|
@ -130,3 +131,4 @@ Tasks 7–10 (Requirement 6) follow the same reasoning: task 9 has no genuine da
|
|||
- **Task 6 — a real memory-safety lesson, caught by review before it shipped**: the first implementation attempt set `plugin->sched = (void*)sim` (a `xce_sched_sim_driver_t*`, disguised) before invoking the limit plugin's `config_hook`, relying on both `xce_scheduler_t` and `xce_sched_sim_driver_t` happening to have `xce_sched_core_t core` as their first struct member. An independent reviewer rejected this: `core.h`'s own `xce_core_cache_clear_cb_t` doc comment documents that this EXACT cross-cast pattern already corrupted memory once before in this codebase (`cache_clear()` reinterpreting `xce_sched_sim_driver_t`'s `alt_events_file` as a `kvtab_t*`), and there was no compiler/runtime guard against a future edit to the callee reading past `.core`. Fixed by adding `limit_seed_config(plugin, xce_sched_core_t* core, config)` — an honestly-typed entry point taking `core` as a real field access, no cast — with `limit_update_config()` (normal mode) and `sim_bootstrap_limit_plugin()` (sim mode) both delegating to a shared `_impl` function. **Lesson for future work in this codebase**: when a plugin hook or helper is invoked from BOTH a normal-mode (`xce_scheduler_t`) and a sim-mode (`xce_sched_sim_driver_t`) context, never derive one struct type from the other via a first-member cast, even when it "happens to work" today — take `xce_sched_core_t*` as an honest parameter instead. Two independent reviewers (one per round) verified this by reading the actual struct definitions and the full call chain, not by trusting the report.
|
||||
- **Task 7 — a genuine, narrow behavioral corner case surfaced by the promotion, deliberately left as-is**: the pre-existing `repl_format_numeric_resource_value()` had an asymmetry for `"swp"` (whose wire unit is megabytes, not bytes): RAW and BOTH modes printed the *un-converted* raw value, while only HUMAN mode used the MB→bytes-converted value for scaling. The new shared `xce_format_units_value(mode, bytes_value, ...)` interface — a single value argument, matching design.md's specified shape exactly — cannot preserve that asymmetry: `swp` under RAW/BOTH mode in `info worker`/`info resource` now renders the bytes-converted number instead of the original raw MB number. Confirmed by both the implementer and an independent reviewer: no test in the codebase exercises `swp` under RAW or BOTH mode (all existing `swp` tests use HUMAN only), and `swp` is not in Requirement 6's byte-valued-resource list (`mem`/`tmp`/`maxmem`/`maxtmp`/`maxswap`), so this is untested and out of this requirement's scope — not a regression caught by any existing coverage, but technically in tension with Requirement 5.3's literal "preserve all existing behavior unchanged." Left as-is rather than special-cased, since fixing it would mean giving the shared composer an `swp`-specific parameter it has no other reason to carry. If `swp`'s RAW/BOTH display is ever reported as user-visibly wrong, start here.
|
||||
- **Task 8.2 — implementer's own stated regression-safety rationale was inaccurate, though its conclusion held**: the report claimed the 28 pre-existing `test_repl_limit_table.c` tests stay green because they use "slots-based (non-byte-valued)" fixtures unaffected by units-mode. An independent reviewer found this false — every configured-limit fixture in that file hardcodes resource name `"mem"` (a byte-valued resource); `slots`-looking names in that file (`ug1_slots`, etc.) are limit *names*, not resource names. The real reason those 28 tests stay green is that none of them assert on the formatted `"<resname> = <value>"` line's exact text at all — they assert only on limit names, group membership/exclusion text, host-scope-note presence, and separate usage/accounting lines. Lesson: when a report explains *why* an existing test suite is unaffected by a change, verify the stated mechanism against the actual fixtures, not just the pass/fail outcome — a correct outcome can still carry an incorrect explanation, which would mislead the next person who touches this test file expecting a `slots` fixture to exist.
|
||||
- **Task 9 — design.md/tasks.md's own prediction about which pre-existing assertions would break was wrong, and the fix landed with ZERO pre-existing test failures (not the expected handful)**: the original design/task-10.1 framing assumed fixing the resname-classification bug would break `test_repl_limit_table.c`'s existing `"accounted=4.00"`-style assertions (lines ~1355, 1359, 1455, 1537, 1540, 1599). It didn't — every one of those fixtures uses a **whole-number** double value (`4.0`, `10.0`, `0.0`, `2.0`, `3.0`), and `xce_format_units_value(XCE_UNITS_BOTH, ...)`'s `"%f (%s)"` composition renders any whole number's `%f` expansion as `"N.000000"`, whose leading `"N.00"` substring still satisfies the old assertions' bare `strstr()` checks even though the actual output materially changed (now `"accounted=4.000000 (4.00 B)"` instead of the old `"accounted=4.00"`). Independently confirmed by task 9's reviewer, tracing the exact format chain and fixture values, with no counterexample among the 6 citations. **Consequence for task 10.1**: there is no RED→GREEN "fix a broken assertion" work to do — instead, task 10.1 should STRENGTHEN these 6 loose, coincidentally-passing assertions (e.g. assert the full corrected string, or assert a `"B)"`/unit-suffix is present) so they actually prove the fix rather than passing by accident of whole-number formatting. General lesson: a substring-only (`strstr`) assertion with no upper bound or exact-match check is blind to a superset-string regression — the old string being a prefix of the new one is enough to mask a real, materially-different output change.
|
||||
|
|
|
|||
|
|
@ -1352,8 +1352,17 @@ static int repl_print_limit_usage_per_host(xce_sched_sim_driver_t* sim,
|
|||
* already gated existence for this worker; existence must not
|
||||
* be re-derived from this read. */
|
||||
|
||||
/* Task 9 (Requirement 6.1, 6.3, 6.4, 6.5): classify by the
|
||||
* limit's OWN constrained resource (vres->limit->val.resname,
|
||||
* e.g. "mem"), never vres->vres->name (the usage instance's own
|
||||
* synthesized vres name, e.g. "limit=<name>") -- the latter is
|
||||
* never one of xce_is_bytes_valued_resource()'s five recognized
|
||||
* names, so it always misclassifies a byte-valued limit's usage
|
||||
* as non-byte-valued. vres->limit is an existing back-pointer,
|
||||
* already dereferenced elsewhere in this file (repl_print_limit_
|
||||
* usage()'s own `vres->limit != limit` filter). */
|
||||
repl_format_consumable_value_and_used(
|
||||
repl_current_units_mode(sim), vres->vres->name,
|
||||
repl_current_units_mode(sim), vres->limit->val.resname,
|
||||
cap_v->u.fvalue, used, value_str, sizeof(value_str),
|
||||
used_str, sizeof(used_str));
|
||||
|
||||
|
|
@ -1478,8 +1487,15 @@ static void repl_print_limit_usage(xce_sched_sim_driver_t* sim,
|
|||
* gated existence; existence must not be re-derived from this
|
||||
* read. */
|
||||
|
||||
/* Task 9 (Requirement 6.1, 6.3, 6.4, 6.5): classify by the
|
||||
* limit's OWN constrained resource (limit->val.resname, e.g.
|
||||
* "mem"), never vres->vres->name (the usage instance's own
|
||||
* synthesized vres name, e.g. "limit=<name>") -- see this same
|
||||
* fix's doc comment in repl_print_limit_usage_per_host() above
|
||||
* for the full rationale. `limit` is this function's own
|
||||
* parameter, already in scope. */
|
||||
repl_format_consumable_value_and_used(
|
||||
repl_current_units_mode(sim), vres->vres->name,
|
||||
repl_current_units_mode(sim), limit->val.resname,
|
||||
cap_v->u.fvalue, used, value_str, sizeof(value_str),
|
||||
used_str, sizeof(used_str));
|
||||
|
||||
|
|
|
|||
|
|
@ -1668,6 +1668,159 @@ test_info_limit_named_shows_no_active_usage_instance_when_host_scoped_vres_exist
|
|||
"scoped_vres_exists_but_no_worker_populated: PASS\n");
|
||||
}
|
||||
|
||||
/* ---------------------------------------------------------------------- */
|
||||
/* Task 9 (Requirement 6.1, 6.3, 6.4, 6.5): repl_print_limit_usage()'s */
|
||||
/* GLOBAL/SHARED path and repl_print_limit_usage_per_host()'s per-host */
|
||||
/* path both classify a live usage instance's accounted/capacity amounts */
|
||||
/* by the instance's own synthesized vres name (vres->vres->name, e.g. */
|
||||
/* "limit=<name>") instead of the limit's own constrained resource */
|
||||
/* (limit->val.resname / vres->limit->val.resname, "mem") -- */
|
||||
/* xce_is_bytes_valued_resource() is therefore always asked about the */
|
||||
/* wrong string and always returns false for a limit's usage instance, so */
|
||||
/* a byte-valued limit's live accounted/capacity amounts never actually */
|
||||
/* render human-readable even though the exact same limit's CONFIGURED- */
|
||||
/* setting line (task 8.2) already does. These two tests prove the fix. */
|
||||
/* ---------------------------------------------------------------------- */
|
||||
|
||||
/*
|
||||
* Cluster-wide (GLOBAL/SHARED) path fix -- mirrors
|
||||
* test_info_limit_named_shows_accounted_and_capacity_for_matched_and_
|
||||
* charged_global_instance's exact fixture shape, substituting byte-scale
|
||||
* capacity/used amounts (the same 5368709120.0/5000000000.0 pair
|
||||
* test_info_limit_named_shows_byte_valued_amount_human_readable_under_
|
||||
* default_units_mode and test_utils.c's own fractional-scaling test already
|
||||
* establish as "5.00 GB"/"4.66 GB") for the pre-existing fixture's small
|
||||
* 10.0/4.0 pair (too small to distinguish raw from human rendering in a
|
||||
* reader-legible way). RED against today's code: vres->vres->name (e.g.
|
||||
* "limit=byte_usage_limit") is never "mem", so repl_is_bytes_valued_
|
||||
* resource() returns false and both amounts fall back to plain "%.2f" --
|
||||
* neither "GB" string below appears.
|
||||
*/
|
||||
static void
|
||||
test_info_limit_named_shows_global_usage_amounts_human_readable_for_byte_valued_limit(
|
||||
void)
|
||||
{
|
||||
xce_sched_sim_driver_t sim;
|
||||
xce_sched_plugin_t plugin;
|
||||
xce_scheduler_t group_sched;
|
||||
xce_limit_t* matched = NULL;
|
||||
var limits_handle;
|
||||
xce_vres_t* vres;
|
||||
char* captured;
|
||||
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, "mem");
|
||||
register_limit_plugin(&sim, &plugin, &group_sched);
|
||||
add_configured_limit(&plugin, "byte_usage_limit");
|
||||
|
||||
limits_handle = limit_get_configured_limits(&(sim.core));
|
||||
rc = key_find(limits_handle, (void*)"byte_usage_limit", (void**)&matched);
|
||||
TEST_ASSERT_TRUE(rc > 0);
|
||||
TEST_ASSERT_NOT_NULL(matched);
|
||||
|
||||
vres = instantiate_and_register_global_vres(&sim, &plugin, matched);
|
||||
put_shared_fvalue(&(sim.core.sharedInst), vres->vres->idx, 5368709120.0);
|
||||
put_shared_fvalue(&(sim.core.sharedInUse), vres->vres->idx, 5000000000.0);
|
||||
|
||||
captured = capture_info_limit(&sim, "byte_usage_limit");
|
||||
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
strstr(captured, "capacity=5368709120.000000 (5.00 GB)"),
|
||||
"Requirement 6.1/6.5: a byte-valued limit's live CAPACITY amount "
|
||||
"must render human-readable, classified by the limit's OWN "
|
||||
"constrained resource (limit->val.resname, 'mem') -- this fails "
|
||||
"against today's buggy vres->vres->name classification, which "
|
||||
"always prints the plain 'capacity=5368709120.00' fallback "
|
||||
"instead");
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
strstr(captured, "accounted=5000000000.000000 (4.66 GB)"),
|
||||
"Requirement 6.1/6.5: ... and its live ACCOUNTED amount too, using "
|
||||
"the exact same classification fix");
|
||||
|
||||
free(captured);
|
||||
|
||||
printf(
|
||||
"test_info_limit_named_shows_global_usage_amounts_human_readable_"
|
||||
"for_byte_valued_limit: PASS\n");
|
||||
}
|
||||
|
||||
/*
|
||||
* Per-host path fix, doubling as Requirement 6.4's own proof: this
|
||||
* fixture's limit is itself PERCENTAGE-based (add_configured_percent_
|
||||
* limit(), val.percent=1 -- the exact shape task 3.2's own host-scoped
|
||||
* fixtures already use), so a passing test here demonstrates that a limit
|
||||
* expressed as a percentage of a byte-valued resource shows its RESOLVED
|
||||
* ABSOLUTE per-host usage amounts (not the percentage itself) in
|
||||
* human-readable form too. Mirrors test_info_limit_named_shows_separate_
|
||||
* per_host_usage_lines_for_touched_hosts_and_none_for_untouched_host's
|
||||
* exact fixture shape, substituting byte-scale capacity/used amounts for
|
||||
* the pre-existing fixture's small 5.0/2.0 pair. RED against today's code:
|
||||
* same vres->vres->name misclassification as the GLOBAL/SHARED test above.
|
||||
*/
|
||||
static void
|
||||
test_info_limit_named_shows_per_host_usage_amounts_human_readable_for_percent_based_byte_valued_limit(
|
||||
void)
|
||||
{
|
||||
xce_sched_sim_driver_t sim;
|
||||
xce_sched_plugin_t plugin;
|
||||
xce_scheduler_t group_sched;
|
||||
xce_limit_t* matched = NULL;
|
||||
var limits_handle;
|
||||
xce_vres_t* vres;
|
||||
xce_sched_worker_t* wA;
|
||||
char* captured;
|
||||
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, "mem");
|
||||
register_limit_plugin(&sim, &plugin, &group_sched);
|
||||
add_configured_percent_limit(&plugin, "byte_host_usage_limit", "mem");
|
||||
|
||||
limits_handle = limit_get_configured_limits(&(sim.core));
|
||||
rc = key_find(limits_handle, (void*)"byte_host_usage_limit",
|
||||
(void**)&matched);
|
||||
TEST_ASSERT_TRUE(rc > 0);
|
||||
TEST_ASSERT_NOT_NULL(matched);
|
||||
|
||||
vres = instantiate_and_register_host_vres(&sim, &plugin, matched);
|
||||
wA = build_bare_worker(&sim, "hostA");
|
||||
|
||||
put_shared_fvalue(&(wA->resvals), vres->vres->idx, 5368709120.0);
|
||||
put_shared_fvalue(&(wA->resInUse), vres->vres->idx, 5000000000.0);
|
||||
|
||||
captured = capture_info_limit(&sim, "byte_host_usage_limit");
|
||||
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
strstr(captured, "host=hostA"),
|
||||
"sanity: hostA's populated capacity entry must still produce a "
|
||||
"usage line");
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
strstr(captured, "capacity=5368709120.000000 (5.00 GB)"),
|
||||
"Requirement 6.1/6.4/6.5: a percentage-based limit's per-host "
|
||||
"RESOLVED ABSOLUTE capacity amount must render human-readable, "
|
||||
"classified by the limit's OWN constrained resource "
|
||||
"(vres->limit->val.resname, 'mem') -- this fails against today's "
|
||||
"buggy vres->vres->name classification, which always prints the "
|
||||
"plain 'capacity=5368709120.00' fallback instead");
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
strstr(captured, "accounted=5000000000.000000 (4.66 GB)"),
|
||||
"Requirement 6.1/6.4/6.5: ... and its resolved absolute ACCOUNTED "
|
||||
"amount too, not the percentage itself");
|
||||
|
||||
free(captured);
|
||||
|
||||
printf(
|
||||
"test_info_limit_named_shows_per_host_usage_amounts_human_readable_"
|
||||
"for_percent_based_byte_valued_limit: PASS\n");
|
||||
}
|
||||
|
||||
/*
|
||||
* Task 4 (Requirement Set 1, Req 4.1/4.2): the first tests in this file to
|
||||
* drive `info limit <name>`/`info limit all` through the REAL repl_dispatch()
|
||||
|
|
@ -3123,6 +3276,10 @@ int main(void)
|
|||
test_info_limit_named_shows_accounted_zero_for_host_with_capacity_but_never_charged);
|
||||
RUN_TEST(
|
||||
test_info_limit_named_shows_no_active_usage_instance_when_host_scoped_vres_exists_but_no_worker_populated);
|
||||
RUN_TEST(
|
||||
test_info_limit_named_shows_global_usage_amounts_human_readable_for_byte_valued_limit);
|
||||
RUN_TEST(
|
||||
test_info_limit_named_shows_per_host_usage_amounts_human_readable_for_percent_based_byte_valued_limit);
|
||||
RUN_TEST(
|
||||
test_info_limit_dispatch_named_reaches_repl_info_limit_via_real_dispatch);
|
||||
RUN_TEST(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue