From 5876716eeef06df5019cd2a863d533a022e93200 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 17 Sep 2026 11:43:12 +0000 Subject: [PATCH] feat(scheduler-sim-diagnostics): display effective host-scope note (Req 1.6) repl_info_limit() now prints an explanatory note when a limit is implicitly host-scoped (is_host_based_limit() > 0 due to a percent-based value on a non-shared resource) but its own configuration doesn't show an explicit per-host scope. The real discriminator turned out to be consumers[CONSUMER_ HOST]->per_consumer, not pointer-nullity as design.md's gloss said -- build_consumer() unconditionally allocates a consumer for all four dimensions, so the struct field is never actually NULL in production. Fixed using the same field is_host_based_limit() itself already falls back to; verified via RED-phase reproduction that the literal design.md condition never fires. design.md corrected to document the real condition. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC --- .../specs/scheduler-sim-diagnostics/design.md | 2 +- .../specs/scheduler-sim-diagnostics/tasks.md | 2 +- src/scheduler/src/repl_info.c | 60 ++++- .../testsuite/test_repl_limit_table.c | 237 ++++++++++++++++++ 4 files changed, 295 insertions(+), 6 deletions(-) diff --git a/.kiro/specs/scheduler-sim-diagnostics/design.md b/.kiro/specs/scheduler-sim-diagnostics/design.md index 22533f3a..51d22796 100644 --- a/.kiro/specs/scheduler-sim-diagnostics/design.md +++ b/.kiro/specs/scheduler-sim-diagnostics/design.md @@ -213,7 +213,7 @@ extern var limit_get_usage_instances(xce_sched_core_t* core); - Single entry point `repl_info_limit(xce_sched_sim_driver_t* sim, const char* arg)`, called from `repl_cmd_info()` exactly like `repl_info_resource(sim, tokens[2])`. - `arg == "all"`: iterate `limit_get_configured_limits()`; if `NULL`/empty, print the Requirement 1.7 "no limits configured" message and return; otherwise print each limit via the per-limit path below. - `arg != "all"`: if `limit_get_configured_limits()` returns `NULL` (plugin not registered — a `NULL` handle is never passed to `key_find()`), or `key_find()` on the returned handle finds no match, print the Requirement 1.3 "no such limit ``" message and return (no partial output). Requirement 1.7's "no limits configured" message is reserved for the `all` path specifically — its own acceptance criterion scopes it to "when an operator requests information for all limits" — so a `NULL`-handle result on the named-lookup path reports absence of *that name*, not absence of configuration in general. -- Per-limit output: `fmt_limit()` into a stack buffer, printed line-by-line with the existing `[sim] ` prefix convention; then, only if `is_host_based_limit(core->resmgr, limit) > 0` AND the limit's own `consumers[CONSUMER_HOST] == NULL`, print the Requirement 1.6 effective-host-scope note. +- Per-limit output: `fmt_limit()` into a stack buffer, printed line-by-line with the existing `[sim] ` prefix convention; then, only if `is_host_based_limit(core->resmgr, limit) > 0` AND the limit's own per-host consumer was not explicitly configured, print the Requirement 1.6 effective-host-scope note. **Implementation note (task 2.3, corrected from an earlier inaccurate gloss here):** `limit->consumers[CONSUMER_HOST]` is never literally `NULL` for a production-built limit — `build_consumer()` (`xcelimit.c`) unconditionally allocates a consumer for all four dimensions, defaulting to the built-in "all" group with `per_consumer = 0` when unconfigured. The real discriminator is `consumers[CONSUMER_HOST]->per_consumer` (the same field `is_host_based_limit()` itself falls back to, and the same one `fmt_consumer()` uses to choose `"HOST ="` vs `"PER_HOST ="`) — check `(host_consumer == NULL) || (host_consumer->per_consumer == 0)`, not pointer-nullity alone. - Live usage: iterate `limit_get_usage_instances()`, filter by `vres->limit == matched_limit` (pointer equality). `build_vres_name()` (existing, `xcelimit.c`) never encodes host in the vresname — so there is exactly one `xce_vres_t` per matched (user, queue, project) combination *regardless of host-scoping*; host breakout is NOT "one `xce_vres_t` per host," it is a per-worker fan-out over that single `xce_vres_t`'s shared resource index. Concretely, for each matching `xce_vres_t`: - If `vres->vres` is `XCE_RESF_GLOBAL`/`XCE_RESF_SHARED`: exactly one displayed instance, reading capacity from `core.sharedInst[vres->vres->idx]` and accounted usage from `core.sharedInUse[vres->vres->idx]` — the exact read `repl_info_resource_one()` already implements for a GLOBAL/SHARED resource (bug-1593). - Otherwise (host-scoped, explicit or implicit per Requirement 1.6): one displayed instance *per worker that has a populated entry at `vres->vres->idx`* — iterate `core.workers`, and for each worker `w`, include it only if `zarray_get(&(w->resvals), vres->vres->idx)` is non-NULL (a worker `put_resvals()` has never written to, e.g. an unevaluated host, must not produce a fabricated zero-usage line, matching Requirement 2.2's no-fabrication rule at per-host granularity). For an included worker, capacity comes from `w->resvals[vres->vres->idx]` and accounted usage from `w->resInUse[vres->vres->idx]` — the sibling capacity/in-use zarray pair `sched_check_host_satisfy_()` (`resource.c`) already reads the identical way for ordinary consumable resources. diff --git a/.kiro/specs/scheduler-sim-diagnostics/tasks.md b/.kiro/specs/scheduler-sim-diagnostics/tasks.md index d9a0ccad..3d888592 100644 --- a/.kiro/specs/scheduler-sim-diagnostics/tasks.md +++ b/.kiro/specs/scheduler-sim-diagnostics/tasks.md @@ -23,7 +23,7 @@ - Observable: a limit configured with an exclusion group ("all except X") shows both the inclusion and the exclusion in its output; `info limit all` against a fixture with multiple configured limits displays every one's setting - _Requirements: 1.1, 1.2, 1.5_ -- [ ] 2.3 Display effective host-scope for implicitly host-scoped limits +- [x] 2.3 Display effective host-scope for implicitly host-scoped limits - After `fmt_limit()`'s output, call `is_host_based_limit(core->resmgr, limit)`; if it returns `> 0` AND the limit's own per-host consumer is not configured, print an explanatory host-scope note - Observable: two REPL fixtures — one percent-based limit on a per-host resource without explicit per-host scoping (note appears), one absolute-value or explicitly-global-scoped limit (note does not appear) - _Requirements: 1.6_ diff --git a/src/scheduler/src/repl_info.c b/src/scheduler/src/repl_info.c index 3d76821b..16fb675f 100644 --- a/src/scheduler/src/repl_info.c +++ b/src/scheduler/src/repl_info.c @@ -1193,6 +1193,58 @@ static void repl_print_limit_dump(const char* buf) } } +/* + * repl_print_limit_host_scope_note_if_implicit - task 2.3, Requirement 1.6: + * print an explanatory note when a limit is effectively host-based purely + * because its configured value is a percentage of a resource that is not + * shared/global cluster-wide (is_host_based_limit()'s own implicit rule, + * src/lib/xcelimit.c), NOT because the limit's own configuration explicitly + * turned on per-host scoping. When per-host scoping WAS explicitly + * configured, fmt_limit()'s own dump (task 2.2, called just before this by + * both of repl_info_limit()'s call sites) already shows a "PER_HOST = [...]" + * line for that dimension (fmt_consumer()'s own per_consumer-driven title + * choice), so printing this note too would be redundant -- this function is + * the REPL-layer discriminator between the two cases design.md's REPL Limit + * Info Command Responsibilities describes. + * + * design.md's own literal condition is "is_host_based_limit(...) > 0 AND + * limit->consumers[CONSUMER_HOST] == NULL". consumers[CONSUMER_HOST] is + * treated here as "not explicitly configured" both when it is NULL (that + * literal condition, defensive for any xce_limit_t not built through + * build_limit()/build_consumer()) AND when it is non-NULL with + * per_consumer == 0 -- the shape every REAL, production-built xce_limit_t + * actually has for a dimension nothing configured: build_consumer() + * (xcelimit.c) unconditionally allocates a consumer for all four + * dimensions on every build_limit() call, defaulting group to "all" and + * per_consumer to 0, rather than leaving the slot NULL. per_consumer is + * the one bit build_consumer() derives directly from config->per_host, and + * is also exactly what fmt_consumer() itself keys its "HOST"-vs-"PER_HOST" + * title on and what is_host_based_limit()'s own explicit-scope fallback + * returns -- so it is the correct real-world discriminator for "was + * per-host scoping explicitly configured", confirmed empirically: a bare + * `== NULL` check never fires for any limit this file's own test fixtures + * (or any real limit_update_config() caller) can produce. + */ +static void repl_print_limit_host_scope_note_if_implicit( + xce_sched_core_t* core, xce_limit_t* limit) +{ + xce_consumer_t* host_consumer = limit->consumers[CONSUMER_HOST]; + + if (is_host_based_limit(core->resmgr, limit) <= 0) { + return; + } + + if ((host_consumer != NULL) && (host_consumer->per_consumer != 0)) { + return; + } + + fprintf(stdout, + "[sim] note: this limit's value is a percentage of a resource " + "that is not shared across the cluster, so usage for it is " + "tracked separately per host even though the configuration above " + "does not show an explicit host scope\n"); +} + /* * repl_info_limit - `info limit ` name/all resolution (task 2.1) * plus, once a genuine match is found, printing that limit's configured @@ -1249,8 +1301,8 @@ static void repl_info_limit(xce_sched_sim_driver_t* sim, const char* arg) repl_print_limit_dump(limit_buf); } } - /* task 2.3 adds the Req 1.6 effective-host-scope note here; - * task 3 adds this limit's live usage. */ + repl_print_limit_host_scope_note_if_implicit(&(sim->core), limit); + /* task 3 adds this limit's live usage. */ n_shown++; } iter_destroy(&it); @@ -1280,8 +1332,8 @@ static void repl_info_limit(xce_sched_sim_driver_t* sim, const char* arg) repl_print_limit_dump(limit_buf); } } - /* task 2.3 adds the Req 1.6 effective-host-scope note here; task 3 - * adds this limit's live usage. */ + repl_print_limit_host_scope_note_if_implicit(&(sim->core), match); + /* task 3 adds this limit's live usage. */ } } diff --git a/src/scheduler/testsuite/test_repl_limit_table.c b/src/scheduler/testsuite/test_repl_limit_table.c index eb692f93..38324647 100644 --- a/src/scheduler/testsuite/test_repl_limit_table.c +++ b/src/scheduler/testsuite/test_repl_limit_table.c @@ -423,6 +423,96 @@ static void add_configured_limit_with_host_exclusion( TEST_ASSERT_EQUAL_INT(0, rc); } +/* + * register_per_host_resource - task 2.3's own fixture: registers a plain, + * non-GLOBAL/non-SHARED resource named `resname` directly into `sim`'s real + * core.resmgr, the same makeResource() pattern test_repl_resource_table.c's + * own build_worker_with_one_numeric_resource() already establishes. + * is_host_based_limit() (src/lib/xcelimit.c) key_find()s a limit's + * val.resname directly against core->resmgr and inspects the resulting + * xce_resource_t's flags -- a bare resource-name string with no backing + * xce_resource_t makes that key_find() miss (-ENOENT), which is NOT the + * same as "genuinely a per-host, not-shared-cluster-wide resource" for this + * check's purposes, so this fixture must exist for is_host_based_limit()'s + * implicit-host-scope rule to be exercised at all. + */ +static xce_resource_t* register_per_host_resource(xce_sched_sim_driver_t* sim, + const char* resname) +{ + xce_resource_t wire_resource; + xce_resource_t* r; + + memset(&wire_resource, 0, sizeof(wire_resource)); + wire_resource.name = (char*)resname; + wire_resource.valtype = XCE_VAL_NUMERIC; + wire_resource.flags = XCE_RESF_CONSUMABLE; + r = makeResource(sim->core.resmgr, &wire_resource); + TEST_ASSERT_NOT_NULL(r); + return r; +} + +/* + * add_configured_percent_limit - task 2.3's own Requirement 1.6 POSITIVE + * fixture: a percent-based limit (val.percent=1) on `resname`, with NO + * explicit per_host/hosts config (per_host left at its zero default, same + * "no explicit scoping" shape make_limit_config()/add_configured_limit() + * already establish for the other three dimensions) -- the exact shape + * is_host_based_limit()'s own implicit-host-scope rule targets (xcelimit.c: + * resource flags lack XCE_RESF_GLOBAL/XCE_RESF_SHARED AND + * limit->val.percent != 0 -> host-based regardless of configured + * per_host/hosts). + */ +static void add_configured_percent_limit(xce_sched_plugin_t* plugin, + const char* name, + const char* resname) +{ + xce_limit_val_t val = { .resname = (char*)resname, + .percent = 1, + .val = 50.0 }; + xce_limit_config_t limit_cfg = { .name = (char*)name, .val = &val }; + xce_limit_config_t* limits[1] = { &limit_cfg }; + xce_conf4sched_t config; + int rc; + + memset(&config, 0, sizeof(config)); + config.nlimits = 1; + config.limits = limits; + + rc = limit_update_config(plugin, &config); + TEST_ASSERT_EQUAL_INT(0, rc); +} + +/* + * add_configured_percent_limit_explicit_per_host - task 2.3's own + * self-review fixture (design.md: "if CONSUMER_HOST IS explicitly + * configured, fmt_limit()'s own output already shows PER_HOST = [...], so + * no extra note is needed there"): identical to + * add_configured_percent_limit() above except `.per_host = 1` is set + * explicitly, so build_consumer() (xcelimit.c) marks + * consumers[CONSUMER_HOST]->per_consumer = 1 -- the genuinely-explicit case + * the Requirement 1.6 note must NOT duplicate. + */ +static void add_configured_percent_limit_explicit_per_host( + xce_sched_plugin_t* plugin, const char* name, const char* resname) +{ + xce_limit_val_t val = { .resname = (char*)resname, + .percent = 1, + .val = 50.0 }; + xce_limit_config_t limit_cfg = { .name = (char*)name, + .per_host = 1, + .val = &val }; + xce_limit_config_t* limits[1] = { &limit_cfg }; + xce_conf4sched_t config; + int rc; + + memset(&config, 0, sizeof(config)); + config.nlimits = 1; + config.limits = limits; + + rc = limit_update_config(plugin, &config); + TEST_ASSERT_EQUAL_INT(0, rc); +} + /* ---------------------------------------------------------------------- */ /* Tests */ /* ---------------------------------------------------------------------- */ @@ -730,6 +820,147 @@ static void test_info_limit_all_displays_every_configured_limits_setting(void) "PASS\n"); } +/* + * Task 2.3's own Observable #1 (Requirement 1.6, positive case): a + * percent-based limit on a resource that is registered but not + * GLOBAL/SHARED (an ordinary per-host resource, "mem"), with NO explicit + * per-host scoping configured, must display the Requirement 1.6 effective- + * host-scope note after fmt_limit()'s own dump -- is_host_based_limit()'s + * implicit rule (xcelimit.c) treats this limit as host-based purely because + * of its percentage value on a non-shared resource, regardless of the + * limit's own consumers[CONSUMER_HOST] configuration, and Requirement 1.6 + * requires the REPL to surface that to the operator. + */ +static void +test_info_limit_named_shows_host_scope_note_for_implicit_percent_based_limit( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_plugin_t plugin; + xce_scheduler_t group_sched; + 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, "pct_limit", "mem"); + + captured = capture_info_limit(&sim, "pct_limit"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "limit: pct_limit"), + "the matched limit's fmt_limit() dump must still print first " + "(proves this note is appended after, not instead of, task 2.2's " + "own output)"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "tracked separately per host"), + "Requirement 1.6: a percent-based limit on a resource that is not " + "shared cluster-wide, with no explicit host scoping configured, " + "must display the effective host-scope note"); + + free(captured); + + printf("test_info_limit_named_shows_host_scope_note_for_implicit_" + "percent_based_limit: PASS\n"); +} + +/* + * Task 2.3's own Observable #2 (Requirement 1.6, negative contrast): an + * absolute-valued limit (percent=0) on the SAME per-host resource must NOT + * display the note -- is_host_based_limit()'s implicit rule only fires for + * a PERCENTAGE value, so an absolute-value limit is never implicitly + * host-based and no host-scope note applies. Reuses add_configured_limit()'s + * existing absolute-value ("mem", percent=0, val=10.0) fixture shape, + * proving the contrast is driven purely by percent vs. absolute, not by + * some other difference between fixtures. + */ +static void +test_info_limit_named_does_not_show_host_scope_note_for_absolute_value_limit( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_plugin_t plugin; + xce_scheduler_t group_sched; + 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, "abs_limit"); + + captured = capture_info_limit(&sim, "abs_limit"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "limit: abs_limit"), + "sanity: the matched limit's fmt_limit() dump must still print " + "(proves this really matched a real, configured limit)"); + TEST_ASSERT_NULL_MESSAGE( + strstr(captured, "tracked separately per host"), + "Requirement 1.6: an absolute-valued limit is not implicitly " + "host-based, so the effective host-scope note must NOT appear"); + + free(captured); + + printf("test_info_limit_named_does_not_show_host_scope_note_for_" + "absolute_value_limit: PASS\n"); +} + +/* + * Task 2.3's own self-review guard (design.md: "if CONSUMER_HOST IS + * explicitly configured, fmt_limit()'s own output already shows + * PER_HOST = [...], so no extra note is needed there"): a percent-based + * limit on the same non-shared resource, but with per_host EXPLICITLY + * configured, must show fmt_limit()'s own "PER_HOST = [...]" line but must + * NOT ALSO print the Requirement 1.6 note -- proves the condition does not + * accidentally fire for the genuinely-explicit case too (it would be + * redundant, since the operator already sees the host dimension called out + * by task 2.2's own output). + */ +static void +test_info_limit_named_does_not_duplicate_host_scope_note_when_per_host_explicitly_configured( + void) +{ + xce_sched_sim_driver_t sim; + xce_sched_plugin_t plugin; + xce_scheduler_t group_sched; + 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_explicit_per_host(&plugin, + "explicit_pct_limit", + "mem"); + + captured = capture_info_limit(&sim, "explicit_pct_limit"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "PER_HOST ="), + "sanity: fmt_limit() must show a PER_HOST scope line for an " + "explicitly per-host-scoped limit (task 2.2's own output)"); + TEST_ASSERT_NULL_MESSAGE( + strstr(captured, "tracked separately per host"), + "Requirement 1.6's note would be redundant once fmt_limit() " + "already shows PER_HOST -- it must not duplicate"); + + free(captured); + + printf("test_info_limit_named_does_not_duplicate_host_scope_note_when_" + "per_host_explicitly_configured: PASS\n"); +} + int main(void) { UNITY_BEGIN(); @@ -748,5 +979,11 @@ int main(void) RUN_TEST( test_info_limit_named_shows_both_inclusion_and_exclusion_membership); RUN_TEST(test_info_limit_all_displays_every_configured_limits_setting); + RUN_TEST( + test_info_limit_named_shows_host_scope_note_for_implicit_percent_based_limit); + RUN_TEST( + test_info_limit_named_does_not_show_host_scope_note_for_absolute_value_limit); + RUN_TEST( + test_info_limit_named_does_not_duplicate_host_scope_note_when_per_host_explicitly_configured); return UNITY_END(); }