[ovs-dev,v7] ovn: Support configuring the BFD params for the tunnel interfaces

Message ID 20181010060855.5534-1-nusiddiq@redhat.com
State Accepted
Headers show
Series
  • [ovs-dev,v7] ovn: Support configuring the BFD params for the tunnel interfaces
Related show

Commit Message

Numan Siddique Oct. 10, 2018, 6:08 a.m.
From: Numan Siddique <nusiddiq@redhat.com>

With this commit the users can override the default values of
the BFD params - min_rx, min_tx, decay_min_rx and mult if desired.
This can be useful to debug any issues related to BFD (like
frequent BFD state changes).

A new column 'options' is added in NB_Global and SB_Global tables
of OVN_Northbound and OVN_Southbound schemas respectively. CMS
can define the options 'bfd-min-rx', 'bfd-min-tx',
'bfd-decay-min-rx' and 'bfd-mult' in the options column of
NB_Global table row. ovn-northd copies these options from
NB_Global to SB_Global. ovn-controller configures these
options to the tunnel interfaces when enabling BFD.

When BFD is disabled, this patch now clears the 'bfd' column
of the interface row, instead of setting 'enable=false'.

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

v6 -> v7
-------
  * Corrected  a small error in the commit message.

v5 -> v6
--------
  * Addressed the review comments from Ben
    - changed the config options from "bfd:min-rx" to "bfd-min-rx"
    - when disabling the bfd, instead of setting the option
      "enable_bfd=false", now clears the bfd column of the interface row

v4 -> v5
-------
  * Addressed a couple of check_patch util warnings - Line exceeding 80 char
    which was introduced in the v4.

v3 -> v4
--------
  * As per the suggestion from Ben - provided the option to
    centrally configuring the BFD params
    
v2 -> v3
--------
  * Added the test case for mult option

v1 -> v2
-------
  * Addressed review comments by Miguel - added the option 'mult' to configure.

 ovn/controller/bfd.c            | 66 +++++++++++++++++++++++----------
 ovn/controller/bfd.h            |  3 ++
 ovn/controller/ovn-controller.c |  4 +-
 ovn/northd/ovn-northd.c         |  2 +
 ovn/ovn-nb.ovsschema            |  9 +++--
 ovn/ovn-nb.xml                  | 35 +++++++++++++++++
 ovn/ovn-sb.ovsschema            |  9 +++--
 ovn/ovn-sb.xml                  | 38 +++++++++++++++++++
 tests/ovn.at                    | 31 +++++++++++++++-
 9 files changed, 168 insertions(+), 29 deletions(-)

Comments

Ben Pfaff Oct. 11, 2018, 9:12 p.m. | #1
On Wed, Oct 10, 2018 at 11:38:55AM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> With this commit the users can override the default values of
> the BFD params - min_rx, min_tx, decay_min_rx and mult if desired.
> This can be useful to debug any issues related to BFD (like
> frequent BFD state changes).
> 
> A new column 'options' is added in NB_Global and SB_Global tables
> of OVN_Northbound and OVN_Southbound schemas respectively. CMS
> can define the options 'bfd-min-rx', 'bfd-min-tx',
> 'bfd-decay-min-rx' and 'bfd-mult' in the options column of
> NB_Global table row. ovn-northd copies these options from
> NB_Global to SB_Global. ovn-controller configures these
> options to the tunnel interfaces when enabling BFD.
> 
> When BFD is disabled, this patch now clears the 'bfd' column
> of the interface row, instead of setting 'enable=false'.
> 
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

Thanks, applied to master.

Patch

diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c
index 051781f38..94dad236e 100644
--- a/ovn/controller/bfd.c
+++ b/ovn/controller/bfd.c
@@ -38,24 +38,6 @@  bfd_register_ovs_idl(struct ovsdb_idl *ovs_idl)
     ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_bfd_status);
 }
 
-
-static void
-interface_set_bfd(const struct ovsrec_interface *iface, bool bfd_setting)
-{
-    const char *new_setting = bfd_setting ? "true":"false";
-    const char *current_setting = smap_get(&iface->bfd, "enable");
-    if (current_setting && !strcmp(current_setting, new_setting)) {
-        /* If already set to the desired setting we skip setting it again
-         * to avoid flapping to bfd initialization state */
-        return;
-    }
-    const struct smap bfd = SMAP_CONST1(&bfd, "enable", new_setting);
-    ovsrec_interface_verify_bfd(iface);
-    ovsrec_interface_set_bfd(iface, &bfd);
-    VLOG_INFO("%s BFD on interface %s", bfd_setting ? "Enabled" : "Disabled",
-                                        iface->name);
-}
-
 void
 bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int,
                              struct sset *active_tunnels)
@@ -267,6 +249,7 @@  bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name,
         const struct ovsrec_interface_table *interface_table,
         const struct ovsrec_bridge *br_int,
         const struct sbrec_chassis *chassis_rec,
+        const struct sbrec_sb_global_table *sb_global_table,
         const struct hmap *local_datapaths)
 {
 
@@ -292,15 +275,58 @@  bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name,
         }
     }
 
+    const struct sbrec_sb_global *sb
+        = sbrec_sb_global_table_first(sb_global_table);
+    struct smap bfd = SMAP_INITIALIZER(&bfd);
+    smap_add(&bfd, "enable", "true");
+
+    if (sb) {
+        const char *min_rx = smap_get(&sb->options, "bfd-min-rx");
+        const char *decay_min_rx = smap_get(&sb->options, "bfd-decay-min-rx");
+        const char *min_tx = smap_get(&sb->options, "bfd-min-tx");
+        const char *mult = smap_get(&sb->options, "bfd-mult");
+        if (min_rx) {
+            smap_add(&bfd, "min_rx", min_rx);
+        }
+        if (decay_min_rx) {
+            smap_add(&bfd, "decay_min_rx", decay_min_rx);
+        }
+        if (min_tx) {
+            smap_add(&bfd, "min_tx", min_tx);
+        }
+        if (mult) {
+            smap_add(&bfd, "mult", mult);
+        }
+    }
+
     /* Enable or disable bfd */
     const struct ovsrec_interface *iface;
     OVSREC_INTERFACE_TABLE_FOR_EACH (iface, interface_table) {
         if (sset_contains(&tunnels, iface->name)) {
-                interface_set_bfd(
-                        iface, sset_contains(&bfd_ifaces, iface->name));
+            if (sset_contains(&bfd_ifaces, iface->name)) {
+                /* We need to enable BFD for this interface. Configure the
+                 * BFD params if
+                 *  - If BFD was disabled earlier
+                 *  - Or if CMS has updated BFD config options.
+                 */
+                if (!smap_equal(&iface->bfd, &bfd)) {
+                    ovsrec_interface_verify_bfd(iface);
+                    ovsrec_interface_set_bfd(iface, &bfd);
+                    VLOG_INFO("Enabled BFD on interface %s", iface->name);
+                }
+            } else {
+                /* We need to disable BFD for this interface if it was enabled
+                 * earlier. */
+                if (smap_count(&iface->bfd)) {
+                    ovsrec_interface_verify_bfd(iface);
+                    ovsrec_interface_set_bfd(iface, NULL);
+                    VLOG_INFO("Disabled BFD on interface %s", iface->name);
+                }
+            }
          }
     }
 
+    smap_destroy(&bfd);
     sset_destroy(&tunnels);
     sset_destroy(&bfd_ifaces);
     sset_destroy(&bfd_chassis);
diff --git a/ovn/controller/bfd.h b/ovn/controller/bfd.h
index 7ea345a3e..e36820afb 100644
--- a/ovn/controller/bfd.h
+++ b/ovn/controller/bfd.h
@@ -21,7 +21,9 @@  struct ovsdb_idl;
 struct ovsdb_idl_index;
 struct ovsrec_bridge;
 struct ovsrec_interface_table;
+struct ovsrec_open_vswitch_table;
 struct sbrec_chassis;
+struct sbrec_sb_global_table;
 struct sset;
 
 void bfd_register_ovs_idl(struct ovsdb_idl *);
@@ -30,6 +32,7 @@  void bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name,
              const struct ovsrec_interface_table *interface_table,
              const struct ovsrec_bridge *br_int,
              const struct sbrec_chassis *chassis_rec,
+             const struct sbrec_sb_global_table *sb_global_table,
              const struct hmap *local_datapaths);
 void  bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int,
                                    struct sset *active_tunnels);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index f46156021..2b2779a17 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -756,7 +756,9 @@  main(int argc, char *argv[])
                                 sbrec_chassis_by_name,
                                 sbrec_port_binding_by_datapath,
                                 ovsrec_interface_table_get(ovs_idl_loop.idl),
-                                br_int, chassis, &local_datapaths);
+                                br_int, chassis,
+                                sbrec_sb_global_table_get(ovnsb_idl_loop.idl),
+                                &local_datapaths);
                         }
                         physical_run(
                             sbrec_chassis_by_name,
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 31ea5f410..439651f80 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -7138,6 +7138,7 @@  ovnnb_db_run(struct northd_context *ctx,
         sb = sbrec_sb_global_insert(ctx->ovnsb_txn);
     }
     sbrec_sb_global_set_nb_cfg(sb, nb->nb_cfg);
+    sbrec_sb_global_set_options(sb, &nb->options);
     sb_loop->next_cfg = nb->nb_cfg;
 
     cleanup_macam(&macam);
@@ -7641,6 +7642,7 @@  main(int argc, char *argv[])
 
     ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_sb_global);
     add_column_noalert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_nb_cfg);
+    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_options);
 
     ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_logical_flow);
     add_column_noalert(ovnsb_idl_loop.idl,
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 3e7164baa..705cc2705 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Northbound",
-    "version": "5.13.0",
-    "cksum": "1278623084 20312",
+    "version": "5.13.1",
+    "cksum": "749176366 20467",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -19,7 +19,10 @@ 
                 "ssl": {
                     "type": {"key": {"type": "uuid",
                                      "refTable": "SSL"},
-                                     "min": 0, "max": 1}}},
+                                     "min": 0, "max": 1}},
+                "options": {
+                    "type": {"key": "string", "value": "string",
+                             "min": 0, "max": "unlimited"}}},
             "maxRows": 1,
             "isRoot": true},
         "Logical_Switch": {
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 8564ed39c..c0739fe57 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -69,6 +69,41 @@ 
         See <em>External IDs</em> at the beginning of this document.
       </column>
     </group>
+
+    <group title="Common options">
+      <column name="options">
+        This column provides general key/value settings. The supported
+        options are described individually below.
+      </column>
+
+      <group title="Options for configuring BFD">
+        <p>
+          These options apply when <code>ovn-controller</code> configures
+          BFD on tunnels interfaces.
+        </p>
+
+        <column name="options" key="bfd-min-rx">
+          BFD option <code>min-rx</code> value to use when configuring BFD on
+          tunnel interfaces.
+        </column>
+
+        <column name="options" key="bfd-decay-min-rx">
+          BFD option <code>decay-min-rx</code> value to use when configuring
+          BFD on tunnel interfaces.
+        </column>
+
+        <column name="options" key="bfd-min-tx">
+          BFD option <code>min-tx</code> value to use when configuring BFD on
+          tunnel interfaces.
+        </column>
+
+        <column name="options" key="bfd-mult">
+          BFD option <code>mult</code> value to use when configuring BFD on
+          tunnel interfaces.
+        </column>
+      </group>
+    </group>
+
     <group title="Connection Options">
       <column name="connections">
         Database clients to which the Open vSwitch database server should
diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
index ad6ad3b71..33ccd95a8 100644
--- a/ovn/ovn-sb.ovsschema
+++ b/ovn/ovn-sb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Southbound",
-    "version": "1.16.0",
-    "cksum": "3046632234 14844",
+    "version": "1.16.1",
+    "cksum": "2148542239 14999",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -17,7 +17,10 @@ 
                 "ssl": {
                     "type": {"key": {"type": "uuid",
                                      "refTable": "SSL"},
-                                     "min": 0, "max": 1}}},
+                                     "min": 0, "max": 1}},
+                "options": {
+                    "type": {"key": "string", "value": "string",
+                             "min": 0, "max": "unlimited"}}},
             "maxRows": 1,
             "isRoot": true},
         "Chassis": {
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 68e31db31..a4922bede 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -162,7 +162,45 @@ 
       <column name="external_ids">
         See <em>External IDs</em> at the beginning of this document.
       </column>
+
+      <column name="options">
+      </column>
+    </group>
+
+    <group title="Common options">
+      <column name="options">
+        This column provides general key/value settings. The supported
+        options are described individually below.
+      </column>
+
+      <group title="Options for configuring BFD">
+        <p>
+          These options apply when <code>ovn-controller</code> configures
+          BFD on tunnels interfaces.
+        </p>
+
+        <column name="options" key="bfd-min-rx">
+          BFD option <code>min-rx</code> value to use when configuring BFD on
+          tunnel interfaces.
+        </column>
+
+        <column name="options" key="bfd-decay-min-rx">
+          BFD option <code>decay-min-rx</code> value to use when configuring
+          BFD on tunnel interfaces.
+        </column>
+
+        <column name="options" key="bfd-min-tx">
+          BFD option <code>min-tx</code> value to use when configuring BFD on
+          tunnel interfaces.
+        </column>
+
+        <column name="options" key="bfd-mult">
+          BFD option <code>mult</code> value to use when configuring BFD on
+          tunnel interfaces.
+        </column>
+      </group>
     </group>
+
     <group title="Connection Options">
       <column name="connections">
         Database clients to which the Open vSwitch database server should
diff --git a/tests/ovn.at b/tests/ovn.at
index 44475175d..6e322602a 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9226,7 +9226,7 @@  for chassis in gw1 gw2; do
 done
 # make sure BFD is not enabled to hv2, we don't need it
 AT_CHECK([ovs-vsctl --bare --columns bfd find Interface name=ovn-hv2-0],[0],
-         [[enable=false
+         [[
 ]])
 
 
@@ -9240,7 +9240,7 @@  for chassis in gw1 gw2; do
 done
 # make sure BFD is not enabled to hv1, we don't need it
 AT_CHECK([ovs-vsctl --bare --columns bfd find Interface name=ovn-hv1-0],[0],
-         [[enable=false
+         [[
 ]])
 
 sleep 3  # let BFD sessions settle so we get the right flows on the right chassis
@@ -9270,6 +9270,33 @@  AT_CHECK([ovn-sbctl --columns chassis --bare find Port_Binding logical_port=cr-o
          [0],[[1
 ]])
 
+ovn-nbctl --wait=hv set NB_Global . options:"bfd-min-rx"=2000
+as gw2
+for chassis in gw1 hv1 hv2; do
+    echo "checking gw2 -> $chassis"
+    OVS_WAIT_UNTIL([
+    bfd_cfg=$(ovs-vsctl --bare --columns bfd find Interface name=ovn-$chassis-0)
+    test "$bfd_cfg" = "enable=true min_rx=2000"
+])
+done
+ovn-nbctl --wait=hv set NB_Global . options:"bfd-min-tx"=1500
+for chassis in gw1 hv1 hv2; do
+    echo "checking gw2 -> $chassis"
+    OVS_WAIT_UNTIL([
+    bfd_cfg=$(ovs-vsctl --bare --columns bfd find Interface name=ovn-$chassis-0)
+    test "$bfd_cfg" = "enable=true min_rx=2000 min_tx=1500"
+])
+done
+ovn-nbctl remove NB_Global . options "bfd-min-rx"
+ovn-nbctl --wait=hv set NB_Global . options:"bfd-mult"=5
+for chassis in gw1 hv1 hv2; do
+    echo "checking gw2 -> $chassis"
+    OVS_WAIT_UNTIL([
+    bfd_cfg=$(ovs-vsctl --bare --columns bfd find Interface name=ovn-$chassis-0)
+    test "$bfd_cfg" = "enable=true min_tx=1500 mult=5"
+])
+done
+
 OVN_CLEANUP([gw1],[gw2],[hv1],[hv2])
 
 AT_CLEANUP