lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(scheduler): percent-based limit display shows raw fraction, not percent (bug-1831)

Browse source 
fmt_limit()/fmt_limit_units() formatted a percent-based limit's value
as a bare snprintf(..., "%f%%", limit->val.val) -- echoing the raw
stored value with a literal "%" appended. But xce_limit_val_t.val is
genuinely a 0-1 fraction when percent is set (confirmed end-to-end:
no /100 or *100 scaling exists anywhere in the config->capnp->runtime
pipeline; confirmed authoritative by the original hand-written C
literal this codebase's oldest percent-based limit was migrated from,
.val = 0.2 for a documented "20% mem cap"). A correctly-configured 20%
limit therefore displayed as "0.200000%" instead of "20.000000%" --
misleading, though technically an honest echo of the stored value.

Fixed by multiplying by 100.0 before formatting. Display-only --
update_limit_res_value()'s actual capacity computation was already
correct and is untouched. fmt_limit() (a thin XCE_UNITS_RAW wrapper)
inherits this fix for its own percent-branch output too, a deliberate,
intentional change from its pre-fix behavior for percent-based limits
specifically (its 5 pre-existing direct tests only assert rc > 0,
never the exact percent string, so this is safe).

Updated test_limit.c's percent-mode test fixture from an unrealistic
.val=80.0 to a realistic .val=0.8 (assertion text unchanged, since
0.8*100=80 either way) and test_repl_limit_table.c's own percent test
to expect its shared fixture's arbitrary .val=50.0 constant correctly
rendering as "5000.000000%". RED confirmed via the raw Unity testlog's
:FAIL: lines (the aggregate meson summary masks single-test failures,
a known harness quirk in this project); GREEN confirmed after, both
lib and scheduler suites, 0 Unity FAILs.

Discovered during live manual verification of scheduler-sim-diagnostics
Requirement 6 against a real deployed cluster.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
Rongsong Shen 2026-09-20 10:00:12 +00:00
parent 07b5c989b5
commit 0ce7125481
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
4 changed files with 103 additions and 14 deletions
Download patch file Download diff file Expand all files Collapse all files

2
src/lib/testsuite/test_limit.c
View file

@ -455,7 +455,7 @@ static void test_fmt_limit_units_percent_limit_ignores_units_mode(void)
.hosts = NULL,
.projects = NULL,
.val
= &(xce_limit_val_t) { .resname = "mem", .percent = 1, .val = 80.0 }
= &(xce_limit_val_t) { .resname = "mem", .percent = 1, .val = 0.8 }
};
char buf[1024];
xce_units_mode_t modes[] = { XCE_UNITS_RAW, XCE_UNITS_HUMAN,

38
src/lib/xcelimit.c
View file

@ -160,8 +160,21 @@ void kill_consumer(xce_consumer_t* c)
*
* The consumer-dump loop is IDENTICAL to fmt_limit()'s -- untouched. Only
* the final configured-value line's formatting differs:
* - percent != 0: always the bare "%f%%" rendering, regardless of
* units_mode -- a percentage is never byte-scaled (Requirement 6 AC3).
* - percent != 0: always renders `val.val * 100` followed by "%",
* regardless of units_mode -- a percentage is never byte-scaled
* (Requirement 6 AC3). `xce_limit_val_t.val` is genuinely a 0-1
* fraction when `percent != 0` (confirmed end to end against the real
* capacity computation, update_limit_res_value() in
* src/scheduler/src/plugins/limit.c, which multiplies this value
* DIRECTLY against the resource's current value with no /100 anywhere
* in the config→capnp→runtime pipeline -- and against the original
* hand-written C literal this codebase's oldest percent-based limit was
* migrated from, `.val = 0.2` for a documented "20% mem cap"). The *100
* here is display-only, correcting a bug (fixed post-Requirement-6
* shipping) where the raw 0-1 fraction was echoed directly with a "%"
* appended, so a correctly-configured 20% limit displayed as
* "0.200000%" instead of "20.000000%" -- misleading, though not
* incorrect in the sense of matching the stored value.
* - percent == 0, resource is byte-valued (xce_is_bytes_valued_resource())
* or the "swp" special case (its wire unit is MB, not bytes -- the same
* check repl_format_numeric_resource_value() already applies, converted
@ -172,13 +185,18 @@ void kill_consumer(xce_consumer_t* c)
* AC3).
*
* fmt_limit() itself is a one-line wrapper around this function in
* XCE_UNITS_RAW mode. That is provably byte-identical to fmt_limit()'s own
* pre-existing output for every percent/non-percent, byte-valued/
* non-byte-valued shape: xce_format_units_value(XCE_UNITS_RAW, v, ...)
* always renders the bare "%f" of v (see its own implementation), matching
* what the old inline fmt_limit() body produced. This is why
* test_limit.c's five pre-existing fmt_limit() call sites needed zero
* changes -- they are the regression proof.
* XCE_UNITS_RAW mode. For the percent == 0 branches (byte-valued and
* non-byte-valued absolute values), this remains provably byte-identical to
* fmt_limit()'s ORIGINAL pre-Requirement-6 output -- confirmed by
* test_limit.c's five original fmt_limit() call sites needing zero changes
* for those shapes. For the percent != 0 branch, this is now a DELIBERATE
* behavior change from fmt_limit()'s pre-Requirement-6 output (the *100
* display fix above applies to fmt_limit() too, since it shares this same
* branch through the RAW-mode wrapper) -- none of those five original call
* sites assert on the exact percent-display string (they only check
* `rc > 0`), so this change is safe for them, but it is a real, intentional
* change to fmt_limit()'s own output for a percent-based limit, not a
* byte-identical refactor for that one case.
*/
int fmt_limit_units(char* buf, int len, xce_limit_t* limit,
xce_units_mode_t units_mode)
@ -213,7 +231,7 @@ int fmt_limit_units(char* buf, int len, xce_limit_t* limit,
if (limit->val.percent != 0) {
rc = snprintf(buf + pos, len - pos, "\n%s = %f%%\n",
limit->val.resname, limit->val.val);
limit->val.resname, (double)limit->val.val * 100.0);
} else {
bool is_swp = (limit->val.resname != NULL)
&& (strcmp(limit->val.resname, "swp") == 0);

27
src/scheduler/testsuite/test_repl_limit_table.c
View file

@ -3490,7 +3490,7 @@ test_info_limit_named_non_byte_valued_limit_display_completely_unaffected_by_uni
* directly; this closes the same gap at the REPL dispatch level, through
* the real `set units` command and repl_info_limit()'s real
* translate_units_mode() bridge -- neither of which the library-level test
* exercises). The limit is percent-based (val.percent=1, 50%) on the
* exercises). The limit is percent-based (val.percent=1) on the
* byte-valued resource "mem" -- deliberately so, since Requirement 6.3's
* own wording singles out "a limit expressed as a percentage of a resource
* whose own canonical unit is not bytes" as the case that must never be
@ -3500,6 +3500,26 @@ test_info_limit_named_non_byte_valued_limit_display_completely_unaffected_by_uni
* task 9's own per-host byte-valued-percent-limit test already covers for
* the DIFFERENT, RESOLVED-ABSOLUTE usage amount (Requirement 6.4) -- not
* re-litigated here.
*
* Percent-display scale (bug fix, added after Requirement 6 shipped):
* `add_configured_percent_limit()`'s shared fixture stores `val.val = 50.0`
* as an arbitrary test constant -- but `xce_limit_val_t.val` is genuinely a
* 0-1 FRACTION when `percent != 0` (confirmed against the real capacity
* computation, `update_limit_res_value()`, plugins/limit.c, which multiplies
* this value DIRECTLY against the resource's current value with no /100
* anywhere in the pipeline -- traced end to end from
* src/configs/limits.yaml through capnp to this function). `fmt_limit_units()`
* now correctly renders `val.val * 100` for the percent branch, so this
* fixture's raw 50.0 displays as the (visually odd but mathematically
* correct) "5000.000000%" -- not because 50.0 means 5000%, but because 50.0
* was never a realistic percent-scale value to begin with (a real 50% limit
* needs `val.val = 0.5`). The shared fixture is deliberately left unchanged
* here rather than "corrected" to 0.5, since many OTHER tests in this file
* reuse it for their own accounted/capacity math (which depends on the
* capacity computation directly, not on this display fix, and remains
* correct either way) -- changing it would ripple into those tests for no
* benefit to this one, which only cares about verifying units_mode has zero
* effect on the percent line, not about the fixture's real-world realism.
*/
static void
test_info_limit_named_percent_limit_own_percentage_value_never_scaled_by_units_mode(
@ -3530,9 +3550,10 @@ test_info_limit_named_percent_limit_own_percentage_value_never_scaled_by_units_m
captured_raw
= capture_dispatch(&sim, "info limit pct_never_scaled_limit");
TEST_ASSERT_NOT_NULL_MESSAGE(
strstr(captured_raw, "mem = 50.000000%"),
strstr(captured_raw, "mem = 5000.000000%"),
"sanity: the configured-setting line must show the real percent "
"value under RAW mode");
"value under RAW mode (val.val=50.0 * 100, see this test's own "
"header comment on the percent-display scale fix)");
handled = repl_dispatch(&sim, "set units human");
TEST_ASSERT_FALSE(handled);