docs(resource-value-verification): close design-review doc/comment gaps
Two retrospective design-review findings: - design.md's System Flows sequence diagram showed the controller-layer resval check as an independently-launched client session, implying no dependency on the injecting worker staying connected. As-built (bug-1579), host-attrs resources reset to baseline the instant the worker disconnects, so the check actually runs nested inside the still-open worker session via xce_test's `!` shell-escape. Updated the diagram and prose to show this, and documented the still-open residual risk for custom host-attrs resource names in Error Handling. - Added explicit "do not change this format" warning comments at both trace-log call sites (core_events.c's RESVALS_DELTA case, apiserver's OnResourceLocationUpdated) that xce-test-resources' log-grep depends on -- an unrelated logging cleanup at either site would otherwise silently turn every future verification run into a false UNREACHABLE/timeout. No behavior change in either source file -- comments only.
This commit is contained in:
parent
e52c68f471
commit
1bc65d2c47
3 changed files with 31 additions and 5 deletions
|
|
@ -138,8 +138,8 @@ src/tools/
|
||||||
```mermaid
|
```mermaid
|
||||||
sequenceDiagram
|
sequenceDiagram
|
||||||
participant Skill as xce-test-resources
|
participant Skill as xce-test-resources
|
||||||
participant Worker as xce_test (worker ctx)
|
participant Worker as xce_test (worker ctx, ONE session)
|
||||||
participant Client as xce_test (client ctx)
|
participant Nested as xce_test (nested ! client ctx)
|
||||||
participant Ctrl as controller
|
participant Ctrl as controller
|
||||||
participant Sched as scheduler daemon
|
participant Sched as scheduler daemon
|
||||||
participant Api as apiserver
|
participant Api as apiserver
|
||||||
|
|
@ -152,9 +152,12 @@ sequenceDiagram
|
||||||
Ctrl->>Api: notify schedUpdateResvals
|
Ctrl->>Api: notify schedUpdateResvals
|
||||||
Sched->>Sched: apply + trace-log resource, host, value
|
Sched->>Sched: apply + trace-log resource, host, value
|
||||||
Api->>Api: store.ResourceLocations.Put + debug-log resource, host, value
|
Api->>Api: store.ResourceLocations.Put + debug-log resource, host, value
|
||||||
Skill->>Client: resval -n <res> -m synthetic-worker
|
Worker->>Nested: !<shell-cmd> --internal-controller-check (worker STILL connected)
|
||||||
Client->>Ctrl: query
|
Nested->>Ctrl: resval -n <res> -m synthetic-worker (query)
|
||||||
Ctrl-->>Skill: controller-observed value
|
Ctrl-->>Nested: controller-observed value
|
||||||
|
Nested-->>Worker: result written to a marker file (nested exit code ignored)
|
||||||
|
Worker->>Ctrl: quit (worker disconnects HERE -- not before)
|
||||||
|
Skill->>Skill: parse marker file for controller-observed value
|
||||||
Skill->>Sched: tail/grep scheduler log for expected line (bounded wait)
|
Skill->>Sched: tail/grep scheduler log for expected line (bounded wait)
|
||||||
Sched-->>Skill: scheduler-observed value (or timeout)
|
Sched-->>Skill: scheduler-observed value (or timeout)
|
||||||
Skill->>Api: tail/grep apiserver log for expected line (bounded wait)
|
Skill->>Api: tail/grep apiserver log for expected line (bounded wait)
|
||||||
|
|
@ -162,6 +165,8 @@ sequenceDiagram
|
||||||
Skill->>Skill: compare all three vs injected value, emit verdict
|
Skill->>Skill: compare all three vs injected value, emit verdict
|
||||||
```
|
```
|
||||||
|
|
||||||
|
**Load-bearing constraint the diagram makes explicit (as-built, bug-1579)**: the controller-layer check is NOT an independently-launched client session run after the worker disconnects. Any host-attrs-derived resource (`type`/`model`/`cpuf`/`maxcpus`/`maxmem`/`maxswap`/`maxtmp`/`ndisks` — `worker.c`'s `N_HOST_ATTRS`) is reset to baseline by the controller the instant the injecting worker disconnects — so a controller-layer check run as a separate, later session against any host-attrs name would ALWAYS false-MISMATCH, independent of whether the underlying propagation is actually correct (discovered as bug-1579, task 6.2, round-1 REJECTED). The fix nests the controller-layer `resval` query INSIDE the injecting worker's own still-open session, via `xce_test`'s `!<shell-command>` escape (`preprocess_and_run()` — works in any REPL context, including `worker`, with no `leave_ctx()`/disconnect) — the query genuinely runs before `quit`, not merely before some unrelated timeout. The scheduler and apiserver legs have no such constraint: both are pure log-tail/grep against a file, entirely independent of whether the worker connection is still open, so they run after the worker session ends like any other step.
|
||||||
|
|
||||||
Key simplification from the superseded design: no `record`/`replay`/REPL round-trip and no REST/`xcecli` query are needed — the scheduler and apiserver legs both poll their own live daemon's log file for the trace line the injection is expected to produce, within a bounded wait, exactly like the controller leg's own bounded-wait posture. This removes the REPL's `step`/`continue` pitfall entirely (there is no replay step in this flow) and the apiserver's auth/RBAC config from the critical path (see research.md Design Decisions).
|
Key simplification from the superseded design: no `record`/`replay`/REPL round-trip and no REST/`xcecli` query are needed — the scheduler and apiserver legs both poll their own live daemon's log file for the trace line the injection is expected to produce, within a bounded wait, exactly like the controller leg's own bounded-wait posture. This removes the REPL's `step`/`continue` pitfall entirely (there is no replay step in this flow) and the apiserver's auth/RBAC config from the critical path (see research.md Design Decisions).
|
||||||
|
|
||||||
## Requirements Traceability
|
## Requirements Traceability
|
||||||
|
|
@ -343,6 +348,7 @@ This feature is a verification tool, not a production service — "errors" are p
|
||||||
- **Layer mismatch** (Requirements 3.2, 4.2, 5.2): the skill reports which layer(s) failed, with both the injected and observed value, per Requirement 6.3.
|
- **Layer mismatch** (Requirements 3.2, 4.2, 5.2): the skill reports which layer(s) failed, with both the injected and observed value, per Requirement 6.3.
|
||||||
- **Layer unreachable** (e.g. apiserver never came up, or the expected trace line never appears in the scheduler/apiserver log before the bounded wait elapses): treated the same as a mismatch for verdict purposes (FAIL, layer identified, "timeout" distinguished from "mismatch" in the report), not a silent skip.
|
- **Layer unreachable** (e.g. apiserver never came up, or the expected trace line never appears in the scheduler/apiserver log before the bounded wait elapses): treated the same as a mismatch for verdict purposes (FAIL, layer identified, "timeout" distinguished from "mismatch" in the report), not a silent skip.
|
||||||
- **Deployment failure** (controller/scheduler/apiserver fails to start): the skill aborts before attempting injection, with a clear "deployment failed at step X" message. **Cleanup still runs on this path** — this is a deliberate strengthening of `xce-deploy-ha`'s precedent, not a mirror of it: `xce-deploy-ha`'s `deploy-ha.sh` has no trap-based cleanup and leaves already-started processes running on a mid-deploy failure, requiring the caller to run its separate `cleanup.sh` manually (confirmed against `src/tools/deploy-ha.sh`). Requirement 6.5 promises cleanup by default regardless of outcome, unless the operator explicitly requested the deployment be left running — a deployment failure is not that request, so the skill installs a trap (or equivalent) covering every deployment step from the first process start onward, not just the post-injection verification path.
|
- **Deployment failure** (controller/scheduler/apiserver fails to start): the skill aborts before attempting injection, with a clear "deployment failed at step X" message. **Cleanup still runs on this path** — this is a deliberate strengthening of `xce-deploy-ha`'s precedent, not a mirror of it: `xce-deploy-ha`'s `deploy-ha.sh` has no trap-based cleanup and leaves already-started processes running on a mid-deploy failure, requiring the caller to run its separate `cleanup.sh` manually (confirmed against `src/tools/deploy-ha.sh`). Requirement 6.5 promises cleanup by default regardless of outcome, unless the operator explicitly requested the deployment be left running — a deployment failure is not that request, so the skill installs a trap (or equivalent) covering every deployment step from the first process start onward, not just the post-injection verification path.
|
||||||
|
- **Custom host-attrs resource name false-MISMATCH (known residual risk, not fully closed)**: the default resource set avoids every host-attrs-derived name (`type`/`model`/`cpuf`/`maxcpus`/`maxmem`/`maxswap`/`maxtmp`/`ndisks` — `worker.c`'s `N_HOST_ATTRS`) specifically to route around this; the System Flows section above documents the nested-`!`-escape fix and why it closes the controller-layer check for that default set. If an operator passes a CUSTOM `resource=value` argument naming a host-attrs resource, the controller-layer check will still report a false MISMATCH — not because propagation is wrong, but because that resource is reset to baseline the instant the worker session ends, and no code path today re-checks it before that reset for an arbitrary caller-supplied name. This is a known, documented gap (tasks.md task 6.2), not silently swallowed: a genuine fix needs either a C-level `xce_test` change (no REPL path from `worker` context to `client` context without an equivalent disconnect) or a bash coprocess restructuring to keep the worker connected across every layer's check — out of scope for the current implementation. The skill should warn (not silently mismatch) when a caller-supplied resource name matches a known host-attrs name.
|
||||||
|
|
||||||
### Monitoring
|
### Monitoring
|
||||||
Not applicable — this is an on-demand, operator-invoked CLI tool, not a running service. Logs are written under the run's scratch directory, following `xce-deploy-ha`/`xce-test-ha`'s existing `logs/` convention, for post-run inspection.
|
Not applicable — this is an on-demand, operator-invoked CLI tool, not a running service. Logs are written under the run's scratch directory, following `xce-deploy-ha`/`xce-test-ha`'s existing `logs/` convention, for post-run inspection.
|
||||||
|
|
|
||||||
|
|
@ -99,6 +99,16 @@ func (h *DefaultHandler) OnResourceLocationUpdated(res *models.ResourceDistribut
|
||||||
// field carrying whichever of StringValue/NumericValue/BooleanValue is
|
// field carrying whichever of StringValue/NumericValue/BooleanValue is
|
||||||
// live for res.ValueType, so a downstream grep sees exactly what was
|
// live for res.ValueType, so a downstream grep sees exactly what was
|
||||||
// stored, not every possibly-zero field on the struct.
|
// stored, not every possibly-zero field on the struct.
|
||||||
|
//
|
||||||
|
// DO NOT change the "resource-location stored" message string, or the
|
||||||
|
// resource/host/value field names below, without also updating
|
||||||
|
// query_apiserver_resvals() in
|
||||||
|
// .claude/skills/xce-test-resources/run-resource-verify.sh (design.md's
|
||||||
|
// own Revalidation Triggers list this exact pair) -- that script greps
|
||||||
|
// this exact message and parses zap's console-encoder trailing JSON
|
||||||
|
// block for these field names; an unrelated logging cleanup here
|
||||||
|
// silently turns every future verification run into a false
|
||||||
|
// UNREACHABLE/timeout, indistinguishable from a real regression.
|
||||||
var valueField zap.Field
|
var valueField zap.Field
|
||||||
switch res.ValueType {
|
switch res.ValueType {
|
||||||
case int(models.STRING):
|
case int(models.STRING):
|
||||||
|
|
|
||||||
|
|
@ -835,6 +835,16 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events,
|
||||||
* Debug/trace level only (tlog_debug3 via TRACE) -- no
|
* Debug/trace level only (tlog_debug3 via TRACE) -- no
|
||||||
* control-flow change, fires on every applied delta.
|
* control-flow change, fires on every applied delta.
|
||||||
*
|
*
|
||||||
|
* DO NOT change the "resval applied host=... resource=...
|
||||||
|
* value=..." prefix (either branch below) without also
|
||||||
|
* updating query_scheduler_resvals() in
|
||||||
|
* .claude/skills/xce-test-resources/run-resource-verify.sh
|
||||||
|
* (design.md's own Revalidation Triggers list this exact
|
||||||
|
* pair) -- an unrelated logging cleanup that reflows this
|
||||||
|
* text silently turns every future verification run into a
|
||||||
|
* false UNREACHABLE/timeout, indistinguishable from a real
|
||||||
|
* propagation regression without investigation.
|
||||||
|
*
|
||||||
* bug-1573 fix (round-2 remediation): Value_u_which has 7
|
* bug-1573 fix (round-2 remediation): Value_u_which has 7
|
||||||
* tags (ivalue32/uvalue32/ivalue64/uvalue64/svalue/fvalue/
|
* tags (ivalue32/uvalue32/ivalue64/uvalue64/svalue/fvalue/
|
||||||
* bvalue -- controller.capnp.h) and Value_u_bvalue is a real,
|
* bvalue -- controller.capnp.h) and Value_u_bvalue is a real,
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue