On Fri, May 12, 2017 at 12:07 PM, Andy Zhou <[email protected]> wrote: > On Thu, May 11, 2017 at 8:35 PM, Greg Rose <[email protected]> wrote: >> On Thu, 2017-05-11 at 16:16 -0700, Andy Zhou wrote: >>> When multiple bridges connects to the same controller, the controller >>> status should be maintained separately for each bridge. Current >>> logic pushes status updates only based on the connection string, >>> which is the same across multiple bridges when connecting to the >>> same controller. >>> >>> Report-at: >>> https://mail.openvswitch.org/pipermail/ovs-discuss/2017-May/044412.html >>> Reported-by: Tulio Ribeiro <[email protected]> >>> Signed-off-by: Andy Zhou <[email protected]> >>> --- >>> AUTHORS.rst | 1 + >>> tests/bridge.at | 33 +++++++++++++++++++++++++++++++++ >>> vswitchd/bridge.c | 33 +++++++++++++++------------------ >>> 3 files changed, 49 insertions(+), 18 deletions(-) >>> >>> diff --git a/AUTHORS.rst b/AUTHORS.rst >>> index 59639756b09f..147ce48d15ca 100644 >>> --- a/AUTHORS.rst >>> +++ b/AUTHORS.rst >>> @@ -530,6 +530,7 @@ Teemu Koponen [email protected] >>> Thomas Morin [email protected] >>> Timothy Chen [email protected] >>> Torbjorn Tornkvist [email protected] >>> +Tulio Ribeiro [email protected] >>> Tytus Kurek [email protected] >>> Valentin Bud [email protected] >>> Vasiliy Tolstov [email protected] >>> diff --git a/tests/bridge.at b/tests/bridge.at >>> index 3dbabe511780..58b27d445062 100644 >>> --- a/tests/bridge.at >>> +++ b/tests/bridge.at >>> @@ -38,3 +38,36 @@ dummy@ovs-dummy: hit:0 missed:0 >>> OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) >>> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >>> AT_CLEANUP >>> + >>> +dnl When multiple bridges are connected to the same controller, make >>> +dnl sure their status are tracked independently. >>> +AT_SETUP([bridge - multiple bridges share a controller]) >>> +OVS_VSWITCHD_START( >>> + [add-br br1 -- \ >>> + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ >>> + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 ]) >>> + >>> +dnl Start ovs-testcontroller >>> +AT_CHECK([ovs-testcontroller --detach punix:controller], [0], [ignore]) >>> + >>> +dnl Add the controller to both bridges, 5 seconds apart. >>> +AT_CHECK([ovs-vsctl set-controller br0 unix:controller]) >>> +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) >>> +AT_CHECK([ovs-vsctl set-controller br1 unix:controller]) >>> + >>> +dnl Wait for the controller connection to come up >>> +for i in `seq 0 7` >>> +do >>> + AT_CHECK([ovs-appctl time/warp 10], [0], [ignore]) >>> +done >>> + >>> +dnl Make sure the connection status are different >>> +AT_CHECK([ovs-vsctl --columns=status list controller | sort], [0], [dnl >>> + >>> +status : {sec_since_connect="0", state=ACTIVE} >>> +status : {sec_since_connect="5", state=ACTIVE} >>> +]) >>> + >>> +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) >>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >>> +AT_CLEANUP >>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >>> index 31203d1ec232..972146e8da6d 100644 >>> --- a/vswitchd/bridge.c >>> +++ b/vswitchd/bridge.c >>> @@ -2704,34 +2704,31 @@ static void >>> refresh_controller_status(void) >>> { >>> struct bridge *br; >>> - struct shash info; >>> - const struct ovsrec_controller *cfg; >>> - >>> - shash_init(&info); >>> >>> /* Accumulate status for controllers on all bridges. */ >>> HMAP_FOR_EACH (br, node, &all_bridges) { >>> + struct shash info = SHASH_INITIALIZER(&info); >>> ofproto_get_ofproto_controller_info(br->ofproto, &info); >>> - } >>> >>> - /* Update each controller in the database with current status. */ >>> - OVSREC_CONTROLLER_FOR_EACH(cfg, idl) { >>> - struct ofproto_controller_info *cinfo = >>> - shash_find_data(&info, cfg->target); >>> + /* Update each controller of the bridge in the database with >>> + * current status. */ >>> + struct ovsrec_controller **controllers; >>> + size_t n_controllers = bridge_get_controllers(br, &controllers); >>> + size_t i; >>> + for (i = 0; i < n_controllers; i++) { >>> + struct ovsrec_controller *cfg = controllers[i]; >>> + struct ofproto_controller_info *cinfo = >>> + shash_find_data(&info, cfg->target); >>> >>> - if (cinfo) { >>> + ovs_assert(cinfo); >>> ovsrec_controller_set_is_connected(cfg, cinfo->is_connected); >>> - ovsrec_controller_set_role(cfg, ofp12_controller_role_to_str( >>> - cinfo->role)); >>> + const char *role = ofp12_controller_role_to_str(cinfo->role); >>> + ovsrec_controller_set_role(cfg, role); >>> ovsrec_controller_set_status(cfg, &cinfo->pairs); >>> - } else { >>> - ovsrec_controller_set_is_connected(cfg, false); >>> - ovsrec_controller_set_role(cfg, NULL); >>> - ovsrec_controller_set_status(cfg, NULL); >>> } >>> - } >>> >>> - ofproto_free_ofproto_controller_info(&info); >>> + ofproto_free_ofproto_controller_info(&info); >>> + } >>> } >>> >>> /* Update interface and mirror statistics if necessary. */ >> >> I can't that I'm super familiar with this code but this LGTM. >> >> Reviewed-by: Greg Rose <gvrose@[email protected]> > > Thanks for the review. I will push it if I don't hear from Tulio in > the next few days. >>
Pushed to master. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
