[ovs-dev] [PATCH v3 ovn] controller: add datapath meter capability check
Mark Gray
mark.d.gray at redhat.com
Thu Sep 16 09:24:36 UTC 2021
On 16/09/2021 04:42, Mark Michelson wrote:
> Hi Lorenzo, I have some comments down below.
>
Thanks for your changes so far Lorenzo.
> 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()
>> +{
>> + 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.
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 *')
>> +{
>> + 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