[ovs-dev] [PATCH v4] netdev-dpdk: Disable outer udp checksum offload for ice driver.

[Jun Wang]

Fixing the issue of incorrect outer UDP checksum in packets sent by E810.

Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
Reported-at: https://github.com/openvswitch/ovs-issues/issues/321

Signed-off-by: Jun Wang 
---
 lib/netdev-dpdk.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 29a6bf0..1820163 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1354,6 +1354,12 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
 info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
 }
 
+if (!strcmp(info.driver_name, "net_ice")) {
+VLOG_INFO("%s: disabled Tx outer udp checksum offloads for a "
+  "net/ice port.", netdev_get_name(>up));
+info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM;
+}
+
 if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPV4_CKSUM) {
 dev->hw_ol_features |= NETDEV_TX_IPV4_CKSUM_OFFLOAD;
 } else {
-- 
1.8.3.1



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] netdev-dpdk: Disable outer udp checksum offload for ice driver.

[0-day Robot]

Bleep bloop.  Greetings Jun Wang, 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.


checkpatch:
ERROR: Author Jun Wang  needs to sign off.
Lines checked: 36, Warnings: 0, Errors: 1


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 v3] netdev-dpdk: Disable outer udp checksum offload for ice driver.

[Jun Wang]

Fixing the issue of incorrect outer UDP checksum in packets sent by E810.

Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
Reported-at: https://github.com/openvswitch/ovs-issues/issues/321

Signed-ff-by: Jun Wang 
---
 lib/netdev-dpdk.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 29a6bf0..1820163 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1354,6 +1354,12 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
 info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
 }
 
+if (!strcmp(info.driver_name, "net_ice")) {
+VLOG_INFO("%s: disabled Tx outer udp checksum offloads for a "
+  "net/ice port.", netdev_get_name(>up));
+info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM;
+}
+
 if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPV4_CKSUM) {
 dev->hw_ol_features |= NETDEV_TX_IPV4_CKSUM_OFFLOAD;
 } else {
-- 
1.8.3.1



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] Fix broken link for LTS release.

[Igor Zhukov]

Great! You're welcome!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ofctrl: Wait at S_WAIT_BEFORE_CLEAR only once.

[Han Zhou]

On Mon, Mar 18, 2024 at 11:27 AM Mark Michelson  wrote:
>
> Hi Han,
>
> I have a comment below
>
> On 3/5/24 01:27, Han Zhou wrote:
> > The ovn-ofctrl-wait-before-clear setting is designed to minimize
> > downtime during the initial start-up of the ovn-controller. For this
> > purpose, the ovn-controller should wait only once upon entering the
> > S_WAIT_BEFORE_CLEAR state for the first time. Subsequent reconnections
> > to the OVS, such as those occurring during an OVS restart/upgrade,
> > should not trigger this wait. However, the current implemention always
> > waits for the configured time in the S_WAIT_BEFORE_CLEAR state, which
> > can inadvertently delay flow installations during OVS restart/upgrade,
> > potentially causing more harm than good. (The extent of the impact
> > varies based on the method used to restart OVS, including whether flow
> > save/restore tools and the flow-restore-wait feature are employed.)
> >
> > This patch avoids the unnecessary wait after the initial one.
> >
> > Fixes: 896adfd2d8b3 ("ofctrl: Support ovn-ofctrl-wait-before-clear to
reduce down time during upgrade.")
> > Signed-off-by: Han Zhou 
> > ---
> >   controller/ofctrl.c | 1 -
> >   tests/ovn-controller.at | 9 +++--
> >   2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index f14cd79a8dbb..0d72ecbaa167 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -634,7 +634,6 @@ run_S_WAIT_BEFORE_CLEAR(void)
> >   if (!wait_before_clear_time ||
> >   (wait_before_clear_expire &&
> >time_msec() >= wait_before_clear_expire)) {
> > -wait_before_clear_expire = 0;
> >   state = S_CLEAR_FLOWS;
> >   return;
> >   }
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index 37f1ded1bd26..b65e11722cbb 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -2284,10 +2284,15 @@ lflow_run_2=$(ovn-appctl -t ovn-controller
coverage/read-counter lflow_run)
> >   AT_CHECK_UNQUOTED([echo $lflow_run_1], [0], [$lflow_run_2
> >   ])
> >
> > -# Restart OVS this time, and wait until flows are reinstalled
> > +# Restart OVS this time. Flows should be reinstalled without waiting.
> >   OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
> >   start_daemon ovs-vswitchd --enable-dummy=system -vvconn
-vofproto_dpif -vunixctl
> > -OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep -F 10.1.2.4 | grep
-vF 2.2.2.2])
> > +
> > +# Sleep for 3s, which is long enough for the flows to be installed, but
> > +# shorter than the wait-before-clear (5s), to make sure the flows are
installed
> > +# without waiting.
> > +sleep 3
>
> This change makes me nervous. The comment makes sense. However, I worry
> that on slow or loaded systems, relying on the flows to be written
> within 3 seconds may not always work out.
>
> If there were a way to peek into the ofctrl state machine and check that
> we have moved off of S_WAIT_BEFORE_CLEAR by this point, that might work
> better. But that is something that is hard to justify exposing.
>
> I came up with this possible idea:
>   * set wait-before-clear to a time longer than OVS_CTL_TIMEOUT (e.g. 60
> seconds)
>   * Restart ovs
>   * Use OVS_WAIT_UNTIL(...), just like the test used to do.
>
> This way, we get plenty of opportunities to ensure the flows were
> written. In most cases, this probably will actually be quicker than the
> 3 second sleep added in this patch. However, if it takes longer than 3
> seconds, then the test can still pass. If the flows get written
> properly, then we know ovn-controller did not wait for the
> wait-before-clear time.
>

Hi Mark, thanks for your comment! I agree with you that sleep for 3s is not
very reliable. Your suggestion looks better, but I think there is still a
potential problem. The approach assumes that ovn-controller will always
apply the new settings of ofctrl-wait-before-clear. It is true for the
current implementation, but there is nothing preventing us from removing
this logic, so that ovn-controller ignores any ofctrl-wait-before-clear
setting changes after startup. In fact, it is more reasonable to ignore it.
So, let's assume ovn-controller doesn't take care of the changes of the
settings. In this test case, the setting is initially 5s when
ovn-controller starts, and later changing it to 60s doesn't take effect and
ovn-controller still uses the 5s value. And then let's assume
ovn-controller still waits for the 5s after OVS is restarted, and the test
case will pass because OVS_WAIT_UNTIL can wait for much longer than 5s. So
the test will be incorrect.

To avoid this potential problem, I tried another approach. I figured that
simply adding a "ovn-nbctl --wait=hv sync" is sufficient to ensure
ovn-controller had the chance to install flows, without the need to sleep.
So please see if the below diff looks good:

Re: [ovs-dev] [PATCH v2] route-table: Avoid routes from non-standard routing tables.

[Ilya Maximets]

On 3/20/24 19:47, Ilya Maximets wrote:
> Currently, ovs-vswitchd is subscribed to all the routing changes in the
> kernel.  On each change, it marks the internal routing table cache as
> invalid, then resets it and dumps all the routes from the kernel from
> scratch.  The reason for that is kernel routing updates not being
> reliable in a sense that it's hard to tell which route is getting
> removed or modified.  Userspace application has to track the order in
> which route entries are dumped from the kernel.  Updates can get lost
> or even duplicated and the kernel doesn't provide a good mechanism to
> distinguish one route from another.  To my knowledge, dumping all the
> routes from a kernel after each change is the only way to keep the
> cache consistent.  Some more info can be found in the following never
> addressed issues:
>   https://bugzilla.redhat.com/1337860
>   https://bugzilla.redhat.com/1337855
> 
> It seems to be believed that NetworkManager "mostly" does incremental
> updates right.  But it is still not completely correct, will re-dump
> the whole table in certain cases, and it takes a huge amount of very
> complicated code to do the accounting and route comparisons.
> 
> Going back to ovs-vswitchd, it currently dumps routes from all the
> routing tables.  If it will get conflicting routes from multiple
> tables, the cache will not be useful.  The routing cache in userspace
> is primarily used for checking the egress port for tunneled traffic
> and this way also detecting link state changes for a tunnel port.
> For userspace datapath it is used for actual routing of the packet
> after sending to a native tunnel.
> With kernel datapath we don't really have a mechanism to know which
> routing table will actually be used by the kernel after encapsulation,
> so our lookups on a cache may be incorrect because of this as well.
> 
> So, unless all the relevant routes are in the standard tables, the
> lookup in userspace route cache is unreliable.
> 
> Luckily, most setups are not using any complicated routing in
> non-standard tables that OVS has to be aware of.
> 
> It is possible, but unlikely, that standard routing tables are
> completely empty while some other custom table is not, and all the OVS
> tunnel traffic is directed to that table.  That would be the only
> scenario where dumping non-standard tables would make sense.  But it
> seems like this kind of setup will likely need a way to tell OVS from
> which table the routes should be taken, or we'll need to dump routing
> rules and keep a separate cache for each table, so we can first match
> on rules and then lookup correct routes in a specific table.  I'm not
> sure if trying to implement all that is justified.
> 
> For now, stop considering routes from non-standard tables to avoid
> mixing different tables together and also wasting CPU resources.
> 
> This fixes a high CPU usage in ovs-vswitchd in case a BGP daemon is
> running on a same host and in a same network namespace with OVS using
> its own custom routing table.
> 
> Unfortunately, there seems to be no way to tell the kernel to send
> updates only for particular tables.  So, we'll still receive and parse
> all of them.  But they will not result in a full cache invalidation in
> most cases.
> 
> Linux kernel v4.20 introduced filtering support for RTM_GETROUTE dumps.
> So, we can make use of it and dump only standard tables when we get a
> relevant route update.  NETLINK_GET_STRICT_CHK has to be enabled on
> the socket for filtering to work.  There is no reason to not enable it
> by default, if supported.  It is not used outside of NETLINK_ROUTE.
> 
> Fixes: f0e167f0dbad ("route-table: Handle route updates more robustly.")
> Fixes: ea83a2fcd0d3 ("lib: Show tunnel egress interface in ovsdb")
> Reported-at: https://github.com/openvswitch/ovs-issues/issues/185
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-October/052091.html
> Signed-off-by: Ilya Maximets 
> ---
> 
> Version 2:
>   * Changed log level for the NETLINK_GET_STRICT_CHK failure to
> DBG if not supported and WARN if there is an actual failure.
>   * While at it added rate limiting.  It is not necessary, but
> doesn't hurt.
> 
> Note: GitHub actions will likely fail due to ongoing major outage
>   of ppa.launchpad.net from where it installs 32bit toolchains.
>   Will need to request a re-check once the incident is over.

Should be resolved now.

Recheck-request: github-robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] userspace: Allow UDP zero checksum with IPv6 tunnels.

[Ilya Maximets]

On 2/21/24 19:06, Mike Pattrick wrote:
> This patch adopts the proposed RFC 6935 by allowing null UDP checksums
> even if the tunnel protocol is IPv6. This is already supported by Linux
> through the udp6zerocsumtx tunnel option. It is disabled by default and
> IPv6 tunnels are flagged as requiring a checksum, but this patch enables
> the user to set csum=false on IPv6 tunnels.

Hi, Mike.  That's an interesting feature.  Thanks!

Though the patch subject states 'userspace: ' the code seem to affect
tunnels in general, since it modifies tunnel flags for all tunnels.
More on that below.

> 
> Signed-off-by: Mike Pattrick 
> ---
> v2: Changed documentation, and added a NEWS item
> v3: NEWS file merge conflict
> ---
>  NEWS|  3 +++
>  lib/netdev-native-tnl.c |  2 +-
>  lib/netdev-vport.c  | 13 +++--
>  lib/netdev.h|  9 -
>  ofproto/tunnel.c| 11 +--
>  tests/tunnel.at |  6 +++---
>  vswitchd/vswitch.xml| 11 ---
>  7 files changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index c9e4064e6..3a75d3850 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ Post-v3.3.0
>   * Conntrack now supports 'random' flag for selecting ports in a range
> while natting and 'persistent' flag for selection of the IP address
> from a range.
> + * IPv6 UDP tunnels will now honour the csum option. Configuring the

All IPv6 UDP tunnels?

Also, double spaces between sentences.

> +   interface with "options:csum=false" now has the same effect in OVS

s/in OVS//

> +   as the udp6zerocsumtx option has with kernel UDP tunnels.

s/kernel/Linux kenrel/

>  
>  
>  v3.3.0 - 16 Feb 2024
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index dee9ab344..e8258bc4e 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -424,7 +424,7 @@ udp_build_header(const struct netdev_tunnel_config 
> *tnl_cfg,
>  udp = netdev_tnl_ip_build_header(data, params, IPPROTO_UDP, 0);
>  udp->udp_dst = tnl_cfg->dst_port;
>  
> -if (params->is_ipv6 || params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
> +if (params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {

Please add a tunnel-push-pop test that verifies that cehcksums
are skipped.

>  /* Write a value in now to mark that we should compute the checksum
>   * later. 0x is handy because it is transparent to the
>   * calculation. */
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 60caa02fb..f9a778988 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -702,7 +702,9 @@ set_tunnel_config(struct netdev *dev_, const struct smap 
> *args, char **errp)
>  tnl_cfg.dst_port = htons(atoi(node->value));
>  } else if (!strcmp(node->key, "csum") && has_csum) {
>  if (!strcmp(node->value, "true")) {
> -tnl_cfg.csum = true;
> +tnl_cfg.csum = NETDEV_TNL_CSUM_ENABLED;
> +} else if (!strcmp(node->value, "false")) {
> +tnl_cfg.csum = NETDEV_TNL_CSUM_DISABLED;
>  }
>  } else if (!strcmp(node->key, "seq") && has_seq) {
>  if (!strcmp(node->value, "true")) {
> @@ -850,6 +852,11 @@ set_tunnel_config(struct netdev *dev_, const struct smap 
> *args, char **errp)
>  }
>  }
>  
> +/* The default csum state for GRE is special. */

Please explain what does it mean in the comment.  And in the enum comment.

> +if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT && strstr(type, "gre")) {

The tnl_cfg is initialized with memset, but the enum is not guaranteed
to start with a zero value.

> +tnl_cfg.csum = NETDEV_TNL_CSUM_DEFAULT_GRE;
> +}
> +
>  enum tunnel_layers layers = tunnel_supported_layers(type, _cfg);
>  const char *full_type = (strcmp(type, "vxlan") ? type
>   : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE)
> @@ -1026,8 +1033,10 @@ get_tunnel_config(const struct netdev *dev, struct 
> smap *args)
>  }
>  }
>  
> -if (tnl_cfg->csum) {
> +if (tnl_cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
>  smap_add(args, "csum", "true");
> +} else if (tnl_cfg->csum == NETDEV_TNL_CSUM_DISABLED) {
> +smap_add(args, "csum", "false");
>  }
>  
>  if (tnl_cfg->set_seq) {
> diff --git a/lib/netdev.h b/lib/netdev.h
> index 67a8486bd..a79531e6d 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -111,6 +111,13 @@ enum netdev_srv6_flowlabel {
>  SRV6_FLOWLABEL_COMPUTE,
>  };
>  
> +enum netdev_tnl_csum {
> +NETDEV_TNL_CSUM_DEFAULT,
> +NETDEV_TNL_CSUM_ENABLED,
> +NETDEV_TNL_CSUM_DISABLED,
> +NETDEV_TNL_CSUM_DEFAULT_GRE,
> +};

Some comments for the values would be nice to have.

> +
>  /* Configuration specific to tunnels. */
>  struct netdev_tunnel_config {
>  ovs_be64 in_key;
> @@ -139,7 +146,7 @@ struct netdev_tunnel_config {
>  uint8_t tos;
>  bool tos_inherit;
>  

Re: [ovs-dev] [PATCH ovn] controller: Fix ofctrl memory usage underflow.

[Mark Michelson]

Thanks Ales, looks good to me.

Acked-by: Mark Michelson 

On 3/19/24 11:57, Ales Musil wrote:

The memory usage would be increased for size of sb_addrset_ref
struct, but decreased for the size of the struct + the name.
That would slowly lead to underflows in some cases.

Reported-at: https://issues.redhat.com/browse/FDP-507
Signed-off-by: Ales Musil 
---
  controller/ofctrl.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index f14cd79a8..0ef3b8366 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -1112,6 +1112,12 @@ sb_to_flow_size(const struct sb_to_flow *stf)
  return sizeof *stf;
  }
  
+static size_t

+sb_addrset_ref_size(const struct sb_addrset_ref *sar)
+{
+return sizeof *sar + strlen(sar->name) + 1;
+}
+
  static struct sb_to_flow *
  sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid)
  {
@@ -1181,8 +1187,8 @@ link_flow_to_sb(struct ovn_desired_flow_table *flow_table,
  }
  if (!found) {
  sar = xmalloc(sizeof *sar);
-mem_stats.sb_flow_ref_usage += sizeof *sar;
  sar->name = xstrdup(as_info->name);
+mem_stats.sb_flow_ref_usage += sb_addrset_ref_size(sar);
  hmap_init(>as_ip_to_flow_map);
  ovs_list_insert(>addrsets, >list_node);
  }
@@ -1568,7 +1574,7 @@ remove_flows_from_sb_to_flow(struct 
ovn_desired_flow_table *flow_table,
  free(itfn);
  }
  hmap_destroy(>as_ip_to_flow_map);
-mem_stats.sb_flow_ref_usage -= (sizeof *sar + strlen(sar->name) + 1);
+mem_stats.sb_flow_ref_usage -= sb_addrset_ref_size(sar);
  free(sar->name);
  free(sar);
  }


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [Patch ovn] docs: Remove ref. to "ovn-sbctl --no-wait".

[Mark Michelson]

On 3/20/24 06:23, Ales Musil wrote:

On Wed, Mar 13, 2024 at 5:40 PM Martin Kalcok 
wrote:


Couple places in the documentation reference "--wait" and "--no-wait"
options for "ovn-sbctl" but it doesn't support these options [0].

Trying, for example, "ovn-sbctl --no-wait init" exits with:

ovn-sbctl: --no-wait not supported

[0]
https://github.com/ovn-org/ovn/blob/63b35e2f6789f7843363c8f7a92430402bf989f9/utilities/ovn-sbctl.c#L127

Signed-off-by: Martin Kalcok 
---
  Documentation/intro/install/general.rst | 2 +-
  utilities/ovn-sbctl.8.xml   | 8 
  2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/intro/install/general.rst
b/Documentation/intro/install/general.rst
index ab6209482..6efb3a701 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -428,7 +428,7 @@ the first time after you create the databases with
ovsdb-tool, though running
  it at any time is harmless::

  $ ovn-nbctl --no-wait init
-$ ovn-sbctl --no-wait init
+$ ovn-sbctl init

  Start ``ovn-northd``, telling it to connect to the OVN db servers same
  Unix domain socket::
diff --git a/utilities/ovn-sbctl.8.xml b/utilities/ovn-sbctl.8.xml
index 81ab4131d..32035d051 100644
--- a/utilities/ovn-sbctl.8.xml
+++ b/utilities/ovn-sbctl.8.xml
@@ -123,10 +123,10 @@

  Instructs the daemon process to run one or more
ovn-sbctl
  commands described above and reply with the results of running
these
-commands. Accepts the --no-wait, --wait,
---timeout, --dry-run,
--oneline,
-and the options described under Table Formatting
Options
-in addition to the the command-specific options.
+commands. Accepts the --timeout,
--dry-run,
+--oneline, and the options described under
+Table Formatting Options in addition to the the
+command-specific options.


exit
--
2.40.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev



Looks good to me, thanks.

Acked-by: Ales Musil 



Thanks Martin and Ales. I merged this to main and all branches back to 
23.06.


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn branch-22.03] tests: Add helper for tcpdump.

[Mark Michelson]

Hi Ales, thanks for providing this patch. However, branch-22.03 is now 
in maintenance mode. It only is receiving patches for security issues 
and other high-severity issues. I'm not planning to merge this, but if 
there is a good reason that we should, I'm willing to hear it.


On 3/20/24 06:13, Ales Musil wrote:

The way how tcpdump was called in tests was inconsistent,
a lot fo the tests didn't even wait for the tcpdump to properly
start, some of them didn't redirect the stderr which could cause
leak into the test stderr and fail the test.

To prevent that add macro that starts tcpdump and properly
waits for the "listening" state, at the same time redirects
the stderr into separate file.

Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit e8ac18104df0bbd6579d8c62fd4282939631b878)
---
  tests/system-common-macros.at |  25 +++--
  tests/system-ovn-kmod.at  |  10 +-
  tests/system-ovn.at   | 193 +-
  3 files changed, 114 insertions(+), 114 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 28a9873d6..d4f334036 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -299,6 +299,18 @@ m4_define([OVS_START_L7],
 ]
  )
  
+# NETNS_START_TCPDUMP([namespace], [params], [name])

+#
+# Helper to properly start tcpdump and wait for the startup.
+# The tcpdump output is available in .tcpdump file.
+m4_define([NETNS_START_TCPDUMP],
+[
+ NETNS_DAEMONIZE([$1], [tcpdump -l $2 >$3.tcpdump 2>$3.stderr], [$3.pid])
+ OVS_WAIT_UNTIL([grep -q "listening" $3.stderr])
+]
+)
+
+
  # OVS_CHECK_VXLAN()
  #
  # Do basic check for vxlan functionality, skip the test if it's not there.
@@ -453,8 +465,7 @@ chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases
  chmod 775 /var/lib/dhcp
  chmod 664 /var/lib/dhcp/dhcpd6.leases
  
-NS_CHECK_EXEC([server], [tcpdump -nni s1 > pkt.pcap &])

-
+NETNS_START_TCPDUMP([server], [-nni s1], [server])
  NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid])
  ovn-nbctl --wait=hv sync
  
@@ -477,22 +488,20 @@ ovn-nbctl list logical_router_port rp-public > /tmp/rp-public

  ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
  ovn-nbctl set logical_router_port rp-sw1 options:prefix=false
  # Renew message
-NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 1 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew])
  # Reply message with Status OK
-NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix} > reply.pcap &])
+NETNS_START_TCPDUMP([server], [-c 1 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix}], [reply])
  
  OVS_WAIT_UNTIL([

-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew.tcpdump | wc -l)
  test "${total_pkts}" = "1"
  ])
  
  OVS_WAIT_UNTIL([

-total_pkts=$(cat reply.pcap | wc -l)
+total_pkts=$(cat reply.tcpdump | wc -l)
  test "${total_pkts}" = "1"
  ])
  
-kill $(pidof tcpdump)

-
  ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
  ovn-nbctl clear logical_router_port rp-sw0 ipv6_prefix
  OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut 
-c3-16)" = "[2001:1db8:]"])
diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index db4191f31..7122b909c 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -320,14 +320,10 @@ while True:
  NETNS_DAEMONIZE([server], [$PYTHON3 ./server.py > server.log], [server.pid])
  
  dnl Collect packets on server side.

-NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
-  'ip and (icmp or udp)' > server.tcpdump 2>server_err], 
[tcpdump0.pid])
-OVS_WAIT_UNTIL([grep "listening" server_err])
+NETNS_START_TCPDUMP([server], [-U -i server -vnne 'ip and (icmp or udp)'], 
[tcpdump-server])
  
  dnl Collect packets on client side.

-NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
-  'ip and (icmp or udp)' > client.tcpdump 2>client_err], 
[tcpdump1.pid])
-OVS_WAIT_UNTIL([grep "listening" client_err])
+NETNS_START_TCPDUMP([client], [-U -i client -vnne 'ip and (icmp or udp)'], 
[tcpdump-client])
  
  dnl Send two packets to the server with a short interval.

  dnl First packet should generate 'needs frag', the second should result in
@@ -345,7 +341,7 @@ time.sleep(5)
  NS_CHECK_EXEC([client], [$PYTHON3 ./client.py])
  
  dnl Expecting 2 outgoing packets and 2 fragments back - 8 lines total.

-OVS_WAIT_UNTIL([test "$(cat client.tcpdump | wc -l)" = "8"])
+OVS_WAIT_UNTIL([test "$(cat tcpdump-client.tcpdump | wc -l)" = "8"])
  
  ovn-appctl -t ovn-controller vlog/set info
  
diff --git a/tests/system-ovn.at b/tests/system-ovn.at

index 9e2507a7a..d66c9ab63 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -1602,12 +1602,11 @@ OVS_WAIT_UNTIL([
  ovn-nbctl --reject lb-add lb3 

Re: [ovs-dev] [PATCH ovn branch-23.09 1/2] tests: Add helper for tcpdump.

[Mark Michelson]

Thanks Ales, I pushed this and patch 2 to branch-23.09.

On 3/20/24 06:16, Ales Musil wrote:

The way how tcpdump was called in tests was inconsistent,
a lot fo the tests didn't even wait for the tcpdump to properly
start, some of them didn't redirect the stderr which could cause
leak into the test stderr and fail the test.

To prevent that add macro that starts tcpdump and properly
waits for the "listening" state, at the same time redirects
the stderr into separate file.

Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit e8ac18104df0bbd6579d8c62fd4282939631b878)
---
  tests/system-common-macros.at |  29 ++--
  tests/system-ovn-kmod.at  |  26 +--
  tests/system-ovn.at   | 312 ++
  3 files changed, 154 insertions(+), 213 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 8fe93cbdb..683f15fe3 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -271,6 +271,18 @@ m4_define([OVS_START_L7],
 ]
  )
  
+# NETNS_START_TCPDUMP([namespace], [params], [name])

+#
+# Helper to properly start tcpdump and wait for the startup.
+# The tcpdump output is available in .tcpdump file.
+m4_define([NETNS_START_TCPDUMP],
+[
+ NETNS_DAEMONIZE([$1], [tcpdump -l $2 >$3.tcpdump 2>$3.stderr], [$3.pid])
+ OVS_WAIT_UNTIL([grep -q "listening" $3.stderr])
+]
+)
+
+
  # OVS_CHECK_VXLAN()
  #
  # Do basic check for vxlan functionality, skip the test if it's not there.
@@ -437,8 +449,7 @@ chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases
  chmod 775 /var/lib/dhcp
  chmod 664 /var/lib/dhcp/dhcpd6.leases
  
-NS_CHECK_EXEC([server], [tcpdump -nni s1 > pkt.pcap &])

-
+NETNS_START_TCPDUMP([server], [-nni s1], [server])
  NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid])
  ovn-nbctl --wait=hv sync
  
@@ -456,17 +467,17 @@ prefix=$(ovn-nbctl list logical_router_port rp-public | awk -F/ '/ipv6_prefix/{p

  ovn-nbctl list logical_router_port rp-public > /tmp/rp-public
  
  # Wait for 2 renew on each port.

-NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 4 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew])
  # Reply message with Status OK
-NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix} > reply.pcap &])
+NETNS_START_TCPDUMP([server], [-c 4 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix}], [reply])
  
  OVS_WAIT_UNTIL([

-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew.tcpdump | wc -l)
  test "${total_pkts}" = "4"
  ])
  
  OVS_WAIT_UNTIL([

-total_pkts=$(cat reply.pcap | wc -l)
+total_pkts=$(cat reply.tcpdump | wc -l)
  test "${total_pkts}" = "4"
  ])
  
@@ -474,7 +485,7 @@ ovn-nbctl set logical_router_port rp-public options:prefix=false

  ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
  ovn-nbctl --wait=hv set logical_router_port rp-sw1 options:prefix=true
  sleep_sb
-NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 2 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew2])
  
  # Sleep enough to have solicit and renew being sent, then wait for 2 renew.

  # The reply to the request will usually be received as sb is sleeping.
@@ -482,12 +493,10 @@ NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 
ip6[[48:1]]=0x05 and ip6[[113:4]]=
  sleep 10
  wake_up_sb
  OVS_WAIT_UNTIL([
-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew2.tcpdump | wc -l)
  test "${total_pkts}" = "2"
  ])
  
-kill $(pidof tcpdump)

-
  ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
  ovn-nbctl clear logical_router_port rp-sw0 ipv6_prefix
  OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut 
-c3-16)" = "[2001:1db8:]"])
diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index 3a533d5e6..254a4b0a0 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -664,14 +664,11 @@ ovn-nbctl --wait=hv sync
  NETNS_DAEMONIZE([server], [nc -l -u 172.16.1.2 4242 > /dev/null], 
[server.pid])
  
  # Collect ICMP packets on client side

-NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
-icmp > client.pcap 2>client_err], [tcpdump0.pid])
-OVS_WAIT_UNTIL([grep "listening" client_err])
+NETNS_START_TCPDUMP([client], [-U -i client -vnne icmp], [tcpdump-client])
  
  # Collect UDP packets on server side

-NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
-'udp and ip[[6:2]] > 0 and not ip[[6]] = 64' > server.pcap 2>server_err], 
[tcpdump1.pid])
-OVS_WAIT_UNTIL([grep "listening" server_err])
+NETNS_START_TCPDUMP([server], [-U -i server -vnne \
+'udp and ip[[6:2]] > 0 and not ip[[6]] = 64'], [tcpdump-server])
  
  check ip netns exec client python3 << EOF

  import os
@@ 

Re: [ovs-dev] [PATCH ovn branch-23.06 1/2] tests: Add helper for tcpdump.

[Mark Michelson]

Thanks Ales, I pushed this and patch 2 to branch-23.06.

On 3/20/24 06:59, Ales Musil wrote:

The way how tcpdump was called in tests was inconsistent,
a lot fo the tests didn't even wait for the tcpdump to properly
start, some of them didn't redirect the stderr which could cause
leak into the test stderr and fail the test.

To prevent that add macro that starts tcpdump and properly
waits for the "listening" state, at the same time redirects
the stderr into separate file.

Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit e8ac18104df0bbd6579d8c62fd4282939631b878)
---
  tests/system-common-macros.at |  29 ++--
  tests/system-ovn-kmod.at  |  24 +--
  tests/system-ovn.at   | 312 ++
  3 files changed, 153 insertions(+), 212 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 8fe93cbdb..683f15fe3 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -271,6 +271,18 @@ m4_define([OVS_START_L7],
 ]
  )
  
+# NETNS_START_TCPDUMP([namespace], [params], [name])

+#
+# Helper to properly start tcpdump and wait for the startup.
+# The tcpdump output is available in .tcpdump file.
+m4_define([NETNS_START_TCPDUMP],
+[
+ NETNS_DAEMONIZE([$1], [tcpdump -l $2 >$3.tcpdump 2>$3.stderr], [$3.pid])
+ OVS_WAIT_UNTIL([grep -q "listening" $3.stderr])
+]
+)
+
+
  # OVS_CHECK_VXLAN()
  #
  # Do basic check for vxlan functionality, skip the test if it's not there.
@@ -437,8 +449,7 @@ chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases
  chmod 775 /var/lib/dhcp
  chmod 664 /var/lib/dhcp/dhcpd6.leases
  
-NS_CHECK_EXEC([server], [tcpdump -nni s1 > pkt.pcap &])

-
+NETNS_START_TCPDUMP([server], [-nni s1], [server])
  NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid])
  ovn-nbctl --wait=hv sync
  
@@ -456,17 +467,17 @@ prefix=$(ovn-nbctl list logical_router_port rp-public | awk -F/ '/ipv6_prefix/{p

  ovn-nbctl list logical_router_port rp-public > /tmp/rp-public
  
  # Wait for 2 renew on each port.

-NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 4 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew])
  # Reply message with Status OK
-NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix} > reply.pcap &])
+NETNS_START_TCPDUMP([server], [-c 4 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix}], [reply])
  
  OVS_WAIT_UNTIL([

-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew.tcpdump | wc -l)
  test "${total_pkts}" = "4"
  ])
  
  OVS_WAIT_UNTIL([

-total_pkts=$(cat reply.pcap | wc -l)
+total_pkts=$(cat reply.tcpdump | wc -l)
  test "${total_pkts}" = "4"
  ])
  
@@ -474,7 +485,7 @@ ovn-nbctl set logical_router_port rp-public options:prefix=false

  ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
  ovn-nbctl --wait=hv set logical_router_port rp-sw1 options:prefix=true
  sleep_sb
-NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 2 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew2])
  
  # Sleep enough to have solicit and renew being sent, then wait for 2 renew.

  # The reply to the request will usually be received as sb is sleeping.
@@ -482,12 +493,10 @@ NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 
ip6[[48:1]]=0x05 and ip6[[113:4]]=
  sleep 10
  wake_up_sb
  OVS_WAIT_UNTIL([
-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew2.tcpdump | wc -l)
  test "${total_pkts}" = "2"
  ])
  
-kill $(pidof tcpdump)

-
  ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
  ovn-nbctl clear logical_router_port rp-sw0 ipv6_prefix
  OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut 
-c3-16)" = "[2001:1db8:]"])
diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index 50fe8ad9d..66028992d 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -883,14 +883,11 @@ ovn-nbctl --wait=hv sync
  NETNS_DAEMONIZE([server], [nc -l -u 172.16.1.2 4242 > /dev/null], 
[server.pid])
  
  # Collect ICMP packets on client side

-NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
-icmp > client.pcap 2>client_err], [tcpdump0.pid])
-OVS_WAIT_UNTIL([grep "listening" client_err])
+NETNS_START_TCPDUMP([client], [-U -i client -vnne icmp], [tcpdump-client])
  
  # Collect UDP packets on server side

-NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
-'udp and ip[[6:2]] > 0 and not ip[[6]] = 64' > server.pcap 2>server_err], 
[tcpdump1.pid])
-OVS_WAIT_UNTIL([grep "listening" server_err])
+NETNS_START_TCPDUMP([server], [-U -i server -vnne \
+'udp and ip[[6:2]] > 0 and not ip[[6]] = 64'], [tcpdump-server])
  
  check ip netns exec client python3 << EOF

  import os
@@ 

Re: [ovs-dev] [PATCH v2] ovs-monitor-ipsec: LibreSwan autodetect paths.

[Ilya Maximets]

On 3/20/24 19:05, Mike Pattrick wrote:
> In v4.0, LibreSwan changed a default paths that had been hardcoded in
> ovs-monitor-ipsec, breaking some uses of this script. This patch adds
> support for both old and newer versions by auto detecting the version
> of LibreSwan and then choosing the correct path.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975039
> Reported-by: Qijun Ding 
> Fixes: d6afbc00d5b3 ("ipsec: Allow custom file locations.")
> Signed-off-by: Mike Pattrick 
> ---
> v2: Don't extract variables from ipsec script
> ---
>  ipsec/ovs-monitor-ipsec.in | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in
> index 7945162f9..6a71d4f2f 100755
> --- a/ipsec/ovs-monitor-ipsec.in
> +++ b/ipsec/ovs-monitor-ipsec.in
> @@ -21,6 +21,7 @@ import re
>  import subprocess
>  import sys
>  from string import Template
> +from packaging.version import parse

Hmm.  This is not part of a standard library, it's a new dependency
for the script.  We either need to add python3-packaging as a new
dependency or find a different way of checking.  The latter is likely
a better option.  Just parsing out the first number before the dot
and converting to integer might be an easier solution.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovs-monitor-ipsec: LibreSwan autodetect paths.

[Ilya Maximets]

On 3/20/24 19:48, Mike Pattrick wrote:
> On Wed, Mar 20, 2024 at 2:05 PM Mike Pattrick  wrote:
>>
>> In v4.0, LibreSwan changed a default paths that had been hardcoded in
>> ovs-monitor-ipsec, breaking some uses of this script. This patch adds
>> support for both old and newer versions by auto detecting the version
>> of LibreSwan and then choosing the correct path.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975039
>> Reported-by: Qijun Ding 
>> Fixes: d6afbc00d5b3 ("ipsec: Allow custom file locations.")
>> Signed-off-by: Mike Pattrick 
>> ---
>> v2: Don't extract variables from ipsec script
>> ---
> 
> Failed with 503 Service Unavailable
> 
> Recheck-request: github-robot

It is not going to work until the incident is resolved:
  https://status.canonical.com/

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovs-monitor-ipsec: LibreSwan autodetect paths.

[Mike Pattrick]

On Wed, Mar 20, 2024 at 2:05 PM Mike Pattrick  wrote:
>
> In v4.0, LibreSwan changed a default paths that had been hardcoded in
> ovs-monitor-ipsec, breaking some uses of this script. This patch adds
> support for both old and newer versions by auto detecting the version
> of LibreSwan and then choosing the correct path.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975039
> Reported-by: Qijun Ding 
> Fixes: d6afbc00d5b3 ("ipsec: Allow custom file locations.")
> Signed-off-by: Mike Pattrick 
> ---
> v2: Don't extract variables from ipsec script
> ---

Failed with 503 Service Unavailable

Recheck-request: github-robot

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] route-table: Avoid routes from non-standard routing tables.

[Ilya Maximets]

Currently, ovs-vswitchd is subscribed to all the routing changes in the
kernel.  On each change, it marks the internal routing table cache as
invalid, then resets it and dumps all the routes from the kernel from
scratch.  The reason for that is kernel routing updates not being
reliable in a sense that it's hard to tell which route is getting
removed or modified.  Userspace application has to track the order in
which route entries are dumped from the kernel.  Updates can get lost
or even duplicated and the kernel doesn't provide a good mechanism to
distinguish one route from another.  To my knowledge, dumping all the
routes from a kernel after each change is the only way to keep the
cache consistent.  Some more info can be found in the following never
addressed issues:
  https://bugzilla.redhat.com/1337860
  https://bugzilla.redhat.com/1337855

It seems to be believed that NetworkManager "mostly" does incremental
updates right.  But it is still not completely correct, will re-dump
the whole table in certain cases, and it takes a huge amount of very
complicated code to do the accounting and route comparisons.

Going back to ovs-vswitchd, it currently dumps routes from all the
routing tables.  If it will get conflicting routes from multiple
tables, the cache will not be useful.  The routing cache in userspace
is primarily used for checking the egress port for tunneled traffic
and this way also detecting link state changes for a tunnel port.
For userspace datapath it is used for actual routing of the packet
after sending to a native tunnel.
With kernel datapath we don't really have a mechanism to know which
routing table will actually be used by the kernel after encapsulation,
so our lookups on a cache may be incorrect because of this as well.

So, unless all the relevant routes are in the standard tables, the
lookup in userspace route cache is unreliable.

Luckily, most setups are not using any complicated routing in
non-standard tables that OVS has to be aware of.

It is possible, but unlikely, that standard routing tables are
completely empty while some other custom table is not, and all the OVS
tunnel traffic is directed to that table.  That would be the only
scenario where dumping non-standard tables would make sense.  But it
seems like this kind of setup will likely need a way to tell OVS from
which table the routes should be taken, or we'll need to dump routing
rules and keep a separate cache for each table, so we can first match
on rules and then lookup correct routes in a specific table.  I'm not
sure if trying to implement all that is justified.

For now, stop considering routes from non-standard tables to avoid
mixing different tables together and also wasting CPU resources.

This fixes a high CPU usage in ovs-vswitchd in case a BGP daemon is
running on a same host and in a same network namespace with OVS using
its own custom routing table.

Unfortunately, there seems to be no way to tell the kernel to send
updates only for particular tables.  So, we'll still receive and parse
all of them.  But they will not result in a full cache invalidation in
most cases.

Linux kernel v4.20 introduced filtering support for RTM_GETROUTE dumps.
So, we can make use of it and dump only standard tables when we get a
relevant route update.  NETLINK_GET_STRICT_CHK has to be enabled on
the socket for filtering to work.  There is no reason to not enable it
by default, if supported.  It is not used outside of NETLINK_ROUTE.

Fixes: f0e167f0dbad ("route-table: Handle route updates more robustly.")
Fixes: ea83a2fcd0d3 ("lib: Show tunnel egress interface in ovsdb")
Reported-at: https://github.com/openvswitch/ovs-issues/issues/185
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2022-October/052091.html
Signed-off-by: Ilya Maximets 
---

Version 2:
  * Changed log level for the NETLINK_GET_STRICT_CHK failure to
DBG if not supported and WARN if there is an actual failure.
  * While at it added rate limiting.  It is not necessary, but
doesn't hurt.

Note: GitHub actions will likely fail due to ongoing major outage
  of ppa.launchpad.net from where it installs 32bit toolchains.
  Will need to request a re-check once the incident is over.

 lib/netlink-protocol.h | 10 ++
 lib/netlink-socket.c   |  9 +
 lib/route-table.c  | 80 +-
 tests/system-route.at  | 64 +
 4 files changed, 147 insertions(+), 16 deletions(-)

diff --git a/lib/netlink-protocol.h b/lib/netlink-protocol.h
index 6eaa7035a..e4bb28ac9 100644
--- a/lib/netlink-protocol.h
+++ b/lib/netlink-protocol.h
@@ -155,6 +155,11 @@ enum {
 #define NLA_TYPE_MASK   ~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)
 #endif
 
+/* Introduced in v4.4. */
+#ifndef NLM_F_DUMP_FILTERED
+#define NLM_F_DUMP_FILTERED 0x20
+#endif
+
 /* These were introduced all together in 2.6.14.  (We want our programs to
  * support the newer kernel features even if compiled with older headers.) */

Re: [ovs-dev] [PATCH ovn] Fix broken link for LTS release.

[Mark Michelson]

I pushed this change to main and all branches back to 23.06.

On 3/18/24 13:14, Mark Michelson wrote:

Thanks for finding and fixing this!

Acked-by: Mark Michelson 

On 3/4/24 23:54, Igor Zhukov wrote:
I found the broken link at 
https://docs.ovn.org/en/latest/internals/release-process.html.


I believe the correct link is 
https://www.ovn.org/en/releases/#long-term-support


Signed-off-by: Igor Zhukov 
---
  Documentation/internals/release-process.rst | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst

index 26d3f8d4d..988257975 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -203,5 +203,5 @@ Contact
  Use d...@openvswitch.org to discuss the OVN development and release 
process.

-__ https://www.ovn.org/en/releases/long_term_support/
+__ https://www.ovn.org/en/releases/#long-term-support
  __ https://www.ovn.org




___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] ovs-monitor-ipsec: LibreSwan autodetect paths.

[Mike Pattrick]

In v4.0, LibreSwan changed a default paths that had been hardcoded in
ovs-monitor-ipsec, breaking some uses of this script. This patch adds
support for both old and newer versions by auto detecting the version
of LibreSwan and then choosing the correct path.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975039
Reported-by: Qijun Ding 
Fixes: d6afbc00d5b3 ("ipsec: Allow custom file locations.")
Signed-off-by: Mike Pattrick 
---
v2: Don't extract variables from ipsec script
---
 ipsec/ovs-monitor-ipsec.in | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in
index 7945162f9..6a71d4f2f 100755
--- a/ipsec/ovs-monitor-ipsec.in
+++ b/ipsec/ovs-monitor-ipsec.in
@@ -21,6 +21,7 @@ import re
 import subprocess
 import sys
 from string import Template
+from packaging.version import parse
 
 import ovs.daemon
 import ovs.db.idl
@@ -457,14 +458,25 @@ conn prevent_unencrypted_vxlan
 CERTKEY_PREFIX = "ovs_certkey_"
 
 def __init__(self, libreswan_root_prefix, args):
+# Collect version infromation
+self.IPSEC = libreswan_root_prefix + "/usr/sbin/ipsec"
+proc = subprocess.Popen([self.IPSEC, "--version"],
+stdout=subprocess.PIPE,
+encoding="latin1")
+pout, perr = proc.communicate()
+
+v = re.match("^Libreswan (.*)$", pout)
+if v and parse(v.group(1)) >= parse("4.0"):
+ipsec_d = args.ipsec_d if args.ipsec_d else "/var/lib/ipsec/nss"
+else:
+ipsec_d = args.ipsec_d if args.ipsec_d else "/etc/ipsec.d"
+
 ipsec_conf = args.ipsec_conf if args.ipsec_conf else "/etc/ipsec.conf"
-ipsec_d = args.ipsec_d if args.ipsec_d else "/etc/ipsec.d"
 ipsec_secrets = (args.ipsec_secrets if args.ipsec_secrets
 else "/etc/ipsec.secrets")
 ipsec_ctl = (args.ipsec_ctl if args.ipsec_ctl
 else "/run/pluto/pluto.ctl")
 
-self.IPSEC = libreswan_root_prefix + "/usr/sbin/ipsec"
 self.IPSEC_CONF = libreswan_root_prefix + ipsec_conf
 self.IPSEC_SECRETS = libreswan_root_prefix + ipsec_secrets
 self.IPSEC_D = "sql:" + libreswan_root_prefix + ipsec_d
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix busy loop when ofctrl is disconnected.

[Han Zhou]

On Wed, Mar 20, 2024 at 2:56 AM Dumitru Ceara  wrote:
>
> On 3/20/24 06:41, Han Zhou wrote:
> > ovn-controller runs at 100% cpu when OVS exits. This is because the
> > ofctrl_run is not called while ofctrl_wait is always called in the main
> > loop. Because of the missing ofctrl_run, it doesn't even detect that the
> > ofctrl connection is disconnected.
> >
> > This patch fixes the issue by always giving a chance to run ofctrl_run
> > as long as ofctrl_wait is called.
> >
> > Fixes: 1d6d953bf883 ("controller: Don't artificially limit group and
meter IDs to 16bit.")
> > Fixes: 94cbc59dc0f1 ("ovn-controller: Fix use of dangling pointers in
I-P runtime_data.")
> > Signed-off-by: Han Zhou 
> > ---
>
> Thanks for the patch, Han!  I tested it in a sandbox and it fixes the
> issue.  Looks good to me:
>
> Acked-by: Dumitru Ceara 
>
Thanks Dumitru. I applied to main and backported down to branch-23.06.

> Regards,
> Dumitru
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVN technical community meeting - March 19th

[Dumitru Ceara]

On 3/19/24 10:44, Dumitru Ceara wrote:
> Hi all,
> 
> The next OVN technical community meeting is scheduled to happen today,
> at 4PM UTC.
> 
> Feel free to add topics you might want to discuss today to the agenda
> doc below.  I added the ones saw being mentioned in other settings (IRC
> meeting) and what I thought might also be relevant.
> 
> Looking forward to seeing you in the meeting if you can make it.
> 

This is the link to the recording of the meeting:
https://youtu.be/mE1ByXeCQ7A

Notes:
https://docs.google.com/document/d/1dG4GwcYOSs4uArPGtOoaP5tH4KCto-GH_C3tIXSnZZ8/edit#heading=h.6gppr0ku5kz1

I'd like to suggest the following date/time for the next instance of the
meeting:

Monday, May 13th, 3PM UTC

Please let me know how this sounds to you.

Thanks,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH OVN v5 3/4] northd: DHCP Relay Agent support for overlay IPv4 subnets.

[0-day Robot]

References:  <20240320143958.39052-4-naveen.yerramn...@nutanix.com>
 

Bleep bloop.  Greetings Naveen Yerramneni, 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.


checkpatch:
WARNING: Line is 101 characters long (recommended limit is 79)
#132 FILE: northd/northd.c:237:
 * | R2 REG_DHCP_RELAY_DIP_IPV4  | X | | 0 |
|

Lines checked: 626, Warnings: 1, Errors: 0


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

Re: [ovs-dev] [PATCH] route-table: Avoid routes from non-standard routing tables.

[Aaron Conole]

Ilya Maximets  writes:

> On 3/19/24 20:56, Aaron Conole wrote:
>> Ilya Maximets  writes:
>> 
>>> Currently, ovs-vswitchd is subscribed to all the routing changes in the
>>> kernel.  On each change, it marks the internal routing table cache as
>>> invalid, then resets it and dumps all the routes from the kernel from
>>> scratch.  The reason for that is kernel routing updates not being
>>> reliable in a sense that it's hard to tell which route is getting
>>> removed or modified.  Userspace application has to track the order in
>>> which route entries are dumped from the kernel.  Updates can get lost
>>> or even duplicated and the kernel doesn't provide a good mechanism to
>>> distinguish one route from another.  To my knowledge, dumping all the
>>> routes from a kernel after each change is the only way to keep the
>>> cache consistent.  Some more info can be found in the following never
>>> addressed issues:
>>>   https://bugzilla.redhat.com/1337860
>>>   https://bugzilla.redhat.com/1337855
>>>
>>> It seems to be believed that NetworkManager "mostly" does incremental
>>> updates right.  But it is still not completely correct, will re-dump
>>> the whole table in certain cases, and it takes a huge amount of very
>>> complicated code to do the accounting and route comparisons.
>>>
>>> Going back to ovs-vswitchd, it currently dumps routes from all the
>>> routing tables.  If it will get conflicting routes from multiple
>>> tables, the cache will not be useful.  The routing cache in userspace
>>> is primarily used for checking the egress port for tunneled traffic
>>> and this way also detecting link state changes for a tunnel port.
>>> For userspace datapath it is used for actual routing of the packet
>>> after sending to a native tunnel.
>>> With kernel datapath we don't really have a mechanism to know which
>>> routing table will actually be used by the kernel after encapsulation,
>>> so our lookups on a cache may be incorrect because of this as well.
>>>
>>> So, unless all the relevant routes are in the standard tables, the
>>> lookup in userspace route cache is unreliable.
>>>
>>> Luckily, most setups are not using any complicated routing in
>>> non-standard tables that OVS has to be aware of.
>>>
>>> It is possible, but unlikely, that standard routing tables are
>>> completely empty while some other custom table is not, and all the OVS
>>> tunnel traffic is directed to that table.  That would be the only
>>> scenario where dumping non-standard tables would make sense.  But it
>>> seems like this kind of setup will likely need a way to tell OVS from
>>> which table the routes should be taken, or we'll need to dump routing
>>> rules and keep a separate cache for each table, so we can first match
>>> on rules and then lookup correct routes in a specific table.  I'm not
>>> sure if trying to implement all that is justified.
>>>
>>> For now, stop considering routes from non-standard tables to avoid
>>> mixing different tables together and also wasting CPU resources.
>>>
>>> This fixes a high CPU usage in ovs-vswitchd in case a BGP daemon is
>>> running on a same host and in a same network namespace with OVS using
>>> its own custom routing table.
>>>
>>> Unfortunately, there seems to be no way to tell the kernel to send
>>> updates only for particular tables.  So, we'll still receive and parse
>>> all of them.  But they will not result in a full cache invalidation in
>>> most cases.
>>>
>>> Linux kernel v4.20 introduced filtering support for RTM_GETROUTE dumps.
>>> So, we can make use of it and dump only standard tables when we get a
>>> relevant route update.  NETLINK_GET_STRICT_CHK has to be enabled on
>>> the socket for filtering to work.  There is no reason to not enable it
>>> by default, if supported.  It is not used outside of NETLINK_ROUTE.
>>>
>>> Fixes: f0e167f0dbad ("route-table: Handle route updates more robustly.")
>>> Fixes: ea83a2fcd0d3 ("lib: Show tunnel egress interface in ovsdb")
>>> Reported-at: https://github.com/openvswitch/ovs-issues/issues/185
>>> Reported-at: 
>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-October/052091.html
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>>  lib/netlink-protocol.h | 10 ++
>>>  lib/netlink-socket.c   |  8 +
>>>  lib/route-table.c  | 80 +-
>>>  tests/system-route.at  | 64 +
>>>  4 files changed, 146 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/lib/netlink-protocol.h b/lib/netlink-protocol.h
>>> index 6eaa7035a..e4bb28ac9 100644
>>> --- a/lib/netlink-protocol.h
>>> +++ b/lib/netlink-protocol.h
>>> @@ -155,6 +155,11 @@ enum {
>>>  #define NLA_TYPE_MASK   ~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)
>>>  #endif
>>>  
>>> +/* Introduced in v4.4. */
>>> +#ifndef NLM_F_DUMP_FILTERED
>>> +#define NLM_F_DUMP_FILTERED 0x20
>>> +#endif
>>> +
>>>  /* These were introduced all together in 2.6.14.  (We want our programs to
>>>   * support the newer kernel features even if 

[ovs-dev] [PATCH OVN v5 4/4] tests: DHCP Relay Agent support for overlay IPv4 subnets.

[Naveen Yerramneni]

Added tests for DHCP Relay feature.

Signed-off-by: Naveen Yerramneni 
---
 tests/atlocal.in|   3 +
 tests/ovn-northd.at |  38 +++
 tests/ovn.at| 256 
 tests/system-ovn.at | 148 +
 4 files changed, 445 insertions(+)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index 63d891b89..32d1c374e 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -187,6 +187,9 @@ fi
 # Set HAVE_DHCPD
 find_command dhcpd
 
+# Set HAVE_DHCLIENT
+find_command dhclient
+
 # Set HAVE_BFDD_BEACON
 find_command bfdd-beacon
 
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 7893b0540..c042ad381 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -12150,6 +12150,44 @@ check_row_count nb:QoS 0
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check DHCP RELAY])
+ovn_start NORTHD_TYPE
+
+check ovn-nbctl ls-add ls0
+check ovn-nbctl lsp-add ls0 ls0-port1
+check ovn-nbctl lsp-set-addresses ls0-port1 02:00:00:00:00:10
+check ovn-nbctl lr-add lr0
+check ovn-nbctl lrp-add lr0 lrp1 02:00:00:00:00:01 192.168.1.1/24
+check ovn-nbctl lsp-add ls0 lrp1-attachment
+check ovn-nbctl lsp-set-type lrp1-attachment router
+check ovn-nbctl lsp-set-addresses lrp1-attachment 00:00:00:00:ff:02
+check ovn-nbctl lsp-set-options lrp1-attachment router-port=lrp1
+check ovn-nbctl lrp-add lr0 lrp-ext 02:00:00:00:00:02 192.168.2.1/24
+
+dhcp_relay=$(ovn-nbctl create DHCP_Relay servers=172.16.1.1)
+check ovn-nbctl set Logical_Router_port lrp1 dhcp_relay=$dhcp_relay
+check ovn-nbctl set Logical_Switch ls0 
other_config:dhcp_relay_port=lrp1-attachment
+
+check ovn-nbctl --wait=sb sync
+
+ovn-sbctl lflow-list > lflows
+AT_CAPTURE_FILE([lflows])
+
+AT_CHECK([grep -e "DHCP_RELAY_" lflows | sed 's/table=../table=??/'], [0], [dnl
+  table=??(lr_in_ip_input ), priority=110  , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && ip.frag == 0 && udp.src == 
68 && udp.dst == 67), action=(reg9[[7]] = dhcp_relay_req_chk(192.168.1.1, 
172.16.1.1);next; /* DHCP_RELAY_REQ */)
+  table=??(lr_in_ip_input ), priority=110  , match=(ip4.src == 172.16.1.1 
&& ip4.dst == 192.168.1.1 && ip.frag == 0 && udp.src == 67 && udp.dst == 67), 
action=(next;/* DHCP_RELAY_RESP */)
+  table=??(lr_in_dhcp_relay_req), priority=100  , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 
67 && reg9[[7]]), 
action=(ip4.src=192.168.1.1;ip4.dst=172.16.1.1;udp.src=67;next; /* 
DHCP_RELAY_REQ */)
+  table=??(lr_in_dhcp_relay_req), priority=1, match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 
67 && reg9[[7]] == 0), action=(drop; /* DHCP_RELAY_REQ */)
+  table=??(lr_in_dhcp_relay_resp_chk), priority=100  , match=(ip4.src == 
172.16.1.1 && ip4.dst == 192.168.1.1 && udp.src == 67 && udp.dst == 67), 
action=(reg2 = ip4.dst;reg9[[8]] = dhcp_relay_resp_chk(192.168.1.1, 
172.16.1.1);next;/* DHCP_RELAY_RESP */)
+  table=??(lr_in_dhcp_relay_resp), priority=100  , match=(ip4.src == 
172.16.1.1 && reg2 == 192.168.1.1 && udp.src == 67 && udp.dst == 67 && 
reg9[[8]]), action=(ip4.src=192.168.1.1;udp.dst=68;outport="lrp1";output; /* 
DHCP_RELAY_RESP */)
+  table=??(lr_in_dhcp_relay_resp), priority=1, match=(ip4.src == 
172.16.1.1 && reg2 == 192.168.1.1 && udp.src == 67 && udp.dst == 67 && 
reg9[[8]] == 0), action=(drop; /* DHCP_RELAY_RESP */)
+  table=??(ls_in_l2_lkup  ), priority=100  , match=(inport == "ls0-port1" 
&& eth.src == 02:00:00:00:00:10 && ip4.src == 0.0.0.0 && ip4.dst == 
255.255.255.255 && udp.src == 68 && udp.dst == 67), 
action=(eth.dst=02:00:00:00:00:01;outport="lrp1-attachment";next;/* 
DHCP_RELAY_REQ */)
+])
+
+AT_CLEANUP
+])
+
 AT_SETUP([NB_Global and SB_Global incremental processing])
 
 ovn_start
diff --git a/tests/ovn.at b/tests/ovn.at
index 32e3d8b13..c2570167c 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1672,6 +1672,40 @@ reg1[[0]] = put_dhcp_opts(offerip=1.2.3.4, 
domain_name=1.2.3.4);
 reg1[[0]] = put_dhcp_opts(offerip=1.2.3.4, domain_search_list=1.2.3.4);
 DHCPv4 option domain_search_list requires string value.
 
+#dhcp_relay_req_chk
+reg9[[7]] = dhcp_relay_req_chk(192.168.1.1, 172.16.1.1);
+encodes as 
controller(userdata=00.00.00.1c.00.00.00.00.80.01.08.08.00.00.00.07.c0.a8.01.01.ac.10.01.01,pause)
+
+reg9[[7]] = dhcp_relay_req_chk(192.168.1.1,172.16.1.1);
+formats as reg9[[7]] = dhcp_relay_req_chk(192.168.1.1, 172.16.1.1);
+encodes as 
controller(userdata=00.00.00.1c.00.00.00.00.80.01.08.08.00.00.00.07.c0.a8.01.01.ac.10.01.01,pause)
+
+reg9[[7..8]] = dhcp_relay_req_chk(192.168.1.1, 172.16.1.1);
+Cannot use 2-bit field reg9[[7..8]] where 1-bit field is required.
+
+reg9[[7]] = dhcp_relay_req_chk("192.168.1.1", "172.16.1.1");
+Syntax error at `"192.168.1.1"' expecting IPv4 dhcp relay and server ips.
+
+reg9[[7]] = dhcp_relay_req_chk(192.168.1, 172.16.1.1);
+Invalid numeric constant.
+

[ovs-dev] [PATCH OVN v5 3/4] northd: DHCP Relay Agent support for overlay IPv4 subnets.

[Naveen Yerramneni]

NB SCHEMA CHANGES
-
  1. New DHCP_Relay table
  "DHCP_Relay": {
"columns": {
"name": {"type": "string"},
"servers": {"type": {"key": "string",
   "min": 0,
   "max": 1}},
"external_ids": {
"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}}},
"options": {"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}},
"isRoot": true},
  2. New column to Logical_Router_Port table
  "dhcp_relay": {"type": {"key": {"type": "uuid",
"refTable": "DHCP_Relay",
"refType": "strong"},
"min": 0,
"max": 1}},

NEW PIPELINE STAGES
---
Following stage is added for DHCP relay feature.
Some of the flows are fitted into the existing pipeline tages.
  1. lr_in_dhcp_relay_req
   - This stage process the DHCP request packets coming from DHCP clients.
   - DHCP request packets for which dhcp_relay_req_chk action
 (which gets applied in ip input stage) is successful are forwarded to 
DHCP server.
   - DHCP request packets for which dhcp_relay_req_chk action is 
unsuccessful gets dropped.
  2. lr_in_dhcp_relay_resp_chk
   - This stage applied the dhcp_relay_resp_chk action for  DHCP response 
packets coming
 from the DHCP server.
  3. lr_in_dhcp_relay_resp
   - DHCP response packets for which dhcp_relay_resp_chk is sucessful are 
forwarded
 to the DHCP clients.
   - DHCP response packets for which dhcp_relay_resp_chk is unsucessful 
gets dropped.

REGISTRY USAGE
---
  - reg9[7] : To store the result of dhcp_relay_req_chk action.
  - reg9[8] : To store the result of dhcp_relay_resp_chk action.
  - reg2 : To store the original dest ip for DHCP response packets.
   This is required to properly match the packets in
   lr_in_dhcp_relay_resp stage since dhcp_relay_resp_chk action
   changes the dest ip.

FLOWS
-

Following are the flows added when DHCP Relay is configured on one overlay 
subnet,
one additonal flow is added in ls_in_l2_lkup table for each VM part of the 
subnet.

  1. table=27(ls_in_l2_lkup  ), priority=100  , match=(inport ==  
&& eth.src ==  && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && 
udp.src == 68 && udp.dst == 67),
 action=(eth.dst=;outport=;next;/* DHCP_RELAY_REQ */)
  2. table=3 (lr_in_ip_input ), priority=110  , match=(inport ==  && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && ip.frag == 0 && udp.src == 
68 && udp.dst == 67),
 action=(reg9[7] = dhcp_relay_req_chk(, );next; /* 
DHCP_RELAY_REQ */)
  3. table=3 (lr_in_ip_input ), priority=110  , match=(ip4.src == 
 && ip4.dst ==  && udp.src == 67 && udp.dst == 67), 
action=(next;/* DHCP_RELAY_RESP */)
  4. table=4 (lr_in_dhcp_relay_req), priority=100  , match=(inport == "lrp1" && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 
67 && reg9[7]),
 action=(ip4.src=;ip4.dst=;udp.src=67;next; /* 
DHCP_RELAY_REQ */)
  5. table=4 (lr_in_dhcp_relay_req), priority=1, match=(inport ==  && 
ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 
67 && reg9[7] == 0),
 action=(drop; /* DHCP_RELAY_REQ */)
  6. table=18(lr_in_dhcp_relay_resp_chk), priority=100  , match=(ip4.src == 
 && ip4.dst ==  && ip.frag == 0 && udp.src == 67 && udp.dst 
== 67),
 action=(reg2 = ip4.dst;reg9[8] = dhcp_relay_resp_chk(, 
);next;/* DHCP_RELAY_RESP */)
  7. table=19(lr_in_dhcp_relay_resp), priority=100  , match=(ip4.src == 
 && reg2 ==  && udp.src == 67 && udp.dst == 67 && reg9[8]),
 action=(ip4.src=;udp.dst=68;outport=;output; /* DHCP_RELAY_RESP 
*/)
  8. table=19(lr_in_dhcp_relay_resp), priority=1, match=(ip4.src == 
 && reg2 ==  && udp.src == 67 && udp.dst == 67 && reg9[8] 
== 0), action=(drop; /* DHCP_RELAY_RESP */)

Commands to enable the feature
--
  ovn-nbctl create DHCP_Relay name= servers=
  ovn-nbctl set Logical_Router_port  dhcp_relay=
  ovn-nbctl set Logical_Switch  
other_config:dhcp_relay_port=

Limitations:

  - All OVN features that needs IP address to be configured on logical port 
(like proxy arp, etc)
will not be supported for overlay subnets on which DHCP relay is enabled.

Signed-off-by: Naveen Yerramneni 
Co-authored-by: Huzaifa Calcuttawala 
Signed-off-by: Huzaifa Calcuttawala 
CC: Mary Manohar 
---
 northd/northd.c  | 271 ++-
 northd/northd.h  |  41 +++
 ovn-nb.ovsschema |  19 +++-
 ovn-nb.xml   |  39 +++
 tests/ovn.at |   2 +-
 5 files changed, 349 insertions(+), 23 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 

[ovs-dev] [PATCH OVN v5 2/4] controller: DHCP Relay Agent support for overlay IPv4 subnets.

[Naveen Yerramneni]

Added changes in pinctrl to process DHCP Relay opcodes:
  - ACTION_OPCODE_DHCP_RELAY_REQ_CHK: For request packets
  - ACTION_OPCODE_DHCP_RELAY_RESP_CHK: For response packet

Signed-off-by: Naveen Yerramneni 
---
 controller/pinctrl.c | 596 ++-
 lib/ovn-l7.h |   2 +
 2 files changed, 529 insertions(+), 69 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 2d3595cd2..11a5cac62 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -2017,6 +2017,514 @@ is_dhcp_flags_broadcast(ovs_be16 flags)
 return flags & htons(DHCP_BROADCAST_FLAG);
 }
 
+static const char *dhcp_msg_str[] = {
+[0] = "INVALID",
+[DHCP_MSG_DISCOVER] = "DISCOVER",
+[DHCP_MSG_OFFER] = "OFFER",
+[DHCP_MSG_REQUEST] = "REQUEST",
+[OVN_DHCP_MSG_DECLINE] = "DECLINE",
+[DHCP_MSG_ACK] = "ACK",
+[DHCP_MSG_NAK] = "NAK",
+[OVN_DHCP_MSG_RELEASE] = "RELEASE",
+[OVN_DHCP_MSG_INFORM] = "INFORM"
+};
+
+static bool
+dhcp_relay_is_msg_type_supported(uint8_t msg_type)
+{
+return (msg_type >= DHCP_MSG_DISCOVER && msg_type <= OVN_DHCP_MSG_RELEASE);
+}
+
+static const char *dhcp_msg_str_get(uint8_t msg_type)
+{
+if (!dhcp_relay_is_msg_type_supported(msg_type)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "Unknown DHCP msg type: %u", msg_type);
+return "UNKNOWN";
+}
+return dhcp_msg_str[msg_type];
+}
+
+static const struct dhcp_header *
+dhcp_get_hdr_from_pkt(struct dp_packet *pkt_in, const char **in_dhcp_pptr,
+  const char *end)
+{
+/* Validate the DHCP request packet.
+ * Format of the DHCP packet is
+ * ---
+ *| UDP HEADER | DHCP HEADER | 4 Byte DHCP Cookie | DHCP OPTIONS(var len) |
+ * ---
+ */
+
+*in_dhcp_pptr = dp_packet_get_udp_payload(pkt_in);
+if (*in_dhcp_pptr == NULL) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "DHCP: Invalid or incomplete DHCP packet received");
+return NULL;
+}
+
+const struct dhcp_header *dhcp_hdr
+= (const struct dhcp_header *) *in_dhcp_pptr;
+(*in_dhcp_pptr) += sizeof *dhcp_hdr;
+if (*in_dhcp_pptr > end) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "DHCP: Invalid or incomplete DHCP packet received, "
+ "bad data length");
+return NULL;
+}
+
+if (dhcp_hdr->htype != 0x1) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "DHCP: Packet is recieved with "
+"unsupported hardware type");
+return NULL;
+}
+
+if (dhcp_hdr->hlen != 0x6) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "DHCP: Packet is recieved with "
+"unsupported hardware length");
+return NULL;
+}
+
+/* DHCP options follow the DHCP header. The first 4 bytes of the DHCP
+ * options is the DHCP magic cookie followed by the actual DHCP options.
+ */
+ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
+if ((*in_dhcp_pptr) + sizeof magic_cookie > end ||
+get_unaligned_be32((const void *) (*in_dhcp_pptr)) != magic_cookie) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "DHCP: Magic cookie not present in the DHCP packet");
+return NULL;
+}
+
+(*in_dhcp_pptr) += sizeof magic_cookie;
+
+return dhcp_hdr;
+}
+
+static void
+dhcp_parse_options(const char **in_dhcp_pptr, const char *end,
+  const uint8_t **dhcp_msg_type_pptr, ovs_be32 *request_ip_ptr,
+  bool *ipxe_req_ptr, ovs_be32 *server_id_ptr,
+  ovs_be32 *netmask_ptr, ovs_be32 *router_ip_ptr)
+{
+while ((*in_dhcp_pptr) < end) {
+const struct dhcp_opt_header *in_dhcp_opt =
+(const struct dhcp_opt_header *) *in_dhcp_pptr;
+if (in_dhcp_opt->code == DHCP_OPT_END) {
+break;
+}
+if (in_dhcp_opt->code == DHCP_OPT_PAD) {
+(*in_dhcp_pptr) += 1;
+continue;
+}
+(*in_dhcp_pptr) += sizeof *in_dhcp_opt;
+if ((*in_dhcp_pptr) > end) {
+break;
+}
+(*in_dhcp_pptr) += in_dhcp_opt->len;
+if ((*in_dhcp_pptr) > end) {
+break;
+}
+
+switch (in_dhcp_opt->code) {
+case DHCP_OPT_MSG_TYPE:
+if (dhcp_msg_type_pptr && in_dhcp_opt->len == 1) {
+*dhcp_msg_type_pptr = DHCP_OPT_PAYLOAD(in_dhcp_opt);
+}
+break;
+case DHCP_OPT_REQ_IP:
+if (request_ip_ptr && in_dhcp_opt->len == 4) {
+*request_ip_ptr = get_unaligned_be32(
+  DHCP_OPT_PAYLOAD(in_dhcp_opt));
+}
+

[ovs-dev] [PATCH OVN v5 1/4] actions: DHCP Relay Agent support for overlay IPv4 subnets.

[Naveen Yerramneni]

NEW OVN ACTIONS
---
  1. dhcp_relay_req_chk(, )
   - This action executes on the source node on which the DHCP request 
originated.
   - This action relays the DHCP request coming from client to the server.
 Relay-ip is used to update GIADDR in the DHCP header.
  2. dhcp_relay_resp_chk(, )
   - This action executes on the first node (RC node) which processes
 the DHCP response from the server.
   - This action updates  the destination MAC and destination IP so that 
the response
 can be forwarded to the appropriate node from which request was 
originated.
   - Relay-ip, server-ip are used to validate GIADDR and SERVER ID in the 
DHCP payload.

Signed-off-by: Naveen Yerramneni 
---
 include/ovn/actions.h |  27 
 lib/actions.c | 149 ++
 utilities/ovn-trace.c |  67 +++
 3 files changed, 243 insertions(+)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index dcacbb1ff..8d0c6b9fa 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -96,6 +96,8 @@ struct collector_set_ids;
 OVNACT(LOOKUP_ND_IP,  ovnact_lookup_mac_bind_ip) \
 OVNACT(PUT_DHCPV4_OPTS,   ovnact_put_opts)\
 OVNACT(PUT_DHCPV6_OPTS,   ovnact_put_opts)\
+OVNACT(DHCPV4_RELAY_REQ_CHK,  ovnact_dhcp_relay)  \
+OVNACT(DHCPV4_RELAY_RESP_CHK, ovnact_dhcp_relay)  \
 OVNACT(SET_QUEUE, ovnact_set_queue)   \
 OVNACT(DNS_LOOKUP,ovnact_result)  \
 OVNACT(LOG,   ovnact_log) \
@@ -396,6 +398,15 @@ struct ovnact_put_opts {
 size_t n_options;
 };
 
+/* OVNACT_DHCP_RELAY. */
+struct ovnact_dhcp_relay {
+struct ovnact ovnact;
+int family;
+struct expr_field dst;  /* 1-bit destination field. */
+ovs_be32 relay_ipv4;
+ovs_be32 server_ipv4;
+};
+
 /* Valid arguments to SET_QUEUE action.
  *
  * QDISC_MIN_QUEUE_ID is the default queue, so user-defined queues should
@@ -772,6 +783,22 @@ enum action_opcode {
 
 /* multicast group split buffer action. */
 ACTION_OPCODE_MG_SPLIT_BUF,
+
+/* "dhcp_relay_req_chk(relay_ip, server_ip)".
+ *
+ * Arguments follow the action_header, in this format:
+ *   - The 32-bit DHCP relay IP.
+ *   - The 32-bit DHCP server IP.
+ */
+ACTION_OPCODE_DHCP_RELAY_REQ_CHK,
+
+/* "dhcp_relay_resp_chk(relay_ip, server_ip)".
+ *
+ * Arguments follow the action_header, in this format:
+ *   - The 32-bit DHCP relay IP.
+ *   - The 32-bit DHCP server IP.
+ */
+ACTION_OPCODE_DHCP_RELAY_RESP_CHK,
 };
 
 /* Header. */
diff --git a/lib/actions.c b/lib/actions.c
index 71615fc53..d4f4ec2d0 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2706,6 +2706,149 @@ ovnact_controller_event_free(struct 
ovnact_controller_event *event)
 free_gen_options(event->options, event->n_options);
 }
 
+static void
+format_DHCPV4_RELAY_REQ_CHK(const struct ovnact_dhcp_relay *dhcp_relay,
+struct ds *s)
+{
+expr_field_format(_relay->dst, s);
+ds_put_format(s, " = dhcp_relay_req_chk("IP_FMT", "IP_FMT");",
+  IP_ARGS(dhcp_relay->relay_ipv4),
+  IP_ARGS(dhcp_relay->server_ipv4));
+}
+
+static void
+parse_dhcp_relay_req_chk(struct action_context *ctx,
+   const struct expr_field *dst,
+   struct ovnact_dhcp_relay *dhcp_relay)
+{
+/* Skip dhcp_relay_req_chk( */
+lexer_force_match(ctx->lexer, LEX_T_LPAREN);
+
+/* Validate that the destination is a 1-bit, modifiable field. */
+char *error = expr_type_check(dst, 1, true, ctx->scope);
+if (error) {
+lexer_error(ctx->lexer, "%s", error);
+free(error);
+return;
+}
+dhcp_relay->dst = *dst;
+
+/* Parse relay ip and server ip. */
+if (ctx->lexer->token.format == LEX_F_IPV4) {
+dhcp_relay->family = AF_INET;
+dhcp_relay->relay_ipv4 = ctx->lexer->token.value.ipv4;
+lexer_get(ctx->lexer);
+lexer_match(ctx->lexer, LEX_T_COMMA);
+if (ctx->lexer->token.format == LEX_F_IPV4) {
+dhcp_relay->family = AF_INET;
+dhcp_relay->server_ipv4 = ctx->lexer->token.value.ipv4;
+lexer_get(ctx->lexer);
+} else {
+lexer_syntax_error(ctx->lexer, "expecting IPv4 dhcp server ip");
+return;
+}
+} else {
+  lexer_syntax_error(ctx->lexer, "expecting IPv4 dhcp relay "
+  "and server ips");
+  return;
+}
+lexer_force_match(ctx->lexer, LEX_T_RPAREN);
+}
+
+static void
+encode_DHCPV4_RELAY_REQ_CHK(const struct ovnact_dhcp_relay *dhcp_relay,
+const struct ovnact_encode_params *ep,
+struct ofpbuf *ofpacts)
+{
+struct mf_subfield dst = expr_resolve_field(_relay->dst);
+size_t oc_offset = encode_start_controller_op(
+

[ovs-dev] [PATCH OVN v5 0/4] DHCP Relay Agent support for overlay subnets.

[Naveen Yerramneni]

This patch contains changes to enable DHCP Relay Agent support for overlay 
subnets.

USE CASE:
--
  - Enable IP address assignment for overlay subnets from the centralized 
DHCP server present in the underlay network.

PREREQUISITES
--
  - Logical Router Port IP should be assigned (statically) from the same 
overlay subnet which is managed by DHCP server.
  - LRP IP is used for GIADRR field when relaying the DHCP packets and also 
same IP needs to be configured as default gateway for the overlay subnet.
  - Overlay subnets managed by external DHCP server are expected to be 
directly reachable from the underlay network.

EXPECTED PACKET FLOW:
--
Following is the expected packet flow inorder to support DHCP rleay 
functionality in OVN.
  1. DHCP client originates DHCP discovery (broadcast).
  2. DHCP relay (running on the OVN) receives the broadcast and forwards 
the packet to the DHCP server by converting it to unicast.
 While forwarding the packet, it updates the GIADDR in DHCP header to 
its interface IP on which DHCP packet is received and increments hop count.
  3. DHCP server uses GIADDR field to decide the IP address pool from which 
IP has to be assigned and DHCP offer is sent to the same IP (GIADDR).
  4. DHCP relay agent forwards the offer to the client.
  5. DHCP client sends DHCP request (broadcast) packet.
  6. DHCP relay (running on the OVN) receives the broadcast and forwards 
the packet to the DHCP server by converting it to unicast.
 While forwarding the packet, it updates the GIADDR in DHCP header to 
its interface IP on which DHCP packet is received.
  7. DHCP Server sends the ACK packet.
  8. DHCP relay agent forwards the ACK packet to the client.
  9. All the future renew/release packets are directly exchanged between 
DHCP client and DHCP server.

OVN DHCP RELAY PACKET FLOW:

To add DHCP Relay support on OVN, we need to replicate all the behavior 
described above using distributed logical switch and logical router.
At, highlevel packet flow is distributed among Logical Switch and Logical 
Router on source node (where VM is deployed) and redirect chassis(RC) node.
  1. Request packet gets processed on the source node where VM is deployed 
and relays the packet to DHCP server.
  2. Response packet is first processed on RC node (which first recieves 
the packet from underlay network). RC node forwards the packet to the right 
node by filling in the dest MAC and IP.

OVN Packet flow with DHCP relay is explained below.
  1. DHCP client (VM) sends the DHCP discover packet (broadcast).
  2. Logical switch converts the packet to L2 unicast by setting the 
destination MAC to LRP's MAC
  3. Logical Router receives the packet and redirects it to the OVN 
controller.
  4. OVN controller updates the required information(GIADDR, HOP count) in 
the DHCP payload after doing the required checks. If any check fails, packet is 
dropped.
  5. Logical Router converts the packet to L3 unicast and forwards it to 
the server. This packets gets routed like any other packet (via RC node).
  6. Server replies with DHCP offer.
  7. RC node processes the DHCP offer and forwards it to the OVN controller.
  8. OVN controller does sanity checks and  updates the destination MAC 
(available in DHCP header), destination IP (available in DHCP header) and 
reinjects the packet to datapath.
 If any check fails, packet is dropped.
  9. Logical router updates the source IP and port and forwards the packet 
to logical switch.
  10. Logical switch delivers the packet to the DHCP client.
  11. Similar steps are performed for Request and Ack packets.
  12. All the future renew/release packets are directly exchanged between 
DHCP client and DHCP server

NEW OVN ACTIONS
---
  1. dhcp_relay_req_chk(, )
  - This action executes on the source node on which the DHCP request 
originated.
  - This action relays the DHCP request coming from client to the 
server. Relay-ip is used to update GIADDR in the DHCP header.
  2. dhcp_relay_resp_chk(, )
  - This action executes on the first node (RC node) which processes 
the DHCP response from the server.
  - This action updates  the destination MAC and destination IP so that 
the response can be forwarded to the appropriate node from which request was 
originated.
  - Relay-ip, server-ip are used to validate GIADDR and SERVER ID in 
the DHCP payload.

FLOWS
-
Following are the flows added when DHCP Relay is configured on one overlay 
subnet, one additonal flow is added in ls_in_l2_lkup table for each VM part of 
the subnet.

  1. table=27(ls_in_l2_lkup  ), priority=100  , match=(inport == 
 && eth.src ==  && ip4.src == 0.0.0.0 && ip4.dst == 

Re: [ovs-dev] [PATCH] ovs-monitor-ipsec: LibreSwan autodetect paths.

[Mike Pattrick]

On Tue, Mar 19, 2024 at 5:35 PM Ilya Maximets  wrote:
>
> On 3/13/24 22:54, Mike Pattrick wrote:
> > In v4.0, LibreSwan changed a default paths that had been hardcoded in
> > ovs-monitor-ipsec, breaking some uses of this script. This patch adds
> > support for both old and newer versions by auto detecting the location
> > of these paths from LibreSwan shell script environment variables.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975039
> > Reported-by: Qijun Ding 
> > Fixes: d6afbc00d5b3 ("ipsec: Allow custom file locations.")
> > Signed-off-by: Mike Pattrick 
> > ---
> >  ipsec/ovs-monitor-ipsec.in | 31 +++
> >  1 file changed, 27 insertions(+), 4 deletions(-)
> >
>
> Hi, Mike.  Thanks for working on this!
>
> Though using the knowledge that /usr/sbin/ipsec is a shell script
> and that it defines particular variables inside seems like a hack.
>
> Maybe we can just check the version instead?  We know that default
> nss path changed in 4.0.

My motivation for this method was because these paths could be changed
easier than reimplementing the ipsec script, by the maintainers or
downstream distributions. But there's nothing stopping us from
addressing any future changes as they happen, I'll resend with just a
fix for 4.0.

-M

>
> Best regards, Ilya Maximets.
>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] route-table: Avoid routes from non-standard routing tables.

[Ilya Maximets]

On 3/19/24 20:56, Aaron Conole wrote:
> Ilya Maximets  writes:
> 
>> Currently, ovs-vswitchd is subscribed to all the routing changes in the
>> kernel.  On each change, it marks the internal routing table cache as
>> invalid, then resets it and dumps all the routes from the kernel from
>> scratch.  The reason for that is kernel routing updates not being
>> reliable in a sense that it's hard to tell which route is getting
>> removed or modified.  Userspace application has to track the order in
>> which route entries are dumped from the kernel.  Updates can get lost
>> or even duplicated and the kernel doesn't provide a good mechanism to
>> distinguish one route from another.  To my knowledge, dumping all the
>> routes from a kernel after each change is the only way to keep the
>> cache consistent.  Some more info can be found in the following never
>> addressed issues:
>>   https://bugzilla.redhat.com/1337860
>>   https://bugzilla.redhat.com/1337855
>>
>> It seems to be believed that NetworkManager "mostly" does incremental
>> updates right.  But it is still not completely correct, will re-dump
>> the whole table in certain cases, and it takes a huge amount of very
>> complicated code to do the accounting and route comparisons.
>>
>> Going back to ovs-vswitchd, it currently dumps routes from all the
>> routing tables.  If it will get conflicting routes from multiple
>> tables, the cache will not be useful.  The routing cache in userspace
>> is primarily used for checking the egress port for tunneled traffic
>> and this way also detecting link state changes for a tunnel port.
>> For userspace datapath it is used for actual routing of the packet
>> after sending to a native tunnel.
>> With kernel datapath we don't really have a mechanism to know which
>> routing table will actually be used by the kernel after encapsulation,
>> so our lookups on a cache may be incorrect because of this as well.
>>
>> So, unless all the relevant routes are in the standard tables, the
>> lookup in userspace route cache is unreliable.
>>
>> Luckily, most setups are not using any complicated routing in
>> non-standard tables that OVS has to be aware of.
>>
>> It is possible, but unlikely, that standard routing tables are
>> completely empty while some other custom table is not, and all the OVS
>> tunnel traffic is directed to that table.  That would be the only
>> scenario where dumping non-standard tables would make sense.  But it
>> seems like this kind of setup will likely need a way to tell OVS from
>> which table the routes should be taken, or we'll need to dump routing
>> rules and keep a separate cache for each table, so we can first match
>> on rules and then lookup correct routes in a specific table.  I'm not
>> sure if trying to implement all that is justified.
>>
>> For now, stop considering routes from non-standard tables to avoid
>> mixing different tables together and also wasting CPU resources.
>>
>> This fixes a high CPU usage in ovs-vswitchd in case a BGP daemon is
>> running on a same host and in a same network namespace with OVS using
>> its own custom routing table.
>>
>> Unfortunately, there seems to be no way to tell the kernel to send
>> updates only for particular tables.  So, we'll still receive and parse
>> all of them.  But they will not result in a full cache invalidation in
>> most cases.
>>
>> Linux kernel v4.20 introduced filtering support for RTM_GETROUTE dumps.
>> So, we can make use of it and dump only standard tables when we get a
>> relevant route update.  NETLINK_GET_STRICT_CHK has to be enabled on
>> the socket for filtering to work.  There is no reason to not enable it
>> by default, if supported.  It is not used outside of NETLINK_ROUTE.
>>
>> Fixes: f0e167f0dbad ("route-table: Handle route updates more robustly.")
>> Fixes: ea83a2fcd0d3 ("lib: Show tunnel egress interface in ovsdb")
>> Reported-at: https://github.com/openvswitch/ovs-issues/issues/185
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-October/052091.html
>> Signed-off-by: Ilya Maximets 
>> ---
>>  lib/netlink-protocol.h | 10 ++
>>  lib/netlink-socket.c   |  8 +
>>  lib/route-table.c  | 80 +-
>>  tests/system-route.at  | 64 +
>>  4 files changed, 146 insertions(+), 16 deletions(-)
>>
>> diff --git a/lib/netlink-protocol.h b/lib/netlink-protocol.h
>> index 6eaa7035a..e4bb28ac9 100644
>> --- a/lib/netlink-protocol.h
>> +++ b/lib/netlink-protocol.h
>> @@ -155,6 +155,11 @@ enum {
>>  #define NLA_TYPE_MASK   ~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)
>>  #endif
>>  
>> +/* Introduced in v4.4. */
>> +#ifndef NLM_F_DUMP_FILTERED
>> +#define NLM_F_DUMP_FILTERED 0x20
>> +#endif
>> +
>>  /* These were introduced all together in 2.6.14.  (We want our programs to
>>   * support the newer kernel features even if compiled with older headers.) 
>> */
>>  #ifndef NETLINK_ADD_MEMBERSHIP
>> @@ -168,6 +173,11 @@ enum {
>>  #define 

Re: [ovs-dev] [PATCH v2] netdev-dpdk: Disable outer udp checksum offload for ice driver.

[Ilya Maximets]

On 3/20/24 03:23, Jun Wang wrote:
> Signed-off-by: Jun Wang 

Hi.  Thanks for the patch!

Could you, please, add some information about the issue to the
commit message?

Following tags will also be appropriate to have:

Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
Reported-at: https://github.com/openvswitch/ovs-issues/issues/321

> ---
>  lib/netdev-dpdk.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 29a6bf0..1820163 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1354,6 +1354,12 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>  info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
>  }
>  
> +if (!strcmp(info.driver_name, "net_ice")) {

Please, add a 'FIXME' or 'XXX' comment here stating that it is a
workaround that should be removed once there is a fix in DPDK.

Checkpatch will complain, but you may ignore that.

> +VLOG_INFO("%s: disabled Tx outer udp checksum offloads for a "
> +  "net/ice port.", netdev_get_name(>up));
> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM;
> +}
> +
>  if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPV4_CKSUM) {
>  dev->hw_ol_features |= NETDEV_TX_IPV4_CKSUM_OFFLOAD;
>  } else {
Intel CI reported a failure, but it is a false negative.  Can
be ignored.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn branch-23.06 2/2] tests: Address netcat 7.94 changes.

[0-day Robot]

Bleep bloop.  Greetings Ales Musil, 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.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Dumitru Ceara 
Lines checked: 49, Warnings: 1, Errors: 0


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

Re: [ovs-dev] [PATCH ovn branch-23.06 1/2] tests: Add helper for tcpdump.

[0-day Robot]

Bleep bloop.  Greetings Ales Musil, 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.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Dumitru Ceara 
Lines checked: 1098, Warnings: 1, Errors: 0


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

Re: [ovs-dev] [PATCH ovn 1/2] pinctrl: Fix missing MAC_Bindings.

[Xavier Simonart]

Hi

Thanks Ales for the feedback.
There is also a small change needed to make userspace tests to work as well
(as pointed out by ovs_robot).
I'll send a v2.

Thanks
Xavier


On Wed, Mar 20, 2024 at 12:11 PM Ales Musil  wrote:

>
>
> On Wed, Mar 20, 2024 at 8:12 AM Xavier Simonart 
> wrote:
>
>> Pinctrl is responsible of creating MAC_Bindings on peer router datapaths.
>> However, when sb was read-only, this did not happen.
>> This caused the test "neighbor update on same HV" to fail in a flaky way.
>>
>> Signed-off-by: Xavier Simonart 
>> ---
>>
>
> Hi Xavier,
>
> thank you for the patch. I have one comment down below.
>
>  controller/pinctrl.c |   2 +-
>>  tests/ovn-macros.at  |  10 +++-
>>  tests/system-ovn.at  | 127 +++
>>  3 files changed, 137 insertions(+), 2 deletions(-)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index 2d3595cd2..f75b04696 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -4711,7 +4711,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn
>> *ovnsb_idl_txn,
>>  garp_rarp->announce_time = time_msec() + 1000;
>>  garp_rarp->backoff = 1000; /* msec. */
>>  }
>> -} else {
>> +} else if (ovnsb_idl_txn) {
>>  add_garp_rarp(name, laddrs->ea,
>>laddrs->ipv4_addrs[i].addr,
>>binding_rec->datapath->tunnel_key,
>> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
>> index ed93764d3..aaa8824cb 100644
>> --- a/tests/ovn-macros.at
>> +++ b/tests/ovn-macros.at
>> @@ -220,12 +220,14 @@ ovn_start_northd() {
>>  # options are accepted to adjust that:
>>  #   --backup-northd Start a backup northd.
>>  #   --backup-northd=paused  Start the backup northd in the paused state.
>> +#   --use-tcp-to-sb Use tcp to connect to sb.
>>  ovn_start () {
>>  local backup_northd=false
>>  local backup_northd_options=
>>  case $1 in
>>  --backup-northd) backup_northd=true; shift ;;
>>  --backup-northd=paused) backup_northd=true;
>> backup_northd_options=--paused; shift ;;
>> +--use-tcp-to-sb) use_tcp=true; shift ;;
>>  esac
>>  local AZ=$1
>>  local msg_prefix=${AZ:+$AZ: }
>> @@ -246,7 +248,13 @@ ovn_start () {
>>  ovn_start_northd $backup_northd_options backup $AZ
>>  fi
>>
>> -if test X$HAVE_OPENSSL = Xyes; then
>> +if test $use_tcp; then
>> +# Create the SB DB ptcp connection.
>> +ovn-sbctl \
>> +-- --id=@c create connection \
>> +target=\"ptcp:0:127.0.0.1\" \
>> +-- add SB_Global . connections @c
>> +elif test X$HAVE_OPENSSL = Xyes; then
>>  # Create the SB DB pssl+RBAC connection.
>>  ovn-sbctl \
>>  -- --id=@c create connection \
>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> index 54d913c0b..20ddb487f 100644
>> --- a/tests/system-ovn.at
>> +++ b/tests/system-ovn.at
>> @@ -12208,3 +12208,130 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
>> port patch-.*/d
>>  /connection dropped.*/d"])
>>  AT_CLEANUP
>>  ])
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([MAC_Bindings updates on read-only sb])
>> +ovn_start --use-tcp-to-sb
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +ADD_BR([br-int])
>> +
>> +PARSE_LISTENING_PORT([$ovs_base/ovn-sb/ovsdb-server.log], [TCP_PORT])
>> +
>> +# Use tcp to connect to sb
>> +ovs-vsctl \
>> +-- set Open_vSwitch . external-ids:system-id=hv1 \
>> +-- set Open_vSwitch . 
>> external-ids:ovn-remote=tcp:127.0.0.1:$TCP_PORT
>> \
>> +-- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
>> +-- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
>> +-- set bridge br-int fail-mode=secure
>> other-config:disable-in-band=true
>> +
>> +# Start ovn-controller
>> +start_daemon ovn-controller
>> +
>> +# Logical network:
>> +# A public switch (pub) with a localnet port connected to two LRs (lr0
>> and lr1)
>> +# each with a distributed gateway port.
>> +# Two VMs: lp0 on sw0 connected to lr0
>> +#  lp1 on sw1 connected to lr1
>> +#
>> +# This test adds a floating IP on one VM and checks the MAC_Binding
>> entries to be updated properly.
>> +
>> +# By stopping temporarily updates from controller to sb, we are making
>> sb read-only.
>> +# We can't just pause sb to make it read-only, as we expect sb to still
>> handle northd changes.
>> +stop_ovsdb_controller_updates() {
>> +  TCP_PORT=$1
>> +  echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT
>> +  on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j DROP
>> 2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j
>> DROP'
>> +  iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
>>
>
> iptables are not available by default on Fedora (not sure about Ubuntu),
> we should consider using nftables 

Re: [ovs-dev] [Patch ovn v2 2/2] northd: Fix direct access to SNAT network on DR.

[Ales Musil]

On Thu, Mar 14, 2024 at 2:13 PM Martin Kalcok 
wrote:

> Hello all,
> I have one more follow-up regarding the comments in v1. @amusil, you were
> concerned about the impact this change would have on the performance so I
> ran some tests to try to gauge it. I used following setup with two physical
> hosts connected over switch:
>
>|Physical ext. | Hypervisor1
>|   network|
> alice1-|- switch -|-- DLR --- foo1
>|  |
>
> * alice1 acts as external device.
>   * It doesn't run OVN
>   * It's connected to regular physical network.
> * Hypervisor1 runs OVN chassis
> * foo1 is a container behind Distributed LR with SNAT enabled.
>
> Goal of this test was to measure whether the proposed patch affects
> throughput of the traffic that flows from "foo1" to external network. I
> used iperf3 and ran 3x 5 minute measurements with 10 streams ("iperf3 -i 0
> -t 300 -P 10 -c alice1"). I used 5 minute for each measurement to smooth
> out possible jitter and I ran each measurement 3 times to get feel for the
> variance in the network throughput over longer period. I also did some
> trial and error testing that showed that 10 parallel streams reached
> highest throughput.
>
> Following are the three (summary) results with this patch applied:
>
> # run 1 (with patch)
> [SUM]   0.00-300.00 sec   296 GBytes  8.48 Gbits/sec  sender
> [SUM]   0.00-300.00 sec   296 GBytes  8.48 Gbits/sec  receiver
>
> # run 2 (with patch)
> [SUM]   0.00-300.00 sec   288 GBytes  8.24 Gbits/sec sender
> [SUM]   0.00-300.00 sec   288 GBytes  8.23 Gbits/sec receiver
>
> # run 3 (with patch)
> [SUM]   0.00-300.00 sec   299 GBytes  8.55 Gbits/sec sender
> [SUM]   0.00-300.00 sec   299 GBytes  8.55 Gbits/sec receiver
>
> Next thing I did was to rebuild ovn-controller without these patches,
> replaced the ovn-controller binary on the Hypervisor1 and restarted
> ovn-controller daemon. I verified that the feature flag "ct-commit-to-zone"
> was removed from the chassis (in the SB database), which forced the
> recompute, and then I reran the tests:
>
> # run 1 (clean)
> [SUM]   0.00-300.00 sec   300 GBytes  8.59 Gbits/sec sender
> [SUM]   0.00-300.00 sec   300 GBytes  8.59 Gbits/sec receiver
>
> # run 2 (clean)
> [SUM]   0.00-300.00 sec   285 GBytes  8.15 Gbits/sec sender
> [SUM]   0.00-300.00 sec   285 GBytes  8.15 Gbits/sec receiver
>
> # run 3 (clean)
> [SUM]   0.00-300.00 sec   295 GBytes  8.46 Gbits/sec sender
> [SUM]   0.00-300.00 sec   295 GBytes  8.46 Gbits/sec receiver
>
> These tests show that while there was some fluctuation between each
> individual test, when comparing patched and clean version, they come out
> about the same. I'm happy to run more tests/scenarios if you have something
> else in mind that should be tested.
>
>
>
Hi Martin,

thank you for the performance numbers, it looks fine to me. I have some
comments, see down below.


> On Wed, Mar 13, 2024 at 10:17 AM Martin Kalcok <
> martin.kal...@canonical.com> wrote:
>
>> Regarding the failed unstable test in the CI, I suspect that this is not
>> related to the code change, I've had couple successful CI runs in my branch
>> [0].
>>
>> Martin.
>>
>> [0] https://github.com/mkalcok/ovn/actions/runs/8256539983
>>
>> On Tue, Mar 12, 2024 at 8:45 PM Martin Kalcok <
>> martin.kal...@canonical.com> wrote:
>>
>>> This change fixes bug that breaks ability of machines from external
>>> networks to communicate with machines in SNATed networks (specifically
>>> when using a Distributed router).
>>>
>>> Currently when a machine (S1) on an external network tries to talk
>>> over TCP with a machine (A1) in a network that has enabled SNAT, the
>>> connection is established successfully. However after the three-way
>>> handshake, any packets that come from the A1 machine will have their
>>> source address translated by the Distributed router, breaking the
>>> communication.
>>>
>>> Existing rule in `build_lrouter_out_snat_flow` that decides which
>>> packets should be SNATed already tries to avoid SNATing packets in
>>> reply direction with `(!ct.trk || !ct.rpl)`. However, previous stages
>>> in the distributed LR egress pipeline do not initiate the CT state.
>>>
>>> Additionally we need to commit new connections that originate from
>>> external networks into CT, so that the packets in the reply direction
>>> can be properly identified.
>>>
>>> Rationale:
>>>
>>> In my original RFC [0], there were questions about the motivation for
>>> fixing this issue. I'll try to summarize why I think this is a bug
>>> that should be fixed.
>>>
>>> 1. Current implementation for Distributed router already tries to
>>>avoid SNATing packets in the reply direction, it's just missing
>>>initialized CT states to make proper decisions.
>>>
>>> 2. This same scenario works with Gateway Router. I tested with
>>>following setup:
>>>
>>> foo -- R1 -- join - R3 -- alice
>>>   |
>>> bar --R2
>>>
>>> R1 is a Distributed router 

Re: [ovs-dev] [PATCH ovn branch-23.09 2/2] tests: Address netcat 7.94 changes.

[0-day Robot]

Bleep bloop.  Greetings Ales Musil, 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.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Dumitru Ceara 
Lines checked: 49, Warnings: 1, Errors: 0


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

Re: [ovs-dev] [PATCH ovn branch-23.09 1/2] tests: Add helper for tcpdump.

[0-day Robot]

Bleep bloop.  Greetings Ales Musil, 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.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Dumitru Ceara 
Lines checked: 1100, Warnings: 1, Errors: 0


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

Re: [ovs-dev] [Patch ovn v2 1/2] actions: Enable specifying zone for ct_commit.

[Ales Musil]

On Tue, Mar 12, 2024 at 9:18 PM Martin Kalcok 
wrote:

Hi Martin,

sorry for the late reply.

Following up on the comments from v1.
>
> @amusil You were right that the struct in actions.h was not necessary
> then. However I also noticed that I forgot to modify `format_CT_COMMIT_V1`
> function and for that I think the struct is necessary. I need to
> distinguish whether the `ct_commit` action was called with dnat, snat, or
> without any argument to format it properly. If you still don't like it, I
> can try to figure out how to do it without the struct, but I couldn't
> figure out an obvious solution, so I left it in there in this version.
>

I still think we should basically remove any other functionality from
ct_commit_v1 and leave just those two options.


>
> Regarding the action formatting unit tests, I have two
> assumptions/questions:
> 1. There's no way to distinguish router/switch datapaths in these tests. I
> saw that both `ct_commit_nat(dnat)` and `ct_commit_nat(snat)` [0] expect to
> encode into the same zone, although they'd output different zones if they
> were used in LR datapath.
>

Yeah that's correct, this is limitation/intention of the way we encode the
actions in the test-ovn.c.


> 2. When action formats into identical string as was its input (e.g.
> "ct_commit(snat)" -> "ct_commit(snat)"), the test should not use "format
> as" check, otherwise it fails. (This one took me a while to figure out, as
> it was not obvious from the testlog why it was failing)
>

That is correct and a little confusing, if the formatting is the same as
the original input the test utility doesn't produce the output again.

>
> Are these correct?
>
> @numans I though a lot about your suggestions for different action names,
> but I think that current "ct_commit(snat/dnat)" fits semantically the most.
> Brand new actions would share pretty much all of the code with current
> "ct_commit_v1" handling. So to address your comments regarding the backward
> compatibility, I added new feature flag, following Ales' approach in [1]. I
> believe that this should avoid problems of backward incompatibility in
> cases when northd is upgraded but controller is not.
>

I don't think the plan is to backport those or is it? In case of this being
considered as a feature we don't need the feature flag, but that depends on
decision from maintainers.


>
> @0-day Robot: I forgot to run checkpatch.py, my bad. However the only
> problem is 81 char line in ovn-sb.xml and there are already many lines that
> go over this limit. Should I create v3 if this turns out to be the only
> modification needed?
>

That's fine, as you said there are instances where it is over the 79 limit.


>
> [0]
> https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1500-L1511
> [1]
> https://github.com/ovn-org/ovn/commit/43f741c2f029a68a11436e5b14c5bbda6e207dd3#diff-ca917e7415d06776f8ee2baf6102a866c5c31f998e4df93ff8eaa246b65e1da2
>
> On Tue, Mar 12, 2024 at 8:45 PM Martin Kalcok 
> wrote:
>
>> Action `ct_commit` currently does not allow specifying zone into
>> which connection is committed. For example, in LR datapath, the
>> `ct_commit`
>> will always use the DNAT zone.
>>
>> This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
>> explicitly specify the zone into which the connection will be committed.
>> It also comes with new feature flag OVN_FEATURE_CT_COMMIT_TO_ZONE to avoid
>> incompatibility between northd and controller in case when controller does
>> not suport these actions.
>>
>> Original behavior of `ct_commit` without the arguments remains unchanged.
>>
>> Signed-off-by: Martin Kalcok 
>> ---
>>  controller/chassis.c  |  8 
>>  include/ovn/actions.h |  9 +
>>  include/ovn/features.h|  1 +
>>  lib/actions.c | 29 -
>>  northd/en-global-config.c | 10 ++
>>  northd/en-global-config.h |  1 +
>>  ovn-sb.xml| 10 ++
>>  tests/ovn.at  |  7 +++
>>  8 files changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/controller/chassis.c b/controller/chassis.c
>> index ad75df288..9bb2eba95 100644
>> --- a/controller/chassis.c
>> +++ b/controller/chassis.c
>> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct
>> ovs_chassis_cfg *ovs_cfg,
>>  smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
>>  smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
>>  smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true");
>> +smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true");
>>  }
>>
>>  /*
>> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct
>> ovs_chassis_cfg *ovs_cfg,
>>  return true;
>>  }
>>
>> +if (!smap_get_bool(_rec->other_config,
>> +   OVN_FEATURE_CT_COMMIT_TO_ZONE,
>> +   false)) {
>> +return true;
>> +}
>> +
>>  return false;
>>  }
>>
>> @@ -648,6 +655,7 @@ 

Re: [ovs-dev] [PATCH ovn 2/2] pinctrl: Fixed 100% cpu when connection lost to ovs.

[Ales Musil]

On Wed, Mar 20, 2024 at 8:12 AM Xavier Simonart  wrote:

> This issue is happening for instance when running test
> "ovn-controller - Chassis other_config".
>
> Signed-off-by: Xavier Simonart 
> ---
>  controller/pinctrl.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index f75b04696..ec6c7549b 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3627,13 +3627,14 @@ pinctrl_handler(void *arg_)
>
>  rconn_run_wait(swconn);
>  rconn_recv_wait(swconn);
> -send_garp_rarp_wait(send_garp_rarp_time);
> -ipv6_ra_wait(send_ipv6_ra_time);
> -ip_mcast_querier_wait(send_mcast_query_time);
> -svc_monitors_wait(svc_monitors_next_run_time);
> -ipv6_prefixd_wait(send_prefixd_time);
> -bfd_monitor_wait(bfd_time);
> -
> +if (rconn_is_connected(swconn)) {
> +send_garp_rarp_wait(send_garp_rarp_time);
> +ipv6_ra_wait(send_ipv6_ra_time);
> +ip_mcast_querier_wait(send_mcast_query_time);
> +svc_monitors_wait(svc_monitors_next_run_time);
> +ipv6_prefixd_wait(send_prefixd_time);
> +bfd_monitor_wait(bfd_time);
> +}
>  seq_wait(pinctrl_handler_seq, new_seq);
>
>  latch_wait(>pinctrl_thread_exit);
> --
> 2.31.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 1/2] pinctrl: Fix missing MAC_Bindings.

[Ales Musil]

On Wed, Mar 20, 2024 at 8:12 AM Xavier Simonart  wrote:

> Pinctrl is responsible of creating MAC_Bindings on peer router datapaths.
> However, when sb was read-only, this did not happen.
> This caused the test "neighbor update on same HV" to fail in a flaky way.
>
> Signed-off-by: Xavier Simonart 
> ---
>

Hi Xavier,

thank you for the patch. I have one comment down below.

 controller/pinctrl.c |   2 +-
>  tests/ovn-macros.at  |  10 +++-
>  tests/system-ovn.at  | 127 +++
>  3 files changed, 137 insertions(+), 2 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 2d3595cd2..f75b04696 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -4711,7 +4711,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>  garp_rarp->announce_time = time_msec() + 1000;
>  garp_rarp->backoff = 1000; /* msec. */
>  }
> -} else {
> +} else if (ovnsb_idl_txn) {
>  add_garp_rarp(name, laddrs->ea,
>laddrs->ipv4_addrs[i].addr,
>binding_rec->datapath->tunnel_key,
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index ed93764d3..aaa8824cb 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -220,12 +220,14 @@ ovn_start_northd() {
>  # options are accepted to adjust that:
>  #   --backup-northd Start a backup northd.
>  #   --backup-northd=paused  Start the backup northd in the paused state.
> +#   --use-tcp-to-sb Use tcp to connect to sb.
>  ovn_start () {
>  local backup_northd=false
>  local backup_northd_options=
>  case $1 in
>  --backup-northd) backup_northd=true; shift ;;
>  --backup-northd=paused) backup_northd=true;
> backup_northd_options=--paused; shift ;;
> +--use-tcp-to-sb) use_tcp=true; shift ;;
>  esac
>  local AZ=$1
>  local msg_prefix=${AZ:+$AZ: }
> @@ -246,7 +248,13 @@ ovn_start () {
>  ovn_start_northd $backup_northd_options backup $AZ
>  fi
>
> -if test X$HAVE_OPENSSL = Xyes; then
> +if test $use_tcp; then
> +# Create the SB DB ptcp connection.
> +ovn-sbctl \
> +-- --id=@c create connection \
> +target=\"ptcp:0:127.0.0.1\" \
> +-- add SB_Global . connections @c
> +elif test X$HAVE_OPENSSL = Xyes; then
>  # Create the SB DB pssl+RBAC connection.
>  ovn-sbctl \
>  -- --id=@c create connection \
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 54d913c0b..20ddb487f 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -12208,3 +12208,130 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query
> port patch-.*/d
>  /connection dropped.*/d"])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([MAC_Bindings updates on read-only sb])
> +ovn_start --use-tcp-to-sb
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +
> +PARSE_LISTENING_PORT([$ovs_base/ovn-sb/ovsdb-server.log], [TCP_PORT])
> +
> +# Use tcp to connect to sb
> +ovs-vsctl \
> +-- set Open_vSwitch . external-ids:system-id=hv1 \
> +-- set Open_vSwitch . external-ids:ovn-remote=tcp:127.0.0.1:$TCP_PORT
> \
> +-- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> +-- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> +-- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true
> +
> +# Start ovn-controller
> +start_daemon ovn-controller
> +
> +# Logical network:
> +# A public switch (pub) with a localnet port connected to two LRs (lr0
> and lr1)
> +# each with a distributed gateway port.
> +# Two VMs: lp0 on sw0 connected to lr0
> +#  lp1 on sw1 connected to lr1
> +#
> +# This test adds a floating IP on one VM and checks the MAC_Binding
> entries to be updated properly.
> +
> +# By stopping temporarily updates from controller to sb, we are making sb
> read-only.
> +# We can't just pause sb to make it read-only, as we expect sb to still
> handle northd changes.
> +stop_ovsdb_controller_updates() {
> +  TCP_PORT=$1
> +  echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT
> +  on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j DROP
> 2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j
> DROP'
> +  iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
>

iptables are not available by default on Fedora (not sure about Ubuntu), we
should consider using nftables instead.
(/workspace/ovn/tests/system-kmod-testsuite.dir/at-groups/162/test-source:
line 214: iptables: command not found)


> +}
> +restart_ovsdb_controller_updates() {
> +  TCP_PORT=$1
> +  echo Restarting updates from ovn-controller to ovsdb
> +  iptables -D INPUT -p tcp --destination-port $TCP_PORT  -j DROP
> +}
> +
> +# Create logical switches
> +check 

[ovs-dev] [PATCH ovn branch-23.06 1/2] tests: Add helper for tcpdump.

[Ales Musil]

The way how tcpdump was called in tests was inconsistent,
a lot fo the tests didn't even wait for the tcpdump to properly
start, some of them didn't redirect the stderr which could cause
leak into the test stderr and fail the test.

To prevent that add macro that starts tcpdump and properly
waits for the "listening" state, at the same time redirects
the stderr into separate file.

Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit e8ac18104df0bbd6579d8c62fd4282939631b878)
---
 tests/system-common-macros.at |  29 ++--
 tests/system-ovn-kmod.at  |  24 +--
 tests/system-ovn.at   | 312 ++
 3 files changed, 153 insertions(+), 212 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 8fe93cbdb..683f15fe3 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -271,6 +271,18 @@ m4_define([OVS_START_L7],
]
 )
 
+# NETNS_START_TCPDUMP([namespace], [params], [name])
+#
+# Helper to properly start tcpdump and wait for the startup.
+# The tcpdump output is available in .tcpdump file.
+m4_define([NETNS_START_TCPDUMP],
+[
+ NETNS_DAEMONIZE([$1], [tcpdump -l $2 >$3.tcpdump 2>$3.stderr], [$3.pid])
+ OVS_WAIT_UNTIL([grep -q "listening" $3.stderr])
+]
+)
+
+
 # OVS_CHECK_VXLAN()
 #
 # Do basic check for vxlan functionality, skip the test if it's not there.
@@ -437,8 +449,7 @@ chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases
 chmod 775 /var/lib/dhcp
 chmod 664 /var/lib/dhcp/dhcpd6.leases
 
-NS_CHECK_EXEC([server], [tcpdump -nni s1 > pkt.pcap &])
-
+NETNS_START_TCPDUMP([server], [-nni s1], [server])
 NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid])
 ovn-nbctl --wait=hv sync
 
@@ -456,17 +467,17 @@ prefix=$(ovn-nbctl list logical_router_port rp-public | 
awk -F/ '/ipv6_prefix/{p
 ovn-nbctl list logical_router_port rp-public > /tmp/rp-public
 
 # Wait for 2 renew on each port.
-NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 4 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew])
 # Reply message with Status OK
-NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix} > reply.pcap &])
+NETNS_START_TCPDUMP([server], [-c 4 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix}], [reply])
 
 OVS_WAIT_UNTIL([
-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew.tcpdump | wc -l)
 test "${total_pkts}" = "4"
 ])
 
 OVS_WAIT_UNTIL([
-total_pkts=$(cat reply.pcap | wc -l)
+total_pkts=$(cat reply.tcpdump | wc -l)
 test "${total_pkts}" = "4"
 ])
 
@@ -474,7 +485,7 @@ ovn-nbctl set logical_router_port rp-public 
options:prefix=false
 ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
 ovn-nbctl --wait=hv set logical_router_port rp-sw1 options:prefix=true
 sleep_sb
-NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 2 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew2])
 
 # Sleep enough to have solicit and renew being sent, then wait for 2 renew.
 # The reply to the request will usually be received as sb is sleeping.
@@ -482,12 +493,10 @@ NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 
ip6[[48:1]]=0x05 and ip6[[113:4]]=
 sleep 10
 wake_up_sb
 OVS_WAIT_UNTIL([
-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew2.tcpdump | wc -l)
 test "${total_pkts}" = "2"
 ])
 
-kill $(pidof tcpdump)
-
 ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
 ovn-nbctl clear logical_router_port rp-sw0 ipv6_prefix
 OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | 
cut -c3-16)" = "[2001:1db8:]"])
diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index 50fe8ad9d..66028992d 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -883,14 +883,11 @@ ovn-nbctl --wait=hv sync
 NETNS_DAEMONIZE([server], [nc -l -u 172.16.1.2 4242 > /dev/null], [server.pid])
 
 # Collect ICMP packets on client side
-NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
-icmp > client.pcap 2>client_err], [tcpdump0.pid])
-OVS_WAIT_UNTIL([grep "listening" client_err])
+NETNS_START_TCPDUMP([client], [-U -i client -vnne icmp], [tcpdump-client])
 
 # Collect UDP packets on server side
-NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
-'udp and ip[[6:2]] > 0 and not ip[[6]] = 64' > server.pcap 2>server_err], 
[tcpdump1.pid])
-OVS_WAIT_UNTIL([grep "listening" server_err])
+NETNS_START_TCPDUMP([server], [-U -i server -vnne \
+'udp and ip[[6:2]] > 0 and not ip[[6]] = 64'], [tcpdump-server])
 
 check ip netns exec client python3 << EOF
 import os
@@ -898,7 +895,7 @@ import socket
 import sys
 import time
 
-FILE="client.pcap"
+FILE="tcpdump-client.tcpdump"
 
 
 def contains_string(file, str):
@@ -926,8 

[ovs-dev] [PATCH ovn branch-23.06 2/2] tests: Address netcat 7.94 changes.

[Ales Musil]

Fedora received an update of netcat to version 7.94, this version
brings ability for UDP to accept multiple connections without closing
(-k/--keep-open) [0]. That had negative impact on the tests as the UDP
netcat server was closing sooner that expected.

Make sure that the server is alive when we expect it to and avoid
checking kill of server that might be already finished.

[0] https://github.com/nmap/nmap/issues/1223
Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit 0a35824093ddcd774544ec696dc8fe8c279fb603)
---
 tests/system-ovn-kmod.at | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index 66028992d..65016b84f 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -1023,8 +1023,7 @@ wait_for_ports_up
 check ovn-nbctl --wait=hv sync
 
 # Create service that listens for TCP and UDP
-NETNS_DAEMONIZE([vm2], [nc -l -u 1234], [nc0.pid])
-NETNS_DAEMONIZE([vm2], [nc -l -k 1235], [nc1.pid])
+NETNS_DAEMONIZE([vm2], [nc -l -k 1235], [nc0.pid])
 
 test_icmp() {
 # Make sure that a ping works as expected
@@ -1048,7 +1047,9 @@ 
icmp,orig=(src=173.0.1.2,dst=172.16.0.102,id=,type=8,code=0),reply=(src
 }
 
 test_udp() {
+NETNS_DAEMONIZE([vm2], [nc -l -u 1234], [nc1.pid])
 NS_CHECK_EXEC([vm1], [nc -u 30.0.0.1 1234 -p 1222 -z])
+kill $(cat nc1.pid)
 
 AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \
 sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
-- 
2.44.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [Patch ovn] docs: Remove ref. to "ovn-sbctl --no-wait".

[Ales Musil]

On Wed, Mar 13, 2024 at 5:40 PM Martin Kalcok 
wrote:

> Couple places in the documentation reference "--wait" and "--no-wait"
> options for "ovn-sbctl" but it doesn't support these options [0].
>
> Trying, for example, "ovn-sbctl --no-wait init" exits with:
>
> ovn-sbctl: --no-wait not supported
>
> [0]
> https://github.com/ovn-org/ovn/blob/63b35e2f6789f7843363c8f7a92430402bf989f9/utilities/ovn-sbctl.c#L127
>
> Signed-off-by: Martin Kalcok 
> ---
>  Documentation/intro/install/general.rst | 2 +-
>  utilities/ovn-sbctl.8.xml   | 8 
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/intro/install/general.rst
> b/Documentation/intro/install/general.rst
> index ab6209482..6efb3a701 100644
> --- a/Documentation/intro/install/general.rst
> +++ b/Documentation/intro/install/general.rst
> @@ -428,7 +428,7 @@ the first time after you create the databases with
> ovsdb-tool, though running
>  it at any time is harmless::
>
>  $ ovn-nbctl --no-wait init
> -$ ovn-sbctl --no-wait init
> +$ ovn-sbctl init
>
>  Start ``ovn-northd``, telling it to connect to the OVN db servers same
>  Unix domain socket::
> diff --git a/utilities/ovn-sbctl.8.xml b/utilities/ovn-sbctl.8.xml
> index 81ab4131d..32035d051 100644
> --- a/utilities/ovn-sbctl.8.xml
> +++ b/utilities/ovn-sbctl.8.xml
> @@ -123,10 +123,10 @@
>
>  Instructs the daemon process to run one or more
> ovn-sbctl
>  commands described above and reply with the results of running
> these
> -commands. Accepts the --no-wait, --wait,
> ---timeout, --dry-run,
> --oneline,
> -and the options described under Table Formatting
> Options
> -in addition to the the command-specific options.
> +commands. Accepts the --timeout,
> --dry-run,
> +--oneline, and the options described under
> +Table Formatting Options in addition to the the
> +command-specific options.
>
>
>exit
> --
> 2.40.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn branch-22.03] tests: Add helper for tcpdump.

[0-day Robot]

Bleep bloop.  Greetings Ales Musil, 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.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Dumitru Ceara 
Lines checked: 703, Warnings: 1, Errors: 0


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 branch-23.09 2/2] tests: Address netcat 7.94 changes.

[Ales Musil]

Fedora received an update of netcat to version 7.94, this version
brings ability for UDP to accept multiple connections without closing
(-k/--keep-open) [0]. That had negative impact on the tests as the UDP
netcat server was closing sooner that expected.

Make sure that the server is alive when we expect it to and avoid
checking kill of server that might be already finished.

[0] https://github.com/nmap/nmap/issues/1223
Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit 0a35824093ddcd774544ec696dc8fe8c279fb603)
---
 tests/system-ovn-kmod.at | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index 254a4b0a0..86c5e01ab 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -804,8 +804,7 @@ wait_for_ports_up
 check ovn-nbctl --wait=hv sync
 
 # Create service that listens for TCP and UDP
-NETNS_DAEMONIZE([vm2], [nc -l -u 1234], [nc0.pid])
-NETNS_DAEMONIZE([vm2], [nc -l -k 1235], [nc1.pid])
+NETNS_DAEMONIZE([vm2], [nc -l -k 1235], [nc0.pid])
 
 test_icmp() {
 # Make sure that a ping works as expected
@@ -829,7 +828,9 @@ 
icmp,orig=(src=173.0.1.2,dst=172.16.0.102,id=,type=8,code=0),reply=(src
 }
 
 test_udp() {
+NETNS_DAEMONIZE([vm2], [nc -l -u 1234], [nc1.pid])
 NS_CHECK_EXEC([vm1], [nc -u 30.0.0.1 1234 -p 1222 -z])
+kill $(cat nc1.pid)
 
 AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \
 sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
-- 
2.44.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn branch-23.09 1/2] tests: Add helper for tcpdump.

[Ales Musil]

The way how tcpdump was called in tests was inconsistent,
a lot fo the tests didn't even wait for the tcpdump to properly
start, some of them didn't redirect the stderr which could cause
leak into the test stderr and fail the test.

To prevent that add macro that starts tcpdump and properly
waits for the "listening" state, at the same time redirects
the stderr into separate file.

Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit e8ac18104df0bbd6579d8c62fd4282939631b878)
---
 tests/system-common-macros.at |  29 ++--
 tests/system-ovn-kmod.at  |  26 +--
 tests/system-ovn.at   | 312 ++
 3 files changed, 154 insertions(+), 213 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 8fe93cbdb..683f15fe3 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -271,6 +271,18 @@ m4_define([OVS_START_L7],
]
 )
 
+# NETNS_START_TCPDUMP([namespace], [params], [name])
+#
+# Helper to properly start tcpdump and wait for the startup.
+# The tcpdump output is available in .tcpdump file.
+m4_define([NETNS_START_TCPDUMP],
+[
+ NETNS_DAEMONIZE([$1], [tcpdump -l $2 >$3.tcpdump 2>$3.stderr], [$3.pid])
+ OVS_WAIT_UNTIL([grep -q "listening" $3.stderr])
+]
+)
+
+
 # OVS_CHECK_VXLAN()
 #
 # Do basic check for vxlan functionality, skip the test if it's not there.
@@ -437,8 +449,7 @@ chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases
 chmod 775 /var/lib/dhcp
 chmod 664 /var/lib/dhcp/dhcpd6.leases
 
-NS_CHECK_EXEC([server], [tcpdump -nni s1 > pkt.pcap &])
-
+NETNS_START_TCPDUMP([server], [-nni s1], [server])
 NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid])
 ovn-nbctl --wait=hv sync
 
@@ -456,17 +467,17 @@ prefix=$(ovn-nbctl list logical_router_port rp-public | 
awk -F/ '/ipv6_prefix/{p
 ovn-nbctl list logical_router_port rp-public > /tmp/rp-public
 
 # Wait for 2 renew on each port.
-NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 4 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew])
 # Reply message with Status OK
-NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix} > reply.pcap &])
+NETNS_START_TCPDUMP([server], [-c 4 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix}], [reply])
 
 OVS_WAIT_UNTIL([
-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew.tcpdump | wc -l)
 test "${total_pkts}" = "4"
 ])
 
 OVS_WAIT_UNTIL([
-total_pkts=$(cat reply.pcap | wc -l)
+total_pkts=$(cat reply.tcpdump | wc -l)
 test "${total_pkts}" = "4"
 ])
 
@@ -474,7 +485,7 @@ ovn-nbctl set logical_router_port rp-public 
options:prefix=false
 ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
 ovn-nbctl --wait=hv set logical_router_port rp-sw1 options:prefix=true
 sleep_sb
-NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 2 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew2])
 
 # Sleep enough to have solicit and renew being sent, then wait for 2 renew.
 # The reply to the request will usually be received as sb is sleeping.
@@ -482,12 +493,10 @@ NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 
ip6[[48:1]]=0x05 and ip6[[113:4]]=
 sleep 10
 wake_up_sb
 OVS_WAIT_UNTIL([
-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew2.tcpdump | wc -l)
 test "${total_pkts}" = "2"
 ])
 
-kill $(pidof tcpdump)
-
 ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
 ovn-nbctl clear logical_router_port rp-sw0 ipv6_prefix
 OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | 
cut -c3-16)" = "[2001:1db8:]"])
diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index 3a533d5e6..254a4b0a0 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -664,14 +664,11 @@ ovn-nbctl --wait=hv sync
 NETNS_DAEMONIZE([server], [nc -l -u 172.16.1.2 4242 > /dev/null], [server.pid])
 
 # Collect ICMP packets on client side
-NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
-icmp > client.pcap 2>client_err], [tcpdump0.pid])
-OVS_WAIT_UNTIL([grep "listening" client_err])
+NETNS_START_TCPDUMP([client], [-U -i client -vnne icmp], [tcpdump-client])
 
 # Collect UDP packets on server side
-NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
-'udp and ip[[6:2]] > 0 and not ip[[6]] = 64' > server.pcap 2>server_err], 
[tcpdump1.pid])
-OVS_WAIT_UNTIL([grep "listening" server_err])
+NETNS_START_TCPDUMP([server], [-U -i server -vnne \
+'udp and ip[[6:2]] > 0 and not ip[[6]] = 64'], [tcpdump-server])
 
 check ip netns exec client python3 << EOF
 import os
@@ -679,7 +676,7 @@ import socket
 import sys
 import time
 
-FILE="client.pcap"
+FILE="tcpdump-client.tcpdump"
 
 
 def contains_string(file, str):
@@ -707,8 

[ovs-dev] [PATCH ovn branch-22.03] tests: Add helper for tcpdump.

[Ales Musil]

The way how tcpdump was called in tests was inconsistent,
a lot fo the tests didn't even wait for the tcpdump to properly
start, some of them didn't redirect the stderr which could cause
leak into the test stderr and fail the test.

To prevent that add macro that starts tcpdump and properly
waits for the "listening" state, at the same time redirects
the stderr into separate file.

Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit e8ac18104df0bbd6579d8c62fd4282939631b878)
---
 tests/system-common-macros.at |  25 +++--
 tests/system-ovn-kmod.at  |  10 +-
 tests/system-ovn.at   | 193 +-
 3 files changed, 114 insertions(+), 114 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 28a9873d6..d4f334036 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -299,6 +299,18 @@ m4_define([OVS_START_L7],
]
 )
 
+# NETNS_START_TCPDUMP([namespace], [params], [name])
+#
+# Helper to properly start tcpdump and wait for the startup.
+# The tcpdump output is available in .tcpdump file.
+m4_define([NETNS_START_TCPDUMP],
+[
+ NETNS_DAEMONIZE([$1], [tcpdump -l $2 >$3.tcpdump 2>$3.stderr], [$3.pid])
+ OVS_WAIT_UNTIL([grep -q "listening" $3.stderr])
+]
+)
+
+
 # OVS_CHECK_VXLAN()
 #
 # Do basic check for vxlan functionality, skip the test if it's not there.
@@ -453,8 +465,7 @@ chown root:dhcpd /var/lib/dhcp /var/lib/dhcp/dhcpd6.leases
 chmod 775 /var/lib/dhcp
 chmod 664 /var/lib/dhcp/dhcpd6.leases
 
-NS_CHECK_EXEC([server], [tcpdump -nni s1 > pkt.pcap &])
-
+NETNS_START_TCPDUMP([server], [-nni s1], [server])
 NETNS_DAEMONIZE([server], [dhcpd -6 -f s1 > dhcpd.log 2>&1], [dhcpd.pid])
 ovn-nbctl --wait=hv sync
 
@@ -477,22 +488,20 @@ ovn-nbctl list logical_router_port rp-public > 
/tmp/rp-public
 ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
 ovn-nbctl set logical_router_port rp-sw1 options:prefix=false
 # Renew message
-NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix} > renew.pcap &])
+NETNS_START_TCPDUMP([server], [-c 1 -nni s1 ip6[[48:1]]=0x05 and 
ip6[[113:4]]=0x${prefix}], [renew])
 # Reply message with Status OK
-NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix} > reply.pcap &])
+NETNS_START_TCPDUMP([server], [-c 1 -nni s1 ip6[[48:1]]=0x07 and 
ip6[[81:4]]=0x${prefix}], [reply])
 
 OVS_WAIT_UNTIL([
-total_pkts=$(cat renew.pcap | wc -l)
+total_pkts=$(cat renew.tcpdump | wc -l)
 test "${total_pkts}" = "1"
 ])
 
 OVS_WAIT_UNTIL([
-total_pkts=$(cat reply.pcap | wc -l)
+total_pkts=$(cat reply.tcpdump | wc -l)
 test "${total_pkts}" = "1"
 ])
 
-kill $(pidof tcpdump)
-
 ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
 ovn-nbctl clear logical_router_port rp-sw0 ipv6_prefix
 OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | 
cut -c3-16)" = "[2001:1db8:]"])
diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index db4191f31..7122b909c 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -320,14 +320,10 @@ while True:
 NETNS_DAEMONIZE([server], [$PYTHON3 ./server.py > server.log], [server.pid])
 
 dnl Collect packets on server side.
-NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
-  'ip and (icmp or udp)' > server.tcpdump 2>server_err], 
[tcpdump0.pid])
-OVS_WAIT_UNTIL([grep "listening" server_err])
+NETNS_START_TCPDUMP([server], [-U -i server -vnne 'ip and (icmp or udp)'], 
[tcpdump-server])
 
 dnl Collect packets on client side.
-NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
-  'ip and (icmp or udp)' > client.tcpdump 2>client_err], 
[tcpdump1.pid])
-OVS_WAIT_UNTIL([grep "listening" client_err])
+NETNS_START_TCPDUMP([client], [-U -i client -vnne 'ip and (icmp or udp)'], 
[tcpdump-client])
 
 dnl Send two packets to the server with a short interval.
 dnl First packet should generate 'needs frag', the second should result in
@@ -345,7 +341,7 @@ time.sleep(5)
 NS_CHECK_EXEC([client], [$PYTHON3 ./client.py])
 
 dnl Expecting 2 outgoing packets and 2 fragments back - 8 lines total.
-OVS_WAIT_UNTIL([test "$(cat client.tcpdump | wc -l)" = "8"])
+OVS_WAIT_UNTIL([test "$(cat tcpdump-client.tcpdump | wc -l)" = "8"])
 
 ovn-appctl -t ovn-controller vlog/set info
 
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 9e2507a7a..d66c9ab63 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -1602,12 +1602,11 @@ OVS_WAIT_UNTIL([
 ovn-nbctl --reject lb-add lb3 30.0.0.10:80 ""
 ovn-nbctl ls-lb-add foo lb3
 # Filter reset segments
-NS_CHECK_EXEC([foo1], [tcpdump -l -c 1 -neei foo1 ip[[33:1]]=0x14 > rst.pcap 
2>tcpdump_err &])
-OVS_WAIT_UNTIL([grep "listening" tcpdump_err])
+NETNS_START_TCPDUMP([foo1], [-c 1 -neei foo1 ip[[33:1]]=0x14], [rst])
 NS_CHECK_EXEC([foo1], [wget -q 30.0.0.10],[4])
 
 OVS_WAIT_UNTIL([
-n_reset=$(cat 

Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix busy loop when ofctrl is disconnected.

[Dumitru Ceara]

On 3/20/24 06:41, Han Zhou wrote:
> ovn-controller runs at 100% cpu when OVS exits. This is because the
> ofctrl_run is not called while ofctrl_wait is always called in the main
> loop. Because of the missing ofctrl_run, it doesn't even detect that the
> ofctrl connection is disconnected.
> 
> This patch fixes the issue by always giving a chance to run ofctrl_run
> as long as ofctrl_wait is called.
> 
> Fixes: 1d6d953bf883 ("controller: Don't artificially limit group and meter 
> IDs to 16bit.")
> Fixes: 94cbc59dc0f1 ("ovn-controller: Fix use of dangling pointers in I-P 
> runtime_data.")
> Signed-off-by: Han Zhou 
> ---

Thanks for the patch, Han!  I tested it in a sandbox and it fixes the
issue.  Looks good to me:

Acked-by: Dumitru Ceara 

Regards,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 3/6] Migrate commands to extended unixctl API.

[Jakob Meng]

On 19.03.24 16:41, Eelco Chaudron wrote:
>
> On 19 Mar 2024, at 16:19, Ilya Maximets wrote:
>
>> On 3/19/24 15:54, Eelco Chaudron wrote:
>>>
>>> On 19 Mar 2024, at 15:46, Ilya Maximets wrote:
>>>
 On 3/19/24 15:17, Eelco Chaudron wrote:
>
> On 19 Mar 2024, at 15:00, Ilya Maximets wrote:
>
>> On 3/19/24 14:41, Jakob Meng wrote:
>>>
>>> On 19.03.24 13:22, Ilya Maximets wrote:
 On 3/19/24 13:21, Ilya Maximets wrote:
> On 3/19/24 13:17, Eelco Chaudron wrote:
>> On 19 Mar 2024, at 13:11, Jakob Meng wrote:
>>
>>> Hi!
>>>
>>> On 15.03.24 11:19, Eelco Chaudron wrote:
 On 18 Jan 2024, at 16:26, jm...@redhat.com wrote:

> ...
 Thank for the patch! What a beast to go trough ;)
>>> Thank you for doing it anyway ☺️
>>>
 I believe the current approach is acceptable. However, we could 
 also
 incorporate union callbacks: if registration only supports text, 
 we would
 use callback A, and if multiple formats exist, we could employ the 
 new style
 callback. This would mitigate the need for a significant overhaul. 
 Just a
 suggestion, as I'm ok with the current approach. :)
>>> This trades many-initial-changes-across-a-lot-of-files for a 
>>> more-complex-registration-and-callback-logic.
>>>
>>> We could also make struct unixctl_conn visible (move it from 
>>> lib/unixctl.c to lib/unixctl.h) and let callees read the output 
>>> format from its 'fmt' member. Then we would not have to change the 
>>> callback signature at all.
>> Or have a utility function to return the format, which keeps the 
>> structure invisible.
> +1, just add some access methods.
>
>>> I do not have a strong opinion here. What do you, Ilya and the 
>>> others think?
>> I’ll wait for others to chime in...
> I don't think changing all the callbacks is a good thing to do.
> i.e. this patch should not exist or be very small.
 * or it should be very small.
>>> How about unixctl_command_register?
>>>
>>> Should we change the existing function signature (like I did)?
>>> Or add a second function, like unixctl_command_register_fmt?
>>> Or should we add another function to set/change supported formats of an 
>>> already registered command?
>>>
>>> Example:
>>>
>>> // Get output format which the user has choosen (defaults to txt)
>>> enum ovs_output_fmt
>>> unixctl_command_get_output_format(struct unixctl_conn *);
>>>
>>> // Set output formats that a command supports
>>> int
>>> unixctl_command_set_output_formats(const char *name, unsigned int 
>>> output_fmts);
>>>
>>> Wdyt?
>> I'm actually not sure why we need to register them differently in the
>> first palace.  Make the callback check what was the requested format
>> and use appropriate reply function.
> I guess this can be done with the unixctl_command_get_output_format() 
> function?
>
>> Why we need to fail the requests
>> for callbacks that don't support JSON replies?  Why can't we just
>> pack the plain reply into a JSON string?
> I don’t like this, this way we will not know if the command actually 
> supports native JSON or not. Else we might confuse scripts in the future 
> if we start sending some JSONized responses. returning an error might 
> also trigger people to submit a patch to convert more commands to native 
> JSON support :)
 The main problem is that we shouldn't fail commands after they are
 already executed.  It may work for 'show' commands, but it's not
 a thing that can be done for 'set' commands.
>>> I guess this means I would prefer to take a ‘bigger’ change hit and change 
>>> the register procedure (not the actual callback), to include the supported 
>>> output formats. This way we can still error out all the show, set, etc. 
>>> commands that do not support the JSON format before we execute them.
>> Failing the request because it doesn't support the output format is
>> also ugly.  For example, the list-commands doesn't support it, so
>> the user that enabled JSON format on the socket will not be able to
>> even list the commands.  I don't think that's a good interface.
>> I'd rather have just a new command then, e.g. 'dpif/show+json' that
>> would reply in a JSON format and void all the questionable API changes.
> I was looking at this from a single transaction perspective, and then I’m 
> fine with it failing. If you ask for json on the list-command and it does not 
> support it, the error would be clear enough. Not sure why you would feel like 
> this is ugly.

Ilya's 'dpif/show+json' approach has the benefit of a less invasive change. 
Both 

[ovs-dev] [PATCH ovn 1/2] pinctrl: Fix missing MAC_Bindings.

[Xavier Simonart]

Pinctrl is responsible of creating MAC_Bindings on peer router datapaths.
However, when sb was read-only, this did not happen.
This caused the test "neighbor update on same HV" to fail in a flaky way.

Signed-off-by: Xavier Simonart 
---
 controller/pinctrl.c |   2 +-
 tests/ovn-macros.at  |  10 +++-
 tests/system-ovn.at  | 127 +++
 3 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 2d3595cd2..f75b04696 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4711,7 +4711,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn *ovnsb_idl_txn,
 garp_rarp->announce_time = time_msec() + 1000;
 garp_rarp->backoff = 1000; /* msec. */
 }
-} else {
+} else if (ovnsb_idl_txn) {
 add_garp_rarp(name, laddrs->ea,
   laddrs->ipv4_addrs[i].addr,
   binding_rec->datapath->tunnel_key,
diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index ed93764d3..aaa8824cb 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -220,12 +220,14 @@ ovn_start_northd() {
 # options are accepted to adjust that:
 #   --backup-northd Start a backup northd.
 #   --backup-northd=paused  Start the backup northd in the paused state.
+#   --use-tcp-to-sb Use tcp to connect to sb.
 ovn_start () {
 local backup_northd=false
 local backup_northd_options=
 case $1 in
 --backup-northd) backup_northd=true; shift ;;
 --backup-northd=paused) backup_northd=true; 
backup_northd_options=--paused; shift ;;
+--use-tcp-to-sb) use_tcp=true; shift ;;
 esac
 local AZ=$1
 local msg_prefix=${AZ:+$AZ: }
@@ -246,7 +248,13 @@ ovn_start () {
 ovn_start_northd $backup_northd_options backup $AZ
 fi
 
-if test X$HAVE_OPENSSL = Xyes; then
+if test $use_tcp; then
+# Create the SB DB ptcp connection.
+ovn-sbctl \
+-- --id=@c create connection \
+target=\"ptcp:0:127.0.0.1\" \
+-- add SB_Global . connections @c
+elif test X$HAVE_OPENSSL = Xyes; then
 # Create the SB DB pssl+RBAC connection.
 ovn-sbctl \
 -- --id=@c create connection \
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 54d913c0b..20ddb487f 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -12208,3 +12208,130 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port 
patch-.*/d
 /connection dropped.*/d"])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([MAC_Bindings updates on read-only sb])
+ovn_start --use-tcp-to-sb
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+PARSE_LISTENING_PORT([$ovs_base/ovn-sb/ovsdb-server.log], [TCP_PORT])
+
+# Use tcp to connect to sb
+ovs-vsctl \
+-- set Open_vSwitch . external-ids:system-id=hv1 \
+-- set Open_vSwitch . external-ids:ovn-remote=tcp:127.0.0.1:$TCP_PORT \
+-- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+-- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+-- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+
+# Logical network:
+# A public switch (pub) with a localnet port connected to two LRs (lr0 and lr1)
+# each with a distributed gateway port.
+# Two VMs: lp0 on sw0 connected to lr0
+#  lp1 on sw1 connected to lr1
+#
+# This test adds a floating IP on one VM and checks the MAC_Binding entries to 
be updated properly.
+
+# By stopping temporarily updates from controller to sb, we are making sb 
read-only.
+# We can't just pause sb to make it read-only, as we expect sb to still handle 
northd changes.
+stop_ovsdb_controller_updates() {
+  TCP_PORT=$1
+  echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT
+  on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j DROP 
2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j DROP'
+  iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP
+}
+restart_ovsdb_controller_updates() {
+  TCP_PORT=$1
+  echo Restarting updates from ovn-controller to ovsdb
+  iptables -D INPUT -p tcp --destination-port $TCP_PORT  -j DROP
+}
+
+# Create logical switches
+check ovn-nbctl ls-add sw0
+check ovn-nbctl ls-add sw1
+check ovn-nbctl ls-add pub
+
+# Created localnet port on public switch
+check ovn-nbctl lsp-add pub ln-pub
+check ovn-nbctl lsp-set-type ln-pub localnet
+check ovn-nbctl lsp-set-addresses ln-pub unknown
+check ovn-nbctl lsp-set-options ln-pub network_name=phys
+
+# Create logical routers and connect them to public switch
+AT_CHECK([(ovn-nbctl create Logical_Router name=lr0;
+   ovn-nbctl create Logical_Router name=lr1) | uuidfilt], [0], [<0>
+<1>
+])
+check ovn-nbctl lrp-add lr0 lr0-pub f0:00:00:00:00:01 172.24.4.220/24
+check ovn-nbctl 

[ovs-dev] [PATCH ovn 2/2] pinctrl: Fixed 100% cpu when connection lost to ovs.

[Xavier Simonart]

This issue is happening for instance when running test
"ovn-controller - Chassis other_config".

Signed-off-by: Xavier Simonart 
---
 controller/pinctrl.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f75b04696..ec6c7549b 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -3627,13 +3627,14 @@ pinctrl_handler(void *arg_)
 
 rconn_run_wait(swconn);
 rconn_recv_wait(swconn);
-send_garp_rarp_wait(send_garp_rarp_time);
-ipv6_ra_wait(send_ipv6_ra_time);
-ip_mcast_querier_wait(send_mcast_query_time);
-svc_monitors_wait(svc_monitors_next_run_time);
-ipv6_prefixd_wait(send_prefixd_time);
-bfd_monitor_wait(bfd_time);
-
+if (rconn_is_connected(swconn)) {
+send_garp_rarp_wait(send_garp_rarp_time);
+ipv6_ra_wait(send_ipv6_ra_time);
+ip_mcast_querier_wait(send_mcast_query_time);
+svc_monitors_wait(svc_monitors_next_run_time);
+ipv6_prefixd_wait(send_prefixd_time);
+bfd_monitor_wait(bfd_time);
+}
 seq_wait(pinctrl_handler_seq, new_seq);
 
 latch_wait(>pinctrl_thread_exit);
-- 
2.31.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] controller: Track individual address set constants.

[Ales Musil]

On Tue, Mar 19, 2024 at 6:03 PM Ilya Maximets  wrote:

> On 3/19/24 17:43, Ales Musil wrote:
> > Instead of tracking address set per struct expr_constant_set track it
> > per individual struct expr_constant. This allows more fine grained
> > control for I-P processing of address sets in controller. It helps with
> > scenarios like matching on two address sets in one expression e.g.
> > "ip4.src == {$as1, $as2}". This allows any addition or removal of
> > individual adress from the set to be incrementally processed instead
> > of reprocessing all the flows.
> >
> > This unfortunately doesn't help with the following flows:
> > "ip4.src == $as1 && ip4.dst == $as2"
> > "ip4.src == $as1 || ip4.dst == $as2"
> >
> > The memory impact should be minimal as there is only increase of 8 bytes
> > per the struct expr_constant.
> >
> > Signed-off-by: Ales Musil 
> > ---
>
> Hi, Ales.  I didn't fully read the patch, but I have a quick question:
> Did you test it with the case where address sets contain the same
> addresses? i.e. if we have "ip4.src == {$as1, $as2}" and both contain
> the same IP 'A'.  Will removal of the 'A' from one of the sets be handled
> correctly?
>
> Database protects us from such a case with a single address set, but it
> can't with multiple.
>
> Also, we'll need a test for this scenario in case we don't have one
> already.
>
> Best regards, Ilya Maximets.
>
>
 Hi Ilya,

There are actually several tests that have duplicates across two address
sets like you described. The I-P in that case will lead to reprocessing of
the flows, the AS reference is removed when there are duplicates.

Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev