On 8/8/24 12:38, 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 <[email protected]>
> ---
> tests/ovn-controller.at | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 898981867..f61c8c432 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -3308,51 +3308,52 @@ 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 -- \
> 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)])
Hi, Ales. Thanks for the fix!
The change seems fine at a glance (I didn't test), but the test itself
becomes fragile. All the checks are negative here, i.e. we're always
checking that there was no match in the log. If the log message changes
slightly in the future, the test will keep silently passing while not
actually testing anything useful.
There could be a few ways to avoid that:
1. Collect recompute stats per failed node, so we can explicitly check
for a specific reason. May still be a little flaky though.
2. Add at least one positive check that ensures that log we're looking
for can be actually emitted.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
