From 88adec047b6a8a94d9bc0379163e040cc1e3d90a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 24 Jul 2026 13:21:11 -0400 Subject: [PATCH] refactor(health): adopt the shared active-node check (Health 0.3.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ClusterPrimaryHealthCheck was written three days' worth of debugging ago because the shared ActiveNodeHealthCheck selected by RoleLeader and reported Healthy for any node lacking the role. Health 0.3.0 fixes the shared check — oldest Up member, role-preference scoping, Unhealthy when the node owns no active work — so the private copy is now duplication rather than a workaround, and it is deleted. RolePreference [admin, driver] reproduces exactly what it did: a fused central node answers for admin (where the singletons and the AdminUI are pinned), a driver-only site node answers for driver. SelectOldestUpMemberOfRole now delegates to the shared ClusterActiveNode instead of re-implementing the age ordering. This is the point of the change rather than a tidy-up: the redundancy snapshot drives the OPC UA ServiceLevel 250/240 split while the health tier drives Traefik's admin routing, so two copies of "oldest Up member of a role" would be two chances to advertise one node as authoritative while gating the data plane on another. ControlPlane takes a ZB.MOM.WW.Health.Akka reference for it; the layering trade-off is recorded at the PackageReference. Behaviour note: a node carrying neither admin nor driver now answers 503 on the active tier instead of 200. That case is reachable — RoleParser admits dev-only and cluster-role-only nodes — and 503 is correct, since such a node owns no active work and must not be in an active-tier pool. No docker-dev node is affected: every rig node is admin+driver or driver-only. Tests replaced in kind. The rule itself is now pinned in the library against a real two-node cluster; what stays OtOpcUa's own decision is the wiring, so ActiveTierRegistrationTests pins which check is on the active tag, that it is registered on driver-only nodes too, and that it is not on the ready tier. Verified: Host.Tests 25/25, ControlPlane redundancy 15/15. --- Directory.Packages.props | 6 +- .../Redundancy/RedundancyStateActor.cs | 27 ++-- .../ZB.MOM.WW.OtOpcUa.ControlPlane.csproj | 8 + .../Health/ClusterPrimaryHealthCheck.cs | 145 ------------------ .../Health/HealthEndpoints.cs | 33 +++- .../Health/ActiveTierRegistrationTests.cs | 59 +++++++ .../Health/ClusterPrimaryHealthCheckTests.cs | 65 -------- 7 files changed, 113 insertions(+), 230 deletions(-) delete mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/ClusterPrimaryHealthCheck.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Health/ActiveTierRegistrationTests.cs delete mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Health/ClusterPrimaryHealthCheckTests.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index ccb9daa8..a30e3e2d 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -140,7 +140,7 @@ - + - - + + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/RedundancyStateActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/RedundancyStateActor.cs index 3ded7268..2c43f34a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/RedundancyStateActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/RedundancyStateActor.cs @@ -2,6 +2,7 @@ using Akka.Actor; using Akka.Cluster; using Akka.Cluster.Tools.PublishSubscribe; using Akka.Event; +using ZB.MOM.WW.Health.Akka; using ZB.MOM.WW.OtOpcUa.Cluster; using ZB.MOM.WW.OtOpcUa.Commons.Messages.Redundancy; using ZB.MOM.WW.OtOpcUa.Commons.Types; @@ -152,22 +153,28 @@ public sealed class RedundancyStateActor : ReceiveActor, IWithTimers /// The cluster role to scope the selection to. /// The oldest Up member's address for that role, or null when there is none. /// - /// The age-ordering rationale in applies verbatim to any role: - /// oldest, never RoleLeader. Factored out so the rule has exactly one implementation — - /// the health tier that reports which node is active needs the same answer the redundancy - /// snapshot does, and two copies of "oldest Up member of a role" would be two chances to - /// silently disagree about who is in charge. + /// + /// The age-ordering rationale in applies verbatim to any + /// role: oldest, never RoleLeader. The rule itself lives in the shared + /// (ZB.MOM.WW.Health.Akka 0.3.0) so it has exactly + /// one implementation family-wide — this method is the control plane's entry point into it. + /// + /// + /// Delegating rather than re-implementing is what keeps the redundancy snapshot and the + /// /health/active tier structurally in agreement. They answer for different + /// consumers — the OPC UA ServiceLevel 250/240 split reads this election, while an + /// orchestrator routes admin traffic by the health tier — so a divergence would advertise + /// one node as authoritative while gating the data plane on another. Both apps in the + /// family previously kept private copies of this rule, and both got it wrong in a + /// different way. + /// /// public static Address? SelectOldestUpMemberOfRole(IEnumerable members, string role) { ArgumentNullException.ThrowIfNull(members); ArgumentException.ThrowIfNullOrWhiteSpace(role); - return members - .Where(m => m.Status == MemberStatus.Up && m.Roles.Contains(role)) - .OrderBy(m => m, Member.AgeOrdering) - .FirstOrDefault() - ?.Address; + return ClusterActiveNode.OldestUpMember(members, role)?.Address; } private IReadOnlyList BuildSnapshot() diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ZB.MOM.WW.OtOpcUa.ControlPlane.csproj b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ZB.MOM.WW.OtOpcUa.ControlPlane.csproj index d2e52480..a20477da 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ZB.MOM.WW.OtOpcUa.ControlPlane.csproj +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ZB.MOM.WW.OtOpcUa.ControlPlane.csproj @@ -19,6 +19,14 @@ Google.Protobuf + Grpc.Core.Api flow transitively from Commons (the generated client). --> + + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/ClusterPrimaryHealthCheck.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/ClusterPrimaryHealthCheck.cs deleted file mode 100644 index 25e88a99..00000000 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/ClusterPrimaryHealthCheck.cs +++ /dev/null @@ -1,145 +0,0 @@ -using Akka.Actor; -using Akka.Cluster; -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Diagnostics.HealthChecks; -using ZB.MOM.WW.OtOpcUa.Cluster; -using ZB.MOM.WW.OtOpcUa.ControlPlane.Redundancy; - -namespace ZB.MOM.WW.OtOpcUa.Host.Health; - -/// -/// The active tier's answer to "is this the node in charge of this cluster?" — Healthy (200) on the -/// one node that owns the active work for its mesh, Unhealthy (503) on its partner. -/// -/// -/// -/// Replaces the shared ActiveNodeHealthCheck(role: "admin"), which answered a -/// different question and got it wrong twice. It was scoped to the admin role and -/// returns Healthy for any node that lacks that role, so all four driver-only site nodes -/// reported themselves active; and it selected by RoleLeader, the -/// lowest-addressed member, which is not where Akka places singletons. See -/// lmxopcua#494. -/// -/// -/// The rule here is one sentence: this node is active iff it is the oldest Up member -/// carrying its own active role, where the active role is admin when the node has -/// it and driver otherwise. That single rule serves both consumers correctly: -/// -/// -/// -/// A fused admin node answers for the admin role, so Traefik pins the AdminUI to -/// the node actually hosting the cluster singletons — which -/// ClusterSingletonManager places on the oldest member, not the role -/// leader. The two diverge after any restart. -/// -/// -/// A driver-only site node answers for the driver role, which is exactly -/// — the same election that -/// drives IsDriverPrimary and the OPC UA ServiceLevel 250/240 split. So -/// the tier and the ServiceLevel can no longer disagree about which node is Primary. -/// -/// -/// -/// Scoping is per-mesh for free: after per-cluster mesh Phase 6 a node's -/// ClusterState.Members contains only its own application Cluster, so -/// "oldest Up member" is already "oldest Up member of this Cluster" — exactly one -/// node answers 200 per Cluster, by construction rather than by filtering. -/// -/// -/// Startup-safe, matching the shared check it replaces: before the -/// or the cluster is available the result is Degraded, not Unhealthy, so a node that is -/// merely still booting is not reported as a failed standby. -/// -/// -public sealed class ClusterPrimaryHealthCheck : IHealthCheck -{ - private readonly IServiceProvider _serviceProvider; - - /// Initializes a new . - /// - /// The application service provider. The is resolved lazily so the - /// check is startup-safe. - /// - public ClusterPrimaryHealthCheck(IServiceProvider serviceProvider) => - _serviceProvider = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider)); - - /// - /// The role whose oldest Up member owns the active work for . - /// - /// The local member's cluster roles. - /// - /// when the node carries it, otherwise - /// when it carries that, otherwise null for a node in - /// neither role. - /// - /// - /// Admin wins on a fused node because that is the role the cluster singletons — and the AdminUI - /// that Traefik routes to — are pinned to. - /// - public static string? ActiveRoleFor(IEnumerable selfRoles) - { - ArgumentNullException.ThrowIfNull(selfRoles); - - var roles = selfRoles as IReadOnlyCollection ?? selfRoles.ToList(); - - if (roles.Contains(RoleParser.Admin)) - return RoleParser.Admin; - - return roles.Contains(RoleParser.Driver) ? RoleParser.Driver : null; - } - - /// - public Task CheckHealthAsync( - HealthCheckContext context, - CancellationToken cancellationToken = default) - { - var system = _serviceProvider.GetService(); - if (system is null) - return Task.FromResult(HealthCheckResult.Degraded("ActorSystem not yet available.")); - - Member self; - IEnumerable members; - try - { - var cluster = Akka.Cluster.Cluster.Get(system); - self = cluster.SelfMember; - members = cluster.State.Members; - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - return Task.FromResult(HealthCheckResult.Degraded("Akka cluster state not yet accessible.", ex)); - } - - var role = ActiveRoleFor(self.Roles); - if (role is null) - { - // Neither admin nor driver. RoleParser will not produce such a node today, so this is a - // guard rather than a supported topology — reported Healthy because the tier has no - // opinion about a node that owns no active work, not because the node is in charge. - return Task.FromResult(HealthCheckResult.Healthy("Node carries neither the admin nor the driver role.")); - } - - var primary = RedundancyStateActor.SelectOldestUpMemberOfRole(members, role); - - var data = new Dictionary(StringComparer.Ordinal) - { - ["activeRole"] = role, - ["selfAddress"] = self.Address.ToString(), - }; - - if (primary is not null) - data["primary"] = primary.ToString(); - - if (primary is null) - { - // No Up member holds the role — during a cold start, or while every holder is Joining or - // Leaving. Nobody is active, so nobody may claim to be. - return Task.FromResult(HealthCheckResult.Unhealthy( - $"No Up member currently carries the '{role}' role.", exception: null, data)); - } - - return Task.FromResult(primary == self.Address - ? HealthCheckResult.Healthy($"Primary for role '{role}' (oldest Up member).", data) - : HealthCheckResult.Unhealthy($"Standby: '{role}' primary is {primary}.", exception: null, data)); - } -} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/HealthEndpoints.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/HealthEndpoints.cs index 7c9bbad0..f7a740bd 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/HealthEndpoints.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/HealthEndpoints.cs @@ -8,6 +8,7 @@ using ZB.MOM.WW.Health; using ZB.MOM.WW.Health.Akka; using ZB.MOM.WW.Health.EntityFrameworkCore; using ZB.MOM.WW.LocalDb.Replication; +using ZB.MOM.WW.OtOpcUa.Cluster; using ZB.MOM.WW.OtOpcUa.Configuration; using ZB.MOM.WW.OtOpcUa.Host.Configuration; @@ -53,15 +54,33 @@ public static class HealthEndpoints failureStatus: null, tags: new[] { ZbHealthTags.Ready, ZbHealthTags.Active }, args: AkkaClusterStatusPolicy.OtOpcUaCompat) - // "Is this node in charge of its mesh?" — the question the active tier is supposed to - // answer, and the one the shared ActiveNodeHealthCheck(role: "admin") did not. That check - // returned Healthy for any node lacking the admin role, so every driver-only site node - // called itself active, and it selected by RoleLeader (lowest address) rather than by - // age, which is not where Akka places singletons. See lmxopcua#494. - .AddTypeActivatedCheck( + // "Is this node in charge of its mesh?" — Healthy on the one node that owns the active + // work for its Cluster, Unhealthy (503) on its partner, so Traefik pins the AdminUI to the + // node actually hosting the cluster singletons. + // + // Role preference admin-then-driver: a fused central node answers for `admin` (the role + // the singletons and the AdminUI are pinned to), while a driver-only site node answers for + // `driver` — which is exactly RedundancyStateActor.SelectDriverPrimary, the election behind + // IsDriverPrimary and the OPC UA ServiceLevel 250/240 split. Both now route through the + // shared ClusterActiveNode, so the tier and the ServiceLevel cannot disagree. + // + // Scoping is per-mesh for free: after per-cluster mesh Phase 6 a node's ClusterState + // contains only its own application Cluster, so "oldest Up member" is already "oldest Up + // member of this Cluster" — exactly one node answers 200 per Cluster, by construction. + // + // Before Health 0.3.0 this was ActiveNodeHealthCheck(role: "admin"), which answered a + // different question and got it wrong twice: it returned Healthy for any node lacking the + // admin role, so every driver-only site node called itself active, and it selected by + // RoleLeader (lowest address) rather than by age, which is not where Akka places + // singletons. See lmxopcua#494. + .AddTypeActivatedCheck( "cluster-primary", failureStatus: null, - tags: new[] { ZbHealthTags.Active }) + tags: new[] { ZbHealthTags.Active }, + args: new ActiveNodeHealthCheckOptions + { + RolePreference = new[] { RoleParser.Admin, RoleParser.Driver }, + }) // Registered on every node regardless of role (unlike the admin-only configdb probe above): // the check itself resolves ISyncStatus optionally and reports Healthy when LocalDb is absent // (admin-only graphs) or replication is default-OFF, so a plain node is never degraded by it. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Health/ActiveTierRegistrationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Health/ActiveTierRegistrationTests.cs new file mode 100644 index 00000000..1a9730ce --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Health/ActiveTierRegistrationTests.cs @@ -0,0 +1,59 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Diagnostics.HealthChecks; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; +using ZB.MOM.WW.Health; +using ZB.MOM.WW.Health.Akka; +using ZB.MOM.WW.OtOpcUa.Host.Health; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.Health; + +/// +/// Pins OtOpcUa's active-tier wiring. The rule (oldest Up member of a role) now lives in the +/// shared ClusterActiveNode and is tested there against a real two-node cluster; what stays +/// OtOpcUa's own decision — and what regressed in lmxopcua#494 — is which check is +/// registered on the active tag, and with which roles. +/// +public sealed class ActiveTierRegistrationTests +{ + private static HealthCheckRegistration ClusterPrimaryRegistration(bool hasAdmin) => + new ServiceCollection() + .AddOtOpcUaHealth(hasAdmin) + .BuildServiceProvider() + .GetRequiredService>() + .Value.Registrations + .Where(r => r.Name == "cluster-primary") + .ShouldHaveSingleItem(); + + [Fact] + public void Active_tier_uses_the_shared_ActiveNodeHealthCheck() + { + // Not a bespoke copy. Two apps in the family independently hand-rolled this check because the + // shared one selected by RoleLeader; since Health 0.3.0 the shared one is the correct rule, so + // a private reimplementation reappearing here would be the regression. + var registration = ClusterPrimaryRegistration(hasAdmin: true); + + registration.Factory(new ServiceCollection().BuildServiceProvider()) + .ShouldBeOfType(); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Active_tier_check_is_registered_on_every_node_role(bool hasAdmin) + { + // Registered on driver-only nodes too, unlike the admin-only configdb probe. A site pair needs + // an active tier just as much as the central pair does — under the old admin-scoped check all + // four driver-only site nodes answered 200 and no consumer could find a site's Primary. + ClusterPrimaryRegistration(hasAdmin).Tags.ShouldContain(ZbHealthTags.Active); + } + + [Fact] + public void Active_tier_check_is_not_on_the_ready_tier() + { + // Readiness must not depend on being the active node: a healthy standby is ready to serve, and + // tagging it ready would take the whole pair out of rotation whenever one node is standby. + ClusterPrimaryRegistration(hasAdmin: true).Tags.ShouldNotContain(ZbHealthTags.Ready); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Health/ClusterPrimaryHealthCheckTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Health/ClusterPrimaryHealthCheckTests.cs deleted file mode 100644 index 0206ddec..00000000 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Health/ClusterPrimaryHealthCheckTests.cs +++ /dev/null @@ -1,65 +0,0 @@ -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Diagnostics.HealthChecks; -using Shouldly; -using Xunit; -using ZB.MOM.WW.OtOpcUa.Host.Health; - -namespace ZB.MOM.WW.OtOpcUa.Host.Tests.Health; - -/// -/// Pins the rule the active tier now uses to decide which node is in charge, and the startup path -/// that must not report a booting node as a failed standby. -/// -/// -/// The cluster-membership half of the rule — oldest Up member, never RoleLeader — is pinned -/// against a real two-node cluster in RedundancyPrimaryElectionTests, including parity with -/// the redundancy snapshot's own election. What is left to cover here is the role-selection rule and -/// the startup guard. -/// -public sealed class ClusterPrimaryHealthCheckTests -{ - [Fact] - public void Fused_node_answers_for_the_admin_role() - { - // Admin wins on a node carrying both, because the cluster singletons and the AdminUI that - // Traefik routes to are pinned to that role. Answering for 'driver' here could name a - // different node the moment an admin-only or driver-only member joins the mesh. - ClusterPrimaryHealthCheck.ActiveRoleFor(["admin", "driver", "cluster-MAIN"]) - .ShouldBe("admin"); - } - - [Fact] - public void Driver_only_node_answers_for_the_driver_role() - { - // The regression this whole change exists for. Under the previous admin-scoped check every - // site node returned Healthy — "or not a role member" — so all four called themselves - // active and no consumer could find the Primary of a site Cluster. lmxopcua#494. - ClusterPrimaryHealthCheck.ActiveRoleFor(["driver", "cluster-SITE-A"]) - .ShouldBe("driver"); - } - - [Fact] - public void Admin_only_node_answers_for_the_admin_role() - { - ClusterPrimaryHealthCheck.ActiveRoleFor(["admin"]).ShouldBe("admin"); - } - - [Fact] - public void Node_in_neither_role_has_no_active_role() - { - ClusterPrimaryHealthCheck.ActiveRoleFor(["cluster-MAIN"]).ShouldBeNull(); - } - - [Fact] - public async Task No_actor_system_is_degraded_not_unhealthy() - { - // A node that is still booting has not lost an election. Reporting Unhealthy here would make - // every start look like a failed standby, and — now that the tier is a real 503 — would pull - // the node out of Traefik's pool on the way up. - var check = new ClusterPrimaryHealthCheck(new ServiceCollection().BuildServiceProvider()); - - var result = await check.CheckHealthAsync(new HealthCheckContext(), CancellationToken.None); - - result.Status.ShouldBe(HealthStatus.Degraded); - } -}