Re: [ovs-dev] [PATCH] docker: add testing on local docker localhost:5000

[Ginwala, Aliasgar via dev]

Sure Ben.

Thanks Hunchback for the patches. Added few comments/queries to that.

From: Ben Pfaff 
Date: Thursday, April 8, 2021 at 9:06 AM
To: hunchback , Ginwala, Aliasgar 
, Han Zhou , Numan Siddique 

Cc: ovs 
Subject: Re: [ovs-dev] [PATCH] docker: add testing on local docker 
localhost:5000
External Email

Hi Ali and Han, you guys wrote the docker stuff in OVS, and Numan, you
reviewed it.  Do you think you could comment on these proposed changes
from Aidan/hunchback?  I am not competent in Docker myself.

On Fri, Apr 02, 2021 at 07:27:00PM +, hunchback wrote:
> add a set of docker targets which ease the building and testing of
> docker images both on local registry (localhost:5000) as well as on the
> public default registry (docker.io).
>
> Signed-off-by: hunchback 
> ---
>  .gitignore  |  1 +
>  Documentation/intro/install/general.rst | 38 +-
>  Makefile.am |  1 +
>  utilities/automake.mk   |  2 +-
>  utilities/docker/.gitignore |  2 +
>  utilities/docker/Makefile   | 22 --
>  utilities/docker/automake.mk| 53 +
>  utilities/docker/debian/Dockerfile  |  5 ++-
>  8 files changed, 79 insertions(+), 45 deletions(-)
>  create mode 100644 utilities/docker/.gitignore
>  delete mode 100644 utilities/docker/Makefile
>  create mode 100644 utilities/docker/automake.mk
>
> diff --git a/.gitignore b/.gitignore
> index f1cdcf124..b46a0528a 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -79,3 +79,4 @@ testsuite.tmp.orig
>  /Documentation/_build
>  /.venv
>  /cxx-check
> +*.mod
> diff --git a/Documentation/intro/install/general.rst 
> b/Documentation/intro/install/general.rst
> index c4300cd53..8a3609627 100644
> --- a/Documentation/intro/install/general.rst
> +++ b/Documentation/intro/install/general.rst
> @@ -510,40 +510,38 @@ For ovs vswitchd, we need to load ovs kernel modules on 
> host.
>
>  Hence, OVS containers kernel version needs to be same as that of host kernel.
>
> -Export following variables in .env  and place it under
> -project root::
> +If you want to change the default values for building an image then set these
> +variables::
>
> -$ OVS_BRANCH=
> -$ OVS_VERSION=
> -$ DISTRO=
> -$ KERNEL_VERSION=
> -$ GITHUB_SRC=
> -$ DOCKER_REPO=
> +$ export OVS_BRANCH=
> +$ export OVS_VERSION=
> +$ export DISTRO=
> +$ export KERNEL_VERSION=
> +$ export GITHUB_SRC=
> +$ export DOCKER_REPO=
> +$ export DOCKER_TAG=
>
> -To build ovs modules::
> +To setup for using a local registry (localhost:5000)::
>
> -$ cd utilities/docker
> -$ make build
> +$ make docker-registry
> +$ export DOCKER_REPO=localhost:5000/ovsvswitch/ovs
>
> -Compiled Modules will be tagged with docker image
> +To build ovs modules (tagged with docker image)::
>
> -To Push ovs modules::
> +$ make docker-build
Docker is default container engine that is already being used so why need to 
rename.
Not sure if you are planning to add another container run time support ?

>
> -$ make push
> +To push ovs modules to docker repo::
>
> -OVS docker image will be pushed to specified docker repo.
> + $ make docker-push
>
>  Start ovsdb-server using below command::
>
> -$ docker run -itd --net=host --name=ovsdb-server \
> -  : ovsdb-server
> + $ make docker-ovsdb-server
>
>  Start ovs-vswitchd with priviledged mode as it needs to load kernel module in
>  host using below command::
>
> -$ docker run -itd --net=host --name=ovs-vswitchd \
> -  --volumes-from=ovsdb-server -v /lib:/lib --privileged \
> -  : ovs-vswitchd
> +$ make docker-ovs-vswitchd
The intent of make is to compile/build the binary/image.  I don’t get the 
intent of running the binary using docker itself in a make file.
May be you can add new instructions say make run xxx if you still want to 
bundle docker run in make?
>
>  .. note::
>  The debian docker file uses ubuntu 16.04 as a base image for reference.
> diff --git a/Makefile.am b/Makefile.am
> index 691a005ad..e75df1be6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -499,3 +499,4 @@ include datapath-windows/include/automake.mk
>  include windows/automake.mk
>  include selinux/automake.mk
>  include build-aux/automake.mk
> +include utilities/docker/automake.mk
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index e2e22c39a..4d2f20570 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -56,7 +56,7 @@ EXTRA_DIST += \
>utilities/ovs-vlan-test.in \
>utilities/ovs-vsctl-bashcomp.bash \
>utilities/checkpatch.py \
> -utilities/docker/Makefile \
> +utilities/docker/automake.mk \
>  utilities/docker/ovs-override.conf \
>  utilities/docker/start-ovs \
>  utilities/docker/create_ovs_db.sh \
> diff --git a/utilities/docker/.gitignore b/utilities/docker/.gitignore
> new file mode 10

[ovs-dev] [PATCH v2] documentation: add missing sudo to intro install

[Aidan Shribman]

The current documentation assumes everything is run from within root
what is not correct in the general case, the revised install
documentation makes use of `sudo` for specific commands which require to
be run under root permissions.

Signed-off-by: Aidan Shribman 
---
 Documentation/intro/install/general.rst | 46 ++---
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index c4300cd53..28b4bd00a 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -133,7 +133,8 @@ following:
 - A kernel build directory corresponding to the Linux kernel image the module
   is to run on. Under Debian and Ubuntu, for example, each linux-image package
   containing a kernel binary has a corresponding linux-headers package with
-  the required build infrastructure.
+  the required build infrastructure, so the name of the package corresponding
+  with your running kernel is ``linux-headers-$(uname -r)``.
 
 If you are working from a Git tree or snapshot (instead of from a distribution
 tarball), or if you modify the Open vSwitch build system or the database
@@ -385,14 +386,14 @@ Building
 2. Consider running the testsuite. Refer to :doc:`/topics/testing` for
instructions.
 
-3. Run ``make install`` to install the executables and manpages into the
+3. Run ``sudo make install`` to install the executables and manpages into the
running system, by default under ``/usr/local``::
 
-   $ make install
+   $ sudo make install
 
 5. If you built kernel modules, you may install them, e.g.::
 
-   $ make modules_install
+   $ sudo make modules_install
 
It is possible that you already had a Open vSwitch kernel module installed
on your machine that came from upstream Linux (in a different directory). To
@@ -404,10 +405,12 @@ Building
$ config_file="/etc/depmod.d/openvswitch.conf"
$ for module in datapath/linux/*.ko; do
  modname="$(basename ${module})"
- echo "override ${modname%.ko} * extra" >> "$config_file"
- echo "override ${modname%.ko} * weak-updates" >> "$config_file"
+ sudo echo "override ${modname%.ko} * extra" \
+ | tee -a "$config_file" >/dev/null
+ sudo echo "override ${modname%.ko} * weak-updates" \
+ | tee -a "$config_file" >/dev/null
  done
-   $ depmod -a
+   $ sudo depmod -a
 
Finally, load the kernel modules that you need. e.g.::
 
@@ -450,18 +453,18 @@ utility is located in '$(pkgdatadir)/scripts', and 
defaults to
 '/usr/local/share/openvswitch/scripts'.  An example after install might be::
 
 $ export PATH=$PATH:/usr/local/share/openvswitch/scripts
-$ ovs-ctl start
+$ sudo ovs-ctl start
 
 Additionally, the ovs-ctl script allows starting / stopping the daemons
 individually using specific options.  To start just the ovsdb-server::
 
 $ export PATH=$PATH:/usr/local/share/openvswitch/scripts
-$ ovs-ctl --no-ovs-vswitchd start
+$ sudo ovs-ctl --no-ovs-vswitchd start
 
 Likewise, to start just the ovs-vswitchd::
 
 $ export PATH=$PATH:/usr/local/share/openvswitch/scripts
-$ ovs-ctl --no-ovsdb-server start
+$ sudo ovs-ctl --no-ovsdb-server start
 
 Refer to ovs-ctl(8) for more information on ovs-ctl.
 
@@ -472,16 +475,16 @@ machine on which Open vSwitch is installed should run its 
own copy of
 ovsdb-server. Before ovsdb-server itself can be started, configure a
 database that it can use::
 
-   $ mkdir -p /usr/local/etc/openvswitch
-   $ ovsdb-tool create /usr/local/etc/openvswitch/conf.db \
+   $ sudo mkdir -p /usr/local/etc/openvswitch
+   $ sudo ovsdb-tool create /usr/local/etc/openvswitch/conf.db \
vswitchd/vswitch.ovsschema
 
 Configure ovsdb-server to use database created above, to listen on a Unix
 domain socket, to connect to any managers specified in the database itself, and
 to use the SSL configuration in the database::
 
-$ mkdir -p /usr/local/var/run/openvswitch
-$ ovsdb-server --remote=punix:/usr/local/var/run/openvswitch/db.sock \
+$ sudo mkdir -p /usr/local/var/run/openvswitch
+$ sudo ovsdb-server --remote=punix:/usr/local/var/run/openvswitch/db.sock \
 --remote=db:Open_vSwitch,Open_vSwitch,manager_options \
 --private-key=db:Open_vSwitch,SSL,private_key \
 --certificate=db:Open_vSwitch,SSL,certificate \
@@ -496,12 +499,12 @@ Initialize the database using ovs-vsctl. This is only 
necessary the first time
 after you create the database with ovsdb-tool, though running it at any time is
 harmless::
 
-$ ovs-vsctl --no-wait init
+$ sudo ovs-vsctl --no-wait init
 
 Start the main Open vSwitch daemon, telling it to connect to the same Unix
 domain socket::
 
-$ ovs-vswitchd --pidfile --detach --log-file
+$ sudo ovs-vswitchd --pidfile --detach --log-file
 
 Starting OVS in container
 -
@@ 

Re: [ovs-dev] [PATCH v2] documentation: add missing sudo to intro install

[0-day Robot]

Bleep bloop.  Greetings Aidan Shribman, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 87 characters long (recommended limit is 79)
#54 FILE: Documentation/intro/install/general.rst:408:
 sudo echo "override ${modname%.ko} * extra" | tee -a "$config_file" 
>/dev/null

WARNING: Line is 94 characters long (recommended limit is 79)
#55 FILE: Documentation/intro/install/general.rst:409:
 sudo echo "override ${modname%.ko} * weak-updates" | tee -a 
"$config_file" >/dev/null

WARNING: Line is 95 characters long (recommended limit is 79)
#138 FILE: Documentation/intro/install/general.rst:587:
   $ sudo kill `cd /usr/local/var/run/openvswitch && cat ovsdb-server.pid 
ovs-vswitchd.pid`

Lines checked: 153, Warnings: 3, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] docker: add testing on local docker localhost:5000

[Aidan Shribman]

Add a set of docker targets which ease the building and testing of
docker images both on local registry (localhost:5000) as well as on the
public default registry (docker.io).

Signed-off-by: Aidan Shribman 
---
 .gitignore  |  1 +
 Documentation/intro/install/general.rst | 31 +--
 Makefile.am |  1 +
 utilities/automake.mk   |  2 +-
 utilities/docker/.gitignore |  2 ++
 utilities/docker/Makefile   | 22 --
 utilities/docker/automake.mk| 40 +
 utilities/docker/debian/Dockerfile  |  5 ++--
 8 files changed, 64 insertions(+), 40 deletions(-)
 create mode 100644 utilities/docker/.gitignore
 delete mode 100644 utilities/docker/Makefile
 create mode 100644 utilities/docker/automake.mk

diff --git a/.gitignore b/.gitignore
index f1cdcf124..b46a0528a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -79,3 +79,4 @@ testsuite.tmp.orig
 /Documentation/_build
 /.venv
 /cxx-check
+*.mod
diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index c4300cd53..804b7af52 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -510,28 +510,29 @@ For ovs vswitchd, we need to load ovs kernel modules on 
host.
 
 Hence, OVS containers kernel version needs to be same as that of host kernel.
 
-Export following variables in .env  and place it under
-project root::
+If you want to change the default values for building an image then set these
+variables::
 
-$ OVS_BRANCH=
-$ OVS_VERSION=
-$ DISTRO=
-$ KERNEL_VERSION=
-$ GITHUB_SRC=
-$ DOCKER_REPO=
+$ export OVS_BRANCH=
+$ export OVS_VERSION=
+$ export DISTRO=
+$ export KERNEL_VERSION=
+$ export GITHUB_SRC=
+$ export DOCKER_REPO=
+$ export DOCKER_TAG=
 
-To build ovs modules::
+To setup for using a local registry (localhost:5000)::
 
-$ cd utilities/docker
-$ make build
+$ make docker-registry
+$ export DOCKER_REPO=localhost:5000/ovsvswitch/ovs
 
-Compiled Modules will be tagged with docker image
+To build ovs modules (tagged with docker image)::
 
-To Push ovs modules::
+$ make docker-build
 
-$ make push
+To push ovs modules to docker repo::
 
-OVS docker image will be pushed to specified docker repo.
+ $ make docker-push
 
 Start ovsdb-server using below command::
 
diff --git a/Makefile.am b/Makefile.am
index cb8076433..5dcec201d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -499,3 +499,4 @@ include datapath-windows/include/automake.mk
 include windows/automake.mk
 include selinux/automake.mk
 include build-aux/automake.mk
+include utilities/docker/automake.mk
diff --git a/utilities/automake.mk b/utilities/automake.mk
index e2e22c39a..4d2f20570 100644
--- a/utilities/automake.mk
+++ b/utilities/automake.mk
@@ -56,7 +56,7 @@ EXTRA_DIST += \
utilities/ovs-vlan-test.in \
utilities/ovs-vsctl-bashcomp.bash \
utilities/checkpatch.py \
-utilities/docker/Makefile \
+utilities/docker/automake.mk \
 utilities/docker/ovs-override.conf \
 utilities/docker/start-ovs \
 utilities/docker/create_ovs_db.sh \
diff --git a/utilities/docker/.gitignore b/utilities/docker/.gitignore
new file mode 100644
index 0..5d05c158b
--- /dev/null
+++ b/utilities/docker/.gitignore
@@ -0,0 +1,2 @@
+vswitch.ovsschema
+ovsdb-tool
diff --git a/utilities/docker/Makefile b/utilities/docker/Makefile
deleted file mode 100644
index d8b08a3c9..0
--- a/utilities/docker/Makefile
+++ /dev/null
@@ -1,22 +0,0 @@
-#export OVS_BRANCH=branch-2.11
-#export OVS_VERSION=2.11
-#export KERNEL_VERSION=4.15.0-54-generic
-#export DISTRO=debian
-#export GITHUB_SRC=https://github.com/openvswitch/ovs.git
-#export DOCKER_REPO=openvswitch/ovs
-
-# Example:
-#   make build
-#   make push
-
-REPO = ${DOCKER_REPO}
-tag = ${OVS_VERSION}_${DISTRO}_${KERNEL_VERSION}
-
-build: ;docker build -t ${REPO}:${tag} --build-arg DISTRO=${DISTRO} \
---build-arg OVS_BRANCH=${OVS_BRANCH} \
---build-arg KERNEL_VERSION=${KERNEL_VERSION} \
---build-arg GITHUB_SRC=${GITHUB_SRC} -f ${DISTRO}/Dockerfile .
-
-.PHONY: build
-
-push: ;docker push ${REPO}:${tag}
diff --git a/utilities/docker/automake.mk b/utilities/docker/automake.mk
new file mode 100644
index 0..4d9385ac8
--- /dev/null
+++ b/utilities/docker/automake.mk
@@ -0,0 +1,40 @@
+DIR := utilities/docker
+
+OVS_BRANCH ?= branch-2.11
+OVS_VERSION ?= 2.11
+KERNEL_VERSION ?= 4.15.0-54-generic
+DISTRO ?= debian
+GITHUB_SRC ?= https://github.com/openvswitch/ovs.git
+DOCKER_REPO ?= openvswitch/ovs
+DOCKER_TAG ?= ${OVS_VERSION}_${DISTRO}_${KERNEL_VERSION}
+DOCKER_SERVER ?= localhost:5000
+
+.PHONY: docker-registry
+docker-registry:
+   docker rm --force registry 2>/dev/null || true
+   docker run -d -p 5000:5000 --restart=always --name registry registry:2
+   @echo
+   @echo "# For using local repo

[ovs-dev] [PATCH v2] documentation: add missing sudo to intro install

[Aidan Shribman]

The current documentation assumes everything is run from within root
what is not correct in the general case, the revised install
documentation makes use of `sudo` for specific commands which require to
be run under root permissions.

Signed-off-by: Aidan Shribman 
---
 Documentation/intro/install/general.rst | 43 +
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index c4300cd53..8784162df 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -133,7 +133,8 @@ following:
 - A kernel build directory corresponding to the Linux kernel image the module
   is to run on. Under Debian and Ubuntu, for example, each linux-image package
   containing a kernel binary has a corresponding linux-headers package with
-  the required build infrastructure.
+  the required build infrastructure, so the name of the package corresponding
+  with your running kernel is ``linux-headers-$(uname -r)``.
 
 If you are working from a Git tree or snapshot (instead of from a distribution
 tarball), or if you modify the Open vSwitch build system or the database
@@ -385,14 +386,14 @@ Building
 2. Consider running the testsuite. Refer to :doc:`/topics/testing` for
instructions.
 
-3. Run ``make install`` to install the executables and manpages into the
+3. Run ``sudo make install`` to install the executables and manpages into the
running system, by default under ``/usr/local``::
 
-   $ make install
+   $ sudo make install
 
 5. If you built kernel modules, you may install them, e.g.::
 
-   $ make modules_install
+   $ sudo make modules_install
 
It is possible that you already had a Open vSwitch kernel module installed
on your machine that came from upstream Linux (in a different directory). To
@@ -404,10 +405,10 @@ Building
$ config_file="/etc/depmod.d/openvswitch.conf"
$ for module in datapath/linux/*.ko; do
  modname="$(basename ${module})"
- echo "override ${modname%.ko} * extra" >> "$config_file"
- echo "override ${modname%.ko} * weak-updates" >> "$config_file"
+ sudo echo "override ${modname%.ko} * extra" | tee -a "$config_file" 
>/dev/null
+ sudo echo "override ${modname%.ko} * weak-updates" | tee -a 
"$config_file" >/dev/null
  done
-   $ depmod -a
+   $ sudo depmod -a
 
Finally, load the kernel modules that you need. e.g.::
 
@@ -450,18 +451,18 @@ utility is located in '$(pkgdatadir)/scripts', and 
defaults to
 '/usr/local/share/openvswitch/scripts'.  An example after install might be::
 
 $ export PATH=$PATH:/usr/local/share/openvswitch/scripts
-$ ovs-ctl start
+$ sudo ovs-ctl start
 
 Additionally, the ovs-ctl script allows starting / stopping the daemons
 individually using specific options.  To start just the ovsdb-server::
 
 $ export PATH=$PATH:/usr/local/share/openvswitch/scripts
-$ ovs-ctl --no-ovs-vswitchd start
+$ sudo ovs-ctl --no-ovs-vswitchd start
 
 Likewise, to start just the ovs-vswitchd::
 
 $ export PATH=$PATH:/usr/local/share/openvswitch/scripts
-$ ovs-ctl --no-ovsdb-server start
+$ sudo ovs-ctl --no-ovsdb-server start
 
 Refer to ovs-ctl(8) for more information on ovs-ctl.
 
@@ -472,16 +473,16 @@ machine on which Open vSwitch is installed should run its 
own copy of
 ovsdb-server. Before ovsdb-server itself can be started, configure a
 database that it can use::
 
-   $ mkdir -p /usr/local/etc/openvswitch
-   $ ovsdb-tool create /usr/local/etc/openvswitch/conf.db \
+   $ sudo mkdir -p /usr/local/etc/openvswitch
+   $ sudo ovsdb-tool create /usr/local/etc/openvswitch/conf.db \
vswitchd/vswitch.ovsschema
 
 Configure ovsdb-server to use database created above, to listen on a Unix
 domain socket, to connect to any managers specified in the database itself, and
 to use the SSL configuration in the database::
 
-$ mkdir -p /usr/local/var/run/openvswitch
-$ ovsdb-server --remote=punix:/usr/local/var/run/openvswitch/db.sock \
+$ sudo mkdir -p /usr/local/var/run/openvswitch
+$ sudo ovsdb-server --remote=punix:/usr/local/var/run/openvswitch/db.sock \
 --remote=db:Open_vSwitch,Open_vSwitch,manager_options \
 --private-key=db:Open_vSwitch,SSL,private_key \
 --certificate=db:Open_vSwitch,SSL,certificate \
@@ -496,12 +497,12 @@ Initialize the database using ovs-vsctl. This is only 
necessary the first time
 after you create the database with ovsdb-tool, though running it at any time is
 harmless::
 
-$ ovs-vsctl --no-wait init
+$ sudo ovs-vsctl --no-wait init
 
 Start the main Open vSwitch daemon, telling it to connect to the same Unix
 domain socket::
 
-$ ovs-vswitchd --pidfile --detach --log-file
+$ sudo ovs-vswitchd --pidfile --detach --log-file
 
 Starting OVS in container
 -
@@ -559,9 +560,9 @@ At th

Re: [ovs-dev] [PATCH] docker: add testing on local docker localhost:5000

[Aidan Shribman]

Thank you Ali, see my comments inline.

On Thu, Apr 8, 2021, 21:16 Ginwala, Aliasgar  wrote:

> Sure Ben.
>
>
>
> Thanks Hunchback for the patches. Added few comments/queries to that.
>
>
>
> *From: *Ben Pfaff 
> *Date: *Thursday, April 8, 2021 at 9:06 AM
> *To: *hunchback , Ginwala, Aliasgar <
> aginw...@ebay.com>, Han Zhou , Numan Siddique <
> nusid...@redhat.com>
> *Cc: *ovs 
> *Subject: *Re: [ovs-dev] [PATCH] docker: add testing on local docker
> localhost:5000
>
> External Email
>
> Hi Ali and Han, you guys wrote the docker stuff in OVS, and Numan, you
> reviewed it.  Do you think you could comment on these proposed changes
> from Aidan/hunchback?  I am not competent in Docker myself.
>
> On Fri, Apr 02, 2021 at 07:27:00PM +, hunchback wrote:
> > add a set of docker targets which ease the building and testing of
> > docker images both on local registry (localhost:5000) as well as on the
> > public default registry (docker.io).
> >
> > Signed-off-by: hunchback 
> > ---
> >  .gitignore  |  1 +
> >  Documentation/intro/install/general.rst | 38 +-
> >  Makefile.am |  1 +
> >  utilities/automake.mk   |  2 +-
> >  utilities/docker/.gitignore |  2 +
> >  utilities/docker/Makefile   | 22 --
> >  utilities/docker/automake.mk| 53 +
> >  utilities/docker/debian/Dockerfile  |  5 ++-
> >  8 files changed, 79 insertions(+), 45 deletions(-)
> >  create mode 100644 utilities/docker/.gitignore
> >  delete mode 100644 utilities/docker/Makefile
> >  create mode 100644 utilities/docker/automake.mk
> >
> > diff --git a/.gitignore b/.gitignore
> > index f1cdcf124..b46a0528a 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -79,3 +79,4 @@ testsuite.tmp.orig
> >  /Documentation/_build
> >  /.venv
> >  /cxx-check
> > +*.mod
> > diff --git a/Documentation/intro/install/general.rst
> b/Documentation/intro/install/general.rst
> > index c4300cd53..8a3609627 100644
> > --- a/Documentation/intro/install/general.rst
> > +++ b/Documentation/intro/install/general.rst
> > @@ -510,40 +510,38 @@ For ovs vswitchd, we need to load ovs kernel
> modules on host.
> >
> >  Hence, OVS containers kernel version needs to be same as that of host
> kernel.
> >
> > -Export following variables in .env  and place it under
> > -project root::
> > +If you want to change the default values for building an image then set
> these
> > +variables::
> >
> > -$ OVS_BRANCH=
> > -$ OVS_VERSION=
> > -$ DISTRO=
> > -$ KERNEL_VERSION=
> > -$ GITHUB_SRC=
> > -$ DOCKER_REPO=
> > +$ export OVS_BRANCH=
> > +$ export OVS_VERSION=
> > +$ export DISTRO=
> > +$ export KERNEL_VERSION=
> > +$ export GITHUB_SRC=
> > +$ export DOCKER_REPO=
> > +$ export DOCKER_TAG=
> >
> > -To build ovs modules::
> > +To setup for using a local registry (localhost:5000)::
> >
> > -$ cd utilities/docker
> > -$ make build
> > +$ make docker-registry
> > +$ export DOCKER_REPO=localhost:5000/ovsvswitch/ovs
> >
> > -Compiled Modules will be tagged with docker image
> > +To build ovs modules (tagged with docker image)::
> >
> > -To Push ovs modules::
> > +$ make docker-build
>
> Docker is default container engine that is already being used so why need
> to rename.
>
> Not sure if you are planning to add another container run time support ?
>

As these are top-level make targets i used the prefix "docker-" so that
"docker-build" is distinguished from "build" for instance.

>
> >
> > -$ make push
> > +To push ovs modules to docker repo::
> >
> > -OVS docker image will be pushed to specified docker repo.
> > + $ make docker-push
> >
> >  Start ovsdb-server using below command::
> >
> > -$ docker run -itd --net=host --name=ovsdb-server \
> > -  : ovsdb-server
> > + $ make docker-ovsdb-server
> >
> >  Start ovs-vswitchd with priviledged mode as it needs to load kernel
> module in
> >  host using below command::
> >
> > -$ docker run -itd --net=host --name=ovs-vswitchd \
> > -  --volumes-from=ovsdb-server -v /lib:/lib --privileged \
> > -  : ovs-vswitchd
> > +$ make docker-ovs-vswitchd
>
> The intent of make is to compile/build the binary/image.  I don’t get the
> intent of running the binary using docker itself in a make file.
>
ok, i will revert this change

> May be you can add new instructions say make run xxx if you still want to
> bundle docker run in make?
> >
> >  .. note::
> >  The debian docker file uses ubuntu 16.04 as a base image for
> reference.
> > diff --git a/Makefile.am b/Makefile.am
> > index 691a005ad..e75df1be6 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -499,3 +499,4 @@ include datapath-windows/include/automake.mk
> >  include windows/automake.mk
> >  include selinux/automake.mk
> >  include build-aux/automake.mk
> > +include utilities/docker/automake.mk
> > diff --git a/utilities/automake.mk 

Re: [ovs-dev] [PATCH] openvswitch: perform refragmentation for packets which pass through conntrack

[Aaron Conole]

Joe Stringer  writes:

> Hey Aaron, long time no chat :)

Same :)

> On Fri, Mar 19, 2021 at 1:43 PM Aaron Conole  wrote:
>>
>> When a user instructs a flow pipeline to perform connection tracking,
>> there is an implicit L3 operation that occurs - namely the IP fragments
>> are reassembled and then processed as a single unit.  After this, new
>> fragments are generated and then transmitted, with the hint that they
>> should be fragmented along the max rx unit boundary.  In general, this
>> behavior works well to forward packets along when the MTUs are congruent
>> across the datapath.
>>
>> However, if using a protocol such as UDP on a network with mismatching
>> MTUs, it is possible that the refragmentation will still produce an
>> invalid fragment, and that fragmented packet will not be delivered.
>> Such a case shouldn't happen because the user explicitly requested a
>> layer 3+4 function (conntrack), and that function generates new fragments,
>> so we should perform the needed actions in that case (namely, refragment
>> IPv4 along a correct boundary, or send a packet too big in the IPv6 case).
>>
>> Additionally, introduce a test suite for openvswitch with a test case
>> that ensures this MTU behavior, with the expectation that new tests are
>> added when needed.
>>
>> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
>> Signed-off-by: Aaron Conole 
>> ---
>> NOTE: checkpatch reports a whitespace error with the openvswitch.sh
>>   script - this is due to using tab as the IFS value.  This part
>>   of the script was copied from
>>   tools/testing/selftests/net/pmtu.sh so I think should be
>>   permissible.
>>
>>  net/openvswitch/actions.c  |   2 +-
>>  tools/testing/selftests/net/.gitignore |   1 +
>>  tools/testing/selftests/net/Makefile   |   1 +
>>  tools/testing/selftests/net/openvswitch.sh | 394 +
>>  4 files changed, 397 insertions(+), 1 deletion(-)
>>  create mode 100755 tools/testing/selftests/net/openvswitch.sh
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 92a0b67b2728..d858ea580e43 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -890,7 +890,7 @@ static void do_output(struct datapath *dp, struct 
>> sk_buff *skb, int out_port,
>> if (likely(!mru ||
>>(skb->len <= mru + vport->dev->hard_header_len))) 
>> {
>> ovs_vport_send(vport, skb, ovs_key_mac_proto(key));
>> -   } else if (mru <= vport->dev->mtu) {
>> +   } else if (mru) {
>> struct net *net = read_pnet(&dp->net);
>>
>> ovs_fragment(net, vport, skb, mru, key);
>
> I thought about this for a while. For a bit of context, my
> recollection is that in the initial design, there was an attempt to
> minimize the set of assumptions around L3 behaviour and despite
> performing this pseudo-L3 action of connection tracking, attempt a
> "bump-in-the-wire" approach where OVS is serving as an L2 switch and
> if you wanted L3 features, you need to build them on top or explicitly
> define that you're looking for L3 semantics. In this case, you're
> interpreting that the combination of the conntrack action + an output
> action implies that L3 routing is being performed. Hence, OVS should
> act like a router and either refragment or generate ICMP PTB in the
> case where MTU differs. According to the flow table, the rest of the
> routing functionality (MAC handling for instance) may or may not have
> been performed at this point, but we basically leave that up to the
> SDN controller to implement the right behaviour. In relation to this
> particular check, the idea was to retain the original geometry of the
> packet such that it's as though there were no functionality performed
> in the middle at all. OVS happened to do connection tracking (which
> implicitly involved queueing fragments), but if you treat it as an
> opaque box, you have ports connected and OVS is simply performing
> forwarding between the ports.

I've been going back and forth on this.  On the one hand, Open vSwitch
is supposed to only care about 'just' the L2 forwarding, with some
additional processing to assist.  After that, it's up to an L3 layer to
really provide additional support, and the idea is that the controller
or something else should really be guiding this higher level
intelligence.

The issue I have is that we do some of the high level intelligence here
to support conntrack, and it's done in a way that is a bit unintuitive.
As an example, you write:

  ... the idea was to retain the original geometry of the packet such
  that it's as though there were no functionality performed in the
  middle at all

But, the fragmentation engine isn't guaranteed to reassemble exactly the
same packets.

Consider the scenario where there is an upstream router that has
performed it's own mitm fragmentation.  There can be 

[ovs-dev] [PATCH ovn] controller: Monitor all logical flows that refer to datapath groups.

[Dumitru Ceara]

Considering two logical datapaths (DP1 and DP2) and a logical flow that
is shared between the two, ovn-northd will create the following SB
records:

Datapaths: DP1, DP2
Datapath_Group: DPG1 {dps=[DP1, DP2]}
Logical_Flow: LF {dp_group=DPG1}

If an ovn-controller is conditionally monitoring DP1 and DP2 its
monitor conditions look like this:

Datapath_Group table when:
 - Datapath_Group.datapaths in {DP1._uuid, DP2._uuid}
Logical_Flow table when:
 - Logical_Flow.logical_datapath in {DP1._uuid, DP2._uuid}
 - Logical_Flow.logical_dp_group in {DPG1._uuid}

If at this point a new logical datapath DP3 is created (e.g., a LS is
added) and the logical flow LF is shared with this third datapath, then
ovn-northd populates the SB with the following contents, replacing
old DPG1 with DPG1-new:

Datapaths: DP1, DP2, DP3
Datapath_Group: DPG1-new {dps=[DP1, DP2, DP3]}
Logical_Flow: LF {dp_group=DPG1-new}

At this point, the SB ovsdb-server will send the following updates to
ovn-controller, to match the current monitor condition it requested:

Datapaths:
- "insert" DP3

Datapath_Group:
- "delete" DPG1
- "insert" DPG1-new {dps=[DP1, DP2, DP3]}

Logical_Flow:
- "delete" LF

The logical flow is deleted from the client's view of the database
because the monitor condition for Logical_Flow records doesn't include
DPG1-new._uuid.

Now ovn-controller will:
- delete all openflows corresponding to LF, including the ones generated
  for datapaths DP1 and DP2.
- compute new set of monitor conditions and send the monitor_cond_change
  request to the SB:

Datapath_Group table when Datapath_Group.uuid in {DPG1-new._uuid}
Logical_Flow table when:
 - Logical_Flow.logical_datapath in {DP1._uuid, DP2._uuid, DP3._uuid}
 - Logical_Flow.logical_dp_group in {DPG1-new._uuid}

Upon processing this new set of conditions, the SB sends the following
update:
Logical_Flow:
- "insert" LF <--- now LF.logical_dp_group matches the condition.

Finally, ovn-controller will add all openflows that correspond to LF, on
all datapaths, DP1, DP2, DP3.

There is therefore a window in which traffic on DP1 and DP2 might be
dropped because openflows corresponding to LF1 on DP1 and DP2 have been
removed but not yet reinstalled.

To avoid this we now unconditionally monitor all logical flows that
refer to datapath groups.

Fixes: 4b946b366c4c ("controller: Add support for Logical Datapath Groups.")
Reported-at: https://bugzilla.redhat.com/1947056
Suggested-by: Ilya Maximets 
Signed-off-by: Dumitru Ceara 
---
 controller/ovn-controller.c | 26 +-
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 16c8ecb21..6f7c9ea61 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -259,23 +259,15 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
uuid);
 }
 
-/* Updating conditions to receive logical flows that references
- * datapath groups containing local datapaths. */
-const struct sbrec_logical_dp_group *group;
-SBREC_LOGICAL_DP_GROUP_FOR_EACH (group, ovnsb_idl) {
-struct uuid *uuid = CONST_CAST(struct uuid *,
-   &group->header_.uuid);
-size_t i;
-
-for (i = 0; i < group->n_datapaths; i++) {
-if (get_local_datapath(local_datapaths,
-   group->datapaths[i]->tunnel_key)) {
-sbrec_logical_flow_add_clause_logical_dp_group(
-&lf, OVSDB_F_EQ, uuid);
-break;
-}
-}
-}
+/* Datapath groups are immutable, which means a new group record is
+ * created when a datapath is added to a group.  The logical flows
+ * referencing a datapath group are also updated in such cases but the
+ * new group UUID is not known by ovn-controller until the SB update
+ * is received.  To avoid unnecessarily removing and adding lflows
+ * that reference datapath groups, set the monitor condition to always
+ * request all of them.
+ */
+sbrec_logical_flow_add_clause_logical_dp_group(&lf, OVSDB_F_NE, NULL);
 }
 
 out:;
-- 
2.27.0

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

Re: [ovs-dev] [PATCH V6 00/13] Netdev vxlan-decap offload

[Sriharsha Basavapatna via dev]

On Wed, Apr 7, 2021 at 2:50 PM Eli Britstein  wrote:

>
> On 4/7/2021 12:10 PM, Sriharsha Basavapatna wrote:
>
>
> On Sun, Apr 4, 2021 at 3:25 PM Eli Britstein  wrote:
>
>> VXLAN decap in OVS-DPDK configuration consists of two flows:
>> F1: in_port(ens1f0),eth(),ipv4(),udp(), actions:tnl_pop(vxlan_sys_4789)
>> F2: tunnel(),in_port(vxlan_sys_4789),eth(),ipv4(), actions:ens1f0_0
>>
>> F1 is a classification flow. It has outer headers matches and it
>> classifies the packet as a VXLAN packet, and using tnl_pop action the
>> packet continues processing in F2.
>> F2 is a flow that has matches on tunnel metadata as well as on the inner
>> packet headers (as any other flow).
>>
>> In order to fully offload VXLAN decap path, both F1 and F2 should be
>> offloaded. As there are more than one flow in HW, it is possible that
>> F1 is done by HW but F2 is not. Packet is received by SW, and should be
>> processed starting from F2 as F1 was already done by HW.
>> Rte_flows are applicable only on physical port IDs. Keeping the original
>> physical in port on which the packet is received on enables applying
>> vport flows (e.g. F2) on that physical port.
>>
>> This patch-set makes use of [1] introduced in DPDK 20.11, that adds API
>> for tunnel offloads.
>>
>> Note that MLX5 PMD has a bug that the tnl_pop private actions must be
>> first. In OVS it is not.
>> Fixing this issue is scheduled for 21.05 (and stable 20.11.2).
>> Meanwhile, tests were done with a workaround for it [2].
>>
>> v2-v1:
>> - Tracking original in_port, and applying vport on that physical port
>> instead of all PFs.
>> v3-v2:
>> - Traversing ports using a new API instead of flow_dump.
>> - Refactor packet state recover logic, with bug fix for error pop_header.
>> - One ref count for netdev in non-tunnel case.
>> - Rename variables, comments, rebase.
>> v4-v3:
>> - Extract orig_in_port from physdev for flow modify.
>> - Miss handling fixes.
>> v5-v4:
>> - Drop refactor offload rule creation commit.
>> - Comment about setting in_port in restore.
>> - Refactor vports flow offload commit.
>> v6-v5:
>> - Fixed duplicate netdev ref bug.
>>
>
> Can you provide some info on this bug ?  and what changes were done to fix
> this ?
>
> With v5, the 2 netdevs sent to ufid_to_rte_flow_associate are always
> non-NULL (was not like this previously), and there was this line:
>
> +data->netdev = vport ? netdev_ref(vport) : physdev;
>
> As the "vport" was always non-null, even for non-tunnels, it took another
> ref of it, but in disassociate, only one close was done.
>
> With v6 it is like this (changed arguments names a bit)
>
> +data->physdev = netdev != physdev ? netdev_ref(physdev) : physdev;
>
> Checking the netdevs are different, not non-NULL.
>
> Thanks,
> -Harsha
>
>>
>> Travis:
>> v1: https://travis-ci.org/github/elibritstein/OVS/builds/756418552
>> v2 :
>> https://travis-ci.org/github/elibritstein/OVS/builds/758382963
>> v3 :
>> https://travis-ci.org/github/elibritstein/OVS/builds/761089087
>> v4 :
>> https://travis-ci.org/github/elibritstein/OVS/builds/763146966
>> v5 :
>> https://travis-ci.org/github/elibritstein/OVS/builds/765271879
>> v6 :
>> https://travis-ci.org/github/elibritstein/OVS/builds/765816800
>>
>> GitHub Actions:
>> v1: https://github.com/elibritstein/OVS/actions/runs/515334647
>> v2: https://github.com/elibritstein/OVS/actions/runs/554986007
>> v3: https://github.com/elibritstein/OVS/actions/runs/613226225
>> v4: https://github.com/elibritstein/OVS/actions/runs/658415274
>> v5: https://github.com/elibritstein/OVS/actions/runs/704357369
>> v6: https://github.com/elibritstein/OVS/actions/runs/716304028
>>
>> [1] https://mails.dpdk.org/archives/dev/2020-October/187314.html
>> [2] https://github.com/elibritstein/dpdk-stable/pull/1
>>
>>
>> Eli Britstein (10):
>>   netdev-offload: Add HW miss packet state recover API
>>   netdev-dpdk: Introduce DPDK tunnel APIs
>>   netdev-offload: Introduce an API to traverse ports
>>   netdev-dpdk: Add flow_api support for netdev vxlan vports
>>   netdev-offload-dpdk: Implement HW miss packet recover for vport
>>   dpif-netdev: Add HW miss packet state recover logic
>>   netdev-offload-dpdk: Change log rate limits
>>   netdev-offload-dpdk: Support tunnel pop action
>>   netdev-offload-dpdk: Support vports flows offload
>>   netdev-dpdk-offload: Add vxlan pattern matching function
>>
>> Ilya Maximets (2):
>>   netdev-offload: Allow offloading to netdev without ifindex.
>>   netdev-offload: Disallow offloading to unrelated tunneling vports.
>>
>> Sriharsha Basavapatna (1):
>>   dpif-netdev: Provide orig_in_port in metadata for tunneled packets
>>
>>  Documentation/howto/dpdk.rst  |  

Re: [ovs-dev] [PATCH] docker: add testing on local docker localhost:5000

[Ben Pfaff]

Hi Ali and Han, you guys wrote the docker stuff in OVS, and Numan, you
reviewed it.  Do you think you could comment on these proposed changes
from Aidan/hunchback?  I am not competent in Docker myself.

On Fri, Apr 02, 2021 at 07:27:00PM +, hunchback wrote:
> add a set of docker targets which ease the building and testing of
> docker images both on local registry (localhost:5000) as well as on the
> public default registry (docker.io).
> 
> Signed-off-by: hunchback 
> ---
>  .gitignore  |  1 +
>  Documentation/intro/install/general.rst | 38 +-
>  Makefile.am |  1 +
>  utilities/automake.mk   |  2 +-
>  utilities/docker/.gitignore |  2 +
>  utilities/docker/Makefile   | 22 --
>  utilities/docker/automake.mk| 53 +
>  utilities/docker/debian/Dockerfile  |  5 ++-
>  8 files changed, 79 insertions(+), 45 deletions(-)
>  create mode 100644 utilities/docker/.gitignore
>  delete mode 100644 utilities/docker/Makefile
>  create mode 100644 utilities/docker/automake.mk
> 
> diff --git a/.gitignore b/.gitignore
> index f1cdcf124..b46a0528a 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -79,3 +79,4 @@ testsuite.tmp.orig
>  /Documentation/_build
>  /.venv
>  /cxx-check
> +*.mod
> diff --git a/Documentation/intro/install/general.rst 
> b/Documentation/intro/install/general.rst
> index c4300cd53..8a3609627 100644
> --- a/Documentation/intro/install/general.rst
> +++ b/Documentation/intro/install/general.rst
> @@ -510,40 +510,38 @@ For ovs vswitchd, we need to load ovs kernel modules on 
> host.
>  
>  Hence, OVS containers kernel version needs to be same as that of host kernel.
>  
> -Export following variables in .env  and place it under
> -project root::
> +If you want to change the default values for building an image then set 
> these 
> +variables::
>  
> -$ OVS_BRANCH=
> -$ OVS_VERSION=
> -$ DISTRO=
> -$ KERNEL_VERSION=
> -$ GITHUB_SRC=
> -$ DOCKER_REPO=
> +$ export OVS_BRANCH=
> +$ export OVS_VERSION=
> +$ export DISTRO=
> +$ export KERNEL_VERSION=
> +$ export GITHUB_SRC=
> +$ export DOCKER_REPO=
> +$ export DOCKER_TAG=
>  
> -To build ovs modules::
> +To setup for using a local registry (localhost:5000)::
>  
> -$ cd utilities/docker
> -$ make build
> +$ make docker-registry
> +$ export DOCKER_REPO=localhost:5000/ovsvswitch/ovs
>  
> -Compiled Modules will be tagged with docker image
> +To build ovs modules (tagged with docker image)::
>  
> -To Push ovs modules::
> +$ make docker-build
>  
> -$ make push
> +To push ovs modules to docker repo::
>  
> -OVS docker image will be pushed to specified docker repo.
> + $ make docker-push
>  
>  Start ovsdb-server using below command::
>  
> -$ docker run -itd --net=host --name=ovsdb-server \
> -  : ovsdb-server
> + $ make docker-ovsdb-server
>  
>  Start ovs-vswitchd with priviledged mode as it needs to load kernel module in
>  host using below command::
>  
> -$ docker run -itd --net=host --name=ovs-vswitchd \
> -  --volumes-from=ovsdb-server -v /lib:/lib --privileged \
> -  : ovs-vswitchd
> +$ make docker-ovs-vswitchd
>  
>  .. note::
>  The debian docker file uses ubuntu 16.04 as a base image for reference.
> diff --git a/Makefile.am b/Makefile.am
> index 691a005ad..e75df1be6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -499,3 +499,4 @@ include datapath-windows/include/automake.mk
>  include windows/automake.mk
>  include selinux/automake.mk
>  include build-aux/automake.mk
> +include utilities/docker/automake.mk
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index e2e22c39a..4d2f20570 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -56,7 +56,7 @@ EXTRA_DIST += \
>   utilities/ovs-vlan-test.in \
>   utilities/ovs-vsctl-bashcomp.bash \
>   utilities/checkpatch.py \
> -utilities/docker/Makefile \
> +utilities/docker/automake.mk \
>  utilities/docker/ovs-override.conf \
>  utilities/docker/start-ovs \
>  utilities/docker/create_ovs_db.sh \
> diff --git a/utilities/docker/.gitignore b/utilities/docker/.gitignore
> new file mode 100644
> index 0..5d05c158b
> --- /dev/null
> +++ b/utilities/docker/.gitignore
> @@ -0,0 +1,2 @@
> +vswitch.ovsschema
> +ovsdb-tool
> diff --git a/utilities/docker/Makefile b/utilities/docker/Makefile
> deleted file mode 100644
> index d8b08a3c9..0
> --- a/utilities/docker/Makefile
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -#export OVS_BRANCH=branch-2.11
> -#export OVS_VERSION=2.11
> -#export KERNEL_VERSION=4.15.0-54-generic
> -#export DISTRO=debian
> -#export GITHUB_SRC=https://github.com/openvswitch/ovs.git
> -#export DOCKER_REPO=openvswitch/ovs
> -
> -# Example:
> -#   make build
> -#   make push
> -
> -REPO = ${DOCKER_REPO}
> -tag = ${OVS_VERSION}_${DISTRO}_${KE

Re: [ovs-dev] [PATCH] documentation: add missing sudo to intro install

[Ben Pfaff]

On Fri, Apr 02, 2021 at 07:34:43PM +, hunchback wrote:
> the current documentation assumes everything is run from within root
> what is not correct in the general case, the revised install
> documentation makes use of `sudo` for specific commands which require to
> be run under root permissions.

Please write sentences in commit messages starting with a capital
letter.

> Signed-off-by: hunchback 

> -  the required build infrastructure.
> +  the required build infrastructure, so the name of the package correspoing
> +  with your running kernel is ``linux-headers-$(uname -r)``.

"correspoing" => "corresponding"

> - echo "override ${modname%.ko} * extra" >> "$config_file"
> - echo "override ${modname%.ko} * weak-updates" >> "$config_file"
> + sudo echo "override ${modname%.ko} * extra" >> "$config_file"
> + sudo echo "override ${modname%.ko} * weak-updates" >> "$config_file"

That won't actually work.  The usual dodge is to use "tee", like this:
sudo echo "something" | tee -a /root/owned/file > /dev/null

>  to use the SSL configuration in the database::
>  
> -$ mkdir -p /usr/local/var/run/openvswitch
> +$ sudo mkdir -p /usr/local/var/run/openvswitch
>  $ ovsdb-server --remote=punix:/usr/local/var/run/openvswitch/db.sock \
>  --remote=db:Open_vSwitch,Open_vSwitch,manager_options \
>  --private-key=db:Open_vSwitch,SSL,private_key \

I guess that ovsdb-server should run as root also.

>  1. Stop the Open vSwitch daemons, e.g.::
>  
> -   $ kill `cd /usr/local/var/run/openvswitch && cat ovsdb-server.pid 
> ovs-vswitchd.pid`
> +   $ sudo killall -9 ovsdb-server ovs-vswitchd

The above change will be a bit more disruptive.  On my machine it would
be likely to cause test failures if I was running "make check", for
example.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] make: don't prompt during build

[Ben Pfaff]

On Fri, Apr 02, 2021 at 07:08:19PM +, hunchback wrote:
> When running repeated builds using `make build` you get prompts in cases
> the `mv` command is about to overwrite a file which is write-protect.
> This patch forced the `mv` w/o prompting for approval.
> 
> Signed-off-by: hunchback 

I think you must be using an unusual configuration, since no one else
has reported this in the 10+ years of the project.

I applied this to master (but I changed "hunchback" to Aidan Shribman;
real names are better).

Thanks,

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

Re: [ovs-dev] [PATCH ovn v3] documentation: add sudo to install

[Ben Pfaff]

On Wed, Apr 07, 2021 at 10:27:25PM +0100, Mark Gray wrote:
> On 07/04/2021 20:43, Aidan Shribman wrote:
> > From: hunchback 
> > 
> > add documentation of install sequence sudo to support running install
> > sequence as non-root user
> > 
> > Signed-off-by: Aidan Shribman 
> > ---
> >  Documentation/intro/install/general.rst | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/intro/install/general.rst 
> > b/Documentation/intro/install/general.rst
> > index 08a03a632..ee4827242 100644
> > --- a/Documentation/intro/install/general.rst
> > +++ b/Documentation/intro/install/general.rst
> > @@ -371,7 +371,7 @@ Building
> >  3. Run ``make install`` to install the executables and manpages into the
> > running system, by default under ``/usr/local``::
> >  
> > -   $ make install
> > +   $ sudo make install
> 
> Maybe the prompt could be changed to "#" to imply root privileges?

It seems to be the "modern" thing to assume people are running as an
unprivileged user but using sudo.  It's what most distros encourage
these days.  It's probably better to go along with it, than to assume
that users know when they need to run as root; the users who know what
they're doing will also know when to skip typing "sudo".
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] LLDP: add new command to show LLDP neighbor info

[Ben Pfaff]

> >>> +if (id != NULL) {
> >>
> >>It's safe to call free(NULL), so , please, don't check.
> > [Rick] Does it mean that we override the original free() method, so that it 
> > won't crash when we call free(NULL)? If so, that is good and I don't need 
> > to check here.
> 
> It's part of a C standard starting at least from C89:
> """
> 4.10.3.2 The free function
> ...
> If ptr is a null pointer, no action occurs.
> """
> 
> 'man 3 free' suggests the same.

It's also part of the coding style document (more for Rick than for
Ilya):

Functions that destroy an instance of a dynamically-allocated type should
accept and ignore a null pointer argument. Code that calls such a function
(including the C standard library function ``free()``) should omit a
null-pointer check. We find that this usually makes code easier to read.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] dpif-netlink: fix using uninitialized info.tc_modify_flow_deleted in out label

[0-day Robot]

Bleep bloop.  Greetings wangyunjian, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Mengfan Lv 
Lines checked: 40, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 2/3] netdev-offload-tc: Probe for support for any of the ct_state flags

[Marcelo Ricardo Leitner]

On Mon, Mar 15, 2021 at 07:41:11PM +0100, Simon Horman wrote:
> On Tue, Mar 09, 2021 at 11:12:51AM -0300, Marcelo Ricardo Leitner wrote:
> > On Sun, Mar 07, 2021 at 04:22:02PM +0200, Paul Blakey wrote:
> > > Upstream kernel now rejects unsupported ct_state flags.
> > > Earlier kernels, ignored it but still echoed back the requested ct_state,
> > > if ct_state was supported. ct_state initial support had trk, new, est,
> > > and rel flags.
> > > 
> > > If kernel echos back ct_state, assume support for trk, new, est, and
> > > rel. If kernel rejects a specific unsupported flag, continue and
> > > use reject mechanisim to probe for flags rep and inv.
> > > 
> > > Disallow inserting rules with unnsupported ct_state flags.
> > > 
> > > Signed-off-by: Paul Blakey 
> > > Acked-by: Roi Dayan 
> > 
> > Reviewed-by: Marcelo Ricardo Leitner 
> 
> Thanks, pushed to master.

Simon, Paul, can we have the feature probing (but not the support for
the new bits) backported back to 2.13 please? Not ignoring the bits is
a quite critical fix for that branch.

Thanks,
Marcelo

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

[ovs-dev] [PATCH v2] dpif-netlink: fix using uninitialized info.tc_modify_flow_deleted in out label

[wangyunjian]

From: Yunjian Wang 

Before info.tc_modify_flow_deleted is assigned a value, error
processing of other statements goes to the out label. In the
out label, the uninitialized variant is used for condition
determination, which may cause uncertain behavior.

Fixes: 65b84d4a32bd ("dpif-netlink: avoid netlink modify flow put op failed 
after tc modify flow put op failed.")
Signed-off-by: Mengfan Lv 
Signed-off-by: Yunjian Wang 
---
v2:
   update commit log
---
 lib/dpif-netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index ceb56c685..50520f8c0 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2061,6 +2061,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct 
dpif_flow_put *put)
 uint8_t csum_on = false;
 int err;
 
+info.tc_modify_flow_deleted = false;
 if (put->flags & DPIF_FP_PROBE) {
 return EOPNOTSUPP;
 }
@@ -2105,7 +2106,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct 
dpif_flow_put *put)
 info.tunnel_csum_on = csum_on;
 info.recirc_id_shared_with_tc = (dpif->user_features
  & OVS_DP_F_TC_RECIRC_SHARING);
-info.tc_modify_flow_deleted = false;
 err = netdev_flow_put(dev, &match,
   CONST_CAST(struct nlattr *, put->actions),
   put->actions_len,
-- 
2.23.0

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

Re: [ovs-dev] [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.

[Eelco Chaudron]

On 8 Apr 2021, at 14:05, Martin Varghese wrote:


On Wed, Apr 07, 2021 at 03:49:07PM +, Jan Scheurich wrote:

Hi Martin,

I guess you are aware of the original design document we wrote for 
generic encap/decap and NSH support:

https://docs.google.com/document/d/1oWMYUH8sjZJzWa72o2q9kU0N6pNE-rwZcLH3-kbbDR8/edit#

It is no longer 100% aligned with the final implementation in OVS but 
still a good reference for understanding the design principles behind 
the implementation and some specifics for Ethernet and NSH 
encap/decap use cases.


Please find some more answers/comments below.

BR, Jan


-Original Message-
From: Martin Varghese 
Sent: Wednesday, 7 April, 2021 10:43
To: Jan Scheurich 
Cc: Eelco Chaudron ; d...@openvswitch.org;
pshe...@ovn.org; martin.vargh...@nokia.com
Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS packet 
type.


On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:

Hi,

Thanks for the heads up. The interaction with MPLS push/pop is a 
use case
that was likely not tested during the NSH and generic encap/decap 
design. It's
complex code and a long time ago. I'm willing to help, but I will 
need some

time to go back and have a look.


It would definitely help, if you could provide a minimal example 
for

reproducing the problem.




Hi Jan ,

Thanks for your help.

I was trying to implement ENCAP/DECAP support for MPLS.

The programming of datapath flow for the below  userspace rule fails 
as there
is set(eth() action between pop_mpls and recirc ovs-ofctl -O 
OpenFlow13 add-

flow br_mpls2 "in_port=$egress_port,dl_type=0x8847
actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1

2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
system: failed to put[create] (Invalid argument) 
ufid:1dddb0ba-27fe-44ea-

9a99-5815764b4b9c
recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state
(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(0x8847)
,mpls(label=2/0x0,tc=0/0,ttl=64/0x0,bos=1/1),
actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x45)



Conceptually, what should happen in this scenario is that, after the 
second decap(packet_type(ns=0,type=0) action, OVS processes the 
unchanged inner packet as packet type PT_ETH, i.e. as L2 Ethernet 
frame. Overwriting the existing Ethernet header with zero values  
through set(eth()) is clearly incorrect. That is a logical error 
inside the ofproto-dpif-xlate module (see below).


I believe the netdev userspace datapath would still have accepted the 
incorrect datapath flow. I have too little experience with the kernel 
datapath to explain why that rejects the datapath flow as invalid.


Unlike in the Ethernet and NSH cases, the MPLS header does not 
contain any indication about the inner packet type. That is why the 
packet_type must be provided by the SDN controller as part of the 
decap() action.  And the ofproto-dpif-xlate module must consider the 
specified inner packet type when continuing the translation. In the 
general case, a decap() action should trigger recirculation for 
reparsing of the inner packet, so the new packet type must be set 
before recirculation. (Exceptions to the general recirculation rule 
are those where OVS has already parsed further into the packet and 
ofproto can modify the flow on the fly: decap(Ethernet) and possibly 
decap(MPLS) for all but the last bottom of stack label).


I have had a look at your new code for encap/decap of MPLS headers, 
but I must admit I cannot fully judge in how far re-using the 
existing translation functions for MPLS label stacks written for the 
legacy push/pop_mpls case (i.e. manipulating a label stack between 
the L2 and the L3 headers of a PT_ETH Packet) are possible to re-use 
in the new context.


BTW: Do you support multiple MPLS label encap or decap actions with 
your patch? Have you tested that?



Yes, I will add those tests too.


Maybe you could add some tests the same way NSH does, by sending a 
packet and verifying the modified content. The current test does encap 
than decap, so if both go wrong, or are skipped we are not actually 
testing anything.


I am uncertain about the handling of the ethertype of the 
decapsulated inner packet. In the design base, the ethertype that is 
set in the existing L2 header of the packet after pop_mpls of the 
last label is coming from the pop_mpls action, while in the 
decap(packet_type(0,0)) case the entire inner packet should be 
recirculated as is with packet_type PT_ETH.


case PT_MPLS: {
 int n;
 ovs_be16 ethertype;

 flow->packet_type = decap->new_pkt_type;
 ethertype = pt_ns_type_be(flow->packet_type);

 n = flow_count_mpls_labels(flow, ctx->wc);
 flow_pop_mpls(flow, n, ethertype, ctx->wc);
 if (!ctx->xbridge->support.add_mpls) {
ctx->xout->slow |= SLO

Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule version to include latest ovsdb-idl fixes.

[Dumitru Ceara]

On 4/8/21 2:05 PM, Mark Michelson wrote:
> I pushed this to master. Thanks!
> 
> On 4/2/21 1:38 PM, Ben Pfaff wrote:
>> On Fri, Apr 02, 2021 at 12:32:33PM +0200, Dumitru Ceara wrote:
>>> Specifically:
>>>    95689f166818 ("ovsdb-idl: Preserve references for deleted rows.")
>>>    ac85cdb38c1f ("ovsdb-idl: Mark arc sources as updated when
>>> destination is deleted.")
>>>
>>> Signed-off-by: Dumitru Ceara 
>>
>> Seems reasonable.  I didn't review the specifics.
>>
>> Acked-by: Ben Pfaff 
>> ___

Thank you!

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

Re: [ovs-dev] [PATCH] LLDP: add new command to show LLDP neighbor info

[Aaron Conole]

"Rick Zhong"  writes:

> Hi Ilya,
>
> I took your advice and will submit a new merge request.

Please send an email with a link to the pull request.  It's best if you
can use 'git send-email' to send the series to the list instead.

> As to the existing "autoattach" commands, it requires DCBX enabled on peer 
> device. Otherwise, nothing is shown by the
> command.
> That's why I didn't merge into it.
>
> Best regards,
> Rick Zhong
>
> At 2021-04-08 01:55:20, "Ilya Maximets"  wrote:
>>On 4/7/21 7:08 PM, Rick Zhong wrote:
>>> Hi Ilya,
>>> 
>>> Many thanks for your attention on this and reply. Please see my comments 
>>> inline.
>>> 
>>> 
>>> And I will try the 'git' commands as you mentioned.
>>> 
>>> Best regards,
>>> 
>>> Rick Zhong
>>> 
>>> 
>>> At 2021-04-07 20:21:19, "Ilya Maximets"  wrote:
On 3/24/21 4:56 AM, Rick Zhong wrote:
> Dear OVS reviewers/supervisors:
> 
> 
> This patch is related to LLDP which provides a new command "ovs-appctl 
> lldp/neighbor" to show LLDP neighbor info when LLDP is enabled on OVS 
> interfaces.
> 
> 
> With this new command, user is enable to get LLDP neighbor info even if 
> not in SPB network.
> 
> 
> One limitation is that when multiple peer Management IP addresses are 
> found by LLDP, only one Management IP address is displayed by the command.
> 
> 
> The patch is well-tested on Linux.
> 
> 
> Related commit: af4b3d3 and e4bc70c (add new command to show LLDP 
> neighbor info #349)
> 
> 
> Signed-off-by: Rick Zhong (winsome8...@163.com)

Hi.  Thanks for working on this!
The patch format is a bit unusual, you might want to consider using
'git format-patch' and 'git send-email' to send patches to the mail-list.

Aaron, could you take a look at this change from the LLDP perspective?

Few comments inline.

> 
> 
> =
> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
> index 05c1dd434..4c8ab9126 100644
> --- a/lib/ovs-lldp.c
> +++ b/lib/ovs-lldp.c
> @@ -324,6 +324,84 @@ aa_print_isid_status(struct ds *ds, struct lldp 
> *lldp) OVS_REQUIRES(mutex)
>  }
>  }
>  
> +static void
> +lldp_print_neighbor_port(struct ds *ds, struct lldpd_hardware *hw)
> +{
> +struct lldpd_port *port;
> +
> +LIST_FOR_EACH (port, p_entries, &hw->h_rports) {
> +const char *none_str = "";

Can we use "" here like other commands does?
>>> [Rick] Sure. No problem.
>>> 
> +char *id = NULL;
> +const char *name = NULL;
> +const char *port_id = NULL;
> +char ipaddress[INET6_ADDRSTRLEN + 1];
> +memset(ipaddress, 0, INET6_ADDRSTRLEN + 1);
> +
> +if (port->p_chassis) {
> +if (port->p_chassis->c_id_len > 0) {
> +chassisid_to_string(port->p_chassis->c_id,
> +port->p_chassis->c_id_len, &id);
> +}
> +
> +name = port->p_chassis->c_name;
> +
> +struct lldpd_mgmt *mgmt;
> +LIST_FOR_EACH (mgmt, m_entries, &port->p_chassis->c_mgmt) {
> +int af;
> +size_t alen;
> +switch (mgmt->m_family) {
> +case LLDPD_AF_IPV4:
> +alen = INET_ADDRSTRLEN + 1;
> +af = AF_INET;
> +break;
> +case LLDPD_AF_IPV6:
> +alen = INET6_ADDRSTRLEN + 1;
> +af = AF_INET6;
> +break;
> +default:
> +continue;
> +}
> +
> +if (inet_ntop(af, &mgmt->m_addr, ipaddress, alen) == 
> NULL) {
> +continue;
> +}
> +break;

OVS already has some formatting functions that converts ip addresses
to strings, so it's better to use them.  For this particular case,
I think, we can use some thing like this:

struct in6_addr ip = in6addr_any;
...

LIST_FOR_EACH (mgmt, m_entries, &port->p_chassis->c_mgmt) {
switch (mgmt->m_family) {
case LLDPD_AF_IPV4:
in6_addr_set_mapped_ipv4(&ip, &mgmt->m_addr.inet);
break;
case LLDPD_AF_IPV6:
ip = mgmt->m_addr.inet6;
break;
default:
continue;
}
}

...
ds_put_cstr(ds, "  Neighbor Management IP: ");
ipv6_format_mapped(&ip, ds);
ds_put_char(ds, "\n");
>>> [Rick] Thanks for your example. Actually this part of IP conversion codes 
>>> were copied from anothe

[ovs-dev] [PATCH ovn] ofctrl: Send all flow modifications in a bundle.

[Ilya Maximets]

If some OF rules needs to be replaced due to logical flow changes,
ovn-controller deletes old rules first and adds new ones later.
In a complex scenario with big number of flows a lot of time
can pass between these events leading to the dataplane downtime
and packet loss.  Also, while these changes are in progress,
OVS will use incomplete pipelines that will also lead to packet
drops.

To avoid this, all flow modifications should be done atomically,
so there will be always some version of OF rules installed that
can handle dataplane traffic and it will be complete in terms of
reflecting some consistent set of logical flows.  Wrapping all
flow modifications into atomic ordered bundle to achieve that.

Reported-by: Dumitru Ceara 
Reported-at: https://bugzilla.redhat.com/1947398
Signed-off-by: Ilya Maximets 
---
 controller/ofctrl.c | 80 +++--
 1 file changed, 62 insertions(+), 18 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 415d9b7e1..0b24d98b3 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -29,6 +29,7 @@
 #include "openvswitch/list.h"
 #include "openvswitch/match.h"
 #include "openvswitch/ofp-actions.h"
+#include "openvswitch/ofp-bundle.h"
 #include "openvswitch/ofp-flow.h"
 #include "openvswitch/ofp-group.h"
 #include "openvswitch/ofp-match.h"
@@ -1567,10 +1568,22 @@ encode_flow_mod(struct ofputil_flow_mod *fm)
 }
 
 static void
-add_flow_mod(struct ofputil_flow_mod *fm, struct ovs_list *msgs)
+add_flow_mod(struct ofputil_flow_mod *fm,
+ struct ofputil_bundle_ctrl_msg *bc,
+ struct ovs_list *msgs)
 {
 struct ofpbuf *msg = encode_flow_mod(fm);
-ovs_list_push_back(msgs, &msg->list_node);
+struct ofputil_bundle_add_msg bam = {
+.bundle_id = bc->bundle_id,
+.flags = bc->flags,
+.msg   = msg->data,
+};
+struct ofpbuf *bundle_msg;
+
+bundle_msg = ofputil_encode_bundle_add(OFP15_VERSION, &bam);
+
+ofpbuf_delete(msg);
+ovs_list_push_back(msgs, &bundle_msg->list_node);
 }
 
 /* group_table. */
@@ -1752,7 +1765,9 @@ add_meter(struct ovn_extend_table_info *m_desired,
 }
 
 static void
-installed_flow_add(struct ovn_flow *d, struct ovs_list *msgs)
+installed_flow_add(struct ovn_flow *d,
+   struct ofputil_bundle_ctrl_msg *bc,
+   struct ovs_list *msgs)
 {
 /* Send flow_mod to add flow. */
 struct ofputil_flow_mod fm = {
@@ -1764,11 +1779,12 @@ installed_flow_add(struct ovn_flow *d, struct ovs_list 
*msgs)
 .new_cookie = htonll(d->cookie),
 .command = OFPFC_ADD,
 };
-add_flow_mod(&fm, msgs);
+add_flow_mod(&fm, bc, msgs);
 }
 
 static void
 installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d,
+   struct ofputil_bundle_ctrl_msg *bc,
struct ovs_list *msgs)
 {
 /* Update actions in installed flow. */
@@ -1787,7 +1803,7 @@ installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d,
 /* Use OFPFC_ADD so that cookie can be updated. */
 fm.command = OFPFC_ADD;
 }
-add_flow_mod(&fm, msgs);
+add_flow_mod(&fm, bc, msgs);
 
 /* Replace 'i''s actions and cookie by 'd''s. */
 free(i->ofpacts);
@@ -1797,7 +1813,9 @@ installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d,
 }
 
 static void
-installed_flow_del(struct ovn_flow *i, struct ovs_list *msgs)
+installed_flow_del(struct ovn_flow *i,
+   struct ofputil_bundle_ctrl_msg *bc,
+   struct ovs_list *msgs)
 {
 struct ofputil_flow_mod fm = {
 .match = i->match,
@@ -1805,11 +1823,12 @@ installed_flow_del(struct ovn_flow *i, struct ovs_list 
*msgs)
 .table_id = i->table_id,
 .command = OFPFC_DELETE_STRICT,
 };
-add_flow_mod(&fm, msgs);
+add_flow_mod(&fm, bc, msgs);
 }
 
 static void
 update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table,
+  struct ofputil_bundle_ctrl_msg *bc,
   struct ovs_list *msgs)
 {
 ovs_assert(ovs_list_is_empty(&flow_table->tracked_flows));
@@ -1823,7 +1842,7 @@ update_installed_flows_by_compare(struct 
ovn_desired_flow_table *flow_table,
 if (!d) {
 /* Installed flow is no longer desirable.  Delete it from the
  * switch and from installed_flows. */
-installed_flow_del(&i->flow, msgs);
+installed_flow_del(&i->flow, bc, msgs);
 ovn_flow_log(&i->flow, "removing installed");
 
 hmap_remove(&installed_flows, &i->match_hmap_node);
@@ -1832,7 +1851,7 @@ update_installed_flows_by_compare(struct 
ovn_desired_flow_table *flow_table,
 if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len,
d->flow.ofpacts, d->flow.ofpacts_len) ||
 i->flow.cookie != d->flow.cookie) {
-installed_flow_mod(&i->flow, &d->flow, msgs);
+installed_fl

Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule version to include latest ovsdb-idl fixes.

[Mark Michelson]

I pushed this to master. Thanks!

On 4/2/21 1:38 PM, Ben Pfaff wrote:

On Fri, Apr 02, 2021 at 12:32:33PM +0200, Dumitru Ceara wrote:

Specifically:
   95689f166818 ("ovsdb-idl: Preserve references for deleted rows.")
   ac85cdb38c1f ("ovsdb-idl: Mark arc sources as updated when destination is 
deleted.")

Signed-off-by: Dumitru Ceara 


Seems reasonable.  I didn't review the specifics.

Acked-by: Ben Pfaff 
___
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 v4 1/2] Encap & Decap actions for MPLS packet type.

[Martin Varghese]

On Wed, Apr 07, 2021 at 03:49:07PM +, Jan Scheurich wrote:
> Hi Martin,
> 
> I guess you are aware of the original design document we wrote for generic 
> encap/decap and NSH support:
> https://docs.google.com/document/d/1oWMYUH8sjZJzWa72o2q9kU0N6pNE-rwZcLH3-kbbDR8/edit#
> 
> It is no longer 100% aligned with the final implementation in OVS but still a 
> good reference for understanding the design principles behind the 
> implementation and some specifics for Ethernet and NSH encap/decap use cases.
> 
> Please find some more answers/comments below.
> 
> BR, Jan 
> 
> > -Original Message-
> > From: Martin Varghese 
> > Sent: Wednesday, 7 April, 2021 10:43
> > To: Jan Scheurich 
> > Cc: Eelco Chaudron ; d...@openvswitch.org;
> > pshe...@ovn.org; martin.vargh...@nokia.com
> > Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.
> > 
> > On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:
> > > Hi,
> > >
> > > Thanks for the heads up. The interaction with MPLS push/pop is a use case
> > that was likely not tested during the NSH and generic encap/decap design. 
> > It's
> > complex code and a long time ago. I'm willing to help, but I will need some
> > time to go back and have a look.
> > >
> > > It would definitely help, if you could provide a minimal example for
> > reproducing the problem.
> > >
> > 
> > Hi Jan ,
> > 
> > Thanks for your help.
> > 
> > I was trying to implement ENCAP/DECAP support for MPLS.
> > 
> > The programming of datapath flow for the below  userspace rule fails as 
> > there
> > is set(eth() action between pop_mpls and recirc ovs-ofctl -O OpenFlow13 add-
> > flow br_mpls2 "in_port=$egress_port,dl_type=0x8847
> > actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1
> > 
> > 2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
> > system: failed to put[create] (Invalid argument) ufid:1dddb0ba-27fe-44ea-
> > 9a99-5815764b4b9c
> > recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state
> > (0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
> > 00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(0x8847)
> > ,mpls(label=2/0x0,tc=0/0,ttl=64/0x0,bos=1/1),
> > actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x45)
> > 
> 
> Conceptually, what should happen in this scenario is that, after the second 
> decap(packet_type(ns=0,type=0) action, OVS processes the unchanged inner 
> packet as packet type PT_ETH, i.e. as L2 Ethernet frame. Overwriting the 
> existing Ethernet header with zero values  through set(eth()) is clearly 
> incorrect. That is a logical error inside the ofproto-dpif-xlate module (see 
> below).
> 
> I believe the netdev userspace datapath would still have accepted the 
> incorrect datapath flow. I have too little experience with the kernel 
> datapath to explain why that rejects the datapath flow as invalid.
> 
> Unlike in the Ethernet and NSH cases, the MPLS header does not contain any 
> indication about the inner packet type. That is why the packet_type must be 
> provided by the SDN controller as part of the decap() action.  And the 
> ofproto-dpif-xlate module must consider the specified inner packet type when 
> continuing the translation. In the general case, a decap() action should 
> trigger recirculation for reparsing of the inner packet, so the new packet 
> type must be set before recirculation. (Exceptions to the general 
> recirculation rule are those where OVS has already parsed further into the 
> packet and ofproto can modify the flow on the fly: decap(Ethernet) and 
> possibly decap(MPLS) for all but the last bottom of stack label).
> 
> I have had a look at your new code for encap/decap of MPLS headers, but I 
> must admit I cannot fully judge in how far re-using the existing translation 
> functions for MPLS label stacks written for the legacy push/pop_mpls case 
> (i.e. manipulating a label stack between the L2 and the L3 headers of a 
> PT_ETH Packet) are possible to re-use in the new context. 
> 
> BTW: Do you support multiple MPLS label encap or decap actions with your 
> patch? Have you tested that? 
>
Yes, I will add those tests too.
> I am uncertain about the handling of the ethertype of the decapsulated inner 
> packet. In the design base, the ethertype that is set in the existing L2 
> header of the packet after pop_mpls of the last label is coming from the 
> pop_mpls action, while in the decap(packet_type(0,0)) case the entire inner 
> packet should be recirculated as is with packet_type PT_ETH. 
> 
> case PT_MPLS: {
>  int n;
>  ovs_be16 ethertype;
> 
>  flow->packet_type = decap->new_pkt_type;
>  ethertype = pt_ns_type_be(flow->packet_type);
> 
>  n = flow_count_mpls_labels(flow, ctx->wc);
>  flow_pop_mpls(flow, n, ethertype, ctx->wc);
>  if (!ctx->xbridge->support.add_mpls) {
> ctx->xout->slow |= SLOW_ACTION;

Re: [ovs-dev] [PATCH ovn] expr: Combine multiple ipv4 with wildcard mask.(Internet mail)

[陈供明]

On 2021/4/7, 12:01 AM, "Dumitru Ceara"  wrote:

On 4/6/21 12:11 PM, Mark Gray wrote:
> I had a conversation with Dumitru and this patch came up in the
> conversation. He made an interesting suggestion (Dumitru, please correct
> me if I get this wrong) that this could be refactored as an external
> tool. This cmdline tool could, for example, take a set of IP addresses
> and return the wildcard representation. For example,
> 
> # ./ovs-new-tool 
>  ... 
> 
> Then this tool could optionally be used by a CMS. The other advantage of
> this is that it could also be used in other places.

Given that OVN is essentially a stack of compilers (NB -> SB ->
openflow) I was wondering if it would be reasonable and worth it to go a
step forward and implement this feature to "combine IP addresses with
wildcard" as a library.  This would allow using it at different levels
in the stack, e.g.:
- at CMS level/as a standalone cmdline tool.

My concern is whether the process after cms can correctly handle this
non-standard cidr format (1.1.1.0/255.255.255.253).

- in ovn-northd (for example when generating SB address sets from NB
address sets)

If the non-standard cidr format ip address can be handled reasonably
here, I think this is a good idea.

- in ovn-controller (as the current patch is doing).

Thanks,
Gongming

Regards,
Dumitru



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

Re: [ovs-dev] [PATCH ovn] expr: Combine multiple ipv4 with wildcard mask.(Internet mail)

[陈供明]

On 2021/4/6, 6:11 PM, "Mark Gray"  wrote:

On 01/04/2021 13:28, gmingchen(陈供明) wrote:
> Correct some words
> 
> 
> On 2021/4/1, 8:17 PM, "gmingchen(陈供明)"  wrote:
> 
> 
> 
> On 2021/3/30, 4:59 PM, "Mark Gray"  wrote:
> 
> On 29/03/2021 13:30, gmingchen(陈供明) wrote:
> > 
> > On 2021/3/25, 11:30 PM, "Mark Gray"  
wrote:
> > 
> > On 19/03/2021 13:01, Dumitru Ceara wrote:
> > > On 3/10/21 2:29 PM, gmingchen(陈供明) wrote:
> > >> From: Gongming Chen 
> > >>
> > 
> > Thanks for the patch. Looks like a lot of thought went into 
the
> > algorithm and it has been interesting to review.
> > 
> > Do you know if there are any well-known algorithms to do 
this? It seems
> > like a common problem? If there was, it may be better to 
use it as we
> > could reference standard documentation.
> > 
> > Hi Mark,
> > First of all, thanks for the review.
> > 
> > Unfortunately, I did not find a well-known algorithms.
> 
> Ok. Thats a shame. It's kind of like IP subnetting so I thought 
there
> may be something.
> 
> > 
> > >> This patch merges ipv4 addresses with wildcard masks, 
and replaces this
> > >> ipv4 addresses with the combined ip/mask. This will 
greatly reduce the
> > >> entries in the ovs security group flow table, especially 
when the host
> > >> size is large.
> > >>
> > >> Analysis in the simplest scenario, a network 1.1.1.0/24 
network,create 253
> > >> ports(1.1.1.2-1.1.1.254).
> > >> Only focus on the number of ip addresses, the original 
253 addresses will
> > >> be combined into 13 addresses.
> > >> 1.1.1.2/31
> > >> 1.1.1.4/30
> > >> 1.1.1.8/29
> > >> 1.1.1.16/28
> > >> 1.1.1.32/27
> > >> 1.1.1.64/26
> > >> 1.1.1.128/26
> > >> 1.1.1.192/27
> > >> 1.1.1.224/28
> > >> 1.1.1.240/29
> > >> 1.1.1.248/30
> > >> 1.1.1.252/31
> > >> 1.1.1.254
> > >>
> > >> Some scenes are similar to the following:
> > >> 1.1.1.2, 1.1.1.6
> > >> After the combine:
> > >> 1.1.1.2/255.255.255.251
> > >> You can use ovn-match-ip utility to match ip.
> > >> such as:
> > >> ovs-ofctl dump-flows br-int | ovn-match-ip 1.1.1.6
> > >> 1.1.1.2/255.255.255.251 will show.
> > >>
> > >> Simple description of the algorithm.
> > >> There are two situations
> > >> 1. Combine once
> > >> such as:
> > >> 1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
> > >> Combined into: 1.1.1.0/31, 1.0.1.0/31
> > >> 2. Combine multiple times
> > >> 1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
> > >> Combined into: 1.0.1.0/255.254.255.254
> > >>
> > >> Considering the actual scene and simplicity, the first 
case is used to
> > >> combine once.
> > >>
> > >> ...00...
> > >> ...01...
> > >> ...10...
> > >> ...11...
> > >> "..." means the same value omitted.
> > >> Obviously, the above value can be expressed as 
...00.../11100111. This
> > >> continuous interval that can be represented by one or 
several wildcard
> > >> masks is called a segment.
> > >> Only if all 2< > >> exist, can they be combined into 00...(n)/00...( n)
> > >>
> > >> First sort all the values by size. Iterate through each 
value.
> > >> 1. Find a new segment, where two values differ only by 1 
bit, such as
> > >> ...0... and ...1...
> > >> diff = ip_next ^ ip
> > >> if (diff & (diff-1)) == 0
> > >> new_segment = true
> > >> The first non-zero place in the high direction of ip is 
the end of the
> > >> segment(segment_end).
> > >> For example...100... and...101..., the segment_end is 
...111...
> > >>
> > >> 2. Count the number of consecutive and less than 
continuous_size in the
> > >> segment.
> > >> diff = ip_next - ip
> > >

Re: [ovs-dev] [PATCH ovn 1/2] system-ovn.at: Make tests more portable.

[Dumitru Ceara]

On 4/8/21 11:07 AM, Mark Gray wrote:
> On 02/04/2021 10:35, Dumitru Ceara wrote:
>> Due to slight differences in behavior and/or output of some of the
>> utilities used by the tests when run on different distributions some
>> tests were failing, e.g., when run on Ubuntu 20.04.
>>
>> Use "tcpdump -nn" to avoid host and port resolution;  also update "nc"
>> usage to avoid options that behave differently on various distributions.
>>
>> This commit adds no guarantee that tests will pass on all possible
>> distributions, only tested on: Fedora 32 and Ubuntu 20.04.
>>
>> Signed-off-by: Dumitru Ceara 
>> ---
>>  tests/system-ovn.at |   75 
>> +--
>>  1 file changed, 31 insertions(+), 44 deletions(-)
>>
>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> index 4885303d1..b6c679907 100644
>> --- a/tests/system-ovn.at
>> +++ b/tests/system-ovn.at
>> @@ -4555,13 +4555,13 @@ NS_CHECK_EXEC([lsp], [nc 88.88.88.89 8080 -z], [0])
>>  
>>  # Capture IPv4 UDP hairpinned packets.
>>  filter="dst 42.42.42.1 and dst port 2021 and udp"
>> -NS_CHECK_EXEC([lsp], [tcpdump -n -c 2 -i lsp ${filter} > lsp.pcap &])
>> +NS_CHECK_EXEC([lsp], [tcpdump -nn -c 2 -i lsp ${filter} > lsp.pcap &])
>>  
>>  sleep 1
>>  
>>  # Generate IPv4 UDP hairpin traffic.
>> -NS_CHECK_EXEC([lsp], [nc -u 88.88.88.88 4040 -z &], [0])
>> -NS_CHECK_EXEC([lsp], [nc -u 88.88.88.89 4040 -z &], [0])
>> +NS_CHECK_EXEC([lsp], [echo a | nc -u 88.88.88.88 4040 &], [0])
> 
> Why this change?

One of the differences between OpenBSD and nmap netcat is how "-z" is
handled for UDP connections.  In the nmap netcat case it actually sends
a packet, while in the other it doesn't.

> 
>> +NS_CHECK_EXEC([lsp], [echo a | nc -u 88.88.88.89 4040 &], [0])
>>  
>>  # Check hairpin traffic.
>>  OVS_WAIT_UNTIL([
>> @@ -4639,15 +4639,15 @@ NS_CHECK_EXEC([lsp], [timeout 2s nc -k -l 4200::1 
>> 4041 &], [0])
>>  NS_CHECK_EXEC([lsp], [nc 8800::0088 8080 -z], [0])
>>  NS_CHECK_EXEC([lsp], [nc 8800::0089 8080 -z], [0])
>>  
>> -# Capture IPv4 UDP hairpinned packets.
>> +# Capture IPv6 UDP hairpinned packets.
>>  filter="dst 4200::1 and dst port 2021 and udp"
>> -NS_CHECK_EXEC([lsp], [tcpdump -n -c 2 -i lsp $filter > lsp.pcap &])
>> +NS_CHECK_EXEC([lsp], [tcpdump -nn -c 2 -i lsp $filter > lsp.pcap &])
>>  
>>  sleep 1
>>  
>>  # Generate IPv6 UDP hairpin traffic.
>> -NS_CHECK_EXEC([lsp], [nc -u 8800::0088 4040 -z &], [0])
>> -NS_CHECK_EXEC([lsp], [nc -u 8800::0089 4040 -z &], [0])
>> +NS_CHECK_EXEC([lsp], [echo a | nc -u 8800::0088 4040 &], [0])
>> +NS_CHECK_EXEC([lsp], [echo a | nc -u 8800::0089 4040 &], [0])
>>  
>>  # Check hairpin traffic.
>>  OVS_WAIT_UNTIL([
>> @@ -4757,21 +4757,17 @@ ADD_VETH(sw1-p1-rej, sw1-p1-rej, br-int, 
>> "20.0.0.3/24", "40:54:00:00:00:03", \
>>  sleep 1
>>  
>>  # Capture packets in sw0-p1-rej.
>> -NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -n -c 4 -i sw0-p1-rej tcp > 
>> sw0-p1-rej-ip4.pcap &], [0])
>> +NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -nn -c 4 -i sw0-p1-rej tcp > 
>> sw0-p1-rej-ip4.pcap &], [0])
>>  
>>  sleep 1
>>  
>>  OVS_WAIT_UNTIL([
>> -ip netns exec sw0-p1-rej nc  10.0.0.4 80 2> r
>> -res=$(cat r)
>> -test "$res" = "Ncat: Connection refused."
>> +ip netns exec sw0-p1-rej nc -vz 10.0.0.4 80 2>&1 | grep -i 'connection 
>> refused'
>>  ])
>>  
>>  # Now send traffic to port 84
>>  OVS_WAIT_UNTIL([
>> -ip netns exec sw0-p1-rej nc  10.0.0.4 84 2> r
>> -res=$(cat r)
>> -test "$res" = "Ncat: Connection refused."
>> +ip netns exec sw0-p1-rej nc -vz 10.0.0.4 84 2>&1 | grep -i 'connection 
>> refused'
>>  ])
>>  
>>  OVS_WAIT_UNTIL([
>> @@ -4789,14 +4785,12 @@ OVS_WAIT_UNTIL([
>>  # Without this sleep, test case fails intermittently.
>>  sleep 3
>>  
>> -NS_CHECK_EXEC([sw0-p2-rej], [tcpdump -n -c 2 -i sw0-p2-rej tcp port 80 > 
>> sw0-p2-rej-ip6.pcap &], [0])
>> +NS_CHECK_EXEC([sw0-p2-rej], [tcpdump -nn -c 2 -i sw0-p2-rej tcp port 80 > 
>> sw0-p2-rej-ip6.pcap &], [0])
>>  
>>  sleep 1
>>  
>>  OVS_WAIT_UNTIL([
>> -ip netns exec sw0-p2-rej nc -6 aef0::3 80 2> r
>> -res=$(cat r)
>> -test "$res" = "Ncat: Connection refused."
>> +ip netns exec sw0-p2-rej nc -vz6 aef0::3 80 2>&1 | grep -i 'connection 
>> refused'
>>  ])
>>  
>>  
>> @@ -4811,57 +4805,55 @@ ovn-nbctl acl-add sw1 to-lport 1004 "ip" 
>> allow-related
>>  ovn-nbctl --log acl-add pg0 to-lport 1004 "outport == @pg0 && ip && tcp && 
>> tcp.dst == 84" reject
>>  
>>  OVS_WAIT_UNTIL([
>> -ip netns exec sw1-p1-rej nc  10.0.0.4 84 2> r
>> -res=$(cat r)
>> -test "$res" = "Ncat: Connection refused."
>> +ip netns exec sw1-p1-rej nc -vz 10.0.0.4 84 2>&1 | grep -i 'connection 
>> refused'
> 
> Is there a risk that you will pick up another spurious "connection
> refused"? Why not keep to "ncat: connection refused"? Or is that one of
> the differences between Fedora and Ubuntu.

It's the latter, another difference between OpenBSD and nmap netcat.

> 
> Also, why are you adding "-z"?

If there's a bug in 

Re: [ovs-dev] [PATCH ovn] northd: Fix "Destination unknown" table number in man pages.

[Mark Gray]

On 05/04/2021 16:53, Alexey Roytman wrote:
> From: Alexey Roytman 
> 
> The actual number of the "Destination unknown" table, is 24 and is not 23
> as mentioned.
> 
> Signed-off-by: Alexey Roytman 
> ---
>  northd/ovn-northd.8.xml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index a62f5c057..e42a2249e 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1619,7 +1619,7 @@ output;
>
>  
>  
> -Ingress Table 23 Destination unknown
> +Ingress Table 24 Destination unknown
>  
>  
>This table handles the packets whose destination was not found or
> 

Quick look in the sandbox confirms this

$ ovn-sbctl lflow-list | grep table=24
  table=24(ls_in_l2_unknown   ), priority=50   , match=(outport ==
"none"), action=(drop;)
  table=24(ls_in_l2_unknown   ), priority=0, match=(1), action=(output;)
  table=24(ls_in_l2_unknown   ), priority=50   , match=(outport ==
"none"), action=(drop;)
  table=24(ls_in_l2_unknown   ), priority=0, match=(1), action=(output;)


Acked-by: Mark D. Gray 

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

Re: [ovs-dev] [PATCH ovn 2/2] ci: Enable OVN system tests in GitHub Actions runs.

[Mark Gray]

On 02/04/2021 10:35, Dumitru Ceara wrote:
> For now only enable system tests using the kernel datapath.  Running
> the system tests with the userspace datapath makes some tests fail
> due to known issues in the OVS userspace conntrack implementation which
> are actively worked on.
> 
> To save CI time, only enable system test runs for a single item in the
> test matrix ("linux gcc system-test").
> 
> We're also moving to Ubuntu 20.04 LTS in CI to avoid hitting:
> https://bugzilla.redhat.com/1550097
> 
> Signed-off-by: Dumitru Ceara 
> ---
>  .ci/linux-build.sh |   11 +++
>  .github/workflows/test.yml |7 ---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index d17a96f49..76a2ff459 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -59,6 +59,17 @@ if [ "$TESTSUITE" ]; then
>  cat */_build/sub/tests/testsuite.log
>  exit 1
>  fi
> +
> +if [ "$TESTSUITE" = "system-test" ]; then
> +# Reconfigure build with required OPTS, rebuild and run system tests.
> +configure_ovn $OPTS
> +make -j4 || { cat config.log; exit 1; }
> +if ! sudo make -j4 check-kernel RECHECK=yes; then
> +# system-kmod-testsuite.log is necessary for debugging.
> +cat tests/system-kmod-testsuite.log
> +exit 1
> +fi
> +fi
>  else
>  configure_ovn $OPTS
>  make -j4 || { cat config.log; exit 1; }
> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
> index 91bd1e538..b963e2244 100644
> --- a/.github/workflows/test.yml
> +++ b/.github/workflows/test.yml
> @@ -11,7 +11,7 @@ jobs:
>build-linux:
>  env:
>dependencies: |
> -automake libtool gcc bc libjemalloc1 libjemalloc-dev\
> +automake libtool gcc bc libjemalloc2 libjemalloc-dev\
>  libssl-dev llvm-dev libelf-dev libnuma-dev libpcap-dev  \
>  selinux-policy-dev
>m32_dependecies: gcc-multilib
> @@ -23,7 +23,7 @@ jobs:
>ASAN:${{ matrix.asan }}
>  
>  name: linux ${{ join(matrix.*, ' ') }}
> -runs-on: ubuntu-18.04
> +runs-on: ubuntu-20.04
>  
>  strategy:
>fail-fast: false
> @@ -35,7 +35,7 @@ jobs:
>  opts: --disable-ssl
>  
>- compiler: gcc
> -testsuite:test
> +testsuite:system-test
>- compiler: clang
>  testsuite:test
>  asan: asan
> @@ -113,6 +113,7 @@ jobs:
>  mkdir logs
>  cp config.log ./logs/
>  cp -r ./*/_build/sub/tests/testsuite.* ./logs/ || true
> +cp -r ./tests/system-kmod-testsuite.* ./logs/ || true
>  tar -czvf logs.tgz logs/
>  
>  - name: upload logs on failure
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 

Acked-by: Mark D. Gray 

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

Re: [ovs-dev] [PATCH ovn 1/2] system-ovn.at: Make tests more portable.

[Mark Gray]

On 02/04/2021 10:35, Dumitru Ceara wrote:
> Due to slight differences in behavior and/or output of some of the
> utilities used by the tests when run on different distributions some
> tests were failing, e.g., when run on Ubuntu 20.04.
> 
> Use "tcpdump -nn" to avoid host and port resolution;  also update "nc"
> usage to avoid options that behave differently on various distributions.
> 
> This commit adds no guarantee that tests will pass on all possible
> distributions, only tested on: Fedora 32 and Ubuntu 20.04.
> 
> Signed-off-by: Dumitru Ceara 
> ---
>  tests/system-ovn.at |   75 
> +--
>  1 file changed, 31 insertions(+), 44 deletions(-)
> 
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 4885303d1..b6c679907 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -4555,13 +4555,13 @@ NS_CHECK_EXEC([lsp], [nc 88.88.88.89 8080 -z], [0])
>  
>  # Capture IPv4 UDP hairpinned packets.
>  filter="dst 42.42.42.1 and dst port 2021 and udp"
> -NS_CHECK_EXEC([lsp], [tcpdump -n -c 2 -i lsp ${filter} > lsp.pcap &])
> +NS_CHECK_EXEC([lsp], [tcpdump -nn -c 2 -i lsp ${filter} > lsp.pcap &])
>  
>  sleep 1
>  
>  # Generate IPv4 UDP hairpin traffic.
> -NS_CHECK_EXEC([lsp], [nc -u 88.88.88.88 4040 -z &], [0])
> -NS_CHECK_EXEC([lsp], [nc -u 88.88.88.89 4040 -z &], [0])
> +NS_CHECK_EXEC([lsp], [echo a | nc -u 88.88.88.88 4040 &], [0])

Why this change?

> +NS_CHECK_EXEC([lsp], [echo a | nc -u 88.88.88.89 4040 &], [0])
>  
>  # Check hairpin traffic.
>  OVS_WAIT_UNTIL([
> @@ -4639,15 +4639,15 @@ NS_CHECK_EXEC([lsp], [timeout 2s nc -k -l 4200::1 
> 4041 &], [0])
>  NS_CHECK_EXEC([lsp], [nc 8800::0088 8080 -z], [0])
>  NS_CHECK_EXEC([lsp], [nc 8800::0089 8080 -z], [0])
>  
> -# Capture IPv4 UDP hairpinned packets.
> +# Capture IPv6 UDP hairpinned packets.
>  filter="dst 4200::1 and dst port 2021 and udp"
> -NS_CHECK_EXEC([lsp], [tcpdump -n -c 2 -i lsp $filter > lsp.pcap &])
> +NS_CHECK_EXEC([lsp], [tcpdump -nn -c 2 -i lsp $filter > lsp.pcap &])
>  
>  sleep 1
>  
>  # Generate IPv6 UDP hairpin traffic.
> -NS_CHECK_EXEC([lsp], [nc -u 8800::0088 4040 -z &], [0])
> -NS_CHECK_EXEC([lsp], [nc -u 8800::0089 4040 -z &], [0])
> +NS_CHECK_EXEC([lsp], [echo a | nc -u 8800::0088 4040 &], [0])
> +NS_CHECK_EXEC([lsp], [echo a | nc -u 8800::0089 4040 &], [0])
>  
>  # Check hairpin traffic.
>  OVS_WAIT_UNTIL([
> @@ -4757,21 +4757,17 @@ ADD_VETH(sw1-p1-rej, sw1-p1-rej, br-int, 
> "20.0.0.3/24", "40:54:00:00:00:03", \
>  sleep 1
>  
>  # Capture packets in sw0-p1-rej.
> -NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -n -c 4 -i sw0-p1-rej tcp > 
> sw0-p1-rej-ip4.pcap &], [0])
> +NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -nn -c 4 -i sw0-p1-rej tcp > 
> sw0-p1-rej-ip4.pcap &], [0])
>  
>  sleep 1
>  
>  OVS_WAIT_UNTIL([
> -ip netns exec sw0-p1-rej nc  10.0.0.4 80 2> r
> -res=$(cat r)
> -test "$res" = "Ncat: Connection refused."
> +ip netns exec sw0-p1-rej nc -vz 10.0.0.4 80 2>&1 | grep -i 'connection 
> refused'
>  ])
>  
>  # Now send traffic to port 84
>  OVS_WAIT_UNTIL([
> -ip netns exec sw0-p1-rej nc  10.0.0.4 84 2> r
> -res=$(cat r)
> -test "$res" = "Ncat: Connection refused."
> +ip netns exec sw0-p1-rej nc -vz 10.0.0.4 84 2>&1 | grep -i 'connection 
> refused'
>  ])
>  
>  OVS_WAIT_UNTIL([
> @@ -4789,14 +4785,12 @@ OVS_WAIT_UNTIL([
>  # Without this sleep, test case fails intermittently.
>  sleep 3
>  
> -NS_CHECK_EXEC([sw0-p2-rej], [tcpdump -n -c 2 -i sw0-p2-rej tcp port 80 > 
> sw0-p2-rej-ip6.pcap &], [0])
> +NS_CHECK_EXEC([sw0-p2-rej], [tcpdump -nn -c 2 -i sw0-p2-rej tcp port 80 > 
> sw0-p2-rej-ip6.pcap &], [0])
>  
>  sleep 1
>  
>  OVS_WAIT_UNTIL([
> -ip netns exec sw0-p2-rej nc -6 aef0::3 80 2> r
> -res=$(cat r)
> -test "$res" = "Ncat: Connection refused."
> +ip netns exec sw0-p2-rej nc -vz6 aef0::3 80 2>&1 | grep -i 'connection 
> refused'
>  ])
>  
>  
> @@ -4811,57 +4805,55 @@ ovn-nbctl acl-add sw1 to-lport 1004 "ip" allow-related
>  ovn-nbctl --log acl-add pg0 to-lport 1004 "outport == @pg0 && ip && tcp && 
> tcp.dst == 84" reject
>  
>  OVS_WAIT_UNTIL([
> -ip netns exec sw1-p1-rej nc  10.0.0.4 84 2> r
> -res=$(cat r)
> -test "$res" = "Ncat: Connection refused."
> +ip netns exec sw1-p1-rej nc -vz 10.0.0.4 84 2>&1 | grep -i 'connection 
> refused'

Is there a risk that you will pick up another spurious "connection
refused"? Why not keep to "ncat: connection refused"? Or is that one of
the differences between Fedora and Ubuntu.

Also, why are you adding "-z"?
>  ])
>  
>  # Now test for IPv4 UDP.
> -NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -n -c 1 -i sw0-p1-rej udp port 90 > 
> sw0-p1-rej-udp.pcap &], [0])
> -NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -n -c 1 -i sw0-p1-rej icmp > 
> sw0-p1-rej-icmp.pcap &], [0])
> +NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -nn -c 1 -i sw0-p1-rej udp port 90 > 
> sw0-p1-rej-udp.pcap &], [0])
> +NS_CHECK_EXEC([sw0-p1-rej], [tcpdump -nn -c 1 -i sw0-p1-rej icmp > 
> sw0-p1-rej-icmp.pcap &

[ovs-dev] [PATCH ovn v2] ovn-nbctl: fails to delete and add lr-nat in trx

[Aidan Shribman]

The delete operation did not update the internal state cuasing the add
operation to wrongly believe that the lr-nat entry is still present and
thus can't be added.

Below is the sequance is failed before and now passes:

***
echo "add switch"
ovn-nbctl ls-del ls0 2>/dev/null
ovn-nbctl ls-add ls0
ovn-nbctl ls-list
ovn-nbctl show ls0
ovn-nbctl lsp-del lsp0 2>/dev/null
ovn-nbctl lsp-add ls0 lsp0 00:00:00:00:00:01 10.0.0.0/24
ovn-nbctl lsp-list ls0

echo "add router"
ovn-nbctl ls-del lr0 2>/dev/null
ovn-nbctl lr-add lr0
ovn-nbctl lr-list
ovn-nbctl show lr0

echo "add nat"
ovn-nbctl lr-nat-del lr0 2>/dev/null
ovn-nbctl lr-nat-add lr0 dnat_and_snat 192.168.0.3 10.0.0.3 \
lsp0 f0:00:00:00:00:03
ovn-nbctl lr-nat-list lr0

echo "FIXED: delete nat (all of type) + add nat"
ovn-nbctl lr-nat-del lr0 dnat_and_snat -- \
--may-exist lr-nat-add lr0 dnat_and_snat 192.168.0.3 10.0.0.3 \
lsp0 f0:00:00:00:00:03
ovn-nbctl lr-nat-list lr0

echo "FIXED: delete nat (all of external_ip) + add"
ovn-nbctl lr-nat-del lr0 dnat_and_snat 192.168.0.3 -- \
--may-exist lr-nat-add lr0 dnat_and_snat 192.168.0.3 10.0.0.3 \
lsp0 f0:00:00:00:00:03
ovn-nbctl lr-nat-list lr0
***

Signed-off-by: Aidan Shribman 
Fixed: 1928226 (ovn-nbctl fails to delete/add lr-nat in trx)
---
 utilities/ovn-nbctl.c | 33 ++---
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 184058356..db1ac4f5b 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -4531,11 +4531,18 @@ nbctl_lr_nat_del(struct ctl_context *ctx)
 
 if (ctx->argc == 3) {
 /*Deletes all NATs with the specified type. */
+struct nbrec_nat **keep_nat = xmalloc(lr->n_nat * sizeof *keep_nat);
+size_t n_keep_nat = 0;
 for (size_t i = 0; i < lr->n_nat; i++) {
-if (!strcmp(nat_type, lr->nat[i]->type)) {
-nbrec_logical_router_update_nat_delvalue(lr, lr->nat[i]);
+if (strcmp(nat_type, lr->nat[i]->type)) {
+keep_nat[n_keep_nat++] = lr->nat[i];
 }
 }
+if (n_keep_nat < lr->n_nat) {
+nbrec_logical_router_verify_nat(lr);
+nbrec_logical_router_set_nat(lr, keep_nat, n_keep_nat);
+}
+free(keep_nat);
 return;
 }
 
@@ -4547,31 +4554,27 @@ nbctl_lr_nat_del(struct ctl_context *ctx)
 
 int is_snat = !strcmp("snat", nat_type);
 /* Remove the matching NAT. */
+struct nbrec_nat **keep_nat = xmalloc(lr->n_nat * sizeof *keep_nat);
+size_t n_keep_nat = 0;
 for (size_t i = 0; i < lr->n_nat; i++) {
 struct nbrec_nat *nat = lr->nat[i];
-bool should_return = false;
 char *old_ip = normalize_prefix_str(is_snat
 ? nat->logical_ip
 : nat->external_ip);
-if (!old_ip) {
-continue;
-}
-if (!strcmp(nat_type, nat->type) && !strcmp(nat_ip, old_ip)) {
-nbrec_logical_router_update_nat_delvalue(lr, nat);
-should_return = true;
+if (!old_ip || strcmp(nat_type, nat->type) || strcmp(nat_ip, old_ip)) {
+keep_nat[n_keep_nat++] = lr->nat[i];
 }
 free(old_ip);
-if (should_return) {
-goto cleanup;
-}
 }
 
-if (must_exist) {
+if (n_keep_nat < lr->n_nat) {
+nbrec_logical_router_verify_nat(lr);
+nbrec_logical_router_set_nat(lr, keep_nat, n_keep_nat);
+} else if (must_exist) {
 ctl_error(ctx, "no matching NAT with the type (%s) and %s (%s)",
   nat_type, is_snat ? "logical_ip" : "external_ip", nat_ip);
 }
-
-cleanup:
+free(keep_nat);
 free(nat_ip);
 }
 
-- 
2.25.1

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