diff --git a/docs/plans/2026-07-21-per-cluster-mesh-design.md b/docs/plans/2026-07-21-per-cluster-mesh-design.md index 4b83db54..a2f9036c 100644 --- a/docs/plans/2026-07-21-per-cluster-mesh-design.md +++ b/docs/plans/2026-07-21-per-cluster-mesh-design.md @@ -1,9 +1,10 @@ # Per-Cluster Akka Mesh — design -> **Status:** design for review. No implementation planned yet. +> **Status:** design for review. **Phases 0a and 0b are DONE and merged** (2026-07-21); Phases 1–7 +> are not started and each needs its own plan. > **Decisions settled 2026-07-21** — see §6: one central DB with driver nodes disconnected from it, -> the two-node keep-oldest gap CLOSED upstream and still live here (§6.2 — most urgent item), and -> their auth posture matched for now. +> the two-node keep-oldest gap closed upstream and now closed here too (§6.2), and their auth posture +> matched for now. ## 1. What this changes @@ -142,6 +143,12 @@ the dual-primary shape archreview 03/S4 exists to prevent. and should not wait for this design.** It also happens to be exactly what the separate-mesh model needs, so it is not throwaway work. +**FIXED 2026-07-21 (Phase 0b).** `RedundancyStateActor.BuildSnapshot` selects the oldest Up member +carrying the `driver` role, matching singleton placement; `NodeRedundancyState.IsRoleLeaderForDriver` +was renamed `IsDriverPrimary` so the contract stops asserting the old derivation. Pinned by +`RedundancyPrimaryElectionTests`, which builds a real two-node cluster in which the oldest node holds +the *higher* address, so the two orderings genuinely disagree. + ## 5. Target architecture Mirror ScadaBridge's shape, adapted to OtOpcUa's existing substrate: @@ -231,11 +238,20 @@ Its commit message carries the analysis, live-proven on their rig: `keep-oldest { down-if-alone = on }`, `stable-after = 15s`, with `BuildClusterOptions` setting the matching typed `KeepOldestOption { DownIfAlone = true }`. -So a two-node OtOpcUa pair **cannot fail over when the oldest node crashes**: the survivor downs -itself. That is a total-outage defect in the redundancy feature itself, it is independent of this -design, and the sister project has already done the analysis and shipped the fix. **It should be -Phase 0 alongside the oldest-Up role derivation**, and it needs its own live gate — a -crash-the-oldest drill, since the failure only appears in the 1-vs-1 case. +So a two-node OtOpcUa pair **could not fail over when the oldest node crashed**: the survivor +downed itself. That was a total-outage defect in the redundancy feature itself, independent of this +design. + +**FIXED 2026-07-21 (Phase 0a).** `Cluster:SplitBrainResolverStrategy` now selects the downing +provider and defaults to `auto-down` — Akka's `AutoDowning` with a 15s window — so the leader among +the reachable members downs the unreachable peer and a crash of either node fails over. `keep-oldest` +remains selectable for deployments that prefer partition-safety. Pinned by +`SplitBrainResolverActivationTests`, which asserts the provider read back off a running ActorSystem +rather than the configured one. See `docs/Redundancy.md`. + +**Its live gate is still outstanding**, and cannot run on today's rig: the failure only appears in a +1-vs-1 split, and `docker-dev` is a single six-node mesh. The crash-the-oldest drill therefore lands +with Phase 6/7, which makes every mesh exactly two nodes. ### 6.3 DECIDED — match ScadaBridge's auth posture for now @@ -250,8 +266,8 @@ Deliberately not a task plan — per-phase plans follow, one at a time. | Phase | Content | Independent of the split? | |---|---|---| -| 0a | Downing strategy: keep-oldest cannot survive an oldest-node crash in a 2-node cluster (§6.2) | **Yes** — most urgent | -| 0b | Oldest-Up role derivation (§4) | **Yes** | +| 0a | Downing strategy: keep-oldest cannot survive an oldest-node crash in a 2-node cluster (§6.2) | **Yes** — **DONE 2026-07-21**, live gate deferred to Phase 7 | +| 0b | Oldest-Up role derivation (§4) | **Yes** — **DONE 2026-07-21** | | 1 | `ClusterNode` gains Akka + gRPC address columns; coordinator sources its expected-ack set from the DB | Yes | | 2 | Comm actors + receptionist registration; ClusterClient transport; deploy notify + acks across the boundary | No | | 3 | Config fetch-and-cache: artifact served by central, driver nodes read LocalDb (§6.1) | No | diff --git a/docs/plans/2026-07-21-per-cluster-mesh-phase1.md b/docs/plans/2026-07-21-per-cluster-mesh-phase1.md new file mode 100644 index 00000000..1c749f87 --- /dev/null +++ b/docs/plans/2026-07-21-per-cluster-mesh-phase1.md @@ -0,0 +1,191 @@ +# Per-Cluster Mesh — Phase 1 plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:executing-plans to implement this plan task-by-task. + +**Parent design:** [`2026-07-21-per-cluster-mesh-design.md`](2026-07-21-per-cluster-mesh-design.md). +Phases 0a and 0b are done and merged; this is the next phase. + +**Goal:** Give `ClusterNode` the transport addresses central will need to reach each cluster +directly, and cut `ConfigPublishCoordinator`'s last dependency on shared cluster membership by +sourcing its expected-ack set from those rows. + +**Architecture:** Additive schema change plus one substitution inside the coordinator. Nothing else +moves; the deploy channel stays on DistributedPubSub until Phase 2. + +**Status:** NOT STARTED. Read §"The decision this phase forces" before executing — it changes +deploy-seal semantics and should be confirmed, not assumed. + +--- + +## Why this is smaller than the design implies + +The design says "`ClusterNode` gains Akka + gRPC address columns; coordinator sources its +expected-ack set from the DB", which reads as though the two are one change. They are not, and the +second does not depend on the first. + +`ConfigPublishCoordinator.DiscoverDriverNodes` builds `NodeId.Parse($"{host}:{port}")` from +`Cluster.State.Members`. `ClusterNode.NodeId` is **already in that identifier space** — the rig seeds +`central-1:4053`, `site-a-1:4053` and so on (`docker-dev/seed/seed-clusters.sql:65`), and its comment +says so explicitly, because `NodeDeploymentState.NodeId` is FK-bound to `ClusterNode.NodeId` and a +mismatch throws FK 547 on deploy. The coordinator already writes `NodeDeploymentState` rows keyed by +the membership-derived id and they already satisfy that FK — which is standing proof the two sets +agree today. + +So the coordinator switch is a substitution inside one private method, and the address columns are +purely forward-looking groundwork for Phase 2's ClusterClient and Phase 5's gRPC dial. + +## The decision this phase forces + +**Membership-derived and DB-derived expected-ack sets are not the same set, and the difference is a +behaviour change on the deploy path.** + +| Situation | Today (membership) | After (DB rows) | +|---|---|---| +| Configured node is running | expected, acks, seals | same | +| Configured node is **stopped** | not expected — deploy seals **successfully** without it | expected — deploy waits, then **fails at the apply deadline** | +| Node running but not in `ClusterNode` | expected (and then FK 547 on the state row) | not expected; its ack is discarded | + +The middle row is the one that matters. Today a deployment can seal green while a node that should +have received it was down and did not — the operator is told the fleet is deployed when it is not. +Failing instead is arguably the correct behaviour and is the reason the design moves this to the DB +(central cannot see cluster membership once the meshes split). + +But it is a behaviour change, and it needs an operator escape hatch or a node taken down for +maintenance blocks every deployment. `ClusterNode.Enabled` already exists and is exactly that hatch, +so the query filters on it: **`Enabled = 1` rows are expected to ack; disabled rows are not.** + +**Confirm this before executing Task 3.** If the preference is to keep sealing green past a down +node, the alternative is to intersect the DB set with current membership, which preserves today's +behaviour but does not survive the mesh split — it would have to be revisited in Phase 2 anyway. + +--- + +## Task 1: Address columns on `ClusterNode` + +**Classification:** small +**Estimated implement time:** ~4 min +**Parallelizable with:** none (Task 2 migrates what this declares) + +**Files:** +- Modify: `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Entities/ClusterNode.cs` +- Modify: the `ClusterNode` entity configuration in `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/` (find the `IEntityTypeConfiguration` or the `OnModelCreating` block) +- Test: `tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/` (mirror an existing entity-mapping test) + +Add two nullable columns beside the existing `OpcUaPort` / `DashboardPort`: + +- `int AkkaPort` — default `4053`. The remoting port central will use to build the ClusterClient + contact point. Non-nullable with a default, because every node has one. +- `int? GrpcPort` — nullable, no default. The Phase 5 telemetry-stream port. Nullable is deliberate: + it does not exist yet, and a non-null default would assert a port nothing listens on. + +Document on each property that these are **central's dial targets**, not the node's own binding +configuration — the node binds from `Cluster:Port` in its own appsettings, and these columns +duplicate that value for a reader that cannot see the node's config. That duplication is the reason +Task 4 exists. + +**Step 1:** Write a mapping test asserting a round-tripped `ClusterNode` preserves both values and +that `AkkaPort` defaults to 4053. +**Step 2:** Run it; expect failure to compile. +**Step 3:** Add the properties + mapping. +**Step 4:** Run; expect pass. +**Step 5:** Commit. + +## Task 2: EF migration + +**Classification:** small +**Estimated implement time:** ~3 min +**Parallelizable with:** none + +**Files:** +- Create: a new migration under the Configuration project's `Migrations/` folder + +```bash +dotnet ef migrations add AddClusterNodeTransportPorts \ + --project src/Core/ZB.MOM.WW.OtOpcUa.Configuration +``` + +Verify the generated `Up` adds `AkkaPort` with `defaultValue: 4053` and `GrpcPort` as nullable, and +that `Down` drops both. **Check the migration is additive only** — an `AlterColumn` on anything else +means the model had drifted from the last migration and that drift must be understood before this +ships, not carried along by it. + +## Task 3: Coordinator sources the expected-ack set from `ClusterNode` + +**Classification:** high-risk — this is the deploy path +**Estimated implement time:** ~5 min +**Parallelizable with:** none + +**Files:** +- Modify: `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Coordinators/ConfigPublishCoordinator.cs` (`DiscoverDriverNodes`, ~line 262, and the class doc comment at line 21 which states the membership rule) +- Test: `tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/ConfigPublishCoordinatorTests.cs` + +Replace the `Cluster.State.Members` scan with a query over enabled `ClusterNode` rows. Keep +`NodeIdComparer` — the case-insensitivity rationale in its comment applies unchanged, and SQL +collation makes it more relevant, not less. + +Points to get right: + +- **Filter on `Enabled`.** See "The decision this phase forces". +- **Scope to the deployment's cluster if the deployment is cluster-scoped.** `DeploymentArtifact.ResolveClusterScope` + already defines that scoping; a fleet-wide deployment expects every enabled node, a cluster-scoped + one expects only that cluster's. Getting this wrong makes every cluster-scoped deploy hang on nodes + that were never sent it. **Read `ResolveClusterScope` before writing the query.** +- **The empty-set branch still matters.** `HandleDispatch` seals immediately when the set is empty, + and its log message says "no driver-role members in cluster" — reword it, because the reason is now + "no enabled ClusterNode rows", which is a configuration error rather than a topology observation. +- The DB is read inside `HandleDispatch`, which already opens a context; reuse it rather than opening + a second. + +**Tests to add:** enabled-only filtering; cluster-scope filtering; empty set seals with the new +reason; an ack from a node absent from `ClusterNode` is discarded. **Positive control on each:** +delete the filter and confirm the test goes red — the existing suite passed throughout the +membership era, so a test that also passes against the old derivation is testing nothing. + +## Task 4: Reconcile the duplicated port with the node's own config + +**Classification:** standard +**Estimated implement time:** ~4 min +**Parallelizable with:** none + +`ClusterNode.AkkaPort` and the node's own `Cluster:Port` are the same fact in two places, and nothing +makes them agree. A node that binds 4054 while its row says 4053 is unreachable from central in Phase +2, and the symptom will be a silent absence of acks rather than an error. + +Options, in preference order: + +1. **Have the node assert its row on startup** — on join, compare `Cluster:Port` / + `Cluster:PublicHostname` to its own `ClusterNode` row and log an Error (not a Warning) on + mismatch. Cheap, no schema change, no write path from driver nodes to the ConfigDb (which Phase 4 + removes anyway — so put this check on an **admin** node, reading the membership it can already see). +2. Deploy-time validation in `DraftValidator`, alongside the existing gates. + +Pick one and implement it. Do not skip this task: an unenforced duplicated address is the exact shape +of the `Modbus`/`ModbusTcp` drift and the `TwinCat`/`Focas` drift already recorded in this repo. + +## Task 5: Rig seed + docs + +**Classification:** small +**Estimated implement time:** ~3 min + +**Files:** +- Modify: `docker-dev/seed/seed-clusters.sql` — add `AkkaPort` to the six INSERTs (all 4053) so a + freshly-seeded rig matches a migrated one. Leave `GrpcPort` null. +- Modify: `docs/Configuration.md` — document both columns. +- Modify: `docs/plans/2026-07-21-per-cluster-mesh-design.md` — mark Phase 1 done in §7. + +## Task 6: Live gate + +**Classification:** high-risk +**Estimated implement time:** n/a — operator-driven + +Offline tests cannot cover the substitution's real risk, which is that the DB set and the live set +disagree on the rig. Run on `docker-dev`: + +1. Deploy with all six nodes up → seals green, six `NodeDeploymentState` rows, no FK 547. +2. Deploy a **cluster-scoped** config → expected set is that cluster's nodes only. +3. `docker stop` one node, deploy → **fails at the apply deadline** naming the missing node. This is + the new behaviour; confirm the failure is legible, because an operator will meet it. +4. Set that node's `Enabled = 0`, deploy again → seals green without it. This proves the escape + hatch, and is the check that makes step 3 acceptable to ship. + +Record results in a gate doc beside this plan, per the Phase 1/Phase 2 precedent.