[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