Hi Numan
Thanks for the patch.
I am wondering whether we could not reduce the length of the ovn.at test
and make it easier to follow.
As of now, for each hypervisor, we call check_offlows_for_datapath for each
expected local datapath and also check a few datapaths expected to be
non-local .
Then we also check "ovn-appctl -t ovn-controller debug/dump-local-datapaths"
listing all local datapaths.
Could we have something like "check_local_datapth" e.g.
"check_local_datapth gw1 sw0,lr0,public" which would do all expected checks
on gw1:
- check that it contains flows for the listed datapaths
- check that there is no flows for any other datapaths
- check that debug/dump-local-datapaths returns exactly those datapaths.
So something like (without checking the datapath type):
+check_local_datapath() {
+ hv=$1
+ expected_dps=$2
+ OVS_WAIT_UNTIL(
+ [check_expected $hv $expected_dps], [echo "Did not find flows on
$expected_dps on $hv"])
+ OVS_WAIT_UNTIL(
+ [check_unexpected $hv $expected_dps], [echo "Found unexpected flows
on $hv"])
+ found=$(as $hv ovn-appctl -t ovn-controller debug/dump-local-datapaths
| grep -v "Local" | sed 's/.*Datapath:\s*\([[a-z0-9-]]*\).*/\1/')
+ dps=$(echo "$expected_dps" | tr ',' '\n' | sort | tr '\n' ' ')
+ found=$(echo "$found" | sort | tr '\n' ' ')
+ check test "$dps" = "$found"
+}
+
+check_unexpected() {
+ hv=$1
+ dps=$2
+ flows=$(as $hv ovs-ofctl dump-flows br-int)
+
+ # Check for flows from unexpected datapaths
+ dps=$(echo "$dps" | tr ',' ' ')
+ echo "Looking for unexpected flows on $hv"
+ for dp in $dps
+ do
+ if [[ -n "${dp}" ]]; then
+ key=$(printf '0x%x' $(ovn-sbctl --bare --columns tunnel_key list
datapath $dp))
+ if [[ -n "${key}" ]]; then
+ flows=$(echo "$flows" | grep -v "metadata=$key" )
+ fi
+ fi
+ done
+ n_flows=$(echo "$flows" | grep "metadata=" | wc -l)
+ if [[ "${n_flows}" != "0" ]]; then
+ #echo "hv $hv has $n_flows remaining flows"
+ return 1
+ fi
+}
+
+check_expected() {
+ # Check for flows from expected datapaths
+ hv=$1
+ dps=$2
+ dps=$(echo "$dps" | tr ',' ' ')
+ for dp in $dps
+ do
+ if [[ -n "${dp}" ]]; then
+ key=$(printf '0x%x' $(ovn-sbctl --bare --columns tunnel_key list
datapath $dp))
+ if [[ -n "${key}" ]]; then
+ echo "Looking for expected flows in $hv for $dp i.e. tunnel_key
$key"
+ n_flows=$(as $hv ovs-ofctl dump-flows br-int | grep -c
"metadata=$key")
+ if [[ "${n_flows}" -eq "0" ]]; then
+ #echo "hv $hv has no flows on dp $dp"
+ return 1
+ fi
+ fi
+ fi
+ done
+}
+
Then we can replace all occurrences of check_offlows_for_datapath and
dump-local-datapaths by just four lines (one for each hv) on each nb/sb
change.
WDYT?
Thanks
Xavier
On Thu, Mar 13, 2025 at 5:36 PM Numan Siddique <[email protected]> wrote:
> On Thu, Mar 13, 2025 at 12:21 PM Han Zhou <[email protected]> wrote:
> >
> >
> >
> > On Sat, Mar 1, 2025 at 6:38 AM <[email protected]> wrote:
> >>
> >> From: Numan Siddique <[email protected]>
> >>
> >> This patch series optimizes ovn-controller to add only relevant
> >> datapaths to its "local datapaths" map if the logical topology
> >> has a single flat provider logical switch connected to multiple
> >> routers with a distributed gateway port.
> >>
> >> (Patch 3 has a detailed commit message.)
> >>
> >> v1 -> v2
> >> -----
> >> * Rebased to resolve the conflicts.
> >>
> >> Numan Siddique (3):
> >> controller: Store local binding lports in local_datapath.
> >> northd: Add a flag 'only_dgp_peer_ports' to the SB Datapath.
> >> controller: Optimize adding 'dps' to the local datapaths.
> >>
> >> controller/binding.c | 324 +++++++++++---
> >> controller/binding.h | 2 +
> >> controller/local_data.c | 98 ++++-
> >> controller/local_data.h | 10 +-
> >> controller/lport.c | 12 +
> >> controller/lport.h | 4 +
> >> controller/ovn-controller.c | 38 ++
> >> northd/northd.c | 31 +-
> >> northd/northd.h | 4 +
> >> tests/multinode.at | 185 +++++++-
> >> tests/ovn-northd.at | 59 +++
> >> tests/ovn-performance.at | 6 +-
> >> tests/ovn.at | 853 ++++++++++++++++++++++++++++++++++++
> >> 13 files changed, 1545 insertions(+), 81 deletions(-)
> >>
> >> --
> >> 2.48.1
> >>
> >> _______________________________________________
> >> dev mailing list
> >> [email protected]
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> > Hi Numan,
> >
> > Sorry for the slow review. I have a general comment for this series. I
> believe the exact same performance optimization is supposed to be achieved
> by the below commit:
> >
> > 22298fd37 ovn-controller: Don't flood fill local datapaths beyond DGP
> boundary.
> >
> > The scenarios are mentioned in detail in the commit message of that
> patch, which is very similar to what you described here.
> >
> > The only exception was when there are distributed NAT or when the
> redirection type is set to "bridged". In these cases it will fall back to
> add peer datapath as local. See SB:Port_Binding:options:always-redirect. Is
> this something that your patch is targeting? Otherwise I wonder why it
> didn't work without your patch.
>
> Hi Han,
>
> I wanted to mention in the commit message and in the cover letter that
> this patch series is an extension of the commit 22298fd37. I somehow
> missed it.
>
> Yes, I'm targeting the scenario of distributed NAT,
>
> Numan
>
>
> > Could you help clarify?
> >
> > Thanks,
> > Han
> >
> >>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
