[ovs-dev] [PATCH v5 ovn] controller: add datapath meter capability check
Mark Gray
mark.d.gray at redhat.com
Fri Sep 17 10:20:32 UTC 2021
On 17/09/2021 11:12, 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 v4:
> - move rconn setup in ovs_feature_support_update and rename it in
> ovs_feature_support_run
> - drop ovs_feature_support_init()
> - rename ovs_feature_support_deinit in ovs_feature_support_destroy
> Changes since v3:
> - add missing rconn_run_wait() and rconn_recv_wait() at the end of swconn for
> loop
> - cosmetics
> 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 | 7 +--
> include/ovn/features.h | 6 ++-
> lib/actions.c | 3 ++
> lib/automake.mk | 1 +
> lib/features.c | 95 ++++++++++++++++++++++++++++++++++++-
> lib/test-ovn-features.c | 6 +--
> tests/ovn.at | 4 +-
> 7 files changed, 112 insertions(+), 10 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 0031a1035..3347396f8 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -3569,9 +3569,9 @@ main(int argc, char *argv[])
> * 'br_int_dp' is valid only if an OVS transaction is possible.
> */
> if (ovs_idl_txn
> - && ovs_feature_support_update(br_int_dp
> - ? &br_int_dp->capabilities
> - : NULL)) {
> + && ovs_feature_support_run(br_int_dp ?
> + &br_int_dp->capabilities : NULL,
> + br_int ? br_int->name : NULL)) {
> VLOG_INFO("OVS feature set changed, force recompute.");
> engine_set_force_recompute(true);
> }
> @@ -3898,6 +3898,7 @@ loop_done:
> ovsdb_idl_loop_destroy(&ovs_idl_loop);
> ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
>
> + ovs_feature_support_destroy();
> free(ovs_remote);
> service_stop();
>
> diff --git a/include/ovn/features.h b/include/ovn/features.h
> index c35d59b14..d12a8eb0d 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -28,13 +28,17 @@
> */
> 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_destroy(void);
> bool ovs_feature_is_supported(enum ovs_feature_value feature);
> -bool ovs_feature_support_update(const struct smap *ovs_capabilities);
> +bool ovs_feature_support_run(const struct smap *ovs_capabilities,
> + const char *br_name);
>
> #endif
> 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..3ec2d26af 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,9 +69,87 @@ ovs_feature_is_supported(enum ovs_feature_value feature)
> return supported_ovs_features & feature;
> }
>
> +static void
> +ovs_feature_rconn_setup(const char *br_name)
> +{
> + 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_name);
> + if (strcmp(target, rconn_get_target(swconn))) {
> + VLOG_INFO("%s: connecting to switch", target);
> + rconn_connect(swconn, target, target);
> + }
> + free(target);
> + }
> +}
> +
> +static bool
> +ovs_feature_get_openflow_cap(const char *br_name)
> +{
> + if (!br_name) {
> + return false;
> + }
> +
> + ovs_feature_rconn_setup(br_name);
> +
> + 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 < 50; 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);
> + }
> + rconn_run_wait(swconn);
> + rconn_recv_wait(swconn);
> +
> + return ret;
> +}
> +
> +void
> +ovs_feature_support_destroy(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)
> +ovs_feature_support_run(const struct smap *ovs_capabilities,
> + const char *br_name)
> {
> static struct smap empty_caps = SMAP_INITIALIZER(&empty_caps);
> bool updated = false;
> @@ -69,6 +158,10 @@ ovs_feature_support_update(const struct smap *ovs_capabilities)
> ovs_capabilities = &empty_caps;
> }
>
> + if (ovs_feature_get_openflow_cap(br_name)) {
> + 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/lib/test-ovn-features.c b/lib/test-ovn-features.c
> index deb97581e..aabc547e6 100644
> --- a/lib/test-ovn-features.c
> +++ b/lib/test-ovn-features.c
> @@ -26,15 +26,15 @@ test_ovn_features(struct ovs_cmdl_context *ctx OVS_UNUSED)
> struct smap features = SMAP_INITIALIZER(&features);
>
> smap_add(&features, "ct_zero_snat", "false");
> - ovs_assert(!ovs_feature_support_update(&features));
> + ovs_assert(!ovs_feature_support_run(&features, NULL));
> ovs_assert(!ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT));
>
> smap_replace(&features, "ct_zero_snat", "true");
> - ovs_assert(ovs_feature_support_update(&features));
> + ovs_assert(ovs_feature_support_run(&features, NULL));
> ovs_assert(ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT));
>
> smap_add(&features, "unknown_feature", "true");
> - ovs_assert(!ovs_feature_support_update(&features));
> + ovs_assert(!ovs_feature_support_run(&features, NULL));
>
> smap_destroy(&features);
> }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 30625ec37..9e55c0d57 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)
>
Acked-by: Mark D. Gray <mark.d.gray at redhat.com>
More information about the dev
mailing list