On Wed, May 10, 2023 at 1:11 PM Xavier Simonart <[email protected]> wrote:
> Hi Ales > Hi Xavier, thank you for the review. > > Thanks for the patch. > I have small questions/comments. > Otherwise, it looks good to me. > Thanks > Xavier > > On Wed, May 10, 2023 at 10:04 AM Ales Musil <[email protected]> wrote: > >> The test expected that the packet statistics will be >> immediately reflected after packet inject, however that >> might not be true on slower systems. Use OVS_WAIT_UNTIL >> instead to ensure that the packet really went through. >> Also align the IPv4 and IPv6 variant and fix some checks >> that were suggesting flow statistics check, but checked >> logical flows instead. >> >> Signed-off-by: Ales Musil <[email protected]> >> --- >> tests/ovn.at | 73 ++++++++++++++-------------------------------------- >> 1 file changed, 19 insertions(+), 54 deletions(-) >> >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 31f8d34d8..44ec955b2 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -7809,7 +7809,6 @@ ls3_p1_mac=00:00:00:01:02:05 >> check ovn-nbctl --wait=hv lr-policy-add R1 10 "ip4.src==192.168.1.0/24 >> && ip4.dst==172.16.1.0/24" drop >> >> # Check logical flow >> -ovn-sbctl dump-flows > sbflows >> AT_CHECK([ovn-sbctl dump-flows | grep lr_in_policy | grep "192.168.1.0" >> | wc -l], [0], [dnl >> 1 >> ]) >> @@ -7822,12 +7821,9 @@ packet="inport==\"ls1-lp1\" && >> eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac && >> OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt >> "$packet"]) >> >> # Check if packet hit the drop policy >> -AT_CHECK([ovs-ofctl dump-flows br-int | \ >> +OVS_WAIT_UNTIL([test "1" = "$(ovs-ofctl dump-flows br-int | \ >> grep "nw_src=192.168.1.0/24,nw_dst=172.16.1.0/24 actions=drop" | \ >> - grep "priority=10" | \ >> - grep "n_packets=1" | wc -l], [0], [dnl >> -1 >> -]) >> + grep "priority=10" | grep "n_packets=1" | wc -l)"]) >> >> # Expected to drop the packet. >> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" pbr-hv/vif2-tx.pcap > >> vif2.packets >> @@ -7849,12 +7845,9 @@ packet="inport==\"ls1-lp1\" && >> eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac && >> OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt >> "$packet"]) >> >> # Check if packet hit the allow policy >> -sleep 1 >> -AT_CHECK([ovn-sbctl dump-flows | grep lr_in_policy | \ >> - grep "192.168.1.0" | \ >> - grep "priority=20" | wc -l], [0], [dnl >> -1 >> -]) >> +OVS_WAIT_UNTIL([test "1" = "$(ovs-ofctl dump-flows br-int | \ >> + grep "nw_src=192.168.1.0/24,nw_dst=172.16.1.0/24" | \ >> + grep "priority=20" | grep "n_packets=1" | wc -l)"]) >> >> # Expected packet has TTL decreased by 1 >> expected="eth.src==$ls2_ro_mac && eth.dst==$ls2_p1_mac && >> @@ -7881,18 +7874,10 @@ packet="inport==\"ls1-lp1\" && >> eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac && >> OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt >> "$packet"]) >> sleep 1 >> >> nit: can't we now also remove the sleep ? > Definitely, removed in v2. -echo "southbound flows" >> -ovn-sbctl --ovs dump-flows > sbflows >> -AT_CAPTURE_FILE([sbflows]) >> -echo "ovs flows" >> -ovs-ofctl dump-flows br-int > brflows >> -AT_CAPTURE_FILE([brflows]) >> # Check if packet hit the allow policy >> -AT_CHECK([grep "nw_src=192.168.1.0/24,nw_dst=172.16.1.0/24" brflows | \ >> - grep "priority=30" | \ >> - grep "n_packets=1" | wc -l], [0], [dnl >> -1 >> -]) >> +OVS_WAIT_UNTIL([test "1" = "$(ovs-ofctl dump-flows br-int | \ >> + grep "nw_src=192.168.1.0/24,nw_dst=172.16.1.0/24" | \ >> + grep "priority=30" | grep "n_packets=1" | wc -l)"]) >> echo "packet hit reroute policy" >> >> # Expected packet has TTL decreased by 1 >> @@ -7995,9 +7980,7 @@ ls3_p1_mac=00:00:00:01:02:05 >> check ovn-nbctl --wait=sb lr-policy-add R1 10 "ip6.src==2001::/64 && >> ip6.dst==2002::/64" drop >> >> # Check logical flow >> -ovn-sbctl dump-flows > sbflows >> -AT_CAPTURE_FILE([sbflows]) >> -AT_CHECK([grep lr_in_policy sbflows | grep "2001" | wc -l], [0], [dnl >> +AT_CHECK([ovn-sbctl dump-flows | grep lr_in_policy | grep "2001" | wc >> -l], [0], [dnl >> 1 >> ]) >> >> @@ -8009,12 +7992,9 @@ packet="inport==\"ls1-lp1\" && >> eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac && >> OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt >> "$packet"]) >> > >> > # Check if packet hit the drop policy >> -AT_CHECK([ovs-ofctl dump-flows br-int | \ >> +OVS_WAIT_UNTIL([test "1" = "$(ovs-ofctl dump-flows br-int | \ >> grep "ipv6_src=2001::/64,ipv6_dst=2002::/64 actions=drop" | \ >> - grep "priority=10" | \ >> - grep "n_packets=1" | wc -l], [0], [dnl >> -1 >> -]) >> + grep "priority=10" | grep "n_packets=1" | wc -l)"]) >> > > > # Expected to drop the packet. >> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" pbr-hv/vif2-tx.pcap > >> vif2.packets >> @@ -8024,9 +8004,7 @@ AT_FAIL_IF([test -s vif2.packets]) >> check ovn-nbctl --wait=sb lr-policy-add R1 20 "ip6.src==2001::/64 && >> ip6.dst==2002::/64" allow >> >> # Check logical flow >> -ovn-sbctl dump-flows > sbflows2 >> -AT_CAPTURE_FILE([sbflows2]) >> -AT_CHECK([grep lr_in_policy sbflows2 | grep "2001" | wc -l], [0], [dnl >> +AT_CHECK([ovn-sbctl dump-flows | grep lr_in_policy | grep "2001" | wc >> -l], [0], [dnl > > 2 >> ]) >> >> @@ -8037,13 +8015,9 @@ packet="inport==\"ls1-lp1\" && >> eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac && >> OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt >> "$packet"]) >> >> # Check if packet hit the allow policy >> -ovn-sbctl dump-flows > sbflows3 >> -AT_CAPTURE_FILE([sbflows3]) >> -AT_CHECK([grep lr_in_policy sbflows3 | \ >> - grep "2001" | \ >> - grep "priority=20" | wc -l], [0], [dnl >> -1 >> -]) >> +AT_CHECK([test "1" = "$(ovs-ofctl dump-flows br-int | \ >> + grep "ipv6_src=2001::/64,ipv6_dst=2002::/64" | \ >> + grep "priority=20" | grep "n_packets=1" | wc -l)"]) >> > Why not using OVS_WAIT_UNTIL? Don't we risk the same as above (i.e. > statistics not updated yet) ? > # Expected packet has TTL decreased by 1 >> expected="eth.src==$ls2_ro_mac && eth.dst==$ls2_p1_mac && >> @@ -8057,9 +8031,7 @@ OVN_CHECK_PACKETS([pbr-hv/vif2-tx.pcap], [expected]) >> check ovn-nbctl --wait=sb lr-policy-add R1 30 "ip6.src==2001::/64 && >> ip6.dst==2002::/64" reroute 2003::2 >> >> # Check logical flow >> -ovn-sbctl dump-flows > sbflows4 >> -AT_CAPTURE_FILE([sbflows4]) >> -AT_CHECK([grep lr_in_policy sbflows4 | \ >> +AT_CHECK([ovn-sbctl dump-flows | grep lr_in_policy | \ >> grep "2001" | \ >> grep "priority=30" | wc -l], [0], [dnl >> 1 >> @@ -8070,18 +8042,11 @@ packet="inport==\"ls1-lp1\" && >> eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac && >> ip6 && ip.ttl==64 && ip6.src==$ls1_p1_ip && ip6.dst==$ls2_p1_ip && >> udp && udp.src==53 && udp.dst==4369" >> OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt >> "$packet"]) >> -sleep 1 >> >> -ovn-sbctl dump-flows > sbflows5 >> -ovs-ofctl dump-flows br-int > offlows5 >> -AT_CAPTURE_FILE([sbflows5]) >> -AT_CAPTURE_FILE([offlows5]) >> # Check if packet hit the allow policy >> -AT_CHECK([grep "ipv6_src=2001::/64,ipv6_dst=2002::/64" offlows5 | \ >> - grep "priority=30" | \ >> - grep "n_packets=1" | wc -l], [0], [dnl >> -1 >> -]) >> +AT_CHECK([test "1" = "$(ovs-ofctl dump-flows br-int | \ >> + grep "ipv6_src=2001::/64,ipv6_dst=2002::/64" | \ >> + grep "priority=30" | grep "n_packets=1" | wc -l)"]) >> >> Same comment as above: why not OVS_WAIT_UNTIL ? > Yeah, you are right for both cases, updated in v2. > # Expected packet has TTL decreased by 1 >> expected="eth.src==$ls3_ro_mac && eth.dst==$ls3_p1_mac && >> -- >> 2.40.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
