From: Tom Pantelis Date: Tue, 26 Jul 2016 15:55:09 +0000 (-0400) Subject: Fix missing LeaderStateChanged event X-Git-Tag: release/boron~29 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=fc793885b446b6777ce9d86e7a4867951a8561bb 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 --- 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 602a76ba42..3037059c3d 100644 --- 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 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 adbc5e8eb7..bfd147498a 100644 --- 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.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)));