Akka.Cluster.Tools: fix mutability and oldest state bugs with `ClusterSingletonManager` #7298

Aaronontheweb · 2024-07-24T02:59:55Z

Changes

Looks like the list of available nodes was getting mangled - seems like a basic mutability issue inside the ClusterSingletonManager.

close #6973
close #7196

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

This change follows the Akka.NET API Compatibility Guidelines.
This change follows the Akka.NET Wire Compatibility Guidelines.
I have reviewed my own pull request.
Design discussion issue Akka.Cluster.Sharding: duplicate shards / entities #6973 and Akka.Cluster.Tools.Singleton: singleton moves earlier than expected - as soon as new node joins #7196

Looks like the list of available nodes was getting mangled - seems like a basic mutability issue inside the `ClusterSingletonManager`.

…ChangedBuffer

Aaronontheweb

Detailed my changes

Aaronontheweb · 2024-07-25T17:31:35Z

src/contrib/cluster/Akka.Cluster.Tools.Tests/Singleton/ClusterSingletonRestart2Spec.cs

@@ -31,7 +31,7 @@ public class ClusterSingletonRestart2Spec : AkkaSpec
                                                                                           akka.loglevel = INFO
                                                                                           akka.actor.provider = "cluster"
                                                                                           akka.cluster.roles = [singleton]
-                                                                                           #akka.cluster.auto-down-unreachable-after = 2s
+                                                                                           akka.cluster.split-brain-resolver.stable-after = 2s


Bug fixes for this spec - after doing a thorough review of some the cases where we had a race condition, it all comes down to whether or not _sys1 was able to fully get MemberRemoved or whether or not it became Unreachable first. Speeding up the downing provider's decision-making process both ensures that this member gets removed quickly AND ensures that the number of retries during the HandOver process get capped so the test can complete on-time.

Aaronontheweb · 2024-07-25T17:32:23Z

src/contrib/cluster/Akka.Cluster.Tools/Singleton/ClusterSingletonManager.cs

I added nullability changes in here, cleaned up garbage XML-DOC comments, and did some minor reformatting - hence why this appears to be a "large" diff. However, I will comment on the real and true changes below.

Aaronontheweb · 2024-07-25T17:33:08Z

src/contrib/cluster/Akka.Cluster.Tools/Singleton/ClusterSingletonManager.cs

-        public BecomingOldestData(List<UniqueAddress> previousOldest)
+        public ImmutableList<UniqueAddress> PreviousOldest { get; }
+
+        public BecomingOldestData(ImmutableList<UniqueAddress> previousOldest)


Moved to using Immutable lists everywhere for all states and messages. These are only sent when movement is occurring in the cluster, which is inherently low-volume, so there's no performance concerns.

Aaronontheweb · 2024-07-25T17:36:43Z

src/contrib/cluster/Akka.Cluster.Tools/Singleton/ClusterSingletonManager.cs

            {
                self.Tell(new LeaseLost(reason));
            }).ContinueWith(r =>
            {
                if (r.IsFaulted || r.IsCanceled)
                    return (object)new AcquireLeaseFailure(r.Exception);
                return new AcquireLeaseResult(r.Result);
-            }).PipeTo(Self);
+            }).PipeTo(self);


Fixed a potential safety issue here with using the correct closure.

Aaronontheweb · 2024-07-25T17:37:06Z

src/contrib/cluster/Akka.Cluster.Tools/Singleton/ClusterSingletonManager.cs

@@ -769,15 +723,20 @@ private void GetNextOldestChanged()
        private State<ClusterSingletonState, IClusterSingletonData> TryAcquireLease()
        {
            var self = Self;
-            lease.Acquire(reason =>
+
+            if (_lease == null)


Added a null check here to appease nullability checks

Aaronontheweb · 2024-07-25T17:37:46Z

src/contrib/cluster/Akka.Cluster.Tools/Singleton/ClusterSingletonManager.cs

            {
-                return GoTo(ClusterSingletonState.End).Using(EndData.Instance);
+                return GoTo(ClusterSingletonState.Younger).Using(new YoungerData(ImmutableList<UniqueAddress>.Empty));


No longer using null List<T> - using empty immutable lists instead.

Aaronontheweb · 2024-07-25T17:48:38Z

src/contrib/cluster/Akka.Cluster.Tools/Singleton/ClusterSingletonManager.cs

@@ -911,24 +867,38 @@ private void InitializeFSM()
                    case OldestChangedBuffer.OldestChanged oldestChanged when e.StateData is YoungerData youngerData:
                    {
                        _oldestChangedReceived = true;
-                        if (oldestChanged.Oldest != null && oldestChanged.Oldest.Equals(_selfUniqueAddress))
+                        if (oldestChanged.NewOldest != null && oldestChanged.NewOldest.Equals(_selfUniqueAddress))


This and a corresponding change in the OldestChangedBuffer are the two big fixes in this PR, aside from dealing with some of the minor mutability issues.

The Problem

When we receive a MemberRemoved for an older node that was previously hosting a cluster singleton, we intentionally delay processing of that MemberRemoved event via the DelayedMemberRemoved mechanism. This is designed to ensure that in the event that there is a split brain and the SBR was responsible for removing that old node, that node still gets a chance to shut itself down and die cleanly before we take over. This is especially important for singletons using Akka.Persistence, so we don't have two competing actors using the same persistence id and database. So, we still assume that node is the oldest up to ~20s after it's removal.

This is normally not an issue - when we attempt to send hand-overs to that node they'll time out eventually and we'll just recreate the singleton in a matter of 10-20 seconds typically.

However, where this design creates a problem is during rolling updates in a managed environment like Kubernetes, where restarts can happen really quickly.

Imagine this scenario:

Cluster of node1 (oldest), node2, node3 - all have the right role to host the singleton.

node1 is replaced first by K8s - it's terminated via CLR process exit; uses CoordinatedShutdown to try to exit gracefully as best as it can.

node2 sees that node1 is exiting and does the correct thing - messages it for a handover and becomes the new singleton host and is now the oldest node.

Kubernetes restarts node1, which joins the cluster under a new UniqueMemberAddress but with the same Address as before.

Kubernetes then takes down node2 - per the update process.

If all of the prior steps happen in under 20 seconds, node3 will be notified that it is becoming the oldest node and it will start messaging node1, which is ACTUALLY YOUNGER THAN IT, to do the hand-over. node1 will ACK that hand-over right away and we'll have two separate cluster singletons active at the same time, until node2 completely exits. This issue is one of the two bugs that caused Akka.Cluster.Sharding: duplicate shards / entities #6973 - the member ordering problem that we fixed on Akka.Cluster.Tools.Singleton / Akka.Cluster.Sharding: fix duplicate shards caused by incorrect ClusterSingletonManager HandOver #7297 being the other.

So to fix this issue, we can either speed up member removals or do something even simpler - just include the address of the previous "oldest" node in the OldestChanged message and put that one at the front of the list in our Younger and BecomingOldest states. That way, we're always messaging the most recent home of the singleton using live data from the OldestChangedBuffer. I've run this experiment thousands of times in our test lab and can confirm that it eliminated the issue completely.

Aaronontheweb · 2024-07-25T17:49:45Z

src/contrib/cluster/Akka.Cluster.Tools/Singleton/OldestChangedBuffer.cs

            /// </summary>
-            public UniqueAddress Oldest { get; }


The big fix - make sure that the OldestChanged message includes BOTH the "new" oldest node and the previous oldest node. This ensures that we don't have any data consistency problems in the event of a rolling update.

Aaronontheweb · 2024-07-25T17:50:33Z

src/contrib/cluster/Akka.Cluster.Tools/Singleton/ClusterSingletonManager.cs

-                            return GoTo(ClusterSingletonState.BecomingOldest).Using(new BecomingOldestData(youngerData.Oldest));
+                            // explicitly re-order the list to make sure that the oldest, as indicated to us by the OldestChangedBuffer,
+                            //  is the first element - resolves bug https://github.com/akkadotnet/akka.net/issues/6973
+                            var newOldestState = oldestChanged.PreviousOldest switch


Re-arrange the state we're going to retain so we make sure the PreviousOldest node, as reported by the OldestChangedBuffer, appears at the front of the list. This ensures that the BecomingOldest node is always performing the hand-off with the correct party in the event of a rolling update.

Arkatufus

LGTM

fix mutability bugs with ClusterSingletonManager

e392a9b

Looks like the list of available nodes was getting mangled - seems like a basic mutability issue inside the `ClusterSingletonManager`.

Aaronontheweb added akka-cluster-sharding akka-cluster-tools labels Jul 24, 2024

Aaronontheweb added this to the 1.5.27 milestone Jul 24, 2024

Aaronontheweb added 2 commits July 23, 2024 22:03

fixed another mutability error

679f828

explicitly re-order "oldest nodes" based on data received from Oldest…

4fbf6a9

…ChangedBuffer

Aaronontheweb mentioned this pull request Jul 24, 2024

Akka.Cluster.Sharding: duplicate shards / entities #6973

Closed

Aaronontheweb added 8 commits July 24, 2024 15:22

WIP nullability

74e7e68

fixed nullability issues and API approvals

8df0c3e

Merge branch 'dev' into fix-6973-early-ClusterSingletonManager-handoff

6808836

removed debug logging

b2a3b4e

removed old code

122b16c

fixed some nullability issues

9e4a4a9

fix racy cluster singleton restart spec

7a5d68c

Merge branch 'dev' into fix-6973-early-ClusterSingletonManager-handoff

85a93a4

Aaronontheweb added the critical label Jul 25, 2024

Aaronontheweb commented Jul 25, 2024

View reviewed changes

Arkatufus approved these changes Jul 25, 2024

View reviewed changes

Aaronontheweb merged commit 7180810 into akkadotnet:dev Jul 25, 2024
12 checks passed

Aaronontheweb deleted the fix-6973-early-ClusterSingletonManager-handoff branch July 25, 2024 18:29

This was referenced Jul 25, 2024

Akka.Cluster.Tools: deprecate ClustersSingletonManagerSettings.ConsiderAppVersion #7302

Merged

V1.5.27 release notes #7303

Merged

Akka.Cluster.Tools.Singleton: log the correct oldest member on transition #7309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Akka.Cluster.Tools: fix mutability and oldest state bugs with `ClusterSingletonManager` #7298

Akka.Cluster.Tools: fix mutability and oldest state bugs with `ClusterSingletonManager` #7298

Aaronontheweb commented Jul 24, 2024 •

edited

Loading

Aaronontheweb left a comment

Aaronontheweb Jul 25, 2024

Aaronontheweb Jul 25, 2024

Aaronontheweb Jul 25, 2024

Aaronontheweb Jul 25, 2024

Aaronontheweb Jul 25, 2024

Aaronontheweb Jul 25, 2024

Aaronontheweb Jul 25, 2024

Aaronontheweb Jul 25, 2024

Aaronontheweb Jul 25, 2024

Arkatufus left a comment

Akka.Cluster.Tools: fix mutability and oldest state bugs with ClusterSingletonManager #7298

Akka.Cluster.Tools: fix mutability and oldest state bugs with ClusterSingletonManager #7298

Conversation

Aaronontheweb commented Jul 24, 2024 • edited Loading

Changes

Checklist

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The Problem

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arkatufus left a comment

Choose a reason for hiding this comment

Akka.Cluster.Tools: fix mutability and oldest state bugs with `ClusterSingletonManager` #7298

Akka.Cluster.Tools: fix mutability and oldest state bugs with `ClusterSingletonManager` #7298

Aaronontheweb commented Jul 24, 2024 •

edited

Loading