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 <nusid...@redhat.com> wrote:
> On Tue, Oct 9, 2018 at 2:18 PM <nusid...@redhat.com> wrote:
>
> > From: Numan Siddique <nusid...@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 <nusid...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
--
Miguel Ángel Ajo
OSP / Networking DFG, OVN Squad Engineering
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
