On Thu, May 11, 2017 at 8:35 PM, Greg Rose <gvrose8...@gmail.com> 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 <tribe...@lasige.di.fc.ul.pt> >> Signed-off-by: Andy Zhou <az...@ovn.org> >> --- >> 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 kopo...@nicira.com >> Thomas Morin thomas.mo...@orange.com >> Timothy Chen tc...@nicira.com >> Torbjorn Tornkvist kruska...@gmail.com >> +Tulio Ribeiro tribe...@lasige.di.fc.ul.pt >> Tytus Kurek tytus.ku...@pega.com >> Valentin Bud valen...@hackaserver.com >> Vasiliy Tolstov v.tols...@selfip.ru >> 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@8...@gmail.com>
Thanks for the review. I will push it if I don't hear from Tulio in the next few days. > > > _______________________________________________ > 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