controller: Fix bfd up too early after unexpected reboot.

If a server unexpectedly rebooted, OVS, when restarted, sets BFD
UP on bfd-enabled geneve tunnels.
However, if it takes time to restart OVN, an HA gw chassis
would attract the traffic while being unable to handle it
(as no flows), resulting in traffic loss.

This is fixed by re-using ovs flow-restore-wait.
If set, OVS waits (prevents upcalls, ignores bfd, ...) until reset.
Once OVS receives the notification of flow-restore-wait being false,
it restarts handling upcalls, bfd... and ignores any new change to
flow-restore-wait.

Hence, on chassis hosting ha gateways, OVN toggles flow-restore-wait:
it set it to false, waits for ack from OVS and then sets it back to true.
If server reboots, OVS will see flow-restore-wait being true.

OVN also sets external_ids->ovn-managed-flow-restore-wait when setting
flow-restore-wait. When set, it tells that OVN once set flow-restore-wait.

"ovs-ctl restart" also uses flow-restore-wait: when called, it saves the
flows, stops "ovs-vswitchd", sets "flow-restore-wait" to true, restarts
"ovs-vswitchd", restores the flows and finally removes "flow-restore-wait".
So OVS will wait either for "ovs-ctl restart" to remove "flow-restore-wait"
or for OVN to set "flow-restore-wait" to false.

Reported-at: https://issues.redhat.com/browse/FDP-3075
Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Author: simonartxavier

controller/bfd.c

@@ -117,13 +117,14 @@ bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int,
  *
  * If 'our_chassis' is C5 then this function returns empty bfd set.
  */
-void
+bool
 bfd_calculate_chassis(
     const struct sbrec_chassis *our_chassis,
     const struct sbrec_ha_chassis_group_table *ha_chassis_grp_table,
     struct sset *bfd_chassis)
 {
     const struct sbrec_ha_chassis_group *ha_chassis_grp;
+    bool chassis_is_ha_gw = false;
     SBREC_HA_CHASSIS_GROUP_TABLE_FOR_EACH (ha_chassis_grp,
                                            ha_chassis_grp_table) {
         bool is_ha_chassis = false;
@@ -143,6 +144,7 @@ bfd_calculate_chassis(
             sset_add(&grp_chassis, ha_ch->chassis->name);
             if (our_chassis == ha_ch->chassis) {
                 is_ha_chassis = true;
+                chassis_is_ha_gw = true;
                 bfd_setup_required = true;
             }
         }
@@ -178,6 +180,7 @@ bfd_calculate_chassis(
         }
         sset_destroy(&grp_chassis);
     }
+    return chassis_is_ha_gw;
 }
 
 void

controller/bfd.h

@@ -16,6 +16,8 @@
 #ifndef OVN_BFD_H
 #define OVN_BFD_H 1
 
+#include <stdbool.h>
+
 struct hmap;
 struct ovsdb_idl;
 struct ovsdb_idl_index;
@@ -36,7 +38,7 @@ void bfd_run(const struct ovsrec_interface_table *,
              const struct sbrec_sb_global_table *,
              const struct ovsrec_open_vswitch_table *);
 
-void bfd_calculate_chassis(
+bool bfd_calculate_chassis(
     const struct sbrec_chassis *,
     const struct sbrec_ha_chassis_group_table *,
     struct sset *);

controller/ovn-controller.8.xml

@@ -531,6 +531,17 @@
         65535.
       </dd>
 
+      <dt>
+        <code>external_ids:ovn-managed-flow-restore-wait</code> in the
+        <code>Open_vSwitch</code> table
+      </dt>
+      <dd>
+        When set to true, this key indicates that <code>ovn-controller</code>
+        has set the <code>other_config:flow-restore-wait</code> option.
+        The key is set when <code>ovn-controller</code> enables
+        flow-restore-wait and removed when it clears it.
+      </dd>
+
       <dt>
         <code>external_ids:ct-zone-*</code> in the <code>Bridge</code> table
       </dt>

controller/ovn-controller.c

@@ -211,6 +211,152 @@ static char *get_file_system_id(void)
     free(filename);
     return ret;
 }
+
+/* Set/unset flow-restore-wait, and inc ovs next_cfg if false
+ * When set to true, also sets ovn-managed-flow-restore-wait to true to
+ * indicate ownership */
+static void
+set_flow_restore_wait(struct ovsdb_idl_txn *ovs_idl_txn,
+                      const struct ovsrec_open_vswitch *cfg,
+                      const struct smap *other_config,
+                      const bool val, bool ovn_managed)
+{
+    struct smap new_config;
+    smap_clone(&new_config, other_config);
+    smap_replace(&new_config, "flow-restore-wait", val ? "true": "false");
+    ovsrec_open_vswitch_set_other_config(cfg, &new_config);
+    if (val) {
+        ovsrec_open_vswitch_update_external_ids_setkey(
+            cfg, "ovn-managed-flow-restore-wait", "true");
+    } else if (ovn_managed) {
+        ovsrec_open_vswitch_update_external_ids_delkey(
+            cfg, "ovn-managed-flow-restore-wait");
+    }
+    ovsdb_idl_txn_increment(ovs_idl_txn, &cfg->header_,
+                            &ovsrec_open_vswitch_col_next_cfg, true);
+    smap_destroy(&new_config);
+}
+
+static void
+manage_flow_restore_wait(struct ovsdb_idl_txn *ovs_idl_txn,
+                         const struct ovsrec_open_vswitch *cfg,
+                         uint64_t ofctrl_cur_cfg, uint64_t ovs_next_cfg,
+                         int ovs_txn_status, bool is_ha_gw)
+{
+    enum flow_restore_wait_state {
+        FRW_INIT,              /* Initial state */
+        FRW_WAIT_TXN_COMPLETE, /* Sent false, waiting txn to complete */
+        FRW_TXN_SUCCESS,       /* Txn completed. Waiting for OVS Ack. */
+        FRW_DONE               /* Everything completed */
+    };
+
+    static int64_t frw_next_cfg;
+    static enum flow_restore_wait_state frw_state;
+    static bool ofctrl_was_connected = false;
+
+    bool ofctrl_connected = ofctrl_is_connected();
+
+    if (!ovs_idl_txn || !cfg) {
+        return;
+    }
+
+    /* If OVS is stopped/started, make sure flow-restore-wait is toggled. */
+    if (ofctrl_connected && !ofctrl_was_connected) {
+        frw_state = FRW_INIT;
+    }
+    ofctrl_was_connected = ofctrl_connected;
+
+    if (!ofctrl_connected) {
+        return;
+    }
+
+    bool frw = smap_get_bool(&cfg->other_config, "flow-restore-wait", false);
+    bool ovn_managed_once = smap_get_bool(&cfg->external_ids,
+                                          "ovn-managed-flow-restore-wait",
+                                          false);
+
+    if (frw && !ovn_managed_once) {
+        /* frw has been set by ovs-ctl. Do not touch. */
+        return;
+    }
+
+    if (!is_ha_gw) {
+        if (frw) {
+            /* frw has once been set by OVN. We are now not an HA chassis
+             * anymore, unset it. */
+            set_flow_restore_wait(ovs_idl_txn, cfg, &cfg->other_config,
+                                  false, ovn_managed_once);
+        }
+        /* else we are not an HA chassis and frw is false. Ignore it. */
+        return;
+    }
+
+    switch (frw_state) {
+    case FRW_INIT:
+        if (ofctrl_cur_cfg > 0) {
+            set_flow_restore_wait(ovs_idl_txn, cfg, &cfg->other_config,
+                                  false, ovn_managed_once);
+            frw_state = FRW_WAIT_TXN_COMPLETE;
+            VLOG_INFO("Setting flow-restore-wait=false "
+                      "(cur_cfg=%"PRIu64")", ofctrl_cur_cfg);
+        }
+        break;
+
+    case FRW_WAIT_TXN_COMPLETE:
+        /* if (ovs_idl_txn != NULL), the transaction completed.
+         * When the transaction completed, it either failed
+         * (ovs_txn_status == 0) or succeeded (ovs_txn_status != 0). */
+        if (ovs_txn_status == 0) {
+            /* Previous transaction failed. */
+            set_flow_restore_wait(ovs_idl_txn, cfg, &cfg->other_config,
+                                  false, ovn_managed_once);
+            break;
+        }
+        /* txn succeeded, get next_cfg */
+        frw_next_cfg = ovs_next_cfg;
+        frw_state = FRW_TXN_SUCCESS;
+        /* fall through */
+
+    case FRW_TXN_SUCCESS:
+        if (ovs_next_cfg < frw_next_cfg) {
+            /* DB was reset, next_cfg went backwards. */
+            VLOG_INFO("OVS DB reset (next_cfg %"PRId64" -> %"PRIu64"), "
+                      "resetting state",
+                      frw_next_cfg, ovs_next_cfg);
+            set_flow_restore_wait(ovs_idl_txn, cfg, &cfg->other_config,
+                                  false, ovn_managed_once);
+            frw_state = FRW_WAIT_TXN_COMPLETE;
+            break;
+        }
+
+        if (!frw) {
+            if (cfg->cur_cfg >= frw_next_cfg) {
+                set_flow_restore_wait(ovs_idl_txn, cfg, &cfg->other_config,
+                                      true, ovn_managed_once);
+                frw_state = FRW_DONE;
+                VLOG_INFO("Setting flow-restore-wait=true");
+            }
+        } else {
+            /* The transaction to false succeeded but frw is true.
+             * So, another task already set it to true. */
+            frw_state = FRW_DONE;
+            VLOG_INFO("flow-restore-wait was already true");
+        }
+        break;
+    case FRW_DONE:
+        if (!frw) {
+            /* frw has been removed (e.g. by ovs-ctl restart) or is false
+             * (e.g. txn failed.) */
+            set_flow_restore_wait(ovs_idl_txn, cfg, &cfg->other_config,
+                                  false, ovn_managed_once);
+            frw_state = FRW_WAIT_TXN_COMPLETE;
+            VLOG_INFO("OVS frw cleared, restarting flow-restore-wait sequence "
+                      "(cur_cfg=%"PRIu64")", ofctrl_cur_cfg);
+        }
+        break;
+    }
+}
+
 /* Only set monitor conditions on tables that are available in the
  * server schema.
  */
@@ -3381,6 +3527,7 @@ en_mac_cache_cleanup(void *data)
 
 struct ed_type_bfd_chassis {
     struct sset bfd_chassis;
+    bool is_ha_gw;
 };
 
 static void *
@@ -3409,8 +3556,9 @@ en_bfd_chassis_run(struct engine_node *node, void *data OVS_UNUSED)
         = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
 
     sset_clear(&bfd_chassis->bfd_chassis);
-    bfd_calculate_chassis(chassis, ha_chassis_grp_table,
-                          &bfd_chassis->bfd_chassis);
+    bfd_chassis->is_ha_gw = bfd_calculate_chassis(chassis,
+                                                  ha_chassis_grp_table,
+                                                  &bfd_chassis->bfd_chassis);
     return EN_UPDATED;
 }
 
@@ -7117,6 +7265,7 @@ main(int argc, char *argv[])
     struct unixctl_server *unixctl;
     struct ovn_exit_args exit_args = {0};
     struct br_int_remote br_int_remote = {0};
+    static uint64_t next_cfg = 0;
     int retval;
 
     /* Read from system-id-override file once on startup. */
@@ -7444,6 +7593,7 @@ main(int argc, char *argv[])
 
     /* Main loop. */
     int ovnsb_txn_status = 1;
+    int ovs_txn_status = 1;
     bool sb_monitor_all = false;
     struct tracked_acl_ids *tracked_acl_ids = NULL;
     while (!exit_args.exiting) {
@@ -7545,6 +7695,11 @@ main(int argc, char *argv[])
         pinctrl_update_swconn(br_int_remote.target,
                               br_int_remote.probe_interval);
 
+        if (cfg && ovs_idl_txn && ovs_txn_status == -1) {
+            /* txn was in progress and is now completed */
+            next_cfg = cfg->next_cfg;
+        }
+
         /* Enable ACL matching for double tagged traffic. */
         if (ovs_idl_txn && cfg) {
             int vlan_limit = smap_get_int(
@@ -7894,6 +8049,13 @@ main(int argc, char *argv[])
                     stopwatch_start(OFCTRL_SEQNO_RUN_STOPWATCH_NAME,
                                     time_msec());
                     ofctrl_seqno_run(ofctrl_get_cur_cfg());
+                    if (ovs_idl_txn && bfd_chassis_data) {
+                        manage_flow_restore_wait(ovs_idl_txn, cfg,
+                                                 ofctrl_get_cur_cfg(),
+                                                 next_cfg, ovs_txn_status,
+                                                 bfd_chassis_data->is_ha_gw);
+                    }
+
                     stopwatch_stop(OFCTRL_SEQNO_RUN_STOPWATCH_NAME,
                                    time_msec());
                     stopwatch_start(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
@@ -7993,7 +8155,7 @@ main(int argc, char *argv[])
             OVS_NOT_REACHED();
         }
 
-        int ovs_txn_status = ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
+        ovs_txn_status = ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
         if (!ovs_txn_status) {
             /* The transaction failed. */
             vif_plug_clear_deleted(
@@ -8012,6 +8174,9 @@ main(int argc, char *argv[])
                     &vif_plug_deleted_iface_ids);
             vif_plug_finish_changed(
                     &vif_plug_changed_iface_ids);
+            if (cfg) {
+                next_cfg = cfg->next_cfg;
+            }
         } else if (ovs_txn_status == -1) {
             /* The commit is still in progress */
         } else {
@@ -8085,7 +8250,7 @@ main(int argc, char *argv[])
             }
 
             ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
-            int ovs_txn_status = ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
+            ovs_txn_status = ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
             if (!ovs_txn_status) {
                 /* The transaction failed. */
                 vif_plug_clear_deleted(

tests/multinode-macros.at

@@ -41,6 +41,28 @@ m4_define([M_START_TCPDUMP],
     ]
 )
 
+m4_define([M_START_TCPDUMPS_RECURSIVE_], [
+     m4_if(m4_eval($# > 3), [1], [dnl
+        names="$names $3"
+        echo "Running podman exec $1 tcpdump -l $2 >$3.tcpdump 2>$3.stderr"
+        podman exec $1 tcpdump -l $2 >$3.tcpdump 2>$3.stderr &
+        echo "podman exec $1 ps -ef | grep -v grep | grep tcpdump && podman exec $1 killall tcpdump" >> cleanup
+        M_START_TCPDUMPS_RECURSIVE_(m4_shift(m4_shift(m4_shift($@))))
+        ])
+    ]
+)
+
+# Start Multiple tcpdump. Useful to speed up when many tcpdump
+# must be started as waiting for "listening" takes usually 1 second.
+m4_define([M_START_TCPDUMPS],
+    [
+     names=""
+     M_START_TCPDUMPS_RECURSIVE_($@)
+     for name in $names; do
+         OVS_WAIT_UNTIL([grep -q "listening" ${name}.stderr])
+     done
+    ]
+)
 
 # M_FORMAT_CT([ip-addr])
 #
@@ -480,6 +502,30 @@ m_is_fedora() {
     m_central_as grep -qi fedora /etc/os-release
 }
 
+# Run ovs-vsctl using Host socket
+host_ovs_vsctl() {
+    # Discover host OVS socket on first call
+    if [[ -z "$HOST_OVS_SOCK" ]]; then
+        for sock in /run/openvswitch/db.sock /var/run/openvswitch/db.sock /usr/local/var/run/openvswitch/db.sock; do
+            if [[ -S "$sock" ]]; then
+                HOST_OVS_SOCK=$sock
+                break
+            fi
+        done
+        # Fallback on unusual prefix: discover from running process
+        if [[ -z "$HOST_OVS_SOCK" ]]; then
+            HOST_OVS_SOCK=$(ps aux | grep '[o]vsdb-server' | grep -oP 'punix:\K[^, ]+' | while read s; do
+                [[ -S "$s" ]] && [[ "$s" != *"$OVS_RUNDIR"* ]] && echo "$s" && break
+            done)
+        fi
+        if [[ -z "$HOST_OVS_SOCK" ]]; then
+            echo "ERROR: Could not find host OVS socket" >&2
+            AT_FAIL_IF([:])
+        fi
+    fi
+    ovs-vsctl --db=unix:$HOST_OVS_SOCK "$@"
+}
+
 # M_START_L4_SERVER([fake_node], [namespace], [ip_addr], [port], [reply_string], [pidfile])
 #
 # Helper to properly start l4 server in inside 'fake_node''s namespace'.