BUG 2185 : Disable all internal switching of behavior 90/26290/1
authorMoiz Raja <moraja@cisco.com>
Mon, 3 Aug 2015 23:20:31 +0000 (16:20 -0700)
committerTom Pantelis <tpanteli@brocade.com>
Wed, 2 Sep 2015 00:34:44 +0000 (00:34 +0000)
Since we do not depend on Raft for changing behavior when elections
are disabled we need to disable all internal switching of behaviors.

Added specific Leader tests to check the following,
1. Do not switch to Follower when you receive an AppendEntriesReply
   from a Follower with a higher term
2. Do not switch to IsolatedLeader even when no Follower is sending
   AppendEntriesReply

Change-Id: Ic2b4f76813f35db190e108306a62af5397d31658
Signed-off-by: Moiz Raja <moraja@cisco.com>
(cherry picked from commit 9baf6a2a5e494f70af407f04631980857a26daf9)

opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehavior.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Candidate.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/ForwardMessageToBehaviorActor.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java

index 134cde8..85f2f15 100644 (file)
@@ -333,7 +333,7 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior {
 
                 context.getTermInformation().updateAndPersist(rpc.getTerm(), null);
 
-                return switchBehavior(new Follower(context));
+                return internalSwitchBehavior(RaftState.Follower);
             }
         }
 
index 6ef4914..7e6175d 100644 (file)
@@ -427,16 +427,27 @@ public abstract class AbstractRaftActorBehavior implements RaftActorBehavior {
 
     @Override
     public RaftActorBehavior switchBehavior(RaftActorBehavior behavior) {
-        LOG.info("{} :- Switching from behavior {} to {}", logName(), this.state(), behavior.state());
+        return internalSwitchBehavior(behavior);
+    }
+
+    protected RaftActorBehavior internalSwitchBehavior(RaftState newState) {
+        if(context.getRaftPolicy().automaticElectionsEnabled()){
+            return internalSwitchBehavior(newState.createBehavior(context));
+        }
+        return this;
+    }
+
+    private RaftActorBehavior internalSwitchBehavior(RaftActorBehavior newBehavior) {
+        LOG.info("{} :- Switching from behavior {} to {}", logName(), this.state(), newBehavior.state());
         try {
             close();
         } catch (Exception e) {
             LOG.error("{}: Failed to close behavior : {}", logName(), this.state(), e);
         }
-
-        return behavior;
+        return newBehavior;
     }
 
+
     protected int getMajorityVoteCount(int numPeers) {
         // Votes are required from a majority of the peers including self.
         // The numMajority field therefore stores a calculated value
index 1964564..a59a020 100644 (file)
@@ -103,7 +103,7 @@ public class Candidate extends AbstractRaftActorBehavior {
         }
 
         if (voteCount >= votesRequired) {
-            return switchBehavior(new Leader(context));
+            return internalSwitchBehavior(RaftState.Leader);
         }
 
         return this;
@@ -129,7 +129,7 @@ public class Candidate extends AbstractRaftActorBehavior {
             if (rpc.getTerm() > context.getTermInformation().getCurrentTerm()) {
                 context.getTermInformation().updateAndPersist(rpc.getTerm(), null);
 
-                return switchBehavior(new Follower(context));
+                return internalSwitchBehavior(RaftState.Follower);
             }
         }
 
@@ -143,7 +143,7 @@ public class Candidate extends AbstractRaftActorBehavior {
                 // who we do not know about (as a peer)
                 // to send a message to the candidate
 
-                return switchBehavior(new Leader(context));
+                return internalSwitchBehavior(RaftState.Leader);
             }
             startNewTerm();
             scheduleElection(electionDuration());
index d4755e6..0cd6fba 100644 (file)
@@ -313,7 +313,7 @@ public class Follower extends AbstractRaftActorBehavior {
 
         if (message instanceof ElectionTimeout) {
             LOG.debug("{}: Received ElectionTimeout - switching to Candidate", logName());
-            return switchBehavior(new Candidate(context));
+            return internalSwitchBehavior(RaftState.Candidate);
 
         } else if (message instanceof InstallSnapshot) {
             InstallSnapshot installSnapshot = (InstallSnapshot) message;
index 4f77711..8f544dc 100644 (file)
@@ -40,7 +40,7 @@ public class IsolatedLeader extends AbstractLeader {
         // changes its state to Follower, hence we only need to switch to Leader if the state is still Isolated
         if (ret.state() == RaftState.IsolatedLeader && !isLeaderIsolated()) {
             LOG.info("IsolatedLeader {} switching from IsolatedLeader to Leader", leaderId);
-            return switchBehavior(new Leader(context));
+            return internalSwitchBehavior(RaftState.Leader);
         }
         return ret;
     }
index ebcdcd4..1e58fbe 100644 (file)
@@ -13,6 +13,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
 import java.util.concurrent.TimeUnit;
 import org.opendaylight.controller.cluster.raft.RaftActorContext;
+import org.opendaylight.controller.cluster.raft.RaftState;
 import org.opendaylight.controller.cluster.raft.base.messages.IsolatedLeaderCheck;
 
 /**
@@ -54,7 +55,7 @@ public class Leader extends AbstractLeader {
                 LOG.warn("{}: At least {} followers need to be active, Switching {} from Leader to IsolatedLeader",
                         context.getId(), minIsolatedLeaderPeerCount, leaderId);
 
-                return switchBehavior(new IsolatedLeader(context));
+                return internalSwitchBehavior(RaftState.IsolatedLeader);
             }
         }
 
index 5179ddc..9b877a7 100644 (file)
@@ -51,6 +51,8 @@ import org.opendaylight.controller.cluster.raft.messages.InstallSnapshot;
 import org.opendaylight.controller.cluster.raft.messages.InstallSnapshotReply;
 import org.opendaylight.controller.cluster.raft.messages.RaftRPC;
 import org.opendaylight.controller.cluster.raft.messages.RequestVoteReply;
+import org.opendaylight.controller.cluster.raft.policy.DefaultRaftPolicy;
+import org.opendaylight.controller.cluster.raft.policy.RaftPolicy;
 import org.opendaylight.controller.cluster.raft.utils.ForwardMessageToBehaviorActor;
 import org.opendaylight.controller.cluster.raft.utils.MessageCollectorActor;
 import scala.concurrent.duration.FiniteDuration;
@@ -1424,6 +1426,53 @@ public class LeaderTest extends AbstractLeaderTest {
         assertEquals("Follower's lastTerm", 2, followerActorContext.getReplicatedLog().lastTerm());
     }
 
+    @Test
+    public void testHandleAppendEntriesReplyWithNewerTerm(){
+        logStart("testHandleAppendEntriesReplyWithNewerTerm");
+
+        MockRaftActorContext leaderActorContext = createActorContext();
+        ((DefaultConfigParamsImpl)leaderActorContext.getConfigParams()).setHeartBeatInterval(
+                new FiniteDuration(10000, TimeUnit.SECONDS));
+
+        leaderActorContext.setReplicatedLog(
+                new MockRaftActorContext.MockReplicatedLogBuilder().createEntries(0, 2, 2).build());
+
+        leader = new Leader(leaderActorContext);
+        leaderActor.underlyingActor().setBehavior(leader);
+        leaderActor.tell(new AppendEntriesReply("foo", 20, false, 1000, 10, (short) 1), ActorRef.noSender());
+
+        AppendEntriesReply appendEntriesReply = MessageCollectorActor.expectFirstMatching(leaderActor, AppendEntriesReply.class);
+
+        assertEquals(false, appendEntriesReply.isSuccess());
+        assertEquals(RaftState.Follower, leaderActor.underlyingActor().getFirstBehaviorChange().state());
+
+        MessageCollectorActor.clearMessages(leaderActor);
+    }
+
+    @Test
+    public void testHandleAppendEntriesReplyWithNewerTermWhenElectionsAreDisabled(){
+        logStart("testHandleAppendEntriesReplyWithNewerTermWhenElectionsAreDisabled");
+
+        MockRaftActorContext leaderActorContext = createActorContext();
+        ((DefaultConfigParamsImpl)leaderActorContext.getConfigParams()).setHeartBeatInterval(
+                new FiniteDuration(10000, TimeUnit.SECONDS));
+
+        leaderActorContext.setReplicatedLog(
+                new MockRaftActorContext.MockReplicatedLogBuilder().createEntries(0, 2, 2).build());
+        leaderActorContext.setRaftPolicy(createRaftPolicy(false, false));
+
+        leader = new Leader(leaderActorContext);
+        leaderActor.underlyingActor().setBehavior(leader);
+        leaderActor.tell(new AppendEntriesReply("foo", 20, false, 1000, 10, (short) 1), ActorRef.noSender());
+
+        AppendEntriesReply appendEntriesReply = MessageCollectorActor.expectFirstMatching(leaderActor, AppendEntriesReply.class);
+
+        assertEquals(false, appendEntriesReply.isSuccess());
+        assertEquals(RaftState.Leader, leaderActor.underlyingActor().getFirstBehaviorChange().state());
+
+        MessageCollectorActor.clearMessages(leaderActor);
+    }
+
     @Test
     public void testHandleAppendEntriesReplySuccess() throws Exception {
         logStart("testHandleAppendEntriesReplySuccess");
@@ -1597,56 +1646,69 @@ public class LeaderTest extends AbstractLeaderTest {
         Assert.assertTrue(behavior instanceof Leader);
     }
 
-    @Test
-    public void testIsolatedLeaderCheckTwoFollowers() throws Exception {
-        logStart("testIsolatedLeaderCheckTwoFollowers");
-
-        new JavaTestKit(getSystem()) {{
-
-            ActorRef followerActor1 = getTestActor();
-            ActorRef followerActor2 = getTestActor();
+    private RaftActorBehavior setupIsolatedLeaderCheckTestWithTwoFollowers(RaftPolicy raftPolicy){
+        ActorRef followerActor1 = getSystem().actorOf(MessageCollectorActor.props(), "follower-1");
+        ActorRef followerActor2 = getSystem().actorOf(MessageCollectorActor.props(), "follower-2");
 
-            MockRaftActorContext leaderActorContext = createActorContext();
+        MockRaftActorContext leaderActorContext = createActorContext();
 
-            Map<String, String> peerAddresses = new HashMap<>();
-            peerAddresses.put("follower-1", followerActor1.path().toString());
-            peerAddresses.put("follower-2", followerActor2.path().toString());
+        Map<String, String> peerAddresses = new HashMap<>();
+        peerAddresses.put("follower-1", followerActor1.path().toString());
+        peerAddresses.put("follower-2", followerActor2.path().toString());
 
-            leaderActorContext.setPeerAddresses(peerAddresses);
+        leaderActorContext.setPeerAddresses(peerAddresses);
+        leaderActorContext.setRaftPolicy(raftPolicy);
 
-            leader = new Leader(leaderActorContext);
+        leader = new Leader(leaderActorContext);
 
-            leader.markFollowerActive("follower-1");
-            leader.markFollowerActive("follower-2");
-            RaftActorBehavior behavior = leader.handleMessage(leaderActor, new IsolatedLeaderCheck());
-            Assert.assertTrue("Behavior not instance of Leader when all followers are active",
+        leader.markFollowerActive("follower-1");
+        leader.markFollowerActive("follower-2");
+        RaftActorBehavior behavior = leader.handleMessage(leaderActor, new IsolatedLeaderCheck());
+        Assert.assertTrue("Behavior not instance of Leader when all followers are active",
                 behavior instanceof Leader);
 
-            // kill 1 follower and verify if that got killed
-            final JavaTestKit probe = new JavaTestKit(getSystem());
-            probe.watch(followerActor1);
-            followerActor1.tell(PoisonPill.getInstance(), ActorRef.noSender());
-            final Terminated termMsg1 = probe.expectMsgClass(Terminated.class);
-            assertEquals(termMsg1.getActor(), followerActor1);
-
-            leader.markFollowerInActive("follower-1");
-            leader.markFollowerActive("follower-2");
-            behavior = leader.handleMessage(leaderActor, new IsolatedLeaderCheck());
-            Assert.assertTrue("Behavior not instance of Leader when majority of followers are active",
+        // kill 1 follower and verify if that got killed
+        final JavaTestKit probe = new JavaTestKit(getSystem());
+        probe.watch(followerActor1);
+        followerActor1.tell(PoisonPill.getInstance(), ActorRef.noSender());
+        final Terminated termMsg1 = probe.expectMsgClass(Terminated.class);
+        assertEquals(termMsg1.getActor(), followerActor1);
+
+        leader.markFollowerInActive("follower-1");
+        leader.markFollowerActive("follower-2");
+        behavior = leader.handleMessage(leaderActor, new IsolatedLeaderCheck());
+        Assert.assertTrue("Behavior not instance of Leader when majority of followers are active",
                 behavior instanceof Leader);
 
-            // kill 2nd follower and leader should change to Isolated leader
-            followerActor2.tell(PoisonPill.getInstance(), null);
-            probe.watch(followerActor2);
-            followerActor2.tell(PoisonPill.getInstance(), ActorRef.noSender());
-            final Terminated termMsg2 = probe.expectMsgClass(Terminated.class);
-            assertEquals(termMsg2.getActor(), followerActor2);
-
-            leader.markFollowerInActive("follower-2");
-            behavior = leader.handleMessage(leaderActor, new IsolatedLeaderCheck());
-            Assert.assertTrue("Behavior not instance of IsolatedLeader when majority followers are inactive",
-                behavior instanceof IsolatedLeader);
-        }};
+        // kill 2nd follower and leader should change to Isolated leader
+        followerActor2.tell(PoisonPill.getInstance(), null);
+        probe.watch(followerActor2);
+        followerActor2.tell(PoisonPill.getInstance(), ActorRef.noSender());
+        final Terminated termMsg2 = probe.expectMsgClass(Terminated.class);
+        assertEquals(termMsg2.getActor(), followerActor2);
+
+        leader.markFollowerInActive("follower-2");
+        return leader.handleMessage(leaderActor, new IsolatedLeaderCheck());
+    }
+
+    @Test
+    public void testIsolatedLeaderCheckTwoFollowers() throws Exception {
+        logStart("testIsolatedLeaderCheckTwoFollowers");
+
+        RaftActorBehavior behavior = setupIsolatedLeaderCheckTestWithTwoFollowers(DefaultRaftPolicy.INSTANCE);
+
+        Assert.assertTrue("Behavior not instance of IsolatedLeader when majority followers are inactive",
+            behavior instanceof IsolatedLeader);
+    }
+
+    @Test
+    public void testIsolatedLeaderCheckTwoFollowersWhenElectionsAreDisabled() throws Exception {
+        logStart("testIsolatedLeaderCheckTwoFollowersWhenElectionsAreDisabled");
+
+        RaftActorBehavior behavior = setupIsolatedLeaderCheckTestWithTwoFollowers(createRaftPolicy(false, true));
+
+        Assert.assertTrue("Behavior should not switch to IsolatedLeader because elections are disabled",
+                behavior instanceof Leader);
     }
 
     @Test
index 9bcfcd9..63810d8 100644 (file)
@@ -8,18 +8,21 @@
 
 package org.opendaylight.controller.cluster.raft.utils;
 
+import static org.junit.Assert.assertTrue;
 import akka.actor.Props;
+import java.util.ArrayList;
+import java.util.List;
 import org.opendaylight.controller.cluster.raft.behaviors.RaftActorBehavior;
 
 public class ForwardMessageToBehaviorActor extends MessageCollectorActor {
     private RaftActorBehavior behavior;
+    private List<RaftActorBehavior> behaviorChanges = new ArrayList<>();
 
     @Override
     public void onReceive(Object message) throws Exception {
         if(behavior != null) {
-            behavior.handleMessage(sender(), message);
+            behaviorChanges.add(behavior.handleMessage(sender(), message));
         }
-
         super.onReceive(message);
     }
 
@@ -30,5 +33,25 @@ public class ForwardMessageToBehaviorActor extends MessageCollectorActor {
     public void setBehavior(RaftActorBehavior behavior){
         this.behavior = behavior;
     }
+
+    public RaftActorBehavior getFirstBehaviorChange() {
+        assertTrue("no behavior changes present", behaviorChanges.size() > 0);
+        return behaviorChanges.get(0);
+    }
+
+    public RaftActorBehavior getLastBehaviorChange() {
+        assertTrue("no behavior changes present", behaviorChanges.size() > 0);
+        return behaviorChanges.get(behaviorChanges.size() - 1);
+    }
+
+    public List<RaftActorBehavior> getBehaviorChanges(){
+        return behaviorChanges;
+    }
+
+    @Override
+    public void clear() {
+        super.clear();
+        behaviorChanges.clear();
+    }
 }
 
index b0773bf..2c5b6cc 100644 (file)
@@ -45,7 +45,7 @@ public class MessageCollectorActor extends UntypedActor {
         if(GET_ALL_MESSAGES.equals(message)) {
             getSender().tell(new ArrayList<>(messages), getSelf());
         } else if(CLEAR_MESSAGES.equals(message)) {
-            messages.clear();
+            clear();
         } else if(message != null) {
             messages.add(SerializationUtils.fromSerializable(message));
         }