On Fri, Jul 14, 2023 at 11:39 AM Xavier Simonart <[email protected]> wrote:
> Hi Ales > > Thanks for looking into this. > > On Fri, Jul 14, 2023 at 8:17 AM Ales Musil <[email protected]> wrote: > >> >> >> On Thu, Jul 13, 2023 at 1:08 PM Xavier Simonart <[email protected]> >> wrote: >> >>> Signed-off-by: Xavier Simonart <[email protected]> >>> --- >>> >> >> Hi Xavier, >> >> >>> tests/ovn.at | 22 +++++++++++++++++----- >>> 1 file changed, 17 insertions(+), 5 deletions(-) >>> >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index cd6d4b9ff..4ae33567f 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -17058,7 +17058,7 @@ rtr_l2_ip=$(ip_to_hex 172 16 1 1) >>> l1_ip=$(ip_to_hex 192 168 1 2) >>> >>> check ovn-nbctl mirror-add mirror0 gre 0 to-lport 192.168.1.12 >>> -check ovn-nbctl lsp-attach-mirror ls1-lp1 mirror0 >>> +check ovn-nbctl --wait=hv lsp-attach-mirror ls1-lp1 mirror0 >>> >>> # Send ping packet and check for mirrored packet of the reply >>> test_ipv4_icmp_request 1 000000010203 0000000102f1 $l1_ip $rtr_l2_ip >>> 0000 8510 03ff 8d10 "gre" "to-lport" >>> @@ -17071,7 +17071,9 @@ as hv1 reset_pcap_file br-phys_n1 hv1/br-phys_n1 >>> rm -f br-phys_n1.expected >>> rm -f vif1.expected >>> >>> -check ovn-nbctl set mirror . type=erspan >>> +check ovn-nbctl --wait=hv set mirror . type=erspan >>> +# Wait for port to get updated >>> +OVS_WAIT_UNTIL([test 1 = `ovs-appctl dpif/show | grep ovn-mirror | grep >>> -c erspan`]) >>> >>> # Send ping packet and check for mirrored packet of the reply >>> test_ipv4_icmp_request 1 000000010203 0000000102f1 $l1_ip $rtr_l2_ip >>> 0000 8510 03ff 8d10 "erspan" "to-lport" >>> @@ -17084,7 +17086,15 @@ as hv1 reset_pcap_file br-phys_n1 hv1/br-phys_n1 >>> rm -f br-phys_n1.expected >>> rm -f vif1.expected >>> >>> -check ovn-nbctl set mirror . filter=from-lport >>> +check ovn-nbctl --wait=hv set mirror . filter=from-lport >>> + >>> +# First make sure conf.db got updated >>> +vif1=$(ovs-vsctl get Port vif1 _uuid) >>> +OVS_WAIT_UNTIL([test `ovs-vsctl get mirror mirror0 select_src_port | tr >>> -d "[[]]"` = $vif1]) >>> +# Then make sure ovs-vswitchd got opportunity to run : run some random >>> ovs-apctl command twice, so >>> +# mirror_run could run >>> +ovs-appctl dpif/show >>> +ovs-appctl dpif/show >>> >> >> I'm not sure I understand what is the purpose of this "Then make sure >> ovs-vswitchd got opportunity to run". >> Could you please elaborate a bit? >> >> > My understanding of those race conditions are the following - but I might > be wrong > When we run ovn-nbctl --wait=hv, there is no guarantee that, after > ovn-nbctl returns, conf.db is already updated. > If that ovsdb-server process is very slow (or not scheduled on a busy > system), it might still take some time before the db gets properly updated. > That's the reason for the OVS_WAIT_UNTIL above. > But even after the db gets updated, ovs-vswitchd has to get notified of > that db change and run mirror_configure (sorry the above comment on > mirror_run was wrong) to reconfigure the mirror. > And, again, it might take a while for ovs-vswitchd process to get > scheduled on a busy system. > The only way I found to ensure that ovs-vswitchd had run at least one loop > is to have some appctl command talking to ovs-vswitchd (i.e. ovs-appctl > dpif/show does not show the change, and I did not find any other > appctl/vsctl command to do show it). > I am not sure that "one" ovs-appctl would be enough as it does not > guarantee the run of a full loop in ovs-vswitchd. > This is really hacky, but I do not know any way. > Do you agree with that analysis, and, if yes, do you have any better way > to fix this ? > Ok that is reasonable, unfortunately I don't have a better idea how to achieve that. Would you mind putting this analysis into the commit message so we have it documented? > >>> # Send ping packet and check for mirrored packet of the request >>> test_ipv4_icmp_request 1 000000010203 0000000102f1 $l1_ip $rtr_l2_ip >>> 0000 8510 03ff 8d10 "erspan" "from-lport" >>> @@ -17097,7 +17107,9 @@ as hv1 reset_pcap_file br-phys_n1 hv1/br-phys_n1 >>> rm -f br-phys_n1.expected >>> rm -f vif1.expected >>> >>> -check ovn-nbctl set mirror . type=gre >>> +check ovn-nbctl --wait=hv set mirror . type=gre >>> +# Wait for port to get updated >>> +OVS_WAIT_UNTIL([test 1 = `ovs-appctl dpif/show | grep ovn-mirror | grep >>> -c gre`]) >>> >>> # Send ping packet and check for mirrored packet of the request >>> test_ipv4_icmp_request 1 000000010203 0000000102f1 $l1_ip $rtr_l2_ip >>> 0000 8510 03ff 8d10 "gre" "from-lport" >>> @@ -17243,7 +17255,7 @@ AT_CHECK([cat mirror1.packets | sort], [0], >>> [expout]) >>> AT_CHECK([cat mirror2.packets | sort], [0], [expout]) >>> >>> port_src_old=$(ovs-vsctl get mirror mirror-from-lp1 select_src_port) >>> -check ovn-nbctl set mirror $uuid1 filter=both >>> +check ovn-nbctl --wait=hv set mirror $uuid1 filter=both >>> port_src_new=$(ovs-vsctl get mirror mirror-from-lp1 select_src_port) >>> port_dst_new=$(ovs-vsctl get mirror mirror-from-lp1 select_dst_port) >>> AT_CHECK([test $port_src_old = $port_src_new], [0], []) >>> -- >>> 2.31.1 >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> Thanks, >> Ales >> >> Thanks > Xavier > >> -- >> >> Ales Musil >> >> Senior Software Engineer - OVN Core >> >> Red Hat EMEA <https://www.redhat.com> >> >> [email protected] IM: amusil >> <https://red.ht/sig> >> > 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
