[ovs-dev] [PATCH v6] ovn: Support configuring the BFD params for the tunnel interfaces

Miguel Angel Ajo Pelayo majopela at redhat.com
Fri Feb 1 14:25:36 UTC 2019


Hmm, numan I see an older version of your patch on patchworks [1], but not
v6
May be there was an issue with mail?

[1] https://patchwork.ozlabs.org/patch/973888/

On Fri, Feb 1, 2019 at 3:17 PM Miguel Angel Ajo Pelayo <majopela at redhat.com>
wrote:

> Hey,
>
>    Did we get this merged in the end?,
>
>    We're having customers facing issues related to this and we may
> need to throttle the BFD settings.y
>
> On Tue, Oct 9, 2018 at 10:54 AM Numan Siddique <nusiddiq at redhat.com>
> wrote:
>
>> On Tue, Oct 9, 2018 at 2:18 PM <nusiddiq at redhat.com> wrote:
>>
>> > From: Numan Siddique <nusiddiq at 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_bfd=false'.
>> >
>>
>> If this patch is fine, can you please update from 'enable_bfd=false' to
>> 'enable=false' in the last line of the commit message before applying.
>>
>> Thanks
>> Numan
>>
>>
>> >
>> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
>> > ---
>> >
>> > 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(-)
>> >
>> > 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
>> > --
>> > 2.17.1
>> >
>> >
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>
> --
> Miguel Ángel Ajo
> OSP / Networking DFG, OVN Squad Engineering
>


-- 
Miguel Ángel Ajo
OSP / Networking DFG, OVN Squad Engineering


More information about the dev mailing list