[ovs-dev] [PATCH ovn] Remove lport from related_lports if there is no binding

[Priyankar Jain]

Currently during port migration, two chassis (source and destination)
can try to claim the same logical switch port simultaneously for a
short-period of time until the tap is deleted on source hypervisor.
ovn-controllers on these 2 hosts constantly receives port-binding
updates about other chassis claiming the port and as a result it tries
to claim the port again (because its chassis has a tap interface
referencing the LSP). This flapping ends once CMS cleans up tap
interface from the source chassis.

Now following steps occur during a single iteration inc-proc-eng during
flapping:

1. PB update received on OVN controller about other chassis owning the
   port.
2. ovn-controller tries to claim the port.
3. It installs the OVS flows for the port and updates the runtime_data
   to include this port in locally relevant ports.
4. If some change to runtime data happens as part of 3, port-groups
   containing the affected ports are recomputed. It uses related_lports
   runtime data to compute the port-groups.

Finally, ovn-controller sends a port-binding update to SB changing the
chassis to itself.
At a later point of time, SB sends the notification to ovn-controller
about (4) being completed.

Once CMS deletes the tap interface, ovn-controller receives the
notification and updates the runtime data accordingly.

Issue: ovs-flows are (sometimes)not cleaned up upon port migration.

If the notification of OVS interface deletion is received before SB
acks the PortBinding update, then ovn-controller does not cleanup
related_lports leading to incorrect port-groups computation.

i.e if the order of events is as follows:

1. PB update received on OVN controller about other chassis owning the
   port.
2. ovn-controller claims the port, installs OVS flows and sends the
   PortBinding update to SB.
3. OVS interface deletion notification received by ovn-controller.
4. SB ack received for step-2 PB update.

This commit fixes this issue by removing the logical_port from related
port even in case there is no binding available locally.

Signed-off-by: Priyankar Jain 
---
 controller/binding.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/controller/binding.c b/controller/binding.c
index 9b0647b70..9889be5c7 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1568,6 +1568,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
 || is_additional_chassis(pb, b_ctx_in->chassis_rec)) {
 /* Release the lport if there is no lbinding. */
 if (!lbinding_set || !can_bind) {
+remove_related_lport(pb, b_ctx_out);
 return release_lport(pb, b_ctx_in->chassis_rec,
  !b_ctx_in->ovnsb_idl_txn,
  b_ctx_out->tracked_dp_bindings,
-- 
2.37.1 (Apple Git-137.1)

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

Re: [ovs-dev] [PATCH ovn v2] northd: Add logical flow to skip GARP with LLA

[Ales Musil]

On Tue, May 30, 2023 at 11:59 AM Ales Musil  wrote:

> Skip GARP packet with link-local address being advertised
> when "always_learn_from_arp_request=false", this should
> prevent huge grow of MAC Binding table. To keep the option
> consistent overwrite the previous MAC with LLA if it was
> already stored in DB.
>
> Signed-off-by: Ales Musil 
> ---
> v2: Remove leftover from previous tests.
> ---
>  northd/northd.c | 11 +++
>  tests/ovn.at| 78 -
>  2 files changed, 63 insertions(+), 26 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index a6eca916b..ff305fc67 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11951,6 +11951,17 @@ build_neigh_learning_flows_for_lrouter(
>  ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
>ds_cstr(actions));
>
> +if (!learn_from_arp_request) {
> +/* Add flow to skip LLA only if we don't know it already. */
> +ds_clear(actions);
> +ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> +  " = lookup_nd(inport, ip6.src, nd.tll); "
> +  REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> +  " = lookup_nd_ip(inport, ip6.src); next;");
> +ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
> +  "nd_na && ip6.src == fe80::/10", ds_cstr(actions));
> +}
> +
>  /* For other packet types, we can skip neighbor learning.
>   * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
>  ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 6f9fbbfd2..7a2658f40 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -5073,6 +5073,7 @@ AT_CLEANUP
>
>  OVN_FOR_EACH_NORTHD([
>  AT_SETUP([IP relocation using GARP request])
> +AT_SKIP_IF([test $HAVE_SCAPY = no])
>  ovn_start
>
>  # Logical network:
> @@ -5172,7 +5173,9 @@ done
>  test_ip() {
>  # This packet has bad checksums but logical L3 routing doesn't check.
>  local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
> -local
> packet=${dst_mac}${src_mac}0800451c4011${src_ip}${dst_ip}00350008
> +local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \
> +IP(dst='${dst_ip}', src='${src_ip}')/ \
> +UDP(sport=53, dport=4369)")
>  shift; shift; shift; shift; shift
>  hv=hv`vif_to_hv $inport`
>  as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
> @@ -5187,7 +5190,9 @@ test_ip() {
>  # Routing decrements TTL and updates source and dest MAC
>  # (and checksum).
>  out_lrp=`vif_to_lrp $outport`
> -echo
> f0${outport}ff0${out_lrp}0800451c"3f1101"00${src_ip}${dst_ip}00350008
> +echo $(fmt_pkt "Ether(dst='f0:00:00:00:00:${outport}',
> src='00:00:00:00:ff:${out_lrp}')/ \
> +IP(src='${src_ip}', dst='${dst_ip}', ttl=63)/
> \
> +UDP(sport=53, dport=4369)")
>  fi >> $outport.expected
>  done
>  }
> @@ -5203,8 +5208,10 @@ test_ip() {
>  # SHA and REPLY_HA are each 12 hex digits.
>  # SPA and TPA are each 8 hex digits.
>  test_arp() {
> -local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
> -local
> request=${sha}08060001080006040001${sha}${spa}${tpa}
> +local inport=$1 sha=$2 spa=$3 tpa=$3
> +local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> src='${sha}')/ \
> + ARP(hwsrc='${sha}',
> hwdst='ff:ff:ff:ff:ff:ff', psrc='${spa}', pdst='${tpa}')")
> +
>  hv=hv`vif_to_hv $inport`
>  as $hv ovs-appctl netdev-dummy/receive vif$inport $request
>
> @@ -5217,53 +5224,72 @@ test_arp() {
>  echo $request >> $i$j$k.expected
>  fi
>  done
> +}
>
> -# Expect to receive the reply, if any.
> -if test X$reply_ha != X; then
> -lrp=`vif_to_lrp $inport`
> -local
> reply=${sha}ff0${lrp}08060001080006040002${reply_ha}${tpa}${sha}${spa}
> -echo $reply >> $inport.expected
> -fi
> +test_na() {
> +local inport=$1 sha=$2 spa=$3
> +local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> src='${sha}')/ \
> + IPv6(dst='fe80::1', src='${spa}')/ \
> + ICMPv6ND_NA(tgt='${spa}')")
> +
> +hv=hv`vif_to_hv $inport`
> +as $hv ovs-appctl netdev-dummy/receive vif$inport $request
> +
> +# Expect to receive the broadcast ARP on the other logical switch
> ports if
> +# IP address is not configured to the switch patch port.
> +local i=`vif_to_ls $inport`
> +local j
> +for j in 1 2; do
> +if test $i$j != $inport; then
> +echo $request >> $i$j$k.expected
> +fi
> +done
>  }
>
> -# lp11 send GARP request to announce ownership of 192.168.1.100.
> +# lp11 

Re: [ovs-dev] [PATCH ovn 09/14] northd: Incremental processing of VIF additions in 'lflow' node.

[Han Zhou]

On Tue, May 30, 2023 at 4:04 AM Ilya Maximets  wrote:
>
> On 5/29/23 20:42, Han Zhou wrote:
> >
> >
> > On Mon, May 29, 2023 at 9:24 AM Ilya Maximets mailto:i.maxim...@ovn.org>> wrote:
> >>
> >> On 5/13/23 02:03, Han Zhou wrote:
> >> > This patch introduces a change handler for 'northd' input within the
> >> > 'lflow' node. It specifically handles cases when VIFs are created,
which
> >> > is an easier start for lflow incremental processing. Support for
> >> > update/delete will be added later.
> >> >
> >> > Below are the performance test results simulating an ovn-k8s
topology of 500
> >> > nodes x 50 lsp per node:
> >> >
> >> > Before:
> >> > ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001
lsp_1_0001_01 -- lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01
1.0.1.101" -- lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01
1.0.1.101"
> >> > ovn-northd completion:  773ms
> >> >
> >> > After:
> >> > ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001
lsp_1_0001_01 -- lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01
1.0.1.101" -- lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01
1.0.1.101"
> >> > ovn-northd completion:  30ms
> >> >
> >> > It is more than 95% reduction (or 20x faster).
> >> >
> >> > Signed-off-by: Han Zhou mailto:hz...@ovn.org>>
> >> > ---
> >> >  northd/en-lflow.c|  82 -
> >> >  northd/en-lflow.h|   1 +
> >> >  northd/inc-proc-northd.c |   2 +-
> >> >  northd/northd.c  | 245
+--
> >> >  northd/northd.h  |   6 +-
> >> >  tests/ovn-northd.at   |   4 +-
> >> >  6 files changed, 277 insertions(+), 63 deletions(-)
> >> >
> >>
> >> 
> >>
> >> > @@ -5685,7 +5687,18 @@ ovn_dp_group_add_with_reference(struct
ovn_lflow *lflow_ref,
> >> >
> >> >  /* Adds a row with the specified contents to the Logical_Flow table.
> >> >   * Version to use when hash bucket locking is NOT required.
> >> > + *
> >> > + * Note: This function can add generated lflows to the global
variable
> >> > + * temp_lflow_list as its output, controlled by the global variable
> >> > + * add_lflow_to_temp_list. The caller of the ovn_lflow_add_...
marcros can get
> >> > + * a list of lflows generated by setting add_lflow_to_temp_list to
true. The
> >> > + * caller is responsible for initializing the temp_lflow_list, and
also
> >> > + * reset the add_lflow_to_temp_list to false when it is no longer
needed.
> >> > + * XXX: this mechanism is temporary and will be replaced when we
add hash index
> >> > + * to lflow_data and refactor related functions.
> >> >   */
> >> > +static bool add_lflow_to_temp_list = false;
> >> > +static struct ovs_list temp_lflow_list;
> >> >  static void
> >> >  do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath
*od,
> >> >   const unsigned long *dp_bitmap, size_t
dp_bitmap_len,
> >> > @@ -5702,12 +5715,17 @@ do_ovn_lflow_add(struct hmap *lflow_map,
const struct ovn_datapath *od,
> >> >  size_t bitmap_len = od ? ods_size(od->datapaths) :
dp_bitmap_len;
> >> >  ovs_assert(bitmap_len);
> >> >
> >> > -old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority,
match,
> >> > -   actions, ctrl_meter, hash);
> >> > -if (old_lflow) {
> >> > -ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap,
> >> > -bitmap_len);
> >> > -return;
> >> > +if (add_lflow_to_temp_list) {
> >> > +ovs_assert(od);
> >> > +ovs_assert(!dp_bitmap);
> >> > +} else {
> >> > +old_lflow = ovn_lflow_find(lflow_map, NULL, stage,
priority, match,
> >> > +   actions, ctrl_meter, hash);
> >> > +if (old_lflow) {
> >> > +ovn_dp_group_add_with_reference(old_lflow, od,
dp_bitmap,
> >> > +bitmap_len);
> >> > +return;
> >> > +}
> >> >  }
> >>
> >> Hi, Han.  Not a full review, but I'm a bit concerned that this code
doesn't
> >> support datapath groups.  IIUC, if some flows can be aggregated with
datapath
> >> groups, I-P will just add them separately.  Later a full re-compute
will
> >> delete all these flows and replace them with a single grouped one.
Might
> >> cause some unnecessary churn in the cluster.
> >>
> >
> > Hi Ilya, thanks for the feedback. You are right that the current I-P
doesn't support datapath groups, because:
> > 1. The current I-P only supports LSP (regular VIFs) related changes,
which is very unlikely to generate datapath groups. Even when that happens
in rare cases, the logic is still correct and the churn should be small.
> > 2. There are two types of DP groups:
> > a. DP groups that are directly generated when northd has the
knowledge that a lflow should be applied to a group of DPs.
> > b. DP groups that are passively generated by combining lflows
generated for different DPs.
> > Supporting I-P for 

Re: [ovs-dev] [PATCH ovn v7 0/5] Implement MTU Path Discovery for multichassis ports

[Ihar Hrachyshka]

On Tue, May 30, 2023 at 5:06 PM Ihar Hrachyshka  wrote:
>
> On Tue, May 30, 2023 at 2:28 PM Mark Michelson  wrote:
> >
> > Thanks Ihar and Dumitru.
> >
> > I applied this series to main and branch-23.06. I encountered conflicts
> > when trying to apply this to branch-23.03, specifically with patch 4. I
> > suspect a lot of this is due to the fact that the tiered ACL changes are
> > not present in branch-23.03, so
> >
> > * The table numbers don't work out the same in some cases.
> > * ACL flow actions are different prior to 23.06.
> >
> > If you can post backport versions of the patches for 23.03 and below,
> > that would be great, Ihar.
>
> Should I backport down to LTS (22.03)?
>

Sorry, I mean down to 22.06 (non-LTS) because the first release with
multichassis ports is 22.06.

> >
> > Thanks,
> > Mark Michelson
> >
> > On 5/23/23 17:55, Ihar Hrachyshka wrote:
> > > This series fixes a non-optimal behavior with some multichassis ports.
> > >
> > > Specifically,
> > >
> > > - when a multichassis port belongs to a switch that also has a localnet
> > >port,
> > > - because ingress and egress traffic for the port is funnelled through
> > >tunnels to guarantee delivery of packets to all chassis that host the
> > >port,
> > > - because tunnels add overhead to each funnelled packet,
> > > - leaving less space for actual packets,
> > >
> > > ... the effective MTU of the path for multichassis ports is reduced by
> > > the size that takes the tunnel to wrap a packet around. (This depends on
> > > the type of tunnel, also on IP version of the tunnel.)
> > >
> > > When this happens, the port and its peers are not notified about the
> > > reduced MTU for the path to the port, effectively blackholing some of
> > > the larger packets.
> > >
> > > This patch series makes OVN detect these scenarios and handle them as
> > > follows:
> > >
> > > - for all multichassis ports, 4 flows are added - for inport and
> > >outport matches - to detect "too-big" IP packets;
> > > - for all "too-big" packets, 2 flows are added to produce a ICMP
> > >Fragmentation Needed (for IPv4) or Too Big (for IPv6) packet and
> > >return it to the offending sender.
> > >
> > > The proposed implementation is isolated in ovn-controller. An
> > > alternative would be to implement flows producing ICMP errors via the
> > > existing Logical_Flow action icmp4_error, as is done for router ports.
> > > In this case, the 4 flows that detect "too-big" packets would still have
> > > to reside in ovn-controller because of limitations of the current OVN
> > > port model, namely lack of `mtu` as an attribute of OVN logical port.
> > >
> > > Caveats: this works for IP traffic only. The party receiving the ICMP
> > > error should handle it by temporarily lowering the MTU used to reach the
> > > port. Behavior may depend on protocol implementation of the offending
> > > peer as well as its networking stack configuration.
> > >
> > > This patch series was successfully tested with Linux kernel 6.0.8.
> > >
> > > This patch series is designed to simplify backporting process. It is
> > > split in logical pieces to simplify cherry-picking. I consider the
> > > original behavior described at the start of the cover letter a bug and
> > > worth a backport.
> > >
> > > ---
> > > v1: - initial version.
> > > v2: - more system test cases adjusted to new table numbers for egress
> > >pipeline;
> > >  - switch from xxd to coreutils (tr, head) tools to avoid a new
> > >dependency;
> > >  - adjusted a comment in the new test case to avoid "migrator"
> > >terminology that makes no sense outside live migration context -
> > >which is not the only potential use case for multichassis ports;
> > >  - guard the new test case with HAVE_SCAPY;
> > >  - nit: rewrapped some lines in patch 6/7 to avoid unnecessary line
> > >changes;
> > >  - added a NEWS entry for patch 6/7 that implements the core of the
> > >feature;
> > >  - rebased to latest main.
> > > v3: - expose and reuse lib/actions.c:encode_[start|finish]_controller_op
> > >functions;
> > >  - reuse ETH_HEADER_LEN, IP_HEADER_LEN, and IPV6_HEADER_LEN from
> > >ovs:lib/packets.h;
> > >  - add a comment for REGBIT_PKT_LARGER that mentions that the
> > >register is also used in OFTABLE_OUTPUT_LARGE_PKT_DETECT table;
> > >  - squash the patch that makes if-status-mgr track changes to
> > >interface mtu into the patch that introduces the mtu field in the
> > >manager;
> > >  - doc: don't describe path discovery flows in the patch that
> > >introduces new tables (only in the patch that actually implements
> > >the flows);
> > >  - nit: use MAX to shave off several lines of code;
> > >  - nit: remove redundant if_status_mgr_iface_get_mtu declaration from
> > >a header file;
> > >  - rebased to latest main;
> > >  - updated acks based on latest 

Re: [ovs-dev] [PATCH ovn 6/9] release-policy: Document when versions receive new releases.

[Dumitru Ceara]

On 5/30/23 22:22, Mark Michelson wrote:
> On 5/26/23 08:45, Dumitru Ceara wrote:
>> On 5/25/23 20:22, Mark Michelson wrote:
>>> The OVN team has had no concrete policy regarding when releases are
>>> made for a given version. In the past, new releases have been made only
>>> when something critical has been found, meaning that most bug fixes
>>> applied to a given OVN version never end up in a release.
>>>
>>> This change proposes a monthly release per supported OVN version.
>>>
>>> Signed-off-by: Mark Michelson 
>>> ---
>>>   Documentation/internals/release-process.rst | 6 ++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/internals/release-process.rst
>>> b/Documentation/internals/release-process.rst
>>> index 1f4b90143..0287795dc 100644
>>> --- a/Documentation/internals/release-process.rst
>>> +++ b/Documentation/internals/release-process.rst
>>> @@ -152,6 +152,12 @@ approximate:
>>>   | T + 6 | Sep 1, Mar 1, ...   | Release version
>>> x.y.0    |
>>>  
>>> +---+-+--+
>>>   +After the .0 release is created for a given OVN version, that
>>> version will have
>>> +releases made every month until its support lifetime has concluded.
>>> If for some
>>> +reason, no changes occur to a supported OVN version during a month,
>>> then no
>>> +release will be made that month. Therefore, there is no guarantee
>>> about the
>>> +exact number of releases to be expected for any given OVN version.
>>> +
>>>   Release Calendar
>>>   
>>>   
>>
>> It makes sense but (I think I may have mentioned this during an upstream
>> meeting) can we automate the process?  It's going to be 2 patches per
>> release, 2 standard term releases + 2 LTS releases, release every month
>> => 96 patches to ack throughout the year (unless my reasoning is wrong).
> 
> The basic mechanics of a release would be easy to automate. I already
> use a script provided by Ilya to create the release commits, send them
> to the mailing list, and merge the commits and create the tag. The
> things that currently cannot be automated are:
> 
> 1) Acks for release patches. We currently post the patches to the
> mailing list and await acks before merging/tagging. We could potentially
> agree as a project that these commits could just be automatically
> created and pushed/tagged without developer acks.
> 

If the process is fully automated I think it's fine to skip acks for
these patches.

> 2) Signing the tag. When I create OVN releases, I sign the tag with my
> GPG key. I'm not sure what a good automated substitute for this would be.
> 

I can't think of an alternative either.  Maybe we're OK with one of the
maintainers running everything (a script that would live in the repo)
locally like today.  That person would and just push the commits and
signed tags directly without waiting for ACKs.  We could try to add some
validation after the fact, e.g., a GitHub action that runs after a tag
is pushed.  I don't know if that's good enough though.

> 3) Updating ovn.org. For .0 releases, I typically will go through all
> the commits and keep a list of new features and performance
> enhancements. I then write these up on the release page. Automating this
> exact process would not be possible. However we could switch to a
> changelog style where we list all the commits that went into a given
> release. This would be easier to automate. For .1 releases, we shouldn't
> have new features or performance improvements. We should only have bug
> fixes. The changelog approach would fit for these types of releases well.
> 

IMO that's OK.

>>
>> Regards,
>> Dumitru
>>
> 

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

Re: [ovs-dev] [PATCH ovn v7 0/5] Implement MTU Path Discovery for multichassis ports

[Ihar Hrachyshka]

On Tue, May 30, 2023 at 2:28 PM Mark Michelson  wrote:
>
> Thanks Ihar and Dumitru.
>
> I applied this series to main and branch-23.06. I encountered conflicts
> when trying to apply this to branch-23.03, specifically with patch 4. I
> suspect a lot of this is due to the fact that the tiered ACL changes are
> not present in branch-23.03, so
>
> * The table numbers don't work out the same in some cases.
> * ACL flow actions are different prior to 23.06.
>
> If you can post backport versions of the patches for 23.03 and below,
> that would be great, Ihar.

Should I backport down to LTS (22.03)?

>
> Thanks,
> Mark Michelson
>
> On 5/23/23 17:55, Ihar Hrachyshka wrote:
> > This series fixes a non-optimal behavior with some multichassis ports.
> >
> > Specifically,
> >
> > - when a multichassis port belongs to a switch that also has a localnet
> >port,
> > - because ingress and egress traffic for the port is funnelled through
> >tunnels to guarantee delivery of packets to all chassis that host the
> >port,
> > - because tunnels add overhead to each funnelled packet,
> > - leaving less space for actual packets,
> >
> > ... the effective MTU of the path for multichassis ports is reduced by
> > the size that takes the tunnel to wrap a packet around. (This depends on
> > the type of tunnel, also on IP version of the tunnel.)
> >
> > When this happens, the port and its peers are not notified about the
> > reduced MTU for the path to the port, effectively blackholing some of
> > the larger packets.
> >
> > This patch series makes OVN detect these scenarios and handle them as
> > follows:
> >
> > - for all multichassis ports, 4 flows are added - for inport and
> >outport matches - to detect "too-big" IP packets;
> > - for all "too-big" packets, 2 flows are added to produce a ICMP
> >Fragmentation Needed (for IPv4) or Too Big (for IPv6) packet and
> >return it to the offending sender.
> >
> > The proposed implementation is isolated in ovn-controller. An
> > alternative would be to implement flows producing ICMP errors via the
> > existing Logical_Flow action icmp4_error, as is done for router ports.
> > In this case, the 4 flows that detect "too-big" packets would still have
> > to reside in ovn-controller because of limitations of the current OVN
> > port model, namely lack of `mtu` as an attribute of OVN logical port.
> >
> > Caveats: this works for IP traffic only. The party receiving the ICMP
> > error should handle it by temporarily lowering the MTU used to reach the
> > port. Behavior may depend on protocol implementation of the offending
> > peer as well as its networking stack configuration.
> >
> > This patch series was successfully tested with Linux kernel 6.0.8.
> >
> > This patch series is designed to simplify backporting process. It is
> > split in logical pieces to simplify cherry-picking. I consider the
> > original behavior described at the start of the cover letter a bug and
> > worth a backport.
> >
> > ---
> > v1: - initial version.
> > v2: - more system test cases adjusted to new table numbers for egress
> >pipeline;
> >  - switch from xxd to coreutils (tr, head) tools to avoid a new
> >dependency;
> >  - adjusted a comment in the new test case to avoid "migrator"
> >terminology that makes no sense outside live migration context -
> >which is not the only potential use case for multichassis ports;
> >  - guard the new test case with HAVE_SCAPY;
> >  - nit: rewrapped some lines in patch 6/7 to avoid unnecessary line
> >changes;
> >  - added a NEWS entry for patch 6/7 that implements the core of the
> >feature;
> >  - rebased to latest main.
> > v3: - expose and reuse lib/actions.c:encode_[start|finish]_controller_op
> >functions;
> >  - reuse ETH_HEADER_LEN, IP_HEADER_LEN, and IPV6_HEADER_LEN from
> >ovs:lib/packets.h;
> >  - add a comment for REGBIT_PKT_LARGER that mentions that the
> >register is also used in OFTABLE_OUTPUT_LARGE_PKT_DETECT table;
> >  - squash the patch that makes if-status-mgr track changes to
> >interface mtu into the patch that introduces the mtu field in the
> >manager;
> >  - doc: don't describe path discovery flows in the patch that
> >introduces new tables (only in the patch that actually implements
> >the flows);
> >  - nit: use MAX to shave off several lines of code;
> >  - nit: remove redundant if_status_mgr_iface_get_mtu declaration from
> >a header file;
> >  - rebased to latest main;
> >  - updated acks based on latest review comments.
> > v4: - fix compilation issue in the middle of the series due to previous
> >commit rearrangement.
> > v5: - introduce a new if-status-mgr input to pflow_output, then process
> >OVS interface updates inside if-status-mgr handlers.
> >  - if-status-mgr: remove set_mtu public function, instead other
> >modules should 

Re: [ovs-dev] [PATCH ovn] controller: Handle OpenFlow errors.

[0-day Robot]

Bleep bloop.  Greetings Dumitru Ceara, 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: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 controller: Handle OpenFlow errors.
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".


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] test: Fix expected OpenFlow table numbers.

[Ihar Hrachyshka]

+1. Thanks Dumitru for cleaning it up!

Reviewed-By: Ihar Hrachyshka 

On Tue, May 30, 2023 at 4:51 PM Dumitru Ceara  wrote:
>
> Commit 549e8ccebca7 ("ovn-controller.c: Fix assertion failure during
> address set update.") added a new ovn-controller.at test and commit
> 740f23c19577 ("Add new egress tables to accommodate for too-big packets
> handling") went in without updating the expected numbers for that test.
>
> Fixes: 740f23c19577 ("Add new egress tables to accommodate for too-big 
> packets handling")
> Signed-off-by: Dumitru Ceara 
> ---
>  tests/ovn-controller.at | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 4edd62b24e..7109ff19b2 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2088,7 +2088,7 @@ ovn-appctl -t ovn-controller vlog/set file:dbg
>  ovn-nbctl create address_set name=as1 addresses=8.8.8.8
>  check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == 
> $as1' drop
>  check ovn-nbctl --wait=hv sync
> -AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c "priority=1100"], 
> [0], [1
> +AT_CHECK([ovs-ofctl dump-flows br-int table=47 | grep -c "priority=1100"], 
> [0], [1
>  ])
>
>  # pause ovn-northd
> @@ -2104,13 +2104,13 @@ check as northd-backup ovn-appctl -t ovn-northd pause
>  # undefined. This test runs the scenario ten times to make sure different
>  # orders are covered and handled properly.
>
> -flow_count=$(ovs-ofctl dump-flows br-int table=44 | grep -c "priority=1100")
> +flow_count=$(ovs-ofctl dump-flows br-int table=47 | grep -c "priority=1100")
>  for i in $(seq 10); do
>  # Delete and recreate the SB address set with same name and an extra IP.
>  addrs_=$(fetch_column address_set addresses name=as1)
>  addrs=${addrs_// /,}
>  AT_CHECK([ovn-sbctl destroy address_set as1 -- create address_set 
> name=as1 addresses=$addrs,1.1.1.$i], [0], [ignore])
> -OVS_WAIT_UNTIL([test $(as hv1 ovs-ofctl dump-flows br-int table=44 | 
> grep -c "priority=1100") = "$(($i + 1))"])
> +OVS_WAIT_UNTIL([test $(as hv1 ovs-ofctl dump-flows br-int table=47 | 
> grep -c "priority=1100") = "$(($i + 1))"])
>  done
>
>  OVN_CLEANUP([hv1])
> --
> 2.31.1
>

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

Re: [ovs-dev] [PATCH ovn v7 0/5] Implement MTU Path Discovery for multichassis ports

[Dumitru Ceara]

On 5/30/23 20:27, Mark Michelson wrote:
> Thanks Ihar and Dumitru.
> 
> I applied this series to main and branch-23.06. I encountered conflicts

Thanks, Mark!  Unfortunately, I had missed the fact that a new test had
been added in the meantime and that we had to update the table numbers
for that one too.  CI was failing because of that.

I posted a fix for it:

https://patchwork.ozlabs.org/project/ovn/patch/20230530205146.960485-1-dce...@redhat.com/

Regards,
Dumitru

> when trying to apply this to branch-23.03, specifically with patch 4. I
> suspect a lot of this is due to the fact that the tiered ACL changes are
> not present in branch-23.03, so
> 
> * The table numbers don't work out the same in some cases.
> * ACL flow actions are different prior to 23.06.
> 
> If you can post backport versions of the patches for 23.03 and below,
> that would be great, Ihar.
> 
> Thanks,
> Mark Michelson
> 
> On 5/23/23 17:55, Ihar Hrachyshka wrote:
>> This series fixes a non-optimal behavior with some multichassis ports.
>>
>> Specifically,
>>
>> - when a multichassis port belongs to a switch that also has a localnet
>>    port,
>> - because ingress and egress traffic for the port is funnelled through
>>    tunnels to guarantee delivery of packets to all chassis that host the
>>    port,
>> - because tunnels add overhead to each funnelled packet,
>> - leaving less space for actual packets,
>>
>> ... the effective MTU of the path for multichassis ports is reduced by
>> the size that takes the tunnel to wrap a packet around. (This depends on
>> the type of tunnel, also on IP version of the tunnel.)
>>
>> When this happens, the port and its peers are not notified about the
>> reduced MTU for the path to the port, effectively blackholing some of
>> the larger packets.
>>
>> This patch series makes OVN detect these scenarios and handle them as
>> follows:
>>
>> - for all multichassis ports, 4 flows are added - for inport and
>>    outport matches - to detect "too-big" IP packets;
>> - for all "too-big" packets, 2 flows are added to produce a ICMP
>>    Fragmentation Needed (for IPv4) or Too Big (for IPv6) packet and
>>    return it to the offending sender.
>>
>> The proposed implementation is isolated in ovn-controller. An
>> alternative would be to implement flows producing ICMP errors via the
>> existing Logical_Flow action icmp4_error, as is done for router ports.
>> In this case, the 4 flows that detect "too-big" packets would still have
>> to reside in ovn-controller because of limitations of the current OVN
>> port model, namely lack of `mtu` as an attribute of OVN logical port.
>>
>> Caveats: this works for IP traffic only. The party receiving the ICMP
>> error should handle it by temporarily lowering the MTU used to reach the
>> port. Behavior may depend on protocol implementation of the offending
>> peer as well as its networking stack configuration.
>>
>> This patch series was successfully tested with Linux kernel 6.0.8.
>>
>> This patch series is designed to simplify backporting process. It is
>> split in logical pieces to simplify cherry-picking. I consider the
>> original behavior described at the start of the cover letter a bug and
>> worth a backport.
>>
>> ---
>> v1: - initial version.
>> v2: - more system test cases adjusted to new table numbers for egress
>>    pipeline;
>>  - switch from xxd to coreutils (tr, head) tools to avoid a new
>>    dependency;
>>  - adjusted a comment in the new test case to avoid "migrator"
>>    terminology that makes no sense outside live migration context -
>>    which is not the only potential use case for multichassis ports;
>>  - guard the new test case with HAVE_SCAPY;
>>  - nit: rewrapped some lines in patch 6/7 to avoid unnecessary line
>>    changes;
>>  - added a NEWS entry for patch 6/7 that implements the core of the
>>    feature;
>>  - rebased to latest main.
>> v3: - expose and reuse lib/actions.c:encode_[start|finish]_controller_op
>>    functions;
>>  - reuse ETH_HEADER_LEN, IP_HEADER_LEN, and IPV6_HEADER_LEN from
>>    ovs:lib/packets.h;
>>  - add a comment for REGBIT_PKT_LARGER that mentions that the
>>    register is also used in OFTABLE_OUTPUT_LARGE_PKT_DETECT table;
>>  - squash the patch that makes if-status-mgr track changes to
>>    interface mtu into the patch that introduces the mtu field in the
>>    manager;
>>  - doc: don't describe path discovery flows in the patch that
>>    introduces new tables (only in the patch that actually implements
>>    the flows);
>>  - nit: use MAX to shave off several lines of code;
>>  - nit: remove redundant if_status_mgr_iface_get_mtu declaration from
>>    a header file;
>>  - rebased to latest main;
>>  - updated acks based on latest review comments.
>> v4: - fix compilation issue in the middle of the series due to previous
>>    commit rearrangement.
>> v5: - introduce a new if-status-mgr input 

[ovs-dev] [PATCH ovn] test: Fix expected OpenFlow table numbers.

[Dumitru Ceara]

Commit 549e8ccebca7 ("ovn-controller.c: Fix assertion failure during
address set update.") added a new ovn-controller.at test and commit
740f23c19577 ("Add new egress tables to accommodate for too-big packets
handling") went in without updating the expected numbers for that test.

Fixes: 740f23c19577 ("Add new egress tables to accommodate for too-big packets 
handling")
Signed-off-by: Dumitru Ceara 
---
 tests/ovn-controller.at | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 4edd62b24e..7109ff19b2 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -2088,7 +2088,7 @@ ovn-appctl -t ovn-controller vlog/set file:dbg
 ovn-nbctl create address_set name=as1 addresses=8.8.8.8
 check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == 
$as1' drop
 check ovn-nbctl --wait=hv sync
-AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c "priority=1100"], 
[0], [1
+AT_CHECK([ovs-ofctl dump-flows br-int table=47 | grep -c "priority=1100"], 
[0], [1
 ])
 
 # pause ovn-northd
@@ -2104,13 +2104,13 @@ check as northd-backup ovn-appctl -t ovn-northd pause
 # undefined. This test runs the scenario ten times to make sure different
 # orders are covered and handled properly.
 
-flow_count=$(ovs-ofctl dump-flows br-int table=44 | grep -c "priority=1100")
+flow_count=$(ovs-ofctl dump-flows br-int table=47 | grep -c "priority=1100")
 for i in $(seq 10); do
 # Delete and recreate the SB address set with same name and an extra IP.
 addrs_=$(fetch_column address_set addresses name=as1)
 addrs=${addrs_// /,}
 AT_CHECK([ovn-sbctl destroy address_set as1 -- create address_set name=as1 
addresses=$addrs,1.1.1.$i], [0], [ignore])
-OVS_WAIT_UNTIL([test $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep 
-c "priority=1100") = "$(($i + 1))"])
+OVS_WAIT_UNTIL([test $(as hv1 ovs-ofctl dump-flows br-int table=47 | grep 
-c "priority=1100") = "$(($i + 1))"])
 done
 
 OVN_CLEANUP([hv1])
-- 
2.31.1

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

Re: [ovs-dev] [PATCH ovn] controller: Handle OpenFlow errors.

[Dumitru Ceara]

On 5/30/23 22:48, Dumitru Ceara wrote:
> Whenever an OpenFlow error is returned by OvS, trigger a reconnect of
> the OpenFlow (rconn) connection.  This will clear any installed OpenFlow
> rules/groups. To ensure consistency, trigger a full I-P recompute too.
> 
> An example of scenario that can result in an OpenFlow error returned by
> OvS follows (describing two main loop iterations in ovn-controller):
>   - Iteration I:
> a. get updates from SB
> b. process these updates and generate "desired" openflows (lets assume
> this generates quite a lot of desired openflow modifications)
> c.1. add bundle-open msg to rconn
> c.2. add openflow mod msgs to rconn (only some of these make it through,
> the rest gets queued, the rconn is backlogged at this point).
> c.3. add bundle-commit msg to rconn (this gets queued)
> 
>   - Iteration II:
> a. get updates from SB (rconn is still backlogged)
> b. process the updates and generate "desired" openflows (lets assume
> this takes 10+ seconds for the specific SB updates)
> 
> At some point, while step II.b was being executed OvS declared the bundle
> operation (started at I.c.1) timeout.  We now act on this error by
> reconnecting which in turn triggers a flush of the rconn backlog and
> gives more chance to the next full recompute to succeed in installing
> all flows.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2134880
> Reported-by: François Rigault 
> CC: Ilya Maximets 
> Signed-off-by: Dumitru Ceara 
> ---

I re-sent this one by accident, please ignore it.  Sorry for the noise!

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

[ovs-dev] [PATCH ovn] controller: Handle OpenFlow errors.

[Dumitru Ceara]

Whenever an OpenFlow error is returned by OvS, trigger a reconnect of
the OpenFlow (rconn) connection.  This will clear any installed OpenFlow
rules/groups. To ensure consistency, trigger a full I-P recompute too.

An example of scenario that can result in an OpenFlow error returned by
OvS follows (describing two main loop iterations in ovn-controller):
  - Iteration I:
a. get updates from SB
b. process these updates and generate "desired" openflows (lets assume
this generates quite a lot of desired openflow modifications)
c.1. add bundle-open msg to rconn
c.2. add openflow mod msgs to rconn (only some of these make it through,
the rest gets queued, the rconn is backlogged at this point).
c.3. add bundle-commit msg to rconn (this gets queued)

  - Iteration II:
a. get updates from SB (rconn is still backlogged)
b. process the updates and generate "desired" openflows (lets assume
this takes 10+ seconds for the specific SB updates)

At some point, while step II.b was being executed OvS declared the bundle
operation (started at I.c.1) timeout.  We now act on this error by
reconnecting which in turn triggers a flush of the rconn backlog and
gives more chance to the next full recompute to succeed in installing
all flows.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2134880
Reported-by: François Rigault 
CC: Ilya Maximets 
Signed-off-by: Dumitru Ceara 
---
 controller/ofctrl.c | 17 ++---
 controller/ofctrl.h |  2 +-
 controller/ovn-controller.c | 10 --
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index b1ba1c743a..1da23bc27e 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -766,13 +766,18 @@ ofctrl_get_mf_field_id(void)
 
 /* Runs the OpenFlow state machine against 'br_int', which is local to the
  * hypervisor on which we are running.  Attempts to negotiate a Geneve option
- * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE. */
-void
+ * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE.
+ *
+ * Returns 'true' if an OpenFlow reconnect happened; 'false' otherwise.
+ */
+bool
 ofctrl_run(const struct ovsrec_bridge *br_int,
const struct ovsrec_open_vswitch_table *ovs_table,
struct shash *pending_ct_zones)
 {
 char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
+bool reconnected = false;
+
 if (strcmp(target, rconn_get_target(swconn))) {
 VLOG_INFO("%s: connecting to switch", target);
 rconn_connect(swconn, target, target);
@@ -782,10 +787,12 @@ ofctrl_run(const struct ovsrec_bridge *br_int,
 rconn_run(swconn);
 
 if (!rconn_is_connected(swconn)) {
-return;
+goto done;
 }
+
 if (seqno != rconn_get_connection_seqno(swconn)) {
 seqno = rconn_get_connection_seqno(swconn);
+reconnected = true;
 state = S_NEW;
 
 /* Reset the state of any outstanding ct flushes to resend them. */
@@ -855,6 +862,9 @@ ofctrl_run(const struct ovsrec_bridge *br_int,
  * point, so ensure that we come back again without waiting. */
 poll_immediate_wake();
 }
+
+done:
+return reconnected;
 }
 
 void
@@ -909,6 +919,7 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
 } else if (type == OFPTYPE_ERROR) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300);
 log_openflow_rl(, VLL_INFO, oh, "OpenFlow error");
+rconn_reconnect(swconn);
 } else {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300);
 log_openflow_rl(, VLL_DBG, oh, "OpenFlow packet ignored");
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index f5751e3ee4..105f9370be 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -51,7 +51,7 @@ struct ovn_desired_flow_table {
 void ofctrl_init(struct ovn_extend_table *group_table,
  struct ovn_extend_table *meter_table,
  int inactivity_probe_interval);
-void ofctrl_run(const struct ovsrec_bridge *br_int,
+bool ofctrl_run(const struct ovsrec_bridge *br_int,
 const struct ovsrec_open_vswitch_table *,
 struct shash *pending_ct_zones);
 enum mf_field_id ofctrl_get_mf_field_id(void);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 1151d36644..b301c50157 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -5075,8 +5075,14 @@ main(int argc, char *argv[])
 
 if (br_int) {
 ct_zones_data = engine_get_data(_ct_zones);
-if (ct_zones_data) {
-ofctrl_run(br_int, ovs_table, _zones_data->pending);
+if (ct_zones_data && ofctrl_run(br_int, ovs_table,
+_zones_data->pending)) {
+static struct vlog_rate_limit rl
+= VLOG_RATE_LIMIT_INIT(1, 

Re: [ovs-dev] [PATCH ovn 6/9] release-policy: Document when versions receive new releases.

[Mark Michelson]

On 5/26/23 08:45, Dumitru Ceara wrote:

On 5/25/23 20:22, Mark Michelson wrote:

The OVN team has had no concrete policy regarding when releases are
made for a given version. In the past, new releases have been made only
when something critical has been found, meaning that most bug fixes
applied to a given OVN version never end up in a release.

This change proposes a monthly release per supported OVN version.

Signed-off-by: Mark Michelson 
---
  Documentation/internals/release-process.rst | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index 1f4b90143..0287795dc 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -152,6 +152,12 @@ approximate:
  | T + 6 | Sep 1, Mar 1, ...   | Release version x.y.0|
  +---+-+--+
  
+After the .0 release is created for a given OVN version, that version will have

+releases made every month until its support lifetime has concluded. If for some
+reason, no changes occur to a supported OVN version during a month, then no
+release will be made that month. Therefore, there is no guarantee about the
+exact number of releases to be expected for any given OVN version.
+
  Release Calendar
  
  


It makes sense but (I think I may have mentioned this during an upstream
meeting) can we automate the process?  It's going to be 2 patches per
release, 2 standard term releases + 2 LTS releases, release every month
=> 96 patches to ack throughout the year (unless my reasoning is wrong).


The basic mechanics of a release would be easy to automate. I already 
use a script provided by Ilya to create the release commits, send them 
to the mailing list, and merge the commits and create the tag. The 
things that currently cannot be automated are:


1) Acks for release patches. We currently post the patches to the 
mailing list and await acks before merging/tagging. We could potentially 
agree as a project that these commits could just be automatically 
created and pushed/tagged without developer acks.


2) Signing the tag. When I create OVN releases, I sign the tag with my 
GPG key. I'm not sure what a good automated substitute for this would be.


3) Updating ovn.org. For .0 releases, I typically will go through all 
the commits and keep a list of new features and performance 
enhancements. I then write these up on the release page. Automating this 
exact process would not be possible. However we could switch to a 
changelog style where we list all the commits that went into a given 
release. This would be easier to automate. For .1 releases, we shouldn't 
have new features or performance improvements. We should only have bug 
fixes. The changelog approach would fit for these types of releases well.




Regards,
Dumitru



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

Re: [ovs-dev] [PATCH 1/2] stream-ssl: Disable alerts on unexpected EOF.

[Simon Horman]

On Fri, May 26, 2023 at 06:57:09PM +0200, Ilya Maximets wrote:
> On 5/25/23 15:21, Simon Horman wrote:
> > On Wed, May 17, 2023 at 06:51:04PM +0200, Ilya Maximets wrote:
> >> OpenSSL 3.0 enabled alerts for unexpected EOF by default.  It supposed
> >> to alert the application whenever the connection terminated without
> >> a proper close_notify.  And that should allow applications to take
> >> actions to protect themselves from potential TLS truncation attack.
> >> This is how it looks like in the log:
> >>
> >>  |stream_ssl|WARN|SSL_read: error:0A000126:SSL routines::unexpected eof 
> >> while reading
> >>  |jsonrpc|WARN|ssl:127.0.0.1:34288: receive error: Input/output error
> >>  |reconnect|WARN|ssl:127.0.0.1:34288: connection dropped (Input/output 
> >> error)
> >>
> >> The problem is that clients based on OVS libraries do not wait for
> >> the proper termination if it didn't happen right away.  It means that
> >> chances to have alerts on the server side for every single disconnection
> >> are very high.
> >>
> >> None of the high level protocols supported by OVS daemons can carry
> >> state between re-connections, e.g., there are no session cookies or
> >> anything like that.  So, the TLS truncation attack is no applicable.
> >>
> >> Disable the alert to avoid unnecessary warnings in the log.
> >>
> >> Signed-off-by: Ilya Maximets 
> > 
> > Reviewed-by: Simon Horman 
> 
> Thanks for review!  I applied this one patch for now.
> Backported down to 2.17 since it's an issue with support for OpenSSL 3.0+.
> 
> > Are there any plans to enhance the client-side behaviour?
> 
> It's a bit unclear how to do.  SSL_shutdown() may fail with either
> WANT_READ or WANT_WRITE.  On a nonblocking socket that means that
> the application is expected to poll for the file descriptor to become
> readable or writable.  That means that we either block in the
> stream_close(), which may take unpredictable amount of time, or we
> need to fail the stream_close() with EAGAIN.  At this point we will
> have the same dilemma for a library that calls stream_close().
> Currently, we're just force-closing the socket instead.

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

Re: [ovs-dev] [PATCH 2/2] tests: Check ovsdb-server logs in OVSDB tests.

[Simon Horman]

On Fri, May 26, 2023 at 03:11:01PM +0200, Ilya Maximets wrote:
> On 5/25/23 15:20, Simon Horman wrote:
> > On Wed, May 17, 2023 at 06:51:05PM +0200, Ilya Maximets wrote:
> >> Many OVSDB tests are not checking the server log for warnings or
> >> errors.  Some are not even using the log file.  It's mostly OK as we're
> >> usually checking the user-visible behavior.  But it would also be nice
> >> to detect some internal warnings if there are some.
> >>
> >> Moving the OVSDB_SERVER_SHUTDOWN macro to the common place, adding
> >> the call to check_logs into it and making OVSDB tests use this macro.
> >>
> >> Signed-off-by: Ilya Maximets 
> > 
> > Reviewed-by: Simon Horman 
> > 
> > As an aside.
> > Some of the lines in the test suite are excessively long.
> > 
> 
> I guess, I can post v2 trying to wrap some lines that this patch is touching.
> I just thought it would be harder to review this way.  What do you think?

I think that is a change for another time.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovsdb: Condition: Process condition changes incrementally.

[Simon Horman]

On Fri, May 26, 2023 at 07:56:06PM +0200, Ilya Maximets wrote:
> My commit hook capitalized 'c' in the 'condition'.  I can fix that
> before applying.

Sounds good.

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

Re: [ovs-dev] [PATCH ovn v2] controller: Ignore DNS queries with RRs

[Simon Horman]

On Fri, May 26, 2023 at 01:38:54PM +0200, Ales Musil wrote:
> On Fri, May 26, 2023 at 12:56 PM Simon Horman 
> wrote:
> 
> > On Thu, May 25, 2023 at 12:23:43PM -0400, Brian Haley wrote:
> > > Sorry for the top post, but I was wondering if there was a way to
> > re-trigger
> > > the bot testing action on a patch? Somehow the testing on the v2 one
> > failed
> > > even though v1 passed [0]. Since the only change was in the commit
> > message
> > > seems it could just be a flaky test? Unless I'm missing something.
> >
> > Hi Brian,
> >
> > I would suspect it is a flaky test too.  Probably that should be addressed,
> > for the reason that I think your email illustrates: it diminishes the value
> > of the tests as a whole.
> >
> > As for your question. No, AFAIK, the tests can't be re-triggered.
> > However, these tests are executed by GitHub actions.
> > And if you push to a GitHub repository, say a copy of the
> > OVN repository one that you control, then the tests will run.
> > And you can also re-trigger them there.
> >
> > In this case the series_356692 branch of the ovsrobot/ovn repository
> > would be of particular interest.
> >
> > Link: https://github.com/ovsrobot/ovn/actions/runs/5083467342
>
> Hi,
> 
> the test takes longer than the set timeout. I've posted a patch to extend
> the timeout to the same value as ovn-kubernetes uses, this should hopefully
> help.

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

Re: [ovs-dev] [PATCH ovn v7 0/5] Implement MTU Path Discovery for multichassis ports

[Mark Michelson]

Thanks Ihar and Dumitru.

I applied this series to main and branch-23.06. I encountered conflicts 
when trying to apply this to branch-23.03, specifically with patch 4. I 
suspect a lot of this is due to the fact that the tiered ACL changes are 
not present in branch-23.03, so


* The table numbers don't work out the same in some cases.
* ACL flow actions are different prior to 23.06.

If you can post backport versions of the patches for 23.03 and below, 
that would be great, Ihar.


Thanks,
Mark Michelson

On 5/23/23 17:55, Ihar Hrachyshka wrote:

This series fixes a non-optimal behavior with some multichassis ports.

Specifically,

- when a multichassis port belongs to a switch that also has a localnet
   port,
- because ingress and egress traffic for the port is funnelled through
   tunnels to guarantee delivery of packets to all chassis that host the
   port,
- because tunnels add overhead to each funnelled packet,
- leaving less space for actual packets,

... the effective MTU of the path for multichassis ports is reduced by
the size that takes the tunnel to wrap a packet around. (This depends on
the type of tunnel, also on IP version of the tunnel.)

When this happens, the port and its peers are not notified about the
reduced MTU for the path to the port, effectively blackholing some of
the larger packets.

This patch series makes OVN detect these scenarios and handle them as
follows:

- for all multichassis ports, 4 flows are added - for inport and
   outport matches - to detect "too-big" IP packets;
- for all "too-big" packets, 2 flows are added to produce a ICMP
   Fragmentation Needed (for IPv4) or Too Big (for IPv6) packet and
   return it to the offending sender.

The proposed implementation is isolated in ovn-controller. An
alternative would be to implement flows producing ICMP errors via the
existing Logical_Flow action icmp4_error, as is done for router ports.
In this case, the 4 flows that detect "too-big" packets would still have
to reside in ovn-controller because of limitations of the current OVN
port model, namely lack of `mtu` as an attribute of OVN logical port.

Caveats: this works for IP traffic only. The party receiving the ICMP
error should handle it by temporarily lowering the MTU used to reach the
port. Behavior may depend on protocol implementation of the offending
peer as well as its networking stack configuration.

This patch series was successfully tested with Linux kernel 6.0.8.

This patch series is designed to simplify backporting process. It is
split in logical pieces to simplify cherry-picking. I consider the
original behavior described at the start of the cover letter a bug and
worth a backport.

---
v1: - initial version.
v2: - more system test cases adjusted to new table numbers for egress
   pipeline;
 - switch from xxd to coreutils (tr, head) tools to avoid a new
   dependency;
 - adjusted a comment in the new test case to avoid "migrator"
   terminology that makes no sense outside live migration context -
   which is not the only potential use case for multichassis ports;
 - guard the new test case with HAVE_SCAPY;
 - nit: rewrapped some lines in patch 6/7 to avoid unnecessary line
   changes;
 - added a NEWS entry for patch 6/7 that implements the core of the
   feature;
 - rebased to latest main.
v3: - expose and reuse lib/actions.c:encode_[start|finish]_controller_op
   functions;
 - reuse ETH_HEADER_LEN, IP_HEADER_LEN, and IPV6_HEADER_LEN from
   ovs:lib/packets.h;
 - add a comment for REGBIT_PKT_LARGER that mentions that the
   register is also used in OFTABLE_OUTPUT_LARGE_PKT_DETECT table;
 - squash the patch that makes if-status-mgr track changes to
   interface mtu into the patch that introduces the mtu field in the
   manager;
 - doc: don't describe path discovery flows in the patch that
   introduces new tables (only in the patch that actually implements
   the flows);
 - nit: use MAX to shave off several lines of code;
 - nit: remove redundant if_status_mgr_iface_get_mtu declaration from
   a header file;
 - rebased to latest main;
 - updated acks based on latest review comments.
v4: - fix compilation issue in the middle of the series due to previous
   commit rearrangement.
v5: - introduce a new if-status-mgr input to pflow_output, then process
   OVS interface updates inside if-status-mgr handlers.
 - if-status-mgr: remove set_mtu public function, instead other
   modules should call to if_status_mgr_iface_update. This allows the
   callers to not care which particular fields if-status-mgr would
   like to persist from the OVS record, keeping concerns separated.
v6: - update more tests in ovn-controller.at to reflect new table
   numbers.
v7: - remove ovs submodule bump that creeped in by mistake.
---

Ihar Hrachyshka (5):
   Track ip version of tunnel in chassis_tunnel struct
   Track interface MTU in 

Re: [ovs-dev] Scale testing OVN with ovn-heater for OpenStack use cases

[Felix Huettner via dev]

Hi Dumitru,

On Fri, May 26, 2023 at 01:30:54PM +0200, Dumitru Ceara wrote:
> On 5/24/23 09:37, Felix Huettner wrote:
> > Hi everyone,
>
> Hi Felix,
>
> >
> > Ilya mentioned to me that you will want to bring openstack examples to
> > ovn-heater.
> >
>
> Yes, we're discussing that.
>
> > I wanted to ask how to best join this effort. It would be great for us
>
> Everyone is welcome to contribute! :)
>

Great, we will try that out.

> > to have a tool to scale test ovn. Also we have an ovn deployment with
> > currently 450 compute nodes where we can try to extract the typically
> > used northbound entries from.
> >
>
> This is great!  I'm not an OpenStack user so I'm probably not using the
> right terminology and I might be missing things but I think we're
> interested in:
> - how many tenant networks do you have in the cluster
> - for each tenant network:
>   - how many VMs (on average but maybe also min/max/etc)
>   - how many floating IPs (dnat-and-snat)
>   - how many compute nodes run VMs from this network
> - for each compute:
>   - how many VMs run on it (on average but also min/max/etc)
>

Ok, so i'll try to give an overview below.
If you need any more detail just ask.

General amount of nodes:
* ~500 Hypervisors
* 9 Nodes that are just serve as gateway chassis (connected to 4
  different external networks)

Amount of tenant Networks (== logical routers): 2300 (only 1400 are
actually hosting VMs).

Amount of logical Routers: 2010 (2000 of these have a chassisredirect
port that is hosted on one of the 9 nodes.

* Amount of VMs:
   * min: 1
   * max: 300
   * avg: 4.5
   * 630 networks only have a single VM, 230 have two VMs and 190 have 3
 VMs.
* Amount of Hypervisors per network:
   * min: 1
   * max: 90
   * avg: 3.6
   * 650 networks only strech on a single hypervisor, 250 on two
 hypervisors, 180 on three hypervisors

Regarding floating ips we have currently 1530. I'll give you the amount
per Logical router:
* min: 1
* max: 230
* avg: 2.7
* 430 routers have a single floating ip, 140 routers have 2 floating
  ips.

Regarding VMs per Hypervisor we have:
* min: 1
* max: 80
* avg: 15


Other potentially interesting values:

* Amount of Load_Balancer: 0
* Amount of Logical_Switch_Port type=virtual: 800
* Amount of Port_Group: 6000
* Amount of ACL: 35500
   * All of these referce portgroups in `match` using `@` or `$`
   * Referencing other portgroups using `$`: 8300

Regards
Felix

> Mayba Frode and Daniel can expand on this.
>
> Regards,
> Dumitru
>
> > Thanks
> > Felix
> >
> > On Tue, May 16, 2023 at 05:38:25PM +0200, Daniel Alvarez Sanchez wrote:
> >> Hey Frode, Dumitru, thanks a lot for initiating this
> >>
> >> On Thu, May 11, 2023 at 6:05 PM Frode Nordahl 
> >> wrote:
> >>
> >>> Hello, Dumitru, Daniel and team,
> >>>
> >>> On Thu, May 11, 2023 at 5:23 PM Dumitru Ceara  wrote:
> 
>  Hi Frode,
> 
>  During an internal discussion with RedHat OpenStack folks (Daniel in cc)
>  about scalability of OVN in OpenStack deployments I remembered that you
>  mentioned that you're thinking of enhancing ovn-heater in order to test
>  your types of deployments [0].  I assumed that those are also OpenStack
>  based deployments so I was wondering if there's an opportunity for
>  collaboration here.
> >>>
> >>> Thank you for reaching out to me on this topic. We do indeed have scale
> >>> testing in the context of OpenStack on our roadmap and welcome the
> >>> invitation to collaborate on this.
> >>>
> >>> As you know we did some preparation work together already as you refer
> >>> to in [0], and we thank you for your help in upstreaming ovn-heater.
> >>>
> >>> We have yet to schedule exactly when in the cycle to work on this, I
> >>> guess a next step could be to set up a meet to share some ideas and
> >>> see how it aligns?
> >>
> >>
> >>> If that resonates with you, I'd be happy to organize.
> >>>
> >>
> >> Sounds good, please do :)
> >>
> >>
> >>>
>  Having OpenStack-like scenarios in ovn-heater would really allow OVN
>  developers to profile/troubleshoot/optimize OVN for those use cases too.
> 
>  Well, in general, the above is true for any other CMS too.
> >>>
> >>> Yes, this is what we have been thinking as well. We are supporting other
> >>> products in their journey with OVN as well, so similar projects may come
> >>> from those in the future.
> >>>
> >>
> >> Yes, we can try to model OpenStack-like scenarios for ovn-heater where the
> >> resources are generally more distributed across the hypervisors than in k8s
> >> for example, perhaps larger L2 domain sizes, etcetera.
> >> Happy to discuss further!
> >>
> >> daniel
> >>
> >>
> >>> --
> >>> Frode Nordahl
> >>>
>  Thanks,
>  Dumitru
> 
>  [0]
> >>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-March/402680.html
> 
> >>>
> >>>
> >> ___
> >> dev mailing list
> >> d...@openvswitch.org
> >> 

Re: [ovs-dev] [PATCH ovn 5/5] controller, northd: pass arp/nd from HW VTEP to lrouter pipeline

[Vladislav Odintsov]

v2 of patch #5 was submitted:

https://patchwork.ozlabs.org/project/ovn/patch/20230530150328.1224972-1-odiv...@gmail.com/

> On 30 May 2023, at 13:12, Vladislav Odintsov  wrote:
> 
> 
> 
>> On 30 May 2023, at 13:10, Dumitru Ceara  wrote:
>> 
>> On 5/30/23 11:59, Vladislav Odintsov wrote:
>>> Hi Dumitru,
>>> 
>>> thanks for the review!
>>> My answers are inline.
>>> 
 On 30 May 2023, at 12:27, Dumitru Ceara  wrote:
 
 On 5/19/23 20:18, Vladislav Odintsov wrote:
> This patch is intended to make next two changes:
> 
> 1. Support create/update of MAC_Binding for GARP/ND from HW VTEP.
> 
> Prior to this patch MAC_Binding records were created only when LRP issued
> ARP request/Neighbor solicitation.  If IP to MAC in network attached via
> vtep lport was changed the old MAC_Binding prevented normal
> communication.  Now router port (chassisredirect) in logical switch with
> vtep lport catches GARP or Neighbor Solicitation and updates MAC_Binding.
> 
> New flow with max priority was added in ls_in_arp_nd_resp and additional
> OF rule in table 37 (OFTABLE_REMOTE_OUTPUT) to process multicast packets
> received from RAMP_SWITCH.
> 
> 2. Answer to ARP/ND on requests coming from HW VTEP in lrouter pipeline.
> 
> This is needed to reduce duplicated arp-responses, which were generated
> by OVN arp-responder flows in node, where chassisredirect port located
> with responses coming directly from VM interfaces.
> 
> As a result of this change, now vifs located on same chassis, where
> chassisredirect ports are located receive ARP/ND mcast packets, which
> previously were suppressed by arp-responder on the node.
> 
> Tests and documentation were updated to conform to new behaviour.
> 
> Signed-off-by: Vladislav Odintsov 
> ---
 
 Hi Vladislav,
 
 Thanks for the patch.
 
> controller/binding.c| 48 
> controller/local_data.h |  3 ++
> controller/physical.c   | 46 +--
> northd/northd.c | 10 +
> northd/ovn-northd.8.xml |  6 +++
> tests/ovn.at | 99
> -
 
 Could you please add a NEWS entry for this behavior change?
>>> 
>>> Sure, will do it in v2.
>>> 
 
> 6 files changed, 207 insertions(+), 5 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index c7a2b3f10..5932ad785 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -509,6 +509,30 @@ update_ld_multichassis_ports(const struct
> sbrec_port_binding *binding_rec,
>}
> }
> 
> +static void
> +update_ld_vtep_port(const struct sbrec_port_binding *binding_rec,
> +struct hmap *local_datapaths)
> +{
> +struct local_datapath *ld
> += get_local_datapath(local_datapaths,
> + binding_rec->datapath->tunnel_key);
> +if (!ld) {
> +return;
> +}
> +
> +if (ld->vtep_port && strcmp(ld->vtep_port->logical_port,
> +binding_rec->logical_port)) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +VLOG_WARN_RL(, "vtep port '%s' already set for datapath "
> + "'%"PRId64"', skipping the new port '%s'.",
> + ld->vtep_port->logical_port,
> + binding_rec->datapath->tunnel_key,
> + binding_rec->logical_port);
> +return;
> +}
> +ld->vtep_port = binding_rec;
> +}
> +
> static void
> update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
>struct shash *bridge_mappings,
> @@ -1987,6 +2011,7 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> struct binding_ctx_out *b_ctx_out)
>struct ovs_list external_lports =
> OVS_LIST_INITIALIZER(_lports);
>struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER(
>
> _ports);
> +struct ovs_list vtep_lports = OVS_LIST_INITIALIZER(_lports);
> 
>struct lport {
>struct ovs_list list_node;
> @@ -2010,8 +2035,13 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> struct binding_ctx_out *b_ctx_out)
> 
>switch (lport_type) {
>case LP_PATCH:
> +update_related_lport(pb, b_ctx_out);
> +break;
>case LP_VTEP:
>update_related_lport(pb, b_ctx_out);
> +struct lport *vtep_lport = xmalloc(sizeof *vtep_lport);
> +vtep_lport->pb = pb;
> +ovs_list_push_back(_lports, _lport->list_node);
>break;
> 
>case LP_LOCALPORT:
> @@ -2111,6 

[ovs-dev] [PATCH ovn v2] controller, northd: pass arp/nd from HW VTEP to lrouter pipeline

[Vladislav Odintsov]

This patch is intended to make next two changes:

1. Support create/update of MAC_Binding for GARP/ND from HW VTEP.

Prior to this patch MAC_Binding records were created only when LRP issued
ARP request/Neighbor solicitation.  If IP to MAC in network attached via
vtep lport was changed the old MAC_Binding prevented normal
communication.  Now router port (chassisredirect) in logical switch with
vtep lport catches GARP or Neighbor Solicitation and updates MAC_Binding.

New flow with max priority was added in ls_in_arp_nd_resp and additional
OF rule in table 37 (OFTABLE_REMOTE_OUTPUT) to process multicast packets
received from RAMP_SWITCH.

2. Answer to ARP/ND on requests coming from HW VTEP in lrouter pipeline.

This is needed to reduce duplicated arp-responses, which were generated
by OVN arp-responder flows in node, where chassisredirect port located
with responses coming directly from VM interfaces.

As a result of this change, now vifs located on same chassis, where
chassisredirect ports are located receive ARP/ND mcast packets, which
previously were suppressed by arp-responder on the node.

Tests and documentation were updated to conform to new behaviour.

Signed-off-by: Vladislav Odintsov 
---
v1 -> v2:
  - Addressed Dumitru's review comments.

---
 NEWS|  2 +
 controller/binding.c| 47 +++
 controller/local_data.h |  3 ++
 controller/physical.c   | 45 ++-
 northd/northd.c | 10 +
 northd/ovn-northd.8.xml |  6 +++
 tests/ovn.at| 99 -
 7 files changed, 208 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index ec79e5fe7..2c31e5259 100644
--- a/NEWS
+++ b/NEWS
@@ -37,6 +37,8 @@ OVN v23.06.0 - xx xxx 
   - Support for tiered ACLs has been added. This allows for ACLs to be layered
 into separate tiers of priority. For more information, please see the
 ovn-nb and ovn-northd manpages.
+  - Support to create/update MAC_Binding when GARP received from VTEP (RAMP)
+switch on l3dgw port.
 
 OVN v23.03.0 - 03 Mar 2023
 --
diff --git a/controller/binding.c b/controller/binding.c
index c7a2b3f10..a90466bd6 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -509,6 +509,30 @@ update_ld_multichassis_ports(const struct 
sbrec_port_binding *binding_rec,
 }
 }
 
+static void
+update_ld_vtep_port(const struct sbrec_port_binding *binding_rec,
+struct hmap *local_datapaths)
+{
+struct local_datapath *ld
+= get_local_datapath(local_datapaths,
+ binding_rec->datapath->tunnel_key);
+if (!ld) {
+return;
+}
+
+if (ld->vtep_port && strcmp(ld->vtep_port->logical_port,
+binding_rec->logical_port)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "vtep port '%s' already set for datapath "
+ "'%"PRId64"', skipping the new port '%s'.",
+ ld->vtep_port->logical_port,
+ binding_rec->datapath->tunnel_key,
+ binding_rec->logical_port);
+return;
+}
+ld->vtep_port = binding_rec;
+}
+
 static void
 update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
 struct shash *bridge_mappings,
@@ -1987,6 +2011,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
binding_ctx_out *b_ctx_out)
 struct ovs_list external_lports = OVS_LIST_INITIALIZER(_lports);
 struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER(
 _ports);
+struct ovs_list vtep_lports = OVS_LIST_INITIALIZER(_lports);
 
 struct lport {
 struct ovs_list list_node;
@@ -2010,8 +2035,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
binding_ctx_out *b_ctx_out)
 
 switch (lport_type) {
 case LP_PATCH:
+update_related_lport(pb, b_ctx_out);
+break;
 case LP_VTEP:
 update_related_lport(pb, b_ctx_out);
+struct lport *vtep_lport = xmalloc(sizeof *vtep_lport);
+vtep_lport->pb = pb;
+ovs_list_push_back(_lports, _lport->list_node);
 break;
 
 case LP_LOCALPORT:
@@ -2111,6 +2141,15 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
binding_ctx_out *b_ctx_out)
 free(multichassis_lport);
 }
 
+/* Run through vtep lport list to see if there are vtep ports
+ * on local datapaths discovered from above loop, and update the
+ * corresponding local datapath accordingly. */
+struct lport *vtep_lport;
+LIST_FOR_EACH_POP (vtep_lport, list_node, _lports) {
+update_ld_vtep_port(vtep_lport->pb, b_ctx_out->local_datapaths);
+free(vtep_lport);
+}
+
 shash_destroy(_mappings);
 /* Remove stale QoS entries. */
 ovs_qos_entries_gc(b_ctx_in->ovs_idl_txn, 

Re: [ovs-dev] [PATCH ovn v4] controller: Ignore DNS queries with RRs

[Dumitru Ceara]

On 5/26/23 16:48, Brian Haley wrote:
> DNS queries with optional records (RRs), for example, with
> cookies for EDNS, are not supported by the OVN resolver.
> Trying to reply will result in mangled responses that
> clients do not understand - the ANSWER section will
> contain an incorrect option.
> 
> Instead, just return early when one is present, which
> will trigger a negative response and cause clients to
> go to the upstream forwarder, hopefully resulting in a
> successful query.
> 
> In our testing, the resolver only retries if the
> response is correctly formatted, which now happens
> with this change.
> 
> Reported-at: https://github.com/ovn-org/ovn/issues/192
> Reported-by: Nicolas Bock 
> Signed-off-by: Brian Haley 
> ---

Thanks for the patch, Brian!

Applied to main and backported to all branches down to 22.03.

Regards,
Dumitru

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

Re: [ovs-dev] [PATCH ovn] ci: ovn-kubernetes: Align the timeouts with u/s ovnk

[Dumitru Ceara]

On 5/26/23 13:46, Dumitru Ceara wrote:
> On 5/26/23 13:33, Ales Musil wrote:
>> We saw a lot of failures recently due to jobs timing out.
>> Align the timeouts with upstream ovn-kubernetes.
> 
> Hi Ales,
> 
> Thanks for this!
> 
> Nit: I'd point to the ovn-kubernetes commit (or permalink) where
> timeouts were increased.
> 
> If this passes ovn-kubernetes tests in the robot run [0]:
> 
> Acked-by: Dumitru Ceara 

It passed CI.  I changed my ack into a signed-off-by and pushed the
patch to the main branch and backported it to all branches down to 22.03.

Regards,
Dumitru

> 
> Regards,
> Dumitru
> 
> [0] https://github.com/ovsrobot/ovn/actions/runs/5090383556
> 

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

Re: [ovs-dev] [PATCH ovn v2] tests: Fixed load balancing system-tests

[Dumitru Ceara]

On 1/18/23 21:09, Mark Michelson wrote:
> Thanks, Xavier.
> 
> Acked-by: Mark Michelson 
> 
> I pushed this change to main and branch-22.12. The patch did not apply
> cleanly to branch-22.09 and anything before. I suspect that those
> branches have this problem, too though. If you could post backports for
> 22.09, 22.06, and 22.03, I can rubber-stamp those and merge them as well.
> 

This is a bit weird but trying to cherry pick the patch from
branch-22.12 to branch-22.09 it applied correctly.  I went ahead and
pushed it to branch 22.09, 22.06 and 22.03 as system tests were failing
without this change.

Regards,
Dumitru

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

Re: [ovs-dev] [PATCH ovn] controller: Handle OpenFlow errors.

[Dumitru Ceara]

On 5/25/23 13:40, Ales Musil wrote:
> 
> 
> On Thu, May 25, 2023 at 10:37 AM Dumitru Ceara  > wrote:
> 
> Whenever an OpenFlow error is returned by OvS, trigger a reconnect of
> the OpenFlow (rconn) connection.  This will clear any installed OpenFlow
> rules/groups. To ensure consistency, trigger a full I-P recompute too.
> 
> An example of scenario that can result in an OpenFlow error returned by
> OvS follows (describing two main loop iterations in ovn-controller):
>   - Iteration I:
>     a. get updates from SB
>     b. process these updates and generate "desired" openflows (lets
> assume
>     this generates quite a lot of desired openflow modifications)
>     c.1. add bundle-open msg to rconn
>     c.2. add openflow mod msgs to rconn (only some of these make it
> through,
>     the rest gets queued, the rconn is backlogged at this point).
>     c.3. add bundle-commit msg to rconn (this gets queued)
> 
>   - Iteration II:
>     a. get updates from SB (rconn is still backlogged)
>     b. process the updates and generate "desired" openflows (lets assume
>     this takes 10+ seconds for the specific SB updates)
> 
> At some point, while step II.b was being executed OvS declared the
> bundle
> operation (started at I.c.1) timeout.  We now act on this error by
> reconnecting which in turn triggers a flush of the rconn backlog and
> gives more chance to the next full recompute to succeed in installing
> all flows.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2134880
> 
> Reported-by: François Rigault  >
> CC: Ilya Maximets mailto:i.maxim...@ovn.org>>
> Signed-off-by: Dumitru Ceara  >
> 
> 
> Hi Dumitru,
> 
> sorry for being nitpicky, please see my comment below.
>  
> 
> ---
>  controller/ofctrl.c         | 17 ++---
>  controller/ofctrl.h         |  2 +-
>  controller/ovn-controller.c | 10 --
>  3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index b1ba1c743a..1da23bc27e 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -766,13 +766,18 @@ ofctrl_get_mf_field_id(void)
> 
>  /* Runs the OpenFlow state machine against 'br_int', which is local
> to the
>   * hypervisor on which we are running.  Attempts to negotiate a
> Geneve option
> - * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE. */
> -void
> + * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE.
> + *
> + * Returns 'true' if an OpenFlow reconnect happened; 'false' otherwise.
> + */
> +bool
>  ofctrl_run(const struct ovsrec_bridge *br_int,
>             const struct ovsrec_open_vswitch_table *ovs_table,
>             struct shash *pending_ct_zones)
>  {
>      char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> br_int->name);
> +    bool reconnected = false;
> +
>      if (strcmp(target, rconn_get_target(swconn))) {
>          VLOG_INFO("%s: connecting to switch", target);
>          rconn_connect(swconn, target, target);
> @@ -782,10 +787,12 @@ ofctrl_run(const struct ovsrec_bridge *br_int,
>      rconn_run(swconn);
> 
>      if (!rconn_is_connected(swconn)) {
> -        return;
> +        goto done;
> 
> 
> We don't have to introduce the "done" label. AFIACT you can just return
> the "reconnected" right here with the same effect.
>  
> 
>      }
> +
>      if (seqno != rconn_get_connection_seqno(swconn)) {
>          seqno = rconn_get_connection_seqno(swconn);
> +        reconnected = true;
>          state = S_NEW;
> 
>          /* Reset the state of any outstanding ct flushes to resend
> them. */
> @@ -855,6 +862,9 @@ ofctrl_run(const struct ovsrec_bridge *br_int,
>           * point, so ensure that we come back again without waiting. */
>          poll_immediate_wake();
>      }
> +
> +done:
> +    return reconnected;
>  }
> 
>  void
> @@ -909,6 +919,7 @@ ofctrl_recv(const struct ofp_header *oh, enum
> ofptype type)
>      } else if (type == OFPTYPE_ERROR) {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30,
> 300);
>          log_openflow_rl(, VLL_INFO, oh, "OpenFlow error");
> +        rconn_reconnect(swconn);
>      } else {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30,
> 300);
>          log_openflow_rl(, VLL_DBG, oh, "OpenFlow packet ignored");
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index f5751e3ee4..105f9370be 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -51,7 +51,7 @@ struct 

Re: [ovs-dev] [PATCH ovn 4/5] controller: fix typo in comments

[Dumitru Ceara]

On 5/19/23 20:18, Vladislav Odintsov wrote:
> Signed-off-by: Vladislav Odintsov 
> ---

Applied to main and backported down to 22.06, thanks!

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

Re: [ovs-dev] [PATCH ovn 3/5] controller: move put_load for port-binding in function

[Dumitru Ceara]

On 5/19/23 20:18, Vladislav Odintsov wrote:
> This is done to simplify and remove duplication of code.
> New function will be used in next patch.
> 
> Signed-off-by: Vladislav Odintsov 
> ---

Applied to main branch, thanks!

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

Re: [ovs-dev] [PATCH ovn 2/5] northd: build vtep hairpin lflows only for lswitches with vtep lports

[Dumitru Ceara]

On 5/19/23 20:18, Vladislav Odintsov wrote:
> There is no need to build lflows for datapaths, where there are no vtep type
> lports.  For instance, edge-outside logical switch, which has no vtep lports
> but has many chassis redirect lport, can have many useless lflows.
> 
> Signed-off-by: Vladislav Odintsov 
> ---

Applied to main and backported to all stable branches down to 22.09, thanks!

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

Re: [ovs-dev] [PATCH ovn 1/5] northd: fix ls_in_hairpin l3dgw flow generation

[Dumitru Ceara]

On 5/19/23 20:18, Vladislav Odintsov wrote:
> This patch fixes a situation, where logical flow with incorrect syntax could
> be generated.  If a logical switch has two attached logical router ports and
> one of them has configured gateway chassis, then incorrect flow can have the
> match like:
> `reg0[14] == 1 && (is_chassis_resident("cr-lrp2") || ` or
> `is_chassis_resident("cr-lrp1"))`
> 
> The flow's match was reworked to have at maximum one 'is_chassis_resident()'
> part.  For each cr-lport a new lflow is created.  There should not be many
> cr-lports within one datapath (normally there is just one), so the lflows
> count shouldn't increase dramatically.
> 
> Now the match looks like:
> `reg0[14] == 1 && is_chassis_resident("cr-lrp2")`
> 
> As an additional enhancement, the code became easier and tests were also
> simplified.
> 
> Documentation and relevant testcases were updated.
> 
> Fixes: 4e90bcf55c2e ("controller, northd, vtep: support routed networks with 
> HW VTEP")
> Signed-off-by: Vladislav Odintsov 
> ---
>  northd/northd.c | 35 ++-
>  northd/ovn-northd.8.xml | 13 +++--
>  tests/ovn.at| 17 +++--
>  3 files changed, 24 insertions(+), 41 deletions(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 07b127cdf..d6c26735d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -7819,37 +7819,30 @@ static void
>  build_vtep_hairpin(struct ovn_datapath *od, struct hmap *lflows)
>  {
>  /* Ingress Pre-ARP flows for VTEP hairpining traffic. Priority 1000:
> - * Packets that received from non-VTEP ports should continue processing. 
> */
> -
> + * Packets that received from VTEP ports must go directly to L2LKP table.
> + */

I changed this to:

"Packets received from VTEP ports must go directly to L2LKP table."

and applied it to main and backported to all stable branches down to 22.06.

Regards,
Dumitru

>  char *action = xasprintf("next(pipeline=ingress, table=%d);",
>   ovn_stage_get_table(S_SWITCH_IN_L2_LKUP));
> -/* send all traffic from VTEP directly to L2LKP table. */
>  ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 1000,
>REGBIT_FROM_RAMP" == 1", action);
>  free(action);
>  
> -struct ds match = DS_EMPTY_INITIALIZER;
> -size_t n_ports = od->n_router_ports;
> -bool dp_has_l3dgw_ports = false;
> -for (int i = 0; i < n_ports; i++) {
> -if (is_l3dgw_port(od->router_ports[i]->peer)) {
> -ds_put_format(, "%sis_chassis_resident(%s)%s",
> -  i == 0 ? REGBIT_FROM_RAMP" == 1 && (" : "",
> -  od->router_ports[i]->peer->cr_port->json_key,
> -  i < n_ports - 1 ? " || " : ")");
> -dp_has_l3dgw_ports = true;
> -}
> -}
> -
>  /* Ingress pre-arp flow for traffic from VTEP (ramp) switch.
>  * Priority 2000: Packets, that were received from VTEP (ramp) switch and
>  * router ports of current datapath are l3dgw ports and they reside on
>  * current chassis, should be passed to next table for ARP/ND hairpin
> -* processing.
> -*/
> -if (dp_has_l3dgw_ports) {
> -ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 2000, ds_cstr(),
> -  "next;");
> +* processing. */
> +struct ds match = DS_EMPTY_INITIALIZER;
> +for (int i = 0; i < od->n_router_ports; i++) {
> +struct ovn_port *op = od->router_ports[i]->peer;
> +if (is_l3dgw_port(op)) {
> +ds_clear();
> +ds_put_format(,
> +  REGBIT_FROM_RAMP" == 1 && is_chassis_resident(%s)",
> +  op->cr_port->json_key);
> +ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 2000,
> +  ds_cstr(), "next;");
> +}
>  }
>  ds_destroy();
>  }
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 540fe03bd..a8ef00a28 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1144,16 +1144,17 @@
>
>  
>For each distributed gateway router port RP attached to
> -  the logical switch, a priority-2000 flow is added with the match
> -  reg0[14] == 1  is_chassis_resident(RP)
> -   and action next; to pass the traffic to the
> -  next table to respond to the ARP requests for the router port IPs.
> +  the logical switch and has chassis redirect port cr-RP, 
> a
> +  priority-2000 flow is added with the match
> +  
> +reg0[14] == 1  is_chassis_resident(cr-RP)
> +  
> +  and action next;.
>  
>  
>  
>reg0[14] register bit is set in the ingress L2 port
> -   security check table for traffic received from HW VTEP (ramp)
> -   ports.
> +  security check table for traffic received from HW VTEP (ramp) 
> ports.
>  
>   

[ovs-dev] [PATCH v2] Add editorconfig file

[Robin Jarry]

EditorConfig is a file format and collection of text editor plugins for
maintaining consistent coding styles between different editors and IDEs.

Initialize the file following the coding rules in
Documentation/internals/contributing/coding-style.rst

In order for this file to be taken into account (unless they use an
editor with built-in EditorConfig support), developers will have to
install a plugin.

Note: The max_line_length property is only supported by a limited number
of EditorConfig plugins. It will be ignored if unsupported.

Link: https://editorconfig.org/
Link: https://github.com/editorconfig/editorconfig-emacs
Link: https://github.com/editorconfig/editorconfig-vim
Link: 
https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length
Signed-off-by: Robin Jarry 
---

Notes:
v2: add .editorconfig to EXTRA_DIST

 .editorconfig | 14 ++
 Makefile.am   |  1 +
 2 files changed, 15 insertions(+)
 create mode 100644 .editorconfig

diff --git a/.editorconfig b/.editorconfig
new file mode 100644
index ..f7f43ecfeea3
--- /dev/null
+++ b/.editorconfig
@@ -0,0 +1,14 @@
+# See https://editorconfig.org/ for syntax reference.
+
+root = true
+
+[*]
+end_of_line = lf
+insert_final_newline = true
+trim_trailing_whitespace = true
+charset = utf-8
+max_line_length = 79
+
+[*.{c,h}]
+indent_style = space
+indent_size = 4
diff --git a/Makefile.am b/Makefile.am
index df9c33dfe631..db341504d37f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -82,6 +82,7 @@ EXTRA_DIST = \
.ci/osx-build.sh \
.ci/osx-prepare.sh \
.cirrus.yml \
+   .editorconfig \
.github/workflows/build-and-test.yml \
appveyor.yml \
boot.sh \
-- 
2.40.1

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

[ovs-dev] [PATCH] netdev-dpdk: fix warning with gcc 13

[Robin Jarry]

GCC now reports uninitialized warnings from function return values.

../lib/netdev-dpdk.c: In function 'netdev_dpdk_mempool_configure':
../lib/netdev-dpdk.c:964:22: warning: 'dmp' may be used uninitialized 
[-Wmaybe-uninitialized]
  964 | dev->dpdk_mp = dmp;
  | ~^
../lib/netdev-dpdk.c:854:21: note: 'dmp' was declared here
  854 | struct dpdk_mp *dmp, *next;
  | ^~~

Signed-off-by: Robin Jarry 
---
 lib/netdev-dpdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 9f380a910e9f..829764c59550 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -851,7 +851,7 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
 static struct dpdk_mp *
 dpdk_mp_get(struct netdev_dpdk *dev, int mtu)
 {
-struct dpdk_mp *dmp, *next;
+struct dpdk_mp *dmp = NULL, *next;
 bool reuse = false;
 
 ovs_mutex_lock(_mp_mutex);
-- 
2.40.1

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

[ovs-dev] [PATCH] Add editorconfig file

[Robin Jarry]

EditorConfig is a file format and collection of text editor plugins for
maintaining consistent coding styles between different editors and IDEs.

Initialize the file following the coding rules in
Documentation/internals/contributing/coding-style.rst

In order for this file to be taken into account (unless they use an
editor with built-in EditorConfig support), developers will have to
install a plugin.

Note: The max_line_length property is only supported by a limited number
of EditorConfig plugins. It will be ignored if unsupported.

Link: https://editorconfig.org/
Link: https://github.com/editorconfig/editorconfig-emacs
Link: https://github.com/editorconfig/editorconfig-vim
Link: 
https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length
Signed-off-by: Robin Jarry 
---
 .editorconfig | 14 ++
 1 file changed, 14 insertions(+)
 create mode 100644 .editorconfig

diff --git a/.editorconfig b/.editorconfig
new file mode 100644
index ..f7f43ecfeea3
--- /dev/null
+++ b/.editorconfig
@@ -0,0 +1,14 @@
+# See https://editorconfig.org/ for syntax reference.
+
+root = true
+
+[*]
+end_of_line = lf
+insert_final_newline = true
+trim_trailing_whitespace = true
+charset = utf-8
+max_line_length = 79
+
+[*.{c,h}]
+indent_style = space
+indent_size = 4
-- 
2.40.1

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

Re: [ovs-dev] [PATCH ovn] controller: don't report UDP as unsupported proto in svc_check

[Vladislav Odintsov]

Hi Dumitru,

I agree with the requested changes, such approach seems much better.
I’ve reworked patch and just sent v2. Please, check it out:

https://patchwork.ozlabs.org/project/ovn/patch/20230530124157.1223591-1-odiv...@gmail.com/

> On 30 May 2023, at 12:44, Dumitru Ceara  wrote:
> 
> On 5/24/23 13:03, Ales Musil wrote:
>> On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov 
>> wrote:
>> 
>>> Depending on the udp service, it can reply with some udp data.
>>> In that case ovn-controller will warn with next message:
>>> 
>>> pinctrl(ovn_pinctrl0)|WARN|handle service check: Unsupported protocol -
>>> [11]
>>> 
>>> With this patch ovn-controller ignores UDP packets, which came to
>>> pinctrl_handle_svc_check().  This is not something abnormal, so don't
>>> write warnings.
>>> 
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1913162
>>> Signed-off-by: Vladislav Odintsov 
>>> ---
>>> controller/pinctrl.c | 7 +++
>>> 1 file changed, 7 insertions(+)
>>> 
>>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>>> index b4be22020..f2e753cdb 100644
>>> --- a/controller/pinctrl.c
>>> +++ b/controller/pinctrl.c
>>> @@ -,6 +,13 @@ pinctrl_handle_svc_check(struct rconn *swconn,
>>> const struct flow *ip_flow,
>>> ip_proto = in_ip->ip6_nxt;
>>> }
>>> 
>>> +if (ip_proto == IPPROTO_UDP) {
>>> +/* We should do nothing if we got UDP packet.
>>> + * If service is running, it can respond with any UDP data,
>>> + * so just ingore it. */
>>> + return;
>>> +}
>>> +
>>> if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_ICMP &&
>>> ip_proto != IPPROTO_ICMPV6) {
>>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>>> --
>>> 2.36.1
>>> 
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>> 
>>> 
>> Looks good to me, thanks.
>> 
>> Acked-by: Ales Musil 
>> 
> 
> Hi, Vladislav, Ales,
> 
> I think it might be better to just restrict the logical flow condition
> to the type of traffic we actually want to handle in slow path (in
> pinctrl).
> 
> That is, this flow (and the others that match on ethernet destination
> being equal to $svc_monitor_mac):
> 
>ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 110,
>  "eth.dst == $svc_monitor_mac",
>  "handle_svc_check(inport);");
> 
> should probably be changed to explicitly match on the supported protocol
> list, e.g.:
> 
>ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 110,
>  "eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)",
>  "handle_svc_check(inport);");
> 
> Thanks,
> Dumitru
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Regards,
Vladislav Odintsov

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

[ovs-dev] [PATCH ovn v2] northd: match only on supported protocols to handle_svc_check

[Vladislav Odintsov]

Depending on the udp service, it can reply with some udp data.
In that case ovn-controller will warn with next message:

pinctrl(ovn_pinctrl0)|WARN|handle service check: Unsupported protocol - [11]

This is not something abnormal, so it needs to be fixed.
With this patch ovn-northd changes match of appropriate lflow, which sends
traffic to ovn-controller's pinctrl thread to handle_svc_check action.
Now only supported protocols allowed to reach ovn-controller when destined
to $svc_monitor_mac (tcp, icmp, icmpv6).

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1913162
Signed-off-by: Vladislav Odintsov 
---
v1 -> v2:
  - Addressed Dumitru's suggestion to match on supported protocols
instead of validating protocol inside pinctrl thread.
---
 northd/northd.c |  2 +-
 tests/ovn-northd.at | 30 +++---
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index a6eca916b..c459887b7 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -9124,7 +9124,7 @@ build_lswitch_destination_lookup_bmcast(struct 
ovn_datapath *od,
 ovs_assert(od->nbs);
 
 ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 110,
-  "eth.dst == $svc_monitor_mac",
+  "eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)",
   "handle_svc_check(inport);");
 
 struct mcast_switch_info *mcast_sw_info = >mcast_info.sw;
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index e3669bdf5..8eadad8bf 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4974,7 +4974,7 @@ check ovn-nbctl lsp-set-options ls2-ro2 
router-port=ro2-ls2
 ovn-sbctl lflow-list ls1 > ls1_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | 
sort], [0], [dnl
   table=??(ls_in_l2_lkup  ), priority=0, match=(1), action=(outport = 
get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup  ), priority=110  , match=(eth.dst == 
$svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup  ), priority=110  , match=(eth.dst == 
$svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup  ), priority=50   , match=(eth.dst == 
00:00:00:00:01:01), action=(outport = "ls1-ro1"; output;)
   table=??(ls_in_l2_lkup  ), priority=50   , match=(eth.dst == 
00:00:00:00:01:02), action=(outport = "vm1"; output;)
   table=??(ls_in_l2_lkup  ), priority=70   , match=(eth.mcast), 
action=(outport = "_MC_flood"; output;)
@@ -4986,7 +4986,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 
's/table=../table=??/' | sort],
 ovn-sbctl lflow-list ls2 > ls2_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed 's/table=../table=??/' | 
sort], [0], [dnl
   table=??(ls_in_l2_lkup  ), priority=0, match=(1), action=(outport = 
get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup  ), priority=110  , match=(eth.dst == 
$svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup  ), priority=110  , match=(eth.dst == 
$svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup  ), priority=50   , match=(eth.dst == 
00:00:00:00:02:01), action=(outport = "ls2-ro2"; output;)
   table=??(ls_in_l2_lkup  ), priority=50   , match=(eth.dst == 
00:00:00:00:02:02), action=(outport = "vm2"; output;)
   table=??(ls_in_l2_lkup  ), priority=70   , match=(eth.mcast), 
action=(outport = "_MC_flood"; output;)
@@ -5006,7 +5006,7 @@ check ovn-nbctl --wait=sb lr-nat-add ro2 snat 20.0.0.200 
192.168.2.200/30
 ovn-sbctl lflow-list ls1 > ls1_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 's/table=../table=??/' | 
sort], [0], [dnl
   table=??(ls_in_l2_lkup  ), priority=0, match=(1), action=(outport = 
get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup  ), priority=110  , match=(eth.dst == 
$svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup  ), priority=110  , match=(eth.dst == 
$svc_monitor_mac && (tcp || icmp || icmp6)), action=(handle_svc_check(inport);)
   table=??(ls_in_l2_lkup  ), priority=50   , match=(eth.dst == 
00:00:00:00:01:01), action=(outport = "ls1-ro1"; output;)
   table=??(ls_in_l2_lkup  ), priority=50   , match=(eth.dst == 
00:00:00:00:01:02), action=(outport = "vm1"; output;)
   table=??(ls_in_l2_lkup  ), priority=70   , match=(eth.mcast), 
action=(outport = "_MC_flood"; output;)
@@ -5019,7 +5019,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 
's/table=../table=??/' | sort],
 ovn-sbctl lflow-list ls2 > ls2_lflows
 AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed 's/table=../table=??/' | 
sort], [0], [dnl
   table=??(ls_in_l2_lkup  ), priority=0, match=(1), action=(outport = 
get_fdb(eth.dst); next;)
-  table=??(ls_in_l2_lkup  ), priority=110  , match=(eth.dst == 
$svc_monitor_mac), action=(handle_svc_check(inport);)
+  table=??(ls_in_l2_lkup  ), priority=110  , match=(eth.dst == 

Re: [ovs-dev] [PATCH v5] backtrace: Extend the backtrace functionality

[Ilya Maximets]

On 5/30/23 09:34, Ales Musil wrote:
> 
> 
> On Mon, May 29, 2023 at 7:15 PM Ilya Maximets  > wrote:
> 
> On 5/25/23 08:33, Ales Musil wrote:
> > Use the backtrace functions that is provided by libc,
> > this allows us to get backtrace that is independent of
> > the current memory map of the process. Which in turn can
> > be used for debugging/tracing purpose. The backtrace
> > is not 100% accurate due to various optimizations, most
> > notably "-fomit-frame-pointer" and LTO. This might result
> > that the line in source file doesn't correspond to the
> > real line. However, it should be able to pinpoint at least
> > the function where the backtrace was called.
> >
> > The usage for SIGSEGV is determined during compilation
> > based on available libraries. Libunwind has higher priority
> > if both methods are available to keep the compatibility with
> > current behavior.
> >
> > The backtrace is not marked as signal safe however the backtrace
> > manual page gives more detailed explanation why it might be the
> > case [0]. Load the "libgcc" or equivalent in advance within the
> > "fatal_signal_init" which should ensure that subsequent calls
> > to backtrace* do not call malloc and are signal safe.
> >
> > The typical backtrace will look similar to the one below:
> > /lib64/libopenvswitch-3.1.so 
> .0(backtrace_capture+0x1e) [0x7fc5db298dfe]
> > /lib64/libopenvswitch-3.1.so 
> .0(log_backtrace_at+0x57) [0x7fc5db2999e7]
> > /lib64/libovsdb-3.1.so.0(ovsdb_txn_complete+0x7b) [0x7fc5db56247b]
> > /lib64/libovsdb-3.1.so.0(ovsdb_txn_propose_commit_block+0x8d) 
> [0x7fc5db563a8d]
> > ovsdb-server(+0xa661) [0x562cfce2e661]
> > ovsdb-server(+0x7e39) [0x562cfce2be39]
> > /lib64/libc.so.6(+0x27b4a) [0x7fc5db048b4a]
> > /lib64/libc.so.6(__libc_start_main+0x8b) [0x7fc5db048c0b]
> > ovsdb-server(+0x8c35) [0x562cfce2cc35]
> >
> > backtrace.h elaborates on how to effectively get the line
> > information associated with the addressed presented in the
> > backtrace.
> >
> > [0]
> > backtrace() and backtrace_symbols_fd() don't call malloc()
> > explicitly, but they are part of libgcc, which gets loaded
> > dynamically when first used.  Dynamic loading usually triggers
> > a call to malloc(3).  If you need certain calls to these two
> > functions to not allocate memory (in signal handlers, for
> > example), you need to make sure libgcc is loaded beforehand
> >
> > Reported-at: https://bugzilla.redhat.com/2177760 
> 
> > Signed-off-by: Ales Musil mailto:amu...@redhat.com>>
> > ---
> > v2: Extend the current use of libunwind rather than replacing it.
> > v3: Allow vlog_fd to be also 0.
> >     Return the backtrace log from monitor in updated form.
> >     Return use of the "vlog_direct_write_to_log_file_unsafe".
> > v4: Rebase on top of current master.
> >     Address comments from Ilya:
> >     Address the coding style issues.
> >     Make sure that the backrace received by monitor doesn't
> >     have more than maximum allowed frames.
> >     Change the backtrace_format to accept delimiter.
> >     Remove unnecessary checks in the tests.
> > v5: Rebase on top of current master.
> >     Address missed comment from v3:
> >     Reaname the vlog_fd to vlog_get_log_file_fd_unsafe to reflect that
> >     this is really unsafe.
> >     Return the ovsdb backtrace to correct position in log and
> >     keep the previous format.
> > ---
> >  include/openvswitch/vlog.h |   3 +
> >  lib/backtrace.c            | 116 +
> >  lib/backtrace.h            |  57 +++---
> >  lib/fatal-signal.c         |  52 +++--
> >  lib/ovsdb-error.c          |  12 +---
> >  lib/vlog.c                 |   7 +++
> >  m4/openvswitch.m4          |   9 ++-
> >  tests/atlocal.in            |   2 +
> >  tests/daemon.at             |  45 ++
> >  9 files changed, 227 insertions(+), 76 deletions(-)
> >



> > @@ -77,41 +83,75 @@ log_backtrace_at(const char *msg, const char *where)
> >      }
> > 
> >      ds_put_cstr(, where);
> > -    VLOG_ERR("%s", backtrace_format(, ));
> > +    ds_put_cstr(, " backtrace:\n");
> > +    backtrace_format(, , "\n");
> > +    VLOG_ERR("%s", ds_cstr_ro());
> > 
> >      ds_destroy();
> >  }
> > 
> > +static bool
> > +read_received_backtrace(int fd, void *dest, size_t len)
> > +{
> > +    VLOG_WARN("%s fd %d", __func__, fd);
> > +    fcntl(fd, F_SETFL, O_NONBLOCK);
> 
> This breaks the windows build:
> 
> [00:06:56] 

Re: [ovs-dev] [PATCH v13 4/4] userspace: Enable L4 checksum offloading by default.

[Ilya Maximets]

On 5/29/23 21:56, Mike Pattrick wrote:
> On Wed, May 24, 2023 at 8:56 AM Ilya Maximets  wrote:
>>
>> On 5/17/23 05:11, Mike Pattrick wrote:
>>> From: Flavio Leitner 
>>>
>>> The netdev receiving packets is supposed to provide the flags
>>> indicating if the L4 checksum was verified and it is OK or BAD,
>>> otherwise the stack will check when appropriate by software.
>>>
>>> If the packet comes with good checksum, then postpone the
>>> checksum calculation to the egress device if needed.
>>>
>>> When encapsulate a packet with that flag, set the checksum
>>> of the inner L4 header since that is not yet supported.
>>>
>>> Calculate the L4 checksum when the packet is going to be sent
>>> over a device that doesn't support the feature.
>>>
>>> Linux tap devices allows enabling L3 and L4 offload, so this
>>> patch enables the feature. However, Linux socket interface
>>> remains disabled because the API doesn't allow enabling
>>> those two features without enabling TSO too.
>>>
>>> Signed-off-by: Flavio Leitner 
>>> Co-authored-by: Mike Pattrick 
>>> Signed-off-by: Mike Pattrick 
>>>
>>> ---
>>>  Since v9:
>>>   - Extended miniflow_extract changes into avx512 code
>>>   - Formatting changes
>>>   - Note that we cannot currently enable checksum offloading in
>>> CONFIGURE_VETH_OFFLOADS for check-system-userspace as
>>> netdev-linux.c currently only parses the vnet header if TSO
>>> is enabled.
>>>  Since v10:
>>>   - No change
>>>  Since v11:
>>>   - Added AVX512 IPv6 checksum offload support.
>>>   - Improved error messages and logging.
>>>  Since v12:
>>>   - Added missing mutex annotations
>>>
>>> Signed-off-by: Mike Pattrick 
>>> ---
>>>  lib/conntrack.c  |  15 +-
>>>  lib/dp-packet.c  |  25 
>>>  lib/dp-packet.h  |  78 +-
>>>  lib/dpif-netdev-extract-avx512.c |  62 +++-
>>>  lib/flow.c   |  23 +++
>>>  lib/netdev-dpdk.c| 176 +++---
>>>  lib/netdev-linux.c   | 243 +--
>>>  lib/netdev-native-tnl.c  |  32 +---
>>>  lib/netdev.c |  46 ++
>>>  lib/odp-execute-avx512.c |  88 ++-
>>>  lib/packets.c| 175 +-
>>>  lib/packets.h|   3 +
>>>  12 files changed, 688 insertions(+), 278 deletions(-)
>>>
>>
>> 
>>
>>> @@ -1403,7 +1409,6 @@ static int
>>>  netdev_linux_batch_rxq_recv_tap(struct netdev_rxq_linux *rx, int mtu,
>>>  struct dp_packet_batch *batch)
>>>  {
>>> -int virtio_net_hdr_size;
>>>  ssize_t retval;
>>>  size_t std_len;
>>>  int iovlen;
>>> @@ -1413,16 +1418,14 @@ netdev_linux_batch_rxq_recv_tap(struct 
>>> netdev_rxq_linux *rx, int mtu,
>>>  /* Use the buffer from the allocated packet below to receive MTU
>>>   * sized packets and an aux_buf for extra TSO data. */
>>>  iovlen = IOV_TSO_SIZE;
>>> -virtio_net_hdr_size = sizeof(struct virtio_net_hdr);
>>>  } else {
>>>  /* Use only the buffer from the allocated packet. */
>>>  iovlen = IOV_STD_SIZE;
>>> -virtio_net_hdr_size = 0;
>>>  }
>>>
>>>  /* The length here needs to be accounted in the same way when the
>>>   * aux_buf is allocated so that it can be prepended to TSO buffer. */
>>> -std_len = virtio_net_hdr_size + VLAN_ETH_HEADER_LEN + mtu;
>>> +std_len = sizeof(struct virtio_net_hdr) + VLAN_ETH_HEADER_LEN + mtu;
>>>  for (i = 0; i < NETDEV_MAX_BURST; i++) {
>>>  struct dp_packet *buffer;
>>>  struct dp_packet *pkt;
>>> @@ -1462,7 +1465,7 @@ netdev_linux_batch_rxq_recv_tap(struct 
>>> netdev_rxq_linux *rx, int mtu,
>>>  pkt = buffer;
>>>  }
>>>
>>> -if (virtio_net_hdr_size && netdev_linux_parse_vnet_hdr(pkt)) {
>>> +if (netdev_linux_parse_vnet_hdr(pkt)) {
>>
>> If TUNSETOFFLOAD failed, we will not have a vnet header, right?
> 
> The vnet header is linked to TUNSETIFF not TUNSETOFFLOAD. Support for
> this was added in 2.6.34 (Feb 2010):
> https://github.com/torvalds/linux/commit/b9fb9ee07e67fce0b7bfd517a48710465706c30a#diff-75b86939050dfaa1707f33042b068bd819e6cd34efae37535046e648a4ecd413R529
> 
> That said, I notice the releases FAQ says:
> 
> "Open vSwitch userspace is not sensitive to the Linux kernel version.
> It should build against almost any kernel, certainly against 2.6.32
> and later."
> 
> This statement is probably still true, it might be able to build but
> wouldn't run properly if a userspace tap port was used. I'll update
> that line in the documentation to reflect this.

Another option is to check if the kernel supports VNET_HDR with
TUNGETFEATURES and not use it if not supported.  TUNGETFEATURES
is available since 2.6.27.

> 
> 
> Thanks,
> M
> 
> N.B. For posterity, and because I tracked it down anyways, the support
> for TUNSETOFFLOAD was added in 3.11 in 2013
> 

Re: [ovs-dev] [PATCH ovn 09/14] northd: Incremental processing of VIF additions in 'lflow' node.

[Ilya Maximets]

On 5/29/23 20:42, Han Zhou wrote:
> 
> 
> On Mon, May 29, 2023 at 9:24 AM Ilya Maximets  > wrote:
>>
>> On 5/13/23 02:03, Han Zhou wrote:
>> > This patch introduces a change handler for 'northd' input within the
>> > 'lflow' node. It specifically handles cases when VIFs are created, which
>> > is an easier start for lflow incremental processing. Support for
>> > update/delete will be added later.
>> >
>> > Below are the performance test results simulating an ovn-k8s topology of 
>> > 500
>> > nodes x 50 lsp per node:
>> >
>> > Before:
>> > ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 -- 
>> > lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" -- 
>> > lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101"
>> >     ovn-northd completion:          773ms
>> >
>> > After:
>> > ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 -- 
>> > lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" -- 
>> > lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101"
>> >     ovn-northd completion:          30ms
>> >
>> > It is more than 95% reduction (or 20x faster).
>> >
>> > Signed-off-by: Han Zhou mailto:hz...@ovn.org>>
>> > ---
>> >  northd/en-lflow.c        |  82 -
>> >  northd/en-lflow.h        |   1 +
>> >  northd/inc-proc-northd.c |   2 +-
>> >  northd/northd.c          | 245 +--
>> >  northd/northd.h          |   6 +-
>> >  tests/ovn-northd.at       |   4 +-
>> >  6 files changed, 277 insertions(+), 63 deletions(-)
>> >
>>
>> 
>>
>> > @@ -5685,7 +5687,18 @@ ovn_dp_group_add_with_reference(struct ovn_lflow 
>> > *lflow_ref,
>> >
>> >  /* Adds a row with the specified contents to the Logical_Flow table.
>> >   * Version to use when hash bucket locking is NOT required.
>> > + *
>> > + * Note: This function can add generated lflows to the global variable
>> > + * temp_lflow_list as its output, controlled by the global variable
>> > + * add_lflow_to_temp_list. The caller of the ovn_lflow_add_... marcros 
>> > can get
>> > + * a list of lflows generated by setting add_lflow_to_temp_list to true. 
>> > The
>> > + * caller is responsible for initializing the temp_lflow_list, and also
>> > + * reset the add_lflow_to_temp_list to false when it is no longer needed.
>> > + * XXX: this mechanism is temporary and will be replaced when we add hash 
>> > index
>> > + * to lflow_data and refactor related functions.
>> >   */
>> > +static bool add_lflow_to_temp_list = false;
>> > +static struct ovs_list temp_lflow_list;
>> >  static void
>> >  do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od,
>> >                   const unsigned long *dp_bitmap, size_t dp_bitmap_len,
>> > @@ -5702,12 +5715,17 @@ do_ovn_lflow_add(struct hmap *lflow_map, const 
>> > struct ovn_datapath *od,
>> >      size_t bitmap_len = od ? ods_size(od->datapaths) : dp_bitmap_len;
>> >      ovs_assert(bitmap_len);
>> >
>> > -    old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
>> > -                               actions, ctrl_meter, hash);
>> > -    if (old_lflow) {
>> > -        ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap,
>> > -                                        bitmap_len);
>> > -        return;
>> > +    if (add_lflow_to_temp_list) {
>> > +        ovs_assert(od);
>> > +        ovs_assert(!dp_bitmap);
>> > +    } else {
>> > +        old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, 
>> > match,
>> > +                                   actions, ctrl_meter, hash);
>> > +        if (old_lflow) {
>> > +            ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap,
>> > +                                            bitmap_len);
>> > +            return;
>> > +        }
>> >      }
>>
>> Hi, Han.  Not a full review, but I'm a bit concerned that this code doesn't
>> support datapath groups.  IIUC, if some flows can be aggregated with datapath
>> groups, I-P will just add them separately.  Later a full re-compute will
>> delete all these flows and replace them with a single grouped one.  Might
>> cause some unnecessary churn in the cluster.
>>
> 
> Hi Ilya, thanks for the feedback. You are right that the current I-P doesn't 
> support datapath groups, because:
> 1. The current I-P only supports LSP (regular VIFs) related changes, which is 
> very unlikely to generate datapath groups. Even when that happens in rare 
> cases, the logic is still correct and the churn should be small.
> 2. There are two types of DP groups:
>     a. DP groups that are directly generated when northd has the knowledge 
> that a lflow should be applied to a group of DPs.
>     b. DP groups that are passively generated by combining lflows generated 
> for different DPs.
> Supporting I-P for the type-a DP groups is relatively straightforward, which 
> would be part of the I-P logic for the object that generates the DP group, 
> e.g. for LB 

Re: [ovs-dev] [PATCH ovn 5/5] controller, northd: pass arp/nd from HW VTEP to lrouter pipeline

[Vladislav Odintsov]

> On 30 May 2023, at 13:10, Dumitru Ceara  wrote:
> 
> On 5/30/23 11:59, Vladislav Odintsov wrote:
>> Hi Dumitru,
>> 
>> thanks for the review!
>> My answers are inline.
>> 
>>> On 30 May 2023, at 12:27, Dumitru Ceara  wrote:
>>> 
>>> On 5/19/23 20:18, Vladislav Odintsov wrote:
 This patch is intended to make next two changes:
 
 1. Support create/update of MAC_Binding for GARP/ND from HW VTEP.
 
 Prior to this patch MAC_Binding records were created only when LRP issued
 ARP request/Neighbor solicitation.  If IP to MAC in network attached via
 vtep lport was changed the old MAC_Binding prevented normal
 communication.  Now router port (chassisredirect) in logical switch with
 vtep lport catches GARP or Neighbor Solicitation and updates MAC_Binding.
 
 New flow with max priority was added in ls_in_arp_nd_resp and additional
 OF rule in table 37 (OFTABLE_REMOTE_OUTPUT) to process multicast packets
 received from RAMP_SWITCH.
 
 2. Answer to ARP/ND on requests coming from HW VTEP in lrouter pipeline.
 
 This is needed to reduce duplicated arp-responses, which were generated
 by OVN arp-responder flows in node, where chassisredirect port located
 with responses coming directly from VM interfaces.
 
 As a result of this change, now vifs located on same chassis, where
 chassisredirect ports are located receive ARP/ND mcast packets, which
 previously were suppressed by arp-responder on the node.
 
 Tests and documentation were updated to conform to new behaviour.
 
 Signed-off-by: Vladislav Odintsov 
 ---
>>> 
>>> Hi Vladislav,
>>> 
>>> Thanks for the patch.
>>> 
 controller/binding.c| 48 
 controller/local_data.h |  3 ++
 controller/physical.c   | 46 +--
 northd/northd.c | 10 +
 northd/ovn-northd.8.xml |  6 +++
 tests/ovn.at | 99
 -
>>> 
>>> Could you please add a NEWS entry for this behavior change?
>> 
>> Sure, will do it in v2.
>> 
>>> 
 6 files changed, 207 insertions(+), 5 deletions(-)
 
 diff --git a/controller/binding.c b/controller/binding.c
 index c7a2b3f10..5932ad785 100644
 --- a/controller/binding.c
 +++ b/controller/binding.c
 @@ -509,6 +509,30 @@ update_ld_multichassis_ports(const struct
 sbrec_port_binding *binding_rec,
 }
 }
 
 +static void
 +update_ld_vtep_port(const struct sbrec_port_binding *binding_rec,
 +struct hmap *local_datapaths)
 +{
 +struct local_datapath *ld
 += get_local_datapath(local_datapaths,
 + binding_rec->datapath->tunnel_key);
 +if (!ld) {
 +return;
 +}
 +
 +if (ld->vtep_port && strcmp(ld->vtep_port->logical_port,
 +binding_rec->logical_port)) {
 +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
 +VLOG_WARN_RL(, "vtep port '%s' already set for datapath "
 + "'%"PRId64"', skipping the new port '%s'.",
 + ld->vtep_port->logical_port,
 + binding_rec->datapath->tunnel_key,
 + binding_rec->logical_port);
 +return;
 +}
 +ld->vtep_port = binding_rec;
 +}
 +
 static void
 update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
 struct shash *bridge_mappings,
 @@ -1987,6 +2011,7 @@ binding_run(struct binding_ctx_in *b_ctx_in,
 struct binding_ctx_out *b_ctx_out)
 struct ovs_list external_lports =
 OVS_LIST_INITIALIZER(_lports);
 struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER(
 
 _ports);
 +struct ovs_list vtep_lports = OVS_LIST_INITIALIZER(_lports);
 
 struct lport {
 struct ovs_list list_node;
 @@ -2010,8 +2035,13 @@ binding_run(struct binding_ctx_in *b_ctx_in,
 struct binding_ctx_out *b_ctx_out)
 
 switch (lport_type) {
 case LP_PATCH:
 +update_related_lport(pb, b_ctx_out);
 +break;
 case LP_VTEP:
 update_related_lport(pb, b_ctx_out);
 +struct lport *vtep_lport = xmalloc(sizeof *vtep_lport);
 +vtep_lport->pb = pb;
 +ovs_list_push_back(_lports, _lport->list_node);
 break;
 
 case LP_LOCALPORT:
 @@ -2111,6 +2141,16 @@ binding_run(struct binding_ctx_in *b_ctx_in,
 struct binding_ctx_out *b_ctx_out)
 free(multichassis_lport);
 }
 
 +/* Run through vtep lport list to see if there are vtep ports
 + * on local datapaths discovered from above loop, and 

Re: [ovs-dev] [PATCH ovn 5/5] controller, northd: pass arp/nd from HW VTEP to lrouter pipeline

[Dumitru Ceara]

On 5/30/23 11:59, Vladislav Odintsov wrote:
> Hi Dumitru,
> 
> thanks for the review!
> My answers are inline.
> 
>> On 30 May 2023, at 12:27, Dumitru Ceara  wrote:
>>
>> On 5/19/23 20:18, Vladislav Odintsov wrote:
>>> This patch is intended to make next two changes:
>>>
>>> 1. Support create/update of MAC_Binding for GARP/ND from HW VTEP.
>>>
>>> Prior to this patch MAC_Binding records were created only when LRP issued
>>> ARP request/Neighbor solicitation.  If IP to MAC in network attached via
>>> vtep lport was changed the old MAC_Binding prevented normal
>>> communication.  Now router port (chassisredirect) in logical switch with
>>> vtep lport catches GARP or Neighbor Solicitation and updates MAC_Binding.
>>>
>>> New flow with max priority was added in ls_in_arp_nd_resp and additional
>>> OF rule in table 37 (OFTABLE_REMOTE_OUTPUT) to process multicast packets
>>> received from RAMP_SWITCH.
>>>
>>> 2. Answer to ARP/ND on requests coming from HW VTEP in lrouter pipeline.
>>>
>>> This is needed to reduce duplicated arp-responses, which were generated
>>> by OVN arp-responder flows in node, where chassisredirect port located
>>> with responses coming directly from VM interfaces.
>>>
>>> As a result of this change, now vifs located on same chassis, where
>>> chassisredirect ports are located receive ARP/ND mcast packets, which
>>> previously were suppressed by arp-responder on the node.
>>>
>>> Tests and documentation were updated to conform to new behaviour.
>>>
>>> Signed-off-by: Vladislav Odintsov 
>>> ---
>>
>> Hi Vladislav,
>>
>> Thanks for the patch.
>>
>>> controller/binding.c    | 48 
>>> controller/local_data.h |  3 ++
>>> controller/physical.c   | 46 +--
>>> northd/northd.c | 10 +
>>> northd/ovn-northd.8.xml |  6 +++
>>> tests/ovn.at | 99
>>> -
>>
>> Could you please add a NEWS entry for this behavior change?
> 
> Sure, will do it in v2.
> 
>>
>>> 6 files changed, 207 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/controller/binding.c b/controller/binding.c
>>> index c7a2b3f10..5932ad785 100644
>>> --- a/controller/binding.c
>>> +++ b/controller/binding.c
>>> @@ -509,6 +509,30 @@ update_ld_multichassis_ports(const struct
>>> sbrec_port_binding *binding_rec,
>>> }
>>> }
>>>
>>> +static void
>>> +update_ld_vtep_port(const struct sbrec_port_binding *binding_rec,
>>> +    struct hmap *local_datapaths)
>>> +{
>>> +    struct local_datapath *ld
>>> +    = get_local_datapath(local_datapaths,
>>> + binding_rec->datapath->tunnel_key);
>>> +    if (!ld) {
>>> +    return;
>>> +    }
>>> +
>>> +    if (ld->vtep_port && strcmp(ld->vtep_port->logical_port,
>>> +    binding_rec->logical_port)) {
>>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>>> +    VLOG_WARN_RL(, "vtep port '%s' already set for datapath "
>>> + "'%"PRId64"', skipping the new port '%s'.",
>>> + ld->vtep_port->logical_port,
>>> + binding_rec->datapath->tunnel_key,
>>> + binding_rec->logical_port);
>>> +    return;
>>> +    }
>>> +    ld->vtep_port = binding_rec;
>>> +}
>>> +
>>> static void
>>> update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
>>> struct shash *bridge_mappings,
>>> @@ -1987,6 +2011,7 @@ binding_run(struct binding_ctx_in *b_ctx_in,
>>> struct binding_ctx_out *b_ctx_out)
>>> struct ovs_list external_lports =
>>> OVS_LIST_INITIALIZER(_lports);
>>> struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER(
>>> 
>>> _ports);
>>> +    struct ovs_list vtep_lports = OVS_LIST_INITIALIZER(_lports);
>>>
>>> struct lport {
>>> struct ovs_list list_node;
>>> @@ -2010,8 +2035,13 @@ binding_run(struct binding_ctx_in *b_ctx_in,
>>> struct binding_ctx_out *b_ctx_out)
>>>
>>> switch (lport_type) {
>>> case LP_PATCH:
>>> +    update_related_lport(pb, b_ctx_out);
>>> +    break;
>>> case LP_VTEP:
>>> update_related_lport(pb, b_ctx_out);
>>> +    struct lport *vtep_lport = xmalloc(sizeof *vtep_lport);
>>> +    vtep_lport->pb = pb;
>>> +    ovs_list_push_back(_lports, _lport->list_node);
>>> break;
>>>
>>> case LP_LOCALPORT:
>>> @@ -2111,6 +2141,16 @@ binding_run(struct binding_ctx_in *b_ctx_in,
>>> struct binding_ctx_out *b_ctx_out)
>>> free(multichassis_lport);
>>> }
>>>
>>> +    /* Run through vtep lport list to see if there are vtep ports
>>> + * on local datapaths discovered from above loop, and update the
>>> + * corresponding local datapath accordingly. */
>>> +    struct lport *vtep_lport;
>>> +    ovs_list_size(_lports);
>>
>> I guess this is a leftover.
> 
> Oops, will fix it in v2.
> 
>>

Re: [ovs-dev] [PATCH ovn v7 2/5] Track interface MTU in if-status-mgr

[Dumitru Ceara]

On 5/23/23 23:55, Ihar Hrachyshka wrote:
> This will be used in a later patch to calculate the effective interface
> MTU after considering tunneling overhead.
> 
> NOTE: ideally, OVN would support Logical_Port MTU, in which case we
> wouldn't have to track OVSDB for interfaces.
> 
> Signed-off-by: Ihar Hrachyshka 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH ovn v7 5/5] Implement MTU Path Discovery for multichassis ports

[Dumitru Ceara]

On 5/23/23 23:55, Ihar Hrachyshka wrote:
> When a multichassis port belongs to a switch with a localnet port,
> packets originating or directed to the multichassis port are NOT sent
> thorugh the localnet port. Instead, tunneling is enforced in-cluster to
> guarantee delivery of all packets to all chassis of the port.
> 
> This behavior has an unfortunate side effect, where - because of
> additional tunnel header added to each packet - the effective MTU of the
> path for multichassis ports changes from what's set as mtu_request. This
> effectively makes OVN to black hole all packets for the port that use
> full capacity of the interface MTU. This breaks usual TCP / UDP
> services, among other things (SSH, iperf sessions etc.)
> 
> This patch adds flows so that
> - (in table 38) detect too-big packets (table 38), and then
> - (in table 39) icmp fragmentation needed / too big errors are sent
>   back to offending port.
> 
> Once the error is received, the sender is expected to adjust the route
> MTU accordingly, sending the next packets with the new path MTU. After a
> multichassis port is re-assigned to a single chassis, the effective path
> MTU is restored to "usual". Peers will eventually see their "learned"
> path MTU cache expire, which will make them switch back to the "usual"
> MTU.
> 
> Among other scenarios, this patch helps to maintain existing services
> working during live migration of a VM, if multichassis ports are used.
> (E.g. in OpenStack Nueutron.)
> 
> Fixes: 7084cf437421 ("Always funnel multichassis port traffic through 
> tunnels")
> 
> Signed-off-by: Ihar Hrachyshka 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

[ovs-dev] [PATCH ovn v2] northd: Add logical flow to skip GARP with LLA

[Ales Musil]

Skip GARP packet with link-local address being advertised
when "always_learn_from_arp_request=false", this should
prevent huge grow of MAC Binding table. To keep the option
consistent overwrite the previous MAC with LLA if it was
already stored in DB.

Signed-off-by: Ales Musil 
---
v2: Remove leftover from previous tests.
---
 northd/northd.c | 11 +++
 tests/ovn.at| 78 -
 2 files changed, 63 insertions(+), 26 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index a6eca916b..ff305fc67 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -11951,6 +11951,17 @@ build_neigh_learning_flows_for_lrouter(
 ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
   ds_cstr(actions));
 
+if (!learn_from_arp_request) {
+/* Add flow to skip LLA only if we don't know it already. */
+ds_clear(actions);
+ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
+  " = lookup_nd(inport, ip6.src, nd.tll); "
+  REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
+  " = lookup_nd_ip(inport, ip6.src); next;");
+ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
+  "nd_na && ip6.src == fe80::/10", ds_cstr(actions));
+}
+
 /* For other packet types, we can skip neighbor learning.
  * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
 ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
diff --git a/tests/ovn.at b/tests/ovn.at
index 6f9fbbfd2..7a2658f40 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5073,6 +5073,7 @@ AT_CLEANUP
 
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([IP relocation using GARP request])
+AT_SKIP_IF([test $HAVE_SCAPY = no])
 ovn_start
 
 # Logical network:
@@ -5172,7 +5173,9 @@ done
 test_ip() {
 # This packet has bad checksums but logical L3 routing doesn't check.
 local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
-local 
packet=${dst_mac}${src_mac}0800451c4011${src_ip}${dst_ip}00350008
+local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \
+IP(dst='${dst_ip}', src='${src_ip}')/ \
+UDP(sport=53, dport=4369)")
 shift; shift; shift; shift; shift
 hv=hv`vif_to_hv $inport`
 as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
@@ -5187,7 +5190,9 @@ test_ip() {
 # Routing decrements TTL and updates source and dest MAC
 # (and checksum).
 out_lrp=`vif_to_lrp $outport`
-echo 
f0${outport}ff0${out_lrp}0800451c"3f1101"00${src_ip}${dst_ip}00350008
+echo $(fmt_pkt "Ether(dst='f0:00:00:00:00:${outport}', 
src='00:00:00:00:ff:${out_lrp}')/ \
+IP(src='${src_ip}', dst='${dst_ip}', ttl=63)/ \
+UDP(sport=53, dport=4369)")
 fi >> $outport.expected
 done
 }
@@ -5203,8 +5208,10 @@ test_ip() {
 # SHA and REPLY_HA are each 12 hex digits.
 # SPA and TPA are each 8 hex digits.
 test_arp() {
-local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
-local 
request=${sha}08060001080006040001${sha}${spa}${tpa}
+local inport=$1 sha=$2 spa=$3 tpa=$3
+local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \
+ ARP(hwsrc='${sha}', hwdst='ff:ff:ff:ff:ff:ff', 
psrc='${spa}', pdst='${tpa}')")
+
 hv=hv`vif_to_hv $inport`
 as $hv ovs-appctl netdev-dummy/receive vif$inport $request
 
@@ -5217,53 +5224,72 @@ test_arp() {
 echo $request >> $i$j$k.expected
 fi
 done
+}
 
-# Expect to receive the reply, if any.
-if test X$reply_ha != X; then
-lrp=`vif_to_lrp $inport`
-local 
reply=${sha}ff0${lrp}08060001080006040002${reply_ha}${tpa}${sha}${spa}
-echo $reply >> $inport.expected
-fi
+test_na() {
+local inport=$1 sha=$2 spa=$3
+local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \
+ IPv6(dst='fe80::1', src='${spa}')/ \
+ ICMPv6ND_NA(tgt='${spa}')")
+
+hv=hv`vif_to_hv $inport`
+as $hv ovs-appctl netdev-dummy/receive vif$inport $request
+
+# Expect to receive the broadcast ARP on the other logical switch ports if
+# IP address is not configured to the switch patch port.
+local i=`vif_to_ls $inport`
+local j
+for j in 1 2; do
+if test $i$j != $inport; then
+echo $request >> $i$j$k.expected
+fi
+done
 }
 
-# lp11 send GARP request to announce ownership of 192.168.1.100.
+# lp11 send GARP request to announce ownership of 192.168.1.100 and 
fe80::abcd:1.
 
-sha=f011
-spa=`ip_to_hex 192 168 1 100`
-tpa=$spa
+sha="f0:00:00:00:00:11"
+spa="192.168.1.100"
+spa6="fe80::abcd:1"
 
 # When always_learn_from_arp_request=false, the new mac-binding will not be 

Re: [ovs-dev] [PATCH ovn 5/5] controller, northd: pass arp/nd from HW VTEP to lrouter pipeline

[Vladislav Odintsov]

Hi Dumitru,

thanks for the review!
My answers are inline.

> On 30 May 2023, at 12:27, Dumitru Ceara  wrote:
> 
> On 5/19/23 20:18, Vladislav Odintsov wrote:
>> This patch is intended to make next two changes:
>> 
>> 1. Support create/update of MAC_Binding for GARP/ND from HW VTEP.
>> 
>> Prior to this patch MAC_Binding records were created only when LRP issued
>> ARP request/Neighbor solicitation.  If IP to MAC in network attached via
>> vtep lport was changed the old MAC_Binding prevented normal
>> communication.  Now router port (chassisredirect) in logical switch with
>> vtep lport catches GARP or Neighbor Solicitation and updates MAC_Binding.
>> 
>> New flow with max priority was added in ls_in_arp_nd_resp and additional
>> OF rule in table 37 (OFTABLE_REMOTE_OUTPUT) to process multicast packets
>> received from RAMP_SWITCH.
>> 
>> 2. Answer to ARP/ND on requests coming from HW VTEP in lrouter pipeline.
>> 
>> This is needed to reduce duplicated arp-responses, which were generated
>> by OVN arp-responder flows in node, where chassisredirect port located
>> with responses coming directly from VM interfaces.
>> 
>> As a result of this change, now vifs located on same chassis, where
>> chassisredirect ports are located receive ARP/ND mcast packets, which
>> previously were suppressed by arp-responder on the node.
>> 
>> Tests and documentation were updated to conform to new behaviour.
>> 
>> Signed-off-by: Vladislav Odintsov 
>> ---
> 
> Hi Vladislav,
> 
> Thanks for the patch.
> 
>> controller/binding.c| 48 
>> controller/local_data.h |  3 ++
>> controller/physical.c   | 46 +--
>> northd/northd.c | 10 +
>> northd/ovn-northd.8.xml |  6 +++
>> tests/ovn.at | 99 
>> -
> 
> Could you please add a NEWS entry for this behavior change?

Sure, will do it in v2.

> 
>> 6 files changed, 207 insertions(+), 5 deletions(-)
>> 
>> diff --git a/controller/binding.c b/controller/binding.c
>> index c7a2b3f10..5932ad785 100644
>> --- a/controller/binding.c
>> +++ b/controller/binding.c
>> @@ -509,6 +509,30 @@ update_ld_multichassis_ports(const struct 
>> sbrec_port_binding *binding_rec,
>> }
>> }
>> 
>> +static void
>> +update_ld_vtep_port(const struct sbrec_port_binding *binding_rec,
>> +struct hmap *local_datapaths)
>> +{
>> +struct local_datapath *ld
>> += get_local_datapath(local_datapaths,
>> + binding_rec->datapath->tunnel_key);
>> +if (!ld) {
>> +return;
>> +}
>> +
>> +if (ld->vtep_port && strcmp(ld->vtep_port->logical_port,
>> +binding_rec->logical_port)) {
>> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> +VLOG_WARN_RL(, "vtep port '%s' already set for datapath "
>> + "'%"PRId64"', skipping the new port '%s'.",
>> + ld->vtep_port->logical_port,
>> + binding_rec->datapath->tunnel_key,
>> + binding_rec->logical_port);
>> +return;
>> +}
>> +ld->vtep_port = binding_rec;
>> +}
>> +
>> static void
>> update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
>> struct shash *bridge_mappings,
>> @@ -1987,6 +2011,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
>> binding_ctx_out *b_ctx_out)
>> struct ovs_list external_lports = OVS_LIST_INITIALIZER(_lports);
>> struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER(
>> _ports);
>> +struct ovs_list vtep_lports = OVS_LIST_INITIALIZER(_lports);
>> 
>> struct lport {
>> struct ovs_list list_node;
>> @@ -2010,8 +2035,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
>> binding_ctx_out *b_ctx_out)
>> 
>> switch (lport_type) {
>> case LP_PATCH:
>> +update_related_lport(pb, b_ctx_out);
>> +break;
>> case LP_VTEP:
>> update_related_lport(pb, b_ctx_out);
>> +struct lport *vtep_lport = xmalloc(sizeof *vtep_lport);
>> +vtep_lport->pb = pb;
>> +ovs_list_push_back(_lports, _lport->list_node);
>> break;
>> 
>> case LP_LOCALPORT:
>> @@ -2111,6 +2141,16 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
>> binding_ctx_out *b_ctx_out)
>> free(multichassis_lport);
>> }
>> 
>> +/* Run through vtep lport list to see if there are vtep ports
>> + * on local datapaths discovered from above loop, and update the
>> + * corresponding local datapath accordingly. */
>> +struct lport *vtep_lport;
>> +ovs_list_size(_lports);
> 
> I guess this is a leftover.

Oops, will fix it in v2.

> 
>> +LIST_FOR_EACH_POP (vtep_lport, list_node, _lports) {
>> +update_ld_vtep_port(vtep_lport->pb, b_ctx_out->local_datapaths);
>> +free(vtep_lport);
>> 

Re: [ovs-dev] [PATCH ovn] northd: Add logical flow to skip GARP with LLA

[Ales Musil]

On Tue, May 30, 2023 at 11:56 AM Ales Musil  wrote:

> Skip GARP packet with link-local address being advertised
> when "always_learn_from_arp_request=false", this should
> prevent huge grow of MAC Binding table. To keep the option
> consistent overwrite the previous MAC with LLA if it was
> already stored in DB.
>
> Signed-off-by: Ales Musil 
> ---
>  northd/northd.c | 11 ++
>  tests/ovn.at| 90 +++--
>  2 files changed, 75 insertions(+), 26 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index a6eca916b..ff305fc67 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11951,6 +11951,17 @@ build_neigh_learning_flows_for_lrouter(
>  ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
>ds_cstr(actions));
>
> +if (!learn_from_arp_request) {
> +/* Add flow to skip LLA only if we don't know it already. */
> +ds_clear(actions);
> +ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> +  " = lookup_nd(inport, ip6.src, nd.tll); "
> +  REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> +  " = lookup_nd_ip(inport, ip6.src); next;");
> +ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
> +  "nd_na && ip6.src == fe80::/10", ds_cstr(actions));
> +}
> +
>  /* For other packet types, we can skip neighbor learning.
>   * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
>  ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 6f9fbbfd2..63cedd334 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -5073,6 +5073,7 @@ AT_CLEANUP
>
>  OVN_FOR_EACH_NORTHD([
>  AT_SETUP([IP relocation using GARP request])
> +AT_SKIP_IF([test $HAVE_SCAPY = no])
>  ovn_start
>
>  # Logical network:
> @@ -5172,7 +5173,9 @@ done
>  test_ip() {
>  # This packet has bad checksums but logical L3 routing doesn't check.
>  local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
> -local
> packet=${dst_mac}${src_mac}0800451c4011${src_ip}${dst_ip}00350008
> +local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \
> +IP(dst='${dst_ip}', src='${src_ip}')/ \
> +UDP(sport=53, dport=4369)")
>  shift; shift; shift; shift; shift
>  hv=hv`vif_to_hv $inport`
>  as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
> @@ -5187,7 +5190,9 @@ test_ip() {
>  # Routing decrements TTL and updates source and dest MAC
>  # (and checksum).
>  out_lrp=`vif_to_lrp $outport`
> -echo
> f0${outport}ff0${out_lrp}0800451c"3f1101"00${src_ip}${dst_ip}00350008
> +echo $(fmt_pkt "Ether(dst='f0:00:00:00:00:${outport}',
> src='00:00:00:00:ff:${out_lrp}')/ \
> +IP(src='${src_ip}', dst='${dst_ip}', ttl=63)/
> \
> +UDP(sport=53, dport=4369)")
>  fi >> $outport.expected
>  done
>  }
> @@ -5203,8 +5208,10 @@ test_ip() {
>  # SHA and REPLY_HA are each 12 hex digits.
>  # SPA and TPA are each 8 hex digits.
>  test_arp() {
> -local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
> -local
> request=${sha}08060001080006040001${sha}${spa}${tpa}
> +local inport=$1 sha=$2 spa=$3 tpa=$3
> +local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> src='${sha}')/ \
> + ARP(hwsrc='${sha}',
> hwdst='ff:ff:ff:ff:ff:ff', psrc='${spa}', pdst='${tpa}')")
> +
>  hv=hv`vif_to_hv $inport`
>  as $hv ovs-appctl netdev-dummy/receive vif$inport $request
>
> @@ -5217,53 +5224,72 @@ test_arp() {
>  echo $request >> $i$j$k.expected
>  fi
>  done
> +}
>
> -# Expect to receive the reply, if any.
> -if test X$reply_ha != X; then
> -lrp=`vif_to_lrp $inport`
> -local
> reply=${sha}ff0${lrp}08060001080006040002${reply_ha}${tpa}${sha}${spa}
> -echo $reply >> $inport.expected
> -fi
> +test_na() {
> +local inport=$1 sha=$2 spa=$3
> +local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> src='${sha}')/ \
> + IPv6(dst='fe80::1', src='${spa}')/ \
> + ICMPv6ND_NA(tgt='${spa}')")
> +
> +hv=hv`vif_to_hv $inport`
> +as $hv ovs-appctl netdev-dummy/receive vif$inport $request
> +
> +# Expect to receive the broadcast ARP on the other logical switch
> ports if
> +# IP address is not configured to the switch patch port.
> +local i=`vif_to_ls $inport`
> +local j
> +for j in 1 2; do
> +if test $i$j != $inport; then
> +echo $request >> $i$j$k.expected
> +fi
> +done
>  }
>
> -# lp11 send GARP request to announce ownership of 192.168.1.100.
> +# lp11 send GARP request to announce ownership of 

[ovs-dev] [PATCH ovn] northd: Add logical flow to skip GARP with LLA

[Ales Musil]

Skip GARP packet with link-local address being advertised
when "always_learn_from_arp_request=false", this should
prevent huge grow of MAC Binding table. To keep the option
consistent overwrite the previous MAC with LLA if it was
already stored in DB.

Signed-off-by: Ales Musil 
---
 northd/northd.c | 11 ++
 tests/ovn.at| 90 +++--
 2 files changed, 75 insertions(+), 26 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index a6eca916b..ff305fc67 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -11951,6 +11951,17 @@ build_neigh_learning_flows_for_lrouter(
 ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
   ds_cstr(actions));
 
+if (!learn_from_arp_request) {
+/* Add flow to skip LLA only if we don't know it already. */
+ds_clear(actions);
+ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
+  " = lookup_nd(inport, ip6.src, nd.tll); "
+  REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
+  " = lookup_nd_ip(inport, ip6.src); next;");
+ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
+  "nd_na && ip6.src == fe80::/10", ds_cstr(actions));
+}
+
 /* For other packet types, we can skip neighbor learning.
  * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
 ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 0, "1",
diff --git a/tests/ovn.at b/tests/ovn.at
index 6f9fbbfd2..63cedd334 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5073,6 +5073,7 @@ AT_CLEANUP
 
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([IP relocation using GARP request])
+AT_SKIP_IF([test $HAVE_SCAPY = no])
 ovn_start
 
 # Logical network:
@@ -5172,7 +5173,9 @@ done
 test_ip() {
 # This packet has bad checksums but logical L3 routing doesn't check.
 local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
-local 
packet=${dst_mac}${src_mac}0800451c4011${src_ip}${dst_ip}00350008
+local packet=$(fmt_pkt "Ether(dst='${dst_mac}', src='${src_mac}')/ \
+IP(dst='${dst_ip}', src='${src_ip}')/ \
+UDP(sport=53, dport=4369)")
 shift; shift; shift; shift; shift
 hv=hv`vif_to_hv $inport`
 as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
@@ -5187,7 +5190,9 @@ test_ip() {
 # Routing decrements TTL and updates source and dest MAC
 # (and checksum).
 out_lrp=`vif_to_lrp $outport`
-echo 
f0${outport}ff0${out_lrp}0800451c"3f1101"00${src_ip}${dst_ip}00350008
+echo $(fmt_pkt "Ether(dst='f0:00:00:00:00:${outport}', 
src='00:00:00:00:ff:${out_lrp}')/ \
+IP(src='${src_ip}', dst='${dst_ip}', ttl=63)/ \
+UDP(sport=53, dport=4369)")
 fi >> $outport.expected
 done
 }
@@ -5203,8 +5208,10 @@ test_ip() {
 # SHA and REPLY_HA are each 12 hex digits.
 # SPA and TPA are each 8 hex digits.
 test_arp() {
-local inport=$1 sha=$2 spa=$3 tpa=$4 reply_ha=$5
-local 
request=${sha}08060001080006040001${sha}${spa}${tpa}
+local inport=$1 sha=$2 spa=$3 tpa=$3
+local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \
+ ARP(hwsrc='${sha}', hwdst='ff:ff:ff:ff:ff:ff', 
psrc='${spa}', pdst='${tpa}')")
+
 hv=hv`vif_to_hv $inport`
 as $hv ovs-appctl netdev-dummy/receive vif$inport $request
 
@@ -5217,53 +5224,72 @@ test_arp() {
 echo $request >> $i$j$k.expected
 fi
 done
+}
 
-# Expect to receive the reply, if any.
-if test X$reply_ha != X; then
-lrp=`vif_to_lrp $inport`
-local 
reply=${sha}ff0${lrp}08060001080006040002${reply_ha}${tpa}${sha}${spa}
-echo $reply >> $inport.expected
-fi
+test_na() {
+local inport=$1 sha=$2 spa=$3
+local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${sha}')/ \
+ IPv6(dst='fe80::1', src='${spa}')/ \
+ ICMPv6ND_NA(tgt='${spa}')")
+
+hv=hv`vif_to_hv $inport`
+as $hv ovs-appctl netdev-dummy/receive vif$inport $request
+
+# Expect to receive the broadcast ARP on the other logical switch ports if
+# IP address is not configured to the switch patch port.
+local i=`vif_to_ls $inport`
+local j
+for j in 1 2; do
+if test $i$j != $inport; then
+echo $request >> $i$j$k.expected
+fi
+done
 }
 
-# lp11 send GARP request to announce ownership of 192.168.1.100.
+# lp11 send GARP request to announce ownership of 192.168.1.100 and 
fe80::abcd:1.
 
-sha=f011
-spa=`ip_to_hex 192 168 1 100`
-tpa=$spa
+sha="f0:00:00:00:00:11"
+spa="192.168.1.100"
+spa6="fe80::abcd:1"
 
 # When always_learn_from_arp_request=false, the new mac-binding will not be 
learned
 # through GARP request.
 ovn-nbctl 

[ovs-dev] [PATCH ovn v2] tests: Fixed "nested containers" test

[Xavier Simonart]

When a port is removed from a logical switch, the ct-zone is
flushed, then the ct-zone-id is removed from external_ids.
This is done in two steps (ct-zone-id is removed when the transaction
flushing the ct_zone is complete).
ovn-nbctl --wait=hv sync does not take this into account, and hence checking
external_ids right after lsp-del lsp; ovn-nbctl --wait=hv sync might
fail.

Signed-off-by: Xavier Simonart 

v2: handled Mark's comment (i.e added comment in test)
---
 tests/ovn.at | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 6f9fbbfd2..8cc76317d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10238,14 +10238,21 @@ AT_CHECK([test ! -z $foo2_zoneid])
 bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int external_ids:ct-zone-bar2)
 AT_CHECK([test ! -z $bar2_zoneid])
 
-ovn-nbctl lsp-del bar2
+# When a port is removed from a logical switch, the ct-zone is flushed, then
+# the ct-zone-id is removed from external_ids. This is done in two steps(
+# ct-zone-id is removed when the transaction flushing the ct_zone is complete).
+# ovn-nbctl --wait=hv sync does not take this into account, and hence we need
+# two "wait=hv" before we are sure that the ct-zone-id is removed from
+# external_ids.
+ovn-nbctl --wait=hv lsp-del bar2
 ovn-nbctl --wait=hv sync
 
 bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int external_ids:ct-zone-bar2)
 AT_CHECK([test  -z $bar2_zoneid])
 
 # Add back bar2
-ovn-nbctl lsp-add bar bar2 vm2 1 \
+# Same comment as above: two "wait=hv" are needed.
+ovn-nbctl --wait=hv lsp-add bar bar2 vm2 1 \
 -- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3"
 wait_for_ports_up
 ovn-nbctl --wait=hv sync
-- 
2.31.1

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

Re: [ovs-dev] [PATCH ovn] tests: Fixed "nested containers" test

[Xavier Simonart]

Hi Mark

Thanks for the review and sorry for the delay.
Yes, you are right, this could have been easily modified as part of a test
"clean up" ...
I'll go for option 1, so that the test fails in case ovn is modified and a
longer sync time is needed. Hence we'll get notified - it might be expected
(in which case we'll fix the test) ... or not.

Thanks
Xavier

On Mon, May 1, 2023 at 9:05 PM Mark Michelson  wrote:

> Hi Xavier,
>
> See below for my review
>
> On 4/28/23 08:00, Xavier Simonart wrote:
> > When a port is removed from a logical switch, the ct-zone is
> > flushed, then the ct-zone-id is removed from external_ids.
> > This is done in two steps (ct-zone-id is removed when the ransaction
> > flushing the ct_zone is complete).
> > ovn-nbctl --wait=hv sync does not take this into account, and hence
> checking
> > external_ids right after lsp-del lsp; ovn-nbctl --wait=hv sync might
> > fail.
> >
> > Signed-off-by: Xavier Simonart 
> > ---
> >   tests/ovn.at | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 7e804699a..7b8604caf 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -10218,14 +10218,14 @@ AT_CHECK([test ! -z $foo2_zoneid])
> >   bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int
> external_ids:ct-zone-bar2)
> >   AT_CHECK([test ! -z $bar2_zoneid])
> >
> > -ovn-nbctl lsp-del bar2
> > +ovn-nbctl --wait=hv lsp-del bar2
> >   ovn-nbctl --wait=hv sync
>
> I think you should do one of two things here:
>
> 1) Use two different --wait=hv calls, but add a comment explaining why
> they are there. I could see someone trying to be "helpful" and removing
> the sync command, since it looks redundant.
>
> 2) Instead of adding an additional "--wait=hv" to the lsp-del command,
> you could change the below AT_CHECK to be an OVS_WAIT_UNTIL, so that as
> long as it eventually succeeds, the test will pass.
>
> >
> >   bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int
> external_ids:ct-zone-bar2)
> >   AT_CHECK([test  -z $bar2_zoneid])
> >
> >   # Add back bar2
> > -ovn-nbctl lsp-add bar bar2 vm2 1 \
> > +ovn-nbctl --wait=hv lsp-add bar bar2 vm2 1 \
> >   -- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3"
> >   wait_for_ports_up
> >   ovn-nbctl --wait=hv sync
>
> My comment above applies here, too.
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] controller: don't report UDP as unsupported proto in svc_check

[Dumitru Ceara]

On 5/24/23 13:03, Ales Musil wrote:
> On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov 
> wrote:
> 
>> Depending on the udp service, it can reply with some udp data.
>> In that case ovn-controller will warn with next message:
>>
>> pinctrl(ovn_pinctrl0)|WARN|handle service check: Unsupported protocol -
>> [11]
>>
>> With this patch ovn-controller ignores UDP packets, which came to
>> pinctrl_handle_svc_check().  This is not something abnormal, so don't
>> write warnings.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1913162
>> Signed-off-by: Vladislav Odintsov 
>> ---
>>  controller/pinctrl.c | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index b4be22020..f2e753cdb 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -,6 +,13 @@ pinctrl_handle_svc_check(struct rconn *swconn,
>> const struct flow *ip_flow,
>>  ip_proto = in_ip->ip6_nxt;
>>  }
>>
>> +if (ip_proto == IPPROTO_UDP) {
>> +/* We should do nothing if we got UDP packet.
>> + * If service is running, it can respond with any UDP data,
>> + * so just ingore it. */
>> + return;
>> +}
>> +
>>  if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_ICMP &&
>>  ip_proto != IPPROTO_ICMPV6) {
>>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>> --
>> 2.36.1
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Looks good to me, thanks.
> 
> Acked-by: Ales Musil 
> 

Hi, Vladislav, Ales,

I think it might be better to just restrict the logical flow condition
to the type of traffic we actually want to handle in slow path (in
pinctrl).

That is, this flow (and the others that match on ethernet destination
being equal to $svc_monitor_mac):

ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 110,
  "eth.dst == $svc_monitor_mac",
  "handle_svc_check(inport);");

should probably be changed to explicitly match on the supported protocol
list, e.g.:

ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 110,
  "eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)",
  "handle_svc_check(inport);");

Thanks,
Dumitru

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

Re: [ovs-dev] [PATCH ovn 5/5] controller, northd: pass arp/nd from HW VTEP to lrouter pipeline

[Dumitru Ceara]

On 5/19/23 20:18, Vladislav Odintsov wrote:
> This patch is intended to make next two changes:
> 
> 1. Support create/update of MAC_Binding for GARP/ND from HW VTEP.
> 
> Prior to this patch MAC_Binding records were created only when LRP issued
> ARP request/Neighbor solicitation.  If IP to MAC in network attached via
> vtep lport was changed the old MAC_Binding prevented normal
> communication.  Now router port (chassisredirect) in logical switch with
> vtep lport catches GARP or Neighbor Solicitation and updates MAC_Binding.
> 
> New flow with max priority was added in ls_in_arp_nd_resp and additional
> OF rule in table 37 (OFTABLE_REMOTE_OUTPUT) to process multicast packets
> received from RAMP_SWITCH.
> 
> 2. Answer to ARP/ND on requests coming from HW VTEP in lrouter pipeline.
> 
> This is needed to reduce duplicated arp-responses, which were generated
> by OVN arp-responder flows in node, where chassisredirect port located
> with responses coming directly from VM interfaces.
> 
> As a result of this change, now vifs located on same chassis, where
> chassisredirect ports are located receive ARP/ND mcast packets, which
> previously were suppressed by arp-responder on the node.
> 
> Tests and documentation were updated to conform to new behaviour.
> 
> Signed-off-by: Vladislav Odintsov 
> ---

Hi Vladislav,

Thanks for the patch.

>  controller/binding.c| 48 
>  controller/local_data.h |  3 ++
>  controller/physical.c   | 46 +--
>  northd/northd.c | 10 +
>  northd/ovn-northd.8.xml |  6 +++
>  tests/ovn.at| 99 -

Could you please add a NEWS entry for this behavior change?

>  6 files changed, 207 insertions(+), 5 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index c7a2b3f10..5932ad785 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -509,6 +509,30 @@ update_ld_multichassis_ports(const struct 
> sbrec_port_binding *binding_rec,
>  }
>  }
>  
> +static void
> +update_ld_vtep_port(const struct sbrec_port_binding *binding_rec,
> +struct hmap *local_datapaths)
> +{
> +struct local_datapath *ld
> += get_local_datapath(local_datapaths,
> + binding_rec->datapath->tunnel_key);
> +if (!ld) {
> +return;
> +}
> +
> +if (ld->vtep_port && strcmp(ld->vtep_port->logical_port,
> +binding_rec->logical_port)) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +VLOG_WARN_RL(, "vtep port '%s' already set for datapath "
> + "'%"PRId64"', skipping the new port '%s'.",
> + ld->vtep_port->logical_port,
> + binding_rec->datapath->tunnel_key,
> + binding_rec->logical_port);
> +return;
> +}
> +ld->vtep_port = binding_rec;
> +}
> +
>  static void
>  update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
>  struct shash *bridge_mappings,
> @@ -1987,6 +2011,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
> binding_ctx_out *b_ctx_out)
>  struct ovs_list external_lports = OVS_LIST_INITIALIZER(_lports);
>  struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER(
>  _ports);
> +struct ovs_list vtep_lports = OVS_LIST_INITIALIZER(_lports);
>  
>  struct lport {
>  struct ovs_list list_node;
> @@ -2010,8 +2035,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
> binding_ctx_out *b_ctx_out)
>  
>  switch (lport_type) {
>  case LP_PATCH:
> +update_related_lport(pb, b_ctx_out);
> +break;
>  case LP_VTEP:
>  update_related_lport(pb, b_ctx_out);
> +struct lport *vtep_lport = xmalloc(sizeof *vtep_lport);
> +vtep_lport->pb = pb;
> +ovs_list_push_back(_lports, _lport->list_node);
>  break;
>  
>  case LP_LOCALPORT:
> @@ -2111,6 +2141,16 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct 
> binding_ctx_out *b_ctx_out)
>  free(multichassis_lport);
>  }
>  
> +/* Run through vtep lport list to see if there are vtep ports
> + * on local datapaths discovered from above loop, and update the
> + * corresponding local datapath accordingly. */
> +struct lport *vtep_lport;
> +ovs_list_size(_lports);

I guess this is a leftover.

> +LIST_FOR_EACH_POP (vtep_lport, list_node, _lports) {
> +update_ld_vtep_port(vtep_lport->pb, b_ctx_out->local_datapaths);
> +free(vtep_lport);
> +}
> +
>  shash_destroy(_mappings);
>  /* Remove stale QoS entries. */
>  ovs_qos_entries_gc(b_ctx_in->ovs_idl_txn, b_ctx_in->ovsrec_port_by_qos,
> @@ -2175,6 +2215,11 @@ remove_pb_from_local_datapath(const struct 
> sbrec_port_binding *pb,
>  }
>  } else if 

[ovs-dev] [PATCH ovn] tests: fix flaky Multiple OVS interfaces bound to same logical porta

[Xavier Simonart]

Fixes: 52cb9c3b41c2 ("ovn-controller: Fixed missing flows after interface 
deletion")
Signed-off-by: Xavier Simonart 
---
 tests/ovn.at | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/ovn.at b/tests/ovn.at
index 6f9fbbfd2..b2ac2e068 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -34402,6 +34402,7 @@ m4_define([MULTIPLE_OVS_INT],
ovs-ofctl dump-flows br-int | grep $cookie |
sed -e 's/duration=[[0-9.]]*s, //g' |
sed -e 's/idle_age=[[0-9]]*, //g' |
+   sed -e 's/hard_age=[[0-9]]*, //g' |
sed -e 's/n_packets=[[0-9]]*, //g' |
sed -e 's/n_bytes=[[0-9]]*, //g'
}
-- 
2.31.1

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

Re: [ovs-dev] [PATCH v3 3/7] odp-util: Extract vxlan gbp option encoding to a function

[Gavin Li via dev]

On 5/26/2023 5:10 PM, Eelco Chaudron wrote:

External email: Use caution opening links or attachments


On 15 May 2023, at 10:23, Roi Dayan wrote:


From: Gavin Li 

Extract vxlan gbp option encoding to odp_encode_gbp_raw to be used in
following commits.

Signed-off-by: Gavin Li 
Reviewed-by: Roi Dayan 
Reviewed-by: Simon Horman 
---
  lib/odp-util.c | 2 +-
  lib/odp-util.h | 5 +
  2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index f62dc86c5f9e..bf34c61fec58 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3281,7 +3281,7 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl 
*tun_key,

  vxlan_opts_ofs = nl_msg_start_nested(a, 
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS);
  nl_msg_put_u32(a, OVS_VXLAN_EXT_GBP,
-   (tun_key->gbp_flags << 16) | ntohs(tun_key->gbp_id));
+   odp_encode_gbp_raw(tun_key->gbp_flags, 
tun_key->gbp_id));

One small nit, this is over 79 chars.

ACK



  nl_msg_end_nested(a, vxlan_opts_ofs);
  }

diff --git a/lib/odp-util.h b/lib/odp-util.h
index cf762bdc3547..163efe7a87b5 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -382,6 +382,11 @@ static inline void odp_decode_gbp_raw(uint32_t gbp_raw,
  *flags = (gbp_raw >> 16) & 0xFF;
  }

+static inline uint32_t odp_encode_gbp_raw(uint8_t flags, ovs_be16 id)
+{
+return (flags << 16) | ntohs(id);
+}
+
  struct attr_len_tbl {
  int len;
  const struct attr_len_tbl *next;
--
2.38.0

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

Re: [ovs-dev] [PATCH v5] backtrace: Extend the backtrace functionality

[Ales Musil]

On Mon, May 29, 2023 at 7:15 PM Ilya Maximets  wrote:

> On 5/25/23 08:33, Ales Musil wrote:
> > Use the backtrace functions that is provided by libc,
> > this allows us to get backtrace that is independent of
> > the current memory map of the process. Which in turn can
> > be used for debugging/tracing purpose. The backtrace
> > is not 100% accurate due to various optimizations, most
> > notably "-fomit-frame-pointer" and LTO. This might result
> > that the line in source file doesn't correspond to the
> > real line. However, it should be able to pinpoint at least
> > the function where the backtrace was called.
> >
> > The usage for SIGSEGV is determined during compilation
> > based on available libraries. Libunwind has higher priority
> > if both methods are available to keep the compatibility with
> > current behavior.
> >
> > The backtrace is not marked as signal safe however the backtrace
> > manual page gives more detailed explanation why it might be the
> > case [0]. Load the "libgcc" or equivalent in advance within the
> > "fatal_signal_init" which should ensure that subsequent calls
> > to backtrace* do not call malloc and are signal safe.
> >
> > The typical backtrace will look similar to the one below:
> > /lib64/libopenvswitch-3.1.so.0(backtrace_capture+0x1e) [0x7fc5db298dfe]
> > /lib64/libopenvswitch-3.1.so.0(log_backtrace_at+0x57) [0x7fc5db2999e7]
> > /lib64/libovsdb-3.1.so.0(ovsdb_txn_complete+0x7b) [0x7fc5db56247b]
> > /lib64/libovsdb-3.1.so.0(ovsdb_txn_propose_commit_block+0x8d)
> [0x7fc5db563a8d]
> > ovsdb-server(+0xa661) [0x562cfce2e661]
> > ovsdb-server(+0x7e39) [0x562cfce2be39]
> > /lib64/libc.so.6(+0x27b4a) [0x7fc5db048b4a]
> > /lib64/libc.so.6(__libc_start_main+0x8b) [0x7fc5db048c0b]
> > ovsdb-server(+0x8c35) [0x562cfce2cc35]
> >
> > backtrace.h elaborates on how to effectively get the line
> > information associated with the addressed presented in the
> > backtrace.
> >
> > [0]
> > backtrace() and backtrace_symbols_fd() don't call malloc()
> > explicitly, but they are part of libgcc, which gets loaded
> > dynamically when first used.  Dynamic loading usually triggers
> > a call to malloc(3).  If you need certain calls to these two
> > functions to not allocate memory (in signal handlers, for
> > example), you need to make sure libgcc is loaded beforehand
> >
> > Reported-at: https://bugzilla.redhat.com/2177760
> > Signed-off-by: Ales Musil 
> > ---
> > v2: Extend the current use of libunwind rather than replacing it.
> > v3: Allow vlog_fd to be also 0.
> > Return the backtrace log from monitor in updated form.
> > Return use of the "vlog_direct_write_to_log_file_unsafe".
> > v4: Rebase on top of current master.
> > Address comments from Ilya:
> > Address the coding style issues.
> > Make sure that the backrace received by monitor doesn't
> > have more than maximum allowed frames.
> > Change the backtrace_format to accept delimiter.
> > Remove unnecessary checks in the tests.
> > v5: Rebase on top of current master.
> > Address missed comment from v3:
> > Reaname the vlog_fd to vlog_get_log_file_fd_unsafe to reflect that
> > this is really unsafe.
> > Return the ovsdb backtrace to correct position in log and
> > keep the previous format.
> > ---
> >  include/openvswitch/vlog.h |   3 +
> >  lib/backtrace.c| 116 +
> >  lib/backtrace.h|  57 +++---
> >  lib/fatal-signal.c |  52 +++--
> >  lib/ovsdb-error.c  |  12 +---
> >  lib/vlog.c |   7 +++
> >  m4/openvswitch.m4  |   9 ++-
> >  tests/atlocal.in   |   2 +
> >  tests/daemon.at|  45 ++
> >  9 files changed, 227 insertions(+), 76 deletions(-)
> >
> > diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
> > index e53ce6d81..c9202a23e 100644
> > --- a/include/openvswitch/vlog.h
> > +++ b/include/openvswitch/vlog.h
> > @@ -148,6 +148,9 @@ void vlog_set_syslog_target(const char *target);
> >  /* Write directly to log file. */
> >  void vlog_direct_write_to_log_file_unsafe(const char *s);
> >
> > +/* Return the current vlog file descriptor. */
>
> s/vlog/log/
>
> > +int  vlog_get_log_file_fd_unsafe(void);
>
> One too many spaces.
>

Done.


> > +
> >  /* Initialization. */
> >  void vlog_init(void);
> >  void vlog_enable_async(void);
> > diff --git a/lib/backtrace.c b/lib/backtrace.c
> > index 2853d5ff1..4cac5e14c 100644
> > --- a/lib/backtrace.c
> > +++ b/lib/backtrace.c
> > @@ -32,12 +32,27 @@ VLOG_DEFINE_THIS_MODULE(backtrace);
> >  void
> >  backtrace_capture(struct backtrace *b)
> >  {
> > -void *frames[BACKTRACE_MAX_FRAMES];
> > -int i;
> > +b->n_frames = backtrace(b->frames, BACKTRACE_MAX_FRAMES);
> > +}
> > +
> > +void
> > +backtrace_format(const struct backtrace *bt, struct ds *ds,
> > + const char *delimiter)
>
> Haivng the output argument in the middle of the list is strange.
> 

[ovs-dev] [PATCH v6] backtrace: Extend the backtrace functionality

[Ales Musil]

Use the backtrace functions that is provided by libc,
this allows us to get backtrace that is independent of
the current memory map of the process. Which in turn can
be used for debugging/tracing purpose. The backtrace
is not 100% accurate due to various optimizations, most
notably "-fomit-frame-pointer" and LTO. This might result
that the line in source file doesn't correspond to the
real line. However, it should be able to pinpoint at least
the function where the backtrace was called.

The usage for SIGSEGV is determined during compilation
based on available libraries. Libunwind has higher priority
if both methods are available to keep the compatibility with
current behavior.

The backtrace is not marked as signal safe however the backtrace
manual page gives more detailed explanation why it might be the
case [0]. Load the "libgcc" or equivalent in advance within the
"fatal_signal_init" which should ensure that subsequent calls
to backtrace* do not call malloc and are signal safe.

The typical backtrace will look similar to the one below:
/lib64/libopenvswitch-3.1.so.0(backtrace_capture+0x1e) [0x7fc5db298dfe]
/lib64/libopenvswitch-3.1.so.0(log_backtrace_at+0x57) [0x7fc5db2999e7]
/lib64/libovsdb-3.1.so.0(ovsdb_txn_complete+0x7b) [0x7fc5db56247b]
/lib64/libovsdb-3.1.so.0(ovsdb_txn_propose_commit_block+0x8d) [0x7fc5db563a8d]
ovsdb-server(+0xa661) [0x562cfce2e661]
ovsdb-server(+0x7e39) [0x562cfce2be39]
/lib64/libc.so.6(+0x27b4a) [0x7fc5db048b4a]
/lib64/libc.so.6(__libc_start_main+0x8b) [0x7fc5db048c0b]
ovsdb-server(+0x8c35) [0x562cfce2cc35]

backtrace.h elaborates on how to effectively get the line
information associated with the addressed presented in the
backtrace.

[0]
backtrace() and backtrace_symbols_fd() don't call malloc()
explicitly, but they are part of libgcc, which gets loaded
dynamically when first used.  Dynamic loading usually triggers
a call to malloc(3).  If you need certain calls to these two
functions to not allocate memory (in signal handlers, for
example), you need to make sure libgcc is loaded beforehand

Reported-at: https://bugzilla.redhat.com/2177760
Signed-off-by: Ales Musil 
---
v2: Extend the current use of libunwind rather than replacing it.
v3: Allow vlog_fd to be also 0.
Return the backtrace log from monitor in updated form.
Return use of the "vlog_direct_write_to_log_file_unsafe".
v4: Rebase on top of current master.
Address comments from Ilya:
Address the coding style issues.
Make sure that the backrace received by monitor doesn't
have more than maximum allowed frames.
Change the backtrace_format to accept delimiter.
Remove unnecessary checks in the tests.
v5: Rebase on top of current master.
Address missed comment from v3:
Reaname the vlog_fd to vlog_get_log_file_fd_unsafe to reflect that
this is really unsafe.
Return the ovsdb backtrace to correct position in log and
keep the previous format.
v6: Rebase on top of current master.
Adrress comments from Ilya:
Some missed style comments.
Fix the compilation failure on Windows.
Fix BSD test failure.
---
 include/openvswitch/vlog.h |   3 +
 lib/backtrace.c| 128 ++---
 lib/backtrace.h|  58 ++---
 lib/fatal-signal.c |  53 +--
 lib/ovsdb-error.c  |  12 +---
 lib/vlog.c |   7 ++
 m4/openvswitch.m4  |   9 ++-
 tests/atlocal.in   |   2 +
 tests/daemon.at|  50 +++
 9 files changed, 245 insertions(+), 77 deletions(-)

diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
index e53ce6d81..481e1c0f0 100644
--- a/include/openvswitch/vlog.h
+++ b/include/openvswitch/vlog.h
@@ -148,6 +148,9 @@ void vlog_set_syslog_target(const char *target);
 /* Write directly to log file. */
 void vlog_direct_write_to_log_file_unsafe(const char *s);
 
+/* Return the current log file descriptor. */
+int vlog_get_log_file_fd_unsafe(void);
+
 /* Initialization. */
 void vlog_init(void);
 void vlog_enable_async(void);
diff --git a/lib/backtrace.c b/lib/backtrace.c
index 2853d5ff1..3a61e5591 100644
--- a/lib/backtrace.c
+++ b/lib/backtrace.c
@@ -32,12 +32,27 @@ VLOG_DEFINE_THIS_MODULE(backtrace);
 void
 backtrace_capture(struct backtrace *b)
 {
-void *frames[BACKTRACE_MAX_FRAMES];
-int i;
+b->n_frames = backtrace(b->frames, BACKTRACE_MAX_FRAMES);
+}
+
+void
+backtrace_format(struct ds *ds, const struct backtrace *bt,
+ const char *delimiter)
+{
+if (bt->n_frames) {
+char **symbols = backtrace_symbols(bt->frames, bt->n_frames);
+
+if (!symbols) {
+return;
+}
 
-b->n_frames = backtrace(frames, BACKTRACE_MAX_FRAMES);
-for (i = 0; i < b->n_frames; i++) {
-b->frames[i] = (uintptr_t) frames[i];
+for (int i = 0; i < bt->n_frames - 1; i++) {
+ds_put_format(ds, "%s%s", symbols[i], delimiter);
+}
+
+ds_put_format(ds, "%s", 

Re: [ovs-dev] [PATCH v13] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back

[Eelco Chaudron]

On 26 May 2023, at 22:51, Ilya Maximets wrote:

> On 5/26/23 15:09, Eelco Chaudron wrote:
>>
>>
>> On 26 May 2023, at 14:03, Balazs Nemeth wrote:
>>
>>> The only way that stats->{n_packets,n_bytes} would decrease is due to an
>>> overflow, or if there are bugs in how statistics are handled. In the
>>> past, there were multiple issues that caused a jump backward. A
>>> workaround was in place to set the statistics to 0 in that case. When
>>> this happened while the revalidator was under heavy load, the workaround
>>> had an unintended side effect where should_revalidate returned false
>>> causing the flow to be removed because the metric it calculated was
>>> based on a bogus value. Since many of those bugs have now been
>>> identified and resolved, there is no need to set the statistics to 0. In
>>> addition, the (unlikely) overflow still needs to be handled
>>> appropriately. If an unexpected jump does happen, just log it as a
>>> warning.
>>>
>>> Signed-off-by: Balazs Nemeth 
>>
>> Thanks for making the final change! Here is an example of the log message 
>> for others reviewing:
>>
>> 2023-05-26T12:55:07.590Z|3|ofproto_dpif_upcall(revalidator14)|WARN|Unexpected
>>  jump in packet stats from 0 to 1 when handling ukey 
>> ufid:90c082b0-7aa6-442a-9b86-04d10fe9ede6 
>> recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0)
>> ,ct_mark(0),ct_label(0),eth(src=6e:48:c8:77:d3:8c,dst=33:33:00:00:00:16),eth_type(0x86dd),ipv6(src=::,dst=ff02::16,label=0,proto=58,tclass=0,hlimit=1,frag=no),key32(40
>>  00),icmpv6(type=143,code=0), actions:1,3
>>
>> One nit on a missing “,” compared to the dp flow dump, but I think Ilya can 
>> add this on commit.
>>
>> Acked-by: Eelco Chaudron 
>
> Thanks!  I added the comma and applied the patch.

Are you planning on back porting this, as all other relevant patches where?

Cheers,

Eelco

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