| Message ID | 20230705155736.1512985-1-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] tests: fixed userspace-system tests not properly cleaned up | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Wed, Jul 5, 2023 at 5:57 PM Xavier Simonart <xsimonar@redhat.com> wrote: > Usually, when cleaning up a system test we execute (for the ovn-vswitchd > process): > - OVS_TRAFFIC_VSWITCHD_STOP (i.e. ovs-appctl -t $1 exit --cleanup, wait up > to 30 seconds > and report failure if not properly cleaned up). > - kill_ovs_vswitchd (i.e. if ovs-vsitwhd still running, > ovs-appctl -t ovs-vswitchd exit --cleanup, wait 1 to 10 seconds and kill > ovs-vswitvchd > if not properly cleaned up). > > If we do not execute OVS_TRAFFIC_VSWITCHD_STOP, wait time is much smaller > which might result in > - clean up not done properly, ovs-vswitchd killed, ovs-netdev not deleted, > and no error reported > - next tests will fail > > Using this patch, we properly execute OVS_TRAFFIC_VSWITCHD_STOP, so the > delay waiting for > ovs-vswitchd to exit is longer, and, if by any chance this delay is still > too short, > the proper test will fail (i.e. the first test failing will be the one > unable to clean). > > Note that, after this patch, some extra tests might fail such as > "load-balancer template IPv4". > This is due to the fact that, instead of killing ovn-controller at exit, > we now properly exit and check the unexpected presence of warnings. In > other words, we might now higlight some existing issues which were > ignoring before. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > v2: - updated based on Ales' feedback: > - some cleanup wrongly done twice (e.g. for DNAT LR hairpin IPv4) > causing test to always fail > - replaced the 'kill ovn-controller' in some tests by proper cleanup > - added a few additional missing OVS_WAIT_AND_EXIT (ovsdb-server) > --- > tests/system-ovn.at | 103 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 99 insertions(+), 4 deletions(-) > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 44a8072d6..8f6a647a0 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -5819,6 +5819,21 @@ > tcp,orig=(src=42.42.42.3,dst=42.42.42.2,sport=<clnt_s_port>,dport=4242),reply=(s > > tcp,orig=(src=42.42.42.3,dst=66.66.66.66,sport=<clnt_s_port>,dport=666),reply=(src=42.42.42.2,dst=42.42.42.3,sport=4242,dport=<clnt_s_port>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) > ]) > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > AT_CLEANUP > ]) > > @@ -5915,6 +5930,21 @@ > tcp,orig=(src=4242::3,dst=4242::2,sport=<clnt_s_port>,dport=4242),reply=(src=424 > > tcp,orig=(src=4242::3,dst=6666::1,sport=<clnt_s_port>,dport=666),reply=(src=4242::2,dst=4242::3,sport=4242,dport=<clnt_s_port>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) > ]) > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > AT_CLEANUP > ]) > > @@ -6716,7 +6746,7 @@ AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 > options:qos_burst=1000000]) > OVS_WAIT_UNTIL([tc class show dev ovs-public | \ > grep -q 'class htb .* prio 0 rate 5Gbit ceil 6Gbit burst > 125000b cburst 124500b']) > > -kill $(pidof ovn-controller) > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > @@ -6867,7 +6897,7 @@ Allowing connections from 1000::a > wait_column "up" nb:bfd status logical_port=rp-public > ovn-nbctl destroy bfd $uuid_v6 > > -kill $(pidof ovn-controller) > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > @@ -7121,7 +7151,7 @@ NS_CHECK_EXEC([vm2], [ping -q -c 3 -i 0.3 -w 2 > 172.18.2.10 | FORMAT_PING], \ > 3 packets transmitted, 3 received, 0% packet loss, time 0ms > ]) > > -kill $(pidof ovn-controller) > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > @@ -7238,6 +7268,21 @@ NS_CHECK_EXEC([vm1], [ping -q -c 3 -i 0.3 -w 2 > 172.18.2.12 | FORMAT_PING], \ > 3 packets transmitted, 3 received, 0% packet loss, time 0ms > ]) > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > AT_CLEANUP > ]) > > @@ -7421,7 +7466,7 @@ OVS_WAIT_UNTIL([ > ]) > kill $(pidof tcpdump) > > -kill $(pidof ovn-controller) > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > @@ -8494,6 +8539,16 @@ > tcp,orig=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),reply= > AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > + > as > OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > /connection dropped.*/d"]) > @@ -8646,6 +8701,16 @@ NS_CHECK_EXEC([ls1p1], [ping6 -q -c 3 -i 0.3 -w 2 > 1711::1 | FORMAT_PING], \ > ]) > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > + > as > OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > /connection dropped.*/d > @@ -9168,6 +9233,21 @@ OVS_WAIT_UNTIL([ > test "${requests}" -ge "6" > ]) > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > AT_CLEANUP > ]) > > @@ -9307,6 +9387,21 @@ OVS_WAIT_UNTIL([ > test "${requests}" -ge "6" > ]) > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > AT_CLEANUP > ]) > > -- > 2.31.1 > > Looks good to me, thanks! Acked-by: Ales Musil <amusil@redhat.com>
On 7/7/23 10:46, Ales Musil wrote: > On Wed, Jul 5, 2023 at 5:57 PM Xavier Simonart <xsimonar@redhat.com> wrote: > >> Usually, when cleaning up a system test we execute (for the ovn-vswitchd >> process): >> - OVS_TRAFFIC_VSWITCHD_STOP (i.e. ovs-appctl -t $1 exit --cleanup, wait up >> to 30 seconds >> and report failure if not properly cleaned up). >> - kill_ovs_vswitchd (i.e. if ovs-vsitwhd still running, >> ovs-appctl -t ovs-vswitchd exit --cleanup, wait 1 to 10 seconds and kill >> ovs-vswitvchd >> if not properly cleaned up). >> >> If we do not execute OVS_TRAFFIC_VSWITCHD_STOP, wait time is much smaller >> which might result in >> - clean up not done properly, ovs-vswitchd killed, ovs-netdev not deleted, >> and no error reported >> - next tests will fail >> >> Using this patch, we properly execute OVS_TRAFFIC_VSWITCHD_STOP, so the >> delay waiting for >> ovs-vswitchd to exit is longer, and, if by any chance this delay is still >> too short, >> the proper test will fail (i.e. the first test failing will be the one >> unable to clean). >> >> Note that, after this patch, some extra tests might fail such as >> "load-balancer template IPv4". >> This is due to the fact that, instead of killing ovn-controller at exit, >> we now properly exit and check the unexpected presence of warnings. In >> other words, we might now higlight some existing issues which were >> ignoring before. Hi Xavier, Ales, Ales shared this incremental privately which adds the "offending" warnings to the list of ignored ones. At the same time we opened a bug to track the reason why the warning was issued in the first place: https://bugzilla.redhat.com/show_bug.cgi?id=2222252 diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index f4ebdafc72..b0101330f5 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -287,6 +287,7 @@ check_logs () { /timeval.*context switches: [[0-9]]* voluntary, [[0-9]]* involuntary/d /ovs_rcu.*blocked [[0-9]]* ms waiting for .* to quiesce/d /Dropped [[0-9]]* log messages/d +/.*Trying to release unknown interface.*/d /|WARN|/p /|ERR|/p /|EMER|/p" ${logs} --- I had another look at this patch and I think we should also add the following change: diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at index 6b6181e8cc..97c6e433bb 100644 --- a/tests/system-common-macros.at +++ b/tests/system-common-macros.at @@ -488,7 +488,7 @@ AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut -c3-16], [0 [] ]) -kill $(pidof ovn-controller) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb OVS_APP_EXIT_AND_WAIT([ovsdb-server]) --- If this looks OK to you can fold both of them in and remove the note from the commit log. Please let me know what you think. Thanks, Dumitru >> >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >> >> --- >> v2: - updated based on Ales' feedback: >> - some cleanup wrongly done twice (e.g. for DNAT LR hairpin IPv4) >> causing test to always fail >> - replaced the 'kill ovn-controller' in some tests by proper cleanup >> - added a few additional missing OVS_WAIT_AND_EXIT (ovsdb-server) >> --- >> tests/system-ovn.at | 103 ++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 99 insertions(+), 4 deletions(-) >> >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index 44a8072d6..8f6a647a0 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -5819,6 +5819,21 @@ >> tcp,orig=(src=42.42.42.3,dst=42.42.42.2,sport=<clnt_s_port>,dport=4242),reply=(s >> >> tcp,orig=(src=42.42.42.3,dst=66.66.66.66,sport=<clnt_s_port>,dport=666),reply=(src=42.42.42.2,dst=42.42.42.3,sport=4242,dport=<clnt_s_port>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) >> ]) >> >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> + >> +as ovn-sb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as ovn-nb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as northd >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) >> + >> +as >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> +/connection dropped.*/d"]) >> + >> AT_CLEANUP >> ]) >> >> @@ -5915,6 +5930,21 @@ >> tcp,orig=(src=4242::3,dst=4242::2,sport=<clnt_s_port>,dport=4242),reply=(src=424 >> >> tcp,orig=(src=4242::3,dst=6666::1,sport=<clnt_s_port>,dport=666),reply=(src=4242::2,dst=4242::3,sport=4242,dport=<clnt_s_port>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) >> ]) >> >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> + >> +as ovn-sb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as ovn-nb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as northd >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) >> + >> +as >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> +/connection dropped.*/d"]) >> + >> AT_CLEANUP >> ]) >> >> @@ -6716,7 +6746,7 @@ AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 >> options:qos_burst=1000000]) >> OVS_WAIT_UNTIL([tc class show dev ovs-public | \ >> grep -q 'class htb .* prio 0 rate 5Gbit ceil 6Gbit burst >> 125000b cburst 124500b']) >> >> -kill $(pidof ovn-controller) >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> >> as ovn-sb >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> @@ -6867,7 +6897,7 @@ Allowing connections from 1000::a >> wait_column "up" nb:bfd status logical_port=rp-public >> ovn-nbctl destroy bfd $uuid_v6 >> >> -kill $(pidof ovn-controller) >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> >> as ovn-sb >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> @@ -7121,7 +7151,7 @@ NS_CHECK_EXEC([vm2], [ping -q -c 3 -i 0.3 -w 2 >> 172.18.2.10 | FORMAT_PING], \ >> 3 packets transmitted, 3 received, 0% packet loss, time 0ms >> ]) >> >> -kill $(pidof ovn-controller) >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> >> as ovn-sb >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> @@ -7238,6 +7268,21 @@ NS_CHECK_EXEC([vm1], [ping -q -c 3 -i 0.3 -w 2 >> 172.18.2.12 | FORMAT_PING], \ >> 3 packets transmitted, 3 received, 0% packet loss, time 0ms >> ]) >> >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> + >> +as ovn-sb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as ovn-nb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as northd >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) >> + >> +as >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> +/connection dropped.*/d"]) >> + >> AT_CLEANUP >> ]) >> >> @@ -7421,7 +7466,7 @@ OVS_WAIT_UNTIL([ >> ]) >> kill $(pidof tcpdump) >> >> -kill $(pidof ovn-controller) >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> >> as ovn-sb >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> @@ -8494,6 +8539,16 @@ >> tcp,orig=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),reply= >> AT_CHECK([ovs-appctl dpctl/flush-conntrack]) >> >> OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> + >> +as ovn-sb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as ovn-nb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as northd >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) >> + >> as >> OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> /connection dropped.*/d"]) >> @@ -8646,6 +8701,16 @@ NS_CHECK_EXEC([ls1p1], [ping6 -q -c 3 -i 0.3 -w 2 >> 1711::1 | FORMAT_PING], \ >> ]) >> >> OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> + >> +as ovn-sb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as ovn-nb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as northd >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) >> + >> as >> OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> /connection dropped.*/d >> @@ -9168,6 +9233,21 @@ OVS_WAIT_UNTIL([ >> test "${requests}" -ge "6" >> ]) >> >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> + >> +as ovn-sb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as ovn-nb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as northd >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) >> + >> +as >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> +/connection dropped.*/d"]) >> + >> AT_CLEANUP >> ]) >> >> @@ -9307,6 +9387,21 @@ OVS_WAIT_UNTIL([ >> test "${requests}" -ge "6" >> ]) >> >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> + >> +as ovn-sb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as ovn-nb >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> + >> +as northd >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) >> + >> +as >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d >> +/connection dropped.*/d"]) >> + >> AT_CLEANUP >> ]) >> >> -- >> 2.31.1 >> >> > Looks good to me, thanks! > > Acked-by: Ales Musil <amusil@redhat.com> >
On Wed, Jul 12, 2023 at 8:20 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 7/7/23 10:46, Ales Musil wrote: > > On Wed, Jul 5, 2023 at 5:57 PM Xavier Simonart <xsimonar@redhat.com> > wrote: > > > >> Usually, when cleaning up a system test we execute (for the ovn-vswitchd > >> process): > >> - OVS_TRAFFIC_VSWITCHD_STOP (i.e. ovs-appctl -t $1 exit --cleanup, wait > up > >> to 30 seconds > >> and report failure if not properly cleaned up). > >> - kill_ovs_vswitchd (i.e. if ovs-vsitwhd still running, > >> ovs-appctl -t ovs-vswitchd exit --cleanup, wait 1 to 10 seconds and > kill > >> ovs-vswitvchd > >> if not properly cleaned up). > >> > >> If we do not execute OVS_TRAFFIC_VSWITCHD_STOP, wait time is much > smaller > >> which might result in > >> - clean up not done properly, ovs-vswitchd killed, ovs-netdev not > deleted, > >> and no error reported > >> - next tests will fail > >> > >> Using this patch, we properly execute OVS_TRAFFIC_VSWITCHD_STOP, so the > >> delay waiting for > >> ovs-vswitchd to exit is longer, and, if by any chance this delay is > still > >> too short, > >> the proper test will fail (i.e. the first test failing will be the one > >> unable to clean). > >> > >> Note that, after this patch, some extra tests might fail such as > >> "load-balancer template IPv4". > >> This is due to the fact that, instead of killing ovn-controller at exit, > >> we now properly exit and check the unexpected presence of warnings. In > >> other words, we might now higlight some existing issues which were > >> ignoring before. > > Hi Xavier, Ales, > > Ales shared this incremental privately which adds the "offending" > warnings to the list of ignored ones. At the same time we opened a bug > to track the reason why the warning was issued in the first place: > > https://bugzilla.redhat.com/show_bug.cgi?id=2222252 > > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index f4ebdafc72..b0101330f5 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -287,6 +287,7 @@ check_logs () { > /timeval.*context switches: [[0-9]]* voluntary, [[0-9]]* involuntary/d > /ovs_rcu.*blocked [[0-9]]* ms waiting for .* to quiesce/d > /Dropped [[0-9]]* log messages/d > +/.*Trying to release unknown interface.*/d > /|WARN|/p > /|ERR|/p > /|EMER|/p" ${logs} > --- > > I had another look at this patch and I think we should also add the > following change: > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 6b6181e8cc..97c6e433bb 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -488,7 +488,7 @@ AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 > ipv6_prefix | cut -c3-16], [0 > [] > ]) > > -kill $(pidof ovn-controller) > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > --- > > If this looks OK to you can fold both of them in and remove the > note from the commit log. > > Please let me know what you think. > > Thanks, > Dumitru > > Hi Ales, Dumitru Both look OK to me. Thanks, Dumitru and Ales Thanks Xavier > >> > >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > >> > >> --- > >> v2: - updated based on Ales' feedback: > >> - some cleanup wrongly done twice (e.g. for DNAT LR hairpin IPv4) > >> causing test to always fail > >> - replaced the 'kill ovn-controller' in some tests by proper > cleanup > >> - added a few additional missing OVS_WAIT_AND_EXIT (ovsdb-server) > >> --- > >> tests/system-ovn.at | 103 ++++++++++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 99 insertions(+), 4 deletions(-) > >> > >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at > >> index 44a8072d6..8f6a647a0 100644 > >> --- a/tests/system-ovn.at > >> +++ b/tests/system-ovn.at > >> @@ -5819,6 +5819,21 @@ > >> > tcp,orig=(src=42.42.42.3,dst=42.42.42.2,sport=<clnt_s_port>,dport=4242),reply=(s > >> > >> > tcp,orig=(src=42.42.42.3,dst=66.66.66.66,sport=<clnt_s_port>,dport=666),reply=(src=42.42.42.2,dst=42.42.42.3,sport=4242,dport=<clnt_s_port>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) > >> ]) > >> > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> + > >> +as ovn-sb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as ovn-nb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as northd > >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > >> + > >> +as > >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >> +/connection dropped.*/d"]) > >> + > >> AT_CLEANUP > >> ]) > >> > >> @@ -5915,6 +5930,21 @@ > >> > tcp,orig=(src=4242::3,dst=4242::2,sport=<clnt_s_port>,dport=4242),reply=(src=424 > >> > >> > tcp,orig=(src=4242::3,dst=6666::1,sport=<clnt_s_port>,dport=666),reply=(src=4242::2,dst=4242::3,sport=4242,dport=<clnt_s_port>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) > >> ]) > >> > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> + > >> +as ovn-sb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as ovn-nb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as northd > >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > >> + > >> +as > >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >> +/connection dropped.*/d"]) > >> + > >> AT_CLEANUP > >> ]) > >> > >> @@ -6716,7 +6746,7 @@ AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 > >> options:qos_burst=1000000]) > >> OVS_WAIT_UNTIL([tc class show dev ovs-public | \ > >> grep -q 'class htb .* prio 0 rate 5Gbit ceil 6Gbit > burst > >> 125000b cburst 124500b']) > >> > >> -kill $(pidof ovn-controller) > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> > >> as ovn-sb > >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> @@ -6867,7 +6897,7 @@ Allowing connections from 1000::a > >> wait_column "up" nb:bfd status logical_port=rp-public > >> ovn-nbctl destroy bfd $uuid_v6 > >> > >> -kill $(pidof ovn-controller) > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> > >> as ovn-sb > >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> @@ -7121,7 +7151,7 @@ NS_CHECK_EXEC([vm2], [ping -q -c 3 -i 0.3 -w 2 > >> 172.18.2.10 | FORMAT_PING], \ > >> 3 packets transmitted, 3 received, 0% packet loss, time 0ms > >> ]) > >> > >> -kill $(pidof ovn-controller) > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> > >> as ovn-sb > >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> @@ -7238,6 +7268,21 @@ NS_CHECK_EXEC([vm1], [ping -q -c 3 -i 0.3 -w 2 > >> 172.18.2.12 | FORMAT_PING], \ > >> 3 packets transmitted, 3 received, 0% packet loss, time 0ms > >> ]) > >> > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> + > >> +as ovn-sb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as ovn-nb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as northd > >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > >> + > >> +as > >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >> +/connection dropped.*/d"]) > >> + > >> AT_CLEANUP > >> ]) > >> > >> @@ -7421,7 +7466,7 @@ OVS_WAIT_UNTIL([ > >> ]) > >> kill $(pidof tcpdump) > >> > >> -kill $(pidof ovn-controller) > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> > >> as ovn-sb > >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> @@ -8494,6 +8539,16 @@ > >> > tcp,orig=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),reply= > >> AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > >> > >> OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> + > >> +as ovn-sb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as ovn-nb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as northd > >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > >> + > >> as > >> OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >> /connection dropped.*/d"]) > >> @@ -8646,6 +8701,16 @@ NS_CHECK_EXEC([ls1p1], [ping6 -q -c 3 -i 0.3 -w 2 > >> 1711::1 | FORMAT_PING], \ > >> ]) > >> > >> OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> + > >> +as ovn-sb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as ovn-nb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as northd > >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > >> + > >> as > >> OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >> /connection dropped.*/d > >> @@ -9168,6 +9233,21 @@ OVS_WAIT_UNTIL([ > >> test "${requests}" -ge "6" > >> ]) > >> > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> + > >> +as ovn-sb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as ovn-nb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as northd > >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > >> + > >> +as > >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >> +/connection dropped.*/d"]) > >> + > >> AT_CLEANUP > >> ]) > >> > >> @@ -9307,6 +9387,21 @@ OVS_WAIT_UNTIL([ > >> test "${requests}" -ge "6" > >> ]) > >> > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> + > >> +as ovn-sb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as ovn-nb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as northd > >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > >> + > >> +as > >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >> +/connection dropped.*/d"]) > >> + > >> AT_CLEANUP > >> ]) > >> > >> -- > >> 2.31.1 > >> > >> > > Looks good to me, thanks! > > > > Acked-by: Ales Musil <amusil@redhat.com> > > > >
On 7/14/23 09:19, Xavier Simonart wrote: > On Wed, Jul 12, 2023 at 8:20 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> On 7/7/23 10:46, Ales Musil wrote: >>> On Wed, Jul 5, 2023 at 5:57 PM Xavier Simonart <xsimonar@redhat.com> >> wrote: >>> >>>> Usually, when cleaning up a system test we execute (for the ovn-vswitchd >>>> process): >>>> - OVS_TRAFFIC_VSWITCHD_STOP (i.e. ovs-appctl -t $1 exit --cleanup, wait >> up >>>> to 30 seconds >>>> and report failure if not properly cleaned up). >>>> - kill_ovs_vswitchd (i.e. if ovs-vsitwhd still running, >>>> ovs-appctl -t ovs-vswitchd exit --cleanup, wait 1 to 10 seconds and >> kill >>>> ovs-vswitvchd >>>> if not properly cleaned up). >>>> >>>> If we do not execute OVS_TRAFFIC_VSWITCHD_STOP, wait time is much >> smaller >>>> which might result in >>>> - clean up not done properly, ovs-vswitchd killed, ovs-netdev not >> deleted, >>>> and no error reported >>>> - next tests will fail >>>> >>>> Using this patch, we properly execute OVS_TRAFFIC_VSWITCHD_STOP, so the >>>> delay waiting for >>>> ovs-vswitchd to exit is longer, and, if by any chance this delay is >> still >>>> too short, >>>> the proper test will fail (i.e. the first test failing will be the one >>>> unable to clean). >>>> >>>> Note that, after this patch, some extra tests might fail such as >>>> "load-balancer template IPv4". >>>> This is due to the fact that, instead of killing ovn-controller at exit, >>>> we now properly exit and check the unexpected presence of warnings. In >>>> other words, we might now higlight some existing issues which were >>>> ignoring before. >> >> Hi Xavier, Ales, >> >> Ales shared this incremental privately which adds the "offending" >> warnings to the list of ignored ones. At the same time we opened a bug >> to track the reason why the warning was issued in the first place: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=2222252 >> >> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >> index f4ebdafc72..b0101330f5 100644 >> --- a/tests/ofproto-macros.at >> +++ b/tests/ofproto-macros.at >> @@ -287,6 +287,7 @@ check_logs () { >> /timeval.*context switches: [[0-9]]* voluntary, [[0-9]]* involuntary/d >> /ovs_rcu.*blocked [[0-9]]* ms waiting for .* to quiesce/d >> /Dropped [[0-9]]* log messages/d >> +/.*Trying to release unknown interface.*/d >> /|WARN|/p >> /|ERR|/p >> /|EMER|/p" ${logs} >> --- >> >> I had another look at this patch and I think we should also add the >> following change: >> >> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at >> index 6b6181e8cc..97c6e433bb 100644 >> --- a/tests/system-common-macros.at >> +++ b/tests/system-common-macros.at >> @@ -488,7 +488,7 @@ AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 >> ipv6_prefix | cut -c3-16], [0 >> [] >> ]) >> >> -kill $(pidof ovn-controller) >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> >> as ovn-sb >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> --- >> >> If this looks OK to you can fold both of them in and remove the >> note from the commit log. >> >> Please let me know what you think. >> >> Thanks, >> Dumitru >> >> Hi Ales, Dumitru > > Both look OK to me. Thanks, Dumitru and Ales > Thanks, Xavier and Ales! I applied the patch to main and 23.06. It didn't apply cleanly to older branches but I don't think that's too big of a problem. Regards, Dumitru
diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 44a8072d6..8f6a647a0 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -5819,6 +5819,21 @@ tcp,orig=(src=42.42.42.3,dst=42.42.42.2,sport=<clnt_s_port>,dport=4242),reply=(s tcp,orig=(src=42.42.42.3,dst=66.66.66.66,sport=<clnt_s_port>,dport=666),reply=(src=42.42.42.2,dst=42.42.42.3,sport=4242,dport=<clnt_s_port>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) ]) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + AT_CLEANUP ]) @@ -5915,6 +5930,21 @@ tcp,orig=(src=4242::3,dst=4242::2,sport=<clnt_s_port>,dport=4242),reply=(src=424 tcp,orig=(src=4242::3,dst=6666::1,sport=<clnt_s_port>,dport=666),reply=(src=4242::2,dst=4242::3,sport=4242,dport=<clnt_s_port>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) ]) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + AT_CLEANUP ]) @@ -6716,7 +6746,7 @@ AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 options:qos_burst=1000000]) OVS_WAIT_UNTIL([tc class show dev ovs-public | \ grep -q 'class htb .* prio 0 rate 5Gbit ceil 6Gbit burst 125000b cburst 124500b']) -kill $(pidof ovn-controller) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb OVS_APP_EXIT_AND_WAIT([ovsdb-server]) @@ -6867,7 +6897,7 @@ Allowing connections from 1000::a wait_column "up" nb:bfd status logical_port=rp-public ovn-nbctl destroy bfd $uuid_v6 -kill $(pidof ovn-controller) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb OVS_APP_EXIT_AND_WAIT([ovsdb-server]) @@ -7121,7 +7151,7 @@ NS_CHECK_EXEC([vm2], [ping -q -c 3 -i 0.3 -w 2 172.18.2.10 | FORMAT_PING], \ 3 packets transmitted, 3 received, 0% packet loss, time 0ms ]) -kill $(pidof ovn-controller) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb OVS_APP_EXIT_AND_WAIT([ovsdb-server]) @@ -7238,6 +7268,21 @@ NS_CHECK_EXEC([vm1], [ping -q -c 3 -i 0.3 -w 2 172.18.2.12 | FORMAT_PING], \ 3 packets transmitted, 3 received, 0% packet loss, time 0ms ]) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + AT_CLEANUP ]) @@ -7421,7 +7466,7 @@ OVS_WAIT_UNTIL([ ]) kill $(pidof tcpdump) -kill $(pidof ovn-controller) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb OVS_APP_EXIT_AND_WAIT([ovsdb-server]) @@ -8494,6 +8539,16 @@ tcp,orig=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),reply= AT_CHECK([ovs-appctl dpctl/flush-conntrack]) OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + as OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d /connection dropped.*/d"]) @@ -8646,6 +8701,16 @@ NS_CHECK_EXEC([ls1p1], [ping6 -q -c 3 -i 0.3 -w 2 1711::1 | FORMAT_PING], \ ]) OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + as OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d /connection dropped.*/d @@ -9168,6 +9233,21 @@ OVS_WAIT_UNTIL([ test "${requests}" -ge "6" ]) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + AT_CLEANUP ]) @@ -9307,6 +9387,21 @@ OVS_WAIT_UNTIL([ test "${requests}" -ge "6" ]) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + AT_CLEANUP ])
Usually, when cleaning up a system test we execute (for the ovn-vswitchd process): - OVS_TRAFFIC_VSWITCHD_STOP (i.e. ovs-appctl -t $1 exit --cleanup, wait up to 30 seconds and report failure if not properly cleaned up). - kill_ovs_vswitchd (i.e. if ovs-vsitwhd still running, ovs-appctl -t ovs-vswitchd exit --cleanup, wait 1 to 10 seconds and kill ovs-vswitvchd if not properly cleaned up). If we do not execute OVS_TRAFFIC_VSWITCHD_STOP, wait time is much smaller which might result in - clean up not done properly, ovs-vswitchd killed, ovs-netdev not deleted, and no error reported - next tests will fail Using this patch, we properly execute OVS_TRAFFIC_VSWITCHD_STOP, so the delay waiting for ovs-vswitchd to exit is longer, and, if by any chance this delay is still too short, the proper test will fail (i.e. the first test failing will be the one unable to clean). Note that, after this patch, some extra tests might fail such as "load-balancer template IPv4". This is due to the fact that, instead of killing ovn-controller at exit, we now properly exit and check the unexpected presence of warnings. In other words, we might now higlight some existing issues which were ignoring before. Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- v2: - updated based on Ales' feedback: - some cleanup wrongly done twice (e.g. for DNAT LR hairpin IPv4) causing test to always fail - replaced the 'kill ovn-controller' in some tests by proper cleanup - added a few additional missing OVS_WAIT_AND_EXIT (ovsdb-server) --- tests/system-ovn.at | 103 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 4 deletions(-)