[ovs-dev] [PATCH v3 ovn] controller: add datapath meter capability check

Mark Gray mark.d.gray at redhat.com
Thu Sep 16 14:00:15 UTC 2021


On 16/09/2021 14:46, Lorenzo Bianconi wrote:
>> On 16/09/2021 04:42, Mark Michelson wrote:
>>> Hi Lorenzo, I have some comments down below.
>>>
>>
>> Thanks for your changes so far Lorenzo.
> 
> Hi Mark,
> 
> thx for the review
> 
>>
>>> On 9/9/21 7:27 PM, Lorenzo Bianconi wrote:
>>>> Dump datapath meter capabilities before configuring meters in
>>>> ovn-controller
>>>>
>>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
>>>> ---
>>>> Changes since v2:
>>>> - move meter capability logic in lib/features.c
>>>>
>>>> Changes since v1:
>>>> - move rconn in ovn-controller to avoid concurrency issues
>>>> ---
>>>>   controller/ovn-controller.c |  4 ++
>>>>   include/ovn/features.h      |  7 +++
>>>>   lib/actions.c               |  3 ++
>>>>   lib/automake.mk             |  1 +
>>>>   lib/features.c              | 89 +++++++++++++++++++++++++++++++++++++
>>>>   tests/ovn.at                |  4 +-
>>>>   6 files changed, 106 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>>>> index 0031a1035..8c6d4393b 100644
>>>> --- a/controller/ovn-controller.c
>>>> +++ b/controller/ovn-controller.c
>>>> @@ -3523,6 +3523,9 @@ main(int argc, char *argv[])
>>>>                          ovsrec_server_has_datapath_table(ovs_idl_loop.idl)
>>>>                          ? &br_int_dp
>>>>                          : NULL);
>>>> +        if (br_int) {
>>>> +            ovs_feature_support_init(br_int);
>>>> +        }
>>>>   
>>>>           /* Enable ACL matching for double tagged traffic. */
>>>>           if (ovs_idl_txn) {
>>>> @@ -3898,6 +3901,7 @@ loop_done:
>>>>       ovsdb_idl_loop_destroy(&ovs_idl_loop);
>>>>       ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
>>>>   
>>>> +    ovs_feature_support_deinit();
>>>>       free(ovs_remote);
>>>>       service_stop();
>>>>   
>>>> diff --git a/include/ovn/features.h b/include/ovn/features.h
>>>> index c35d59b14..4a6d45d88 100644
>>>> --- a/include/ovn/features.h
>>>> +++ b/include/ovn/features.h
>>>> @@ -23,17 +23,24 @@
>>>>   /* ovn-controller supported feature names. */
>>>>   #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif"
>>>>   
>>>> +struct ovsrec_bridge;
>>>> +struct rconn;
>>>> +
>>>
>>> The forward declaration of rconn is unnecessary here.
>>>
>>>>   /* OVS datapath supported features.  Based on availability OVN might generate
>>>>    * different types of openflows.
>>>>    */
>>>>   enum ovs_feature_support_bits {
>>>>       OVS_CT_ZERO_SNAT_SUPPORT_BIT,
>>>> +    OVS_DP_METER_SUPPORT_BIT,
>>>>   };
>>>>   
>>>>   enum ovs_feature_value {
>>>>       OVS_CT_ZERO_SNAT_SUPPORT = (1 << OVS_CT_ZERO_SNAT_SUPPORT_BIT),
>>>> +    OVS_DP_METER_SUPPORT = (1 << OVS_DP_METER_SUPPORT_BIT),
>>>>   };
>>>>   
>>>> +void ovs_feature_support_init(const struct ovsrec_bridge *br_int);
>>>> +void ovs_feature_support_deinit(void);
>>>>   bool ovs_feature_is_supported(enum ovs_feature_value feature);
>>>>   bool ovs_feature_support_update(const struct smap *ovs_capabilities);
>>>>   
>>>> diff --git a/lib/actions.c b/lib/actions.c
>>>> index c572e88ae..7cf6be308 100644
>>>> --- a/lib/actions.c
>>>> +++ b/lib/actions.c
>>>> @@ -87,6 +87,9 @@ encode_start_controller_op(enum action_opcode opcode, bool pause,
>>>>       oc->max_len = UINT16_MAX;
>>>>       oc->reason = OFPR_ACTION;
>>>>       oc->pause = pause;
>>>> +    if (!ovs_feature_is_supported(OVS_DP_METER_SUPPORT)) {
>>>> +        meter_id = NX_CTLR_NO_METER;
>>>> +    }
>>>>       oc->meter_id = meter_id;
>>>>   
>>>>       struct action_header ah = { .opcode = htonl(opcode) };
>>>> diff --git a/lib/automake.mk b/lib/automake.mk
>>>> index ddfe33948..9f9f447d5 100644
>>>> --- a/lib/automake.mk
>>>> +++ b/lib/automake.mk
>>>> @@ -2,6 +2,7 @@ lib_LTLIBRARIES += lib/libovn.la
>>>>   lib_libovn_la_LDFLAGS = \
>>>>           $(OVS_LTINFO) \
>>>>           -Wl,--version-script=$(top_builddir)/lib/libovn.sym \
>>>> +        $(OVS_LIBDIR)/libopenvswitch.la \
>>>>           $(AM_LDFLAGS)
>>>>   lib_libovn_la_SOURCES = \
>>>>   	lib/acl-log.c \
>>>> diff --git a/lib/features.c b/lib/features.c
>>>> index fddf4c450..bd7ba79ef 100644
>>>> --- a/lib/features.c
>>>> +++ b/lib/features.c
>>>> @@ -18,7 +18,14 @@
>>>>   #include <stdlib.h>
>>>>   
>>>>   #include "lib/util.h"
>>>> +#include "lib/dirs.h"
>>>> +#include "socket-util.h"
>>>> +#include "lib/vswitch-idl.h"
>>>>   #include "openvswitch/vlog.h"
>>>> +#include "openvswitch/ofpbuf.h"
>>>> +#include "openvswitch/rconn.h"
>>>> +#include "openvswitch/ofp-msgs.h"
>>>> +#include "openvswitch/ofp-meter.h"
>>>>   #include "ovn/features.h"
>>>>   
>>>>   VLOG_DEFINE_THIS_MODULE(features);
>>>> @@ -40,11 +47,15 @@ static uint32_t supported_ovs_features;
>>>>   
>>>>   static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>>>>   
>>>> +/* ovs-vswitchd connection. */
>>>> +static struct rconn *swconn;
>>>> +
>>>>   static bool
>>>>   ovs_feature_is_valid(enum ovs_feature_value feature)
>>>>   {
>>>>       switch (feature) {
>>>>       case OVS_CT_ZERO_SNAT_SUPPORT:
>>>> +    case OVS_DP_METER_SUPPORT:
>>>>           return true;
>>>>       default:
>>>>           return false;
>>>> @@ -58,6 +69,80 @@ ovs_feature_is_supported(enum ovs_feature_value feature)
>>>>       return supported_ovs_features & feature;
>>>>   }
>>>>   
>>>> +static bool
>>>> +ovn_controller_get_ofp_capa(void)
>> This is definitely a nit, but I think I prefer something like
>>
>> static bool
>> ovs_feature_get_openflow_cap()
> 
> This will trigger the following warning:
> 
> b/features.c: In function ‘ovn_controller_get_ofp_capa’:
> lib/features.c:73:1: warning: old-style function definition [-Wold-style-definition]         

Probably my mistake as I forgot the void

static bool
ovs_feature_get_openflow_cap(void)


>>
>>>> +{
>>>> +    if (!swconn) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    rconn_run(swconn);
>>>> +    if (!rconn_is_connected(swconn)) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    bool ret = false;
>>>> +    /* dump datapath meter capabilities. */
>>>> +    struct ofpbuf *msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
>>>> +                                      rconn_get_version(swconn), 0);
>>>> +    rconn_send(swconn, msg, NULL);
>>>> +    for (int i = 0; i < 10; i++) {
>>>> +        msg = rconn_recv(swconn);
>>>> +        if (!msg) {
>>>> +            break;
>>>> +        }
>>>
>>> This rconn usage seems like it could be unreliable. If ovs-vswitchd is 
>>> super busy, it's possible that ovs-vswitchd will not be able to respond 
>>> to the request before we call rconn_recv().
>>
>> Maybe vconn would be more appropriate here? rconn does a lot of things
>> (queuing, reliability, etc) that I don't think are needed here. It is
>> also tricky to manage because you need to call rconn_run(), etc. As all
>> we need to do is quickly check the capabilities, I think this use case
>> is similar to calling `ovs-ofctrl show` (which also report the
>> capabilities) - this uses a vconn connection -
>> https://github.com/openvswitch/ovs/blob/849a40ccfb9c7c6bba635b517caac4f12ab63eee/utilities/ovs-ofctl.c#L833.
> 
> I am fine with it, I have just reused the same approach used in ofctrl or
> pinctrl.
> 
>>
>>
>> This would also mean you could remove the init() and deinit() function,
>> simplifying the code. If the connection fails for some reason, we can
>> try again on the next iteration.
>>
>>>
>>> I think a more reliable way of dealing with this would be to make use of 
>>> the poll loop to ensure that we only try to read from the rconn when it 
>>> has data ready.
>>>
>>> Also, why are there 10 iterations? I would have expected for this to 
>>> loop continually until rconn_recv() no longer returns any data.
>>>
>>>> +
>>>> +        const struct ofp_header *oh = msg->data;
>>>> +        enum ofptype type;
>>>> +        ofptype_decode(&type, oh);
>>>> +
>>>> +        if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) {
>>>> +            struct ofputil_meter_features mf;
>>>> +            ofputil_decode_meter_features(oh, &mf);
>>>> +
>>>> +            bool old_state = supported_ovs_features & OVS_DP_METER_SUPPORT;
>>>> +            bool new_state = mf.max_meters > 0;
>>>> +
>>>> +            if (old_state != new_state) {
>>>> +                ret = true;
>>>> +                if (new_state) {
>>>> +                    supported_ovs_features |= OVS_DP_METER_SUPPORT;
>>>> +                } else {
>>>> +                    supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
>>>> +                }
>>>> +            }
>>>> +        }
>>>> +        ofpbuf_delete(msg);
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +void
>>>> +ovs_feature_support_init(const struct ovsrec_bridge *br_int)
>>
>> I think you should remove the dependency on 'struct ovsrec_bridge' by
>> passing the bridge name to this function (i.e. a 'char *')
> 
> ack, I will fix it
> 
> Regards,
> Lorenzo
> 
>>
>>>> +{
>>>> +    if (!swconn) {
>>>> +        swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP15_VERSION);
>>>> +    }
>>>> +
>>>> +    if (swconn && !rconn_is_connected(swconn)) {
>>>> +        char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
>>>> +                                 br_int->name);
>>>> +        if (strcmp(target, rconn_get_target(swconn))) {
>>>> +            VLOG_INFO("%s: connecting to switch", target);
>>>> +            rconn_connect(swconn, target, target);
>>>> +        }
>>>> +        free(target);
>>>> +    }
>>>> +}
>>>> +
>>>> +void
>>>> +ovs_feature_support_deinit(void)
>>>> +{
>>>> +    rconn_destroy(swconn);
>>>> +    swconn = NULL;
>>>> +}
>>>> +
>>>>   /* Returns 'true' if the set of tracked OVS features has been updated. */
>>>>   bool
>>>>   ovs_feature_support_update(const struct smap *ovs_capabilities)
>>>> @@ -69,6 +154,10 @@ ovs_feature_support_update(const struct smap *ovs_capabilities)
>>>>           ovs_capabilities = &empty_caps;
>>>>       }
>>>>   
>>>> +    if (ovn_controller_get_ofp_capa()) {
>>>
>>> Can the openflow capabilities change while ovs-vswitchd is running? If 
>>> they cannot, then should we only request OFP capabilities after a reconnect?
>>>
>>>> +        updated = true;
>>>> +    }
>>>> +
>>>>       for (size_t i = 0; i < ARRAY_SIZE(all_ovs_features); i++) {
>>>>           enum ovs_feature_value value = all_ovs_features[i].value;
>>>>           const char *name = all_ovs_features[i].name;
>>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>>> index 5104a6895..fdee26ac9 100644
>>>> --- a/tests/ovn.at
>>>> +++ b/tests/ovn.at
>>>> @@ -1529,9 +1529,9 @@ log(verdict=allow, severity=warning);
>>>>   log(name="test1", verdict=drop, severity=info);
>>>>       encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31)
>>>>   log(verdict=drop, severity=info, meter="meter1");
>>>> -    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06,meter_id=4)
>>>> +    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06)
>>>>   log(name="test1", verdict=drop, severity=info, meter="meter1");
>>>> -    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31,meter_id=4)
>>>> +    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31)
>>>>   log(verdict=drop);
>>>>       formats as log(verdict=drop, severity=info);
>>>>       encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06)
>>>>
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>



More information about the dev mailing list