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 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
> > ---
> > 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
