From 4ee0f6af9e601cbb5f69a486526d1011314bbfed Mon Sep 17 00:00:00 2001

From: Ben Pfaff <blp@ovn.org>

Date: Thu, 19 Mar 2020 17:53:10 -0700

Subject: [PATCH 01/15] ofproto-dpif-xlate: Fix recirculation when in_port is

 OFPP_CONTROLLER.

[ upstream commit c5a910dd92ecbad24f86b4c59b4ff8105b5149fd ]

Recirculation usually requires finding the pre-recirculation input port.

Packets sent by the controller, with in_port of OFPP_CONTROLLER or

OFPP_NONE, do not have a real input port data structure, only a port

number.  The code in xlate_lookup_ofproto_() mishandled this case,

failing to return the ofproto data structure.  This commit fixes the

problem and adds a test to guard against regression.

Reported-by: Numan Siddique <numans@ovn.org>

Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/368642.html

Tested-by: Numan Siddique <numans@ovn.org>

Acked-by: Numan Siddique <numans@ovn.org>

Signed-off-by: Ben Pfaff <blp@ovn.org>

Resolves: #1775160

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

---

 ofproto/ofproto-dpif-xlate.c | 25 +++++++++++++++++++++----

 tests/ofproto-dpif.at        | 30 ++++++++++++++++++++++++++++++

 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c

index 4407f9c97a..54cfbfbdff 100644

--- a/ofproto/ofproto-dpif-xlate.c

+++ b/ofproto/ofproto-dpif-xlate.c

@@ -1516,15 +1516,32 @@ xlate_lookup_ofproto_(const struct dpif_backer *backer,

             return NULL;

         }

-        /* If recirculation was initiated due to bond (in_port = OFPP_NONE)

-         * then frozen state is static and xport_uuid is not defined, so xport

-         * cannot be restored from frozen state. */

-        if (recirc_id_node->state.metadata.in_port != OFPP_NONE) {

+        ofp_port_t in_port = recirc_id_node->state.metadata.in_port;

+        if (in_port != OFPP_NONE && in_port != OFPP_CONTROLLER) {

             struct uuid xport_uuid = recirc_id_node->state.xport_uuid;

             xport = xport_lookup_by_uuid(xcfg, &xport_uuid);

             if (xport && xport->xbridge && xport->xbridge->ofproto) {

                 goto out;

             }

+        } else {

+            /* OFPP_NONE and OFPP_CONTROLLER are not real ports.  They indicate

+             * that the packet originated from the controller via an OpenFlow

+             * "packet-out".  The right thing to do is to find just the

+             * ofproto.  There is no xport, which is OK.

+             *

+             * OFPP_NONE can also indicate that a bond caused recirculation. */

+            struct uuid uuid = recirc_id_node->state.ofproto_uuid;

+            const struct xbridge *bridge = xbridge_lookup_by_uuid(xcfg, &uuid);

+            if (bridge && bridge->ofproto) {

+                if (errorp) {

+                    *errorp = NULL;

+                }

+                *xportp = NULL;

+                if (ofp_in_port) {

+                    *ofp_in_port = in_port;

+                }

+                return bridge->ofproto;

+            }

         }

     }

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at

index ff1cc93707..d444cf0844 100644

--- a/tests/ofproto-dpif.at

+++ b/tests/ofproto-dpif.at

@@ -5171,6 +5171,36 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 2

 OVS_VSWITCHD_STOP

 AT_CLEANUP

+# Checks for regression against a bug in which OVS dropped packets

+# with in_port=CONTROLLER when they were recirculated (because

+# CONTROLLER isn't a real port and could not be looked up).

+AT_SETUP([ofproto-dpif - packet-out recirculation])

+OVS_VSWITCHD_START

+add_of_ports br0 1 2

+

+AT_DATA([flows.txt], [dnl

+table=0 ip actions=mod_dl_dst:83:83:83:83:83:83,ct(table=1)

+table=1 ip actions=ct(commit),output:2

+])

+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])

+

+packet=ffffffffffff00102030405008004500001c00000000401100000a000002ffffffff0035111100080000

+AT_CHECK([ovs-ofctl packet-out br0 "in_port=controller packet=$packet actions=table"])

+

+# Dumps out the flow table, extracts the number of packets that have gone

+# through the (single) flow in table 1, and returns success if it's exactly 1.

+#

+# If this remains 0, then the recirculation isn't working properly since the

+# packet never goes through flow in table 1.

+check_flows () {

+    n=$(ovs-ofctl dump-flows br0 table=1 | sed -n 's/.*n_packets=\([[0-9]]\{1,\}\).*/\1/p')

+    echo "n_packets=$n"

+    test "$n" = 1

+}

+OVS_WAIT_UNTIL([check_flows], [ovs dump-flows br0])

+

+OVS_VSWITCHD_STOP

+AT_CLEANUP

 AT_SETUP([ofproto-dpif - debug_slow action])

 OVS_VSWITCHD_START

-- 

2.25.1

From 71f25b7920093daa59827a0a4be4095309aec6ff Mon Sep 17 00:00:00 2001

From: Timothy Redaelli <tredaelli@redhat.com>

Date: Thu, 19 Mar 2020 20:05:39 +0100

Subject: [PATCH 02/15] bugtool: Fix for Python3.

Currently ovs-bugtool tool doesn't start on Python 3.

This commit fixes ovs-bugtool to make it works on Python 3.

Replaced StringIO.StringIO with io.BytesIO since the script is

processing binary data.

Reported-at: https://bugzilla.redhat.com/1809241

Reported-by: Flavio Leitner <fbl@sysclose.org>

Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>

Co-authored-by: William Tu <u9012063@gmail.com>

Signed-off-by: William Tu <u9012063@gmail.com>

(cherry picked from commit 9e6c00bca9af29031d0e160d33174b7ae99b9244)

---

 utilities/bugtool/ovs-bugtool.in | 48 +++++++++++++++++---------------

 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in

index e55bfc2ed5..47f3c4629f 100755

--- a/utilities/bugtool/ovs-bugtool.in

+++ b/utilities/bugtool/ovs-bugtool.in

@@ -33,8 +33,7 @@

 # or func_output().

 #

-import StringIO

-import commands

+from io import BytesIO

 import fcntl

 import getopt

 import hashlib

@@ -48,7 +47,7 @@ import warnings

 import zipfile

 from select import select

 from signal import SIGTERM

-from subprocess import PIPE, Popen

+from subprocess import PIPE, Popen, check_output

 from xml.dom.minidom import getDOMImplementation, parse

@@ -348,7 +347,7 @@ def collect_data():

             cap = v['cap']

             if 'cmd_args' in v:

                 if 'output' not in v.keys():

-                    v['output'] = StringIOmtime()

+                    v['output'] = BytesIOmtime()

                 if v['repeat_count'] > 0:

                     if cap not in process_lists:

                         process_lists[cap] = []

@@ -373,20 +372,23 @@ def collect_data():

         if 'filename' in v and v['filename'].startswith('/proc/'):

             # proc files must be read into memory

             try:

-                f = open(v['filename'], 'r')

+                f = open(v['filename'], 'rb')

                 s = f.read()

                 f.close()

                 if check_space(cap, v['filename'], len(s)):

-                    v['output'] = StringIOmtime(s)

+                    v['output'] = BytesIOmtime(s)

             except:

                 pass

         elif 'func' in v:

             try:

                 s = v['func'](cap)

             except Exception as e:

-                s = str(e)

+                s = str(e).encode()

             if check_space(cap, k, len(s)):

-                v['output'] = StringIOmtime(s)

+                if isinstance(s, str):

+                    v['output'] = BytesIOmtime(s.encode())

+                else:

+                    v['output'] = BytesIOmtime(s)

 def main(argv=None):

@@ -704,7 +706,7 @@ exclude those logs from the archive.

     # permit the user to filter out data

     # We cannot use iteritems, since we modify 'data' as we pass through

-    for (k, v) in sorted(data.items()):

+    for (k, v) in data.items():

         cap = v['cap']

         if 'filename' in v:

             key = k[0]

@@ -721,7 +723,7 @@ exclude those logs from the archive.

     # include inventory

     data['inventory.xml'] = {'cap': None,

-                        'output': StringIOmtime(make_inventory(data, subdir))}

+                        'output': BytesIOmtime(make_inventory(data, subdir))}

     # create archive

     if output_fd == -1:

@@ -782,7 +784,7 @@ def dump_scsi_hosts(cap):

 def module_info(cap):

-    output = StringIO.StringIO()

+    output = BytesIO()

     modules = open(PROC_MODULES, 'r')

     procs = []

@@ -806,7 +808,7 @@ def multipathd_topology(cap):

 def dp_list():

-    output = StringIO.StringIO()

+    output = BytesIO()

     procs = [ProcOutput([OVS_DPCTL, 'dump-dps'],

              caps[CAP_NETWORK_STATUS][MAX_TIME], output)]

@@ -828,7 +830,7 @@ def collect_ovsdb():

             if os.path.isfile(OPENVSWITCH_COMPACT_DB):

                 os.unlink(OPENVSWITCH_COMPACT_DB)

-            output = StringIO.StringIO()

+            output = BytesIO()

             max_time = 5

             procs = [ProcOutput(['ovsdb-tool', 'compact',

                                 OPENVSWITCH_CONF_DB, OPENVSWITCH_COMPACT_DB],

@@ -871,7 +873,7 @@ def fd_usage(cap):

 def dump_rdac_groups(cap):

-    output = StringIO.StringIO()

+    output = BytesIO()

     procs = [ProcOutput([MPPUTIL, '-a'], caps[cap][MAX_TIME], output)]

     run_procs([procs])

@@ -896,7 +898,7 @@ def load_plugins(just_capabilities=False, filter=None):

         for node in nodelist:

             if node.nodeType == node.TEXT_NODE:

                 rc += node.data

-        return rc.encode()

+        return rc

     def getBoolAttr(el, attr, default=False):

         ret = default

@@ -1037,7 +1039,7 @@ def make_tar(subdir, suffix, output_fd, output_file):

                     s = os.stat(v['filename'])

                     ti.mtime = s.st_mtime

                     ti.size = s.st_size

-                    tf.addfile(ti, open(v['filename']))

+                    tf.addfile(ti, open(v['filename'], 'rb'))

             except:

                 pass

     finally:

@@ -1095,12 +1097,12 @@ def make_inventory(inventory, subdir):

     s.setAttribute('date', time.strftime('%c'))

     s.setAttribute('hostname', platform.node())

     s.setAttribute('uname', ' '.join(platform.uname()))

-    s.setAttribute('uptime', commands.getoutput(UPTIME))

+    s.setAttribute('uptime', check_output(UPTIME).decode())

     document.getElementsByTagName(INVENTORY_XML_ROOT)[0].appendChild(s)

     map(lambda k_v: inventory_entry(document, subdir, k_v[0], k_v[1]),

         inventory.items())

-    return document.toprettyxml()

+    return document.toprettyxml().encode()

 def inventory_entry(document, subdir, k, v):

@@ -1301,7 +1303,7 @@ class ProcOutput(object):

             line = self.proc.stdout.readline()

         else:

             line = self.proc.stdout.read(self.bufsize)

-        if line == '':

+        if line == b'':

             # process exited

             self.proc.stdout.close()

             self.status = self.proc.wait()

@@ -1391,13 +1393,13 @@ def get_free_disk_space(path):

     return s.f_frsize * s.f_bfree

-class StringIOmtime(StringIO.StringIO):

-    def __init__(self, buf=''):

-        StringIO.StringIO.__init__(self, buf)

+class BytesIOmtime(BytesIO):

+    def __init__(self, buf=b''):

+        BytesIO.__init__(self, buf)

         self.mtime = time.time()

     def write(self, s):

-        StringIO.StringIO.write(self, s)

+        BytesIO.write(self, s)

         self.mtime = time.time()

-- 

2.25.1

From 914d885061c9f7e7e6e5f921065301e08837e122 Mon Sep 17 00:00:00 2001

From: Han Zhou <hzhou@ovn.org>

Date: Fri, 28 Feb 2020 18:07:04 -0800

Subject: [PATCH 03/15] raft-rpc: Fix message format.

[ upstream commit 78c8011f58daec41ec97440f2e42795699322742 ]

Signed-off-by: Han Zhou <hzhou@ovn.org>

Signed-off-by: Ben Pfaff <blp@ovn.org>

Resolves: #1836305

Signed-off-by: Dumitru Ceara <dceara@redhat.com>

---

 ovsdb/raft-rpc.c | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovsdb/raft-rpc.c b/ovsdb/raft-rpc.c

index 18c83fe9c2..dd14d81091 100644

--- a/ovsdb/raft-rpc.c

+++ b/ovsdb/raft-rpc.c

@@ -544,8 +544,8 @@ raft_format_install_snapshot_request(

     ds_put_format(s, " last_index=%"PRIu64, rq->last_index);

     ds_put_format(s, " last_term=%"PRIu64, rq->last_term);

     ds_put_format(s, " last_eid="UUID_FMT, UUID_ARGS(&rq->last_eid));

-    ds_put_cstr(s, " last_servers=");

     ds_put_format(s, " election_timer=%"PRIu64, rq->election_timer);

+    ds_put_cstr(s, " last_servers=");

     struct hmap servers;

     struct ovsdb_error *error =

-- 

2.25.1

From 8ff30dfee6cb075e36ed38b77695ff03321ce12b Mon Sep 17 00:00:00 2001

From: Han Zhou <hzhou@ovn.org>

Date: Fri, 28 Feb 2020 18:07:05 -0800

Subject: [PATCH 04/15] ovsdb-server: Don't disconnect clients after raft

 install_snapshot.

[ upstream commit f0c8b44c5832c36989fad78927407fc14e64ce46 ]

When "schema" field is found in read_db(), there can be two cases:

1. There is a schema change in clustered DB and the "schema" is the new one.

2. There is a install_snapshot RPC happened, which caused log compaction on the

server and the next log is just the snapshot, which always constains "schema"

field, even though the schema hasn't been changed.

The current implementation doesn't handle case 2), and always assume the schema

is changed hence disconnect all clients of the server. It can cause stability

problem when there are big number of clients connected when this happens in

a large scale environment.

Signed-off-by: Han Zhou <hzhou@ovn.org>

Signed-off-by: Ben Pfaff <blp@ovn.org>

Resolves: #1836305

Signed-off-by: Dumitru Ceara <dceara@redhat.com>

---

 ovsdb/ovsdb-server.c   |  3 ++-

 tests/ovsdb-cluster.at | 56 ++++++++++++++++++++++++++++++++++++++++++

 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c

index b6957d7300..d416f1b606 100644

--- a/ovsdb/ovsdb-server.c

+++ b/ovsdb/ovsdb-server.c

@@ -543,7 +543,8 @@ parse_txn(struct server_config *config, struct db *db,

           struct ovsdb_schema *schema, const struct json *txn_json,

           const struct uuid *txnid)

 {

-    if (schema) {

+    if (schema && (!db->db->schema || strcmp(schema->version,

+                                             db->db->schema->version))) {

         /* We're replacing the schema (and the data).  Destroy the database

          * (first grabbing its storage), then replace it with the new schema.

          * The transaction must also include the replacement data.

diff --git a/tests/ovsdb-cluster.at b/tests/ovsdb-cluster.at

index 3a0bd4579e..5b6188b96f 100644

--- a/tests/ovsdb-cluster.at

+++ b/tests/ovsdb-cluster.at

@@ -273,6 +273,62 @@ OVS_WAIT_UNTIL([ovs-appctl -t "`pwd`"/s4 cluster/status $schema_name | grep "Ele

 AT_CLEANUP

+

+AT_BANNER([OVSDB cluster install snapshot RPC])

+

+AT_SETUP([OVSDB cluster - install snapshot RPC])

+AT_KEYWORDS([ovsdb server positive unix cluster snapshot])

+

+n=3

+schema_name=`ovsdb-tool schema-name $abs_srcdir/idltest.ovsschema`

+ordinal_schema > schema

+AT_CHECK([ovsdb-tool '-vPATTERN:console:%c|%p|%m' create-cluster s1.db $abs_srcdir/idltest.ovsschema unix:s1.raft], [0], [], [stderr])

+cid=`ovsdb-tool db-cid s1.db`

+schema_name=`ovsdb-tool schema-name $abs_srcdir/idltest.ovsschema`

+for i in `seq 2 $n`; do

+    AT_CHECK([ovsdb-tool join-cluster s$i.db $schema_name unix:s$i.raft unix:s1.raft])

+done

+

+on_exit 'kill `cat *.pid`'

+for i in `seq $n`; do

+    AT_CHECK([ovsdb-server -v -vconsole:off -vsyslog:off --detach --no-chdir --log-file=s$i.log --pidfile=s$i.pid --unixctl=s$i --remote=punix:s$i.ovsdb s$i.db])

+done

+for i in `seq $n`; do

+    AT_CHECK([ovsdb_client_wait unix:s$i.ovsdb $schema_name connected])

+done

+

+# Kill one follower (s2) and write some data to cluster, so that the follower is falling behind

+printf "\ns2: stopping\n"

+OVS_APP_EXIT_AND_WAIT_BY_TARGET([`pwd`/s2], [s2.pid])

+

+AT_CHECK([ovsdb-client transact unix:s1.ovsdb '[["idltest",

+      {"op": "insert",

+       "table": "simple",

+       "row": {"i": 1}}]]'], [0], [ignore], [ignore])

+

+# Compact leader online to generate snapshot

+AT_CHECK([ovs-appctl -t "`pwd`"/s1 ovsdb-server/compact])

+

+# Start the follower s2 again.

+AT_CHECK([ovsdb-server -v -vconsole:off -vsyslog:off --detach --no-chdir --log-file=s2.log --pidfile=s2.pid --unixctl=s2 --remote=punix:s2.ovsdb s2.db])

+AT_CHECK([ovsdb_client_wait unix:s2.ovsdb $schema_name connected])

+

+# A client transaction through s2. During this transaction, there will be a

+# install_snapshot RPC because s2 detects it is behind and s1 doesn't have the

+# pre_log_index requested by s2 because it is already compacted.

+# After the install_snapshot RPC process, the transaction through s2 should

+# succeed.

+AT_CHECK([ovsdb-client transact unix:s2.ovsdb '[["idltest",

+      {"op": "insert",

+       "table": "simple",

+       "row": {"i": 1}}]]'], [0], [ignore], [ignore])

+

+for i in `seq $n`; do

+    OVS_APP_EXIT_AND_WAIT_BY_TARGET([`pwd`/s$i], [s$i.pid])

+done

+

+AT_CLEANUP

+

 OVS_START_SHELL_HELPERS

-- 

2.25.1

From e732012d7be335650398ff03c2431c64b2c4aaba Mon Sep 17 00:00:00 2001

From: Han Zhou <hzhou@ovn.org>

Date: Fri, 28 Feb 2020 18:07:06 -0800

Subject: [PATCH 05/15] raft: Fix raft_is_connected() when there is no leader

 yet.

[ upstream commit adc64ab057345f7004c44bf92363b9adda862134 ]

If there is never a leader known by the current server, it's status

should be "disconnected" to the cluster. Without this patch, when

a server in cluster is restarted, before it successfully connecting

back to the cluster it will appear as connected, which is wrong.

Signed-off-by: Han Zhou <hzhou@ovn.org>

Signed-off-by: Ben Pfaff <blp@ovn.org>

Resolves: #1836305

Signed-off-by: Dumitru Ceara <dceara@redhat.com>

---

 ovsdb/raft.c           | 10 ++++++++--

 tests/ovsdb-cluster.at | 35 +++++++++++++++++++++++++++++++++++

 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/ovsdb/raft.c b/ovsdb/raft.c

index 4789bc4f22..6cd7b0041a 100644

--- a/ovsdb/raft.c

+++ b/ovsdb/raft.c

@@ -298,6 +298,11 @@ struct raft {

     bool had_leader;            /* There has been leader elected since last

                                    election initiated. This is to help setting

                                    candidate_retrying. */

+

+    /* For all. */

+    bool ever_had_leader;       /* There has been leader elected since the raft

+                                   is initialized, meaning it is ever

+                                   connected. */

 };

 /* All Raft structures. */

@@ -1024,7 +1029,8 @@ raft_is_connected(const struct raft *raft)

             && !raft->joining

             && !raft->leaving

             && !raft->left

-            && !raft->failed);

+            && !raft->failed

+            && raft->ever_had_leader);

     VLOG_DBG("raft_is_connected: %s\n", ret? "true": "false");

     return ret;

 }

@@ -2519,7 +2525,7 @@ static void

 raft_set_leader(struct raft *raft, const struct uuid *sid)

 {

     raft->leader_sid = *sid;

-    raft->had_leader = true;

+    raft->ever_had_leader = raft->had_leader = true;

     raft->candidate_retrying = false;

 }

diff --git a/tests/ovsdb-cluster.at b/tests/ovsdb-cluster.at

index 5b6188b96f..0aa4564480 100644

--- a/tests/ovsdb-cluster.at

+++ b/tests/ovsdb-cluster.at

@@ -179,6 +179,41 @@ AT_KEYWORDS([ovsdb server negative unix cluster disconnect])

 ovsdb_test_cluster_disconnect 5 leader yes

 AT_CLEANUP

+AT_SETUP([OVSDB cluster - initial status should be disconnected])

+AT_KEYWORDS([ovsdb server negative unix cluster disconnect])

+

+n=3

+schema_name=`ovsdb-tool schema-name $abs_srcdir/idltest.ovsschema`

+ordinal_schema > schema

+AT_CHECK([ovsdb-tool '-vPATTERN:console:%c|%p|%m' create-cluster s1.db $abs_srcdir/idltest.ovsschema unix:s1.raft], [0], [], [stderr])

+cid=`ovsdb-tool db-cid s1.db`

+schema_name=`ovsdb-tool schema-name $abs_srcdir/idltest.ovsschema`

+for i in `seq 2 $n`; do

+    AT_CHECK([ovsdb-tool join-cluster s$i.db $schema_name unix:s$i.raft unix:s1.raft])

+done

+

+on_exit 'kill `cat *.pid`'

+for i in `seq $n`; do

+    AT_CHECK([ovsdb-server -v -vconsole:off -vsyslog:off --detach --no-chdir --log-file=s$i.log --pidfile=s$i.pid --unixctl=s$i --remote=punix:s$i.ovsdb s$i.db])

+done

+for i in `seq $n`; do

+    AT_CHECK([ovsdb_client_wait unix:s$i.ovsdb $schema_name connected])

+done

+

+# Stop all servers, and start the s1 only, to test initial connection status

+# when there is no leader yet.

+for i in `seq 1 $n`; do

+    OVS_APP_EXIT_AND_WAIT_BY_TARGET([`pwd`/s$i], [s$i.pid])

+done

+i=1

+AT_CHECK([ovsdb-server -v -vconsole:off -vsyslog:off --detach --no-chdir --log-file=s$i.log --pidfile=s$i.pid --unixctl=s$i --remote=punix:s$i.ovsdb s$i.db])

+

+# The initial status should be disconnected. So wait should fail.

+AT_CHECK([ovsdb_client_wait --timeout=1 unix:s$i.ovsdb $schema_name connected], [142], [ignore], [ignore])

+OVS_APP_EXIT_AND_WAIT_BY_TARGET([`pwd`/s$i], [s$i.pid])

+

+AT_CLEANUP

+

 AT_BANNER([OVSDB cluster election timer change])

-- 

2.25.1

From 053b78c8d60ffb4d212fd7894f91be52027f291f Mon Sep 17 00:00:00 2001

From: Han Zhou <hzhou@ovn.org>

Date: Fri, 28 Feb 2020 18:07:07 -0800

Subject: [PATCH 06/15] raft: Avoid busy loop during leader election.

[ upstream commit 3ae90e1899c5a05148ea1870d9bb4ac3c05e3a19 ]

When a server doesn't see a leader yet, e.g. during leader re-election,

if a transaction comes from a client, it will cause 100% CPU busy loop.

With debug log enabled it is like:

2020-02-28T04:04:35.631Z|00059|poll_loop|DBG|wakeup due to 0-ms timeout at ../ovsdb/trigger.c:164

2020-02-28T04:04:35.631Z|00062|poll_loop|DBG|wakeup due to 0-ms timeout at ../ovsdb/trigger.c:164

2020-02-28T04:04:35.631Z|00065|poll_loop|DBG|wakeup due to 0-ms timeout at ../ovsdb/trigger.c:164

2020-02-28T04:04:35.631Z|00068|poll_loop|DBG|wakeup due to 0-ms timeout at ../ovsdb/trigger.c:164

2020-02-28T04:04:35.631Z|00071|poll_loop|DBG|wakeup due to 0-ms timeout at ../ovsdb/trigger.c:164

2020-02-28T04:04:35.631Z|00074|poll_loop|DBG|wakeup due to 0-ms timeout at ../ovsdb/trigger.c:164

2020-02-28T04:04:35.631Z|00077|poll_loop|DBG|wakeup due to 0-ms timeout at ../ovsdb/trigger.c:164

...

The problem is that in ovsdb_trigger_try(), all cluster errors are treated

as temporary error and retry immediately. This patch fixes it by introducing

'run_triggers_now', which tells if a retry is needed immediately. When the

cluster error is with detail 'not leader', we don't immediately retry, but

will wait for the next poll event to trigger the retry. When 'not leader'

status changes, there must be a event, i.e. raft RPC that changes the

status, so the trigger is guaranteed to be triggered, without busy loop.

Signed-off-by: Han Zhou <hzhou@ovn.org>

Signed-off-by: Ben Pfaff <blp@ovn.org>

Resolves: #1836305

Signed-off-by: Dumitru Ceara <dceara@redhat.com>

---

 ovsdb/ovsdb.c       |  2 +-

 ovsdb/ovsdb.h       |  1 +

 ovsdb/transaction.c |  2 +-

 ovsdb/trigger.c     | 11 +++++++++--

 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c

index cfc96b32f8..7e683e6815 100644

--- a/ovsdb/ovsdb.c

+++ b/ovsdb/ovsdb.c

@@ -414,7 +414,7 @@ ovsdb_create(struct ovsdb_schema *schema, struct ovsdb_storage *storage)

     db->storage = storage;

     ovs_list_init(&db->monitors);

     ovs_list_init(&db->triggers);

-    db->run_triggers = false;

+    db->run_triggers_now = db->run_triggers = false;

     shash_init(&db->tables);

     if (schema) {

diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h

index 32e5333163..5c30a83d92 100644

--- a/ovsdb/ovsdb.h

+++ b/ovsdb/ovsdb.h

@@ -83,6 +83,7 @@ struct ovsdb {

     /* Triggers. */

     struct ovs_list triggers;   /* Contains "struct ovsdb_trigger"s. */

     bool run_triggers;

+    bool run_triggers_now;

     struct ovsdb_table *rbac_role;

diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c

index 369436bffb..8ffefcf7c9 100644

--- a/ovsdb/transaction.c

+++ b/ovsdb/transaction.c

@@ -967,7 +967,7 @@ ovsdb_txn_complete(struct ovsdb_txn *txn)

 {

     if (!ovsdb_txn_is_empty(txn)) {

-        txn->db->run_triggers = true;

+        txn->db->run_triggers_now = txn->db->run_triggers = true;

         ovsdb_monitors_commit(txn->db, txn);

         ovsdb_error_assert(for_each_txn_row(txn, ovsdb_txn_update_weak_refs));

         ovsdb_error_assert(for_each_txn_row(txn, ovsdb_txn_row_commit));

diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c

index 7e62e90ae3..0372302af4 100644

--- a/ovsdb/trigger.c

+++ b/ovsdb/trigger.c

@@ -141,7 +141,7 @@ ovsdb_trigger_run(struct ovsdb *db, long long int now)

     struct ovsdb_trigger *t, *next;

     bool run_triggers = db->run_triggers;

-    db->run_triggers = false;

+    db->run_triggers_now = db->run_triggers = false;

     bool disconnect_all = false;

@@ -160,7 +160,7 @@ ovsdb_trigger_run(struct ovsdb *db, long long int now)

 void

 ovsdb_trigger_wait(struct ovsdb *db, long long int now)

 {

-    if (db->run_triggers) {

+    if (db->run_triggers_now) {

         poll_immediate_wake();

     } else {

         long long int deadline = LLONG_MAX;

@@ -319,9 +319,16 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now)

             if (!strcmp(ovsdb_error_get_tag(error), "cluster error")) {

                 /* Temporary error.  Transition back to "initialized" state to

                  * try again. */

+                char *err_s = ovsdb_error_to_string(error);

+                VLOG_DBG("cluster error %s", err_s);

+

                 jsonrpc_msg_destroy(t->reply);

                 t->reply = NULL;

                 t->db->run_triggers = true;

+                if (!strstr(err_s, "not leader")) {

+                    t->db->run_triggers_now = true;

+                }

+                free(err_s);

                 ovsdb_error_destroy(error);

             } else {

                 /* Permanent error.  Transition to "completed" state to report

-- 

2.25.1

From cc3d02699203e2fe9d9fd384d09e268ba614828d Mon Sep 17 00:00:00 2001

From: Han Zhou <hzhou@ovn.org>

Date: Fri, 28 Feb 2020 18:07:10 -0800

Subject: [PATCH 07/15] raft: Fix next_index in install_snapshot reply

 handling.

[ upstream commit 877618fc833273d1e29e012b5e925d51cba80ff5 ]

When a leader handles install_snapshot reply, the next_index for

the follower should be log_start instead of log_end, because there

can be new entries added in leader's log after initiating the

install_snapshot procedure.  Also, it should send all the accumulated

entries to follower in the following append-request message, instead

of sending 0 entries, to speed up the converge.

Without this fix, there is no functional problem, but it takes

uncessary extra rounds of append-requests responsed with "inconsistency"

by follower, although finally will be converged.

Signed-off-by: Han Zhou <hzhou@ovn.org>

Signed-off-by: Ben Pfaff <blp@ovn.org>

Resolves: #1836305

Signed-off-by: Dumitru Ceara <dceara@redhat.com>

---

 ovsdb/raft.c | 5 +++--

 1 file changed, 3 insertions(+), 2 deletions(-)