From 5bf123cc20f3af355d4ea52819e6899e468e5d7a Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 13 Aug 2026 08:27:16 +0000 Subject: [PATCH] refactor(controller,xceadmin): drop generic Resource.unit, hardcode bytes for maxmem Simplification per review feedback on e3b3ffa: a generic schema-driven unit field touched the capnp wire schema (2 structs), C structs (xce_resource_t/xce_resval_t), resobj.c's deep-copy/destroy, opget.c's reply-build lookup, resources.yaml/resources.schema.json, and mapping.py -- real cross-cutting cost for a mechanism only one resource (maxmem) actually needs today. Reverted all of that. The controller CLI (test.c) now just knows "maxmem" is bytes-valued via a small explicit is_bytes_valued_resource() name list, and adaptively picks the best-fit human unit at display time (format_bytes_human, unchanged) -- same observable behavior (resval/ workerlist render maxmem as e.g. "30.90 GB"), zero wire schema surface. Regenerated the xceadmin golden fixture back to its pre-unit-field size (5304 bytes, exactly matching the size right after the mxj-only fix, confirming a clean, complete removal). Verified: 1095/1095 controller_unit_test (3 runs; one hit the pre-existing flaky segfault already logged as bug-1553, unrelated), 228/228 xceadmin pytest, clean rebuild of lib/controller/xceadmin/xce_test. Co-Authored-By: Claude Sonnet 5 --- src/configs/resources.yaml | 1 - src/configs/schema/resources.schema.json | 1 - src/controller/opget.c | 11 ----- src/controller/testsuite/test.c | 41 +++++++++++++++--- src/include/xce/types.h | 2 - src/lib/resobj.c | 2 - src/schema/controller.capnp | 14 ------ src/xceadmin/src/xcecommon/mapping.py | 8 +--- src/xceadmin/tests/fixtures/full/expected.bin | Bin 5488 -> 5304 bytes src/xceadmin/tests/test_mapping.py | 14 ------ 10 files changed, 36 insertions(+), 58 deletions(-) diff --git a/src/configs/resources.yaml b/src/configs/resources.yaml index a92b6b2a..1b2b00a5 100644 --- a/src/configs/resources.yaml +++ b/src/configs/resources.yaml @@ -58,7 +58,6 @@ resources: - name: maxmem type: numeric - unit: bytes description: "max memory in bytes (built-in host attribute; CLI display converts to a human-readable unit)" - name: maxswap diff --git a/src/configs/schema/resources.schema.json b/src/configs/schema/resources.schema.json index eaa3e835..b11620a4 100644 --- a/src/configs/schema/resources.schema.json +++ b/src/configs/schema/resources.schema.json @@ -13,7 +13,6 @@ "consumable": { "type": "boolean" }, "release": { "type": "boolean" }, "description": { "type": "string" }, - "unit": { "type": "string", "enum": ["bytes"], "description": "Display-unit hint for CLI/UI formatting; the stored value itself is always in this unit, never converted at rest. Omit for a plain unitless number (the default for every existing resource)." }, "resourcemap": { "type": "array", "items": { diff --git a/src/controller/opget.c b/src/controller/opget.c index fab329d8..4194bd51 100644 --- a/src/controller/opget.c +++ b/src/controller/opget.c @@ -62,20 +62,9 @@ static int do_reply_resvals(cluster_t* c, endpoint_t* ep, rc = get_resval(c, condition->condition.reskey->resname, condition->condition.reskey->hostname, &val); if (rc >= 0) { - xce_resource_t* r = NULL; - resval.val = &val; resval.name = condition->condition.reskey->resname; resval.host = condition->condition.reskey->hostname; - resval.unit = NULL; - - lock(c->resmgr, RTTI_LOCK_READ); - key_find(c->resmgr, (void*)resval.name, (void**)&r); - unlock(c->resmgr); - if (r != NULL) { - resval.unit = r->unit; - } - resvaldata.n_total = 1; resvaldata.vals = vals; } diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index 0d11699f..a1b1052a 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -2036,11 +2036,40 @@ cleanup: } /* - * Render a raw byte count as a human-readable string (e.g. "30.90 GB"). - * Only used for display -- the stored/wire value stays raw bytes always, - * this never feeds back into anything that's compared or persisted. A - * pure function so it's directly unit-testable without a live client - * connection. + * Resource names whose canonical stored/wire value is raw bytes (not a + * generic schema-driven unit system -- deliberately kept as a small, + * explicit, client-side list rather than a wire field, since exactly one + * resource needs this today and a wire-level unit hint would have grown + * every Resource/ResourceVal message regardless of whether it's used). + * CLI display picks the best-fit human unit adaptively (format_bytes_human); + * the stored value is never converted. + */ +static int is_bytes_valued_resource(const char* name) +{ + static const char* const BYTES_VALUED_RESOURCES[] = { "maxmem" }; + const int nres = (int)(sizeof(BYTES_VALUED_RESOURCES) + / sizeof(BYTES_VALUED_RESOURCES[0])); + int i; + + if (name == NULL) { + return 0; + } + + for (i = 0; i < nres; i++) { + if (strcmp(name, BYTES_VALUED_RESOURCES[i]) == 0) { + return 1; + } + } + + return 0; +} + +/* + * Render a raw byte count as a human-readable string (e.g. "30.90 GB"), + * adaptively picking the smallest unit the value fits in. Only used for + * display -- the stored/wire value stays raw bytes always, this never + * feeds back into anything that's compared or persisted. A pure function + * so it's directly unit-testable without a live client connection. */ static void format_bytes_human(double bytes, char* buf, size_t bufsize) { @@ -2098,7 +2127,7 @@ int cmd_resval(int argc, char* argv[]) printf("%s\n", val->val->u.svalue); break; case Value_u_fvalue: - if ((val->unit != NULL) && (strcmp(val->unit, "bytes") == 0)) { + if (is_bytes_valued_resource(val->name)) { char hbuf[64]; format_bytes_human(val->val->u.fvalue, hbuf, diff --git a/src/include/xce/types.h b/src/include/xce/types.h index 19b2e337..4c0ee34a 100644 --- a/src/include/xce/types.h +++ b/src/include/xce/types.h @@ -49,7 +49,6 @@ typedef struct { char* name; char* host; xce_val_t* val; - char* unit; } xce_resval_t; typedef struct { @@ -143,7 +142,6 @@ typedef struct { int ndistributions; res_distribution_t** distributions; xce_val_t* hbased_defined; - char* unit; } xce_resource_t; typedef struct { diff --git a/src/lib/resobj.c b/src/lib/resobj.c index 866289f9..aa0b4aa2 100644 --- a/src/lib/resobj.c +++ b/src/lib/resobj.c @@ -35,7 +35,6 @@ static int resobj_init(meta_obj_t* obj, va_list ap) dst->name = makeString(src->name); dst->desc = makeString(src->desc); - dst->unit = makeString(src->unit); #if 0 if (src->distributions != NULL) { @@ -65,7 +64,6 @@ static void resobj_destroy(meta_obj_t* obj) dereference(r->name); dereference(r->desc); - dereference(r->unit); memset(priv, 0x0, sizeof(*priv)); diff --git a/src/schema/controller.capnp b/src/schema/controller.capnp index 83e9786c..42df5db3 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -209,17 +209,6 @@ struct Resource $C.mapname("xce_resource_t"){ lastReport @7: UInt64 $C.mapname("last_report"); # distributions @8: List(ResourceDistribution) $C.maplistcount("ndistributions"); # hostBased @9: Value $C.mapname("hbased_defined"); - # capnp requires ordinals to be sequential with no holes, so @8/@9 above - # (commented out, not yet implemented) cannot be skipped -- unit takes @8 - # here; whoever implements distributions/hostBased will need to renumber - # them to @9/@10 rather than reclaiming @8. - # Optional display-unit hint (e.g. "bytes") for resources whose canonical - # internal value isn't already human-scale -- empty/unset means "raw - # number, no conversion" (every pre-existing resource, unaffected). CLI - # display (resval/workerlist) converts to a human-readable form for - # known units instead of the raw value; the stored/wire value itself - # stays canonical (e.g. maxmem stays bytes, never converted at rest). - unit @8: Text; } struct Threshold $C.mapname("xce_res_threshold_t"){ @@ -357,9 +346,6 @@ struct ResourceVal $C.mapname("xce_resval_t") { name @0: Text $C.mapname("name"); host @1: Text $C.mapname("host"); val @2: Value $C.mapname("val"); - # Display-unit hint copied from the resource's own definition (Resource.unit) - # at reply-build time -- empty/unset means "raw number, no conversion". - unit @3: Text $C.mapname("unit"); } struct ResourceVals $C.mapname("xce_resval_data_t") { diff --git a/src/xceadmin/src/xcecommon/mapping.py b/src/xceadmin/src/xcecommon/mapping.py index f7d8cb89..70bfd2c2 100644 --- a/src/xceadmin/src/xcecommon/mapping.py +++ b/src/xceadmin/src/xcecommon/mapping.py @@ -71,11 +71,7 @@ reason -- never a silent drop. runtime (e.g. ``scheduler.c`` copies these three fields straight out of the parsed resource). See :data:`_RESOURCE_VALTYPE`, :data:`_RESOURCE_ORDTYPE`, and the flag constants below for the exact - values. ``unit`` is mapped straight through from YAML when present -- - a display-only hint (e.g. ``"bytes"``) the controller CLI (``resval``/ - ``workerlist``) uses to render a human-readable value; the stored - value itself is never converted, only its display. ``idx`` and - ``lastReport`` remain unset: ``idx`` has no YAML source at all + values. ``idx`` and ``lastReport`` remain unset: ``idx`` has no YAML source at all (controller/runtime-assigned), and ``lastReport`` is a runtime timestamp field (epoch seconds of last report) -- excluded by Requirement 9.2's "no timestamp" rule. ``resourcemap`` has no @@ -328,8 +324,6 @@ def _build_resource(item: dict, target: "capnp._DynamicStructBuilder") -> None: if item.get("release"): flags |= _RESOURCE_FLAG_RELEASE target.flags = flags - if "unit" in item: - target.unit = item["unit"] def _build_queue(item: dict, target: "capnp._DynamicStructBuilder") -> None: diff --git a/src/xceadmin/tests/fixtures/full/expected.bin b/src/xceadmin/tests/fixtures/full/expected.bin index 83feb28659a56ca3b5fabe5e0ecc5a7bcb21c74a..3da1557fc2c2c3f9808a8625a75f7fe13158db0e 100644 GIT binary patch delta 464 zcmeyMwL_DYfq{Wx+C)};=WrGVhL_9?4DE~z42EnB4Dt*N41rL(kd1+%oPmL%l%0Vg zoPmMiEjt5)JR<``YD%IZ$bc9I1_l-erpf-yioBvs3=BpL3=AOp(!{mOe1c323`z_P z3?O@NU?^go+{dWIrwCO5qN5-}s!R+F zP7DkTU>YH01r>s-(uAr4(OwW$x=aiVAXOlG7DUJtDg>gn7$+}dQsMP{|X7M}b;m}Rm8v&7^F%pzd+1ZD|FUSt{8zl;nFQIj7E3xm|L zzGY-!(1P(8Z%z(m5uTjGBEx!=k%7T#;zQxdD_A5rH!?yzw{bEftMJ4RB9jeRSvWz? oS;@%2&;_!ccOl4B1_p+uj0_C3CO%Z=oeR|iqMJ6?vVP_V01QM@6#xJL delta 507 zcmdm?`9X`7fq{Wx?L=06XMR=&23HmahIU2PKls?PGz);S>z;KbBfgzlM zfx(l5fkB>;fgv>|(GX-n3cz#K54%z`&3@nUN7Bzy}fl=>pM3jFYv5WhSrV+>8Ru50z}CrzEW#k; hI9D< ugroups/hgroups (named groups) # ---------------------------------------------------------------------------