Re: [ovs-dev] [PATCH v6 3/3] userspace: Add Generic Segmentation Offloading.

[Ilya Maximets]

On 10/31/23 20:51, Mike Pattrick wrote:
> From: Flavio Leitner 
> 
> This provides a software implementation in the case
> the egress netdev doesn't support segmentation in hardware.
> 
> The challenge here is to guarantee packet ordering in the
> original batch that may be full of TSO packets. Each TSO
> packet can go up to ~64kB, so with segment size of 1440
> that means about 44 packets for each TSO. Each batch has
> 32 packets, so the total batch amounts to 1408 normal
> packets.
> 
> The segmentation estimates the total number of packets
> and then the total number of batches. Then allocate
> enough memory and finally do the work.
> 
> Finally each batch is sent in order to the netdev.
> 
> Signed-off-by: Flavio Leitner 
> Co-authored-by: Mike Pattrick 
> Signed-off-by: Mike Pattrick 
> 
> ---
> 
> v4:
>  - Various formatting changes
>  - Fixed memory leak in soft-gso code if packet is flagged
>for GSO but incorrectly lacks segment size.
> 
> v5:
>  - Corrected indentation and comments
>  - Fixed pointer arithmatic issue
>  - Added a test to exercise software gso code
> ---
>  lib/automake.mk |   2 +
>  lib/dp-packet-gso.c | 168 
>  lib/dp-packet-gso.h |  23 ++
>  lib/dp-packet.h |   7 ++
>  lib/netdev-dpdk.c   |  44 ---
>  lib/netdev.c| 139 +
>  lib/packets.c   |   4 +-
>  tests/system-traffic.at |  45 +++
>  8 files changed, 367 insertions(+), 65 deletions(-)
>  create mode 100644 lib/dp-packet-gso.c
>  create mode 100644 lib/dp-packet-gso.h
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 24b0ffefe..82dd85c5f 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
>   lib/dpctl.h \
>   lib/dp-packet.h \
>   lib/dp-packet.c \
> + lib/dp-packet-gso.c \
> + lib/dp-packet-gso.h \
>   lib/dpdk.h \
>   lib/dpif-netdev-extract-study.c \
>   lib/dpif-netdev-lookup.h \
> diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c
> new file mode 100644
> index 0..44dc8061e
> --- /dev/null
> +++ b/lib/dp-packet-gso.c
> @@ -0,0 +1,168 @@
> +/*
> + * Copyright (c) 2023 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 
> +#include 
> +
> +#include "dp-packet.h"
> +#include "dp-packet-gso.h"
> +#include "netdev-provider.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(dp_packet_gso);
> +
> +/* Retuns a new packet that is a segment of packet 'p'.
> + *
> + * The new packet is initialized with 'hdr_len' bytes from the
> + * start of packet 'p' and then appended with 'data_len' bytes
> + * from the 'data' buffer.
> + *
> + * Note: The packet headers are not updated. */
> +static struct dp_packet *
> +dp_packet_gso_seg_new(const struct dp_packet *p, size_t hdr_len,
> +  const char *data, size_t data_len)
> +{
> +struct dp_packet *seg = dp_packet_new_with_headroom(hdr_len + data_len,
> +
> dp_packet_headroom(p));
> +
> +/* Append the original packet headers and then the payload. */
> +dp_packet_put(seg, dp_packet_data(p), hdr_len);
> +dp_packet_put(seg, data, data_len);
> +
> +/* The new segment should have the same offsets. */
> +seg->l2_5_ofs = p->l2_5_ofs;
> +seg->l3_ofs = p->l3_ofs;
> +seg->l4_ofs = p->l4_ofs;
> +
> +/* The protocol headers remain the same, so preserve hash and mark. */
> +*dp_packet_rss_ptr(seg) = *dp_packet_rss_ptr(p);
> +*dp_packet_flow_mark_ptr(seg) = *dp_packet_flow_mark_ptr(p);
> +
> +/* The segment should inherit all the offloading flags from the
> + * original packet, except for the TCP segmentation, external
> + * buffer and indirect buffer flags. */
> +*dp_packet_ol_flags_ptr(seg) = *dp_packet_ol_flags_ptr(p)
> +& DP_PACKET_OL_SUPPORTED_MASK;
> +
> +dp_packet_hwol_reset_tcp_seg(seg);
> +
> +return seg;
> +}
> +
> +/* Returns the calculated number of TCP segments in packet 'p'. */
> +int
> +dp_packet_gso_nr_segs(struct dp_packet *p)
> +{
> +uint16_t segsz = dp_packet_get_tso_segsz(p);
> +const char *data_tail;
> +const char *data_pos;
> +
> +data_pos = dp_packet_get_tcp_payload(p);
> +data_tail = (char *) dp_packet_tail(p) - dp_packet_l2_pad_size(p);
> +
> +return 

Re: [ovs-dev] [PATCH v6 2/3] userspace: Respect tso/gso segment size.

[Ilya Maximets]

On 10/31/23 20:51, Mike Pattrick wrote:
> From: Flavio Leitner 
> 
> Currently OVS will calculate the segment size based on the
> MTU of the egress port. That usually happens to be correct
> when the ports share the same MTU, but that is not always true.
> 
> Therefore, if the segment size is provided, then use that and
> make sure the over sized packets are dropped.
> 
> Signed-off-by: Flavio Leitner 
> Co-authored-by: Mike Pattrick 
> Signed-off-by: Mike Pattrick 
> ---
>  lib/dp-packet.c|  3 ++
>  lib/dp-packet.h| 26 
>  lib/netdev-dpdk.c  | 12 +++-
>  lib/netdev-linux.c | 76 --
>  4 files changed, 94 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index ed004c3b9..920402369 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -34,6 +34,7 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, 
> enum dp_packet_source so
>  pkt_metadata_init(>md, 0);
>  dp_packet_reset_cutlen(b);
>  dp_packet_reset_offload(b);
> +dp_packet_set_tso_segsz(b, 0);
>  /* Initialize implementation-specific fields of dp_packet. */
>  dp_packet_init_specific(b);
>  /* By default assume the packet type to be Ethernet. */
> @@ -203,6 +204,8 @@ dp_packet_clone_with_headroom(const struct dp_packet 
> *buffer, size_t headroom)
>  *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer);
>  *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK;
>  
> +dp_packet_set_tso_segsz(new_buffer, dp_packet_get_tso_segsz(buffer));
> +
>  if (dp_packet_rss_valid(buffer)) {
>  dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer));
>  }
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 70ddf8aa4..6a924f3ff 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -126,6 +126,7 @@ struct dp_packet {
>  uint32_t ol_flags;  /* Offloading flags. */
>  uint32_t rss_hash;  /* Packet hash. */
>  uint32_t flow_mark; /* Packet flow mark. */
> +uint16_t tso_segsz; /* TCP TSO segment size. */
>  #endif
>  enum dp_packet_source source;  /* Source of memory allocated as 'base'. 
> */
>  
> @@ -166,6 +167,9 @@ static inline void dp_packet_set_size(struct dp_packet *, 
> uint32_t);
>  static inline uint16_t dp_packet_get_allocated(const struct dp_packet *);
>  static inline void dp_packet_set_allocated(struct dp_packet *, uint16_t);
>  
> +static inline uint16_t dp_packet_get_tso_segsz(const struct dp_packet *);
> +static inline void dp_packet_set_tso_segsz(struct dp_packet *, uint16_t);
> +
>  void *dp_packet_resize_l2(struct dp_packet *, int increment);
>  void *dp_packet_resize_l2_5(struct dp_packet *, int increment);
>  static inline void *dp_packet_eth(const struct dp_packet *);
> @@ -644,6 +648,17 @@ dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
>  b->mbuf.buf_len = s;
>  }
>  
> +static inline uint16_t
> +dp_packet_get_tso_segsz(const struct dp_packet *p)
> +{
> +return p->mbuf.tso_segsz;
> +}
> +
> +static inline void
> +dp_packet_set_tso_segsz(struct dp_packet *p, uint16_t s)
> +{
> +p->mbuf.tso_segsz = s;
> +}
>  #else /* DPDK_NETDEV */
>  
>  static inline void
> @@ -700,6 +715,17 @@ dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
>  b->allocated_ = s;
>  }
>  
> +static inline uint16_t
> +dp_packet_get_tso_segsz(const struct dp_packet *p)
> +{
> +return p->tso_segsz;
> +}
> +
> +static inline void
> +dp_packet_set_tso_segsz(struct dp_packet *p, uint16_t s)
> +{
> +p->tso_segsz = s;
> +}
>  #endif /* DPDK_NETDEV */
>  
>  static inline void
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 55700250d..9f20cc689 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2453,6 +2453,7 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, 
> struct rte_mbuf *mbuf)
>  
>  if (mbuf->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
>  struct tcp_header *th = dp_packet_l4(pkt);
> +int hdr_len;
>  
>  if (!th) {
>  VLOG_WARN_RL(, "%s: TCP Segmentation without L4 header"
> @@ -2462,7 +2463,15 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, 
> struct rte_mbuf *mbuf)
>  
>  mbuf->l4_len = TCP_OFFSET(th->tcp_ctl) * 4;
>  mbuf->ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
> +hdr_len = mbuf->l2_len + mbuf->l3_len + mbuf->l4_len;
>  mbuf->tso_segsz = dev->mtu - mbuf->l3_len - mbuf->l4_len;
> +if (OVS_UNLIKELY((hdr_len + mbuf->tso_segsz) > dev->max_packet_len)) 
> {
> +VLOG_WARN_RL(, "%s: Oversized TSO packet. "
> + "hdr: %"PRIu32", gso: %"PRIu32", max len: 
> %"PRIu32"",
> + dev->up.name, hdr_len, mbuf->tso_segsz,
> + dev->max_packet_len);
> +return false;
> +}
>  
>  if (mbuf->ol_flags & RTE_MBUF_F_TX_IPV4) {
>  mbuf->ol_flags |= 

Re: [ovs-dev] [PATCH v6 1/3] netdev-linux: Use ethtool to detect offload support.

[Ilya Maximets]

On 10/31/23 20:51, Mike Pattrick wrote:
> Currently when userspace-tso is enabled, netdev-linux interfaces will
> indicate support for all offload flags regardless of interface
> configuration. This patch checks for which offload features are enabled
> during netdev construction.
> 
> Signed-off-by: Mike Pattrick 
> 
> --
> 
> v6:
>  - Removed legacy comment
>  - Reverse xmas
> ---
>  lib/netdev-linux.c  | 146 ++--
>  tests/ofproto-macros.at |   1 +
>  2 files changed, 140 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index cca340879..a46f5127f 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -558,6 +558,7 @@ static bool netdev_linux_miimon_enabled(void);
>  static void netdev_linux_miimon_run(void);
>  static void netdev_linux_miimon_wait(void);
>  static int netdev_linux_get_mtu__(struct netdev_linux *netdev, int *mtup);
> +static void netdev_linux_set_ol(struct netdev *netdev);
> 
>  static bool
>  is_tap_netdev(const struct netdev *netdev)
> @@ -959,14 +960,8 @@ netdev_linux_construct(struct netdev *netdev_)
>  return error;
>  }
> 
> -/* The socket interface doesn't offer the option to enable only
> - * csum offloading without TSO. */

This comment seems to be still sort of valid.  i.e. we're only calling the
netdev_linux_set_ol() when TSO is enabled, because if we call it just for
the checksum, we'll get TSO flags as well.  Maybe just re-phrase it instead
of removing?  Or maybe even keep as is?

>  if (userspace_tso_enabled()) {
> -netdev_->ol_flags |= NETDEV_TX_OFFLOAD_TCP_TSO;
> -netdev_->ol_flags |= NETDEV_TX_OFFLOAD_TCP_CKSUM;
> -netdev_->ol_flags |= NETDEV_TX_OFFLOAD_UDP_CKSUM;
> -netdev_->ol_flags |= NETDEV_TX_OFFLOAD_SCTP_CKSUM;
> -netdev_->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
> +netdev_linux_set_ol(netdev_);
>  }
> 
>  error = get_flags(>up, >ifi_flags);
> @@ -2381,6 +2376,143 @@ netdev_internal_get_stats(const struct netdev 
> *netdev_,
>  return error;
>  }
> 
> +static int
> +netdev_linux_read_stringset_info(struct netdev_linux *netdev, uint32_t *len)
> +{
> +union {
> +struct ethtool_sset_info hdr;
> +struct {
> +uint64_t pad[2];
> +uint32_t sset_len[1];
> +};
> +} sset_info;
> +int error;
> +
> +sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
> +sset_info.hdr.reserved = 0;
> +sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES;
> +
> +error = netdev_linux_do_ethtool(netdev->up.name,

netdev_get_name(>up)

> +(struct ethtool_cmd *)_info,

'''
Put a space between the ``()`` used in a cast and the expression
whose type is cast: ``(void *) 0``.
'''

> +ETHTOOL_GSSET_INFO, "ETHTOOL_GSSET_INFO");

Please, indent to the level of '('.

> +if (error) {
> +return error;
> +}
> +*len = sset_info.sset_len[0];

According to the ethtool API, the sset_mask is also an output
parameter.  We should probably check that our ETH_SS_FEATURES
flag is set there.  If not, the access above may be invalid.

> +return 0;
> +}
> +
> +
> +static int
> +netdev_linux_read_definitions(struct netdev_linux *netdev,
> +  struct ethtool_gstrings **pstrings)
> +{
> +struct ethtool_gstrings *strings = NULL;
> +uint32_t len = 0;
> +int error = 0;
> +
> +error = netdev_linux_read_stringset_info(netdev, );
> +if (error || !len) {
> +return error;
> +}
> +strings = xcalloc(1, sizeof(*strings) + len * ETH_GSTRING_LEN);

This should be just xzalloc.
And don't parenthesize the argument of sizeof.

> +if (!strings) {
> +return ENOMEM;

xcalloc can't fail, hence the 'x'.

> +}
> +
> +strings->cmd = ETHTOOL_GSTRINGS;
> +strings->string_set = ETH_SS_FEATURES;
> +strings->len = len;
> +error = netdev_linux_do_ethtool(netdev->up.name,
> +(struct ethtool_cmd *) strings,
> +ETHTOOL_GSTRINGS, "ETHTOOL_GSTRINGS");

Indentation.

> +if (error) {
> +goto out;
> +}
> +
> +for (int i = 0; i < len; i++) {
> +strings->data[(i + 1) * ETH_GSTRING_LEN - 1] = 0;
> +}
> +
> +*pstrings = strings;
> +
> +return 0;
> +out:
> +*pstrings = NULL;
> +free(strings);
> +return error;
> +}
> +
> +static void
> +netdev_linux_set_ol(struct netdev *netdev_)
> +{
> +struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> +struct ethtool_gfeatures *features = NULL;
> +struct ethtool_gstrings *names = NULL;
> +int error;
> +
> +COVERAGE_INC(netdev_get_ethtool);
> +
> +error = netdev_linux_read_definitions(netdev, );
> +if (error) {
> +return;
> +}
> +
> +features = xmalloc(sizeof *features +
> +   DIV_ROUND_UP(names->len, 32) *
> +   sizeof features->features[0]);
> +if (!features) {
> +goto out;

Re: [ovs-dev] [PATCH] python: idl: Handle monitor_canceled

[0-day Robot]

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

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


checkpatch:
WARNING: The subject summary should end with a dot.
Subject: python: idl: Handle monitor_canceled
Lines checked: 35, Warnings: 1, Errors: 0


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

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

Re: [ovs-dev] [PATCH] python: idl: Handle monitor_canceled

[Dumitru Ceara]

On 11/16/23 23:16, Terry Wilson wrote:
> Currently python-ovs claims to be "db change aware" but does not
> parse the "monitor_canceled" notification. Transactions can continue
> being made, but the monitor updates will not be sent. Adding a
> force_reconnect() upon receiving a "monitor_canceled" notification
> resolves this issue.
> 
> Signed-off-by: Terry Wilson 
> ---

Hi Terry,

Thanks for the patch.

>  python/ovs/db/idl.py | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 16ece0334..cfd81a1ec 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -481,6 +481,10 @@ class Idl(object):
>  break
>  else:
>  self.__parse_update(msg.params[1], OVSDB_UPDATE)
> +elif (msg.type == ovs.jsonrpc.Message.T_NOTIFY
> +and msg.method == "monitor_canceled"):
> +self.force_reconnect()
> +break

I didn't test this at all, I was just looking at the code, so I might be
completely wrong but can't we avoid a full reconnect and instead just
restart the fsm (self.restart_fsm())?  The C version of the IDL/CS layer
does something along those lines (it also cancels the other monitor):

https://github.com/openvswitch/ovs/blob/7b514aba0e91c535024508624724a83a3df87b71/lib/ovsdb-cs.c#L1661-L1678

Also, force_reconnect() messes with the backoff a bit; I'm not
completely sure we want that on monitor_cancel.

>  elif (msg.type == ovs.jsonrpc.Message.T_REPLY
>and self._monitor_request_id is not None
>and self._monitor_request_id == msg.id):

Regards,
Dumitru

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

[ovs-dev] [PATCH] python: idl: Handle monitor_canceled

[Terry Wilson]

Currently python-ovs claims to be "db change aware" but does not
parse the "monitor_canceled" notification. Transactions can continue
being made, but the monitor updates will not be sent. Adding a
force_reconnect() upon receiving a "monitor_canceled" notification
resolves this issue.

Signed-off-by: Terry Wilson 
---
 python/ovs/db/idl.py | 4 
 1 file changed, 4 insertions(+)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 16ece0334..cfd81a1ec 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -481,6 +481,10 @@ class Idl(object):
 break
 else:
 self.__parse_update(msg.params[1], OVSDB_UPDATE)
+elif (msg.type == ovs.jsonrpc.Message.T_NOTIFY
+and msg.method == "monitor_canceled"):
+self.force_reconnect()
+break
 elif (msg.type == ovs.jsonrpc.Message.T_REPLY
   and self._monitor_request_id is not None
   and self._monitor_request_id == msg.id):
-- 
2.34.3

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

Re: [ovs-dev] [PATCH v3 1/1] netdev-offload-dpdk: Fix GRE without a key match.

[Ilya Maximets]

On 10/31/23 16:14, Salem Sol via dev wrote:
> In case there is no match on GRE key, avoid adding the key match item.
> 
> Fixes: 7617d0583c73 ("netdev-offload-dpdk: Add support for matching on gre 
> fields.")
> Signed-off-by: Salem Sol 
> ---
>  lib/netdev-offload-dpdk.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 992627fa2..b2bb9013e 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -1336,16 +1336,19 @@ parse_gre_match(struct flow_patterns *patterns,
>  greh_spec->k = !!(match->flow.tunnel.flags & FLOW_TNL_F_KEY);
>  greh_mask->k = 1;
>  
> -key_spec = xzalloc(sizeof *key_spec);
> -key_mask = xzalloc(sizeof *key_mask);
> +if (greh_spec->k) {
> +key_spec = xzalloc(sizeof *key_spec);
> +key_mask = xzalloc(sizeof *key_mask);
>  
> -*key_spec = htonl(ntohll(match->flow.tunnel.tun_id));
> -*key_mask = htonl(ntohll(match->wc.masks.tunnel.tun_id));
> +*key_spec = htonl(ntohll(match->flow.tunnel.tun_id));
> +*key_mask = htonl(ntohll(match->wc.masks.tunnel.tun_id));
> +
> +add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_GRE_KEY, key_spec,
> + key_mask, NULL);
> +}
>  
>  consumed_masks->tunnel.tun_id = 0;

We do not always consume tun_id now.  Above line should likely be moved
under the if condition.

>  consumed_masks->tunnel.flags &= ~FLOW_TNL_F_KEY;
> -add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_GRE_KEY, key_spec,
> - key_mask, NULL);
>  }
>  
>  consumed_masks->tunnel.flags &= ~FLOW_TNL_F_DONT_FRAGMENT;

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

Re: [ovs-dev] [PATCH] lib/conntrack.c:compatible with nat with no action(direction)

[Aaron Conole]

Eelco Chaudron  writes:

> On 16 Nov 2023, at 11:07, Joseph Zhong wrote:
>
>> This patch is to avoid generating incorrect conntrack entry
>> In a certain use case of conntrack flow that if flow included
>> ct(commit, nat) action, but no detail action/direction specified,
>> CT will generate incorrect conntrack entry.
>> For example, add below flow:
>> ip,priority=500,in_port=1,ct_state=-trk actions=ct(table=1,nat)'
>> ip,priority=500,in_port=2,ct_state=-trk actions=ct(table=1,nat)'
>> table=1,in_port=1,ip,ct_state=+trk+new actions=ct*(commit,nat)*,2
>> table=1,in_port=1,ip,ct_state=-new+trk+est actions=2
>> table=1,in_port=2,ip,ct_state=-new+trk+est actions=1
>> start traffic from 192.168.2.2 to 192.168.2.7
>> ovs dpdk datpath generate CT entry as below:
>> icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
>> reply=(src=*0.0.0.0*,dst=192.168.2.2,id=17038,type=0,code=0)
>> reply key src 0.0.0.0 is generated not correct by "nat_get_unique_tuple".
>> but ovs kernel datapath will generate correct ct entry as below:
>> icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
>> reply=(src=192.168.2.7,dst=192.168.2.2,id=17038,type=0,code=0)
>>
>> To compatible with this use case of flow, and also be consistent with
>> kernel datapath's behavior, this patch treat this nat without action
>> specified as not nat, and don't do "nat_get_unique_tuple" and malloc
>> a nat_conn that is attached to nc.
>
> Hi Joseph,
>
> Thanks for the patch, I’m not a conntrack expert so I’ll let Aaron, or
> Paolo review it. But would it be possible to have a test case for
> this?

It should for sure be possible to have a test case for this.

> Cheers,
>
> Eelco
>
>
>> Signed-off-by: Zhong Zhong 
>>
>> ---
>>  lib/conntrack.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 47a443f..581b62b 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -942,7 +942,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
>> *pkt,

Something happened with your patch here and that caused issues with the
robot.  Please fix.

>>  nc->parent_key = alg_exp->parent_key;
>>  }
>> - if (nat_action_info) {
>> + if (nat_action_info && nat_action_info->nat_action) {
>>  nc->nat_action = nat_action_info->nat_action;
>>  if (alg_exp) {
>> --

I think this will break expectations case with NAT.

We need a more comprehensive solution here, because there are cases to
legitimately flow into a ct(commit,nat) pipeline with a new connection
and expect that things should "just work."

Note that the kernel side has other considerations as well - because the
NAT table is shared with other subsystems.  That said, I think the
expectation case is important and should be retained, so to get a more
consistent behavior, we probably need to have multiple checks for the
nat_action value.

And that showcases a deficiency in the design of the userspace NAT as
well, because it assumes:

  1) either SRC or DST but not both
  2) SRC or DST is set when calling these routines.


>> -- 
>> Best Regards
>>
>> Zhong, Zhong
>> Email: zhongzh...@gmail.com
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Re: [ovs-dev] [OVN RFC 0/7] OVN IC bugfixes & proposals/questions

[Terry Wilson]

On Tue, Jan 24, 2023 at 8:00 AM Ilya Maximets  wrote:
>
> On 1/24/23 14:12, Vladislav Odintsov wrote:
> > Hi Ilya,
> >
> > could you please take a look on this?
> > Maybe you can advice any direction how to investigate this issue?
> >
> > Thanks in advance.
> >
> > Regards,
> > Vladislav Odintsov
> >
> >> On 24 Nov 2022, at 21:10, Anton Vazhnetsov  wrote:
> >>
> >> Hi, Terry!
> >>
> >> In continuation to our yesterday’s conversation [0], we were able to 
> >> reproduce the issue with KeyError. We found that the problem is not 
> >> connected with ovsdb-server load but it appears when the ovsdb-server 
> >> schema is converted online (it even doesn’t matter whether the real ovs 
> >> schema is changed) while the active connection persists.
> >> Please use next commands to reproduce it:
> >>
> >> # in terminal 1
> >>
> >> ovsdb-tool create ./ovs.db /usr/share/ovn/ovn-nb.ovsschema
> >> ovsdb-server --remote punix://$(pwd)/ovs.sock  
> >> $(pwd)/ovs.db -vconsole:dbg
> >>
> >>
> >> # in terminal 2. run python shell
> >> python3
> >> # setup connection
> >> import ovsdbapp.schema.ovn_northbound.impl_idl as nb_idl
> >> from ovsdbapp.backend.ovs_idl import connection
> >>
> >> remote = "unix:///"
> >>
> >> def get_idl():
> >>"""Connection getter."""
> >>
> >>idl = connection.OvsdbIdl.from_server(remote, "OVN_Northbound",
> >>  leader_only=False)
> >>return nb_idl.OvnNbApiIdlImpl(connection.Connection(idl, 100))
> >>
> >> idl = get_idl()
> >>
> >>
> >> # in terminal 1
> >> ovsdb-client convert unix:$(pwd)/ovs.sock /usr/share/ovn/ovn-nb.ovsschema
> >>
> >> # in terminal 2 python shell:
> >> idl.ls_add("test").execute()
> >>
> >>
> >> We get following traceback:
> >>
> >> Traceback (most recent call last):
> >>  File 
> >> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/connection.py", 
> >> line 131, in run
> >>txn.results.put(txn.do_commit())
> >>  File 
> >> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/transaction.py",
> >>  line 143, in do_commit
> >>self.post_commit(txn)
> >>  File 
> >> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/transaction.py",
> >>  line 73, in post_commit
> >>command.post_commit(txn)
> >>  File 
> >> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/command.py", 
> >> line 94, in post_commit
> >>row = self.api.tables[self.table_name].rows[real_uuid]
> >>  File "/usr/lib64/python3.6/collections/__init__.py", line 991, in 
> >> __getitem__
> >>raise KeyError(key)
> >> KeyError: UUID('0256afa4-6dd0-4c2c-b6a2-686a360ab331')
> >>
> >> In ovsdb-server debug logs we see that update2 or update3 messages are not 
> >> sent from server in response to client’s transaction, just reply with 
> >> result UUID:
> >> 2022-11-24T17:42:36Z|00306|poll_loop|DBG|wakeup due to [POLLIN] on fd 18 
> >> (///root/ovsdb-problem/ovs.sock<->) at lib/stream-fd.c:157
> >> 2022-11-24T17:42:36Z|00307|jsonrpc|DBG|unix#5: received request, 
> >> method="transact", 
> >> params=["OVN_Northbound",{"uuid-name":"row03ef28d6_93f1_43bc_b07a_eae58d4bd1c5","table":"Logical_Switch","op":"insert","row":{"name”:"test"}}],
> >>  id=5
> >> 2022-11-24T17:42:36Z|00308|jsonrpc|DBG|unix#5: send reply, 
> >> result=[{"uuid":["uuid","4eb7c407-beec-46ca-b816-19f942e57721"]}], id=5
> >>
> >> We checked same with ovn-nbctl running in daemon mode and found that the 
> >> problem is not reproduced (ovsdb-server after database conversion sends 
> >> out update3 message to ovn-nbctl daemon process in response to 
> >> transaction, for example ovs-appctl -t  run ls-add 
> >> test-ls):
>
> If the update3 is not sent, it means that the client doesn't monitor
> this table.  You need to look at monitor requests sent and replied
> to figure out the difference.
>
> Since the database conversion is involved, server will close all
> monitors on schema update and notify all clients that are db_change_aware.
> Clients should re-send monitor requests at this point.  If clients
> are not db_change_aware, server will just disconnect them, so they
> can re-connect and see the new schema and send new monitor requests.
>
> On a quick glance I don't see python idl handling 'monitor_canceled'
> notification.  That is most likely a root cause.  Python IDL claims
> to be db_change_aware, but it doesn't seem to be.
>

Reviving a very old thread, but yes this is definitely the issue. I
just tested and see that we get the monitor_canceled and do not do
anything with it. Parsing it and calling self.force_reconnect() seems
to fix the issue. I can send a patch shortly.

Terry


> Best regards, Ilya Maximets.
>
> >> 2022-11-24T17:54:51Z|00623|jsonrpc|DBG|unix#7: received request, 
> >> method="transact", 
> >> params=["OVN_Northbound",{"uuid-name":"rowcdb152ce_a9af_4761_b965_708ad300fcb7","table":"Logical_Switch","op":"insert","row":{"name":"test-ls"}},{"comment":"ovn-nbctl:
> >>  run ls-add test-ls","op":"comment"}], id=5
> >> 

Re: [ovs-dev] [PATCH ovn v2 04/18] northd: Move router ports SB PB options sync to sync_to_sb_pb node.

[Numan Siddique]

On Wed, Nov 15, 2023 at 1:24 AM Han Zhou  wrote:
>
> On Tue, Nov 14, 2023 at 9:40 PM Han Zhou  wrote:
> >
> >
> >
> > On Thu, Oct 26, 2023 at 11:15 AM  wrote:
> > >
> > > From: Numan Siddique 
> > >
> > > It also moves the logical router port IPv6 prefix delegation
> > > updates to "sync-from-sb" engine node.
> > >
> > > Signed-off-by: Numan Siddique 
> > > ---
> > >  northd/en-northd.c  |   2 +-
> > >  northd/en-sync-sb.c |   3 +-
> > >  northd/northd.c | 283 ++--
> > >  northd/northd.h |   6 +-
> > >  tests/ovn-northd.at |  31 -
> > >  5 files changed, 198 insertions(+), 127 deletions(-)
> > >
> > > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > > index 96c2ce9f69..13e731cad9 100644
> > > --- a/northd/en-northd.c
> > > +++ b/northd/en-northd.c
> > > @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node
> *node,
> > >  northd_get_input_data(node, _data);
> > >
> > >  if (!northd_handle_sb_port_binding_changes(
> > > -input_data.sbrec_port_binding_table, >ls_ports)) {
> > > +input_data.sbrec_port_binding_table, >ls_ports,
> >lr_ports)) {
> > >  return false;
> > >  }
> > >
> > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> > > index 2540fcfb97..a14c609acd 100644
> > > --- a/northd/en-sync-sb.c
> > > +++ b/northd/en-sync-sb.c
> > > @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void
> *data OVS_UNUSED)
> > >  const struct engine_context *eng_ctx = engine_get_context();
> > >  struct northd_data *northd_data = engine_get_input_data("northd",
> node);
> > >
> > > -sync_pbs(eng_ctx->ovnsb_idl_txn, _data->ls_ports);
> > > +sync_pbs(eng_ctx->ovnsb_idl_txn, _data->ls_ports,
> > > + _data->lr_ports);
> > >  engine_set_node_state(node, EN_UPDATED);
> > >  }
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index 9ce1b2cb5a..c9c7045755 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> > >  {
> > >  sbrec_port_binding_set_datapath(op->sb, op->od->sb);
> > >  if (op->nbrp) {
> > > +/* Note: SB port binding options for router ports are set in
> > > + * sync_pbs(). */
> > > +
> > >  /* If the router is for l3 gateway, it resides on a chassis
> > >   * and its port type is "l3gateway". */
> > >  const char *chassis_name = smap_get(>od->nbr->options,
> "chassis");
> > > @@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> > >  sbrec_port_binding_set_type(op->sb, "patch");
> > >  }
> > >
> > > -struct smap new;
> > > -smap_init();
> > >  if (is_cr_port(op)) {
> > >  ovs_assert(sbrec_chassis_by_name);
> > >  ovs_assert(sbrec_chassis_by_hostname);
> > >  ovs_assert(sbrec_ha_chassis_grp_by_name);
> > >  ovs_assert(active_ha_chassis_grps);
> > > -const char *redirect_type = smap_get(>nbrp->options,
> > > - "redirect-type");
> > >
> > >  if (op->nbrp->ha_chassis_group) {
> > >  if (op->nbrp->n_gateway_chassis) {
> > > @@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> > >  /* Delete the legacy gateway_chassis from the pb. */
> > >  sbrec_port_binding_set_gateway_chassis(op->sb, NULL,
> 0);
> > >  }
> > > -smap_add(, "distributed-port", op->nbrp->name);
> > > -
> > > -bool always_redirect =
> > > -!op->od->has_distributed_nat &&
> > > -!l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> > > -
> > > -if (redirect_type) {
> > > -smap_add(, "redirect-type", redirect_type);
> > > -/* XXX Why can't we enable always-redirect when
> redirect-type
> > > - * is bridged? */
> > > -if (!strcmp(redirect_type, "bridged")) {
> > > -always_redirect = false;
> > > -}
> > > -}
> > > -
> > > -if (always_redirect) {
> > > -smap_add(, "always-redirect", "true");
> > > -}
> > > -} else {
> > > -if (op->peer) {
> > > -smap_add(, "peer", op->peer->key);
> > > -if (op->nbrp->ha_chassis_group ||
> > > -op->nbrp->n_gateway_chassis) {
> > > -char *redirect_name =
> > > -ovn_chassis_redirect_name(op->nbrp->name);
> > > -smap_add(, "chassis-redirect-port",
> redirect_name);
> > > -free(redirect_name);
> > > -}
> > > -}
> > > -if (chassis_name) {
> > > -smap_add(, "l3gateway-chassis", chassis_name);
> > > -

Re: [ovs-dev] [PATCH ovn v2 00/18] northd lflow incremental processing

[Numan Siddique]

On Thu, Nov 16, 2023 at 2:54 PM Han Zhou  wrote:
>
> On Wed, Nov 15, 2023 at 7:32 PM Numan Siddique  wrote:
> >
> > On Wed, Nov 15, 2023 at 2:59 AM Han Zhou  wrote:
> > >
> > > On Thu, Oct 26, 2023 at 11:12 AM  wrote:
> > > >
> > > > From: Numan Siddique 
> > > >
> > > > This patch series adds incremental processing in the lflow engine
> > > > node to handle changes to northd and other engine nodes.
> > > > Changed related to load balancers and NAT are mainly handled in
> > > > this patch series.
> > > >
> > > > This patch series can also be found here -
> > > https://github.com/numansiddique/ovn/tree/northd_lflow_ip/v1
> > >
> > > Thanks Numan for the great improvement!
> >
> > Hi Han,
> >
> > Thanks for the review comments.  I understand it is hard to review
> > somany patches, especially related to I-P.
> > I appreciate the time spent on it and  the feedback.
> >
> > >
> > > I spent some time these days to review the series and now at patch 10. I
> > > need to take a break and so I just sent the comments I had so far.
> > >
> > > I also did scale testing for northd with
> > > https://github.com/hzhou8/ovn-test-script for a 500 chassis, 50 lsp /
> > > chassis setup, and noticed that the recompute latency has increased 20%
> > > after the series. I think in general it is expected to have some
> > > performance penalty for the recompute case because of the new indexes
> > > created for I-P. However, the patch 10 "northd: Refactor lflow
> management
> > > into a separate module." alone introduces a 10% latency increase, which
> > > necessitates more investigation.
> >
> > Before sending out the series I  did some testing on recomputes with a
> > large OVN NB DB and SB DB
> > (from a 500 node ovn-heater density heavy run).
> > I'm aware of the increase in recomputes.  And I did some more testing
> > today as well.
> >
> > In my testing,  I can see that the increase in latency is due to the
> > new engine nodes added (lr_lbnat mainly)
> > and due to housekeeping of the lflow references.  I do not see any
> > increase due to the new lflow-mgr.c added in patch 10.
> >
> > I compared patch 9 and patch 10 separately and there is no difference
> > in lflow engine node recompute time between patch 9 and 10.
> >
>
> My results were different. My test profile simulates the ovn-k8s topology
> (central mode, not IC) with 500 nodes, 50 LSP/node, with no LB, small
> amount of ACLs and PGs.
> (
> https://github.com/hzhou8/ovn-test-script/blob/main/args.500ch_50lsp_1pg
> )
>
> The test results for ovn-northd recompute time are:
> main: 1118.3
> p9: 1129.5
> p10: 1243.4 ==> 10% more than p9
> p18: 1357.6
>
> I am not sure if the p10 increase is related to the hash change or
> something else.
>
> > Below are the results with the time taken for the mentioned engine
> > nodes in msecs for a recompute for some of the individual patches in
> > the series.
> >
> >
> > The sample OVN DBs have
> >
> > 
> > Resource  Total
> > ---
> > Logical switches1001
> > 
> > Logical routers  501
> > 
> > Router ports 1501
> > 
> > Switch ports 29501
> > 
> > Load balancers35253
> > 
> > Load balancer group 1
> > 
> > SB Logical flows268349
> > 
> > SB DB groups  509
> > 
> >
> > There is one load balancer group which has all the load balancers and
> > it is associated with all the logical switches and routers.
> >
> > Below is the time taken for each engine node in msec
> >
> > -
> > Engine nodes | lb_data | northd  | lr_lbnat  | lflow  |
> > -
> > ovn-northd-main  | 358  | 2455| x | 2082 |
> > -
> > ovn-northd-p1| 373   | 2476| x | 2170   |
> > -
> > ovn-northd-p5| 367   | 2413| x | 2195   |
> > -
> > ovn-northd-p6| 354   | 688 | 1815  | 2442|
> > -
> > ovn-northd-p7| 357   | 683 | 1778  | 2806|
> > -
> > ovn-northd-p9| 352   | 682 | 1781  | 2781|
> > -
> > ovn-northd-p10   | 365  | 838 | 1707  | 2812|
> > -
> > ovn-northd-p13   | 362  | 1066| 1882  | 

Re: [ovs-dev] [PATCH ovn v3 0/2] Stabilize CI by pinning container, runner and Python versions.

[Mark Michelson]

On 11/16/23 12:35, Ales Musil wrote:



On Thu, Nov 16, 2023 at 6:26 PM Dumitru Ceara > wrote:


All of the above were changed to track the latest available releases.
Initially that seemed like a good idea but in practice, a new
release would
potentially (silently) cause CI runs that used to pass on given stable
versions to unexpectedly start failing.

To address that this series pins all versions we can control allowing us
to use different values for different branches [0].

NOTE: this series of patches will look slightly different on each stable
branch because we need to bump to different OVS stable branch tips and
we pin to different container and python versions on different stable
branches.

NOTE2: this patch DOES NOT change the current behavior of always using
the CI container image we built from the main branch contents.  As
discussed in the v2 of this series [1], that has issues so we need to
follow up in the future.  Until then we just unblock CI.

[0]
https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.09

https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.06

https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.03

https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.12

https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.09

https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.06

https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.03


[1]
https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409482.html 


Changes in v3:
- dropped the patch that builds per-branch CI container images.
Changes in v2:
- added first patch to bump OVS submodule
- addressed Ales' review comments:
   - moved the logic to determine which image to use out of ci.sh;
     it's now in the workflow itself.
   - moved setting of OVN_IMAGE in the same block with all related
     env vars
- added a note for maintainers in the release process documentation
   to mention that the new workflow will likely have to be triggered
   manually (at least once) when branching for a new release.  GitHub
   doesn't allow periodic jobs on non-default branches.

Dumitru Ceara (2):
       ovs: Bump submodule to include E721 fixes.
       ci: Pin Python, Fedora and Ubuntu runner versions.


  .github/workflows/containers.yml               | 2 +-
  .github/workflows/ovn-fake-multinode-tests.yml | 6 +++---
  .github/workflows/ovn-kubernetes.yml           | 4 ++--
  .github/workflows/test.yml                     | 6 +++---
  utilities/containers/fedora/Dockerfile         | 2 +-
  5 files changed, 10 insertions(+), 10 deletions(-)


The series looks good to me, thanks.
I have only one question on 2/2.

Acked-by: Ales Musil mailto:amu...@redhat.com>>

Acked-by: Mark Michelson 




--

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com 







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

Re: [ovs-dev] [PATCH v6 1/1] ofproto-dpif-trace: Improve conjunctive match tracing.

[Ilya Maximets]

On 11/15/23 10:47, Nobuhiro MIKI wrote:
> A conjunctive flow consists of two or more multiple flows with
> conjunction actions. When input to the ofproto/trace command
> matches a conjunctive flow, it outputs flows of all dimensions.
> 
> Acked-by: Simon Horman 
> Signed-off-by: Nobuhiro MIKI 
> ---
> v6:
> * Support multiple tables with resubmit action
> v5:
> * Support tunnel metadata
> v4:
> * Fix conj_id matching
> * Fix priority matching
> * Add a new test
> v3:
> * Remove struct flow changes.
> * Use struct 'cls_rule' instead of struct 'flow'.
> * Add priority and id conditionals for 'soft' arrays.
> * Use 'minimask' in struct 'cls_rule' as mask.
> * Use hmapx instead of ovs_list to store conj flows.
> * Passe 'conj_flows' as an argument only when tracing.
> v2:
> * Reimplemented v1 with a safer and cleaner approach,
>   since v1 was a messy implementation that rewrote const variables.
> ---
>  lib/classifier.c | 51 ---
>  lib/classifier.h |  4 +-
>  lib/ovs-router.c |  5 +-
>  lib/tnl-ports.c  |  6 +--
>  ofproto/ofproto-dpif-xlate.c | 67 +---
>  ofproto/ofproto-dpif.c   | 25 ++---
>  ofproto/ofproto-dpif.h   |  3 +-
>  tests/classifier.at  | 99 
>  tests/test-classifier.c  |  8 +--
>  9 files changed, 238 insertions(+), 30 deletions(-)

Thanks for the update and the added tests!

This version looks good to me and works fine in all the tested cases.
Applied.

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

Re: [ovs-dev] [PATCH v5] ovs-ofctl: Implement compose-packet --bare [--bad-csum].

[Ilya Maximets]

On 11/15/23 17:42, Simon Horman wrote:
> On Tue, Nov 14, 2023 at 05:59:37PM +, Ihar Hrachyshka wrote:
>> With --bare, it will produce a bare hexified payload with no spaces or
>> offset indicators inserted, which is useful in tests to produce frames
>> to pass to e.g. `ovs-ofctl receive`.
>>
>> With --bad-csum, it will produce a frame that has an invalid IP checksum
>> (applicable to IPv4 only because IPv6 doesn't have checksums.)
>>
>> The command is now more useful in tests, where we may need to produce
>> hex frame payloads to compare observed frames against.
>>
>> As an example of the tool use, a single test case is converted to it.
>> The test uses both normal --bare and --bad-csum behaviors of the
>> command, confirming they work as advertised.
>>
>> Signed-off-by: Ihar Hrachyshka 
> 
> Thanks Ihar,
> 
> I have checked and as far as I can tell this addresses
> the concerns raised by Ilya for v5.
> 
> Acked-by: Simon Horman 

Thanks, Ihar and Simon!  Applied.

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

Re: [ovs-dev] [PATCH ovn v2 00/18] northd lflow incremental processing

[Han Zhou]

On Wed, Nov 15, 2023 at 7:32 PM Numan Siddique  wrote:
>
> On Wed, Nov 15, 2023 at 2:59 AM Han Zhou  wrote:
> >
> > On Thu, Oct 26, 2023 at 11:12 AM  wrote:
> > >
> > > From: Numan Siddique 
> > >
> > > This patch series adds incremental processing in the lflow engine
> > > node to handle changes to northd and other engine nodes.
> > > Changed related to load balancers and NAT are mainly handled in
> > > this patch series.
> > >
> > > This patch series can also be found here -
> > https://github.com/numansiddique/ovn/tree/northd_lflow_ip/v1
> >
> > Thanks Numan for the great improvement!
>
> Hi Han,
>
> Thanks for the review comments.  I understand it is hard to review
> somany patches, especially related to I-P.
> I appreciate the time spent on it and  the feedback.
>
> >
> > I spent some time these days to review the series and now at patch 10. I
> > need to take a break and so I just sent the comments I had so far.
> >
> > I also did scale testing for northd with
> > https://github.com/hzhou8/ovn-test-script for a 500 chassis, 50 lsp /
> > chassis setup, and noticed that the recompute latency has increased 20%
> > after the series. I think in general it is expected to have some
> > performance penalty for the recompute case because of the new indexes
> > created for I-P. However, the patch 10 "northd: Refactor lflow
management
> > into a separate module." alone introduces a 10% latency increase, which
> > necessitates more investigation.
>
> Before sending out the series I  did some testing on recomputes with a
> large OVN NB DB and SB DB
> (from a 500 node ovn-heater density heavy run).
> I'm aware of the increase in recomputes.  And I did some more testing
> today as well.
>
> In my testing,  I can see that the increase in latency is due to the
> new engine nodes added (lr_lbnat mainly)
> and due to housekeeping of the lflow references.  I do not see any
> increase due to the new lflow-mgr.c added in patch 10.
>
> I compared patch 9 and patch 10 separately and there is no difference
> in lflow engine node recompute time between patch 9 and 10.
>

My results were different. My test profile simulates the ovn-k8s topology
(central mode, not IC) with 500 nodes, 50 LSP/node, with no LB, small
amount of ACLs and PGs.
(
https://github.com/hzhou8/ovn-test-script/blob/main/args.500ch_50lsp_1pg
)

The test results for ovn-northd recompute time are:
main: 1118.3
p9: 1129.5
p10: 1243.4 ==> 10% more than p9
p18: 1357.6

I am not sure if the p10 increase is related to the hash change or
something else.

> Below are the results with the time taken for the mentioned engine
> nodes in msecs for a recompute for some of the individual patches in
> the series.
>
>
> The sample OVN DBs have
>
> 
> Resource  Total
> ---
> Logical switches1001
> 
> Logical routers  501
> 
> Router ports 1501
> 
> Switch ports 29501
> 
> Load balancers35253
> 
> Load balancer group 1
> 
> SB Logical flows268349
> 
> SB DB groups  509
> 
>
> There is one load balancer group which has all the load balancers and
> it is associated with all the logical switches and routers.
>
> Below is the time taken for each engine node in msec
>
> -
> Engine nodes | lb_data | northd  | lr_lbnat  | lflow  |
> -
> ovn-northd-main  | 358  | 2455| x | 2082 |
> -
> ovn-northd-p1| 373   | 2476| x | 2170   |
> -
> ovn-northd-p5| 367   | 2413| x | 2195   |
> -
> ovn-northd-p6| 354   | 688 | 1815  | 2442|
> -
> ovn-northd-p7| 357   | 683 | 1778  | 2806|
> -
> ovn-northd-p9| 352   | 682 | 1781  | 2781|
> -
> ovn-northd-p10   | 365  | 838 | 1707  | 2812|
> -
> ovn-northd-p13   | 362  | 1066| 1882  | 2917|
> -
> ovn-northd-p15   | 359  | 1046| 1688  | 2907|
> -
> ovn-northd-p18   | 379  | 1066| 1729  | 2886|
> 

Re: [ovs-dev] [PATCH ovn v3 2/2] ci: Pin Python, Fedora and Ubuntu runner versions.

[Dumitru Ceara]

On 11/16/23 18:34, Ales Musil wrote:
> On Thu, Nov 16, 2023 at 6:26 PM Dumitru Ceara  wrote:
> 
>> We initially thought always using the latest releases would be more
>> maintainable because we didn't have to bump versions manually.  It turns
>> out that it's the opposite, CI suddenly starts to fail on versions where
>> it used to pass.
>>
>> To avoid ever changing tools in our CI due to newer releases of
>> Python/Fedora/Ubuntu.  We can always bump versions manually.
>>
>> Fixes: 5ee07b32a01d ("ci: Change all GitHub CI jobs to use ubuntu-latest.")
>> Fixes: 60a53abaa38a ("ci: Add automation for building the containers")
>> Signed-off-by: Dumitru Ceara 
>> ---
>>
> 
> Hi Dumitru,
> 
> thank you for the v3. I have one small question below.
> 

Thanks for the review!

> 
>>  .github/workflows/containers.yml   |2 +-
>>  .github/workflows/ovn-fake-multinode-tests.yml |6 +++---
>>  .github/workflows/ovn-kubernetes.yml   |4 ++--
>>  .github/workflows/test.yml |6 +++---
>>  utilities/containers/fedora/Dockerfile |2 +-
>>  5 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/.github/workflows/containers.yml
>> b/.github/workflows/containers.yml
>> index 57e815ed86..bdd1180872 100644
>> --- a/.github/workflows/containers.yml
>> +++ b/.github/workflows/containers.yml
>> @@ -15,7 +15,7 @@ env:
>>
>>  jobs:
>>container:
>> -runs-on: ubuntu-latest
>> +runs-on: ubuntu-22.04
>>  strategy:
>>matrix:
>>  distro: [ fedora, ubuntu ]
>> diff --git a/.github/workflows/ovn-fake-multinode-tests.yml
>> b/.github/workflows/ovn-fake-multinode-tests.yml
>> index 9a5cd83a65..25610df534 100644
>> --- a/.github/workflows/ovn-fake-multinode-tests.yml
>> +++ b/.github/workflows/ovn-fake-multinode-tests.yml
>> @@ -13,7 +13,7 @@ concurrency:
>>  jobs:
>>build:
>>  name: Build ovn-fake-multinode image
>> -runs-on: ubuntu-latest
>> +runs-on: ubuntu-22.04
>>  strategy:
>>matrix:
>>  cfg:
>> @@ -69,7 +69,7 @@ jobs:
>>  path: /tmp/_output/ovn_${{ matrix.cfg.branch }}_image.tar
>>
>>multinode-tests:
>> -runs-on: ubuntu-latest
>> +runs-on: ubuntu-22.04
>>  timeout-minutes: 15
>>  needs: [build]
>>  strategy:
>> @@ -158,7 +158,7 @@ jobs:
>>  - name: set up python
>>uses: actions/setup-python@v4
>>with:
>> -python-version: '3.x'
>> +python-version: '3.12'
>>
>>  - name: Check out ovn
>>uses: actions/checkout@v3
>> diff --git a/.github/workflows/ovn-kubernetes.yml
>> b/.github/workflows/ovn-kubernetes.yml
>> index d9a91874ff..1689396d66 100644
>> --- a/.github/workflows/ovn-kubernetes.yml
>> +++ b/.github/workflows/ovn-kubernetes.yml
>> @@ -24,7 +24,7 @@ env:
>>  jobs:
>>build:
>>  name: Build
>> -runs-on: ubuntu-latest
>> +runs-on: ubuntu-22.04
>>  steps:
>>  - name: Enable Docker experimental features
>>run: |
>> @@ -62,7 +62,7 @@ jobs:
>>e2e:
>>  name: e2e
>>  if: github.event_name != 'schedule'
>> -runs-on: ubuntu-latest
>> +runs-on: ubuntu-22.04
>>  timeout-minutes: 220
>>  strategy:
>>fail-fast: false
>> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
>> index 5c5ce6ed10..a5ccb7e4ae 100644
>> --- a/.github/workflows/test.yml
>> +++ b/.github/workflows/test.yml
>> @@ -94,7 +94,7 @@ jobs:
>>SANITIZERS:  ${{ matrix.cfg.sanitizers }}
>>
>>  name: linux ${{ join(matrix.cfg.*, ' ') }}
>> -runs-on: ubuntu-latest
>> +runs-on: ubuntu-22.04
>>
>>  strategy:
>>fail-fast: false
>> @@ -219,7 +219,7 @@ jobs:
>>  - name: set up python
>>uses: actions/setup-python@v4
>>with:
>> -python-version: '3.x'
>> +python-version: '3.12'
>>  - name: prepare
>>run:  ./.ci/osx-prepare.sh
>>  - name: build
>> @@ -233,7 +233,7 @@ jobs:
>>
>>build-linux-rpm:
>>  name: linux rpm fedora
>> -runs-on: ubuntu-latest
>> +runs-on: ubuntu-22.04
>>  container: fedora:latest
>>  timeout-minutes: 30
>>
>> diff --git a/utilities/containers/fedora/Dockerfile
>> b/utilities/containers/fedora/Dockerfile
>> index 4058d7f5be..066bb0b957 100755
>> --- a/utilities/containers/fedora/Dockerfile
>> +++ b/utilities/containers/fedora/Dockerfile
>> @@ -1,4 +1,4 @@
>> -FROM quay.io/fedora/fedora:latest
>> +FROM registry.fedoraproject.org/fedora:latest
> 
> 
> Why did we switch from quay? I guess it shouldn't matter, just curious.

I had the impression yesterday that there was no fedora:37 on quay.io.
However that's not the case, I was probably doing something wrong.  I
can change it back but it feels more appropriate to use Fedora's own
registry.

> 
> 
>>
>>
>>  ARG CONTAINERS_PATH
>>
>>
>>
> Thanks,
> Ales
> 

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

Re: [ovs-dev] [PATCH ovn v3 0/2] Stabilize CI by pinning container, runner and Python versions.

[Ales Musil]

On Thu, Nov 16, 2023 at 6:26 PM Dumitru Ceara  wrote:

> All of the above were changed to track the latest available releases.
> Initially that seemed like a good idea but in practice, a new release would
> potentially (silently) cause CI runs that used to pass on given stable
> versions to unexpectedly start failing.
>
> To address that this series pins all versions we can control allowing us
> to use different values for different branches [0].
>
> NOTE: this series of patches will look slightly different on each stable
> branch because we need to bump to different OVS stable branch tips and
> we pin to different container and python versions on different stable
> branches.
>
> NOTE2: this patch DOES NOT change the current behavior of always using
> the CI container image we built from the main branch contents.  As
> discussed in the v2 of this series [1], that has issues so we need to
> follow up in the future.  Until then we just unblock CI.
>
> [0]
> https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.09
> https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.06
> https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.03
> https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.12
> https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.09
> https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.06
> https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.03
>
> [1]
> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409482.html
>
> Changes in v3:
> - dropped the patch that builds per-branch CI container images.
> Changes in v2:
> - added first patch to bump OVS submodule
> - addressed Ales' review comments:
>   - moved the logic to determine which image to use out of ci.sh;
> it's now in the workflow itself.
>   - moved setting of OVN_IMAGE in the same block with all related
> env vars
> - added a note for maintainers in the release process documentation
>   to mention that the new workflow will likely have to be triggered
>   manually (at least once) when branching for a new release.  GitHub
>   doesn't allow periodic jobs on non-default branches.
>
> Dumitru Ceara (2):
>   ovs: Bump submodule to include E721 fixes.
>   ci: Pin Python, Fedora and Ubuntu runner versions.
>
>
>  .github/workflows/containers.yml   | 2 +-
>  .github/workflows/ovn-fake-multinode-tests.yml | 6 +++---
>  .github/workflows/ovn-kubernetes.yml   | 4 ++--
>  .github/workflows/test.yml | 6 +++---
>  utilities/containers/fedora/Dockerfile | 2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
>
>
The series looks good to me, thanks.
I have only one question on 2/2.

Acked-by: Ales Musil 


-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

Re: [ovs-dev] [PATCH ovn v3 2/2] ci: Pin Python, Fedora and Ubuntu runner versions.

[Ales Musil]

On Thu, Nov 16, 2023 at 6:26 PM Dumitru Ceara  wrote:

> We initially thought always using the latest releases would be more
> maintainable because we didn't have to bump versions manually.  It turns
> out that it's the opposite, CI suddenly starts to fail on versions where
> it used to pass.
>
> To avoid ever changing tools in our CI due to newer releases of
> Python/Fedora/Ubuntu.  We can always bump versions manually.
>
> Fixes: 5ee07b32a01d ("ci: Change all GitHub CI jobs to use ubuntu-latest.")
> Fixes: 60a53abaa38a ("ci: Add automation for building the containers")
> Signed-off-by: Dumitru Ceara 
> ---
>

Hi Dumitru,

thank you for the v3. I have one small question below.


>  .github/workflows/containers.yml   |2 +-
>  .github/workflows/ovn-fake-multinode-tests.yml |6 +++---
>  .github/workflows/ovn-kubernetes.yml   |4 ++--
>  .github/workflows/test.yml |6 +++---
>  utilities/containers/fedora/Dockerfile |2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/.github/workflows/containers.yml
> b/.github/workflows/containers.yml
> index 57e815ed86..bdd1180872 100644
> --- a/.github/workflows/containers.yml
> +++ b/.github/workflows/containers.yml
> @@ -15,7 +15,7 @@ env:
>
>  jobs:
>container:
> -runs-on: ubuntu-latest
> +runs-on: ubuntu-22.04
>  strategy:
>matrix:
>  distro: [ fedora, ubuntu ]
> diff --git a/.github/workflows/ovn-fake-multinode-tests.yml
> b/.github/workflows/ovn-fake-multinode-tests.yml
> index 9a5cd83a65..25610df534 100644
> --- a/.github/workflows/ovn-fake-multinode-tests.yml
> +++ b/.github/workflows/ovn-fake-multinode-tests.yml
> @@ -13,7 +13,7 @@ concurrency:
>  jobs:
>build:
>  name: Build ovn-fake-multinode image
> -runs-on: ubuntu-latest
> +runs-on: ubuntu-22.04
>  strategy:
>matrix:
>  cfg:
> @@ -69,7 +69,7 @@ jobs:
>  path: /tmp/_output/ovn_${{ matrix.cfg.branch }}_image.tar
>
>multinode-tests:
> -runs-on: ubuntu-latest
> +runs-on: ubuntu-22.04
>  timeout-minutes: 15
>  needs: [build]
>  strategy:
> @@ -158,7 +158,7 @@ jobs:
>  - name: set up python
>uses: actions/setup-python@v4
>with:
> -python-version: '3.x'
> +python-version: '3.12'
>
>  - name: Check out ovn
>uses: actions/checkout@v3
> diff --git a/.github/workflows/ovn-kubernetes.yml
> b/.github/workflows/ovn-kubernetes.yml
> index d9a91874ff..1689396d66 100644
> --- a/.github/workflows/ovn-kubernetes.yml
> +++ b/.github/workflows/ovn-kubernetes.yml
> @@ -24,7 +24,7 @@ env:
>  jobs:
>build:
>  name: Build
> -runs-on: ubuntu-latest
> +runs-on: ubuntu-22.04
>  steps:
>  - name: Enable Docker experimental features
>run: |
> @@ -62,7 +62,7 @@ jobs:
>e2e:
>  name: e2e
>  if: github.event_name != 'schedule'
> -runs-on: ubuntu-latest
> +runs-on: ubuntu-22.04
>  timeout-minutes: 220
>  strategy:
>fail-fast: false
> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
> index 5c5ce6ed10..a5ccb7e4ae 100644
> --- a/.github/workflows/test.yml
> +++ b/.github/workflows/test.yml
> @@ -94,7 +94,7 @@ jobs:
>SANITIZERS:  ${{ matrix.cfg.sanitizers }}
>
>  name: linux ${{ join(matrix.cfg.*, ' ') }}
> -runs-on: ubuntu-latest
> +runs-on: ubuntu-22.04
>
>  strategy:
>fail-fast: false
> @@ -219,7 +219,7 @@ jobs:
>  - name: set up python
>uses: actions/setup-python@v4
>with:
> -python-version: '3.x'
> +python-version: '3.12'
>  - name: prepare
>run:  ./.ci/osx-prepare.sh
>  - name: build
> @@ -233,7 +233,7 @@ jobs:
>
>build-linux-rpm:
>  name: linux rpm fedora
> -runs-on: ubuntu-latest
> +runs-on: ubuntu-22.04
>  container: fedora:latest
>  timeout-minutes: 30
>
> diff --git a/utilities/containers/fedora/Dockerfile
> b/utilities/containers/fedora/Dockerfile
> index 4058d7f5be..066bb0b957 100755
> --- a/utilities/containers/fedora/Dockerfile
> +++ b/utilities/containers/fedora/Dockerfile
> @@ -1,4 +1,4 @@
> -FROM quay.io/fedora/fedora:latest
> +FROM registry.fedoraproject.org/fedora:latest


Why did we switch from quay? I guess it shouldn't matter, just curious.


>
>
>  ARG CONTAINERS_PATH
>
>
>
Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

[ovs-dev] [PATCH ovn v3 2/2] ci: Pin Python, Fedora and Ubuntu runner versions.

[Dumitru Ceara]

We initially thought always using the latest releases would be more
maintainable because we didn't have to bump versions manually.  It turns
out that it's the opposite, CI suddenly starts to fail on versions where
it used to pass.

To avoid ever changing tools in our CI due to newer releases of
Python/Fedora/Ubuntu.  We can always bump versions manually.

Fixes: 5ee07b32a01d ("ci: Change all GitHub CI jobs to use ubuntu-latest.")
Fixes: 60a53abaa38a ("ci: Add automation for building the containers")
Signed-off-by: Dumitru Ceara 
---
 .github/workflows/containers.yml   |2 +-
 .github/workflows/ovn-fake-multinode-tests.yml |6 +++---
 .github/workflows/ovn-kubernetes.yml   |4 ++--
 .github/workflows/test.yml |6 +++---
 utilities/containers/fedora/Dockerfile |2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/.github/workflows/containers.yml b/.github/workflows/containers.yml
index 57e815ed86..bdd1180872 100644
--- a/.github/workflows/containers.yml
+++ b/.github/workflows/containers.yml
@@ -15,7 +15,7 @@ env:
 
 jobs:
   container:
-runs-on: ubuntu-latest
+runs-on: ubuntu-22.04
 strategy:
   matrix:
 distro: [ fedora, ubuntu ]
diff --git a/.github/workflows/ovn-fake-multinode-tests.yml 
b/.github/workflows/ovn-fake-multinode-tests.yml
index 9a5cd83a65..25610df534 100644
--- a/.github/workflows/ovn-fake-multinode-tests.yml
+++ b/.github/workflows/ovn-fake-multinode-tests.yml
@@ -13,7 +13,7 @@ concurrency:
 jobs:
   build:
 name: Build ovn-fake-multinode image
-runs-on: ubuntu-latest
+runs-on: ubuntu-22.04
 strategy:
   matrix:
 cfg:
@@ -69,7 +69,7 @@ jobs:
 path: /tmp/_output/ovn_${{ matrix.cfg.branch }}_image.tar
 
   multinode-tests:
-runs-on: ubuntu-latest
+runs-on: ubuntu-22.04
 timeout-minutes: 15
 needs: [build]
 strategy:
@@ -158,7 +158,7 @@ jobs:
 - name: set up python
   uses: actions/setup-python@v4
   with:
-python-version: '3.x'
+python-version: '3.12'
 
 - name: Check out ovn
   uses: actions/checkout@v3
diff --git a/.github/workflows/ovn-kubernetes.yml 
b/.github/workflows/ovn-kubernetes.yml
index d9a91874ff..1689396d66 100644
--- a/.github/workflows/ovn-kubernetes.yml
+++ b/.github/workflows/ovn-kubernetes.yml
@@ -24,7 +24,7 @@ env:
 jobs:
   build:
 name: Build
-runs-on: ubuntu-latest
+runs-on: ubuntu-22.04
 steps:
 - name: Enable Docker experimental features
   run: |
@@ -62,7 +62,7 @@ jobs:
   e2e:
 name: e2e
 if: github.event_name != 'schedule'
-runs-on: ubuntu-latest
+runs-on: ubuntu-22.04
 timeout-minutes: 220
 strategy:
   fail-fast: false
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 5c5ce6ed10..a5ccb7e4ae 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -94,7 +94,7 @@ jobs:
   SANITIZERS:  ${{ matrix.cfg.sanitizers }}
 
 name: linux ${{ join(matrix.cfg.*, ' ') }}
-runs-on: ubuntu-latest
+runs-on: ubuntu-22.04
 
 strategy:
   fail-fast: false
@@ -219,7 +219,7 @@ jobs:
 - name: set up python
   uses: actions/setup-python@v4
   with:
-python-version: '3.x'
+python-version: '3.12'
 - name: prepare
   run:  ./.ci/osx-prepare.sh
 - name: build
@@ -233,7 +233,7 @@ jobs:
 
   build-linux-rpm:
 name: linux rpm fedora
-runs-on: ubuntu-latest
+runs-on: ubuntu-22.04
 container: fedora:latest
 timeout-minutes: 30
 
diff --git a/utilities/containers/fedora/Dockerfile 
b/utilities/containers/fedora/Dockerfile
index 4058d7f5be..066bb0b957 100755
--- a/utilities/containers/fedora/Dockerfile
+++ b/utilities/containers/fedora/Dockerfile
@@ -1,4 +1,4 @@
-FROM quay.io/fedora/fedora:latest
+FROM registry.fedoraproject.org/fedora:latest
 
 ARG CONTAINERS_PATH
 

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

[ovs-dev] [PATCH ovn v3 1/2] ovs: Bump submodule to include E721 fixes.

[Dumitru Ceara]

Specifically the following commit:
  fdbf0bb2aed5 ("flake8: Fix E721 check failures.")

Signed-off-by: Dumitru Ceara 
---
 ovs |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovs b/ovs
index 19770fc307..fdbf0bb2ae 16
--- a/ovs
+++ b/ovs
@@ -1 +1 @@
-Subproject commit 19770fc307054cd72572348c93904557c3618402
+Subproject commit fdbf0bb2aed53e70b455eb1adcfda8d8278ea690

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

[ovs-dev] [PATCH ovn v3 0/2] Stabilize CI by pinning container, runner and Python versions.

[Dumitru Ceara]

All of the above were changed to track the latest available releases.
Initially that seemed like a good idea but in practice, a new release would
potentially (silently) cause CI runs that used to pass on given stable
versions to unexpectedly start failing.

To address that this series pins all versions we can control allowing us
to use different values for different branches [0].

NOTE: this series of patches will look slightly different on each stable
branch because we need to bump to different OVS stable branch tips and
we pin to different container and python versions on different stable
branches.

NOTE2: this patch DOES NOT change the current behavior of always using
the CI container image we built from the main branch contents.  As
discussed in the v2 of this series [1], that has issues so we need to
follow up in the future.  Until then we just unblock CI.

[0]
https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.09
https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.06
https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-23.03
https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.12
https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.09
https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.06
https://github.com/dceara/ovn/tree/pin-versions-in-ci-v3-22.03

[1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409482.html

Changes in v3:
- dropped the patch that builds per-branch CI container images.
Changes in v2:
- added first patch to bump OVS submodule
- addressed Ales' review comments:
  - moved the logic to determine which image to use out of ci.sh;
it's now in the workflow itself.
  - moved setting of OVN_IMAGE in the same block with all related
env vars
- added a note for maintainers in the release process documentation
  to mention that the new workflow will likely have to be triggered
  manually (at least once) when branching for a new release.  GitHub
  doesn't allow periodic jobs on non-default branches.

Dumitru Ceara (2):
  ovs: Bump submodule to include E721 fixes.
  ci: Pin Python, Fedora and Ubuntu runner versions.


 .github/workflows/containers.yml   | 2 +-
 .github/workflows/ovn-fake-multinode-tests.yml | 6 +++---
 .github/workflows/ovn-kubernetes.yml   | 4 ++--
 .github/workflows/test.yml | 6 +++---
 utilities/containers/fedora/Dockerfile | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

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

Re: [ovs-dev] [PATCH ovn v2] tests: Remove broken "feature inactivity probe" test.

[Dumitru Ceara]

On 11/16/23 16:21, Xavier Simonart wrote:
> Hi Dumitru
> 
> Looks good to me, thanks.
> Acked-by: Xavier Simonart 
> 

Thanks!  Applied and backported down to 22.03.  One flaky test less to
worry about! :)

> Thanks
> Xavier
> 
> On Thu, Nov 16, 2023 at 2:54 PM Dumitru Ceara  wrote:
>>
>> The test incorrectly assumed that "ovn-nbctl --wait=hv sync" will always
>> send an OpenFlow barrier to ovs-vswitchd.  That doesn't happen unless
>> there are other OpenFlow (rule or group) changes that need to be
>> programmed in the datapath.
>>
>> An initial attempt to fix this [0] uncovered the fact that there's no
>> easy way to cover all possible scenarios [1].  It seems that the effort
>> to fix the test is not justified for the case it tests for (that
>> inactivity probes are handled by the features module - that is quite
>> obviously handled in the code).  It's therefore more reasonable to just
>> skip the test (to avoid noise in CI).
>>
>> Spotted in CI, mostly on oversubscribed systems.  A log sample that
>> shows that ovn-controller didn't generate any barrier request for the
>> nbctl sync request:
>>
>>   2023-11-15T12:12:22.937Z|00084|vconn|DBG|unix#4: received: 
>> OFPT_BARRIER_REQUEST (OF1.5) (xid=0x13):
>>   2023-11-15T12:12:22.937Z|00085|vconn|DBG|unix#4: sent (Success): 
>> OFPT_BARRIER_REPLY (OF1.5) (xid=0x13):
>>   ...
>>   2023-11-15T12:12:23.032Z|00090|unixctl|DBG|received request 
>> time/warp["6"], id=0
>>   2023-11-15T12:12:23.032Z|00091|unixctl|DBG|replying with success, id=0: 
>> "warped"
>>   2023-11-15T12:12:23.042Z|00094|vconn|DBG|unix#3: sent (Success): 
>> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>>   2023-11-15T12:12:23.042Z|00095|vconn|DBG|unix#4: sent (Success): 
>> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>>   2023-11-15T12:12:23.042Z|00097|vconn|DBG|unix#5: sent (Success): 
>> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>>   2023-11-15T12:12:23.042Z|00098|unixctl|DBG|received request 
>> time/warp["6"], id=0
>>   2023-11-15T12:12:23.042Z|00099|unixctl|DBG|replying with success, id=0: 
>> "warped"
>>   2023-11-15T12:12:23.052Z|00100|rconn|ERR|br-int<->unix#3: no response to 
>> inactivity probe after 60 seconds, disconnecting
>>   2023-11-15T12:12:23.052Z|00101|rconn|ERR|br-int<->unix#4: no response to 
>> inactivity probe after 60 seconds, disconnecting
>>   2023-11-15T12:12:23.052Z|00102|rconn|ERR|br-int<->unix#5: no response to 
>> inactivity probe after 60 seconds, disconnecting
>>   2023-11-15T12:12:23.052Z|00103|unixctl|DBG|received request 
>> time/warp["6"], id=0
>>
>> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409473.html
>> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409530.html
>>
>> Fixes: ff00142808dc ("controller: Fixed ovs/ovn(features) connection lost 
>> when running more than 120 seconds")
>> Signed-off-by: Dumitru Ceara 
>> ---
>> V2:
>> - removed the test instead of trying to fix it (after discussion with
>> Xavier).
>> ---
>>  tests/ovn.at | 31 ---
>>  1 file changed, 31 deletions(-)
>>
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index b8c61f87fb..198387d93e 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -35342,37 +35342,6 @@ OVN_CLEANUP([hv1],[hv2])
>>  AT_CLEANUP
>>  ])
>>
>> -OVN_FOR_EACH_NORTHD([
>> -AT_SETUP([feature inactivity probe])
>> -ovn_start
>> -net_add n1
>> -
>> -sim_add hv1
>> -as hv1
>> -check ovs-vsctl add-br br-phys
>> -ovn_attach n1 br-phys 192.168.0.1
>> -
>> -dnl Ensure that there are 4 openflow connections.
>> -OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version' 
>> hv1/ovs-vswitchd.log)" -eq "4"])
>> -
>> -dnl "Wait" 3 times 60 seconds and ensure ovn-controller writes to the
>> -dnl openflow connections in the meantime.  This should allow ovs-vswitchd
>> -dnl to probe the openflow connections at least twice.
>> -
>> -as hv1 ovs-appctl time/warp 6
>> -check ovn-nbctl --wait=hv sync
>> -
>> -as hv1 ovs-appctl time/warp 6
>> -check ovn-nbctl --wait=hv sync
>> -
>> -as hv1 ovs-appctl time/warp 6
>> -check ovn-nbctl --wait=hv sync
>> -
>> -AT_CHECK([test -z "`grep disconnecting hv1/ovs-vswitchd.log`"])
>> -OVN_CLEANUP([hv1])
>> -AT_CLEANUP
>> -])
>> -
>>  OVN_FOR_EACH_NORTHD([
>>  AT_SETUP([Logical flows with Chassis_Template_Var references])
>>  AT_KEYWORDS([templates])
>> --
>> 2.39.3
>>
> 

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

Re: [ovs-dev] [PATCH v2] mcast-snooping: Store IGMP/MLD protocol version.

[Eelco Chaudron]

On 16 Nov 2023, at 15:58, Eelco Chaudron wrote:

> On 16 Nov 2023, at 15:08, Mohammad Heib wrote:
>
>> Store the igmp/mld protocol version into the
>> mcast_group internally.
>>
>> This can be used by ovs consumers to update
>> about the igmp/mld version of each group.
>>
>> Signed-off-by: Mohammad Heib 
>
> Hi Mohammad,
>
> Thanks for the patch, I have not reviewed the actual code yet, but it would 
> be good to include a use case for this patch (maybe expand this to a series). 
> This way it’s clear why we need to store this information.
>
> Cheers,
>
> Eelco

After a quick code review I have one comment, see below.

>> ---
>>  lib/mcast-snooping.c | 16 +---
>>  lib/mcast-snooping.h |  9 ++---
>>  ofproto/ofproto-dpif-xlate.c |  6 --
>>  3 files changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
>> index 029ca2855..926daf9ac 100644
>> --- a/lib/mcast-snooping.c
>> +++ b/lib/mcast-snooping.c
>> @@ -389,7 +389,7 @@ mcast_snooping_prune_expired(struct mcast_snooping *ms,
>>  bool
>>  mcast_snooping_add_group(struct mcast_snooping *ms,
>>   const struct in6_addr *addr,
>> - uint16_t vlan, void *port)
>> + uint16_t vlan, void *port, uint8_t grp_proto)
>>  OVS_REQ_WRLOCK(ms->rwlock)
>>  {
>>  bool learned;
>> @@ -415,6 +415,7 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
>>  hmap_insert(>table, >hmap_node, hash);
>>  grp->addr = *addr;
>>  grp->vlan = vlan;
>> +grp->protocol_version = grp_proto;
>>  ovs_list_init(>bundle_lru);
>>  learned = true;
>>  ms->need_revalidate = true;
>> @@ -431,17 +432,17 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
>>
>>  bool
>>  mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
>> - uint16_t vlan, void *port)
>> + uint16_t vlan, void *port, uint8_t grp_proto)
>>  OVS_REQ_WRLOCK(ms->rwlock)
>>  {
>>  struct in6_addr addr = in6_addr_mapped_ipv4(ip4);
>> -return mcast_snooping_add_group(ms, , vlan, port);
>> +return mcast_snooping_add_group(ms, , vlan, port, grp_proto);
>>  }
>>
>>  int
>>  mcast_snooping_add_report(struct mcast_snooping *ms,
>>const struct dp_packet *p,
>> -  uint16_t vlan, void *port)
>> +  uint16_t vlan, void *port, uint8_t grp_proto)
>>  {
>>  ovs_be32 ip4;
>>  size_t offset;
>> @@ -478,7 +479,7 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
>>  || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
>>  ret = mcast_snooping_leave_group4(ms, ip4, vlan, port);
>>  } else {
>> -ret = mcast_snooping_add_group4(ms, ip4, vlan, port);
>> +ret = mcast_snooping_add_group4(ms, ip4, vlan, port, grp_proto);
>>  }
>>  if (ret) {
>>  count++;
>> @@ -513,7 +514,7 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
>>
>>  switch (mld->type) {
>>  case MLD_REPORT:
>> -ret = mcast_snooping_add_group(ms, addr, vlan, port);
>> +ret = mcast_snooping_add_group(ms, addr, vlan, port, MLD_REPORT);
>>  if (ret) {
>>  count++;
>>  }
>> @@ -545,7 +546,8 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
>>  || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
>>  ret = mcast_snooping_leave_group(ms, addr, vlan, port);
>>  } else {
>> -ret = mcast_snooping_add_group(ms, addr, vlan, port);
>> +ret = mcast_snooping_add_group(ms, addr, vlan, port,
>> +   MLD2_REPORT);
>>  }
>>  if (ret) {
>>  count++;
>> diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
>> index f120405da..6321b63ab 100644
>> --- a/lib/mcast-snooping.h
>> +++ b/lib/mcast-snooping.h
>> @@ -51,6 +51,9 @@ struct mcast_group {
>>  /* VLAN tag. */
>>  uint16_t vlan;
>>
>> +/* Multicast group IPv6/IPv4 Protocol version IGMPv1,2,3 or MLDv1,2 */
>> +uint8_t protocol_version;

Not sure if this is the correct way to store the data. You are storing the IGMP 
type definition for IGMP, and for MLD the MLD ICMPv6 value.
I would prefer a newly defined type (enum) that is globally unique because a 
new protocol in the future might use an overlapping value.

>>  /* Node in parent struct mcast_snooping group_lru. */
>>  struct ovs_list group_node OVS_GUARDED;
>>
>> @@ -185,14 +188,14 @@ mcast_snooping_lookup4(const struct mcast_snooping 
>> *ms, ovs_be32 ip4,
>>  /* Learning. */
>>  bool mcast_snooping_add_group(struct mcast_snooping *ms,
>>const struct in6_addr *addr,
>> -  uint16_t vlan, void *port)
>> + 

Re: [ovs-dev] [PATCH ovn v2] tests: Remove broken "feature inactivity probe" test.

[Xavier Simonart]

Hi Dumitru

Looks good to me, thanks.
Acked-by: Xavier Simonart 

Thanks
Xavier

On Thu, Nov 16, 2023 at 2:54 PM Dumitru Ceara  wrote:
>
> The test incorrectly assumed that "ovn-nbctl --wait=hv sync" will always
> send an OpenFlow barrier to ovs-vswitchd.  That doesn't happen unless
> there are other OpenFlow (rule or group) changes that need to be
> programmed in the datapath.
>
> An initial attempt to fix this [0] uncovered the fact that there's no
> easy way to cover all possible scenarios [1].  It seems that the effort
> to fix the test is not justified for the case it tests for (that
> inactivity probes are handled by the features module - that is quite
> obviously handled in the code).  It's therefore more reasonable to just
> skip the test (to avoid noise in CI).
>
> Spotted in CI, mostly on oversubscribed systems.  A log sample that
> shows that ovn-controller didn't generate any barrier request for the
> nbctl sync request:
>
>   2023-11-15T12:12:22.937Z|00084|vconn|DBG|unix#4: received: 
> OFPT_BARRIER_REQUEST (OF1.5) (xid=0x13):
>   2023-11-15T12:12:22.937Z|00085|vconn|DBG|unix#4: sent (Success): 
> OFPT_BARRIER_REPLY (OF1.5) (xid=0x13):
>   ...
>   2023-11-15T12:12:23.032Z|00090|unixctl|DBG|received request 
> time/warp["6"], id=0
>   2023-11-15T12:12:23.032Z|00091|unixctl|DBG|replying with success, id=0: 
> "warped"
>   2023-11-15T12:12:23.042Z|00094|vconn|DBG|unix#3: sent (Success): 
> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>   2023-11-15T12:12:23.042Z|00095|vconn|DBG|unix#4: sent (Success): 
> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>   2023-11-15T12:12:23.042Z|00097|vconn|DBG|unix#5: sent (Success): 
> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>   2023-11-15T12:12:23.042Z|00098|unixctl|DBG|received request 
> time/warp["6"], id=0
>   2023-11-15T12:12:23.042Z|00099|unixctl|DBG|replying with success, id=0: 
> "warped"
>   2023-11-15T12:12:23.052Z|00100|rconn|ERR|br-int<->unix#3: no response to 
> inactivity probe after 60 seconds, disconnecting
>   2023-11-15T12:12:23.052Z|00101|rconn|ERR|br-int<->unix#4: no response to 
> inactivity probe after 60 seconds, disconnecting
>   2023-11-15T12:12:23.052Z|00102|rconn|ERR|br-int<->unix#5: no response to 
> inactivity probe after 60 seconds, disconnecting
>   2023-11-15T12:12:23.052Z|00103|unixctl|DBG|received request 
> time/warp["6"], id=0
>
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409473.html
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409530.html
>
> Fixes: ff00142808dc ("controller: Fixed ovs/ovn(features) connection lost 
> when running more than 120 seconds")
> Signed-off-by: Dumitru Ceara 
> ---
> V2:
> - removed the test instead of trying to fix it (after discussion with
> Xavier).
> ---
>  tests/ovn.at | 31 ---
>  1 file changed, 31 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index b8c61f87fb..198387d93e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -35342,37 +35342,6 @@ OVN_CLEANUP([hv1],[hv2])
>  AT_CLEANUP
>  ])
>
> -OVN_FOR_EACH_NORTHD([
> -AT_SETUP([feature inactivity probe])
> -ovn_start
> -net_add n1
> -
> -sim_add hv1
> -as hv1
> -check ovs-vsctl add-br br-phys
> -ovn_attach n1 br-phys 192.168.0.1
> -
> -dnl Ensure that there are 4 openflow connections.
> -OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version' 
> hv1/ovs-vswitchd.log)" -eq "4"])
> -
> -dnl "Wait" 3 times 60 seconds and ensure ovn-controller writes to the
> -dnl openflow connections in the meantime.  This should allow ovs-vswitchd
> -dnl to probe the openflow connections at least twice.
> -
> -as hv1 ovs-appctl time/warp 6
> -check ovn-nbctl --wait=hv sync
> -
> -as hv1 ovs-appctl time/warp 6
> -check ovn-nbctl --wait=hv sync
> -
> -as hv1 ovs-appctl time/warp 6
> -check ovn-nbctl --wait=hv sync
> -
> -AT_CHECK([test -z "`grep disconnecting hv1/ovs-vswitchd.log`"])
> -OVN_CLEANUP([hv1])
> -AT_CLEANUP
> -])
> -
>  OVN_FOR_EACH_NORTHD([
>  AT_SETUP([Logical flows with Chassis_Template_Var references])
>  AT_KEYWORDS([templates])
> --
> 2.39.3
>

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

[ovs-dev] [PATCH v4 2/3] dpctl, ovs-ofctl: Unify parsing of ct-flush arguments.

[Ales Musil]

In order to make the command extensible unify the arguments
parsing into single function. This will be later on used
for the mark and labels arguments.

Signed-off-by: Ales Musil 
---
v4: Rebase on top of current master.
v3: Rebase on top of current master.
Fix the system-tests failure.
---
 include/openvswitch/ofp-ct.h |  5 ++--
 lib/dpctl.c  | 41 ---
 lib/ofp-ct.c | 47 +++-
 tests/system-traffic.at  |  2 +-
 utilities/ovs-ofctl.c| 37 ++--
 5 files changed, 62 insertions(+), 70 deletions(-)

diff --git a/include/openvswitch/ofp-ct.h b/include/openvswitch/ofp-ct.h
index c8023c309..cd6192e6f 100644
--- a/include/openvswitch/ofp-ct.h
+++ b/include/openvswitch/ofp-ct.h
@@ -58,8 +58,9 @@ bool ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *, 
uint8_t ip_proto);
 bool ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *, uint8_t ip_proto);
 
 void ofp_ct_match_format(struct ds *, const struct ofp_ct_match *);
-bool ofp_ct_tuple_parse(struct ofp_ct_tuple *, const char *,
-struct ds *, uint8_t *ip_proto, uint16_t *l3_type);
+bool ofp_ct_match_parse(const char **, int argc, struct ds *,
+struct ofp_ct_match *, bool *with_zone,
+uint16_t *zone_id);
 
 enum ofperr ofp_ct_match_decode(struct ofp_ct_match *, bool *with_zone,
 uint16_t *zone_id, const struct ofp_header *);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index cd12625a1..bbab5881e 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1773,48 +1773,17 @@ dpctl_flush_conntrack(int argc, const char *argv[],
 struct dpif *dpif = NULL;
 struct ofp_ct_match match = {0};
 struct ds ds = DS_EMPTY_INITIALIZER;
-uint16_t zone, *pzone = NULL;
+uint16_t zone;
 int error;
 int args = argc - 1;
-int zone_pos = 1;
+bool with_zone = false;
 
 if (dp_arg_exists(argc, argv)) {
 args--;
-zone_pos = 2;
-}
-
-/* Parse zone. */
-if (args && !strncmp(argv[zone_pos], "zone=", 5)) {
-if (!ovs_scan(argv[zone_pos], "zone=%"SCNu16, )) {
-ds_put_cstr(, "failed to parse zone");
-error = EINVAL;
-goto error;
-}
-pzone = 
-args--;
-}
-
-/* Parse ct tuples. */
-for (int i = 0; i < 2; i++) {
-if (!args) {
-break;
-}
-
-struct ofp_ct_tuple *tuple =
-i ? _reply : _orig;
-const char *arg = argv[argc - args];
-
-if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, , _proto,
-  _type)) {
-error = EINVAL;
-goto error;
-}
-args--;
 }
 
-/* Report error if there is more than one unparsed argument. */
-if (args > 0) {
-ds_put_cstr(, "invalid arguments");
+if (args && !ofp_ct_match_parse([argc - args], args, , ,
+_zone, )) {
 error = EINVAL;
 goto error;
 }
@@ -1825,7 +1794,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
 return error;
 }
 
-error = ct_dpif_flush(dpif, pzone, );
+error = ct_dpif_flush(dpif, with_zone ?  : NULL, );
 if (!error) {
 dpif_close(dpif);
 return 0;
diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c
index 85a9d8bec..32aeb5455 100644
--- a/lib/ofp-ct.c
+++ b/lib/ofp-ct.c
@@ -98,7 +98,7 @@ ofp_ct_match_format(struct ds *ds, const struct ofp_ct_match 
*match)
 /* Parses a specification of a conntrack 5-tuple from 's' into 'tuple'.
  * Returns true on success.  Otherwise, returns false and puts the error
  * message in 'ds'. */
-bool
+static bool
 ofp_ct_tuple_parse(struct ofp_ct_tuple *tuple, const char *s,
struct ds *ds, uint8_t *ip_proto, uint16_t *l3_type)
 {
@@ -216,6 +216,51 @@ error:
 return false;
 }
 
+/* Parses a specification of a conntrack match from 'argv' into 'match'.
+ * Returns true on success. Otherwise, returns false and puts the error
+ * message in 'ds'. */
+bool
+ofp_ct_match_parse(const char **argv, int argc, struct ds *ds,
+   struct ofp_ct_match *match, bool *with_zone,
+   uint16_t *zone_id)
+{
+int args = argc;
+
+/* Parse zone. */
+if (args && !strncmp(argv[argc - args], "zone=", 5)) {
+if (!ovs_scan(argv[argc - args], "zone=%"SCNu16, zone_id)) {
+ds_put_cstr(ds, "failed to parse zone");
+return false;
+}
+*with_zone = true;
+args--;
+}
+
+/* Parse ct tuples. */
+for (int i = 0; i < 2; i++) {
+if (!args) {
+break;
+}
+
+struct ofp_ct_tuple *tuple =
+i ? >tuple_reply : >tuple_orig;
+const char *arg = argv[argc - args];
+
+if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, ds, >ip_proto,
+  

[ovs-dev] [PATCH v4 3/3] openflow: Allow CT flush to match on mark and labels.

[Ales Musil]

Extend the current NX_CT_FLUSH with four additional fields,
that allow to match on CT entry "mark" or "labels". This
is encoded as separate TLV values which is backward compatible.
Versions that do not support them will simply ignore it.

Extend also the ovs-dpctl and ovs-ofctl command line tools with
option to specify those two matching parameters for the "ct-flush"
command.

Reported-at: https://issues.redhat.com/browse/FDP-55
Signed-off-by: Ales Musil 
---
v4: Rebase on top of current master.
Address comments from Ilya:
- Add NEWs entry.
- Adjust the flags to use unsigned int.
- Make the encode function more user-friendly.
- Adjust the tests.
v3: Rebase on top of current master.
v2: Make sure that the mask decoding matches the dpctl/ovs-ofctl interface.
---
 NEWS  |   4 +
 include/openflow/nicira-ext.h |   4 +
 include/openvswitch/ofp-ct.h  |   9 ++-
 lib/ct-dpif.c |  12 ++-
 lib/dpctl.c   |   5 +-
 lib/ofp-ct.c  | 135 +-
 tests/ofp-print.at|  85 +
 tests/ovs-ofctl.at|  42 ++-
 tests/system-traffic.at   | 112 ++--
 utilities/ovs-ofctl.8.in  |  31 
 utilities/ovs-ofctl.c |  12 +--
 11 files changed, 381 insertions(+), 70 deletions(-)

diff --git a/NEWS b/NEWS
index 43aea97b5..528c8b0a1 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,7 @@ Post-v3.2.0
from older version is supported but it may trigger more leader elections
during the process, and error logs complaining unrecognized fields may
be observed on old nodes.
+<<< HEAD
- ovs-appctl:
  * Output of 'dpctl/show' command no longer shows interface configuration
status, only values of the actual configuration options, a.k.a.
@@ -13,6 +14,9 @@ Post-v3.2.0
a.k.a. 'configured' values, can be found in the 'status' column of
the Interface table, i.e. with 'ovs-vsctl get interface <..> status'.
Reported names adjusted accordingly.
+   - OpenFlow:
+ * Extended the NXT_CT_FLUSH to support matching on CT label and mark
+   fields.
 
 
 v3.2.0 - 17 Aug 2023
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 768775898..959845ce6 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -1075,6 +1075,10 @@ enum nx_ct_flush_tlv_type {
 * by 'enum nx_ct_flush_tuple_tlv_type'*/
 /* Primitive types. */
 NXT_CT_ZONE_ID = 2,/* be16 zone id. */
+NXT_CT_MARK = 3,   /* be32 mark. */
+NXT_CT_MARK_MASK = 4,  /* be32 mark mask. */
+NXT_CT_LABELS = 5, /* be128 labels. */
+NXT_CT_LABELS_MASK = 6,/* be128 labels mask. */
 };
 
 /* CT flush nested TLVs. */
diff --git a/include/openvswitch/ofp-ct.h b/include/openvswitch/ofp-ct.h
index cd6192e6f..d57b62678 100644
--- a/include/openvswitch/ofp-ct.h
+++ b/include/openvswitch/ofp-ct.h
@@ -51,11 +51,16 @@ struct ofp_ct_match {
 
 struct ofp_ct_tuple tuple_orig;
 struct ofp_ct_tuple tuple_reply;
+
+uint32_t mark;
+uint32_t mark_mask;
+
+ovs_u128 labels;
+ovs_u128 labels_mask;
 };
 
 bool ofp_ct_match_is_zero(const struct ofp_ct_match *);
-bool ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *, uint8_t ip_proto);
-bool ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *, uint8_t ip_proto);
+bool ofp_ct_match_is_five_tuple(const struct ofp_ct_match *);
 
 void ofp_ct_match_format(struct ds *, const struct ofp_ct_match *);
 bool ofp_ct_match_parse(const char **, int argc, struct ds *,
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index f59c6e560..0fd14b99f 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -269,6 +269,15 @@ ct_dpif_entry_cmp(const struct ct_dpif_entry *entry,
 return false;
 }
 
+if ((match->mark & match->mark_mask) != (entry->mark & match->mark_mask)) {
+return false;
+}
+
+if (!ovs_u128_equals(ovs_u128_and(match->labels, match->labels_mask),
+ ovs_u128_and(entry->labels, match->labels_mask))) {
+return false;
+}
+
 return true;
 }
 
@@ -295,8 +304,7 @@ ct_dpif_flush_tuple(struct dpif *dpif, const uint16_t *zone,
 
 /* If we have full five tuple in original and empty reply tuple just
  * do the flush over original tuple directly. */
-if (ofp_ct_tuple_is_five_tuple(>tuple_orig, match->ip_proto) &&
-ofp_ct_tuple_is_zero(>tuple_reply, match->ip_proto)) {
+if (ofp_ct_match_is_five_tuple(match)) {
 struct ct_dpif_tuple tuple;
 
 ct_dpif_tuple_from_ofp_ct_tuple(>tuple_orig, ,
diff --git a/lib/dpctl.c b/lib/dpctl.c
index bbab5881e..9d28a91ba 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -2981,8 +2981,9 @@ static const struct dpctl_command all_commands[] = {
   0, 4, dpctl_dump_conntrack, DP_RO },
 { "dump-conntrack-exp", "[dp] [zone=N]",
   0, 2, 

[ovs-dev] [PATCH v4 0/3] Extend the CT flush with mark and labels fields

[Ales Musil]

The CT flush is not capable of partial flush based
on the touples and zone combinations. Extend it
so it also accepts mark and labels. As part of this
series unify parsing of arguments in dpctl and ovs-ofctl
for the ct flush command.

Ales Musil (3):
  ofp-prop: Add helper for parsing and storing of ovs_u128.
  dpctl, ovs-ofctl: Unify parsing of ct-flush arguments.
  openflow: Allow CT flush to match on mark and labels.

 NEWS   |   4 +
 include/openflow/nicira-ext.h  |   4 +
 include/openvswitch/ofp-ct.h   |  14 ++-
 include/openvswitch/ofp-prop.h |   5 +
 lib/ct-dpif.c  |  12 ++-
 lib/dpctl.c|  46 ++---
 lib/ofp-ct.c   | 182 -
 lib/ofp-prop.c |  42 
 tests/ofp-print.at |  85 +++
 tests/ovs-ofctl.at |  42 +++-
 tests/system-traffic.at| 112 +---
 utilities/ovs-ofctl.8.in   |  31 +++---
 utilities/ovs-ofctl.c  |  49 +++--
 13 files changed, 489 insertions(+), 139 deletions(-)

-- 
2.41.0

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

[ovs-dev] [PATCH v4 1/3] ofp-prop: Add helper for parsing and storing of ovs_u128.

[Ales Musil]

Add helper methods that allow us to store and parse the
ovs_u128 type.

Signed-off-by: Ales Musil 
---
v4: Rebase on top of current master.
Use ofprop_put instead of manual ofpbuf_start/put/end.
v3: Rebase on top of current master.
v2: Add missing ofpprop_parse_be128() function.
---
 include/openvswitch/ofp-prop.h |  5 
 lib/ofp-prop.c | 42 ++
 2 files changed, 47 insertions(+)

diff --git a/include/openvswitch/ofp-prop.h b/include/openvswitch/ofp-prop.h
index e676f8dc0..451f5dae2 100644
--- a/include/openvswitch/ofp-prop.h
+++ b/include/openvswitch/ofp-prop.h
@@ -84,10 +84,13 @@ enum ofperr ofpprop_pull(struct ofpbuf *msg, struct ofpbuf 
*property,
 enum ofperr ofpprop_parse_be16(const struct ofpbuf *, ovs_be16 *value);
 enum ofperr ofpprop_parse_be32(const struct ofpbuf *, ovs_be32 *value);
 enum ofperr ofpprop_parse_be64(const struct ofpbuf *, ovs_be64 *value);
+enum ofperr ofpprop_parse_be128(const struct ofpbuf *property,
+ovs_be128 *value);
 enum ofperr ofpprop_parse_u8(const struct ofpbuf *, uint8_t *value);
 enum ofperr ofpprop_parse_u16(const struct ofpbuf *, uint16_t *value);
 enum ofperr ofpprop_parse_u32(const struct ofpbuf *, uint32_t *value);
 enum ofperr ofpprop_parse_u64(const struct ofpbuf *, uint64_t *value);
+enum ofperr ofpprop_parse_u128(const struct ofpbuf *, ovs_u128 *value);
 enum ofperr ofpprop_parse_uuid(const struct ofpbuf *, struct uuid *);
 enum ofperr ofpprop_parse_nested(const struct ofpbuf *, struct ofpbuf *);
 
@@ -98,10 +101,12 @@ void *ofpprop_put_zeros(struct ofpbuf *, uint64_t type, 
size_t len);
 void ofpprop_put_be16(struct ofpbuf *, uint64_t type, ovs_be16 value);
 void ofpprop_put_be32(struct ofpbuf *, uint64_t type, ovs_be32 value);
 void ofpprop_put_be64(struct ofpbuf *, uint64_t type, ovs_be64 value);
+void ofpprop_put_be128(struct ofpbuf *, uint64_t type, ovs_be128 value);
 void ofpprop_put_u8(struct ofpbuf *, uint64_t type, uint8_t value);
 void ofpprop_put_u16(struct ofpbuf *, uint64_t type, uint16_t value);
 void ofpprop_put_u32(struct ofpbuf *, uint64_t type, uint32_t value);
 void ofpprop_put_u64(struct ofpbuf *, uint64_t type, uint64_t value);
+void ofpprop_put_u128(struct ofpbuf *, uint64_t type, ovs_u128 value);
 void ofpprop_put_bitmap(struct ofpbuf *, uint64_t type, uint64_t bitmap);
 void ofpprop_put_flag(struct ofpbuf *, uint64_t type);
 void ofpprop_put_uuid(struct ofpbuf *, uint64_t type, const struct uuid *);
diff --git a/lib/ofp-prop.c b/lib/ofp-prop.c
index 8b2d8a85a..2e323a08e 100644
--- a/lib/ofp-prop.c
+++ b/lib/ofp-prop.c
@@ -184,6 +184,20 @@ ofpprop_parse_be64(const struct ofpbuf *property, ovs_be64 
*value)
 return 0;
 }
 
+/* Attempts to parse 'property' as a property containing a 128-bit value.  If
+ * successful, stores the value into '*value' and returns 0; otherwise returns
+ * an OpenFlow error. */
+enum ofperr
+ofpprop_parse_be128(const struct ofpbuf *property, ovs_be128 *value)
+{
+ovs_be128 *p = property->msg;
+if (ofpbuf_msgsize(property) != sizeof *p) {
+return OFPERR_OFPBPC_BAD_LEN;
+}
+*value = *p;
+return 0;
+}
+
 /* Attempts to parse 'property' as a property containing a 8-bit value.  If
  * successful, stores the value into '*value' and returns 0; otherwise returns
  * an OpenFlow error. */
@@ -250,6 +264,20 @@ ofpprop_parse_u64(const struct ofpbuf *property, uint64_t 
*value)
 return 0;
 }
 
+/* Attempts to parse 'property' as a property containing a 128-bit value.  If
+ * successful, stores the value into '*value' and returns 0; otherwise returns
+ * an OpenFlow error. */
+enum ofperr
+ofpprop_parse_u128(const struct ofpbuf *property, ovs_u128 *value)
+{
+ovs_be128 *p = property->msg;
+if (ofpbuf_msgsize(property) != sizeof *p) {
+return OFPERR_OFPBPC_BAD_LEN;
+}
+*value = ntoh128(*p);
+return 0;
+}
+
 /* Attempts to parse 'property' as a property containing a UUID.  If
  * successful, stores the value into '*uuid' and returns 0; otherwise returns
  * an OpenFlow error. */
@@ -351,6 +379,13 @@ ofpprop_put_be64(struct ofpbuf *msg, uint64_t type, 
ovs_be64 value)
 ofpprop_end(msg, start);
 }
 
+/* Adds a property with the given 'type' and 128-bit 'value' to 'msg'. */
+void
+ofpprop_put_be128(struct ofpbuf *msg, uint64_t type, ovs_be128 value)
+{
+ofpprop_put(msg, type, , sizeof value);
+}
+
 /* Adds a property with the given 'type' and 8-bit 'value' to 'msg'. */
 void
 ofpprop_put_u8(struct ofpbuf *msg, uint64_t type, uint8_t value)
@@ -381,6 +416,13 @@ ofpprop_put_u64(struct ofpbuf *msg, uint64_t type, 
uint64_t value)
 ofpprop_put_be64(msg, type, htonll(value));
 }
 
+/* Adds a property with the given 'type' and 64-bit 'value' to 'msg'. */
+void
+ofpprop_put_u128(struct ofpbuf *msg, uint64_t type, ovs_u128 value)
+{
+ofpprop_put_be128(msg, type, hton128(value));
+}
+
 /* Appends a property to 'msg' whose type is 'type' and whose contents is a
  * series of 

Re: [ovs-dev] [PATCH v2] mcast-snooping: Store IGMP/MLD protocol version.

[Eelco Chaudron]

On 16 Nov 2023, at 15:08, Mohammad Heib wrote:

> Store the igmp/mld protocol version into the
> mcast_group internally.
>
> This can be used by ovs consumers to update
> about the igmp/mld version of each group.
>
> Signed-off-by: Mohammad Heib 

Hi Mohammad,

Thanks for the patch, I have not reviewed the actual code yet, but it would be 
good to include a use case for this patch (maybe expand this to a series). This 
way it’s clear why we need to store this information.

Cheers,

Eelco

> ---
>  lib/mcast-snooping.c | 16 +---
>  lib/mcast-snooping.h |  9 ++---
>  ofproto/ofproto-dpif-xlate.c |  6 --
>  3 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> index 029ca2855..926daf9ac 100644
> --- a/lib/mcast-snooping.c
> +++ b/lib/mcast-snooping.c
> @@ -389,7 +389,7 @@ mcast_snooping_prune_expired(struct mcast_snooping *ms,
>  bool
>  mcast_snooping_add_group(struct mcast_snooping *ms,
>   const struct in6_addr *addr,
> - uint16_t vlan, void *port)
> + uint16_t vlan, void *port, uint8_t grp_proto)
>  OVS_REQ_WRLOCK(ms->rwlock)
>  {
>  bool learned;
> @@ -415,6 +415,7 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
>  hmap_insert(>table, >hmap_node, hash);
>  grp->addr = *addr;
>  grp->vlan = vlan;
> +grp->protocol_version = grp_proto;
>  ovs_list_init(>bundle_lru);
>  learned = true;
>  ms->need_revalidate = true;
> @@ -431,17 +432,17 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
>
>  bool
>  mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
> - uint16_t vlan, void *port)
> + uint16_t vlan, void *port, uint8_t grp_proto)
>  OVS_REQ_WRLOCK(ms->rwlock)
>  {
>  struct in6_addr addr = in6_addr_mapped_ipv4(ip4);
> -return mcast_snooping_add_group(ms, , vlan, port);
> +return mcast_snooping_add_group(ms, , vlan, port, grp_proto);
>  }
>
>  int
>  mcast_snooping_add_report(struct mcast_snooping *ms,
>const struct dp_packet *p,
> -  uint16_t vlan, void *port)
> +  uint16_t vlan, void *port, uint8_t grp_proto)
>  {
>  ovs_be32 ip4;
>  size_t offset;
> @@ -478,7 +479,7 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
>  || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
>  ret = mcast_snooping_leave_group4(ms, ip4, vlan, port);
>  } else {
> -ret = mcast_snooping_add_group4(ms, ip4, vlan, port);
> +ret = mcast_snooping_add_group4(ms, ip4, vlan, port, grp_proto);
>  }
>  if (ret) {
>  count++;
> @@ -513,7 +514,7 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
>
>  switch (mld->type) {
>  case MLD_REPORT:
> -ret = mcast_snooping_add_group(ms, addr, vlan, port);
> +ret = mcast_snooping_add_group(ms, addr, vlan, port, MLD_REPORT);
>  if (ret) {
>  count++;
>  }
> @@ -545,7 +546,8 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
>  || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
>  ret = mcast_snooping_leave_group(ms, addr, vlan, port);
>  } else {
> -ret = mcast_snooping_add_group(ms, addr, vlan, port);
> +ret = mcast_snooping_add_group(ms, addr, vlan, port,
> +   MLD2_REPORT);
>  }
>  if (ret) {
>  count++;
> diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
> index f120405da..6321b63ab 100644
> --- a/lib/mcast-snooping.h
> +++ b/lib/mcast-snooping.h
> @@ -51,6 +51,9 @@ struct mcast_group {
>  /* VLAN tag. */
>  uint16_t vlan;
>
> +/* Multicast group IPv6/IPv4 Protocol version IGMPv1,2,3 or MLDv1,2 */
> +uint8_t protocol_version;
> +
>  /* Node in parent struct mcast_snooping group_lru. */
>  struct ovs_list group_node OVS_GUARDED;
>
> @@ -185,14 +188,14 @@ mcast_snooping_lookup4(const struct mcast_snooping *ms, 
> ovs_be32 ip4,
>  /* Learning. */
>  bool mcast_snooping_add_group(struct mcast_snooping *ms,
>const struct in6_addr *addr,
> -  uint16_t vlan, void *port)
> +  uint16_t vlan, void *port, uint8_t grp_proto)
>  OVS_REQ_WRLOCK(ms->rwlock);
>  bool mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
> -   uint16_t vlan, void *port)
> +   uint16_t vlan, void *port, uint8_t grp_proto)
>  OVS_REQ_WRLOCK(ms->rwlock);
>  int mcast_snooping_add_report(struct mcast_snooping *ms,
>const struct dp_packet *p,
> - 

Re: [ovs-dev] [PATCH v3 3/3] openflow: Allow CT flush to match on mark and labels.

[Ilya Maximets]

On 11/2/23 10:37, Ales Musil wrote:
> 
> 
> On Wed, Oct 25, 2023 at 12:25 AM Ilya Maximets  > wrote:
> 
> On 10/18/23 08:28, Ales Musil wrote:
> > Extend the current NX_CT_FLUSH with four additional fields,
> > that allow to match on CT entry "mark" or "labels". This
> > is encoded as separate TLV values which is backward compatible.
> > Versions that do not support them will simply ignore it.
> 
> Hmm.  Just noticed that.  This doesn't seem right.  If unknown
> property is passed, OVS should fail with OFPPROP_UNKNOWN().
> This probably should be a separate fix that we'll need to
> backport to stable versions.  If user requests flushing a
> specific label, we should not flush everything just because
> we do not understand the request.
> 
> Some more comments inline.
> 
> Best regards, Ilya Maximets.
> 
> 
> Hi Ilya,
> 
> thank you for the review. It makes sense to report unknown values, it's a bit 
> unfortunate because now we will probably need an additional feature flag to 
> indicate that this is supported WDYT?

But it's not new.  If we don't fail than the requested fields will
just be silently ignored, so the controller can't really use the
feature anyway.  You'll need a way to detect support regardless.
If we fail though, the failure itself can be used as a probe.

> I'll wait with v4 until the feature flag is resolved.
>  
> 
> 
> >
> > Extend also the ovs-dpctl and ovs-ofctl command line tools with
> > option to specify those two matching parameters for the "ct-flush"
> > command.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-55 
> 
> > Signed-off-by: Ales Musil mailto:amu...@redhat.com>>
> > ---
> > v3: Rebase on top of current master.
> > v2: Make sure that the mask decoding matches the dpctl/ovs-ofctl 
> interface.
> > ---
> >  include/openflow/nicira-ext.h |   4 +
> >  include/openvswitch/ofp-ct.h  |   9 +-
> >  lib/ct-dpif.c                 |  12 ++-
> >  lib/dpctl.c                   |   5 +-
> >  lib/ofp-ct.c                  | 151 +-
> >  tests/ofp-print.at             |  56 +
> >  tests/ovs-ofctl.at             |  32 +++
> >  tests/system-traffic.at        | 112 
> -
> >  utilities/ovs-ofctl.8.in       |  13 +--
> >  utilities/ovs-ofctl.c         |   5 +-
> >  10 files changed, 344 insertions(+), 55 deletions(-)
> 
> The change needs a NEWS entry.
> 
> >
> > diff --git a/include/openflow/nicira-ext.h 
> b/include/openflow/nicira-ext.h
> > index 768775898..959845ce6 100644
> > --- a/include/openflow/nicira-ext.h
> > +++ b/include/openflow/nicira-ext.h
> > @@ -1075,6 +1075,10 @@ enum nx_ct_flush_tlv_type {
> >                                  * by 'enum 
> nx_ct_flush_tuple_tlv_type'*/
> >      /* Primitive types. */
> >      NXT_CT_ZONE_ID = 2,        /* be16 zone id. */
> > +    NXT_CT_MARK = 3,           /* be32 mark. */
> > +    NXT_CT_MARK_MASK = 4,      /* be32 mark mask. */
> > +    NXT_CT_LABELS = 5,         /* be128 labels. */
> > +    NXT_CT_LABELS_MASK = 6,    /* be128 labels mask. */
> >  };
> > 
> >  /* CT flush nested TLVs. */
> > diff --git a/include/openvswitch/ofp-ct.h b/include/openvswitch/ofp-ct.h
> > index cd6192e6f..d57b62678 100644
> > --- a/include/openvswitch/ofp-ct.h
> > +++ b/include/openvswitch/ofp-ct.h
> > @@ -51,11 +51,16 @@ struct ofp_ct_match {
> > 
> >      struct ofp_ct_tuple tuple_orig;
> >      struct ofp_ct_tuple tuple_reply;
> > +
> > +    uint32_t mark;
> > +    uint32_t mark_mask;
> > +
> > +    ovs_u128 labels;
> > +    ovs_u128 labels_mask;
> >  };
> > 
> >  bool ofp_ct_match_is_zero(const struct ofp_ct_match *);
> > -bool ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *, uint8_t 
> ip_proto);
> > -bool ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *, uint8_t 
> ip_proto);
> > +bool ofp_ct_match_is_five_tuple(const struct ofp_ct_match *);
> > 
> >  void ofp_ct_match_format(struct ds *, const struct ofp_ct_match *);
> >  bool ofp_ct_match_parse(const char **, int argc, struct ds *,
> > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> > index f59c6e560..0fd14b99f 100644
> > --- a/lib/ct-dpif.c
> > +++ b/lib/ct-dpif.c
> > @@ -269,6 +269,15 @@ ct_dpif_entry_cmp(const struct ct_dpif_entry 
> *entry,
> >          return false;
> >      }
> > 
> > +    if ((match->mark & match->mark_mask) != (entry->mark & 
> match->mark_mask)) {
> > +        return false;
> > +    }
> > +
> > +    if (!ovs_u128_equals(ovs_u128_and(match->labels, 
> match->labels_mask),
> > +         

Re: [ovs-dev] [PATCH] mcast-snooping: store IGMP/MLD protocol version

[0-day Robot]

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

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


checkpatch:
WARNING: The subject summary should start with a capital.
WARNING: The subject summary should end with a dot.
Subject: mcast-snooping: store IGMP/MLD protocol version
WARNING: Line is 86 characters long (recommended limit is 79)
#84 FILE: lib/mcast-snooping.c:549:
ret = mcast_snooping_add_group(ms, addr, vlan, port, 
MLD2_REPORT);

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


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

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

[ovs-dev] [PATCH v2] mcast-snooping: Store IGMP/MLD protocol version.

[Mohammad Heib]

Store the igmp/mld protocol version into the
mcast_group internally.

This can be used by ovs consumers to update
about the igmp/mld version of each group.

Signed-off-by: Mohammad Heib 
---
 lib/mcast-snooping.c | 16 +---
 lib/mcast-snooping.h |  9 ++---
 ofproto/ofproto-dpif-xlate.c |  6 --
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
index 029ca2855..926daf9ac 100644
--- a/lib/mcast-snooping.c
+++ b/lib/mcast-snooping.c
@@ -389,7 +389,7 @@ mcast_snooping_prune_expired(struct mcast_snooping *ms,
 bool
 mcast_snooping_add_group(struct mcast_snooping *ms,
  const struct in6_addr *addr,
- uint16_t vlan, void *port)
+ uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock)
 {
 bool learned;
@@ -415,6 +415,7 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
 hmap_insert(>table, >hmap_node, hash);
 grp->addr = *addr;
 grp->vlan = vlan;
+grp->protocol_version = grp_proto;
 ovs_list_init(>bundle_lru);
 learned = true;
 ms->need_revalidate = true;
@@ -431,17 +432,17 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
 
 bool
 mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
- uint16_t vlan, void *port)
+ uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock)
 {
 struct in6_addr addr = in6_addr_mapped_ipv4(ip4);
-return mcast_snooping_add_group(ms, , vlan, port);
+return mcast_snooping_add_group(ms, , vlan, port, grp_proto);
 }
 
 int
 mcast_snooping_add_report(struct mcast_snooping *ms,
   const struct dp_packet *p,
-  uint16_t vlan, void *port)
+  uint16_t vlan, void *port, uint8_t grp_proto)
 {
 ovs_be32 ip4;
 size_t offset;
@@ -478,7 +479,7 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
 || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
 ret = mcast_snooping_leave_group4(ms, ip4, vlan, port);
 } else {
-ret = mcast_snooping_add_group4(ms, ip4, vlan, port);
+ret = mcast_snooping_add_group4(ms, ip4, vlan, port, grp_proto);
 }
 if (ret) {
 count++;
@@ -513,7 +514,7 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
 
 switch (mld->type) {
 case MLD_REPORT:
-ret = mcast_snooping_add_group(ms, addr, vlan, port);
+ret = mcast_snooping_add_group(ms, addr, vlan, port, MLD_REPORT);
 if (ret) {
 count++;
 }
@@ -545,7 +546,8 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
 || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
 ret = mcast_snooping_leave_group(ms, addr, vlan, port);
 } else {
-ret = mcast_snooping_add_group(ms, addr, vlan, port);
+ret = mcast_snooping_add_group(ms, addr, vlan, port,
+   MLD2_REPORT);
 }
 if (ret) {
 count++;
diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
index f120405da..6321b63ab 100644
--- a/lib/mcast-snooping.h
+++ b/lib/mcast-snooping.h
@@ -51,6 +51,9 @@ struct mcast_group {
 /* VLAN tag. */
 uint16_t vlan;
 
+/* Multicast group IPv6/IPv4 Protocol version IGMPv1,2,3 or MLDv1,2 */
+uint8_t protocol_version;
+
 /* Node in parent struct mcast_snooping group_lru. */
 struct ovs_list group_node OVS_GUARDED;
 
@@ -185,14 +188,14 @@ mcast_snooping_lookup4(const struct mcast_snooping *ms, 
ovs_be32 ip4,
 /* Learning. */
 bool mcast_snooping_add_group(struct mcast_snooping *ms,
   const struct in6_addr *addr,
-  uint16_t vlan, void *port)
+  uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock);
 bool mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
-   uint16_t vlan, void *port)
+   uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock);
 int mcast_snooping_add_report(struct mcast_snooping *ms,
   const struct dp_packet *p,
-  uint16_t vlan, void *port)
+  uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock);
 int mcast_snooping_add_mld(struct mcast_snooping *ms,
const struct dp_packet *p,
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e24377330..26bd678cd 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2781,7 +2781,8 @@ update_mcast_snooping_table4__(const struct 

[ovs-dev] [PATCH] mcast-snooping: store IGMP/MLD protocol version

[Mohammad Heib]

Store the igmp/mld protocol version into the
mcast_group internally.

This can be used by ovs consumers to update
about the igmp/mld version of each group.

Signed-off-by: Mohammad Heib 
---
 lib/mcast-snooping.c | 15 ---
 lib/mcast-snooping.h |  9 ++---
 ofproto/ofproto-dpif-xlate.c |  6 --
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
index 029ca2855..185723861 100644
--- a/lib/mcast-snooping.c
+++ b/lib/mcast-snooping.c
@@ -389,7 +389,7 @@ mcast_snooping_prune_expired(struct mcast_snooping *ms,
 bool
 mcast_snooping_add_group(struct mcast_snooping *ms,
  const struct in6_addr *addr,
- uint16_t vlan, void *port)
+ uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock)
 {
 bool learned;
@@ -415,6 +415,7 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
 hmap_insert(>table, >hmap_node, hash);
 grp->addr = *addr;
 grp->vlan = vlan;
+grp->protocol_version = grp_proto;
 ovs_list_init(>bundle_lru);
 learned = true;
 ms->need_revalidate = true;
@@ -431,17 +432,17 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
 
 bool
 mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
- uint16_t vlan, void *port)
+ uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock)
 {
 struct in6_addr addr = in6_addr_mapped_ipv4(ip4);
-return mcast_snooping_add_group(ms, , vlan, port);
+return mcast_snooping_add_group(ms, , vlan, port, grp_proto);
 }
 
 int
 mcast_snooping_add_report(struct mcast_snooping *ms,
   const struct dp_packet *p,
-  uint16_t vlan, void *port)
+  uint16_t vlan, void *port, uint8_t grp_proto)
 {
 ovs_be32 ip4;
 size_t offset;
@@ -478,7 +479,7 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
 || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
 ret = mcast_snooping_leave_group4(ms, ip4, vlan, port);
 } else {
-ret = mcast_snooping_add_group4(ms, ip4, vlan, port);
+ret = mcast_snooping_add_group4(ms, ip4, vlan, port, grp_proto);
 }
 if (ret) {
 count++;
@@ -513,7 +514,7 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
 
 switch (mld->type) {
 case MLD_REPORT:
-ret = mcast_snooping_add_group(ms, addr, vlan, port);
+ret = mcast_snooping_add_group(ms, addr, vlan, port, MLD_REPORT);
 if (ret) {
 count++;
 }
@@ -545,7 +546,7 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
 || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
 ret = mcast_snooping_leave_group(ms, addr, vlan, port);
 } else {
-ret = mcast_snooping_add_group(ms, addr, vlan, port);
+ret = mcast_snooping_add_group(ms, addr, vlan, port, 
MLD2_REPORT);
 }
 if (ret) {
 count++;
diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
index f120405da..6321b63ab 100644
--- a/lib/mcast-snooping.h
+++ b/lib/mcast-snooping.h
@@ -51,6 +51,9 @@ struct mcast_group {
 /* VLAN tag. */
 uint16_t vlan;
 
+/* Multicast group IPv6/IPv4 Protocol version IGMPv1,2,3 or MLDv1,2 */
+uint8_t protocol_version;
+
 /* Node in parent struct mcast_snooping group_lru. */
 struct ovs_list group_node OVS_GUARDED;
 
@@ -185,14 +188,14 @@ mcast_snooping_lookup4(const struct mcast_snooping *ms, 
ovs_be32 ip4,
 /* Learning. */
 bool mcast_snooping_add_group(struct mcast_snooping *ms,
   const struct in6_addr *addr,
-  uint16_t vlan, void *port)
+  uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock);
 bool mcast_snooping_add_group4(struct mcast_snooping *ms, ovs_be32 ip4,
-   uint16_t vlan, void *port)
+   uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock);
 int mcast_snooping_add_report(struct mcast_snooping *ms,
   const struct dp_packet *p,
-  uint16_t vlan, void *port)
+  uint16_t vlan, void *port, uint8_t grp_proto)
 OVS_REQ_WRLOCK(ms->rwlock);
 int mcast_snooping_add_mld(struct mcast_snooping *ms,
const struct dp_packet *p,
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e24377330..26bd678cd 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2781,7 +2781,8 @@ update_mcast_snooping_table4__(const struct xlate_ctx 
*ctx,
 switch (ntohs(flow->tp_src)) {
 

Re: [ovs-dev] [PATCH] lib/conntrack.c:compatible with nat with no action(direction)

[Eelco Chaudron]

On 16 Nov 2023, at 11:07, Joseph Zhong wrote:

> This patch is to avoid generating incorrect conntrack entry
> In a certain use case of conntrack flow that if flow included
> ct(commit, nat) action, but no detail action/direction specified,
> CT will generate incorrect conntrack entry.
> For example, add below flow:
> ip,priority=500,in_port=1,ct_state=-trk actions=ct(table=1,nat)'
> ip,priority=500,in_port=2,ct_state=-trk actions=ct(table=1,nat)'
> table=1,in_port=1,ip,ct_state=+trk+new actions=ct*(commit,nat)*,2
> table=1,in_port=1,ip,ct_state=-new+trk+est actions=2
> table=1,in_port=2,ip,ct_state=-new+trk+est actions=1
> start traffic from 192.168.2.2 to 192.168.2.7
> ovs dpdk datpath generate CT entry as below:
> icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
> reply=(src=*0.0.0.0*,dst=192.168.2.2,id=17038,type=0,code=0)
> reply key src 0.0.0.0 is generated not correct by "nat_get_unique_tuple".
> but ovs kernel datapath will generate correct ct entry as below:
> icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
> reply=(src=192.168.2.7,dst=192.168.2.2,id=17038,type=0,code=0)
>
> To compatible with this use case of flow, and also be consistent with
> kernel datapath's behavior, this patch treat this nat without action
> specified as not nat, and don't do "nat_get_unique_tuple" and malloc
> a nat_conn that is attached to nc.

Hi Joseph,

Thanks for the patch, I’m not a conntrack expert so I’ll let Aaron, or Paolo 
review it. But would it be possible to have a test case for this?

Cheers,

Eelco


> Signed-off-by: Zhong Zhong 
>
> ---
>  lib/conntrack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 47a443f..581b62b 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -942,7 +942,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
>  nc->parent_key = alg_exp->parent_key;
>  }
> - if (nat_action_info) {
> + if (nat_action_info && nat_action_info->nat_action) {
>  nc->nat_action = nat_action_info->nat_action;
>  if (alg_exp) {
> --
>
> -- 
> Best Regards
>
> Zhong, Zhong
> Email: zhongzh...@gmail.com
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Re: [ovs-dev] [PATCH] lib/conntrack.c:compatible with nat with no action(direction)

[0-day Robot]

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

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


git-am:
error: corrupt patch at line 10
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 lib/conntrack.c:compatible with nat with no 
action(direction)
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Patch skipped due to previous failure.

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

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

Re: [ovs-dev] [PATCH ovn] tests: Fix race in "feature inactivity probe" test.

[Dumitru Ceara]

On 11/16/23 12:59, Dumitru Ceara wrote:
> On 11/16/23 12:49, Xavier Simonart wrote:
>> Hi Dumitru
>>
> 
> Hi Xavier,
> 
>> Thanks for the patch. It clearly decreases the failure count.
>> I am afraid however it does not fix all potential issues.
>> My understanding is the following:
>> The test goal was to ensure ovs does not disconnect the connections on
>> inactivity probe.There are 4 connections with inactivity probe (3 in
>> earlier OVN versions):
>> - feature
>> - pinctrl
>> - statctrl
>> - ofctrl
>> For those connections, ovs will send ECHO REQUEST and expects
>> ovn-controller to reply with ECHO REPLY, within 60 seconds.
>> The patch you sent ensures ofctrl runs between the warp, so ensures
>> that ovn-controller replied with ECHO REPLY to the ECHO_REQUEST for
>> ofctrl.
>> However, ECHO REQUEST for pinctrl connection is handled by the pinctrl
>> thread, and nothing guarantees that pinctrl had time to run between
>> two warps, so that it handled the ECHO REQUEST within those
>> (simulated) 60 seconds.
>> Similar issue happens with statctrl.
> 
> I had missed those cases..
> 
>> Then, another issue can occur with the "feature" connection:
>> ovn-controller handles the ECHO_REQUEST for that connection in the
>> main ovn-controller loop, but only if there is an ovs idl transaction
>> available ("ovs db is not read-only").
>> So, if ovsdb is very slow (and still handling the ovn-installed-ts
>> insertion for instance), ovn-controller will not get any ovs idl
>> transaction available, and won't handle the ECHO_REQUEST for the
>> "feature" connection, or at least not in time.
>> I am not sure how to fix those issues without making the test overly complex.
>>
> 
> +1
> 
> Also, the test itself is quite artificial, I suggest we remove it all
> together.  If you agree with that, I can post v2.
> 

It's probably easier to just discuss on the patch itself.  I posted v2:
https://patchwork.ozlabs.org/project/ovn/patch/20231116135410.1259216-1-dce...@redhat.com/

Regards,
Dumitru

> Thanks,
> Dumitru
> 
>> Thanks
>> Xavier
>>
>> On Thu, Nov 16, 2023 at 10:39 AM Ales Musil  wrote:
>>>
>>> On Wed, Nov 15, 2023 at 2:40 PM Dumitru Ceara  wrote:
>>>
 The test incorrectly assumed that "ovn-nbctl --wait=hv sync" will always
 send an OpenFlow barrier to ovs-vswitchd.  That doesn't happen unless
 there are other OpenFlow (rule or group) changes that need to be
 programmed in the datapath.

 Fix the test by actually installing new rules in between warp
 invocations.

 Spotted in CI, mostly on oversubscribed systems.  A log sample that
 shows that ovn-controller didn't generate any barrier request for the
 nbctl sync request:

   2023-11-15T12:12:22.937Z|00084|vconn|DBG|unix#4: received:
 OFPT_BARRIER_REQUEST (OF1.5) (xid=0x13):
   2023-11-15T12:12:22.937Z|00085|vconn|DBG|unix#4: sent (Success):
 OFPT_BARRIER_REPLY (OF1.5) (xid=0x13):
   ...
   2023-11-15T12:12:23.032Z|00090|unixctl|DBG|received request
 time/warp["6"], id=0
   2023-11-15T12:12:23.032Z|00091|unixctl|DBG|replying with success, id=0:
 "warped"
   2023-11-15T12:12:23.042Z|00094|vconn|DBG|unix#3: sent (Success):
 OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
   2023-11-15T12:12:23.042Z|00095|vconn|DBG|unix#4: sent (Success):
 OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
   2023-11-15T12:12:23.042Z|00097|vconn|DBG|unix#5: sent (Success):
 OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
   2023-11-15T12:12:23.042Z|00098|unixctl|DBG|received request
 time/warp["6"], id=0
   2023-11-15T12:12:23.042Z|00099|unixctl|DBG|replying with success, id=0:
 "warped"
   2023-11-15T12:12:23.052Z|00100|rconn|ERR|br-int<->unix#3: no response to
 inactivity probe after 60 seconds, disconnecting
   2023-11-15T12:12:23.052Z|00101|rconn|ERR|br-int<->unix#4: no response to
 inactivity probe after 60 seconds, disconnecting
   2023-11-15T12:12:23.052Z|00102|rconn|ERR|br-int<->unix#5: no response to
 inactivity probe after 60 seconds, disconnecting
   2023-11-15T12:12:23.052Z|00103|unixctl|DBG|received request
 time/warp["6"], id=0

 Fixes: ff00142808dc ("controller: Fixed ovs/ovn(features) connection lost
 when running more than 120 seconds")
 Signed-off-by: Dumitru Ceara 
 ---
  tests/ovn.at | 15 +++
  1 file changed, 15 insertions(+)

 diff --git a/tests/ovn.at b/tests/ovn.at
 index b8c61f87fb..face39e5c2 100644
 --- a/tests/ovn.at
 +++ b/tests/ovn.at
 @@ -35352,20 +35352,35 @@ as hv1
  check ovs-vsctl add-br br-phys
  ovn_attach n1 br-phys 192.168.0.1

 +check ovn-nbctl ls-add ls
 +
  dnl Ensure that there are 4 openflow connections.
  OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version'
 hv1/ovs-vswitchd.log)" -eq "4"])

 +as hv1 check ovs-vsctl -- add-port br-int lsp0   \

[ovs-dev] [PATCH ovn v2] tests: Remove broken "feature inactivity probe" test.

[Dumitru Ceara]

The test incorrectly assumed that "ovn-nbctl --wait=hv sync" will always
send an OpenFlow barrier to ovs-vswitchd.  That doesn't happen unless
there are other OpenFlow (rule or group) changes that need to be
programmed in the datapath.

An initial attempt to fix this [0] uncovered the fact that there's no
easy way to cover all possible scenarios [1].  It seems that the effort
to fix the test is not justified for the case it tests for (that
inactivity probes are handled by the features module - that is quite
obviously handled in the code).  It's therefore more reasonable to just
skip the test (to avoid noise in CI).

Spotted in CI, mostly on oversubscribed systems.  A log sample that
shows that ovn-controller didn't generate any barrier request for the
nbctl sync request:

  2023-11-15T12:12:22.937Z|00084|vconn|DBG|unix#4: received: 
OFPT_BARRIER_REQUEST (OF1.5) (xid=0x13):
  2023-11-15T12:12:22.937Z|00085|vconn|DBG|unix#4: sent (Success): 
OFPT_BARRIER_REPLY (OF1.5) (xid=0x13):
  ...
  2023-11-15T12:12:23.032Z|00090|unixctl|DBG|received request 
time/warp["6"], id=0
  2023-11-15T12:12:23.032Z|00091|unixctl|DBG|replying with success, id=0: 
"warped"
  2023-11-15T12:12:23.042Z|00094|vconn|DBG|unix#3: sent (Success): 
OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
  2023-11-15T12:12:23.042Z|00095|vconn|DBG|unix#4: sent (Success): 
OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
  2023-11-15T12:12:23.042Z|00097|vconn|DBG|unix#5: sent (Success): 
OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
  2023-11-15T12:12:23.042Z|00098|unixctl|DBG|received request 
time/warp["6"], id=0
  2023-11-15T12:12:23.042Z|00099|unixctl|DBG|replying with success, id=0: 
"warped"
  2023-11-15T12:12:23.052Z|00100|rconn|ERR|br-int<->unix#3: no response to 
inactivity probe after 60 seconds, disconnecting
  2023-11-15T12:12:23.052Z|00101|rconn|ERR|br-int<->unix#4: no response to 
inactivity probe after 60 seconds, disconnecting
  2023-11-15T12:12:23.052Z|00102|rconn|ERR|br-int<->unix#5: no response to 
inactivity probe after 60 seconds, disconnecting
  2023-11-15T12:12:23.052Z|00103|unixctl|DBG|received request 
time/warp["6"], id=0

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409473.html
[1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409530.html

Fixes: ff00142808dc ("controller: Fixed ovs/ovn(features) connection lost when 
running more than 120 seconds")
Signed-off-by: Dumitru Ceara 
---
V2:
- removed the test instead of trying to fix it (after discussion with
Xavier).
---
 tests/ovn.at | 31 ---
 1 file changed, 31 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index b8c61f87fb..198387d93e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -35342,37 +35342,6 @@ OVN_CLEANUP([hv1],[hv2])
 AT_CLEANUP
 ])
 
-OVN_FOR_EACH_NORTHD([
-AT_SETUP([feature inactivity probe])
-ovn_start
-net_add n1
-
-sim_add hv1
-as hv1
-check ovs-vsctl add-br br-phys
-ovn_attach n1 br-phys 192.168.0.1
-
-dnl Ensure that there are 4 openflow connections.
-OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version' 
hv1/ovs-vswitchd.log)" -eq "4"])
-
-dnl "Wait" 3 times 60 seconds and ensure ovn-controller writes to the
-dnl openflow connections in the meantime.  This should allow ovs-vswitchd
-dnl to probe the openflow connections at least twice.
-
-as hv1 ovs-appctl time/warp 6
-check ovn-nbctl --wait=hv sync
-
-as hv1 ovs-appctl time/warp 6
-check ovn-nbctl --wait=hv sync
-
-as hv1 ovs-appctl time/warp 6
-check ovn-nbctl --wait=hv sync
-
-AT_CHECK([test -z "`grep disconnecting hv1/ovs-vswitchd.log`"])
-OVN_CLEANUP([hv1])
-AT_CLEANUP
-])
-
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([Logical flows with Chassis_Template_Var references])
 AT_KEYWORDS([templates])
-- 
2.39.3

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

[ovs-dev] [PATCH] lib/conntrack.c:compatible with nat with no action(direction)

[Joseph Zhong]

This patch is to avoid generating incorrect conntrack entry
In a certain use case of conntrack flow that if flow included
ct(commit, nat) action, but no detail action/direction specified,
CT will generate incorrect conntrack entry.
For example, add below flow:
ip,priority=500,in_port=1,ct_state=-trk actions=ct(table=1,nat)'
ip,priority=500,in_port=2,ct_state=-trk actions=ct(table=1,nat)'
table=1,in_port=1,ip,ct_state=+trk+new actions=ct*(commit,nat)*,2
table=1,in_port=1,ip,ct_state=-new+trk+est actions=2
table=1,in_port=2,ip,ct_state=-new+trk+est actions=1
start traffic from 192.168.2.2 to 192.168.2.7
ovs dpdk datpath generate CT entry as below:
icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
reply=(src=*0.0.0.0*,dst=192.168.2.2,id=17038,type=0,code=0)
reply key src 0.0.0.0 is generated not correct by "nat_get_unique_tuple".
but ovs kernel datapath will generate correct ct entry as below:
icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
reply=(src=192.168.2.7,dst=192.168.2.2,id=17038,type=0,code=0)

To compatible with this use case of flow, and also be consistent with
kernel datapath's behavior, this patch treat this nat without action
specified as not nat, and don't do "nat_get_unique_tuple" and malloc
a nat_conn that is attached to nc.

Signed-off-by: Zhong Zhong 

---
 lib/conntrack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 47a443f..581b62b 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -942,7 +942,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
 nc->parent_key = alg_exp->parent_key;
 }
- if (nat_action_info) {
+ if (nat_action_info && nat_action_info->nat_action) {
 nc->nat_action = nat_action_info->nat_action;
 if (alg_exp) {
--

-- 
Best Regards

Zhong, Zhong
Email: zhongzh...@gmail.com
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Dumitru Ceara]

On 11/16/23 13:44, Ales Musil wrote:
> On Thu, Nov 16, 2023 at 1:40 PM Dumitru Ceara  wrote:
> 
>> On 11/16/23 13:05, Ilya Maximets wrote:
>>> On 11/16/23 10:17, Dumitru Ceara wrote:
 On 11/16/23 10:02, Dumitru Ceara wrote:
> On 11/16/23 09:51, Dumitru Ceara wrote:
>> On 11/16/23 09:44, Ales Musil wrote:
>>> On Thu, Nov 16, 2023 at 9:34 AM Dumitru Ceara 
>> wrote:
>>>
 On 11/16/23 00:12, Ilya Maximets wrote:
> On 11/15/23 22:20, Mark Michelson wrote:
>> On 11/15/23 12:02, Ilya Maximets wrote:
>>> On 11/15/23 17:00, Dumitru Ceara wrote:
 All of the above were changed to track the latest available
>> releases.
 Initially that seemed like a good idea but in practice, a new
>> release
 would
 potentially (silently) cause CI runs that used to pass on given
>> stable
 versions to unexpectedly start failing.

 To address that this series pins all versions we can control
>> allowing
 us
 to use different values for different branches.

 NOTE: the last commit of this series will look slightly
>> different on
 stable branches, e.g., on branches <= 23.06 we need Python <=
>> 3.11 and
 Fedora 37.

 The first commit of the series bumps the OVS submodule to
>> include the
 latest fixes to errors reported by recent versions of flake8.

 Changes in v2:
 - added first patch to bump OVS submodule
 - addressed Ales' review comments:
- moved the logic to determine which image to use out of
>> ci.sh;
  it's now in the workflow itself.
- moved setting of OVN_IMAGE in the same block with all
>> related
  env vars
 - added a note for maintainers in the release process
>> documentation
to mention that the new workflow will likely have to be
>> triggered
manually (at least once) when branching for a new release.
>> GitHub
doesn't allow periodic jobs on non-default branches.
>>>
>>> IMHO, that sounds horrible to maintain.  Can we just build
>> things per
 run?
>>
>> What if we set up the "Containers" action to run on branch
>> creation?
>> Would that adequately substitute for the manual run that creates
>> the
 image?
>>
>> If we only have to run the action once each time a branch is
>> created, I
>> don't think that qualifies as "horrible" to maintain. This could
>> be
>> lumped into our branch creation scripts. However, if it can be
 automated
>> as I suggested above, that would be better.
>
> The job will never be re-run, meaning we'll be using the same
>> container
> for many years.  In case we'll need an update for this container,
>> it will
> need to be done manually for each branch, except for main.
>
> Also, in the patch, the Cirrus CI is set to use the container from
>> main,
> so this will need to be adjusted for each branch, will need to
>> become
> part of the release patches (?).
>
> Another issue with current implementation of containers is that
>> it's
> a pain for external contributor to change dependencies, because
>> they'll
> have to modify the CI in multiple places in order to build and use
>> their
> own containers.
> And with these new changes it will become worse.  For example, I
>> never
 use
> 'main' or 'branch-*' branches in my own forks.  And CI depends on
>> the
 branch
> name now, and it will fall back to an unmaintained old-style-named
>> image.
> Might not be easy to figure out why all your builds are failing,
 especially
> if you're new to the project.
>

 We could try to make it build the container image every time on
>> stable
 branches and avoid any manual steps.
>>>
>>> But the branch name will still be a problem.  Every time people use any
>>> branch name other than 'main' for their own development, they'll get an
>>> outdated container, unless they change the CI scripts.  Or am I missing
>>> something?
>>>
>>
>> Yeah, we'd have to make it build the container image every time on
>> non-default branches.  That's a problem because the fedora container
>> takes so long.  So we'd have to separate the two types of container
>> builds.  Seems like a lot of trouble to go through indeed.
>>

>>
>>> How much value these containers actually have?

 I think you mentioned the benefit lower already:
 - we can share the artifact (container image) with Cirrus CI;
>> aarch64
 container build takes significantly longer
 - we can share the artifact 

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Ales Musil]

On Thu, Nov 16, 2023 at 1:40 PM Dumitru Ceara  wrote:

> On 11/16/23 13:05, Ilya Maximets wrote:
> > On 11/16/23 10:17, Dumitru Ceara wrote:
> >> On 11/16/23 10:02, Dumitru Ceara wrote:
> >>> On 11/16/23 09:51, Dumitru Ceara wrote:
>  On 11/16/23 09:44, Ales Musil wrote:
> > On Thu, Nov 16, 2023 at 9:34 AM Dumitru Ceara 
> wrote:
> >
> >> On 11/16/23 00:12, Ilya Maximets wrote:
> >>> On 11/15/23 22:20, Mark Michelson wrote:
>  On 11/15/23 12:02, Ilya Maximets wrote:
> > On 11/15/23 17:00, Dumitru Ceara wrote:
> >> All of the above were changed to track the latest available
> releases.
> >> Initially that seemed like a good idea but in practice, a new
> release
> >> would
> >> potentially (silently) cause CI runs that used to pass on given
> stable
> >> versions to unexpectedly start failing.
> >>
> >> To address that this series pins all versions we can control
> allowing
> >> us
> >> to use different values for different branches.
> >>
> >> NOTE: the last commit of this series will look slightly
> different on
> >> stable branches, e.g., on branches <= 23.06 we need Python <=
> 3.11 and
> >> Fedora 37.
> >>
> >> The first commit of the series bumps the OVS submodule to
> include the
> >> latest fixes to errors reported by recent versions of flake8.
> >>
> >> Changes in v2:
> >> - added first patch to bump OVS submodule
> >> - addressed Ales' review comments:
> >>- moved the logic to determine which image to use out of
> ci.sh;
> >>  it's now in the workflow itself.
> >>- moved setting of OVN_IMAGE in the same block with all
> related
> >>  env vars
> >> - added a note for maintainers in the release process
> documentation
> >>to mention that the new workflow will likely have to be
> triggered
> >>manually (at least once) when branching for a new release.
> GitHub
> >>doesn't allow periodic jobs on non-default branches.
> >
> > IMHO, that sounds horrible to maintain.  Can we just build
> things per
> >> run?
> 
>  What if we set up the "Containers" action to run on branch
> creation?
>  Would that adequately substitute for the manual run that creates
> the
> >> image?
> 
>  If we only have to run the action once each time a branch is
> created, I
>  don't think that qualifies as "horrible" to maintain. This could
> be
>  lumped into our branch creation scripts. However, if it can be
> >> automated
>  as I suggested above, that would be better.
> >>>
> >>> The job will never be re-run, meaning we'll be using the same
> container
> >>> for many years.  In case we'll need an update for this container,
> it will
> >>> need to be done manually for each branch, except for main.
> >>>
> >>> Also, in the patch, the Cirrus CI is set to use the container from
> main,
> >>> so this will need to be adjusted for each branch, will need to
> become
> >>> part of the release patches (?).
> >>>
> >>> Another issue with current implementation of containers is that
> it's
> >>> a pain for external contributor to change dependencies, because
> they'll
> >>> have to modify the CI in multiple places in order to build and use
> their
> >>> own containers.
> >>> And with these new changes it will become worse.  For example, I
> never
> >> use
> >>> 'main' or 'branch-*' branches in my own forks.  And CI depends on
> the
> >> branch
> >>> name now, and it will fall back to an unmaintained old-style-named
> image.
> >>> Might not be easy to figure out why all your builds are failing,
> >> especially
> >>> if you're new to the project.
> >>>
> >>
> >> We could try to make it build the container image every time on
> stable
> >> branches and avoid any manual steps.
> >
> > But the branch name will still be a problem.  Every time people use any
> > branch name other than 'main' for their own development, they'll get an
> > outdated container, unless they change the CI scripts.  Or am I missing
> > something?
> >
>
> Yeah, we'd have to make it build the container image every time on
> non-default branches.  That's a problem because the fedora container
> takes so long.  So we'd have to separate the two types of container
> builds.  Seems like a lot of trouble to go through indeed.
>
> >>
> 
> > How much value these containers actually have?
> >>
> >> I think you mentioned the benefit lower already:
> >> - we can share the artifact (container image) with Cirrus CI;
> aarch64
> >> container build takes significantly longer
> >> - we can share the artifact with downstream CIs
> >>
> 
>  As patch 2 mentions, it allows 

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Dumitru Ceara]

On 11/16/23 13:05, Ilya Maximets wrote:
> On 11/16/23 10:17, Dumitru Ceara wrote:
>> On 11/16/23 10:02, Dumitru Ceara wrote:
>>> On 11/16/23 09:51, Dumitru Ceara wrote:
 On 11/16/23 09:44, Ales Musil wrote:
> On Thu, Nov 16, 2023 at 9:34 AM Dumitru Ceara  wrote:
>
>> On 11/16/23 00:12, Ilya Maximets wrote:
>>> On 11/15/23 22:20, Mark Michelson wrote:
 On 11/15/23 12:02, Ilya Maximets wrote:
> On 11/15/23 17:00, Dumitru Ceara wrote:
>> All of the above were changed to track the latest available releases.
>> Initially that seemed like a good idea but in practice, a new release
>> would
>> potentially (silently) cause CI runs that used to pass on given 
>> stable
>> versions to unexpectedly start failing.
>>
>> To address that this series pins all versions we can control allowing
>> us
>> to use different values for different branches.
>>
>> NOTE: the last commit of this series will look slightly different on
>> stable branches, e.g., on branches <= 23.06 we need Python <= 3.11 
>> and
>> Fedora 37.
>>
>> The first commit of the series bumps the OVS submodule to include the
>> latest fixes to errors reported by recent versions of flake8.
>>
>> Changes in v2:
>> - added first patch to bump OVS submodule
>> - addressed Ales' review comments:
>>- moved the logic to determine which image to use out of ci.sh;
>>  it's now in the workflow itself.
>>- moved setting of OVN_IMAGE in the same block with all related
>>  env vars
>> - added a note for maintainers in the release process documentation
>>to mention that the new workflow will likely have to be triggered
>>manually (at least once) when branching for a new release.  GitHub
>>doesn't allow periodic jobs on non-default branches.
>
> IMHO, that sounds horrible to maintain.  Can we just build things per
>> run?

 What if we set up the "Containers" action to run on branch creation?
 Would that adequately substitute for the manual run that creates the
>> image?

 If we only have to run the action once each time a branch is created, I
 don't think that qualifies as "horrible" to maintain. This could be
 lumped into our branch creation scripts. However, if it can be
>> automated
 as I suggested above, that would be better.
>>>
>>> The job will never be re-run, meaning we'll be using the same container
>>> for many years.  In case we'll need an update for this container, it 
>>> will
>>> need to be done manually for each branch, except for main.
>>>
>>> Also, in the patch, the Cirrus CI is set to use the container from main,
>>> so this will need to be adjusted for each branch, will need to become
>>> part of the release patches (?).
>>>
>>> Another issue with current implementation of containers is that it's
>>> a pain for external contributor to change dependencies, because they'll
>>> have to modify the CI in multiple places in order to build and use their
>>> own containers.
>>> And with these new changes it will become worse.  For example, I never
>> use
>>> 'main' or 'branch-*' branches in my own forks.  And CI depends on the
>> branch
>>> name now, and it will fall back to an unmaintained old-style-named 
>>> image.
>>> Might not be easy to figure out why all your builds are failing,
>> especially
>>> if you're new to the project.
>>>
>>
>> We could try to make it build the container image every time on stable
>> branches and avoid any manual steps.
> 
> But the branch name will still be a problem.  Every time people use any
> branch name other than 'main' for their own development, they'll get an
> outdated container, unless they change the CI scripts.  Or am I missing
> something?
> 

Yeah, we'd have to make it build the container image every time on
non-default branches.  That's a problem because the fedora container
takes so long.  So we'd have to separate the two types of container
builds.  Seems like a lot of trouble to go through indeed.

>>

> How much value these containers actually have?
>>
>> I think you mentioned the benefit lower already:
>> - we can share the artifact (container image) with Cirrus CI; aarch64
>> container build takes significantly longer
>> - we can share the artifact with downstream CIs
>>

 As patch 2 mentions, it allows for us to use different distro bases for
 each version.
>>>
>>> This can be done by simply specifying 'container: name' in the job,
>>> GHA will fetch and use that container.
>>>
 But I also think that having the container image cached

Re: [ovs-dev] [PATCH v2 0/2] Add argument to skip committer signoff check

[Simon Horman]

On Sun, Nov 05, 2023 at 10:38:08AM +0200, Roi Dayan via dev wrote:
> Hi,
> 
> This v2 is just adding checkpatch.at case to verify the flag.

...

Thanks Roi,

I have applied this patch-set.

- checkpatch: Add argument to skip committer signoff check.
  https://github.com/openvswitch/ovs/commit/74bfe3701407
- checkpatch.at: Add cases to verify skip committer check.
  https://github.com/openvswitch/ovs/commit/3e0d8d1f4b63
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Ilya Maximets]

On 11/16/23 10:17, Dumitru Ceara wrote:
> On 11/16/23 10:02, Dumitru Ceara wrote:
>> On 11/16/23 09:51, Dumitru Ceara wrote:
>>> On 11/16/23 09:44, Ales Musil wrote:
 On Thu, Nov 16, 2023 at 9:34 AM Dumitru Ceara  wrote:

> On 11/16/23 00:12, Ilya Maximets wrote:
>> On 11/15/23 22:20, Mark Michelson wrote:
>>> On 11/15/23 12:02, Ilya Maximets wrote:
 On 11/15/23 17:00, Dumitru Ceara wrote:
> All of the above were changed to track the latest available releases.
> Initially that seemed like a good idea but in practice, a new release
> would
> potentially (silently) cause CI runs that used to pass on given stable
> versions to unexpectedly start failing.
>
> To address that this series pins all versions we can control allowing
> us
> to use different values for different branches.
>
> NOTE: the last commit of this series will look slightly different on
> stable branches, e.g., on branches <= 23.06 we need Python <= 3.11 and
> Fedora 37.
>
> The first commit of the series bumps the OVS submodule to include the
> latest fixes to errors reported by recent versions of flake8.
>
> Changes in v2:
> - added first patch to bump OVS submodule
> - addressed Ales' review comments:
>- moved the logic to determine which image to use out of ci.sh;
>  it's now in the workflow itself.
>- moved setting of OVN_IMAGE in the same block with all related
>  env vars
> - added a note for maintainers in the release process documentation
>to mention that the new workflow will likely have to be triggered
>manually (at least once) when branching for a new release.  GitHub
>doesn't allow periodic jobs on non-default branches.

 IMHO, that sounds horrible to maintain.  Can we just build things per
> run?
>>>
>>> What if we set up the "Containers" action to run on branch creation?
>>> Would that adequately substitute for the manual run that creates the
> image?
>>>
>>> If we only have to run the action once each time a branch is created, I
>>> don't think that qualifies as "horrible" to maintain. This could be
>>> lumped into our branch creation scripts. However, if it can be
> automated
>>> as I suggested above, that would be better.
>>
>> The job will never be re-run, meaning we'll be using the same container
>> for many years.  In case we'll need an update for this container, it will
>> need to be done manually for each branch, except for main.
>>
>> Also, in the patch, the Cirrus CI is set to use the container from main,
>> so this will need to be adjusted for each branch, will need to become
>> part of the release patches (?).
>>
>> Another issue with current implementation of containers is that it's
>> a pain for external contributor to change dependencies, because they'll
>> have to modify the CI in multiple places in order to build and use their
>> own containers.
>> And with these new changes it will become worse.  For example, I never
> use
>> 'main' or 'branch-*' branches in my own forks.  And CI depends on the
> branch
>> name now, and it will fall back to an unmaintained old-style-named image.
>> Might not be easy to figure out why all your builds are failing,
> especially
>> if you're new to the project.
>>
>
> We could try to make it build the container image every time on stable
> branches and avoid any manual steps.

But the branch name will still be a problem.  Every time people use any
branch name other than 'main' for their own development, they'll get an
outdated container, unless they change the CI scripts.  Or am I missing
something?

>
>>>
 How much value these containers actually have?
>
> I think you mentioned the benefit lower already:
> - we can share the artifact (container image) with Cirrus CI; aarch64
> container build takes significantly longer
> - we can share the artifact with downstream CIs
>
>>>
>>> As patch 2 mentions, it allows for us to use different distro bases for
>>> each version.
>>
>> This can be done by simply specifying 'container: name' in the job,
>> GHA will fetch and use that container.
>>
>>> But I also think that having the container image cached
>>> allows for quicker test runs since we are not having to reconstruct the
>>> environment each time we perform test runs.
>>
>> It takes less than 2 minutes to create a full x86 environment.  We spend
>> more time re-building OVS and OVN for every variant of tests, even though
>> most of them are using the same compiler flags.
>>
>> Anyways, there is no need to push the container into registry, we can
> build

Re: [ovs-dev] [PATCH] netdev-offload-tc: Fix offload of tunnel key tp_src.

[Marcelo Ricardo Leitner]

Hi Vladislav,

On Wed, Nov 15, 2023 at 04:13:13PM +0300, Vladislav Odintsov wrote:
...
> Final flow: 
> arp,reg11=0x2,reg12=0x6,reg14=0x4,reg15=0x2,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_ipv6_src=::,tun_ipv6_dst=::,tun_gbp_id=0,tun_gbp_flags=0,tun_tos=0,tun_ttl=64,tun_erspan_ver=0,gtpu_flags=0,gtpu_msgtype=0,tun_flags=csum|key,tun_metadata0=0x40003,metadata=0x6,in_port=17,vlan_tci=0x,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=169.254.100.1,arp_tpa=169.254.100.3,arp_op=1,arp_sha=00:00:c9:99:bd:0e,arp_tha=00:00:00:00:00:00
> Megaflow: 
> recirc_id=0,ct_state=-new-est-rel-rpl-trk,ct_label=0/0x3,eth,arp,tun_id=0x6,tun_src=10.1.0.103,tun_dst=10.1.0.109,tun_tos=0,tun_flags=-df+csum+key,tun_metadata0=0x40003,in_port=17,dl_src=00:00:c9:99:bd:0e,dl_dst=ff:ff:ff:ff:ff:ff,arp_tpa=169.254.100.3,arp_op=1
> Datapath actions: 
> set(tunnel(tun_id=0xff0002,src=10.1.0.109,dst=10.1.1.18,ttl=64,tp_src=3275,tp_dst=6081,geneve({class=0x102,type=0x80,len=4,0x55008e}),flags(df|csum|key))),5
>
> The traffic is an OVN interconnection usecase, where there is a transit switch
> and two LRs are connected to it on different Availability Zones.  LR on AZ1
> tries to resolve ARP for LR in another AZ.

In the flow here I see in_port=17 and output to port 5. Can you please
confirm which ports are those? The 2 ports of the CX6, maybe?
Asking because I'm a bit surprised that hairpin traffic with such
encap operations actually works.

Thanks,
Marcelo

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

Re: [ovs-dev] [PATCH ovn] tests: Fix race in "feature inactivity probe" test.

[Dumitru Ceara]

On 11/16/23 12:49, Xavier Simonart wrote:
> Hi Dumitru
> 

Hi Xavier,

> Thanks for the patch. It clearly decreases the failure count.
> I am afraid however it does not fix all potential issues.
> My understanding is the following:
> The test goal was to ensure ovs does not disconnect the connections on
> inactivity probe.There are 4 connections with inactivity probe (3 in
> earlier OVN versions):
> - feature
> - pinctrl
> - statctrl
> - ofctrl
> For those connections, ovs will send ECHO REQUEST and expects
> ovn-controller to reply with ECHO REPLY, within 60 seconds.
> The patch you sent ensures ofctrl runs between the warp, so ensures
> that ovn-controller replied with ECHO REPLY to the ECHO_REQUEST for
> ofctrl.
> However, ECHO REQUEST for pinctrl connection is handled by the pinctrl
> thread, and nothing guarantees that pinctrl had time to run between
> two warps, so that it handled the ECHO REQUEST within those
> (simulated) 60 seconds.
> Similar issue happens with statctrl.

I had missed those cases..

> Then, another issue can occur with the "feature" connection:
> ovn-controller handles the ECHO_REQUEST for that connection in the
> main ovn-controller loop, but only if there is an ovs idl transaction
> available ("ovs db is not read-only").
> So, if ovsdb is very slow (and still handling the ovn-installed-ts
> insertion for instance), ovn-controller will not get any ovs idl
> transaction available, and won't handle the ECHO_REQUEST for the
> "feature" connection, or at least not in time.
> I am not sure how to fix those issues without making the test overly complex.
> 

+1

Also, the test itself is quite artificial, I suggest we remove it all
together.  If you agree with that, I can post v2.

Thanks,
Dumitru

> Thanks
> Xavier
> 
> On Thu, Nov 16, 2023 at 10:39 AM Ales Musil  wrote:
>>
>> On Wed, Nov 15, 2023 at 2:40 PM Dumitru Ceara  wrote:
>>
>>> The test incorrectly assumed that "ovn-nbctl --wait=hv sync" will always
>>> send an OpenFlow barrier to ovs-vswitchd.  That doesn't happen unless
>>> there are other OpenFlow (rule or group) changes that need to be
>>> programmed in the datapath.
>>>
>>> Fix the test by actually installing new rules in between warp
>>> invocations.
>>>
>>> Spotted in CI, mostly on oversubscribed systems.  A log sample that
>>> shows that ovn-controller didn't generate any barrier request for the
>>> nbctl sync request:
>>>
>>>   2023-11-15T12:12:22.937Z|00084|vconn|DBG|unix#4: received:
>>> OFPT_BARRIER_REQUEST (OF1.5) (xid=0x13):
>>>   2023-11-15T12:12:22.937Z|00085|vconn|DBG|unix#4: sent (Success):
>>> OFPT_BARRIER_REPLY (OF1.5) (xid=0x13):
>>>   ...
>>>   2023-11-15T12:12:23.032Z|00090|unixctl|DBG|received request
>>> time/warp["6"], id=0
>>>   2023-11-15T12:12:23.032Z|00091|unixctl|DBG|replying with success, id=0:
>>> "warped"
>>>   2023-11-15T12:12:23.042Z|00094|vconn|DBG|unix#3: sent (Success):
>>> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>>>   2023-11-15T12:12:23.042Z|00095|vconn|DBG|unix#4: sent (Success):
>>> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>>>   2023-11-15T12:12:23.042Z|00097|vconn|DBG|unix#5: sent (Success):
>>> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>>>   2023-11-15T12:12:23.042Z|00098|unixctl|DBG|received request
>>> time/warp["6"], id=0
>>>   2023-11-15T12:12:23.042Z|00099|unixctl|DBG|replying with success, id=0:
>>> "warped"
>>>   2023-11-15T12:12:23.052Z|00100|rconn|ERR|br-int<->unix#3: no response to
>>> inactivity probe after 60 seconds, disconnecting
>>>   2023-11-15T12:12:23.052Z|00101|rconn|ERR|br-int<->unix#4: no response to
>>> inactivity probe after 60 seconds, disconnecting
>>>   2023-11-15T12:12:23.052Z|00102|rconn|ERR|br-int<->unix#5: no response to
>>> inactivity probe after 60 seconds, disconnecting
>>>   2023-11-15T12:12:23.052Z|00103|unixctl|DBG|received request
>>> time/warp["6"], id=0
>>>
>>> Fixes: ff00142808dc ("controller: Fixed ovs/ovn(features) connection lost
>>> when running more than 120 seconds")
>>> Signed-off-by: Dumitru Ceara 
>>> ---
>>>  tests/ovn.at | 15 +++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index b8c61f87fb..face39e5c2 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -35352,20 +35352,35 @@ as hv1
>>>  check ovs-vsctl add-br br-phys
>>>  ovn_attach n1 br-phys 192.168.0.1
>>>
>>> +check ovn-nbctl ls-add ls
>>> +
>>>  dnl Ensure that there are 4 openflow connections.
>>>  OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version'
>>> hv1/ovs-vswitchd.log)" -eq "4"])
>>>
>>> +as hv1 check ovs-vsctl -- add-port br-int lsp0   \
>>> +-- set Interface lsp0 external-ids:iface-id=lsp0 \
>>> +-- add-port br-int lsp1  \
>>> +-- set Interface lsp1 external-ids:iface-id=lsp1 \
>>> +-- add-port br-int lsp2  \
>>> +-- set Interface lsp2 external-ids:iface-id=lsp2
>>> +
>>>  dnl "Wait" 3 times 60 seconds and ensure 

Re: [ovs-dev] [PATCH ovn] tests: Fix race in "feature inactivity probe" test.

[Xavier Simonart]

Hi Dumitru

Thanks for the patch. It clearly decreases the failure count.
I am afraid however it does not fix all potential issues.
My understanding is the following:
The test goal was to ensure ovs does not disconnect the connections on
inactivity probe.There are 4 connections with inactivity probe (3 in
earlier OVN versions):
- feature
- pinctrl
- statctrl
- ofctrl
For those connections, ovs will send ECHO REQUEST and expects
ovn-controller to reply with ECHO REPLY, within 60 seconds.
The patch you sent ensures ofctrl runs between the warp, so ensures
that ovn-controller replied with ECHO REPLY to the ECHO_REQUEST for
ofctrl.
However, ECHO REQUEST for pinctrl connection is handled by the pinctrl
thread, and nothing guarantees that pinctrl had time to run between
two warps, so that it handled the ECHO REQUEST within those
(simulated) 60 seconds.
Similar issue happens with statctrl.
Then, another issue can occur with the "feature" connection:
ovn-controller handles the ECHO_REQUEST for that connection in the
main ovn-controller loop, but only if there is an ovs idl transaction
available ("ovs db is not read-only").
So, if ovsdb is very slow (and still handling the ovn-installed-ts
insertion for instance), ovn-controller will not get any ovs idl
transaction available, and won't handle the ECHO_REQUEST for the
"feature" connection, or at least not in time.
I am not sure how to fix those issues without making the test overly complex.

Thanks
Xavier

On Thu, Nov 16, 2023 at 10:39 AM Ales Musil  wrote:
>
> On Wed, Nov 15, 2023 at 2:40 PM Dumitru Ceara  wrote:
>
> > The test incorrectly assumed that "ovn-nbctl --wait=hv sync" will always
> > send an OpenFlow barrier to ovs-vswitchd.  That doesn't happen unless
> > there are other OpenFlow (rule or group) changes that need to be
> > programmed in the datapath.
> >
> > Fix the test by actually installing new rules in between warp
> > invocations.
> >
> > Spotted in CI, mostly on oversubscribed systems.  A log sample that
> > shows that ovn-controller didn't generate any barrier request for the
> > nbctl sync request:
> >
> >   2023-11-15T12:12:22.937Z|00084|vconn|DBG|unix#4: received:
> > OFPT_BARRIER_REQUEST (OF1.5) (xid=0x13):
> >   2023-11-15T12:12:22.937Z|00085|vconn|DBG|unix#4: sent (Success):
> > OFPT_BARRIER_REPLY (OF1.5) (xid=0x13):
> >   ...
> >   2023-11-15T12:12:23.032Z|00090|unixctl|DBG|received request
> > time/warp["6"], id=0
> >   2023-11-15T12:12:23.032Z|00091|unixctl|DBG|replying with success, id=0:
> > "warped"
> >   2023-11-15T12:12:23.042Z|00094|vconn|DBG|unix#3: sent (Success):
> > OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
> >   2023-11-15T12:12:23.042Z|00095|vconn|DBG|unix#4: sent (Success):
> > OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
> >   2023-11-15T12:12:23.042Z|00097|vconn|DBG|unix#5: sent (Success):
> > OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
> >   2023-11-15T12:12:23.042Z|00098|unixctl|DBG|received request
> > time/warp["6"], id=0
> >   2023-11-15T12:12:23.042Z|00099|unixctl|DBG|replying with success, id=0:
> > "warped"
> >   2023-11-15T12:12:23.052Z|00100|rconn|ERR|br-int<->unix#3: no response to
> > inactivity probe after 60 seconds, disconnecting
> >   2023-11-15T12:12:23.052Z|00101|rconn|ERR|br-int<->unix#4: no response to
> > inactivity probe after 60 seconds, disconnecting
> >   2023-11-15T12:12:23.052Z|00102|rconn|ERR|br-int<->unix#5: no response to
> > inactivity probe after 60 seconds, disconnecting
> >   2023-11-15T12:12:23.052Z|00103|unixctl|DBG|received request
> > time/warp["6"], id=0
> >
> > Fixes: ff00142808dc ("controller: Fixed ovs/ovn(features) connection lost
> > when running more than 120 seconds")
> > Signed-off-by: Dumitru Ceara 
> > ---
> >  tests/ovn.at | 15 +++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index b8c61f87fb..face39e5c2 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -35352,20 +35352,35 @@ as hv1
> >  check ovs-vsctl add-br br-phys
> >  ovn_attach n1 br-phys 192.168.0.1
> >
> > +check ovn-nbctl ls-add ls
> > +
> >  dnl Ensure that there are 4 openflow connections.
> >  OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version'
> > hv1/ovs-vswitchd.log)" -eq "4"])
> >
> > +as hv1 check ovs-vsctl -- add-port br-int lsp0   \
> > +-- set Interface lsp0 external-ids:iface-id=lsp0 \
> > +-- add-port br-int lsp1  \
> > +-- set Interface lsp1 external-ids:iface-id=lsp1 \
> > +-- add-port br-int lsp2  \
> > +-- set Interface lsp2 external-ids:iface-id=lsp2
> > +
> >  dnl "Wait" 3 times 60 seconds and ensure ovn-controller writes to the
> >  dnl openflow connections in the meantime.  This should allow ovs-vswitchd
> >  dnl to probe the openflow connections at least twice.
> >
> >  as hv1 ovs-appctl time/warp 6
> > +check ovn-nbctl lsp-add ls lsp0
> > +wait_for_ports_up
> >  check ovn-nbctl --wait=hv sync

Re: [ovs-dev] [PATCH v2 1/2] mcast-snooping: Test per port explicit flooding.

[David Marchand]

On Thu, Nov 16, 2023 at 12:32 PM Ilya Maximets  wrote:
> > +AT_CHECK([
> > +ovs-vsctl set bridge br0 \
> > +datapath_type=dummy \
> > +mcast_snooping_enable=true \
> > +other-config:mcast-snooping-disable-flood-unregistered=false
>
> Nit:
> Not a full review, but in case you're sending a new version for Eelco's
> comments, please, add more indentation to the 3 lines above, so they
> are not on the same level with ovs-vsctl.

I did not see this comment.
Well, I'll wait for a full review before sending a new revision...


-- 
David Marchand

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

[ovs-dev] [PATCH v3 3/3] mcast-snooping: Fix comments format.

[David Marchand]

Capitalize comments and end them with a . when needed.

Signed-off-by: David Marchand 
---
 tests/mcast-snooping.at | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
index faeb7890d9..890e6aca00 100644
--- a/tests/mcast-snooping.at
+++ b/tests/mcast-snooping.at
@@ -31,13 +31,13 @@ dummy@ovs-dummy: hit:0 missed:0
 
 ovs-appctl time/stop
 
-# Send IGMPv3 query on p2 with vlan 1725
+# Send IGMPv3 query on p2 with vlan 1725.
 # 5c:8a:38:55:25:52 > 01:00:5e:00:00:01, ethertype 802.1Q (0x8100), length 64: 
vlan 1725, p 0, ethertype IPv4,
 # 172.17.25.1 > 224.0.0.1: igmp query v3
 AT_CHECK([ovs-appctl netdev-dummy/receive p2 \
 
'01005e015c8a38552552810006bd080046c0002401027f00ac111901e00194041164ec1e027d'])
 
-# Send IGMPv3 query on p2 with vlan 1728
+# Send IGMPv3 query on p2 with vlan 1728.
 # 5c:8a:38:55:25:52 > 01:00:5e:00:00:01, ethertype 802.1Q (0x8100), length 64: 
vlan 1728, p 0, ethertype IPv4,
 # 172.17.28.1 > 224.0.0.1: igmp query v3
 AT_CHECK([ovs-appctl netdev-dummy/receive p2 \
@@ -51,13 +51,13 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
 
 AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap])
 
-# Send a multicast packet on p1
+# Send a multicast packet on p1.
 AT_CHECK([
 ovs-appctl netdev-dummy/receive p1 \
 
'in_port(1),eth(src=aa:55:aa:55:00:01,dst=01:00:5e:5e:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=239.94.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)'
 ])
 
-# Check this packet was forwarded exactly once to p2 and has vlan tag 1725
+# Check this packet was forwarded exactly once to p2 and has vlan tag 1725.
 # aa:55:aa:55:00:01 > 01:00:5e:5e:01:01, ethertype 802.1Q (0x8100), length 46: 
vlan 1725, p 0, ethertype IPv4,
 # 10.0.0.1.0 > 239.94.1.1.8000: UDP, length 0
 AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
@@ -450,7 +450,7 @@ AT_CHECK([
 
 ovs-appctl time/stop
 
-# send report packets
+# Send report packets.
 AT_CHECK([
 ovs-appctl netdev-dummy/receive p1  \
 
'01005E010101000C29A027A181010800451C00014002CBAEAC10221EE001010112140CE9E0010101'
@@ -458,7 +458,7 @@ AT_CHECK([
 
'01005E010101000C29A027A281020800451C00014002CBAEAC10221EE001010112140CE9E0010101'
 ], [0])
 
-# send query packets
+# Send query packets.
 AT_CHECK([
 ovs-appctl netdev-dummy/receive p3  \

'01005E010101000C29A027D181010800451C00014002CBCBAC102201E0010104EEEB'
@@ -505,7 +505,7 @@ AT_CHECK([
 
 ovs-appctl time/stop
 
-# send report packets
+# Send report packets.
 AT_CHECK([
 ovs-appctl netdev-dummy/receive p1  \
 
'01005E010101000C29A027A181010800451C00014002CBAEAC10221EE001010112140CE9E0010101'
@@ -513,7 +513,7 @@ AT_CHECK([
 
'01005E010101000C29A027A281020800451C00014002CBAEAC10221EE001010112140CE9E0010101'
 ], [0])
 
-# send query packets
+# Send query packets.
 AT_CHECK([
 ovs-appctl netdev-dummy/receive p2  \

'01005E010101000C29A027D181010800451C00014002CBCBAC102201E0010104EEEB'
-- 
2.41.0

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

[ovs-dev] [PATCH v3 1/3] mcast-snooping: Test per port explicit flooding.

[David Marchand]

Various options affect how the mcast snooping module work.

When multicast snooping is enabled and a reporter is known, it is still
possible to flood associated packets to some other port via the
mcast-snooping-flood option.

If flooding unregistered traffic is disabled, it is still possible to
flood multicast traffic too with the mcast-snooping-flood option.

IGMP reports may have to be flooded to some ports explicitly with the
mcast-snooping-flood-reports option.

Test those parameters.

Acked-by: Simon Horman 
Acked-by: Paolo Valerio 
Signed-off-by: David Marchand 
---
Changes since v2:
- fixed comment,

Changes since v1:
- fixed dest mac address,
- added tests for mcast-snooping-disable-flood-unregistered=true and
  mcast-snooping-flood-reports,

---
 tests/mcast-snooping.at | 280 
 1 file changed, 280 insertions(+)

diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
index d5b7c4774c..9797bca531 100644
--- a/tests/mcast-snooping.at
+++ b/tests/mcast-snooping.at
@@ -105,6 +105,286 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+
+AT_SETUP([mcast - check multicast per port flooding])
+OVS_VSWITCHD_START([])
+
+AT_CHECK([
+ovs-vsctl set bridge br0 \
+datapath_type=dummy \
+mcast_snooping_enable=true \
+other-config:mcast-snooping-disable-flood-unregistered=false
+], [0])
+
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+
+AT_CHECK([
+ovs-vsctl add-port br0 p1 \
+-- set Interface p1 type=dummy other-config:hwaddr=aa:55:aa:55:00:01 
ofport_request=1 \
+-- add-port br0 p2 \
+-- set Interface p2 type=dummy other-config:hwaddr=aa:55:aa:55:00:02 
ofport_request=2 \
+-- add-port br0 p3 \
+-- set Interface p3 type=dummy other-config:hwaddr=aa:55:aa:55:00:03 
ofport_request=3 \
+], [0])
+
+ovs-appctl time/stop
+
+AT_CHECK([ovs-appctl ofproto/trace 
"in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
 [0], [stdout])
+AT_CHECK([grep -v 'Datapath actions:' stdout], [0], [dnl
+Flow: 
udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
+
+bridge("br0")
+-
+ 0. priority 32768
+NORMAL
+ -> unregistered multicast, flooding
+
+Final flow: unchanged
+Megaflow: 
recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
+])
+AT_CHECK([sed -ne 's/^Datapath actions: \(.*\)$/\1/p' stdout | tr "," "\n" | 
sort -n], [0], [dnl
+1
+2
+100
+])
+
+# Send report packets.
+AT_CHECK([
+ovs-appctl netdev-dummy/receive p1  \
+
'01005E010101000C29A027A10800451C00014002CBAEAC10221EE001010112140CE9E0010101'
+], [0])
+AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
+ port  VLAN  GROUPAge
+1 0  224.1.1.1   0
+])
+
+AT_CHECK([ovs-appctl ofproto/trace 
"in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
 [0], [dnl
+Flow: 
udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
+
+bridge("br0")
+-
+ 0. priority 32768
+NORMAL
+ -> forwarding to mcast group port
+
+Final flow: unchanged
+Megaflow: 
recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
+Datapath actions: 1
+])
+
+AT_CHECK([ovs-vsctl set port p2 other_config:mcast-snooping-flood=true])
+
+AT_CHECK([ovs-appctl ofproto/trace 
"in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
 [0], [dnl
+Flow: 
udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
+
+bridge("br0")
+-
+ 0. priority 32768
+NORMAL
+ -> forwarding to mcast group port
+ -> forwarding to mcast flood port
+
+Final flow: unchanged
+Megaflow: 
recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
+Datapath actions: 1,2
+])
+
+AT_CHECK([ovs-vsctl set port p3 other_config:mcast-snooping-flood=true])
+
+AT_CHECK([ovs-appctl ofproto/trace 
"in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
 [0], [dnl
+Flow: 
udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
+
+bridge("br0")
+-
+ 0. priority 32768
+NORMAL
+ 

[ovs-dev] [PATCH v3 2/3] mcast-snooping: Flush flood and report ports when deleting interfaces.

[David Marchand]

When a configuration change triggers an interface destruction/creation
(like for example, setting ofport_request), a port object may still be
referenced as a fport or a rport in the mdb.

Before the fix, when flooding multicast traffic:
bridge("br0")
-
 0. priority 32768
NORMAL
 -> forwarding to mcast group port
 >> mcast flood port is unknown, dropping
 -> mcast flood port is input port, dropping
 -> forwarding to mcast flood port

Before the fix, when flooding igmp report traffic:
bridge("br0")
-
 0. priority 32768
NORMAL
 >> mcast port is unknown, dropping the report
 -> forwarding report to mcast flagged port
 -> mcast port is input port, dropping the Report
 -> forwarding report to mcast flagged port

Add relevant cleanup and update unit tests.

Fixes: 4fbbf8624868 ("mcast-snooping: Flush ports mdb when VLAN configuration 
changed.")
Acked-by: Simon Horman 
Acked-by: Paolo Valerio 
Signed-off-by: David Marchand 
---
Changes since v2:
- christmas tree,
- added some comments in tests,

Changes since v1:
- updated the test on report flooding,

---
 lib/mcast-snooping.c| 17 -
 tests/mcast-snooping.at | 42 +
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
index 029ca28558..43805ae4d5 100644
--- a/lib/mcast-snooping.c
+++ b/lib/mcast-snooping.c
@@ -946,8 +946,9 @@ mcast_snooping_wait(struct mcast_snooping *ms)
 void
 mcast_snooping_flush_bundle(struct mcast_snooping *ms, void *port)
 {
-struct mcast_group *g;
 struct mcast_mrouter_bundle *m;
+struct mcast_port_bundle *p;
+struct mcast_group *g;
 
 if (!mcast_snooping_enabled(ms)) {
 return;
@@ -971,5 +972,19 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms, 
void *port)
 }
 }
 
+LIST_FOR_EACH_SAFE (p, node, >fport_list) {
+if (p->port == port) {
+mcast_snooping_flush_port(p);
+ms->need_revalidate = true;
+}
+}
+
+LIST_FOR_EACH_SAFE (p, node, >rport_list) {
+if (p->port == port) {
+mcast_snooping_flush_port(p);
+ms->need_revalidate = true;
+}
+}
+
 ovs_rwlock_unlock(>rwlock);
 }
diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
index 9797bca531..faeb7890d9 100644
--- a/tests/mcast-snooping.at
+++ b/tests/mcast-snooping.at
@@ -207,6 +207,26 @@ Megaflow: 
recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e
 Datapath actions: 1,2
 ])
 
+# Change p2 ofport to force a ofbundle change and check that the mdb contains
+# no stale port.
+AT_CHECK([ovs-vsctl set interface p2 ofport_request=4])
+
+AT_CHECK([ovs-appctl ofproto/trace 
"in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
 [0], [dnl
+Flow: 
udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
+
+bridge("br0")
+-
+ 0. priority 32768
+NORMAL
+ -> forwarding to mcast group port
+ -> mcast flood port is input port, dropping
+ -> forwarding to mcast flood port
+
+Final flow: unchanged
+Megaflow: 
recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
+Datapath actions: 1,2
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
@@ -381,6 +401,28 @@ This flow is handled by the userspace slow path because it:
   - Uses action(s) not supported by datapath.
 ])
 
+# Change p2 ofport to force a ofbundle change and check that the mdb contains
+# no stale port.
+AT_CHECK([ovs-vsctl set interface p3 ofport_request=4])
+
+AT_CHECK([ovs-appctl ofproto/trace "in_port(1)" 
'01005E010101000C29A027A10800451C00014002CBAEAC10221EE001010112140CE9E0010101'],
 [0], [dnl
+Flow: 
ip,in_port=1,vlan_tci=0x,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_src=172.16.34.30,nw_dst=224.1.1.1,nw_proto=2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=18,tp_dst=20
+
+bridge("br0")
+-
+ 0. priority 32768
+NORMAL
+ -> forwarding report to mcast flagged port
+ -> mcast port is input port, dropping the Report
+ -> forwarding report to mcast flagged port
+
+Final flow: unchanged
+Megaflow: 
recirc_id=0,eth,ip,in_port=1,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_proto=2,nw_frag=no
+Datapath actions: 2,3
+This flow is handled by the userspace slow path because it:
+  - Uses action(s) not supported by datapath.
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
-- 
2.41.0

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

Re: [ovs-dev] [PATCH v2 1/2] mcast-snooping: Test per port explicit flooding.

[Ilya Maximets]

On 11/10/23 18:52, David Marchand wrote:
> Various options affect how the mcast snooping module work.
> 
> When multicast snooping is enabled and a reporter is known, it is still
> possible to flood associated packets to some other port via the
> mcast-snooping-flood option.
> 
> If flooding unregistered traffic is disabled, it is still possible to
> flood multicast traffic too with the mcast-snooping-flood option.
> 
> IGMP reports may have to be flooded to some ports explicitly with the
> mcast-snooping-flood-reports option.
> 
> Test those parameters.
> 
> Signed-off-by: David Marchand 
> ---
> Changes since v1:
> - fixed dest mac address,
> - added tests for mcast-snooping-disable-flood-unregistered=true and
>   mcast-snooping-flood-reports,
> 
> ---
>  tests/mcast-snooping.at | 280 
>  1 file changed, 280 insertions(+)
> 
> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
> index d5b7c4774c..b5474cf392 100644
> --- a/tests/mcast-snooping.at
> +++ b/tests/mcast-snooping.at
> @@ -105,6 +105,286 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +
> +AT_SETUP([mcast - check multicast per port flooding])
> +OVS_VSWITCHD_START([])
> +
> +AT_CHECK([
> +ovs-vsctl set bridge br0 \
> +datapath_type=dummy \
> +mcast_snooping_enable=true \
> +other-config:mcast-snooping-disable-flood-unregistered=false

Nit:
Not a full review, but in case you're sending a new version for Eelco's
comments, please, add more indentation to the 3 lines above, so they
are not on the same level with ovs-vsctl.

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

Re: [ovs-dev] [PATCH v2 2/2] mcast-snooping: Flush flood and report ports when deleting interfaces.

[Eelco Chaudron]

On 16 Nov 2023, at 12:24, David Marchand wrote:

> On Thu, Nov 16, 2023 at 10:38 AM Eelco Chaudron  wrote:
>>> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
>>> index b5474cf392..1ce31168e8 100644
>>> --- a/tests/mcast-snooping.at
>>> +++ b/tests/mcast-snooping.at
>>> @@ -207,6 +207,24 @@ Megaflow: 
>>> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e
>>>  Datapath actions: 1,2
>>>  ])
>>>
>>> +AT_CHECK([ovs-vsctl set interface p2 ofport_request=4])
>>
>> Can we add a comment here (and below) to indicate why we do this? Just to 
>> understand what we test here.
>
> Wdyt of:
> +# Change p2 ofport to force a ofbundle change and check that the mdb contains
> +# no stale port.

Yes this looks good to me!

Thanks,

Eelco

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

Re: [ovs-dev] [PATCH v2 1/2] mcast-snooping: Test per port explicit flooding.

[Eelco Chaudron]

On 16 Nov 2023, at 12:13, David Marchand wrote:

> On Thu, Nov 16, 2023 at 11:57 AM Eelco Chaudron  wrote:
>> On 10 Nov 2023, at 18:52, David Marchand wrote:
>>> +Final flow: unchanged
>>> +Megaflow: 
>>> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
>>> +])
>>> +AT_CHECK([sed -ne 's/^Datapath actions: \(.*\)$/\1/p' stdout | tr "," "\n" 
>>> | sort -n], [0], [dnl
>>> +1
>>> +2
>>> +100
>>> +])
>>> +
>>> +# send report packets
>>
>> Please add capital and dots to all comments.
>
> I don't mind, but the rest of this file is not consistent to this convention.
>
> $ git grep \\# origin/master -- tests/mcast-snooping.at
> ...
> origin/master:tests/mcast-snooping.at:# send report packets
> origin/master:tests/mcast-snooping.at:# send query packets
> origin/master:tests/mcast-snooping.at:# send report packets
> origin/master:tests/mcast-snooping.at:# send query packets

Agreed, but we like to be adherent for new additions.

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

Re: [ovs-dev] [PATCH v2 1/2] mcast-snooping: Test per port explicit flooding.

[Eelco Chaudron]

On 16 Nov 2023, at 12:10, David Marchand wrote:

> Hello Eelco,
>
> On Thu, Nov 16, 2023 at 11:57 AM Eelco Chaudron  wrote:
>
> [snip]
>
>>> +bridge("br0")
>>> +-
>>> + 0. priority 32768
>>> +NORMAL
>>> + -> forwarding to mcast group port
>>> + -> forwarding to mcast flood port
>>> +
>>> +Final flow: unchanged
>>> +Megaflow: 
>>> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
>>> +Datapath actions: 1,2
>>
>>
>> Are we sure the order here is always 1,2 vs the first test you sorted them? 
>> Same for all the other multi-port tests below?
>>
>> I did run the test 200+ times, and it seems ok. Trying to understand this, 
>> as I can see the first one reporting 100,1,2 and 100,2,1.
>
> struct mcast_output out = MCAST_OUTPUT_INIT;
> ...
> if (grp) {
> xlate_normal_mcast_send_group(ctx, ms, grp, in_xbundle, );
> xlate_normal_mcast_send_fports(ctx, ms, in_xbundle, );
> xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, ,
>  );
> ...
> mcast_output_finish(ctx, , in_xbundle, );
>
> With:
> static void
> mcast_output_finish(struct xlate_ctx *ctx, struct mcast_output *out,
> struct xbundle *in_xbundle, struct xvlan *xvlan)
> {
> if (out->flood) {
> xlate_normal_flood(ctx, in_xbundle, xvlan);
> } else {
> for (size_t i = 0; i < out->n; i++) {
> output_normal(ctx, out->xbundles[i], xvlan);
> }
> }
> ...
>
>
> In this case, there is no flooding (contrary to previous tests) over
> all the ports from this bridge.
> There is only one "group" port and one "flood" port and the order is fixed.

Thanks, now it makes sense :)

//Eelco

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

Re: [ovs-dev] [PATCH v2 2/2] mcast-snooping: Flush flood and report ports when deleting interfaces.

[David Marchand]

On Thu, Nov 16, 2023 at 10:38 AM Eelco Chaudron  wrote:
> > diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
> > index b5474cf392..1ce31168e8 100644
> > --- a/tests/mcast-snooping.at
> > +++ b/tests/mcast-snooping.at
> > @@ -207,6 +207,24 @@ Megaflow: 
> > recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e
> >  Datapath actions: 1,2
> >  ])
> >
> > +AT_CHECK([ovs-vsctl set interface p2 ofport_request=4])
>
> Can we add a comment here (and below) to indicate why we do this? Just to 
> understand what we test here.

Wdyt of:
+# Change p2 ofport to force a ofbundle change and check that the mdb contains
+# no stale port.


-- 
David Marchand

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

Re: [ovs-dev] [PATCH v2 1/2] mcast-snooping: Test per port explicit flooding.

[David Marchand]

On Thu, Nov 16, 2023 at 11:57 AM Eelco Chaudron  wrote:
> On 10 Nov 2023, at 18:52, David Marchand wrote:
> > +Final flow: unchanged
> > +Megaflow: 
> > recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
> > +])
> > +AT_CHECK([sed -ne 's/^Datapath actions: \(.*\)$/\1/p' stdout | tr "," "\n" 
> > | sort -n], [0], [dnl
> > +1
> > +2
> > +100
> > +])
> > +
> > +# send report packets
>
> Please add capital and dots to all comments.

I don't mind, but the rest of this file is not consistent to this convention.

$ git grep \\# origin/master -- tests/mcast-snooping.at
...
origin/master:tests/mcast-snooping.at:# send report packets
origin/master:tests/mcast-snooping.at:# send query packets
origin/master:tests/mcast-snooping.at:# send report packets
origin/master:tests/mcast-snooping.at:# send query packets


-- 
David Marchand

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

Re: [ovs-dev] [PATCH v2 1/2] mcast-snooping: Test per port explicit flooding.

[David Marchand]

Hello Eelco,

On Thu, Nov 16, 2023 at 11:57 AM Eelco Chaudron  wrote:

[snip]

> > +bridge("br0")
> > +-
> > + 0. priority 32768
> > +NORMAL
> > + -> forwarding to mcast group port
> > + -> forwarding to mcast flood port
> > +
> > +Final flow: unchanged
> > +Megaflow: 
> > recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
> > +Datapath actions: 1,2
>
>
> Are we sure the order here is always 1,2 vs the first test you sorted them? 
> Same for all the other multi-port tests below?
>
> I did run the test 200+ times, and it seems ok. Trying to understand this, as 
> I can see the first one reporting 100,1,2 and 100,2,1.

struct mcast_output out = MCAST_OUTPUT_INIT;
...
if (grp) {
xlate_normal_mcast_send_group(ctx, ms, grp, in_xbundle, );
xlate_normal_mcast_send_fports(ctx, ms, in_xbundle, );
xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, ,
 );
...
mcast_output_finish(ctx, , in_xbundle, );

With:
static void
mcast_output_finish(struct xlate_ctx *ctx, struct mcast_output *out,
struct xbundle *in_xbundle, struct xvlan *xvlan)
{
if (out->flood) {
xlate_normal_flood(ctx, in_xbundle, xvlan);
} else {
for (size_t i = 0; i < out->n; i++) {
output_normal(ctx, out->xbundles[i], xvlan);
}
}
...


In this case, there is no flooding (contrary to previous tests) over
all the ports from this bridge.
There is only one "group" port and one "flood" port and the order is fixed.


-- 
David Marchand

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

Re: [ovs-dev] [PATCH v4 1/6] Add global option for JSON output to ovs-appctl.

[0-day Robot]

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

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


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#701 FILE: utilities/ovs-appctl.c:114:
  -f, --format=FMT   Output format. One of: 'json', or 'text'\n\

Lines checked: 808, Warnings: 2, Errors: 0


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

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

Re: [ovs-dev] [PATCH v2 1/2] mcast-snooping: Test per port explicit flooding.

[Eelco Chaudron]

On 10 Nov 2023, at 18:52, David Marchand wrote:

> Various options affect how the mcast snooping module work.
>
> When multicast snooping is enabled and a reporter is known, it is still
> possible to flood associated packets to some other port via the
> mcast-snooping-flood option.
>
> If flooding unregistered traffic is disabled, it is still possible to
> flood multicast traffic too with the mcast-snooping-flood option.
>
> IGMP reports may have to be flooded to some ports explicitly with the
> mcast-snooping-flood-reports option.
>
> Test those parameters.
>
> Signed-off-by: David Marchand 

Thanks David for adding more test cases!

One small nit, and a question below.

Cheers,

Eelco


> ---
> Changes since v1:
> - fixed dest mac address,
> - added tests for mcast-snooping-disable-flood-unregistered=true and
>   mcast-snooping-flood-reports,
>
> ---
>  tests/mcast-snooping.at | 280 
>  1 file changed, 280 insertions(+)
>
> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
> index d5b7c4774c..b5474cf392 100644
> --- a/tests/mcast-snooping.at
> +++ b/tests/mcast-snooping.at
> @@ -105,6 +105,286 @@ AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> +
> +AT_SETUP([mcast - check multicast per port flooding])
> +OVS_VSWITCHD_START([])
> +
> +AT_CHECK([
> +ovs-vsctl set bridge br0 \
> +datapath_type=dummy \
> +mcast_snooping_enable=true \
> +other-config:mcast-snooping-disable-flood-unregistered=false
> +], [0])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +
> +AT_CHECK([
> +ovs-vsctl add-port br0 p1 \
> +-- set Interface p1 type=dummy other-config:hwaddr=aa:55:aa:55:00:01 
> ofport_request=1 \
> +-- add-port br0 p2 \
> +-- set Interface p2 type=dummy other-config:hwaddr=aa:55:aa:55:00:02 
> ofport_request=2 \
> +-- add-port br0 p3 \
> +-- set Interface p3 type=dummy other-config:hwaddr=aa:55:aa:55:00:03 
> ofport_request=3 \
> +], [0])
> +
> +ovs-appctl time/stop
> +
> +AT_CHECK([ovs-appctl ofproto/trace 
> "in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
>  [0], [stdout])
> +AT_CHECK([grep -v 'Datapath actions:' stdout], [0], [dnl
> +Flow: 
> udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
> +
> +bridge("br0")
> +-
> + 0. priority 32768
> +NORMAL
> + -> unregistered multicast, flooding
> +
> +Final flow: unchanged
> +Megaflow: 
> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
> +])
> +AT_CHECK([sed -ne 's/^Datapath actions: \(.*\)$/\1/p' stdout | tr "," "\n" | 
> sort -n], [0], [dnl
> +1
> +2
> +100
> +])
> +
> +# send report packets

Please add capital and dots to all comments.

+# Send report packets.

> +AT_CHECK([
> +ovs-appctl netdev-dummy/receive p1  \
> +
> '01005E010101000C29A027A10800451C00014002CBAEAC10221EE001010112140CE9E0010101'
> +], [0])
> +AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
> + port  VLAN  GROUPAge
> +1 0  224.1.1.1   0
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace 
> "in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
>  [0], [dnl
> +Flow: 
> udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
> +
> +bridge("br0")
> +-
> + 0. priority 32768
> +NORMAL
> + -> forwarding to mcast group port
> +
> +Final flow: unchanged
> +Megaflow: 
> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
> +Datapath actions: 1
> +])
> +
> +AT_CHECK([ovs-vsctl set port p2 other_config:mcast-snooping-flood=true])
> +
> +AT_CHECK([ovs-appctl ofproto/trace 
> "in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
>  [0], [dnl
> +Flow: 
> udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
> +
> +bridge("br0")
> +-
> + 0. priority 32768
> +NORMAL
> + -> forwarding to mcast group port
> + -> forwarding to mcast flood port
> +
> +Final flow: unchanged
> +Megaflow: 
> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
> +Datapath actions: 1,2


Are we sure the order here is always 1,2 vs the first test you sorted them? 
Same for all the other multi-port tests below?

I did run the test 200+ 

Re: [ovs-dev] [RFC v3 0/4] Add global option to output JSON from ovs-appctl cmds.

[Jakob Meng]

Thank you again for all your input! Your comments have been incorporated into a 
new patch series v4:

https://patchwork.ozlabs.org/project/openvswitch/list/?series=382459=both=*

On 25.10.23 11:37, jm...@redhat.com wrote:
> From: Jakob Meng 
>
> Add global option to output JSON from ovs-appctl cmds.
>
> This patch is an update of [0] with the following major changes:
> * The JSON-RPC API change is now backward compatible. One can use an
>   updated client (ovs-appctl/dpctl) with an old server (ovs-vswitchd)
>   and vice versa. Of course, JSON output only works when both are
>   updated.
> * tests/pmd.at from forth patch now features an example of how the
>   output looks like when a command does not support JSON output.
> * The patch has been split into a series of four. The first patch
>   introduces the '-f,--format' option for ovs-appctl/ovs-dpctl and
>   necessary changes to the JSON-RPC API. It does not yet pass the
>   output format to individual commands because that requires a lot
>   of changes. Those changes have been split out into the third patch
>   to increase readability of the series.
> * The second patch introduces equivalent changes to the Python files.
> * The third patch moves all commands to the updated functions in
>   lib/unixctl.*, in particular unixctl_command_register() and the
>   unixctl_cb_func type, as well as their Python counterparts. The
>   output is still text-only (no json) for all commands.
> * The forth patch shows how JSON output could be implemented using
>   'dpif/show' as an example.
>
> The following paragraphs are taken from the previous patch revision
> and have been updated to changes mentioned above.
>
> For monitoring systems such as Prometheus it would be beneficial if OVS
> and OVS-DPDK would expose statistics in a machine-readable format.
> Several approaches like UNIX socket, OVSDB queries and JSON output from
> ovs-xxx tools have been proposed [2],[3]. This proof of concept
> describes one way how ovs-xxx tools could output JSON in addition to
> plain-text for humans.
>
> This patch follows an alternative approach to RFC [1] which
> implemented JSON output as a separate option for each command like
> 'dpif/show'. The option was called '-o|--output' in the latter. It
> has been renamed to '-f,--format'  because ovs-appctl already has a
> short option '-o' which prints the available ovs-appctl options
> ('--option'). The new option name '-f,--format' is in line with
> ovsdb-client where it controls output formatting, too.
>
> An example call would be 'ovs-appctl --format json dpif/show' as
> shown in tests/pmd.at of the forth patch. By default, the output
> format is plain-text as before.
>
> With this patch, all commands announce their support for output
> formats when being registered with unixctl_command_register() from
> lib/unixctl.*, e.g. OVS_OUTPUT_FMT_TEXT and/or OVS_OUTPUT_FMT_JSON.
> When a requested output format is not supported by a command, then
> process_command() in lib/unixctl.c will return an error. This is an
> advantage over the previous approach [1] where each command would have
> to parse the output format option and handle requests for unsupported
> output formats on its own.
>
> The question whether JSON output should be pretty printed and sorted
> remains. In most cases it would be unnecessary because machines
> consume the output or humans could use jq for pretty printing. However,
> it would make tests more readable (for humans) without having to use jq
> (which would require us to introduce a dependency on jq).
>
> Wdyt?
>
> [0] 
> https://patchwork.ozlabs.org/project/openvswitch/patch/20231020104320.1417664-2-jm...@redhat.com/
> [1] 
> https://patchwork.ozlabs.org/project/openvswitch/patch/20231020092205.710399-2-jm...@redhat.com/
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1824861#c7
> [3] 
> https://patchwork.ozlabs.org/project/openvswitch/patch/20230831131122.284913-1-rja...@redhat.com/
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng 
>
> Jakob Meng (4):
>   Add global option for JSON output to ovs-appctl/dpctl.
>   python: Add global option for JSON output to Python tools.
>   Migrate commands to extended unixctl API.
>   ofproto: Add JSON output for 'dpif/show' command.
>
>  lib/bfd.c  |  16 ++-
>  lib/cfm.c  |  13 +-
>  lib/command-line.c |  36 +
>  lib/command-line.h |  10 ++
>  lib/coverage.c |  11 +-
>  lib/dpctl.c| 129 ++---
>  lib/dpctl.h|   4 +
>  lib/dpdk.c |  15 +-
>  lib/dpif-netdev-perf.c |   1 +
>  lib/dpif-netdev-perf.h |   1 +
>  lib/dpif-netdev.c  |  84 ++-
>  lib/dpif-netlink.c |   6 +-
>  lib/lacp.c |   7 +-
>  lib/memory.c   |   5 +-
>  lib/netdev-dpdk.c  |  16 ++-
>  lib/netdev-dummy.c |  30 ++--
>  

[ovs-dev] [PATCH v4 6/6] unixctl: Pass output format as command args via JSON-RPC API.

[jmeng]

From: Jakob Meng 

A previous patch had changed the JSON-RPC API in lib/unixctl.* (and
its Python counterpart) in order to allow transporting the requested
output format from ovs-xxx tools to ovs-vswitchd across unix sockets:

The meaning of 'method' and 'params' had be changed: 'method' would be
'execute/v1' when an output format other than 'text' is requested. In
that case, the first parameter of the JSON array 'params' would be the
designated command, the second one the output format and the rest
will be command args.
That change allowed to transport the output format in a backward
compatible way: One could use an updated client (ovs-appctl) with an
old server (ovs-vswitchd) and vice versa. Of course, JSON output would
only work when both sides have been updated.

This patch reverts the JSON-RPC API to the old behaviour: The command
will be copied to JSON-RPC's 'method' parameter while command args will
be copied to the 'params' parameter.
The client will inject the output format into the command args and the
server will parse and remove it before passing the args to the command
callback.

The advantage of this (old) approach is a simpler JSON-RPC API but at
the cost of inferior usability: ovs-xxx tools are no longer able to
detect missing JSON support at the server side. Old servers will simply
pass the output format as an arg to the command which would then result
in an error such as 'command takes at most ... arguments' or a non-\
uniform error from the command callback.

Signed-off-by: Jakob Meng 
---
 lib/unixctl.c| 147 ---
 python/ovs/unixctl/client.py |  19 ++---
 python/ovs/unixctl/server.py |  37 -
 python/ovs/util.py   |   1 -
 4 files changed, 91 insertions(+), 113 deletions(-)

diff --git a/lib/unixctl.c b/lib/unixctl.c
index e7edbb154..1eaf6edb0 100644
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -63,8 +63,6 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
 
 static struct shash commands = SHASH_INITIALIZER();
 
-static const char *rpc_marker = "execute/v1";
-
 static void
 unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
   const char *argv[] OVS_UNUSED,
@@ -292,11 +290,9 @@ process_command(struct unixctl_conn *conn, struct 
jsonrpc_msg *request)
 
 struct unixctl_command *command;
 struct json_array *params;
-const char *method;
-enum ovs_output_fmt fmt;
+enum ovs_output_fmt fmt = OVS_OUTPUT_FMT_TEXT;
 struct svec argv = SVEC_EMPTY_INITIALIZER;
-int args_offset;
-bool plain_rpc;
+int argc;
 
 COVERAGE_INC(unixctl_received);
 conn->request_id = json_clone(request->id);
@@ -310,20 +306,9 @@ process_command(struct unixctl_conn *conn, struct 
jsonrpc_msg *request)
 free(id_s);
 }
 
-/* The JSON-RPC API requires an indirection in order to allow transporting
- * additional data like the output format besides command and args.  For
- * backward compatibility with older clients the plain RPC is still
- * supported. */
-plain_rpc = strcmp(request->method, rpc_marker);
-args_offset = plain_rpc ? 0 : 2;
-
 params = json_array(request->params);
-if (!plain_rpc && (params->n < 2)) {
-error = xasprintf("JSON-RPC API mismatch: Unexpected # of params:"\
-  " %"PRIuSIZE, params->n);
-goto error;
-}
 
+/* Verify type of arguments. */
 for (size_t i = 0; i < params->n; i++) {
 if (params->elems[i]->type != JSON_STRING) {
 error = xasprintf("command has non-string argument: %s",
@@ -332,54 +317,70 @@ process_command(struct unixctl_conn *conn, struct 
jsonrpc_msg *request)
 }
 }
 
-/* Extract method name. */
-method = plain_rpc ? request->method : json_string(params->elems[0]);
+argc = params->n;
 
-/* Extract output format. */
-if (plain_rpc) {
-fmt = OVS_OUTPUT_FMT_TEXT;
-} else {
-if (!ovs_output_fmt_from_string(json_string(params->elems[1]), )) {
-error = xasprintf("invalid output format: %s",
-  json_string(params->elems[1]));
-goto error;
+/* Extract and process command args. */
+svec_add(, request->method);
+for (size_t i = 0; i < params->n; i++) {
+if (!strcmp(json_string(params->elems[i]), "-f") ||
+!strcmp(json_string(params->elems[i]), "--format"))
+{
+/* Parse output format argument. */
+
+if ((i + 1) == (params->n)) {
+error = xasprintf("option requires an argument -- %s",
+  json_string(params->elems[i]));
+goto error;
+}
+
+/* Move index to option argument. */
+i++;
+
+if (!ovs_output_fmt_from_string(json_string(params->elems[i]),
+))
+{
+error = xasprintf("option %s has 

[ovs-dev] [PATCH v4 2/6] python: Add global option for JSON output to Python tools.

[jmeng]

From: Jakob Meng 

This patch introduces support for different output formats to the
Python code, as did the previous commit for ovs-xxx tools like
'ovs-appctl --format json dpif/show'.
In particular, tests/appctl.py gains a global option '-f,--format'
which allows users to request JSON instead of plain-text for humans.

This patch does not yet pass the choosen output format to commands.
Doing so requires changes to all command_register() calls and all
command callbacks. To improve readibility those changes have been
split out into a follow up patch. Respectively, whenever an output
format other than 'text' is choosen for tests/appctl.py, the script
will fail.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 NEWS   |  2 +
 python/ovs/unixctl/__init__.py | 39 +++---
 python/ovs/unixctl/client.py   | 20 +++--
 python/ovs/unixctl/server.py   | 74 +++---
 python/ovs/util.py |  9 +
 tests/appctl.py|  7 +++-
 tests/unixctl-py.at|  7 
 7 files changed, 126 insertions(+), 32 deletions(-)

diff --git a/NEWS b/NEWS
index 12a895629..fc77a1613 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,8 @@ Post-v3.2.0
Reported names adjusted accordingly.
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
+   - Python:
+ * Added support for choosing the output format, e.g. 'json' or 'text'.
 
 
 v3.2.0 - 17 Aug 2023
diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py
index 8ee312943..6d9bd5715 100644
--- a/python/ovs/unixctl/__init__.py
+++ b/python/ovs/unixctl/__init__.py
@@ -20,10 +20,14 @@ commands = {}
 
 
 class _UnixctlCommand(object):
-def __init__(self, usage, min_args, max_args, callback, aux):
+# FIXME: Output format will be passed as 'output_fmts' to the command in
+#later patch.
+def __init__(self, usage, min_args, max_args,  # FIXME: output_fmts,
+ callback, aux):
 self.usage = usage
 self.min_args = min_args
 self.max_args = max_args
+# FIXME: self.output_fmts = output_fmts
 self.callback = callback
 self.aux = aux
 
@@ -42,10 +46,13 @@ def _unixctl_help(conn, unused_argv, unused_aux):
 conn.reply(reply)
 
 
-def command_register(name, usage, min_args, max_args, callback, aux):
+def command_register_fmt(name, usage, min_args, max_args, output_fmts,
+ callback, aux):
 """ Registers a command with the given 'name' to be exposed by the
 UnixctlServer. 'usage' describes the arguments to the command; it is used
-only for presentation to the user in "help" output.
+only for presentation to the user in "help" output.  'output_fmts' is a
+bitmap that defines what output formats a command supports, e.g.
+OVS_OUTPUT_FMT_TEXT | OVS_OUTPUT_FMT_JSON.
 
 'callback' is called when the command is received.  It is passed a
 UnixctlConnection object, the list of arguments as unicode strings, and
@@ -63,8 +70,30 @@ def command_register(name, usage, min_args, max_args, 
callback, aux):
 assert callable(callback)
 
 if name not in commands:
-commands[name] = _UnixctlCommand(usage, min_args, max_args, callback,
- aux)
+commands[name] = _UnixctlCommand(usage, min_args, max_args,
+ # FIXME: output_fmts,
+ callback, aux)
+
+
+# FIXME: command_register() will be replaced with command_register_fmt() in a
+#later patch of this series. It is is kept temporarily to reduce the
+#amount of changes in this patch.
+def command_register(name, usage, min_args, max_args, callback, aux):
+""" Registers a command with the given 'name' to be exposed by the
+UnixctlServer. 'usage' describes the arguments to the command; it is used
+only for presentation to the user in "help" output.
+
+'callback' is called when the command is received.  It is passed a
+UnixctlConnection object, the list of arguments as unicode strings, and
+'aux'.  Normally 'callback' should reply by calling
+UnixctlConnection.reply() or UnixctlConnection.reply_error() before it
+returns, but if the command cannot be handled immediately, then it can
+defer the reply until later.  A given connection can only process a single
+request at a time, so a reply must be made eventually to avoid blocking
+that connection."""
+
+command_register_fmt(name, usage, min_args, max_args,
+ ovs.util.OutputFormat.TEXT, callback, aux)
 
 
 def socket_name_from_target(target):
diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
index 8283f99bb..29a5f3df9 100644
--- a/python/ovs/unixctl/client.py
+++ b/python/ovs/unixctl/client.py
@@ -26,13 +26,20 @@ class UnixctlClient(object):
 

[ovs-dev] [PATCH v4 4/6] ofproto: Add JSON output for 'dpif/show' command.

[jmeng]

From: Jakob Meng 

The 'dpif/show' command now supports machine-readable JSON output in
addition to the plain-text output for humans. An example would be:

  ovs-appctl --format json dpif/show

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 ofproto/ofproto-dpif.c | 129 -
 tests/pmd.at   |  28 +
 2 files changed, 144 insertions(+), 13 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 516ec6280..eedf65e07 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -20,6 +20,7 @@
 #include "bond.h"
 #include "bundle.h"
 #include "byte-order.h"
+#include "command-line.h"
 #include "connectivity.h"
 #include "connmgr.h"
 #include "coverage.h"
@@ -6360,8 +6361,103 @@ done:
 return changed;
 }
 
+static struct json *
+dpif_show_backer_json(const struct dpif_backer *backer)
+{
+struct json *json_backer = json_object_create();
+
+/* Add dpif name to JSON. */
+json_object_put_string(json_backer, "name", dpif_name(backer->dpif));
+
+/* Add dpif stats to JSON. */
+struct dpif_dp_stats dp_stats;
+dpif_get_dp_stats(backer->dpif, _stats);
+struct json *json_dp_stats = json_object_create();
+json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, dp_stats.n_hit);
+json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
+   dp_stats.n_missed);
+json_object_put(json_backer, "stats", json_dp_stats);
+
+/* Add ofprotos to JSON. */
+struct json *json_ofprotos = json_array_create_empty();
+struct shash ofproto_shash;
+shash_init(_shash);
+const struct shash_node **ofprotos = get_ofprotos(_shash);
+for (size_t i = 0; i < shash_count(_shash); i++) {
+struct ofproto_dpif *ofproto = ofprotos[i]->data;
+
+if (ofproto->backer != backer) {
+continue;
+}
+
+struct json *json_ofproto = json_object_create();
+
+/* Add ofproto name to JSON array. */
+json_object_put_string(json_ofproto, "name", ofproto->up.name);
+
+/* Add ofproto ports to JSON array. */
+struct json *json_ofproto_ports = json_array_create_empty();
+const struct shash_node **ports;
+ports = shash_sort(>up.port_by_name);
+for (size_t j = 0; j < shash_count(>up.port_by_name); j++) {
+const struct shash_node *port = ports[j];
+struct ofport *ofport = port->data;
+
+struct json * json_ofproto_port = json_object_create();
+/* Add ofproto port netdev name to JSON sub array. */
+json_object_put_string(json_ofproto_port, "netdev_name",
+   netdev_get_name(ofport->netdev));
+/* Add ofproto port ofp port to JSON sub array. */
+json_object_put_format(json_ofproto_port, "ofp_port", "%u",
+   ofport->ofp_port);
+
+/* Add ofproto port odp port to JSON sub array. */
+odp_port_t odp_port = ofp_port_to_odp_port(ofproto,
+   ofport->ofp_port);
+if (odp_port != ODPP_NONE) {
+json_object_put_format(json_ofproto_port, "odp_port",
+   "%"PRIu32, odp_port);
+} else {
+json_object_put_string(json_ofproto_port, "odp_port", "none");
+}
+
+/* Add ofproto port netdev type to JSON sub array. */
+json_object_put_string(json_ofproto_port, "netdev_type",
+   netdev_get_type(ofport->netdev));
+
+/* Add ofproto port config to JSON sub array. */
+struct json *json_port_config = json_object_create();
+struct smap config;
+smap_init();
+if (!netdev_get_config(ofport->netdev, )) {
+struct smap_node *node;
+
+SMAP_FOR_EACH (node, ) {
+json_object_put_string(json_port_config, node->key,
+   node->value);
+}
+}
+smap_destroy();
+json_object_put(json_ofproto_port, "netdev_config",
+json_port_config);
+
+json_array_add(json_ofproto_ports, json_ofproto_port);
+} /* End of ofproto port(s). */
+
+free(ports);
+json_object_put(json_ofproto, "ports", json_ofproto_ports);
+
+json_array_add(json_ofprotos, json_ofproto);
+} /* End of ofproto(s). */
+shash_destroy(_shash);
+free(ofprotos);
+
+json_object_put(json_backer, "ofprotos", json_ofprotos);
+return json_backer;
+}
+
 static void
-dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
+dpif_show_backer_text(const struct dpif_backer *backer, struct ds *ds)
 {
 const struct shash_node **ofprotos;
 struct dpif_dp_stats dp_stats;
@@ -6427,21 +6523,27 @@ 

[ovs-dev] [PATCH v4 5/6] appctl: Add option '--pretty' for pretty-printing JSON output.

[jmeng]

From: Jakob Meng 

Signed-off-by: Jakob Meng 
---
 NEWS   |  3 +++
 lib/unixctl.c  |  4 ++--
 lib/unixctl.h  |  2 +-
 tests/pmd.at   |  2 +-
 utilities/ovs-appctl.c | 19 +--
 5 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/NEWS b/NEWS
index fc77a1613..1796895a7 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,9 @@ Post-v3.2.0
Reported names adjusted accordingly.
  * Added new option [-f|--format] to choose the output format, e.g. 'json'
or 'text' (by default).
+ * Added new option [--pretty] to print JSON output in a readable fashion.
+   E.g. members of objects and elements of arrays are printed one per line,
+   with indentation.
- Python:
  * Added support for choosing the output format, e.g. 'json' or 'text'.
 
diff --git a/lib/unixctl.c b/lib/unixctl.c
index c82a5e92d..e7edbb154 100644
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -553,7 +553,7 @@ unixctl_client_create(const char *path, struct jsonrpc 
**client)
  * '*err' if not NULL. */
 int
 unixctl_client_transact(struct jsonrpc *client, const char *command, int argc,
-char *argv[], enum ovs_output_fmt fmt,
+char *argv[], enum ovs_output_fmt fmt, int fmt_flags,
 char **result, char **err)
 {
 struct jsonrpc_msg *request, *reply;
@@ -622,7 +622,7 @@ unixctl_client_transact(struct jsonrpc *client, const char 
*command, int argc,
 *result = xstrdup(json_string(reply->result));
 } else if (reply->result->type == JSON_OBJECT ||
reply->result->type == JSON_ARRAY) {
-*result = json_to_string(reply->result, 0);
+*result = json_to_string(reply->result, fmt_flags);
 } else {
 VLOG_WARN("%s: unexpected result type in JSON rpc reply: %s",
   jsonrpc_get_name(client),
diff --git a/lib/unixctl.h b/lib/unixctl.h
index 4b8193d9d..33d0152e5 100644
--- a/lib/unixctl.h
+++ b/lib/unixctl.h
@@ -39,7 +39,7 @@ int unixctl_client_create(const char *path, struct jsonrpc 
**client);
 int unixctl_client_transact(struct jsonrpc *client,
 const char *command,
 int argc, char *argv[],
-enum ovs_output_fmt fmt,
+enum ovs_output_fmt fmt, int fmt_flags,
 char **result, char **error);
 
 /* Command registration. */
diff --git a/tests/pmd.at b/tests/pmd.at
index de2dc0974..21d59bf63 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -105,7 +105,7 @@ dummy@ovs-dummy: hit:0 missed:0
 p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
 ])
 
-AT_CHECK([ovs-appctl --format json dpif/show], [0], [dnl
+AT_CHECK([ovs-appctl --format json --pretty dpif/show], [0], [dnl
 [[
   {
 "name": "dummy@ovs-dummy",
diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
index feff1be0b..606f293c5 100644
--- a/utilities/ovs-appctl.c
+++ b/utilities/ovs-appctl.c
@@ -26,6 +26,7 @@
 #include "daemon.h"
 #include "dirs.h"
 #include "openvswitch/dynamic-string.h"
+#include "openvswitch/json.h"
 #include "jsonrpc.h"
 #include "process.h"
 #include "timeval.h"
@@ -38,6 +39,7 @@ static void usage(void);
 /* Parsed command line args. */
 struct cmdl_args {
 enum ovs_output_fmt format;
+int format_flags;
 char *target;
 };
 
@@ -67,7 +69,8 @@ main(int argc, char *argv[])
 cmd_argc = argc - optind;
 cmd_argv = cmd_argc ? argv + optind : NULL;
 error = unixctl_client_transact(client, cmd, cmd_argc, cmd_argv,
-args->format, _result, _error);
+args->format, args->format_flags,
+_result, _error);
 if (error) {
 ovs_fatal(error, "%s: transaction error", args->target);
 }
@@ -113,6 +116,11 @@ Other options:\n\
   --timeout=SECS wait at most SECS seconds for a response\n\
   -f, --format=FMT   Output format. One of: 'json', or 'text'\n\
  ('text', by default)\n\
+  --pretty   By default, JSON in output is printed as compactly as\n\
+ possible. This option causes JSON in output to be\n\
+ printed in a more readable fashion. Members of objects\n\
+ and elements of arrays are printed one per line, with\n\
+ indentation.\n\
   -h, --help Print this helpful information\n\
   -V, --version  Display ovs-appctl version information\n",
program_name, program_name);
@@ -124,6 +132,7 @@ cmdl_args_create(void) {
 struct cmdl_args *args = xmalloc(sizeof *args);
 
 args->format = OVS_OUTPUT_FMT_TEXT;
+args->format_flags = 0;
 args->target = NULL;
 
 return args;
@@ -143,7 +152,8 @@ parse_command_line(int argc, char *argv[])
 {
 enum {
 OPT_START = UCHAR_MAX + 1,
-VLOG_OPTION_ENUMS
+

[ovs-dev] [PATCH v4 1/6] Add global option for JSON output to ovs-appctl.

[jmeng]

From: Jakob Meng 

For monitoring systems such as Prometheus it would be beneficial if
OVS would expose statistics in a machine-readable format.

This patch introduces support for different output formats to ovs-xxx
tools. They gain a global option '-f,--format' which allows users to
request JSON instead of plain-text for humans. An example call
implemented in a later patch is 'ovs-appctl --format json dpif/show'.

For that it is necessary to change the JSON-RPC API lib/unixctl.*
which ovs-xxx tools use to communicate with ovs-vswitchd across unix
sockets. It now allows to transport the requested output format
besides the command and its args. This change has been implemented in
a backward compatible way. One can use an updated client (ovs-appctl)
with an old server (ovs-vswitchd) and vice versa. Of course, JSON
output only works when both sides have been updated.

Previously, the command was copied to the 'method' parameter in
JSON-RPC while the args were copied to the 'params' parameter. Without
any other means to transport parameters via JSON-RPC left, the meaning
of 'method' and 'params' had to be changed: 'method' will now be
'execute/v1' when an output format other than 'text' is requested. In
that case, the first parameter of the JSON array 'params' will now be
the designated command, the second one the output format and the rest
will be command args.

unixctl_command_register() in lib/unixctl.* has been cloned as
unixctl_command_register_fmt() in order to demonstrate the new API.
The latter will be replaced with the former in a later patch. The
new function gained an int argument called 'output_fmts' with which
commands have to announce their support for output formats.

Function type unixctl_cb_func in lib/unixctl.h has gained a new arg
'enum ovs_output_fmt fmt'. For now, it has been added as a comment
only for the huge changes reason mentioned earlier. The output format
which is passed via unix socket to ovs-vswitchd will be converted into
a value of type 'enum ovs_output_fmt' in lib/unixctl.c and then passed
to said 'fmt' arg of the choosen command handler (in a future patch).
When a requested output format is not supported by a command,
then process_command() in lib/unixctl.c will return an error.

This patch does not yet pass the choosen output format to commands.
Doing so requires changes to all unixctl_command_register() calls and
all command callbacks. To improve readibility those changes have been
split out into a follow up patch. Respectively, whenever an output
format other than 'text' is choosen for ovs-xxx tools, they will fail.
By default, the output format is plain-text as before.

In popular tools like kubectl the option for output control is usually
called '-o|--output' instead of '-f,--format'. But ovs-appctl already
has an short option '-o' which prints the available ovs-appctl options
('--option'). The now choosen name also better alines with ovsdb-client
where '-f,--format' controls output formatting.

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng 
---
 NEWS   |   2 +
 lib/command-line.c |  36 ++
 lib/command-line.h |  10 ++
 lib/dpctl.h|   4 +
 lib/unixctl.c  | 249 +++--
 lib/unixctl.h  |  17 ++-
 tests/pmd.at   |   5 +
 utilities/ovs-appctl.c |  67 ---
 utilities/ovs-dpctl.c  |   1 +
 9 files changed, 319 insertions(+), 72 deletions(-)

diff --git a/NEWS b/NEWS
index 43aea97b5..12a895629 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,8 @@ Post-v3.2.0
a.k.a. 'configured' values, can be found in the 'status' column of
the Interface table, i.e. with 'ovs-vsctl get interface <..> status'.
Reported names adjusted accordingly.
+ * Added new option [-f|--format] to choose the output format, e.g. 'json'
+   or 'text' (by default).
 
 
 v3.2.0 - 17 Aug 2023
diff --git a/lib/command-line.c b/lib/command-line.c
index 967f4f5d5..775e0430a 100644
--- a/lib/command-line.c
+++ b/lib/command-line.c
@@ -24,6 +24,7 @@
 #include "ovs-thread.h"
 #include "util.h"
 #include "openvswitch/vlog.h"
+#include "openvswitch/json.h"
 
 VLOG_DEFINE_THIS_MODULE(command_line);
 
@@ -53,6 +54,41 @@ ovs_cmdl_long_options_to_short_options(const struct option 
options[])
 return xstrdup(short_options);
 }
 
+const char *
+ovs_output_fmt_to_string(enum ovs_output_fmt fmt)
+{
+switch (fmt) {
+case OVS_OUTPUT_FMT_TEXT:
+return "text";
+
+case OVS_OUTPUT_FMT_JSON:
+return "json";
+
+default:
+return NULL;
+}
+}
+
+struct json *
+ovs_output_fmt_to_json(enum ovs_output_fmt fmt)
+{
+const char *string = ovs_output_fmt_to_string(fmt);
+return string ? json_string_create(string) : NULL;
+}
+
+bool
+ovs_output_fmt_from_string(const char *string, enum ovs_output_fmt *fmt)
+{
+if (!strcmp(string, "text")) {
+*fmt = OVS_OUTPUT_FMT_TEXT;
+} else if (!strcmp(string, "json")) {
+*fmt = 

[ovs-dev] [PATCH v4 0/6] Add global option to output JSON from ovs-appctl cmds.

[jmeng]

From: Jakob Meng 

This patch series builds upon [0] and brings the following changes:
* Last patch in series shows how output format and other args could be passed 
via JSON-RPC API without the indirection introduced in the first patch (at the 
cost of inferiour error handling)
* By default, the JSON output will not be prettyfied nor sorted
* Added global option '--pretty' to ovs-appctl for prettyfy and sort the JSON 
output
* Added missing check for supported output formats to server.py
* Commented @enum.verify(enum.NAMED_FLAGS) because its available in Python 
3.11+ only
* Renamed 'fmt' to 'output_fmts' in Python code to highlight that multiple 
values are possible at the same time
* Added NEWS entries for '--format' and '--pretty' options
* Fixed flake8 errors
* Replaced TODO with FIXME
* Dropped option '--format' from ovs-dpctl
* No longer mention OVS-DPDK in commit msgs
* Adopted comments to style guide.

What has not been changed from previous series is the function type 
unixctl_cb_func in lib/unixctl.h: The output format continues to be passed via 
new arg 'enum ovs_output_fmt fmt'. We discussed whether this should be moved to 
'struct unixctl_conn' or a new struct [1] but did not come to a conclusion yet.

[0] 
https://patchwork.ozlabs.org/project/openvswitch/list/?series=379267=%2A=both
[1] https://patchwork.ozlabs.org/comment/3208683/

In particular, I would like to ask for your opinion on the last patch in the 
series: What do you think about both JSON-RPC API approaches?

Thanks!

Jakob Meng (6):
  Add global option for JSON output to ovs-appctl.
  python: Add global option for JSON output to Python tools.
  Migrate commands to extended unixctl API.
  ofproto: Add JSON output for 'dpif/show' command.
  appctl: Add option '--pretty' for pretty-printing JSON output.
  unixctl: Pass output format as command args via JSON-RPC API.

 NEWS   |   7 ++
 lib/bfd.c  |  16 ++-
 lib/cfm.c  |  13 ++-
 lib/command-line.c |  36 ++
 lib/command-line.h |  10 ++
 lib/coverage.c |  11 +-
 lib/dpctl.c| 129 +
 lib/dpctl.h|   4 +
 lib/dpdk.c |  15 ++-
 lib/dpif-netdev-perf.c |   1 +
 lib/dpif-netdev-perf.h |   1 +
 lib/dpif-netdev.c  |  84 --
 lib/dpif-netlink.c |   6 +-
 lib/lacp.c |   7 +-
 lib/memory.c   |   5 +-
 lib/netdev-dpdk.c  |  16 ++-
 lib/netdev-dummy.c |  30 +++--
 lib/netdev-native-tnl.c|   4 +-
 lib/netdev-native-tnl.h|   4 +-
 lib/netdev-vport.c |   1 +
 lib/odp-execute.c  |  10 +-
 lib/ovs-lldp.c |  16 ++-
 lib/ovs-router.c   |  26 +++--
 lib/rstp.c |  22 ++--
 lib/stopwatch.c|  10 +-
 lib/stp.c  |  20 ++--
 lib/timeval.c  |  13 ++-
 lib/tnl-neigh-cache.c  |  35 +++---
 lib/tnl-ports.c|   6 +-
 lib/unixctl.c  | 206 -
 lib/unixctl.h  |  11 +-
 lib/vlog.c |  46 +---
 ofproto/bond.c |  32 +++--
 ofproto/ofproto-dpif-trace.c   |  10 +-
 ofproto/ofproto-dpif-upcall.c  |  81 +
 ofproto/ofproto-dpif.c | 187 +-
 ofproto/ofproto.c  |  10 +-
 ofproto/tunnel.c   |   4 +-
 ovsdb/file.c   |   4 +-
 ovsdb/ovsdb-client.c   |  40 ---
 ovsdb/ovsdb-server.c   | 122 ++-
 ovsdb/ovsdb.c  |   4 +-
 ovsdb/raft.c   |  28 +++--
 python/ovs/unixctl/__init__.py |  19 +--
 python/ovs/unixctl/client.py   |   9 +-
 python/ovs/unixctl/server.py   |  75 +++-
 python/ovs/util.py |   8 ++
 python/ovs/vlog.py |  12 +-
 tests/appctl.py|   7 +-
 tests/pmd.at   |  33 ++
 tests/test-netflow.c   |   4 +-
 tests/test-sflow.c |   4 +-
 tests/test-unixctl.c   |  33 --
 tests/test-unixctl.py  |  25 ++--
 tests/unixctl-py.at|   7 ++
 utilities/ovs-appctl.c |  82 ++---
 utilities/ovs-dpctl.c  |   1 +
 utilities/ovs-ofctl.c  |  43 ---
 vswitchd/bridge.c  |  25 ++--
 vswitchd/ovs-vswitchd.c|   5 +-
 60 files changed, 1250 insertions(+), 485 deletions(-)

--
2.39.2

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

Re: [ovs-dev] [PATCH ovn] tests: Fix race in "feature inactivity probe" test.

[Ales Musil]

On Wed, Nov 15, 2023 at 2:40 PM Dumitru Ceara  wrote:

> The test incorrectly assumed that "ovn-nbctl --wait=hv sync" will always
> send an OpenFlow barrier to ovs-vswitchd.  That doesn't happen unless
> there are other OpenFlow (rule or group) changes that need to be
> programmed in the datapath.
>
> Fix the test by actually installing new rules in between warp
> invocations.
>
> Spotted in CI, mostly on oversubscribed systems.  A log sample that
> shows that ovn-controller didn't generate any barrier request for the
> nbctl sync request:
>
>   2023-11-15T12:12:22.937Z|00084|vconn|DBG|unix#4: received:
> OFPT_BARRIER_REQUEST (OF1.5) (xid=0x13):
>   2023-11-15T12:12:22.937Z|00085|vconn|DBG|unix#4: sent (Success):
> OFPT_BARRIER_REPLY (OF1.5) (xid=0x13):
>   ...
>   2023-11-15T12:12:23.032Z|00090|unixctl|DBG|received request
> time/warp["6"], id=0
>   2023-11-15T12:12:23.032Z|00091|unixctl|DBG|replying with success, id=0:
> "warped"
>   2023-11-15T12:12:23.042Z|00094|vconn|DBG|unix#3: sent (Success):
> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>   2023-11-15T12:12:23.042Z|00095|vconn|DBG|unix#4: sent (Success):
> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>   2023-11-15T12:12:23.042Z|00097|vconn|DBG|unix#5: sent (Success):
> OFPT_ECHO_REQUEST (OF1.5) (xid=0x0): 0 bytes of payload
>   2023-11-15T12:12:23.042Z|00098|unixctl|DBG|received request
> time/warp["6"], id=0
>   2023-11-15T12:12:23.042Z|00099|unixctl|DBG|replying with success, id=0:
> "warped"
>   2023-11-15T12:12:23.052Z|00100|rconn|ERR|br-int<->unix#3: no response to
> inactivity probe after 60 seconds, disconnecting
>   2023-11-15T12:12:23.052Z|00101|rconn|ERR|br-int<->unix#4: no response to
> inactivity probe after 60 seconds, disconnecting
>   2023-11-15T12:12:23.052Z|00102|rconn|ERR|br-int<->unix#5: no response to
> inactivity probe after 60 seconds, disconnecting
>   2023-11-15T12:12:23.052Z|00103|unixctl|DBG|received request
> time/warp["6"], id=0
>
> Fixes: ff00142808dc ("controller: Fixed ovs/ovn(features) connection lost
> when running more than 120 seconds")
> Signed-off-by: Dumitru Ceara 
> ---
>  tests/ovn.at | 15 +++
>  1 file changed, 15 insertions(+)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index b8c61f87fb..face39e5c2 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -35352,20 +35352,35 @@ as hv1
>  check ovs-vsctl add-br br-phys
>  ovn_attach n1 br-phys 192.168.0.1
>
> +check ovn-nbctl ls-add ls
> +
>  dnl Ensure that there are 4 openflow connections.
>  OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version'
> hv1/ovs-vswitchd.log)" -eq "4"])
>
> +as hv1 check ovs-vsctl -- add-port br-int lsp0   \
> +-- set Interface lsp0 external-ids:iface-id=lsp0 \
> +-- add-port br-int lsp1  \
> +-- set Interface lsp1 external-ids:iface-id=lsp1 \
> +-- add-port br-int lsp2  \
> +-- set Interface lsp2 external-ids:iface-id=lsp2
> +
>  dnl "Wait" 3 times 60 seconds and ensure ovn-controller writes to the
>  dnl openflow connections in the meantime.  This should allow ovs-vswitchd
>  dnl to probe the openflow connections at least twice.
>
>  as hv1 ovs-appctl time/warp 6
> +check ovn-nbctl lsp-add ls lsp0
> +wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
>
>  as hv1 ovs-appctl time/warp 6
> +check ovn-nbctl lsp-add ls lsp1
> +wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
>
>  as hv1 ovs-appctl time/warp 6
> +check ovn-nbctl lsp-add ls lsp2
> +wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
>
>  AT_CHECK([test -z "`grep disconnecting hv1/ovs-vswitchd.log`"])
> --
> 2.39.3
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>

Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

Re: [ovs-dev] [PATCH v2 2/2] mcast-snooping: Flush flood and report ports when deleting interfaces.

[Eelco Chaudron]

On 10 Nov 2023, at 18:52, David Marchand wrote:

> When a configuration change triggers an interface destruction/creation
> (like for example, setting ofport_request), a port object may still be
> referenced as a fport or a rport in the mdb.
>
> Before the fix, when flooding multicast traffic:
> bridge("br0")
> -
>  0. priority 32768
> NORMAL
>  -> forwarding to mcast group port
>  >> mcast flood port is unknown, dropping
>  -> mcast flood port is input port, dropping
>  -> forwarding to mcast flood port
>
> Before the fix, when flooding igmp report traffic:
> bridge("br0")
> -
>  0. priority 32768
> NORMAL
>  >> mcast port is unknown, dropping the report
>  -> forwarding report to mcast flagged port
>  -> mcast port is input port, dropping the Report
>  -> forwarding report to mcast flagged port
>
> Add relevant cleanup and update unit tests.
>
> Fixes: 4fbbf8624868 ("mcast-snooping: Flush ports mdb when VLAN configuration 
> changed.")
> Signed-off-by: David Marchand 

Thanks for the fix, one small nit, and a request for a comment in the test case.

Cheers,

Eelco

> ---
> Changes since v1:
> - updated the test on report flooding,
>
> ---
>  lib/mcast-snooping.c| 15 +++
>  tests/mcast-snooping.at | 38 ++
>  2 files changed, 53 insertions(+)
>
> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> index 029ca28558..34755447f8 100644
> --- a/lib/mcast-snooping.c
> +++ b/lib/mcast-snooping.c
> @@ -948,6 +948,7 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms, 
> void *port)
>  {
>  struct mcast_group *g;
>  struct mcast_mrouter_bundle *m;
> +struct mcast_port_bundle *p;

nit: while we are at it, can we move this to reverse Christmas tree?

  struct mcast_mrouter_bundle *m;
  struct mcast_port_bundle *p;
  struct mcast_group *g;

>
>  if (!mcast_snooping_enabled(ms)) {
>  return;
> @@ -971,5 +972,19 @@ mcast_snooping_flush_bundle(struct mcast_snooping *ms, 
> void *port)
>  }
>  }
>
> +LIST_FOR_EACH_SAFE (p, node, >fport_list) {
> +if (p->port == port) {
> +mcast_snooping_flush_port(p);
> +ms->need_revalidate = true;
> +}
> +}
> +
> +LIST_FOR_EACH_SAFE (p, node, >rport_list) {
> +if (p->port == port) {
> +mcast_snooping_flush_port(p);
> +ms->need_revalidate = true;
> +}
> +}
> +
>  ovs_rwlock_unlock(>rwlock);
>  }
> diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
> index b5474cf392..1ce31168e8 100644
> --- a/tests/mcast-snooping.at
> +++ b/tests/mcast-snooping.at
> @@ -207,6 +207,24 @@ Megaflow: 
> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e
>  Datapath actions: 1,2
>  ])
>
> +AT_CHECK([ovs-vsctl set interface p2 ofport_request=4])

Can we add a comment here (and below) to indicate why we do this? Just to 
understand what we test here.

> +
> +AT_CHECK([ovs-appctl ofproto/trace 
> "in_port(3),eth(src=aa:55:aa:55:00:ff,dst=01:00:5e:01:01:01),eth_type(0x0800),ipv4(src=10.0.0.1,dst=224.1.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)"],
>  [0], [dnl
> +Flow: 
> udp,in_port=3,vlan_tci=0x,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_src=10.0.0.1,nw_dst=224.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=8000
> +
> +bridge("br0")
> +-
> + 0. priority 32768
> +NORMAL
> + -> forwarding to mcast group port
> + -> mcast flood port is input port, dropping
> + -> forwarding to mcast flood port
> +
> +Final flow: unchanged
> +Megaflow: 
> recirc_id=0,eth,udp,in_port=3,dl_src=aa:55:aa:55:00:ff,dl_dst=01:00:5e:01:01:01,nw_dst=224.1.1.1,nw_frag=no
> +Datapath actions: 1,2
> +])
> +
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> @@ -381,6 +399,26 @@ This flow is handled by the userspace slow path because 
> it:
>- Uses action(s) not supported by datapath.
>  ])
>
> +AT_CHECK([ovs-vsctl set interface p3 ofport_request=4])
> +
> +AT_CHECK([ovs-appctl ofproto/trace "in_port(1)" 
> '01005E010101000C29A027A10800451C00014002CBAEAC10221EE001010112140CE9E0010101'],
>  [0], [dnl
> +Flow: 
> ip,in_port=1,vlan_tci=0x,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_src=172.16.34.30,nw_dst=224.1.1.1,nw_proto=2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=18,tp_dst=20
> +
> +bridge("br0")
> +-
> + 0. priority 32768
> +NORMAL
> + -> forwarding report to mcast flagged port
> + -> mcast port is input port, dropping the Report
> + -> forwarding report to mcast flagged port
> +
> +Final flow: unchanged
> +Megaflow: 
> recirc_id=0,eth,ip,in_port=1,dl_src=00:0c:29:a0:27:a1,dl_dst=01:00:5e:01:01:01,nw_proto=2,nw_frag=no
> +Datapath actions: 2,3
> +This flow is handled by the userspace slow path because it:
> +  - Uses action(s) not supported by datapath.
> +])
> +
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> -- 
> 2.41.0
>
> 

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Dumitru Ceara]

On 11/16/23 10:02, Dumitru Ceara wrote:
> On 11/16/23 09:51, Dumitru Ceara wrote:
>> On 11/16/23 09:44, Ales Musil wrote:
>>> On Thu, Nov 16, 2023 at 9:34 AM Dumitru Ceara  wrote:
>>>
 On 11/16/23 00:12, Ilya Maximets wrote:
> On 11/15/23 22:20, Mark Michelson wrote:
>> On 11/15/23 12:02, Ilya Maximets wrote:
>>> On 11/15/23 17:00, Dumitru Ceara wrote:
 All of the above were changed to track the latest available releases.
 Initially that seemed like a good idea but in practice, a new release
 would
 potentially (silently) cause CI runs that used to pass on given stable
 versions to unexpectedly start failing.

 To address that this series pins all versions we can control allowing
 us
 to use different values for different branches.

 NOTE: the last commit of this series will look slightly different on
 stable branches, e.g., on branches <= 23.06 we need Python <= 3.11 and
 Fedora 37.

 The first commit of the series bumps the OVS submodule to include the
 latest fixes to errors reported by recent versions of flake8.

 Changes in v2:
 - added first patch to bump OVS submodule
 - addressed Ales' review comments:
- moved the logic to determine which image to use out of ci.sh;
  it's now in the workflow itself.
- moved setting of OVN_IMAGE in the same block with all related
  env vars
 - added a note for maintainers in the release process documentation
to mention that the new workflow will likely have to be triggered
manually (at least once) when branching for a new release.  GitHub
doesn't allow periodic jobs on non-default branches.
>>>
>>> IMHO, that sounds horrible to maintain.  Can we just build things per
 run?
>>
>> What if we set up the "Containers" action to run on branch creation?
>> Would that adequately substitute for the manual run that creates the
 image?
>>
>> If we only have to run the action once each time a branch is created, I
>> don't think that qualifies as "horrible" to maintain. This could be
>> lumped into our branch creation scripts. However, if it can be
 automated
>> as I suggested above, that would be better.
>
> The job will never be re-run, meaning we'll be using the same container
> for many years.  In case we'll need an update for this container, it will
> need to be done manually for each branch, except for main.
>
> Also, in the patch, the Cirrus CI is set to use the container from main,
> so this will need to be adjusted for each branch, will need to become
> part of the release patches (?).
>
> Another issue with current implementation of containers is that it's
> a pain for external contributor to change dependencies, because they'll
> have to modify the CI in multiple places in order to build and use their
> own containers.
> And with these new changes it will become worse.  For example, I never
 use
> 'main' or 'branch-*' branches in my own forks.  And CI depends on the
 branch
> name now, and it will fall back to an unmaintained old-style-named image.
> Might not be easy to figure out why all your builds are failing,
 especially
> if you're new to the project.
>

 We could try to make it build the container image every time on stable
 branches and avoid any manual steps.

>>
>>> How much value these containers actually have?

 I think you mentioned the benefit lower already:
 - we can share the artifact (container image) with Cirrus CI; aarch64
 container build takes significantly longer
 - we can share the artifact with downstream CIs

>>
>> As patch 2 mentions, it allows for us to use different distro bases for
>> each version.
>
> This can be done by simply specifying 'container: name' in the job,
> GHA will fetch and use that container.
>
>> But I also think that having the container image cached
>> allows for quicker test runs since we are not having to reconstruct the
>> environment each time we perform test runs.
>
> It takes less than 2 minutes to create a full x86 environment.  We spend
> more time re-building OVS and OVN for every variant of tests, even though
> most of them are using the same compiler flags.
>
> Anyways, there is no need to push the container into registry, we can
 build
> it and pass to a next job via artifacts.  I don't think spending extra
 1-2
> minutes per workflow (not job, workflow) is a significant problem, if it
> allows to just build whatever the current dependencies are and not think
> about what kind of outdated build from which branch is going to be used.
>
> The only downside of this is that we 

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Dumitru Ceara]

On 11/16/23 09:51, Dumitru Ceara wrote:
> On 11/16/23 09:44, Ales Musil wrote:
>> On Thu, Nov 16, 2023 at 9:34 AM Dumitru Ceara  wrote:
>>
>>> On 11/16/23 00:12, Ilya Maximets wrote:
 On 11/15/23 22:20, Mark Michelson wrote:
> On 11/15/23 12:02, Ilya Maximets wrote:
>> On 11/15/23 17:00, Dumitru Ceara wrote:
>>> All of the above were changed to track the latest available releases.
>>> Initially that seemed like a good idea but in practice, a new release
>>> would
>>> potentially (silently) cause CI runs that used to pass on given stable
>>> versions to unexpectedly start failing.
>>>
>>> To address that this series pins all versions we can control allowing
>>> us
>>> to use different values for different branches.
>>>
>>> NOTE: the last commit of this series will look slightly different on
>>> stable branches, e.g., on branches <= 23.06 we need Python <= 3.11 and
>>> Fedora 37.
>>>
>>> The first commit of the series bumps the OVS submodule to include the
>>> latest fixes to errors reported by recent versions of flake8.
>>>
>>> Changes in v2:
>>> - added first patch to bump OVS submodule
>>> - addressed Ales' review comments:
>>>- moved the logic to determine which image to use out of ci.sh;
>>>  it's now in the workflow itself.
>>>- moved setting of OVN_IMAGE in the same block with all related
>>>  env vars
>>> - added a note for maintainers in the release process documentation
>>>to mention that the new workflow will likely have to be triggered
>>>manually (at least once) when branching for a new release.  GitHub
>>>doesn't allow periodic jobs on non-default branches.
>>
>> IMHO, that sounds horrible to maintain.  Can we just build things per
>>> run?
>
> What if we set up the "Containers" action to run on branch creation?
> Would that adequately substitute for the manual run that creates the
>>> image?
>
> If we only have to run the action once each time a branch is created, I
> don't think that qualifies as "horrible" to maintain. This could be
> lumped into our branch creation scripts. However, if it can be
>>> automated
> as I suggested above, that would be better.

 The job will never be re-run, meaning we'll be using the same container
 for many years.  In case we'll need an update for this container, it will
 need to be done manually for each branch, except for main.

 Also, in the patch, the Cirrus CI is set to use the container from main,
 so this will need to be adjusted for each branch, will need to become
 part of the release patches (?).

 Another issue with current implementation of containers is that it's
 a pain for external contributor to change dependencies, because they'll
 have to modify the CI in multiple places in order to build and use their
 own containers.
 And with these new changes it will become worse.  For example, I never
>>> use
 'main' or 'branch-*' branches in my own forks.  And CI depends on the
>>> branch
 name now, and it will fall back to an unmaintained old-style-named image.
 Might not be easy to figure out why all your builds are failing,
>>> especially
 if you're new to the project.

>>>
>>> We could try to make it build the container image every time on stable
>>> branches and avoid any manual steps.
>>>
>
>> How much value these containers actually have?
>>>
>>> I think you mentioned the benefit lower already:
>>> - we can share the artifact (container image) with Cirrus CI; aarch64
>>> container build takes significantly longer
>>> - we can share the artifact with downstream CIs
>>>
>
> As patch 2 mentions, it allows for us to use different distro bases for
> each version.

 This can be done by simply specifying 'container: name' in the job,
 GHA will fetch and use that container.

> But I also think that having the container image cached
> allows for quicker test runs since we are not having to reconstruct the
> environment each time we perform test runs.

 It takes less than 2 minutes to create a full x86 environment.  We spend
 more time re-building OVS and OVN for every variant of tests, even though
 most of them are using the same compiler flags.

 Anyways, there is no need to push the container into registry, we can
>>> build
 it and pass to a next job via artifacts.  I don't think spending extra
>>> 1-2
 minutes per workflow (not job, workflow) is a significant problem, if it
 allows to just build whatever the current dependencies are and not think
 about what kind of outdated build from which branch is going to be used.

 The only downside of this is that we can't share artifact with Cirrus CI
 or some downstream CIs, but that also doesn't seem to be a significant
 issue.  1-2 minutes per 

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Dumitru Ceara]

On 11/16/23 09:44, Ales Musil wrote:
> On Thu, Nov 16, 2023 at 9:34 AM Dumitru Ceara  wrote:
> 
>> On 11/16/23 00:12, Ilya Maximets wrote:
>>> On 11/15/23 22:20, Mark Michelson wrote:
 On 11/15/23 12:02, Ilya Maximets wrote:
> On 11/15/23 17:00, Dumitru Ceara wrote:
>> All of the above were changed to track the latest available releases.
>> Initially that seemed like a good idea but in practice, a new release
>> would
>> potentially (silently) cause CI runs that used to pass on given stable
>> versions to unexpectedly start failing.
>>
>> To address that this series pins all versions we can control allowing
>> us
>> to use different values for different branches.
>>
>> NOTE: the last commit of this series will look slightly different on
>> stable branches, e.g., on branches <= 23.06 we need Python <= 3.11 and
>> Fedora 37.
>>
>> The first commit of the series bumps the OVS submodule to include the
>> latest fixes to errors reported by recent versions of flake8.
>>
>> Changes in v2:
>> - added first patch to bump OVS submodule
>> - addressed Ales' review comments:
>>- moved the logic to determine which image to use out of ci.sh;
>>  it's now in the workflow itself.
>>- moved setting of OVN_IMAGE in the same block with all related
>>  env vars
>> - added a note for maintainers in the release process documentation
>>to mention that the new workflow will likely have to be triggered
>>manually (at least once) when branching for a new release.  GitHub
>>doesn't allow periodic jobs on non-default branches.
>
> IMHO, that sounds horrible to maintain.  Can we just build things per
>> run?

 What if we set up the "Containers" action to run on branch creation?
 Would that adequately substitute for the manual run that creates the
>> image?

 If we only have to run the action once each time a branch is created, I
 don't think that qualifies as "horrible" to maintain. This could be
 lumped into our branch creation scripts. However, if it can be
>> automated
 as I suggested above, that would be better.
>>>
>>> The job will never be re-run, meaning we'll be using the same container
>>> for many years.  In case we'll need an update for this container, it will
>>> need to be done manually for each branch, except for main.
>>>
>>> Also, in the patch, the Cirrus CI is set to use the container from main,
>>> so this will need to be adjusted for each branch, will need to become
>>> part of the release patches (?).
>>>
>>> Another issue with current implementation of containers is that it's
>>> a pain for external contributor to change dependencies, because they'll
>>> have to modify the CI in multiple places in order to build and use their
>>> own containers.
>>> And with these new changes it will become worse.  For example, I never
>> use
>>> 'main' or 'branch-*' branches in my own forks.  And CI depends on the
>> branch
>>> name now, and it will fall back to an unmaintained old-style-named image.
>>> Might not be easy to figure out why all your builds are failing,
>> especially
>>> if you're new to the project.
>>>
>>
>> We could try to make it build the container image every time on stable
>> branches and avoid any manual steps.
>>

> How much value these containers actually have?
>>
>> I think you mentioned the benefit lower already:
>> - we can share the artifact (container image) with Cirrus CI; aarch64
>> container build takes significantly longer
>> - we can share the artifact with downstream CIs
>>

 As patch 2 mentions, it allows for us to use different distro bases for
 each version.
>>>
>>> This can be done by simply specifying 'container: name' in the job,
>>> GHA will fetch and use that container.
>>>
 But I also think that having the container image cached
 allows for quicker test runs since we are not having to reconstruct the
 environment each time we perform test runs.
>>>
>>> It takes less than 2 minutes to create a full x86 environment.  We spend
>>> more time re-building OVS and OVN for every variant of tests, even though
>>> most of them are using the same compiler flags.
>>>
>>> Anyways, there is no need to push the container into registry, we can
>> build
>>> it and pass to a next job via artifacts.  I don't think spending extra
>> 1-2
>>> minutes per workflow (not job, workflow) is a significant problem, if it
>>> allows to just build whatever the current dependencies are and not think
>>> about what kind of outdated build from which branch is going to be used.
>>>
>>> The only downside of this is that we can't share artifact with Cirrus CI
>>> or some downstream CIs, but that also doesn't seem to be a significant
>>> issue.  1-2 minutes per workflow is not that much.  Building the aarch64
>>> container takes a lot more time, but switching it from fedora to ubuntu
>>> takes only 10 minutes to 

Re: [ovs-dev] [PATCH ovn v2 1/3] ovs: Bump submodule to include E721 fixes.

[Ales Musil]

On Wed, Nov 15, 2023 at 5:01 PM Dumitru Ceara  wrote:

> Specifically the following commit:
>   fdbf0bb2aed5 ("flake8: Fix E721 check failures.")
>
> Signed-off-by: Dumitru Ceara 
> ---
>  ovs |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovs b/ovs
> index 19770fc307..fdbf0bb2ae 16
> --- a/ovs
> +++ b/ovs
> @@ -1 +1 @@
> -Subproject commit 19770fc307054cd72572348c93904557c3618402
> +Subproject commit fdbf0bb2aed53e70b455eb1adcfda8d8278ea690
>
>

Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

Re: [ovs-dev] [PATCH v2 2/2] checkpatch.at: Add cases to verify skip committer check.

[Eelco Chaudron]

On 5 Nov 2023, at 9:38, Roi Dayan wrote:

> First case without the skip flag should fail.
> Second case uses the skip flag and should pass.
>
> Signed-off-by: Roi Dayan 
> ---

Thanks for adding the testcase.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Ales Musil]

On Thu, Nov 16, 2023 at 9:34 AM Dumitru Ceara  wrote:

> On 11/16/23 00:12, Ilya Maximets wrote:
> > On 11/15/23 22:20, Mark Michelson wrote:
> >> On 11/15/23 12:02, Ilya Maximets wrote:
> >>> On 11/15/23 17:00, Dumitru Ceara wrote:
>  All of the above were changed to track the latest available releases.
>  Initially that seemed like a good idea but in practice, a new release
> would
>  potentially (silently) cause CI runs that used to pass on given stable
>  versions to unexpectedly start failing.
> 
>  To address that this series pins all versions we can control allowing
> us
>  to use different values for different branches.
> 
>  NOTE: the last commit of this series will look slightly different on
>  stable branches, e.g., on branches <= 23.06 we need Python <= 3.11 and
>  Fedora 37.
> 
>  The first commit of the series bumps the OVS submodule to include the
>  latest fixes to errors reported by recent versions of flake8.
> 
>  Changes in v2:
>  - added first patch to bump OVS submodule
>  - addressed Ales' review comments:
> - moved the logic to determine which image to use out of ci.sh;
>   it's now in the workflow itself.
> - moved setting of OVN_IMAGE in the same block with all related
>   env vars
>  - added a note for maintainers in the release process documentation
> to mention that the new workflow will likely have to be triggered
> manually (at least once) when branching for a new release.  GitHub
> doesn't allow periodic jobs on non-default branches.
> >>>
> >>> IMHO, that sounds horrible to maintain.  Can we just build things per
> run?
> >>
> >> What if we set up the "Containers" action to run on branch creation?
> >> Would that adequately substitute for the manual run that creates the
> image?
> >>
> >> If we only have to run the action once each time a branch is created, I
> >> don't think that qualifies as "horrible" to maintain. This could be
> >> lumped into our branch creation scripts. However, if it can be
> automated
> >> as I suggested above, that would be better.
> >
> > The job will never be re-run, meaning we'll be using the same container
> > for many years.  In case we'll need an update for this container, it will
> > need to be done manually for each branch, except for main.
> >
> > Also, in the patch, the Cirrus CI is set to use the container from main,
> > so this will need to be adjusted for each branch, will need to become
> > part of the release patches (?).
> >
> > Another issue with current implementation of containers is that it's
> > a pain for external contributor to change dependencies, because they'll
> > have to modify the CI in multiple places in order to build and use their
> > own containers.
> > And with these new changes it will become worse.  For example, I never
> use
> > 'main' or 'branch-*' branches in my own forks.  And CI depends on the
> branch
> > name now, and it will fall back to an unmaintained old-style-named image.
> > Might not be easy to figure out why all your builds are failing,
> especially
> > if you're new to the project.
> >
>
> We could try to make it build the container image every time on stable
> branches and avoid any manual steps.
>
> >>
> >>> How much value these containers actually have?
>
> I think you mentioned the benefit lower already:
> - we can share the artifact (container image) with Cirrus CI; aarch64
> container build takes significantly longer
> - we can share the artifact with downstream CIs
>
> >>
> >> As patch 2 mentions, it allows for us to use different distro bases for
> >> each version.
> >
> > This can be done by simply specifying 'container: name' in the job,
> > GHA will fetch and use that container.
> >
> >> But I also think that having the container image cached
> >> allows for quicker test runs since we are not having to reconstruct the
> >> environment each time we perform test runs.
> >
> > It takes less than 2 minutes to create a full x86 environment.  We spend
> > more time re-building OVS and OVN for every variant of tests, even though
> > most of them are using the same compiler flags.
> >
> > Anyways, there is no need to push the container into registry, we can
> build
> > it and pass to a next job via artifacts.  I don't think spending extra
> 1-2
> > minutes per workflow (not job, workflow) is a significant problem, if it
> > allows to just build whatever the current dependencies are and not think
> > about what kind of outdated build from which branch is going to be used.
> >
> > The only downside of this is that we can't share artifact with Cirrus CI
> > or some downstream CIs, but that also doesn't seem to be a significant
> > issue.  1-2 minutes per workflow is not that much.  Building the aarch64
> > container takes a lot more time, but switching it from fedora to ubuntu
> > takes only 10 minutes to build, so might be reasonable to just rebuild it
> > on-demand 

Re: [ovs-dev] [PATCH v2 1/2] checkpatch: Add argument to skip committer signoff check.

[Eelco Chaudron]

On 5 Nov 2023, at 9:38, Roi Dayan wrote:

> From: Salem Sol 
>
> Introduce --skip-committer-signoff arg that can be used internally
> by groups using gerrit for code reviews and gerrit maintainers could
> do the rebase instead of the author or push upstream commits to be
> merged through gerrit.
>
> Signed-off-by: Salem Sol 
> Acked-by: Roi Dayan 

Thanks for resubmitting this with a test case!

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH ovn v2 0/3] Stabilize CI by pinning container, runner and Python versions.

[Dumitru Ceara]

On 11/16/23 00:12, Ilya Maximets wrote:
> On 11/15/23 22:20, Mark Michelson wrote:
>> On 11/15/23 12:02, Ilya Maximets wrote:
>>> On 11/15/23 17:00, Dumitru Ceara wrote:
 All of the above were changed to track the latest available releases.
 Initially that seemed like a good idea but in practice, a new release would
 potentially (silently) cause CI runs that used to pass on given stable
 versions to unexpectedly start failing.

 To address that this series pins all versions we can control allowing us
 to use different values for different branches.

 NOTE: the last commit of this series will look slightly different on
 stable branches, e.g., on branches <= 23.06 we need Python <= 3.11 and
 Fedora 37.

 The first commit of the series bumps the OVS submodule to include the
 latest fixes to errors reported by recent versions of flake8.

 Changes in v2:
 - added first patch to bump OVS submodule
 - addressed Ales' review comments:
- moved the logic to determine which image to use out of ci.sh;
  it's now in the workflow itself.
- moved setting of OVN_IMAGE in the same block with all related
  env vars
 - added a note for maintainers in the release process documentation
to mention that the new workflow will likely have to be triggered
manually (at least once) when branching for a new release.  GitHub
doesn't allow periodic jobs on non-default branches.
>>>
>>> IMHO, that sounds horrible to maintain.  Can we just build things per run?
>>
>> What if we set up the "Containers" action to run on branch creation? 
>> Would that adequately substitute for the manual run that creates the image?
>>
>> If we only have to run the action once each time a branch is created, I 
>> don't think that qualifies as "horrible" to maintain. This could be 
>> lumped into our branch creation scripts. However, if it can be automated 
>> as I suggested above, that would be better.
> 
> The job will never be re-run, meaning we'll be using the same container
> for many years.  In case we'll need an update for this container, it will
> need to be done manually for each branch, except for main.
> 
> Also, in the patch, the Cirrus CI is set to use the container from main,
> so this will need to be adjusted for each branch, will need to become
> part of the release patches (?).
> 
> Another issue with current implementation of containers is that it's
> a pain for external contributor to change dependencies, because they'll
> have to modify the CI in multiple places in order to build and use their
> own containers.
> And with these new changes it will become worse.  For example, I never use
> 'main' or 'branch-*' branches in my own forks.  And CI depends on the branch
> name now, and it will fall back to an unmaintained old-style-named image.
> Might not be easy to figure out why all your builds are failing, especially
> if you're new to the project.
> 

We could try to make it build the container image every time on stable
branches and avoid any manual steps.

>>
>>> How much value these containers actually have?

I think you mentioned the benefit lower already:
- we can share the artifact (container image) with Cirrus CI; aarch64
container build takes significantly longer
- we can share the artifact with downstream CIs

>>
>> As patch 2 mentions, it allows for us to use different distro bases for 
>> each version.
> 
> This can be done by simply specifying 'container: name' in the job,
> GHA will fetch and use that container.
> 
>> But I also think that having the container image cached 
>> allows for quicker test runs since we are not having to reconstruct the 
>> environment each time we perform test runs.
> 
> It takes less than 2 minutes to create a full x86 environment.  We spend
> more time re-building OVS and OVN for every variant of tests, even though
> most of them are using the same compiler flags.
> 
> Anyways, there is no need to push the container into registry, we can build
> it and pass to a next job via artifacts.  I don't think spending extra 1-2
> minutes per workflow (not job, workflow) is a significant problem, if it
> allows to just build whatever the current dependencies are and not think
> about what kind of outdated build from which branch is going to be used.
> 
> The only downside of this is that we can't share artifact with Cirrus CI
> or some downstream CIs, but that also doesn't seem to be a significant
> issue.  1-2 minutes per workflow is not that much.  Building the aarch64
> container takes a lot more time, but switching it from fedora to ubuntu
> takes only 10 minutes to build, so might be reasonable to just rebuild it
> on-demand as well.
> 

Ales, why do we need the Cirrus CI image to be Fedora based?

> Not a strong opinion, but the branch name thing might be the most annoying,
> if I understand correctly.
> 

On the (very) short term however, my problem is that I can't