From e3b3ffaa975eee04170ff367e9ee6a556a7f89b4 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 13 Aug 2026 08:12:09 +0000 Subject: [PATCH] fix(controller,xceadmin): fix zero-slot scheduling block, misleading resval logs, byte-unit display Three bugs found while diagnosing a live controller log, all in the worker resource-value path: 1. Every host relying on the documented hosts.yaml auto-detect default (no max_slot, meaning "use CPU count") got 0 scheduling slots, permanently blocking the scheduler from placing jobs on it. mxj is UInt32 on the wire and can't carry the schema's -1 sentinel; mapping.py worked around this by leaving it unset, which defaults to 0 -- and 0 is never < 0, so apply_join_state()'s auto-detect fallback never fired. Fixed by encoding the sentinel as 0xFFFFFFFF, which the controller's `int mxj` field reinterprets back to -1 via its existing implicit uint32_t->int conversion. No C-side or wire schema change needed for this part; the fallback itself was already correct but had zero test coverage for a negative mxj, now added. 2. update_resvals()/update_resval_list() logged the failure reason via PERROR, which reads process-global errno -- but update_resval()'s failure paths return raw negative codes directly and never touch errno, so the controller log filled with misleading "update_resval: Success" / "update_resval: Resource temporarily unavailable" lines instead of the real reason. Fixed to decode the actual rc via xce_strerror_r and name the failing resource. 3. maxmem is canonically stored/wired as raw bytes but had no unit metadata, so resval/workerlist printed it as a bare number that read as garbage (e.g. workerlist's fixed-width columns visually smeared a multi-GB byte count across the MaxCPU/MaxMem headers). Added an optional `unit` field to the Resource/ResourceVal capnp structs (and xce_resource_t/xce_resval_t/resources.yaml) so resval and workerlist can render a human-readable value (e.g. "30.90 GB") while the stored value stays raw bytes -- display-only, every other resource unaffected. Regenerated the xceadmin golden fixture (fixtures/full/expected.bin) twice: once for the mxj sentinel encoding change, once for the new Resource.unit field growing every resource's wire size by one pointer slot. Both diffs verified byte-for-byte isolated to exactly the expected change before regenerating. Verified: 1095/1095 controller_unit_test (stable across repeated runs), 229/229 xceadmin pytest, clean rebuild of lib/controller/xceadmin/xce_test. Two related findings logged to .wolf/buglog.json but deliberately not fixed here (out of scope per discussion): a pre-existing flaky segfault in the combined controller_unit_test binary, and maxswap/maxtmp being hardcoded stub values on the worker side. Co-Authored-By: Claude Sonnet 5 --- src/configs/resources.yaml | 3 +- src/configs/schema/resources.schema.json | 1 + src/controller/opget.c | 11 ++ src/controller/testsuite/test.c | 47 ++++- src/controller/testsuite/test_worker_join.c | 184 ++++++++++++++++++ src/controller/worker.c | 24 ++- src/include/xce/types.h | 2 + src/lib/resobj.c | 2 + src/schema/controller.capnp | 14 ++ src/xceadmin/src/xcecommon/mapping.py | 48 ++++- src/xceadmin/tests/fixtures/full/expected.bin | Bin 5304 -> 5488 bytes src/xceadmin/tests/test_mapping.py | 38 +++- 12 files changed, 354 insertions(+), 20 deletions(-) diff --git a/src/configs/resources.yaml b/src/configs/resources.yaml index 2bebb32c..a92b6b2a 100644 --- a/src/configs/resources.yaml +++ b/src/configs/resources.yaml @@ -58,7 +58,8 @@ resources: - name: maxmem type: numeric - description: "max memory in MB (built-in host attribute)" + unit: bytes + description: "max memory in bytes (built-in host attribute; CLI display converts to a human-readable unit)" - name: maxswap type: numeric diff --git a/src/configs/schema/resources.schema.json b/src/configs/schema/resources.schema.json index b11620a4..eaa3e835 100644 --- a/src/configs/schema/resources.schema.json +++ b/src/configs/schema/resources.schema.json @@ -13,6 +13,7 @@ "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 4194bd51..fab329d8 100644 --- a/src/controller/opget.c +++ b/src/controller/opget.c @@ -62,9 +62,20 @@ 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 2aaeeb36..0d11699f 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -1050,6 +1050,9 @@ static char* worker_status_str(uint32_t status) return "UNKOWN"; } +/* forward decl: defined near cmd_resval, below, which also uses it */ +static void format_bytes_human(double bytes, char* buf, size_t bufsize); + int cmd_workerlist(int argc, char* argv[]) { int rc, i; @@ -1068,11 +1071,19 @@ int cmd_workerlist(int argc, char* argv[]) "MaxSwap", "MaxTmp", "NDisks"); for (i = 0; i < rc; i++) { - printf("%-20s%-8s%-25s%-25s%12f%12d%12f%12f%12f%12d\n", + /* MaxMem is stored/wired as raw bytes (xce_resource_t.unit="bytes" + * for the maxmem resource definition) -- humanize for display only, + * every other column stays as-is. */ + char maxmem_buf[64]; + + format_bytes_human(workers[i]->attrs->maxmem, maxmem_buf, + sizeof(maxmem_buf)); + + printf("%-20s%-8s%-25s%-25s%12f%12d%12s%12f%12f%12d\n", workers[i]->name, worker_status_str(workers[i]->status), workers[i]->attrs->hosttype, workers[i]->attrs->hostmodel, workers[i]->attrs->cpufactor, workers[i]->attrs->maxcpus, - workers[i]->attrs->maxmem, workers[i]->attrs->maxswap, + maxmem_buf, workers[i]->attrs->maxswap, workers[i]->attrs->maxtmp, workers[i]->attrs->ndisks); } @@ -2024,6 +2035,28 @@ cleanup: return 0; } +/* + * 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. + */ +static void format_bytes_human(double bytes, char* buf, size_t bufsize) +{ + static const char* const UNITS[] = { "B", "KB", "MB", "GB", "TB", "PB" }; + const int nunits = (int)(sizeof(UNITS) / sizeof(UNITS[0])); + double val = bytes; + int i = 0; + + while (val >= 1024.0 && i < nunits - 1) { + val /= 1024.0; + i++; + } + + snprintf(buf, bufsize, "%.2f %s", val, UNITS[i]); +} + int cmd_resval(int argc, char* argv[]) { char* host = NULL; @@ -2065,7 +2098,15 @@ int cmd_resval(int argc, char* argv[]) printf("%s\n", val->val->u.svalue); break; case Value_u_fvalue: - printf("%f\n", val->val->u.fvalue); + if ((val->unit != NULL) && (strcmp(val->unit, "bytes") == 0)) { + char hbuf[64]; + + format_bytes_human(val->val->u.fvalue, hbuf, + sizeof(hbuf)); + printf("%f (%s)\n", val->val->u.fvalue, hbuf); + } else { + printf("%f\n", val->val->u.fvalue); + } break; case Value_u_bvalue: printf("%s\n", (val->val->u.bvalue == 0) ? "false" : "true"); diff --git a/src/controller/testsuite/test_worker_join.c b/src/controller/testsuite/test_worker_join.c index 08367d12..e7a4d620 100644 --- a/src/controller/testsuite/test_worker_join.c +++ b/src/controller/testsuite/test_worker_join.c @@ -17,6 +17,7 @@ #include #include #include +#include #include @@ -152,6 +153,132 @@ static float get_worker_res_fval(worker_t* w, const char* name) return (v != NULL) ? v->u.fvalue : -1.0f; } +/* + * Capture whatever update_resvals()/update_resval_list() print during a + * failing update_resval() call, as raw runtime output rather than a + * source-text scan. This test binary never calls mylog_init()/tlog_init() + * (only the real daemon main()s do -- see test_apply_correctness_ + * verification.c's own header comment, and the identical established + * precedent in scheduler/testsuite/test_scheduler_connect.c's + * capture_stdout_during_sched_init()), so tlog's internal root log handle + * stays NULL and every ERROR()/WARN() call falls through tlog's early- + * print path, which writes the fully formatted line directly to + * STDOUT_FILENO via a raw write() -- genuine runtime log capture, not a + * guess about which stream PERROR/ERROR happens to use. + */ +static char* capture_stdout_during(void (*fn)(void* ctx), void* ctx) +{ + char tmp[] = "/tmp/xce_worker_join_logcap_XXXXXX"; + int fd = mkstemp(tmp); + TEST_ASSERT_TRUE_MESSAGE(fd >= 0, + "fixture: mkstemp for stdout capture must succeed"); + + fflush(stdout); + int saved_stdout = dup(STDOUT_FILENO); + TEST_ASSERT_TRUE_MESSAGE(saved_stdout >= 0, + "fixture: dup(STDOUT_FILENO) must succeed"); + TEST_ASSERT_TRUE_MESSAGE(dup2(fd, STDOUT_FILENO) >= 0, + "fixture: dup2 redirect of STDOUT_FILENO must " + "succeed"); + + fn(ctx); + + fflush(stdout); + dup2(saved_stdout, STDOUT_FILENO); + close(saved_stdout); + + off_t len = lseek(fd, 0, SEEK_CUR); + TEST_ASSERT_TRUE_MESSAGE(len >= 0, + "fixture: lseek on capture file must succeed"); + + char* buf = (char*)calloc(1, (size_t)len + 1); + TEST_ASSERT_NOT_NULL(buf); + if (len > 0) { + TEST_ASSERT_TRUE_MESSAGE(lseek(fd, 0, SEEK_SET) == 0, + "fixture: rewind of capture file must " + "succeed"); + ssize_t nread = read(fd, buf, (size_t)len); + TEST_ASSERT_TRUE_MESSAGE(nread == len, + "fixture: reading back captured buffer " + "must succeed"); + } + close(fd); + unlink(tmp); + + return buf; +} + +typedef struct { + cluster_t* c; + worker_t* w; + xce_resval_t* val; +} update_resvals_call_ctx_t; + +static void call_update_resvals_1(void* ctx_) +{ + update_resvals_call_ctx_t* ctx = (update_resvals_call_ctx_t*)ctx_; + update_resvals(ctx->c, ctx->w, 1, ctx->val); +} + +/* + * update_resval() returns its failure reason as a raw negative code + * directly (key_find/zarray_put convention -- never via errno), so + * update_resvals()/update_resval_list() logging it via PERROR (which reads + * the process-global errno) reported whatever errno happened to be left + * over from an unrelated earlier call: "Success" if nothing had touched + * it, or a stray EAGAIN/etc from some other syscall in the same event-loop + * iteration -- never the real reason a resource update failed. This test + * reproduces that exact symptom (ambient errno=EAGAIN, unrelated to the + * genuine failure) and proves the fix decodes the actual rc instead. + */ +static void test_update_resvals_logs_actual_rc_not_stale_errno(void) +{ + setup_test_cluster(); + + hostattrs_t attrs; + memset(&attrs, 0, sizeof(attrs)); + attrs.hosttype = makeString("LINUX"); + attrs.hostmodel = makeString("x86"); + worker_t* w = make_test_worker("host-resval-log", &attrs); + TEST_ASSERT_NOT_NULL(w); + + xce_val_t fval; + memset(&fval, 0, sizeof(fval)); + fval.which = Value_u_fvalue; + fval.u.fvalue = 1.0f; + + xce_resval_t val; + val.name = "does-not-exist-in-resmgr"; + val.val = &fval; + + /* Simulate the exact reported symptom: ambient errno unrelated to the + * real failure, left over from some other unrelated call. */ + errno = EAGAIN; + + update_resvals_call_ctx_t ctx = { .c = &g_tc.c, .w = w, .val = &val }; + char* captured = capture_stdout_during(call_update_resvals_1, &ctx); + + TEST_ASSERT_NOT_NULL(captured); + TEST_ASSERT_NULL_MESSAGE( + strstr(captured, "Resource temporarily unavailable"), + "must not log the ambient errno's string (EAGAIN) -- that is not " + "why update_resval failed"); + TEST_ASSERT_NULL_MESSAGE( + strstr(captured, "Success"), + "must not log \"Success\" for a call that returned rc<0"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "does-not-exist-in-resmgr"), + "must name which resource failed"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "No such file or directory"), + "must log the real reason (-ENOENT, from key_find on an " + "unregistered resource name)"); + + free(captured); + teardown_test_cluster(); + printf("test_update_resvals_logs_actual_rc_not_stale_errno: PASS\n"); +} + /* ── Task 1.1: Tests for apply_join_state ──────────────────────────── */ /* @@ -295,6 +422,60 @@ static void test_apply_join_state_updates_resource_values(void) printf("test_apply_join_state_updates_resource_values: PASS\n"); } +/* + * hconfig.mxj == -1 is the schema's documented "auto-detect" sentinel + * (hosts.schema.json: "max_slot": {"minimum": -1, "description": "-1 means + * auto-detect"}) -- apply_join_state must fall back to the worker's own + * reported maxcpus, NOT persist -1 (or, before the xceadmin mapping fix, + * silently ship 0 and permanently disable scheduling on that host: mxj is + * UInt32 on the wire, so an unset/absent max_slot used to decode as 0, + * which is not < 0 and so never reached this fallback -- see + * xcecommon.mapping.HOSTCONFIG_MXJ_AUTO_DETECT). This is the ONLY test in + * this file that exercises the nslots<0 branch; every sibling test above + * uses a positive hconfig.mxj and so cannot catch a regression here. + */ +static void test_apply_join_state_mxj_negative_one_falls_back_to_maxcpus(void) +{ + setup_test_cluster(); + + hostattrs_t attrs; + memset(&attrs, 0, sizeof(attrs)); + attrs.hosttype = makeString("LINUX"); + attrs.hostmodel = makeString("x86"); + attrs.cpufactor = 1.0f; + attrs.maxcpus = 12; + attrs.maxmem = 64; + attrs.maxswap = 128; + attrs.maxtmp = 256; + attrs.ndisks = 2; + + worker_t* w = make_test_worker("host-ajs-autodetect", &attrs); + TEST_ASSERT_NOT_NULL(w); + + xce_hostconfig_t hconfig; + memset(&hconfig, 0, sizeof(hconfig)); + hconfig.hosttype = (char*)"!"; + hconfig.hostmodel = (char*)"!"; + hconfig.mxj = -1; + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + + lock(w, RTTI_LOCK_WRITE); + int rc = apply_join_state(&g_tc.c, w, &ep, &hconfig); + unlock(w); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE( + 12.0f, get_worker_res_fval(w, "slots"), + "mxj=-1 must auto-detect slots from the worker's reported maxcpus, " + "not persist -1 or silently leave slots at 0"); + + teardown_test_cluster(); + printf("test_apply_join_state_mxj_negative_one_falls_back_to_maxcpus: " + "PASS\n"); +} + /* ── Task 1.2: Tests for init_worker ────────────────────────────────── */ /* @@ -520,10 +701,13 @@ void test_worker_join(void) /* xstring_table_init must be called once before any makeString usage */ xstring_table_init(139); + RUN_TEST(test_update_resvals_logs_actual_rc_not_stale_errno); + /* Task 1.1: apply_join_state (shared status/resource update) */ RUN_TEST(test_apply_join_state_clears_unavail); RUN_TEST(test_apply_join_state_applies_hosttype_override); RUN_TEST(test_apply_join_state_updates_resource_values); + RUN_TEST(test_apply_join_state_mxj_negative_one_falls_back_to_maxcpus); /* Task 1.2: init_worker (first-time join) */ RUN_TEST(test_init_worker_creates_new_entry); diff --git a/src/controller/worker.c b/src/controller/worker.c index e0c4e3d6..069c88f2 100644 --- a/src/controller/worker.c +++ b/src/controller/worker.c @@ -115,7 +115,7 @@ static int update_resval(cluster_t* c, worker_t* w, xce_resval_t* val) } if (r == NULL) { ERROR("resource `%s' can not be found", val->name); - return -errno; + return -ENOENT; } if (r->idx < 0) { @@ -130,6 +130,24 @@ static int update_resval(cluster_t* c, worker_t* w, xce_resval_t* val) return rc; } +/* + * update_resval() returns its failure reason as a raw negative code + * directly (key_find/zarray_put convention throughout this codebase -- + * never via errno), so logging it with PERROR (which reads the process- + * global errno) reported whatever errno happened to be left over from an + * unrelated earlier call -- "Success" if nothing had touched it, or a + * stray EAGAIN/etc from some other syscall in the same event-loop + * iteration, never the real reason. Decode the actual rc instead. + */ +static void log_update_resval_failure(const char* name, int rc) +{ + char ebuf[128]; + + ebuf[0] = '\0'; + xce_strerror_r(-rc, ebuf, sizeof(ebuf)); + ERROR("update_resval `%s': %s", name, ebuf); +} + int update_resvals(cluster_t* c, worker_t* w, int nres, xce_resval_t* vals) { int rc = 0; @@ -138,7 +156,7 @@ int update_resvals(cluster_t* c, worker_t* w, int nres, xce_resval_t* vals) for (i = 0; i < nres; i++) { rc = update_resval(c, w, &(vals[i])); if (rc < 0) { - PERROR("update_resval"); + log_update_resval_failure(vals[i].name, rc); } } @@ -153,7 +171,7 @@ int update_resval_list(cluster_t* c, worker_t* w, xce_resval_list_t* l) for (i = 0; i < l->count; i++) { rc = update_resval(c, w, l->vals[i]); if (rc < 0) { - PERROR("update_resval"); + log_update_resval_failure(l->vals[i]->name, rc); } } diff --git a/src/include/xce/types.h b/src/include/xce/types.h index 4c0ee34a..19b2e337 100644 --- a/src/include/xce/types.h +++ b/src/include/xce/types.h @@ -49,6 +49,7 @@ typedef struct { char* name; char* host; xce_val_t* val; + char* unit; } xce_resval_t; typedef struct { @@ -142,6 +143,7 @@ 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 aa0b4aa2..866289f9 100644 --- a/src/lib/resobj.c +++ b/src/lib/resobj.c @@ -35,6 +35,7 @@ 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) { @@ -64,6 +65,7 @@ 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 42df5db3..83e9786c 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -209,6 +209,17 @@ 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"){ @@ -346,6 +357,9 @@ 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 3e6f95d8..f7d8cb89 100644 --- a/src/xceadmin/src/xcecommon/mapping.py +++ b/src/xceadmin/src/xcecommon/mapping.py @@ -71,9 +71,13 @@ 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. ``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 + 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 + (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 destination: the capnp fields it would map to (``distributions``, ``hostBased``) are commented out of the ``Resource`` struct @@ -95,13 +99,19 @@ reason -- never a silent drop. no corresponding ``QueueConf`` field at all. ``HostConfig`` (``hosts.yaml`` hosts) - ``description`` has no corresponding field. ``mxj`` is set only when - YAML ``max_slot >= 0``; the schema's documented ``-1`` ("auto-detect") - sentinel is left unmapped because ``mxj`` is ``UInt32`` and cannot - represent a negative value (verified: assigning ``-1`` raises - ``capnp.KjException`` rather than wrapping) -- shipping ``mxj`` unset - (default ``0``) preserves "not sourced" instead of crashing the build - or silently wrapping to ``4294967295``. + ``description`` has no corresponding field. ``mxj`` is ``UInt32`` on the + wire and cannot represent the schema's documented ``-1`` ("auto-detect + from CPU count") sentinel directly (verified: assigning ``-1`` raises + ``capnp.KjException`` rather than wrapping). When YAML ``max_slot`` is + absent or negative, ``mxj`` is instead set to ``HOSTCONFIG_MXJ_AUTO_DETECT`` + (``0xFFFFFFFF``) -- the controller's ``int mxj`` field receives this via + ``capn_read32()``'s implicit ``uint32_t`` -> ``int`` conversion, which + reinterprets it back to ``-1`` on this project's (two's-complement) + target platforms, so ``apply_join_state()``'s ``if (nslots < 0)`` + auto-detect fallback fires as intended. Previously this left ``mxj`` + unset (default ``0``), which is indistinguishable from "explicitly + configure 0 slots" -- silently disabling scheduling on every host that + relies on auto-detect (bug, fixed). ``GroupConfig`` used as an embedded ``Membership`` (``QueueConf.users``/ ``.hosts``, ``LimitConfig.users``/``.queues``/``.projects``/``.hosts``) @@ -318,6 +328,8 @@ 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: @@ -341,6 +353,20 @@ def _build_queue(item: dict, target: "capnp._DynamicStructBuilder") -> None: _build_time_window(item.get("run_window"), target.runWindow) +HOSTCONFIG_MXJ_AUTO_DETECT = 0xFFFFFFFF +"""Wire bit-pattern for the ``max_slot: -1`` ("auto-detect from CPU count") +sentinel. ``mxj`` is ``UInt32`` on the wire, so the negative sentinel itself +cannot be assigned directly (pycapnp raises ``capnp.KjException`` for a +negative value) -- but ``0xFFFFFFFF`` is a valid in-range ``UInt32`` value +that the controller's ``int mxj`` field (``xce/types.h``) receives via +``capn_read32()``'s implicit ``uint32_t`` -> ``int`` conversion, which on +this project's target platforms (two's-complement) reinterprets it back to +``-1`` -- reconstructing the sentinel losslessly end-to-end. See module +docstring, ``HostConfig``, for why this is unset-vs-sentinel and not a +schema change. +""" + + def _build_host_config(item: dict, target: "capnp._DynamicStructBuilder") -> None: """Populate one ``HostConfig`` list item from a ``hosts.yaml`` ``hosts`` entry. See module docstring, ``HostConfig``, for what is intentionally @@ -354,6 +380,8 @@ def _build_host_config(item: dict, target: "capnp._DynamicStructBuilder") -> Non max_slot = item.get("max_slot") if max_slot is not None and max_slot >= 0: target.mxj = max_slot + else: + target.mxj = HOSTCONFIG_MXJ_AUTO_DETECT _build_config_var_list(item.get("resource"), target) _build_threshold_list(item.get("sched_stop"), target, "thresholds") _build_time_window(item.get("dispatch_window"), target.dispatchwindows) diff --git a/src/xceadmin/tests/fixtures/full/expected.bin b/src/xceadmin/tests/fixtures/full/expected.bin index cfb8788b31f17101bf1980a77dece44758a77014..83feb28659a56ca3b5fabe5e0ecc5a7bcb21c74a 100644 GIT binary patch delta 517 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; sI9D<@2h0Ee|Nn1x;VoeY02UZ&wEzGB delta 474 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? wS;@%2&;_!ccOl4B1_p+uj0_C3CO%Z=oeR|iqMJ6?vQ~4jfTCjaAKnsn07L^*(EtDd diff --git a/src/xceadmin/tests/test_mapping.py b/src/xceadmin/tests/test_mapping.py index 90ad6487..8694874c 100644 --- a/src/xceadmin/tests/test_mapping.py +++ b/src/xceadmin/tests/test_mapping.py @@ -173,6 +173,20 @@ def test_map_list_section_noop_on_empty_or_missing_items(cluster_config): assert len(cluster_config.resources) == 0 +def test_map_list_section_resources_maps_unit_when_present(cluster_config): + items = [ + {"name": "maxmem", "type": "numeric", "unit": "bytes"}, + {"name": "maxcpus", "type": "numeric"}, + ] + mapping.map_list_section(_spec_for("resources"), items, cluster_config) + + r0, r1 = cluster_config.resources + assert r0.unit == "bytes" + # Every existing resource has no "unit" key -- must stay unset (empty + # Text default), not silently inherit or default to any value. + assert r1.unit == "" + + # --------------------------------------------------------------------------- # map_list_section: users.yaml/hosts.yaml -> ugroups/hgroups (named groups) # --------------------------------------------------------------------------- @@ -247,9 +261,11 @@ def test_map_list_section_host_config_full_field_mapping(cluster_config): assert h.name == "default" assert h.type == "x86_64-linux-gnu" assert h.model == "x86_64-linux-gnu" - # max_slot == -1 (auto-detect sentinel) cannot be represented by - # UInt32 mxj -- deliberately left at default (see module docstring). - assert h.mxj == 0 + # max_slot == -1 (auto-detect sentinel) cannot be represented directly + # by UInt32 mxj -- mapped to the 0xFFFFFFFF bit-pattern instead, which + # the controller's `int mxj` field reinterprets back to -1 (see module + # docstring and HOSTCONFIG_MXJ_AUTO_DETECT). + assert h.mxj == mapping.HOSTCONFIG_MXJ_AUTO_DETECT assert [r.name for r in h.resources] == ["mg", "maxswap"] assert h.resources[0].value == "" @@ -275,6 +291,22 @@ def test_map_list_section_host_config_sets_mxj_when_max_slot_nonnegative(cluster assert cluster_config.hostConfigs[0].mxj == 256 +def test_map_list_section_host_config_sets_mxj_auto_detect_sentinel_when_max_slot_absent( + cluster_config, +): + """The production case: a host entry with no `max_slot` key at all + (the normal way to request auto-detect, per hosts.schema.json) must + NOT silently ship mxj=0 -- that reads to the controller as "explicitly + configure 0 slots" and permanently blocks scheduling on that host + (worker.c apply_join_state: `if (nslots < 0) nslots = maxcpus;` never + fires for 0). It must ship the 0xFFFFFFFF sentinel instead.""" + items = [{"hostname": "worker3"}] + mapping.map_list_section(_spec_for("hostConfigs"), items, cluster_config) + + assert cluster_config.hostConfigs[0].mxj == mapping.HOSTCONFIG_MXJ_AUTO_DETECT + assert mapping.HOSTCONFIG_MXJ_AUTO_DETECT != 0 + + def test_time_window_day_prefixed_spec_parses_day_hour_minute(cluster_config): items = [ {