[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