From: Tom Pantelis Date: Mon, 8 May 2017 16:49:51 +0000 (-0400) Subject: Fix timing issue in PartitionedCandidateOnStartupElection*Test X-Git-Tag: release/nitrogen~277 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=e83057f284dc3c9bf84063a47a18bb41a26aebf3;ds=sidebyside Fix timing issue in PartitionedCandidateOnStartupElection*Test 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 --- diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeaderElectionScenarioTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeaderElectionScenarioTest.java index 5d445bc6a5..137ea4a694 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeaderElectionScenarioTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeaderElectionScenarioTest.java @@ -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, CountDownLatch> messagesReceivedLatches = new ConcurrentHashMap<>(); Map, 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 leaderActor) { Uninterruptibles.sleepUninterruptibly(HEARTBEAT_INTERVAL, TimeUnit.MILLISECONDS); - leaderActor.underlyingActor().behavior.handleMessage(leaderActor, SendHeartBeat.INSTANCE); + leaderActor.tell(SendImmediateHeartBeat.INSTANCE, ActorRef.noSender()); } } diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/DelayedMessagesElectionScenarioTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/DelayedMessagesElectionScenarioTest.java index a0ae3e6df7..825a55b921 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/DelayedMessagesElectionScenarioTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/DelayedMessagesElectionScenarioTest.java @@ -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 diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedCandidateOnStartupElectionScenarioTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedCandidateOnStartupElectionScenarioTest.java index f8eab9a38b..3f5ca17d7a 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedCandidateOnStartupElectionScenarioTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedCandidateOnStartupElectionScenarioTest.java @@ -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. diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedLeadersElectionScenarioTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedLeadersElectionScenarioTest.java index 255f4f7448..cf02a1f23e 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedLeadersElectionScenarioTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/PartitionedLeadersElectionScenarioTest.java @@ -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