[ovs-dev] [PATCH] bridge: Fix controller status update

[Andy Zhou]

On Thu, May 11, 2017 at 8:35 PM, Greg Rose <gvrose8192 at 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 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>

Thanks for the review. I will push it if I don't hear from Tulio in
the next few days.
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev