date:20201103

Re: [ovs-dev] [PATCH ovn 0/2] Further restrict ARP/ND broadcast domain for E-W and S-N traffic.

2020-11-03 Thread Numan Siddique

On Wed, Nov 4, 2020 at 3:44 AM Mark Michelson  wrote:
>
> Aside from spelling "update" properly in the title of patch 1, this
> looks good to me.
>
> Acked-by: Mark Michelson

Thanks Dumitru for fixing this issue and thanks Mark for the reviews.

I applied this patch series to master by updating the typo in the
commit message of patch 1
and with the changes below in the patch 1.


diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 795b52fc3..a5236564b 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -189,7 +189,7 @@ static void run_put_mac_bindings(
 struct ovsdb_idl_txn *ovnsb_idl_txn,
 struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
 struct ovsdb_idl_index *sbrec_port_binding_by_key,
-struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip);
+struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
 OVS_REQUIRES(pinctrl_mutex);
 static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
 static void flush_put_mac_bindings(void);
@@ -3851,8 +3851,7 @@ mac_binding_add(struct ovsdb_idl_txn *ovnsb_idl_txn,
 {
 /* Convert ethernet argument to string form for database. */
 char mac_string[ETH_ADDR_STRLEN + 1];
-snprintf(mac_string, sizeof mac_string,
-ETH_ADDR_FMT, ETH_ADDR_ARGS(ea));
+snprintf(mac_string, sizeof mac_string, ETH_ADDR_FMT, ETH_ADDR_ARGS(ea));

 const struct sbrec_mac_binding *b =
 mac_binding_lookup(sbrec_mac_binding_by_lport_ip, logical_port, ip);

*

Compilation with clang was failing in patch 1 with the below error message


../controller/pinctrl.c:193:5: error: declaration does not declare
anything [-Werror,-Wmissing-declarations]
OVS_REQUIRES(pinctrl_mutex);
^
/home/nusiddiq/workspace_cpp/openvswitch/ovs/include/openvswitch/compiler.h:126:5:
note: expanded from macro 'OVS_REQUIRES'
__attribute__((exclusive_locks_required(__VA_ARGS__)))
*

Thanks.

@Dumitru Ceara  I think this needs to be backported right ? If so, can
you please submit the patches to the branches.
The 2nd patch doesn't apply cleanly to the branch-20.09.

Thanks
Numan


>
> On 11/3/20 10:50 AM, Dumitru Ceara wrote:
> > This series aims at further restricting ARP/ND broadcast domain for self
> > originated packets.  Until now those packets were always flooded on all
> > ports of the switch.
> >
> > The first patch of the series is needed in order to remove dependency on
> > self-originated GARPs to be sent through the network (OVS) between OVN
> > logical ports.
> >
> > Signed-off-by: Dumitru Ceara 
> >
> > Dumitru Ceara (2):
> >pinctrl: Directly udpate MAC_Bindings created by self originated 
> > GARPs.
> >ovn-northd: Limit self originated ARP/ND broadcast domain.
> >
> >
> >   controller/pinctrl.c|  108 
> > ++-
> >   lib/mcast-group-index.h |1
> >   northd/ovn-northd.8.xml |   19 
> >   northd/ovn-northd.c |   84 +++--
> >   tests/ovn.at|   50 ++
> >   5 files changed, 173 insertions(+), 89 deletions(-)
> >
> > ___
> > 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
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn 09/12] northd: Move functions from ovn-northd.c into ovn-util.

2020-11-03 Thread Ben Pfaff

From: Justin Pettit 

The upcoming ddlog implementation of northd needs these functions too,
so they should be in a common library.

Signed-off-by: Justin Pettit 

ovn: Break out functions needed for ddlog.
---
 lib/ovn-util.c  | 79 +++--
 lib/ovn-util.h  | 11 +++
 northd/ovn-northd.c | 51 -
 3 files changed, 80 insertions(+), 61 deletions(-)

diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index 321fc89e275a..abe6b04a7701 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -13,17 +13,21 @@
  */
 
 #include 
+
+#include "ovn-util.h"
+
+#include 
 #include 
 
 #include "daemon.h"
-#include "ovn-util.h"
-#include "ovn-dirs.h"
-#include "openvswitch/vlog.h"
 #include "openvswitch/ofp-parse.h"
+#include "openvswitch/vlog.h"
+#include "ovn-dirs.h"
 #include "ovn-nb-idl.h"
 #include "ovn-sb-idl.h"
+#include "socket-util.h"
+#include "svec.h"
 #include "unixctl.h"
-#include 
 
 VLOG_DEFINE_THIS_MODULE(ovn_util);
 
@@ -240,27 +244,37 @@ extract_ip_addresses(const char *address, struct 
lport_addresses *laddrs)
 bool
 extract_lrp_networks(const struct nbrec_logical_router_port *lrp,
  struct lport_addresses *laddrs)
+{
+return extract_lrp_networks__(lrp->mac, lrp->networks, lrp->n_networks,
+  laddrs);
+}
+
+/* Separate out the body of 'extract_lrp_networks()' for use from DDlog,
+ * which does not know the 'nbrec_logical_router_port' type. */
+bool
+extract_lrp_networks__(char *mac, char **networks, size_t n_networks,
+   struct lport_addresses *laddrs)
 {
 memset(laddrs, 0, sizeof *laddrs);
 
-if (!eth_addr_from_string(lrp->mac, >ea)) {
+if (!eth_addr_from_string(mac, >ea)) {
 laddrs->ea = eth_addr_zero;
 return false;
 }
 snprintf(laddrs->ea_s, sizeof laddrs->ea_s, ETH_ADDR_FMT,
  ETH_ADDR_ARGS(laddrs->ea));
 
-for (int i = 0; i < lrp->n_networks; i++) {
+for (int i = 0; i < n_networks; i++) {
 ovs_be32 ip4;
 struct in6_addr ip6;
 unsigned int plen;
 char *error;
 
-error = ip_parse_cidr(lrp->networks[i], , );
+error = ip_parse_cidr(networks[i], , );
 if (!error) {
 if (!ip4) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_WARN_RL(, "bad 'networks' %s", lrp->networks[i]);
+VLOG_WARN_RL(, "bad 'networks' %s", networks[i]);
 continue;
 }
 
@@ -269,13 +283,13 @@ extract_lrp_networks(const struct 
nbrec_logical_router_port *lrp,
 }
 free(error);
 
-error = ipv6_parse_cidr(lrp->networks[i], , );
+error = ipv6_parse_cidr(networks[i], , );
 if (!error) {
 add_ipv6_netaddr(laddrs, ip6, plen);
 } else {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
 VLOG_INFO_RL(, "invalid syntax '%s' in networks",
- lrp->networks[i]);
+ networks[i]);
 free(error);
 }
 }
@@ -333,6 +347,23 @@ destroy_lport_addresses(struct lport_addresses *laddrs)
 free(laddrs->ipv6_addrs);
 }
 
+/* Go through 'addresses' and add found IPv4 addresses to 'ipv4_addrs' and
+ * IPv6 addresses to 'ipv6_addrs'. */
+void
+split_addresses(const char *addresses, struct svec *ipv4_addrs,
+struct svec *ipv6_addrs)
+{
+struct lport_addresses laddrs;
+extract_lsp_addresses(addresses, );
+for (size_t k = 0; k < laddrs.n_ipv4_addrs; k++) {
+svec_add(ipv4_addrs, laddrs.ipv4_addrs[k].addr_s);
+}
+for (size_t k = 0; k < laddrs.n_ipv6_addrs; k++) {
+svec_add(ipv6_addrs, laddrs.ipv6_addrs[k].addr_s);
+}
+destroy_lport_addresses();
+}
+
 /* Allocates a key for NAT conntrack zone allocation for a provided
  * 'key' record and a 'type'.
  *
@@ -663,3 +694,31 @@ ovn_smap_get_uint(const struct smap *smap, const char 
*key, unsigned int def)
 
 return u_value;
 }
+
+/* For a 'key' of the form "IP:port" or just "IP", sets 'port' and
+ * 'ip_address'.  The caller must free() the memory allocated for
+ * 'ip_address'.
+ * Returns true if parsing of 'key' was successful, false otherwise.
+ */
+bool
+ip_address_and_port_from_lb_key(const char *key, char **ip_address,
+uint16_t *port, int *addr_family)
+{
+struct sockaddr_storage ss;
+if (!inet_parse_active(key, 0, , false)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "bad ip address or port for load balancer key %s",
+ key);
+*ip_address = NULL;
+*port = 0;
+*addr_family = 0;
+return false;
+}
+
+struct ds s = DS_EMPTY_INITIALIZER;
+ss_format_address_nobracks(, );
+*ip_address = ds_steal_cstr();
+*port = ss_get_port();
+*addr_family = ss.ss_family;
+return

[ovs-dev] [PATCH ovn 06/12] tests: Improve "reject ACL" test.

2020-11-03 Thread Ben Pfaff

This makes it more debuggable.

Signed-off-by: Ben Pfaff 
---
 tests/ovn-northd.at | 67 +
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 9e7d8750f8fd..0bf20c1a7053 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -909,14 +909,14 @@ AT_CHECK([
 ])
 
 # Stateful FIP with ALLOWED_IPs
-ovn-nbctl lr-nat-del DR snat  50.0.0.11
-ovn-nbctl lr-nat-del CR snat  50.0.0.11
+check ovn-nbctl lr-nat-del DR snat  50.0.0.11
+check ovn-nbctl lr-nat-del CR snat  50.0.0.11
 
-ovn-nbctl lr-nat-add DR dnat_and_snat  172.16.1.2 50.0.0.11
-ovn-nbctl lr-nat-add CR dnat_and_snat  172.16.1.2 50.0.0.11
+check ovn-nbctl lr-nat-add DR dnat_and_snat  172.16.1.2 50.0.0.11
+check ovn-nbctl lr-nat-add CR dnat_and_snat  172.16.1.2 50.0.0.11
 
-ovn-nbctl lr-nat-update-ext-ip DR dnat_and_snat 172.16.1.2 allowed_range
-ovn-nbctl lr-nat-update-ext-ip CR dnat_and_snat 172.16.1.2 allowed_range
+check ovn-nbctl lr-nat-update-ext-ip DR dnat_and_snat 172.16.1.2 allowed_range
+check ovn-nbctl lr-nat-update-ext-ip CR dnat_and_snat 172.16.1.2 allowed_range
 
 ovn-nbctl show DR
 ovn-sbctl dump-flows DR
@@ -1691,45 +1691,59 @@ AT_CLEANUP
 AT_SETUP([ovn-northd -- reject ACL])
 ovn_start
 
-ovn-nbctl ls-add sw0
-ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl ls-add sw0
+check ovn-nbctl lsp-add sw0 sw0-p1
 
-ovn-nbctl ls-add sw1
-ovn-nbctl lsp-add sw1 sw1-p1
+check ovn-nbctl ls-add sw1
+check ovn-nbctl lsp-add sw1 sw1-p1
+
+check ovn-nbctl pg-add pg0 sw0-p1 sw1-p1
+check ovn-nbctl acl-add pg0 from-lport 1002 "inport == @pg0 && ip4 && tcp && 
tcp.dst == 80" reject
+check ovn-nbctl acl-add pg0 to-lport 1003 "outport == @pg0 && ip6 && udp" 
reject
+
+check ovn-nbctl --wait=hv sync
 
-ovn-nbctl pg-add pg0 sw0-p1 sw1-p1
-ovn-nbctl acl-add pg0 from-lport 1002 "inport == @pg0 && ip4 && tcp && tcp.dst 
== 80" reject
-ovn-nbctl acl-add pg0 to-lport 1003 "outport == @pg0 && ip6 && udp" reject
+AS_BOX([1])
 
-ovn-nbctl --wait=hv sync
+ovn-sbctl dump-flows sw0 > sw0flows
+AT_CAPTURE_FILE([sw0flows])
+ovn-sbctl dump-flows sw1 > sw1flows
+AT_CAPTURE_FILE([sw1flows])
 
-AT_CHECK([ovn-sbctl lflow-list sw0 | grep "ls_in_acl" | grep pg0 | sort], [0], 
[dnl
+AT_CHECK([grep "ls_in_acl" sw0flows | grep pg0 | sort], [0], [dnl
   table=7 (ls_in_acl  ), priority=2002 , dnl
 match=(inport == @pg0 && ip4 && tcp && tcp.dst == 80), dnl
 action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is 
implicit. */ outport <-> inport; next(pipeline=egress,table=6); };)
 ])
 
-AT_CHECK([ovn-sbctl lflow-list sw1 | grep "ls_in_acl" | grep pg0 | sort], [0], 
[dnl
+AT_CHECK([grep "ls_in_acl" sw1flows | grep pg0 | sort], [0], [dnl
   table=7 (ls_in_acl  ), priority=2002 , dnl
 match=(inport == @pg0 && ip4 && tcp && tcp.dst == 80), dnl
 action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is 
implicit. */ outport <-> inport; next(pipeline=egress,table=6); };)
 ])
 
-AT_CHECK([ovn-sbctl lflow-list sw0 | grep "ls_out_acl" | grep pg0 | sort], 
[0], [dnl
+AT_CHECK([grep "ls_out_acl" sw0flows | grep pg0 | sort], [0], [dnl
   table=5 (ls_out_acl ), priority=2003 , dnl
 match=(outport == @pg0 && ip6 && udp), dnl
 action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is 
implicit. */ outport <-> inport; next(pipeline=ingress,table=20); };)
 ])
 
-AT_CHECK([ovn-sbctl lflow-list sw1 | grep "ls_out_acl" | grep pg0 | sort], 
[0], [dnl
+AT_CHECK([grep "ls_out_acl" sw1flows | grep pg0 | sort], [0], [dnl
   table=5 (ls_out_acl ), priority=2003 , dnl
 match=(outport == @pg0 && ip6 && udp), dnl
 action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is 
implicit. */ outport <-> inport; next(pipeline=ingress,table=20); };)
 ])
 
-ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && udp" reject
+AS_BOX([2])
 
-AT_CHECK([ovn-sbctl lflow-list sw0 | grep "ls_out_acl" | grep pg0 | sort], 
[0], [dnl
+ovn-nbctl --wait=sb acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && udp" 
reject
+
+ovn-sbctl dump-flows sw0 > sw0flows2
+AT_CAPTURE_FILE([sw0flows2])
+ovn-sbctl dump-flows sw1 > sw1flows2
+AT_CAPTURE_FILE([sw1flows2])
+
+AT_CHECK([grep "ls_out_acl" sw0flows2 | grep pg0 | sort], [0], [dnl
   table=5 (ls_out_acl ), priority=2002 , dnl
 match=(outport == @pg0 && ip4 && udp), dnl
 action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is 
implicit. */ outport <-> inport; next(pipeline=ingress,table=20); };)
@@ -1738,7 +1752,7 @@ match=(outport == @pg0 && ip6 && udp), dnl
 action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is 
implicit. */ outport <-> inport; next(pipeline=ingress,table=20); };)
 ])
 
-AT_CHECK([ovn-sbctl lflow-list sw1 | grep "ls_out_acl" | grep pg0 | sort], 
[0], [dnl
+AT_CHECK([grep "ls_out_acl" sw1flows2 | grep pg0 | sort], [0], [dnl
   table=5 (ls_out_acl ), priority=2002 , dnl
 match=(outport == @pg0 && ip4 && udp), dnl

[ovs-dev] [PATCH ovn 11/12] tests: Prepare for multiple northd types.

2020-11-03 Thread Ben Pfaff

The idea is to run each test twice, once with ovn-northd, once
with ovn-northd-ddlog.  To do that, we add a macro OVN_FOR_EACH_NORTHD
and bracket each test that uses ovn-northd in it.

Signed-off-by: Ben Pfaff 
---
 tests/ovn-ic.at |  11 +-
 tests/ovn-macros.at |  96 ++--
 tests/ovn-northd.at | 161 ++-
 tests/ovn.at| 264 +---
 tests/ovs-macros.at |  20 ++--
 tests/system-ovn.at |  50 -
 6 files changed, 463 insertions(+), 139 deletions(-)

diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
index 0638af401295..2a4fba031f36 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -1,4 +1,5 @@
 AT_BANNER([OVN Interconnection Controller])
+OVN_FOR_EACH_NORTHD([
 AT_SETUP([ovn-ic -- AZ register])
 
 ovn_init_ic_db
@@ -29,7 +30,9 @@ availability-zone az3
 OVN_CLEANUP_IC([az1], [az2])
 
 AT_CLEANUP
+])
 
+OVN_FOR_EACH_NORTHD([
 AT_SETUP([ovn-ic -- transit switch handling])
 
 ovn_init_ic_db
@@ -59,7 +62,9 @@ check_column ts2 nb:Logical_Switch name
 OVN_CLEANUP_IC([az1])
 
 AT_CLEANUP
+])
 
+OVN_FOR_EACH_NORTHD([
 AT_SETUP([ovn-ic -- gateway sync])
 
 ovn_init_ic_db
@@ -120,8 +125,9 @@ OVN_CLEANUP_SBOX(gw2)
 OVN_CLEANUP_IC([az1], [az2])
 
 AT_CLEANUP
+])
 
-
+OVN_FOR_EACH_NORTHD([
 AT_SETUP([ovn-ic -- port sync])
 
 ovn_init_ic_db
@@ -185,7 +191,9 @@ OVN_CLEANUP_SBOX(gw1)
 OVN_CLEANUP_IC([az1], [az2])
 
 AT_CLEANUP
+])
 
+OVN_FOR_EACH_NORTHD([
 AT_SETUP([ovn-ic -- route sync])
 
 ovn_init_ic_db
@@ -310,3 +318,4 @@ OVS_WAIT_WHILE([ovn_as az1 ovn-nbctl lr-route-list lr1 | 
grep learned | grep 10.
 OVN_CLEANUP_IC([az1], [az2])
 
 AT_CLEANUP
+])
diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index 5b9c2dee6812..105ad0ab7f7f 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -47,10 +47,12 @@ m4_define([OVN_CLEANUP],[
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 
 as northd
-OVS_APP_EXIT_AND_WAIT([ovn-northd])
+OVS_APP_EXIT_AND_WAIT([[$NORTHD_TYPE]])
 
-as northd-backup
-OVS_APP_EXIT_AND_WAIT([ovn-northd])
+if test -d northd-backup; then
+as northd-backup
+OVS_APP_EXIT_AND_WAIT([[$NORTHD_TYPE]])
+fi
 
 OVN_CLEANUP_VSWITCH([main])
 ])
@@ -69,10 +71,12 @@ m4_define([OVN_CLEANUP_AZ],[
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 
 as $1/northd
-OVS_APP_EXIT_AND_WAIT([ovn-northd])
+OVS_APP_EXIT_AND_WAIT([[$NORTHD_TYPE]])
 
-as $1/northd-backup
-OVS_APP_EXIT_AND_WAIT([ovn-northd])
+if test -d $1/northd-backup; then
+as $1/northd-backup
+OVS_APP_EXIT_AND_WAIT([[$NORTHD_TYPE]])
+fi
 
 as $1/ic
 OVS_APP_EXIT_AND_WAIT([ovn-ic])
@@ -134,15 +138,48 @@ ovn_init_ic_db () {
 ovn_init_db ovn-ic-sb
 }
 
-# ovn_start [AZ]
+# ovn_start_northd (primary|backup) [AZ]
+ovn_start_northd() {
+local priority=$1
+local AZ=$2
+local msg_prefix=${AZ:+$AZ: }
+local d_prefix=${AZ:+$AZ/}
+
+local suffix=
+case $priority in
+backup) suffix=-backup ;;
+esac
+
+local northd_args=
+case ${NORTHD_TYPE:=ovn-northd} in
+ovn-northd) ;;
+ovn-northd-ddlog) 
northd_args="--ddlog-record=${AZ:+$AZ/}replay$suffix.dat -v" ;;
+esac
+
+local name=${d_prefix}northd${suffix}
+echo "${prefix}starting $name"
+test -d "$ovs_base/$name" || mkdir "$ovs_base/$name"
+as $name start_daemon $NORTHD_TYPE $northd_args -vjsonrpc \
+   --ovnnb-db=$OVN_NB_DB --ovnsb-db=$OVN_SB_DB
+}
+
+# ovn_start [--no-backup-northd] [AZ]
 #
 # Creates and initializes ovn-sb and ovn-nb databases and starts their
 # ovsdb-server instance, sets appropriate environment variables so that
 # ovn-sbctl and ovn-nbctl use them by default, and starts ovn-northd running
 # against them.
 ovn_start () {
-if test -n "$1"; then
-mkdir "$ovs_base"/$1
+local backup_northd=:
+case $1 in
+--no-backup-northd) backup_northd=false; shift ;;
+esac
+local AZ=$1
+local msg_prefix=${AZ:+$AZ: }
+local d_prefix=${AZ:+$AZ/}
+
+if test -n "$AZ"; then
+mkdir "$ovs_base"/$AZ
 fi
 
 ovn_init_db ovn-sb $1; ovn-sbctl init
@@ -150,36 +187,19 @@ ovn_start () {
 if test -n "$1"; then
 ovn-nbctl set NB_Global . name=$1
 fi
-local ovn_sb_db=$OVN_SB_DB
-local ovn_nb_db=$OVN_NB_DB
 
-local as_d=northd
-if test -n "$1"; then
-as_d=$1/$as_d
+ovn_start_northd primary $AZ
+if $backup_northd; then
+ovn_start_northd backup $AZ
 fi
-echo "starting ovn-northd"
-mkdir "$ovs_base"/$as_d
-as $as_d start_daemon ovn-northd -v \
-   --ovnnb-db=$ovn_nb_db \
-   --ovnsb-db=$ovn_sb_db
 
-as_d=northd-backup
-if test -n "$1"; then
-as_d=$1/$as_d
-fi
-echo "starting backup ovn-northd"
-mkdir "$ovs_base"/$as_d
-as $as_d start_daemon ovn-northd -v \
-   --ovnnb-db=$ovn_nb_db \
-   --ovnsb-db=$ovn_sb_db
+if test -n "$AZ"; then
+ovn-nbctl

[ovs-dev] [PATCH ovn 10/12] Export `VLOG_WARN` and `VLOG_ERR` from libovn for use in ddlog

2020-11-03 Thread Ben Pfaff

From: Leonid Ryzhyk 

Export `ddlog_warn` and `ddlog_err` functions that are just wrappers
around `VLOG_WARN` and `VLOG_ERR`.  This is not ideal because the
functions are exported by `ovn_util.c` and the resulting log messages use
`ovn_util` as module name.  More importantly, these functions do not do
log rate limiting.

Signed-off-by: Leonid Ryzhyk 
Signed-off-by: Ben Pfaff 
---
 lib/ovn-util.c | 17 +
 lib/ovn-util.h |  6 ++
 2 files changed, 23 insertions(+)

diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index abe6b04a7701..eb4f14efffa6 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -722,3 +722,20 @@ ip_address_and_port_from_lb_key(const char *key, char 
**ip_address,
 *addr_family = ss.ss_family;
 return true;
 }
+
+#ifdef DDLOG
+
+/* Callbacks used by the ddlog northd code to print warnings and errors.
+ */
+void
+ddlog_warn(const char *msg)
+{
+VLOG_WARN("%s", msg);
+}
+
+void
+ddlog_err(const char *msg)
+{
+VLOG_ERR("%s", msg);
+}
+#endif
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index a39cbef5a47e..77d0936a5fbc 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -230,4 +230,10 @@ char *str_tolower(const char *orig);
 bool ip_address_and_port_from_lb_key(const char *key, char **ip_address,
  uint16_t *port, int *addr_family);
 
+#ifdef DDLOG
+void ddlog_warn(const char *msg);
+void ddlog_err(const char *msg);
+#endif
+
+
 #endif
-- 
2.26.2

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

[ovs-dev] [PATCH ovn 07/12] Documentation: Update repo information and prerequisites.

2020-11-03 Thread Ben Pfaff

At the time this was written, there weren't any OVN releases independent
of OVS, but now there are so we should mention them.

Also, I don't think it's worth talking about compilers other than the
three listed.  We haven't tested with them.

Signed-off-by: Ben Pfaff 
---
 Documentation/intro/install/general.rst | 27 -
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 80f1c9dfb71f..75bf29ba6b75 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -39,12 +39,23 @@ repository, which you can clone into a directory named 
"ovn" with::
 
 Cloning the repository leaves the "master" branch initially checked
 out.  This is the right branch for general development.
+If, on the other hand, if you want to build a particular released
+version, you can check it out by running a command such as the
+following from the "ovs" directory::
 
-As of now there are no official OVN releases.
+$ git checkout v20.09.0
 
-Before building OVN you should configure and build OVS.
-Please see the Open vSwitch documentation 
(https://docs.openvswitch.org/en/latest/intro/install/)
-to build and install OVS https://github.com/openvswitch/ovs.git
+The repository also has a branch for each release series.  For
+example, to obtain the latest fixes in the Open vSwitch 2.7.x release
+series, which might include bug fixes that have not yet been in any
+released version, you can check it out from the "ovs" directory with::
+
+$ git checkout origin/branch-20.09
+
+If you do not want to use Git, you can also obtain tarballs for `OVN
+release versions `, or download a
+ZIP file for any snapshot from the `GitHub web interface
+`.
 
 .. _general-build-reqs:
 
@@ -54,9 +65,11 @@ Build Requirements
 To compile the userspace programs in the OVN distribution, you will
 need the following software:
 
+- Open vSwitch (https://docs.openvswitch.org/en/latest/intro/install/).
+
 - GNU make
 
-- A C compiler, such as:
+- One of the following C compilers:
 
   - GCC 4.6 or later.
 
@@ -65,10 +78,6 @@ need the following software:
   - MSVC 2013. Refer to :doc:`windows` for additional Windows build
 instructions.
 
-- While OVN may be compatible with other compilers, optimal support for atomic
-  operations may be missing, making OVN very slow
-  (see ``ovs/lib/ovs-atomic.h``).
-
 - libssl, from OpenSSL, is optional but recommended if you plan to connect the
   OVN services to the OVN DB ovsdb-servers securely. If libssl is installed,
   then OVN will automatically build with support for it.
-- 
2.26.2

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

[ovs-dev] [PATCH ovn 08/12] ovn-l7: Move ipv6_addr_is_routable_multicast() into new .c file.

2020-11-03 Thread Ben Pfaff

This makes it possible for the upcoming ovn-northd-ddlog program to call
into it from Rust.

Signed-off-by: Ben Pfaff 
---
 lib/automake.mk |  1 +
 lib/ovn-l7.c| 39 +++
 lib/ovn-l7.h| 19 +--
 3 files changed, 41 insertions(+), 18 deletions(-)
 create mode 100644 lib/ovn-l7.c

diff --git a/lib/automake.mk b/lib/automake.mk
index f3e9c8818b2b..d38d5c50c768 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -19,6 +19,7 @@ lib_libovn_la_SOURCES = \
lib/mcast-group-index.h \
lib/lex.c \
lib/ovn-l7.h \
+   lib/ovn-l7.c \
lib/ovn-util.c \
lib/ovn-util.h \
lib/logical-fields.c \
diff --git a/lib/ovn-l7.c b/lib/ovn-l7.c
new file mode 100644
index ..3a5f3f3ec02e
--- /dev/null
+++ b/lib/ovn-l7.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2020 Red Hat.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+
+#include "ovn-l7.h"
+
+bool
+ipv6_addr_is_routable_multicast(const struct in6_addr *ip)
+{
+if (!ipv6_addr_is_multicast(ip)) {
+return false;
+}
+
+/* Check multicast group scope, RFC 4291, 2.7. */
+switch (ip->s6_addr[1] & 0x0F) {
+case 0x00:
+case 0x01:
+case 0x02:
+case 0x03:
+case 0x0F:
+return false;
+default:
+return true;
+}
+}
diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h
index c3e8fd660983..a3bfd4e539e3 100644
--- a/lib/ovn-l7.h
+++ b/lib/ovn-l7.h
@@ -420,24 +420,7 @@ controller_event_opts_destroy(struct 
controller_event_options *opts)
 }
 }
 
-static inline bool
-ipv6_addr_is_routable_multicast(const struct in6_addr *ip) {
-if (!ipv6_addr_is_multicast(ip)) {
-return false;
-}
-
-/* Check multicast group scope, RFC 4291, 2.7. */
-switch (ip->s6_addr[1] & 0x0F) {
-case 0x00:
-case 0x01:
-case 0x02:
-case 0x03:
-case 0x0F:
-return false;
-default:
-return true;
-}
-}
+bool ipv6_addr_is_routable_multicast(const struct in6_addr *);
 
 static inline bool
 ipv6_addr_is_host_zero(const struct in6_addr *prefix,
-- 
2.26.2

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

[ovs-dev] [PATCH ovn 03/12] tests: Fix port numbering in "1 LR with distributed gateway port".

2020-11-03 Thread Ben Pfaff

This test assumed that datapaths and ports were assigned particular
tunnel keys, but this isn't guaranteed.  Fix the problem by requesting
the required keys, with options:requested-tnl-key and
other_config:requested-tnl-key.

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 130 ---
 1 file changed, 71 insertions(+), 59 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 6282d0a6bb8a..8686e133241c 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10051,41 +10051,47 @@ check ovs-vsctl -- add-port br-int hv3-vif1 -- \
 # for ARP resolution).
 OVN_POPULATE_ARP
 
-ovn-nbctl create Logical_Router name=R1
+check ovn-nbctl lr-add R1 -- set Logical_Router R1 options:requested-tnl-key=1
 
-check ovn-nbctl ls-add foo
-check ovn-nbctl ls-add alice
-check ovn-nbctl ls-add outside
+check ovn-nbctl ls-add foo -- set Logical_Switch foo 
other_config:requested-tnl-key=2
+check ovn-nbctl ls-add alice -- set Logical_Switch alice 
other_config:requested-tnl-key=3
+check ovn-nbctl ls-add outside -- set Logical_Switch outside 
other_config:requested-tnl-key=4
 
 # Connect foo to R1
-check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24
-check ovn-nbctl lsp-add foo rp-foo -- set Logical_Switch_Port rp-foo \
-type=router options:router-port=foo \
+check ovn-nbctl lrp-add R1 foo 00:00:01:01:02:03 192.168.1.1/24 \
+-- set Logical_Router_Port foo options:requested-tnl-key=1
+check ovn-nbctl lsp-add foo rp-foo \
+-- set Logical_Switch_Port rp-foo type=router options:router-port=foo 
options:requested-tnl-key=1 \
 -- lsp-set-addresses rp-foo router
 
 # Connect alice to R1 as distributed router gateway port on hv2
 check ovn-nbctl lrp-add R1 alice 00:00:02:01:02:03 172.16.1.1/24 \
+-- set Logical_Router_Port alice options:requested-tnl-key=2 \
 -- lrp-set-gateway-chassis alice hv2
-check ovn-nbctl lsp-add alice rp-alice -- set Logical_Switch_Port rp-alice \
-type=router options:router-port=alice \
+check ovn-nbctl lsp-add alice rp-alice \
+-- set Logical_Switch_Port rp-alice type=router options:router-port=alice 
options:requested-tnl-key=1 \
 -- lsp-set-addresses rp-alice router
 
 # Create logical port foo1 in foo
 check ovn-nbctl lsp-add foo foo1 \
+-- set Logical_Switch_Port foo1 options:requested-tnl-key=2 \
 -- lsp-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2"
 
 # Create logical port outside1 in outside
 check ovn-nbctl lsp-add outside outside1 \
+-- set Logical_Switch_Port outside1 options:requested-tnl-key=1 \
 -- lsp-set-addresses outside1 "f0:00:00:01:02:04 172.16.1.3"
 
 # Create localnet port in alice
-check ovn-nbctl lsp-add alice ln-alice
+check ovn-nbctl --wait=sb lsp-add alice ln-alice \
+-- set Logical_Switch_Port ln-alice options:requested-tnl-key=2
 check ovn-nbctl lsp-set-addresses ln-alice unknown
 check ovn-nbctl lsp-set-type ln-alice localnet
 check ovn-nbctl lsp-set-options ln-alice network_name=phys
 
 # Create localnet port in outside
-check ovn-nbctl lsp-add outside ln-outside
+check ovn-nbctl --wait=sb lsp-add outside ln-outside \
+-- set Logical_Switch_Port ln-outside options:requested-tnl-key=2
 check ovn-nbctl lsp-set-addresses ln-outside unknown
 check ovn-nbctl lsp-set-type ln-outside localnet
 check ovn-nbctl lsp-set-options ln-outside network_name=phys
@@ -10098,60 +10104,66 @@ check as hv3 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 dnl Allow some time for ovn-northd and ovn-controller to catch up.
 ovn-nbctl --wait=hv sync
 
-echo "-NB dump-"
-ovn-nbctl show
-echo "-"
-ovn-nbctl list logical_router
-echo "-"
-ovn-nbctl list logical_router_port
-echo "-"
+(echo "-NB dump-"
+ ovn-nbctl show
+ echo "-"
+ ovn-nbctl list logical_router
+ echo "-"
+ ovn-nbctl list logical_router_port) > nbdump
+AT_CAPTURE_FILE([nbdump])
 
-echo "-SB dump-"
-ovn-sbctl list datapath_binding
-echo "-"
-ovn-sbctl list port_binding
-echo "-"
-ovn-sbctl dump-flows
-echo "-"
-ovn-sbctl list chassis
-ovn-sbctl list encap
-echo "-- Gateway_Chassis dump (SBDB) ---"
-ovn-sbctl list Gateway_Chassis
-echo "-- Port_Binding chassisredirect ---"
-ovn-sbctl find Port_Binding type=chassisredirect
-echo "---"
+(echo "-SB dump-"
+ ovn-sbctl list datapath_binding
+ echo "-"
+ ovn-sbctl list port_binding
+ echo "-"
+ ovn-sbctl list chassis
+ ovn-sbctl list encap
+ echo "-- Gateway_Chassis dump (SBDB) ---"
+ ovn-sbctl list Gateway_Chassis
+ echo "-- Port_Binding chassisredirect ---"
+ ovn-sbctl find Port_Binding type=chassisredirect) > sbdump
+AT_CAPTURE_FILE([sbdump])
 
-echo "-- hv1 dump --"
-as hv1 ovs-ofctl show br-int
-as hv1 ovs-ofctl dump-flows br-int
-echo

[ovs-dev] [PATCH ovn 05/12] tests: Improve "check allowed/disallowed external dnat..." test.

2020-11-03 Thread Ben Pfaff

This makes it more debuggable.

Signed-off-by: Ben Pfaff 
---
 tests/ovn-northd.at | 116 +++-
 1 file changed, 62 insertions(+), 54 deletions(-)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index ae845e4eafd4..9e7d8750f8fd 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -820,84 +820,92 @@ ovn_start
 #
 # DR is connected to S1 and CR is connected to S2
 
-ovn-sbctl chassis-add gw1 geneve 127.0.0.1
+check ovn-sbctl chassis-add gw1 geneve 127.0.0.1
 
-ovn-nbctl lr-add DR
-ovn-nbctl lrp-add DR DR-S1 02:ac:10:01:00:01 172.16.1.1/24
+check ovn-nbctl lr-add DR
+check ovn-nbctl lrp-add DR DR-S1 02:ac:10:01:00:01 172.16.1.1/24
 
 cr_uuid=$(ovn-nbctl create Logical_Router name=CR)
-ovn-nbctl lrp-add CR CR-S2 02:ac:10:01:00:01 172.16.1.1/24
+check ovn-nbctl lrp-add CR CR-S2 02:ac:10:01:00:01 172.16.1.1/24
 
-ovn-nbctl ls-add S1
-ovn-nbctl lsp-add S1 S1-DR
-ovn-nbctl lsp-set-type S1-DR router
-ovn-nbctl lsp-set-addresses S1-DR router
-ovn-nbctl --wait=sb lsp-set-options S1-DR router-port=DR-S1
+check ovn-nbctl ls-add S1
+check ovn-nbctl lsp-add S1 S1-DR
+check ovn-nbctl lsp-set-type S1-DR router
+check ovn-nbctl lsp-set-addresses S1-DR router
+check ovn-nbctl --wait=sb lsp-set-options S1-DR router-port=DR-S1
 
-ovn-nbctl ls-add S2
-ovn-nbctl lsp-add S2 S2-CR
-ovn-nbctl lsp-set-type S2-CR router
-ovn-nbctl lsp-set-addresses S2-CR router
-ovn-nbctl --wait=sb lsp-set-options S2-CR router-port=CR-S2
+check ovn-nbctl ls-add S2
+check ovn-nbctl lsp-add S2 S2-CR
+check ovn-nbctl lsp-set-type S2-CR router
+check ovn-nbctl lsp-set-addresses S2-CR router
+check ovn-nbctl --wait=sb lsp-set-options S2-CR router-port=CR-S2
 
-ovn-nbctl lrp-set-gateway-chassis DR-S1 gw1
+check ovn-nbctl lrp-set-gateway-chassis DR-S1 gw1
 
-uuid=`ovn-sbctl --columns=_uuid --bare find Port_Binding logical_port=cr-DR-S1`
+uuid=$(fetch_column Port_Binding _uuid logical_port=cr-DR-S1)
 echo "CR-LRP UUID is: " $uuid
 
-ovn-nbctl set Logical_Router $cr_uuid options:chassis=gw1
-ovn-nbctl --wait=hv sync
+check ovn-nbctl set Logical_Router $cr_uuid options:chassis=gw1
+check ovn-nbctl --wait=hv sync
 
 ovn-nbctl create Address_Set name=allowed_range addresses=\"1.1.1.1\"
 ovn-nbctl create Address_Set name=disallowed_range addresses=\"2.2.2.2\"
 
 # SNAT with ALLOWED_IPs
-ovn-nbctl lr-nat-add DR snat  172.16.1.1 50.0.0.11
-ovn-nbctl lr-nat-update-ext-ip DR snat 50.0.0.11 allowed_range
+check ovn-nbctl lr-nat-add DR snat  172.16.1.1 50.0.0.11
+check ovn-nbctl lr-nat-update-ext-ip DR snat 50.0.0.11 allowed_range
 
-ovn-nbctl lr-nat-add CR snat  172.16.1.1 50.0.0.11
-ovn-nbctl lr-nat-update-ext-ip CR snat 50.0.0.11 allowed_range
+check ovn-nbctl lr-nat-add CR snat  172.16.1.1 50.0.0.11
+check ovn-nbctl lr-nat-update-ext-ip CR snat 50.0.0.11 allowed_range
 
-OVS_WAIT_UNTIL([test 3 = `ovn-sbctl dump-flows DR | grep lr_out_snat | wc -l`])
-OVS_WAIT_UNTIL([test 3 = `ovn-sbctl dump-flows CR | grep lr_out_snat | wc -l`])
+check ovn-nbctl --wait=sb sync
 
-AT_CHECK([ovn-sbctl dump-flows DR | grep lr_out_snat | grep "ip4.src == 
50.0.0.11" | grep "ip4.dst == $allowed_range" | wc -l], [0], [1
-])
-AT_CHECK([ovn-sbctl dump-flows CR | grep lr_out_snat | grep "ip4.src == 
50.0.0.11" | grep "ip4.dst == $allowed_range" | wc -l], [0], [1
+ovn-sbctl dump-flows DR > drflows
+AT_CAPTURE_FILE([drflows])
+ovn-sbctl dump-flows CR > crflows
+AT_CAPTURE_FILE([crflows])
+
+AT_CHECK([
+  grep -c lr_out_snat drflows
+  grep -c lr_out_snat crflows
+  grep lr_out_snat drflows | grep "ip4.src == 50.0.0.11" | grep -c "ip4.dst == 
$allowed_range"
+  grep lr_out_snat crflows | grep "ip4.src == 50.0.0.11" | grep -c "ip4.dst == 
$allowed_range"], [0], [dnl
+3
+3
+1
+1
 ])
 
 # SNAT with DISALLOWED_IPs
-ovn-nbctl lr-nat-del DR snat  50.0.0.11
-ovn-nbctl lr-nat-del CR snat  50.0.0.11
-
-ovn-nbctl lr-nat-add DR snat  172.16.1.1 50.0.0.11
-ovn-nbctl lr-nat-add CR snat  172.16.1.1 50.0.0.11
+check ovn-nbctl lr-nat-del DR snat  50.0.0.11
+check ovn-nbctl lr-nat-del CR snat  50.0.0.11
 
-ovn-nbctl --is-exempted lr-nat-update-ext-ip DR snat 50.0.0.11 disallowed_range
-ovn-nbctl --is-exempted lr-nat-update-ext-ip CR snat 50.0.0.11 disallowed_range
+check ovn-nbctl lr-nat-add DR snat  172.16.1.1 50.0.0.11
+check ovn-nbctl lr-nat-add CR snat  172.16.1.1 50.0.0.11
 
-ovn-sbctl dump-flows DR
-ovn-sbctl dump-flows CR
+check ovn-nbctl --is-exempted lr-nat-update-ext-ip DR snat 50.0.0.11 
disallowed_range
+check ovn-nbctl --is-exempted lr-nat-update-ext-ip CR snat 50.0.0.11 
disallowed_range
 
-OVS_WAIT_UNTIL([test 4 = `ovn-sbctl dump-flows DR | grep lr_out_snat | \
-wc -l`])
-OVS_WAIT_UNTIL([test 4 = `ovn-sbctl dump-flows CR | grep lr_out_snat | \
-wc -l`])
-
-ovn-nbctl show DR
-ovn-sbctl dump-flows DR
-
-ovn-nbctl show CR
-ovn-sbctl dump-flows CR
-
-AT_CHECK([ovn-sbctl dump-flows DR | grep lr_out_snat | grep "ip4.src == 
50.0.0.11" | grep "ip4.dst == $disallowed_range" | grep "priority=162" | wc 
-l], [0], [1
-])
-AT_CHECK([ovn-sbctl

[ovs-dev] [PATCH ovn 00/12] DDlog implementation of ovn-northd

2020-11-03 Thread Ben Pfaff

This is also available at:
https://github.com/blp/ovs-reviews/tree/ddlog7

Ben Pfaff (9):
  tests: Improve "dhcpv4 : 1 HV, 2 LS, 2 LSPs/LS".
  tests: Improve "ipam" test.
  tests: Fix port numbering in "1 LR with distributed gateway port".
  tests: More careful checking.
  tests: Improve "check allowed/disallowed external dnat..." test.
  tests: Improve "reject ACL" test.
  Documentation: Update repo information and prerequisites.
  ovn-l7: Move ipv6_addr_is_routable_multicast() into new .c file.
  tests: Prepare for multiple northd types.

Justin Pettit (1):
  northd: Move functions from ovn-northd.c into ovn-util.

Leonid Ryzhyk (2):
  Export `VLOG_WARN` and `VLOG_ERR` from libovn for use in ddlog
  ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.

 Documentation/automake.mk |2 +
 Documentation/intro/install/general.rst   |   58 +-
 Documentation/topics/debugging-ddlog.rst  |  280 +
 Documentation/topics/index.rst|1 +
 Documentation/tutorials/ddlog-new-feature.rst |  362 +
 Documentation/tutorials/index.rst |1 +
 NEWS  |6 +
 acinclude.m4  |   43 +
 configure.ac  |5 +
 lib/automake.mk   |1 +
 lib/ovn-l7.c  |   39 +
 lib/ovn-l7.h  |   19 +-
 lib/ovn-util.c|   96 +-
 lib/ovn-util.h|   17 +
 m4/ovn.m4 |   16 +
 northd/.gitignore |4 +
 northd/automake.mk|  104 +
 northd/helpers.dl |  114 +
 northd/ipam.dl|  492 ++
 northd/lrouter.dl |  701 ++
 northd/lswitch.dl |  607 ++
 northd/multicast.dl   |  260 +
 northd/ovn-nb.dlopts  |   13 +
 northd/ovn-northd-ddlog.c | 1752 
 northd/ovn-northd.c   |   51 -
 northd/ovn-sb.dlopts  |   28 +
 northd/ovn.dl |  373 +
 northd/ovn.rs |  843 ++
 northd/ovn.toml   |2 +
 northd/ovn_northd.dl  | 7456 +
 northd/ovsdb2ddlog2c  |  127 +
 tests/atlocal.in  |8 +
 tests/network-functions.at|   18 +
 tests/ovn-ic.at   |   11 +-
 tests/ovn-macros.at   |   99 +-
 tests/ovn-northd.at   |  432 +-
 tests/ovn.at  | 1021 +--
 tests/ovs-macros.at   |   20 +-
 tests/system-ovn.at   |   50 +-
 tutorial/ovs-sandbox  |   24 +-
 utilities/checkpatch.py   |2 +-
 utilities/ovn-ctl |   20 +-
 42 files changed, 14794 insertions(+), 784 deletions(-)
 create mode 100644 Documentation/topics/debugging-ddlog.rst
 create mode 100644 Documentation/tutorials/ddlog-new-feature.rst
 create mode 100644 lib/ovn-l7.c
 create mode 100644 northd/helpers.dl
 create mode 100644 northd/ipam.dl
 create mode 100644 northd/lrouter.dl
 create mode 100644 northd/lswitch.dl
 create mode 100644 northd/multicast.dl
 create mode 100644 northd/ovn-nb.dlopts
 create mode 100644 northd/ovn-northd-ddlog.c
 create mode 100644 northd/ovn-sb.dlopts
 create mode 100644 northd/ovn.dl
 create mode 100644 northd/ovn.rs
 create mode 100644 northd/ovn.toml
 create mode 100644 northd/ovn_northd.dl
 create mode 100755 northd/ovsdb2ddlog2c

-- 
2.26.2

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

[ovs-dev] [PATCH ovn 01/12] tests: Improve "dhcpv4 : 1 HV, 2 LS, 2 LSPs/LS".

2020-11-03 Thread Ben Pfaff

This change makes the test much more debuggable:

- Make it easier to find the particular failing test.

- Use correct checksums so that the packets can be compared for
  equality instead of omitting the checksum field.

- Factor out packet comparison and produce better error messages on
  failure.

Signed-off-by: Ben Pfaff 
---
 tests/network-functions.at |  18 
 tests/ovn.at   | 184 -
 2 files changed, 75 insertions(+), 127 deletions(-)

diff --git a/tests/network-functions.at b/tests/network-functions.at
index a149e9da4c58..c583bc31e881 100644
--- a/tests/network-functions.at
+++ b/tests/network-functions.at
@@ -52,6 +52,24 @@ test_csum 45764001c3d90a03aca80003 
 AT_CLEANUP
 
 OVS_START_SHELL_HELPERS
+# ip4_csum_inplace IP4_HEADER
+#
+# Outputs IP4_HEADER with the checksum filled in properly.
+# The checksum must initially be .  IP4_HEADER must be
+# 40 hex digits.
+ip4_csum_inplace() {
+local csum=$(ip_csum $1)
+echo "$1" | sed "s/^\(\)/\1$csum/"
+}
+OVS_END_SHELL_HELPERS
+
+AT_SETUP([ip4_csum_inplace])
+AT_CHECK([ip4_csum_inplace 457340004011c0a80001c0a800c7], [0],
+[457340004011b861c0a80001c0a800c7
+])
+AT_CLEANUP
+
+OVS_START_SHELL_HELPERS
 # ip6_pseudoheader IP6_HEADER NEXT_HEADER PAYLOAD_LEN
 #
 # where:
diff --git a/tests/ovn.at b/tests/ovn.at
index 4be484051966..5666058d99ca 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5294,7 +5294,12 @@ sleep 2
 as hv1 ovs-vsctl show
 
 # This shell function sends a DHCP request packet
-# test_dhcp INPORT SRC_MAC DHCP_TYPE BROADCAST CIADDR OFFER_IP REQUEST_IP 
ETH_BOOT USE_IP ...
+#
+# The first argument is just the number of calls to this function so
+# far (1, 2, ...).  This is redundant, but it makes it easier to find
+# the failures by searching for the number.
+#
+# test_dhcp PACKET_NUM INPORT SRC_MAC DHCP_TYPE BROADCAST CIADDR OFFER_IP 
REQUEST_IP ETH_BOOT USE_IP ...
 packet_num=0
 test_dhcp() {
 local expect_resume=:
@@ -5308,6 +5313,10 @@ test_dhcp() {
 esac
 done
 
+packet_num=$(expr $packet_num + 1)
+AT_FAIL_IF([test $packet_num != $1])
+shift
+
 local inport=$1 src_mac=$2 dhcp_type=$3 broadcast=$4 ciaddr=$5 offer_ip=$6 
request_ip=$7 eth_boot=$8 use_ip=$9
 shift; shift; shift; shift; shift; shift; shift; shift; shift;
 
@@ -5319,7 +5328,6 @@ test_dhcp() {
 src_ip=`ip_to_hex 0 0 0 0`
 dst_ip=`ip_to_hex 255 255 255 255`
 fi
-packet_num=$(expr $packet_num + 1)
 
 AS_BOX([dhcp test packet $packet_num])
 
@@ -5415,7 +5423,9 @@ test_dhcp() {
 ip_len=$(printf "%x" $ip_len)
 udp_len=$(printf "%x" $udp_len)
 # $ip_len var will be in 3 digits i.e 134. So adding a '0' before 
$ip_len
-local 
reply=${src_mac}${srv_mac}080045100${ip_len}8011${srv_ip}${reply_dst_ip}
+local reply=${src_mac}${srv_mac}0800
+local ip_header=45100${ip_len}8011${srv_ip}${reply_dst_ip}
+reply=${reply}$(ip4_csum_inplace $ip_header)
 # udp header and dhcp header.
 # $udp_len var will be in 3 digits. So adding a '0' before $udp_len
 
reply=${reply}004300440${udp_len}020106006359aa76${flags}${ciaddr}
@@ -5460,6 +5470,16 @@ test_dhcp() {
 fi
 }
 
+compare_dhcp_packets() {
+received=$($PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif$1-tx.pcap)
+expected=$(cat $1.expected)
+
+if test "$received" != "$expected"; then
+AT_CHECK_UNQUOTED([echo "$received"; tcpdump_hex "$received"], [0],
+[$(echo "$expected"; tcpdump_hex "$expected")])
+fi
+}
+
 reset_pcap_file() {
 local iface=$1
 local pcap_file=$2
@@ -5481,14 +5501,8 @@ server_ip=`ip_to_hex 10 0 0 1`
 ciaddr=`ip_to_hex 0 0 0 0`
 request_ip=0
 expected_dhcp_opts=33040e100104ff0003040a0136040a01
-test_dhcp 1 f001 01 0 $ciaddr $offer_ip $request_ip 0 0 ff11 
$server_ip 02 $expected_dhcp_opts
-
-$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif1-tx.pcap > 1.packets
-cat 1.expected | cut -c -48 > expout
-AT_CHECK([cat 1.packets | cut -c -48], [0], [expout])
-# Skipping the IPv4 checksum.
-cat 1.expected | cut -c 53- > expout
-AT_CHECK([cat 1.packets | cut -c 53-], [0], [expout])
+test_dhcp 1 1 f001 01 0 $ciaddr $offer_ip $request_ip 0 0 ff11 
$server_ip 02 $expected_dhcp_opts
+compare_dhcp_packets 1
 
 # --
 
@@ -5506,14 +5520,8 @@ server_ip=`ip_to_hex 10 0 0 1`
 ciaddr=`ip_to_hex 0 0 0 0`
 request_ip=$offer_ip
 expected_dhcp_opts=33040e100104ff0003040a0136040a01
-test_dhcp 2 f002 03 0 $ciaddr $offer_ip $request_ip 0 0 ff11 
$server_ip 05 $expected_dhcp_opts
-
-$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets
-cat 2.expected | cut -c -48 > expout
-AT_CHECK([cat 2.packets | cut -c -48], [0], [expout])
-# Skipping the IPv4

[ovs-dev] [PATCH ovn 04/12] tests: More careful checking.

2020-11-03 Thread Ben Pfaff

This commit adds further use of "check" and other ways to make sure that
commands run during a test succeed.

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 125 +--
 1 file changed, 52 insertions(+), 73 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 8686e133241c..ffd86270f7c1 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -17125,10 +17125,7 @@ send_igmp_v3_report hv1-vif4 hv1 \
 /dev/null
 
 # Check that the IGMP Group is learned by all switches.
-OVS_WAIT_UNTIL([
-total_entries=`ovn-sbctl find IGMP_Group | grep "239.0.1.68" -c`
-test "${total_entries}" = "3"
-])
+wait_row_count IGMP_Group 3 address=239.0.1.68
 check ovn-nbctl --wait=hv sync
 
 # Send traffic from sw3 and make sure it is relayed by rtr
@@ -17191,10 +17188,7 @@ OVS_WAIT_UNTIL(
 ovn-sbctl ip-multicast-flush sw1
 ovn-sbctl ip-multicast-flush sw2
 ovn-sbctl ip-multicast-flush sw3
-OVS_WAIT_UNTIL([
-total_entries=`ovn-sbctl find IGMP_Group | grep "239.0.1.68" -c`
-test "${total_entries}" = "0"
-])
+wait_row_count IGMP_Group 0 address=239.0.1.68
 check ovn-nbctl --wait=hv sync
 
 as hv1 reset_pcap_file hv1-vif1 hv1/vif1
@@ -17230,10 +17224,7 @@ send_igmp_v3_report hv1-vif2 hv1 \
 expected_reports
 
 # Check that the IGMP Group is learned.
-OVS_WAIT_UNTIL([
-total_entries=`ovn-sbctl find IGMP_Group | grep "239.0.1.68" -c`
-test "${total_entries}" = "1"
-])
+wait_row_count IGMP_Group 1 address=239.0.1.68
 check ovn-nbctl --wait=hv sync
 
 # Send traffic from sw1-p21
@@ -17283,30 +17274,18 @@ as hv2 ovs-vsctl set open . 
external_ids:ovn-monitor-all=true
 as hv3 ovs-vsctl set open . external_ids:ovn-monitor-all=true
 
 # Wait until ovn-monitor-all is processed by ovn-controller.
-OVS_WAIT_UNTIL([
-test $(ovn-sbctl get chassis hv1 other_config:ovn-monitor-all) = '"true"'
-])
-OVS_WAIT_UNTIL([
-test $(ovn-sbctl get chassis hv2 other_config:ovn-monitor-all) = '"true"'
-])
-OVS_WAIT_UNTIL([
-test $(ovn-sbctl get chassis hv3 other_config:ovn-monitor-all) = '"true"'
-])
+wait_row_count Chassis 1 name=hv1 other_config:ovn-monitor-all='"true"'
+wait_row_count Chassis 1 name=hv2 other_config:ovn-monitor-all='"true"'
+wait_row_count Chassis 1 name=hv3 other_config:ovn-monitor-all='"true"'
 
 # Inject a fake IGMP_Group entry.
-dp=$(ovn-sbctl --bare --columns _uuid list Datapath_Binding sw3)
-ch=$(ovn-sbctl --bare --columns _uuid list Chassis hv3)
-ovn-sbctl create IGMP_Group address="239.0.1.42" datapath=$dp chassis=$ch
+dp=$(fetch_column Datapath_Binding _uuid external_ids:name=sw2)
+ch=$(fetch_column Chassis _uuid name=hv3)
+ovn-sbctl create IGMP_Group address=239.0.1.42 datapath=$dp chassis=$ch
 
 ovn-nbctl --wait=hv sync
-OVS_WAIT_UNTIL([
-total_entries=`ovn-sbctl find IGMP_Group | grep "239.0.1.68" -c`
-test "${total_entries}" = "1"
-])
-OVS_WAIT_UNTIL([
-total_entries=`ovn-sbctl find IGMP_Group | grep "239.0.1.42" -c`
-test "${total_entries}" = "1"
-])
+wait_row_count IGMP_Group 1 address=239.0.1.68
+wait_row_count IGMP_Group 1 address=239.0.1.42
 
 OVN_CLEANUP([hv1], [hv2], [hv3])
 AT_CLEANUP
@@ -17443,28 +17422,28 @@ store_ip_multicast_pkt() {
 echo ${packet} >> ${outfile}
 }
 
-ovn-nbctl ls-add sw1
-ovn-nbctl ls-add sw2
-ovn-nbctl ls-add sw3
-
-ovn-nbctl lsp-add sw1 sw1-p11
-ovn-nbctl lsp-add sw1 sw1-p12
-ovn-nbctl lsp-add sw1 sw1-p21
-ovn-nbctl lsp-add sw1 sw1-p22
-ovn-nbctl lsp-add sw2 sw2-p1
-ovn-nbctl lsp-add sw2 sw2-p2
-ovn-nbctl lsp-add sw3 sw3-p1
-ovn-nbctl lsp-add sw3 sw3-p2
-ovn-nbctl lsp-add sw3 sw3-ln\
+check ovn-nbctl ls-add sw1
+check ovn-nbctl ls-add sw2
+check ovn-nbctl ls-add sw3
+
+check ovn-nbctl lsp-add sw1 sw1-p11
+check ovn-nbctl lsp-add sw1 sw1-p12
+check ovn-nbctl lsp-add sw1 sw1-p21
+check ovn-nbctl lsp-add sw1 sw1-p22
+check ovn-nbctl lsp-add sw2 sw2-p1
+check ovn-nbctl lsp-add sw2 sw2-p2
+check ovn-nbctl lsp-add sw3 sw3-p1
+check ovn-nbctl lsp-add sw3 sw3-p2
+check ovn-nbctl lsp-add sw3 sw3-ln  \
 -- lsp-set-type sw3-ln localnet \
 -- lsp-set-options sw3-ln network_name=phys
 
-ovn-nbctl lr-add rtr
-ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10::fe/64
-ovn-nbctl lrp-add rtr rtr-sw2 00:00:00:00:02:00 20::fe/64
-ovn-nbctl lrp-add rtr rtr-sw3 00:00:00:00:03:00 30::fe/64
+check ovn-nbctl lr-add rtr
+check ovn-nbctl lrp-add rtr rtr-sw1 00:00:00:00:01:00 10::fe/64
+check ovn-nbctl lrp-add rtr rtr-sw2 00:00:00:00:02:00 20::fe/64
+check ovn-nbctl lrp-add rtr rtr-sw3 00:00:00:00:03:00 30::fe/64
 
-ovn-nbctl lsp-add sw1 sw1-rtr  \
+check ovn-nbctl lsp-add sw1 sw1-rtr\
 -- lsp-set-type sw1-rtr router \
 -- lsp-set-addresses sw1-rtr 00:00:00:00:01:00 \
 -- lsp-set-options sw1-rtr router-port=rtr-sw1
@@ -17480,17 +17459,17 @@ check ovn-nbctl lsp-add sw3 sw3-rtr\
 # Conntrack marks all IPv6 Neighbor Discovery and MLD packets as invalid,
 # make sure to test that conntrack is

[ovs-dev] [PATCH ovn 02/12] tests: Improve "ipam" test.

2020-11-03 Thread Ben Pfaff

It's easier to follow this way.

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 271 ++-
 1 file changed, 72 insertions(+), 199 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 5666058d99ca..6282d0a6bb8a 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -7031,95 +7031,54 @@ AT_CLEANUP
 AT_SETUP([ovn -- ipam])
 ovn_start
 
+check_dynamic_addresses() {
+local arg
+case $2 in
+('') arg='[[]]' ;;
+(*) arg="\"$2\"" ;;
+esac
+check_row_count nb:Logical_Switch_Port 1 name="$1" dynamic_addresses="$arg"
+}
+
 # Add a port to a switch that does not have a subnet set, then set the
 # subnet which should result in an address being allocated for the port.
 ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
 ovn-nbctl ls-add sw0
 ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
 ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=192.168.1.0/24
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p0 dynamic_addresses], [0],
-["0a:00:00:a8:01:03 192.168.1.2"
-])
+check_dynamic_addresses p0 "0a:00:00:a8:01:03 192.168.1.2"
 
 # Add 9 more ports to sw0, addresses should all be unique.
 for n in `seq 1 9`; do
 ovn-nbctl --wait=sb lsp-add sw0 "p$n" -- lsp-set-addresses "p$n" dynamic
 done
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p1 dynamic_addresses], [0],
-["0a:00:00:a8:01:04 192.168.1.3"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p2 dynamic_addresses], [0],
-["0a:00:00:a8:01:05 192.168.1.4"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p3 dynamic_addresses], [0],
-["0a:00:00:a8:01:06 192.168.1.5"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p4 dynamic_addresses], [0],
-["0a:00:00:a8:01:07 192.168.1.6"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p5 dynamic_addresses], [0],
-["0a:00:00:a8:01:08 192.168.1.7"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p6 dynamic_addresses], [0],
-["0a:00:00:a8:01:09 192.168.1.8"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p7 dynamic_addresses], [0],
-["0a:00:00:a8:01:0a 192.168.1.9"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p8 dynamic_addresses], [0],
-["0a:00:00:a8:01:0b 192.168.1.10"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p9 dynamic_addresses], [0],
-["0a:00:00:a8:01:0c 192.168.1.11"
-])
+for i in `seq 1 9`; do
+mac=0a:00:00:a8:01:$(printf "%02x" $(expr $i + 3))
+ip=192.168.1.$(expr $i + 2)
+check_dynamic_addresses p$i "$mac $ip"
+done
 
 # Trying similar tests with a second switch. MAC addresses should be unique
 # across both switches but IP's only need to be unique within the same switch.
 ovn-nbctl ls-add sw1
 ovn-nbctl lsp-add sw1 p10 -- lsp-set-addresses p10 dynamic
 ovn-nbctl --wait=sb add Logical-Switch sw1 other_config subnet=192.168.1.0/24
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p10 dynamic_addresses], [0],
- ["0a:00:00:a8:01:0d 192.168.1.2"
-])
+check_row_count nb:Logical_Switch_Port 1 name=p10 
dynamic_addresses='"0a:00:00:a8:01:0d 192.168.1.2"'
 
 for n in `seq 11 19`; do
 ovn-nbctl --wait=sb lsp-add sw1 "p$n" -- lsp-set-addresses "p$n" dynamic
 done
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p11 dynamic_addresses], [0],
- ["0a:00:00:a8:01:0e 192.168.1.3"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p12 dynamic_addresses], [0],
- ["0a:00:00:a8:01:0f 192.168.1.4"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p13 dynamic_addresses], [0],
- ["0a:00:00:a8:01:10 192.168.1.5"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p14 dynamic_addresses], [0],
- ["0a:00:00:a8:01:11 192.168.1.6"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p15 dynamic_addresses], [0],
- ["0a:00:00:a8:01:12 192.168.1.7"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p16 dynamic_addresses], [0],
- ["0a:00:00:a8:01:13 192.168.1.8"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p17 dynamic_addresses], [0],
- ["0a:00:00:a8:01:14 192.168.1.9"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p18 dynamic_addresses], [0],
- ["0a:00:00:a8:01:15 192.168.1.10"
-])
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p19 dynamic_addresses], [0],
- ["0a:00:00:a8:01:16 192.168.1.11"
-])
+for i in `seq 11 19`; do
+mac=0a:00:00:a8:01:$(printf "%02x" $(expr $i + 3))
+ip=192.168.1.$(expr $i - 8)
+check_dynamic_addresses p$i "$mac $ip"
+done
 
 # Change a port's address to test for multiple ip's for a single address entry
 # and addresses set by the user.
 ovn-nbctl lsp-set-addresses p0 "0a:00:00:a8:01:17 192.168.1.2 192.168.1.12 
192.168.1.14"
 ovn-nbctl --wait=sb lsp-add sw0 p20 -- lsp-set-addresses p20 dynamic
-AT_CHECK([ovn-nbctl get Logical-Switch-Port p20 dynamic_addresses], [0],
- ["0a:00:00:a8:01:18 192.168.1.13"
-])
+check_dynamic_addresses p20 "0a:00:00:a8:01:18 192.168.1.13"
 
 # Test for logical router port address management.
 ovn-nbctl create Logical_Router name=R1
@@ -7128,36 +7087,26 @@ network="192.168.1.1/24"

[ovs-dev] Apply For Loan!

2020-11-03 Thread aaaa aaaa

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

Re: [ovs-dev] [PATCH 1/2] compat: Fix build issue on RHEL 7.7

2020-11-03 Thread Yi-Hung Wei

On Tue, Nov 3, 2020 at 4:06 PM Gregory Rose  wrote:
> On 11/3/2020 2:17 PM, Yi-Hung Wei wrote:
> > On Tue, Oct 20, 2020 at 10:07 AM Greg Rose  wrote:
> >>
> >> RHEL 7.7 has a KABI fixup in struct sk_buff to backport the member
> >> name change of l4_rxhash to l4_hash.  This exposed a couple of
> >> issues in patch 8063e0958780 which was intended to remove support
> >> for kernels older than 3.10.
> >>
> >> Remove stale code and add a compat level check to detect the change.
> >> This fixes a compile error on RHEL 7.7.
> >>
> >> Fixes: 8063e0958780 ("datapath: Drop support for kernel older than 3.10")
> >> Signed-off-by: Greg Rose 
> >
> > Hi Greg,
> >
> > Thanks for the patch.  I found the compilation error on RHEL 7.7
> > actually happens starting from a more recent patch as follows.  If
> > that is the case, please update the fix tag.
> >
> >   2020-05-25 9ba57fc7 ("datapath: Add hash info to upcall")
>
> Hi Yi-Hung,
>
> I don't think I'm fixing anything in that patch.  There's nothing
> wrong with it that I can see.  It properly depended on the
> HAVE_L4_RXHASH define which is not part of that patch but was
> from 75e93287db ("datapath: improve l4_rxhash regex").
>
> Seems to me fixing something would require changes to the code
> that's being fixed.

Hi Greg,

I thought your change in datapath.c is to fix the following new added
lines in 9ba57fc7 ("datapath: Add hash info to upcall").  I found
it because the compilation goes fine before this patch on RHEL 7.7.

9ba57fc7 ("datapath: Add hash info to upcall")
@@ -523,6 +525,25 @@ static int queue_userspace_packet(struct datapath
*dp, struct sk_buff *skb,
.
+#ifdef HAVE_L4_RXHASH
+   if (skb->l4_rxhash)
+#else
+   if (skb->l4_hash)
+#endif
+   hash |= OVS_PACKET_HASH_L4_BIT;



> >> diff --git a/acinclude.m4 b/acinclude.m4
> >> index 1460289ca..8e80d7930 100644
> >> --- a/acinclude.m4
> >> +++ b/acinclude.m4
> >> @@ -879,6 +879,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> >> [OVS_DEFINE([HAVE_SKB_ZEROCOPY])])
> >> OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_rxhash],
> >> [OVS_DEFINE([HAVE_L4_RXHASH])])
> >> +  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_hash],
> >> +  [OVS_DEFINE([HAVE_L4_HASH])])
> > Looks like the main compatibility issue on using either l4_rxhash or
> > l4_hash is due to the kernel ABI update on skbuff.h from the following
> > commit that firstly introduced in 3.15 kernel.
> >
> > 2014-03-24 61b905da33ae (("net: Rename skb->rxhash to skb->hash").
> >
> > I also found that starting from RHEL 7.2, RHEL introduced the new ABI.
> >
> > __u8 RH_KABI_RENAME(l4_rxhash, l4_hash):1;
> >
> >  From Documentation/faq/releases.rst, the oldest kernel that OVS
> > supports from 2.10.x is 3.16, I think we can drop the compatibility
> > support on "l4_rxhash" and only use "l4_hash" in the code base.
> >
> > If that makes sense to you, let's drop the following in acindlue.m4,
> > and clean up the update on datapath.c and
> > ./datapath/linux/compat/include/linux/skbuff.h accordingly.
>
> So we don't want to support RHEL 7.2 and older?  I know we only
> "officially" support 3.16 and above but RHEL 7.x 3.10 kernel is
> a special case.

The new ABI is introduced in RHEL7.2, so the support that we will drop
is RHEL 7.0 and RHEL 7.1. Since RHEL 7.1 was introduced in 2015 March,
I think it is probably fine.


> >> @@ -975,8 +977,6 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> >>
> >> OVS_GREP_IFELSE([$KSRC/include/net/sock.h], [sk_no_check_tx])
> >> OVS_GREP_IFELSE([$KSRC/include/linux/udp.h], [no_check6_tx])
> >> -  OVS_GREP_IFELSE([$KSRC/include/linux/utsrelease.h], [el6],
> >> -  [OVS_DEFINE([HAVE_RHEL6_PER_CPU])])
> >> OVS_FIND_PARAM_IFELSE([$KSRC/include/net/protocol.h],
> >>   [udp_add_offload], [net],
> >>   [OVS_DEFINE([HAVE_UDP_ADD_OFFLOAD_TAKES_NET])])
> > Good catch. I think it is a valid clean up.  But since it does not fix
> > 9ba57fc7 ("datapath: Add hash info to upcall"), should we move it
> > along with the changes in
> > ./datapath/linux/compat/include/linux/percpu.h to a separate patch?
>
> But it does fix 8063e0958780
> ("datapath: Drop support for kernel older than 3.10")
Yes, it does, and we should clean it up in the compat code. My
suggestion is to put it to a separate patch so that we may apply this
clean up patch to the older branches ( before this 9ba57fc7
("datapath: Add hash info to upcall") is introduced), if needed.

> So if we agree to drop support for RHEL 7.2 then yes, we can do
> as you suggest.
>
> As I think about it, dropping support for the OOT kernel driver
> for RHEL 7.2 is probably fine.  I doubt anyone will complain
> and that will simplify the code as you have pointed out.
>


> >> diff --git a/datapath/linux/compat/include/linux/skbuff.h 
> >> b/datapath/linux/compat/include/linux/skbuff.h
> >> index 204ce5497..94479f57b

Re: [ovs-dev] [PATCH net v2] net: openvswitch: silence suspicious RCU usage warning

2020-11-03 Thread Jakub Kicinski

On Tue,  3 Nov 2020 09:25:49 +0100 Eelco Chaudron wrote:
> Silence suspicious RCU usage warning in ovs_flow_tbl_masks_cache_resize()
> by replacing rcu_dereference() with rcu_dereference_ovsl().
> 
> In addition, when creating a new datapath, make sure it's configured under
> the ovs_lock.
> 
> Fixes: 9bf24f594c6a ("net: openvswitch: make masks cache size configurable")
> Reported-by: syzbot+9a8f8bfcc56e85780...@syzkaller.appspotmail.com
> Signed-off-by: Eelco Chaudron 
> ---
> v2: - Moved local variable initialization above lock
> - Renamed jump label to indicate unlocking

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

Re: [ovs-dev] [PATCH 1/2] compat: Fix build issue on RHEL 7.7

2020-11-03 Thread Gregory Rose





On 11/3/2020 2:17 PM, Yi-Hung Wei wrote:

On Tue, Oct 20, 2020 at 10:07 AM Greg Rose  wrote:


RHEL 7.7 has a KABI fixup in struct sk_buff to backport the member
name change of l4_rxhash to l4_hash.  This exposed a couple of
issues in patch 8063e0958780 which was intended to remove support
for kernels older than 3.10.

Remove stale code and add a compat level check to detect the change.
This fixes a compile error on RHEL 7.7.

Fixes: 8063e0958780 ("datapath: Drop support for kernel older than 3.10")
Signed-off-by: Greg Rose 


Hi Greg,

Thanks for the patch.  I found the compilation error on RHEL 7.7
actually happens starting from a more recent patch as follows.  If
that is the case, please update the fix tag.

  2020-05-25 9ba57fc7 ("datapath: Add hash info to upcall")


Hi Yi-Hung,

I don't think I'm fixing anything in that patch.  There's nothing
wrong with it that I can see.  It properly depended on the
HAVE_L4_RXHASH define which is not part of that patch but was
from 75e93287db ("datapath: improve l4_rxhash regex").

Seems to me fixing something would require changes to the code
that's being fixed.





diff --git a/acinclude.m4 b/acinclude.m4
index 1460289ca..8e80d7930 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -879,6 +879,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
[OVS_DEFINE([HAVE_SKB_ZEROCOPY])])
OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_rxhash],
[OVS_DEFINE([HAVE_L4_RXHASH])])
+  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_hash],
+  [OVS_DEFINE([HAVE_L4_HASH])])

Looks like the main compatibility issue on using either l4_rxhash or
l4_hash is due to the kernel ABI update on skbuff.h from the following
commit that firstly introduced in 3.15 kernel.

2014-03-24 61b905da33ae (("net: Rename skb->rxhash to skb->hash").

I also found that starting from RHEL 7.2, RHEL introduced the new ABI.

__u8 RH_KABI_RENAME(l4_rxhash, l4_hash):1;

 From Documentation/faq/releases.rst, the oldest kernel that OVS
supports from 2.10.x is 3.16, I think we can drop the compatibility
support on "l4_rxhash" and only use "l4_hash" in the code base.

If that makes sense to you, let's drop the following in acindlue.m4,
and clean up the update on datapath.c and
./datapath/linux/compat/include/linux/skbuff.h accordingly.


So we don't want to support RHEL 7.2 and older?  I know we only
"officially" support 3.16 and above but RHEL 7.x 3.10 kernel is
a special case.




OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_rxhash],
[OVS_DEFINE([HAVE_L4_RXHASH])])
+  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_hash],
+  [OVS_DEFINE([HAVE_L4_HASH])])





@@ -975,8 +977,6 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [

OVS_GREP_IFELSE([$KSRC/include/net/sock.h], [sk_no_check_tx])
OVS_GREP_IFELSE([$KSRC/include/linux/udp.h], [no_check6_tx])
-  OVS_GREP_IFELSE([$KSRC/include/linux/utsrelease.h], [el6],
-  [OVS_DEFINE([HAVE_RHEL6_PER_CPU])])
OVS_FIND_PARAM_IFELSE([$KSRC/include/net/protocol.h],
  [udp_add_offload], [net],
  [OVS_DEFINE([HAVE_UDP_ADD_OFFLOAD_TAKES_NET])])

Good catch. I think it is a valid clean up.  But since it does not fix
9ba57fc7 ("datapath: Add hash info to upcall"), should we move it
along with the changes in
./datapath/linux/compat/include/linux/percpu.h to a separate patch?


But it does fix 8063e0958780
("datapath: Drop support for kernel older than 3.10")

So if we agree to drop support for RHEL 7.2 then yes, we can do
as you suggest.

As I think about it, dropping support for the OOT kernel driver
for RHEL 7.2 is probably fine.  I doubt anyone will complain
and that will simplify the code as you have pointed out.





diff --git a/datapath/datapath.c b/datapath/datapath.c
index 52a59f135..09fb3b1fc 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -529,7 +529,7 @@ static int queue_userspace_packet(struct datapath *dp, 
struct sk_buff *skb,
 hash |= OVS_PACKET_HASH_SW_BIT;
  #endif

-#ifdef HAVE_L4_RXHASH
+#if defined(HAVE_L4_RXHASH) && !defined(HAVE_L4_HASH)
 if (skb->l4_rxhash)
  #else
 if (skb->l4_hash)


Looks like we can get rid of all #ifdef, and only leave

 if (skb->l4_hash)





diff --git a/datapath/linux/compat/include/linux/skbuff.h 
b/datapath/linux/compat/include/linux/skbuff.h
index 204ce5497..94479f57b 100644
--- a/datapath/linux/compat/include/linux/skbuff.h
+++ b/datapath/linux/compat/include/linux/skbuff.h
@@ -278,8 +278,10 @@ static inline void skb_clear_hash(struct sk_buff *skb)
  #ifdef HAVE_RXHASH
 skb->rxhash = 0;
  #endif
-#if defined(HAVE_L4_RXHASH) && !defined(HAVE_RHEL_OVS_HOOK)
+#if defined(HAVE_L4_RXHASH) && !defined(HAVE_L4_HASH)
 skb->l4_rxhash = 0;
+#else
+   skb->l4_hash = 0;
  #endif
  }
  #endif

We can also clean up skb_clear_hash() if we only care l4_hash.


So yes, we can

[ovs-dev] [PATCH v3 ovn 0/1] northd: Enhance the implementation of ACL log meters.

2020-11-03 Thread Flavio Fernandes

Using meters is a great way to keep the ovn-controllers from getting
overwhelmed with ACL log events. Since multiple ACL rows with logging
enabled can refer to the same meter name, I ran a little experiment
to better understand how that behaves [1].

>From that experiment, we see that a 'noisy' ACL match could consume
all the events allowed by the meter, shadowing logs for other ACLs
that also use the same meter. The thought of maintaining a meter row
per ACL at the NB side is a solution, but it could easily become a
management burden for the CMS. A much better approach would be to
leverage northd to take care of this on behalf of the ACLs.

As northd populates SB meter table from NB meter table, a new logic
checks if the meter is configured as 'shared'. Such config is kept
as a new option in nb_global. Shared meters result in additional
rows in the SB that have the same attributes of the original (aka
template) meter except for its name has the ACL UUID appended to
it.

Last but not least, northd takes care of using the corresponding
meter name as the action in the logging of the ACL.

[1]:
https://github.com/flavio-fernandes/ovsdbapp_playground/blob/acl_meter_issue/scripts/acl_meter.sh

Flavio Fernandes (1):
northd: Enhance the implementation of ACL log meters.

northd/ovn-northd.c | 201
ovn-nb.xml | 14 +++
tests/ovn-northd.at | 92
3 files changed, 255 insertions(+), 52 deletions(-)

--
2.17.1

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

Re: [ovs-dev] [PATCH 2/2] compat: Fix compile warning

2020-11-03 Thread Yi-Hung Wei

On Tue, Oct 20, 2020 at 10:07 AM Greg Rose  wrote:
>
> In ../compat/nf_conntrack_reasm.c nf_frags_cache_name is declared
> if OVS_NF_DEFRAG6_BACKPORT is defined.  However, later in the patch
> it is only used if HAVE_INET_FRAGS_WITH_FRAGS_WORK is defined and
> HAVE_INET_FRAGS_RND is not defined.  This will cause a compile warning
> about unused variables.
>
> Fix it up by using the same defines that enable its use to decide
> if it should be declared and avoid the compiler warning.
>
> Fixes: 4a90b277baca ("compat: Fixup ipv6 fragmentation on 4.9.135+ kernels")
> Signed-off-by: Greg Rose 
> ---

LGTM.

Acked-by: Yi-Hung Wei 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] compat: Fix build issue on RHEL 7.7

2020-11-03 Thread Yi-Hung Wei

On Tue, Oct 20, 2020 at 10:07 AM Greg Rose  wrote:
>
> RHEL 7.7 has a KABI fixup in struct sk_buff to backport the member
> name change of l4_rxhash to l4_hash.  This exposed a couple of
> issues in patch 8063e0958780 which was intended to remove support
> for kernels older than 3.10.
>
> Remove stale code and add a compat level check to detect the change.
> This fixes a compile error on RHEL 7.7.
>
> Fixes: 8063e0958780 ("datapath: Drop support for kernel older than 3.10")
> Signed-off-by: Greg Rose 

Hi Greg,

Thanks for the patch.  I found the compilation error on RHEL 7.7
actually happens starting from a more recent patch as follows.  If
that is the case, please update the fix tag.

 2020-05-25 9ba57fc7 ("datapath: Add hash info to upcall")


> diff --git a/acinclude.m4 b/acinclude.m4
> index 1460289ca..8e80d7930 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -879,6 +879,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>[OVS_DEFINE([HAVE_SKB_ZEROCOPY])])
>OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_rxhash],
>[OVS_DEFINE([HAVE_L4_RXHASH])])
> +  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_hash],
> +  [OVS_DEFINE([HAVE_L4_HASH])])
Looks like the main compatibility issue on using either l4_rxhash or
l4_hash is due to the kernel ABI update on skbuff.h from the following
commit that firstly introduced in 3.15 kernel.

2014-03-24 61b905da33ae (("net: Rename skb->rxhash to skb->hash").

I also found that starting from RHEL 7.2, RHEL introduced the new ABI.

__u8 RH_KABI_RENAME(l4_rxhash, l4_hash):1;

>From Documentation/faq/releases.rst, the oldest kernel that OVS
supports from 2.10.x is 3.16, I think we can drop the compatibility
support on "l4_rxhash" and only use "l4_hash" in the code base.

If that makes sense to you, let's drop the following in acindlue.m4,
and clean up the update on datapath.c and
./datapath/linux/compat/include/linux/skbuff.h accordingly.

>OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_rxhash],
>[OVS_DEFINE([HAVE_L4_RXHASH])])
> +  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_hash],
> +  [OVS_DEFINE([HAVE_L4_HASH])])



> @@ -975,8 +977,6 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>
>OVS_GREP_IFELSE([$KSRC/include/net/sock.h], [sk_no_check_tx])
>OVS_GREP_IFELSE([$KSRC/include/linux/udp.h], [no_check6_tx])
> -  OVS_GREP_IFELSE([$KSRC/include/linux/utsrelease.h], [el6],
> -  [OVS_DEFINE([HAVE_RHEL6_PER_CPU])])
>OVS_FIND_PARAM_IFELSE([$KSRC/include/net/protocol.h],
>  [udp_add_offload], [net],
>  [OVS_DEFINE([HAVE_UDP_ADD_OFFLOAD_TAKES_NET])])
Good catch. I think it is a valid clean up.  But since it does not fix
9ba57fc7 ("datapath: Add hash info to upcall"), should we move it
along with the changes in
./datapath/linux/compat/include/linux/percpu.h to a separate patch?


> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 52a59f135..09fb3b1fc 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -529,7 +529,7 @@ static int queue_userspace_packet(struct datapath *dp, 
> struct sk_buff *skb,
> hash |= OVS_PACKET_HASH_SW_BIT;
>  #endif
>
> -#ifdef HAVE_L4_RXHASH
> +#if defined(HAVE_L4_RXHASH) && !defined(HAVE_L4_HASH)
> if (skb->l4_rxhash)
>  #else
> if (skb->l4_hash)

Looks like we can get rid of all #ifdef, and only leave
> if (skb->l4_hash)



> diff --git a/datapath/linux/compat/include/linux/skbuff.h 
> b/datapath/linux/compat/include/linux/skbuff.h
> index 204ce5497..94479f57b 100644
> --- a/datapath/linux/compat/include/linux/skbuff.h
> +++ b/datapath/linux/compat/include/linux/skbuff.h
> @@ -278,8 +278,10 @@ static inline void skb_clear_hash(struct sk_buff *skb)
>  #ifdef HAVE_RXHASH
> skb->rxhash = 0;
>  #endif
> -#if defined(HAVE_L4_RXHASH) && !defined(HAVE_RHEL_OVS_HOOK)
> +#if defined(HAVE_L4_RXHASH) && !defined(HAVE_L4_HASH)
> skb->l4_rxhash = 0;
> +#else
> +   skb->l4_hash = 0;
>  #endif
>  }
>  #endif
We can also clean up skb_clear_hash() if we only care l4_hash.

Thanks,

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

Re: [ovs-dev] [PATCH ovn 0/2] Further restrict ARP/ND broadcast domain for E-W and S-N traffic.

2020-11-03 Thread Mark Michelson

Aside from spelling "update" properly in the title of patch 1, this 
looks good to me.


Acked-by: Mark Michelson

On 11/3/20 10:50 AM, Dumitru Ceara wrote:

This series aims at further restricting ARP/ND broadcast domain for self
originated packets.  Until now those packets were always flooded on all
ports of the switch.

The first patch of the series is needed in order to remove dependency on
self-originated GARPs to be sent through the network (OVS) between OVN
logical ports.

Signed-off-by: Dumitru Ceara 

Dumitru Ceara (2):
   pinctrl: Directly udpate MAC_Bindings created by self originated GARPs.
   ovn-northd: Limit self originated ARP/ND broadcast domain.


  controller/pinctrl.c|  108 ++-
  lib/mcast-group-index.h |1
  northd/ovn-northd.8.xml |   19 
  northd/ovn-northd.c |   84 +++--
  tests/ovn.at|   50 ++
  5 files changed, 173 insertions(+), 89 deletions(-)

___
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

[ovs-dev] [PATCH v2 ovn 1/1] northd: Enhance the implementation of ACL log meters.

2020-11-03 Thread Flavio Fernandes

When multiple ACL rows use the same meter for log rate-limiting, the
'noisiest' ACL matches may completely consume the meter and shadow other
ACL logs. This patch introduces an option in NB_Global that allows for a
meter to rate-limit each ACL log separately.

The change is backward compatible. Based on a well known option, northd
will turn a single meter in the NB into multiple meters in the SB by
appending the ACL uuid to its name. It will also make action in logical
flow use the unique meter name for each affected ACL log. In order to
make the meter behave as described, add this option:

  ovn-nbctl set nb_global . options:acl_shared_log_meters="${meter_name}"

If more than one meter is wanted, simply use a comma to separate each name.

Reported-by: Flavio Fernandes 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
Signed-off-by: Flavio Fernandes 
---
v1 -> v2:
 * rebase master b38e10f4b

TODO: I do not yet know the implications that this change have on the new
  ddlog based implementation of northd. Will read up and contact folks on 
that.

 northd/ovn-northd.c | 201 
 ovn-nb.xml  |  14 +++
 tests/ovn-northd.at |  99 ++
 3 files changed, 262 insertions(+), 52 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8800a3d3c..721e7be97 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5378,8 +5378,53 @@ build_acl_hints(struct ovn_datapath *od, struct hmap 
*lflows)
 }
 }
 
+static bool
+is_a_shared_meter(const struct smap *nb_options, const char *meter_name)
+{
+bool is_shared_meter = false;
+const char *meters = smap_get(nb_options, "acl_shared_log_meters");
+if (meters && meters[0]) {
+char *cur, *next, *start;
+next = start = xstrdup(meters);
+while ((cur = strsep(, ",")) && *cur) {
+if (!strcmp(meter_name, cur)) {
+is_shared_meter = true;
+break;
+}
+}
+free(start);
+}
+return is_shared_meter;
+}
+
+static char*
+alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl)
+{
+return xasprintf("%s__" UUID_FMT,
+ acl->meter, UUID_ARGS(>header_.uuid));
+}
+
+static void
+build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl,
+const struct smap *nb_options)
+{
+if (!acl->meter) {
+return;
+}
+
+/* If ACL log meter uses a shared meter, use unique Meter name. */
+if (is_a_shared_meter(nb_options, acl->meter)) {
+char *meter_name = alloc_acl_log_unique_meter_name(acl);
+ds_put_format(actions, "meter=\"%s\", ", meter_name);
+free(meter_name);
+} else {
+ds_put_format(actions, "meter=\"%s\", ", acl->meter);
+}
+}
+
 static void
-build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
+build_acl_log(struct ds *actions, const struct nbrec_acl *acl,
+  const struct smap *nb_options)
 {
 if (!acl->log) {
 return;
@@ -5407,9 +5452,7 @@ build_acl_log(struct ds *actions, const struct nbrec_acl 
*acl)
 ds_put_cstr(actions, "verdict=allow, ");
 }
 
-if (acl->meter) {
-ds_put_format(actions, "meter=\"%s\", ", acl->meter);
-}
+build_acl_log_meter(actions, acl, nb_options);
 
 ds_chomp(actions, ' ');
 ds_chomp(actions, ',');
@@ -5420,7 +5463,8 @@ static void
 build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
enum ovn_stage stage, struct nbrec_acl *acl,
struct ds *extra_match, struct ds *extra_actions,
-   const struct ovsdb_idl_row *stage_hint)
+   const struct ovsdb_idl_row *stage_hint,
+   const struct smap *nb_options)
 {
 struct ds match = DS_EMPTY_INITIALIZER;
 struct ds actions = DS_EMPTY_INITIALIZER;
@@ -5432,7 +5476,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct 
hmap *lflows,
   ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK)
   : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP));
 
-build_acl_log(, acl);
+build_acl_log(, acl, nb_options);
 if (extra_match->length > 0) {
 ds_put_format(, "(%s) && ", extra_match->string);
 }
@@ -5457,7 +5501,8 @@ build_reject_acl_rules(struct ovn_datapath *od, struct 
hmap *lflows,
 
 static void
 consider_acl(struct hmap *lflows, struct ovn_datapath *od,
- struct nbrec_acl *acl, bool has_stateful)
+ struct nbrec_acl *acl, bool has_stateful,
+ const struct smap *nb_options)
 {
 bool ingress = !strcmp(acl->direction, "from-lport") ? true :false;
 enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL;
@@ -5471,7 +5516,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
  * associated conntrack entry and would return "+invalid". */
 if

[ovs-dev] [PATCH v2 ovn 0/1] northd: Enhance the implementation of ACL log meters.

2020-11-03 Thread Flavio Fernandes

Last but not least, northd takes care of using the corresponding
meter name as the action in the logging of the ACL.

[1]:
https://github.com/flavio-fernandes/ovsdbapp_playground/blob/acl_meter_issue/scripts/acl_meter.sh

Flavio Fernandes (1):
northd: Enhance the implementation of ACL log meters.

northd/ovn-northd.c | 201
ovn-nb.xml | 14 +++
tests/ovn-northd.at | 99 ++
3 files changed, 262 insertions(+), 52 deletions(-)

--
2.17.1

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

[ovs-dev] Disposiciones para sectores específicos

2020-11-03 Thread T-MEC- Aduana, arancel y logística

Webinar en Vivo: 
T-MEC implicaciones aduanales, arancelarias y logísticas en importaciones y 
exportaciones.
18 y 19 de Noviembre - Horario de 10:00 a 17:00 Hrs.

Aportaremos a los interesados en el comercio exterior mexicano con Estados 
Unidos y Canadá, los aspectos más relevantes del nuevo T-MEC, así como
 los cambios que ha conllevado en la legislación aduanera y arancelaria 
mexicana, con el fin fin de que puedan aplicar estos conocimientos con 
efectividad 
y precisión en sus relaciones comerciales y operativas con clientes, 
proveedores, transportistas, agentes aduanales y aduanas y demás autoridades 
gubernamentales relacionada.

Objetivos Específicos:

- El T-MEC en contexto.
- Regulaciones y procedimientos aduanales y arancelarios bajo el T-MEC. 
- Disposiciones para sectores específicos.
- Políticas públicas derivadas del T-MEC 

El nuevo T- MEC deja atrás la mera desgravación y la promoción del comercio 
trilateral que motivaron al acuerdo de 1994, enfocándose en crear nuevas 
condiciones de comercio. 

Para mayor información, responder sobre este correo con la palabra T-MEC + los 
siguientes datos:

NOMBRE:
TELÉFONO:
EMPRESA:
CORREO ALTERNO: 

Para información inmediata llamar al:
(+52) 55 15 54 66 30 - (+52) 55 30 16 70 85
O puede enviarnos un mensaje vía whatsapp 

Innova Learn México - innovalearn. mx - Mérida, Yucatán, México


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

Re: [ovs-dev] [PATCH ovn v2 2/7] northd: Refactor load balancer vip parsing.

2020-11-03 Thread Numan Siddique

On Tue, Nov 3, 2020 at 3:14 PM Numan Siddique  wrote:
>
> On Fri, Oct 30, 2020 at 1:52 AM Dumitru Ceara  wrote:
> >
> > On 10/27/20 6:16 PM, num...@ovn.org wrote:
> > > From: Numan Siddique 
> > >
> > > Parsing of the load balancer VIPs is moved to a separate file - lib/lb.c.
> > > ovn-northd makes use of these functions. Upcoming patch will make use of 
> > > these
> > > util functions for parsing SB Load_Balancers.
> > >
> > > Signed-off-by: Numan Siddique 
> > > ---
> > >  lib/automake.mk |   4 +-
> > >  lib/lb.c| 236 
> > >  lib/lb.h|  77 
> > >  lib/ovn-util.c  |  28 +
> > >  lib/ovn-util.h  |   2 +
> > >  northd/ovn-northd.c | 286 +---
> > >  6 files changed, 378 insertions(+), 255 deletions(-)
> > >  create mode 100644 lib/lb.c
> > >  create mode 100644 lib/lb.h
> > >
> > > diff --git a/lib/automake.mk b/lib/automake.mk
> > > index f3e9c8818b..430cd11fc6 100644
> > > --- a/lib/automake.mk
> > > +++ b/lib/automake.mk
> > > @@ -23,7 +23,9 @@ lib_libovn_la_SOURCES = \
> > >   lib/ovn-util.h \
> > >   lib/logical-fields.c \
> > >   lib/inc-proc-eng.c \
> > > - lib/inc-proc-eng.h
> > > + lib/inc-proc-eng.h \
> > > + lib/lb.c \
> > > + lib/lb.h
> > >  nodist_lib_libovn_la_SOURCES = \
> > >   lib/ovn-dirs.c \
> > >   lib/ovn-nb-idl.c \
> > > diff --git a/lib/lb.c b/lib/lb.c
> > > new file mode 100644
> > > index 00..db2d3d552f
> > > --- /dev/null
> > > +++ b/lib/lb.c
> > > @@ -0,0 +1,236 @@
> > > +/* Copyright (c) 2020, Red Hat, Inc.
> > > + *
> > > + * Licensed under the Apache License, Version 2.0 (the "License");
> > > + * you may not use this file except in compliance with the License.
> > > + * You may obtain a copy of the License at:
> > > + *
> > > + * http://www.apache.org/licenses/LICENSE-2.0
> > > + *
> > > + * Unless required by applicable law or agreed to in writing, software
> > > + * distributed under the License is distributed on an "AS IS" BASIS,
> > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or 
> > > implied.
> > > + * See the License for the specific language governing permissions and
> > > + * limitations under the License.
> > > + */
> > > +
> > > +#include 
> > > +
> > > +#include "lb.h"
> > > +#include "lib/ovn-nb-idl.h"
> > > +#include "lib/ovn-sb-idl.h"
> > > +#include "lib/ovn-util.h"
> > > +
> > > +/* OpenvSwitch lib includes. */
> > > +#include "openvswitch/vlog.h"
> > > +#include "lib/smap.h"
> > > +
> > > +VLOG_DEFINE_THIS_MODULE(lb);
> > > +
> > > +static struct ovn_lb *
> > > +ovn_lb_create(const struct smap *vips)
> > > +{
> > > +struct ovn_lb *lb = xzalloc(sizeof *lb);
> > > +
> > > +lb->n_vips = smap_count(vips);
> > > +lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
> > > +struct smap_node *node;
> > > +size_t n_vips = 0;
> > > +
> > > +SMAP_FOR_EACH (node, vips) {
> > > +char *vip;
> > > +uint16_t port;
> > > +int addr_family;
> > > +
> > > +if (!ip_address_and_port_from_key(node->key, , ,
> > > +  _family)) {
> > > +continue;
> > > +}
> > > +
> > > +lb->vips[n_vips].vip = vip;
> > > +lb->vips[n_vips].vip_port = port;
> > > +lb->vips[n_vips].addr_family = addr_family;
> > > +lb->vips[n_vips].vip_port_str = xstrdup(node->key);
> > > +lb->vips[n_vips].backend_ips = xstrdup(node->value);
> > > +
> > > +char *tokstr = xstrdup(node->value);
> > > +char *save_ptr = NULL;
> > > +char *token;
> > > +size_t n_backends = 0;
> > > +/* Format for a backend ips : IP1:port1,IP2:port2,...". */
> > > +for (token = strtok_r(tokstr, ",", _ptr);
> > > +token != NULL;
> > > +token = strtok_r(NULL, ",", _ptr)) {
> > > +n_backends++;
> > > +}
> > > +
> > > +free(tokstr);
> > > +tokstr = xstrdup(node->value);
> > > +save_ptr = NULL;
> > > +
> > > +lb->vips[n_vips].n_backends = n_backends;
> > > +lb->vips[n_vips].backends = xcalloc(n_backends,
> > > +sizeof (struct 
> > > lb_vip_backend));
> >
> > This should be 'sizeof *lb->vips[n_vips].backends'.
>
> This part of the code is moved from ovn-northd.c. We could use both -
> an expression
> or a structure in sizeof. The coding guidelines give preference to the former.
>
> >
> > > +
> >
> > Nit: It's probably fine to remove an empty line here, the one above is 
> > already
> > indented enough to the right.
> >
> > > +size_t i = 0;
> > > +for (token = strtok_r(tokstr, ",", _ptr);
> > > +token != NULL;
> > > +token = strtok_r(NULL, ",", _ptr)) {
> > > +char *backend_ip;
> > > +uint16_t backend_port;
> > > +
> > > +if

Re: [ovs-dev] operstate of internal interface(s) when physical member operstate is down

2020-11-03 Thread Ben Pfaff

On Mon, Oct 26, 2020 at 10:56:02AM +0100, Matthias May via dev wrote:
> I'm looking for a way to replicate the behaviour of a standard bridge.
> In a standard bridge when all members of the bridge report their
> operstate as down, the bridge itself reports its operstate down as well.
> 
> Is there any way to achieve this?

I don't think that OVS has this built in, but a controller could
implement that behavior.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Is the Open Flow switch able to calculate the gap of sequence number

2020-11-03 Thread Ben Pfaff

Oh, it's about wireless.  OVS doesn't have special 802.11 support.

On Tue, Oct 27, 2020 at 06:51:08AM +, Soliman Awad Alshra´a Abdullah TU 
Ilmenau wrote:
> 
> Dear sir,
> 
> 
> Attached you find the requested article
> 
> Thanks
> M.Sc Abdullah Soliman
> Technische Universität Ilmenau
> Fakultät für Elektrotechnik und Informationstechnik
> Fachgebiet Kommunikationsnetze
> Besucheradresse:
> Helmholtzplatz 2
> 98693 Ilmenau
> 
> Postadresse:
> PF 10 05 65
> 98684 Ilmenau
> 
> 
> Telefon
> 
> +49 3677 69-2698
> 
> [cid:image001.png@01D62526.1691FEE0]  
> abdullah.alsh...@tu-ilmenau.de
> [cid:image002.png@01D62526.1691FEE0]  
> www.tu-ilmenau.de/it-kn
> 
> [cid:image003.jpg@01D62526.1691FEE0]
> 
> 
> 
> [cid:image004.png@01D62526.1691FEE0]
> 
> 
> 
> 
> From: Ben Pfaff 
> Sent: 26 October 2020 18:44:03
> To: Soliman Awad Alshra´a Abdullah TU Ilmenau
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] Is the Open Flow switch able to calculate the gap of 
> sequence number
> 
> On Fri, Oct 23, 2020 at 02:38:06PM +, Soliman Awad Alshra´a Abdullah TU 
> Ilmenau wrote:
> > OpenFLow switch is able to keep track of the sequence number of each 
> > traffic flow to detect MAC spoofing attack. Upon reception of a frame, the 
> > algorithm calculates the gap G between the sequence number of the current 
> > frame and that of the last frame received from the same source address. If 
> > G = 0, the current frame is considered as a re-transmitted frame, while if 
> > G = 1 or G = 2, the current frame is considered the right one. But, if the 
> > gap between the current frame and previous frame is in between 3 and 4096, 
> > then it is considered an abnormal sequence number.
> >
> > In my case, I use Ryu Controller and I would like to do the same work, 
> > where the switch sends the alert to the controller after the switch detects 
> > the gap.
> 
> This doesn't make sense to me.  Frames don't have sequence numbers.
> 
> Can you cite the publication that makes this claim?


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

Re: [ovs-dev] [PATCH] ofp-parse: Support "igmp" keyword in flows.

2020-11-03 Thread Ben Pfaff

On Tue, Nov 03, 2020 at 03:53:51AM +0100, Ilya Maximets wrote:
> On 11/3/20 12:28 AM, Ben Pfaff wrote:
> > match_format() prints out "igmp" for IGMP flows, but
> > ofp_parse_protocol() didn't accept it, which meant that OVS would print
> > out a flow that it wouldn't re-parse.  This fixes the problem and adds
> > a test.
> 
> I'm a bit confused.  IIUC, matching on igmp is not supported by any
> OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2',
> but there is no special OF header for "igmp" or "igmp_type" or "igmp_code".
> While it seems easy to add parsing of "igmp" itself, its fields ("igmp_type"
> and "igmp_code") could appear in the output too and parsing of these fields
> will, I guess, require a new OF extension.  Is it correct?  If so, maybe
> it's better to remove "igmp*" printing code from the match_format() instead?

I hadn't thought about that.  I was just thinking of this as a short
form of "ip,nw_proto=2".

It looks like OVS's support for IGMP matching is inconsistent.
flow_extract() pulls the igmp_type and igmp_code values into tp_src and
tp_dst.  The kernel datapath doesn't do that, though, and there's no
support for the fields at the OpenFlow level.  I guess it has never been
important enough.

OK, I won't complain either way then.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn] northd: fix lb_action when there are no active backends for lb health_check

2020-11-03 Thread Lorenzo Bianconi

Fix the following warning reported by ovn-controller when there are no
active backends for lb health_check and selection_fields have been
configured for hash computation

flow|WARN|error parsing actions "drop; 
hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");":
Syntax error at `hash_fields' expecting end of input.

Fixes: 5af304e747 ("Support selection fields in load balancer.")
Signed-off-by: Lorenzo Bianconi 
---
 northd/ovn-northd.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8800a3d3c..88d3e3ed2 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -3598,6 +3598,8 @@ static void build_lb_vip_ct_lb_actions(struct lb_vip 
*lb_vip,
struct ds *action,
char *selection_fields)
 {
+bool skip_hash_fields = false;
+
 if (lb_vip->health_check) {
 ds_put_cstr(action, "ct_lb(backends=");
 
@@ -3616,6 +3618,7 @@ static void build_lb_vip_ct_lb_actions(struct lb_vip 
*lb_vip,
 }
 
 if (!n_active_backends) {
+skip_hash_fields = true;
 ds_clear(action);
 ds_put_cstr(action, "drop;");
 } else {
@@ -3626,7 +3629,7 @@ static void build_lb_vip_ct_lb_actions(struct lb_vip 
*lb_vip,
 ds_put_format(action, "ct_lb(backends=%s);", lb_vip->backend_ips);
 }
 
-if (selection_fields && selection_fields[0]) {
+if (!skip_hash_fields && selection_fields && selection_fields[0]) {
 ds_chomp(action, ';');
 ds_chomp(action, ')');
 ds_put_format(action, "; hash_fields=\"%s\");", selection_fields);
-- 
2.26.2

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

Re: [ovs-dev] [PATCH] Documentation: Fix rendering of extra repo info for RHEL 8

2020-11-03 Thread Gregory Rose





On 11/3/2020 2:12 AM, Timothy Redaelli wrote:

In commit a82083ee3091 ("Documentation: Add extra repo info for RHEL 8")
a newline was missing to correctly generate the code block to add
codeready-builder repository.

This commit adds the missing newline to correctly generate the code block
with the RHEL 8 codeready-builder instructions.

Fixes: a82083ee3091 ("Documentation: Add extra repo info for RHEL 8")
Cc: gvrose8...@gmail.com
Signed-off-by: Timothy Redaelli 
---
  Documentation/intro/install/fedora.rst | 1 +
  1 file changed, 1 insertion(+)

diff --git a/Documentation/intro/install/fedora.rst 
b/Documentation/intro/install/fedora.rst
index e5324e1df..4a2f3507c 100644
--- a/Documentation/intro/install/fedora.rst
+++ b/Documentation/intro/install/fedora.rst
@@ -70,6 +70,7 @@ repositories to help yum-builddep, e.g.::
  $ subscription-manager repos --enable=rhel-7-server-optional-rpms
  
  or for RHEL 8::

+
  $ subscription-manager repos \
--enable=codeready-builder-for-rhel-8-x86_64-rpms
  



I'd never noticed that.

Thanks!

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

Re: [ovs-dev] [PATCH 0/5] Mitigate RAFT memory consumption issues.

2020-11-03 Thread Ilya Maximets

On 10/28/20 12:03 PM, Dumitru Ceara wrote:
> On 10/26/20 2:42 AM, Ilya Maximets wrote:
>> Under a heavy load or in a long run memory consumption if ovsdb-server
>> process that is part of a RAFT cluster could reach very high values.
>> From my experience it could be up to 60-100 GB.  In these conditions
>> it's likely that ovsdb-server will be killed by OOM-killer or just
>> will not be able to work properly wasting time on processing outdated
>> or unneeded data.  There are 3 main parts that consumes most of the
>> memory:
>>
>>  1. Backlog on RAFT connections between servers.
>>  2. Local RAFT log.
>>  3. Libc doesn't return memory back to system.
>>
>> Backlog could start growing if one of remote servers doesn't doing
>> well and is not able to process requests in time.  This sending
>> backlog could contain snapshots or even just big number of big
>> append requests.  It could grow to tens of GBs really fast and
>> most of this data might be even unnecessary if it becomes obsolete
>> by one of the previous requests or if current 'term' changes and
>> all the old messages should be dropped.  Solution for this is
>> to monitor the size of the current backlog and disconnect if it grows
>> too big since it will be easier to just reconnect and send one new
>> snapshot.
>>
>> Local RAFT log contains all the DB changes that are not part of a
>> snapshot yet.  Since snapshots are taken at most once in 10 minutes,
>> log could grow pretty big.  Up to tens of thousands of entries and
>> each of these entries could be fairly big by themselves.  That being
>> said RAFT log could grow up to tens of GBs too.
>>
>> One extra point for memory consumption is that memory likely doesn't
>> go away even after calling free() due to implementation of a C memory
>> allocators.  And this happens a lot.  ovsdb-server process usually
>> holds a lot of system memory even if the database is almost empty.
>> This heap memory might be returned back to OS by using malloc_trim().
>>
>> --
>> All of these issues was found on branch-2.13, but it always hard to
>> distinguish new features from the bug fix when we're talking about
>> scaling issues.  Anyway, I think, it'll be good to have these
>> patches (if they are any good) backorted to 2.13, especially because
>> it's going to be our next LTS.  Thoughts?
>>
> 
> Hi Ilya,
> 
> I think although these might be considered features, without them there 
> doesn't
> seem to be a way to address the memory consumption issues in production
> deployments.
> 
> In my opinion, these should definitely go to 2.13 branch too.

OK.  Thanks!
I'll backport them.

> 
> Thanks,
> Dumitru
> 
>> Ilya Maximets (5):
>>   raft: Add log length to the memory report.
>>   ovsdb-server: Reclaim heap memory after compaction.
>>   raft: Set threshold on backlog for raft connections.
>>   raft: Make backlog thresholds configurable.
>>   raft: Avoid having more than one snapshot in-flight.
>>
>>  NEWS|  6 +++
>>  configure.ac|  1 +
>>  lib/jsonrpc.c   | 57 -
>>  lib/jsonrpc.h   |  6 +++
>>  ovsdb/ovsdb-server.1.in |  9 
>>  ovsdb/ovsdb-server.c| 41 +-
>>  ovsdb/ovsdb.c   | 12 +-
>>  ovsdb/ovsdb.h   |  3 +-
>>  ovsdb/raft-private.c|  1 -
>>  ovsdb/raft-private.h|  4 +-
>>  ovsdb/raft.c| 93 +
>>  11 files changed, 199 insertions(+), 34 deletions(-)
>>
> 

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

[ovs-dev] [PATCH v2 0/2] Mitigate RAFT memory consumption issues.

2020-11-03 Thread Ilya Maximets

Version 2:
 - Skipping 3 patches that already applied.
 - Fixed use of uninitialized backlog limits in jsonrpc_session,
   i.e. added initialization on each jsorpc_session_open*()
 - Added Acked-by from Dumitru to the last patch.

Original cover letter:

Under a heavy load or in a long run memory consumption if ovsdb-server
process that is part of a RAFT cluster could reach very high values.
>From my experience it could be up to 60-100 GB.  In these conditions
it's likely that ovsdb-server will be killed by OOM-killer or just
will not be able to work properly wasting time on processing outdated
or unneeded data.  There are 3 main parts that consumes most of the
memory:

 1. Backlog on RAFT connections between servers.
 2. Local RAFT log.
 3. Libc doesn't return memory back to system.

Backlog could start growing if one of remote servers doesn't doing
well and is not able to process requests in time.  This sending
backlog could contain snapshots or even just big number of big
append requests.  It could grow to tens of GBs really fast and
most of this data might be even unnecessary if it becomes obsolete
by one of the previous requests or if current 'term' changes and
all the old messages should be dropped.  Solution for this is
to monitor the size of the current backlog and disconnect if it grows
too big since it will be easier to just reconnect and send one new
snapshot.

Local RAFT log contains all the DB changes that are not part of a
snapshot yet.  Since snapshots are taken at most once in 10 minutes,
log could grow pretty big.  Up to tens of thousands of entries and
each of these entries could be fairly big by themselves.  That being
said RAFT log could grow up to tens of GBs too.

One extra point for memory consumption is that memory likely doesn't
go away even after calling free() due to implementation of a C memory
allocators.  And this happens a lot.  ovsdb-server process usually
holds a lot of system memory even if the database is almost empty.
This heap memory might be returned back to OS by using malloc_trim().

--
All of these issues was found on branch-2.13, but it always hard to
distinguish new features from the bug fix when we're talking about
scaling issues.  Anyway, I think, it'll be good to have these
patches (if they are any good) backorted to 2.13, especially because
it's going to be our next LTS.  Thoughts?


Ilya Maximets (2):
  raft: Set threshold on backlog for raft connections.
  raft: Make backlog thresholds configurable.

 NEWS|  3 +++
 lib/jsonrpc.c   | 60 -
 lib/jsonrpc.h   |  6 +
 ovsdb/ovsdb-server.1.in |  5 
 ovsdb/raft.c| 50 ++
 5 files changed, 123 insertions(+), 1 deletion(-)

-- 
2.25.4

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

[ovs-dev] [PATCH v2 2/2] raft: Make backlog thresholds configurable.

2020-11-03 Thread Ilya Maximets

New appctl 'cluster/set-backlog-threshold' to configure thresholds
on backlog of raft jsonrpc connections.  Could be used, for example,
in some extreme conditions where size of a database expected to be
very large, i.e. comparable with default 4GB threshold.

Acked-by: Dumitru Ceara 
Signed-off-by: Ilya Maximets 
---
 NEWS|  1 +
 ovsdb/ovsdb-server.1.in |  5 
 ovsdb/raft.c| 55 +
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/NEWS b/NEWS
index ebdf8758b..c0819bf93 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,7 @@ Post-v2.14.0
after every DB compaction back to OS.  Disabled by default.
  * Maximum backlog on RAFT connections limited to 500 messages or 4GB.
Once threshold reached, connection is dropped (and re-established).
+   Use the 'cluster/set-backlog-threshold' command to change limits.
- DPDK:
  * Removed support for vhost-user dequeue zero-copy.
- The environment variable OVS_UNBOUND_CONF, if set, is now used
diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
index 07a36cc7d..5a7f3ba13 100644
--- a/ovsdb/ovsdb-server.1.in
+++ b/ovsdb/ovsdb-server.1.in
@@ -381,6 +381,11 @@ This command must be executed on the leader.  It initiates 
the change to the
 cluster.  To see if the change takes effect (committed), use
 \fBcluster/status\fR to show the current setting.  Once a change is committed,
 it persists at server restarts.
+.IP "\fBcluster/set\-backlog\-threshold \fIdb\fR \fIn_msgs\fR \fIn_bytes\fR"
+Sets the backlog limits for \fIdb\fR's RAFT connections to a maximum of
+\fIn_msgs\fR messages or \fIn_bytes\fR bytes.  If the backlog on one of the
+connections reaches the limit, it will be disconnected (and re-established).
+Values are checked only if the backlog contains more than 50 messages.
 .
 .so lib/vlog-unixctl.man
 .so lib/memory-unixctl.man
diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index 67c714ff4..760dfca6d 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -305,6 +305,12 @@ struct raft {
 bool ever_had_leader;   /* There has been leader elected since the raft
is initialized, meaning it is ever
connected. */
+
+/* Connection backlog limits. */
+#define DEFAULT_MAX_BACKLOG_N_MSGS500
+#define DEFAULT_MAX_BACKLOG_N_BYTES   UINT32_MAX
+size_t conn_backlog_max_n_msgs;   /* Number of messages. */
+size_t conn_backlog_max_n_bytes;  /* Number of bytes. */
 };
 
 /* All Raft structures. */
@@ -412,6 +418,9 @@ raft_alloc(void)
 
 raft->election_timer = ELECTION_BASE_MSEC;
 
+raft->conn_backlog_max_n_msgs = DEFAULT_MAX_BACKLOG_N_MSGS;
+raft->conn_backlog_max_n_bytes = DEFAULT_MAX_BACKLOG_N_BYTES;
+
 return raft;
 }
 
@@ -925,9 +934,6 @@ raft_reset_ping_timer(struct raft *raft)
 raft->ping_timeout = time_msec() + raft->election_timer / 3;
 }
 
-#define RAFT_MAX_BACKLOG_N_MSGS500
-#define RAFT_MAX_BACKLOG_BYTES UINT32_MAX
-
 static void
 raft_add_conn(struct raft *raft, struct jsonrpc_session *js,
   const struct uuid *sid, bool incoming)
@@ -943,8 +949,8 @@ raft_add_conn(struct raft *raft, struct jsonrpc_session *js,
 conn->incoming = incoming;
 conn->js_seqno = jsonrpc_session_get_seqno(conn->js);
 jsonrpc_session_set_probe_interval(js, 0);
-jsonrpc_session_set_backlog_threshold(js, RAFT_MAX_BACKLOG_N_MSGS,
-  RAFT_MAX_BACKLOG_BYTES);
+jsonrpc_session_set_backlog_threshold(js, raft->conn_backlog_max_n_msgs,
+  raft->conn_backlog_max_n_bytes);
 }
 
 /* Starts the local server in an existing Raft cluster, using the local copy of
@@ -4717,6 +4723,42 @@ raft_unixctl_change_election_timer(struct unixctl_conn 
*conn,
 unixctl_command_reply(conn, "change of election timer initiated.");
 }
 
+static void
+raft_unixctl_set_backlog_threshold(struct unixctl_conn *conn,
+   int argc OVS_UNUSED, const char *argv[],
+   void *aux OVS_UNUSED)
+{
+const char *cluster_name = argv[1];
+unsigned long long n_msgs, n_bytes;
+struct raft_conn *r_conn;
+
+struct raft *raft = raft_lookup_by_name(cluster_name);
+if (!raft) {
+unixctl_command_reply_error(conn, "unknown cluster");
+return;
+}
+
+if (!str_to_ullong(argv[2], 10, _msgs)
+|| !str_to_ullong(argv[3], 10, _bytes)) {
+unixctl_command_reply_error(conn, "invalid argument");
+return;
+}
+
+if (n_msgs < 50 || n_msgs > SIZE_MAX || n_bytes > SIZE_MAX) {
+unixctl_command_reply_error(conn, "values out of range");
+return;
+}
+
+raft->conn_backlog_max_n_msgs = n_msgs;
+raft->conn_backlog_max_n_bytes = n_bytes;
+
+LIST_FOR_EACH (r_conn, list_node, >conns) {
+jsonrpc_session_set_backlog_threshold(r_conn->js, n_msgs, n_bytes);
+}

[ovs-dev] [PATCH v2 1/2] raft: Set threshold on backlog for raft connections.

2020-11-03 Thread Ilya Maximets

RAFT messages could be fairly big.  If something abnormal happens to
one of the servers in a cluster it may not be able to process all the
incoming messages in a timely manner.  This results in jsonrpc backlog
growth on the sender's side.  For example if follower gets many new
clients at once that it needs to serve, or it decides to take a
snapshot in a period of high number of database changes.
If backlog grows large enough it becomes harder and harder for follower
to process incoming raft messages, it sends outdated replies and
starts receiving snapshots and the whole raft log from the leader.
Sometimes backlog grows too high (60GB in this example):

  jsonrpc|INFO|excessive sending backlog, jsonrpc: ssl:,
   num of msgs: 15370, backlog: 61731060773.

In this case OS might actually decide to kill the sender to free some
memory.  Anyway, It could take a lot of time for such a server to catch
up with the rest of the cluster if it has so much data to receive and
process.

Introducing backlog thresholds for jsonrpc connections.
If sending backlog will exceed particular values (500 messages or
4GB in size), connection will be dropped and re-created.  This will
allow to drop all the current backlog and start over increasing
chances of cluster recovery.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=129
Signed-off-by: Ilya Maximets 
---
 NEWS  |  2 ++
 lib/jsonrpc.c | 60 ++-
 lib/jsonrpc.h |  6 ++
 ovsdb/raft.c  |  5 +
 4 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 2860a8e9c..ebdf8758b 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,8 @@ Post-v2.14.0
  * New unixctl command 'ovsdb-server/memory-trim-on-compaction on|off'.
If turned on, ovsdb-server will try to reclaim all the unused memory
after every DB compaction back to OS.  Disabled by default.
+ * Maximum backlog on RAFT connections limited to 500 messages or 4GB.
+   Once threshold reached, connection is dropped (and re-established).
- DPDK:
  * Removed support for vhost-user dequeue zero-copy.
- The environment variable OVS_UNBOUND_CONF, if set, is now used
diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
index ecbc939fe..eb611018d 100644
--- a/lib/jsonrpc.c
+++ b/lib/jsonrpc.c
@@ -50,6 +50,10 @@ struct jsonrpc {
 struct ovs_list output; /* Contains "struct ofpbuf"s. */
 size_t output_count;/* Number of elements in "output". */
 size_t backlog;
+
+/* Limits. */
+size_t max_output;  /* 'output_count' disconnection threshold. */
+size_t max_backlog; /* 'backlog' disconnection threshold. */
 };
 
 /* Rate limit for error messages. */
@@ -178,6 +182,17 @@ jsonrpc_get_backlog(const struct jsonrpc *rpc)
 return rpc->status ? 0 : rpc->backlog;
 }
 
+/* Sets thresholds for send backlog.  If send backlog contains more than
+ * 'max_n_msgs' messages or larger than 'max_backlog_bytes' bytes, connection
+ * will be dropped. */
+void
+jsonrpc_set_backlog_threshold(struct jsonrpc *rpc,
+  size_t max_n_msgs, size_t max_backlog_bytes)
+{
+rpc->max_output = max_n_msgs;
+rpc->max_backlog = max_backlog_bytes;
+}
+
 /* Returns the number of bytes that have been received on 'rpc''s underlying
  * stream.  (The value wraps around if it exceeds UINT_MAX.) */
 unsigned int
@@ -261,9 +276,26 @@ jsonrpc_send(struct jsonrpc *rpc, struct jsonrpc_msg *msg)
 rpc->backlog += length;
 
 if (rpc->output_count >= 50) {
-VLOG_INFO_RL(, "excessive sending backlog, jsonrpc: %s, num of"
+static struct vlog_rate_limit bl_rl = VLOG_RATE_LIMIT_INIT(5, 5);
+bool disconnect = false;
+
+VLOG_INFO_RL(_rl, "excessive sending backlog, jsonrpc: %s, num of"
  " msgs: %"PRIuSIZE", backlog: %"PRIuSIZE".", rpc->name,
  rpc->output_count, rpc->backlog);
+if (rpc->max_output && rpc->output_count > rpc->max_output) {
+disconnect = true;
+VLOG_WARN("sending backlog exceeded maximum number of messages (%"
+  PRIuSIZE" > %"PRIuSIZE"), disconnecting, jsonrpc: %s.",
+  rpc->output_count, rpc->max_output, rpc->name);
+} else if (rpc->max_backlog && rpc->backlog > rpc->max_backlog) {
+disconnect = true;
+VLOG_WARN("sending backlog exceeded maximum size (%"PRIuSIZE" > %"
+  PRIuSIZE" bytes), disconnecting, jsonrpc: %s.",
+  rpc->backlog, rpc->max_backlog, rpc->name);
+}
+if (disconnect) {
+jsonrpc_error(rpc, E2BIG);
+}
 }
 
 if (rpc->backlog == length) {
@@ -787,6 +819,10 @@ struct jsonrpc_session {
 int last_error;
 unsigned int seqno;
 uint8_t dscp;
+
+/* Limits for jsonrpc. */
+size_t max_n_msgs;
+size_t max_backlog_bytes;
 };
 
 static void
@@ -842,6 +878,8 @@

Re: [ovs-dev] [PATCH 3/5] raft: Set threshold on backlog for raft connections.

2020-11-03 Thread Ilya Maximets

On 10/28/20 11:49 AM, Dumitru Ceara wrote:
> On 10/26/20 2:42 AM, Ilya Maximets wrote:
>> RAFT messages could be fairly big.  If something abnormal happens to
>> one of the servers in a cluster it may not be able to process all the
>> incoming messages in a timely manner.  This results in jsonrpc backlog
>> growth on the sender's side.  For example if follower gets many new
>> clients at once that it needs to serve, or it decides to take a
>> snapshot in a period of high number of database changes.
>> If backlog grows large enough it becomes harder and harder for follower
>> to process incoming raft messages, it sends outdated replies and
>> starts receiving snapshots and the whole raft log from the leader.
>> Sometimes backlog grows too high (60GB in this example):
>>
>>   jsonrpc|INFO|excessive sending backlog, jsonrpc: ssl:,
>>num of msgs: 15370, backlog: 61731060773.
>>
>> In this case OS might actually decide to kill the sender to free some
>> memory.  Anyway, It could take a lot of time for such a server to catch
>> up with the rest of the cluster if it has so much data to receive and
>> process.
>>
>> Introducing backlog thresholds for jsonrpc connections.
>> If sending backlog will exceed particular values (500 messages or
>> 4GB in size), connection will be dropped and re-created.  This will
>> allow to drop all the current backlog and start over increasing
>> chances of cluster recovery.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=129
>> Signed-off-by: Ilya Maximets 
>> ---
>>  NEWS  |  2 ++
>>  lib/jsonrpc.c | 57 ++-
>>  lib/jsonrpc.h |  6 ++
>>  ovsdb/raft.c  |  5 +
>>  4 files changed, 69 insertions(+), 1 deletion(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 2860a8e9c..ebdf8758b 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -6,6 +6,8 @@ Post-v2.14.0
>>   * New unixctl command 'ovsdb-server/memory-trim-on-compaction on|off'.
>> If turned on, ovsdb-server will try to reclaim all the unused memory
>> after every DB compaction back to OS.  Disabled by default.
>> + * Maximum backlog on RAFT connections limited to 500 messages or 4GB.
>> +   Once threshold reached, connection is dropped (and re-established).
>> - DPDK:
>>   * Removed support for vhost-user dequeue zero-copy.
>> - The environment variable OVS_UNBOUND_CONF, if set, is now used
>> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
>> index ecbc939fe..435824844 100644
>> --- a/lib/jsonrpc.c
>> +++ b/lib/jsonrpc.c
>> @@ -50,6 +50,10 @@ struct jsonrpc {
>>  struct ovs_list output; /* Contains "struct ofpbuf"s. */
>>  size_t output_count;/* Number of elements in "output". */
>>  size_t backlog;
>> +
>> +/* Limits. */
>> +size_t max_output;  /* 'output_count' disconnection threshold. 
>> */
>> +size_t max_backlog; /* 'backlog' disconnection threshold. */
>>  };
>>  
>>  /* Rate limit for error messages. */
>> @@ -178,6 +182,17 @@ jsonrpc_get_backlog(const struct jsonrpc *rpc)
>>  return rpc->status ? 0 : rpc->backlog;
>>  }
>>  
>> +/* Sets thresholds for send backlog.  If send backlog contains more than
>> + * 'max_n_msgs' messages or larger than 'max_backlog_bytes' bytes, 
>> connection
>> + * will be dropped. */
>> +void
>> +jsonrpc_set_backlog_threshold(struct jsonrpc *rpc,
>> +  size_t max_n_msgs, size_t max_backlog_bytes)
>> +{
>> +rpc->max_output = max_n_msgs;
>> +rpc->max_backlog = max_backlog_bytes;
>> +}
>> +
>>  /* Returns the number of bytes that have been received on 'rpc''s underlying
>>   * stream.  (The value wraps around if it exceeds UINT_MAX.) */
>>  unsigned int
>> @@ -261,9 +276,26 @@ jsonrpc_send(struct jsonrpc *rpc, struct jsonrpc_msg 
>> *msg)
>>  rpc->backlog += length;
>>  
>>  if (rpc->output_count >= 50) {
>> -VLOG_INFO_RL(, "excessive sending backlog, jsonrpc: %s, num of"
>> +static struct vlog_rate_limit bl_rl = VLOG_RATE_LIMIT_INIT(5, 5);
>> +bool disconnect = false;
>> +
>> +VLOG_INFO_RL(_rl, "excessive sending backlog, jsonrpc: %s, num 
>> of"
>>   " msgs: %"PRIuSIZE", backlog: %"PRIuSIZE".", rpc->name,
>>   rpc->output_count, rpc->backlog);
>> +if (rpc->max_output && rpc->output_count > rpc->max_output) {
>> +disconnect = true;
>> +VLOG_WARN("sending backlog exceeded maximum number of messages 
>> (%"
>> +  PRIuSIZE" > %"PRIuSIZE"), disconnecting, jsonrpc: 
>> %s.",
>> +  rpc->output_count, rpc->max_output, rpc->name);
>> +} else if (rpc->max_backlog && rpc->backlog > rpc->max_backlog) {
>> +disconnect = true;
>> +VLOG_WARN("sending backlog exceeded maximum size (%"PRIuSIZE" > 
>> %"
>> +  PRIuSIZE" bytes), disconnecting, jsonrpc: %s.",
>> +  rpc->backlog,

[ovs-dev] [PATCH ovn 0/2] Further restrict ARP/ND broadcast domain for E-W and S-N traffic.

2020-11-03 Thread Dumitru Ceara

This series aims at further restricting ARP/ND broadcast domain for self
originated packets.  Until now those packets were always flooded on all
ports of the switch.

The first patch of the series is needed in order to remove dependency on
self-originated GARPs to be sent through the network (OVS) between OVN
logical ports.

Signed-off-by: Dumitru Ceara 

Dumitru Ceara (2):
  pinctrl: Directly udpate MAC_Bindings created by self originated GARPs.
  ovn-northd: Limit self originated ARP/ND broadcast domain.


 controller/pinctrl.c|  108 ++-
 lib/mcast-group-index.h |1 
 northd/ovn-northd.8.xml |   19 
 northd/ovn-northd.c |   84 +++--
 tests/ovn.at|   50 ++
 5 files changed, 173 insertions(+), 89 deletions(-)

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

[ovs-dev] [PATCH ovn 2/2] ovn-northd: Limit self originated ARP/ND broadcast domain.

2020-11-03 Thread Dumitru Ceara

Considering the following large scale deployment:
external-network -- public-logical-switch -- router-1 -- sw1 -- VIF-1
  -- router-2 -- sw2 -- VIF-2
  ...
  -- router-n -- swn -- VIF-n

To avoid hitting the max number of OVS resubmits (4K currently) OVN already
restricted the broadcast domain for ARP/ND requests received from the
external network and targeting an OVN-owned IP (e.g., owned by router-x).
Such packets are not flooded in the broadcast domain of the public logical
switch and instead are forwarded only to the port connecting router-x.

However, the max number of OVS resubmits can also be hit in the following
scenarios:
- router-x tries to resolve an IP owned by router-y (e.g., VIF-x trying to
  reach VIF-y via floating IP).
- router-x tries to resolve an IP owned by the external network.

Because ARP/ND requests in the above cases are originated by OVN router ports
they were being flooded in the complete broadcast domain of the public
logical switch.

Instead, we now create a new Multicast_Group for each logical switch and add
all non-router ports to it.  ARP/ND requests are now forwarded to the router
port that owns the IP (if any) and then flooded in this restricted MC_FLOOD_L2
broadcast domain.

Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever 
possible.")
Signed-off-by: Dumitru Ceara 
---
 lib/mcast-group-index.h |1 +
 northd/ovn-northd.8.xml |   19 ++-
 northd/ovn-northd.c |   84 +--
 tests/ovn.at|   50 +---
 4 files changed, 86 insertions(+), 68 deletions(-)

diff --git a/lib/mcast-group-index.h b/lib/mcast-group-index.h
index ba995ba..72af117 100644
--- a/lib/mcast-group-index.h
+++ b/lib/mcast-group-index.h
@@ -30,6 +30,7 @@ enum ovn_mcast_tunnel_keys {
 OVN_MCAST_MROUTER_FLOOD_TUNNEL_KEY,
 OVN_MCAST_MROUTER_STATIC_TUNNEL_KEY,
 OVN_MCAST_STATIC_TUNNEL_KEY,
+OVN_MCAST_FLOOD_L2_TUNNEL_KEY,
 OVN_MIN_IP_MULTICAST,
 OVN_MAX_IP_MULTICAST = OVN_MAX_MULTICAST,
 };
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9b96ce9..b37cecd 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1367,18 +1367,19 @@ output;
   
 
   
-Priority-80 flows for each port connected to a logical router
-matching self originated GARP/ARP request/ND packets. These packets
-are flooded to the MC_FLOOD which contains all logical
-ports.
+Priority-80 flows for each IP address/VIP/NAT address owned by a
+router port connected to the switch. These flows match ARP requests
+and ND packets for the specific IP addresses.  Matched packets are
+forwarded only to the router that owns the IP address and to the
+MC_FLOOD_L2 multicast group which contains all non-router
+logical ports.
   
 
   
-Priority-75 flows for each IP address/VIP/NAT address owned by a
-router port connected to the switch. These flows match ARP requests
-and ND packets for the specific IP addresses.  Matched packets are
-forwarded only to the router that owns the IP address and, if
-present, to the localnet port of the logical switch.
+Priority-75 flows for each port connected to a logical router
+matching self originated ARP request/ND packets.  These packets
+are flooded to the MC_FLOOD_L2 which contains all
+non-router logical ports.
   
 
   
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8800a3d..dbe5fa6 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -1559,6 +1559,12 @@ lsp_is_external(const struct nbrec_logical_switch_port 
*nbsp)
 }
 
 static bool
+lsp_is_router(const struct nbrec_logical_switch_port *nbsp)
+{
+return !strcmp(nbsp->type, "router");
+}
+
+static bool
 lrport_is_enabled(const struct nbrec_logical_router_port *lrport)
 {
 return !lrport->enabled || *lrport->enabled;
@@ -2488,7 +2494,7 @@ join_logical_ports(struct northd_context *ctx,
  * to their peers. */
 struct ovn_port *op;
 HMAP_FOR_EACH (op, key_node, ports) {
-if (op->nbsp && !strcmp(op->nbsp->type, "router") && !op->derived) {
+if (op->nbsp && lsp_is_router(op->nbsp) && !op->derived) {
 const char *peer_name = smap_get(>nbsp->options, 
"router-port");
 if (!peer_name) {
 continue;
@@ -3105,7 +3111,7 @@ ovn_port_update_sbrec(struct northd_context *ctx,
 
 sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
 } else {
-if (strcmp(op->nbsp->type, "router")) {
+if (!lsp_is_router(op->nbsp)) {
 uint32_t queue_id = smap_get_int(
 >sb->options, "qdisc_queue_id", 0);
 bool has_qos = port_has_qos_params(>nbsp->options);
@@ -3843,6 +3849,10 @@

[ovs-dev] [PATCH ovn 1/2] pinctrl: Directly udpate MAC_Bindings created by self originated GARPs.

2020-11-03 Thread Dumitru Ceara

OVN uses GARPs to announce changes to locally owned NAT addresses.  This is
OK when updating upstream router caches but is unnecessary for updating OVN
logical router MAC_Bindings.

ovn-controller already has the information required for directly
updating/inserting the MAC_Bindings that would be created by neighbor
routers.

This also has the advantage that GARPs don't necessarily need to be flooded
in the complete L2 domain of the switch and that router patch ports can be
skipped.  An upcoming commit will take advantage of this.

Suggested-by: Lorenzo Bianconi 
Fixes: 81e928526b8a ("ovn-controller: Inject GARPs to logical switch pipeline 
to update neighbors")
Signed-off-by: Dumitru Ceara 
---
 controller/pinctrl.c |  108 --
 1 file changed, 87 insertions(+), 21 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 08adc10..795b52f 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -189,7 +189,7 @@ static void run_put_mac_bindings(
 struct ovsdb_idl_txn *ovnsb_idl_txn,
 struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
 struct ovsdb_idl_index *sbrec_port_binding_by_key,
-struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
+struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip);
 OVS_REQUIRES(pinctrl_mutex);
 static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
 static void flush_put_mac_bindings(void);
@@ -200,8 +200,10 @@ static void init_send_garps_rarps(void);
 static void destroy_send_garps_rarps(void);
 static void send_garp_rarp_wait(long long int send_garp_rarp_time);
 static void send_garp_rarp_prepare(
+struct ovsdb_idl_txn *ovnsb_idl_txn,
 struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
 struct ovsdb_idl_index *sbrec_port_binding_by_name,
+struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
 const struct ovsrec_bridge *,
 const struct sbrec_chassis *,
 const struct hmap *local_datapaths,
@@ -3146,8 +3148,9 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
  sbrec_mac_binding_by_lport_ip);
 run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key, chassis);
-send_garp_rarp_prepare(sbrec_port_binding_by_datapath,
-   sbrec_port_binding_by_name, br_int, chassis,
+send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
+   sbrec_port_binding_by_name,
+   sbrec_mac_binding_by_lport_ip, br_int, chassis,
local_datapaths, active_tunnels);
 prepare_ipv6_ras(local_datapaths);
 prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name,
@@ -3838,6 +3841,65 @@ mac_binding_lookup(struct ovsdb_idl_index 
*sbrec_mac_binding_by_lport_ip,
 return retval;
 }
 
+/* Update or add an IP-MAC binding for 'logical_port'. */
+static void
+mac_binding_add(struct ovsdb_idl_txn *ovnsb_idl_txn,
+struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+const char *logical_port,
+const struct sbrec_datapath_binding *dp,
+struct eth_addr ea, const char *ip)
+{
+/* Convert ethernet argument to string form for database. */
+char mac_string[ETH_ADDR_STRLEN + 1];
+snprintf(mac_string, sizeof mac_string,
+ETH_ADDR_FMT, ETH_ADDR_ARGS(ea));
+
+const struct sbrec_mac_binding *b =
+mac_binding_lookup(sbrec_mac_binding_by_lport_ip, logical_port, ip);
+if (!b) {
+b = sbrec_mac_binding_insert(ovnsb_idl_txn);
+sbrec_mac_binding_set_logical_port(b, logical_port);
+sbrec_mac_binding_set_ip(b, ip);
+sbrec_mac_binding_set_mac(b, mac_string);
+sbrec_mac_binding_set_datapath(b, dp);
+} else if (strcmp(b->mac, mac_string)) {
+sbrec_mac_binding_set_mac(b, mac_string);
+}
+}
+
+/* Simulate the effect of a GARP on local datapaths, i.e., create MAC_Bindings
+ * on peer router datapaths.
+ */
+static void
+send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
+  struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+  const struct hmap *local_datapaths,
+  const struct sbrec_port_binding *in_pb,
+  struct eth_addr ea, ovs_be32 ip)
+{
+const struct local_datapath *ldp =
+get_local_datapath(local_datapaths, in_pb->datapath->tunnel_key);
+
+ovs_assert(ldp);
+for (size_t i = 0; i < ldp->n_peer_ports; i++) {
+const struct sbrec_port_binding *local = ldp->peer_ports[i].local;
+const struct sbrec_port_binding *remote = ldp->peer_ports[i].remote;
+
+/* Skip "ingress" port. */
+if (local == in_pb) {
+continue;
+}
+
+struct ds ip_s = DS_EMPTY_INITIALIZER;
+
+ip_format_masked(ip, OVS_BE32_MAX, _s);
+mac_binding_add(ovnsb_idl_txn,

Re: [ovs-dev] [PATCH 5/5] raft: Avoid having more than one snapshot in-flight.

2020-11-03 Thread Ilya Maximets

On 10/28/20 11:58 AM, Dumitru Ceara wrote:
> On 10/26/20 2:42 AM, Ilya Maximets wrote:
>> Previous commit 8c2c503bdb0d ("raft: Avoid sending equal snapshots.")
>> took a "safe" approach to not send only exactly same snapshot
>> installation requests.  However, it doesn't make much sense to send
>> more than one snapshot at a time.  If obsolete snapshot installed,
>> leader will re-send the most recent one.
>>
>> With this change leader will have only 1 snapshot in-flight per
>> connection.  This will reduce backlogs on raft connections in case
>> new snapshot created while 'install_snapshot_request' is in progress
>> or if election timer changed in that period.
>>
>> Also, not tracking the exact 'install_snapshot_request' we've sent
>> allows to simplify the code.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=129
>> Fixes: 8c2c503bdb0d ("raft: Avoid sending equal snapshots.")
>> Signed-off-by: Ilya Maximets 
>> ---
> 
> Looks good to me, thanks!
> 
> Acked-by: Dumitru Ceara 
> 


Thanks!  Applied to master.
Will backport down to 2.13 as soon as TravisCI finishes the check.

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

Re: [ovs-dev] [PATCH 2/5] ovsdb-server: Reclaim heap memory after compaction.

2020-11-03 Thread Ilya Maximets

On 10/28/20 11:57 AM, Dumitru Ceara wrote:
> On 10/26/20 2:42 AM, Ilya Maximets wrote:
>> Compaction happens at most once in 10 minutes.  That is a big time
>> interval for a heavy loaded ovsdb-server in cluster mode.
>> In 10 minutes raft logs could grow up to tens of thousands of entries
>> with tens of gigabytes in total size.
>> While compaction cleans up raft log entries, the memory in many cases
>> is not returned to the system, but kept in the heap of running
>> ovsdb-server process, and it could stay in this condition for a really
>> long time.  In the end one performance spike could lead to a fast
>> growth of the raft log and this memory will never (for a really long
>> time) be released to the system even if the database if empty.
>>
>> Simple example how to reproduce with OVN sandbox:
>>
>> 1. make sandbox SANDBOXFLAGS='--nbdb-model=clustered --sbdb-model=clustered'
>>
>> 2. Run following script that creates 1 port group, adds 4000 acls and
>>removes all of that in the end:
>>
>># cat ../memory-test.sh
>>pg_name=my_port_group
>>export OVN_NB_DAEMON=$(ovn-nbctl --pidfile --detach --log-file 
>> -vsocket_util:off)
>>ovn-nbctl pg-add $pg_name
>>for i in $(seq 1 4000); do
>>  echo "Iteration: $i"
>>  ovn-nbctl --log acl-add $pg_name from-lport $i udp drop
>>done
>>ovn-nbctl acl-del $pg_name
>>ovn-nbctl pg-del $pg_name
>>ovs-appctl -t $(pwd)/sandbox/nb1 memory/show
>>ovn-appctl -t ovn-nbctl exit
>>---
>>
>> 3. Stopping one of Northbound DB servers:
>>ovs-appctl -t $(pwd)/sandbox/nb1 exit
>>
>>Make sure that ovsdb-server didn't compact the database before
>>it was stopped.  Now we have a db file on disk that contains
>>4000 fairly big transactions inside.
>>
>> 4. Trying to start same ovsdb-server with this file.
>>
>># cd sandbox && ovsdb-server <...> nb1.db
>>
>>At this point ovsdb-server reads all the transactions from db
>>file and performs all of them as fast as it can one by one.
>>When it finishes this, raft log contains 4000 entries and
>>ovsdb-server consumes (on my system) ~13GB of memory while
>>database is empty.  And libc will likely never return this memory
>>back to system, or, at least, will hold it for a really long time.
>>
>> This patch adds a new command 'ovsdb-server/memory-trim-on-compaction'.
>> It's disabled by default, but once enabled, ovsdb-server will call
>> 'malloc_trim(0)' after every successful compaction to try to return
>> unused heap memory back to system.  This is glibc-specific, so we
>> need to detect function availability in a build time.
>> Disabled by default since it adds from 1% to 30% (depending on the
>> current state) to the snapshot creation time and, also, next memory
>> allocations will likely require requests to kernel and that might be
>> slower.  Could be enabled by default later if considered broadly
>> beneficial.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=129
>> Signed-off-by: Ilya Maximets 
>> ---
> 
> Looks good to me, thanks!
> 
> Acked-by: Dumitru Ceara 
> 


Thanks!  There was a small issue with ifdef since the value always defined
but with different values.  I fixed it by s/ifdef/if/  and s/ifndef /if !/.
With that applied to master.
Will backport down to 2.13 with dependencies as soon as TravisCI finishes the 
check.

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

Re: [ovs-dev] [PATCH 1/5] raft: Add log length to the memory report.

2020-11-03 Thread Ilya Maximets

On 10/28/20 11:56 AM, Dumitru Ceara wrote:
> On 10/26/20 2:42 AM, Ilya Maximets wrote:
>> In many cases a big part of a memory consumed by ovsdb-server process
>> is a raft log, so it's important to add its length to the memory
>> report.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
>>  ovsdb/raft.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/ovsdb/raft.c b/ovsdb/raft.c
>> index 657eed813..ac85c6b67 100644
>> --- a/ovsdb/raft.c
>> +++ b/ovsdb/raft.c
>> @@ -1029,6 +1029,7 @@ raft_get_memory_usage(const struct raft *raft, struct 
>> simap *usage)
>>  }
>>  simap_increase(usage, "raft-backlog-kB", backlog / 1000);
>>  simap_increase(usage, "raft-connections", cnt);
>> +simap_increase(usage, "raft-log", raft->log_end - raft->log_start);
>>  }
>>  
>>  /* Returns true if 'raft' has completed joining its cluster, has not left or
>>
> 
> Looks good to me, thanks!
> 
> Acked-by: Dumitru Ceara  

Thanks!  Applied to master.
Will backport down to 2.13 with dependencies as soon as TravisCI finishes the 
check.

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

Re: [ovs-dev] [PATCH v4 2/2] netdev-dpdk: Add option to configure VF MAC address.

2020-11-03 Thread Kevin Traynor

On 30/10/2020 15:45, Gaetan Rivet wrote:
> In some cloud topologies, using DPDK VF representors in guest requires
> configuring a VF before it is assigned to the guest.
> 
> A first basic option for such configuration is setting the VF MAC
> address. Add a key 'dpdk-vf-mac' to the 'options' column of the Interface
> table.
> 
> This option can be used as such:
> 
>$ ovs-vsctl add-port br0 dpdk-rep0 -- set Interface dpdk-rep0 type=dpdk \
>   options:dpdk-vf-mac=00:11:22:33:44:55
> 
> Signed-off-by: Gaetan Rivet 
> Suggested-by: Ilya Maximets 
> Acked-by: Eli Britstein 
> ---
>  Documentation/topics/dpdk/phy.rst | 25 
>  NEWS  |  2 +
>  lib/netdev-dpdk.c | 67 +++
>  vswitchd/vswitch.xml  | 18 +
>  4 files changed, 112 insertions(+)
> 
> diff --git a/Documentation/topics/dpdk/phy.rst 
> b/Documentation/topics/dpdk/phy.rst
> index 55a98e2b0..1415e1b8c 100644
> --- a/Documentation/topics/dpdk/phy.rst
> +++ b/Documentation/topics/dpdk/phy.rst
> @@ -379,6 +379,31 @@ an eth device whose mac address is 
> ``00:11:22:33:44:55``::
>  $ ovs-vsctl add-port br0 dpdk-mac -- set Interface dpdk-mac type=dpdk \
> options:dpdk-devargs="class=eth,mac=00:11:22:33:44:55"
>  
> +Representor specific configuration
> +~~
> +
> +In some topologies, a VF must be configured before being assigned to a
> +guest (VM) machine.  This configuration is done through VF-specific fields
> +in the ``options`` column of the ``Interface`` table.
> +
> +.. important::
> +
> +   Some DPDK port use `bifurcated drivers `__,
> +   which means that a kernel netdevice remains when Open vSwitch is stopped.
> +
> +   In such case, any configuration applied to a VF would remain set on the
> +   kernel netdevice, and be inherited from it when Open vSwitch is restarted,
> +   even if the options described in this section are unset from Open vSwitch.
> +
> +.. _bifurcated-drivers: 
> http://doc.dpdk.org/guides/linux_gsg/linux_drivers.html#bifurcated-driver
> +
> +- Configure the VF MAC address::
> +
> +$ ovs-vsctl set Interface dpdk-rep0 options:dpdk-vf-mac=00:11:22:33:44:55
> +
> +On successful configuration, the requested MAC is shown in the ``mac_in_use``
> +column of the ``Interface`` table.
> +

True, but it's simpler to say it's the configured mac. If you want to
qualify with the config success, you should add for config failure also.

If everyone agrees with using dpdk-vf-mac as output from multiple
commands where it can be different, then I think it deserves a comment
here. Even a slightly nicer version of

ovs-appctl dpctl/show -> configured dpdk-vf-mac=00:11:22:33:44:55
ovs-vsctl show -> requested dpdk-vf-mac=33:11:22:33:44:55
ovs-vsctl get Interface myport status -> configured
dpdk-vf-mac=00:11:22:33:44:55

Aside from that LGTM.

>  Jumbo Frames
>  
>  
> diff --git a/NEWS b/NEWS
> index 8bb5bdc3f..b8cb3e227 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -5,6 +5,8 @@ Post-v2.14.0
> status of the storage that's backing a database.
> - DPDK:
>   * Removed support for vhost-user dequeue zero-copy.
> + * New 'options:dpdk-vf-mac' field for DPDK interface of VF ports,
> +   that allows configuring the MAC address of a VF representor.
> - The environment variable OVS_UNBOUND_CONF, if set, is now used
>   as the DNS resolver's (unbound) configuration file.
> - Linux datapath:
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 084f97807..d678def4b 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -522,6 +522,9 @@ struct netdev_dpdk {
>   * otherwise interrupt mode is used. */
>  bool requested_lsc_interrupt_mode;
>  bool lsc_interrupt_mode;
> +
> +/* VF configuration. */
> +struct eth_addr requested_hwaddr;
>  );
>  
>  PADDED_MEMBERS(CACHE_LINE_SIZE,
> @@ -1692,6 +1695,16 @@ out:
>  return ret;
>  }
>  
> +static bool
> +dpdk_port_is_representor(struct netdev_dpdk *dev)
> +OVS_REQUIRES(dev->mutex)
> +{
> +struct rte_eth_dev_info dev_info;
> +
> +rte_eth_dev_info_get(dev->port_id, _info);
> +return (*dev_info.dev_flags) & RTE_ETH_DEV_REPRESENTOR;
> +}
> +
>  static int
>  netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)
>  {
> @@ -1726,6 +1739,11 @@ netdev_dpdk_get_config(const struct netdev *netdev, 
> struct smap *args)
>  }
>  smap_add(args, "lsc_interrupt_mode",
>   dev->lsc_interrupt_mode ? "true" : "false");
> +
> +if (dpdk_port_is_representor(dev)) {
> +smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
> +ETH_ADDR_ARGS(dev->requested_hwaddr));
> +}
>  }
>  ovs_mutex_unlock(>mutex);
>  
> @@ -1905,6 +1923,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
> struct smap *args,
>  {RTE_FC_RX_PAUSE, RTE_FC_FULL}
>  };
>

[ovs-dev] [PATCH v3 2/2] netdev-dpdk: Add option to configure VF MAC address.

2020-11-03 Thread Kevin Traynor

On 27/10/2020 14:03, Ilya Maximets wrote:
> On 10/20/20 1:15 PM, Kevin Traynor wrote:
>> On 16/09/2020 16:17, Gaetan Rivet wrote:
>>> In some cloud topologies, using DPDK VF representors in guest requires
>>> configuring a VF before it is assigned to the guest.
>>>
>>> A first basic option for such configuration is setting the VF MAC
>>> address. Add a key 'dpdk-vf-mac' to the 'options' column of the Interface
>>> table.
>>>
>>> This option can be used as such:
>>>
>>>$ ovs-vsctl add-port br0 dpdk-rep0 -- set Interface dpdk-rep0 type=dpdk \
>>>   options:dpdk-vf-mac=00:11:22:33:44:55
>>>
>>
>> If I got it right, it doesn't seem like it solves any issues by limiting
>> to representor, but is just an attempt to limit some of the edge cases
>> around bifurcated devices to the devices where the requested
>> functionality is really needed now.
>>
>> This limitation has some costs...
>>
>> We will now have, mac, mac_in_use, dpdk-vf-mac, configured_hwaddr,
>> requested_hwaddr for the user to understand. I think it can be confusing.
> 
> To be clear, 'configured_hwaddr/requested_hwaddr' type of report should not
> exist at least in the output of 'get_config()' function.  Same applies
> to all existing '{configured,requested}_something' reports.
> 'get_config()' intended to report things that could be copied and passed 
> directly
> to database.  OTOH, 'get_status()' should report what is an actual device
> status, i.e. 'dpdk-vf-mac' should be in 'get_config()' and 'configured_hwaddr'
> should be in 'get_status()', but it should be named differently, e.g. just
> 'dpdk-vf-mac' and the callback it placed in already says that it's actually
> 'configured'.  In short:
>   - 'dpdk-vf-mac' from the 'get_config()' should report dev->requested_hwaddr.
>   - 'dpdk-vf-mac' from the 'get_status()' should report dev->hwaddr.

If an invalid mac is attempted or it fails to be set, this is what the
user will see for dpdk-vf-mac:

ovs-appctl dpctl/show -> dpdk-vf-mac=00:11:22:33:44:55
ovs-vsctl show -> dpdk-vf-mac=33:11:22:33:44:55
ovs-vsctl get Interface myport status -> dpdk-vf-mac=00:11:22:33:44:55

For completeness, i'll add the failed attempt is logged clearly.

I suppose we can tell the user to check the 'mac_in_use', but multiple
dpdk-vf-mac entries for the same device with different values is confusing.

requested_dpdk-vf-mac configured_dpdk-vf-mac from 'ovs-appctl
dpctl/show' would be explicit. At a minimum we should document which one
is requested and which one is configured.

> Of course, all this should be reported only if device is a representor.
> 
> 
>>
>> 'ovs-vsctl show' will show the dpdk-vf-mac, which is the main one we
>> document, but it's not clear if it was successful or not. (yes there is
>> a log entry, but it is a separate place).
>>
>> As it is specific for representors, if we ever want to allow setting of
>> mac on any non representor DPDK ports, we have an awkward user
>> interface. We would need to make another 'dpdk-mac' type option, or we
>> decide then to allow use mac in interface table but that it doesn't work
>> for representors, or there are two ways to set for representors etc.
>> None of this seems great.
>>
>> My feeling is that it is making things complicated for the user with
>> this many knobs, where we could just have mac and mac_in_use, live with
>> the edge cases (as Gaetan pointed out, we already do for MTU) and we
>> know as well if we ever need to extend further, the user interface will
>> stay simple. Just my 2C, maybe there's a good argument why we can't do
>> it like this.
> 
> Setting mac for physical device has lots of split-brain issues and unclear
> ways how and when undo changes as we do not know what was original mac or
> what to do if there was no mac at all at the start.  Moreover, some devices
> will not allow setting mac to it's original value (e.g. all zeroes).
> One more point is that vf and representor itself are different devices and
> these devices could have different mac addresses.  For now we agreed that

> DPDK api works for representors in a way that it always changes parameters
> of a virtual function, not representors', but that is not a very clear thing
> for all users (e.g. for DPDK developers), so 'vf' in 'dpdk-vf-mac' 
> specifically
> points to that difference.
> 

That's a good point, it does distinguish and iirc some people made
the point that this could be important.

> After a long discussion we decided to make this option as specific as 
> possible,
> so users will not try to use it to configure anything else beside mac of a
> virtual function.  All restrictions and downsides of configuration of vf mac
> was/should be documented.
> 

ok, I can see the pros of this type of design too so I won't object, but
agree the downsides like the additional fields and ability for user to
understand clearly the output from commands above should be documented.

> 
>>
>> Few comments on the code as it is now below.
>>
>>
>>> Signed-off-by: Gaetan Rivet

Re: [ovs-dev] [PATCH] netdev-offload-tc: Use single thread for probing tc features

2020-11-03 Thread Ilya Maximets

On 11/2/20 1:07 PM, Roi Dayan wrote:
> There is no need for a thread start per probe.

This sounds like we're actually starting some pthreads here.
I think, it should be something like:
"There is no need for a 'once' variable per probe."

Same for the patch name. E.g.
"netdev-offload-tc: Use single 'once' variable for probing tc features."

One minor comment inline.

Best regards, Ilya Maximets.

> 
> Signed-off-by: Roi Dayan 
> Reviewed-by: Paul Blakey 
> ---
>  lib/netdev-offload-tc.c | 11 +++
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index e828a8683910..53662ef3f0e6 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1988,8 +1988,7 @@ probe_tc_block_support(int ifindex)
>  static int
>  netdev_tc_init_flow_api(struct netdev *netdev)
>  {
> -static struct ovsthread_once multi_mask_once = 
> OVSTHREAD_ONCE_INITIALIZER;
> -static struct ovsthread_once block_once = OVSTHREAD_ONCE_INITIALIZER;
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>  enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
>  uint32_t block_id = 0;
>  struct tcf_id id;
> @@ -2014,16 +2013,12 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>  /* make sure there is no ingress/egress qdisc */
>  tc_add_del_qdisc(ifindex, false, 0, hook);
>  
> -if (ovsthread_once_start(_once)) {
> +if (ovsthread_once_start()) {
>  probe_tc_block_support(ifindex);
>  /* Need to re-fetch block id as it depends on feature availability. 
> */
>  block_id = get_block_id_from_netdev(netdev);
> -ovsthread_once_done(_once);
> -}
> -
> -if (ovsthread_once_start(_mask_once)) {

It might be good to have an empty line here to visually separate block-related
things from the 'mask' ones.

>  probe_multi_mask_per_prio(ifindex);
> -ovsthread_once_done(_mask_once);
> +ovsthread_once_done();
>  }
>  
>  error = tc_add_del_qdisc(ifindex, true, block_id, hook);
> 

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

[ovs-dev] [PATCH] Documentation: Fix rendering of extra repo info for RHEL 8

2020-11-03 Thread Timothy Redaelli

In commit a82083ee3091 ("Documentation: Add extra repo info for RHEL 8")
a newline was missing to correctly generate the code block to add
codeready-builder repository.

This commit adds the missing newline to correctly generate the code block
with the RHEL 8 codeready-builder instructions.

Fixes: a82083ee3091 ("Documentation: Add extra repo info for RHEL 8")
Cc: gvrose8...@gmail.com
Signed-off-by: Timothy Redaelli 
---
 Documentation/intro/install/fedora.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/intro/install/fedora.rst 
b/Documentation/intro/install/fedora.rst
index e5324e1df..4a2f3507c 100644
--- a/Documentation/intro/install/fedora.rst
+++ b/Documentation/intro/install/fedora.rst
@@ -70,6 +70,7 @@ repositories to help yum-builddep, e.g.::
 $ subscription-manager repos --enable=rhel-7-server-optional-rpms
 
 or for RHEL 8::
+
 $ subscription-manager repos \
   --enable=codeready-builder-for-rhel-8-x86_64-rpms
 
-- 
2.28.0

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

Re: [ovs-dev] [PATCH ovn v2 2/7] northd: Refactor load balancer vip parsing.

2020-11-03 Thread Numan Siddique

On Fri, Oct 30, 2020 at 1:52 AM Dumitru Ceara  wrote:
>
> On 10/27/20 6:16 PM, num...@ovn.org wrote:
> > From: Numan Siddique 
> >
> > Parsing of the load balancer VIPs is moved to a separate file - lib/lb.c.
> > ovn-northd makes use of these functions. Upcoming patch will make use of 
> > these
> > util functions for parsing SB Load_Balancers.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  lib/automake.mk |   4 +-
> >  lib/lb.c| 236 
> >  lib/lb.h|  77 
> >  lib/ovn-util.c  |  28 +
> >  lib/ovn-util.h  |   2 +
> >  northd/ovn-northd.c | 286 +---
> >  6 files changed, 378 insertions(+), 255 deletions(-)
> >  create mode 100644 lib/lb.c
> >  create mode 100644 lib/lb.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index f3e9c8818b..430cd11fc6 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -23,7 +23,9 @@ lib_libovn_la_SOURCES = \
> >   lib/ovn-util.h \
> >   lib/logical-fields.c \
> >   lib/inc-proc-eng.c \
> > - lib/inc-proc-eng.h
> > + lib/inc-proc-eng.h \
> > + lib/lb.c \
> > + lib/lb.h
> >  nodist_lib_libovn_la_SOURCES = \
> >   lib/ovn-dirs.c \
> >   lib/ovn-nb-idl.c \
> > diff --git a/lib/lb.c b/lib/lb.c
> > new file mode 100644
> > index 00..db2d3d552f
> > --- /dev/null
> > +++ b/lib/lb.c
> > @@ -0,0 +1,236 @@
> > +/* Copyright (c) 2020, Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include 
> > +
> > +#include "lb.h"
> > +#include "lib/ovn-nb-idl.h"
> > +#include "lib/ovn-sb-idl.h"
> > +#include "lib/ovn-util.h"
> > +
> > +/* OpenvSwitch lib includes. */
> > +#include "openvswitch/vlog.h"
> > +#include "lib/smap.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(lb);
> > +
> > +static struct ovn_lb *
> > +ovn_lb_create(const struct smap *vips)
> > +{
> > +struct ovn_lb *lb = xzalloc(sizeof *lb);
> > +
> > +lb->n_vips = smap_count(vips);
> > +lb->vips = xcalloc(lb->n_vips, sizeof (struct lb_vip));
> > +struct smap_node *node;
> > +size_t n_vips = 0;
> > +
> > +SMAP_FOR_EACH (node, vips) {
> > +char *vip;
> > +uint16_t port;
> > +int addr_family;
> > +
> > +if (!ip_address_and_port_from_key(node->key, , ,
> > +  _family)) {
> > +continue;
> > +}
> > +
> > +lb->vips[n_vips].vip = vip;
> > +lb->vips[n_vips].vip_port = port;
> > +lb->vips[n_vips].addr_family = addr_family;
> > +lb->vips[n_vips].vip_port_str = xstrdup(node->key);
> > +lb->vips[n_vips].backend_ips = xstrdup(node->value);
> > +
> > +char *tokstr = xstrdup(node->value);
> > +char *save_ptr = NULL;
> > +char *token;
> > +size_t n_backends = 0;
> > +/* Format for a backend ips : IP1:port1,IP2:port2,...". */
> > +for (token = strtok_r(tokstr, ",", _ptr);
> > +token != NULL;
> > +token = strtok_r(NULL, ",", _ptr)) {
> > +n_backends++;
> > +}
> > +
> > +free(tokstr);
> > +tokstr = xstrdup(node->value);
> > +save_ptr = NULL;
> > +
> > +lb->vips[n_vips].n_backends = n_backends;
> > +lb->vips[n_vips].backends = xcalloc(n_backends,
> > +sizeof (struct 
> > lb_vip_backend));
>
> This should be 'sizeof *lb->vips[n_vips].backends'.

This part of the code is moved from ovn-northd.c. We could use both -
an expression
or a structure in sizeof. The coding guidelines give preference to the former.

>
> > +
>
> Nit: It's probably fine to remove an empty line here, the one above is already
> indented enough to the right.
>
> > +size_t i = 0;
> > +for (token = strtok_r(tokstr, ",", _ptr);
> > +token != NULL;
> > +token = strtok_r(NULL, ",", _ptr)) {
> > +char *backend_ip;
> > +uint16_t backend_port;
> > +
> > +if (!ip_address_and_port_from_key(token, _ip,
> > +  _port,
> > +  _family)) {
> > +continue;
> > +}
> > +
> > +lb->vips[n_vips].backends[i].ip = backend_ip;
> > +lb->vips[n_vips].backends[i].port = backend_port;
> > +

Re: [ovs-dev] [PATCH net] net: openvswitch: silence suspicious RCU usage warning

2020-11-03 Thread Eelco Chaudron




On 2 Nov 2020, at 20:51, Jakub Kicinski wrote:

> On Mon, 02 Nov 2020 09:52:19 +0100 Eelco Chaudron wrote:
>> On 30 Oct 2020, at 22:28, Jakub Kicinski wrote:
 @@ -1695,6 +1695,9 @@ static int ovs_dp_cmd_new(struct sk_buff *skb,
 struct genl_info *info)
if (err)
goto err_destroy_ports;

 +  /* So far only local changes have been made, now need the lock. */
 +  ovs_lock();
>>>
>>> Should we move the lock below assignments to param?
>>>
>>> Looks a little strange to protect stack variables with a global lock.
>>
>> You are right, I should have moved it down after the assignment. I will
>> send out a v2.
>>
>>> Let's update the name of the label.
>>
>> Guess now it is, unlock and destroy meters, so what label are you
>> looking for?
>>
>> err_unlock_and_destroy_meters: which looks a bit long, or just
>> err_unlock:
>
> I feel like I saw some names like err_unlock_and_destroy_meters in OvS
> code, but can't find them in this file right now.
>
> I'd personally go for kist err_unlock, or maybe err_unlock_ovs as is
> used in other functions in this file.
>
> But as long as it starts with err_unlock it's fine by me :)

Ack, sent out a v2.

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

[ovs-dev] [PATCH net v2] net: openvswitch: silence suspicious RCU usage warning

2020-11-03 Thread Eelco Chaudron

Silence suspicious RCU usage warning in ovs_flow_tbl_masks_cache_resize()
by replacing rcu_dereference() with rcu_dereference_ovsl().

In addition, when creating a new datapath, make sure it's configured under
the ovs_lock.

Fixes: 9bf24f594c6a ("net: openvswitch: make masks cache size configurable")
Reported-by: syzbot+9a8f8bfcc56e85780...@syzkaller.appspotmail.com
Signed-off-by: Eelco Chaudron 
---
v2: - Moved local variable initialization above lock
- Renamed jump label to indicate unlocking

 net/openvswitch/datapath.c   |   14 +++---
 net/openvswitch/flow_table.c |2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 832f898edb6a..9d6ef6cb9b26 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1703,13 +1703,13 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct 
genl_info *info)
parms.port_no = OVSP_LOCAL;
parms.upcall_portids = a[OVS_DP_ATTR_UPCALL_PID];
 
-   err = ovs_dp_change(dp, a);
-   if (err)
-   goto err_destroy_meters;
-
/* So far only local changes have been made, now need the lock. */
ovs_lock();
 
+   err = ovs_dp_change(dp, a);
+   if (err)
+   goto err_unlock_and_destroy_meters;
+
vport = new_vport();
if (IS_ERR(vport)) {
err = PTR_ERR(vport);
@@ -1725,8 +1725,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct 
genl_info *info)
ovs_dp_reset_user_features(skb, info);
}
 
-   ovs_unlock();
-   goto err_destroy_meters;
+   goto err_unlock_and_destroy_meters;
}
 
err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
@@ -1741,7 +1740,8 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct 
genl_info *info)
ovs_notify(_datapath_genl_family, reply, info);
return 0;
 
-err_destroy_meters:
+err_unlock_and_destroy_meters:
+   ovs_unlock();
ovs_meters_exit(dp);
 err_destroy_ports:
kfree(dp->ports);
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index f3486a37361a..c89c8da99f1a 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -390,7 +390,7 @@ static struct mask_cache *tbl_mask_cache_alloc(u32 size)
 }
 int ovs_flow_tbl_masks_cache_resize(struct flow_table *table, u32 size)
 {
-   struct mask_cache *mc = rcu_dereference(table->mask_cache);
+   struct mask_cache *mc = rcu_dereference_ovsl(table->mask_cache);
struct mask_cache *new;
 
if (size == mc->cache_size)

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

48 matches

Mail list logo