On Fri, Mar 14, 2025 at 11:35 AM Xavier Simonart <[email protected]> wrote: > > 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?
Hi Xavier, I could not get a chance to look into your comments. I'll definitely incorporate your changes if it makes the test code easier to follow. Thanks Numan > > 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
