Re: [ovs-dev] [PATCH] vconn: Count vconn_sent regardless of log level.
On Thu, Jan 04, 2024 at 03:16:59PM +0100, Maximets wrote:
> On 1/4/24 14:53, Cheng Li wrote:
> > vconn_sent counter is supposed to increase each time send() is
> > called, no matter if the vconn log debug is on or off.
>
> Good catch! But I think the intention was fr it to be increased
> each time we successfully sent something, not when the function
> was called. This will match the behavior of the vconn_received
> counter. i.e. we should probably put the counter under if (!retval).
>
> What do you think?
Make sense. I will send a v2.
>
> Best regards, Ilya Maximets.
>
> >
> > Signed-off-by: Cheng Li
> > ---
> > lib/vconn.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vconn.c b/lib/vconn.c
> > index b556762..2513b03 100644
> > --- a/lib/vconn.c
> > +++ b/lib/vconn.c
> > @@ -681,8 +681,8 @@ do_send(struct vconn *vconn, struct ofpbuf *msg)
> > ovs_assert(msg->size >= sizeof(struct ofp_header));
> >
> > ofpmsg_update_length(msg);
> > +COVERAGE_INC(vconn_sent);
> > if (!VLOG_IS_DBG_ENABLED()) {
> > -COVERAGE_INC(vconn_sent);
> > retval = (vconn->vclass->send)(vconn, msg);
> > } else {
> > char *s = ofp_to_string(msg->data, msg->size, NULL, NULL, 1);
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v3 ovn] ovn: add geneve PMTUD support
On Fri, Dec 22, 2023 at 11:27 AM Lorenzo Bianconi
wrote:
>
> Introduce specif flows for E/W ICMPv{4,6} packets if tunnelled packets
> do not fit path MTU. This patch enable PMTUD for East/West Geneve traffic.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2241711
> Signed-off-by: Lorenzo Bianconi
Hi Lorenzo,
Thanks for the patch. Please see below for a few comments.
> ---
> Changes since v2:
> - the icmp error forwarding for n/s traffic
> - add vxlan tests
> - merge IPv6 test cases
> Changes since v1:
> - add fix for vxlan and stt tunnels
> ---
> NEWS| 1 +
> controller/physical.c | 31 +++-
> northd/northd.c | 72 +
> northd/ovn-northd.8.xml | 29
> tests/multinode.at | 348 +++-
> tests/ovn-northd.at | 21 +++
> 6 files changed, 499 insertions(+), 3 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index e10fb79dd..acb3b854f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,7 @@ Post v23.09.0
> connection method and doesn't require additional probing.
> external_ids:ovn-openflow-probe-interval configuration option for
> ovn-controller no longer matters and is ignored.
> + - Enable PMTU discovery on geneve tunnels for E/W traffic.
>
> OVN v23.09.0 - 15 Sep 2023
> --
> diff --git a/controller/physical.c b/controller/physical.c
> index ba88e1d8b..78cde3e2a 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -2440,9 +2440,36 @@ physical_run(struct physical_ctx *p_ctx,
> OVS_NOT_REACHED();
> }
>
> -put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
> -
> +struct ofpbuf *tunnel_ofpacts = ofpbuf_clone(&ofpacts);
> +put_resubmit(OFTABLE_LOCAL_OUTPUT, tunnel_ofpacts);
> ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, 0, &match,
> +tunnel_ofpacts, hc_uuid);
> +ofpbuf_delete(tunnel_ofpacts);
> +
> +/* Add specif flows for E/W ICMPv{4,6} packets if tunnelled packets
> do not
> + * fit path MTU.
> + */
> +put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts);
> +
> +/* IPv4 */
> +match_init_catchall(&match);
> +match_set_in_port(&match, tun->ofport);
> +match_set_dl_type(&match, htons(ETH_TYPE_IP));
> +match_set_nw_proto(&match, IPPROTO_ICMP);
> +match_set_icmp_type(&match, 3);
> +match_set_icmp_code(&match, 4);
> +
> +ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 120, 0, &match,
> +&ofpacts, hc_uuid);
> +/* IPv6 */
> +match_init_catchall(&match);
> +match_set_in_port(&match, tun->ofport);
> +match_set_dl_type(&match, htons(ETH_TYPE_IPV6));
> +match_set_nw_proto(&match, IPPROTO_ICMPV6);
> +match_set_icmp_type(&match, 2);
> +match_set_icmp_code(&match, 0);
> +
> +ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 120, 0, &match,
> &ofpacts, hc_uuid);
> }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 617f292fe..a020f2097 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -12794,6 +12794,75 @@ build_lrouter_force_snat_flows(struct hmap *lflows,
> struct ovn_datapath *od,
> ds_destroy(&actions);
> }
>
> +/* Following flows are used to manage traffic redirected by the kernel
> + * (e.g. ICMP errors packets) that enter the cluster from the geneve ports
> + */
> +static void
> +build_lrouter_icmp_packet_toobig_admin_flows(
> +struct ovn_port *op, struct hmap *lflows,
> +struct ds *match, struct ds *actions)
> +{
> +ovs_assert(op->nbrp);
> +
> +if (is_l3dgw_port(op)) {
> +ds_clear(match);
> +ds_put_format(match,
> + "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
> + " (ip6 && icmp6.type == 2 && icmp6.code == 0)) && "
> + "eth.dst == %s && !is_chassis_resident(%s)",
> + op->nbrp->mac, op->cr_port->json_key);
> +ds_clear(actions);
> +ds_put_format(actions, "outport = inport; inport = %s; next;",
> + op->json_key);
> +ovn_lflow_add(lflows, op->od, S_ROUTER_IN_ADMISSION, 120,
> + ds_cstr(match), ds_cstr(actions));
> +}
> +
> +/* default flow */
> +ovn_lflow_add(lflows, op->od, S_ROUTER_IN_ADMISSION, 110,
> + "(ip4 && icmp4.type == 3 && icmp4.code == 4) || "
> + "(ip6 && icmp6.type == 2 && icmp6.code == 0)", "next; ");
> +}
> +
I don't think there is a need for default flow. If I understand
correctly, we are trying to handle
the scenario when the kernel generates the icmp needs frag error
packet. For the normal case i.e icmp
needs a frag error packet not generated by the kernel, it should
continue the normal flow.
> +static void
> +build_lswitch_icmp_packet_toobig_admin_flows(
> +struct ovn_p
Re: [ovs-dev] [PATCH 20/22] ovsdb-server: Allow user-provided config files.
On Wed, Dec 13, 2023 at 7:05 PM Ilya Maximets wrote: > -/* Clears and replaces 'remotes' and 'dbnames' by a configuration read from > - * 'config_file', which must have been previously written by save_config(). > */ > -static void > +/* Clears and replaces 'remotes' and 'db_conf' by a configuration read from > + * 'config_file', which must have been previously written by save_config() > + * or provided by the user with --config-file. > + * Returns 'true', if parsing was successful, 'false' otherwise. */ > +static bool > load_config(FILE *config_file, struct shash *remotes, > struct shash *db_conf, char **sync_from, > char **sync_exclude, bool *is_backup) > @@ -2890,17 +3117,34 @@ load_config(FILE *config_file, struct shash *remotes, > struct json *json; I'm wondering if having an argument for everything we parse out of a config file might get a little unwieldy down the road. Say we add configuration of SSL stuff, etc. Maybe it's something we could modify as it becomes an issue, but it might be nice to have something for config options that is similar to what we have for registering unixctl commands or struct ctl_command_syntax. Documentation could be added as part of the registration/definition of the config option, there could be a .get() that parses the values out of the json, and a .run() that gets called after all of the parsing is shown to succeed. Terry ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] python: idl: Handle monitor_canceled
On Fri, Jan 5, 2024 at 9:56 AM Simon Horman wrote:
>
> On Mon, Dec 18, 2023 at 05:31:24PM -0600, Terry Wilson wrote:
> > Currently python-ovs claims to be "db change aware" but does not
> > parse the "monitor_canceled" notification. Transactions can continue
> > being made, but the monitor updates will not be sent. This handles
> > monitor_cancel similarly to how ovsdb-cs currently does.
> >
> > Signed-off-by: Terry Wilson
>
> Hi Terry,
>
> is it possible to add a test to tests/ovsdb-idl.at for this feature?
It might be, but it seems like it'd be a bit of work and I'm not sure
if I'll have the time to look at it for a while. I'm just trying to
bring the functionality up to what the C IDL has and from what I can
tell this feature isn't tested in the C IDL either. What I'm doing to
manually test is to run this:
import logging
import ovs
import ovsdbapp.schema.ovn_northbound.impl_idl as nb_idl
from ovsdbapp.backend.ovs_idl import connection, vlog
print(f"Using OVS {ovs.__file__}")
logging.basicConfig(level=logging.DEBUG)
vlog.use_python_logger()
LOG = logging.getLogger(__name__)
remote = "unix:///home/twilson/src/ovn/tutorial/sandbox/nb1.ovsdb"
def get_idl():
"""Connection getter."""
idl = connection.OvsdbIdl.from_server(remote, "OVN_Northbound",
leader_only=False)
return nb_idl.OvnNbApiIdlImpl(connection.Connection(idl, 100))
idl = get_idl()
LOG.info(f"monitor_id: {str(idl.idl.uuid)}")
LOG.info(f"server_monitor_id: {str(idl.idl.server_monitor_uuid)}")
input(" Press Enter ")
idl.ls_add("test").execute(check_error=True)
and then in another window running:
ovsdb-client convert
unix:/home/twilson/src/ovn/tutorial/sandbox/nb1.ovsdb
/home/twilson/src/ovn/ovn-nb.ovsschema
and then pressing enter in the original window. Without the patch,
after running the ovsdb-client convert, you get:
DEBUG:ovsdbapp.backend.ovs_idl.vlog:unix:///home/twilson/src/ovn/tutorial/sandbox/nb1.ovsdb:
received unexpected notification message
and then ovsdbapp starts throwing exceptions. With the patch, things
work as one would expect.
The problem with testing is that the client connection needs to be
active during the monitor_canceled that happens when ovsdb-client
convert is called. The tests in ovsdb-idl.at use ovsdb-client transact
for doing everything, so it's not easy to do the convert between
connection and transaction execution.
Maybe something could be added test test-ovsdb.py stuff.
Terry
Terry
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] lib/backtrace: Fix error in log_backtrace() documentation.
The documentation for log_backtrace() states the backtrace is
logged at DEBUG level, while in reality it is logged at ERROR
level.
Fixes: d0b99d38edab ("backtrace: Add log_backtrace()")
Signed-off-by: Frode Nordahl
---
lib/backtrace.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/backtrace.h b/lib/backtrace.h
index 9ccafd6d4..a2506da5f 100644
--- a/lib/backtrace.h
+++ b/lib/backtrace.h
@@ -26,7 +26,7 @@
#endif
/* log_backtrace() will save the backtrace of a running program
- * into the log at the DEBUG level.
+ * into the log at the ERROR level.
*
* To use it, insert the following code to where backtrace is
* desired:
--
2.34.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 16/16] northd: Add northd change handler for sync_to_sb_lb node.
On Tue, Dec 19, 2023 at 2:37 AM Han Zhou wrote:
>
> On Mon, Nov 27, 2023 at 6:39 PM wrote:
> >
> > From: Numan Siddique
> >
> > Any changes to northd engine node due to load balancers
> > are now handled in 'sync_to_sb_lb' node to sync the changed
> > load balancers to SB load balancers.
> >
> > The logic to sync the SB load balancers is changed a bit and it
> > now mimics the SB lflow sync.
> >
> > Below are the scale testing results done with all the patches applied
> > in this series using ovn-heater. The test ran the scenario -
> > ocp-500-density-heavy.yml [1].
> >
> > The resuts are:
> >
> >
> ---
> > Min (s) Median (s) 90%ile (s)
> 99%ile (s) Max (s) Mean (s)Total (s) Count
> Failed
> >
> ---
> > Iteration Total 0.1368831.1290161.192001
> 1.2041671.2127280.66501783.127099 125 0
> > Namespace.add_ports 0.0052160.0057360.007034
> 0.0154860.0189780.0062110.776373125 0
> > WorkerNode.bind_port0.0350300.0460820.052469
> 0.0582930.0603110.04597311.493259 250 0
> > WorkerNode.ping_port0.0050570.0067271.047692
> 1.0692531.0713360.26689666.724094 250 0
> >
> ---
> >
> > The results with the present main [2] are:
> >
> >
> ---
> > Min (s) Median (s) 90%ile (s)
> 99%ile (s) Max (s) Mean (s)Total (s) Count
> Failed
> >
> ---
> > Iteration Total 0.1354912.2238053.311270
> 3.3390783.3453461.729172216.146495 125 0
> > Namespace.add_ports 0.0053800.0057440.006819
> 0.0187730.0208000.0062920.786532125 0
> > WorkerNode.bind_port0.0341790.0460550.053488
> 0.0588010.0710430.04611711.529311 250 0
> > WorkerNode.ping_port0.0049560.0069523.086952
> 3.1917433.1928070.791544197.886026 250 0
> >
> ---
> >
> > [1] -
> https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml
> > [2] - 2a12cda890a7("controller, northd: Wait for cleanup before replying
> to exit")
> >
> > Signed-off-by: Numan Siddique
> > ---
> > northd/en-sync-sb.c | 445 +--
> > northd/inc-proc-northd.c | 1 +
> > northd/lflow-mgr.c | 196 ++---
> > northd/lflow-mgr.h | 23 +-
> > northd/northd.c | 238 -
> > northd/northd.h | 6 -
> > tests/ovn-northd.at | 103 +
> > 7 files changed, 585 insertions(+), 427 deletions(-)
> >
> > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> > index 73b30272c4..62c5dbd20f 100644
> > --- a/northd/en-sync-sb.c
> > +++ b/northd/en-sync-sb.c
> > @@ -30,6 +30,7 @@
> > #include "lib/ovn-nb-idl.h"
> > #include "lib/ovn-sb-idl.h"
> > #include "lib/ovn-util.h"
> > +#include "lflow-mgr.h"
> > #include "northd.h"
> >
> > #include "openvswitch/vlog.h"
> > @@ -53,6 +54,38 @@ static void build_port_group_address_set(const struct
> nbrec_port_group *,
> > struct svec *ipv4_addrs,
> > struct svec *ipv6_addrs);
> >
> > +struct sb_lb_table;
> > +struct sb_lb_record;
> > +static void sb_lb_table_init(struct sb_lb_table *);
> > +static void sb_lb_table_clear(struct sb_lb_table *);
> > +static struct sb_lb_record *sb_lb_table_find(struct hmap *sb_lbs,
> > + const struct uuid *);
> > +static void sb_lb_table_build_and_sync(struct sb_lb_table *,
> > +struct ovsdb_idl_txn *ovnsb_txn,
> > +const struct sbrec_load_balancer_table *,
> > +const struct
> sbrec_logical_dp_group_table *,
> > +str
Re: [ovs-dev] [PATCH v5] system-dpdk: Test with mlx5 devices.
On 22/11/2023 16:09, David Marchand wrote: > The DPDK unit test only runs if vfio or igb_uio kernel modules are loaded: > on systems with only mlx5, this test is always skipped. > > Besides, the test tries to grab the first device listed by dpdk-devbind.py, > regardless of the PCI device status regarding kmod binding. > > Remove dependency on this DPDK script and use a minimal script that > reads PCI sysfs. > > This script is not perfect, as one can imagine PCI devices bound to > vfio-pci for virtual machines. > Plus, this script only tries to take over vfio-pci devices. mlx5 devices > can't be taken over blindly as it could mean losing connectivity to the > machine if the netdev was in use for this system. > > For those two reasons, add a new environment variable DPDK_PCI_ADDR for > testers to select the PCI device of their liking. > For consistency and grep, the temporary file PCI_ADDR is renamed > to DPDK_PCI_ADDR. > > Reviewed-by: Maxime Coquelin > Acked-by: Eelco Chaudron > Signed-off-by: David Marchand > --- > Changes since v4: > - separated from the original series, > - rebased, > - dropped mlx5 devices from the discovery script, > - documented DPDK_PCI_ADDR env variable, > > Changes since v3: > - fixed nit from Maxime, > > Changes since v2: > - sorted logs alphabetically, > > --- > Documentation/topics/testing.rst | 11 ++--- > tests/automake.mk| 1 + > tests/system-dpdk-find-device.py | 39 > tests/system-dpdk-macros.at | 10 ++-- > tests/system-dpdk.at | 14 ++-- > 5 files changed, 57 insertions(+), 18 deletions(-) > create mode 100755 tests/system-dpdk-find-device.py Tested with a combination of mlx5 and intel NICs with/without DPDK_PCI_ADDR and the correct NIC is selected. Acked-by: Kevin Traynor ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] python: idl: Handle monitor_canceled
On Mon, Dec 18, 2023 at 05:31:24PM -0600, Terry Wilson wrote: > Currently python-ovs claims to be "db change aware" but does not > parse the "monitor_canceled" notification. Transactions can continue > being made, but the monitor updates will not be sent. This handles > monitor_cancel similarly to how ovsdb-cs currently does. > > Signed-off-by: Terry Wilson Hi Terry, is it possible to add a test to tests/ovsdb-idl.at for this feature? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 8/8] python: ovs: flow: Add meter_id to controller.
On Fri, Jan 05, 2024 at 12:47:00PM +0100, Adrian Moreno wrote: > Add missing option to controller action. > > Signed-off-by: Adrian Moreno Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 7/8] python: ovs: flow: Make check_pkt_len action a list.
On Fri, Jan 05, 2024 at 12:46:59PM +0100, Adrian Moreno wrote: > In general, most actions must be lists since the keys can be repeated. > > Signed-off-by: Adrian Moreno Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 6/8] python: ovs: flow: Add idle_age to openflow flows.
On Fri, Jan 05, 2024 at 12:46:58PM +0100, Adrian Moreno wrote: > Add missing key. > > Signed-off-by: Adrian Moreno Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 5/8] python: tests: Refactor test_odp section testing.
On Fri, Jan 05, 2024 at 12:46:57PM +0100, Adrian Moreno wrote: > Avoid code duplication by moving the section testing code to its own > function. > > Signed-off-by: Adrian Moreno Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/5] ovsdb: transaction: Calculate added/removed from diff.
On Fri, Jan 5, 2024 at 9:40 AM Ilya Maximets wrote:
>
> On 1/1/24 07:37, Mike Pattrick wrote:
> > On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets wrote:
> >>
> >> In case the difference between 'old' and 'new' rows is readily
> >> available, it can be used to construct added/removed datums
> >> instead. Diffs are typically much smaller than the column
> >> itself. This change more than doubles the performance of a
> >> transaction replay.
> >>
> >> For example, with this change applied, initial read of OVSDB
> >> file containing 136K small transactions for large OVN port
> >> groups and address sets on my laptop takes 11 seconds vs 24
> >> seconds without.
> >>
> >> Signed-off-by: Ilya Maximets
> >> ---
> >> lib/ovsdb-data.c| 26 ++
> >> lib/ovsdb-data.h| 1 +
> >> ovsdb/transaction.c | 15 ---
> >> 3 files changed, 39 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
> >> index f18f74298..f0d7bee23 100644
> >> --- a/lib/ovsdb-data.c
> >> +++ b/lib/ovsdb-data.c
> >> @@ -2238,6 +2238,8 @@ ovsdb_symbol_table_insert(struct ovsdb_symbol_table
> >> *symtab,
> >> /* APIs for Generating and apply diffs. */
> >>
> >> /* Find what needs to be added to and removed from 'old' to construct
> >> 'new'.
> >> + * If the optional 'diff' is porvided, it can be used to speed up
> >> processing,
> >
> > Provided
> >
> >> + * in case it is smaller than the original 'old' and 'new'.
> >> *
> >> * The 'added' and 'removed' datums are always safe; the orders of keys
> >> are
> >> * maintained since they are added in order. */
> >> @@ -2246,6 +2248,7 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added,
> >>struct ovsdb_datum *removed,
> >>const struct ovsdb_datum *old,
> >>const struct ovsdb_datum *new,
> >> + const struct ovsdb_datum *diff,
> >>const struct ovsdb_type *type)
> >> {
> >> size_t oi, ni;
> >> @@ -2258,6 +2261,29 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added,
> >> return;
> >> }
> >>
> >> +/* Use diff, if provided, unless it's comparable in size. */
> >> +if (diff && diff->n * 2 < old->n + new->n) {
> >
> > I guess the motivation for the size check is that the old algorithm is
> > O(n) and the diff algorithm is O(n lg n)? If so it may be worthwhile
> > to note that in the comment, it wasn't initially clear to me. Or am I
> > misunderstanding this?
>
> That's correct. If the size is comparable, the linear comparison
> should be faster. I can update the comment like this:
>
> /* Use diff, if provided, unless it's comparable in size. With a large
> * diff, the O(n log n) binary search of each element may be slower than
> * a simple O(n) comparison between old and new. */
>
> What do you think?
I think that's a good comment, thoroughly clarifies the conditional.
-M
>
> Best regards, Ilya Maximets.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 4/8] python: ovs: flow: Add dp hash and meter actions.
On Fri, Jan 05, 2024 at 12:46:56PM +0100, Adrian Moreno wrote: > Add missing actions. > > Signed-off-by: Adrian Moreno Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/8] python: ovs: flow: Add sample to nested actions.
On Fri, Jan 05, 2024 at 12:46:55PM +0100, Adrian Moreno wrote: > Add the sample action to those that can be called in nested actions > (such as clone). > > Signed-off-by: Adrian Moreno Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/8] python: ovs: flow: Fix typo in n_packets.
On Fri, Jan 05, 2024 at 12:46:53PM +0100, Adrian Moreno wrote: > They key used in flows is "n_packets". > > Signed-off-by: Adrian Moreno Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/8] python: tests: Add info and key tests for OFPFlows.
On Fri, Jan 05, 2024 at 12:46:54PM +0100, Adrian Moreno wrote: > Parsing of info and matches was being tested as generic k-v parsing. > Also verify we don't find any unexpected field. > > Signed-off-by: Adrian Moreno > --- > python/ovs/tests/test_ofp.py | 73 +++- > 1 file changed, 55 insertions(+), 18 deletions(-) > > diff --git a/python/ovs/tests/test_ofp.py b/python/ovs/tests/test_ofp.py > index 27bcf0c47..5d2736ab4 100644 > --- a/python/ovs/tests/test_ofp.py > +++ b/python/ovs/tests/test_ofp.py > @@ -6,6 +6,30 @@ from ovs.flow.kv import KeyValue, ParseError > from ovs.flow.decoders import EthMask, IPMask, decode_mask > > > +def do_test_section(input_string, section, expected): > +flow = OFPFlow(input_string) > +kv_list = flow.section(section).data > + > +for i in range(len(expected)): > +assert expected[i].key == kv_list[i].key > +assert expected[i].value == kv_list[i].value > + > +# Assert positions relative to action string are OK. > +pos = flow.section(section).pos > +string = flow.section(section).string > + > +kpos = kv_list[i].meta.kpos > +kstr = kv_list[i].meta.kstring > +vpos = kv_list[i].meta.vpos > +vstr = kv_list[i].meta.vstring > +assert string[kpos : kpos + len(kstr)] == kstr > +if vpos != -1: > +assert string[vpos : vpos + len(vstr)] == vstr > + > +# Assert string meta is correct. > +assert input_string[pos : pos + len(string)] == string Hi Adrian, I'm probably missing something obvious. But I wonder if there a possibility that kv_list contains more elements than expected, and if so, is that something we should check for? > + > + > @pytest.mark.parametrize( > "input_string,expected", > [ ... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/5] ovsdb: transaction: Don't try to diff unchanged columns.
On Fri, Jan 5, 2024 at 9:30 AM Ilya Maximets wrote:
>
> On 12/29/23 17:28, Mike Pattrick wrote:
> > On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets wrote:
> >>
> >> While reassessing weak references the code attempts to collect added
> >> and removed atoms, even if the column didn't change. In case the
> >> column contains a large set, it may take significant amount of time
> >> to process.
> >>
> >> Add a check for the column actually being changed either by removing
> >> references to deleted rows or by direct removal.
> >>
> >> For example, rows in OVN Port_Group tables frequently have two large
> >> sets - 'ports' and 'acls'. In case a new ACL is added to the set
> >> without changing the ports, ports don't need to be reassessed.
> >>
> >> Fixes: 4dbff9f0a685 ("ovsdb: transaction: Incremental reassessment of weak
> >> refs.")
> >> Signed-off-by: Ilya Maximets
> >> ---
> >> ovsdb/transaction.c | 18 +++---
> >> 1 file changed, 11 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
> >> index bbe4cddc1..a482588a0 100644
> >> --- a/ovsdb/transaction.c
> >> +++ b/ovsdb/transaction.c
> >> @@ -745,13 +745,17 @@ assess_weak_refs(struct ovsdb_txn *txn, struct
> >> ovsdb_txn_row *txn_row)
> >> ovsdb_datum_destroy(&deleted_refs, &column->type);
> >>
> >> /* Generating the difference between old and new data. */
> >> -if (txn_row->old) {
> >> -ovsdb_datum_added_removed(&added, &removed,
> >> -
> >> &txn_row->old->fields[column->index],
> >> - datum, &column->type);
> >> -} else {
> >> -ovsdb_datum_init_empty(&removed);
> >> -ovsdb_datum_clone(&added, datum);
> >> +ovsdb_datum_init_empty(&added);
> >> +ovsdb_datum_init_empty(&removed);
> >
> > Nit: Why init these here if they will be overwritten in
> > ovsdb_datum_added_removed and ovsdb_datum_clone? Couldn't this be
> > included in an else?
>
> We could do that, it just seems a little less error-prone this way.
> We'll need to initialize both in the common 'else' and also initialize
> 'removed' in the clone case, i.e.:
>
> if (datum->n != orig_n
> || bitmap_is_set(txn_row->changed, column->index)) {
> if (txn_row->old) {
> ovsdb_datum_added_removed(&added, &removed,
>
> &txn_row->old->fields[column->index],
> datum, &column->type);
> } else {
> ovsdb_datum_init_empty(&removed);
> ovsdb_datum_clone(&added, datum);
> }
> } else {
> ovsdb_datum_init_empty(&added);
> ovsdb_datum_init_empty(&removed);
> }
>
> I don't think the initialization here is performance-critical, so
> I went with a slightly easier to read solution, that also saves
> a couple lines of code. But I have no real objections for the
> 'else' variant above, if you think it is better.
>
> What's your opinion?
I think you're right, your way is much more straightforward.
-M
>
> >
> > Otherwise, looks good!
> >
> > Acked-by: Mike Pattrick
> >
> >> +if (datum->n != orig_n
> >> +|| bitmap_is_set(txn_row->changed, column->index)) {
> >> +if (txn_row->old) {
> >> +ovsdb_datum_added_removed(&added, &removed,
> >> +
> >> &txn_row->old->fields[column->index],
> >> + datum, &column->type);
> >> +} else {
> >> +ovsdb_datum_clone(&added, datum);
> >> +}
> >> }
> >>
> >> /* Checking added data and creating new references. */
> >> --
> >> 2.43.0
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/5] ovsdb: transaction: Calculate added/removed from diff.
On 1/1/24 07:37, Mike Pattrick wrote:
> On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets wrote:
>>
>> In case the difference between 'old' and 'new' rows is readily
>> available, it can be used to construct added/removed datums
>> instead. Diffs are typically much smaller than the column
>> itself. This change more than doubles the performance of a
>> transaction replay.
>>
>> For example, with this change applied, initial read of OVSDB
>> file containing 136K small transactions for large OVN port
>> groups and address sets on my laptop takes 11 seconds vs 24
>> seconds without.
>>
>> Signed-off-by: Ilya Maximets
>> ---
>> lib/ovsdb-data.c| 26 ++
>> lib/ovsdb-data.h| 1 +
>> ovsdb/transaction.c | 15 ---
>> 3 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
>> index f18f74298..f0d7bee23 100644
>> --- a/lib/ovsdb-data.c
>> +++ b/lib/ovsdb-data.c
>> @@ -2238,6 +2238,8 @@ ovsdb_symbol_table_insert(struct ovsdb_symbol_table
>> *symtab,
>> /* APIs for Generating and apply diffs. */
>>
>> /* Find what needs to be added to and removed from 'old' to construct 'new'.
>> + * If the optional 'diff' is porvided, it can be used to speed up
>> processing,
>
> Provided
>
>> + * in case it is smaller than the original 'old' and 'new'.
>> *
>> * The 'added' and 'removed' datums are always safe; the orders of keys are
>> * maintained since they are added in order. */
>> @@ -2246,6 +2248,7 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added,
>>struct ovsdb_datum *removed,
>>const struct ovsdb_datum *old,
>>const struct ovsdb_datum *new,
>> + const struct ovsdb_datum *diff,
>>const struct ovsdb_type *type)
>> {
>> size_t oi, ni;
>> @@ -2258,6 +2261,29 @@ ovsdb_datum_added_removed(struct ovsdb_datum *added,
>> return;
>> }
>>
>> +/* Use diff, if provided, unless it's comparable in size. */
>> +if (diff && diff->n * 2 < old->n + new->n) {
>
> I guess the motivation for the size check is that the old algorithm is
> O(n) and the diff algorithm is O(n lg n)? If so it may be worthwhile
> to note that in the comment, it wasn't initially clear to me. Or am I
> misunderstanding this?
That's correct. If the size is comparable, the linear comparison
should be faster. I can update the comment like this:
/* Use diff, if provided, unless it's comparable in size. With a large
* diff, the O(n log n) binary search of each element may be slower than
* a simple O(n) comparison between old and new. */
What do you think?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/5] ovsdb: transaction: Don't try to diff unchanged columns.
On 12/29/23 17:28, Mike Pattrick wrote:
> On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets wrote:
>>
>> While reassessing weak references the code attempts to collect added
>> and removed atoms, even if the column didn't change. In case the
>> column contains a large set, it may take significant amount of time
>> to process.
>>
>> Add a check for the column actually being changed either by removing
>> references to deleted rows or by direct removal.
>>
>> For example, rows in OVN Port_Group tables frequently have two large
>> sets - 'ports' and 'acls'. In case a new ACL is added to the set
>> without changing the ports, ports don't need to be reassessed.
>>
>> Fixes: 4dbff9f0a685 ("ovsdb: transaction: Incremental reassessment of weak
>> refs.")
>> Signed-off-by: Ilya Maximets
>> ---
>> ovsdb/transaction.c | 18 +++---
>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
>> index bbe4cddc1..a482588a0 100644
>> --- a/ovsdb/transaction.c
>> +++ b/ovsdb/transaction.c
>> @@ -745,13 +745,17 @@ assess_weak_refs(struct ovsdb_txn *txn, struct
>> ovsdb_txn_row *txn_row)
>> ovsdb_datum_destroy(&deleted_refs, &column->type);
>>
>> /* Generating the difference between old and new data. */
>> -if (txn_row->old) {
>> -ovsdb_datum_added_removed(&added, &removed,
>> - &txn_row->old->fields[column->index],
>> - datum, &column->type);
>> -} else {
>> -ovsdb_datum_init_empty(&removed);
>> -ovsdb_datum_clone(&added, datum);
>> +ovsdb_datum_init_empty(&added);
>> +ovsdb_datum_init_empty(&removed);
>
> Nit: Why init these here if they will be overwritten in
> ovsdb_datum_added_removed and ovsdb_datum_clone? Couldn't this be
> included in an else?
We could do that, it just seems a little less error-prone this way.
We'll need to initialize both in the common 'else' and also initialize
'removed' in the clone case, i.e.:
if (datum->n != orig_n
|| bitmap_is_set(txn_row->changed, column->index)) {
if (txn_row->old) {
ovsdb_datum_added_removed(&added, &removed,
&txn_row->old->fields[column->index],
datum, &column->type);
} else {
ovsdb_datum_init_empty(&removed);
ovsdb_datum_clone(&added, datum);
}
} else {
ovsdb_datum_init_empty(&added);
ovsdb_datum_init_empty(&removed);
}
I don't think the initialization here is performance-critical, so
I went with a slightly easier to read solution, that also saves
a couple lines of code. But I have no real objections for the
'else' variant above, if you think it is better.
What's your opinion?
>
> Otherwise, looks good!
>
> Acked-by: Mike Pattrick
>
>> +if (datum->n != orig_n
>> +|| bitmap_is_set(txn_row->changed, column->index)) {
>> +if (txn_row->old) {
>> +ovsdb_datum_added_removed(&added, &removed,
>> +
>> &txn_row->old->fields[column->index],
>> + datum, &column->type);
>> +} else {
>> +ovsdb_datum_clone(&added, datum);
>> +}
>> }
>>
>> /* Checking added data and creating new references. */
>> --
>> 2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] treewide: Cleanup free() calls.
On 1/4/24 20:56, Mark Michelson wrote:
> Thanks Ales and Dumitru.
>
Thanks Mark!
> I applied this patch to main only, since it's not really fixing any
> problems. If you think this should be backported, let me know and I can
> apply it to older branches.
>
I don't think it's really needed right now. Maybe just if we realize it
causes conflicts with other backports. Otherwise main is fine.
Regards,
Dumitru
> Mark Michelson
>
> On 1/4/24 12:17, Ales Musil wrote:
>> On Thu, Jan 4, 2024 at 3:29 PM Dumitru Ceara wrote:
>>
>>> It's perfectly fine to call free(NULL). Take advantage of that to
>>> simplify the code. Use the safer CONST_CAST way of "unconsting"
>>> pointers we pass to free while we're at it.
>>>
>>> Signed-off-by: Dumitru Ceara
>>> ---
>>> controller/ovn-controller.c | 12 +++-
>>> controller/pinctrl.c | 5 +
>>> controller/vif-plug.c | 12 +++-
>>> lib/expr.c | 22 --
>>> lib/ovn-parallel-hmap.c | 6 ++
>>> northd/ipam.c | 2 +-
>>> 6 files changed, 18 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>>> index 760b7788be..8709c1ae2a 100644
>>> --- a/controller/ovn-controller.c
>>> +++ b/controller/ovn-controller.c
>>> @@ -688,9 +688,7 @@ add_pending_ct_zone_entry(struct shash
>>> *pending_ct_zones,
>>> */
>>> struct ct_zone_pending_entry *existing =
>>> shash_replace(pending_ct_zones, name, pending);
>>> - if (existing) {
>>> - free(existing);
>>> - }
>>> + free(existing);
>>> }
>>>
>>> static bool
>>> @@ -6099,12 +6097,8 @@ loop_done:
>>>
>>> ovs_feature_support_destroy();
>>> free(ovs_remote);
>>> - if (file_system_id) {
>>> - free(file_system_id);
>>> - }
>>> - if (cli_system_id) {
>>> - free(cli_system_id);
>>> - }
>>> + free(file_system_id);
>>> + free(cli_system_id);
>>> ovn_exit_args_finish(&exit_args);
>>> unixctl_server_destroy(unixctl);
>>> service_stop();
>>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>>> index 5a35d56f6b..12055a6756 100644
>>> --- a/controller/pinctrl.c
>>> +++ b/controller/pinctrl.c
>>> @@ -1282,10 +1282,7 @@ fill_ipv6_prefix_state(struct ovsdb_idl_txn
>>> *ovnsb_idl_txn,
>>> struct ipv6_prefixd_state *pfd;
>>>
>>> if (!smap_get_bool(&pb->options, "ipv6_prefix", false)) {
>>> - pfd = shash_find_and_delete(&ipv6_prefixd,
>>> pb->logical_port);
>>> - if (pfd) {
>>> - free(pfd);
>>> - }
>>> + free(shash_find_and_delete(&ipv6_prefixd,
>>> pb->logical_port));
>>> continue;
>>> }
>>>
>>> diff --git a/controller/vif-plug.c b/controller/vif-plug.c
>>> index 38348bf544..d4c7552eab 100644
>>> --- a/controller/vif-plug.c
>>> +++ b/controller/vif-plug.c
>>> @@ -142,15 +142,9 @@ destroy_port_ctx(struct vif_plug_port_ctx *ctx)
>>> {
>>> smap_destroy(&ctx->vif_plug_port_ctx_in.lport_options);
>>> smap_destroy(&ctx->vif_plug_port_ctx_in.iface_options);
>>> - if (ctx->vif_plug_port_ctx_in.lport_name) {
>>> - free((char *)ctx->vif_plug_port_ctx_in.lport_name);
>>> - }
>>> - if (ctx->vif_plug_port_ctx_in.iface_name) {
>>> - free((char *)ctx->vif_plug_port_ctx_in.iface_name);
>>> - }
>>> - if (ctx->vif_plug_port_ctx_in.iface_type) {
>>> - free((char *)ctx->vif_plug_port_ctx_in.iface_type);
>>> - }
>>> + free(CONST_CAST(char *, ctx->vif_plug_port_ctx_in.lport_name));
>>> + free(CONST_CAST(char *, ctx->vif_plug_port_ctx_in.iface_name));
>>> + free(CONST_CAST(char *, ctx->vif_plug_port_ctx_in.iface_type));
>>> /* Note that data associated with ctx->vif_plug_port_ctx_out
>>> must be
>>> * destroyed by the plug provider implementation with a call to
>>> * vif_plug_port_ctx_destroy prior to calling this function */
>>> diff --git a/lib/expr.c b/lib/expr.c
>>> index f5d2032334..0cb44e1b6f 100644
>>> --- a/lib/expr.c
>>> +++ b/lib/expr.c
>>> @@ -179,17 +179,13 @@ expr_combine(enum expr_type type, struct expr *a,
>>> struct expr *b)
>>> } else {
>>> ovs_list_push_back(&a->andor, &b->node);
>>> }
>>> - if (a->as_name) {
>>> - free(a->as_name);
>>> - a->as_name = NULL;
>>> - }
>>> + free(a->as_name);
>>> + a->as_name = NULL;
>>> return a;
>>> } else if (b->type == type) {
>>> ovs_list_push_front(&b->andor, &a->node);
>>> - if (b->as_name) {
>>> - free(b->as_name);
>>> - b->as_name = NULL;
>>> - }
>>> + free(b->as_name);
>>> + b->as_name = NULL;
>>> return b;
>>> } else {
>>> struct expr *e = expr_create_andor(type);
>>> @@ -879,12 +875,10 @@ parse_constant(struct expr_context *ctx, struct
>>> expr_constant_set *cs,
>>>
Re: [ovs-dev] [PATCH] ovsdb-idl: Reparse reference src before dst is deleted
On 1/5/24 04:13, Tao Liu wrote: > > On 1/4/24 7:56 PM, Ilya Maximets wrote: >> On 7/4/23 12:41, Tao Liu wrote: >>> Hi, Ilya. Thanks for your comment. >>> >>> On 7/4/23 4:10 AM, Ilya Maximets wrote: On 7/2/23 11:59, Tao Liu wrote: > Commit 02f31a1262fc has fixed the row references when insertion and > deletion execute in one IDL run. However, we can still hit ovn-controller > crash in pressure test where pb->datapath == NULL. It is triggered by > reference dst deletion in same IDL run: > > idl run: > update1: > insert port_binding > insert datapath_binding > update2: > delete datapath_binding > delete port_binding > > Fix it by reparse reference src before dst deletion. Hi. Thanks for the patch! There seems to be an issue indeed. Removed rows should have valid references to other removed rows. However, I'm not sure this solution solves the issue in a general case as it depends on the order of events. If events in the update2 will be in opposite order, the issue will still be there. >>> The opposite order of update2 has been fixed in commit 02f31a1262fc, >>> which now moved to >>> >>> first reparse_row(). >>> In a more general case we may also have two rows reference each other and we need to preserve both references. We probably could: 1. Postpone all the deletions, execute insertions and updates. 2. Call ovsdb_idl_reparse_refs_to_inserted() to re-parse every row that needs re-parsing due to newly added rows. 3. Delete the rows that need to be deleted. 4. Call ovsdb_idl_reparse_deleted() to get rid of references to deleted rows from non-deleted ones. >>> Sounds like a good idea, that makes things more clear. >>> What do you think? Dumitru, maybe you have some thoughts on this? And we will need need a unit test for this issue. >>> Will try to add one. >> Hi, Tao Liu. >> >> Are you still working on a fix for this issue? Or maybe someone >> else should take a look into that? >> >> Best regards, Ilya Maximets. > > > Hi, Ilya > > I'm not going on a new version. It would be appreciate if you can help > to continue this fix. > > Best regards, Tao OK. No problem. We'll find someone to work on this, or I'll try to find some time for this myself. For the record, I'm not sure the sequence of steps I proposed above is fully correct, since the order between insertions and deletions might be significant, if they come from different updates. So, we may have to batch re-parsing (i.e. re-parse accumulated refs to deleted when we encounter an insertion and re-parse accumulated refs to inserted when we encounter a deletion), but that needs some more thinking. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 6/8] python: ovs: flow: Add idle_age to openflow flows.
Add missing key.
Signed-off-by: Adrian Moreno
---
python/ovs/flow/ofp.py | 1 +
python/ovs/tests/test_ofp.py | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/python/ovs/flow/ofp.py b/python/ovs/flow/ofp.py
index f1a720d75..3d3226c91 100644
--- a/python/ovs/flow/ofp.py
+++ b/python/ovs/flow/ofp.py
@@ -176,6 +176,7 @@ class OFPFlow(Flow):
"idle_timeout": decode_time,
"hard_timeout": decode_time,
"hard_age": decode_time,
+"idle_age": decode_time,
}
return KVDecoders(args)
diff --git a/python/ovs/tests/test_ofp.py b/python/ovs/tests/test_ofp.py
index 9e2721acf..4bcbf9cdf 100644
--- a/python/ovs/tests/test_ofp.py
+++ b/python/ovs/tests/test_ofp.py
@@ -618,7 +618,7 @@ def test_act(input_string, expected):
"input_string,expected",
[
(
-"cookie=0x35f946ead8d8f9e4, duration=97746.271s, table=0,
n_packets=12, n_bytes=254, priority=4,in_port=1", # noqa: E501
+"cookie=0x35f946ead8d8f9e4, duration=97746.271s, table=0,
n_packets=12, n_bytes=254, idle_age=117, priority=4,in_port=1", # noqa: E501
(
[
KeyValue("cookie", 0x35f946ead8d8f9e4),
@@ -626,6 +626,7 @@ def test_act(input_string, expected):
KeyValue("table", 0),
KeyValue("n_packets", 12),
KeyValue("n_bytes", 254),
+KeyValue("idle_age", 117),
],
[
KeyValue("priority", 4),
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 8/8] python: ovs: flow: Add meter_id to controller.
Add missing option to controller action.
Signed-off-by: Adrian Moreno
---
python/ovs/flow/ofp_act.py | 1 +
python/ovs/tests/test_ofp.py | 15 +++
2 files changed, 16 insertions(+)
diff --git a/python/ovs/flow/ofp_act.py b/python/ovs/flow/ofp_act.py
index c540443ea..2c85076a3 100644
--- a/python/ovs/flow/ofp_act.py
+++ b/python/ovs/flow/ofp_act.py
@@ -54,6 +54,7 @@ def decode_controller(value):
"id": decode_int,
"userdata": decode_default,
"pause": decode_flag,
+"meter_id": decode_int,
}
)
)(value)
diff --git a/python/ovs/tests/test_ofp.py b/python/ovs/tests/test_ofp.py
index 4bcbf9cdf..59eab8c3a 100644
--- a/python/ovs/tests/test_ofp.py
+++ b/python/ovs/tests/test_ofp.py
@@ -50,6 +50,21 @@ def do_test_section(input_string, section, expected):
KeyValue("controller", {"max_len": 200}),
],
),
+(
+
"actions=controller(max_len=123,reason=no_match,id=456,userdata=00.00.00.12.00.00.00.00,meter_id=12)",
# noqa: E501
+[
+KeyValue(
+"controller",
+{
+"max_len": 123,
+"reason": "no_match",
+"id": 456,
+"userdata": "00.00.00.12.00.00.00.00",
+"meter_id": 12,
+}
+),
+],
+),
(
"actions=enqueue(foo,42),enqueue:foo:42,enqueue(bar,4242)",
[
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 7/8] python: ovs: flow: Make check_pkt_len action a list.
In general, most actions must be lists since the keys can be repeated.
Signed-off-by: Adrian Moreno
---
python/ovs/flow/odp.py | 6 --
python/ovs/tests/test_odp.py | 12 ++--
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/python/ovs/flow/odp.py b/python/ovs/flow/odp.py
index 46697a1bc..7d9b165d4 100644
--- a/python/ovs/flow/odp.py
+++ b/python/ovs/flow/odp.py
@@ -375,13 +375,15 @@ class ODPFlow(Flow):
KVDecoders(
decoders=_decoders,
default_free=decode_free_output,
-)
+),
+is_list=True,
),
"le": nested_kv_decoder(
KVDecoders(
decoders=_decoders,
default_free=decode_free_output,
-)
+),
+is_list=True,
),
}
)
diff --git a/python/ovs/tests/test_odp.py b/python/ovs/tests/test_odp.py
index 8147a31d1..d52056794 100644
--- a/python/ovs/tests/test_odp.py
+++ b/python/ovs/tests/test_odp.py
@@ -517,24 +517,24 @@ def test_odp_fields(input_string, expected):
"check_pkt_len",
{
"size": 200,
-"gt": {"output": {"port": 4}},
-"le": {"output": {"port": 5}},
+"gt": [{"output": {"port": 4}}],
+"le": [{"output": {"port": 5}}],
},
),
KeyValue(
"check_pkt_len",
{
"size": 200,
-"gt": {"drop": True},
-"le": {"output": {"port": 5}},
+"gt": [{"drop": True}],
+"le": [{"output": {"port": 5}}],
},
),
KeyValue(
"check_pkt_len",
{
"size": 200,
-"gt": {"ct": {"nat": True}},
-"le": {"drop": True},
+"gt": [{"ct": {"nat": True}}],
+"le": [{"drop": True}],
},
),
],
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 5/8] python: tests: Refactor test_odp section testing.
Avoid code duplication by moving the section testing code to its own
function.
Signed-off-by: Adrian Moreno
---
python/ovs/tests/test_odp.py | 66 ++--
1 file changed, 26 insertions(+), 40 deletions(-)
diff --git a/python/ovs/tests/test_odp.py b/python/ovs/tests/test_odp.py
index d60947a5c..8147a31d1 100644
--- a/python/ovs/tests/test_odp.py
+++ b/python/ovs/tests/test_odp.py
@@ -13,6 +13,30 @@ from ovs.flow.decoders import (
)
+def do_test_section(input_string, section, expected):
+flow = ODPFlow(input_string)
+kv_list = flow.section(section).data
+
+for i in range(len(expected)):
+assert expected[i].key == kv_list[i].key
+assert expected[i].value == kv_list[i].value
+
+# Assert positions relative to action string are OK.
+pos = flow.section(section).pos
+string = flow.section(section).string
+
+kpos = kv_list[i].meta.kpos
+kstr = kv_list[i].meta.kstring
+vpos = kv_list[i].meta.vpos
+vstr = kv_list[i].meta.vstring
+assert string[kpos : kpos + len(kstr)] == kstr
+if vpos != -1:
+assert string[vpos : vpos + len(vstr)] == vstr
+
+# Assert string meta is correct.
+assert input_string[pos : pos + len(string)] == string
+
+
@pytest.mark.parametrize(
"input_string,expected",
[
@@ -109,26 +133,7 @@ from ovs.flow.decoders import (
],
)
def test_odp_fields(input_string, expected):
-odp = ODPFlow(input_string)
-match = odp.match_kv
-for i in range(len(expected)):
-assert expected[i].key == match[i].key
-assert expected[i].value == match[i].value
-
-# Assert positions relative to action string are OK.
-mpos = odp.section("match").pos
-mstring = odp.section("match").string
-
-kpos = match[i].meta.kpos
-kstr = match[i].meta.kstring
-vpos = match[i].meta.vpos
-vstr = match[i].meta.vstring
-assert mstring[kpos : kpos + len(kstr)] == kstr
-if vpos != -1:
-assert mstring[vpos : vpos + len(vstr)] == vstr
-
-# Assert mstring meta is correct.
-assert input_string[mpos : mpos + len(mstring)] == mstring
+do_test_section(input_string, "match", expected)
@pytest.mark.parametrize(
@@ -549,23 +554,4 @@ def test_odp_fields(input_string, expected):
],
)
def test_odp_actions(input_string, expected):
-odp = ODPFlow(input_string)
-actions = odp.actions_kv
-for i in range(len(expected)):
-assert expected[i].key == actions[i].key
-assert expected[i].value == actions[i].value
-
-# Assert positions relative to action string are OK.
-apos = odp.section("actions").pos
-astring = odp.section("actions").string
-
-kpos = actions[i].meta.kpos
-kstr = actions[i].meta.kstring
-vpos = actions[i].meta.vpos
-vstr = actions[i].meta.vstring
-assert astring[kpos : kpos + len(kstr)] == kstr
-if vpos != -1:
-assert astring[vpos : vpos + len(vstr)] == vstr
-
-# Assert astring meta is correct.
-assert input_string[apos : apos + len(astring)] == astring
+do_test_section(input_string, "actions", expected)
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 3/8] python: ovs: flow: Add sample to nested actions.
Add the sample action to those that can be called in nested actions
(such as clone).
Signed-off-by: Adrian Moreno
---
python/ovs/flow/odp.py | 29 +++--
python/ovs/tests/test_ofp.py | 14 ++
2 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/python/ovs/flow/odp.py b/python/ovs/flow/odp.py
index 88aee17fb..ef7e5d6b8 100644
--- a/python/ovs/flow/odp.py
+++ b/python/ovs/flow/odp.py
@@ -336,6 +336,21 @@ class ODPFlow(Flow):
**ODPFlow._tnl_action_decoder_args(),
}
+_decoders["sample"] = nested_kv_decoder(
+KVDecoders(
+{
+"sample": (lambda x: float(x.strip("%"))),
+"actions": nested_kv_decoder(
+KVDecoders(
+decoders=_decoders,
+default_free=decode_free_output,
+),
+is_list=True,
+),
+}
+)
+)
+
_decoders["clone"] = nested_kv_decoder(
KVDecoders(decoders=_decoders, default_free=decode_free_output),
is_list=True,
@@ -343,20 +358,6 @@ class ODPFlow(Flow):
return {
**_decoders,
-"sample": nested_kv_decoder(
-KVDecoders(
-{
-"sample": (lambda x: float(x.strip("%"))),
-"actions": nested_kv_decoder(
-KVDecoders(
-decoders=_decoders,
-default_free=decode_free_output,
-),
-is_list=True,
-),
-}
-)
-),
"check_pkt_len": nested_kv_decoder(
KVDecoders(
{
diff --git a/python/ovs/tests/test_ofp.py b/python/ovs/tests/test_ofp.py
index 5d2736ab4..9e2721acf 100644
--- a/python/ovs/tests/test_ofp.py
+++ b/python/ovs/tests/test_ofp.py
@@ -569,6 +569,20 @@ def do_test_section(input_string, section, expected):
),
],
),
+(
+"actions=LOCAL,clone(sample(probability=123))",
+[
+KeyValue("output", {"port": "LOCAL"}),
+KeyValue(
+"clone",
+[
+{"sample": {
+"probability": 123,
+}},
+]
+),
+],
+),
(
"actions=doesnotexist(1234)",
ParseError,
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 1/8] python: ovs: flow: Fix typo in n_packets.
They key used in flows is "n_packets".
Signed-off-by: Adrian Moreno
---
python/ovs/flow/ofp.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/python/ovs/flow/ofp.py b/python/ovs/flow/ofp.py
index 20231fd9f..f1a720d75 100644
--- a/python/ovs/flow/ofp.py
+++ b/python/ovs/flow/ofp.py
@@ -170,7 +170,7 @@ class OFPFlow(Flow):
args = {
"table": decode_int,
"duration": decode_time,
-"n_packet": decode_int,
+"n_packets": decode_int,
"n_bytes": decode_int,
"cookie": decode_int,
"idle_timeout": decode_time,
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 4/8] python: ovs: flow: Add dp hash and meter actions.
Add missing actions.
Signed-off-by: Adrian Moreno
---
python/ovs/flow/odp.py | 9 +
python/ovs/tests/test_odp.py | 12
2 files changed, 21 insertions(+)
diff --git a/python/ovs/flow/odp.py b/python/ovs/flow/odp.py
index ef7e5d6b8..46697a1bc 100644
--- a/python/ovs/flow/odp.py
+++ b/python/ovs/flow/odp.py
@@ -204,6 +204,7 @@ class ODPFlow(Flow):
"""Generate the arguments for the action KVDecoders."""
_decoders = {
"drop": decode_flag,
+"meter": decode_int,
"lb_output": decode_int,
"trunc": decode_int,
"recirc": decode_int,
@@ -334,6 +335,14 @@ class ODPFlow(Flow):
)
),
**ODPFlow._tnl_action_decoder_args(),
+"hash": nested_kv_decoder(
+KVDecoders(
+{
+"l4": decode_int,
+"sym_l4": decode_int,
+}
+)
+),
}
_decoders["sample"] = nested_kv_decoder(
diff --git a/python/ovs/tests/test_odp.py b/python/ovs/tests/test_odp.py
index a50d3185c..d60947a5c 100644
--- a/python/ovs/tests/test_odp.py
+++ b/python/ovs/tests/test_odp.py
@@ -534,6 +534,18 @@ def test_odp_fields(input_string, expected):
),
],
),
+(
+"actions:meter(1),hash(l4(0))",
+[
+KeyValue("meter", 1),
+KeyValue(
+"hash",
+{
+"l4": 0,
+}
+),
+],
+),
],
)
def test_odp_actions(input_string, expected):
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 2/8] python: tests: Add info and key tests for OFPFlows.
Parsing of info and matches was being tested as generic k-v parsing.
Also verify we don't find any unexpected field.
Signed-off-by: Adrian Moreno
---
python/ovs/tests/test_ofp.py | 73 +++-
1 file changed, 55 insertions(+), 18 deletions(-)
diff --git a/python/ovs/tests/test_ofp.py b/python/ovs/tests/test_ofp.py
index 27bcf0c47..5d2736ab4 100644
--- a/python/ovs/tests/test_ofp.py
+++ b/python/ovs/tests/test_ofp.py
@@ -6,6 +6,30 @@ from ovs.flow.kv import KeyValue, ParseError
from ovs.flow.decoders import EthMask, IPMask, decode_mask
+def do_test_section(input_string, section, expected):
+flow = OFPFlow(input_string)
+kv_list = flow.section(section).data
+
+for i in range(len(expected)):
+assert expected[i].key == kv_list[i].key
+assert expected[i].value == kv_list[i].value
+
+# Assert positions relative to action string are OK.
+pos = flow.section(section).pos
+string = flow.section(section).string
+
+kpos = kv_list[i].meta.kpos
+kstr = kv_list[i].meta.kstring
+vpos = kv_list[i].meta.vpos
+vstr = kv_list[i].meta.vstring
+assert string[kpos : kpos + len(kstr)] == kstr
+if vpos != -1:
+assert string[vpos : vpos + len(vstr)] == vstr
+
+# Assert string meta is correct.
+assert input_string[pos : pos + len(string)] == string
+
+
@pytest.mark.parametrize(
"input_string,expected",
[
@@ -570,27 +594,40 @@ from ovs.flow.decoders import EthMask, IPMask, decode_mask
def test_act(input_string, expected):
if isinstance(expected, type):
with pytest.raises(expected):
-ofp = OFPFlow(input_string)
+OFPFlow(input_string)
return
-ofp = OFPFlow(input_string)
-actions = ofp.actions_kv
+do_test_section(input_string, "actions", expected)
-for i in range(len(expected)):
-assert expected[i].key == actions[i].key
-assert expected[i].value == actions[i].value
-# Assert positions relative to action string are OK.
-apos = ofp.section("actions").pos
-astring = ofp.section("actions").string
+@pytest.mark.parametrize(
+"input_string,expected",
+[
+(
+"cookie=0x35f946ead8d8f9e4, duration=97746.271s, table=0,
n_packets=12, n_bytes=254, priority=4,in_port=1", # noqa: E501
+(
+[
+KeyValue("cookie", 0x35f946ead8d8f9e4),
+KeyValue("duration", 97746.271),
+KeyValue("table", 0),
+KeyValue("n_packets", 12),
+KeyValue("n_bytes", 254),
+],
+[
+KeyValue("priority", 4),
+KeyValue("in_port", 1)
+],
+),
+),
+],
+)
+def test_key(input_string, expected):
+if isinstance(expected, type):
+with pytest.raises(expected):
+OFPFlow(input_string)
+return
-kpos = actions[i].meta.kpos
-kstr = actions[i].meta.kstring
-vpos = actions[i].meta.vpos
-vstr = actions[i].meta.vstring
-assert astring[kpos : kpos + len(kstr)] == kstr
-if vpos != -1:
-assert astring[vpos : vpos + len(vstr)] == vstr
+input_string += " actions=drop"
-# Assert astring meta is correct.
-assert input_string[apos : apos + len(astring)] == astring
+do_test_section(input_string, "info", expected[0])
+do_test_section(input_string, "match", expected[1])
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 0/8] python: Miscelaneous flow parsing fixes.
This series contains some miscelaneous fixes in the flow parsing library. V4: - Fixed pytests and added more tests for each fix. V2: - Rebased and dropped first patch. Adrian Moreno (8): python: ovs: flow: Fix typo in n_packets. python: tests: Add info and key tests for OFPFlows. python: ovs: flow: Add sample to nested actions. python: ovs: flow: Add dp hash and meter actions. python: tests: Refactor test_odp section testing. python: ovs: flow: Add idle_age to openflow flows. python: ovs: flow: Make check_pkt_len action a list. python: ovs: flow: Add meter_id to controller. python/ovs/flow/odp.py | 44 +-- python/ovs/flow/ofp.py | 3 +- python/ovs/flow/ofp_act.py | 1 + python/ovs/tests/test_odp.py | 90 +++--- python/ovs/tests/test_ofp.py | 103 +-- 5 files changed, 160 insertions(+), 81 deletions(-) -- 2.43.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 5/6] python: ovs: flow: Make check_pkt_len action a list.
On 1/2/24 15:26, Ilya Maximets wrote: On 12/20/23 18:57, Adrian Moreno wrote: In general, most actions must be lists since the keys can be repeated. Signed-off-by: Adrian Moreno --- python/ovs/flow/odp.py | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/python/ovs/flow/odp.py b/python/ovs/flow/odp.py index 46697a1bc..7d9b165d4 100644 --- a/python/ovs/flow/odp.py +++ b/python/ovs/flow/odp.py @@ -375,13 +375,15 @@ class ODPFlow(Flow): KVDecoders( decoders=_decoders, default_free=decode_free_output, -) +), +is_list=True, ), "le": nested_kv_decoder( KVDecoders( decoders=_decoders, default_free=decode_free_output, -) +), +is_list=True, ), } ) Hi, Adrian. This change breaks the pytest. Please, adjust the unit tests accordingly. Speaking of tests, it might make sense to add some for other patches in the set as well, if applicable. Wow, will have to look into my scripts and see why pytest were being skipped! Thanks. Best regards, Ilya Maximets. -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
