| Message ID | 72c687b8bb6cc5eb1a55c7dcd9710ed953033ce1.1631229791.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v3] controller: add datapath meter capability check | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Hi Lorenzo, I have some comments down below. 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@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) > +{ > + 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(). 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) > +{ > + 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) >
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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> 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@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] > > >> +{ > >> + 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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
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@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@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>
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; + /* 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) +{ + 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; + } + + 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) +{ + 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()) { + 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)
Dump datapath meter capabilities before configuring meters in ovn-controller Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@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(-)