| Message ID | 1494544577-4705-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
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 <tribeiro@lasige.di.fc.ul.pt> > Signed-off-by: Andy Zhou <azhou@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 koponen@nicira.com > Thomas Morin thomas.morin@orange.com > Timothy Chen tchen@nicira.com > Torbjorn Tornkvist kruskakli@gmail.com > +Tulio Ribeiro tribeiro@lasige.di.fc.ul.pt > Tytus Kurek Tytus.Kurek@pega.com > Valentin Bud valentin@hackaserver.com > Vasiliy Tolstov v.tolstov@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@8192@gmail.com>
On Thu, May 11, 2017 at 8:35 PM, Greg Rose <gvrose8192@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 <tribeiro@lasige.di.fc.ul.pt> >> Signed-off-by: Andy Zhou <azhou@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 koponen@nicira.com >> Thomas Morin thomas.morin@orange.com >> Timothy Chen tchen@nicira.com >> Torbjorn Tornkvist kruskakli@gmail.com >> +Tulio Ribeiro tribeiro@lasige.di.fc.ul.pt >> Tytus Kurek Tytus.Kurek@pega.com >> Valentin Bud valentin@hackaserver.com >> Vasiliy Tolstov v.tolstov@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@8192@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, May 12, 2017 at 12:07 PM, Andy Zhou <azhou@ovn.org> wrote: > On Thu, May 11, 2017 at 8:35 PM, Greg Rose <gvrose8192@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 <tribeiro@lasige.di.fc.ul.pt> >>> Signed-off-by: Andy Zhou <azhou@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 koponen@nicira.com >>> Thomas Morin thomas.morin@orange.com >>> Timothy Chen tchen@nicira.com >>> Torbjorn Tornkvist kruskakli@gmail.com >>> +Tulio Ribeiro tribeiro@lasige.di.fc.ul.pt >>> Tytus Kurek Tytus.Kurek@pega.com >>> Valentin Bud valentin@hackaserver.com >>> Vasiliy Tolstov v.tolstov@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@8192@gmail.com> > > Thanks for the review. I will push it if I don't hear from Tulio in > the next few days. >> Pushed to master.
diff --git a/AUTHORS.rst b/AUTHORS.rst index 59639756b09f..147ce48d15ca 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -530,6 +530,7 @@ Teemu Koponen koponen@nicira.com Thomas Morin thomas.morin@orange.com Timothy Chen tchen@nicira.com Torbjorn Tornkvist kruskakli@gmail.com +Tulio Ribeiro tribeiro@lasige.di.fc.ul.pt Tytus Kurek Tytus.Kurek@pega.com Valentin Bud valentin@hackaserver.com Vasiliy Tolstov v.tolstov@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. */
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 <tribeiro@lasige.di.fc.ul.pt> Signed-off-by: Andy Zhou <azhou@ovn.org> --- AUTHORS.rst | 1 + tests/bridge.at | 33 +++++++++++++++++++++++++++++++++ vswitchd/bridge.c | 33 +++++++++++++++------------------ 3 files changed, 49 insertions(+), 18 deletions(-)