[ovs-dev] [PATCH] bridge: Fix controller status update
Greg Rose
gvrose8192 at gmail.com
Fri May 12 03:35:01 UTC 2017
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 at lasige.di.fc.ul.pt>
> Signed-off-by: Andy Zhou <azhou at 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 at nicira.com
> Thomas Morin thomas.morin at orange.com
> Timothy Chen tchen at nicira.com
> Torbjorn Tornkvist kruskakli at gmail.com
> +Tulio Ribeiro tribeiro at lasige.di.fc.ul.pt
> Tytus Kurek Tytus.Kurek at pega.com
> Valentin Bud valentin at hackaserver.com
> Vasiliy Tolstov v.tolstov at 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 at 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 at 8192@gmail.com>
More information about the dev
mailing list