Fix timing issue in PartitionedCandidateOnStartupElection*Test 81/56681/4
authorTom Pantelis <tompantelis@gmail.com>
Mon, 8 May 2017 16:49:51 +0000 (12:49 -0400)
committerRobert Varga <nite@hq.sk>
Wed, 10 May 2017 09:19:09 +0000 (09:19 +0000)
If the initial AppendEntries sent by the leader (member 1) to member 3
is delayed enough such that the behavior field in MemberActor is already
set by the test code, the AppendEntries message will be forwarded to the
Candidate behavior and yield incorrect results for the test. To prevent this,
we really shouldn't set and access the behavior field directly but instead
do so via messages to maintain actor encapsulation.

Change-Id: If497583ce648e62e3279e5abff19cb8702943c17
Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeaderElectionScenarioTest.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/DelayedMessagesElectionScenarioTest.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedCandidateOnStartupElectionScenarioTest.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedLeadersElectionScenarioTest.java

index 5d445bc..137ea4a 100644 (file)
@@ -14,9 +14,13 @@ import static org.junit.Assert.assertTrue;
 import akka.actor.ActorRef;
 import akka.actor.ActorSystem;
 import akka.actor.Props;
+import akka.actor.Status;
 import akka.dispatch.Dispatchers;
+import akka.pattern.Patterns;
 import akka.testkit.JavaTestKit;
 import akka.testkit.TestActorRef;
+import akka.util.Timeout;
+import com.google.common.base.Throwables;
 import com.google.common.util.concurrent.Uninterruptibles;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
@@ -33,6 +37,8 @@ import org.opendaylight.controller.cluster.raft.messages.AppendEntriesReply;
 import org.opendaylight.controller.cluster.raft.utils.MessageCollectorActor;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import scala.concurrent.Await;
+import scala.concurrent.duration.Duration;
 import scala.concurrent.duration.FiniteDuration;
 
 /**
@@ -44,8 +50,7 @@ public class AbstractLeaderElectionScenarioTest {
     static final int HEARTBEAT_INTERVAL = 50;
 
     static class MemberActor extends MessageCollectorActor {
-
-        volatile RaftActorBehavior behavior;
+        private volatile RaftActorBehavior behavior;
         Map<Class<?>, CountDownLatch> messagesReceivedLatches = new ConcurrentHashMap<>();
         Map<Class<?>, Boolean> dropMessagesToBehavior = new ConcurrentHashMap<>();
         CountDownLatch behaviorStateChangeLatch;
@@ -61,6 +66,25 @@ public class AbstractLeaderElectionScenarioTest {
                 return;
             }
 
+            if (message instanceof SetBehavior) {
+                behavior = ((SetBehavior)message).behavior;
+                ((SetBehavior)message).context.setCurrentBehavior(behavior);
+                return;
+            }
+
+            if (message instanceof GetBehaviorState) {
+                if (behavior != null) {
+                    getSender().tell(behavior.state(), self());
+                } else {
+                    getSender().tell(new Status.Failure(new IllegalStateException(
+                            "RaftActorBehavior is not set in MemberActor")), self());
+                }
+            }
+
+            if (message instanceof SendImmediateHeartBeat) {
+                message = SendHeartBeat.INSTANCE;
+            }
+
             try {
                 if (behavior != null && !dropMessagesToBehavior.containsKey(message.getClass())) {
                     final RaftActorBehavior nextBehavior = behavior.handleMessage(getSender(), message);
@@ -82,6 +106,15 @@ public class AbstractLeaderElectionScenarioTest {
             }
         }
 
+        @Override
+        public void postStop() throws Exception {
+            super.postStop();
+
+            if (behavior != null) {
+                behavior.close();
+            }
+        }
+
         void expectBehaviorStateChange() {
             behaviorStateChangeLatch = new CountDownLatch(1);
         }
@@ -142,6 +175,30 @@ public class AbstractLeaderElectionScenarioTest {
         }
     }
 
+    static class SendImmediateHeartBeat {
+        public static final SendImmediateHeartBeat INSTANCE = new SendImmediateHeartBeat();
+
+        private SendImmediateHeartBeat() {
+        }
+    }
+
+    static class GetBehaviorState {
+        public static final GetBehaviorState INSTANCE = new GetBehaviorState();
+
+        private GetBehaviorState() {
+        }
+    }
+
+    static class SetBehavior {
+        RaftActorBehavior behavior;
+        MockRaftActorContext context;
+
+        SetBehavior(RaftActorBehavior behavior, MockRaftActorContext context) {
+            this.behavior = behavior;
+            this.context = context;
+        }
+    }
+
     protected final Logger testLog = LoggerFactory.getLogger(MockRaftActorContext.class);
     protected final ActorSystem system = ActorSystem.create("test");
     protected final TestActorFactory factory = new TestActorFactory(system);
@@ -168,17 +225,6 @@ public class AbstractLeaderElectionScenarioTest {
 
     @After
     public void tearDown() throws Exception {
-
-        if (member1Actor.behavior != null) {
-            member1Actor.behavior.close();
-        }
-        if (member2Actor.behavior != null) {
-            member2Actor.behavior.close();
-        }
-        if (member3Actor.behavior != null) {
-            member3Actor.behavior.close();
-        }
-
         JavaTestKit.shutdownActorSystem(system);
     }
 
@@ -198,8 +244,15 @@ public class AbstractLeaderElectionScenarioTest {
         return context;
     }
 
+    @SuppressWarnings("checkstyle:IllegalCatch")
     void verifyBehaviorState(String name, MemberActor actor, RaftState expState) {
-        assertEquals(name + " behavior state", expState, actor.behavior.state());
+        try {
+            RaftState actualState = (RaftState) Await.result(Patterns.ask(actor.self(), GetBehaviorState.INSTANCE,
+                    Timeout.apply(5, TimeUnit.SECONDS)), Duration.apply(5, TimeUnit.SECONDS));
+            assertEquals(name + " behavior state", expState, actualState);
+        } catch (Exception e) {
+            Throwables.propagate(e);
+        }
     }
 
     void initializeLeaderBehavior(MemberActor actor, MockRaftActorContext context, int numActiveFollowers)
@@ -230,8 +283,8 @@ public class AbstractLeaderElectionScenarioTest {
 
         context.setCurrentBehavior(leader);
 
-        // Delay assignment here so the AppendEntriesReply isn't forwarded to the behavior.
-        actor.behavior = leader;
+        // Delay assignment of the leader behavior so the AppendEntriesReply isn't forwarded to the behavior.
+        actor.self().tell(new SetBehavior(leader, context), ActorRef.noSender());
 
         actor.forwardCapturedMessagesToBehavior(AppendEntriesReply.class, ActorRef.noSender());
         actor.clear();
@@ -247,6 +300,6 @@ public class AbstractLeaderElectionScenarioTest {
 
     void sendHeartbeat(TestActorRef<MemberActor> leaderActor) {
         Uninterruptibles.sleepUninterruptibly(HEARTBEAT_INTERVAL, TimeUnit.MILLISECONDS);
-        leaderActor.underlyingActor().behavior.handleMessage(leaderActor, SendHeartBeat.INSTANCE);
+        leaderActor.tell(SendImmediateHeartBeat.INSTANCE, ActorRef.noSender());
     }
 }
index a0ae3e6..825a55b 100644 (file)
@@ -183,8 +183,8 @@ public class DelayedMessagesElectionScenarioTest extends AbstractLeaderElectionS
         DefaultConfigParamsImpl member2ConfigParams = newConfigParams();
         member2Context.setConfigParams(member2ConfigParams);
 
-        member2Actor.behavior = new Follower(member2Context);
-        member2Context.setCurrentBehavior(member2Actor.behavior);
+        member2Actor.self().tell(new SetBehavior(new Follower(member2Context), member2Context),
+                ActorRef.noSender());
 
         // Create member 3's behavior initially as Follower
 
@@ -196,8 +196,8 @@ public class DelayedMessagesElectionScenarioTest extends AbstractLeaderElectionS
         DefaultConfigParamsImpl member3ConfigParams = newConfigParams();
         member3Context.setConfigParams(member3ConfigParams);
 
-        member3Actor.behavior = new Follower(member3Context);
-        member3Context.setCurrentBehavior(member3Actor.behavior);
+        member3Actor.self().tell(new SetBehavior(new Follower(member3Context), member3Context),
+                ActorRef.noSender());
 
         // Create member 1's behavior initially as Leader
 
index f8eab9a..3f5ca17 100644 (file)
@@ -182,9 +182,8 @@ public class PartitionedCandidateOnStartupElectionScenarioTest extends AbstractL
 
         member2Actor.dropMessagesToBehavior(RequestVote.class, numCandidateElections);
 
-        Candidate member3Behavior = new Candidate(member3Context);
-        member3Actor.behavior = member3Behavior;
-        member3Context.setCurrentBehavior(member3Behavior);
+        member3Actor.self().tell(new SetBehavior(new Candidate(member3Context), member3Context),
+                ActorRef.noSender());
 
         // Send several additional ElectionTimeouts to Candidate member 3. Each ElectionTimeout will
         // start a new term so Candidate member 3's current term will be greater than the leader's
@@ -234,8 +233,8 @@ public class PartitionedCandidateOnStartupElectionScenarioTest extends AbstractL
         member2Context.setLastApplied(replicatedLog.lastIndex());
         member2Context.getTermInformation().update(3, "member1");
 
-        member2Actor.behavior = new Follower(member2Context);
-        member2Context.setCurrentBehavior(member2Actor.behavior);
+        member2Actor.self().tell(new SetBehavior(new Follower(member2Context), member2Context),
+                ActorRef.noSender());
 
         // Create member 1's behavior as Leader.
 
index 255f4f7..cf02a1f 100644 (file)
@@ -288,8 +288,8 @@ public class PartitionedLeadersElectionScenarioTest extends AbstractLeaderElecti
         DefaultConfigParamsImpl member2ConfigParams = newConfigParams();
         member2Context.setConfigParams(member2ConfigParams);
 
-        member2Actor.behavior = new Follower(member2Context);
-        member2Context.setCurrentBehavior(member2Actor.behavior);
+        member2Actor.self().tell(new SetBehavior(new Follower(member2Context), member2Context),
+                ActorRef.noSender());
 
         // Create member 3's behavior initially as Follower
 
@@ -301,8 +301,8 @@ public class PartitionedLeadersElectionScenarioTest extends AbstractLeaderElecti
         DefaultConfigParamsImpl member3ConfigParams = newConfigParams();
         member3Context.setConfigParams(member3ConfigParams);
 
-        member3Actor.behavior = new Follower(member3Context);
-        member3Context.setCurrentBehavior(member3Actor.behavior);
+        member3Actor.self().tell(new SetBehavior(new Follower(member3Context), member3Context),
+                ActorRef.noSender());
 
         // Create member 1's behavior initially as Leader
 

©2013 OpenDaylight, A Linux Foundation Collaborative Project. All Rights Reserved.
OpenDaylight is a registered trademark of The OpenDaylight Project, Inc.
Linux Foundation and OpenDaylight are registered trademarks of the Linux Foundation.
Linux is a registered trademark of Linus Torvalds.