| Message ID | 20240808130312.1666065-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] test: Fix flaky I-P test. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Thanks Ales. Acked-by: Mark Michelson <mmichels@redhat.com> On 8/8/24 09:03, Ales Musil wrote: > The test was checking if there was any recompute after all operations. > This proven to be flaky because there might have been "random" > recompute happening in the middle due to the following events: > > 1) SB becomes read only > 2) non_vif_data handler for OVS_interface fails > 3) Recompute of non_vif_data is not allowed because SB is read only > 4) non_vif_data is marked as cancelled > 5) Next engine run will do force recompute of all nodes > > To prevent that check specifically for the handler that is supposed > to gracefully handle the interface changes. > > Fixes: 603f38da062b ("controller: Remove OvS iface type check in I-P processing.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Address comment from Ilya: > - Add recompute case to the test so if the debug format changes the test fails. > - Add missing checks for ovs-vsctl. > --- > tests/ovn-controller.at | 43 +++++++++++++++++++++++++---------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 898981867..dcab5f2e9 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -3308,51 +3308,60 @@ net_add n1 > sim_add hv1 > ovs-vsctl add-br br-phys > ovn_attach n1 br-phys 192.168.0.20 > +ovn-appctl vlog/set inc_proc_eng:dbg > > check ovn-nbctl ls-add ls0 > check ovn-nbctl lsp-add ls0 vif > > -ovn-appctl inc-engine/clear-stats > +m4_define([HANDLER_MESSAGE], [runtime_data, recompute (failed handler for input ovs_interface_shadow)]) > > -ovs-vsctl -- add-port br-int vif -- \ > +check ovs-vsctl -- add-port br-int vif -- \ > set Interface vif external-ids:iface-id=vif > wait_row_count Port_Binding 1 logical_port="vif" up=true > +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) > > -ovs-vsctl del-port br-int vif > +check ovs-vsctl del-port br-int vif > wait_row_count Port_Binding 1 logical_port="vif" up=false > +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) > > -ovs-vsctl add-port br-int vif -- \ > +check ovs-vsctl add-port br-int vif -- \ > set Interface vif type=dummy -- \ > set Interface vif external-ids:iface-id=vif > wait_row_count Port_Binding 1 logical_port="vif" up=true > +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) > > -ovs-vsctl del-port br-int vif > +check ovs-vsctl del-port br-int vif > wait_row_count Port_Binding 1 logical_port="vif" up=false > +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) > > -ovs-vsctl add-port br-int vif -- \ > +check ovs-vsctl add-port br-int vif -- \ > set Interface vif type=geneve -- \ > set Interface vif options:remote_ip=1.1.1.1 external-ids:iface-id=vif > wait_row_count Port_Binding 1 logical_port="vif" up=true > +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) > > -ovs-vsctl del-port br-int vif > +check ovs-vsctl del-port br-int vif > wait_row_count Port_Binding 1 logical_port="vif" up=false > +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) > > # Make sure that setting iface in two different transaction doesn't > # cause recompute. > -ovs-vsctl add-port br-int vif > -ovs-vsctl set Interface vif external-ids:iface-id=vif > +check ovs-vsctl add-port br-int vif > +check ovs-vsctl set Interface vif external-ids:iface-id=vif > wait_row_count Port_Binding 1 logical_port="vif" up=true > +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) > > -ovs-vsctl del-port br-int vif > +check ovs-vsctl del-port br-int vif > wait_row_count Port_Binding 1 logical_port="vif" up=false > +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) > > -AT_CHECK([ovn-appctl inc-engine/show-stats runtime_data |\ > - sed "s/- compute:\s\+[[0-9]]\+/- compute: ??/"], [0], [dnl > -Node: runtime_data > -- recompute: 0 > -- compute: ?? > -- cancel: 0 > -]) > +# Make sure that setting dummy iface in two different transactions > +# causes recompute. > +check ovs-vsctl add-port br-int vif -- \ > + set Interface vif type=dummy > +check ovs-vsctl set Interface vif external-ids:iface-id=vif > +wait_row_count Port_Binding 1 logical_port="vif" up=true > +AT_CHECK([test $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log) -ge 1]) > > OVN_CLEANUP([hv1]) > AT_CLEANUP
On 8/8/24 15:03, Ales Musil wrote: > The test was checking if there was any recompute after all operations. > This proven to be flaky because there might have been "random" > recompute happening in the middle due to the following events: > > 1) SB becomes read only > 2) non_vif_data handler for OVS_interface fails > 3) Recompute of non_vif_data is not allowed because SB is read only > 4) non_vif_data is marked as cancelled > 5) Next engine run will do force recompute of all nodes > > To prevent that check specifically for the handler that is supposed > to gracefully handle the interface changes. > > Fixes: 603f38da062b ("controller: Remove OvS iface type check in I-P processing.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Address comment from Ilya: > - Add recompute case to the test so if the debug format changes the test fails. > - Add missing checks for ovs-vsctl. > --- > tests/ovn-controller.at | 43 +++++++++++++++++++++++++---------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > Seems reasonable. Acked-by: Ilya Maximets <i.maximets@ovn.org>
I merged this to main. On 8/9/24 11:39, Mark Michelson wrote: > Thanks Ales. > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 8/8/24 09:03, Ales Musil wrote: >> The test was checking if there was any recompute after all operations. >> This proven to be flaky because there might have been "random" >> recompute happening in the middle due to the following events: >> >> 1) SB becomes read only >> 2) non_vif_data handler for OVS_interface fails >> 3) Recompute of non_vif_data is not allowed because SB is read only >> 4) non_vif_data is marked as cancelled >> 5) Next engine run will do force recompute of all nodes >> >> To prevent that check specifically for the handler that is supposed >> to gracefully handle the interface changes. >> >> Fixes: 603f38da062b ("controller: Remove OvS iface type check in I-P >> processing.") >> Signed-off-by: Ales Musil <amusil@redhat.com> >> --- >> v2: Address comment from Ilya: >> - Add recompute case to the test so if the debug format changes >> the test fails. >> - Add missing checks for ovs-vsctl. >> --- >> tests/ovn-controller.at | 43 +++++++++++++++++++++++++---------------- >> 1 file changed, 26 insertions(+), 17 deletions(-) >> >> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >> index 898981867..dcab5f2e9 100644 >> --- a/tests/ovn-controller.at >> +++ b/tests/ovn-controller.at >> @@ -3308,51 +3308,60 @@ net_add n1 >> sim_add hv1 >> ovs-vsctl add-br br-phys >> ovn_attach n1 br-phys 192.168.0.20 >> +ovn-appctl vlog/set inc_proc_eng:dbg >> check ovn-nbctl ls-add ls0 >> check ovn-nbctl lsp-add ls0 vif >> -ovn-appctl inc-engine/clear-stats >> +m4_define([HANDLER_MESSAGE], [runtime_data, recompute (failed handler >> for input ovs_interface_shadow)]) >> -ovs-vsctl -- add-port br-int vif -- \ >> +check ovs-vsctl -- add-port br-int vif -- \ >> set Interface vif external-ids:iface-id=vif >> wait_row_count Port_Binding 1 logical_port="vif" up=true >> +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) >> -ovs-vsctl del-port br-int vif >> +check ovs-vsctl del-port br-int vif >> wait_row_count Port_Binding 1 logical_port="vif" up=false >> +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) >> -ovs-vsctl add-port br-int vif -- \ >> +check ovs-vsctl add-port br-int vif -- \ >> set Interface vif type=dummy -- \ >> set Interface vif external-ids:iface-id=vif >> wait_row_count Port_Binding 1 logical_port="vif" up=true >> +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) >> -ovs-vsctl del-port br-int vif >> +check ovs-vsctl del-port br-int vif >> wait_row_count Port_Binding 1 logical_port="vif" up=false >> +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) >> -ovs-vsctl add-port br-int vif -- \ >> +check ovs-vsctl add-port br-int vif -- \ >> set Interface vif type=geneve -- \ >> set Interface vif options:remote_ip=1.1.1.1 >> external-ids:iface-id=vif >> wait_row_count Port_Binding 1 logical_port="vif" up=true >> +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) >> -ovs-vsctl del-port br-int vif >> +check ovs-vsctl del-port br-int vif >> wait_row_count Port_Binding 1 logical_port="vif" up=false >> +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) >> # Make sure that setting iface in two different transaction doesn't >> # cause recompute. >> -ovs-vsctl add-port br-int vif >> -ovs-vsctl set Interface vif external-ids:iface-id=vif >> +check ovs-vsctl add-port br-int vif >> +check ovs-vsctl set Interface vif external-ids:iface-id=vif >> wait_row_count Port_Binding 1 logical_port="vif" up=true >> +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) >> -ovs-vsctl del-port br-int vif >> +check ovs-vsctl del-port br-int vif >> wait_row_count Port_Binding 1 logical_port="vif" up=false >> +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) >> -AT_CHECK([ovn-appctl inc-engine/show-stats runtime_data |\ >> - sed "s/- compute:\s\+[[0-9]]\+/- compute: ??/"], [0], [dnl >> -Node: runtime_data >> -- recompute: 0 >> -- compute: ?? >> -- cancel: 0 >> -]) >> +# Make sure that setting dummy iface in two different transactions >> +# causes recompute. >> +check ovs-vsctl add-port br-int vif -- \ >> + set Interface vif type=dummy >> +check ovs-vsctl set Interface vif external-ids:iface-id=vif >> +wait_row_count Port_Binding 1 logical_port="vif" up=true >> +AT_CHECK([test $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log) >> -ge 1]) >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 898981867..dcab5f2e9 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -3308,51 +3308,60 @@ net_add n1 sim_add hv1 ovs-vsctl add-br br-phys ovn_attach n1 br-phys 192.168.0.20 +ovn-appctl vlog/set inc_proc_eng:dbg check ovn-nbctl ls-add ls0 check ovn-nbctl lsp-add ls0 vif -ovn-appctl inc-engine/clear-stats +m4_define([HANDLER_MESSAGE], [runtime_data, recompute (failed handler for input ovs_interface_shadow)]) -ovs-vsctl -- add-port br-int vif -- \ +check ovs-vsctl -- add-port br-int vif -- \ set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl add-port br-int vif -- \ +check ovs-vsctl add-port br-int vif -- \ set Interface vif type=dummy -- \ set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl add-port br-int vif -- \ +check ovs-vsctl add-port br-int vif -- \ set Interface vif type=geneve -- \ set Interface vif options:remote_ip=1.1.1.1 external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) # Make sure that setting iface in two different transaction doesn't # cause recompute. -ovs-vsctl add-port br-int vif -ovs-vsctl set Interface vif external-ids:iface-id=vif +check ovs-vsctl add-port br-int vif +check ovs-vsctl set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -AT_CHECK([ovn-appctl inc-engine/show-stats runtime_data |\ - sed "s/- compute:\s\+[[0-9]]\+/- compute: ??/"], [0], [dnl -Node: runtime_data -- recompute: 0 -- compute: ?? -- cancel: 0 -]) +# Make sure that setting dummy iface in two different transactions +# causes recompute. +check ovs-vsctl add-port br-int vif -- \ + set Interface vif type=dummy +check ovs-vsctl set Interface vif external-ids:iface-id=vif +wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log) -ge 1]) OVN_CLEANUP([hv1]) AT_CLEANUP
The test was checking if there was any recompute after all operations. This proven to be flaky because there might have been "random" recompute happening in the middle due to the following events: 1) SB becomes read only 2) non_vif_data handler for OVS_interface fails 3) Recompute of non_vif_data is not allowed because SB is read only 4) non_vif_data is marked as cancelled 5) Next engine run will do force recompute of all nodes To prevent that check specifically for the handler that is supposed to gracefully handle the interface changes. Fixes: 603f38da062b ("controller: Remove OvS iface type check in I-P processing.") Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Address comment from Ilya: - Add recompute case to the test so if the debug format changes the test fails. - Add missing checks for ovs-vsctl. --- tests/ovn-controller.at | 43 +++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 17 deletions(-)