Re: [ovs-dev] [RFC v2 ovn 0/4] rework OVN QoS implementation

[Ihar Hrachyshka]

On Tue, Apr 4, 2023 at 12:57 PM Ihar Hrachyshka  wrote:
>
> Hi,
>
> thanks for v2. I will leave general comments to the series here, and I
> will post some specific comments to corresponding patches where
> appropriate.
>
> --
>
> The series claims that VIF support for QoS was not present before.
> ("add QoS support for logical switch port interfaces" etc.) This is
> not correct. QoS min-rate filters are applied to LSP by Neutron. Then
> - as intended - ovn-controller creates corresponding qdisc queues on
> egress interfaces that control bandwidth via HTB. I confirm LSP
> settings are working here: https://paste.centos.org/view/4eedac05 (you
> can apply it to main; it passes).

Looks like the link expired. I've put the patch that demonstrates LSP
qos_ options in action here for your reference:
https://github.com/booxter/ovn/commit/bd15173a0753371e59871cc96b63b3dedcc70092

>
> The series, for LSP ports, switches from setting queues on egress
> interfaces to setting them on TAP interfaces. This is wrong. I think
> Rodolfo has a more detailed explanation captured in document, (I hope
> he can share it here shortly) but the gist is that physical interfaces
> are hardware and provide the actual bandwidth, and that's where HTB
> algo should run to shape traffic aggregated from all.
>
> On Fri, Mar 31, 2023 at 6:40 AM Lorenzo Bianconi
>  wrote:
> >
> > Rework OVN QoS implementation in order to configure it through OVS QoS
> > table instead of running tc command directly bypassing OVS.
> >
> > Changes since v1:
> > - get rid of qos_ovs_port from logical_switch_port option column and let 
> > ovn to
> >   compute it
> > - add VIF QoS support
> > - take into account qos_min_rate in port_has_qos_params
> >
> > Lorenzo Bianconi (4):
> >   controller: configure qos through ovs qos table and do not run tc
> > directly
> >   controller: improve ovs port lookup by name and qos
> >   controller: add QoS support for logical switch port interfaces
> >   northd: take into account qos_min_rate in port_has_qos_params
> >
> >  controller/binding.c| 429 +---
> >  controller/binding.h|   5 +-
> >  controller/ovn-controller.c |  35 ++-
> >  controller/ovsport.c|  32 +++
> >  controller/ovsport.h|   5 +
> >  northd/northd.c |   2 +-
> >  tests/system-ovn.at | 101 -
> >  7 files changed, 367 insertions(+), 242 deletions(-)
> >
> > --
> > 2.39.2
> >

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

Re: [ovs-dev] [PATCH ovn v4 0/2] expr: Optimize OR expressions.

[Ilya Maximets]

On 4/5/23 21:48, Ilya Maximets wrote:
> This patch set covers removal of expressions which are subsets of other
> wider expressions.  This allows to avoid flow explosion in case of
> negative matches.  More details are in commit messages.
> 
> Version 2:
>   * Became a patch set.
>   * Added tests and missing bitmap.h include.
>   * Code switched to work with bitwise maskable fields only (ORDINAL).
>   * Added a new patch to combine smaller expressions into wider ones.
>   * Added a patch to fix a crash uncovered with expression aggregation.
> 
> Version 3:
>   * Dropped patch 3 for performance reasons for now, because it doesn't
> allow to make use of I-P in many cases.
>   * Patch 1 re-worked to not cause performance issues for normal
> address sets generated in OVN.
>   * Performance of the patch 1 significantly improved by not perfroming
> a full n^2 search and not comparing huge empty parts of subvalues.
> The patch became a bit less straightforward, but I hope it's still
> fairly readable.
> 
> Version 4:
>   * Added extra comments.
>   * Added ACK from Han to patch 2.
>   * Re-worked path shortening (next[]) to track the last non-NULL entry.
>   * Restricted superset optmization to expressions that do not track
> address sets.  To preserve ability to use I-P. [Han]
Forgot to add:

* Fixed the memset value: s/0xf/0xff/.  [Han]

> 
> Ilya Maximets (2):
>   expr: Remove supersets from OR expressions.
>   expr: Avoid crash if all sub-expressions crushed down to 'true'.
> 
>  include/ovn/expr.h |   1 +
>  lib/expr.c | 255 +++--
>  tests/ovn.at   |  12 +++
>  3 files changed, 212 insertions(+), 56 deletions(-)
> 

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

[ovs-dev] [PATCH ovn v4 1/2] expr: Remove supersets from OR expressions.

[Ilya Maximets]

While crushing OR expressions, OVN removes exact replicas of sub
expressions.  However, there could be many CMP expressions that are
supersets of each other.  These are most likely to be created as a
result of cross-product while expanding brackets in the AND expression
in crush_and_numeric(), i.e. while converting
"x && (a0 || a1) && (b0 || b1)" into "xa0b0 || xa0b1 || xa1b0 || xa1b1".

In addition to removal of exact duplicates introducing scan and removal
of supersets of other existing sub-expressions to reduce the amount of
generated flows.  This adds extra computations, but should save time
later, since less flows will be generated.

Example:

  "ip4.src == 172.168.0.0/16 && ip4.src!={172.168.13.0/24, 172.168.15.0/24}"

  Processing of this expression yields 42 flows:

  $ ./tests/ovstest test-ovn expr-to-flows <<< "$expr"

  ip,nw_src=172.168.0.0/255.255.1.0
  ip,nw_src=172.168.0.0/255.255.10.0
  ip,nw_src=172.168.0.0/255.255.12.0
  ip,nw_src=172.168.0.0/255.255.3.0
  ip,nw_src=172.168.0.0/255.255.4.0
  ip,nw_src=172.168.0.0/255.255.5.0
  ip,nw_src=172.168.0.0/255.255.6.0
  ip,nw_src=172.168.0.0/255.255.8.0
  ip,nw_src=172.168.0.0/255.255.9.0
  ip,nw_src=172.168.128.0/17
  <... 32 more flows ...>

  We can see that many flows above do overlap, e.g. 255.255.3.0
  mask is a superset of 255.255.1.0.  Everything that matches
  255.255.3.0, will match 255.255.1.0 as well (the value is the same).

  By removing all the unnecessary supersets, the set of flows can
  be reduced from 42 down to 7:

  ip,nw_src=172.168.0.0/255.255.1.0
  ip,nw_src=172.168.0.0/255.255.4.0
  ip,nw_src=172.168.0.0/255.255.8.0
  ip,nw_src=172.168.128.0/17
  ip,nw_src=172.168.16.0/255.255.16.0
  ip,nw_src=172.168.32.0/255.255.32.0
  ip,nw_src=172.168.64.0/255.255.64.0

This change should be particularly useful for expressions with
inequality checks, like the one above.  Such expressions are
frequent among ACL rules.

  "ip4.src != {172.168.13.0/24, 172.168.14.0/24, 172.168.15.0/24}"

  Brefore:
  $ ./tests/ovstest test-ovn expr-to-flows <<< "$expr" | wc -l
  2894

  After:
  $ ./tests/ovstest test-ovn expr-to-flows <<< "$expr" | wc -l
  23

Superset lookups are performed only if there are expressions with
more bits in the mask than in the current one.  So, there is no extra
cost for equality checks on normal address sets, like port group sets,
where all the IPs are exect matches or have the same prefix length
otherwise.

Also, the superset optimization is not performed if expression is
tracking an address set.  This is done in order to preserve ability to
use address set I-P even if the user specified overlapping addresses
and CIDRs.  Note: having exact duplicates in user-specified sets is
highly unlikely, because of database constraints.

Use of bitmaps instead of subvalue functions significantly speeds up
processing since most of the subvalue space is an all-zero empty space.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2177197
Reported-by: Nadia Pinaeva 
Signed-off-by: Ilya Maximets 
---
 include/ovn/expr.h |   1 +
 lib/expr.c | 253 +++--
 tests/ovn.at   |  12 +++
 3 files changed, 211 insertions(+), 55 deletions(-)

diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 80d95ff67..c48f82398 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -384,6 +384,7 @@ struct expr {
 struct {
 union mf_subvalue value;
 union mf_subvalue mask;
+size_t mask_n_bits;
 };
 };
 } cmp;
diff --git a/lib/expr.c b/lib/expr.c
index 0a6f7e574..0cf54d486 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -15,21 +15,23 @@
  */
 
 #include 
+#include "bitmap.h"
 #include "byte-order.h"
-#include "openvswitch/json.h"
+#include "hmapx.h"
 #include "nx-match.h"
 #include "openvswitch/dynamic-string.h"
+#include "openvswitch/json.h"
 #include "openvswitch/match.h"
 #include "openvswitch/ofp-actions.h"
-#include "openvswitch/vlog.h"
 #include "openvswitch/shash.h"
+#include "openvswitch/vlog.h"
+#include "ovn-util.h"
 #include "ovn/expr.h"
 #include "ovn/lex.h"
 #include "ovn/logical-fields.h"
 #include "simap.h"
 #include "sset.h"
 #include "util.h"
-#include "ovn-util.h"
 
 VLOG_DEFINE_THIS_MODULE(expr);
 
@@ -2520,6 +2522,198 @@ crush_and_string(struct expr *expr, const struct 
expr_symbol *symbol)
 return expr_fix(expr);
 }
 
+static int
+compare_cmps_3way(const struct expr *a, const struct expr *b)
+{
+ovs_assert(a->cmp.symbol == b->cmp.symbol);
+if (!a->cmp.symbol->width) {
+return strcmp(a->cmp.string, b->cmp.string);
+} else if (a->cmp.mask_n_bits != b->cmp.mask_n_bits) {
+return a->cmp.mask_n_bits < b->cmp.mask_n_bits ? -1 : 1;
+} else {
+int d = memcmp(>cmp.value, >cmp.value, sizeof a->cmp.value);
+if (!d) {
+d = memcmp(>cmp.mask, >cmp.mask, sizeof a->cmp.mask);
+}
+return d;
+}
+}
+

[ovs-dev] [PATCH ovn v4 2/2] expr: Avoid crash if all sub-expressions crushed down to 'true'.

[Ilya Maximets]

expr_sort() can potentially return NULL if all sub-expressions got
crushed into 'true'.  This didn't happen before, but can happen
with more aggressive optimizations in crush_or().

Acked-by: Han Zhou 
Signed-off-by: Ilya Maximets 
---
 lib/expr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/expr.c b/lib/expr.c
index 0cf54d486..9659b9b63 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -3003,7 +3003,7 @@ expr_sort(struct expr *expr)
 }
 free(subs);
 
-return expr;
+return expr ? expr : expr_create_boolean(true);
 }
 
 static struct expr *expr_normalize_or(struct expr *expr);
-- 
2.39.2

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

[ovs-dev] [PATCH ovn v4 0/2] expr: Optimize OR expressions.

[Ilya Maximets]

This patch set covers removal of expressions which are subsets of other
wider expressions.  This allows to avoid flow explosion in case of
negative matches.  More details are in commit messages.

Version 2:
  * Became a patch set.
  * Added tests and missing bitmap.h include.
  * Code switched to work with bitwise maskable fields only (ORDINAL).
  * Added a new patch to combine smaller expressions into wider ones.
  * Added a patch to fix a crash uncovered with expression aggregation.

Version 3:
  * Dropped patch 3 for performance reasons for now, because it doesn't
allow to make use of I-P in many cases.
  * Patch 1 re-worked to not cause performance issues for normal
address sets generated in OVN.
  * Performance of the patch 1 significantly improved by not perfroming
a full n^2 search and not comparing huge empty parts of subvalues.
The patch became a bit less straightforward, but I hope it's still
fairly readable.

Version 4:
  * Added extra comments.
  * Added ACK from Han to patch 2.
  * Re-worked path shortening (next[]) to track the last non-NULL entry.
  * Restricted superset optmization to expressions that do not track
address sets.  To preserve ability to use I-P. [Han]

Ilya Maximets (2):
  expr: Remove supersets from OR expressions.
  expr: Avoid crash if all sub-expressions crushed down to 'true'.

 include/ovn/expr.h |   1 +
 lib/expr.c | 255 +++--
 tests/ovn.at   |  12 +++
 3 files changed, 212 insertions(+), 56 deletions(-)

-- 
2.39.2

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

[ovs-dev] [PATCH v2] github: Test building Fedora RPMs.

[Ilya Maximets]

Testing that RPMs can be built to catch possible spec file
issues like missing dependencies.

GitHub seems to have an agreement with Docker Hub about rate
limiting of image downloads, so it should not affect us.
We may switch to quay.io if that will ever become a problem
in the future.

Signed-off-by: Ilya Maximets 
---

Version 2:
  * Changed the task name from build-rpm-fedora to build-linux-rpm. [David]
  * Switched to ubuntu-latest.  [David]

 .github/workflows/build-and-test.yml | 37 
 1 file changed, 37 insertions(+)

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index 82675b973..39649c1b5 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -242,3 +242,40 @@ jobs:
   with:
 name: deb-packages-${{ matrix.dpdk }}-dpdk
 path: '/home/runner/work/ovs/*.deb'
+
+  build-linux-rpm:
+name: linux rpm fedora
+runs-on: ubuntu-latest
+container: fedora:37
+timeout-minutes: 30
+
+strategy:
+  fail-fast: false
+
+steps:
+- name: checkout
+  uses: actions/checkout@v3
+- name: install dependencies
+  run: |
+dnf install -y rpm-build dnf-plugins-core
+sed -e 's/@VERSION@/0.0.1/' rhel/openvswitch-fedora.spec.in \
+> /tmp/ovs.spec
+dnf builddep -y /tmp/ovs.spec
+rm -f /tmp/ovs.spec
+
+- name: configure
+  run:  ./boot.sh && ./configure
+
+- name: build
+  run:  make rpm-fedora
+
+- name: install
+  run:  dnf install -y rpm/rpmbuild/RPMS/*/*.rpm
+
+- name: upload rpm packages
+  uses: actions/upload-artifact@v3
+  with:
+name: rpm-packages
+path: |
+  rpm/rpmbuild/SRPMS/*.rpm
+  rpm/rpmbuild/RPMS/*/*.rpm
-- 
2.39.2

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

Re: [ovs-dev] [PATCH] github: Test building Fedora RPMs.

[Ilya Maximets]

On 4/5/23 15:51, David Marchand wrote:
> On Thu, Jan 19, 2023 at 2:15 PM Ilya Maximets  wrote:
>>
>> Testing that RPMs can be built to catch possible spec file
>> issues like missing dependencies.
>>
>> GitHub seems to have an agreement with Docker Hub about rate
>> limiting of image downloads, so it should not affect us.
>> We may switch to quay.io if that will ever become a problem
>> in the future.
>>
>> Signed-off-by: Ilya Maximets 
> 
> It lgtm, just a few comments.
> 
> 
> The deb jobs have a check on the generated python libraries.
> Can't we have issues with rpms?

We check the C-extension for python libraries in debian packaging.
We do not build it in fedora rpms.

> 
> 
>> ---
>>  .github/workflows/build-and-test.yml | 37 
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/.github/workflows/build-and-test.yml 
>> b/.github/workflows/build-and-test.yml
>> index 82675b973..883d44b4e 100644
>> --- a/.github/workflows/build-and-test.yml
>> +++ b/.github/workflows/build-and-test.yml
>> @@ -242,3 +242,40 @@ jobs:
>>with:
>>  name: deb-packages-${{ matrix.dpdk }}-dpdk
>>  path: '/home/runner/work/ovs/*.deb'
>> +
>> +  build-rpm-fedora:
> 
> The existing job names are:
>   build-linux:
>   build-osx:
>   build-linux-deb:
> 
> So this new name does not seem to follow a convention (if there is one).
> I would have expected "build-linux-rpm".

OK.

> 
> 
>> +name: linux rpm fedora
>> +runs-on: ubuntu-22.04
> 
> In this test, we don't care about a specific version of Ubuntu, all
> that matters is to get a working container.
> I would go with ubuntu-latest.

OK.

> 
> 
>> +container: fedora:37
>> +timeout-minutes: 30
>> +
>> +strategy:
>> +  fail-fast: false
> 
> 

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

Re: [ovs-dev] [PATCH ovn v3 1/2] expr: Remove supersets from OR expressions.

[Ilya Maximets]

On 3/31/23 23:30, Han Zhou wrote:
> 
> 
> On Fri, Mar 31, 2023 at 5:34 AM Ilya Maximets  > wrote:
>>
>> On 3/31/23 06:39, Han Zhou wrote:
>> >
>> >
>> > On Wed, Mar 29, 2023 at 9:05 AM Ilya Maximets > >  > > >> wrote:
>> >>
>> >> While crushing OR expressions, OVN removes exact replicas of sub
>> >> expressions.  However, there could be many CMP expressions that are
>> >> supersets of each other.  These are most likely to be created as a
>> >> result of cross-product while expanding brackets in the AND expression
>> >> in crush_and_numeric(), i.e. while converting
>> >> "x && (a0 || a1) && (b0 || b1)" into "xa0b0 || xa0b1 || xa1b0 || xa1b1".
>> >>
>> >> In addition to removal of exact duplicates introducing scan and removal
>> >> of supersets of other existing sub-expressions to reduce the amount of
>> >> generated flows.  This adds extra computations, but should save time
>> >> later, since less flows will be generated.
>> >>
>> >> Example:
>> >>
>> >>   "ip4.src == 172.168.0.0/16  
>> >> > && 
>> >> ip4.src!={172.168.13.0/24  
>> >> >, 172.168.15.0/24 
>> >>  > >> >}"
>> >>
>> >>   Processing of this expression yields 42 flows:
>> >>
>> >>   $ ./tests/ovstest test-ovn expr-to-flows <<< "$expr"
>> >>
>> >>   ip,nw_src=172.168.0.0/255.255.1.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.10.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.12.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.3.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.4.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.5.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.6.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.8.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.9.0  
>> >> >
>> >>   ip,nw_src=172.168.128.0/17  
>> >> >
>> >>   <... 32 more flows ...>
>> >>
>> >>   We can see that many flows above do overlap, e.g. 255.255.3.0
>> >>   mask is a superset of 255.255.1.0.  Everything that matches
>> >>   255.255.3.0, will match 255.255.1.0 as well (the value is the same).
>> >>
>> >>   By removing all the unnecessary supersets, the set of flows can
>> >>   be reduced from 42 down to 7:
>> >>
>> >>   ip,nw_src=172.168.0.0/255.255.1.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.4.0  
>> >> >
>> >>   ip,nw_src=172.168.0.0/255.255.8.0  
>> >> >
>> >>   ip,nw_src=172.168.128.0/17  
>> >> >
>> >>   ip,nw_src=172.168.16.0/255.255.16.0  
>> >> >
>> >>   ip,nw_src=172.168.32.0/255.255.32.0  
>> >> >
>> >>   ip,nw_src=172.168.64.0/255.255.64.0  
>> >> >
>> >>
>> >> This change should be particularly useful for expressions with
>> >> inequality checks, like the one above.  Such expressions are
>> >> frequent among ACL rules.
>> >>
>> >>   "ip4.src != {172.168.13.0/24  
>> >> >, 172.168.14.0/24 
>> >>  > >> >, 172.168.15.0/24  
>> >> >}"
>> >>
>> >>   Brefore:
>> >>   $ ./tests/ovstest test-ovn expr-to-flows <<< 

Re: [ovs-dev] [PATCH ovn v2 4/9] ci: Add Ubuntu based Dockerfile

[Frode Nordahl]

On Wed, Mar 15, 2023 at 7:29 AM Ales Musil  wrote:
>
> Add Dockerfile based on Ubuntu that contains all
> dependencies and can be later on used by CI.
>
> Signed-off-by: Ales Musil 
> ---
>  utilities/automake.mk  |  1 +
>  utilities/containers/Makefile  | 11 ++---
>  utilities/containers/ubuntu/Dockerfile | 62 ++
>  3 files changed, 69 insertions(+), 5 deletions(-)
>  create mode 100755 utilities/containers/ubuntu/Dockerfile
>
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index 142fe6810..bb261439d 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -41,6 +41,7 @@ EXTRA_DIST += \
>  utilities/containers/Makefile \
>  utilities/containers/py-requirements.txt \
>  utilities/containers/fedora/Dockerfile \
> +utilities/containers/ubuntu/Dockerfile \
>  utilities/docker/Makefile \
>  utilities/docker/start-ovn \
>  utilities/docker/ovn_default_nb_port \
> diff --git a/utilities/containers/Makefile b/utilities/containers/Makefile
> index ef1d42aca..d79e4ad5e 100644
> --- a/utilities/containers/Makefile
> +++ b/utilities/containers/Makefile
> @@ -1,10 +1,11 @@
>  CONTAINER_CMD ?= podman
> -DISTRO ?= fedora
>  IMAGE_NAME ?= "ovn-org/ovn-tests"
>  CONTAINERS_PATH ?= "."
>
> -.PHONY: build
> +distros := fedora ubuntu
>
> -build:
> -   $(CONTAINER_CMD) build --no-cache --rm -t $(IMAGE_NAME) \
> -   -f $(DISTRO)/Dockerfile . 
> --build-arg=CONTAINERS_PATH=$(CONTAINERS_PATH)
> +.PHONY: $(distros)
> +
> +$(distros):
> +   $(CONTAINER_CMD) build --no-cache --rm -t $(IMAGE_NAME):$@ \
> +   -f $@/Dockerfile . --build-arg=CONTAINERS_PATH=$(CONTAINERS_PATH)
> diff --git a/utilities/containers/ubuntu/Dockerfile 
> b/utilities/containers/ubuntu/Dockerfile
> new file mode 100755
> index 0..85b048cf1
> --- /dev/null
> +++ b/utilities/containers/ubuntu/Dockerfile
> @@ -0,0 +1,62 @@
> +FROM registry.hub.docker.com/library/ubuntu:22.04
> +
> +ARG CONTAINERS_PATH
> +
> +# Update distro, install packages and clean any possible leftovers
> +RUN apt update -y \
> +&& \
> +apt upgrade -y \
> +&& \
> +apt install -y \
> +automake \
> +bc \
> +clang \
> +ethtool \
> +gcc \
> +git \
> +init \
> +iproute2 \
> +iputils-ping \
> +isc-dhcp-server \
> +kmod \
> +libelf-dev \
> +libjemalloc2 \
> +libjemalloc-dev \
> +libnuma-dev \
> +libpcap-dev \
> +libssl-dev \
> +libtool \
> +llvm-dev \
> +ncat \
> +net-tools \
> +python3-dev \
> +python3-pip \
> +selinux-policy-dev \
> +sudo \
> +tcpdump \
> +wget \
> +&& \
> +apt autoremove \
> +&& \
> +apt clean
> +
> +# Compile sparse from source
> +WORKDIR /workspace/sparse
> +
> +RUN git clone git://git.kernel.org/pub/scm/devel/sparse/sparse.git \
> +/workspace/sparse \
> +&& \
> +make -j4 PREFIX=/usr HAVE_LLVM= HAVE_SQLITE= install
> +
> +WORKDIR /workspace
> +
> +COPY $CONTAINERS_PATH/py-requirements.txt /tmp/py-requirements.txt
> +
> +# Update and install pip dependencies
> +RUN python3 -m pip install --upgrade pip \
> +&& \
> +python3 -m pip install wheel \
> +&& \
> +python3 -m pip install -r /tmp/py-requirements.txt
> +
> +CMD ["/sbin/init"]
> --
> 2.39.2
>

Thank you for putting this together!

As mentioned on another patch, I'm a bit worried about yet another
place to track dependencies, but to be pragmatic we need to start
somewhere. We'll eventually pick up refurbishing the upstream OVN
debian packaging, and can probably help convert this to using the deb
`mk-build-deps` approach OVS uses (as proposed by Ilya on a different
thread) at that point in time.

Otherwise, LGTM.

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

Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for ingress and egress pkts policer.

[lin huang]

Hi Simon,

: ) 
Thanks for your reply.

There are some explanations about your comments.

Best regards, Huang Lin.

> -Original Message-
> From: Simon Horman 
> Sent: Wednesday, April 5, 2023 8:28 PM
> To: lin huang 
> Cc: d...@openvswitch.org; i.maxim...@ovn.org
> Subject: Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for ingress and
> egress pkts policer.
> 
> On Tue, Apr 04, 2023 at 01:46:05PM +, lin huang wrote:
> > Hi Ilya,
> >
> > Pls review my code.
> >
> > I want to add a new policer which is pps-based in netdev-dpdk module.
> > The policer is divided into ingress and egress part. Both use the ovs native
> tocken bucket library as the counter.
> > Compared to bandwidth-based policers, the pps-based policer is more
> effective at combating low-and-slow types of DDoS attacks.
> >
> > This new ingress policer is configured using the "policer_kpkts_rate" and
> "policer_kpkts_burst" configuration statement. Here is the example.
> > ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
> > ovs-vsctl set interface dpdk0 policer_kpkts_burst=2
> >
> > This new egress policer is configured using the " egress-pkts-policer "
> configuration statement. Here is the example.
> > ovs-vsctl set port vhost-user0 qos=@newqos -- \
> > --id=@newqos create qos type=egress-pkts-policer
> other-config:pkts_rate=2048 \
> > other-config:pkts_burst=2048`
> 
> I have some high level questions about this.
> 
> 1. For ingress there is, as you mention above,
>already options at the ovs-vsctl for port-based PPS policing.
> 
>Did you consider making the egress options you are adding consistent
>with the existing ingress options?
>f.e.  egress_policing_kpkts_rate and egress_policing_kpkts_rate.

Yes we can add new egress options in interface. But this work will 
conflict the 
userspace datapath egress QoS (egress policing) framework. 
The framework needs to set up a QoS instance which is configured by 
cir/cbs/pps... to a specific port.
So, I add a new egress policer called pkts-policer.

> 
> 2. Metering operates on egress, is a policer, and can support PPS.
>Did you consider using this instead of adding a new egress policer?

In my opinion, meter table is used in flow rate limit not in interface 
traffic.
Interface policer is different from meter policer which you can police a 
specific flow.
The new egress policer is just in userspace datapath not in kernel 
datapath.
Does the new policer involve the kernel datapath?

> 
>There is an upstreamed implementation of this for upstreamed
>for the kernel datapath with and without TC HW offload.

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

Re: [ovs-dev] [RFC ovn 1/2] northd: add ACL Sampling

[Dumitru Ceara]

On 4/5/23 09:20, Adrian Moreno wrote:
> Thanks for the comments Dumitru!
> 
> On 3/30/23 12:02, Dumitru Ceara wrote:
>> On 10/18/22 17:59, Adrian Moreno wrote:
>>> Introduce a new table called Sample where per-flow IPFIX configuration
>>> can be specified.
>>> Also, reference rows from such table from the ACL table to enable the
>>> configuration of ACL sampling. If enabled, northd will add a sample
>>> action to each ACL-related logical flow.
>>>
>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>   northd/northd.c  | 31 ++-
>>>   ovn-nb.ovsschema | 23 ++-
>>>   ovn-nb.xml   | 31 +++
>>>   3 files changed, 83 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index 61d474840..3e09e3a0f 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -6194,6 +6194,27 @@ build_acl_log(struct ds *actions, const struct
>>> nbrec_acl *acl,
>>>   ds_put_cstr(actions, "); ");
>>>   }
>>>   +static void
>>> +build_acl_sample(struct ds *actions, const struct nbrec_acl *acl)
>>> +{
>>> +    if (!acl->sample) {
>>> +    return;
>>> +    }
>>> +    ds_put_format(actions, "sample(probability=%"PRId16","
>>> +   "collector_set=%d,"
>>> +   "obs_domain=%hd,",
>>> +    (uint16_t) acl->sample->probability,
>>> +    (uint32_t) acl->sample->collector_set_id,
>>> +    (uint8_t) acl->sample->obs_domain_id);
>>> +
>>> +    if (acl->sample->obs_point_id) {
>>> +    ds_put_format(actions, "obs_point=%"PRId32");",
>>> +  (uint32_t) *acl->sample->obs_point_id);
>>> +    } else {
>>> +    ds_put_cstr(actions, "obs_point=$cookie);");
>>> +    }
>>> +}
>>> +
>>>   static void
>>>   build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
>>>  enum ovn_stage stage, struct nbrec_acl *acl,
>>> @@ -6260,6 +6281,7 @@ consider_acl(struct hmap *lflows, struct
>>> ovn_datapath *od,
>>>   if (!strcmp(acl->action, "allow-stateless")) {
>>>   ds_clear(actions);
>>>   build_acl_log(actions, acl, meter_groups);
>>> +    build_acl_sample(actions, acl);
>>>   ds_put_cstr(actions, "next;");
>>>   ovn_lflow_add_with_hint(lflows, od, stage,
>>>   acl->priority + OVN_ACL_PRI_OFFSET,
>>> @@ -6275,6 +6297,7 @@ consider_acl(struct hmap *lflows, struct
>>> ovn_datapath *od,
>>>   if (!has_stateful) {
>>>   ds_clear(actions);
>>>   build_acl_log(actions, acl, meter_groups);
>>> +    build_acl_sample(actions, acl);
>>>   ds_put_cstr(actions, "next;");
>>>   ovn_lflow_add_with_hint(lflows, od, stage,
>>>   acl->priority +
>>> OVN_ACL_PRI_OFFSET,
>>> @@ -6304,6 +6327,7 @@ consider_acl(struct hmap *lflows, struct
>>> ovn_datapath *od,
>>>     REG_LABEL" = %"PRId64"; ", acl->label);
>>>   }
>>>   build_acl_log(actions, acl, meter_groups);
>>> +    build_acl_sample(actions, acl);
>>>   ds_put_cstr(actions, "next;");
>>>   ovn_lflow_add_with_hint(lflows, od, stage,
>>>   acl->priority +
>>> OVN_ACL_PRI_OFFSET,
>>> @@ -6329,6 +6353,7 @@ consider_acl(struct hmap *lflows, struct
>>> ovn_datapath *od,
>>>     REG_LABEL" = %"PRId64"; ", acl->label);
>>>   }
>>>   build_acl_log(actions, acl, meter_groups);
>>> +    build_acl_sample(actions, acl);
>>>   ds_put_cstr(actions, "next;");
>>>   ovn_lflow_add_with_hint(lflows, od, stage,
>>>   acl->priority +
>>> OVN_ACL_PRI_OFFSET,
>>> @@ -6349,7 +6374,7 @@ consider_acl(struct hmap *lflows, struct
>>> ovn_datapath *od,
>>>    */
>>>   bool log_related = smap_get_bool(>options,
>>> "log-related",
>>>    false);
>>> -    if (acl->log && acl->label && log_related) {
>>> +    if ((acl->log || acl->sample) && acl->label &&
>>> log_related) {
>>>   /* Related/reply flows need to be set on the
>>> opposite pipeline
>>>    * from where the ACL itself is set.
>>>    */
>>> @@ -6365,6 +6390,7 @@ consider_acl(struct hmap *lflows, struct
>>> ovn_datapath *od,
>>>     use_ct_inv_match ? " && !ct.inv" : "",
>>>     ct_blocked_match, acl->label);
>>>   build_acl_log(actions, acl, meter_groups);
>>> +    build_acl_sample(actions, acl);
>>>   ds_put_cstr(actions, "next;");
>>>   ovn_lflow_add_with_hint(lflows, od, log_related_stage,
>>>   UINT16_MAX - 2,

Re: [ovs-dev] [PATCH ovn v2 1/9] ci: Add missing packages to the container

[Frode Nordahl]

On Wed, Mar 29, 2023 at 12:58 PM Ilya Maximets  wrote:
>
> On 3/29/23 12:36, Dumitru Ceara wrote:
> > On 3/29/23 12:32, Ales Musil wrote:
> >> On Wed, Mar 29, 2023 at 9:55 AM Dumitru Ceara  wrote:
> >>
> >>> On 3/29/23 09:43, Dumitru Ceara wrote:
>  On 3/29/23 07:29, Ales Musil wrote:
> > On Tue, Mar 28, 2023 at 4:48 PM Dumitru Ceara 
> >>> wrote:
> >
> >> On 3/15/23 07:29, Ales Musil wrote:
> >>> Add missing jemalloc, kmod and scapy packages.
> >>> Install scapy through pip, because the
> >>> python3-scapy package has a lot of dependencies
> >>> that would increase the overall size of the image
> >>> by ~800 MB.
> >>>
> >>> Signed-off-by: Ales Musil 
> >>> ---
> >>>  utilities/containers/fedora/Dockerfile   | 2 ++
> >>>  utilities/containers/py-requirements.txt | 1 +
> >>>  2 files changed, 3 insertions(+)
> >>>
> >>> diff --git a/utilities/containers/fedora/Dockerfile
> >> b/utilities/containers/fedora/Dockerfile
> >>> index be8cd6ff2..d24bf1232 100755
> >>> --- a/utilities/containers/fedora/Dockerfile
> >>> +++ b/utilities/containers/fedora/Dockerfile
> >>> @@ -19,7 +19,9 @@ RUN dnf -y update \
> >>>  iproute \
> >>>  iproute-tc \
> >>>  iputils \
> >>> +jemalloc-devel \
> >>>  kernel-devel \
> >>> +kmod \
> >>>  libcap-ng-devel \
> >>>  libtool \
> >>>  net-tools \
> >>
> >> I think it might be better to follow the official installation
> >>> procedure
> >> and instead of doing this do something like:
> >>
> >>
> >>
> >>> https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/.ci/ovn-kubernetes/Dockerfile#L19
> >>
> >> RUN sed -e 's/@VERSION@/0.0.1/' rhel/openvswitch-fedora.spec.in >
> >> /tmp/ovs.spec
> >> RUN dnf builddep -y /tmp/ovs.spec
> >>
> >>
> >>
> >>> https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/.ci/ovn-kubernetes/Dockerfile#L28
> >>
> >> RUN sed -e 's/@VERSION@/0.0.1/' rhel/ovn-fedora.spec.in >
> >>> /tmp/ovn.spec
> >> RUN dnf builddep -y /tmp/ovn.spec
> >>
> >> There's still the risk that a new dependency is added and the new image
> >> hasn't been built/pushed yet.  How do you think it's best to address
> >> that?
> >>
> >
> > We can trigger the image manually so if something new is added we will
> > trigger the job
> > once it is merged. I'm more concerned about those being build
> >>> dependencies,
> > not test dependencies
> 
>  Good point.
> 
> > which can be misleading. However if the vote is in favor of updating the
> > spec file with all the dependencies
> 
>  I'm not sure we want to add stuff like nc or tcpdump as build
>  dependencies though.  Can we at least shorten the list by first
>  installing the build deps and then installing the packages required for
>  testing?
> 
> >>>
> >>> After another look at the spec file for Fedora I see we have:
> >>>
> >>> %if %{with check}
> >>> BuildRequires: tcpdump
> >>> %endif
> >>>
> >>> Can we use this and conditionally install packages only if we plan to
> >>> run checks?
> >>>
> > for tests I'm fine with that.
> >
> >
> 
> >>>
> >>>
> >> After testing the suggestion there is a couple points that are more in
> >> favor of
> >> list inside the Dockerfile:
> >>
> >> 1) The dependencies installed from the spec file increase the container
> >> size by 500 MB,
> >> it due to installation of python3-sphinx as rpm vs the pip installation of
> >> sphinx.
> >> Which is IMO very bad, going from 1.25 GB to 1.77 GB (>40% increase).
> >>
> >> 2) I'm not sure if we can do something similar on Ubuntu.
>
> FWIW, we can.  Look for mk-build-deps in .ci/linux-build.sh in OVS.
> Assuming OVN has the same list of dependencies, even if OVN's
> debian packaging is messed up, OVS'es is fine and can be used.

I share the concern of manually maintaining a list of dependencies for
the container images, but we need to start somewhere. At this point in
time I do believe the Open vSwitch and OVN build dependencies overlap
completely, so Ilya's suggestion here might work.

I guess this is a further incentive to pick up the refurbishing of the
upstream OVN Debian packaging.

-- 
Frode Nordahl

> Best regards, Ilya Maximets.
>
> >>
> >> 3) Currently we actually use pip to install some python dependencies and 
> >> the
> >> dependencies in containers follow this approach.
> >>
> >
> > It's a bit unfortunate but let's keep it as it is then.  Thanks a lot
> > for checking!
> >
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] github: Test building Fedora RPMs.

[David Marchand]

On Thu, Jan 19, 2023 at 2:15 PM Ilya Maximets  wrote:
>
> Testing that RPMs can be built to catch possible spec file
> issues like missing dependencies.
>
> GitHub seems to have an agreement with Docker Hub about rate
> limiting of image downloads, so it should not affect us.
> We may switch to quay.io if that will ever become a problem
> in the future.
>
> Signed-off-by: Ilya Maximets 

It lgtm, just a few comments.


The deb jobs have a check on the generated python libraries.
Can't we have issues with rpms?


> ---
>  .github/workflows/build-and-test.yml | 37 
>  1 file changed, 37 insertions(+)
>
> diff --git a/.github/workflows/build-and-test.yml 
> b/.github/workflows/build-and-test.yml
> index 82675b973..883d44b4e 100644
> --- a/.github/workflows/build-and-test.yml
> +++ b/.github/workflows/build-and-test.yml
> @@ -242,3 +242,40 @@ jobs:
>with:
>  name: deb-packages-${{ matrix.dpdk }}-dpdk
>  path: '/home/runner/work/ovs/*.deb'
> +
> +  build-rpm-fedora:

The existing job names are:
  build-linux:
  build-osx:
  build-linux-deb:

So this new name does not seem to follow a convention (if there is one).
I would have expected "build-linux-rpm".


> +name: linux rpm fedora
> +runs-on: ubuntu-22.04

In this test, we don't care about a specific version of Ubuntu, all
that matters is to get a working container.
I would go with ubuntu-latest.


> +container: fedora:37
> +timeout-minutes: 30
> +
> +strategy:
> +  fail-fast: false


-- 
David Marchand

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

Re: [ovs-dev] [PATCH ovn v2] controller: Clear tunnels from old integration bridge

[Ihar Hrachyshka]

Thank you for v2! All good now.

Acked-By: Ihar Hrachyshka 

On Mon, Apr 3, 2023 at 5:50 AM Ales Musil  wrote:
>
> After integration bridge change the tunnels would
> stay on the old bridge preventing new tunnels creation
> and disrupting traffic. Detect the bridge change
> and clear the tunnels from the old integration bridge.
>
> Reported-at: https://bugzilla.redhat.com/2173635
> Signed-off-by: Ales Musil 
> ---
> v2: Make sure that we don't clear tunnels belonging to
> other ovn-controllers on the same host.
> ---
>  controller/encaps.c |  42 -
>  controller/encaps.h |   4 +-
>  controller/ovn-controller.c |  26 +-
>  tests/ovn.at| 168 
>  4 files changed, 236 insertions(+), 4 deletions(-)
>
> diff --git a/controller/encaps.c b/controller/encaps.c
> index 2662eaf98..4a79030b8 100644
> --- a/controller/encaps.c
> +++ b/controller/encaps.c
> @@ -386,6 +386,21 @@ chassis_tzones_overlap(const struct sset 
> *transport_zones,
>  return false;
>  }
>
> +static void
> +clear_old_tunnels(const struct ovsrec_bridge *old_br_int, const char *prefix,
> +  size_t prefix_len)
> +{
> +for (size_t i = 0; i < old_br_int->n_ports; i++) {
> +const struct ovsrec_port *port = old_br_int->ports[i];
> +const char *id = smap_get(>external_ids, "ovn-chassis-id");
> +if (id && !strncmp(port->name, prefix, prefix_len)) {
> +VLOG_DBG("Clearing old tunnel port \"%s\" (%s) from bridge "
> + "\"%s\".", port->name, id, old_br_int->name);
> +ovsrec_bridge_update_ports_delvalue(old_br_int, port);
> +}
> +}
> +}
> +
>  void
>  encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct ovsrec_bridge *br_int,
> @@ -393,12 +408,37 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct sbrec_chassis *this_chassis,
> const struct sbrec_sb_global *sbg,
> const struct ovsrec_open_vswitch_table *ovs_table,
> -   const struct sset *transport_zones)
> +   const struct sset *transport_zones,
> +   const struct ovsrec_bridge_table *bridge_table,
> +   const char *br_int_name)
>  {
>  if (!ovs_idl_txn || !br_int) {
>  return;
>  }
>
> +if (!br_int_name) {
> +/* The controller has just started, we need to look through all
> + * bridges for old tunnel ports. */
> +char *tunnel_prefix = xasprintf("ovn%s-", 
> get_chassis_idx(ovs_table));
> +size_t prefix_len = strlen(tunnel_prefix);
> +
> +const struct ovsrec_bridge *br;
> +OVSREC_BRIDGE_TABLE_FOR_EACH (br, bridge_table) {
> +if (!strcmp(br->name, br_int->name)) {
> +continue;
> +}
> +clear_old_tunnels(br, tunnel_prefix, prefix_len);
> +}
> +
> +free(tunnel_prefix);
> +} else if (strcmp(br_int_name, br_int->name)) {
> +/* The integration bridge was changed, clear tunnel ports from
> + * the old one. */
> +const struct ovsrec_bridge *old_br_int =
> +get_bridge(bridge_table, br_int_name);
> +clear_old_tunnels(old_br_int, "", 0);
> +}
> +
>  const struct sbrec_chassis *chassis_rec;
>
>  struct tunnel_ctx tc = {
> diff --git a/controller/encaps.h b/controller/encaps.h
> index 867c6f28c..cf38dac1a 100644
> --- a/controller/encaps.h
> +++ b/controller/encaps.h
> @@ -35,7 +35,9 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
>  const struct sbrec_chassis *,
>  const struct sbrec_sb_global *,
>  const struct ovsrec_open_vswitch_table *,
> -const struct sset *transport_zones);
> +const struct sset *transport_zones,
> +const struct ovsrec_bridge_table *bridge_table,
> +const char *br_int_name);
>
>  bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn,
>  const struct ovsrec_bridge *br_int);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 76be2426e..242d93823 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -536,6 +536,21 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>  *br_int_ = br_int;
>  }
>
> +static void
> +consider_br_int_change(const struct ovsrec_bridge *br_int, char 
> **current_name)
> +{
> +ovs_assert(current_name);
> +
> +if (!*current_name) {
> +*current_name = xstrdup(br_int->name);
> +}
> +
> +if (strcmp(*current_name, br_int->name)) {
> +free(*current_name);
> +*current_name = xstrdup(br_int->name);
> +}
> +}
> +
>  static void
>  update_ssl_config(const struct ovsrec_ssl_table *ssl_table)
>  {
> @@ -4918,6 +4933,8 @@ main(int argc, char *argv[])
>  char *ovn_version = ovn_get_internal_version();
>  VLOG_INFO("OVN internal version is : [%s]", ovn_version);
>
> +

Re: [ovs-dev] [PATCH v2] util: fix an issue that thread name cannot be set

[Eelco Chaudron]

On 31 Mar 2023, at 10:11, Songtao Zhan wrote:

> To: d...@openvswitch.org,
> i.maxim...@ovn.org
>
> The name of the current thread consists of a name with a maximum
> length of 16 bytes and a thread ID. The final name may be longer
> than 16 bytes. If the name is longer than 16 bytes, the thread
> name will fail to be set

I replied to the v1, but did not see any response, so I repeat this on v2:

“
Thanks for the patch, do you have examples of when this happens? Maybe we 
should also change the thread naming to avoid this?

See one additional item below.
“

>
> Signed-off-by: Songtao Zhan 
> ---
>
> Notes:
> v2:
>  - modify code formatting and comments
>
>  lib/util.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/lib/util.c b/lib/util.c
> index 96a71550d..b0eb9f343 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -645,6 +645,10 @@ set_subprogram_name(const char *subprogram_name)
>  free(subprogram_name_set(pname));
>
>  #if HAVE_GLIBC_PTHREAD_SETNAME_NP
> +/* The maximum thead name including '\0' supported is 16. */
> +if (strlen(pname) > 15) {
> +pname[15] = '\0';

Not sure what is better, but would it make sense to use the last upper 
characters? This way if we do truncate we know the internal thread id, as 
naming in OVS, in general, is “xasprintf("%s%u", aux.name, id)”.
If we do this we could add some indication is was truncated, like 
“LONGTHREAD123456” would become “>NGTHREAD123456”.

> +}
>  pthread_setname_np(pthread_self(), pname);
>  #elif HAVE_NETBSD_PTHREAD_SETNAME_NP
>  pthread_setname_np(pthread_self(), "%s", pname);
> -- 
> 2.31.1
>
>
>
>
> zhan...@chinatelecom.cn
> ___
> 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] [RFC v2 ovn 0/4] rework OVN QoS implementation

[Rodolfo Alonso Hernandez]

Hello:

I've made a copy in my personal google account to be able to share the doc
link to anyone:
https://docs.google.com/document/d/1Q4IV4tTahPfAwEj5lenYzskpEk1Sp2zIYY1ZL_ws0DM/edit?usp=sharing.
This link has commenter permissions.

Regards.

On Tue, Apr 4, 2023 at 6:57 PM Ihar Hrachyshka  wrote:

> Hi,
>
> thanks for v2. I will leave general comments to the series here, and I
> will post some specific comments to corresponding patches where
> appropriate.
>
> --
>
> The series claims that VIF support for QoS was not present before.
> ("add QoS support for logical switch port interfaces" etc.) This is
> not correct. QoS min-rate filters are applied to LSP by Neutron. Then
> - as intended - ovn-controller creates corresponding qdisc queues on
> egress interfaces that control bandwidth via HTB. I confirm LSP
> settings are working here: https://paste.centos.org/view/4eedac05 (you
> can apply it to main; it passes).
>
> The series, for LSP ports, switches from setting queues on egress
> interfaces to setting them on TAP interfaces. This is wrong. I think
> Rodolfo has a more detailed explanation captured in document, (I hope
> he can share it here shortly) but the gist is that physical interfaces
> are hardware and provide the actual bandwidth, and that's where HTB
> algo should run to shape traffic aggregated from all.
>
> On Fri, Mar 31, 2023 at 6:40 AM Lorenzo Bianconi
>  wrote:
> >
> > Rework OVN QoS implementation in order to configure it through OVS QoS
> > table instead of running tc command directly bypassing OVS.
> >
> > Changes since v1:
> > - get rid of qos_ovs_port from logical_switch_port option column and let
> ovn to
> >   compute it
> > - add VIF QoS support
> > - take into account qos_min_rate in port_has_qos_params
> >
> > Lorenzo Bianconi (4):
> >   controller: configure qos through ovs qos table and do not run tc
> > directly
> >   controller: improve ovs port lookup by name and qos
> >   controller: add QoS support for logical switch port interfaces
> >   northd: take into account qos_min_rate in port_has_qos_params
> >
> >  controller/binding.c| 429 +---
> >  controller/binding.h|   5 +-
> >  controller/ovn-controller.c |  35 ++-
> >  controller/ovsport.c|  32 +++
> >  controller/ovsport.h|   5 +
> >  northd/northd.c |   2 +-
> >  tests/system-ovn.at | 101 -
> >  7 files changed, 367 insertions(+), 242 deletions(-)
> >
> > --
> > 2.39.2
> >
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC ovn 0/3] rework OVN QoS implementation

[Rodolfo Alonso Hernandez]

I don't think that will be a problem because:
* The number of external interfaces will be one or two (each interface is
an expensive network card).
* We'll have as many TC rules as ports. If we have 100 ports (for example)
in a compute node, that will be 100 rules (max). That is not a big deal for
the TC engine when matching with the queue ID.

Regards.

On Thu, Mar 30, 2023 at 7:00 PM Ihar Hrachyshka  wrote:

> On Thu, Mar 30, 2023 at 6:44 AM Rodolfo Alonso Hernandez
>  wrote:
> >
> > Hello:
> >
> > In OpenStack Neutron we are only using the "qos_min_rate" setting in the
> LSP (for max rate and DSCP we use QoS registers), only for egress traffic.
> The way it is supposed to work is the TC rules should be applied on the
> egress interface. In the case of VLAN or flat networks (not tunneled), the
> physical bridge will have an interface; this interface should receive the
> TC rules.
> >
> > When testing the current code (not the new patch series) using
> OpenStack, we deployed the environment with two physical bridges connected
> to two physical networks. Then we created two ports, one per network. In
> OpenStack Neutron we are only using the "qos_min_rate" setting. Each time a
> QoS was defined for a LSP, both physical bridges external interfaces were
> receiving the TC configuration, instead of only the physical interface of
> the network.
> >
>
> I think this is just the way it's implemented: every egress iface gets
> the same queues configured. But these queues are tied to ports
> (through unique qdisc_queue_id). It's just that, in general, some
> queues on some egress interfaces won't be utilized (depending on the
> logical topology).
>
> Is it a *real* problem? Do we expect e.g. performance issues because
> of unnecessary queues defined?
>
> > NOTE: in Neutron we are only defining the "qos_min_rate", as commented
> before. Because OVN makes a direct translation between the OVN parameters
> and the TC rules, we also need to define the "qos_max_rate", same as when
> executing a TC command ("rate" cannot be defined without "ceil"). But this
> is something we need to fix in Neutron.
> >
> > About the new series, it is difficult for Neutron to set the
> "qos_ovs_port" in the LSP. Neutron is acting as a CMS and can't read the
> local OVS database of a particular node. We can have access to OVN NB and
> SB, but currently it is not possible to read each node's local OVS
> database. If this is going to be the implementation, we'll need to refactor
> how "min-bw" feature works, using some agents deployed in the nodes to
> retrieve this info (NOTE: we have agents in the nodes but not for this
> purpose; in any case, that can be considered).
> >
> > Regards.
> >
> > On Wed, Mar 29, 2023 at 11:31 PM Lorenzo Bianconi <
> lorenzo.bianc...@redhat.com> wrote:
> >>
> >> > Hi Lorenzo et al.
> >>
> >> Hi Ihar,
> >>
> >> thx for reviewing the series.
> >>
> >> >
> >> > I took a look at the series and have a number of concerns. Note that I
> >> > looked at the series in context of OpenStack and the way Neutron uses
> >> > qos_ options. Some points below are not directly relevant to the
> >> > series but I think they add important context.
> >> >
> >> > ---
> >> >
> >> > First, I will agree with the direction of the patch - it is beneficial
> >> > to switch from TC to using OVS queues to configure LSP level qos_*
> >> > settings.
> >> >
> >> > But, see below.
> >> >
> >> > 1. The 2/3 patch explicitly claims that the qos_min_rate option is for
> >> > localnet ports only: "QoS is only supported on localnet port for the
> >> > moment". Actually, it's vice versa: the option is documented for VIF
> >> > (regular) ports
> >> > only:
> https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/ovn-nb.xml#L1096
> >> > To compare, the localnet section of the document doesn't list this
> >> > option:
> https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/ovn-nb.xml#L1034
> >> >
> >> > This assumption about the application of the options to localnet
> >> > ports, and not to other port types, seems to creep into the 1/3 patch
> >> > of the series too.
> >> >
> >> > I tried to understand where the misunderstanding comes from. Perhaps
> >> > it's because the "egress qos" system test was added to cover a
> >> > supposed I-P regression where qos is not applied to localnet ports.
> >> > Then later, when adding qos_min_rate option support, I patched the
> >> > existing test case, without realizing that it sets qos rules to
> >> > localnet port, not regular VIF port (as per NB documentation). We
> >> > should probably fix the test case (or add a new one for regular ports
> >> > if we decide that localnet qos is a valid scenario).
> >> >
> >> > Or perhaps it's because we call consider_localnet_lport with qos_map
> >> > passed, so it also configures qos queues for these ports.
> >> >
> >> > Regardless, it's clear that regular (VIF) ports should support qos_
> >> > options, so any changes to disable qos 

Re: [ovs-dev] [External] Re: [PATCH v5] utilities/ofctl: add-meters for save and restore

[Simon Horman]

Hi Wan Junjie,

On Tue, Apr 04, 2023 at 03:42:47PM +0800, Wan Junjie wrote:
> Hi Simon,
> 
> On Wed, Mar 29, 2023 at 11:57 PM Simon Horman  
> wrote:
> > On Fri, Mar 10, 2023 at 09:03:48PM +0800, Wan Junjie wrote:

...

> > > @@ -805,5 +831,73 @@ ofputil_format_meter_mod(struct ds *s, const struct 
> > > ofputil_meter_mod *mm)
> > >  ds_put_format(s, " cmd:%d ", mm->command);
> > >  }
> > >
> > > -ofputil_format_meter_config(s, >meter);
> > > +ofputil_format_meter_config(s, >meter, false);
> > > +}
> > > +
> > > +/* If 'command' is given as -2, each line may start with a command name 
> > > ("add",
> > > + * "modify", "delete").  A missing command name is treated as "add".
> > > + */
> > > +char * OVS_WARN_UNUSED_RESULT
> > > +parse_ofp_meter_mod_file(const char *file_name,
> > > + int command,
> > > + struct ofputil_meter_mod **mms, size_t *n_mms,
> > > + enum ofputil_protocol *usable_protocols)
> > > +{
> >
> > This appears to largely duplicate parse_ofp_group_mod_file().
> > Could shared code be used?
> >
> They don't share structures like ofputil_meter_mod. An option is to
> union them or make a generic function with void pointer.
> But this may be a bad idea, like parse_ofp_group_mod_file did not
> reuse code from parse_ofp_flow_mod_file, we may change
> meter's structure in the future. Then the functions will diff a lot
> and be hard to manage in one function.
> If we want to make all the ofp structure reuse a common code then a
> huge refactor to the ofp level will be needed.

Understood. I do think there would be some value in consolidating things.
But I accept that it would be complex. And can be considered out of scope
for this patchset.

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

Re: [ovs-dev] [PATCH ovn] northd: rever ct.inv drop flows

[Simon Horman]

On Tue, Apr 04, 2023 at 08:13:05PM +0200, Lorenzo Bianconi wrote:
> Partially revert the following commit since it introduces a regression
> when we want to directly connect to a backend ip from a client outside
> the cluster for gw-router-port scenario.
> 
> commit e3bc68c3be6967916674119b14fe2bef081ac6ad
> Author: Lorenzo Bianconi 
> Date:   Mon Mar 20 19:30:13 2023 +0100
> 
> northd: drop ct.inv packets in post snat and lb_aff_learn stages
> 
> Drop ip packets with ct status set to invalid in post snat and
> lb_aff_learn router stages.
> Skip ICMPv{4,6} error messages packet in ct.inv rules in order to
> avoid to introduce too complicated code.
> 
> Even if the issue is not directly introduced by the commit above, it is
> not easy to fix it without committing all IP traffic to connection
> tracking or adding a flow per load-balancer backend.
> 
> Signed-off-by: Lorenzo Bianconi 

Reviewed-by: Simon Horman 

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

Re: [ovs-dev] [PATCH ovn] northd: take into account qos_min_rate in port_has_qos_params

[Simon Horman]

On Tue, Apr 04, 2023 at 11:01:26PM +0200, Lorenzo Bianconi wrote:
> Similar to qos_max_rate and qos_burst, take into account qos_min_rate in
> port_has_qos_params routine.
> 
> Fixes: dbf12e5fe1f7 ("qos: add support for port minimum bandwidth guarantee")
> Signed-off-by: Lorenzo Bianconi 

Hi Lorenzo,

I am unsure if it is related to this change, or perhaps a transient error,
but the ovn-kubernetes workflow run failed when run against this patch.

https://patchwork.ozlabs.org/project/ovn/patch/48d88894b808e03bbcc513b8793bc92d85eea657.1680642050.git.lorenzo.bianc...@redhat.com/

FWIIW, I am experimenting with re-running the workflow here:

https://github.com/horms/ovn/actions/runs/4618557664
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1 0/2] Add netdev-dpdk support for ingress and egress pkts policer.

[Simon Horman]

On Tue, Apr 04, 2023 at 01:46:05PM +, lin huang wrote:
> Hi Ilya,
> 
> Pls review my code. 
> 
> I want to add a new policer which is pps-based in netdev-dpdk module.
> The policer is divided into ingress and egress part. Both use the ovs native 
> tocken bucket library as the counter.
> Compared to bandwidth-based policers, the pps-based policer is more effective 
> at combating low-and-slow types of DDoS attacks.
> 
> This new ingress policer is configured using the "policer_kpkts_rate" and 
> "policer_kpkts_burst" configuration statement. Here is the example.
> ovs-vsctl set interface dpdk0 policer_kpkts_rate=2
> ovs-vsctl set interface dpdk0 policer_kpkts_burst=2
> 
> This new egress policer is configured using the " egress-pkts-policer " 
> configuration statement. Here is the example.
> ovs-vsctl set port vhost-user0 qos=@newqos -- \
> --id=@newqos create qos type=egress-pkts-policer 
> other-config:pkts_rate=2048 \
> other-config:pkts_burst=2048`

I have some high level questions about this.

1. For ingress there is, as you mention above,
   already options at the ovs-vsctl for port-based PPS policing.

   Did you consider making the egress options you are adding consistent
   with the existing ingress options?
   f.e.  egress_policing_kpkts_rate and egress_policing_kpkts_rate.

2. Metering operates on egress, is a policer, and can support PPS.
   Did you consider using this instead of adding a new egress policer?

   There is an upstreamed implementation of this for upstreamed
   for the kernel datapath with and without TC HW offload.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH net v3] net: openvswitch: fix race on port output

[Felix Huettner via dev]

assume the following setup on a single machine:
1. An openvswitch instance with one bridge and default flows
2. two network namespaces "server" and "client"
3. two ovs interfaces "server" and "client" on the bridge
4. for each ovs interface a veth pair with a matching name and 32 rx and
   tx queues
5. move the ends of the veth pairs to the respective network namespaces
6. assign ip addresses to each of the veth ends in the namespaces (needs
   to be the same subnet)
7. start some http server on the server network namespace
8. test if a client in the client namespace can reach the http server

when following the actions below the host has a chance of getting a cpu
stuck in a infinite loop:
1. send a large amount of parallel requests to the http server (around
   3000 curls should work)
2. in parallel delete the network namespace (do not delete interfaces or
   stop the server, just kill the namespace)

there is a low chance that this will cause the below kernel cpu stuck
message. If this does not happen just retry.
Below there is also the output of bpftrace for the functions mentioned
in the output.

The series of events happening here is:
1. the network namespace is deleted calling
   `unregister_netdevice_many_notify` somewhere in the process
2. this sets first `NETREG_UNREGISTERING` on both ends of the veth and
   then runs `synchronize_net`
3. it then calls `call_netdevice_notifiers` with `NETDEV_UNREGISTER`
4. this is then handled by `dp_device_event` which calls
   `ovs_netdev_detach_dev` (if a vport is found, which is the case for
   the veth interface attached to ovs)
5. this removes the rx_handlers of the device but does not prevent
   packages to be sent to the device
6. `dp_device_event` then queues the vport deletion to work in
   background as a ovs_lock is needed that we do not hold in the
   unregistration path
7. `unregister_netdevice_many_notify` continues to call
   `netdev_unregister_kobject` which sets `real_num_tx_queues` to 0
8. port deletion continues (but details are not relevant for this issue)
9. at some future point the background task deletes the vport

If after 7. but before 9. a packet is send to the ovs vport (which is
not deleted at this point in time) which forwards it to the
`dev_queue_xmit` flow even though the device is unregistering.
In `skb_tx_hash` (which is called in the `dev_queue_xmit`) path there is
a while loop (if the packet has a rx_queue recorded) that is infinite if
`dev->real_num_tx_queues` is zero.

To prevent this from happening we update `do_output` to handle devices
without carrier the same as if the device is not found (which would
be the code path after 9. is done).

Additionally we now produce a warning in `skb_tx_hash` if we will hit
the inifinite loop.

bpftrace (first word is function name):

__dev_queue_xmit server: real_num_tx_queues: 1, cpu: 2, pid: 28024, tid: 28024, 
skb_addr: 0x9edb6f207000, reg_state: 1
netdev_core_pick_tx server: addr: 0x9f0a46d4a000 real_num_tx_queues: 1, 
cpu: 2, pid: 28024, tid: 28024, skb_addr: 0x9edb6f207000, reg_state: 1
dp_device_event server: real_num_tx_queues: 1 cpu 9, pid: 21024, tid: 21024, 
event 2, reg_state: 1
synchronize_rcu_expedited: cpu 9, pid: 21024, tid: 21024
synchronize_rcu_expedited: cpu 9, pid: 21024, tid: 21024
synchronize_rcu_expedited: cpu 9, pid: 21024, tid: 21024
synchronize_rcu_expedited: cpu 9, pid: 21024, tid: 21024
dp_device_event server: real_num_tx_queues: 1 cpu 9, pid: 21024, tid: 21024, 
event 6, reg_state: 2
ovs_netdev_detach_dev server: real_num_tx_queues: 1 cpu 9, pid: 21024, tid: 
21024, reg_state: 2
netdev_rx_handler_unregister server: real_num_tx_queues: 1, cpu: 9, pid: 21024, 
tid: 21024, reg_state: 2
synchronize_rcu_expedited: cpu 9, pid: 21024, tid: 21024
netdev_rx_handler_unregister ret server: real_num_tx_queues: 1, cpu: 9, pid: 
21024, tid: 21024, reg_state: 2
dp_device_event server: real_num_tx_queues: 1 cpu 9, pid: 21024, tid: 21024, 
event 27, reg_state: 2
dp_device_event server: real_num_tx_queues: 1 cpu 9, pid: 21024, tid: 21024, 
event 22, reg_state: 2
dp_device_event server: real_num_tx_queues: 1 cpu 9, pid: 21024, tid: 21024, 
event 18, reg_state: 2
netdev_unregister_kobject: real_num_tx_queues: 1, cpu: 9, pid: 21024, tid: 21024
synchronize_rcu_expedited: cpu 9, pid: 21024, tid: 21024
ovs_vport_send server: real_num_tx_queues: 0, cpu: 2, pid: 28024, tid: 28024, 
skb_addr: 0x9edb6f207000, reg_state: 2
__dev_queue_xmit server: real_num_tx_queues: 0, cpu: 2, pid: 28024, tid: 28024, 
skb_addr: 0x9edb6f207000, reg_state: 2
netdev_core_pick_tx server: addr: 0x9f0a46d4a000 real_num_tx_queues: 0, 
cpu: 2, pid: 28024, tid: 28024, skb_addr: 0x9edb6f207000, reg_state: 2
broken device server: real_num_tx_queues: 0, cpu: 2, pid: 28024, tid: 28024
ovs_dp_detach_port server: real_num_tx_queues: 0 cpu 9, pid: 9124, tid: 9124, 
reg_state: 2
synchronize_rcu_expedited: cpu 9, pid: 33604, tid: 33604

stuck message:

watchdog: BUG: soft lockup - CPU#5 stuck 

Re: [ovs-dev] [RFC ovn 0/2] ACL Sampling using per-flow IPFIX

[Adrian Moreno]

On 3/30/23 12:03, Dumitru Ceara wrote:

On 3/21/23 10:48, Adrian Moreno wrote:



On 3/17/23 20:59, Numan Siddique wrote:

On Tue, Oct 18, 2022 at 12:00 PM Adrian Moreno 
wrote:


Based on the introduction of the OVN "sample" action (still WIP) [1],
the proposal of this RFC is to use per-flow IPFIX sampling to increase
visibility on ACLs.

The idea of ACL sampling is very similar to the already existing ACL
logging whith the following key differences:

- Using IPFIX sampling collects header information of the actual packet
    that was dropped / accepted by the ACL. This information is key to
    debug an issue or understand the traffic profile that traverses the
    ACLs.

- With ACL logging, the information goes to the ovn-controller,
    adding pressure to it. Using IPFIX sampling can offload the
    ovn-controller by sending samples to external IPFIX collectors.

- Using the sample action, we don't need to rely on a meter to limit the
    amount of data we process since we have the sampling
rate/probability.

- Using IPFIX as standard format makes the solution interoperable so
    it's possible to combine with other IPFIX sources to build
    comprehensive observability tools.

This RFC includes a prototype implementation based on the creation of a
new NBDB table "Sample" and a reference to it from the ACL table. This
would allow the use of per-flow IPFIX sampling to add visibility to
other areas of OVN as the needs arise.

[1]
https://patchwork.ozlabs.org/project/ovn/patch/20221017131403.563877-2-amore...@redhat.com/


Adrian Moreno (2):
    northd: add ACL Sampling
    ovn-nbctl: add sample to acl-add


Hi Adrian,

Do you plan to submit formal patches ?  Or you're expecting any
feedback on this series before submitting formally ?

If so,  I can take a look at the rfc patches.



Hi Numan,

I am planning to do some performance benchmarking and add it to the
formal patch but I would love to get some general feedback on the topic.
Whether the approach seems sane (adding sample actions in ACL lflows),
whether the general NBDB API is going in the proper direction or if
there is some pitfall I'm ignoring. Of course I'm not asking for a full
review but a general go/no-go would be nice.


Hi Adrian,

I'm not sure if Numan has other comments.  FWIW I had a look at the two
patches in the series and I think the direction is ok.  I did share some
comments on patch 1/2.

Thanks for working on this!



Thank you for your comments and feedback.



Regards,
Dumitru



--
Adrián Moreno

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

Re: [ovs-dev] [RFC ovn 1/2] northd: add ACL Sampling

[Adrian Moreno]

Thanks for the comments Dumitru!

On 3/30/23 12:02, Dumitru Ceara wrote:

On 10/18/22 17:59, Adrian Moreno wrote:

Introduce a new table called Sample where per-flow IPFIX configuration
can be specified.
Also, reference rows from such table from the ACL table to enable the
configuration of ACL sampling. If enabled, northd will add a sample
action to each ACL-related logical flow.

Signed-off-by: Adrian Moreno 
---
  northd/northd.c  | 31 ++-
  ovn-nb.ovsschema | 23 ++-
  ovn-nb.xml   | 31 +++
  3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 61d474840..3e09e3a0f 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -6194,6 +6194,27 @@ build_acl_log(struct ds *actions, const struct nbrec_acl 
*acl,
  ds_put_cstr(actions, "); ");
  }
  
+static void

+build_acl_sample(struct ds *actions, const struct nbrec_acl *acl)
+{
+if (!acl->sample) {
+return;
+}
+ds_put_format(actions, "sample(probability=%"PRId16","
+   "collector_set=%d,"
+   "obs_domain=%hd,",
+(uint16_t) acl->sample->probability,
+(uint32_t) acl->sample->collector_set_id,
+(uint8_t) acl->sample->obs_domain_id);
+
+if (acl->sample->obs_point_id) {
+ds_put_format(actions, "obs_point=%"PRId32");",
+  (uint32_t) *acl->sample->obs_point_id);
+} else {
+ds_put_cstr(actions, "obs_point=$cookie);");
+}
+}
+
  static void
  build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
 enum ovn_stage stage, struct nbrec_acl *acl,
@@ -6260,6 +6281,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
  if (!strcmp(acl->action, "allow-stateless")) {
  ds_clear(actions);
  build_acl_log(actions, acl, meter_groups);
+build_acl_sample(actions, acl);
  ds_put_cstr(actions, "next;");
  ovn_lflow_add_with_hint(lflows, od, stage,
  acl->priority + OVN_ACL_PRI_OFFSET,
@@ -6275,6 +6297,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
  if (!has_stateful) {
  ds_clear(actions);
  build_acl_log(actions, acl, meter_groups);
+build_acl_sample(actions, acl);
  ds_put_cstr(actions, "next;");
  ovn_lflow_add_with_hint(lflows, od, stage,
  acl->priority + OVN_ACL_PRI_OFFSET,
@@ -6304,6 +6327,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
REG_LABEL" = %"PRId64"; ", acl->label);
  }
  build_acl_log(actions, acl, meter_groups);
+build_acl_sample(actions, acl);
  ds_put_cstr(actions, "next;");
  ovn_lflow_add_with_hint(lflows, od, stage,
  acl->priority + OVN_ACL_PRI_OFFSET,
@@ -6329,6 +6353,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
REG_LABEL" = %"PRId64"; ", acl->label);
  }
  build_acl_log(actions, acl, meter_groups);
+build_acl_sample(actions, acl);
  ds_put_cstr(actions, "next;");
  ovn_lflow_add_with_hint(lflows, od, stage,
  acl->priority + OVN_ACL_PRI_OFFSET,
@@ -6349,7 +6374,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
   */
  bool log_related = smap_get_bool(>options, "log-related",
   false);
-if (acl->log && acl->label && log_related) {
+if ((acl->log || acl->sample) && acl->label && log_related) {
  /* Related/reply flows need to be set on the opposite pipeline
   * from where the ACL itself is set.
   */
@@ -6365,6 +6390,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
use_ct_inv_match ? " && !ct.inv" : "",
ct_blocked_match, acl->label);
  build_acl_log(actions, acl, meter_groups);
+build_acl_sample(actions, acl);
  ds_put_cstr(actions, "next;");
  ovn_lflow_add_with_hint(lflows, od, log_related_stage,
  UINT16_MAX - 2,
@@ -6402,6 +6428,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
  } else {
  ds_put_format(match, " && (%s)", acl->match);
  build_acl_log(actions, acl, meter_groups);
+build_acl_sample(actions, acl);
  ds_put_cstr(actions, debug_implicit_drop_action());
  ovn_lflow_add_with_hint(lflows, od, stage,
  acl->priority