[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 20:36:16 UTC 2019
Oops thank you I don’t know how I looked exactly... :)
On Fri, 1 Feb 2019 at 18:14, Numan Siddique <nusiddiq at redhat.com> wrote:
>
>
> On Fri, Feb 1, 2019, 7:55 PM Miguel Angel Ajo Pelayo <majopela at redhat.com>
> wrote:
>
>> 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/
>>
>
> Hi Miguel,
> The patch is merged long back :) -
> https://github.com/openvswitch/ovs/commit/2d661a2733010d7e94560c624edbe7f192952b3d
>
> Thanks
> Numan
>
>
>
>> 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
>>
> --
Miguel Ángel Ajo
OSP / Networking DFG, OVN Squad Engineering
More information about the dev
mailing list