Re: [ovs-dev] [PATCH v5 ovn] controller: add datapath meter capability check
On 17/09/2021 11:12, Lorenzo Bianconi wrote:
> Dump datapath meter capabilities before configuring meters in
> ovn-controller
>
> Signed-off-by: Lorenzo Bianconi
> ---
> 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
> - ? _int_dp->capabilities
> - : NULL)) {
> +&& ovs_feature_support_run(br_int_dp ?
> + _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(_idl_loop);
> ovsdb_idl_loop_destroy(_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
>
> #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 &
[ovs-dev] [PATCH v5 ovn] controller: add datapath meter capability check
Dump datapath meter capabilities before configuring meters in
ovn-controller
Signed-off-by: Lorenzo Bianconi
---
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
- ? _int_dp->capabilities
- : NULL)) {
+&& ovs_feature_support_run(br_int_dp ?
+ _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(_idl_loop);
ovsdb_idl_loop_destroy(_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
#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
