From: Moiz Raja Date: Mon, 3 Aug 2015 23:20:31 +0000 (-0700) Subject: BUG 2185 : Disable all internal switching of behavior X-Git-Tag: release/beryllium~329 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=c68d251880d95d6d2f8df70c67d2cdd3a3a47685 BUG 2185 : Disable all internal switching of behavior 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 (cherry picked from commit 9baf6a2a5e494f70af407f04631980857a26daf9) --- diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java index 134cde8720..85f2f153ab 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java @@ -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); } } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehavior.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehavior.java index 6ef4914477..7e6175d933 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehavior.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehavior.java @@ -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 diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Candidate.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Candidate.java index 196456440b..a59a02051a 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Candidate.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Candidate.java @@ -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()); diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java index d4755e6358..0cd6fbab52 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java @@ -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; diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java index 4f77711a4d..8f544dc32f 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java @@ -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; } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java index ebcdcd40fb..1e58fbe541 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java @@ -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); } } diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java index 5179ddc481..9b877a7cdc 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java @@ -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 peerAddresses = new HashMap<>(); - peerAddresses.put("follower-1", followerActor1.path().toString()); - peerAddresses.put("follower-2", followerActor2.path().toString()); + Map 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 diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/ForwardMessageToBehaviorActor.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/ForwardMessageToBehaviorActor.java index 9bcfcd9091..63810d8882 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/ForwardMessageToBehaviorActor.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/ForwardMessageToBehaviorActor.java @@ -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 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 getBehaviorChanges(){ + return behaviorChanges; + } + + @Override + public void clear() { + super.clear(); + behaviorChanges.clear(); + } } diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java index b0773bf7ca..2c5b6cc34b 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java @@ -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)); }