Re: [ovs-dev] [PATCH v2 ovn] introduce rdnss, dnssl and route_info opt in put_nd_ra_opts action
> On Tue, Feb 8, 2022 at 1:27 PM Mark Michelson wrote:
> >
> > Thanks Lorenzo.
> >
> > Acked-by: Mark Michelson
>
> Thanks. I applied to the main branch.
Thanks Numan. I guess this patch can be considered a bug-fix, so can you please
backport it? Thanks.
Regards,
Lorenzo
>
> Numan
>
> >
> > On 2/4/22 19:19, Lorenzo Bianconi wrote:
> > > Send non-periodic Router Advertisement with rdnss, dnssl and route_info
> > > options if configured by the user.
> > >
> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1851788
> > > Signed-off-by: Lorenzo Bianconi
> > > ---
> > > Changes since v1:
> > > - copy input string before running strtok_r() in encode_ra_dnssl_opt() and
> > >encode_ra_route_info_opt().
> > > - cap max len value in encode_ra_dnssl_opt().
> > > ---
> > > controller/pinctrl.c | 35 ++
> > > include/ovn/actions.h | 1 +
> > > lib/actions.c | 153 ++
> > > lib/ovn-l7.h | 3 +
> > > northd/northd.c | 16 +
> > > tests/ovn.at | 30 +++--
> > > 6 files changed, 204 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > > index 293aecea2..ac6ec26e0 100644
> > > --- a/controller/pinctrl.c
> > > +++ b/controller/pinctrl.c
> > > @@ -3712,37 +3712,14 @@ static void
> > > packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32 lifetime,
> > > char *dnssl_data)
> > > {
> > > -char *dnssl_list, *t0, *r0 = NULL, dnssl[255] = {};
> > > size_t prev_l4_size = dp_packet_l4_size(b);
> > > -size_t size = sizeof(struct ovs_nd_dnssl);
> > > -int i = 0;
> > > +char dnssl[255] = {};
> > > +int size;
> > >
> > > -dnssl_list = xstrdup(dnssl_data);
> > > -
> > > -/* Multiple DNS Search List must be 'comma' separated
> > > - * (e.g. "a.b.c, d.e.f"). Domain names must be encoded
> > > - * as described in Section 3.1 of RFC1035.
> > > - * (e.g if dns list is a.b.c,www.ovn.org, it will be encoded as:
> > > - * 01 61 01 62 01 63 00 03 77 77 77 03 6f 76 63 03 6f 72 67 00
> > > - */
> > > -for (t0 = strtok_r(dnssl_list, ",", ); t0;
> > > - t0 = strtok_r(NULL, ",", )) {
> > > -char *t1, *r1 = NULL;
> > > -
> > > -size += strlen(t0) + 2;
> > > -if (size > sizeof(dnssl)) {
> > > -goto out;
> > > -}
> > > -
> > > -for (t1 = strtok_r(t0, ".", ); t1;
> > > - t1 = strtok_r(NULL, ".", )) {
> > > -dnssl[i++] = strlen(t1);
> > > -memcpy([i], t1, strlen(t1));
> > > -i += strlen(t1);
> > > -}
> > > -dnssl[i++] = 0;
> > > +size = encode_ra_dnssl_opt(dnssl_data, dnssl, sizeof(dnssl));
> > > +if (size < 0) {
> > > +return;
> > > }
> > > -size = ROUND_UP(size, 8);
> > >
> > > struct ip6_hdr *nh = dp_packet_l3(b);
> > > nh->ip6_plen = htons(prev_l4_size + size);
> > > @@ -3760,8 +3737,6 @@ packet_put_ra_dnssl_opt(struct dp_packet *b,
> > > ovs_be32 lifetime,
> > > uint32_t icmp_csum = packet_csum_pseudoheader6(dp_packet_l3(b));
> > > ra->icmph.icmp6_cksum = csum_finish(csum_continue(icmp_csum, ra,
> > > prev_l4_size +
> > > size));
> > > -out:
> > > -free(dnssl_list);
> > > }
> > >
> > > static void
> > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > > index cdef5fb03..0641b927e 100644
> > > --- a/include/ovn/actions.h
> > > +++ b/include/ovn/actions.h
> > > @@ -807,5 +807,6 @@ void ovnacts_encode(const struct ovnact[], size_t
> > > ovnacts_len,
> > >
> > > void ovnacts_free(struct ovnact[], size_t ovnacts_len);
> > > char *ovnact_op_to_string(uint32_t);
> > > +int encode_ra_dnssl_opt(char *data, char *buf, int buf_len);
> > >
> > > #endif /* ovn/actions.h */
> > > diff --git a/lib/actions.c b/lib/actions.c
> > > index d5d8391bb..5d3caaf2b 100644
> > > --- a/lib/actions.c
> > > +++ b/lib/actions.c
> > > @@ -40,6 +40,7 @@
> > > #include "simap.h"
> > > #include "uuid.h"
> > > #include "socket-util.h"
> > > +#include "lib/ovn-util.h"
> > >
> > > VLOG_DEFINE_THIS_MODULE(actions);
> > >
> > > @@ -2992,6 +2993,15 @@ parse_put_nd_ra_opts(struct action_context *ctx,
> > > const struct expr_field *dst,
> > > case ND_OPT_MTU:
> > > ok = c->format == LEX_F_DECIMAL;
> > > break;
> > > +
> > > +case ND_OPT_RDNSS:
> > > +ok = c->format == LEX_F_IPV6;
> > > +break;
> > > +
> > > +case ND_OPT_ROUTE_INFO_TYPE:
> > > +case ND_OPT_DNSSL:
> > > +ok = !!c->string;
> > > +break;
> > > }
> > >
> > > if (!ok) {
> > > @@ -3022,6 +3032,109 @@ format_PUT_ND_RA_OPTS(const struct
> > > ovnact_put_opts *po,
> > > format_put_opts("put_nd_ra_opts", po, s);
> > > }
> > >
> > > +int encode_ra_dnssl_opt(char *data, char
Re: [ovs-dev] [PATCH ovn v3] northd: Add feature to log reply and related ACL traffic.
Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 northd: Add feature to log reply and related ACL traffic. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3] northd: Add feature to log reply and related ACL traffic.
It can be desirable for replies to stateful ACLs to be logged. And in
some cases, it can actually be a bit confusing why they aren't logged.
Consider a situation where a port group called "port_group" exists and
logical switch ports swp1 and swp2 belong to it. We create the following
ACL, where logging is enabled:
from-lport 100 'inport == @port_group' allow-stateless
swp1 sends traffic to swp2 and swp2 responds within the same connection.
In this case, the logs will show both the packets from swp1 to swp2, as
well as the response packets from swp2 to swp1.
Now change the ACL:
from-lport 100 'inport == @port_group' allow-related
Now with the same traffic pattern, the packets from swp1 to swp2 are
logged, but the packets from swp2 to swp1 are not. Why is that?
The reason is that as a shortcut, when stateful ACLs are used, a single
priority 65532 flow is programmed to allow reply traffic to pass. When
no stateful ACL is present, the reply traffic is at the mercy of the
stateless ACL on the reply. Therefore, with the stateful ACL, the reply
traffic is not actually hitting an ACL but is let through by default,
but with the stateless ACL, the reply traffic does hit the ACL
evaluation, so the reply traffic is logged.
This change adds a feature that allows for reply traffic to be
optionally logged for stateful ACLs, therefore allowing for the behavior
to be similar for both ACL types. Since logging reply traffic requires
adding more flows, it is not enabled by default. In order to have reply
traffic logged, the ACL must have logging enabled, be stateful, have a
label, and have the new log_related column set to true,
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2031150
Signed-off-by: Mark Michelson
---
v2 -> v3:
* Added item to NEWS
* Added flow documentation to ovn-northd.8.xml
* Added "options" column to ACL instead of having a dedicated
log_related column
---
NEWS| 4 +
northd/northd.c | 42 +
northd/ovn-northd.8.xml | 8 +-
ovn-nb.ovsschema| 9 +-
ovn-nb.xml | 15 ++
tests/automake.mk | 3 +-
tests/check_acl_log.py | 107
tests/ovn-northd.at | 253
tests/system-ovn.at | 359
9 files changed, 796 insertions(+), 4 deletions(-)
create mode 100644 tests/check_acl_log.py
diff --git a/NEWS b/NEWS
index 194557410..5435aa606 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,9 @@
Post v21.12.0
-
+ - ACLs now have an "options" column for configuration of extra options.
+ - A new ACL option, "log-related" has been added that allows for reply and
+related traffic to be logged for an ACL in addition to the traffic that
+directly matches the ACL.
OVN v21.12.0 - 22 Dec 2021
--
diff --git a/northd/northd.c b/northd/northd.c
index 22e783ff6..f545891f8 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -6242,6 +6242,48 @@ consider_acl(struct hmap *lflows, struct ovn_datapath
*od,
acl->priority + OVN_ACL_PRI_OFFSET,
ds_cstr(match), ds_cstr(actions),
>header_);
+
+/* Related and reply traffic are universally allowed by priority
+ * 65532 flows created in build_acls(). If logging is enabled on
+ * the ACL, then we need to ensure that the related and reply
+ * traffic is logged, so we install a slightly higher-priority
+ * flow that matches the ACL, allows the traffic, and logs it.
+ */
+bool log_related = smap_get_bool(>options, "log-related",
false);
+if (acl->log && acl->label && log_related) {
+/* Related/reply flows need to be set on the opposite pipeline
+ * from where the ACL itself is set.
+ */
+enum ovn_stage log_related_stage = ingress ?
+S_SWITCH_OUT_ACL :
+S_SWITCH_IN_ACL;
+ds_clear(match);
+ds_clear(actions);
+
+ds_put_format(match, "ct.est && !ct.rel && !ct.new%s && "
+ "ct.rpl && ct_label.blocked == 0 && "
+ "ct_label.label == %" PRId64,
+ use_ct_inv_match ? " && !ct.inv" : "",
+ acl->label);
+build_acl_log(actions, acl, meter_groups);
+ds_put_cstr(actions, "next;");
+ovn_lflow_add_with_hint(lflows, od, log_related_stage,
+UINT16_MAX - 2,
+ds_cstr(match), ds_cstr(actions),
+>header_);
+
+ds_clear(match);
+ds_put_format(match, "!ct.est && ct.rel && !ct.new%s && "
+ "ct_label.blocked == 0
Re: [ovs-dev] [PATCH 1/5] ofproto-dpif: fix packet_execute_prepare
On 1/28/22 17:14, Aaron Conole wrote:
> From: Peng He
>
> In the commit 1df7f7aac8c976690167261fec9a50d832ef9e7e, the packet
> metadata's in_port has been changed from ofp port into odp port,
> however, the odp->flow->in_port is still ofp_port. This patch changes
> the odp->flow->in_port into odp_port.
>
> Fixes: 1df7f7aac8 ("ofproto-dpif: Restore packet metadata when a
> continuation is resumed.")
Nit: please, don't wrap the tags.
I'm sorry, but I'm getting trouble relating the mentioned commit
with the code change here. Commit 1df7f7aac8 made only cosmetic
change to the 'execution' path in order to split out the function.
The real change was made only for the packet resume, which doesn't
seem to be the case for the current patch.
I managed to trace the in_port update in the 'execution' path down
to commit 758c456df570 ("dpif: Use explicit packet metadata.").
It significantly changed the way code is structured, so it's more
likely a commit to blame.
I don't think that commit message here makes sense. So, I'm not
sure what exactly this patch is fixing. Am I missing something?
I think, we need a new commit message that describes what actually
the problem is and how it affects packet processing. And we
definitely need a unit test for that patch.
Best regards, Ilya Maximets.
> Signed-off-by: Peng He
> Acked-by: Mike Pattrick
> Signed-off-by: Aaron Conole
> ---
> ofproto/ofproto-dpif.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 8143dd965f..fed037b8d9 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5011,6 +5011,7 @@ packet_execute_prepare(struct ofproto *ofproto_,
> ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port,
> opo->packet);
>
> +opo->flow->in_port = opo->packet->md.in_port;
> ofproto_dpif_packet_out_delete(aux);
> opo->aux = execute;
> }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] vtep: provide option to check ovn-controller-vtep and ovn-northd versions
On Wed, Feb 2, 2022 at 3:39 PM Vladislav Odintsov wrote:
>
> Simlar to ovn-controller's behavior with checking internal
> version of ovn-northd and ovn-controller when option
> ovn-match-northd-version is defined, this commit adds same
> functionality for the ovn-controller-vtep daemon.
>
> This configuration option is located in the configured
> hardware_vtep database in the Global table's
> other_config:ovn-match-northd-version column/key.
> True value enforces check, while false or not defined values
> skip the check.
>
> The appropriate testcase is added as well.
>
> Signed-off-by: Vladislav Odintsov
Thanks. I applied this patch to the main branch.
Can you please submit another patch adding a news entry for this ?
Numan
> ---
> controller-vtep/ovn-controller-vtep.8.xml | 23 +-
> controller-vtep/ovn-controller-vtep.c | 50 +++--
> tests/ovn-controller-vtep.at | 55 +++
> 3 files changed, 124 insertions(+), 4 deletions(-)
>
> diff --git a/controller-vtep/ovn-controller-vtep.8.xml
> b/controller-vtep/ovn-controller-vtep.8.xml
> index 2c706e46e..0b9987bdb 100644
> --- a/controller-vtep/ovn-controller-vtep.8.xml
> +++ b/controller-vtep/ovn-controller-vtep.8.xml
> @@ -34,7 +34,7 @@
>information from both the ovnsb and the vtep database. If the
>database locations are not given from command line, the default
>is the db.sock in local OVSDB's 'run' directory.
> - The datapath location must take one of the following forms:
> + The database location must take one of the following forms:
>
>
>
> @@ -77,4 +77,25 @@
>
>
>
> +
> +
> + ovn-controller-vtep assumes it gets configuration
> + information from the following keys in the Global
> + table of the connected hardware_vtep database:
> +
> +
> +
> +
> + other_config:ovn-match-northd-version
> +
> +The boolean flag indicates if ovn-controller-vtep needs
> to
> +check ovn-northd version. If this flag is set to true
> and
> +the ovn-northd's version (reported in the Southbound
> +database) doesn't match with the ovn-controller-vtep's
> +internal version, then it will stop processing the southbound and
> +connected hardware_vtep database changes.
> +The default value is considered false if this option is not defined.
> +
> +
> +
>
> diff --git a/controller-vtep/ovn-controller-vtep.c
> b/controller-vtep/ovn-controller-vtep.c
> index 1d35c7f04..50f412b95 100644
> --- a/controller-vtep/ovn-controller-vtep.c
> +++ b/controller-vtep/ovn-controller-vtep.c
> @@ -30,6 +30,7 @@
> #include "fatal-signal.h"
> #include "openvswitch/poll-loop.h"
> #include "simap.h"
> +#include "ovsdb-idl.h"
> #include "stream.h"
> #include "stream-ssl.h"
> #include "unixctl.h"
> @@ -45,6 +46,8 @@
> #include "vtep.h"
> #include "ovn-controller-vtep.h"
>
> +VLOG_DEFINE_THIS_MODULE(main);
> +
> static unixctl_cb_func ovn_controller_vtep_exit;
>
> static void parse_options(int argc, char *argv[]);
> @@ -54,6 +57,37 @@ static char *vtep_remote;
> static char *ovnsb_remote;
> static char *default_db_;
>
> +/* Returns true if the northd internal version stored in SB_Global
> + * and ovn-controller-vtep internal version match.
> + */
> +static bool
> +check_northd_version(struct ovsdb_idl *vtep_idl, struct ovsdb_idl *ovnsb_idl,
> + const char *version)
> +{
> +const struct vteprec_global *cfg = vteprec_global_first(vtep_idl);
> +if (!cfg || !smap_get_bool(>other_config,
> "ovn-match-northd-version",
> + false)) {
> +return true;
> +}
> +
> +const struct sbrec_sb_global *sb = sbrec_sb_global_first(ovnsb_idl);
> +if (!sb) {
> +return false;
> +}
> +
> +const char *northd_version =
> +smap_get_def(>options, "northd_internal_version", "");
> +
> +if (strcmp(northd_version, version)) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +VLOG_WARN_RL(, "controller-vtep version - %s mismatch with northd
> "
> + "version - %s", version, northd_version);
> +return false;
> +}
> +
> +return true;
> +}
> +
> int
> main(int argc, char *argv[])
> {
> @@ -91,6 +125,9 @@ main(int argc, char *argv[])
> ovsdb_idl_create(ovnsb_remote, _idl_class, true, true));
> ovsdb_idl_get_initial_snapshot(ovnsb_idl_loop.idl);
>
> +char *ovn_version = ovn_get_internal_version();
> +VLOG_INFO("OVN internal version is : [%s]", ovn_version);
> +
> /* Main loop. */
> exiting = false;
> while (!exiting) {
> @@ -109,9 +146,16 @@ main(int argc, char *argv[])
> memory_report();
> simap_destroy();
> }
> -gateway_run();
> -binding_run();
> -vtep_run();
> +
> +if
Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option
On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov wrote:
>
> When transport node has multiple interfaces (vlans) and
> ovn-encap-ip on different hosts need to be configured
> from different VLANs source IP for encapsulated packet
> can be not the same, which is expected by remote system.
>
> Explicitely setting local_ip resolves such problem.
>
> Signed-off-by: Vladislav Odintsov
Hi Vladislav,
Can you please add the code to copy the new added config from OVS
database to the
other_config column of Chassis table in Southbound db ?
Please take a look at "struct ovs_chassis_cfg" in controller/chassis.c
Thanks
Numan
> ---
> controller/encaps.c | 37 +
> controller/ovn-controller.8.xml | 7 +++
> tests/ovn-controller.at | 9
> 3 files changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/controller/encaps.c b/controller/encaps.c
> index 66e0cd8cd..3b0c92931 100644
> --- a/controller/encaps.c
> +++ b/controller/encaps.c
> @@ -23,6 +23,7 @@
> #include "openvswitch/vlog.h"
> #include "lib/ovn-sb-idl.h"
> #include "ovn-controller.h"
> +#include "smap.h"
>
> VLOG_DEFINE_THIS_MODULE(encaps);
>
> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct
> sbrec_sb_global *sbg,
> smap_add(, "dst_port", dst_port);
> }
>
> +const struct ovsrec_open_vswitch *cfg =
> +ovsrec_open_vswitch_table_first(ovs_table);
> +
> +bool set_local_ip = false;
> +if (cfg) {
> +/* If the tos option is configured, get it */
> +const char *encap_tos = smap_get_def(>external_ids,
> + "ovn-encap-tos", "none");
> +
> +if (encap_tos && strcmp(encap_tos, "none")) {
> +smap_add(, "tos", encap_tos);
> +}
> +
> +/* If ovn-set-local-ip option is configured, get it */
> +set_local_ip = smap_get_bool(>external_ids, "ovn-set-local-ip",
> + false);
> +}
> +
> /* Add auth info if ipsec is enabled. */
> if (sbg->ipsec) {
> +set_local_ip = true;
> +smap_add(, "remote_name", new_chassis_id);
> +}
> +
> +if (set_local_ip) {
> const struct sbrec_chassis *this_chassis = tc->this_chassis;
> const char *local_ip = NULL;
>
> @@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct
> sbrec_sb_global *sbg,
> if (local_ip) {
> smap_add(, "local_ip", local_ip);
> }
> -smap_add(, "remote_name", new_chassis_id);
> -}
> -
> -const struct ovsrec_open_vswitch *cfg =
> -ovsrec_open_vswitch_table_first(ovs_table);
> -/* If the tos option is configured, get it */
> -if (cfg) {
> -const char *encap_tos = smap_get_def(>external_ids,
> - "ovn-encap-tos", "none");
> -
> -if (encap_tos && strcmp(encap_tos, "none")) {
> -smap_add(, "tos", encap_tos);
> -}
> }
>
> /* If there's an existing chassis record that does not need any change,
> diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
> index e9708fe64..cc9a7d1c2 100644
> --- a/controller/ovn-controller.8.xml
> +++ b/controller/ovn-controller.8.xml
> @@ -304,6 +304,13 @@
> of how many entries there are in the cache. By default this is set
> to
> 3 (30 seconds).
>
> + external_ids:ovn-set-local-ip
> +
> +The boolean flag indicates if ovn-controller when create
> +tunnel ports should set local_ip parameter. Can be
> +heplful to pin source outer IP for the tunnel when multiple
> interfaces
> +are used on the host for overlay traffic.
> +
>
>
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 2f39e5f3e..9e6302e5a 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve])
> ovs-vsctl del-port ovn-fakech-0
> OVS_WAIT_UNTIL([check_tunnel_property type geneve])
>
> +# set `ovn-set-local-ip` option to true and check if tunnel parameters
> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""])
> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true
> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""])
> +
> +# Change the local_ip on the OVS side and check than OVN fixes it
> +ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1"
> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""])
> +
> # Gracefully terminate daemons
> OVN_CLEANUP_SBOX([hv])
> OVN_CLEANUP_VSWITCH([main])
> --
> 2.30.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] lflow: Fix conjunction ID allocation problem with DP groups.
When DP group is enabled, a lflow attached to a DP group will be
processed against each of the DPs in the group one by one. The current
conjunction ID allocation algorithm didn't take into account the DP, so
when a lflow is processed, if it generates conjunctions, it will try to
allocate conjunction IDs repeatedly for each DP. The function
lflow_conj_ids_alloc has a protection to avoid leaking the IDs - it
free the existing IDs before allocating again for the same lflow.
Because of this the coverage counter 'lflow_conj_free_unexpected' will
increase.
In most cases this is not a problem. Most likely each allocation for the
same lflow would get the same ID and use it for different OVS flows with
different DP metadata match, without conflict.
However, there is a chance (although extremely small) that this can lead
to double allocation, when different DPs have different n_conjs for the
same lflow. For example:
- 2 DPs (DP1 and DP2) in a DPG used by lflow A, which uses port-group
PG1, in the below form:
inport == @PG1 && (tcp.src == {1, 2, 3} || tcp.dst == {4, 5, 6})
This can generate flows with n_conjs of 1 or 2, depending on how
many ports PG1 is evaluated to. If PG1 is evaluated with no ports,
then there is no match (no flows). If PG1 is evaluated to 1 port,
then there is an conjunction (n_conjs == 1) due to the '||'. If PG1
is evaluated to 2 or more ports, there will be 2 conjunctions
(n_conjs == 2).
- Assume PG1 is divided into SB port-groups PG1-DP1, PG1-DP2. PG1-DP1
has 1 port, PG1-DP2 has 2 ports.
- Now the lflow is parsed against DP1 first, which end up with n_conjs =
1, and allocates a single conj_id (say, ID_1).
- The lflow is parsed against DP2, which end up with n_conjs = 2. Now it
frees the existing ID_1 allocated for the lflow, and reallocates 2
conjunction IDs. The algorithm would try to allocate ID_1 and
(ID_1 + 1). However, assume that ID_1 + 1 happened to be allocated by
another lflow-2 already (its uuid prefix happened to be ID_1 + 1,
which is unfortunate), so the algorithm would move on to the next
available continuous 2 IDs and allocate for the lflow for DP2.
- At this moment, the ID_1 allocated for lflow for DP1 is still in use,
but lost track in the conj_ids table (already free-ed). Now lflow-3
comes and happens to have its uuid prefix being ID_1, although very
unlikely but still possible, and it requires a conjunction ID, so ID_1
is then allocated to the lflow-3.
- Now ID_1 is used by OVS flows of lflow-1 and lflow-3. Double
allocation! Packets hitting these flows would have unpredictable
behavior.
The fix is rather straightforward. Just take DP uuid as part of the
allocation key, together with lflow uuid, and change the hash algorithm
to consider both uuids. A major change, though, is the free part. When a
lflow is deleted/reprocessed, we need to free the IDs for all the DPs,
so the patch introduced another hmap index from lflow uuid to the
list of DP nodes.
For unit test, the algorithm itself didn't change much except for the
hashing. So for the convenience of testing it introduces a test_mode so
that it still uses the lflow uuid prefix for unit test only.
The patch also adds a check in the PG test to make sure the counter
lflow_conj_free_unexpected is always 0. The updated test would fail
without this patch.
Fixes: b609aeeb0 ("lflow: Consistent conjunction id generation.")
Signed-off-by: Han Zhou
---
controller/lflow-conj-ids.c | 183 ---
controller/lflow-conj-ids.h | 9 +-
controller/lflow.c | 2 +
controller/test-lflow-conj-ids.c | 7 +-
tests/ovn-lflow-conj-ids.at | 26 ++---
tests/ovn.at | 4 +
6 files changed, 174 insertions(+), 57 deletions(-)
diff --git a/controller/lflow-conj-ids.c b/controller/lflow-conj-ids.c
index bfe63862a..95fcff067 100644
--- a/controller/lflow-conj-ids.c
+++ b/controller/lflow-conj-ids.c
@@ -17,6 +17,8 @@
#include "coverage.h"
#include "lflow-conj-ids.h"
#include "util.h"
+#include "hash.h"
+#include "openvswitch/list.h"
COVERAGE_DEFINE(lflow_conj_conflict);
COVERAGE_DEFINE(lflow_conj_alloc);
@@ -30,33 +32,64 @@ struct conj_id_node {
uint32_t conj_id;
};
-/* Node in struct conj_ids.lflow_conj_ids. */
struct lflow_conj_node {
-struct hmap_node hmap_node;
+struct hmap_node hmap_node; /* Node in struct conj_ids.lflow_conj_ids. */
+struct ovs_list list_node; /* Node in struct lflow_to_dps_node.dps. */
struct uuid lflow_uuid;
+struct uuid dp_uuid;
uint32_t start_conj_id;
uint32_t n_conjs;
};
+struct lflow_to_dps_node {
+struct hmap_node hmap_node; /* Node in struct conj_ids.lflow_to_dps. */
+struct uuid lflow_uuid;
+struct ovs_list dps; /* List of DPs the lflow belongs to. Each node is
+struct lflow_conj_node.list_node. */
+};
+
+static bool test_mode = false;
+
static void lflow_conj_ids_insert_(struct conj_ids *,
Re: [ovs-dev] [PATCH v4 ovn] ovn-nbctl: add the capability to specify CoPP name
On Fri, Feb 11, 2022 at 9:29 AM Dumitru Ceara wrote: > > On 1/29/22 15:31, Lorenzo Bianconi wrote: > > Introduce the capability to specify CoPP name in order to > > reuse the same CoPP reference on multiple datapaths. > > Introduce the following CoPP commands: > > - ovn-nbctl copp-add > > - ovn-nbctl copp-del > > - ovn-nbctl copp-list > > - ovn-nbctl ls-copp-add > > - ovn-nbctl lr-copp-add > > > > Introduce name and external_ids columns in CoPP table. > > > > Reported-ad: https://bugzilla.redhat.com/show_bug.cgi?id=2040852 > > Signed-off-by: Lorenzo Bianconi > > --- > > Looks good to me, thanks! > > Acked-by: Dumitru Ceara Thanks. I applied this patch to the main branch. Numan > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 ovn] introduce rdnss, dnssl and route_info opt in put_nd_ra_opts action
On Tue, Feb 8, 2022 at 1:27 PM Mark Michelson wrote:
>
> Thanks Lorenzo.
>
> Acked-by: Mark Michelson
Thanks. I applied to the main branch.
Numan
>
> On 2/4/22 19:19, Lorenzo Bianconi wrote:
> > Send non-periodic Router Advertisement with rdnss, dnssl and route_info
> > options if configured by the user.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1851788
> > Signed-off-by: Lorenzo Bianconi
> > ---
> > Changes since v1:
> > - copy input string before running strtok_r() in encode_ra_dnssl_opt() and
> >encode_ra_route_info_opt().
> > - cap max len value in encode_ra_dnssl_opt().
> > ---
> > controller/pinctrl.c | 35 ++
> > include/ovn/actions.h | 1 +
> > lib/actions.c | 153 ++
> > lib/ovn-l7.h | 3 +
> > northd/northd.c | 16 +
> > tests/ovn.at | 30 +++--
> > 6 files changed, 204 insertions(+), 34 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index 293aecea2..ac6ec26e0 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -3712,37 +3712,14 @@ static void
> > packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32 lifetime,
> > char *dnssl_data)
> > {
> > -char *dnssl_list, *t0, *r0 = NULL, dnssl[255] = {};
> > size_t prev_l4_size = dp_packet_l4_size(b);
> > -size_t size = sizeof(struct ovs_nd_dnssl);
> > -int i = 0;
> > +char dnssl[255] = {};
> > +int size;
> >
> > -dnssl_list = xstrdup(dnssl_data);
> > -
> > -/* Multiple DNS Search List must be 'comma' separated
> > - * (e.g. "a.b.c, d.e.f"). Domain names must be encoded
> > - * as described in Section 3.1 of RFC1035.
> > - * (e.g if dns list is a.b.c,www.ovn.org, it will be encoded as:
> > - * 01 61 01 62 01 63 00 03 77 77 77 03 6f 76 63 03 6f 72 67 00
> > - */
> > -for (t0 = strtok_r(dnssl_list, ",", ); t0;
> > - t0 = strtok_r(NULL, ",", )) {
> > -char *t1, *r1 = NULL;
> > -
> > -size += strlen(t0) + 2;
> > -if (size > sizeof(dnssl)) {
> > -goto out;
> > -}
> > -
> > -for (t1 = strtok_r(t0, ".", ); t1;
> > - t1 = strtok_r(NULL, ".", )) {
> > -dnssl[i++] = strlen(t1);
> > -memcpy([i], t1, strlen(t1));
> > -i += strlen(t1);
> > -}
> > -dnssl[i++] = 0;
> > +size = encode_ra_dnssl_opt(dnssl_data, dnssl, sizeof(dnssl));
> > +if (size < 0) {
> > +return;
> > }
> > -size = ROUND_UP(size, 8);
> >
> > struct ip6_hdr *nh = dp_packet_l3(b);
> > nh->ip6_plen = htons(prev_l4_size + size);
> > @@ -3760,8 +3737,6 @@ packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32
> > lifetime,
> > uint32_t icmp_csum = packet_csum_pseudoheader6(dp_packet_l3(b));
> > ra->icmph.icmp6_cksum = csum_finish(csum_continue(icmp_csum, ra,
> > prev_l4_size +
> > size));
> > -out:
> > -free(dnssl_list);
> > }
> >
> > static void
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index cdef5fb03..0641b927e 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -807,5 +807,6 @@ void ovnacts_encode(const struct ovnact[], size_t
> > ovnacts_len,
> >
> > void ovnacts_free(struct ovnact[], size_t ovnacts_len);
> > char *ovnact_op_to_string(uint32_t);
> > +int encode_ra_dnssl_opt(char *data, char *buf, int buf_len);
> >
> > #endif /* ovn/actions.h */
> > diff --git a/lib/actions.c b/lib/actions.c
> > index d5d8391bb..5d3caaf2b 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -40,6 +40,7 @@
> > #include "simap.h"
> > #include "uuid.h"
> > #include "socket-util.h"
> > +#include "lib/ovn-util.h"
> >
> > VLOG_DEFINE_THIS_MODULE(actions);
> >
> > @@ -2992,6 +2993,15 @@ parse_put_nd_ra_opts(struct action_context *ctx,
> > const struct expr_field *dst,
> > case ND_OPT_MTU:
> > ok = c->format == LEX_F_DECIMAL;
> > break;
> > +
> > +case ND_OPT_RDNSS:
> > +ok = c->format == LEX_F_IPV6;
> > +break;
> > +
> > +case ND_OPT_ROUTE_INFO_TYPE:
> > +case ND_OPT_DNSSL:
> > +ok = !!c->string;
> > +break;
> > }
> >
> > if (!ok) {
> > @@ -3022,6 +3032,109 @@ format_PUT_ND_RA_OPTS(const struct ovnact_put_opts
> > *po,
> > format_put_opts("put_nd_ra_opts", po, s);
> > }
> >
> > +int encode_ra_dnssl_opt(char *data, char *buf, int buf_len)
> > +{
> > +size_t size = sizeof(struct ovs_nd_dnssl);
> > +char *dnssl = xstrdup(data);
> > +char *t0, *r0 = NULL;
> > +int i = 0;
> > +
> > +/* Multiple DNS Search List must be 'comma' separated
> > + * (e.g. "a.b.c, d.e.f"). Domain names must be encoded
> > + * as described in Section 3.1 of RFC1035.
> > + * (e.g if dns list is
Re: [ovs-dev] [PATCH ovn v3] acl-log: Log the direction (logical pipeline) of the matching ACL.
On Fri, Feb 11, 2022 at 9:42 AM Dumitru Ceara wrote:
>
> It's useful to differentiate between ingress and egress pipelines in the
> ACL logs. To achieve this we determine the direction by interpreting the
> openflow table ID when processing packets punted to pinctrl by "log()"
> action.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992641
> Signed-off-by: Dumitru Ceara
> ---
> v3: Simplify the patch as suggested by Numan; detect pipeline based on
> openflow table id.
> v2: Add the "direction" after the "verdict" and "severity" fields in the
> ACL logs.
> ---
Thanks for v3.
I've one minor comment. I somehow feel using "from-lport" and
"to-lport" instead of "IN" and "OUT"
would be more appropriate. But I'm fine with the present patch too.
Acked-by: Numan Siddique
Thanks
Numan
> NEWS | 2 ++
> controller/pinctrl.c | 4 ++-
> lib/acl-log.c | 8 +++--
> lib/acl-log.h | 3 +-
> tests/ovn.at | 68 ++-
> utilities/ovn-trace.c | 9 --
> 6 files changed, 73 insertions(+), 21 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 194557410b..a01820dfe6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,7 @@
> Post v21.12.0
> -
> + - When configured to log packtes matching ACLs, log the direction (logical
> +pipeline) too.
>
> OVN v21.12.0 - 22 Dec 2021
> --
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index fd0bccdb6c..e275b5e08a 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3166,7 +3166,9 @@ process_packet_in(struct rconn *swconn, const struct
> ofp_header *msg)
> break;
>
> case ACTION_OPCODE_LOG:
> -handle_acl_log(, );
> +handle_acl_log(, ,
> + pin.table_id < OFTABLE_LOG_EGRESS_PIPELINE
> + ? "IN" : "OUT");
> break;
>
> case ACTION_OPCODE_PUT_ND_RA_OPTS:
> diff --git a/lib/acl-log.c b/lib/acl-log.c
> index 220b6dc30e..9530dd7635 100644
> --- a/lib/acl-log.c
> +++ b/lib/acl-log.c
> @@ -76,7 +76,8 @@ log_severity_from_string(const char *name)
> }
>
> void
> -handle_acl_log(const struct flow *headers, struct ofpbuf *userdata)
> +handle_acl_log(const struct flow *headers, struct ofpbuf *userdata,
> + const char *direction)
> {
> if (!VLOG_IS_INFO_ENABLED()) {
> return;
> @@ -94,9 +95,10 @@ handle_acl_log(const struct flow *headers, struct ofpbuf
> *userdata)
> struct ds ds = DS_EMPTY_INITIALIZER;
> ds_put_cstr(, "name=");
> json_string_escape(name_len ? name : "", );
> -ds_put_format(, ", verdict=%s, severity=%s: ",
> +ds_put_format(, ", verdict=%s, severity=%s, direction=%s: ",
>log_verdict_to_string(lph->verdict),
> - log_severity_to_string(lph->severity));
> + log_severity_to_string(lph->severity),
> + direction);
> flow_format(, headers, NULL);
>
> VLOG_INFO("%s", ds_cstr());
> diff --git a/lib/acl-log.h b/lib/acl-log.h
> index 4f23f790dd..da7fa2f02c 100644
> --- a/lib/acl-log.h
> +++ b/lib/acl-log.h
> @@ -49,6 +49,7 @@ const char *log_verdict_to_string(uint8_t verdict);
> const char *log_severity_to_string(uint8_t severity);
> uint8_t log_severity_from_string(const char *name);
>
> -void handle_acl_log(const struct flow *headers, struct ofpbuf *userdata);
> +void handle_acl_log(const struct flow *headers, struct ofpbuf *userdata,
> +const char *direction);
>
> #endif /* lib/acl-log.h */
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 957eb7850f..cfb9818ced 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -8965,33 +8965,59 @@ ovn-nbctl lsp-set-addresses lp2 $lp2_mac
> ovn-nbctl --wait=sb sync
> wait_for_ports_up
>
> -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==80' drop
> -ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000
> 'tcp.dst==81' drop
> +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==80' drop
> +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 from-lport
> 1000 'tcp.dst==81' drop
> +
> +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==180' drop
> +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000
> 'tcp.dst==181' drop
> +
> +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==82' allow
> +ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 from-lport
> 1000 'tcp.dst==83' allow
>
> ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==82' allow
> ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 to-lport 1000
> 'tcp.dst==83' allow
>
> +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==84' allow-related
> +ovn-nbctl --log acl-add lsw0 from-lport 1000 'tcp.dst==85' allow-related
> +
> ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==84' allow-related
> ovn-nbctl --log acl-add lsw0 to-lport 1000 'tcp.dst==85' allow-related
>
> -ovn-nbctl acl-add lsw0 to-lport 1000
Re: [ovs-dev] [PATCH v2 ovn] ovn-nbctl: report peer addresses running lsp-get-addresses for patch ports
On Thu, Jan 20, 2022 at 11:09 AM Lorenzo Bianconi
wrote:
>
> Report logical router port addresses running lsp-get-addresses if the
> logical switch port address is set to "router".
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2028040
> Signed-off-by: Lorenzo Bianconi
> ---
> Changes since v1:
> - rely on lrp_by_name_or_uuid
Thanks for v2. I applied this patch to the main branch.
Numan
> ---
> tests/ovn-nbctl.at| 10 ++
> utilities/ovn-nbctl.c | 29 +
> 2 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index a43a1ce8f..539a121c0 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -181,6 +181,16 @@ unknown
>
> AT_CHECK([ovn-nbctl lsp-set-addresses lp0])
> AT_CHECK([ovn-nbctl lsp-get-addresses lp0], [0], [dnl
> +])
> +
> +AT_CHECK([ovn-nbctl lr-add lr0])
> +AT_CHECK([ovn-nbctl lrp-add lr0 rp-ls0 aa:bb:bb:00:00:01 192.168.0.1/24])
> +AT_CHECK([ovn-nbctl lsp-add ls0 ls0-rp])
> +AT_CHECK([ovn-nbctl lsp-set-addresses ls0-rp router])
> +AT_CHECK([ovn-nbctl lsp-set-type ls0-rp router])
> +AT_CHECK([ovn-nbctl lsp-set-options ls0-rp router-port=rp-ls0])
> +AT_CHECK([ovn-nbctl lsp-get-addresses ls0-rp], [0], [dnl
> +aa:bb:bb:00:00:01 192.168.0.1/24
> ])])
>
> dnl -
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 55b0f5124..f3209417a 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -1494,8 +1494,18 @@ nbctl_pre_lsp_get_addresses(struct ctl_context *ctx)
> {
> ovsdb_idl_add_column(ctx->idl, _logical_switch_port_col_name);
> ovsdb_idl_add_column(ctx->idl, _logical_switch_port_col_addresses);
> +ovsdb_idl_add_column(ctx->idl, _logical_switch_port_col_options);
> +
> +ovsdb_idl_add_column(ctx->idl, _logical_router_col_ports);
> +ovsdb_idl_add_column(ctx->idl, _logical_router_port_col_name);
> +ovsdb_idl_add_column(ctx->idl, _logical_router_port_col_mac);
> +ovsdb_idl_add_column(ctx->idl, _logical_router_port_col_networks);
> }
>
> +static char * OVS_WARN_UNUSED_RESULT
> +lrp_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist,
> +const struct nbrec_logical_router_port **lrp_p);
> +
> static void
> nbctl_lsp_get_addresses(struct ctl_context *ctx)
> {
> @@ -1511,6 +1521,21 @@ nbctl_lsp_get_addresses(struct ctl_context *ctx)
> return;
> }
>
> +const char *router_port = smap_get(>options, "router-port");
> +if (lsp->n_addresses == 1 && !strcmp(lsp->addresses[0], "router") &&
> +router_port) {
> +const struct nbrec_logical_router_port *lrp;
> +error = lrp_by_name_or_uuid(ctx, router_port, false, );
> +if (lrp) {
> +ds_put_format(>output, "%s", lrp->mac);
> +for (size_t j = 0; j < lrp->n_networks; j++) {
> +ds_put_format(>output, " %s", lrp->networks[j]);
> +}
> +ds_put_cstr(>output, "\n");
> +return;
> +}
> +}
> +
> svec_init();
> for (i = 0; i < lsp->n_addresses; i++) {
> svec_add(, lsp->addresses[i]);
> @@ -4108,10 +4133,6 @@ nbctl_pre_lr_route_add(struct ctl_context *ctx)
>
> _logical_router_static_route_col_external_ids);
> }
>
> -static char * OVS_WARN_UNUSED_RESULT
> -lrp_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist,
> -const struct nbrec_logical_router_port **lrp_p);
> -
> static void
> nbctl_lr_route_add(struct ctl_context *ctx)
> {
> --
> 2.34.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Documentation: Update USDT documentation to include systemtap dependency
Eelco Chaudron writes: > Update the documentation to include details on SystemTap dependency > when enabling USDT probes. > > Suggested-by: Adrian Moreno > Signed-off-by: Eelco Chaudron > --- > Documentation/topics/usdt-probes.rst |5 + > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/topics/usdt-probes.rst > b/Documentation/topics/usdt-probes.rst > index cdded4f90..7ce19aaed 100644 > --- a/Documentation/topics/usdt-probes.rst > +++ b/Documentation/topics/usdt-probes.rst > @@ -50,6 +50,11 @@ is used :: > > checking whether USDT probes are enabled... yes > > +As USDT probes internally use the ``DTRACE_PROBExx`` macros, which are part > of > +the SystemTap framework, you need to install the appropriate package for your > +Linux distribution. For example, on Fedora, you need to install the > +``systemtap-sdt-devel`` package. > + > The patch LGTM. Acked-by: Paolo Valerio ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: add an option to assign pmd rxq to all numas
Hi Wan Junjie,
On 27/01/2022 11:43, Wan Junjie wrote:
When assign a rxq to a pmd, the rxq will only be assigned to
the numa it belongs. An exception is that the numa has no non-iso
pmd there.
For example, we have one vm port (vhu) with all its rxqs in numa1,
while one phy port (p0) in numa0 with all its rxqs in numa 0.
we have four pmds in two numas.
See tables below, paras are listed as (port, queue, numa, core)
p0 0 0 20
p0 1 0 21
p0 2 0 20
p0 3 0 21
vhu 0 1 40
vhu 1 1 41
vhu 2 1 40
vhu 2 1 41
In this scenario, ovs-dpdk underperforms another setting that
all p0's queues pinned to four pmds using pmd-rxq-affinity. With
pmd-rxq-isolate=false, we can make vhu get polled on numa 1.
p0 0 0 20
p0 1 1 40
p0 2 0 21
p0 3 1 41
vhu 0 1 40
vhu 1 1 41
vhu 2 1 40
vhu 2 1 41
Then we found that with really high throughput, say pmd 40, 41
will reach 100% cycles busy, and pmd 20, 21 reach 70% cycles
busy. The unbalanced rxqs on these pmds became the bottle neck.
OTOH, you could also look at it that you are contributing to that
overload by pinning p0 to those cores. So the partial pinning you are
doing here is not a good solution for your config. If you had of
continued to pin all the queues, then you would have had a better solution.
pmd-rxq-numa=false would ignore the rxq's numa attribute when
assign rxq to pmds. This could make full use of the cores from
different numas while we have limited core resources for pmds.
Yes, with this config and this traffic, it is beneficial. The challenge
is trying to also make it not cause performance regressions for other
configs. Or at least having some control about that.
With the code below (and the user enabling) there is no preference for
local-numa polling, so it can be always local-numa or cross-numa. Have
you tested the performance drop that will occur if cross-numa polling is
selected instead of local-numa for different cases? It can be quite
large, like 40%.
Another issue is that if you allow polling to switch numa commonly you
also make estimates for assignments less reliable. I don't see any good
way around this, but there might still be net benefit to lose some
accuracy in order to be able to utilize all the cores. Especially if you
consider worst cases where one numa pmds are overloaded and another numa
pmds are idle.
Btw, this patch is similar in functionality to the one posted by Anurag
[0] and there was also some discussion about this approach here [1].
Another option is to only use cross-numa cores when local-numa ones are
overloaded. That way we can get the benefit from local-numa polling
for performance and also utilize cross-numa under overload conditions.
Though it comes with the expense of complexity and defining what
overloaded is etc. I have a an RFC to do that, I will post for
discussion once I clean it up.
thanks,
Kevin.
[0] https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384547.html
[1] https://mail.openvswitch.org/pipermail/ovs-dev/2021-July/385695.html
Signed-off-by: Wan Junjie
---
lib/dpif-netdev.c| 30 +---
tests/pmd.at | 106 +++
vswitchd/vswitch.xml | 19 +++-
3 files changed, 148 insertions(+), 7 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index e28e0b554..6b54a3a4c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -297,6 +297,7 @@ struct dp_netdev {
struct ovs_mutex tx_qid_pool_mutex;
/* Rxq to pmd assignment type. */
enum sched_assignment_type pmd_rxq_assign_type;
+bool pmd_rxq_assign_numa;
bool pmd_iso;
/* Protects the access of the 'struct dp_netdev_pmd_thread'
@@ -1844,6 +1845,7 @@ create_dp_netdev(const char *name, const struct
dpif_class *class,
cmap_init(>poll_threads);
dp->pmd_rxq_assign_type = SCHED_CYCLES;
+dp->pmd_rxq_assign_numa = true;
ovs_mutex_init(>tx_qid_pool_mutex);
/* We need 1 Tx queue for each possible core + 1 for non-PMD threads. */
@@ -4867,6 +4869,14 @@ dpif_netdev_set_config(struct dpif *dpif, const struct
smap *other_config)
dp_netdev_request_reconfigure(dp);
}
+bool rxq_assign_numa = smap_get_bool(other_config, "pmd-rxq-numa", true);
+if (dp->pmd_rxq_assign_numa != rxq_assign_numa) {
+dp->pmd_rxq_assign_numa = rxq_assign_numa;
+VLOG_INFO("Rxq to PMD numa assignment changed to: \'%s\'.",
+rxq_assign_numa ? "numa enabled": "numa disabled");
+dp_netdev_request_reconfigure(dp);
+}
+
struct pmd_auto_lb *pmd_alb = >pmd_alb;
rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval",
@@ -5869,6 +5879,7 @@ static void
sched_numa_list_schedule(struct sched_numa_list *numa_list,
struct dp_netdev *dp,
enum sched_assignment_type algo,
+ bool assign_numa,
enum
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On Fri, Feb 11, 2022 at 3:53 AM Dumitru Ceara wrote: > > On 2/11/22 00:54, Numan Siddique wrote: > > On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara wrote: > >> > >> We still need to try to ovsdb_idl_loop_commit_and_wait() on instances > >> that are in standby mode. That's because they need to try to take the > >> lock. But if they're in standby they won't actually build a transaction > >> json and will return early because they don't own the SB lock. > >> > >> That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we > >> shouldn't act on it. > >> > >> Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear() > >> should be called only on active instances. The standby or paused ones > >> will get the incremental updates when they become active. Otherwise we > >> would be forced to perform a full recompute when the instance becomes > >> active. > > > > Hi Dumitru, > > > > Hi Numan, > > Thanks for the review! > > > I've a question on the track clear being moved out of the standby instances. > > To ensure correctness, I suppose it's better to trigger a full recompute > > when a > > standby instance becomes active. What do you think? > > > > I might be wrong but I don't think that's necessary. It may also be > quite costly as full recomputes can take quite long. > > > Also lets say CMS does the below operations > > - Add a logical switch S1 > > - Add a logical port p1 in S1 > > - Add a logical port p2 in S1 > > - Delete logical port p2 > > - Delete a logical switch. > > > > With this patch since we are not clearing the tracking information, > > how does ovn-northd > > process the tracked changes when it becomes active ? > > When ovn-northd becomes active, from a Northbound database perspective, > there were no changes; that is, S1 didn't exist when it was last active > and it doesn't exist now either. > > So, there should be no NB change to process. Accumulating tracked > changes without calling clear() on the standby has exactly this effect. > > From a Southbound database perspective there are two cases: > > a. The former active northd processed some (but not all) of the NB > changes and executed their corresponding SB transactions. In this case, > the standby northd also receives update messages for the SB records that > were changed. The standby northd tracks these changes. > > When the standby northd becomes active it will: > - determine that NB state didn't change > - SB state changed and needs to be reconciled (today we do this with the > help of a NULL change handler for SB_* tables which will trigger a full > recompute). > > b. The former active northd processed all of the NB changes and executed > their corresponding SB transactions. In this case, the final state of > the NB and SB databases should be equivalent to their initial states. > NB/SB changes will be accumulated by the change tracking mechanism on > the standby resulting in empty tracked changes lists. This is fine > because the new active northd doesn't need to do anything, the DB > contents are already consistent. > > c. The former active northd processed none of the NB changes yet. This > is very similar to case "b" above, the new active northd doesn't need to > change anything in the NB/SB and it won't do that either. Thanks for the detailed explanation. Acked-by: Numan Siddique Numan > > > > > Thanks > > Numan > > > > Thanks, > Dumitru > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 13/18] python: detect changes in flow formatting code
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > In order to minimize the risk of having the python flow parsing code and > the C flow formatting code divert, add a target that checks if the > formatting code has been changed since the last revision and warn the > developer if it has. > > The script also makes it easy to update the dependency file so hopefully > it will not cause too much trouble for a developer that has modifed the > file without changing the flow string format. > > Signed-off-by: Adrian Moreno Changes look good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] acl-log: Log the direction (logical pipeline) of the matching ACL.
On 2/8/22 18:25, Numan Siddique wrote: > On Wed, Feb 2, 2022 at 5:17 PM Mark Michelson wrote: >> >> Acked-by: Mark Michelson >> >> On 2/2/22 12:26, Dumitru Ceara wrote: >>> It's useful to differentiate between ingress and egress pipelines in the >>> ACL logs. To achieve this we expand the "log()" logical action and pass >>> an optional direction. >>> >>> This behavior change is implemented in a backwards compatible way such >>> that it doesn't break existing deployments even in the case when >>> ovn-northd gets updated before ovn-controller. >>> >>> To achieve this, ovn-northd determines first if all chassis in the >>> cluster have been upgraded to a version that supports the new "log()" >>> action format. >>> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992641 >>> Signed-off-by: Dumitru Ceara >>> --- >>> v2: Add the "direction" after the "verdict" and "severity" fields in the >>> ACL logs. > > Hi Dumitru, > > Thanks for the patch. The patch LGTM. > > I've an alternate suggestion to log the direction. > When the packet is received by ovn-controller, it can determine the > open flow table id from the table_id field of 'struct > ofputil_packet_in'. > From the open flow table id, it should be simple enough to figure out > if this is from ingress or egress direction. > > If we take this approach, we don't have to make any changes to the > 'acl_log' OVN action. > > Let me know what you think. > You're right this is way better, thanks for the suggestion! I sent a v3: http://patchwork.ozlabs.org/project/ovn/list/?series=285670 Thanks, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3] acl-log: Log the direction (logical pipeline) of the matching ACL.
It's useful to differentiate between ingress and egress pipelines in the
ACL logs. To achieve this we determine the direction by interpreting the
openflow table ID when processing packets punted to pinctrl by "log()"
action.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992641
Signed-off-by: Dumitru Ceara
---
v3: Simplify the patch as suggested by Numan; detect pipeline based on
openflow table id.
v2: Add the "direction" after the "verdict" and "severity" fields in the
ACL logs.
---
NEWS | 2 ++
controller/pinctrl.c | 4 ++-
lib/acl-log.c | 8 +++--
lib/acl-log.h | 3 +-
tests/ovn.at | 68 ++-
utilities/ovn-trace.c | 9 --
6 files changed, 73 insertions(+), 21 deletions(-)
diff --git a/NEWS b/NEWS
index 194557410b..a01820dfe6 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,7 @@
Post v21.12.0
-
+ - When configured to log packtes matching ACLs, log the direction (logical
+pipeline) too.
OVN v21.12.0 - 22 Dec 2021
--
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index fd0bccdb6c..e275b5e08a 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -3166,7 +3166,9 @@ process_packet_in(struct rconn *swconn, const struct
ofp_header *msg)
break;
case ACTION_OPCODE_LOG:
-handle_acl_log(, );
+handle_acl_log(, ,
+ pin.table_id < OFTABLE_LOG_EGRESS_PIPELINE
+ ? "IN" : "OUT");
break;
case ACTION_OPCODE_PUT_ND_RA_OPTS:
diff --git a/lib/acl-log.c b/lib/acl-log.c
index 220b6dc30e..9530dd7635 100644
--- a/lib/acl-log.c
+++ b/lib/acl-log.c
@@ -76,7 +76,8 @@ log_severity_from_string(const char *name)
}
void
-handle_acl_log(const struct flow *headers, struct ofpbuf *userdata)
+handle_acl_log(const struct flow *headers, struct ofpbuf *userdata,
+ const char *direction)
{
if (!VLOG_IS_INFO_ENABLED()) {
return;
@@ -94,9 +95,10 @@ handle_acl_log(const struct flow *headers, struct ofpbuf
*userdata)
struct ds ds = DS_EMPTY_INITIALIZER;
ds_put_cstr(, "name=");
json_string_escape(name_len ? name : "", );
-ds_put_format(, ", verdict=%s, severity=%s: ",
+ds_put_format(, ", verdict=%s, severity=%s, direction=%s: ",
log_verdict_to_string(lph->verdict),
- log_severity_to_string(lph->severity));
+ log_severity_to_string(lph->severity),
+ direction);
flow_format(, headers, NULL);
VLOG_INFO("%s", ds_cstr());
diff --git a/lib/acl-log.h b/lib/acl-log.h
index 4f23f790dd..da7fa2f02c 100644
--- a/lib/acl-log.h
+++ b/lib/acl-log.h
@@ -49,6 +49,7 @@ const char *log_verdict_to_string(uint8_t verdict);
const char *log_severity_to_string(uint8_t severity);
uint8_t log_severity_from_string(const char *name);
-void handle_acl_log(const struct flow *headers, struct ofpbuf *userdata);
+void handle_acl_log(const struct flow *headers, struct ofpbuf *userdata,
+const char *direction);
#endif /* lib/acl-log.h */
diff --git a/tests/ovn.at b/tests/ovn.at
index 957eb7850f..cfb9818ced 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8965,33 +8965,59 @@ ovn-nbctl lsp-set-addresses lp2 $lp2_mac
ovn-nbctl --wait=sb sync
wait_for_ports_up
-ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==80' drop
-ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000
'tcp.dst==81' drop
+ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==80' drop
+ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 from-lport 1000
'tcp.dst==81' drop
+
+ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==180' drop
+ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000
'tcp.dst==181' drop
+
+ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==82' allow
+ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 from-lport 1000
'tcp.dst==83' allow
ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==82' allow
ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 to-lport 1000
'tcp.dst==83' allow
+ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==84' allow-related
+ovn-nbctl --log acl-add lsw0 from-lport 1000 'tcp.dst==85' allow-related
+
ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==84' allow-related
ovn-nbctl --log acl-add lsw0 to-lport 1000 'tcp.dst==85' allow-related
-ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==86' reject
-ovn-nbctl --wait=hv --log --severity=alert --name=reject-flow acl-add lsw0
to-lport 1000 'tcp.dst==87' reject
+ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==86' reject
+ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 from-lport
1000 'tcp.dst==87' reject
+
+ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==186' reject
+ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 to-lport 1000
'tcp.dst==187' reject
+
+ovn-nbctl --wait=hv sync
ovn-sbctl dump-flows >
Re: [ovs-dev] [PATCH v2 12/18] tests: Wrap test-odp to also run python parsers
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > test-odp is used to parse datapath flow actions and matches within the > odp tests. Wrap calls to this tool in a python script that also parses > them using the python flow parsing library. > > Signed-off-by: Adrian Moreno Changes look good to me. Acked-by: Eelco Chaudron > --- > tests/automake.mk | 2 ++ > tests/odp.at | 36 ++-- > tests/ovs-test-dpparse.py | 70 +++ > 3 files changed, 90 insertions(+), 18 deletions(-) > create mode 100755 tests/ovs-test-dpparse.py > > diff --git a/tests/ovs-test-dpparse.py b/tests/ovs-test-dpparse.py > new file mode 100755 > index 0..5ccb426c1 > --- /dev/null > +++ b/tests/ovs-test-dpparse.py > @@ -0,0 +1,70 @@ > +#!/usr/bin/env python3 Same nit as in the previous patch about removing the full copyright notice. > +""" > +ovs-test-dpparse is just a wrapper around ovs-dpctl > +that also runs the python flow parsing utility to check that flows are > +parseable. > +""" > +import subprocess > +import sys > +import re ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 11/18] tests: wrap ovs-ofctl calls to test python parser
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > Some ovs-ofctl commands are used to parse or dump openflow flows, > specially in ofp-actions.at > > Use a wrapper around ovs-ofctl, called ovs-test-ofparse.py that, apart > from calling ovs-ofctl, also parses its output (or input, depending on > the command) to make sure the python flow parsing library can also parse > the flows. > > Signed-off-by: Adrian Moreno Changes look good to me, one small nit below. Acked-by: Eelco Chaudron > --- > tests/automake.mk | 3 ++ > tests/ofp-actions.at | 46 +-- > tests/ovs-test-ofparse.py | 94 +++ > 3 files changed, 120 insertions(+), 23 deletions(-) > create mode 100755 tests/ovs-test-ofparse.py > > diff --git a/tests/ovs-test-ofparse.py b/tests/ovs-test-ofparse.py > new file mode 100755 > index 0..bf578b6d5 > --- /dev/null > +++ b/tests/ovs-test-ofparse.py > @@ -0,0 +1,94 @@ > +#!/usr/bin/env python3 > + NIT: You removed the entire copyright header, I was only referring to the part below. But I’m fine with it. -+# Breaks lines read from stdin into groups using blank lines as -+# group separators, then sorts lines within the groups for -+# reproducibility. > +""" ovs-test-ofparse is just a wrapper around ovs-ofctl > +that also runs the python flow parsing utility to check that flows are > +parseable. > +""" > + > +import subprocess > +import sys > +import re > + > +from ovs.flows.ofp import OFPFlow > + ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 ovn] ovn-nbctl: add the capability to specify CoPP name
On 1/29/22 15:31, Lorenzo Bianconi wrote: > Introduce the capability to specify CoPP name in order to > reuse the same CoPP reference on multiple datapaths. > Introduce the following CoPP commands: > - ovn-nbctl copp-add > - ovn-nbctl copp-del > - ovn-nbctl copp-list > - ovn-nbctl ls-copp-add > - ovn-nbctl lr-copp-add > > Introduce name and external_ids columns in CoPP table. > > Reported-ad: https://bugzilla.redhat.com/show_bug.cgi?id=2040852 > Signed-off-by: Lorenzo Bianconi > --- Looks good to me, thanks! Acked-by: Dumitru Ceara ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 10/18] python: add a json encoder to flow fields
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > The json encoder can be used to convert Flows to json. > > Acked-by: Eelco Chaudron > Signed-off-by: Adrian Moreno > --- Looks good, repeating ACK :) Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 09/18] python: add flow filtering syntax
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > Based on pyparsing, create a very simple filtering syntax. > > It supports basic logic statements (and, &, or, ||, not, !), numerical > operations (<, >), equality (=, !=) and masking (~=). The latter is only nit: missing command before “and masking”, i.e. “operations (<, >), equality (=, !=), and masking (~=).” > supported in certain fields (IntMask, EthMask, IPMask). > > Masking operation is semantically equivalent to "includes", > therefore: > > ip_src ~= 192.168.1.1 > > means that ip_src field is either a host IP address equal to 192.168.1.1 > or an IPMask that includes it (e.g: 192.168.1.1/24). > > Signed-off-by: Adrian Moreno Looks good! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 08/18] python: add ovs datapath flow parsing
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > A ODPFlow is a Flow with the following sections: > ufid > info (e.g: bytes, packets, dp, etc) > match > actions > > Use a factory class ODPFlowFactory to cache the decoder objects. > > Only three datapath actions require special handling: > gre: because it has double parenthesis > geneve: because it supports many concatenated lists of options > nat: we reuse the decoder used for openflow actions > > Signed-off-by: Adrian Moreno Other than James’s comments this looks good to me. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 07/18] python: introduce OpenFlow Flow parsing
On 28 Jan 2022, at 17:04, Adrian Moreno wrote:
> Introduce OFPFlow class and all its decoders.
>
> Most of the decoders are generic (from decoders.py). Some have special
> syntax and need a specific implementation.
>
> Decoders for nat are moved to the common decoders.py because it's syntax
> is shared with other types of flows (e.g: dpif flows).
>
> Signed-off-by: Adrian Moreno
Some small comments below…
> ---
> python/automake.mk | 2 +
> python/ovs/flows/decoders.py | 108 +
> python/ovs/flows/ofp.py | 453 +++
> python/ovs/flows/ofp_act.py | 243 +++
> 4 files changed, 806 insertions(+)
> create mode 100644 python/ovs/flows/ofp.py
> create mode 100644 python/ovs/flows/ofp_act.py
>
> diff --git a/python/automake.mk b/python/automake.mk
> index b7debfbd9..7b6d6596f 100644
> --- a/python/automake.mk
> +++ b/python/automake.mk
> @@ -31,6 +31,8 @@ ovs_pyfiles = \
> python/ovs/flows/flow.py \
> python/ovs/flows/kv.py \
> python/ovs/flows/list.py \
> + python/ovs/flows/ofp.py \
> + python/ovs/flows/ofp_act.py \
> python/ovs/json.py \
> python/ovs/jsonrpc.py \
> python/ovs/ovsuuid.py \
> diff --git a/python/ovs/flows/decoders.py b/python/ovs/flows/decoders.py
> index 2f8e5bd0a..1462b0b9d 100644
> --- a/python/ovs/flows/decoders.py
> +++ b/python/ovs/flows/decoders.py
> @@ -6,6 +6,7 @@ object.
> """
>
> import netaddr
> +import re
>
>
> class Decoder(object):
> @@ -414,3 +415,110 @@ class IPMask(Decoder):
>
> def to_json(self):
> return str(self)
> +
> +
> +def decode_free_output(value):
> +"""The value of the output action can be found free, i.e: without the
> +'output' keyword. This decoder decodes its value when found this way."""
> +try:
> +return "output", {"port": int(value)}
> +except ValueError:
> +return "output", {"port": value.strip('"')}
> +
> +
> +ipv4 = r"(?:\d{1,3}.?){3}\d{1,3}"
> +ipv4_capture = r"({ipv4})".format(ipv4=ipv4)
> +"""
> +The following IPv6 regexp is a modified version of the one in:
> +https://community.helpsystems.com/forums/intermapper/miscellaneous-topics/5acc4fcf-fa83-e511-80cf-0050568460e4?_ga=2.113564423.1432958022.1523882681-2146416484.1523557976
> +
> +It matches all these types of ipv6 addresses:
> +fe80::::0204:61ff:fe9d:f156
> +fe80:0:0:0:204:61ff:fe9d:f156
> +fe80::204:61ff:fe9d:f156
> +fe80::::0204:61ff:254.157.241.86
> +fe80:0:0:0:0204:61ff:254.157.241.86
> +fe80::204:61ff:254.157.241.86
> +::1
> +2001::
> +fe80::
> +"""
> +ipv6 =
> r"(?:(?:(?:[0-9A-Fa-f]{1,4}:){7}(?:[0-9A-Fa-f]{1,4}|:))|(?:(?:[0-9A-Fa-f]{1,4}:){6}(?::[0-9A-Fa-f]{1,4}|(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){5}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,2})|:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(?:(?:[0-9A-Fa-f]{1,4}:){4}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,3})|(?:(?::[0-9A-Fa-f]{1,4})?:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){3}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,4})|(?:(?::[0-9A-Fa-f]{1,4}){0,2}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){2}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,5})|(?:(?::[0-9A-Fa-f]{1,4}){0,3}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?:(?:[0-9A-Fa-f]{1,4}:){1}(?:(?:(?::[0-9A-Fa-f]{1,4}){1,6})|(?:(?::[0-9A-Fa-f]{1,4}){0,4}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(?::(?:(?:(?::[0-9A-Fa-f]{1,4}){1,7})|(?:(?::[0-9A-Fa-f]{1,4}){0,5}:(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))"
> # noqa: E501
> +ipv6_capture = r"(?:\[*)?({ipv6})(?:\]*)?".format(ipv6=ipv6)
> +port_range = r":(\d+)(?:-(\d+))?"
> +ip_range_regexp = r"{ip_cap}(?:-{ip_cap})?(?:{port_range})?"
> +ipv4_port_regex = re.compile(
> +ip_range_regexp.format(ip_cap=ipv4_capture, port_range=port_range)
> +)
> +ipv6_port_regex = re.compile(
> +ip_range_regexp.format(ip_cap=ipv6_capture, port_range=port_range)
> +)
> +
> +
> +def decode_ip_port_range(value):
> +"""
> +Decodes an IP and port range:
> +{ip_start}-{ip-end}:{port_start}-{port_end}
> +
> +IPv6 addresses are surrounded by "[" and "]" if port ranges are also
> +present
> +
> +Returns the following dictionary:
> +{
> +"addrs": {
> +"start": {ip_start}
> +"end": {ip_end}
> +}
> +"ports": {
> +"start": {port_start},
> +"end": {port_end}
> +}
> +(the "ports" key might be omitted)
> +"""
> +if value.count(":") > 1:
> +match = ipv6_port_regex.match(value)
> +else:
> +match = ipv4_port_regex.match(value)
> +
> +ip_start =
Re: [ovs-dev] [PATCH v2 06/18] python: add flow base class
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > It simplifies the implementation of different types of flows by creating > the concept of Section (e.g: match, action) and automatic accessors for > all the provided Sections > > Signed-off-by: Adrian Moreno Other than the type James found, it looks good to me. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 05/18] build-aux: generate ofp field decoders
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > Based on meta-field information extracted by extract_ofp_fields, > autogenerate the right decoder to be used. > > Signed-off-by: Adrian Moreno Changes look good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 04/18] build-aux: split extract-ofp-fields
On 28 Jan 2022, at 17:04, Adrian Moreno wrote:
> In order to be able to reuse the core extraction logic, split the command
> in two parts. The core extraction logic is moved to python/build while
> the command that writes the different files out of the extracted field
> info is kept in build-aux.
>
> Signed-off-by: Adrian Moreno
Guess you missed one previous comment, rest looks good.
> ---
> build-aux/extract-ofp-fields | 706 -
> python/automake.mk | 1 +
> python/build/extract_ofp_fields.py | 386
> 3 files changed, 579 insertions(+), 514 deletions(-)
> create mode 100644 python/build/extract_ofp_fields.py
>
> diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
> index 8766995d9..efec59c25 100755
> --- a/build-aux/extract-ofp-fields
> +++ b/build-aux/extract-ofp-fields
> @@ -3,85 +3,23 @@
> import getopt
> import sys
> import os.path
> -import re
> import xml.dom.minidom
> import build.nroff
>
> -line = ""
> -
> -# Maps from user-friendly version number to its protocol encoding.
> -VERSION = {"1.0": 0x01,
> - "1.1": 0x02,
> - "1.2": 0x03,
> - "1.3": 0x04,
> - "1.4": 0x05,
> - "1.5": 0x06}
> -VERSION_REVERSE = dict((v,k) for k, v in VERSION.items())
> -
> -TYPES = {"u8": (1, False),
> - "be16": (2, False),
> - "be32": (4, False),
> - "MAC": (6, False),
> - "be64": (8, False),
> - "be128":(16, False),
> - "tunnelMD": (124, True)}
> -
> -FORMATTING = {"decimal":("MFS_DECIMAL", 1, 8),
> - "hexadecimal":("MFS_HEXADECIMAL", 1, 127),
> - "ct state": ("MFS_CT_STATE", 4, 4),
> - "Ethernet": ("MFS_ETHERNET", 6, 6),
> - "IPv4": ("MFS_IPV4", 4, 4),
> - "IPv6": ("MFS_IPV6",16, 16),
> - "OpenFlow 1.0 port": ("MFS_OFP_PORT", 2, 2),
> - "OpenFlow 1.1+ port": ("MFS_OFP_PORT_OXM", 4, 4),
> - "frag": ("MFS_FRAG", 1, 1),
> - "tunnel flags": ("MFS_TNL_FLAGS",2, 2),
> - "TCP flags": ("MFS_TCP_FLAGS",2, 2),
> - "packet type":("MFS_PACKET_TYPE", 4, 4)}
> -
> -PREREQS = {"none": "MFP_NONE",
> - "Ethernet": "MFP_ETHERNET",
> - "ARP": "MFP_ARP",
> - "VLAN VID": "MFP_VLAN_VID",
> - "IPv4": "MFP_IPV4",
> - "IPv6": "MFP_IPV6",
> - "IPv4/IPv6": "MFP_IP_ANY",
> - "NSH": "MFP_NSH",
> - "CT": "MFP_CT_VALID",
> - "MPLS": "MFP_MPLS",
> - "TCP": "MFP_TCP",
> - "UDP": "MFP_UDP",
> - "SCTP": "MFP_SCTP",
> - "ICMPv4": "MFP_ICMPV4",
> - "ICMPv6": "MFP_ICMPV6",
> - "ND": "MFP_ND",
> - "ND solicit": "MFP_ND_SOLICIT",
> - "ND advert": "MFP_ND_ADVERT"}
> -
> -# Maps a name prefix into an (experimenter ID, class) pair, so:
> -#
> -# - Standard OXM classes are written as (0, )
> -#
> -# - Experimenter OXM classes are written as (, 0x)
> -#
> -# If a name matches more than one prefix, the longest one is used.
> -OXM_CLASSES = {"NXM_OF_":(0, 0x, 'extension'),
> - "NXM_NX_":(0, 0x0001, 'extension'),
> - "NXOXM_NSH_": (0x005ad650, 0x, 'extension'),
> - "OXM_OF_":(0, 0x8000, 'standard'),
> - "OXM_OF_PKT_REG": (0, 0x8001, 'standard'),
> - "ONFOXM_ET_": (0x4f4e4600, 0x, 'standard'),
> - "ERICOXM_OF_":(0, 0x1000, 'extension'),
> -
> - # This is the experimenter OXM class for Nicira, which is the
> - # one that OVS would be using instead of NXM_OF_ and NXM_NX_
> - # if OVS didn't have those grandfathered in. It is currently
> - # used only to test support for experimenter OXM, since there
> - # are barely any real uses of experimenter OXM in the wild.
> - "NXOXM_ET_": (0x2320, 0x, 'extension')}
> +from build.extract_ofp_fields import (
> +extract_ofp_fields,
> +PREREQS,
> +OXM_CLASSES,
> +VERSION,
> +fatal,
> +n_errors,
> +)
> +
> +VERSION_REVERSE = dict((v, k) for k, v in VERSION.items())
> +
>
> def oxm_name_to_class(name):
> -prefix = ''
> +prefix = ""
> class_ = None
> for p, c in OXM_CLASSES.items():
> if name.startswith(p) and len(p) > len(prefix):
> @@ -92,267 +30,76 @@ def oxm_name_to_class(name):
>
> def is_standard_oxm(name):
> oxm_vendor, oxm_class, oxm_class_type = oxm_name_to_class(name)
> -return oxm_class_type == 'standard'
> -
> -
> -def decode_version_range(range):
> -if range
Re: [ovs-dev] [PATCH v2 03/18] python: add list parser
On 28 Jan 2022, at 17:04, Adrian Moreno wrote:
> Some openflow or dpif flows encode their arguments in lists, eg:
> "some_action(arg1,arg2,arg3)". In order to decode this in a way that can
> be then stored and queried, add ListParser and ListDecoders classes
> that parse lists into KeyValue instances.
>
> The ListParser / ListDecoders mechanism is quite similar to KVParser and
> KVDecoders. Since the "key" of the different KeyValue objects is now
> ommited, it has to be provided by ListDecoders.
>
> For example, take the openflow action "resubmit" that can be written as:
>
> resubmit([port],[table][,ct])
>
> Can be decoded by creating a ListDecoders instance such as:
>
> ListDecoders([
> ("port", decode_default),
> ("table", decode_int),
> ("ct", decode_flag),
> ])
>
> Naturally, the order of the decoders must be kept.
>
> Signed-off-by: Adrian Moreno
Assuming you will fix the double “to to”, you can add my ack to the next
version.
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 02/18] python: add mask, ip and eth decoders
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > Add more decoders that can be used by KVParser. > > For IPv4 and IPv6 addresses, create a new class that wraps > netaddr.IPAddress. > For Ethernet addresses, create a new class that wraps netaddr.EUI. > For Integers, create a new class that performs basic bitwise mask > comparisons > > Signed-off-by: Adrian Moreno > --- Assuming you will fix James’s comments, you can add my ack to the next version. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 01/18] python: add generic Key-Value parser
On 28 Jan 2022, at 17:04, Adrian Moreno wrote: > Most of ofproto and dpif flows are based on key-value pairs. These > key-value pairs can be represented in several ways, eg: key:value, > key=value, key(value). > > Add the following classes that allow parsing of key-value strings: > * KeyValue: holds a key-value pair > * KeyMetadata: holds some metadata associated with a KeyValue such as > the original key and value strings and their position in the global > string > * KVParser: is able to parse a string and extract it's key-value pairs > as KeyValue instances. Before creating the KeyValue instance it tries > to decode the value via the KVDecoders > * KVDecoders holds a number of decoders that KVParser can use to decode > key-value pairs. It accepts a dictionary of keys and callables to > allow users to specify what decoder (i.e: callable) to use for each > key > > Also, flake8 seems to be incorrectly reporting an error (E203) in: > "slice[index + offset : index + offset]" which is PEP8 compliant. So, > ignore this error. > > Signed-off-by: Adrian Moreno > --- Assuming you will fix James’s comments, you can add my ack to the next version. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex fuzzy test.
On 2/11/22 11:49, Stokes, Ian wrote: >>> -Original Message- >>> From: dev On Behalf Of Kumar Amber >>> Sent: Wednesday 9 February 2022 09:50 >>> To: ovs-dev@openvswitch.org >>> Cc: Amber, Kumar ; david.march...@redhat.com >>> Subject: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex fuzzy >> test. >>> >>> Some specific warning are seen on various systems >>> which may not be visible on others but good to add >>> such logs to test to avoid test-case failure. >>> >>> Thw warning only effects the fuzzy tests due to >>> more than 1000+ flows being offloading simultanously. >>> >>> Wilcarding flow count number as for different systems >>> under test the number could vary in the warning log. >>> >>> Suggested-by: Eelco Chaudron >>> Signed-off-by: Kumar Amber >>> >> >> I didn't see the issue that this patch fixes initially. However, if I make >> the below >> change, I can see the error during the test. >> >> Change: >> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at >> index 9384cf7f0..165b68f0e 100644 >> --- a/tests/system-dpdk.at >> +++ b/tests/system-dpdk.at >> @@ -256,6 +256,8 @@ AT_KEYWORDS([dpdk]) >> AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], []) >> AT_CHECK([$PYTHON3 $srcdir/mfex_fuzzy.py $srcdir], [], [stdout]) >> OVS_DPDK_START() >> +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . other_config:flow- >> limit=13500]) >> + >> >> dnl Add userspace bridge and attach it to OVS >> AT_CHECK([ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev]) >> >> Error: >>> 2022-02-10T09:42:11.350Z|6|ofproto_dpif_upcall(pmd- >> c21/id:101)|WARN|upcall: datapath reached the dynamic limit of 13500 flows. >> >> Which causes a fail: >> 7. system-dpdk.at:254: FAILED (system-dpdk.at:280) >> >> Applying this patch while keeping the above diff to generate the flow limit >> error >> results in the test passing. I can see the "reached the dynamic limit" >> warning in >> the ovs-vswitchd.log but it doesn't cause a test failure. >> >> Finding the value of "13500" as a flow-limit took some experimentation and >> would probably vary based on platform. >> >> With that, and looking at the .at changes, LGTM. >> >> Acked-by: Cian Ferriter > > Thanks for testing and reviewing folks. > > So does it make sense to apply this to both 2.17 and 2.16 also? Since it's a test-only change, and a fix actually, it should be OK to backport. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/13] Fix undefined behavior in loop macros
On 2/11/22 12:36, Adrian Moreno wrote:
>
>
> On 2/9/22 16:46, Dumitru Ceara wrote:
>> On 2/9/22 16:23, Dumitru Ceara wrote:
>>> However, that's not the only issue with sparse. It seems on specific
>>> distributions (e.g., on my Fedora 34 test machine) sparse fails to use
>>> the right headers. I made it work with this change, although I'm not
>>> sure this is the best way of doing things:
>>>
>>> diff --git a/acinclude.m4 b/acinclude.m4
>>> index 0c360fd1ef73..f704bf36cdfe 100644
>>> --- a/acinclude.m4
>>> +++ b/acinclude.m4
>>> @@ -1424,7 +1424,7 @@ AC_DEFUN([OVS_ENABLE_SPARSE],
>>> : ${SPARSE=sparse}
>>> AC_SUBST([SPARSE])
>>> AC_CONFIG_COMMANDS_PRE(
>>> - [CC='$(if $(C:0=),env REAL_CC="'"$CC"'" CHECK="$(SPARSE)
>>> $(SPARSE_WERROR) -I $(top_srcdir)/include/sparse $(SPARSEFLAGS)
>>> $(SPARSE_EXTRA_INCLUDES) " cgcc $(CGCCFLAGS),'"$CC"')'])
>>> + [CC='$(if $(C:0=),env REAL_CC="'"$CC"'" CHECK="$(SPARSE)
>>> $(SPARSE_WERROR) -I $(top_srcdir)/include/sparse -I
>>> $(top_srcdir)/include $(SPARSEFLAGS) $(SPARSE_EXTRA_INCLUDES) " cgcc
>>> $(CGCCFLAGS),'"$CC"')'])
>>> AC_ARG_ENABLE(
>>> [sparse],
>>> ---
>>
>> Turns out this was kind of my fault because I had OVS headers installed
>> on the test system . Those headers obviously had the old macro
>> definitions. So we need to make sure we use the ones from the tree
>> instead.
>>
>
> Do you mean we don't need to bump the sparse version, only ensure the
> tree headers are included?
We need both, sorry for the confusion.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/13] Fix undefined behavior in loop macros
On 2/9/22 16:46, Dumitru Ceara wrote:
On 2/9/22 16:23, Dumitru Ceara wrote:
However, that's not the only issue with sparse. It seems on specific
distributions (e.g., on my Fedora 34 test machine) sparse fails to use
the right headers. I made it work with this change, although I'm not
sure this is the best way of doing things:
diff --git a/acinclude.m4 b/acinclude.m4
index 0c360fd1ef73..f704bf36cdfe 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -1424,7 +1424,7 @@ AC_DEFUN([OVS_ENABLE_SPARSE],
: ${SPARSE=sparse}
AC_SUBST([SPARSE])
AC_CONFIG_COMMANDS_PRE(
- [CC='$(if $(C:0=),env REAL_CC="'"$CC"'" CHECK="$(SPARSE) $(SPARSE_WERROR) -I
$(top_srcdir)/include/sparse $(SPARSEFLAGS) $(SPARSE_EXTRA_INCLUDES) " cgcc $(CGCCFLAGS),'"$CC"')'])
+ [CC='$(if $(C:0=),env REAL_CC="'"$CC"'" CHECK="$(SPARSE) $(SPARSE_WERROR) -I
$(top_srcdir)/include/sparse -I $(top_srcdir)/include $(SPARSEFLAGS) $(SPARSE_EXTRA_INCLUDES) " cgcc
$(CGCCFLAGS),'"$CC"')'])
AC_ARG_ENABLE(
[sparse],
---
Turns out this was kind of my fault because I had OVS headers installed
on the test system . Those headers obviously had the old macro
definitions. So we need to make sure we use the ones from the tree instead.
Do you mean we don't need to bump the sparse version, only ensure the tree
headers are included?
Thanks
--
Adrián Moreno
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex fuzzy test.
> -Original Message- > From: Stokes, Ian > Sent: Friday 11 February 2022 10:49 > To: Ferriter, Cian ; Amber, Kumar > ; ovs- > d...@openvswitch.org > Cc: Amber, Kumar ; david.march...@redhat.com > Subject: RE: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex > fuzzy test. > > > > -Original Message- > > > From: dev On Behalf Of Kumar Amber > > > Sent: Wednesday 9 February 2022 09:50 > > > To: ovs-dev@openvswitch.org > > > Cc: Amber, Kumar ; david.march...@redhat.com > > > Subject: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex > > > fuzzy > > test. > > > > > > Some specific warning are seen on various systems > > > which may not be visible on others but good to add > > > such logs to test to avoid test-case failure. > > > > > > Thw warning only effects the fuzzy tests due to > > > more than 1000+ flows being offloading simultanously. > > > > > > Wilcarding flow count number as for different systems > > > under test the number could vary in the warning log. > > > > > > Suggested-by: Eelco Chaudron > > > Signed-off-by: Kumar Amber > > > > > > > I didn't see the issue that this patch fixes initially. However, if I make > > the below > > change, I can see the error during the test. > > > > Change: > > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at > > index 9384cf7f0..165b68f0e 100644 > > --- a/tests/system-dpdk.at > > +++ b/tests/system-dpdk.at > > @@ -256,6 +256,8 @@ AT_KEYWORDS([dpdk]) > > AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], []) > > AT_CHECK([$PYTHON3 $srcdir/mfex_fuzzy.py $srcdir], [], [stdout]) > > OVS_DPDK_START() > > +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . other_config:flow- > > limit=13500]) > > + > > > > dnl Add userspace bridge and attach it to OVS > > AT_CHECK([ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev]) > > > > Error: > > > 2022-02-10T09:42:11.350Z|6|ofproto_dpif_upcall(pmd- > > c21/id:101)|WARN|upcall: datapath reached the dynamic limit of 13500 flows. > > > > Which causes a fail: > > 7. system-dpdk.at:254: FAILED (system-dpdk.at:280) > > > > Applying this patch while keeping the above diff to generate the flow limit > > error > > results in the test passing. I can see the "reached the dynamic limit" > > warning in > > the ovs-vswitchd.log but it doesn't cause a test failure. > > > > Finding the value of "13500" as a flow-limit took some experimentation and > > would probably vary based on platform. > > > > With that, and looking at the .at changes, LGTM. > > > > Acked-by: Cian Ferriter > > Thanks for testing and reviewing folks. > > So does it make sense to apply this to both 2.17 and 2.16 also? > > Thanks > ian I think it makes sense to backport this patch. Thanks, Cian ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex fuzzy test.
> > -Original Message- > > From: dev On Behalf Of Kumar Amber > > Sent: Wednesday 9 February 2022 09:50 > > To: ovs-dev@openvswitch.org > > Cc: Amber, Kumar ; david.march...@redhat.com > > Subject: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex fuzzy > test. > > > > Some specific warning are seen on various systems > > which may not be visible on others but good to add > > such logs to test to avoid test-case failure. > > > > Thw warning only effects the fuzzy tests due to > > more than 1000+ flows being offloading simultanously. > > > > Wilcarding flow count number as for different systems > > under test the number could vary in the warning log. > > > > Suggested-by: Eelco Chaudron > > Signed-off-by: Kumar Amber > > > > I didn't see the issue that this patch fixes initially. However, if I make > the below > change, I can see the error during the test. > > Change: > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at > index 9384cf7f0..165b68f0e 100644 > --- a/tests/system-dpdk.at > +++ b/tests/system-dpdk.at > @@ -256,6 +256,8 @@ AT_KEYWORDS([dpdk]) > AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], []) > AT_CHECK([$PYTHON3 $srcdir/mfex_fuzzy.py $srcdir], [], [stdout]) > OVS_DPDK_START() > +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . other_config:flow- > limit=13500]) > + > > dnl Add userspace bridge and attach it to OVS > AT_CHECK([ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev]) > > Error: > > 2022-02-10T09:42:11.350Z|6|ofproto_dpif_upcall(pmd- > c21/id:101)|WARN|upcall: datapath reached the dynamic limit of 13500 flows. > > Which causes a fail: > 7. system-dpdk.at:254: FAILED (system-dpdk.at:280) > > Applying this patch while keeping the above diff to generate the flow limit > error > results in the test passing. I can see the "reached the dynamic limit" > warning in > the ovs-vswitchd.log but it doesn't cause a test failure. > > Finding the value of "13500" as a flow-limit took some experimentation and > would probably vary based on platform. > > With that, and looking at the .at changes, LGTM. > > Acked-by: Cian Ferriter Thanks for testing and reviewing folks. So does it make sense to apply this to both 2.17 and 2.16 also? Thanks ian > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On 2/11/22 00:54, Numan Siddique wrote: > On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara wrote: >> >> We still need to try to ovsdb_idl_loop_commit_and_wait() on instances >> that are in standby mode. That's because they need to try to take the >> lock. But if they're in standby they won't actually build a transaction >> json and will return early because they don't own the SB lock. >> >> That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we >> shouldn't act on it. >> >> Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear() >> should be called only on active instances. The standby or paused ones >> will get the incremental updates when they become active. Otherwise we >> would be forced to perform a full recompute when the instance becomes >> active. > > Hi Dumitru, > Hi Numan, Thanks for the review! > I've a question on the track clear being moved out of the standby instances. > To ensure correctness, I suppose it's better to trigger a full recompute > when a > standby instance becomes active. What do you think? > I might be wrong but I don't think that's necessary. It may also be quite costly as full recomputes can take quite long. > Also lets say CMS does the below operations > - Add a logical switch S1 > - Add a logical port p1 in S1 > - Add a logical port p2 in S1 > - Delete logical port p2 > - Delete a logical switch. > > With this patch since we are not clearing the tracking information, > how does ovn-northd > process the tracked changes when it becomes active ? When ovn-northd becomes active, from a Northbound database perspective, there were no changes; that is, S1 didn't exist when it was last active and it doesn't exist now either. So, there should be no NB change to process. Accumulating tracked changes without calling clear() on the standby has exactly this effect. >From a Southbound database perspective there are two cases: a. The former active northd processed some (but not all) of the NB changes and executed their corresponding SB transactions. In this case, the standby northd also receives update messages for the SB records that were changed. The standby northd tracks these changes. When the standby northd becomes active it will: - determine that NB state didn't change - SB state changed and needs to be reconciled (today we do this with the help of a NULL change handler for SB_* tables which will trigger a full recompute). b. The former active northd processed all of the NB changes and executed their corresponding SB transactions. In this case, the final state of the NB and SB databases should be equivalent to their initial states. NB/SB changes will be accumulated by the change tracking mechanism on the standby resulting in empty tracked changes lists. This is fine because the new active northd doesn't need to do anything, the DB contents are already consistent. c. The former active northd processed none of the NB changes yet. This is very similar to case "b" above, the new active northd doesn't need to change anything in the NB/SB and it won't do that either. > > Thanks > Numan > Thanks, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
