| Message ID | 1605196487-8429-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ovn-controller: Propagate nb_cfg to the local OVS DB. | expand |
On Thu, Nov 12, 2020 at 9:25 PM Dumitru Ceara <dceara@redhat.com> wrote: > > The NB.NB_Global.nb_cfg value gets propagated to > Chassis_Private.nb_cfg (and then to NB.NB_Global.hv_cfg) as soon as > ovn-controller has finished installing OVS flows corresponding to > the NB DB state. > > However, if the CMS runs monitoring applications on the chassis itself, > in order to detect that the NB changes have been applied, it has to > connect to the NB/SB database. In a scaled deployment this additional > connection might induce performance issues. > > In order to avoid that we now (also) propagate nb_cfg to the local OVS > DB, in table Open_vSwitch, as external_id "ovn-nb-cfg". > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Hi Dumitru, Thanks for the patch. Overall the patch LGTM. I have a couple of comments. 1. I think it needs to be documented that the ovn-controller writes 'ovn-nb-cfg' to the local ovsdb. 2. I think it's better if the ovn-controller writes the ovn-nb-cfg to the integration bridge's external_ids column. We already have Ihar's multiple ovn-controller support patch for review and it would conflict for this usecase. Thanks Numan > --- > controller/ovn-controller.c | 53 ++++++++++++++++++++++++++++++++++----------- > tests/ovn-controller.at | 21 ++++++++++++++++++ > 2 files changed, 61 insertions(+), 13 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index a06cae3..e05afc2 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -85,6 +85,8 @@ static unixctl_cb_func debug_delay_nb_cfg_report; > > #define CONTROLLER_LOOP_STOPWATCH_NAME "ovn-controller-flow-generation" > > +#define OVS_NB_CFG_NAME "ovn-nb-cfg" > + > static char *parse_options(int argc, char *argv[]); > OVS_NO_RETURN static void usage(void); > > @@ -733,6 +735,41 @@ get_nb_cfg(const struct sbrec_sb_global_table *sb_global_table) > return sb ? sb->nb_cfg : 0; > } > > +/* Propagates the local cfg seqno, 'cur_cfg', to the chassis_private record > + * and to the local OVS DB. > + */ > +static void > +store_nb_cfg(struct ovsdb_idl_txn *sb_txn, struct ovsdb_idl_txn *ovs_txn, > + const struct sbrec_chassis_private *chassis, > + const struct ovsrec_open_vswitch *ovs_cfg, > + unsigned int delay_nb_cfg_report, > + int64_t cur_cfg) > +{ > + if (!cur_cfg) { > + return; > + } > + > + if (sb_txn && chassis && cur_cfg != chassis->nb_cfg) { > + sbrec_chassis_private_set_nb_cfg(chassis, cur_cfg); > + sbrec_chassis_private_set_nb_cfg_timestamp(chassis, time_wall_msec()); > + > + if (delay_nb_cfg_report) { > + VLOG_INFO("Sleep for %u sec", delay_nb_cfg_report); > + xsleep(delay_nb_cfg_report); > + } > + } > + > + if (ovs_txn && ovs_cfg && > + cur_cfg != smap_get_ullong(&ovs_cfg->external_ids, > + OVS_NB_CFG_NAME, 0)) { > + char *cur_cfg_str = xasprintf("%"PRId64, cur_cfg); > + ovsrec_open_vswitch_update_external_ids_setkey(ovs_cfg, > + OVS_NB_CFG_NAME, > + cur_cfg_str); > + free(cur_cfg_str); > + } > +} > + > static const char * > get_transport_zones(const struct ovsrec_open_vswitch_table *ovs_table) > { > @@ -2632,19 +2669,9 @@ main(int argc, char *argv[]) > engine_set_force_recompute(false); > } > > - if (ovnsb_idl_txn && chassis_private) { > - int64_t cur_cfg = ofctrl_get_cur_cfg(); > - if (cur_cfg && cur_cfg != chassis_private->nb_cfg) { > - sbrec_chassis_private_set_nb_cfg(chassis_private, cur_cfg); > - sbrec_chassis_private_set_nb_cfg_timestamp( > - chassis_private, time_wall_msec()); > - if (delay_nb_cfg_report) { > - VLOG_INFO("Sleep for %u sec", delay_nb_cfg_report); > - xsleep(delay_nb_cfg_report); > - } > - } > - } > - > + store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private, > + ovsrec_open_vswitch_first(ovs_idl_loop.idl), > + delay_nb_cfg_report, ofctrl_get_cur_cfg()); > > if (pending_pkt.conn) { > struct ed_type_addr_sets *as_data = > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 014a977..f672bc9 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -411,3 +411,24 @@ AT_CHECK([ovn-nbctl --timeout=1 --wait=hv sync]) > > OVN_CLEANUP([hv]) > AT_CLEANUP > + > +AT_SETUP([ovn -- nb_cfg sync to OVS]) > +ovn_start > + > +net_add n1 > +sim_add hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +# Wait for ovn-controller to register in the SB. > +wait_row_count Chassis 1 > + > +# Increment nb_cfg. > +check ovn-nbctl --wait=hv sync > + > +# And check that it gets propagated to OVS external_ids. > +as hv1 > +OVS_WAIT_UNTIL([ovs-vsctl get Open_vSwitch . external_ids:ovn-nb-cfg], [0], [1]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 11/16/20 12:02 PM, Numan Siddique wrote: > On Thu, Nov 12, 2020 at 9:25 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >> The NB.NB_Global.nb_cfg value gets propagated to >> Chassis_Private.nb_cfg (and then to NB.NB_Global.hv_cfg) as soon as >> ovn-controller has finished installing OVS flows corresponding to >> the NB DB state. >> >> However, if the CMS runs monitoring applications on the chassis itself, >> in order to detect that the NB changes have been applied, it has to >> connect to the NB/SB database. In a scaled deployment this additional >> connection might induce performance issues. >> >> In order to avoid that we now (also) propagate nb_cfg to the local OVS >> DB, in table Open_vSwitch, as external_id "ovn-nb-cfg". >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Hi Dumitru, Hi Numan, > > Thanks for the patch. Overall the patch LGTM. I have a couple of comments. Thanks for the review. > > 1. I think it needs to be documented that the ovn-controller writes > 'ovn-nb-cfg' to the local ovsdb. > 2. I think it's better if the ovn-controller writes the ovn-nb-cfg to > the integration bridge's external_ids column. > We already have Ihar's multiple ovn-controller support patch for > review and it would conflict for this usecase. Ack, good points, I'll address them in v2. Thanks, Dumitru
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index a06cae3..e05afc2 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -85,6 +85,8 @@ static unixctl_cb_func debug_delay_nb_cfg_report; #define CONTROLLER_LOOP_STOPWATCH_NAME "ovn-controller-flow-generation" +#define OVS_NB_CFG_NAME "ovn-nb-cfg" + static char *parse_options(int argc, char *argv[]); OVS_NO_RETURN static void usage(void); @@ -733,6 +735,41 @@ get_nb_cfg(const struct sbrec_sb_global_table *sb_global_table) return sb ? sb->nb_cfg : 0; } +/* Propagates the local cfg seqno, 'cur_cfg', to the chassis_private record + * and to the local OVS DB. + */ +static void +store_nb_cfg(struct ovsdb_idl_txn *sb_txn, struct ovsdb_idl_txn *ovs_txn, + const struct sbrec_chassis_private *chassis, + const struct ovsrec_open_vswitch *ovs_cfg, + unsigned int delay_nb_cfg_report, + int64_t cur_cfg) +{ + if (!cur_cfg) { + return; + } + + if (sb_txn && chassis && cur_cfg != chassis->nb_cfg) { + sbrec_chassis_private_set_nb_cfg(chassis, cur_cfg); + sbrec_chassis_private_set_nb_cfg_timestamp(chassis, time_wall_msec()); + + if (delay_nb_cfg_report) { + VLOG_INFO("Sleep for %u sec", delay_nb_cfg_report); + xsleep(delay_nb_cfg_report); + } + } + + if (ovs_txn && ovs_cfg && + cur_cfg != smap_get_ullong(&ovs_cfg->external_ids, + OVS_NB_CFG_NAME, 0)) { + char *cur_cfg_str = xasprintf("%"PRId64, cur_cfg); + ovsrec_open_vswitch_update_external_ids_setkey(ovs_cfg, + OVS_NB_CFG_NAME, + cur_cfg_str); + free(cur_cfg_str); + } +} + static const char * get_transport_zones(const struct ovsrec_open_vswitch_table *ovs_table) { @@ -2632,19 +2669,9 @@ main(int argc, char *argv[]) engine_set_force_recompute(false); } - if (ovnsb_idl_txn && chassis_private) { - int64_t cur_cfg = ofctrl_get_cur_cfg(); - if (cur_cfg && cur_cfg != chassis_private->nb_cfg) { - sbrec_chassis_private_set_nb_cfg(chassis_private, cur_cfg); - sbrec_chassis_private_set_nb_cfg_timestamp( - chassis_private, time_wall_msec()); - if (delay_nb_cfg_report) { - VLOG_INFO("Sleep for %u sec", delay_nb_cfg_report); - xsleep(delay_nb_cfg_report); - } - } - } - + store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private, + ovsrec_open_vswitch_first(ovs_idl_loop.idl), + delay_nb_cfg_report, ofctrl_get_cur_cfg()); if (pending_pkt.conn) { struct ed_type_addr_sets *as_data = diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 014a977..f672bc9 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -411,3 +411,24 @@ AT_CHECK([ovn-nbctl --timeout=1 --wait=hv sync]) OVN_CLEANUP([hv]) AT_CLEANUP + +AT_SETUP([ovn -- nb_cfg sync to OVS]) +ovn_start + +net_add n1 +sim_add hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +# Wait for ovn-controller to register in the SB. +wait_row_count Chassis 1 + +# Increment nb_cfg. +check ovn-nbctl --wait=hv sync + +# And check that it gets propagated to OVS external_ids. +as hv1 +OVS_WAIT_UNTIL([ovs-vsctl get Open_vSwitch . external_ids:ovn-nb-cfg], [0], [1]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP
The NB.NB_Global.nb_cfg value gets propagated to Chassis_Private.nb_cfg (and then to NB.NB_Global.hv_cfg) as soon as ovn-controller has finished installing OVS flows corresponding to the NB DB state. However, if the CMS runs monitoring applications on the chassis itself, in order to detect that the NB changes have been applied, it has to connect to the NB/SB database. In a scaled deployment this additional connection might induce performance issues. In order to avoid that we now (also) propagate nb_cfg to the local OVS DB, in table Open_vSwitch, as external_id "ovn-nb-cfg". Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/ovn-controller.c | 53 ++++++++++++++++++++++++++++++++++----------- tests/ovn-controller.at | 21 ++++++++++++++++++ 2 files changed, 61 insertions(+), 13 deletions(-)