On Fri, Feb 1, 2019, 7:55 PM Miguel Angel Ajo Pelayo <[email protected]> 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 < > [email protected]> 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 <[email protected]> >> wrote: >> >>> On Tue, Oct 9, 2018 at 2:18 PM <[email protected]> wrote: >>> >>> > From: Numan Siddique <[email protected]> >>> > >>> > 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 <[email protected]> >>> > --- >>> > >>> > 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 >>> [email protected] >>> 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
