Fix missing LeaderStateChanged event

In RaftActor, the logic to detect a leader state change compares the last
valid leader Id with the current behavior leader Id. Consider the
following leader Id change sequence:

  "member-1" -> null (goes leaderless)
  null -> "member-1" (member-1 becomes leader again)

The first state change will send a LeaderStateChanged event to the
ShardManager with null leader Id causing the ShardManager to clean its
primary shard info cache. However for the second state change, no
LeaderStateChanged event is sent b/c the new leader Id is the same as
the last valid/non-null leader Id. Therefore transactions fail due to no
shard leader.

I changed it to use the last leader Id (null or not) for the comparison
so every state change is detected.

Change-Id: I060872d4712e040b60acfc998914b394a40943af
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>

diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActor.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActor.java
index 602a76ba4215e9fa29b1a187d37547b6a6aa8285..3037059c3d4803e5ae9bd946edc75208eb4b380d 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActor.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActor.java
@@ -460,12 +460,13 @@ public abstract class RaftActor extends AbstractUntypedPersistentActor {
             onStateChanged();
         }
 
+        String lastLeaderId = oldBehavior == null ? null : oldBehaviorState.getLastLeaderId();
         String lastValidLeaderId = oldBehavior == null ? null : oldBehaviorState.getLastValidLeaderId();
         String oldBehaviorStateName = oldBehavior == null ? null : oldBehavior.state().name();
 
         // it can happen that the state has not changed but the leader has changed.
         Optional<ActorRef> roleChangeNotifier = getRoleChangeNotifier();
-        if(!Objects.equals(lastValidLeaderId, currentBehavior.getLeaderId()) ||
+        if(!Objects.equals(lastLeaderId, currentBehavior.getLeaderId()) ||
            oldBehaviorState.getLeaderPayloadVersion() != currentBehavior.getLeaderPayloadVersion()) {
             if(roleChangeNotifier.isPresent()) {
                 roleChangeNotifier.get().tell(newLeaderStateChanged(getId(), currentBehavior.getLeaderId(),
@@ -883,6 +884,7 @@ public abstract class RaftActor extends AbstractUntypedPersistentActor {
     private static abstract class BehaviorState implements Immutable {
         @Nullable abstract RaftActorBehavior getBehavior();
         @Nullable abstract String getLastValidLeaderId();
+        @Nullable abstract String getLastLeaderId();
         @Nullable abstract short getLeaderPayloadVersion();
     }
 
@@ -892,10 +894,13 @@ public abstract class RaftActor extends AbstractUntypedPersistentActor {
     private static final class SimpleBehaviorState extends BehaviorState {
         private final RaftActorBehavior behavior;
         private final String lastValidLeaderId;
+        private final String lastLeaderId;
         private final short leaderPayloadVersion;
 
-        SimpleBehaviorState(final String lastValidLeaderId, final RaftActorBehavior behavior) {
+        SimpleBehaviorState(final String lastValidLeaderId, final String lastLeaderId,
+                final RaftActorBehavior behavior) {
             this.lastValidLeaderId = lastValidLeaderId;
+            this.lastLeaderId = lastLeaderId;
             this.behavior = Preconditions.checkNotNull(behavior);
             this.leaderPayloadVersion = behavior.getLeaderPayloadVersion();
         }
@@ -914,6 +919,11 @@ public abstract class RaftActor extends AbstractUntypedPersistentActor {
         short getLeaderPayloadVersion() {
             return leaderPayloadVersion;
         }
+
+        @Override
+        String getLastLeaderId() {
+            return lastLeaderId;
+        }
     }
 
     /**
@@ -942,9 +952,15 @@ public abstract class RaftActor extends AbstractUntypedPersistentActor {
             short getLeaderPayloadVersion() {
                 return -1;
             }
+
+            @Override
+            String getLastLeaderId() {
+                return null;
+            }
         };
 
         private String lastValidLeaderId;
+        private String lastLeaderId;
 
         BehaviorState capture(final RaftActorBehavior behavior) {
             if (behavior == null) {
@@ -952,12 +968,12 @@ public abstract class RaftActor extends AbstractUntypedPersistentActor {
                 return NULL_BEHAVIOR_STATE;
             }
 
-            final String leaderId = behavior.getLeaderId();
-            if (leaderId != null) {
-                lastValidLeaderId = leaderId;
+            lastLeaderId = behavior.getLeaderId();
+            if (lastLeaderId != null) {
+                lastValidLeaderId = lastLeaderId;
             }
 
-            return new SimpleBehaviorState(lastValidLeaderId, behavior);
+            return new SimpleBehaviorState(lastValidLeaderId, lastLeaderId, behavior);
         }
     }
 

diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/NonVotingFollowerIntegrationTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/NonVotingFollowerIntegrationTest.java
index adbc5e8eb7a81a9cb056c0e81f2063a320a085d3..bfd147498adb4e62632023c2c8a54428687c4ad9 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/NonVotingFollowerIntegrationTest.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/NonVotingFollowerIntegrationTest.java
@@ -8,19 +8,26 @@
 package org.opendaylight.controller.cluster.raft;
 
 import static org.junit.Assert.assertEquals;
+import akka.actor.ActorRef;
+import akka.dispatch.Dispatchers;
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Sets;
 import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
 import org.junit.Test;
+import org.opendaylight.controller.cluster.notifications.LeaderStateChanged;
+import org.opendaylight.controller.cluster.raft.AbstractRaftActorIntegrationTest.TestRaftActor.Builder;
 import org.opendaylight.controller.cluster.raft.ServerConfigurationPayload.ServerInfo;
 import org.opendaylight.controller.cluster.raft.base.messages.ApplyState;
+import org.opendaylight.controller.cluster.raft.base.messages.ElectionTimeout;
 import org.opendaylight.controller.cluster.raft.base.messages.SnapshotComplete;
 import org.opendaylight.controller.cluster.raft.base.messages.UpdateElectionTerm;
 import org.opendaylight.controller.cluster.raft.messages.AppendEntries;
 import org.opendaylight.controller.cluster.raft.policy.DisableElectionsRaftPolicy;
 import org.opendaylight.controller.cluster.raft.utils.InMemoryJournal;
 import org.opendaylight.controller.cluster.raft.utils.MessageCollectorActor;
+import scala.concurrent.duration.FiniteDuration;
 
 /**
  * Integration test for various scenarios involving non-voting followers.
@@ -30,6 +37,7 @@ import org.opendaylight.controller.cluster.raft.utils.MessageCollectorActor;
 public class NonVotingFollowerIntegrationTest extends AbstractRaftActorIntegrationTest {
     private TestRaftActor followerInstance;
     private TestRaftActor leaderInstance;
+    private final Builder follower1Builder = TestRaftActor.newBuilder();
 
     /**
      * Tests non-voting follower re-sync after the non-persistent leader restarts with an empty log. In this
@@ -343,6 +351,37 @@ public class NonVotingFollowerIntegrationTest extends AbstractRaftActorIntegrati
         testLog.info("testFollowerResyncWithMoreLeaderLogEntriesAndDownPeerAfterNonPersistentLeaderRestart ending");
     }
 
+    @Test
+    public void testFollowerLeaderStateChanges() {
+        testLog.info("testFollowerLeaderStateChanges");
+
+        ActorRef roleChangeNotifier = factory.<MessageCollectorActor>createTestActor(
+                MessageCollectorActor.props().withDispatcher(Dispatchers.DefaultDispatcherId()),
+                factory.generateActorId("roleChangeNotifier"));
+        follower1Builder.roleChangeNotifier(roleChangeNotifier);
+
+        setupLeaderAndNonVotingFollower();
+
+        ((DefaultConfigParamsImpl)follower1Context.getConfigParams()).setElectionTimeoutFactor(2);
+        ((DefaultConfigParamsImpl)follower1Context.getConfigParams()).
+                setHeartBeatInterval(FiniteDuration.apply(100, TimeUnit.MILLISECONDS));
+
+        MessageCollectorActor.clearMessages(roleChangeNotifier);
+        follower1Actor.tell(ElectionTimeout.INSTANCE, ActorRef.noSender());
+        followerInstance.startDropMessages(AppendEntries.class);
+
+        LeaderStateChanged leaderStateChanged = MessageCollectorActor.expectFirstMatching(roleChangeNotifier,
+                LeaderStateChanged.class);
+        assertEquals("getLeaderId", null, leaderStateChanged.getLeaderId());
+
+        MessageCollectorActor.clearMessages(roleChangeNotifier);
+        followerInstance.stopDropMessages(AppendEntries.class);
+
+        leaderStateChanged = MessageCollectorActor.expectFirstMatching(roleChangeNotifier,
+                LeaderStateChanged.class);
+        assertEquals("getLeaderId", leaderId, leaderStateChanged.getLeaderId());
+    }
+
     private void createNewLeaderActor() {
         expSnapshotState.clear();
         leaderActor = newTestRaftActor(leaderId, TestRaftActor.newBuilder().peerAddresses(peerAddresses).
@@ -370,7 +409,7 @@ public class NonVotingFollowerIntegrationTest extends AbstractRaftActorIntegrati
         InMemoryJournal.addEntry(follower1Id, 2, persistedServerConfigEntry);
 
         DefaultConfigParamsImpl followerConfigParams = newFollowerConfigParams();
-        follower1Actor = newTestRaftActor(follower1Id, TestRaftActor.newBuilder().peerAddresses(
+        follower1Actor = newTestRaftActor(follower1Id, follower1Builder.peerAddresses(
                 ImmutableMap.of(leaderId, testActorPath(leaderId))).config(followerConfigParams).
                     persistent(Optional.of(false)));