docs: ServiceHosting.md overlay-filename casing note; R3-04 review outcome (both FIX-FIRST items resolved)
This commit is contained in:
Joseph Doherty
@@ -552,3 +552,106 @@ cycle would have been (both derivations are proven against literally identical i
|
||||
nodes' role sources interleaves site-a-2 and site-b-2 on adjacent lines, so a clean git-level split
|
||||
into the plan's two separate commits was not practical; both tasks' edits landed in one commit
|
||||
(`985de0b6`). The rig was NOT brought up as part of either task, per the brief.
|
||||
|
||||
## Review outcome
|
||||
|
||||
A code review after T1–T12 returned verdict **FIX-FIRST**, two items. Both are fixed, each in its
|
||||
own TDD commit, on top of the T1–T12 work above.
|
||||
|
||||
### 1. [Blocking] T7's post-overlay guard was structurally unreachable
|
||||
|
||||
**The finding.** `AkkaClusterOptionsValidator.ValidateAgainstEffectiveRoles` compared
|
||||
`options.Roles` against `_effectiveRoles.Gossiped` — but `ServiceCollectionExtensions.cs`'s
|
||||
PostConfigure (T2's OWN structural-identity fix) unconditionally sets
|
||||
`options.Roles = [.. effectiveRoles.Gossiped]` first. The two values are therefore **equal by
|
||||
construction**, on every real host, always — the guard could never observe a divergence no matter
|
||||
what a role overlay did to `Cluster:Roles`. My original two tests (now deleted) hand-built an
|
||||
`AkkaClusterOptions` instance directly and set `.Roles` to a "mismatched" value bypassing
|
||||
PostConfigure entirely — which exercised the comparison logic in isolation but never proved the
|
||||
guard reachable through the real pipeline. **Concretely, before this fix: an overlay that set
|
||||
`Cluster:Roles` was silently ignored (PostConfigure overwrote it back to the pre-overlay value), not
|
||||
failed at boot** — the exact opposite of the guard's purpose (choice 4: "a role overlay may not set
|
||||
Cluster:Roles").
|
||||
|
||||
**The fix.** `AkkaClusterOptionsValidator` now also takes an optional `IConfiguration?` (default
|
||||
null, same pattern as `EffectiveRoles?` — so every pre-existing zero/one-arg test construction keeps
|
||||
compiling and behaving as before). The guard re-reads the RAW `Cluster:Roles` section directly off
|
||||
the LIVE configuration (`_configuration.GetSection(EffectiveRoles.ConfiguredRolesKey).Get<string[]>()`)
|
||||
rather than the bound options, normalizes it by reusing `EffectiveRoles.Resolve(raw, envRoles: null)`
|
||||
(one rule, stated once — archreview theme 4, rather than re-implementing the configured-path
|
||||
normalization a second time), and compares that against `_effectiveRoles.Gossiped`. Skipped entirely
|
||||
when the raw post-overlay value is empty — required for correctness, not merely tidiness: an
|
||||
`OTOPCUA_ROLES`-only node (most of production, per CLAUDE.md) has an empty `Cluster:Roles` both
|
||||
before AND after any overlay, and comparing "empty" against a non-empty env-derived `Gossiped` list
|
||||
would be a permanent false positive on every one of those nodes. `AddOtOpcUaCluster` now also
|
||||
`TryAddSingleton(configuration)` so a bare `ServiceCollection` test harness (not just
|
||||
`WebApplicationBuilder`, which auto-registers `IConfiguration`) can supply it.
|
||||
|
||||
**The tests.** Both guard tests are rewritten to go through the REAL `AddOtOpcUaCluster` +
|
||||
PostConfigure pipeline, over a `ConfigurationManager` (not a one-shot `ConfigurationBuilder`) — the
|
||||
same "add a provider to the SAME live object after the fact" shape `WebApplicationBuilder.Configuration`
|
||||
has, which is what lets a role overlay be simulated faithfully: `EffectiveRoles.Resolve(configuration)`
|
||||
is called and its result registered BEFORE a second `AddInMemoryCollection` call mutates the same
|
||||
`configuration` object with different `Cluster:Roles` values, mirroring `Program.cs`'s own
|
||||
resolve-then-`AddJsonFile(overlay)` sequencing exactly. Confirmed red first: the "changes" test
|
||||
failed with "should throw OptionsValidationException but did not" against the pre-fix (options.Roles
|
||||
-comparing) validator — direct proof of the reviewer's structural-unreachability claim. Two
|
||||
supporting tests were added beyond the two named in the review comment: `No_overlay_at_all_passes`
|
||||
(the ordinary case) and `EffectiveRoles_injected_without_IConfiguration_skips_the_guard_entirely`
|
||||
(the two-optional-dependency ctor's other skip leg). One self-inflicted false-positive surfaced and
|
||||
was fixed along the way: the new catch-block's error message split `"Cluster:Roles."` onto its own
|
||||
string-literal token, which `RoleAuthorityGuardTests`' own space-inside-the-literal heuristic (T8)
|
||||
correctly flagged as NOT looking like an operator sentence — merging it back into one literal
|
||||
containing surrounding words fixed it without weakening the guard.
|
||||
|
||||
### 2. [Important] `ClusterRoleInfo._roleLeaders` must stay `Ordinal`, not `OrdinalIgnoreCase`
|
||||
|
||||
**The finding.** T5 moved BOTH `_roleLeaders` and `_membersByRole` to `OrdinalIgnoreCase` under one
|
||||
shared rationale (defence in depth against a miscased rolling-upgrade peer). That rationale is right
|
||||
for `_membersByRole` ("every member carrying a spelling of this role" — merging casings is the
|
||||
point) but wrong for `_roleLeaders`: Akka computes — and raises `RoleLeaderChanged` for — role
|
||||
leadership **per exact gossiped role string**. "driver" and "Driver" are two entirely distinct roles
|
||||
to Akka's own role-leader machinery, each independently electing its own leader. Keying
|
||||
`_roleLeaders` case-insensitively collapses both onto ONE dictionary slot, so whichever
|
||||
`RoleLeaderChanged` event landed last silently overwrites the other's leader — turning two real,
|
||||
independent facts Akka tracks separately into one, wrong, merged fact.
|
||||
|
||||
**The fix.** `_roleLeaders` reverted to `StringComparer.Ordinal`; `_membersByRole` stays
|
||||
`OrdinalIgnoreCase` (the split itself, not merely one value, is now documented in the field
|
||||
comment). No other production call site touches `_roleLeaders`'s comparer, so this is a one-line
|
||||
revert plus doc.
|
||||
|
||||
**The test.** `ClusterRoleInfoRoleLeaderCaseTests.RoleLeaderChanged_for_driver_then_Driver_keeps_distinct_entries`
|
||||
— a real two-node cluster (the only faithful option: `HandleRoleLeaderChanged` is `internal` with no
|
||||
`InternalsVisibleTo`, so it cannot be unit-tested directly, and `ClusterRoleInfo`'s constructor needs
|
||||
a genuinely clustered `ActorSystem`). Node A gossips only `"driver"`, node B (joining the same
|
||||
cluster) gossips only `"Driver"` — each is therefore the sole, and so leader, member of its own
|
||||
exact-string role. Confirmed red first: against the pre-fix (`OrdinalIgnoreCase`) comparer, the test
|
||||
failed with `driverLeader` and `capitalDriverLeader` resolving to the SAME address — the exact
|
||||
collision the review predicted, reproduced live rather than merely reasoned about.
|
||||
|
||||
### Optional item — done
|
||||
|
||||
Added one sentence to `docs/ServiceHosting.md`'s "Per-role configuration overlays" section: a plain
|
||||
role's overlay-filename fragment is always lower-case regardless of how it was spelled in
|
||||
`Cluster:Roles`, but a `cluster-{ClusterId}` role's suffix carries into the filename verbatim (since
|
||||
`EffectiveRoles.Gossiped` preserves that spelling by design) — e.g.
|
||||
`Cluster:Roles: ["driver", "cluster-SITE-A"]` loads `appsettings.cluster-SITE-A-driver.json`.
|
||||
|
||||
### Verification
|
||||
|
||||
`dotnet build ZB.MOM.WW.OtOpcUa.slnx` → 0 errors throughout.
|
||||
|
||||
- After item 2 (`ClusterRoleInfo`): `Cluster.Tests` 241 → **242/242** (+1,
|
||||
`ClusterRoleInfoRoleLeaderCaseTests`).
|
||||
- After item 1 (T7 guard): `Cluster.Tests` 242 → **244/244** (net +2: the two old hand-built T7
|
||||
tests deleted, four new ones added —
|
||||
`An_overlay_that_changes_Cluster_Roles_after_resolution_fails_naming_both_views`,
|
||||
`An_overlay_that_repeats_the_same_Cluster_Roles_passes`, `No_overlay_at_all_passes`,
|
||||
`EffectiveRoles_injected_without_IConfiguration_skips_the_guard_entirely` — the pre-existing
|
||||
`No_EffectiveRoles_injected_skips_the_guard_entirely` was kept unchanged).
|
||||
- `Host.Tests` **106/106**, unchanged by either fix (`Program.cs` itself was not touched).
|
||||
- `ControlPlane.Tests` **267/267** — an extra safety check beyond what was asked, since
|
||||
`ClusterRoleInfo` is a shared dependency of `RedundancyStateActor` et al.
|
||||
|
||||
No pushes, no merges.
|
||||
|
||||
@@ -32,7 +32,7 @@ Single-node dev: `OTOPCUA_ROLES=admin,driver` (or the `Cluster:Roles` equivalent
|
||||
|
||||
### Per-role configuration overlays
|
||||
|
||||
`Program.cs` builds a role suffix by joining the resolved `EffectiveRoles.Gossiped` roles **alphabetically** with `-` and loads `appsettings.{roleSuffix}.json` as an optional overlay on top of base `appsettings.json`. Three overlays ship in `src/Server/ZB.MOM.WW.OtOpcUa.Host/`:
|
||||
`Program.cs` builds a role suffix by joining the resolved `EffectiveRoles.Gossiped` roles **alphabetically** with `-` and loads `appsettings.{roleSuffix}.json` as an optional overlay on top of base `appsettings.json`. A plain role (`admin`/`driver`/`dev`) is always lower-case in `Gossiped` regardless of how the operator spelled it, so the overlay filename for those is stable (`appsettings.driver.json`); if a `cluster-{ClusterId}` role is also configured, its suffix carries into the filename **verbatim** (e.g. `Cluster:Roles: ["driver", "cluster-SITE-A"]` loads `appsettings.cluster-SITE-A-driver.json`), since `Gossiped` preserves that spelling by design (see [Configuration.md § `Roles`](Configuration.md#appsettings-sections)). Three overlays ship in `src/Server/ZB.MOM.WW.OtOpcUa.Host/`:
|
||||
|
||||
- `appsettings.admin.json` — admin-only nodes
|
||||
- `appsettings.driver.json` — driver-only nodes
|
||||
|
||||
Reference in New Issue
Block a user