Looks great! but you forgot the testing for the mult option. Rather simple anyway it's obvious it works, but.... it's my only nit :)
On Wed, Sep 26, 2018 at 10:30 AM Numan Siddique <nusid...@redhat.com> wrote: > > > On Tue, Sep 25, 2018 at 10:18 PM Miguel Angel Ajo Pelayo < > majop...@redhat.com> wrote: > >> Nak, >> >> Great work, this will be useful to adjust parameters based on specific >> network conditions, or while debugging network issues (to reduce flapping). >> >> I miss the "mult" parameter to setup the detection multiplier >> (rx_interval * mult). >> >> > > Thanks Anil and Miguel for the review. I sent out v2 adding mult option - > https://patchwork.ozlabs.org/patch/974893/ > > Numan > > > >> >> Thanks a lot, >> Miguel Ángel. >> >> On Tue, Sep 25, 2018 at 5:46 PM Anil Venkata <anilvenk...@redhat.com> >> wrote: >> >>> On Mon, Sep 24, 2018 at 3:12 PM <nusid...@redhat.com> wrote: >>> >>> > From: Numan Siddique <nusid...@redhat.com> >>> > >>> > With this commit the users can override the default BFD params - >>> > min_rx, min_tx and decay_min_rx if desired. This can be useful >>> > to debug any issues related to BFD, like BFD state changes are >>> > seen frequently. >>> > >>> > ovn-controller checks for the options 'ovn-bfd-min-rx', >>> 'ovn-bfd-min-tx' >>> > and 'ovn-bfd-decay-min-rx' in the external-ids of OpenvSwitch table row >>> > and configures these BFD values to the tunnel interfaces. >>> > >>> > Signed-off-by: Numan Siddique <nusid...@redhat.com> >>> > >>> Acked-by: Venkata Anil <vkomm...@redhat.com> >>> >>> > --- >>> > ovn/controller/bfd.c | 45 ++++++++++++++++++++--------- >>> > ovn/controller/bfd.h | 2 ++ >>> > ovn/controller/ovn-controller.8.xml | 9 ++++++ >>> > ovn/controller/ovn-controller.c | 10 ++++--- >>> > tests/ovn.at | 26 +++++++++++++++++ >>> > 5 files changed, 74 insertions(+), 18 deletions(-) >>> > >>> > diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c >>> > index 051781f38..455d7ff1f 100644 >>> > --- a/ovn/controller/bfd.c >>> > +++ b/ovn/controller/bfd.c >>> > @@ -40,20 +40,10 @@ bfd_register_ovs_idl(struct ovsdb_idl *ovs_idl) >>> > >>> > >>> > static void >>> > -interface_set_bfd(const struct ovsrec_interface *iface, bool >>> bfd_setting) >>> > +interface_set_bfd(const struct ovsrec_interface *iface, struct smap >>> *bfd) >>> > { >>> > - 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); >>> > + ovsrec_interface_set_bfd(iface, bfd); >>> > } >>> > >>> > void >>> > @@ -265,6 +255,7 @@ void >>> > bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name, >>> > struct ovsdb_idl_index *sbrec_port_binding_by_datapath, >>> > const struct ovsrec_interface_table *interface_table, >>> > + const struct ovsrec_open_vswitch_table *ovs_table, >>> > const struct ovsrec_bridge *br_int, >>> > const struct sbrec_chassis *chassis_rec, >>> > const struct hmap *local_datapaths) >>> > @@ -292,15 +283,41 @@ bfd_run(struct ovsdb_idl_index >>> > *sbrec_chassis_by_name, >>> > } >>> > } >>> > >>> > + const struct ovsrec_open_vswitch *cfg; >>> > + cfg = ovsrec_open_vswitch_table_first(ovs_table); >>> > + const char *min_rx = NULL; >>> > + const char *decay_min_rx = NULL; >>> > + const char *min_tx = NULL; >>> > + if (cfg) { >>> > + min_rx = smap_get(&cfg->external_ids, "ovn-bfd-min-rx"); >>> > + decay_min_rx = smap_get(&cfg->external_ids, >>> > "ovn-bfd-decay-min-rx"); >>> > + min_tx = smap_get(&cfg->external_ids, "ovn-bfd-min-tx"); >>> > + } >>> > + struct smap bfd = SMAP_INITIALIZER(&bfd); >>> > + 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); >>> > + } >>> > /* 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)); >>> > + bool bfd_setting = sset_contains(&bfd_ifaces, >>> iface->name); >>> > + smap_replace(&bfd, "enable", bfd_setting ? >>> "true":"false"); >>> > + if (!smap_equal(&iface->bfd, &bfd)) { >>> > + interface_set_bfd(iface, &bfd); >>> > + VLOG_INFO("%s BFD on interface %s", >>> > + bfd_setting ? "Enabled" : "Disabled", >>> > 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..9243bec69 100644 >>> > --- a/ovn/controller/bfd.h >>> > +++ b/ovn/controller/bfd.h >>> > @@ -21,6 +21,7 @@ struct ovsdb_idl; >>> > struct ovsdb_idl_index; >>> > struct ovsrec_bridge; >>> > struct ovsrec_interface_table; >>> > +struct ovsrec_open_vswitch_table; >>> > struct sbrec_chassis; >>> > struct sset; >>> > >>> > @@ -28,6 +29,7 @@ void bfd_register_ovs_idl(struct ovsdb_idl *); >>> > void bfd_run(struct ovsdb_idl_index *sbrec_chassis_by_name, >>> > struct ovsdb_idl_index *sbrec_port_binding_by_datapath, >>> > const struct ovsrec_interface_table *interface_table, >>> > + const struct ovsrec_open_vswitch_table *ovs_table, >>> > const struct ovsrec_bridge *br_int, >>> > const struct sbrec_chassis *chassis_rec, >>> > const struct hmap *local_datapaths); >>> > diff --git a/ovn/controller/ovn-controller.8.xml >>> > b/ovn/controller/ovn-controller.8.xml >>> > index 8035638b3..e536f92fa 100644 >>> > --- a/ovn/controller/ovn-controller.8.xml >>> > +++ b/ovn/controller/ovn-controller.8.xml >>> > @@ -159,6 +159,15 @@ >>> > specific to this particular chassis. An example would be: >>> > <code>cms_option1,cms_option2:foo</code>. >>> > </dd> >>> > + >>> > + <dt><code>external_ids:ovn-bfd-min-rx</code></dt> >>> > + <dt><code>external_ids:ovn-bfd-decay-min-rx</code></dt> >>> > + <dt><code>external_ids:ovn-bfd-min-tx</code></dt> >>> > + <dd> >>> > + These are the BFD parameters to use when >>> > <code>ovn-controller</code> >>> > + enables BFD on the tunnel ports. These can be used to >>> override the >>> > + default BFD parameters. >>> > + </dd> >>> > </dl> >>> > >>> > <p> >>> > diff --git a/ovn/controller/ovn-controller.c >>> > b/ovn/controller/ovn-controller.c >>> > index 85921a03a..bca94e521 100644 >>> > --- a/ovn/controller/ovn-controller.c >>> > +++ b/ovn/controller/ovn-controller.c >>> > @@ -765,10 +765,12 @@ main(int argc, char *argv[]) >>> > &flow_table, &group_table, >>> &meter_table); >>> > >>> > if (chassis_id) { >>> > - bfd_run(sbrec_chassis_by_name, >>> > - sbrec_port_binding_by_datapath, >>> > - >>> > ovsrec_interface_table_get(ovs_idl_loop.idl), >>> > - br_int, chassis, &local_datapaths); >>> > + bfd_run( >>> > + sbrec_chassis_by_name, >>> > + sbrec_port_binding_by_datapath, >>> > + >>> ovsrec_interface_table_get(ovs_idl_loop.idl), >>> > + >>> > ovsrec_open_vswitch_table_get(ovs_idl_loop.idl), >>> > + br_int, chassis, &local_datapaths); >>> > } >>> > physical_run( >>> > sbrec_chassis_by_name, >>> > diff --git a/tests/ovn.at b/tests/ovn.at >>> > index 769e09f81..5160d74c5 100644 >>> > --- a/tests/ovn.at >>> > +++ b/tests/ovn.at >>> > @@ -9271,6 +9271,32 @@ AT_CHECK([ovn-sbctl --columns chassis --bare >>> find >>> > Port_Binding logical_port=cr-o >>> > [0],[[1 >>> > ]]) >>> > >>> > +as gw2 >>> > +ovs-vsctl set Open . external-ids:ovn-bfd-min-rx=2000 >>> > +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 >>> > +ovs-vsctl set Open . external-ids:ovn-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 >>> > +ovs-vsctl remove Open . external-ids ovn-bfd-min-rx >>> > +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" >>> > +]) >>> > +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 >>> > >>> _______________________________________________ >>> 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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
