[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