[ovs-dev] [PATCH 5/5] raft: Avoid having more than one snapshot in-flight.

[Ilya Maximets]

Previous commit 8c2c503bdb0d ("raft: Avoid sending equal snapshots.")
took a "safe" approach to not send only exactly same snapshot
installation requests.  However, it doesn't make much sense to send
more than one snapshot at a time.  If obsolete snapshot installed,
leader will re-send the most recent one.

With this change leader will have only 1 snapshot in-flight per
connection.  This will reduce backlogs on raft connections in case
new snapshot created while 'install_snapshot_request' is in progress
or if election timer changed in that period.

Also, not tracking the exact 'install_snapshot_request' we've sent
allows to simplify the code.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1888829
Fixes: 8c2c503bdb0d ("raft: Avoid sending equal snapshots.")
Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
---
 ovsdb/raft-private.c |  1 -
 ovsdb/raft-private.h |  4 ++--
 ovsdb/raft.c         | 42 ++++++++++++++++--------------------------
 3 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/ovsdb/raft-private.c b/ovsdb/raft-private.c
index 9468fdaf4..26d39a087 100644
--- a/ovsdb/raft-private.c
+++ b/ovsdb/raft-private.c
@@ -137,7 +137,6 @@ raft_server_destroy(struct raft_server *s)
     if (s) {
         free(s->address);
         free(s->nickname);
-        free(s->last_install_snapshot_request);
         free(s);
     }
 }
diff --git a/ovsdb/raft-private.h b/ovsdb/raft-private.h
index 1f366b4ab..76b097b89 100644
--- a/ovsdb/raft-private.h
+++ b/ovsdb/raft-private.h
@@ -84,8 +84,8 @@ struct raft_server {
     bool replied;            /* Reply to append_request was received from this
                                 node during current election_timeout interval.
                                 */
-    /* Copy of the last install_snapshot_request sent to this server. */
-    struct raft_install_snapshot_request *last_install_snapshot_request;
+    /* install_snapshot_request has been sent, but there is no response yet. */
+    bool install_snapshot_request_in_progress;
 
     /* For use in adding and removing servers: */
     struct uuid requester_sid;  /* Nonzero if requested via RPC. */
diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index 7cfa66fc4..760dfca6d 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -1448,12 +1448,11 @@ raft_conn_run(struct raft *raft, struct raft_conn *conn)
                            && jsonrpc_session_is_connected(conn->js));
 
     if (reconnected) {
-        /* Clear 'last_install_snapshot_request' since it might not reach the
-         * destination or server was restarted. */
+        /* Clear 'install_snapshot_request_in_progress' since it might not
+         * reach the destination or server was restarted. */
         struct raft_server *server = raft_find_server(raft, &conn->sid);
         if (server) {
-            free(server->last_install_snapshot_request);
-            server->last_install_snapshot_request = NULL;
+            server->install_snapshot_request_in_progress = false;
         }
     }
 
@@ -2575,6 +2574,7 @@ raft_server_init_leader(struct raft *raft, struct raft_server *s)
     s->match_index = 0;
     s->phase = RAFT_PHASE_STABLE;
     s->replied = false;
+    s->install_snapshot_request_in_progress = false;
 }
 
 static void
@@ -3331,31 +3331,19 @@ raft_send_install_snapshot_request(struct raft *raft,
         }
     };
 
-    if (s->last_install_snapshot_request) {
-        struct raft_install_snapshot_request *old, *new;
-
-        old = s->last_install_snapshot_request;
-        new = &rpc.install_snapshot_request;
-        if (   old->term           == new->term
-            && old->last_index     == new->last_index
-            && old->last_term      == new->last_term
-            && old->last_servers   == new->last_servers
-            && old->data           == new->data
-            && old->election_timer == new->election_timer
-            && uuid_equals(&old->last_eid, &new->last_eid)) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
+    if (s->install_snapshot_request_in_progress) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
 
-            VLOG_WARN_RL(&rl, "not sending exact same install_snapshot_request"
-                              " to server %s again", s->nickname);
-            return;
-        }
+        VLOG_INFO_RL(&rl, "not sending snapshot to server %s, "
+                          "already in progress", s->nickname);
+        return;
     }
-    free(s->last_install_snapshot_request);
-    CONST_CAST(struct raft_server *, s)->last_install_snapshot_request
-        = xmemdup(&rpc.install_snapshot_request,
-                  sizeof rpc.install_snapshot_request);
 
-    raft_send(raft, &rpc);
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
+    VLOG_INFO_RL(&rl, "sending snapshot to server %s, %"PRIu64":%"PRIu64".",
+                      s->nickname, raft->term, raft->log_start - 1);
+    CONST_CAST(struct raft_server *, s)->install_snapshot_request_in_progress
+        = raft_send(raft, &rpc);
 }
 
 static void
@@ -4072,6 +4060,8 @@ raft_handle_install_snapshot_reply(
         }
     }
 
+    s->install_snapshot_request_in_progress = false;
+
     if (rpy->last_index != raft->log_start - 1 ||
         rpy->last_term != raft->snap.term) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
-- 
2.25.4