Hi Ales Thanks for looking into this.
On Fri, Jul 14, 2023 at 8:17 AM Ales Musil <amu...@redhat.com> wrote: > > > On Thu, Jul 13, 2023 at 1:08 PM Xavier Simonart <xsimo...@redhat.com> > wrote: > >> Signed-off-by: Xavier Simonart <xsimo...@redhat.com> >> --- >> > > 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 ? > >> # 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 >> d...@openvswitch.org >> 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> > > amu...@redhat.com IM: amusil > <https://red.ht/sig> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
