Hi Mark Thanks for the review and the comments. I was initially afraid of more notifications received within the same transactions causing even less recomputes (hence the -le). However, after a second look I think we can only receive 4 or 5 recomputes in this scenario (1 for lsp added by nb, 1 for pb added by sb, 1 or 2 for pb->chassis and pb->up by sb, and 1 for lsp->up by nb). I would vote for 1) - in case in the future the nb of recomputes differs between north, flow, ...
Thanks Xavier On Tue, Sep 19, 2023 at 5:28 PM Mark Michelson <mmich...@redhat.com> wrote: > Of the 15 patches in this set, this is the only one that I disagree with. > > The problem with this change is that the test writer likely has a > specific amount of recomputes they expect to see based on the changes > that are applied. With the "-le" change, it means that the test could > pass in a situation that should probably actually fail. > > Here are a couple of modification ideas: > > 1) Instead of providing a single number for the expected recompute > count, provide a range instead. The test currently does > `check_recompute_counter 5 5 5` , but it could be changed to something > like `check_recompute_counter 4 5 4 5 4 5`, giving a minimum and maximum > number of recomputes. > > 2) Provide an optional "tolerance" parameter. The test currently does > `check_recompute_counter 5 5 5` but it could be something like > `check_recompute_counter 5 5 5 -1` to indicate that the number could > actually be 5 - 1 and we'd still consider that a pass. > > What do you think? > > On 9/18/23 12:47, Xavier Simonart wrote: > > We might get less recomputes than expected: e.g. Port_Binding->chassis > and > > Port_Binding->up might be received by northd within the same idl > transaction. > > > > Signed-off-by: Xavier Simonart <xsimo...@redhat.com> > > --- > > tests/ovn-northd.at | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index f51e29b11..abb425a8c 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -10010,13 +10010,13 @@ ovn_attach n1 br-phys 192.168.0.11 > > > > check_recompute_counter() { > > northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE > inc-engine/show-stats northd recompute) > > - AT_CHECK([test x$northd_recomp = x$1]) > > + AT_CHECK([test $northd_recomp -le $1]) > > > > lflow_recomp=$(as northd ovn-appctl -t NORTHD_TYPE > inc-engine/show-stats lflow recompute) > > - AT_CHECK([test x$lflow_recomp = x$2]) > > + AT_CHECK([test $lflow_recomp -le $2]) > > > > sync_sb_pb_recomp=$(as northd ovn-appctl -t NORTHD_TYPE > inc-engine/show-stats sync_to_sb_pb recompute) > > - AT_CHECK([test x$sync_sb_pb_recomp = x$3]) > > + AT_CHECK([test $sync_sb_pb_recomp -le $3]) > > } > > > > check ovn-nbctl --wait=hv ls-add ls0 > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev