[ovs-dev] [PATCH ovn] tests: Fix grep warning

[Ales Musil]

The Fedora version of grep (grep (GNU grep) 3.11) complains
about the syntax grep "output\:": grep: warning: stray \ before :

Remove the \ which works also for Ubuntu grep version
(grep (GNU grep) 3.7).

Signed-off-by: Ales Musil 
---
 tests/ovn.at | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 62966752f..cf87b9ad4 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -30435,7 +30435,7 @@ check_packet_tunnel() {
 as $hv
 echo "vif$src -> vif$dst should go through tunnel $local_encap_ip -> 
$remote_encap_ip"
 tunnel_ofport=$(ovs-vsctl --bare --column=ofport find interface 
options:local_ip=$local_encap_ip options:remote_ip=$remote_encap_ip)
-AT_CHECK([test $(ovs-appctl ofproto/trace br-int in_port=vif$src $packet | 
grep "output\:" | awk -F ':' '{ print $2 }') == $tunnel_ofport])
+AT_CHECK([test $(ovs-appctl ofproto/trace br-int in_port=vif$src $packet | 
grep "output:" | awk -F ':' '{ print $2 }') == $tunnel_ofport])
 }
 
 for i in 1 2; do
-- 
2.43.0

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

Re: [ovs-dev] [PATCH ovn 3/3] ovn-controller: Support VIF-based local encap IPs selection.

[Han Zhou]

On Mon, Jan 29, 2024 at 2:41 AM Ales Musil  wrote:
>
>
>
> On Fri, Jan 26, 2024 at 8:05 PM Han Zhou  wrote:
>>
>>
>>
>> On Thu, Jan 25, 2024 at 10:54 PM Ales Musil  wrote:
>> >
>> >
>> >
>> > On Fri, Jan 26, 2024 at 4:07 AM Han Zhou  wrote:
>> >>
>> >>
>> >>
>> >> On Tue, Jan 23, 2024 at 5:29 AM Ales Musil  wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Wed, Jan 17, 2024 at 6:48 AM Han Zhou  wrote:
>> >> >>
>> >> >> Commit dd527a283cd8 partially supported multiple encap IPs. It
supported
>> >> >> remote encap IP selection based on the destination VIF's encap_ip
>> >> >> configuration. This patch adds the support for selecting local
encap IP
>> >> >> based on the source VIF's encap_ip configuration.
>> >> >>
>> >> >> Co-authored-by: Lei Huang 
>> >> >> Signed-off-by: Lei Huang 
>> >> >> Signed-off-by: Han Zhou 
>> >> >> ---
>> >> >
>> >> >
>> >> > Hi Han and Lei,
>> >> >
>> >> > thank you for the patch, I have a couple of comments/questions down
below.
>> >>
>> >>
>> >> Thanks Ales.
>> >>
>> >> >
>> >> >
>> >> >>  NEWS|   3 +
>> >> >>  controller/chassis.c|   2 +-
>> >> >>  controller/local_data.c |   2 +-
>> >> >>  controller/local_data.h |   2 +-
>> >> >>  controller/ovn-controller.8.xml |  30 ++-
>> >> >>  controller/ovn-controller.c |  49 
>> >> >>  controller/physical.c   | 134
++--
>> >> >>  controller/physical.h   |   2 +
>> >> >>  include/ovn/logical-fields.h|   4 +-
>> >> >>  ovn-architecture.7.xml  |  18 -
>> >> >>  tests/ovn.at|  51 +++-
>> >> >>  11 files changed, 243 insertions(+), 54 deletions(-)
>> >> >>
>> >> >> diff --git a/NEWS b/NEWS
>> >> >> index 5f267b4c64cc..5a3eed608617 100644
>> >> >> --- a/NEWS
>> >> >> +++ b/NEWS
>> >> >> @@ -14,6 +14,9 @@ Post v23.09.0
>> >> >>- ovn-northd-ddlog has been removed.
>> >> >>- A new LSP option "enable_router_port_acl" has been added to
enable
>> >> >>  conntrack for the router port whose peer is l3dgw_port if set
it true.
>> >> >> +  - Support selecting encapsulation IP based on the
source/destination VIF's
>> >> >> +settting. See ovn-controller(8) 'external_ids:ovn-encap-ip'
for more
>> >> >> +details.
>> >> >>
>> >> >>  OVN v23.09.0 - 15 Sep 2023
>> >> >>  --
>> >> >> diff --git a/controller/chassis.c b/controller/chassis.c
>> >> >> index a6f13ccc42d5..55f2beb37674 100644
>> >> >> --- a/controller/chassis.c
>> >> >> +++ b/controller/chassis.c
>> >> >> @@ -61,7 +61,7 @@ struct ovs_chassis_cfg {
>> >> >>
>> >> >>  /* Set of encap types parsed from the 'ovn-encap-type'
external-id. */
>> >> >>  struct sset encap_type_set;
>> >> >> -/* Set of encap IPs parsed from the 'ovn-encap-type'
external-id. */
>> >> >> +/* Set of encap IPs parsed from the 'ovn-encap-ip'
external-id. */
>> >> >>  struct sset encap_ip_set;
>> >> >>  /* Interface type list formatted in the OVN-SB Chassis
required format. */
>> >> >>  struct ds iface_types;
>> >> >> diff --git a/controller/local_data.c b/controller/local_data.c
>> >> >> index a9092783958f..8606414f8728 100644
>> >> >> --- a/controller/local_data.c
>> >> >> +++ b/controller/local_data.c
>> >> >> @@ -514,7 +514,7 @@ chassis_tunnels_destroy(struct hmap
*chassis_tunnels)
>> >> >>   */
>> >> >>  struct chassis_tunnel *
>> >> >>  chassis_tunnel_find(const struct hmap *chassis_tunnels, const
char *chassis_id,
>> >> >> -char *remote_encap_ip, char *local_encap_ip)
>> >> >> +char *remote_encap_ip, const char
*local_encap_ip)
>> >> >
>> >> >
>> >> > nit: Unrelated change.
>> >>
>> >>
>> >> Ack
>>
>> Hi Ales, sorry that I just realized this change is related, which is
because of the const char * array introduced in this patch that stores the
parsed encap_ips, and it makes sense to use const char * since we should
never expect this string to be modified in the function.
>>
>> >>
>> >> >
>> >> >
>> >> >>  {
>> >> >>  /*
>> >> >>   * If the specific encap_ip is given, look for the
chassisid_ip entry,
>> >> >> diff --git a/controller/local_data.h b/controller/local_data.h
>> >> >> index bab95bcc3824..ca3905bd20e6 100644
>> >> >> --- a/controller/local_data.h
>> >> >> +++ b/controller/local_data.h
>> >> >> @@ -150,7 +150,7 @@ bool
local_nonvif_data_handle_ovs_iface_changes(
>> >> >>  struct chassis_tunnel *chassis_tunnel_find(const struct hmap
*chassis_tunnels,
>> >> >> const char *chassis_id,
>> >> >> char *remote_encap_ip,
>> >> >> -   char *local_encap_ip);
>> >> >> +   const char
*local_encap_ip);
>> >> >
>> >> >
>> >> > Same as above.
>> >>
>> >>
>> >> Ack
>> >>
>> >> >
>> >> >
>> >> >>
>> >> >>  bool get_chassis_tunnel_ofport(const struct hmap *chassis_tunnels,
>> >> >>  

Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule to latest OVS branch-3.3.

[Numan Siddique]

On Mon, Jan 29, 2024 at 6:43 PM Dumitru Ceara  wrote:
>
> On 1/30/24 00:36, Dumitru Ceara wrote:
> > This picks up the following relevant OVS commits:
> >   8893e24d9d dpdk: Update to use v23.11.
> >   ed738eca39 util: Annotate function that will never return NULL.
> >   77d0bad04 mcast-snooping: Store IGMP/MLD protocol version.
> >   b222593bc6 mcast-snooping: Add group protocol to mdb/show output.
> >   a940a691e7 ovs-atomic: Fix inclusion of Clang header by GCC 14.
> >   b0cf73112d dp-packet: Reset offload/offsets when clearing a packet.
> >
> > This commit also ports the CI DPDK related changes from OVS commit
> > 8893e24d9d09 ("dpdk: Update to use v23.11.") which are required as 23.11
> > is the supported DPDK branch on OVS 3.3.  There's also a small change
> > that had to be made when mangling the prefix path of the installed
> > DPDK version in the container.
> >
> > As a side effect this will also address OVN Fedora 40 (rawhide) build
> > failures on aarch64 and s390x.  They are fixed by a940a691e7d9
> > ("ovs-atomic: Fix inclusion of Clang header by GCC 14.").
> >
> > Suggested-by: Ilya Maximets 
> > Signed-off-by: Dumitru Ceara 
> > ---
>
> CC: Eelco, Mohammad, Numan.


Acked-by: Numan Siddique

Numan

>
> >  .ci/dpdk-build.sh  | 28 ++--
> >  .ci/linux-build.sh | 11 ++-
> >  .github/workflows/test.yml |  4 ++--
> >  controller/pinctrl.c   |  6 +-
> >  ovs|  2 +-
> >  5 files changed, 32 insertions(+), 19 deletions(-)
> >
> > diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
> > index f44ac15b07..0c13c98c98 100755
> > --- a/.ci/dpdk-build.sh
> > +++ b/.ci/dpdk-build.sh
> > @@ -5,25 +5,27 @@ set -x
> >
> >  function build_dpdk()
> >  {
> > -local VERSION_FILE="dpdk-dir/cached-version"
> >  local DPDK_VER=$1
> >  local DPDK_OPTS=""
> > +local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
> > +local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
> >
> > -rm -rf dpdk-dir
> > +rm -rf dpdk-src
> > +rm -rf $DPDK_INSTALL_DIR
> >
> >  if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
> > -git clone --single-branch $DPDK_GIT dpdk-dir -b
"${DPDK_VER##refs/*/}"
> > -pushd dpdk-dir
> > +git clone --single-branch $DPDK_GIT dpdk-src -b
"${DPDK_VER##refs/*/}"
> > +pushd dpdk-src
> >  git log -1 --oneline
> >  else
> >  wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
> >  tar xvf dpdk-$1.tar.xz > /dev/null
> >  DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
> > -mv ${DIR_NAME} dpdk-dir
> > -pushd dpdk-dir
> > +mv ${DIR_NAME} dpdk-src
> > +pushd dpdk-src
> >  fi
> >
> > -# Switching to 'default' machine to make dpdk-dir cache usable on
> > +# Switching to 'default' machine to make the dpdk cache usable on
> >  # different CPUs. We can't be sure that all CI machines are
exactly same.
> >  DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
> >
> > @@ -38,16 +40,22 @@ function build_dpdk()
> >  # only depend on virtio/tap drivers.
> >  # We can disable all remaining drivers to save compilation time.
> >  DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
> > +# OVS depends on the vhost library (and its dependencies).
> > +# net/tap depends on the gso library.
> > +DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
> >
> >  # Install DPDK using prefix.
> > -DPDK_OPTS="$DPDK_OPTS --prefix=$(pwd)/build"
> > +DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
> >
> >  meson $DPDK_OPTS build
> >  ninja -C build
> >  ninja -C build install
> > -
> > -echo "Installed DPDK in $(pwd)"
> >  popd
> > +
> > +# Remove examples sources.
> > +rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
> > +
> > +echo "Installed DPDK in $DPDK_INSTALL_DIR"
> >  echo "${DPDK_VER}" > ${VERSION_FILE}
> >  }
> >
> > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> > index b0c3c9252e..78f17f8bdb 100755
> > --- a/.ci/linux-build.sh
> > +++ b/.ci/linux-build.sh
> > @@ -13,24 +13,25 @@ RECHECK=${RECHECK:-"no"}
> >
> >  function install_dpdk()
> >  {
> > -local VERSION_FILE="dpdk-dir/cached-version"
> > -local DPDK_LIB=$(pwd)/dpdk-dir/build/lib/x86_64-linux-gnu
> > +local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
> > +local VERSION_FILE="${DPDK_INSTALL_DIR}/cached-version"
> > +local DPDK_LIB=${DPDK_INSTALL_DIR}/lib/x86_64-linux-gnu
> >
> >  # Export the following path for pkg-config to find the .pc file.
> >  export PKG_CONFIG_PATH=$DPDK_LIB/pkgconfig/:$PKG_CONFIG_PATH
> >
> >  if [ ! -f "${VERSION_FILE}" ]; then
> > -echo "Could not find DPDK in $(pwd)/dpdk-dir"
> > +echo "Could not find DPDK in $DPDK_INSTALL_DIR"
> >  return 1
> >  fi
> >
> >  # As we build inside a container we need to update the prefix.
> > -sed -i -E 

Re: [ovs-dev] [PATCH ovn v5 08/16] northd: Refactor lflow management into a separate module.

[Han Zhou]

On Mon, Jan 29, 2024 at 7:11 PM Numan Siddique  wrote:

> On Thu, Jan 25, 2024 at 1:08 AM Han Zhou  wrote:
> >
> > On Thu, Jan 11, 2024 at 7:32 AM  wrote:
> > >
> > > From: Numan Siddique 
> > >
> > > ovn_lflow_add() and other related functions/macros are now moved
> > > into a separate module - lflow-mgr.c.  This module maintains a
> > > table 'struct lflow_table' for the logical flows.  lflow table
> > > maintains a hmap to store the logical flows.
> > >
> > > It also maintains the logical switch and router dp groups.
> > >
> > > Previous commits which added lflow incremental processing for
> > > the VIF logical ports, stored the references to
> > > the logical ports' lflows using 'struct lflow_ref_list'.  This
> > > struct is renamed to 'struct lflow_ref' and is part of lflow-mgr.c.
> > > It is  modified a bit to store the resource to lflow references.
> > >
> > > Example usage of 'struct lflow_ref'.
> > >
> > > 'struct ovn_port' maintains 2 instances of lflow_ref.  i,e
> > >
> > > struct ovn_port {
> > >...
> > >...
> > >struct lflow_ref *lflow_ref;
> > >struct lflow_ref *stateful_lflow_ref;
> >
> > Hi Numan,
> >
> > In addition to the lock discussion with you and Dumitru, I still want to
> > discuss another thing of this patch regarding the second lflow_ref:
> > stateful_lflow_ref.
> > I understand that you added this to achieve finer grained I-P especially
> > for router ports. I am wondering how much performance gain is from this.
> > For my understanding it shouldn't matter much since each ovn_port should
> be
> > associated with a very limited number of lflows. Could you provide more
> > insight/data on this? I think it would be better to keep things simple
> > (i.e. one object, one lflow_ref list) unless the benefit is obvious.
> >
> > I am also trying to run another performance regression test for
> recompute,
> > since I am a little concerned about the DP refcnt hmap associated with
> each
> > lflow. I understand it's necessary to handle the duplicated lflow cases,
> > but it looks heavy and removes the opportunities for more efficient
> bitmap
> > operations. Let me know if you have already had evaluated its
> performance.
> >
>
> Hi Han,
>
> I did some testing on a large scaled OVN database.
> The NB database has
>  - 1000 logical switches
>  - 500 routers
>  - 35253 load balancers
>  - Most of these load balancers are associated with all the
> logical switches and routers.
>
> When I run this command for example
>- ovn-nbctl set load_balancer 0d647ff9-4e49-4570-a05d-db670873b7ef
> options:foo=bar
>
> It results in changes to all the logical routers. And the function
> lflow_handle_lr_stateful_changes() is called.
> If you see this function, for each changed router, it also loops
> through the router ports and calls
>  - build_lbnat_lflows_iterate_by_lrp()
>  - build_lbnat_lflows_iterate_by_lsp()
>
> With your suggestion we also need to call
> build_lswitch_and_lrouter_iterate_by_lsp() and
> build_lbnat_lflows_iterate_by_lrp().
>
> I measured the number of lflows referenced in op->lflow_ref and
> op->stateful_lflow_ref
> for each of the logical switch port  and router port pair.  Total
> lflows in lflow_ref
> (of all router ports and their peer switch ports) were 23000 and total
> lflows in stateful_lflow_ref
> were just 4000.
>
> So with just one lflow_ref (as per suggestion) a small update to load
> balancer like above
> would result in generating 27000 logical flows as compared to just 4000.
>
> I think it has a considerable cost in terms of CPU.  And perhaps it
> would matter more
> when ovn-northd runs in a DPU.  My preference would be to have a
> separate lflow ref
> for stateful flows.
>
> Thanks
> Numan


Thanks for the data points! It makes sense to use separate lists.

Regards,
Han

>
>
>
> > Thanks,
> > Han
> >
> > > };
> > >
> > > All the logical flows generated by
> > > build_lswitch_and_lrouter_iterate_by_lsp() uses the
> ovn_port->lflow_ref.
> > >
> > > All the logical flows generated by build_lsp_lflows_for_lbnats()
> > > uses the ovn_port->stateful_lflow_ref.
> > >
> > > When handling the ovn_port changes incrementally, the lflows referenced
> > > in 'struct ovn_port' are cleared and regenerated and synced to the
> > > SB logical flows.
> > >
> > > eg.
> > >
> > > lflow_ref_clear_lflows(op->lflow_ref);
> > > build_lswitch_and_lrouter_iterate_by_lsp(op, ...);
> > > lflow_ref_sync_lflows_to_sb(op->lflow_ref, ...);
> > >
> > > This patch does few more changes:
> > >   -  Logical flows are now hashed without the logical
> > >  datapaths.  If a logical flow is referenced by just one
> > >  datapath, we don't rehash it.
> > >
> > >   -  The synthetic 'hash' column of sbrec_logical_flow now
> > >  doesn't use the logical datapath.  This means that
> > >  when ovn-northd is updated/upgraded and has this commit,
> > >  all the logical flows with 'logical_datapath' column
> > >  set will get deleted and re-added 

Re: [ovs-dev] [PATCH ovn] ovn-ic: fix global blacklist filter for IPv6 addresses

[0-day Robot]

Bleep bloop.  Greetings Roberto Bartzen Acosta, 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: The subject summary should start with a capital.
WARNING: The subject summary should end with a dot.
Subject: ovn-ic: fix global blacklist filter for IPv6 addresses
ERROR: Author Roberto Bartzen Acosta  needs to 
sign off.
WARNING: Line lacks whitespace around operator
#28 FILE: ic/ovn-ic.c:1032:
for (int i = 0; i < (plen/8); i++) {

Lines checked: 158, Warnings: 3, 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 ovn] ovn-ic: fix global blacklist filter for IPv6 addresses

[Roberto Bartzen Acosta via dev]

This commit fixes the prefix filter function as the return condition for IPv6 
addresses is disabling the advertisement of all learned prefixes regardless of 
the match with the blacklist or not.

Reported-at: https://bugs.launchpad.net/ubuntu/+source/ovn/+bug/2046804
Fixes: 57b347c55168 ("ovn-ic: Route advertisement.")
---
 ic/ovn-ic.c | 22 
 tests/ovn-ic.at | 92 +
 2 files changed, 108 insertions(+), 6 deletions(-)

diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 6f8f5734d..d8e038801 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -1024,6 +1024,20 @@ prefix_is_link_local(struct in6_addr *prefix, unsigned 
int plen)
 ((prefix->s6_addr[1] & 0xc0) == 0x80));
 }
 
+static bool
+compare_ipv6_prefixes(const struct in6_addr *s_prefix,
+  const struct in6_addr *d_prefix2, int plen)
+{
+struct in6_addr mask = ipv6_create_mask(plen);
+for (int i = 0; i < (plen/8); i++) {
+if ((s_prefix->s6_addr[i] & mask.s6_addr[i]) ^
+(d_prefix2->s6_addr[i] & mask.s6_addr[i])) {
+return false;
+}
+}
+return true;
+}
+
 static bool
 prefix_is_black_listed(const struct smap *nb_options,
struct in6_addr *prefix,
@@ -1064,12 +1078,8 @@ prefix_is_black_listed(const struct smap *nb_options,
 continue;
 }
 } else {
-struct in6_addr mask = ipv6_create_mask(bl_plen);
-for (int i = 0; i < 16 && mask.s6_addr[i] != 0; i++) {
-if ((prefix->s6_addr[i] & mask.s6_addr[i])
-!= (bl_prefix.s6_addr[i] & mask.s6_addr[i])) {
-continue;
-}
+if (!compare_ipv6_prefixes(prefix, _prefix, bl_plen)) {
+continue;
 }
 }
 matched = true;
diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
index d4c436f84..42ab89aef 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -1274,3 +1274,95 @@ OVN_CLEANUP_IC([az1], [az2])
 
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-ic -- route sync -- IPv6 blacklist filter])
+AT_KEYWORDS([IPv6-route-sync-blacklist])
+
+ovn_init_ic_db
+ovn-ic-nbctl ts-add ts1
+
+for i in 1 2; do
+ovn_start az$i
+ovn_as az$i
+
+# Enable route learning at AZ level
+ovn-nbctl set nb_global . options:ic-route-learn=true
+# Enable route advertising at AZ level
+ovn-nbctl set nb_global . options:ic-route-adv=true
+# Enable blacklist single filter for IPv6
+ovn-nbctl set nb_global . options:ic-route-blacklist="2003:db8:1::/64,\
+2004:::/32,2005:1234:5678::/40"
+
+OVS_WAIT_UNTIL([ovn-nbctl show | grep ts1])
+
+# Create LRP and connect to TS
+ovn-nbctl lr-add lr$i
+ovn-nbctl lrp-add lr$i lrp-lr$i-ts1 aa:aa:aa:aa:aa:0$i 2001:db8:1::$i/64
+ovn-nbctl lsp-add ts1 lsp-ts1-lr$i \
+-- lsp-set-addresses lsp-ts1-lr$i router \
+-- lsp-set-type lsp-ts1-lr$i router \
+-- lsp-set-options lsp-ts1-lr$i router-port=lrp-lr$i-ts1
+
+ovn-nbctl lrp-add lr$i lrp-lr$i-p$i 00:00:00:00:00:0$i 2002:db8:1::$i/64
+
+# Create blacklisted LRPs and connect to TS
+ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext$i \
+11:11:11:11:11:1$i 2003:db8:1::$i/64
+
+ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext2$i \
+22:22:22:22:22:2$i 2004::bbb::$i/48
+
+ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext3$i \
+33:33:33:33:33:3$i 2005:1234:5678::$i/50
+
+done
+
+for i in 1 2; do
+OVS_WAIT_UNTIL([ovn_as az$i ovn-nbctl lr-route-list lr$i | grep learned])
+done
+
+AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr1 |
+awk '/learned/{print $1, $2}' ], [0], [dnl
+2002:db8:1::/64 2001:db8:1::2
+])
+
+for i in 1 2; do
+ovn_as az$i
+
+# Drop blacklist
+ovn-nbctl remove nb_global . options ic-route-blacklist
+
+done
+
+OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr1 |
+awk '/learned/{print $1, $2}' | sort ], [0], [dnl
+2002:db8:1::/64 2001:db8:1::2
+2003:db8:1::/64 2001:db8:1::2
+2004::bbb::/48 2001:db8:1::2
+2005:1234:5678::/50 2001:db8:1::2
+])
+
+for i in 1 2; do
+ovn_as az$i
+
+ovn-nbctl set nb_global . \
+options:ic-route-blacklist="2003:db8:1::/64,2004:db8:1::/64"
+
+# Create an 'extra' blacklisted LRP and connect to TS
+ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext2$i \
+44:44:44:44:44:4$i 2004:db8:1::$i/64
+
+done
+
+OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr1 |
+awk '/learned/{print $1, $2}' | sort ], [0], [dnl
+2002:db8:1::/64 2001:db8:1::2
+2004::bbb::/48 2001:db8:1::2
+2005:1234:5678::/50 2001:db8:1::2
+])
+
+OVN_CLEANUP_IC([az1], [az2])
+
+AT_CLEANUP
+])
-- 
2.25.1


-- 




_'Esta mensagem é direcionada apenas para os endereços constantes no 
cabeçalho inicial. Se você não está listado nos endereços constantes no 
cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa 
mensagem e cuja 

Re: [ovs-dev] [PATCH ovn v5 11/16] northd: Handle lb changes in lflow engine.

[Numan Siddique]

On Fri, Jan 19, 2024 at 6:13 AM Dumitru Ceara  wrote:
>
> On 1/11/24 16:32, num...@ovn.org wrote:
> > From: Numan Siddique 
> >
> > Since northd tracked data has the changed lb data, northd
> > engine handler for lflow engine now handles the lb changes
> > incrementally.  All the lflows generated for each lb is
>
> The first sentence is a bit confusing.  What about re-phrasing it to:
>
> Since northd tracked data has the changed lb information, the lflow
> change handler for northd inputs can now handle lb updates incrementally.

Ack.


>
> > stored in the ovn_lb_datapaths->lflow_ref and this is used
> > similar to how we handle ovn_port changes.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  northd/en-lflow.c   | 11 ++---
> >  northd/lb.c |  3 ++
> >  northd/lb.h | 26 
> >  northd/lflow-mgr.c  | 47 +-
> >  northd/northd.c | 98 +++--
> >  northd/northd.h |  4 ++
> >  tests/ovn-northd.at | 30 ++
> >  7 files changed, 184 insertions(+), 35 deletions(-)
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index fef9a1352d..205605578f 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -123,11 +123,6 @@ lflow_northd_handler(struct engine_node *node,
> >  return false;
> >  }
> >
> > -/* Fall back to recompute if load balancers have changed. */
> > -if (northd_has_lbs_in_tracked_data(_data->trk_data)) {
> > -return false;
> > -}
> > -
> >  const struct engine_context *eng_ctx = engine_get_context();
> >  struct lflow_data *lflow_data = data;
> >
> > @@ -140,6 +135,12 @@ lflow_northd_handler(struct engine_node *node,
> >  return false;
> >  }
> >
> > +if (!lflow_handle_northd_lb_changes(eng_ctx->ovnsb_idl_txn,
> > +_data->trk_data.trk_lbs,
> > +_input, lflow_data->lflow_table)) {
>
> Nit: indentation

Ack.


>
> > +return false;
> > +}
> > +
> >  engine_set_node_state(node, EN_UPDATED);
> >  return true;
> >  }
> > diff --git a/northd/lb.c b/northd/lb.c
> > index e6c8a51911..5fca41e049 100644
> > --- a/northd/lb.c
> > +++ b/northd/lb.c
> > @@ -21,6 +21,7 @@
> >
> >  /* OVN includes */
> >  #include "lb.h"
> > +#include "lflow-mgr.h"
> >  #include "lib/lb.h"
> >  #include "northd.h"
> >
> > @@ -33,6 +34,7 @@ ovn_lb_datapaths_create(const struct ovn_northd_lb *lb, 
> > size_t n_ls_datapaths,
> >  lb_dps->lb = lb;
> >  lb_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
> >  lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
> > +lb_dps->lflow_ref = lflow_ref_create();
> >
> >  return lb_dps;
> >  }
> > @@ -42,6 +44,7 @@ ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *lb_dps)
> >  {
> >  bitmap_free(lb_dps->nb_lr_map);
> >  bitmap_free(lb_dps->nb_ls_map);
> > +lflow_ref_destroy(lb_dps->lflow_ref);
> >  free(lb_dps);
> >  }
> >
> > diff --git a/northd/lb.h b/northd/lb.h
> > index eeef2ea260..de677ca4ef 100644
> > --- a/northd/lb.h
> > +++ b/northd/lb.h
> > @@ -20,6 +20,8 @@
> >  #include "openvswitch/hmap.h"
> >  #include "uuid.h"
> >
> > +struct lflow_ref;
> > +
> >  struct ovn_lb_datapaths {
> >  struct hmap_node hmap_node;
> >
> > @@ -29,6 +31,30 @@ struct ovn_lb_datapaths {
> >
> >  size_t n_nb_lr;
> >  unsigned long *nb_lr_map;
> > +
> > +/* Reference of lflows generated for this load balancer.
> > + *
> > + * This data is initialized and destroyed by the en_northd node, but
> > + * populated and used only by the en_lflow node. Ideally this data 
> > should
> > + * be maintained as part of en_lflow's data (struct lflow_data): a hash
> > + * index from ovn_port key to lflows.  However, it would be less 
> > efficient
> > + * and more complex:
> > + *
> > + * 1. It would require an extra search (using the index) to find the
> > + * lflows.
> > + *
> > + * 2. Building the index needs to be thread-safe, using either a global
> > + * lock which is obviously less efficient, or hash-based lock array 
> > which
> > + * is more complex.
> > + *
> > + * Maintaining the lflow_ref here is more straightforward. The 
> > drawback is
> > + * that we need to keep in mind that this data belongs to en_lflow 
> > node,
> > + * so never access it from any other nodes.
> > + *
> > + * 'lflow_ref' is used to reference logical flows generated for this
> > + *  load balancer.
> > + */
> > +struct lflow_ref *lflow_ref;
> >  };
> >
> >  struct ovn_lb_datapaths *ovn_lb_datapaths_create(const struct 
> > ovn_northd_lb *,
> > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> > index 3cf9696f6e..6cb2a367fe 100644
> > --- a/northd/lflow-mgr.c
> > +++ b/northd/lflow-mgr.c
> > @@ -375,7 +375,15 @@ struct lflow_ref_node {
> >  /* The lflow. */
> >  struct ovn_lflow *lflow;
> >
> > -/* Index id of the datapath 

Re: [ovs-dev] [PATCH ovn v5 11/16] northd: Handle lb changes in lflow engine.

[Numan Siddique]

On Thu, Jan 25, 2024 at 1:25 AM Han Zhou  wrote:
>
> On Thu, Jan 11, 2024 at 7:33 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > Since northd tracked data has the changed lb data, northd
> > engine handler for lflow engine now handles the lb changes
> > incrementally.  All the lflows generated for each lb is
> > stored in the ovn_lb_datapaths->lflow_ref and this is used
> > similar to how we handle ovn_port changes.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  northd/en-lflow.c   | 11 ++---
> >  northd/lb.c |  3 ++
> >  northd/lb.h | 26 
> >  northd/lflow-mgr.c  | 47 +-
> >  northd/northd.c | 98 +++--
> >  northd/northd.h |  4 ++
> >  tests/ovn-northd.at | 30 ++
> >  7 files changed, 184 insertions(+), 35 deletions(-)
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index fef9a1352d..205605578f 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -123,11 +123,6 @@ lflow_northd_handler(struct engine_node *node,
> >  return false;
> >  }
> >
> > -/* Fall back to recompute if load balancers have changed. */
> > -if (northd_has_lbs_in_tracked_data(_data->trk_data)) {
> > -return false;
> > -}
> > -
> >  const struct engine_context *eng_ctx = engine_get_context();
> >  struct lflow_data *lflow_data = data;
> >
> > @@ -140,6 +135,12 @@ lflow_northd_handler(struct engine_node *node,
> >  return false;
> >  }
> >
> > +if (!lflow_handle_northd_lb_changes(eng_ctx->ovnsb_idl_txn,
> > +_data->trk_data.trk_lbs,
> > +_input, lflow_data->lflow_table)) {
> > +return false;
> > +}
> > +
> >  engine_set_node_state(node, EN_UPDATED);
> >  return true;
> >  }
> > diff --git a/northd/lb.c b/northd/lb.c
> > index e6c8a51911..5fca41e049 100644
> > --- a/northd/lb.c
> > +++ b/northd/lb.c
> > @@ -21,6 +21,7 @@
> >
> >  /* OVN includes */
> >  #include "lb.h"
> > +#include "lflow-mgr.h"
> >  #include "lib/lb.h"
> >  #include "northd.h"
> >
> > @@ -33,6 +34,7 @@ ovn_lb_datapaths_create(const struct ovn_northd_lb *lb,
> size_t n_ls_datapaths,
> >  lb_dps->lb = lb;
> >  lb_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
> >  lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
> > +lb_dps->lflow_ref = lflow_ref_create();
> >
> >  return lb_dps;
> >  }
> > @@ -42,6 +44,7 @@ ovn_lb_datapaths_destroy(struct ovn_lb_datapaths
> *lb_dps)
> >  {
> >  bitmap_free(lb_dps->nb_lr_map);
> >  bitmap_free(lb_dps->nb_ls_map);
> > +lflow_ref_destroy(lb_dps->lflow_ref);
> >  free(lb_dps);
> >  }
> >
> > diff --git a/northd/lb.h b/northd/lb.h
> > index eeef2ea260..de677ca4ef 100644
> > --- a/northd/lb.h
> > +++ b/northd/lb.h
> > @@ -20,6 +20,8 @@
> >  #include "openvswitch/hmap.h"
> >  #include "uuid.h"
> >
> > +struct lflow_ref;
> > +
> >  struct ovn_lb_datapaths {
> >  struct hmap_node hmap_node;
> >
> > @@ -29,6 +31,30 @@ struct ovn_lb_datapaths {
> >
> >  size_t n_nb_lr;
> >  unsigned long *nb_lr_map;
> > +
> > +/* Reference of lflows generated for this load balancer.
> > + *
> > + * This data is initialized and destroyed by the en_northd node, but
> > + * populated and used only by the en_lflow node. Ideally this data
> should
> > + * be maintained as part of en_lflow's data (struct lflow_data): a
> hash
> > + * index from ovn_port key to lflows.  However, it would be less
> efficient
> > + * and more complex:
> > + *
> > + * 1. It would require an extra search (using the index) to find the
> > + * lflows.
> > + *
> > + * 2. Building the index needs to be thread-safe, using either a
> global
> > + * lock which is obviously less efficient, or hash-based lock array
> which
> > + * is more complex.
> > + *
> > + * Maintaining the lflow_ref here is more straightforward. The
> drawback is
> > + * that we need to keep in mind that this data belongs to en_lflow
> node,
> > + * so never access it from any other nodes.
> > + *
> > + * 'lflow_ref' is used to reference logical flows generated for this
> > + *  load balancer.
> > + */
> > +struct lflow_ref *lflow_ref;
> >  };
> >
> >  struct ovn_lb_datapaths *ovn_lb_datapaths_create(const struct
> ovn_northd_lb *,
> > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> > index 3cf9696f6e..6cb2a367fe 100644
> > --- a/northd/lflow-mgr.c
> > +++ b/northd/lflow-mgr.c
> > @@ -375,7 +375,15 @@ struct lflow_ref_node {
> >  /* The lflow. */
> >  struct ovn_lflow *lflow;
> >
> > -/* Index id of the datapath this lflow_ref_node belongs to. */
> > +/* Indicates of the lflow was added with dp_group or not using
> > + * ovn_lflow_add_with_dp_group() macro. */
>
> nit: the sentence is a little confusing. Probably more clear to say:
> indicates whether the lflow was added with a 

Re: [ovs-dev] [PATCH ovn v5 03/16] northd: Move router ports SB PB options sync to sync_to_sb_pb node.

[Numan Siddique]

On Fri, Jan 26, 2024 at 4:30 AM Dumitru Ceara  wrote:
>
> On 1/25/24 22:01, Numan Siddique wrote:
> > On Thu, Jan 25, 2024, 3:29 PM Dumitru Ceara  wrote:
> >
> >> On 1/25/24 17:46, Numan Siddique wrote:
> >>> On Tue, Jan 16, 2024 at 6:15 AM Dumitru Ceara  wrote:
> 
>  On 1/11/24 16:28, num...@ovn.org wrote:
> > From: Numan Siddique 
> >
> > It also moves the logical router port IPv6 prefix delegation
> > updates to "sync-from-sb" engine node.
> >
> > With these changes, northd engine node doesn't need to do much
> > for SB Port binding changes other than updating 'op->sb'.  And
> > there is no need to fall back to recompute.  Prior to this patch
> > northd_handle_sb_port_binding_changes() was returning false for
> > all SB port binding updates except for the VIF PBs.  This patch
> > now handles updates to all the SB port binding by setting 'op->sb'.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> 
>  Hi Numan,
> 
>  With this patch applied the "633: ovn-northd.at:10298 SB Port binding
>  incremental processing -- parallelization=yes" test occasionally fails,
>  e.g.:
> 
>  https://github.com/dceara/ovn/actions/runs/7539663594/job/20524238508
> 
>  I saw it locally too when running the test in a loop:
> 
>  It fails with:
>  ovn-nbctl lrp-set-gateway-chassis lrp hv1
>  ./ovn-macros.at:449: "$@"
>  northd recompute count - 2
>  ./ovn-northd.at:10238: test x$northd_recomp = x$1
>  ./ovn-northd.at:10238: exit code was 1, expected 0
> 
>  Here:
> 
> >> https://github.com/dceara/ovn/blob/e1c76b6d347570618591fe6587f6af83445223ec/tests/ovn-northd.at#L10327
> 
>  For some reason in some cases only 2 recomputes get triggered.
> >>>
> >>> Thanks Dumitru and Han for the reviews.
> >>>
> >>> @Dumitru Ceara  I've addressed all the review comments below.
> >>>
> >>> I think the test case is failing because of the missing "--wait=sb" in
> >>> the ovn-nbctl commands.
> >>>
> >>
> >> I don't think it's only that.  It still fails even with your updated
> >> test changes, in the same way:
> >>
> >> as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
> >> ./ovn-macros.at:449: "$@"
> >> ovn-nbctl lrp-set-gateway-chassis lrp hv1
> >> ./ovn-macros.at:449: "$@"
> >> northd recompute count - 2
> >> ./ovn-northd.at:10238: test x$northd_recomp = x$1
> >> ./ovn-northd.at:10238: exit code was 1, expected 0
> >>
> >>> Will the below changes in ovn-northd.at look good to you  ?  If so,
> >>> and if you can provide your ack I can apply this patch.
> >>> Otherwise I'll include it in the v6.
> >>>
> >>
> >> I would prefer if we don't apply it until we figure out this test, the
> >> CI has been relatively stable the last few weeks, I'd rather not
> >> destabilize it if possible.
> >>
> >
> > Sure.  Definitely.
> >
> > Its passing for me locally when run in a loop.
> >
> > Let me dig into it further.
> >
> >
>
> Sorry, Numan, I had made a mistake when applying your incremental patch.
>  With the right changes applied it passes now for me too.
>
> Acked-by: Dumitru Ceara 

Thanks.  I applied this patch to the main.

Patches 1,2 and 3 are merged.  I'll submit the rest of the patches in
v6 and will apply those as one series
after all the reviews and acks.

Thanks
Numan

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

Re: [ovs-dev] [PATCH ovn v5 08/16] northd: Refactor lflow management into a separate module.

[Numan Siddique]

On Thu, Jan 25, 2024 at 1:08 AM Han Zhou  wrote:
>
> On Thu, Jan 11, 2024 at 7:32 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > ovn_lflow_add() and other related functions/macros are now moved
> > into a separate module - lflow-mgr.c.  This module maintains a
> > table 'struct lflow_table' for the logical flows.  lflow table
> > maintains a hmap to store the logical flows.
> >
> > It also maintains the logical switch and router dp groups.
> >
> > Previous commits which added lflow incremental processing for
> > the VIF logical ports, stored the references to
> > the logical ports' lflows using 'struct lflow_ref_list'.  This
> > struct is renamed to 'struct lflow_ref' and is part of lflow-mgr.c.
> > It is  modified a bit to store the resource to lflow references.
> >
> > Example usage of 'struct lflow_ref'.
> >
> > 'struct ovn_port' maintains 2 instances of lflow_ref.  i,e
> >
> > struct ovn_port {
> >...
> >...
> >struct lflow_ref *lflow_ref;
> >struct lflow_ref *stateful_lflow_ref;
>
> Hi Numan,
>
> In addition to the lock discussion with you and Dumitru, I still want to
> discuss another thing of this patch regarding the second lflow_ref:
> stateful_lflow_ref.
> I understand that you added this to achieve finer grained I-P especially
> for router ports. I am wondering how much performance gain is from this.
> For my understanding it shouldn't matter much since each ovn_port should be
> associated with a very limited number of lflows. Could you provide more
> insight/data on this? I think it would be better to keep things simple
> (i.e. one object, one lflow_ref list) unless the benefit is obvious.
>
> I am also trying to run another performance regression test for recompute,
> since I am a little concerned about the DP refcnt hmap associated with each
> lflow. I understand it's necessary to handle the duplicated lflow cases,
> but it looks heavy and removes the opportunities for more efficient bitmap
> operations. Let me know if you have already had evaluated its performance.
>

Hi Han,

I did some testing on a large scaled OVN database.
The NB database has
 - 1000 logical switches
 - 500 routers
 - 35253 load balancers
 - Most of these load balancers are associated with all the
logical switches and routers.

When I run this command for example
   - ovn-nbctl set load_balancer 0d647ff9-4e49-4570-a05d-db670873b7ef
options:foo=bar

It results in changes to all the logical routers. And the function
lflow_handle_lr_stateful_changes() is called.
If you see this function, for each changed router, it also loops
through the router ports and calls
 - build_lbnat_lflows_iterate_by_lrp()
 - build_lbnat_lflows_iterate_by_lsp()

With your suggestion we also need to call
build_lswitch_and_lrouter_iterate_by_lsp() and
build_lbnat_lflows_iterate_by_lrp().

I measured the number of lflows referenced in op->lflow_ref and
op->stateful_lflow_ref
for each of the logical switch port  and router port pair.  Total
lflows in lflow_ref
(of all router ports and their peer switch ports) were 23000 and total
lflows in stateful_lflow_ref
were just 4000.

So with just one lflow_ref (as per suggestion) a small update to load
balancer like above
would result in generating 27000 logical flows as compared to just 4000.

I think it has a considerable cost in terms of CPU.  And perhaps it
would matter more
when ovn-northd runs in a DPU.  My preference would be to have a
separate lflow ref
for stateful flows.

Thanks
Numan


> Thanks,
> Han
>
> > };
> >
> > All the logical flows generated by
> > build_lswitch_and_lrouter_iterate_by_lsp() uses the ovn_port->lflow_ref.
> >
> > All the logical flows generated by build_lsp_lflows_for_lbnats()
> > uses the ovn_port->stateful_lflow_ref.
> >
> > When handling the ovn_port changes incrementally, the lflows referenced
> > in 'struct ovn_port' are cleared and regenerated and synced to the
> > SB logical flows.
> >
> > eg.
> >
> > lflow_ref_clear_lflows(op->lflow_ref);
> > build_lswitch_and_lrouter_iterate_by_lsp(op, ...);
> > lflow_ref_sync_lflows_to_sb(op->lflow_ref, ...);
> >
> > This patch does few more changes:
> >   -  Logical flows are now hashed without the logical
> >  datapaths.  If a logical flow is referenced by just one
> >  datapath, we don't rehash it.
> >
> >   -  The synthetic 'hash' column of sbrec_logical_flow now
> >  doesn't use the logical datapath.  This means that
> >  when ovn-northd is updated/upgraded and has this commit,
> >  all the logical flows with 'logical_datapath' column
> >  set will get deleted and re-added causing some disruptions.
> >
> >   -  With the commit [1] which added I-P support for logical
> >  port changes, multiple logical flows with same match 'M'
> >  and actions 'A' are generated and stored without the
> >  dp groups, which was not the case prior to
> >  that patch.
> >  One example to generate these lflows is:
> >  ovn-nbctl lsp-set-addresses sw0p1 

Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule to latest OVS branch-3.3.

[Dumitru Ceara]

On 1/30/24 00:36, Dumitru Ceara wrote:
> This picks up the following relevant OVS commits:
>   8893e24d9d dpdk: Update to use v23.11.
>   ed738eca39 util: Annotate function that will never return NULL.
>   77d0bad04 mcast-snooping: Store IGMP/MLD protocol version.
>   b222593bc6 mcast-snooping: Add group protocol to mdb/show output.
>   a940a691e7 ovs-atomic: Fix inclusion of Clang header by GCC 14.
>   b0cf73112d dp-packet: Reset offload/offsets when clearing a packet.
> 
> This commit also ports the CI DPDK related changes from OVS commit
> 8893e24d9d09 ("dpdk: Update to use v23.11.") which are required as 23.11
> is the supported DPDK branch on OVS 3.3.  There's also a small change
> that had to be made when mangling the prefix path of the installed
> DPDK version in the container.
> 
> As a side effect this will also address OVN Fedora 40 (rawhide) build
> failures on aarch64 and s390x.  They are fixed by a940a691e7d9
> ("ovs-atomic: Fix inclusion of Clang header by GCC 14.").
> 
> Suggested-by: Ilya Maximets 
> Signed-off-by: Dumitru Ceara 
> ---

CC: Eelco, Mohammad, Numan.

>  .ci/dpdk-build.sh  | 28 ++--
>  .ci/linux-build.sh | 11 ++-
>  .github/workflows/test.yml |  4 ++--
>  controller/pinctrl.c   |  6 +-
>  ovs|  2 +-
>  5 files changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
> index f44ac15b07..0c13c98c98 100755
> --- a/.ci/dpdk-build.sh
> +++ b/.ci/dpdk-build.sh
> @@ -5,25 +5,27 @@ set -x
>  
>  function build_dpdk()
>  {
> -local VERSION_FILE="dpdk-dir/cached-version"
>  local DPDK_VER=$1
>  local DPDK_OPTS=""
> +local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
> +local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
>  
> -rm -rf dpdk-dir
> +rm -rf dpdk-src
> +rm -rf $DPDK_INSTALL_DIR
>  
>  if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
> -git clone --single-branch $DPDK_GIT dpdk-dir -b 
> "${DPDK_VER##refs/*/}"
> -pushd dpdk-dir
> +git clone --single-branch $DPDK_GIT dpdk-src -b 
> "${DPDK_VER##refs/*/}"
> +pushd dpdk-src
>  git log -1 --oneline
>  else
>  wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
>  tar xvf dpdk-$1.tar.xz > /dev/null
>  DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
> -mv ${DIR_NAME} dpdk-dir
> -pushd dpdk-dir
> +mv ${DIR_NAME} dpdk-src
> +pushd dpdk-src
>  fi
>  
> -# Switching to 'default' machine to make dpdk-dir cache usable on
> +# Switching to 'default' machine to make the dpdk cache usable on
>  # different CPUs. We can't be sure that all CI machines are exactly same.
>  DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
>  
> @@ -38,16 +40,22 @@ function build_dpdk()
>  # only depend on virtio/tap drivers.
>  # We can disable all remaining drivers to save compilation time.
>  DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
> +# OVS depends on the vhost library (and its dependencies).
> +# net/tap depends on the gso library.
> +DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
>  
>  # Install DPDK using prefix.
> -DPDK_OPTS="$DPDK_OPTS --prefix=$(pwd)/build"
> +DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
>  
>  meson $DPDK_OPTS build
>  ninja -C build
>  ninja -C build install
> -
> -echo "Installed DPDK in $(pwd)"
>  popd
> +
> +# Remove examples sources.
> +rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
> +
> +echo "Installed DPDK in $DPDK_INSTALL_DIR"
>  echo "${DPDK_VER}" > ${VERSION_FILE}
>  }
>  
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index b0c3c9252e..78f17f8bdb 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -13,24 +13,25 @@ RECHECK=${RECHECK:-"no"}
>  
>  function install_dpdk()
>  {
> -local VERSION_FILE="dpdk-dir/cached-version"
> -local DPDK_LIB=$(pwd)/dpdk-dir/build/lib/x86_64-linux-gnu
> +local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
> +local VERSION_FILE="${DPDK_INSTALL_DIR}/cached-version"
> +local DPDK_LIB=${DPDK_INSTALL_DIR}/lib/x86_64-linux-gnu
>  
>  # Export the following path for pkg-config to find the .pc file.
>  export PKG_CONFIG_PATH=$DPDK_LIB/pkgconfig/:$PKG_CONFIG_PATH
>  
>  if [ ! -f "${VERSION_FILE}" ]; then
> -echo "Could not find DPDK in $(pwd)/dpdk-dir"
> +echo "Could not find DPDK in $DPDK_INSTALL_DIR"
>  return 1
>  fi
>  
>  # As we build inside a container we need to update the prefix.
> -sed -i -E "s|^prefix=.*|prefix=$(pwd)/dpdk-dir/build|" \
> +sed -i -E "s|^prefix=.*|prefix=${DPDK_INSTALL_DIR}|" \
>  "$DPDK_LIB/pkgconfig/libdpdk-libs.pc"
>  
>  # Update the library paths.
>  sudo ldconfig
> -echo "Found cached DPDK $(cat ${VERSION_FILE}) build in $(pwd)/dpdk-dir"
> +echo "Found cached DPDK $(cat 

Re: [ovs-dev] [PATCH ovn 1/2] ovs: Bump submodule to include igmp protocol version.

[Dumitru Ceara]

On 1/29/24 16:30, Ilya Maximets wrote:
> On 1/23/24 14:53, Mohammad Heib wrote:
>> Hi Dumitru,
>>
>> Thank you for the review,
>> yes sure will bump the submodule to the last stable once they fix the issue.
> 
> I applied the BFD fix and also applied the typedef fix for the 
> mcast_group_proto
> enum.  So, make sure to include both for the update.
> 
> There are a few other things though.
> 
> This patch needs to upgrae the DPDk version, since OVS 3.3 will be using DPDK 
> 23.11
> and branch-3.3 is supposed to be used with this version.  22.11 is not 
> supported on
> that branch.  So, you need to port CI changes from:
>   
> https://github.com/openvswitch/ovs/commit/8893e24d9d0921aaf934f263a06ba223ef0db369
> 

It turns out there's another issue at hand, this time on Fedora 40
(rawhide) where OVN builds fail completely without picking up latest
branch-3.3.  That triggered me to go ahead and post a patch to bump OVN
submodule, take care of the DPDK update and pick up all other fixes:

https://patchwork.ozlabs.org/project/ovn/patch/20240129233652.123111-1-dce...@redhat.com/

I hope that's fine.

Regards,
Dumitru

> See one more comment inline.
> 
>> Thanks
>>
>> On Mon, Jan 22, 2024 at 5:01 PM Dumitru Ceara  wrote:
>>
>>> On 1/22/24 15:14, Mohammad Heib wrote:
 Specifically the following commit:
   077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.")

 Also fix a small compilation error due to prototype change.

 Signed-off-by: Mohammad Heib 
 ---
>>>
>>> Hi Mohammad,
>>>
>>> Thanks for the patch!
>>>
  controller/pinctrl.c | 6 +-
  ovs  | 2 +-
  2 files changed, 6 insertions(+), 2 deletions(-)

 diff --git a/controller/pinctrl.c b/controller/pinctrl.c
 index 4992eab08..77bf67e58 100644
 --- a/controller/pinctrl.c
 +++ b/controller/pinctrl.c
 @@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn,
  switch (ntohs(ip_flow->tp_src)) {
  case IGMP_HOST_MEMBERSHIP_REPORT:
  case IGMPV2_HOST_MEMBERSHIP_REPORT:
 +mcast_group_proto grp_proto =
> 
> You're defining a variable inside the case, the whole case should be
> braced for this to work.  It may work with some compilers, but it is
> generally incorrect and CI fails to build because of this.
> Alternatively, move the declaration outside of the switch statement.
> 
 +(ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT)
 +? MCAST_GROUP_IGMPV1
 +: MCAST_GROUP_IGMPV2;
  group_change =
  mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN,
 -  port_key_data);
 +  port_key_data, grp_proto);
  break;
  case IGMP_HOST_LEAVE_MESSAGE:
  group_change =
 diff --git a/ovs b/ovs
 index 4102674b3..b222593bc 16
 --- a/ovs
 +++ b/ovs
 @@ -1 +1 @@
 -Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6
 +Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3
>>>
>>> However, it's probably desirable to bump the submodule to the tip of the
>>> latest stable branch, i.e. branch-3.3:
>>>
>>>
>>> https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases
>>>
>>> That would also fix our scheduled CI runs:
>>>
>>> https://github.com/ovn-org/ovn/actions/runs/7597582431/job/20692570222#step:10:3531
>>>
>>> However, there's a crash in OVS on branch-3.3 that needs to be fixed first:
>>> https://issues.redhat.com/browse/FDP-300
>>>
>>> I'd wait with bumping the submodule until then.
>>>
>>> Regards,
>>> Dumitru
> 

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

[ovs-dev] [PATCH ovn] ovs: Bump submodule to latest OVS branch-3.3.

[Dumitru Ceara]

This picks up the following relevant OVS commits:
  8893e24d9d dpdk: Update to use v23.11.
  ed738eca39 util: Annotate function that will never return NULL.
  77d0bad04 mcast-snooping: Store IGMP/MLD protocol version.
  b222593bc6 mcast-snooping: Add group protocol to mdb/show output.
  a940a691e7 ovs-atomic: Fix inclusion of Clang header by GCC 14.
  b0cf73112d dp-packet: Reset offload/offsets when clearing a packet.

This commit also ports the CI DPDK related changes from OVS commit
8893e24d9d09 ("dpdk: Update to use v23.11.") which are required as 23.11
is the supported DPDK branch on OVS 3.3.  There's also a small change
that had to be made when mangling the prefix path of the installed
DPDK version in the container.

As a side effect this will also address OVN Fedora 40 (rawhide) build
failures on aarch64 and s390x.  They are fixed by a940a691e7d9
("ovs-atomic: Fix inclusion of Clang header by GCC 14.").

Suggested-by: Ilya Maximets 
Signed-off-by: Dumitru Ceara 
---
 .ci/dpdk-build.sh  | 28 ++--
 .ci/linux-build.sh | 11 ++-
 .github/workflows/test.yml |  4 ++--
 controller/pinctrl.c   |  6 +-
 ovs|  2 +-
 5 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
index f44ac15b07..0c13c98c98 100755
--- a/.ci/dpdk-build.sh
+++ b/.ci/dpdk-build.sh
@@ -5,25 +5,27 @@ set -x
 
 function build_dpdk()
 {
-local VERSION_FILE="dpdk-dir/cached-version"
 local DPDK_VER=$1
 local DPDK_OPTS=""
+local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
+local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
 
-rm -rf dpdk-dir
+rm -rf dpdk-src
+rm -rf $DPDK_INSTALL_DIR
 
 if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
-git clone --single-branch $DPDK_GIT dpdk-dir -b "${DPDK_VER##refs/*/}"
-pushd dpdk-dir
+git clone --single-branch $DPDK_GIT dpdk-src -b "${DPDK_VER##refs/*/}"
+pushd dpdk-src
 git log -1 --oneline
 else
 wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
 tar xvf dpdk-$1.tar.xz > /dev/null
 DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
-mv ${DIR_NAME} dpdk-dir
-pushd dpdk-dir
+mv ${DIR_NAME} dpdk-src
+pushd dpdk-src
 fi
 
-# Switching to 'default' machine to make dpdk-dir cache usable on
+# Switching to 'default' machine to make the dpdk cache usable on
 # different CPUs. We can't be sure that all CI machines are exactly same.
 DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
 
@@ -38,16 +40,22 @@ function build_dpdk()
 # only depend on virtio/tap drivers.
 # We can disable all remaining drivers to save compilation time.
 DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
+# OVS depends on the vhost library (and its dependencies).
+# net/tap depends on the gso library.
+DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
 
 # Install DPDK using prefix.
-DPDK_OPTS="$DPDK_OPTS --prefix=$(pwd)/build"
+DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
 
 meson $DPDK_OPTS build
 ninja -C build
 ninja -C build install
-
-echo "Installed DPDK in $(pwd)"
 popd
+
+# Remove examples sources.
+rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
+
+echo "Installed DPDK in $DPDK_INSTALL_DIR"
 echo "${DPDK_VER}" > ${VERSION_FILE}
 }
 
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index b0c3c9252e..78f17f8bdb 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -13,24 +13,25 @@ RECHECK=${RECHECK:-"no"}
 
 function install_dpdk()
 {
-local VERSION_FILE="dpdk-dir/cached-version"
-local DPDK_LIB=$(pwd)/dpdk-dir/build/lib/x86_64-linux-gnu
+local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
+local VERSION_FILE="${DPDK_INSTALL_DIR}/cached-version"
+local DPDK_LIB=${DPDK_INSTALL_DIR}/lib/x86_64-linux-gnu
 
 # Export the following path for pkg-config to find the .pc file.
 export PKG_CONFIG_PATH=$DPDK_LIB/pkgconfig/:$PKG_CONFIG_PATH
 
 if [ ! -f "${VERSION_FILE}" ]; then
-echo "Could not find DPDK in $(pwd)/dpdk-dir"
+echo "Could not find DPDK in $DPDK_INSTALL_DIR"
 return 1
 fi
 
 # As we build inside a container we need to update the prefix.
-sed -i -E "s|^prefix=.*|prefix=$(pwd)/dpdk-dir/build|" \
+sed -i -E "s|^prefix=.*|prefix=${DPDK_INSTALL_DIR}|" \
 "$DPDK_LIB/pkgconfig/libdpdk-libs.pc"
 
 # Update the library paths.
 sudo ldconfig
-echo "Found cached DPDK $(cat ${VERSION_FILE}) build in $(pwd)/dpdk-dir"
+echo "Found cached DPDK $(cat ${VERSION_FILE}) build in $DPDK_INSTALL_DIR"
 }
 
 function configure_ovs()
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 02865b32f2..2503d87d0f 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -16,8 +16,8 @@ jobs:
 env:
   dependencies: gcc libnuma-dev ninja-build
  

[ovs-dev] [PATCH] github: Bump Fedora version to 39.

[Ilya Maximets]

Fedora 37 reached EOL in November.  Switch to the most recent version
to avoid potential CI failures in the future.

Signed-off-by: Ilya Maximets 
---
 .github/workflows/build-and-test.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index ddb425580..fc7558148 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -462,7 +462,7 @@ jobs:
   build-linux-rpm:
 name: linux rpm fedora
 runs-on: ubuntu-latest
-container: fedora:37
+container: fedora:39
 timeout-minutes: 30
 
 strategy:
-- 
2.43.0

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

Re: [ovs-dev] [PATCH v2] github: Update versions of action dependencies (Node.js 20).

[Ilya Maximets]

On 1/26/24 17:00, Eelco Chaudron wrote:
> 
> 
> On 26 Jan 2024, at 15:38, Ilya Maximets wrote:
> 
>> checkout@v3, cache@v3 and setup-python@v4 are using outdated Node.js 16
>> which is now deprecated in GHA [1], so these actions will stop working
>> soon.
>>
>> Updating to most recent major versions with Node.js 20.  This stops GHA
>> from throwing warnings in every build.
>>
>> [1] 
>> https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/
>>
>> While at it also updating upload-artifact version to the latest version.
>> Removed versions from the comment as the general behavior of this
>> action regarding symlinks and wildcards doesn't seem to change between
>> versions much.
>>
>> Signed-off-by: Ilya Maximets 
> 
> Looks good to me! Tests run without any problems.
> 
> Acked-by: Eelco Chaudron 
> 

Thanks!  Applied and backported down to 2.17.

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

Re: [ovs-dev] [PATCH OVN] Add "disable_arp_nd_rsp" option to LSP

[Ihar Hrachyshka]

On Mon, Jan 22, 2024 at 12:22 PM Naveen Yerramneni <
naveen.yerramn...@nutanix.com> wrote:

> This option can be used to enable/disable arp/nd reply flows.
>
> Usecase:
> =
> It is useful to reduce packet loss when VM is being migrated to
>

It may indeed be useful to be able to disable ARP responder for a LS/port.

I am wondering if you have details about your packet loss issues when
migrating a VM. Could you please confirm that we are talking about live
migration (e.g. through libvirt) and that you already use multichassis port
bindings to host the same port on multiple chassis (on source and
destination)? In this case, OVN will set up flows that will clone (flood)
traffic to both locations proactively, for the moment when your hypervisor
switches running the VM from source to destination. You should not observe
(significant) packet losses in this scenario.


> different AZ via VXLAN tunnel. Port is configured in both AZs
> on different logical switches which are sharing same IP subnet.
>

This snippet above suggests to me that you migrate between different
logical switch ports? Could you please elaborate on how you set up your
overlay connectivity for the VM?

The reason I ask is because live migration reuses the same LSP, only
changing the chassis that host(s) the LSP.


> In reality, the port is active on only one logical switch.
> Skipping ARP/ND responder and letting the ARP/ND get flooded to
> learn the location of the port.
>
> Signed-off-by: Naveen Yerramneni 
> ---
>  northd/northd.c | 10 +-
>  tests/ovn-northd.at | 31 +++
>  2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 952f8200d..4e070c0fe 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -1844,6 +1844,12 @@ localnet_can_learn_mac(const struct
> nbrec_logical_switch_port *nbsp)
>  return smap_get_bool(>options, "localnet_learn_fdb", false);
>  }
>
> +static bool
> +lsp_disable_arp_nd_rsp(const struct nbrec_logical_switch_port *nbsp)
> +{
> +return smap_get_bool(>options, "disable_arp_nd_rsp", false);
> +}
> +
>  static bool
>  lsp_is_type_changed(const struct sbrec_port_binding *sb,
>  const struct nbrec_logical_switch_port *nbsp,
> @@ -9921,7 +9927,9 @@ build_lswitch_arp_nd_responder_known_ips(struct
> ovn_port *op,
>  return;
>  }
>
> -if (lsp_is_external(op->nbsp) || op->has_unknown) {
> +if (lsp_is_external(op->nbsp) || op->has_unknown ||
> +   (!strcmp(op->nbsp->type, "") &&
> +lsp_disable_arp_nd_rsp(op->nbsp))) {
>  return;
>  }
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 9a0d418e4..9a36ee810 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -11094,5 +11094,36 @@ AT_CHECK([ovn-sbctl dump-flows S1 | grep pre_acl
> | sed 's/table=./table=?/'], [0
>  ])
>
>
> +AT_CLEANUP
> +])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([check options:disable_arp_nd_rsp for LSP])
> +ovn_start NORTHD_TYPE
> +ovn-nbctl ls-add S1
> +ovn-nbctl --wait=sb lsp-add S1 S1-vm1
> +ovn-nbctl --wait=sb lsp-set-addresses S1-vm1 "50:54:00:00:00:010
> 192.168.0.10 fd00::10"
> +
> +ovn-sbctl dump-flows S1 > S1flows
> +AT_CAPTURE_FILE([S1flows])
> +
> +AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | sed 's/table=../table=??/'],
> [0], [dnl
> +  table=??(ls_in_arp_rsp  ), priority=100  , match=(arp.tpa ==
> 192.168.0.10 && arp.op == 1 && inport == "S1-vm1"), action=(next;)
> +  table=??(ls_in_arp_rsp  ), priority=100  , match=(nd_ns && ip6.dst
> == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport ==
> "S1-vm1"), action=(next;)
> +  table=??(ls_in_arp_rsp  ), priority=50   , match=(arp.tpa ==
> 192.168.0.10 && arp.op == 1), action=(eth.dst = eth.src; eth.src =
> 50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha =
> 50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 192.168.0.10; outport =
> inport; flags.loopback = 1; output;)
> +  table=??(ls_in_arp_rsp  ), priority=50   , match=(nd_ns && ip6.dst
> == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na {
> eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10;
> nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output;
> };)
> +  table=??(ls_in_arp_rsp  ), priority=0, match=(1), action=(next;)
> +])
> +
> +#Set the disable_arp_nd_rsp option and verify the flow
> +ovn-nbctl --wait=sb set logical_switch_port S1-vm1
> options:disable_arp_nd_rsp=true
> +
> +ovn-sbctl dump-flows S1 > S1flows
> +AT_CAPTURE_FILE([S1flows])
> +
> +AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | sed 's/table=../table=??/'],
> [0], [dnl
> +  table=??(ls_in_arp_rsp  ), priority=0, match=(1), action=(next;)
> +])
> +
>  AT_CLEANUP
>  ])
> --
> 2.36.6
>
> ___
> dev mailing list
> d...@openvswitch.org
> 

Re: [ovs-dev] [PATCH ovn 1/2] ovs: Bump submodule to include igmp protocol version.

[Ilya Maximets]

On 1/23/24 14:53, Mohammad Heib wrote:
> Hi Dumitru,
> 
> Thank you for the review,
> yes sure will bump the submodule to the last stable once they fix the issue.

I applied the BFD fix and also applied the typedef fix for the mcast_group_proto
enum.  So, make sure to include both for the update.

There are a few other things though.

This patch needs to upgrae the DPDk version, since OVS 3.3 will be using DPDK 
23.11
and branch-3.3 is supposed to be used with this version.  22.11 is not 
supported on
that branch.  So, you need to port CI changes from:
  
https://github.com/openvswitch/ovs/commit/8893e24d9d0921aaf934f263a06ba223ef0db369

See one more comment inline.

> Thanks
> 
> On Mon, Jan 22, 2024 at 5:01 PM Dumitru Ceara  wrote:
> 
>> On 1/22/24 15:14, Mohammad Heib wrote:
>>> Specifically the following commit:
>>>   077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.")
>>>
>>> Also fix a small compilation error due to prototype change.
>>>
>>> Signed-off-by: Mohammad Heib 
>>> ---
>>
>> Hi Mohammad,
>>
>> Thanks for the patch!
>>
>>>  controller/pinctrl.c | 6 +-
>>>  ovs  | 2 +-
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>>> index 4992eab08..77bf67e58 100644
>>> --- a/controller/pinctrl.c
>>> +++ b/controller/pinctrl.c
>>> @@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn,
>>>  switch (ntohs(ip_flow->tp_src)) {
>>>  case IGMP_HOST_MEMBERSHIP_REPORT:
>>>  case IGMPV2_HOST_MEMBERSHIP_REPORT:
>>> +mcast_group_proto grp_proto =

You're defining a variable inside the case, the whole case should be
braced for this to work.  It may work with some compilers, but it is
generally incorrect and CI fails to build because of this.
Alternatively, move the declaration outside of the switch statement.

>>> +(ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT)
>>> +? MCAST_GROUP_IGMPV1
>>> +: MCAST_GROUP_IGMPV2;
>>>  group_change =
>>>  mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN,
>>> -  port_key_data);
>>> +  port_key_data, grp_proto);
>>>  break;
>>>  case IGMP_HOST_LEAVE_MESSAGE:
>>>  group_change =
>>> diff --git a/ovs b/ovs
>>> index 4102674b3..b222593bc 16
>>> --- a/ovs
>>> +++ b/ovs
>>> @@ -1 +1 @@
>>> -Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6
>>> +Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3
>>
>> However, it's probably desirable to bump the submodule to the tip of the
>> latest stable branch, i.e. branch-3.3:
>>
>>
>> https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases
>>
>> That would also fix our scheduled CI runs:
>>
>> https://github.com/ovn-org/ovn/actions/runs/7597582431/job/20692570222#step:10:3531
>>
>> However, there's a crash in OVS on branch-3.3 that needs to be fixed first:
>> https://issues.redhat.com/browse/FDP-300
>>
>> I'd wait with bumping the submodule until then.
>>
>> Regards,
>> Dumitru

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

Re: [ovs-dev] [PATCH] mcast-snooping: Remove typedef from mcast_group_proto.

[Ilya Maximets]

On 1/28/24 11:20, Mohammad Heib wrote:
> Thanks, Ilya,
> Looks good to me.
> 
> Acked-by: Mohammad Heib mailto:mh...@redhat.com>>
> 

Thanks!  I applied the change and backported to branch-3.3.

Best regards, Ilya Maximets.

> Thanks,
> 
> 
> 
> On Fri, Jan 26, 2024 at 7:11 PM Ilya Maximets  > wrote:
> 
> Typedefs are confusing and the coding style generally advises to not
> use them.  Removing typedef until others start using it.
> 
> This typedef already got me while testing an OVN update to use OVS 3.3
> as a submodule, since the variable was declared in a switch statement
> and it wasn't clearly visible that there is a variable definition in
> one of the cases and braces should be used.  Strangely some versions
> of compilers do not require braces in this case, so OVN change works
> locally, but not in CI.
> 
> Fixes: 077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.")
> Signed-off-by: Ilya Maximets  >
> ---
>  lib/mcast-snooping.c         |  6 +++---
>  lib/mcast-snooping.h         | 12 ++--
>  ofproto/ofproto-dpif-xlate.c |  2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> index 60ef8381e..dc5164b41 100644
> --- a/lib/mcast-snooping.c
> +++ b/lib/mcast-snooping.c
> @@ -58,7 +58,7 @@ mcast_snooping_flood_unreg(const struct mcast_snooping 
> *ms)
>  }
> 
>  char *
> -mcast_snooping_group_protocol_str(mcast_group_proto grp_proto)
> +mcast_snooping_group_protocol_str(enum mcast_group_proto grp_proto)
>  {
>      switch (grp_proto) {
>      case MCAST_GROUP_IGMPV1:
> @@ -414,7 +414,7 @@ bool
>  mcast_snooping_add_group(struct mcast_snooping *ms,
>                           const struct in6_addr *addr,
>                           uint16_t vlan, void *port,
> -                         mcast_group_proto grp_proto)
> +                         enum mcast_group_proto grp_proto)
>      OVS_REQ_WRLOCK(ms->rwlock)
>  {
>      bool learned;
> @@ -460,7 +460,7 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
>  bool
>  mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
>                           uint16_t vlan, void *port,
> -                         mcast_group_proto grp_proto)
> +                         enum mcast_group_proto grp_proto)
>      OVS_REQ_WRLOCK(ms->rwlock)
>  {
>      struct in6_addr addr = in6_addr_mapped_ipv4(ip4);
> diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
> index 76ab4e4f7..de42cf826 100644
> --- a/lib/mcast-snooping.h
> +++ b/lib/mcast-snooping.h
> @@ -40,13 +40,13 @@ struct mcast_snooping;
>  #define MCAST_MROUTER_PORT_IDLE_TIME 180
> 
>  /* Multicast group protocol. */
> -typedef enum {
> +enum mcast_group_proto {
>      MCAST_GROUP_IGMPV1 = 0,
>      MCAST_GROUP_IGMPV2,
>      MCAST_GROUP_IGMPV3,
>      MCAST_GROUP_MLDV1,
>      MCAST_GROUP_MLDV2,
> -} mcast_group_proto;
> +};
> 
>  /* Multicast group entry.
>   * Guarded by owning 'mcast_snooping''s rwlock. */
> @@ -61,7 +61,7 @@ struct mcast_group {
>      uint16_t vlan;
> 
>      /* Multicast group IPv6/IPv4 Protocol version IGMPv1,2,3 or MLDv1,2 
> */
> -    mcast_group_proto protocol_version;
> +    enum mcast_group_proto protocol_version;
> 
>      /* Node in parent struct mcast_snooping group_lru. */
>      struct ovs_list group_node OVS_GUARDED;
> @@ -198,11 +198,11 @@ mcast_snooping_lookup4(const struct mcast_snooping 
> *ms, ovs_be32 ip4,
>  bool mcast_snooping_add_group(struct mcast_snooping *ms,
>                                const struct in6_addr *addr,
>                                uint16_t vlan, void *port,
> -                              mcast_group_proto grp_proto)
> +                              enum mcast_group_proto grp_proto)
>      OVS_REQ_WRLOCK(ms->rwlock);
>  bool mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
>                                 uint16_t vlan, void *port,
> -                               mcast_group_proto grp_proto)
> +                               enum mcast_group_proto grp_proto)
>      OVS_REQ_WRLOCK(ms->rwlock);
>  int mcast_snooping_add_report(struct mcast_snooping *ms,
>                                const struct dp_packet *p,
> @@ -224,7 +224,7 @@ bool mcast_snooping_add_mrouter(struct mcast_snooping 
> *ms, uint16_t vlan,
>      OVS_REQ_WRLOCK(ms->rwlock);
>  bool mcast_snooping_is_query(ovs_be16 igmp_type);
>  bool mcast_snooping_is_membership(ovs_be16 igmp_type);
> -char *mcast_snooping_group_protocol_str(mcast_group_proto grp_proto);
> +char *mcast_snooping_group_protocol_str(enum mcast_group_proto 
> grp_proto);
> 
>  

Re: [ovs-dev] [PATCH ovn] actions: Use random port selection for SNAT with external_port_range.

[Xavier Simonart]

Hi Dumitru

Thanks for the patch.

On Tue, Jan 23, 2024 at 10:44 PM Mark Michelson  wrote:

> Hi Dumitru,
>
> Acked-by: Mark Michelson 
>
> I think this change is good. I looked through the documentation for the
> "external_port_range" column in the NAT table. This change appears to
> actually make the documentation *more* accurate rather than to introduce
> any potentially undesirable behavior changes.
>
> What I find more telling is that the only tests you had to update were
> the actions tests. We apparently don't have any system tests that use
> NAT with an external port range. That's not great :(
>
> On 1/22/24 09:54, Dumitru Ceara wrote:
> > This is to avoid unexpected behavior changes due to the underlying
> > datapath (e.g., kernel) changing defaults.  If we don't explicitly
> > request a port selection algorithm, OVS leaves it up to the
> > datapath to decide how to do the port selection.  Currently that means
> > that source port allocation is not random if the original source port
> > fits in the requested range.
>

This looks good to me.
In most cases, a source port within range was not translated w/ existing
behavior

ovn-nbctl --portrange lr-nat-add rtr snat 66.66.66.66 42.42.42.0/24 1-2

43.43.43.2:15000 -> 42.42.42.2:8080 => 66.66.66.66:15000 ->
42.42.42.2:80 # no PAT as within range

But it was still translated in some cases:

ovn-nbctl --portrange lr-nat-add rtr snat 66.66.66.66 42.42.42.0/24 1-2

43.43.43.2:4 -> 42.42.42.2:8080 => 66.66.66.66:15000 ->
42.42.42.2:80 # PAT. dst port happens to be 15000

43.43.43.2:15000 -> 42.42.42.2:8080 => 66.66.66.66:XXX ->
42.42.42.2:80 # PAT to XXX != 15000 as 15000 already used.

In other words, I do not see how one could rely on existing (pre-patch)
behavior (no PAT if the source port is within the range)
as it is not guaranteed.


>

> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2024-January/410847.html
> > Reported-at: https://issues.redhat.com/browse/FDP-301
> > Fixes: 60bdc8ea78d7 ("NAT: Provide port range in input")
> > Signed-off-by: Dumitru Ceara 
> > ---
> >   lib/actions.c | 16 ++--
> >   tests/ovn.at  |  8 
> >   2 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 38cf4642d6..fdc0529de6 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -1131,8 +1131,20 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
> >   }
> >
> >   if (cn->port_range.exists) {
> > -   nat->range.proto.min = cn->port_range.port_lo;
> > -   nat->range.proto.max = cn->port_range.port_hi;
> > +nat->range.proto.min = cn->port_range.port_lo;
> > +nat->range.proto.max = cn->port_range.port_hi;
> > +
> > +/* Explicitly set the port selection algorithm to "random".
> Otherwise
> > + * it's up to the datapath to choose how to select the port and
> that
> > + * might create unexpected behavior changes when the datapath
> defaults
> > + * change.
> > + *
> > + * NOTE: for the userspace datapath the "random" function
> doesn't
> > + * really generate random ports, it uses "hash" under the hood:
> > + * https://issues.redhat.com/browse/FDP-269. */
> > +if (nat->range.proto.min && nat->range.proto.max) {
> > +nat->flags |= NX_NAT_F_PROTO_RANDOM;
> > +}
> >   }
> >
> >   ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 8cc4c311c1..e8cef83cb5 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1368,7 +1368,7 @@ ct_dnat(fd11::2);
> >   has prereqs ip
> >   ct_dnat(192.168.1.2, 1-3000);
> >   formats as ct_dnat(192.168.1.2,1-3000);
> > -encodes as
> ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000))
> > +encodes as
> ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1
> -3000,random))
> >   has prereqs ip
> >
> >   ct_dnat(192.168.1.2, 192.168.1.3);
> > @@ -1402,7 +1402,7 @@ ct_dnat_in_czone(fd11::2);
> >   has prereqs ip
> >   ct_dnat_in_czone(192.168.1.2, 1-3000);
> >   formats as ct_dnat_in_czone(192.168.1.2,1-3000);
> > -encodes as
> ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000))
> > +encodes as
> ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1
> -3000,random))
> >   has prereqs ip
> >
> >   ct_dnat_in_czone(192.168.1.2, 192.168.1.3);
> > @@ -1436,7 +1436,7 @@ ct_snat(fd11::2);
> >   has prereqs ip
> >   ct_snat(192.168.1.2, 1-3000);
> >   formats as ct_snat(192.168.1.2,1-3000);
> > -encodes as
> ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1-3000))
> > +encodes as
> ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1
> -3000,random))
> >   has prereqs ip
> >
> >   ct_snat(192.168.1.2, 192.168.1.3);
> > @@ -1470,7 +1470,7 @@ ct_snat_in_czone(fd11::2);
> >   has prereqs ip
> >   

[ovs-dev] [PATCH v2] utilities: Add TASK_STOPPED accounting to the kernel_delay.py script.

[Eelco Chaudron]

This changes add statistics for when a thread is put into stop state.
For example with the following:

kill -STOP $(pidof ovs-vswitchd); sleep 1; kill -CONT $(pidof ovs-vswitchd);

Acked-by: Simon Horman 
Signed-off-by: Eelco Chaudron 
---
 v2: - Removed worst/max ns delay zero checks.

 utilities/usdt-scripts/kernel_delay.py  | 110 ++--
 utilities/usdt-scripts/kernel_delay.rst |  24 ++
 2 files changed, 110 insertions(+), 24 deletions(-)

diff --git a/utilities/usdt-scripts/kernel_delay.py 
b/utilities/usdt-scripts/kernel_delay.py
index b2012fdf2..de6b0c9de 100755
--- a/utilities/usdt-scripts/kernel_delay.py
+++ b/utilities/usdt-scripts/kernel_delay.py
@@ -81,7 +81,6 @@ struct event_t {
 
 u32 syscall;
 u64 entry_ts;
-
 };
 
 BPF_RINGBUF_OUTPUT(events, );
@@ -220,7 +219,7 @@ TRACEPOINT_PROBE(raw_syscalls, sys_exit) {
 u64 delta = bpf_ktime_get_ns() - *start_ns;
 val->count++;
 val->total_ns += delta;
-if (val->worst_ns == 0 || delta > val->worst_ns)
+if (delta > val->worst_ns)
 val->worst_ns = delta;
 
 if () {
@@ -243,13 +242,12 @@ TRACEPOINT_PROBE(raw_syscalls, sys_exit) {
 
 
 /*
- * For measuring the thread run time, we need the following.
+ * For measuring the thread stopped time, we need the following.
  */
-struct run_time_data_t {
+struct stop_time_data_t {
 u64 count;
 u64 total_ns;
-u64 max_ns;
-u64 min_ns;
+u64 worst_ns;
 };
 
 struct pid_tid_key_t {
@@ -257,6 +255,43 @@ struct pid_tid_key_t {
 u32  tid;
 };
 
+BPF_HASH(stop_start, u64, u64);
+BPF_HASH(stop_data, struct pid_tid_key_t, struct stop_time_data_t);
+
+static inline void thread_handle_stopped_run(u32 pid, u32 tgid, u64 ktime)
+{
+u64 pid_tgid = (u64) tgid << 32 | pid;
+u64 *start_ns = stop_start.lookup(_tgid);
+
+if (!start_ns || *start_ns == 0)
+return;
+
+struct stop_time_data_t *val, zero = {};
+struct pid_tid_key_t key = { .pid = tgid,
+ .tid = pid };
+
+val = stop_data.lookup_or_try_init(, );
+if (val) {
+u64 delta = ktime - *start_ns;
+val->count++;
+val->total_ns += delta;
+if (delta > val->worst_ns)
+val->worst_ns = delta;
+}
+*start_ns = 0;
+}
+
+
+/*
+ * For measuring the thread run time, we need the following.
+ */
+struct run_time_data_t {
+u64 count;
+u64 total_ns;
+u64 max_ns;
+u64 min_ns;
+};
+
 BPF_HASH(run_start, u64, u64);
 BPF_HASH(run_data, struct pid_tid_key_t, struct run_time_data_t);
 
@@ -282,7 +317,7 @@ static inline void thread_stop_run(u32 pid, u32 tgid, u64 
ktime)
 u64 delta = ktime - *start_ns;
 val->count++;
 val->total_ns += delta;
-if (val->max_ns == 0 || delta > val->max_ns)
+if (delta > val->max_ns)
 val->max_ns = delta;
 if (val->min_ns == 0 || delta < val->min_ns)
 val->min_ns = delta;
@@ -312,6 +347,8 @@ static inline int sched_wakeup__(u32 pid, u32 tgid)
 
 u64 t = bpf_ktime_get_ns();
 ready_start.update(_tgid, );
+
+thread_handle_stopped_run(pid, tgid, t);
 return 0;
 }
 
@@ -336,22 +373,26 @@ RAW_TRACEPOINT_PROBE(sched_switch)
 if (!capture_enabled__())
 return 0;
 
-if (prev-> == TASK_RUNNING && prev->tgid == MONITOR_PID)
-sched_wakeup__(prev->pid, prev->tgid);
-
 if (prev->tgid == MONITOR_PID) {
+u64 prev_pid_tgid = (u64)next->tgid << 32 | next->pid;
 ktime = bpf_ktime_get_ns();
+
+if (prev-> == TASK_RUNNING)
+ready_start.update(_pid_tgid, );
+
+if (prev-> & __TASK_STOPPED)
+stop_start.update(_pid_tgid, );
+
 thread_stop_run(prev->pid, prev->tgid, ktime);
 }
 
-u64 pid_tgid = (u64)next->tgid << 32 | next->pid;
-
 if (next->tgid != MONITOR_PID)
 return 0;
 
 if (ktime == 0)
 ktime = bpf_ktime_get_ns();
 
+u64 pid_tgid = (u64)next->tgid << 32 | next->pid;
 u64 *start_ns = ready_start.lookup(_tgid);
 
 if (start_ns && *start_ns != 0) {
@@ -365,7 +406,7 @@ RAW_TRACEPOINT_PROBE(sched_switch)
 u64 delta = ktime - *start_ns;
 val->count++;
 val->total_ns += delta;
-if (val->worst_ns == 0 || delta > val->worst_ns)
+if (delta > val->worst_ns)
 val->worst_ns = delta;
 }
 *start_ns = 0;
@@ -438,7 +479,7 @@ TRACEPOINT_PROBE(irq, irq_handler_exit)
 u64 delta = bpf_ktime_get_ns() - data->start_ns;
 val->count++;
 val->total_ns += delta;
-if (val->worst_ns == 0 || delta > val->worst_ns)
+if (delta > val->worst_ns)
 val->worst_ns = delta;
 }
 }
@@ -508,7 +549,7 @@ TRACEPOINT_PROBE(irq, softirq_exit)
 u64 delta = bpf_ktime_get_ns() - data->start_ns;
 val->count++;
 val->total_ns += delta;
-if (val->worst_ns == 0 || delta > 

[ovs-dev] [PATCH ovs v2] Documentation: Adding note about using the jemalloc library.

[Roberto Bartzen Acosta via dev]

Updating the reference documentation with the inclusion of possible building 
problems with libjemalloc and solution suggestions.

Reported-at: https://bugs.launchpad.net/ubuntu/+source/openvswitch/+bug/2015748
Signed-off-by: Roberto Bartzen Acosta 
---
 Documentation/intro/install/general.rst | 16 
 1 file changed, 16 insertions(+)

diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 19e360d47..e2eb19510 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -344,6 +344,22 @@ you wish to link with jemalloc add it to LIBS::
 
 $ ./configure LIBS=-ljemalloc
 
+.. note::
+  Linking Open vSwitch with the jemalloc shared library may not work as
+  expected in certain operating system development environments. You can
+  override the automatic compiler decision to avoid possible linker issues by
+  passing ``-fno-lto`` or disabling ``-fno-builtin`` flag since the jemalloc
+  override standard built-in memory allocation functions such as malloc,
+  calloc, etc. Both options can solve possible jemalloc linker issues with pros
+  and cons for each case, feel free to choose the path that appears best to
+  you. Disabling LTO flag example::
+
+  $ ./configure LIBS=-ljemalloc CFLAGS=-fno-lto
+
+  Disabling built-in flag example::
+
+  ./configure LIBS=-ljemalloc CFLAGS=-fno-builtin
+
 .. _general-building:
 
 Building
-- 
2.25.1


-- 




_'Esta mensagem é direcionada apenas para os endereços constantes no 
cabeçalho inicial. Se você não está listado nos endereços constantes no 
cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa 
mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão 
imediatamente anuladas e proibidas'._


* **'Apesar do Magazine Luiza tomar 
todas as precauções razoáveis para assegurar que nenhum vírus esteja 
presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por 
quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.*



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

Re: [ovs-dev] [PATCH ovn 3/3] ovn-controller: Support VIF-based local encap IPs selection.

[Ales Musil]

On Fri, Jan 26, 2024 at 8:05 PM Han Zhou  wrote:

>
>
> On Thu, Jan 25, 2024 at 10:54 PM Ales Musil  wrote:
> >
> >
> >
> > On Fri, Jan 26, 2024 at 4:07 AM Han Zhou  wrote:
> >>
> >>
> >>
> >> On Tue, Jan 23, 2024 at 5:29 AM Ales Musil  wrote:
> >> >
> >> >
> >> >
> >> > On Wed, Jan 17, 2024 at 6:48 AM Han Zhou  wrote:
> >> >>
> >> >> Commit dd527a283cd8 partially supported multiple encap IPs. It
> supported
> >> >> remote encap IP selection based on the destination VIF's encap_ip
> >> >> configuration. This patch adds the support for selecting local encap
> IP
> >> >> based on the source VIF's encap_ip configuration.
> >> >>
> >> >> Co-authored-by: Lei Huang 
> >> >> Signed-off-by: Lei Huang 
> >> >> Signed-off-by: Han Zhou 
> >> >> ---
> >> >
> >> >
> >> > Hi Han and Lei,
> >> >
> >> > thank you for the patch, I have a couple of comments/questions down
> below.
> >>
> >>
> >> Thanks Ales.
> >>
> >> >
> >> >
> >> >>  NEWS|   3 +
> >> >>  controller/chassis.c|   2 +-
> >> >>  controller/local_data.c |   2 +-
> >> >>  controller/local_data.h |   2 +-
> >> >>  controller/ovn-controller.8.xml |  30 ++-
> >> >>  controller/ovn-controller.c |  49 
> >> >>  controller/physical.c   | 134
> ++--
> >> >>  controller/physical.h   |   2 +
> >> >>  include/ovn/logical-fields.h|   4 +-
> >> >>  ovn-architecture.7.xml  |  18 -
> >> >>  tests/ovn.at|  51 +++-
> >> >>  11 files changed, 243 insertions(+), 54 deletions(-)
> >> >>
> >> >> diff --git a/NEWS b/NEWS
> >> >> index 5f267b4c64cc..5a3eed608617 100644
> >> >> --- a/NEWS
> >> >> +++ b/NEWS
> >> >> @@ -14,6 +14,9 @@ Post v23.09.0
> >> >>- ovn-northd-ddlog has been removed.
> >> >>- A new LSP option "enable_router_port_acl" has been added to
> enable
> >> >>  conntrack for the router port whose peer is l3dgw_port if set
> it true.
> >> >> +  - Support selecting encapsulation IP based on the
> source/destination VIF's
> >> >> +settting. See ovn-controller(8) 'external_ids:ovn-encap-ip' for
> more
> >> >> +details.
> >> >>
> >> >>  OVN v23.09.0 - 15 Sep 2023
> >> >>  --
> >> >> diff --git a/controller/chassis.c b/controller/chassis.c
> >> >> index a6f13ccc42d5..55f2beb37674 100644
> >> >> --- a/controller/chassis.c
> >> >> +++ b/controller/chassis.c
> >> >> @@ -61,7 +61,7 @@ struct ovs_chassis_cfg {
> >> >>
> >> >>  /* Set of encap types parsed from the 'ovn-encap-type'
> external-id. */
> >> >>  struct sset encap_type_set;
> >> >> -/* Set of encap IPs parsed from the 'ovn-encap-type'
> external-id. */
> >> >> +/* Set of encap IPs parsed from the 'ovn-encap-ip' external-id.
> */
> >> >>  struct sset encap_ip_set;
> >> >>  /* Interface type list formatted in the OVN-SB Chassis required
> format. */
> >> >>  struct ds iface_types;
> >> >> diff --git a/controller/local_data.c b/controller/local_data.c
> >> >> index a9092783958f..8606414f8728 100644
> >> >> --- a/controller/local_data.c
> >> >> +++ b/controller/local_data.c
> >> >> @@ -514,7 +514,7 @@ chassis_tunnels_destroy(struct hmap
> *chassis_tunnels)
> >> >>   */
> >> >>  struct chassis_tunnel *
> >> >>  chassis_tunnel_find(const struct hmap *chassis_tunnels, const char
> *chassis_id,
> >> >> -char *remote_encap_ip, char *local_encap_ip)
> >> >> +char *remote_encap_ip, const char
> *local_encap_ip)
> >> >
> >> >
> >> > nit: Unrelated change.
> >>
> >>
> >> Ack
>
> Hi Ales, sorry that I just realized this change is related, which is
> because of the const char * array introduced in this patch that stores the
> parsed encap_ips, and it makes sense to use const char * since we should
> never expect this string to be modified in the function.
>
> >>
> >> >
> >> >
> >> >>  {
> >> >>  /*
> >> >>   * If the specific encap_ip is given, look for the chassisid_ip
> entry,
> >> >> diff --git a/controller/local_data.h b/controller/local_data.h
> >> >> index bab95bcc3824..ca3905bd20e6 100644
> >> >> --- a/controller/local_data.h
> >> >> +++ b/controller/local_data.h
> >> >> @@ -150,7 +150,7 @@ bool local_nonvif_data_handle_ovs_iface_changes(
> >> >>  struct chassis_tunnel *chassis_tunnel_find(const struct hmap
> *chassis_tunnels,
> >> >> const char *chassis_id,
> >> >> char *remote_encap_ip,
> >> >> -   char *local_encap_ip);
> >> >> +   const char
> *local_encap_ip);
> >> >
> >> >
> >> > Same as above.
> >>
> >>
> >> Ack
> >>
> >> >
> >> >
> >> >>
> >> >>  bool get_chassis_tunnel_ofport(const struct hmap *chassis_tunnels,
> >> >> const char *chassis_name,
> >> >> diff --git a/controller/ovn-controller.8.xml
> b/controller/ovn-controller.8.xml
> >> >> 

Re: [ovs-dev] [PATCH ovn v3 2/2] northd: Explicitly handle SNAT for ICMP need frag.

[Dumitru Ceara]

On 1/29/24 07:20, Ales Musil wrote:
> Considering following topology:
> client - sw0 - lrp0 - lr - lrp1 - sw1 - server
> sw0 in subnet 192.168.0.0/24
> sw1 in subnet 172.168.0.0/24
> SNAT configured for client
> gateway_mtu=1400 configured for lrp0
> 
> If we send UDP traffic from client to server
> and server responds with packet bigger than 1400
> the following sequence will happen:
> 
> 1) Packet is coming into lr via lrp1
> 2) unSNAT
> 3) Routing, the outport will be set to lrp0
> 4) Check for packet larger will fail
> 5) We will generate ICMP need frag
> 
> However, the last step is wrong from the server
> perspective. The ICMP message will have IP source
> address = lrp1 IP address. Which means that SNAT won't
> happen because the source is not within the sw0 subnet,
> but the inner packet has sw0 subnet address, because it
> was unSNATted. This results in server ignoring the ICMP
> message because server never sent any packet to the
> sw0 subnet.
> 
> In order to prevent this issue perform SNAT for the
> ICMP packet. Because the packet is related to already
> existing connection we just need to perform
> ct_commit_nat(snat) action.
> 
> This is achieved with addition of the following flow for
> "lr_in_larger_pkts" stage (the flow for IPv6 is the in
> regard to the addition):
> 
> match=(inport == "INPORT" && outport == "OUTPORT" && ip4 && REGBIT_PKT_LARGER 
> && REGBIT_EGRESS_LOOPBACK == 0 && ct.trk && ct.rpl && ct.dnat), 
> action=(icmp4_error {flags.icmp_snat = 1; REGBIT_EGRESS_LOOPBACK = 1; 
> REGBIT_PKT_LARGER = 0; eth.dst = ETH_DST; ip4.dst = ip4.src; ip4.src = 
> IP_SRC; ip.ttl = 255; icmp4.type = 3; /* Destination Unreachable. */ 
> icmp4.code = 4; /* Frag Needed and DF was Set. */ icmp4.frag_mtu = 1500; 
> next(pipeline=ingress, table=0); };)
> 
> Also, add flow to "lr_out_post_snat" stage:
> 
> match=(icmp && flags.icmp_snat == 1), action=(ct_commit_nat(snat);)
> 
> Partially revert 0e49f49c73d6 ("northd: Allow need frag to be SNATed")
> which attempted to fix the same issue in a wrong way.
> 
> Also add feature flag for the updated ct_commit_nat action.
> In case there is an update of northd to newer version before all
> controllers are updated.
> 
> Fixes: 0e49f49c73d6 ("northd: Allow need frag to be SNATed")
> Reported-at: https://issues.redhat.com/browse/FDP-134
> Reported-at: https://issues.redhat.com/browse/FDP-159
> Signed-off-by: Ales Musil 
> Acked-by: Dumitru Ceara 
> ---
> v3: Rebase on top of current main.
> v2: Rebase on top of current main.
> Squash the 2/3 and 3/3 from previous version to single commit.
> Add ack from Dumitru.
> ---

Hi Ales,

Before accepting this patch I'd like to try to clarify one thing that
was flagged as a potential issue by Numan (ct.dnat), please see below.

>  controller/chassis.c |   8 ++
>  include/ovn/features.h   |   1 +
>  include/ovn/logical-fields.h |   3 +
>  lib/logical-fields.c |   4 +
>  northd/northd.c  | 192 ---
>  northd/northd.h  |   1 +
>  tests/ovn-northd.at  | 118 ++---
>  tests/ovn.at |   6 +-
>  tests/system-ovn-kmod.at |   3 +-
>  9 files changed, 214 insertions(+), 122 deletions(-)
> 
> diff --git a/controller/chassis.c b/controller/chassis.c
> index a6f13ccc4..ba2e57238 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -370,6 +370,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg 
> *ovs_cfg,
>  smap_replace(config, OVN_FEATURE_CT_LB_RELATED, "true");
>  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");
>  }
>  
>  /*
> @@ -509,6 +510,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_NAT_V2,
> +   false)) {
> +return true;
> +}
> +
>  return false;
>  }
>  
> @@ -640,6 +647,7 @@ update_supported_sset(struct sset *supported)
>  sset_add(supported, OVN_FEATURE_CT_LB_RELATED);
>  sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
>  sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
> +sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
>  }
>  
>  static void
> diff --git a/include/ovn/features.h b/include/ovn/features.h
> index 2c47ab766..08f1d8288 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -27,6 +27,7 @@
>  #define OVN_FEATURE_CT_LB_RELATED "ovn-ct-lb-related"
>  #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp"
>  #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column"
> +#define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2"
>  
>  /* OVS datapath supported features.  Based on availability OVN might generate
>   * different types of openflows.
> diff --git a/include/ovn/logical-fields.h