[ovs-dev] [PATCH v4 2/2] python: replace pyOpenSSL with ssl

2021-10-29 Thread Timothy Redaelli
Currently, pyOpenSSL is half-deprecated upstream and so it's removed on
some distributions (for example on CentOS Stream 9,
https://issues.redhat.com/browse/CS-336), but since OVS only
supports Python 3 it's possible to replace pyOpenSSL with "import ssl"
included in base Python 3.

Stream recv and send had to be splitted as _recv and _send, since SSLError
is a subclass of socket.error and so it was not possible to except for
SSLWantReadError and SSLWantWriteError in recv and send of SSLStream.

TCPstream._open cannot be used in SSLStream, since Python ssl module
requires the SSL socket to be created before connecting it, so
SSLStream._open needs to create the socket, create SSL socket and then
connect the SSL socket.

Reported-by: Timothy Redaelli 
Reported-at: https://bugzilla.redhat.com/1988429
Signed-off-by: Timothy Redaelli 
---
v3 -> v4:
 - Remove useless ValueError in self.socket.shutdown, as reported by
   Terry Wilson. It was wrongly here due to some previous tests, but
   it's not needed.
---
 .ci/linux-prepare.sh |  2 +-
 .cirrus.yml  |  2 +-
 .travis.yml  |  1 -
 python/ovs/poller.py |  6 +--
 python/ovs/stream.py | 91 ++--
 tests/ovsdb-idl.at   |  2 +-
 6 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
index c55125cf7..b9b499bad 100755
--- a/.ci/linux-prepare.sh
+++ b/.ci/linux-prepare.sh
@@ -21,7 +21,7 @@ make -j4 HAVE_LLVM= HAVE_SQLITE= install
 cd ..
 
 pip3 install --disable-pip-version-check --user \
-flake8 hacking sphinx pyOpenSSL wheel setuptools
+flake8 hacking sphinx wheel setuptools
 pip3 install --user --upgrade docutils
 pip3 install --user  'meson==0.47.1'
 
diff --git a/.cirrus.yml b/.cirrus.yml
index 480fea242..a7ae793bc 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -9,7 +9,7 @@ freebsd_build_task:
 
   env:
 DEPENDENCIES: automake libtool gmake gcc wget openssl python3
-PY_DEPS:  sphinx|openssl
+PY_DEPS:  sphinx
 matrix:
   COMPILER: gcc
   COMPILER: clang
diff --git a/.travis.yml b/.travis.yml
index 51d051108..c7aeede06 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -17,7 +17,6 @@ addons:
   - libjemalloc-dev
   - libnuma-dev
   - libpcap-dev
-  - python3-openssl
   - python3-pip
   - python3-sphinx
   - libelf-dev
diff --git a/python/ovs/poller.py b/python/ovs/poller.py
index 3624ec865..157719c3a 100644
--- a/python/ovs/poller.py
+++ b/python/ovs/poller.py
@@ -26,9 +26,9 @@ if sys.platform == "win32":
 import ovs.winutils as winutils
 
 try:
-from OpenSSL import SSL
+import ssl
 except ImportError:
-SSL = None
+ssl = None
 
 try:
 from eventlet import patcher as eventlet_patcher
@@ -73,7 +73,7 @@ class _SelectSelect(object):
 def register(self, fd, events):
 if isinstance(fd, socket.socket):
 fd = fd.fileno()
-if SSL and isinstance(fd, SSL.Connection):
+if ssl and isinstance(fd, ssl.SSLSocket):
 fd = fd.fileno()
 
 if sys.platform != 'win32':
diff --git a/python/ovs/stream.py b/python/ovs/stream.py
index f5a520862..ac5b0fd0c 100644
--- a/python/ovs/stream.py
+++ b/python/ovs/stream.py
@@ -22,9 +22,9 @@ import ovs.socket_util
 import ovs.vlog
 
 try:
-from OpenSSL import SSL
+import ssl
 except ImportError:
-SSL = None
+ssl = None
 
 if sys.platform == 'win32':
 import ovs.winutils as winutils
@@ -322,6 +322,12 @@ class Stream(object):
 The recv function will not block waiting for data to arrive.  If no
 data have been received, it returns (errno.EAGAIN, "") immediately."""
 
+try:
+return self._recv(n)
+except socket.error as e:
+return (ovs.socket_util.get_exception_errno(e), "")
+
+def _recv(self, n):
 retval = self.connect()
 if retval != 0:
 return (retval, "")
@@ -331,10 +337,7 @@ class Stream(object):
 if sys.platform == 'win32' and self.socket is None:
 return self.__recv_windows(n)
 
-try:
-return (0, self.socket.recv(n))
-except socket.error as e:
-return (ovs.socket_util.get_exception_errno(e), "")
+return (0, self.socket.recv(n))
 
 def __recv_windows(self, n):
 if self._read_pending:
@@ -396,6 +399,12 @@ class Stream(object):
 Will not block.  If no bytes can be immediately accepted for
 transmission, returns -errno.EAGAIN immediately."""
 
+try:
+return self._send(buf)
+except socket.error as e:
+return -ovs.socket_util.get_exception_errno(e)
+
+def _send(self, buf):
 retval = self.connect()
 if retval != 0:
 return -retval
@@ -409,10 +418,7 @@ class Stream(object):
 if sys.platform == 'win32' and self.socket is None:
 return self.__send_windows(buf)
 
-try:
-return self.socket.send(buf)
-  

[ovs-dev] [PATCH ovn 1/1] ovn-northd: Treat reachable and unreachable addresses identically.

2021-10-29 Thread Mark Michelson
When an ARP request reaches a logical switch that is connected to
gateway routers, if the IP address in the ARP is an "unreachable"
address, then the ARP is flooded out all logical switch ports.

This can become an issue when a logical switch is connected to many
gateway logical router ports. In one particular OpenStack case, we saw a
logical switch connected to 471 gateway logical router ports. When ARP
requests are flooded to all of those logical router ports, it results in
hitting the resubmit limit in OVS.

To correct this issue, this patch treats reachable and unreachable
addresses the same when receiving an ARP request. If the address can be
traced to a particular router port, then the ARP is unicast only to that
router port. This helps to avoid hitting the resubmit ceiling.

The test "ARP flood for unreachable addresses" is removed since we no
longer flood ARPs when destined for unreachable addresses.

Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=2009323

Signed-off-by: Mark Michelson 
---
 northd/northd.c | 185 
 tests/ovn-northd.at |  99 
 tests/ovn.at|  10 +++
 3 files changed, 23 insertions(+), 271 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index da4f9cd14..59286034d 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -6876,42 +6876,6 @@ build_lrouter_groups(struct hmap *ports, struct ovs_list 
*lr_list)
 }
 }
 
-/* Returns 'true' if the IPv4 'addr' is on the same subnet with one of the
- * IPs configured on the router port.
- */
-static bool
-lrouter_port_ipv4_reachable(const struct ovn_port *op, ovs_be32 addr)
-{
-for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
-struct ipv4_netaddr *op_addr = >lrp_networks.ipv4_addrs[i];
-
-if ((addr & op_addr->mask) == op_addr->network) {
-return true;
-}
-}
-return false;
-}
-
-/* Returns 'true' if the IPv6 'addr' is on the same subnet with one of the
- * IPs configured on the router port.
- */
-static bool
-lrouter_port_ipv6_reachable(const struct ovn_port *op,
-const struct in6_addr *addr)
-{
-for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
-struct ipv6_netaddr *op_addr = >lrp_networks.ipv6_addrs[i];
-
-struct in6_addr nat_addr6_masked =
-ipv6_addr_bitand(addr, _addr->mask);
-
-if (ipv6_addr_equals(_addr6_masked, _addr->network)) {
-return true;
-}
-}
-return false;
-}
-
 /*
  * Ingress table 22: Flows that flood self originated ARP/ND packets in the
  * switching domain.
@@ -6943,23 +6907,6 @@ build_lswitch_rport_arp_req_self_orig_flow(struct 
ovn_port *op,
 if (!nat->external_mac) {
 continue;
 }
-
-/* Check if the ovn port has a network configured on which we could
- * expect ARP requests/NS for the DNAT external_ip.
- */
-if (nat_entry_is_v6(nat_entry)) {
-struct in6_addr *addr = _entry->ext_addrs.ipv6_addrs[0].addr;
-
-if (!lrouter_port_ipv6_reachable(op, addr)) {
-continue;
-}
-} else {
-ovs_be32 addr = nat_entry->ext_addrs.ipv4_addrs[0].addr;
-
-if (!lrouter_port_ipv4_reachable(op, addr)) {
-continue;
-}
-}
 sset_add(_eth_addrs, nat->external_mac);
 }
 
@@ -7012,7 +6959,7 @@ arp_nd_ns_match(const char *ips, int addr_family, struct 
ds *match)
  * switching domain as regular broadcast.
  */
 static void
-build_lswitch_rport_arp_req_flow_for_reachable_ip(const char *ips,
+build_lswitch_rport_arp_req_flow(const char *ips,
 int addr_family, struct ovn_port *patch_op, struct ovn_datapath *od,
 uint32_t priority, struct hmap *lflows,
 const struct ovsdb_idl_row *stage_hint)
@@ -7043,30 +6990,6 @@ build_lswitch_rport_arp_req_flow_for_reachable_ip(const 
char *ips,
 ds_destroy();
 }
 
-/*
- * Ingress table 22: Flows that forward ARP/ND requests for "unreachable" IPs
- * (NAT or load balancer IPs configured on a router that are outside the
- * router's configured subnets).
- * These ARP/ND packets are flooded in the switching domain as regular
- * broadcast.
- */
-static void
-build_lswitch_rport_arp_req_flow_for_unreachable_ip(const char *ips,
-int addr_family, struct ovn_datapath *od, uint32_t priority,
-struct hmap *lflows, const struct ovsdb_idl_row *stage_hint)
-{
-struct ds match = DS_EMPTY_INITIALIZER;
-
-arp_nd_ns_match(ips, addr_family, );
-
-ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_L2_LKUP,
-priority, ds_cstr(),
-"outport = \""MC_FLOOD"\"; output;",
-stage_hint);
-
-ds_destroy();
-}
-
 /*
  * Ingress table 22: Flows that forward ARP/ND requests only to the routers
  * that own the addresses.
@@ -7101,9 +7024,8 @@ 

[ovs-dev] [PATCH ovn 0/1] Unreachable Address Madness

2021-10-29 Thread Mark Michelson
The patch being submitted here was originally submitted as part of a
series intending to fix connectivity issues with OpenStack floating IPs.
You can find that original patch here:

https://mail.openvswitch.org/pipermail/ovs-dev/2021-March/381318.html

Following that discussion, there were concerns about:

1) The validity of the use case being fixed. Specifically, it was asked
if it was valid to configure an unreachable address on a logical router
as either a DNAT external IP or a load balancer VIP.

2) Whether this change was reverting to a premise that caused a flow
explosion in ovn-kubernetes.

In the end, the suggestion was made to flood ARPs destined for
unreachable IPs instead of unicasting them. However, Dumitru did note
that:

"The downside is that we *might* hit the OVS resubmit limit for ARPs
targeting such FIPs on scaled setups but, I guess, that's a different
story."

Unfortunately, that is now happening in OpenStack instances that have
lots of gateway router ports connected to a single "public" logical
switch that receives ARPs via its localnet port. Traffic destined for
unreachable floating IPs never arrives because ARP requests
hit the resubmit limit in OVN. Since the ARP requests never receive
a response, the traffic can never reach the intended target.

Going back to the original concerns about this patch:

Regarding (1), yes, it's valid. OpenStack has relied on being able to
place IPs not in a router port's subnet on that router port for a long
time. It's actually the basis behind why they refer to them as
"floating" IPs. Though I think at this point this is well-understood,
based on our having special cases in ovn-northd to handle unreachable
addresses.

Regarding (2), I'm still not 100% sure about the details behind why this
might cause a flow explosion. If you compare this patch with the one
where we flood ARPs to all switch ports, this has no more logical flows.
We are essentially editing the actions of our existing flows to unicast
the ARPs instead of flooding them.

I'm re-proposing this patch because I think it makes the most sense to
unicast the ARP if we know which logical router port the IP address is
configured on, even if we think it is "unreachable". In addition,
ovn-kubernetes has gone through some massive changes since the original
patch was proposed, and so I don't even know if the concerns originally
raised are still valid.

Mark Michelson (1):
  ovn-northd: Treat reachable and unreachable addresses identically.

 northd/northd.c | 185 
 tests/ovn-northd.at |  99 
 tests/ovn.at|  10 +++
 3 files changed, 23 insertions(+), 271 deletions(-)

-- 
2.31.1

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


[ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support

2021-10-29 Thread Toms Atteka
This change adds a new OpenFlow field OFPXMT_OFB_IPV6_EXTHDR and
packets can be filtered using ipv6_ext flag.

Signed-off-by: Toms Atteka 
---
 include/uapi/linux/openvswitch.h |   6 ++
 net/openvswitch/flow.c   | 140 +++
 net/openvswitch/flow.h   |  14 
 net/openvswitch/flow_netlink.c   |  26 +-
 4 files changed, 184 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index a87b44cd5590..43790f07e4a2 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -342,6 +342,7 @@ enum ovs_key_attr {
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
OVS_KEY_ATTR_NSH,   /* Nested set of ovs_nsh_key_* */
+   OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
 
 #ifdef __KERNEL__
OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
@@ -421,6 +422,11 @@ struct ovs_key_ipv6 {
__u8   ipv6_frag;   /* One of OVS_FRAG_TYPE_*. */
 };
 
+/* separate structure to support backward compatibility with older user space 
*/
+struct ovs_key_ipv6_exthdrs {
+   __u16  hdrs;
+};
+
 struct ovs_key_tcp {
__be16 tcp_src;
__be16 tcp_dst;
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 9d375e74b607..28acb40437ca 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -239,6 +239,144 @@ static bool icmphdr_ok(struct sk_buff *skb)
  sizeof(struct icmphdr));
 }
 
+/**
+ * get_ipv6_ext_hdrs() - Parses packet and sets IPv6 extension header flags.
+ *
+ * @skb: buffer where extension header data starts in packet
+ * @nh: ipv6 header
+ * @ext_hdrs: flags are stored here
+ *
+ * OFPIEH12_UNREP is set if more than one of a given IPv6 extension header
+ * is unexpectedly encountered. (Two destination options headers may be
+ * expected and would not cause this bit to be set.)
+ *
+ * OFPIEH12_UNSEQ is set if IPv6 extension headers were not in the order
+ * preferred (but not required) by RFC 2460:
+ *
+ * When more than one extension header is used in the same packet, it is
+ * recommended that those headers appear in the following order:
+ *  IPv6 header
+ *  Hop-by-Hop Options header
+ *  Destination Options header
+ *  Routing header
+ *  Fragment header
+ *  Authentication header
+ *  Encapsulating Security Payload header
+ *  Destination Options header
+ *  upper-layer header
+ */
+static void get_ipv6_ext_hdrs(struct sk_buff *skb, struct ipv6hdr *nh,
+ u16 *ext_hdrs)
+{
+   u8 next_type = nh->nexthdr;
+   unsigned int start = skb_network_offset(skb) + sizeof(struct ipv6hdr);
+   int dest_options_header_count = 0;
+
+   *ext_hdrs = 0;
+
+   while (ipv6_ext_hdr(next_type)) {
+   struct ipv6_opt_hdr _hdr, *hp;
+
+   switch (next_type) {
+   case IPPROTO_NONE:
+   *ext_hdrs |= OFPIEH12_NONEXT;
+   /* stop parsing */
+   return;
+
+   case IPPROTO_ESP:
+   if (*ext_hdrs & OFPIEH12_ESP)
+   *ext_hdrs |= OFPIEH12_UNREP;
+   if ((*ext_hdrs & ~(OFPIEH12_HOP | OFPIEH12_DEST |
+  OFPIEH12_ROUTER | IPPROTO_FRAGMENT |
+  OFPIEH12_AUTH | OFPIEH12_UNREP)) ||
+   dest_options_header_count >= 2) {
+   *ext_hdrs |= OFPIEH12_UNSEQ;
+   }
+   *ext_hdrs |= OFPIEH12_ESP;
+   break;
+
+   case IPPROTO_AH:
+   if (*ext_hdrs & OFPIEH12_AUTH)
+   *ext_hdrs |= OFPIEH12_UNREP;
+   if ((*ext_hdrs &
+~(OFPIEH12_HOP | OFPIEH12_DEST | OFPIEH12_ROUTER |
+  IPPROTO_FRAGMENT | OFPIEH12_UNREP)) ||
+   dest_options_header_count >= 2) {
+   *ext_hdrs |= OFPIEH12_UNSEQ;
+   }
+   *ext_hdrs |= OFPIEH12_AUTH;
+   break;
+
+   case IPPROTO_DSTOPTS:
+   if (dest_options_header_count == 0) {
+   if (*ext_hdrs &
+   ~(OFPIEH12_HOP | OFPIEH12_UNREP))
+   *ext_hdrs |= OFPIEH12_UNSEQ;
+   *ext_hdrs |= OFPIEH12_DEST;
+   } else if (dest_options_header_count == 1) {
+   if (*ext_hdrs &
+   ~(OFPIEH12_HOP | OFPIEH12_DEST |
+ OFPIEH12_ROUTER | OFPIEH12_FRAG |
+   

Re: [ovs-dev] [ovn] bug: ovn-northd was blocked for changes handling

2021-10-29 Thread Han Zhou
On Fri, Oct 29, 2021 at 9:43 AM Vladislav Odintsov 
wrote:
>
> Hi Han,
>
> thanks for the fix.
> Unfortunately too much time has gone since that failure and I’ve
re-deployed my lab many times and all logs are unavailable.
> I see this problem reproduces when I run our internal tests, which are
quite intensively utilise NB.
> I’ll try reproduce this problem on weekend. I should check for SB logs to
see if failover was in place, right?

Yes, and it would be good to have timestamps for the roles
(leader/follower/candidate) observed for each server from each server's
perspective.

> One question here: as far as I know TS lswitches’ tunnel keys are
allocated in special space of IDs.
> logical_switch’s other_config:requested-tnl-key is used, right?
> Is it possible that lswitch is created with some tunnel_key value (for
instance, 2) and then ovn-northd tries
> to update tunnel_key and fails for some reason. Couldn’t it be a scenario
for the duplicated datapath’s in sb db?

I suspected that, too. However, I didn't see from the code updating
tunnel_key would result in duplicated datapaths. In this case it is likely
that two northds see different versions of the DBs:

(my assumption)
northd1: NB has requested-tnl-key populated, SB has the datapath with key 2
northd2: NB has the newly created TS without requested-tnl-key, SB has no
datapath created for it
Somehow both northds holds the SB lock (in different servers), so northd1
went ahead and updated the key in SB; northd2 went ahead and created the
datapath.

Thanks,
Han

>
> Regards,
> Vladislav Odintsov
>
> On 26 Oct 2021, at 03:55, Han Zhou  wrote:
>
> On Mon, Oct 4, 2021 at 3:02 PM Vladislav Odintsov 
wrote:
>
>
> Hi,
>
> I’ve faced with a next issue using latest OVN (master branch) with OVN-IC
>
> enabled:
>
> ovn-northd CPU utilisation was at ~70-80% and ovsdb-server serving
>
> OVN_Southbound DB was also under heavy load.
>
>
> In ovn-northd.log file there were warnings:
> 2021-10-01T11:14:43.548Z|18845|northd|INFO|deleting Datapath_Binding
>
> dd6af7f7-ea46-4496-988a-e7f9828924d0 with duplicate
> external-ids:logical-switch/router ec35e3e0-2674-47e7-b645-2c9b8b31865b
>
> 2021-10-01T11:14:44.148Z|18846|poll_loop|INFO|Dropped 32 log messages in
>
> last 6 seconds (most recently, 0 seconds ago) due to excessive rate
>
> 2021-10-01T11:14:44.148Z|18847|poll_loop|INFO|wakeup due to [POLLIN] on
>
> fd 19 (172.20.33.110:55202<->172.20.33.102:16642) at lib/stream-ssl.c:832
> (69% CPU usage)
>
> 2021-10-01T11:14:48.336Z|18848|ovsdb_idl|WARN|Dropped 307 log messages in
>
> last 60 seconds (most recently, 0 seconds ago) due to excessive rate
>
> 2021-10-01T11:14:48.336Z|18849|ovsdb_idl|WARN|transaction error:
>
> {"details":"Deletion of 1 weak reference(s) to deleted (or never-existing)
> rows from column \"datapath\" in \"IP_Multicast\" row
> 72bac803-e484-4358-9e48-11911c8aa16f caused this column to become empty,
> but constraints on this column disallow an empty
> column.","error":"constraint violation"}
>
>
> I checked datapath by reported logical-switch id:
>
> ~]# ovn-sbctl find datap
>
> external_ids:logical-switch=ec35e3e0-2674-47e7-b645-2c9b8b31865b
>
> _uuid   : 7e045551-7700-4a50-b0aa-02cb4e1be59d
> external_ids: {interconn-ts=vpc-CCF01DF6-rtb-216BABB1-global,
>
> logical-switch="ec35e3e0-2674-47e7-b645-2c9b8b31865b",
> name=vpc-CCF01DF6-rtb-216BABB1-global}
>
> load_balancers  : []
> tunnel_key  : 16712519
>
> _uuid   : dd6af7f7-ea46-4496-988a-e7f9828924d0
> external_ids: {interconn-ts=vpc-CCF01DF6-rtb-216BABB1-global,
>
> logical-switch="ec35e3e0-2674-47e7-b645-2c9b8b31865b",
> name=vpc-CCF01DF6-rtb-216BABB1-global}
>
> load_balancers  : []
> tunnel_key  : 5
>
>
> It refers to ovn-ic transit switch:
>
> ~]# ovn-ic-sbctl list datapath vpc-CCF01DF6-rtb-216BABB1-global
> _uuid   : fc159fa4-d5ba-46ed-a54c-d00745091021
> external_ids: {}
> transit_switch  : vpc-CCF01DF6-rtb-216BABB1-global
> tunnel_key  : 16712519
>
> ~]# ovn-ic-nbctl list transit vpc-CCF01DF6-rtb-216BABB1-global
> _uuid   : b5312889-92f9-40fe-98f9-2ea7ce3debcc
> external_ids: {}
> name: vpc-CCF01DF6-rtb-216BABB1-global
> other_config: {}
>
> The problem ip-multicast document:
>
> ~]# ovn-sbctl find ip-mul datapath=dd6af7f7-ea46-4496-988a-e7f9828924d0
> _uuid   : 72bac803-e484-4358-9e48-11911c8aa16f
> datapath: dd6af7f7-ea46-4496-988a-e7f9828924d0
> enabled : false
> eth_src : ""
> idle_timeout: 300
> ip4_src : ""
> ip6_src : ""
> querier : true
> query_interval  : 150
> query_max_resp  : 1
> seq_no  : 0
> table_size  : 2048
>
> I tried manually destroy this ip_multicast document, which blocked
>
> deletion of datapath document:
>
> ~]# ovn-sbctl destroy ip-mul 72bac803-e484-4358-9e48-11911c8aa16f
>
> ~]# ovn-sbctl list ip-mul 

[ovs-dev] [PATCH] ofproto-dpif-xlate: Fix check_pkt_larger incomplete translation.

2021-10-29 Thread numans
From: Numan Siddique 

xlate_check_pkt_larger() sets ctx->exit to 'true' at the end
causing the translation to stop. This results in incomplete
datapath rules.

For example, for the below OF rules configured on a bridge,

table=0,in_port=1 
actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1)
table=1,in_port=1,reg1=0x1 
actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4)
table=1,in_port=1,reg1=0x2 actions=output:2
table=1,in_port=1,reg1=0x3 actions=output:4
table=4,in_port=1 actions=output:3

the datapath flow should be

check_pkt_len(size=200,gt(3),le(3)),2,4

But right now it is:

check_pkt_len(size=200,gt(3),le(3))

Actions after the first resubmit(,1) in the first flow in table 0
are never applied.  This patch fixes this issue.

Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2018365
Reported-by: Ihar Hrachyshka 
Signed-off-by: Numan Siddique 
---
 ofproto/ofproto-dpif-xlate.c |  9 -
 tests/ofproto-dpif.at| 17 +
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9d336bc6a6..e22a39a9f4 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6371,6 +6371,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
  * then ctx->exit would be true. Reset to false so that we can
  * do flow translation for 'IF_LESS_EQUAL' case. finish_freezing()
  * would have taken care of Undoing the changes done for freeze. */
+bool old_exit = ctx->exit;
 ctx->exit = false;
 
 offset_attr = nl_msg_start_nested(
@@ -6395,7 +6396,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
 ctx->was_mpls = old_was_mpls;
 ctx->conntracked = old_conntracked;
 ctx->xin->flow = old_flow;
-ctx->exit = true;
+ctx->exit = old_exit;
 }
 
 static void
@@ -7192,6 +7193,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
  ofpacts_len);
 xlate_check_pkt_larger(ctx, ofpact_get_CHECK_PKT_LARGER(a),
remaining_acts, remaining_acts_len);
+if (ctx->xbridge->support.check_pkt_len) {
+/* If datapath supports check_pkt_len, then
+ * xlate_check_pkt_larger() does the translation for the
+ * ofpacts following 'a'. */
+a = ofpact_end(ofpacts, ofpacts_len);
+}
 break;
 }
 }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 31fb163a91..f7c8f98ce5 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -11452,6 +11452,23 @@ Megaflow: recirc_id=0x3,eth,ip,in_port=1,nw_frag=no
 Datapath actions: 4
 ])
 
+ovs-ofctl del-flows br0
+
+AT_DATA([flows.txt], [dnl
+table=0,in_port=1 
actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1)
+table=1,in_port=1,reg1=0x1 
actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4)
+table=1,in_port=1,reg1=0x2 actions=output:2
+table=1,in_port=1,reg1=0x3 actions=output:4
+table=4,in_port=1 actions=output:3
+])
+
+AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'],
 [0], [stdout])
+AT_CHECK([cat stdout | grep Datapath -B1], [0], [dnl
+Megaflow: recirc_id=0,eth,ip,in_port=1,nw_frag=no
+Datapath actions: check_pkt_len(size=200,gt(3),le(3)),2,4
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
-- 
2.31.1

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


Re: [ovs-dev] [ovn] bug: ovn-northd was blocked for changes handling

2021-10-29 Thread Vladislav Odintsov
Hi Han,

thanks for the fix.
Unfortunately too much time has gone since that failure and I’ve re-deployed my 
lab many times and all logs are unavailable.
I see this problem reproduces when I run our internal tests, which are quite 
intensively utilise NB.
I’ll try reproduce this problem on weekend. I should check for SB logs to see 
if failover was in place, right?
One question here: as far as I know TS lswitches’ tunnel keys are allocated in 
special space of IDs.
logical_switch’s other_config:requested-tnl-key is used, right?
Is it possible that lswitch is created with some tunnel_key value (for 
instance, 2) and then ovn-northd tries
to update tunnel_key and fails for some reason. Couldn’t it be a scenario for 
the duplicated datapath’s in sb db?

Regards,
Vladislav Odintsov

> On 26 Oct 2021, at 03:55, Han Zhou  wrote:
> 
> On Mon, Oct 4, 2021 at 3:02 PM Vladislav Odintsov  wrote:
>> 
>> Hi,
>> 
>> I’ve faced with a next issue using latest OVN (master branch) with OVN-IC
> enabled:
>> ovn-northd CPU utilisation was at ~70-80% and ovsdb-server serving
> OVN_Southbound DB was also under heavy load.
>> 
>> In ovn-northd.log file there were warnings:
>> 2021-10-01T11:14:43.548Z|18845|northd|INFO|deleting Datapath_Binding
> dd6af7f7-ea46-4496-988a-e7f9828924d0 with duplicate
> external-ids:logical-switch/router ec35e3e0-2674-47e7-b645-2c9b8b31865b
>> 2021-10-01T11:14:44.148Z|18846|poll_loop|INFO|Dropped 32 log messages in
> last 6 seconds (most recently, 0 seconds ago) due to excessive rate
>> 2021-10-01T11:14:44.148Z|18847|poll_loop|INFO|wakeup due to [POLLIN] on
> fd 19 (172.20.33.110:55202<->172.20.33.102:16642) at lib/stream-ssl.c:832
> (69% CPU usage)
>> 2021-10-01T11:14:48.336Z|18848|ovsdb_idl|WARN|Dropped 307 log messages in
> last 60 seconds (most recently, 0 seconds ago) due to excessive rate
>> 2021-10-01T11:14:48.336Z|18849|ovsdb_idl|WARN|transaction error:
> {"details":"Deletion of 1 weak reference(s) to deleted (or never-existing)
> rows from column \"datapath\" in \"IP_Multicast\" row
> 72bac803-e484-4358-9e48-11911c8aa16f caused this column to become empty,
> but constraints on this column disallow an empty
> column.","error":"constraint violation"}
>> 
>> I checked datapath by reported logical-switch id:
>> 
>> ~]# ovn-sbctl find datap
> external_ids:logical-switch=ec35e3e0-2674-47e7-b645-2c9b8b31865b
>> _uuid   : 7e045551-7700-4a50-b0aa-02cb4e1be59d
>> external_ids: {interconn-ts=vpc-CCF01DF6-rtb-216BABB1-global,
> logical-switch="ec35e3e0-2674-47e7-b645-2c9b8b31865b",
> name=vpc-CCF01DF6-rtb-216BABB1-global}
>> load_balancers  : []
>> tunnel_key  : 16712519
>> 
>> _uuid   : dd6af7f7-ea46-4496-988a-e7f9828924d0
>> external_ids: {interconn-ts=vpc-CCF01DF6-rtb-216BABB1-global,
> logical-switch="ec35e3e0-2674-47e7-b645-2c9b8b31865b",
> name=vpc-CCF01DF6-rtb-216BABB1-global}
>> load_balancers  : []
>> tunnel_key  : 5
>> 
>> 
>> It refers to ovn-ic transit switch:
>> 
>> ~]# ovn-ic-sbctl list datapath vpc-CCF01DF6-rtb-216BABB1-global
>> _uuid   : fc159fa4-d5ba-46ed-a54c-d00745091021
>> external_ids: {}
>> transit_switch  : vpc-CCF01DF6-rtb-216BABB1-global
>> tunnel_key  : 16712519
>> 
>> ~]# ovn-ic-nbctl list transit vpc-CCF01DF6-rtb-216BABB1-global
>> _uuid   : b5312889-92f9-40fe-98f9-2ea7ce3debcc
>> external_ids: {}
>> name: vpc-CCF01DF6-rtb-216BABB1-global
>> other_config: {}
>> 
>> The problem ip-multicast document:
>> 
>> ~]# ovn-sbctl find ip-mul datapath=dd6af7f7-ea46-4496-988a-e7f9828924d0
>> _uuid   : 72bac803-e484-4358-9e48-11911c8aa16f
>> datapath: dd6af7f7-ea46-4496-988a-e7f9828924d0
>> enabled : false
>> eth_src : ""
>> idle_timeout: 300
>> ip4_src : ""
>> ip6_src : ""
>> querier : true
>> query_interval  : 150
>> query_max_resp  : 1
>> seq_no  : 0
>> table_size  : 2048
>> 
>> I tried manually destroy this ip_multicast document, which blocked
> deletion of datapath document:
>> ~]# ovn-sbctl destroy ip-mul 72bac803-e484-4358-9e48-11911c8aa16f
>> 
>> ~]# ovn-sbctl list ip-mul 72bac803-e484-4358-9e48-11911c8aa16f
>> ovn-sbctl: no row "72bac803-e484-4358-9e48-11911c8aa16f" in table
> IP_Multicast
>> 
>> 
>> And problem was resolved. ovn-northd daemon stopped consuming CPU and
> excess datapath was deleted automatically.
>> 
>> ~]# ovn-sbctl find datapath
> external_ids:interconn-ts=vpc-CCF01DF6-rtb-216BABB1-global
>> _uuid   : 7e045551-7700-4a50-b0aa-02cb4e1be59d
>> external_ids: {interconn-ts=vpc-CCF01DF6-rtb-216BABB1-global,
> logical-switch="ec35e3e0-2674-47e7-b645-2c9b8b31865b",
> name=vpc-CCF01DF6-rtb-216BABB1-global}
>> load_balancers  : []
>> tunnel_key  : 16712519
>> 
>> The main problem here is that northd was entirely blocked. No processing
> to SB was done. Even ovn-controllers couldn’t 

[ovs-dev] [PATCH net] cls_flower: Fix inability to match GRE/IPIP packets

2021-10-29 Thread Yoshiki Komachi
When a packet of a new flow arrives in openvswitch kernel module, it dissects
the packet and passes the extracted flow key to ovs-vswtichd daemon. If hw-
offload configuration is enabled, the daemon creates a new TC flower entry to
bypass openvswitch kernel module for the flow (TC flower can also offload flows
to NICs but this time that does not matter).

In this processing flow, I found the following issue in cases of GRE/IPIP
packets.

When ovs_flow_key_extract() in openvswitch module parses a packet of a new
GRE (or IPIP) flow received on non-tunneling vports, it extracts information
of the outer IP header for ip_proto/src_ip/dst_ip match keys.

This means ovs-vswitchd creates a TC flower entry with IP protocol/addresses
match keys whose values are those of the outer IP header. OTOH, TC flower,
which uses flow_dissector (different parser from openvswitch module), extracts
information of the inner IP header.

The following flow is an example to describe the issue in more detail.

   <--- Outer IP -> <-- Inner IP -->
  +--+--+--+--+--+--+
  | ip_proto | src_ip   | dst_ip   | ip_proto | src_ip   | dst_ip   |
  | 47 (GRE) | 192.168.10.1 | 192.168.10.2 | 6 (TCP)  | 10.0.0.1 | 10.0.0.2 |
  +--+--+--+--+--+--+

In this case, TC flower entry and extracted information are shown as below:

  - ovs-vswitchd creates TC flower entry with:
  - ip_proto: 47
  - src_ip: 192.168.10.1
  - dst_ip: 192.168.10.2

  - TC flower extracts below for IP header matches:
  - ip_proto: 6
  - src_ip: 10.0.0.1
  - dst_ip: 10.0.0.2

Thus, GRE or IPIP packets never match the TC flower entry, as each
dissector behaves differently.

IMHO, the behavior of TC flower (flow dissector) does not look correct,
as ip_proto/src_ip/dst_ip in TC flower match means the outermost IP
header information except for GRE/IPIP cases. This patch adds a new
flow_dissector flag FLOW_DISSECTOR_F_STOP_BEFORE_ENCAP which skips
dissection of the encapsulated inner GRE/IPIP header in TC flower
classifier.

Signed-off-by: Yoshiki Komachi 
---
 include/net/flow_dissector.h |  1 +
 net/core/flow_dissector.c| 15 +++
 net/sched/cls_flower.c   |  3 ++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index ffd386ea0dbb..aa33e1092e2c 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -287,6 +287,7 @@ enum flow_dissector_key_id {
 #define FLOW_DISSECTOR_F_PARSE_1ST_FRAGBIT(0)
 #define FLOW_DISSECTOR_F_STOP_AT_FLOW_LABELBIT(1)
 #define FLOW_DISSECTOR_F_STOP_AT_ENCAP BIT(2)
+#define FLOW_DISSECTOR_F_STOP_BEFORE_ENCAP BIT(3)
 
 struct flow_dissector_key {
enum flow_dissector_key_id key_id;
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index bac0184cf3de..0d4bbf534c7d 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1307,6 +1307,11 @@ bool __skb_flow_dissect(const struct net *net,
 
switch (ip_proto) {
case IPPROTO_GRE:
+   if (flags & FLOW_DISSECTOR_F_STOP_BEFORE_ENCAP) {
+   fdret = FLOW_DISSECT_RET_OUT_GOOD;
+   break;
+   }
+
fdret = __skb_flow_dissect_gre(skb, key_control, flow_dissector,
   target_container, data,
   , , , flags);
@@ -1364,6 +1369,11 @@ bool __skb_flow_dissect(const struct net *net,
break;
}
case IPPROTO_IPIP:
+   if (flags & FLOW_DISSECTOR_F_STOP_BEFORE_ENCAP) {
+   fdret = FLOW_DISSECT_RET_OUT_GOOD;
+   break;
+   }
+
proto = htons(ETH_P_IP);
 
key_control->flags |= FLOW_DIS_ENCAPSULATION;
@@ -1376,6 +1386,11 @@ bool __skb_flow_dissect(const struct net *net,
break;
 
case IPPROTO_IPV6:
+   if (flags & FLOW_DISSECTOR_F_STOP_BEFORE_ENCAP) {
+   fdret = FLOW_DISSECT_RET_OUT_GOOD;
+   break;
+   }
+
proto = htons(ETH_P_IPV6);
 
key_control->flags |= FLOW_DIS_ENCAPSULATION;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index eb6345a027e1..aab13ba11767 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -329,7 +329,8 @@ static int fl_classify(struct sk_buff *skb, const struct 
tcf_proto *tp,
ARRAY_SIZE(fl_ct_info_to_flower_map),
post_ct);
skb_flow_dissect_hash(skb, >dissector, _key);
-   skb_flow_dissect(skb, >dissector, _key, 0);
+   skb_flow_dissect(skb, >dissector, _key,
+   

[ovs-dev] unknown OpenFlow message (version 4, type 18, stat 21)

2021-10-29 Thread kk Yoon
To add a wireless parameter request message to the openvswitch, we went
through the following process.

1. wireless parameter message definition
enum ofptype {
OFPTYPE_WPARAMS_REQUEST, /* OFPRAW_OFPST13_WPARAMS_REQUEST. */
OFPTYPE_WPARAMS_REPLY, /* OFPRAW_OFPST13_WPARAMS_REPLY. */
}

enum ofpraw {
/* OFPST 1.3+ (21): void. */
OFPRAW_OFPST13_WPARAMS_REQUEST,

/* OFPST 1.3+ (21): void. */
OFPRAW_OFPST13_WPARAMS_REPLY
}

2. Definition of processing function,
static enum ofperr
handle_wparams_request(struct ofconn* ofconn, const struct ofp_header* oh)
{
VLOG_WARN("handle_wparams_request() called\n");
struct ofpbuf* buf;

buf = ofpraw_alloc_reply(OFPRAW_OFPST13_WPARAMS_REPLY, oh, 0);
ofconn_send_reply(ofconn, buf);
return 0;
}

static enum ofperr
handle_single_part_openflow(struct ofconn *ofconn, const struct ofp_header
*oh,
enum ofptype type)
OVS_EXCLUDED(ofproto_mutex)
{
// VLOG_INFO("type : %d vs %d", type, OFPTYPE_GET_TXPOWER_REQUEST);

switch (type) {
case OFPTYPE_WPARAMS_REQUEST:
return handle_wparams_request(ofconn, oh);
}

but /var/log/openvswitch/ovs-vswitchd.log print
2021-09-10T08:18:32.850Z|18277|ofp_msgs|WARN|unknown OpenFlow message
(version 4, type 18, stat 21)
What's the problem?
Thank you.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] dpif-netdev: Sync PMD ALB state with user commands.

2021-10-29 Thread David Marchand
On Fri, Oct 8, 2021 at 12:19 PM Kevin Traynor  wrote:
>
> Previously, when a user enabled PMD auto load balancer with
> pmd-auto-lb="true", some conditions such as number of PMDs/RxQs
> that were required for a rebalance to take place were checked.
>
> If the configuration meant that a rebalance would not take place
> then PMD ALB was logged as 'disabled' and not run.
>
> Later, if the PMD/RxQ configuration changed whereby a rebalance
> could be effective, PMD ALB was logged as 'enabled' and would run at
> the appropriate time.
>
> This worked ok from a functional view but it is unintuitive for the
> user reading the logs.
>
> e.g. with one PMD (PMD ALB would not be effective)
>
> User enables ALB, but logs say it is disabled because it won't run.
> $ ovs-vsctl set open_vSwitch . other_config:pmd-auto-lb="true"
> |dpif_netdev|INFO|PMD auto load balance is disabled
>
> No dry run takes place.
>
> Add more PMDs (PMD ALB may be effective).
> $ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=50
> |dpif_netdev|INFO|PMD auto load balance is enabled ...
>
> Dry run takes place.
> |dpif_netdev|DBG|PMD auto load balance performing dry run.
>
> A better approach is to simply reflect back the user enable/disable
> state in the logs and deal with whether the rebalance will be effective
> when needed. That is the approach taken in this patch.
>
> To cut down on unneccessary work, some basic checks are also made before
> starting a PMD ALB dry run and debug logs can indicate this to the user.
>
> e.g. with one PMD (PMD ALB would not be effective)
>
> User enables ALB, and logs confirm the user has enabled it.
> $ ovs-vsctl set open_vSwitch . other_config:pmd-auto-lb="true"
> |dpif_netdev|INFO|PMD auto load balance is enabled...
>
> No dry run takes place.
> |dpif_netdev|DBG|PMD auto load balance nothing to do, not enough non-isolated 
> PMDs or RxQs.
>
> Add more PMDs (PMD ALB may be effective).
> $ ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=50
>
> Dry run takes place.
> |dpif_netdev|DBG|PMD auto load balance performing dry run.
>
> Signed-off-by: Kevin Traynor 
>

Reviewed-by: David Marchand 

Two nits below:

[snip]

> -if (pmd_alb->is_enabled != enable_alb || always_log) {
> -pmd_alb->is_enabled = enable_alb;
> +if (pmd_alb->is_enabled != state || always_log) {
> +pmd_alb->is_enabled = state;
>  if (pmd_alb->is_enabled) {
> +uint8_t rebalance_load_thresh;
> +
>  atomic_read_relaxed(_alb->rebalance_load_thresh,
>  _load_thresh);
> -VLOG_INFO("PMD auto load balance is enabled "
> +VLOG_INFO("PMD auto load balance is enabled. "

Nit: I'd either let this log as is, or otherwise put a ',' here, and
finish this log with a '.' after other infos.


>"interval %"PRIu64" mins, "
>"pmd load threshold %"PRIu8"%%, "

[snip]

> @@ -5482,4 +5453,62 @@ sched_numa_list_variance(struct sched_numa_list 
> *numa_list)
>  }
>
> +/*
> + * This function checks that some basic conditions needed for a rebalance to 
> be
> + * effective are met. Such as Rxq scheduling assignment type, more than one
> + * PMD, more than 2 Rxqs on a PMD. If there was no reconfiguration change
> + * since the last check, it reuses the last result.
> + *
> + * It is not intended to be an inclusive check of every condition that may 
> make
> + * a rebalance ineffective. It is done as a quick check so a full
> + * pmd_rebalance_dry_run() can be avoided when it is not needed.
> + */
> +static bool
> +pmd_reblance_dry_run_needed(struct dp_netdev *dp)
> +OVS_REQUIRES(dp->port_mutex)
> +{
> +struct dp_netdev_pmd_thread *pmd;
> +struct pmd_auto_lb *pmd_alb = >pmd_alb;
> +unsigned int cnt = 0;
> +bool multi_rxq = false;
> +
> +/* Check if there was no reconfiguration since last check. */
> +if (!pmd_alb->recheck_config) {
> +if (!pmd_alb->do_dry_run) {
> +VLOG_DBG("PMD auto load balance nothing to do, "
> +  "no configuration changes since last check.");

Nit: extra space as indent.


> +return false;
> +}
> +return true;
> +}


-- 
David Marchand

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


Re: [ovs-dev] [PATCH net] cls_flower: Fix inability to match GRE/IPIP packets

2021-10-29 Thread patchwork-bot+netdevbpf
Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller :

On Fri, 29 Oct 2021 09:21:41 + you wrote:
> When a packet of a new flow arrives in openvswitch kernel module, it dissects
> the packet and passes the extracted flow key to ovs-vswtichd daemon. If hw-
> offload configuration is enabled, the daemon creates a new TC flower entry to
> bypass openvswitch kernel module for the flow (TC flower can also offload 
> flows
> to NICs but this time that does not matter).
> 
> In this processing flow, I found the following issue in cases of GRE/IPIP
> packets.
> 
> [...]

Here is the summary with links:
  - [net] cls_flower: Fix inability to match GRE/IPIP packets
https://git.kernel.org/netdev/net/c/6de6e46d27ef

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


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


Re: [ovs-dev] [PATCH v2 ovn 2/3] CoPP: add self-test for bfd controller action

2021-10-29 Thread Mark Gray
On 21/10/2021 23:18, Lorenzo Bianconi wrote:
> Introduce CoPP selftest for bfd controller action
> 
> Signed-off-by: Lorenzo Bianconi 
> ---
>  tests/system-ovn.at | 28 
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index d003843c3..77c811946 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6743,6 +6743,34 @@ OVS_WAIT_UNTIL([
>  ])
>  kill $(pidof tcpdump)
>  
> +check ovn-nbctl meter-add bfd-meter drop 1 pktps 0
> +check ovn-nbctl --wait=hv lr-copp-add R1 bfd bfd-meter
> +AT_CHECK([ovn-nbctl lr-copp-list R1 |grep bfd], [0], [dnl
> +bfd: bfd-meter
> +])
> +
> +check ovn-nbctl --bfd lr-route-add R1 240.0.0.0/8 172.16.1.50 rp-public
> +printf "%08x" $(ovn-sbctl get bfd . disc) > /tmp/disc
> +NS_EXEC([server], [tcpdump -l -n -i s1 udp port 3784 -Q in > bfd.pcap &])
> +ip netns exec server scapy -H <<-EOF
> +import binascii
> +f = open("/tmp/disc", "r")
> +# scapy does not support BFD protocol
> +# let's hardcode a BFD payload with the proper my-disc field read from the db
> +bfd = binascii.unhexlify("20600518a899e77b" + f.readline().strip() + 
> "000f424f4240")
> +p = IP(src="172.16.1.50", dst="172.16.1.1") / UDP(dport = 3784, sport = 
> 49152) / Raw(load = bfd)
> +send (p, iface='s1', loop = 0, verbose = 0, count = 100)
> +f.close()
> +EOF
> +rm /tmp/disc
> +
> +# 1pps + 1 burst size
> +OVS_WAIT_UNTIL([
> +n_tcp_rst=$(grep Final bfd.pcap | wc -l)
> +test "${n_tcp_rst}" = "2"
> +])
> +kill $(pidof tcpdump)
> +
>  kill $(pidof ovn-controller)
>  
>  as ovn-sb
> 
Acked-by: Mark D. Gray 

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


Re: [ovs-dev] [PATCH v2 ovn 3/3] CoPP: add self-test for tcp-reset controller action

2021-10-29 Thread Mark Gray
On 21/10/2021 23:18, Lorenzo Bianconi wrote:
> Introduce CoPP selftest for tcp-reset controller action
> 
> Signed-off-by: Lorenzo Bianconi 
> ---
>  tests/ovn-northd.at | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 3ff0029f8..afc005156 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3272,6 +3272,17 @@ check ovn-nbctl --wait=hv lr-copp-del r0 icmp6-error
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  ])
>  
> +check ovn-nbctl --wait=hv lr-copp-add r0 tcp-reset meter2
> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> +tcp-reset: meter2
> +])
> +
> +AT_CHECK([ovn-sbctl list logical_flow | grep tcp -A 2 | grep -q meter2])
> +
> +check ovn-nbctl --wait=hv lr-copp-del r0 tcp-reset
> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> +])
> +
>  check ovn-nbctl --wait=hv ls-copp-del sw1 event-elb
>  AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
>  ])
> 
Acked-by: Mark D. Gray 

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


Re: [ovs-dev] [PATCH v2 ovn 1/3] CoPP: add self-test for icmp{4, 6}_error controller action

2021-10-29 Thread Mark Gray
On 21/10/2021 23:18, Lorenzo Bianconi wrote:
> Introduce CoPP selftest for icmp{4,6}_error controller action
> Remove sleep in CoPP test and rely on tcpdump "-l" option.
> 
> Signed-off-by: Lorenzo Bianconi 
> ---
>  tests/ovn-northd.at | 23 +++
>  tests/system-ovn.at | 43 ---
>  2 files changed, 51 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 544820764..3ff0029f8 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3249,6 +3249,29 @@ AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  
>  AT_CHECK([ovn-sbctl list logical_flow | grep arp -A 2 | grep -q meter1],[1])
>  
> +check ovn-nbctl --wait=hv meter-add meter2 drop 400 pktps 10
> +check ovn-nbctl --wait=hv lr-copp-add r0 icmp4-error meter2
> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> +icmp4-error: meter2
> +])
> +
> +AT_CHECK([ovn-sbctl list logical_flow | grep icmp4 -A 2 | grep -q meter2])
> +
> +check ovn-nbctl --wait=hv lr-copp-del r0 icmp4-error
> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> +])
> +
> +check ovn-nbctl --wait=hv lr-copp-add r0 icmp6-error meter2
> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> +icmp6-error: meter2
> +])
> +
> +AT_CHECK([ovn-sbctl list logical_flow | grep icmp6 -A 2 | grep -q meter2])
> +
> +check ovn-nbctl --wait=hv lr-copp-del r0 icmp6-error
> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> +])
> +
>  check ovn-nbctl --wait=hv ls-copp-del sw1 event-elb
>  AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
>  ])
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 345384223..d003843c3 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6669,7 +6669,7 @@ check ovn-nbctl lsp-add public public1 \
>  -- lsp-set-type public1 localnet \
>  -- lsp-set-options public1 network_name=phynet
>  
> -NS_EXEC([sw01], [tcpdump -n -i sw01 icmp -Q in > reject.pcap &])
> +NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
>  check ovn-nbctl meter-add acl-meter drop 1 pktps 0
>  check ovn-nbctl --wait=hv ls-copp-add sw0 reject acl-meter
>  check ovn-nbctl acl-add sw0 from-lport 1002 'inport == "sw01" && ip && udp' 
> reject
> @@ -6679,37 +6679,33 @@ reject: acl-meter
>  ])
>  
>  ip netns exec sw01 scapy -H <<-EOF
> -p = IP(src="192.168.1.2", dst="192.168.1.1")/ UDP(dport = 12345) / 
> Raw(b"X"*64)
> +p = IP(src="192.168.1.2", dst="192.168.1.1") / UDP(dport = 12345) / 
> Raw(b"X"*64)
>  send (p, iface='sw01', loop = 0, verbose = 0, count = 20)
>  EOF
>  
> -sleep 2
> -kill $(pidof tcpdump)
> -
>  # 1pps + 1 burst size
>  OVS_WAIT_UNTIL([
>  n_reject=$(grep unreachable reject.pcap | wc -l)
>  test "${n_reject}" = "2"
>  ])
> +kill $(pidof tcpdump)
>  
>  rm -f reject.pcap
> -NS_EXEC([sw01], [tcpdump -n -i sw01 icmp -Q in > reject.pcap &])
> +NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
>  check ovn-nbctl --wait=hv ls-copp-del sw0 reject
>  
>  ip netns exec sw01 scapy -H <<-EOF
> -p = IP(src="192.168.1.2", dst="192.168.1.1")/ UDP(dport = 12345) / 
> Raw(b"X"*64)
> +p = IP(src="192.168.1.2", dst="192.168.1.1") / UDP(dport = 12345) / 
> Raw(b"X"*64)
>  send (p, iface='sw01', loop = 0, verbose = 0, count = 20)
>  EOF
>  
> -sleep 2
> -kill $(pidof tcpdump)
> -
>  OVS_WAIT_UNTIL([
>  n_reject=$(grep unreachable reject.pcap | wc -l)
>  test "${n_reject}" = "20"
>  ])
> +kill $(pidof tcpdump)
>  
> -NS_EXEC([server], [tcpdump -n -i s1 arp[[24:4]]=0xac100164 > arp.pcap &])
> +NS_EXEC([server], [tcpdump -l -n -i s1 arp[[24:4]]=0xac100164 > arp.pcap &])
>  check ovn-nbctl meter-add arp-meter drop 1 pktps 0
>  check ovn-nbctl --wait=hv lr-copp-add R1 arp-resolve arp-meter
>  AT_CHECK([ovn-nbctl lr-copp-list R1], [0], [dnl
> @@ -6717,18 +6713,35 @@ arp-resolve: arp-meter
>  ])
>  
>  ip netns exec sw01 scapy -H <<-EOF
> -p = IP(src="192.168.1.2", dst="172.16.1.100")/ TCP(dport = 80, flags="S") / 
> Raw(b"X"*64)
> +p = IP(src="192.168.1.2", dst="172.16.1.100") / TCP(dport = 80, flags="S") / 
> Raw(b"X"*64)
>  send (p, iface='sw01', loop = 0, verbose = 0, count = 100)
>  EOF
>  
> -sleep 2
> -kill $(pidof tcpdump)
> -
>  # 1pps + 1 burst size
>  OVS_WAIT_UNTIL([
>  n_arp=$(grep ARP arp.pcap | wc -l)
>  test "${n_arp}" = "2"
>  ])
> +kill $(pidof tcpdump)
> +
> +check ovn-nbctl meter-add icmp-meter drop 1 pktps 0
> +check ovn-nbctl --wait=hv lr-copp-add R1 icmp4-error icmp-meter
> +AT_CHECK([ovn-nbctl lr-copp-list R1 |grep icmp4-error], [0], [dnl
> +icmp4-error: icmp-meter
> +])
> +
> +NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp > icmp.pcap &])
> +ip netns exec sw01 scapy -H <<-EOF
> +p = IP(src="192.168.1.2", dst="172.16.1.100", ttl=1) / TCP(dport = 8080, 
> flags="S") / Raw(b"X"*64)
> +send (p, iface='sw01', loop = 0, verbose = 0, count = 100)
> +EOF
> +
> +# 1pps + 1 burst size
> +OVS_WAIT_UNTIL([
> +n_icmp=$(grep ICMP icmp.pcap | wc -l)
> +test "${n_icmp}" = "2"
> +])
> +kill $(pidof