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 <[email protected]> >> --- >> 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
