Fix delete snapshots criteria

When a snapshot is saved, we attempt to delete prior snapshots as only
the last one is recovered from persistence. For the maxSequenceNr in the
criteria, we use snapshot sequenceNr - snapshot batch count. However
this assumes every snapshot is based on snapshot batch count. We may
snapshot for other reasons, eg install snapshot on follower. This can
leave multiple prior snapshots behind and use up significant disk space.

We should only retain the last snapshot saved so I changed the criteria
to use: maxSequenceNr = the saved snapshot sequenceNr (it's possible the
sequenceNr hasn't changed from th elast snapshot) and maxTimeStamp =
saved snapshot timestamp -1.

Change-Id: I35b1d71ed433d52ecff79ca07a81616e393a7b7f
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>

diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java
index b35f4babb1a4999452a2703229a7eebf48a57fb2..5173dc89f7c6b114ab857eb1970a2793b3c2f2e8 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java
@@ -60,7 +60,7 @@ class RaftActorSnapshotMessageSupport {
         } else if (message instanceof CaptureSnapshotReply) {
             onCaptureSnapshotReply(((CaptureSnapshotReply) message).getSnapshot());
         } else if (COMMIT_SNAPSHOT.equals(message)) {
-            context.getSnapshotManager().commit(-1);
+            context.getSnapshotManager().commit(-1, -1);
         } else if (message instanceof GetSnapshot) {
             onGetSnapshot(sender);
         } else {
@@ -84,11 +84,11 @@ class RaftActorSnapshotMessageSupport {
     }
 
     private void onSaveSnapshotSuccess(SaveSnapshotSuccess success) {
-        log.info("{}: SaveSnapshotSuccess received for snapshot", context.getId());
-
         long sequenceNumber = success.metadata().sequenceNr();
 
-        context.getSnapshotManager().commit(sequenceNumber);
+        log.info("{}: SaveSnapshotSuccess received for snapshot, sequenceNr: {}", context.getId(), sequenceNumber);
+
+        context.getSnapshotManager().commit(sequenceNumber, success.metadata().timestamp());
     }
 
     private void onApplySnapshot(ApplySnapshot message) {

diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java
index dd52c38685156f1de85584c9bf8b1d65376492f5..a362c6f683be25d108589a36bbfc5cbb6152c455 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java
@@ -77,8 +77,8 @@ public class SnapshotManager implements SnapshotState {
     }
 
     @Override
-    public void commit(final long sequenceNumber) {
-        currentState.commit(sequenceNumber);
+    public void commit(final long sequenceNumber, long timeStamp) {
+        currentState.commit(sequenceNumber, timeStamp);
     }
 
     @Override
@@ -177,7 +177,7 @@ public class SnapshotManager implements SnapshotState {
         }
 
         @Override
-        public void commit(final long sequenceNumber) {
+        public void commit(final long sequenceNumber, long timeStamp) {
             LOG.debug("commit should not be called in state {}", this);
         }
 
@@ -385,7 +385,7 @@ public class SnapshotManager implements SnapshotState {
     private class Persisting extends AbstractSnapshotState {
 
         @Override
-        public void commit(final long sequenceNumber) {
+        public void commit(final long sequenceNumber, long timeStamp) {
             LOG.debug("{}: Snapshot success -  sequence number: {}", persistenceId(), sequenceNumber);
 
             if(applySnapshot != null) {
@@ -414,8 +414,8 @@ public class SnapshotManager implements SnapshotState {
                 context.getReplicatedLog().snapshotCommit();
             }
 
-            context.getPersistenceProvider().deleteSnapshots(new SnapshotSelectionCriteria(
-                    sequenceNumber - context.getConfigParams().getSnapshotBatchCount(), Long.MAX_VALUE, 0L, 0L));
+            context.getPersistenceProvider().deleteSnapshots(new SnapshotSelectionCriteria(sequenceNumber,
+                    timeStamp - 1, 0L, 0L));
 
             context.getPersistenceProvider().deleteMessages(lastSequenceNumber);
 

diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java
index 5d1304fe752222214a82b9d877da996dc517feb4..a6bcddfc137a104684d40fd3d301b5513c60ebb7 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java
@@ -57,8 +57,9 @@ public interface SnapshotState {
      * Commit the snapshot by trimming the log
      *
      * @param sequenceNumber
+     * @param timeStamp
      */
-    void commit(long sequenceNumber);
+    void commit(long sequenceNumber, long timeStamp);
 
     /**
      * Rollback the snapshot

diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java
index 32b2e465f6fcf83d32a6e9f213caee16ea020f8a..406ed3c315d807cbcc9426ada4bc9fed9dd6392a 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java
@@ -116,9 +116,10 @@ public class RaftActorSnapshotMessageSupportTest {
     public void testOnSaveSnapshotSuccess() {
 
         long sequenceNumber = 100;
-        sendMessageToSupport(new SaveSnapshotSuccess(new SnapshotMetadata("foo", sequenceNumber, 1234L)));
+        long timeStamp = 1234L;
+        sendMessageToSupport(new SaveSnapshotSuccess(new SnapshotMetadata("foo", sequenceNumber, timeStamp)));
 
-        verify(mockSnapshotManager).commit(eq(sequenceNumber));
+        verify(mockSnapshotManager).commit(eq(sequenceNumber), eq(timeStamp));
     }
 
     @Test
@@ -135,7 +136,7 @@ public class RaftActorSnapshotMessageSupportTest {
 
         sendMessageToSupport(RaftActorSnapshotMessageSupport.COMMIT_SNAPSHOT);
 
-        verify(mockSnapshotManager).commit(eq(-1L));
+        verify(mockSnapshotManager).commit(eq(-1L), eq(-1L));
     }
 
     @Test

diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java
index 6565c59b5f6ad4c9639d4d001f57c76fa2872f91..24ebad57ad2a384cbd16724302c296205c131741 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java
@@ -659,7 +659,7 @@ public class RaftActorTest extends AbstractActorTest {
                 assertTrue(leaderActor.getRaftActorContext().getSnapshotManager().isCapturing());
 
                 // The commit is needed to complete the snapshot creation process
-                leaderActor.getRaftActorContext().getSnapshotManager().commit(-1);
+                leaderActor.getRaftActorContext().getSnapshotManager().commit(-1, -1);
 
                 // capture snapshot reply should remove the snapshotted entries only
                 assertEquals(3, leaderActor.getReplicatedLog().size());
@@ -762,7 +762,7 @@ public class RaftActorTest extends AbstractActorTest {
                 assertTrue(followerActor.getRaftActorContext().getSnapshotManager().isCapturing());
 
                 // The commit is needed to complete the snapshot creation process
-                followerActor.getRaftActorContext().getSnapshotManager().commit(-1);
+                followerActor.getRaftActorContext().getSnapshotManager().commit(-1, -1);
 
                 // capture snapshot reply should remove the snapshotted entries only till replicatedToAllIndex
                 assertEquals(3, followerActor.getReplicatedLog().size()); //indexes 5,6,7 left in the log

diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java
index e35c9bd44a2bb7992136e45e43ded29c720c02d5..c619917fd4f61dba44d54a6746c256ae23a1d5ef 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java
@@ -413,7 +413,7 @@ public class SnapshotManagerTest extends AbstractActorTest {
 
         assertEquals(true, snapshotManager.isCapturing());
 
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 1234L);
 
         assertEquals(false, snapshotManager.isCapturing());
 
@@ -425,9 +425,8 @@ public class SnapshotManagerTest extends AbstractActorTest {
 
         verify(mockDataPersistenceProvider).deleteSnapshots(criteriaCaptor.capture());
 
-        assertEquals(90, criteriaCaptor.getValue().maxSequenceNr()); // sequenceNumber = 100
-                                                                     // config snapShotBatchCount = 10
-                                                                     // therefore maxSequenceNumber = 90
+        assertEquals(100L, criteriaCaptor.getValue().maxSequenceNr());
+        assertEquals(1233L, criteriaCaptor.getValue().maxTimestamp());
 
         MessageCollectorActor.expectFirstMatching(actorRef, SnapshotComplete.class);
     }
@@ -438,7 +437,7 @@ public class SnapshotManagerTest extends AbstractActorTest {
         snapshotManager.captureToInstall(new MockRaftActorContext.MockReplicatedLogEntry(6, 9,
                 new MockRaftActorContext.MockPayload()), -1, "follower-1");
 
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 0);
 
         verify(mockReplicatedLog, never()).snapshotCommit();
 
@@ -450,7 +449,7 @@ public class SnapshotManagerTest extends AbstractActorTest {
 
     @Test
     public void testCommitBeforeCapture(){
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 0);
 
         verify(mockReplicatedLog, never()).snapshotCommit();
 
@@ -470,9 +469,9 @@ public class SnapshotManagerTest extends AbstractActorTest {
 
         snapshotManager.persist(new byte[]{}, Runtime.getRuntime().totalMemory());
 
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 0);
 
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 0);
 
         verify(mockReplicatedLog, times(1)).snapshotCommit();