Oops thank you I don’t know how I looked exactly... :) On Fri, 1 Feb 2019 at 18:14, Numan Siddique <nusid...@redhat.com> wrote:
> > > On Fri, Feb 1, 2019, 7:55 PM Miguel Angel Ajo Pelayo <majop...@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 < >> majop...@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 <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 >>> >> >> >> -- >> Miguel Ángel Ajo >> OSP / Networking DFG, OVN Squad Engineering >> > -- Miguel Ángel Ajo OSP / Networking DFG, OVN Squad Engineering _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev