BUG 2509 : Removing all journal entries from a Followers in-memory journal causes...

This patch simply checks if an AppendEntries#prevLogIndex is equal to the followers ReplicatedLog#snapshotIndex. If
both are the same it also verifies that the AppendEntries#prevLogTerm is equal to the followers ReplicatedLog#snapshotTerm.
This is sufficient in most cases to prevent a re-installation of a snapshot when the AppendEntries message is supposed
to immediately follow the snapshot.

Change-Id: Iab9b493455538544efd06334329f91cc426d61f8
Signed-off-by: Moiz Raja <moraja@cisco.com>

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 b1c73f6f4155e045935fa37912245e3d03d45fc9..cc2e55d51b115cc124890c3e643ed810244a7c24 100644 (file)
--- 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
@@ -45,8 +45,35 @@ public class Follower extends AbstractRaftActorBehavior {
         scheduleElection(electionDuration());
     }
 
+    private boolean isLogEntryPresent(long index){
+        if(index == context.getReplicatedLog().getSnapshotIndex()){
+            return true;
+        }
+
+        ReplicatedLogEntry previousEntry = context.getReplicatedLog()
+                .get(index);
+
+        return previousEntry != null;
+
+    }
+
+    private long getLogEntryTerm(long index){
+        if(index == context.getReplicatedLog().getSnapshotIndex()){
+            return context.getReplicatedLog().getSnapshotTerm();
+        }
+
+        ReplicatedLogEntry previousEntry = context.getReplicatedLog()
+                .get(index);
+
+        if(previousEntry != null){
+            return previousEntry.getTerm();
+        }
+
+        return -1;
+    }
+
     @Override protected RaftActorBehavior handleAppendEntries(ActorRef sender,
-        AppendEntries appendEntries) {
+                                                              AppendEntries appendEntries) {
 
         if(appendEntries.getEntries() != null && appendEntries.getEntries().size() > 0) {
             if(LOG.isDebugEnabled()) {
@@ -67,15 +94,15 @@ public class Follower extends AbstractRaftActorBehavior {
         // 2. Reply false if log doesn’t contain an entry at prevLogIndex
         // whose term matches prevLogTerm (§5.3)
 
-        ReplicatedLogEntry previousEntry = context.getReplicatedLog()
-            .get(appendEntries.getPrevLogIndex());
+        long prevLogTerm = getLogEntryTerm(appendEntries.getPrevLogIndex());
+        boolean prevEntryPresent = isLogEntryPresent(appendEntries.getPrevLogIndex());
 
 
         boolean outOfSync = true;
 
         // First check if the logs are in sync or not
         if (lastIndex() == -1
-            && appendEntries.getPrevLogIndex() != -1) {
+                && appendEntries.getPrevLogIndex() != -1) {
 
             // The follower's log is out of sync because the leader does have
             // an entry at prevLogIndex and this follower has no entries in
@@ -83,34 +110,34 @@ public class Follower extends AbstractRaftActorBehavior {
 
             if(LOG.isDebugEnabled()) {
                 LOG.debug("The followers log is empty and the senders prevLogIndex is {}",
-                    appendEntries.getPrevLogIndex());
+                        appendEntries.getPrevLogIndex());
             }
 
         } else if (lastIndex() > -1
-            && appendEntries.getPrevLogIndex() != -1
-            && previousEntry == null) {
+                && appendEntries.getPrevLogIndex() != -1
+                && !prevEntryPresent) {
 
             // The follower's log is out of sync because the Leader's
             // prevLogIndex entry was not found in it's log
 
             if(LOG.isDebugEnabled()) {
                 LOG.debug("The log is not empty but the prevLogIndex {} was not found in it",
-                    appendEntries.getPrevLogIndex());
+                        appendEntries.getPrevLogIndex());
             }
 
         } else if (lastIndex() > -1
-            && previousEntry != null
-            && previousEntry.getTerm()!= appendEntries.getPrevLogTerm()) {
+                && prevEntryPresent
+                && prevLogTerm != appendEntries.getPrevLogTerm()) {
 
             // The follower's log is out of sync because the Leader's
             // prevLogIndex entry does exist in the follower's log but it has
             // a different term in it
 
-            if(LOG.isDebugEnabled()) {
+            if (LOG.isDebugEnabled()) {
                 LOG.debug(
-                    "Cannot append entries because previous entry term {}  is not equal to append entries prevLogTerm {}"
-                    , previousEntry.getTerm()
-                    , appendEntries.getPrevLogTerm());
+                        "Cannot append entries because previous entry term {}  is not equal to append entries prevLogTerm {}"
+                        , prevLogTerm
+                        , appendEntries.getPrevLogTerm());
             }
         } else {
             outOfSync = false;
@@ -120,9 +147,9 @@ public class Follower extends AbstractRaftActorBehavior {
             // We found that the log was out of sync so just send a negative
             // reply and return
             if(LOG.isDebugEnabled()) {
-                LOG.debug("Follower is out-of-sync, " +
+                LOG.debug("Follower ({}) is out-of-sync, " +
                         "so sending negative reply, lastIndex():{}, lastTerm():{}",
-                    lastIndex(), lastTerm()
+                        context.getId(), lastIndex(), lastTerm()
                 );
             }
             sender.tell(

diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/FollowerTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/FollowerTest.java
index 0ee9693d326e6eff9ff37c77eeaf37b4c8678572..a04d6aeb556cd2f84ffb10ac23302c9e5928451b 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/FollowerTest.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/FollowerTest.java
@@ -421,6 +421,119 @@ public class FollowerTest extends AbstractRaftActorBehaviorTest {
         }};
     }
 
+    @Test
+    public void testHandleAppendEntriesPreviousLogEntryMissing(){
+        new JavaTestKit(getSystem()) {{
+
+            MockRaftActorContext context = (MockRaftActorContext)
+                    createActorContext();
+
+            // Prepare the receivers log
+            MockRaftActorContext.SimpleReplicatedLog log =
+                    new MockRaftActorContext.SimpleReplicatedLog();
+            log.append(
+                    new MockRaftActorContext.MockReplicatedLogEntry(1, 0, new MockRaftActorContext.MockPayload("zero")));
+            log.append(
+                    new MockRaftActorContext.MockReplicatedLogEntry(1, 1, new MockRaftActorContext.MockPayload("one")));
+            log.append(
+                    new MockRaftActorContext.MockReplicatedLogEntry(1, 2, new MockRaftActorContext.MockPayload("two")));
+
+            context.setReplicatedLog(log);
+
+            // Prepare the entries to be sent with AppendEntries
+            List<ReplicatedLogEntry> entries = new ArrayList<>();
+            entries.add(
+                    new MockRaftActorContext.MockReplicatedLogEntry(1, 4, new MockRaftActorContext.MockPayload("two-1")));
+
+            AppendEntries appendEntries =
+                    new AppendEntries(1, "leader-1", 3, 1, entries, 4);
+
+            RaftActorBehavior behavior = createBehavior(context);
+
+            // Send an unknown message so that the state of the RaftActor remains unchanged
+            RaftActorBehavior expected = behavior.handleMessage(getRef(), "unknown");
+
+            RaftActorBehavior raftBehavior =
+                    behavior.handleMessage(getRef(), appendEntries);
+
+            assertEquals(expected, raftBehavior);
+
+            // Also expect an AppendEntriesReply to be sent where success is false
+            final Boolean out = new ExpectMsg<Boolean>(duration("1 seconds"),
+                    "AppendEntriesReply") {
+                // do not put code outside this method, will run afterwards
+                protected Boolean match(Object in) {
+                    if (in instanceof AppendEntriesReply) {
+                        AppendEntriesReply reply = (AppendEntriesReply) in;
+                        return reply.isSuccess();
+                    } else {
+                        throw noMatch();
+                    }
+                }
+            }.get();
+
+            assertEquals(false, out);
+
+        }};
+
+    }
+
+    @Test
+    public void testHandleAppendAfterInstallingSnapshot(){
+        new JavaTestKit(getSystem()) {{
+
+            MockRaftActorContext context = (MockRaftActorContext)
+                    createActorContext();
+
+
+            // Prepare the receivers log
+            MockRaftActorContext.SimpleReplicatedLog log =
+                    new MockRaftActorContext.SimpleReplicatedLog();
+
+            // Set up a log as if it has been snapshotted
+            log.setSnapshotIndex(3);
+            log.setSnapshotTerm(1);
+
+            context.setReplicatedLog(log);
+
+            // Prepare the entries to be sent with AppendEntries
+            List<ReplicatedLogEntry> entries = new ArrayList<>();
+            entries.add(
+                    new MockRaftActorContext.MockReplicatedLogEntry(1, 4, new MockRaftActorContext.MockPayload("two-1")));
+
+            AppendEntries appendEntries =
+                    new AppendEntries(1, "leader-1", 3, 1, entries, 4);
+
+            RaftActorBehavior behavior = createBehavior(context);
+
+            // Send an unknown message so that the state of the RaftActor remains unchanged
+            RaftActorBehavior expected = behavior.handleMessage(getRef(), "unknown");
+
+            RaftActorBehavior raftBehavior =
+                    behavior.handleMessage(getRef(), appendEntries);
+
+            assertEquals(expected, raftBehavior);
+
+            // Also expect an AppendEntriesReply to be sent where success is false
+            final Boolean out = new ExpectMsg<Boolean>(duration("1 seconds"),
+                    "AppendEntriesReply") {
+                // do not put code outside this method, will run afterwards
+                protected Boolean match(Object in) {
+                    if (in instanceof AppendEntriesReply) {
+                        AppendEntriesReply reply = (AppendEntriesReply) in;
+                        return reply.isSuccess();
+                    } else {
+                        throw noMatch();
+                    }
+                }
+            }.get();
+
+            assertEquals(true, out);
+
+        }};
+
+    }
+
 
     /**
      * This test verifies that when InstallSnapshot is received by