Re: [ovs-dev] [PATCH ovn] northd: Support CIDR-based MAC binding aging threshold.

[Han Zhou]

On Fri, Nov 3, 2023 at 1:08 AM Ales Musil  wrote:
>
>
>
> On Tue, Oct 24, 2023 at 9:36 PM Han Zhou  wrote:
>>
>> Enhance MAC_Binding aging to allow CIDR-based threshold configurations.
>> This enables distinct threshold settings for different IP ranges,
>> applying the longest prefix matching for overlapping ranges.
>>
>> A common use case involves setting a default threshold for all IPs, while
>> disabling aging for a specific range and potentially excluding a subset
>> within that range.
>>
>> Signed-off-by: Han Zhou 
>> ---
>
>
> Hi Han,
>
> thank you for the patch, I have a couple of comments down below.

Thanks for your review!

>
>>
>>  northd/aging.c  | 297 +---
>>  northd/aging.h  |   3 +
>>  northd/northd.c |  11 +-
>>  ovn-nb.xml  |  63 +-
>>  tests/ovn.at|  60 ++
>>  5 files changed, 413 insertions(+), 21 deletions(-)
>>
>> diff --git a/northd/aging.c b/northd/aging.c
>> index f626c72c8ca3..e5868211a63b 100644
>> --- a/northd/aging.c
>> +++ b/northd/aging.c
>> @@ -47,12 +47,253 @@ aging_waker_schedule_next_wake(struct aging_waker
*waker, int64_t next_wake_ms)
>>  }
>>  }
>>
>> +struct threshold_entry {
>> +union {
>> +ovs_be32 ipv4;
>> +struct in6_addr ipv6;
>> +} prefix;
>> +bool is_v4;
>
>
> We can avoid the is_v4 whatsover by storing the address as mapped v4 in
'in6_addr', I saw the concern and we can still have separate arrays for v4
and v6. The parsing IMO becomes much simpler if we use the mapped v4 for
both see down below.
>

I did consider using in6_addr, but it would make the
find_threshold_for_ip() slightly more tedious because for every v4 entry,
even if stored in a separate array, we will still need to call
in6_addr_get_mapped_ipv4() to convert it. While using the union we can
directly access what we need (ipv4 or ipv6).
The is_v4 member was needed when using a single array, but not necessary
any more when storing v4 and v6 entries separately, but I kept it just for
convenience, so that I don't need to use another arg in
parse_threshold_entry() to return the AF type. Using in6_addr can avoid the
extra arg, too, but similarly, we would have to always call the macro
IN6_IS_ADDR_V4MAPPED, which doesn't seem cleaner than the is_v4 field to
me. Using ip46_parse_cidr does save several lines of code, but that code
snippet itself is quite small. So with all these being considered, I think
either approach doesn't really make too much difference. I am totally fine
to change it if you still think a single in6_addr is better than the union.

>>
>> +unsigned int plen;
>> +unsigned int threshold;
>
>
> I personally prefer sized integers.
>
I think it may be better to match the type of related helper functions.
For plen, it is ip[v6]_parse_cidr().
For threshold, it is str_to_uint(), or even smap_get_uint() returns
unsigned int.

>>
>> +};
>> +
>> +/* Contains CIDR-based aging threshold configuration parsed from
>> + * "Logical_Router:options:mac_binding_age_threshold".
>> + *
>> + * This struct is also used for non-CIDR-based threshold, e.g. the ones
from
>> + * "NB_Global:other_config:fdb_age_threshold" for the common
aging_context
>> + * interface.
>> + *
>> + * - The arrays `v4_entries` and `v6_entries` are populated with parsed
entries
>> + *   for IPv4 and IPv6 CIDRs, respectively, along with their associated
>> + *   thresholds.  Entries within these arrays are sorted by prefix
length,
>> + *   starting with the longest.
>> + *
>> + * - If a threshold is provided without an accompanying prefix, it's
captured
>> + *   in `default_threshold`.  In cases with multiple unprefixed
thresholds,
>> + *   `default_threshold` will only store the last one.  */
>> +struct threshold_config {
>> +struct threshold_entry *v4_entries;
>> +int n_v4_entries;
>> +struct threshold_entry *v6_entries;
>> +int n_v6_entries;
>> +unsigned int default_threshold;
>> +};
>> +
>> +static int
>> +compare_entries_by_prefix_length(const void *a, const void *b)
>> +{
>> +const struct threshold_entry *entry_a = a;
>> +const struct threshold_entry *entry_b = b;
>> +
>> +return entry_b->plen - entry_a->plen;
>> +}
>> +
>> +/* Parse an ENTRY in the threshold option, with the format:
>> + * [CIDR:]THRESHOLD
>> + *
>> + * Returns true if successful, false if failed. */
>> +static bool
>> +parse_threshold_entry(const char *str, struct threshold_entry *entry)
>> +{
>> +char *colon_ptr;
>> +unsigned int value;
>> +const char *threshold_str;
>> +
>> +colon_ptr = strrchr(str, ':');
>
>
> I find this a bit confusing and probably error prone, could we use
different delimiters for the threshold? I guess we could change it to
'cidr;threshold,cidr;threshold' WDYT? This would allow us to use
'strtok_r()' here and we could spare the multiple copies and replace with a
single one.
>
I thought about that, too, but decided to use ":" instead of ";" because
"A:B" looks more natural to represent "

Re: [ovs-dev] [PATCH ovn] northd: Support CIDR-based MAC binding aging threshold.

[Ilya Maximets]

On 11/2/23 07:26, Han Zhou wrote:
> 
> 
> On Wed, Nov 1, 2023 at 11:25 AM Ilya Maximets  > wrote:
>>
>> On 10/26/23 18:51, Han Zhou wrote:
>> >
>> >
>> > On Wed, Oct 25, 2023 at 11:15 AM Ilya Maximets > >  > > >> wrote:
>> >>
>> >> On 10/24/23 21:35, Han Zhou wrote:
>> >> > Enhance MAC_Binding aging to allow CIDR-based threshold configurations.
>> >> > This enables distinct threshold settings for different IP ranges,
>> >> > applying the longest prefix matching for overlapping ranges.
>> >> >
>> >> > A common use case involves setting a default threshold for all IPs, 
>> >> > while
>> >> > disabling aging for a specific range and potentially excluding a subset
>> >> > within that range.
>> >> >
>> >> > Signed-off-by: Han Zhou mailto:hz...@ovn.org> 
>> >> > >>
>> >> > ---
>> >> >  northd/aging.c  | 297 +---
>> >> >  northd/aging.h  |   3 +
>> >> >  northd/northd.c |  11 +-
>> >> >  ovn-nb.xml      |  63 +-
>> >> >  tests/ovn.at  >    |  60 
>> >> > ++
>> >> >  5 files changed, 413 insertions(+), 21 deletions(-)
>> >> >
>> >> > diff --git a/northd/aging.c b/northd/aging.c
>> >> > index f626c72c8ca3..e5868211a63b 100644
>> >> > --- a/northd/aging.c
>> >> > +++ b/northd/aging.c
>> >> > @@ -47,12 +47,253 @@ aging_waker_schedule_next_wake(struct aging_waker 
>> >> > *waker, int64_t next_wake_ms)
>> >> >      }
>> >> >  }
>> >> >
>> >> > +struct threshold_entry {
>> >> > +    union {
>> >> > +        ovs_be32 ipv4;
>> >> > +        struct in6_addr ipv6;
>> >> > +    } prefix;
>> >> > +    bool is_v4;
>> >>
>> >> Hi, Han.  Thanks for the patch!
>> >>
>> >> Not a full review, but I wonder if it would be cleaner to replace
>> >> all the structure members above with a single 'struct in6_addr prefix;'
>> >> and store ipv4 addresses as ipv6-mapped ipv4.  This will allow to use
>> >> a single array for storing the entries as well.  May save some lines
>> >> of code.
>> >>
>> >> What do you think?
>> >
>> > Thanks Ilya for the review. In fact using a common in6_addr in a single 
>> > array was my first attempt, but then I realized that if there are both 
>> > IPv4 and IPv6 entries in the array, for each mac-binding checking, say if 
>> > it is IPv4, it may have to go through all the IPv6 entries unnecessarily. 
>> > If there are a huge amount of MAC-bindings to check, the time may be 
>> > doubled. Probably this doesn't have a big impact, but using two arrays 
>> > didn't seem too many lines of code, so I went with the current *safe* 
>> > approach. Let me know if you have a strong preference for the alternative, 
>> > and I am happy to change it.
>>
>> That makes sense.
>>
>> Though, if performance here is a concern, we probably should have an
>> actual I-P for MAC binding/FDB aging.  It should be possible to keep
>> track of current entries in a heap or a skiplist based on the expected
>> removal time and adjust whenever actual Sb tables change and not walk
>> over the whole table each time.
>>
>> Another idea to keep in mind is to use a classifier instead of array,
>> but this will only make sense with a large number of different CIDRs
>> in the configuration, otherwise the overhead of the classifier will
>> likely be higher than simple array traversal.
>>
>> With the lack of I-P, I think, it's OK to have the entries in separate
>> arrays for now.
>>
> Cool. I'd say performance is probably not a big concern right now, while I 
> was also trying to avoid adding an obvious performance penalty. I agree that 
> when performance becomes a real concern we should do I-P.
> 
>> >
>> >>
>> >> Also, this patch, probably, needs a NEWS entry.
>> >
>> > I was wondering if this change is big enough to add one. Now I will add it 
>> > since you brought up :).
>>
>> It's a new user-facing API.  Should be in the NEWS, IMO.
>>
> 
> OK, how about the below change:
> 
> 
> diff --git a/NEWS b/NEWS
> index 425dfe0a84e1..6352990ae8f5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,7 @@
>  Post v23.09.0
>  -
> +  - Support CIDR based MAC binding aging threshold. See man ovn-nb for
> +    'mac_binding_age_threshold' for more details.

Looks fine.  Nit: man pages are usually referenced with a number,
i.e. 'See ovs-nb(5)'.

>  
>  OVN v23.09.0 - 15 Sep 2023
>  --
> --
> 
> I can add it in v2 or when merging if there are no other comments.

Sure.  I see Ales posted some comments.

> 
> Thanks,
> Han
> 
>> >
>> > Thanks,
>> > Han
>> >
>> >>
>> >> Best regards, Ilya Maximets.
>>

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

Re: [ovs-dev] [PATCH v7 3/3] netdev-dpdk: Sync and clean {get, set}_config() callbacks.

[Ilya Maximets]

On 10/30/23 10:50, jm...@redhat.com wrote:
> From: Jakob Meng 
> 
> For better usability, the function pairs get_config() and
> set_config() for netdevs should be symmetric: Options which are
> accepted by set_config() should be returned by get_config() and the
> latter should output valid options for set_config() only.
> 
> This patch moves key-value pairs which are no valid options from
> get_config() to the get_status() callback. For example, get_config()
> in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
> previously. For requested rx queues the proper option name is n_rxq,
> so requested_rx_queues has been renamed respectively. Tx queues
> cannot be changed by the user, hence requested_tx_queues has been
> dropped. Both configured_{rx,tx}_queues will be returned as
> n_{r,t}xq in the get_status() callback.
> 
> The netdev dpdk classes no longer share a common get_config() callback,
> instead both the dpdk_class and the dpdk_vhost_client_class define
> their own callbacks. The get_config() callback for dpdk_vhost_class has
> been dropped because it does not have a set_config() callback.
> 
> The documentation in vswitchd/vswitch.xml for status columns as well
> as tests have been updated accordingly.
> 
> Reported-at: https://bugzilla.redhat.com/1949855
> Signed-off-by: Jakob Meng 
> ---
>  Documentation/topics/dpdk/phy.rst |   4 +-
>  NEWS  |   7 ++
>  lib/netdev-dpdk.c | 113 +-
>  tests/system-dpdk.at  |  64 ++---
>  vswitchd/vswitch.xml  |  14 +++-
>  5 files changed, 143 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/topics/dpdk/phy.rst 
> b/Documentation/topics/dpdk/phy.rst
> index f66b106c4..41cc3588a 100644
> --- a/Documentation/topics/dpdk/phy.rst
> +++ b/Documentation/topics/dpdk/phy.rst
> @@ -198,7 +198,7 @@ Example::
> a dedicated queue, it will be explicit::
>  
>$ ovs-vsctl get interface dpdk-p0 status
> -  {..., rx_steering=unsupported}
> +  {..., rx-steering=unsupported}
>  
> More details can often be found in ``ovs-vswitchd.log``::
>  
> @@ -499,7 +499,7 @@ its options::
>  
>  $ ovs-appctl dpctl/show
>  [...]
> -  port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ..., 
> dpdk-vf-mac=00:11:22:33:44:55, ...)
> +  port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
>  
>  $ ovs-vsctl show
>  [...]
> diff --git a/NEWS b/NEWS
> index 6b45492f1..43aea97b5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,13 @@ 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.
> +   - ovs-appctl:
> + * Output of 'dpctl/show' command no longer shows interface configuration
> +   status, only values of the actual configuration options, a.k.a.
> +   'requested' configuration.  The interface configuration status,
> +   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.
>  
>  
>  v3.2.0 - 17 Aug 2023
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 55700250d..29f2b280d 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1905,31 +1905,41 @@ netdev_dpdk_get_config(const struct netdev *netdev, 
> struct smap *args)
>  
>  ovs_mutex_lock(&dev->mutex);
>  
> -smap_add_format(args, "requested_rx_queues", "%d", dev->user_n_rxq);
> -smap_add_format(args, "configured_rx_queues", "%d", netdev->n_rxq);
> -smap_add_format(args, "requested_tx_queues", "%d", dev->requested_n_txq);
> -smap_add_format(args, "configured_tx_queues", "%d", netdev->n_txq);
> -smap_add_format(args, "mtu", "%d", dev->mtu);
> +if (dev->devargs && dev->devargs[0]) {
> +smap_add_format(args, "dpdk-devargs", "%s", dev->devargs);
> +}
>  
> -if (dev->type == DPDK_DEV_ETH) {
> -smap_add_format(args, "n_rxq_desc", "%d", dev->rxq_size);
> -smap_add_format(args, "n_txq_desc", "%d", dev->txq_size);
> -if (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD) {
> -smap_add(args, "rx_csum_offload", "true");
> -} else {
> -smap_add(args, "rx_csum_offload", "false");
> -}
> -if (dev->rx_steer_flags == DPDK_RX_STEER_LACP) {
> -smap_add(args, "rx-steering", "rss+lacp");
> -}
> -smap_add(args, "lsc_interrupt_mode",
> - dev->lsc_interrupt_mode ? "true" : "false");
> +smap_add_format(args, "n_rxq", "%d", dev->user_n_rxq);
>  
> -if (dpdk_port_is_representor(dev)) {
> -smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
> -ETH_ADDR_ARGS(dev->requested_hwaddr));
> -}
> +if (dev->fc_conf.mode == RTE_ETH_FC_TX_PAUSE ||

Re: [ovs-dev] [PATCH v7 2/3] netdev-afxdp: Sync and clean {get, set}_config() callbacks.

[Ilya Maximets]

On 10/30/23 10:49, jm...@redhat.com wrote:
> From: Jakob Meng 
> 
> For better usability, the function pairs get_config() and
> set_config() for netdevs should be symmetric: Options which are
> accepted by set_config() should be returned by get_config() and the
> latter should output valid options for set_config() only. This patch
> also moves key-value pairs which are no valid options from get_config()
> to the get_status() callback.
> 
> The documentation in vswitchd/vswitch.xml for status columns has been
> updated accordingly.
> 
> Reported-at: https://bugzilla.redhat.com/1949855
> Signed-off-by: Jakob Meng 
> ---
>  Documentation/intro/install/afxdp.rst | 12 
>  lib/netdev-afxdp.c| 21 +++--
>  lib/netdev-afxdp.h|  1 +
>  lib/netdev-linux-private.h|  1 +
>  lib/netdev-linux.c|  4 ++--
>  vswitchd/vswitch.xml  | 11 +++
>  6 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/intro/install/afxdp.rst 
> b/Documentation/intro/install/afxdp.rst
> index 51c24bf5b..5776614c8 100644
> --- a/Documentation/intro/install/afxdp.rst
> +++ b/Documentation/intro/install/afxdp.rst
> @@ -219,14 +219,10 @@ Otherwise, enable debugging by::
>ovs-appctl vlog/set netdev_afxdp::dbg
>  
>  To check which XDP mode was chosen by ``best-effort``, you can look for
> -``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``::
> -
> -  # ovs-appctl dpctl/show
> -  netdev@ovs-netdev:
> -<...>
> -port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true,
> -  xdp-mode=best-effort,
> -  xdp-mode-in-use=native-with-zerocopy)
> +``xdp-mode`` in the output of ``ovs-vsctl get interface INT 
> status:xdp-mode``::
> +
> +  # ovs-vsctl get interface ens802f0 status:xdp-mode
> +  "native-with-zerocopy"
>  
>  References
>  --
> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
> index 16f26bc30..b680a1479 100644
> --- a/lib/netdev-afxdp.c
> +++ b/lib/netdev-afxdp.c
> @@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, 
> struct smap *args)
>  ovs_mutex_lock(&dev->mutex);
>  smap_add_format(args, "n_rxq", "%d", netdev->n_rxq);
>  smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name);
> -smap_add_format(args, "xdp-mode-in-use", "%s",
> -xdp_modes[dev->xdp_mode_in_use].name);
>  smap_add_format(args, "use-need-wakeup", "%s",
>  dev->use_need_wakeup ? "true" : "false");
>  ovs_mutex_unlock(&dev->mutex);
> @@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev,
>  
>  return error;
>  }
> +
> +int
> +netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args)
> +{
> +int error = netdev_linux_get_status(netdev, args);
> +
> +if (error) {
> +return error;
> +}
> +
> +struct netdev_linux *dev = netdev_linux_cast(netdev);
> +
> +ovs_mutex_lock(&dev->mutex);
> +smap_add_format(args, "xdp-mode", "%s",
> +xdp_modes[dev->xdp_mode_in_use].name);
> +ovs_mutex_unlock(&dev->mutex);
> +
> +return error;
> +}
> diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h
> index e91cd102d..bd3b9dfbe 100644
> --- a/lib/netdev-afxdp.h
> +++ b/lib/netdev-afxdp.h
> @@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const 
> struct smap *args,
>  int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args);
>  int netdev_afxdp_get_stats(const struct netdev *netdev_,
> struct netdev_stats *stats);
> +int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args);
>  int netdev_afxdp_get_custom_stats(const struct netdev *netdev,
>struct netdev_custom_stats *custom_stats);
>  
> diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
> index 0ecf0f748..188e8438a 100644
> --- a/lib/netdev-linux-private.h
> +++ b/lib/netdev-linux-private.h
> @@ -50,6 +50,7 @@ struct netdev_rxq_linux {
>  };
>  
>  int netdev_linux_construct(struct netdev *);
> +int netdev_linux_get_status(const struct netdev *, struct smap *);
>  void netdev_linux_run(const struct netdev_class *);
>  
>  int get_stats_via_netlink(const struct netdev *netdev_,
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index cca340879..70521e3c7 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -3493,7 +3493,7 @@ netdev_linux_get_next_hop(const struct in_addr *host, 
> struct in_addr *next_hop,
>  return ENXIO;
>  }
>  
> -static int
> +int
>  netdev_linux_get_status(const struct netdev *netdev_, struct smap *smap)
>  {
>  struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> @@ -3759,7 +3759,7 @@ const struct netdev_class netdev_internal_class = {
>  .destruct = netdev_afxdp_destruct,  \
>  .get_stats = netdev_afxdp_get_stats

Re: [ovs-dev] [PATCH v3 ovn] controller: split mg action in table 39 and 40 to fit kernel netlink buffer size

[Numan Siddique]

On Fri, Nov 3, 2023 at 4:11 AM Ales Musil  wrote:
>
> On Fri, Oct 13, 2023 at 11:29 PM Lorenzo Bianconi
>  wrote:
> >
> > Introduce the capability to split multicast group openflow actions
> > created in consider_mc_group routine in multiple buffers if the
> > single buffer size is over netlink buffer size limits.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2232152
> > Signed-off-by: Lorenzo Bianconi 
> > ---
> > Changes since v2:
> > - code refactoring
> > Changes since v1:
> > - set MC_OFPACTS_MAX_MSG_SIZE to 8K
> > - add dedicated unit-test in ovn.at
> > - add entry in NEWS
> > - improve comments
> > - cosmetics
> > ---
> >  NEWS|   1 +
> >  controller/physical.c   | 226 ++--
> >  controller/pinctrl.c|  66 
> >  include/ovn/actions.h   |   3 +
> >  tests/ovn-controller.at |  45 
> >  tests/ovn.at|  57 ++
> >  6 files changed, 320 insertions(+), 78 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 425dfe0a8..199fc8aeb 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -1,5 +1,6 @@
> >  Post v23.09.0
> >  -
> > +  - introduce the capability to support arbitrarily large multicast groups
> >
> >  OVN v23.09.0 - 15 Sep 2023
> >  --
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 72e88a203..7e6dd8a76 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -1969,6 +1969,57 @@ local_output_pb(int64_t tunnel_key, struct ofpbuf 
> > *ofpacts)
> >  put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts);
> >  }
> >
> > +#define MC_OFPACTS_MAX_MSG_SIZE 8192
> > +#define MC_BUF_START_ID 0x9000
> > +
> > +static void
> > +mc_ofctrl_add_flow(const struct sbrec_multicast_group *mc,
> > +   struct match *match, struct ofpbuf *ofpacts,
> > +   struct ofpbuf *ofpacts_last, uint8_t stage,
> > +   size_t index, uint32_t *pflow_index,
> > +   uint16_t prio, struct ovn_desired_flow_table 
> > *flow_table)
> > +
> > +{
> > +/* do not overcome max netlink message size used by ovs-vswitchd to
> > + * send netlink configuration to the kernel. */
> > +if (ofpacts->size < MC_OFPACTS_MAX_MSG_SIZE && index < mc->n_ports - 
> > 1) {
> > +return;
> > +}
> > +
> > +uint32_t flow_index = *pflow_index;
> > +bool is_first = flow_index == MC_BUF_START_ID;
> > +if (!is_first) {
> > +match_set_reg(match, MFF_REG6 - MFF_REG0, flow_index);
> > +prio += 10;
> > +}
> > +
> > +if (index == mc->n_ports - 1) {
> > +ofpbuf_put(ofpacts, ofpacts_last->data, ofpacts_last->size);
> > +} else {
> > +/* Split multicast groups with size greater than
> > + * MC_OFPACTS_MAX_MSG_SIZE in order to not overcome the
> > + * MAX_ACTIONS_BUFSIZE netlink buffer size supported by the kernel.
> > + * In order to avoid all the action buffers to be squashed 
> > together by
> > + * ovs, add a controller action for each configured openflow.
> > + */
> > +size_t oc_offset = encode_start_controller_op(
> > +ACTION_OPCODE_MG_SPLIT_BUF, false, NX_CTLR_NO_METER, 
> > ofpacts);
> > +ovs_be32 val = htonl(++flow_index);
> > +ofpbuf_put(ofpacts, &val, sizeof val);
> > +val = htonl(mc->tunnel_key);
> > +ofpbuf_put(ofpacts, &val, sizeof val);
> > +ofpbuf_put(ofpacts, &stage, sizeof stage);
> > +encode_finish_controller_op(oc_offset, ofpacts);
> > +}
> > +
> > +ofctrl_add_flow(flow_table, stage, prio, mc->header_.uuid.parts[0],
> > +match, ofpacts, &mc->header_.uuid);
> > +ofpbuf_clear(ofpacts);
> > +/* reset MFF_REG6. */
> > +put_load(0, MFF_REG6, 0, 32, ofpacts);
> > +*pflow_index = flow_index;
> > +}
> > +
> >  static void
> >  consider_mc_group(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >enum mf_field_id mff_ovn_geneve,
> > @@ -1990,9 +2041,6 @@ consider_mc_group(struct ovsdb_idl_index 
> > *sbrec_port_binding_by_name,
> >  struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis);
> >  struct sset vtep_chassis = SSET_INITIALIZER(&vtep_chassis);
> >
> > -struct match match;
> > -match_outport_dp_and_port_keys(&match, dp_key, mc->tunnel_key);
> > -
> >  /* Go through all of the ports in the multicast group:
> >   *
> >   *- For remote ports, add the chassis to 'remote_chassis' or
> > @@ -2014,9 +2062,20 @@ consider_mc_group(struct ovsdb_idl_index 
> > *sbrec_port_binding_by_name,
> >   *  the redirect port was added.
> >   */
> >  struct ofpbuf ofpacts, remote_ofpacts, remote_ofpacts_ramp;
> > +struct ofpbuf ofpacts_last, ofpacts_ramp_last;
> >  ofpbuf_init(&ofpacts, 0);
> >  ofpbuf_init(&remote_ofpacts, 0);
> >  ofpbuf_init(&remote_ofpacts_ramp, 0);
> > +ofpbuf_init(&ofpacts

Re: [ovs-dev] [PATCH] release-process: Update LTS designation schedule example.

[Kevin Traynor]

On 03/11/2023 19:01, Ilya Maximets wrote:

It is an example and the dates are not set in stone, so updating
the table it is not very important.  But it's nice to see currently
supported releases there as well as the near future plans.

Signed-off-by: Ilya Maximets 
---
  Documentation/internals/release-process.rst | 16 ++--
  1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index 0eb8e192a..d939c2d3a 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -96,18 +96,22 @@ LTS designation schedule example (depends on current state 
of development):
  +-+--+--+
  | Version | Release Date | Actions  |
  +-+--+--+
-| 2.14| Aug 2020 | 2.14 - new latest stable, 2.13 stable ⟶ new LTS  |
-+-+--+--+
-| 2.15| Feb 2021 | 2.12 - new latest stable, 2.5  LTS ⟶ EOL |
-+-+--+--+
-| 2.16| Aug 2021 | 2.16 - new latest stable |
-+-+--+--+
  | 2.17| Feb 2022 | 2.17 - new latest stable |
  +-+--+--+
  | 3.0 | Aug 2022 | 3.0  - new latest stable, 2.17 stable ⟶ new LTS  |
  +-+--+--+
  | 3.1 | Feb 2023 | 3.1  - new latest stable, 2.13 LTS ⟶ EOL |
  +-+--+--+
+| 3.2 | Aug 2023 | 3.2  - new latest stable |
++-+--+--+
+| 3.3 | Feb 2024 | 3.3  - new latest stable |
++-+--+--+
+| 3.4 | Aug 2024 | 3.4  - new latest stable, 3.3  stable ⟶ new LTS  |
++-+--+--+
+| 3.5 | Feb 2025 | 3.5  - new latest stable, 2.17 LTS ⟶ EOL |
++-+--+--+
+| 3.6 | Aug 2025 | 3.6  - new latest stable |
++-+--+--+
  
  While branches other than LTS and the latest release are not formally

  maintained, the OVS project usually provides stable releases for these 
branches


Acked-by: Kevin Traynor 

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

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

[Numan Siddique]

On Thu, Nov 2, 2023 at 11:11 AM Xavier Simonart  wrote:
>
> Move the submodule to the tip of OVS branch3.2.  This picks up:
> - 0d0e95cd2ae6 ("ovsdb: Fix potential leak when making diff of conditions.")
> fixing multiple memory leaks in OVN unit tests.
>
> Signed-off-by: Xavier Simonart 

Thanks.  Applied to main.

Numan

> ---
>  ovs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovs b/ovs
> index 1d78a3f31..19770fc30 16
> --- a/ovs
> +++ b/ovs
> @@ -1 +1 @@
> -Subproject commit 1d78a3f3164a6bf651b34f52812f38655b28a9ce
> +Subproject commit 19770fc307054cd72572348c93904557c3618402
> --
> 2.31.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] build-aux/extract-ofp-fields: Fix the number of Summary columns.

[Ilya Maximets]

The table has only 6 columns, not 7.  This doesn't really affect
rendering.  Only slightly affects calculations around how much space
the table needs.

Fixes: 96fee5e0a2a0 ("ovs-fields: New manpage to document Open vSwitch and 
OpenFlow fields.")
Signed-off-by: Ilya Maximets 
---
 build-aux/extract-ofp-fields | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
index 89d80c208..2657d9249 100755
--- a/build-aux/extract-ofp-fields
+++ b/build-aux/extract-ofp-fields
@@ -318,7 +318,7 @@ def group_xml_to_nroff(group_node, fields):
 '.SS "Summary:"\n',
 ".TS\n",
 "tab(;),nowarn;\n",
-"l l l l l l l.\n",
+"l l l l l l.\n",
 "Name;Bytes;Mask;RW?;Prereqs;NXM/OXM Support\n",
 r"\_;\_;\_;\_;\_;\_",
 "\n",
-- 
2.41.0

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

[ovs-dev] [PATCH] release-process: Update LTS designation schedule example.

[Ilya Maximets]

It is an example and the dates are not set in stone, so updating
the table it is not very important.  But it's nice to see currently
supported releases there as well as the near future plans.

Signed-off-by: Ilya Maximets 
---
 Documentation/internals/release-process.rst | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index 0eb8e192a..d939c2d3a 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -96,18 +96,22 @@ LTS designation schedule example (depends on current state 
of development):
 +-+--+--+
 | Version | Release Date | Actions  |
 +-+--+--+
-| 2.14| Aug 2020 | 2.14 - new latest stable, 2.13 stable ⟶ new LTS  |
-+-+--+--+
-| 2.15| Feb 2021 | 2.12 - new latest stable, 2.5  LTS ⟶ EOL |
-+-+--+--+
-| 2.16| Aug 2021 | 2.16 - new latest stable |
-+-+--+--+
 | 2.17| Feb 2022 | 2.17 - new latest stable |
 +-+--+--+
 | 3.0 | Aug 2022 | 3.0  - new latest stable, 2.17 stable ⟶ new LTS  |
 +-+--+--+
 | 3.1 | Feb 2023 | 3.1  - new latest stable, 2.13 LTS ⟶ EOL |
 +-+--+--+
+| 3.2 | Aug 2023 | 3.2  - new latest stable |
++-+--+--+
+| 3.3 | Feb 2024 | 3.3  - new latest stable |
++-+--+--+
+| 3.4 | Aug 2024 | 3.4  - new latest stable, 3.3  stable ⟶ new LTS  |
++-+--+--+
+| 3.5 | Feb 2025 | 3.5  - new latest stable, 2.17 LTS ⟶ EOL |
++-+--+--+
+| 3.6 | Aug 2025 | 3.6  - new latest stable |
++-+--+--+
 
 While branches other than LTS and the latest release are not formally
 maintained, the OVS project usually provides stable releases for these branches
-- 
2.41.0

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

[ovs-dev] [RFC OVN] DHCP Relay Agent support for overlay subnets

[naveen.yerramneni]

This patch contains changes to enable DHCP Relay Agent support for overlay 
subnets.

NOTE:
-
  - This patch has required changes to enable basic DHCP Relay 
functionality for overlay subnets. Sending this for review to get the initial 
feedback about the approach taken.

POST RFC REVIEW

  1. Address review comments/suggestions
  2. Address TODOs
  3. Add unit tests
  4. Complete testing

USE CASE:
--
  - Enable IP address assignment for overlay subnets from the centralized 
DHCP server present in the underlay network.

PREREQUISITES
--
  - Logical Router Port IP should be assigned (statically) from the same 
overlay subnet which is managed by DHCP server.
  - LRP IP is used for GIADRR field when relaying the DHCP packets and also 
same IP needs to be configured as default gateway for the overlay subnet.
  - Overlay subnets managed by external DHCP server are expected to be 
directly reachable from the underlay network.

EXPECTED PACKET FLOW:
--
Following is the expected packet flow inorder to support DHCP rleay 
functionality in OVN.
  1. DHCP client originates DHCP discovery (broadcast).
  2. DHCP relay (running on the OVN) receives the broadcast and forwards 
the packet to the DHCP server by converting it to unicast. While forwarding the 
packet, it updates the GIADDR in DHCP header to its
 interface IP on which DHCP packet is received.
  3. DHCP server uses GIADDR field to decide the IP address pool from which 
IP has to be assigned and DHCP offer is sent to the same IP (GIADDR).
  4. DHCP relay agent forwards the offer to the client, it resets the 
GIADDR field when forwarding the offer to the client.
  5. DHCP client sends DHCP request (broadcast) packet.
  6. DHCP relay (running on the OVN) receives the broadcast and forwards 
the packet to the DHCP server by converting it to unicast. While forwarding the 
packet, it updates the GIADDR in DHCP header to its
 interface IP on which DHCP packet is received.
  7. DHCP Server sends the ACK packet.
  8. DHCP relay agent forwards the ACK packet to the client, it resets the 
GIADDR field when forwarding the ACK to the client.
  9. All the future renew/release packets are directly exchanged between 
DHCP client and DHCP server.

OVN DHCP RELAY PACKET FLOW:

To add DHCP Relay support on OVN, we need to replicate all the behavior 
described above using distributed logical switch and logical router.
At, highlevel packet flow is distributed among Logical Switch and Logical 
Router on source node (where VM is deployed) and redirect chassis(RC) node.
  1. Request packet gets processed on the source node where VM is deployed 
and relays the packet to DHCP server.
  2. Response packet is first processed on RC node (which first recieves 
the packet from underlay network). RC node forwards the packet to the right 
node by filling in the dest MAC and IP.

OVN Packet flow with DHCP relay is explained below.
  1. DHCP client (VM) sends the DHCP discover packet (broadcast).
  2. Logical switch converts the packet to L2 unicast by setting the 
destination MAC to LRP's MAC
  3. Logical Router receives the packet and redirects it to the OVN 
controller.
  4. OVN controller updates the required information(GIADDR) in the DHCP 
payload after doing the required checks. If any check fails, packet is dropped.
  5. Logical Router converts the packet to L3 unicast and forwards it to 
the server. This packets gets routed like any other packet (via RC node).
  6. Server replies with DHCP offer.
  7. RC node processes the DHCP offer and forwards it to the OVN controller.
  8. OVN controller does sanity checks and  updates the destination MAC 
(available in DHCP header), destination IP (available in DHCP header), resets 
GIADDR  and reinjects the packet to datapath.
 If any check fails, packet is dropped.
  9. Logical router updates the source IP and port and forwards the packet 
to logical switch.
  10. Logical switch delivers the packet to the DHCP client.
  11. Similar steps are performed for Request and Ack packets.
  12. All the future renew/release packets are directly exchanged between 
DHCP client and DHCP server

NEW OVN ACTIONS
---

  1. dhcp_relay_req(, )
  - This action executes on the source node on which the DHCP request 
originated.
  - This action relays the DHCP request coming from client to the 
server. Relay-ip is used to update GIADDR in the DHCP header.
  2. dhcp_relay_resp_fwd(, )
  - This action executes on the first node (RC node) which processes 
the DHCP response from the server.
  - This action updates  the destination MAC and destination IP so that 
the response can be forwarded to the appropriat

Re: [ovs-dev] [PATCH 1/1] checkpatch: Don't spell check Fixes tag

[Simon Horman]

On Thu, Nov 02, 2023 at 02:56:41PM +0100, Eelco Chaudron wrote:
> 
> 
> On 2 Nov 2023, at 13:59, Roi Dayan via dev wrote:
> 
> > From: Eli Britstein 
> >
> > Fixes tag quotes another commit that might fail in a spell check. Don't
> > fail it.
> >
> > Signed-off-by: Eli Britstein 
> > Acked-by: Roi Dayan 
> > ---
> >  utilities/checkpatch.py | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> > index a56f429d4c38..6b210fab8385 100755
> > --- a/utilities/checkpatch.py
> > +++ b/utilities/checkpatch.py
> > @@ -409,6 +409,9 @@ def check_spelling(line, comment):
> >  if not spell_check_dict or not spellcheck:
> >  return False
> >
> > +if line.startswith('Fixes: '):
> > +return False
> 
> My initial thought was, should we not match it more precisely with something 
> like is_fixes_exact? But it seems like not an issue as using “Fixes: “ to 
> summarize stuff would fail anyway.
> 
> So this looks good to me!
> 
>   Acked-by: Eelco Chaudron 
> 
> I guess who ever commits this patch can add the . at the end of the subject.

Thanks, applied with a '.' appended to the subject.

- checkpatch: Don't spell check Fixes tag.
  https://github.com/openvswitch/ovs/commit/bf843fd439b2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ovn-ic: wakeup on ovsdb transaction failures

[Numan Siddique]

On Fri, Nov 3, 2023 at 5:30 AM Ales Musil  wrote:
>
> On Mon, Oct 23, 2023 at 12:05 PM Xavier Simonart  wrote:
> >
> > If an ovsdb transaction fails (e.g. adding the same tunnel_key as another
> > ovn-ic to the same datapath, for a different port (race condition)),
> > there was no guarentee that ovn-ic would wake up, and that state would stay
> > until ovn-ic wake up, for instance due to some ovn-ic-sb changes.
> >
> > Signed-off-by: Xavier Simonart 
> > ---
> >  ic/ovn-ic.c | 17 +
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> > index e2023c2ba..bb91bad11 100644
> > --- a/ic/ovn-ic.c
> > +++ b/ic/ovn-ic.c
> > @@ -2216,10 +2216,19 @@ main(int argc, char *argv[])
> >  ovn_db_run(&ctx);
> >  }
> >
> > -ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
> > -ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
> > -ovsdb_idl_loop_commit_and_wait(&ovninb_idl_loop);
> > -ovsdb_idl_loop_commit_and_wait(&ovnisb_idl_loop);
> > +int rc1 = ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
> > +int rc2 = ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
> > +int rc3 = ovsdb_idl_loop_commit_and_wait(&ovninb_idl_loop);
> > +int rc4 = ovsdb_idl_loop_commit_and_wait(&ovnisb_idl_loop);
> > +if (!rc1 || !rc2 || !rc3 || !rc4) {
> > +VLOG_DBG(" a transaction failed in: %s %s %s %s",
> > + !rc1 ? "nb" : "", !rc2 ? "sb" : "",
> > + !rc3 ? "ic_nb" : "", rc4 ? "ic_sb" : "");
> > +/* A transaction failed. Wake up immediately to give
> > + * opportunity to send the proper transaction
> > + */
> > +poll_immediate_wake();
> > +}
> >  } else {
> >  /* ovn-ic is paused
> >   *- we still want to handle any db updates and update the
> > --
> > 2.31.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> Looks good to me, thanks.
>
> Acked-by: Ales Musil 

Thanks.  Applied to main.  Will backport to other branches once the CI passes.

Numan

>
> --
>
> 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ovn-ic: fix potential segmentation violation when ts is deleted

[Numan Siddique]

On Fri, Nov 3, 2023 at 5:26 AM Ales Musil  wrote:
>
> On Mon, Oct 23, 2023 at 11:28 AM Xavier Simonart  wrote:
> >
> > Signed-off-by: Xavier Simonart 
> > ---
> >  ic/ovn-ic.c | 9 +++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> > index e2023c2ba..eec466ec4 100644
> > --- a/ic/ovn-ic.c
> > +++ b/ic/ovn-ic.c
> > @@ -1630,13 +1630,18 @@ collect_lr_routes(struct ic_context *ctx,
> >  const struct icnbrec_transit_switch *key;
> >
> >  struct hmap *routes_ad;
> > +const struct icnbrec_transit_switch *t_sw;
> >  for (int i = 0; i < ic_lr->n_isb_pbs; i++) {
> >  isb_pb = ic_lr->isb_pbs[i];
> >  key = icnbrec_transit_switch_index_init_row(
> >  ctx->icnbrec_transit_switch_by_name);
> >  icnbrec_transit_switch_index_set_name(key, isb_pb->transit_switch);
> > -ts_name = icnbrec_transit_switch_index_find(
> > -ctx->icnbrec_transit_switch_by_name, key)->name;
> > +t_sw = icnbrec_transit_switch_index_find(
> > + ctx->icnbrec_transit_switch_by_name, key);
> > +if (!t_sw) {
> > +continue;
> > +}
> > +ts_name = t_sw->name;
> >  icnbrec_transit_switch_index_destroy_row(key);
> >  routes_ad = shash_find_data(routes_ad_by_ts, ts_name);
> >  if (!routes_ad) {
> > --
> > 2.31.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> Looks good to me, thanks.
>
> Acked-by: Ales Musil 

Thanks.  Applied the patch to  main and backported to branch-23.09.

And then I realize that the patch is leaking the memory by not
destroying the index
if index_find() returns NULL.  I submitted a patch to fix it -
https://patchwork.ozlabs.org/project/ovn/patch/20231103155853.4137224-1-num...@ovn.org/

Request to please take a look.

Numan

>
> --
>
> 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn] ovn-ic: Destroy the created index row for ts.

[numans]

From: Numan Siddique 

Otherwise there would be a memory leak if
icnbrec_transit_switch_index_find() returns NULL.

Fixes: cf1b9920c48e("ovn-ic: fix potential segmentation violation when ts is 
deleted")
Signed-off-by: Numan Siddique 
---
 ic/ovn-ic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index eec466ec4f..9c7b236cb4 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -1638,11 +1638,11 @@ collect_lr_routes(struct ic_context *ctx,
 icnbrec_transit_switch_index_set_name(key, isb_pb->transit_switch);
 t_sw = icnbrec_transit_switch_index_find(
  ctx->icnbrec_transit_switch_by_name, key);
+icnbrec_transit_switch_index_destroy_row(key);
 if (!t_sw) {
 continue;
 }
 ts_name = t_sw->name;
-icnbrec_transit_switch_index_destroy_row(key);
 routes_ad = shash_find_data(routes_ad_by_ts, ts_name);
 if (!routes_ad) {
 routes_ad = xzalloc(sizeof *routes_ad);
-- 
2.41.0

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

Re: [ovs-dev] [PATCH ovn] system-tests: Adjust test for fragmented traffic through LB

[Numan Siddique]

On Wed, Oct 18, 2023 at 9:10 AM Ales Musil  wrote:
>
> Previously it was testing only single scenario, adjust the
> test so it tries all known combinations, that is:
> 1) LB only IP on LS
> 2) LB only IP on LR
> 3) LB IP + proto on LS
> 4) LB IP + proto on LR
>
> Signed-off-by: Ales Musil 

Thanks.  Applied to main.

Numan

> ---
>  tests/system-ovn-kmod.at | 99 ++--
>  1 file changed, 54 insertions(+), 45 deletions(-)
>
> diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
> index 2492b9412..a69e93698 100644
> --- a/tests/system-ovn-kmod.at
> +++ b/tests/system-ovn-kmod.at
> @@ -811,12 +811,11 @@ AT_CLEANUP
>  ])
>
>  OVN_FOR_EACH_NORTHD([
> -AT_SETUP([LB correctly de-fragments traffic])
> +AT_SETUP([LB correctly handles fragmented traffic])
>  AT_KEYWORDS([ovnlb])
>
>  CHECK_CONNTRACK()
>  CHECK_CONNTRACK_NAT()
> -AT_SKIP_IF([test $HAVE_SCAPY = no])
>
>  ovn_start
>  OVS_TRAFFIC_VSWITCHD_START()
> @@ -828,8 +827,6 @@ ADD_BR([br-ext])
>  # connected to a router lr.
>  # internal has a server.
>  # client is connected through localnet.
> -#
> -# Load balancer for udp 192.168.1.20:4242 172.16.1.2 4242.
>
>  check ovs-ofctl add-flow br-ext action=normal
>  # Set external-ids in br-int needed for ovn-controller
> @@ -865,6 +862,7 @@ ovn-nbctl lsp-add public ln_port \
>  ADD_NAMESPACES(client)
>  ADD_VETH(client, client, br-ext, "192.168.1.2/24", "f0:00:00:01:02:03", \
>   "192.168.1.1")
> +NS_EXEC([client], [ip l set dev client mtu 900])
>
>  ADD_NAMESPACES(server)
>  ADD_VETH(server, server, br-int, "172.16.1.2/24", "f0:00:0f:01:02:03", \
> @@ -872,62 +870,73 @@ ADD_VETH(server, server, br-int, "172.16.1.2/24", 
> "f0:00:0f:01:02:03", \
>  check ovn-nbctl lsp-add internal server \
>  -- lsp-set-addresses server "f0:00:0f:01:02:03 172.16.1.2"
>
> -# Config OVN load-balancer with a VIP.
> -check ovn-nbctl lb-add lb1 192.168.1.20:4242 172.16.1.2:4242 udp
> -check ovn-nbctl lr-lb-add lr lb1
>  check ovn-nbctl set logical_router lr options:chassis=hv1
> -check ovn-nbctl set logical_router_port lr-internal options:gateway_mtu=800
>
> -ovn-nbctl --wait=hv sync
> +AT_DATA([client.py], [dnl
> +import socket
>
> -NETNS_DAEMONIZE([server], [nc -l -u 172.16.1.2 4242 > /dev/null], 
> [server.pid])
> +sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
> +sock.sendto(b"x" * 1000, ("172.16.1.20", 4242))
> +])
>
> -# Collect ICMP packets on client side
> -NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
> -icmp > client.pcap 2>client_err], [tcpdump0.pid])
> -OVS_WAIT_UNTIL([grep "listening" client_err])
> +test_fragmented_traffic() {
> +check ovn-nbctl --wait=hv sync
>
> -# Collect UDP packets on server side
> -NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
> -'udp and ip[[6:2]] > 0 and not ip[[6]] = 64' > server.pcap 2>server_err], 
> [tcpdump1.pid])
> -OVS_WAIT_UNTIL([grep "listening" server_err])
> +check ovs-appctl dpctl/flush-conntrack
>
> -check ip netns exec client python3 << EOF
> -import os
> -import socket
> -import sys
> -import time
> +NETNS_DAEMONIZE([server], [nc -l -u 172.16.1.2 4242 > /dev/null], 
> [server.pid])
>
> -FILE="client.pcap"
> +# Collect ICMP packets on client side
> +NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
> +udp > client.pcap 2>client_err], [tcpdump0.pid])
> +OVS_WAIT_UNTIL([grep "listening" client_err])
>
> +# Collect UDP packets on server side
> +NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
> +'udp and ip[[6:2]] > 0 and not ip[[6]] = 64' > server.pcap 
> 2>server_err], [tcpdump1.pid])
> +OVS_WAIT_UNTIL([grep "listening" server_err])
>
> -def contains_string(file, str):
> -file = open(file, "r")
> -for line in file.readlines():
> -if str in line:
> -return True
> -return False
> +NS_CHECK_EXEC([client], [$PYTHON3 ./client.py])
> +OVS_WAIT_UNTIL([test "$(cat server.pcap | wc -l)" = "4"])
>
> +check kill $(cat tcpdump0.pid) $(cat tcpdump1.pid) $(cat server.pid)
> +}
>
> -def need_frag_received():
> -for _ in range(20):
> -if os.path.getsize(FILE) and contains_string(FILE, "need to frag"):
> -return True
> -time.sleep(0.5)
> -return False
> +AS_BOX([LB on router without port and protocol])
> +check ovn-nbctl lb-add lb1 172.16.1.20 172.16.1.2
> +check ovn-nbctl lr-lb-add lr lb1
>
> +test_fragmented_traffic
>
> -sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
> -sock.sendto(b"x" * 1000, ("192.168.1.20", 4242))
> -if need_frag_received():
> -sock.sendto(b"x" * 1000, ("192.168.1.20", 4242))
> -else:
> -print("Missing need frag")
> -sys.exit(1)
> -EOF
> +check ovn-nbctl lr-lb-del lr
> +check ovn-nbctl lb-del lb1
> +
> +AS_BOX([LB on router with port and protocol])
> +check ovn-nbctl lb-add lb1 172.16.1.20:4242 172.16.1.2:4242 udp
> +check ovn-nbctl lr-lb-add lr lb1
> +
> +test_fragmented_traffic
> +
> +check ovn-nbctl lr-lb-del lr
> +check ovn-nb

Re: [ovs-dev] [PATCH ovn] nbctl: Add optional ha-chassis-group arg to ha-chassis-group-list

[Numan Siddique]

On Tue, Oct 17, 2023 at 12:25 PM Evgenii Kovalev  wrote:
>
> This commit adds support for optional ha-chassis-group argument to
> ha-chassis-group-list command to show one record.
>
> Also add few tests to validate output from ha-chassis-group-list.
>
> Signed-off-by: Evgenii Kovalev 

Thanks for the patch and for the contribution.  I added your name to
AUTHORS list and applied the patch to the main.

Numan

> ---
>  tests/ovn-nbctl.at| 53 +
>  utilities/ovn-nbctl.8.xml |  8 +++---
>  utilities/ovn-nbctl.c | 55 +++
>  3 files changed, 96 insertions(+), 20 deletions(-)
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index fde3a28ee..7206d1fe5 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -2741,3 +2741,56 @@ cp $PKIDIR/$cert $cert
>  OVS_WAIT_UNTIL([ovn-appctl -t ovn-nbctl run show])
>
>  AT_CLEANUP
> +
> +dnl -
> +
> +AT_SETUP([ovn-nbctl - ha-chassis-group-list group])
> +OVN_NBCTL_TEST_START daemon
> +check ovn-nbctl ha-chassis-group-add chg1
> +chg1uuid=$(fetch_column nb:HA_Chassis_Group _uuid name=chg1)
> +check ovn-nbctl ha-chassis-group-add chg2
> +chg2uuid=$(fetch_column nb:HA_Chassis_Group _uuid name=chg2)
> +check ovn-nbctl ha-chassis-group-add-chassis chg1 hv1 1
> +check ovn-nbctl ha-chassis-group-add-chassis chg1 hv2 2
> +check ovn-nbctl ha-chassis-group-add-chassis chg2 hv3 3
> +check ovn-nbctl ha-chassis-group-add-chassis chg2 hv4 4
> +AT_CHECK([ovn-nbctl ha-chassis-group-list], [0], [ignore])
> +
> +AT_CHECK_UNQUOTED([ovn-nbctl ha-chassis-group-list $chg1uuid | grep chg1 | 
> awk '{print $1}'], [0], [dnl
> +$chg1uuid
> +])
> +AT_CHECK([ovn-nbctl ha-chassis-group-list chg1 | awk '{print $2}' | grep 
> chg], [0], [dnl
> +(chg1)
> +])
> +AT_CHECK([ovn-nbctl ha-chassis-group-list chg1 | awk '{print $2}' | grep -A1 
> hv1], [0], [dnl
> +(hv1)
> +1
> +])
> +AT_CHECK([ovn-nbctl ha-chassis-group-list chg1 | awk '{print $2}' | grep -A1 
> hv2], [0], [dnl
> +(hv2)
> +2
> +])
> +
> +AT_CHECK_UNQUOTED([ovn-nbctl ha-chassis-group-list $chg2uuid | grep chg2 | 
> awk '{print $1}'], [0], [dnl
> +$chg2uuid
> +])
> +AT_CHECK([ovn-nbctl ha-chassis-group-list chg2 | awk '{print $2}' | grep 
> chg], [0], [dnl
> +(chg2)
> +])
> +AT_CHECK([ovn-nbctl ha-chassis-group-list chg2 | awk '{print $2}' | grep -A1 
> hv3], [0], [dnl
> +(hv3)
> +3
> +])
> +AT_CHECK([ovn-nbctl ha-chassis-group-list chg2 | awk '{print $2}' | grep -A1 
> hv4], [0], [dnl
> +(hv4)
> +4
> +])
> +
> +AT_CHECK([ovn-nbctl ha-chassis-group-list negative], [1], [], [dnl
> +ovn-nbctl: negative: ha_chassis_group name not found
> +])
> +AT_CHECK([ovn-nbctl ha-chassis-group-list 
> ----], [1], [], [dnl
> +ovn-nbctl: ----: ha_chassis_group UUID not 
> found
> +])
> +OVN_NBCTL_TEST_STOP "/terminating with signal 15/d"
> +AT_CLEANUP
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index b4af43185..6f74bd557 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -1466,10 +1466,12 @@
>  group does not exist.
>
>
> -  ha-chassis-group-list
> +  ha-chassis-group-list 
> [ha-chassis-group]
>
> -Lists the HA chassis group group along with the
> -HA chassis if any associated with it.
> +Lists all HA chassis groups along with the HA chassis
> +if any associated with it.
> +If ha-chassis-group is also specified, then only the
> +specified ha-chassis-group will be listed.
>
>
>ha-chassis-group-add-chassis group
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 444fbd2fe..9029e8133 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -464,9 +464,9 @@ Port group commands:\n\
>pg-set-ports PG PORTS   Set PORTS on port group PG\n\
>pg-del PG   Delete port group PG\n\
>  HA chassis group commands:\n\
> -  ha-chassis-group-add GRP  Create an HA chassis group GRP\n\
> -  ha-chassis-group-del GRP  Delete the HA chassis group GRP\n\
> -  ha-chassis-group-list List the HA chassis groups\n\
> +  ha-chassis-group-add GRPCreate an HA chassis group GRP\n\
> +  ha-chassis-group-del GRPDelete the HA chassis group GRP\n\
> +  ha-chassis-group-list [GRP] Print the supplied HA chassis group or all if 
> none supplied\n\
>ha-chassis-group-add-chassis GRP CHASSIS PRIORITY Adds an HA\
>  chassis with mandatory PRIORITY to the HA chassis group GRP\n\
>ha-chassis-group-remove-chassis GRP CHASSIS Removes the HA chassis\
> @@ -7260,7 +7260,7 @@ ha_chassis_group_by_name_or_uuid(struct ctl_context 
> *ctx, const char *id,
>  }
>
>  if (!ha_ch_grp && must_exist) {
> -ctx->error = xasprintf("%s: ha_chassi_group %s not found",
> +ctx->error = xasprintf("%s: ha_chassis_group %s not found",
> 

[ovs-dev] [PATCH ovn 4/5] tests: have CHECK_NO_CHANGE_AFTER_RECOMPUTE potentially wait for ports up

[Xavier Simonart]

As otherwise ports might be down before the recompute and up after,
causing the test to fail. This error happened with
"LSP incremental processing" tests.

Signed-off-by: Xavier Simonart 
---
 tests/ovn-northd.at | 40 
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 196fe01fb..c8e28f2df 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -20,6 +20,14 @@ m4_define([_DUMP_DB_TABLES], [
 # sure nothing is changed by the recompute. It is used for ensuring the
 # correctness of incremental processing.
 m4_define([CHECK_NO_CHANGE_AFTER_RECOMPUTE], [
+wait_port_up=$1
+if test X$wait_port_up = X1; then
+# Wait for hv to have received all previous commands
+# Make sure ports are up as otherwise it might come as a difference 
after recompute
+echo "waiting for ports up"
+check ovn-nbctl --wait=hv sync
+wait_for_ports_up
+fi
 _DUMP_DB_TABLES(before)
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/recompute
 check ovn-nbctl --wait=sb sync
@@ -10092,7 +10100,7 @@ check as northd ovn-appctl -t NORTHD_TYPE 
inc-engine/clear-stats
 check ovn-nbctl --wait=sb sync
 check_recompute_counter 0 0 0
 
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 
 # Associate DHCP for lsp0-2
 ovn-nbctl dhcp-options-create 192.168.0.0/24
@@ -10104,7 +10112,7 @@ check as northd ovn-appctl -t NORTHD_TYPE 
inc-engine/clear-stats
 ovn-nbctl --wait=sb lsp-set-dhcpv4-options lsp0-2 $CIDR_UUID
 check_recompute_counter 0 0 0
 
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 
 # Add IPv6 address and associate DHCPv6 for lsp0-2
 check ovn-nbctl lsp-set-addresses lsp0-2 "aa:aa:aa:00:00:01 192.168.0.11 
aef0::4"
@@ -10115,7 +10123,7 @@ check as northd ovn-appctl -t NORTHD_TYPE 
inc-engine/clear-stats
 ovn-nbctl --wait=sb lsp-set-dhcpv6-options lsp0-2 ${d1}
 check_recompute_counter 0 0 0
 
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 
 check ovn-nbctl --wait=hv ls-del ls0
 
@@ -10142,11 +10150,11 @@ ovn-nbctl lsp-add ls0 ls0-lr0
 ovn-nbctl lsp-set-type ls0-lr0 router
 ovn-nbctl lsp-set-addresses ls0-lr0 router
 check ovn-nbctl --wait=sb lsp-set-options ls0-lr0 router-port=lr0-ls0
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 
 ovn-nbctl lb-add lb0 192.168.0.10:80 10.0.0.10:8080
 check ovn-nbctl --wait=sb ls-lb-add ls0 lb0
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 # Add a lsp.  northd and lflow engine shouldn't recompute even though this is
@@ -10233,14 +10241,14 @@ check ovn-nbctl --wait=sb meter-add m drop 1 pktps
 check ovn-nbctl --wait=sb acl-add ls from-lport 1 1 allow
 dnl Only triggers recompute of the sync_meters and lflow nodes.
 check_recompute_counter 0 2 2
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl --wait=sb meter-del m
 check ovn-nbctl --wait=sb acl-del ls
 dnl Only triggers recompute of the sync_meters and lflow nodes.
 check_recompute_counter 0 2 2
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 
 AT_CLEANUP
 ])
@@ -10410,7 +10418,7 @@ check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 check_engine_stats sync_to_sb_lb recompute nocompute
 
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 
 check ovn-nbctl --wait=sb set load_balancer . 
ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
@@ -10431,7 +10439,7 @@ check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 check_engine_stats sync_to_sb_lb recompute nocompute
 
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 
 check ovn-nbctl --wait=sb -- lb-del lb2 -- lb-del lb3
@@ -10440,7 +10448,7 @@ check_engine_stats northd norecompute compute
 check_engine_stats lflow recompute nocompute
 check_engine_stats sync_to_sb_lb recompute nocompute
 
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 
 AT_CHECK([ovn-nbctl --wait=sb \
@@ -10494,7 +10502,7 @@ check_engine_stats lflow recompute nocompute
 # - a recompute in the first iteration (handling northd change)
 # - a compute in the second iteration (handling SB update)
 check_engine_stats sync_to_sb_lb recompute compute
-CHECK_NO_CHANGE_AFTER_RECOMPUTE
+CHECK_NO_CHANGE_AFTER_RECOMPUTE(1)
 
 # Modify the backend of the lb1 vip
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
@@ -10503,7 +10511,7 @@ check_engine_stats lb_data norecompute compute
 check_engine_stats northd norecompute compute
 check_engine_stats lf

[ovs-dev] [PATCH ovn 5/5] tests: fixed race_condition with max_prefix

[Xavier Simonart]

The following tests:
- ipam
- ipam connectivity
- ipam to non-ipam
set the mac_prefix using nbctl.
However, it can happen that both northd and nbctl sets the mac_prefix
at the same time, which might result in the mac_prefix from nbctl to
be dropped.

Signed-off-by: Xavier Simonart 
---
 tests/ovn.at | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index a855392fc..1f75f8c08 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8408,6 +8408,7 @@ check_dynamic_addresses() {
 check_row_count nb:Logical_Switch_Port 1 name="$1" dynamic_addresses="$arg"
 }
 
+check ovn-nbctl --wait=sb sync
 # Add a port to a switch that does not have a subnet set, then set the
 # subnet which should result in an address being allocated for the port.
 ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
@@ -8723,7 +8724,7 @@ OVN_FOR_EACH_NORTHD([
 AT_SETUP([ipam connectivity])
 ovn_start
 
-ovn-nbctl lr-add R1
+ovn-nbctl --wait=sb lr-add R1
 
 # Test for a ping using dynamically allocated addresses.
 ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
@@ -21177,6 +21178,7 @@ OVN_FOR_EACH_NORTHD([
 AT_SETUP([ipam to non-ipam])
 ovn_start
 
+check ovn-nbctl --wait=sb sync
 ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
 ovn-nbctl ls-add sw0
 ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
-- 
2.31.1

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

[ovs-dev] [PATCH ovn 0/5] More unit tests fixes

[Xavier Simonart]

Xavier Simonart (5):
  tests: fixed "interconnection - static multicast" and "- IGMP/MLD
multicast"
  tests: fixed system test "LR with SNAT fragmentation needed for
external server".
  tests: fixed "ovn-nbctl - daemon retry connection"
  tests: have CHECK_NO_CHANGE_AFTER_RECOMPUTE potentially wait for ports
up
  tests: fixed race_condition with max_prefix

 tests/ovn-nbctl.at   |  4 +++-
 tests/ovn-northd.at  | 40 
 tests/ovn.at | 10 +-
 tests/system-ovn-kmod.at |  4 ++--
 4 files changed, 38 insertions(+), 20 deletions(-)

-- 
2.31.1

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

[ovs-dev] [PATCH ovn 2/5] tests: fixed system test "LR with SNAT fragmentation needed for external server".

[Xavier Simonart]

The test was sometimes failing if IPv6 packets were received.

Signed-off-by: Xavier Simonart 
---
 tests/system-ovn-kmod.at | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index 2492b9412..fb95ba881 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -1217,12 +1217,12 @@ NETNS_DAEMONIZE([server], [$PYTHON3 ./server.py > 
server.log], [server.pid])
 
 dnl Collect packets on server side.
 NETNS_DAEMONIZE([server], [tcpdump -l -U -i server -vnne \
-  'icmp or udp' > server.tcpdump 2>server_err], [tcpdump0.pid])
+  'ip and (icmp or udp)' > server.tcpdump 2>server_err], 
[tcpdump0.pid])
 OVS_WAIT_UNTIL([grep "listening" server_err])
 
 dnl Collect packets on client side.
 NETNS_DAEMONIZE([client], [tcpdump -l -U -i client -vnne \
-  'icmp or udp' > client.tcpdump 2>client_err], [tcpdump1.pid])
+  'ip and (icmp or udp)' > client.tcpdump 2>client_err], 
[tcpdump1.pid])
 OVS_WAIT_UNTIL([grep "listening" client_err])
 
 dnl Send two packets to the server with a short interval.
-- 
2.31.1

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

[ovs-dev] [PATCH ovn 3/5] tests: fixed "ovn-nbctl - daemon retry connection"

[Xavier Simonart]

"kill pid" does not wait for process to be terminated.
Wait for ovsdb-server termination before restarting it.

Signed-off-by: Xavier Simonart 
---
 tests/ovn-nbctl.at | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index fde3a28ee..94641f2f0 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -2695,7 +2695,9 @@ dnl 
-
 
 AT_SETUP([ovn-nbctl - daemon retry connection])
 OVN_NBCTL_TEST_START daemon
-AT_CHECK([kill `cat ovsdb-server.pid`])
+pid=$(cat ovsdb-server.pid)
+AT_CHECK([kill $pid])
+OVS_WAIT_WHILE([kill -0 $pid 2>/dev/null])
 AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --log-file 
--remote=punix:$OVS_RUNDIR/ovnnb_db.sock ovn-nb.db], [0], [], [stderr])
 AT_CHECK([ovn-nbctl show], [0], [ignore])
 OVN_NBCTL_TEST_STOP "/terminating with signal 15/d"
-- 
2.31.1

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

[ovs-dev] [PATCH ovn 1/5] tests: fixed "interconnection - static multicast" and "- IGMP/MLD multicast"

[Xavier Simonart]

Signed-off-by: Xavier Simonart 
---
 tests/ovn.at | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tests/ovn.at b/tests/ovn.at
index 637d92bed..a855392fc 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -26366,10 +26366,13 @@ check ovn-nbctl lsp-add ts ts-lr3 \
 wait_for_ports_up
 
 ovn_as az1
+OVS_WAIT_UNTIL([ovn-nbctl show | grep ts-lr2])
 check ovn-nbctl lsp-set-options ts-lr2 requested-chassis=hv2
+OVS_WAIT_UNTIL([ovn-nbctl show | grep ts-lr3])
 check ovn-nbctl lsp-set-options ts-lr3 requested-chassis=hv2
 
 ovn_as az2
+OVS_WAIT_UNTIL([ovn-nbctl show | grep ts-lr1])
 check ovn-nbctl lsp-set-options ts-lr1 requested-chassis=hv1
 
 dnl Enable unregistered IP multicast flooding and IP multicast relay.
@@ -26578,10 +26581,13 @@ check ovn-nbctl lsp-add ts ts-lr3 \
 wait_for_ports_up
 
 ovn_as az1
+OVS_WAIT_UNTIL([ovn-nbctl show | grep ts-lr2])
 check ovn-nbctl lsp-set-options ts-lr2 requested-chassis=hv2
+OVS_WAIT_UNTIL([ovn-nbctl show | grep ts-lr3])
 check ovn-nbctl lsp-set-options ts-lr3 requested-chassis=hv2
 
 ovn_as az2
+OVS_WAIT_UNTIL([ovn-nbctl show | grep ts-lr1])
 check ovn-nbctl lsp-set-options ts-lr1 requested-chassis=hv1
 
 dnl Enable IP multicast snooping and IP multicast relay.  Reports are
-- 
2.31.1

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

Re: [ovs-dev] [PATCH ovn v4] ci: Remove '--recheck' in CI.

[Ales Musil]

On Wed, Nov 1, 2023 at 10:04 AM Dumitru Ceara  wrote:

> If we want to catch new failures faster we have a better chance if CI
> doesn't auto-retry (once).
>
> There are some tests that are still "unstable" and fail every now and
> then.  In order to reduce the number of false negatives keep the
> --recheck for them.  To achieve that we use a new macro, TAG_UNSTABLE,
> to tag all these tests.  The list of "unstable" tests is compiled based
> on the following discussion:
> https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405465.html
>
> In order to avoid new GitHub actions jobs, we re-purpose the last job of
> each target type to also run the unstable tests.  These jobs were
> already running less tests than others so the additional run time should
> not be an issue.
>
> Signed-off-by: Dumitru Ceara 
> ---
> V4:
> - Correctly pass 'SKIP_UNSTABLE' to system tests.
> - tag more tests (1 unit test, 3 system tests) as unstable.
> V3:
> - Addressed Ales' comment:
>   - pass RECHECK to the container running the tests in ci.sh
> V2:
> - Addressed Ales' comments:
>   - always run stable and unstable tests before declaring pass/fail
> Changes in v1 (since RFC):
> - kept recheck for unstable tests
> - introduced TAG_UNSTABLE
> - changed test.yml to run unstable tests in the last batch of every
>   test target type.
> ---
>  .ci/ci.sh  |  2 +-
>  .ci/linux-build.sh | 77 ++
>  .github/workflows/test.yml | 15 
>  tests/ovn-ic.at|  1 +
>  tests/ovn-ipsec.at |  1 +
>  tests/ovn-macros.at|  5 +++
>  tests/ovn-northd.at|  1 +
>  tests/ovn-performance.at   |  1 +
>  tests/ovn.at   | 14 +++
>  tests/system-ovn.at|  3 ++
>  10 files changed, 97 insertions(+), 23 deletions(-)
>
> diff --git a/.ci/ci.sh b/.ci/ci.sh
> index 3f1b41eadc..6beeace840 100755
> --- a/.ci/ci.sh
> +++ b/.ci/ci.sh
> @@ -101,7 +101,7 @@ function run_tests() {
>  && \
>  ARCH=$ARCH CC=$CC LIBS=$LIBS OPTS=$OPTS TESTSUITE=$TESTSUITE \
>  TEST_RANGE=$TEST_RANGE SANITIZERS=$SANITIZERS DPDK=$DPDK \
> -./.ci/linux-build.sh
> +RECHECK=$RECHECK UNSTABLE=$UNSTABLE ./.ci/linux-build.sh
>  "
>  }
>
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index 7270b9e60a..b0c3c9252e 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -9,6 +9,7 @@ COMMON_CFLAGS=""
>  OVN_CFLAGS=""
>  OPTS="$OPTS --enable-Werror"
>  JOBS=${JOBS:-"-j4"}
> +RECHECK=${RECHECK:-"no"}
>
>  function install_dpdk()
>  {
> @@ -99,6 +100,18 @@ function configure_clang()
>  COMMON_CFLAGS="${COMMON_CFLAGS}
> -Wno-error=unused-command-line-argument"
>  }
>
> +function run_tests()
> +{
> +if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
> +TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=$RECHECK \
> +SKIP_UNSTABLE=$SKIP_UNSTABLE
> +then
> +# testsuite.log is necessary for debugging.
> +cat */_build/sub/tests/testsuite.log
> +return 1
> +fi
> +}
> +
>  function execute_tests()
>  {
>  # 'distcheck' will reconfigure with required options.
> @@ -106,27 +119,61 @@ function execute_tests()
>  configure_ovn
>
>  export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
> -if ! make distcheck CFLAGS="${COMMON_CFLAGS} ${OVN_CFLAGS}" $JOBS \
> -TESTSUITEFLAGS="$JOBS $TEST_RANGE" RECHECK=yes
> -then
> -# testsuite.log is necessary for debugging.
> -cat */_build/sub/tests/testsuite.log
> +
> +local stable_rc=0
> +local unstable_rc=0
> +
> +if ! SKIP_UNSTABLE=yes run_tests; then
> +stable_rc=1
> +fi
> +
> +if [ "$UNSTABLE" ]; then
> +if ! SKIP_UNSTABLE=no TEST_RANGE="-k unstable" RECHECK=yes \
> +run_tests; then
> +unstable_rc=1
> +fi
> +fi
> +
> +if [[ $stable_rc -ne 0 ]] || [[ $unstable_rc -ne 0 ]]; then
>  exit 1
>  fi
>  }
>
> +function run_system_tests()
> +{
> +local type=$1
> +local log_file=$2
> +
> +if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE" \
> +RECHECK=$RECHECK SKIP_UNSTABLE=$SKIP_UNSTABLE; then
> +# $log_file is necessary for debugging.
> +cat tests/$log_file
> +return 1
> +fi
> +}
> +
>  function execute_system_tests()
>  {
> -  type=$1
> -  log_file=$2
> -
> -  configure_ovn $OPTS
> -  make $JOBS || { cat config.log; exit 1; }
> -  if ! sudo make $JOBS $type TESTSUITEFLAGS="$TEST_RANGE"
> RECHECK=yes; then
> -  # $log_file is necessary for debugging.
> -  cat tests/$log_file
> -  exit 1
> -  fi
> +configure_ovn $OPTS
> +make $JOBS || { cat config.log; exit 1; }
> +
> +local stable_rc=0
> +local unstable_rc=0
> +
> +if ! SKIP_UNSTABLE=yes run_system_tests $@; then
> +stable_rc=1
> +fi
> +
> +if [ "$UNSTABLE" ]; then
> +if ! SKIP_UNSTABLE=no TEST_RANGE="-k unstable" REC

Re: [ovs-dev] [PATCH ovn v2] controller: fixed potential segfault when changing tunnel_key and deleting ls

[Ales Musil]

On Mon, Oct 30, 2023 at 9:46 AM Xavier Simonart  wrote:

> When a tunnel_key for a datapath was changed, the local_datapaths hmap was
> not properly updated
> as the old/initial entry was not removed.
> - If the datapath was not deleted at the same time, a new entry (for the
> same dp) was created
>   in the local_datapaths as the previous entry was not found (wrong hash).
> - If the datapath was deleted at the same time, the former entry also
> remained (as, again, the hash
>   was wrong). So, we kept a deleted (dp) entry in the hmap, and might
> crash when we used it later.
>
> When tunnel_key is updated for an existing datapath, we now clean the
> local_datapaths,
> removing bad entries (i.e entries for which the hash is not the
> tunnel_key).
>
> This issue was identified by flaky failures of test "ovn-ic -- route
> deletion upon TS deletion".
>
> Backtrace:
> 0  0x00504a9a in hmap_first_with_hash (hmap=0x20f4d90, 
> hmap@entry=0x5d5634,
> hmap=0x20f4d90, hmap@entry=0x5d5634, hash=1956414673) at
> ./include/openvswitch/hmap.h:360
> 1  smap_find__ (smap=smap@entry=0x20f4d90, key=key@entry=0x5d5634
> "snat-ct-zone", key_len=key_len@entry=12, hash=1956414673) at
> lib/smap.c:421
> 2  0x00505073 in smap_get_node (key=0x5d5634 "snat-ct-zone",
> smap=0x20f4d90) at lib/smap.c:217
> 3  smap_get_def (def=0x0, key=0x5d5634 "snat-ct-zone", smap=0x20f4d90) at
> lib/smap.c:208
> 4  smap_get (smap=0x20f4d90, key=key@entry=0x5d5634 "snat-ct-zone") at
> lib/smap.c:200
> 5  0x00419898 in get_common_nat_zone (ldp=0x20a8c40,
> ldp=0x20a8c40) at controller/lflow.c:831
> 6  add_matches_to_flow_table (lflow=lflow@entry=0x21ddf90, 
> ldp=ldp@entry=0x20a8c40,
> matches=matches@entry=0x211bb50, ptable=ptable@entry=10 '\n',
> output_ptable=output_ptable@entry=37 '%', ovnacts=ovnacts@entry
> =0x7ffd19b99de0,
> ingress=true, l_ctx_in=0x7ffd19b9a300, l_ctx_out=0x7ffd19b9a2c0) at
> controller/lflow.c:892
> 7  0x0041a29b in consider_logical_flow__ (lflow=lflow@entry=0x21ddf90,
> dp=0x20eb720, l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300,
> l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1207
> 8  0x0041a587 in consider_logical_flow (lflow=lflow@entry=0x21ddf90,
> is_recompute=is_recompute@entry=false, l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300,
> l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1276
> 9  0x0041e30f in lflow_handle_changed_flows
> (l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300, 
> l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0)
> at controller/lflow.c:271
> 10 0x00439fc7 in lflow_output_sb_logical_flow_handler
> (node=0x7ffd19ba5b70, data=) at
> controller/ovn-controller.c:4045
> 11 0x004682fe in engine_compute (recompute_allowed= out>, node=) at lib/inc-proc-eng.c:441
> 12 engine_run_node (recompute_allowed=true, node=0x7ffd19ba5b70) at
> lib/inc-proc-eng.c:503
> 13 engine_run (recompute_allowed=recompute_allowed@entry=true) at
> lib/inc-proc-eng.c:528
> 14 0x0040ade2 in main (argc=, argv=)
> at controller/ovn-controller.c:5709
>
> Signed-off-by: Xavier Simonart 
>
> ---
> v2: Fixed potential memory leak.
> ---
>  controller/local_data.c | 14 +
>  controller/local_data.h |  4 
>  controller/ovn-controller.c | 22 +++
>  tests/ovn.at| 42 +
>  4 files changed, 82 insertions(+)
>
> diff --git a/controller/local_data.c b/controller/local_data.c
> index 3a7d3afeb..8f0890a14 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -56,6 +56,20 @@ static bool datapath_is_transit_switch(const struct
> sbrec_datapath_binding *);
>
>  static uint64_t local_datapath_usage;
>
> +/* To be used when hmap_node.hash might be wrong e.g. tunnel_key got
> updated */
> +struct local_datapath *
> +get_local_datapath_no_hash(const struct hmap *local_datapaths,
> +uint32_t tunnel_key)
> +{
> +struct local_datapath *ld;
> +HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> +if (ld->datapath->tunnel_key == tunnel_key) {
> +return ld;
> +}
> +}
> +return NULL;
> +}
> +
>  struct local_datapath *
>  get_local_datapath(const struct hmap *local_datapaths, uint32_t
> tunnel_key)
>  {
> diff --git a/controller/local_data.h b/controller/local_data.h
> index f6d8f725f..a1bf0790b 100644
> --- a/controller/local_data.h
> +++ b/controller/local_data.h
> @@ -69,6 +69,10 @@ struct local_datapath *local_datapath_alloc(
>  const struct sbrec_datapath_binding *);
>  struct local_datapath *get_local_datapath(const struct hmap *,
>uint32_t tunnel_key);
> +struct local_datapath
> +*get_local_datapath_no_hash(const struct hmap *local_datapaths,
> + uint32_t tunnel_key);
> +
>  bool
>  need_add_peer_to_local(
>  struct ovsdb_idl_index *sbrec_port_binding_by_name,
> diff --git a/controller/ovn-controller.c b/contro

Re: [ovs-dev] [PATCH ovn] ovn-ic: wakeup on ovsdb transaction failures

[Ales Musil]

On Mon, Oct 23, 2023 at 12:05 PM Xavier Simonart  wrote:
>
> If an ovsdb transaction fails (e.g. adding the same tunnel_key as another
> ovn-ic to the same datapath, for a different port (race condition)),
> there was no guarentee that ovn-ic would wake up, and that state would stay
> until ovn-ic wake up, for instance due to some ovn-ic-sb changes.
>
> Signed-off-by: Xavier Simonart 
> ---
>  ic/ovn-ic.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index e2023c2ba..bb91bad11 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -2216,10 +2216,19 @@ main(int argc, char *argv[])
>  ovn_db_run(&ctx);
>  }
>
> -ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
> -ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
> -ovsdb_idl_loop_commit_and_wait(&ovninb_idl_loop);
> -ovsdb_idl_loop_commit_and_wait(&ovnisb_idl_loop);
> +int rc1 = ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
> +int rc2 = ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
> +int rc3 = ovsdb_idl_loop_commit_and_wait(&ovninb_idl_loop);
> +int rc4 = ovsdb_idl_loop_commit_and_wait(&ovnisb_idl_loop);
> +if (!rc1 || !rc2 || !rc3 || !rc4) {
> +VLOG_DBG(" a transaction failed in: %s %s %s %s",
> + !rc1 ? "nb" : "", !rc2 ? "sb" : "",
> + !rc3 ? "ic_nb" : "", rc4 ? "ic_sb" : "");
> +/* A transaction failed. Wake up immediately to give
> + * opportunity to send the proper transaction
> + */
> +poll_immediate_wake();
> +}
>  } else {
>  /* ovn-ic is paused
>   *- we still want to handle any db updates and update the
> --
> 2.31.1
>
> ___
> 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 ovn] tests: fixed another set of flaky ovn-ic tests

[Ales Musil]

On Mon, Oct 23, 2023 at 12:05 PM Xavier Simonart 
wrote:

> Some ic tests were failing as not waiting, or not waiting properly
> for some information to be available in the az:
> - Adding a port to ts1 before ts1 being created in that az.
> - Setting options for ports before the port got created in the az.
> - Cleaning up an hv/gw and expecting that it would be immediately
>   visible in different az.
> - Expecting routes updates between az to happen immediately.
>
> Reported-at: https://issues.redhat.com/browse/FDP-98
> Signed-off-by: Xavier Simonart 
> ---
>  tests/ovn-ic.at | 44 ++--
>  1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> index 2ae8dd12d..d4c436f84 100644
> --- a/tests/ovn-ic.at
> +++ b/tests/ovn-ic.at
> @@ -92,6 +92,7 @@ check ovn-nbctl lr-add lr1
>  check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24
>  check ovn-nbctl lrp-set-gateway-chassis lrp1 gw-az1
>
> +OVS_WAIT_UNTIL([ovn-nbctl show | grep switch | grep ts1])
>  check ovn-nbctl lsp-add ts1 lsp1 -- \
>  lsp-set-addresses lsp1 router -- \
>  lsp-set-type lsp1 router -- \
> @@ -156,6 +157,7 @@ create_ic_infra() {
>  ovn_as $az
>
>  check ovn-ic-nbctl ts-add $ts
> +OVS_WAIT_UNTIL([ovn-nbctl show | grep switch | grep $ts])
>  check ovn-nbctl lr-add $lr
>  check ovn-nbctl lrp-add $lr $lrp 00:00:00:00:00:0$az_id
> 10.0.$az_id.1/24
>  check ovn-nbctl lrp-set-gateway-chassis $lrp gw-$az
> @@ -227,6 +229,7 @@ for i in 1 2; do
>  check ovn-nbctl lrp-add lr1 lrp$i 00:00:00:00:0$i:01 10.0.$i.1/24
>  check ovn-nbctl lrp-set-gateway-chassis lrp$i gw-az$i
>
> +OVS_WAIT_UNTIL([ovn-nbctl show | grep switch | grep ts1])
>  check ovn-nbctl lsp-add ts1 lsp$i -- \
>  lsp-set-addresses lsp$i router -- \
>  lsp-set-type lsp$i router -- \
> @@ -290,7 +293,7 @@ ovs-vsctl set open . external-ids:ovn-is-interconn=true
>  OVS_WAIT_UNTIL([ovn_as az2 ovn-sbctl show | grep gw1])
>
>  OVN_CLEANUP_SBOX(gw1)
> -AT_CHECK([ovn_as az2 ovn-sbctl show], [0], [dnl
> +OVS_WAIT_FOR_OUTPUT([ovn_as az2 ovn-sbctl show], [0], [dnl
>  ])
>
>  # Test encap change
> @@ -335,17 +338,17 @@ ovn-nbctl lsp-add ts1 lsp-ts1-lr1
>  ovn-nbctl lsp-set-addresses lsp-ts1-lr1 router
>  ovn-nbctl lsp-set-type lsp-ts1-lr1 router
>  ovn-nbctl --wait=hv lsp-set-options lsp-ts1-lr1 router-port=lrp-lr1-ts1
> +OVS_WAIT_UNTIL([ovn_as az2 ovn-nbctl show | grep lsp-ts1-lr1])
>  ovn_as az2 ovn-nbctl lsp-set-options lsp-ts1-lr1 requested-chassis=gw1
>
> -OVS_WAIT_UNTIL([ovn_as az2 ovn-nbctl show | grep "aa:aa:aa:aa:aa:01
> 169.254.100.1/24"])
> -AT_CHECK([ovn_as az2 ovn-nbctl show | uuidfilt], [0], [dnl
> +OVS_WAIT_FOR_OUTPUT([ovn_as az2 ovn-nbctl show | uuidfilt], [0], [dnl
>  switch <0> (ts1)
>  port lsp-ts1-lr1
>  type: remote
>  addresses: [["aa:aa:aa:aa:aa:01 169.254.100.1/24"]]
>  ])
>
> -AT_CHECK([ovn_as az2 ovn-sbctl -f csv -d bare --no-headings --columns
> logical_port,type list port_binding], [0], [dnl
> +OVS_WAIT_FOR_OUTPUT([ovn_as az2 ovn-sbctl -f csv -d bare --no-headings
> --columns logical_port,type list port_binding], [0], [dnl
>  lsp-ts1-lr1,remote
>  ])
>
> @@ -530,6 +533,7 @@ for i in 1 2; do
>  for j in 1 2; do
>  ts=ts$j$j
>  ovn-ic-nbctl --may-exist ts-add $ts
> +OVS_WAIT_UNTIL([ovn-nbctl show | grep switch | grep $ts])
>
>  # Create LRP and connect to TS
>  lr=lr$j$i
> @@ -588,6 +592,7 @@ for i in 1 2; do
>  # Create LRP and connect to TS
>  ovn-nbctl lr-add lr$i
>  ovn-nbctl lrp-add lr$i lrp-lr$i-ts1 aa:aa:aa:aa:aa:0$i
> 169.254.100.$i/24
> +OVS_WAIT_UNTIL([ovn-nbctl show | grep switch | grep ts1])
>  ovn-nbctl lsp-add ts1 lsp-ts1-lr$i \
>  -- lsp-set-addresses lsp-ts1-lr$i router \
>  -- lsp-set-type lsp-ts1-lr$i router \
> @@ -913,6 +918,7 @@ for i in 1 2; do
>  for j in 1 2 3; do
>  ts=ts1$j
>  ovn-ic-nbctl --may-exist ts-add $ts
> +OVS_WAIT_UNTIL([ovn-nbctl show | grep switch | grep $ts])
>
>  lrp=lrp-$lr-$ts
>  lsp=lsp-$ts-$lr
> @@ -938,6 +944,7 @@ for i in 1 2; do
>  for j in 1 2; do
>  ts=ts2$j
>  ovn-ic-nbctl --may-exist ts-add $ts
> +OVS_WAIT_UNTIL([ovn-nbctl show | grep switch | grep $ts])
>
>  lrp=lrp-$lr-$ts
>  lsp=lsp-$ts-$lr
> @@ -961,7 +968,7 @@ ovn_as az2 ovn-nbctl --route-table=rtb3 lr-route-add
> lr12 10.10.10.0/24 192.168.
>  ovn_as az2 ovn-nbctl --wait=sb lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "
> 192.168.0.1/24"
>
>  # Test direct routes from lr12 were learned to lr11
> -AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 |
> +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep
> 192.168 |
>   grep learned | awk '{print $1, $2, $5}' | sort ], [0], [dnl
>  192.168.0.0/24 169.254.101.2 ecmp
>  192.168.0.0/24 169.254.102.2 ecmp
> @@ -969,24 +976,24 @@ AT_CHECK([ovn

Re: [ovs-dev] [PATCH ovn] ovn-ic: fix potential segmentation violation when ts is deleted

[Ales Musil]

On Mon, Oct 23, 2023 at 11:28 AM Xavier Simonart  wrote:
>
> Signed-off-by: Xavier Simonart 
> ---
>  ic/ovn-ic.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index e2023c2ba..eec466ec4 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -1630,13 +1630,18 @@ collect_lr_routes(struct ic_context *ctx,
>  const struct icnbrec_transit_switch *key;
>
>  struct hmap *routes_ad;
> +const struct icnbrec_transit_switch *t_sw;
>  for (int i = 0; i < ic_lr->n_isb_pbs; i++) {
>  isb_pb = ic_lr->isb_pbs[i];
>  key = icnbrec_transit_switch_index_init_row(
>  ctx->icnbrec_transit_switch_by_name);
>  icnbrec_transit_switch_index_set_name(key, isb_pb->transit_switch);
> -ts_name = icnbrec_transit_switch_index_find(
> -ctx->icnbrec_transit_switch_by_name, key)->name;
> +t_sw = icnbrec_transit_switch_index_find(
> + ctx->icnbrec_transit_switch_by_name, key);
> +if (!t_sw) {
> +continue;
> +}
> +ts_name = t_sw->name;
>  icnbrec_transit_switch_index_destroy_row(key);
>  routes_ad = shash_find_data(routes_ad_by_ts, ts_name);
>  if (!routes_ad) {
> --
> 2.31.1
>
> ___
> 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 ovn v2 0/5] Fixed another set of flaky Unit Tests

[Ales Musil]

On Mon, Oct 23, 2023 at 11:23 AM Xavier Simonart 
wrote:

> Xavier Simonart (5):
>   tests: fixed multiple tests not properly waiting for packets to be
> received
>   tests: do not start backup-northd by default
>   tests: fixed "LSP incremental processing"
>   tests: fixed "ipsec -- basic configuration"
>   tests: wait for all flows to be installed before sending packets
>
>  tests/ovn-controller-vtep.at |   3 -
>  tests/ovn-controller.at  |   6 +-
>  tests/ovn-ipsec.at   |   4 +-
>  tests/ovn-macros.at  |  10 +-
>  tests/ovn-northd.at  |  42 ++---
>  tests/ovn.at | 297 ---
>  6 files changed, 129 insertions(+), 233 deletions(-)
>
> --
> 2.31.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
The whole series 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 v3 ovn] controller: split mg action in table 39 and 40 to fit kernel netlink buffer size

[Ales Musil]

On Fri, Oct 13, 2023 at 11:29 PM Lorenzo Bianconi
 wrote:
>
> Introduce the capability to split multicast group openflow actions
> created in consider_mc_group routine in multiple buffers if the
> single buffer size is over netlink buffer size limits.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2232152
> Signed-off-by: Lorenzo Bianconi 
> ---
> Changes since v2:
> - code refactoring
> Changes since v1:
> - set MC_OFPACTS_MAX_MSG_SIZE to 8K
> - add dedicated unit-test in ovn.at
> - add entry in NEWS
> - improve comments
> - cosmetics
> ---
>  NEWS|   1 +
>  controller/physical.c   | 226 ++--
>  controller/pinctrl.c|  66 
>  include/ovn/actions.h   |   3 +
>  tests/ovn-controller.at |  45 
>  tests/ovn.at|  57 ++
>  6 files changed, 320 insertions(+), 78 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 425dfe0a8..199fc8aeb 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,6 @@
>  Post v23.09.0
>  -
> +  - introduce the capability to support arbitrarily large multicast groups
>
>  OVN v23.09.0 - 15 Sep 2023
>  --
> diff --git a/controller/physical.c b/controller/physical.c
> index 72e88a203..7e6dd8a76 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1969,6 +1969,57 @@ local_output_pb(int64_t tunnel_key, struct ofpbuf 
> *ofpacts)
>  put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts);
>  }
>
> +#define MC_OFPACTS_MAX_MSG_SIZE 8192
> +#define MC_BUF_START_ID 0x9000
> +
> +static void
> +mc_ofctrl_add_flow(const struct sbrec_multicast_group *mc,
> +   struct match *match, struct ofpbuf *ofpacts,
> +   struct ofpbuf *ofpacts_last, uint8_t stage,
> +   size_t index, uint32_t *pflow_index,
> +   uint16_t prio, struct ovn_desired_flow_table *flow_table)
> +
> +{
> +/* do not overcome max netlink message size used by ovs-vswitchd to
> + * send netlink configuration to the kernel. */
> +if (ofpacts->size < MC_OFPACTS_MAX_MSG_SIZE && index < mc->n_ports - 1) {
> +return;
> +}
> +
> +uint32_t flow_index = *pflow_index;
> +bool is_first = flow_index == MC_BUF_START_ID;
> +if (!is_first) {
> +match_set_reg(match, MFF_REG6 - MFF_REG0, flow_index);
> +prio += 10;
> +}
> +
> +if (index == mc->n_ports - 1) {
> +ofpbuf_put(ofpacts, ofpacts_last->data, ofpacts_last->size);
> +} else {
> +/* Split multicast groups with size greater than
> + * MC_OFPACTS_MAX_MSG_SIZE in order to not overcome the
> + * MAX_ACTIONS_BUFSIZE netlink buffer size supported by the kernel.
> + * In order to avoid all the action buffers to be squashed together 
> by
> + * ovs, add a controller action for each configured openflow.
> + */
> +size_t oc_offset = encode_start_controller_op(
> +ACTION_OPCODE_MG_SPLIT_BUF, false, NX_CTLR_NO_METER, 
> ofpacts);
> +ovs_be32 val = htonl(++flow_index);
> +ofpbuf_put(ofpacts, &val, sizeof val);
> +val = htonl(mc->tunnel_key);
> +ofpbuf_put(ofpacts, &val, sizeof val);
> +ofpbuf_put(ofpacts, &stage, sizeof stage);
> +encode_finish_controller_op(oc_offset, ofpacts);
> +}
> +
> +ofctrl_add_flow(flow_table, stage, prio, mc->header_.uuid.parts[0],
> +match, ofpacts, &mc->header_.uuid);
> +ofpbuf_clear(ofpacts);
> +/* reset MFF_REG6. */
> +put_load(0, MFF_REG6, 0, 32, ofpacts);
> +*pflow_index = flow_index;
> +}
> +
>  static void
>  consider_mc_group(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>enum mf_field_id mff_ovn_geneve,
> @@ -1990,9 +2041,6 @@ consider_mc_group(struct ovsdb_idl_index 
> *sbrec_port_binding_by_name,
>  struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis);
>  struct sset vtep_chassis = SSET_INITIALIZER(&vtep_chassis);
>
> -struct match match;
> -match_outport_dp_and_port_keys(&match, dp_key, mc->tunnel_key);
> -
>  /* Go through all of the ports in the multicast group:
>   *
>   *- For remote ports, add the chassis to 'remote_chassis' or
> @@ -2014,9 +2062,20 @@ consider_mc_group(struct ovsdb_idl_index 
> *sbrec_port_binding_by_name,
>   *  the redirect port was added.
>   */
>  struct ofpbuf ofpacts, remote_ofpacts, remote_ofpacts_ramp;
> +struct ofpbuf ofpacts_last, ofpacts_ramp_last;
>  ofpbuf_init(&ofpacts, 0);
>  ofpbuf_init(&remote_ofpacts, 0);
>  ofpbuf_init(&remote_ofpacts_ramp, 0);
> +ofpbuf_init(&ofpacts_last, 0);
> +ofpbuf_init(&ofpacts_ramp_last, 0);
> +
> +bool local_ports = false, remote_ports = false, remote_ramp_ports = 
> false;
> +
> +/* local port loop. */
> +uint32_t flow_index = MC_BUF_START_ID;
> +put_load(0, MFF_REG6, 0, 32, &ofpacts);
> +put_load(mc->tunnel_key, MFF_LOG_

Re: [ovs-dev] [PATCH ovn] northd: Support CIDR-based MAC binding aging threshold.

[Ales Musil]

On Tue, Oct 24, 2023 at 9:36 PM Han Zhou  wrote:

> Enhance MAC_Binding aging to allow CIDR-based threshold configurations.
> This enables distinct threshold settings for different IP ranges,
> applying the longest prefix matching for overlapping ranges.
>
> A common use case involves setting a default threshold for all IPs, while
> disabling aging for a specific range and potentially excluding a subset
> within that range.
>
> Signed-off-by: Han Zhou 
> ---
>

Hi Han,

thank you for the patch, I have a couple of comments down below.


>  northd/aging.c  | 297 +---
>  northd/aging.h  |   3 +
>  northd/northd.c |  11 +-
>  ovn-nb.xml  |  63 +-
>  tests/ovn.at|  60 ++
>  5 files changed, 413 insertions(+), 21 deletions(-)
>
> diff --git a/northd/aging.c b/northd/aging.c
> index f626c72c8ca3..e5868211a63b 100644
> --- a/northd/aging.c
> +++ b/northd/aging.c
> @@ -47,12 +47,253 @@ aging_waker_schedule_next_wake(struct aging_waker
> *waker, int64_t next_wake_ms)
>  }
>  }
>
> +struct threshold_entry {
> +union {
> +ovs_be32 ipv4;
> +struct in6_addr ipv6;
> +} prefix;
> +bool is_v4;
>

We can avoid the is_v4 whatsover by storing the address as mapped v4 in
'in6_addr', I saw the concern and we can still have separate arrays for v4
and v6. The parsing IMO becomes much simpler if we use the mapped v4 for
both see down below.


> +unsigned int plen;
> +unsigned int threshold;
>

I personally prefer sized integers.


> +};
> +
> +/* Contains CIDR-based aging threshold configuration parsed from
> + * "Logical_Router:options:mac_binding_age_threshold".
> + *
> + * This struct is also used for non-CIDR-based threshold, e.g. the ones
> from
> + * "NB_Global:other_config:fdb_age_threshold" for the common aging_context
> + * interface.
> + *
> + * - The arrays `v4_entries` and `v6_entries` are populated with parsed
> entries
> + *   for IPv4 and IPv6 CIDRs, respectively, along with their associated
> + *   thresholds.  Entries within these arrays are sorted by prefix length,
> + *   starting with the longest.
> + *
> + * - If a threshold is provided without an accompanying prefix, it's
> captured
> + *   in `default_threshold`.  In cases with multiple unprefixed
> thresholds,
> + *   `default_threshold` will only store the last one.  */
> +struct threshold_config {
> +struct threshold_entry *v4_entries;
> +int n_v4_entries;
> +struct threshold_entry *v6_entries;
> +int n_v6_entries;
> +unsigned int default_threshold;
> +};
> +
> +static int
> +compare_entries_by_prefix_length(const void *a, const void *b)
> +{
> +const struct threshold_entry *entry_a = a;
> +const struct threshold_entry *entry_b = b;
> +
> +return entry_b->plen - entry_a->plen;
> +}
> +
> +/* Parse an ENTRY in the threshold option, with the format:
> + * [CIDR:]THRESHOLD
> + *
> + * Returns true if successful, false if failed. */
> +static bool
> +parse_threshold_entry(const char *str, struct threshold_entry *entry)
> +{
> +char *colon_ptr;
> +unsigned int value;
> +const char *threshold_str;
> +
> +colon_ptr = strrchr(str, ':');
>

I find this a bit confusing and probably error prone, could we use
different delimiters for the threshold? I guess we could change it to
'cidr;threshold,cidr;threshold' WDYT? This would allow us to use
'strtok_r()' here and we could spare the multiple copies and replace with a
single one.


> +if (!colon_ptr) {
> +threshold_str = str;
> +entry->plen = 0;
> +} else {
> +threshold_str = colon_ptr + 1;
> +}
> +
> +if (!str_to_uint(threshold_str, 10, &value)) {
> +return false;
> +}
> +entry->threshold = value;
> +
> +if (!colon_ptr) {
> +return true;
> +}
> +
> +/* ":" was found, so parse the string before ":" as a cidr. */
> +char ip_cidr[128];
> +ovs_strzcpy(ip_cidr, str, MIN(colon_ptr - str + 1, sizeof ip_cidr));
> +char *error = ip_parse_cidr(ip_cidr, &entry->prefix.ipv4,
> &entry->plen);
>

As mentioned earlier by storing it in 'in6_addr' we could use
'ip46_parse_cidr()' which would simplify the logic. We can also adjust the
prefix for v4 so the search is the same for both.


> +if (!error) {
> +entry->is_v4 = true;
> +return true;
> +}
> +free(error);
> +error = ipv6_parse_cidr(ip_cidr, &entry->prefix.ipv6, &entry->plen);
> +if (!error) {
> +entry->is_v4 = false;
> +return true;
> +}
> +free(error);
> +return false;
> +}
> +
> +static void
> +threshold_config_destroy(struct threshold_config *config)
> +{
> +free(config->v4_entries);
> +free(config->v6_entries);
> +config->v4_entries = config->v6_entries = NULL;
> +config->n_v4_entries = config->n_v6_entries = 0;
> +config->default_threshold = 0;
> +}
> +
> +/* Parse the threshold option string, which has the format:
> + * ENTRY[;ENTRY[...]]
> + *
> + *