| Message ID | 20230628062217.3955646-1-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-northd.at: Fix the LSP incremental processing test case. | 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 6/28/23 08:22, Han Zhou wrote: > The test case intended to ensure there are no more than 3 failures in 10 > runs. However, it used "break" to exit the loop whenever a failure is > encountered, end up with at most 1 failure and so the final check for > the failure count will always pass. It should use "continue" instead. > > Fixes: 8c30ba13869e ("ovn-northd.at: Fix unstable LSP incremental processing test.") > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- Hi Han, This specific change looks good to me: Acked-by: Dumitru Ceara <dceara@redhat.com> However, I think the "70% of the time" part of the test is still optimistic. I ran the test in a loop 20 times and it failed after 10 iterations or so. I changed it to 50% locally and it passed all 20 iterations. Do you agree to reducing it to 50%? Thanks, Dumitru > tests/ovn-northd.at | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index ada2d2a4aa5e..88fe93a4e51f 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9522,34 +9522,34 @@ for i in $(seq 10); do > ovs-vsctl add-port br-int lsp${i}-0 -- set interface lsp${i}-0 external_ids:iface-id=lsp${i}-0 > wait_for_ports_up > check ovn-nbctl --wait=hv sync > - check_recompute_counter 5 5 || break > + check_recompute_counter 5 5 || continue > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-1 -- lsp-set-addresses lsp${i}-1 "aa:aa:aa:00:00:01 192.168.0.11" > ovs-vsctl add-port br-int lsp${i}-1 -- set interface lsp${i}-1 external_ids:iface-id=lsp${i}-1 > wait_for_ports_up > check ovn-nbctl --wait=hv sync > - check_recompute_counter 0 0 || break > + check_recompute_counter 0 0 || continue > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-2 -- lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:02 192.168.0.12" > ovs-vsctl add-port br-int lsp${i}-2 -- set interface lsp${i}-2 external_ids:iface-id=lsp${i}-2 > wait_for_ports_up > check ovn-nbctl --wait=hv sync > - check_recompute_counter 0 0 || break > + check_recompute_counter 0 0 || continue > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=hv lsp-del lsp${i}-1 > - check_recompute_counter 0 1 || break > + check_recompute_counter 0 1 || continue > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=hv lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:88 192.168.0.88" > - check_recompute_counter 0 1 || break > + check_recompute_counter 0 1 || continue > > # No change, no recompute > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb sync > - check_recompute_counter 0 0 || break > + check_recompute_counter 0 0 || continue > > done > echo Test failed $fail_count in 10.
On Fri, Jun 30, 2023 at 5:53 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 6/28/23 08:22, Han Zhou wrote: > > The test case intended to ensure there are no more than 3 failures in 10 > > runs. However, it used "break" to exit the loop whenever a failure is > > encountered, end up with at most 1 failure and so the final check for > > the failure count will always pass. It should use "continue" instead. > > > > Fixes: 8c30ba13869e ("ovn-northd.at: Fix unstable LSP incremental processing test.") > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > Hi Han, > > This specific change looks good to me: > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > However, I think the "70% of the time" part of the test is still > optimistic. I ran the test in a loop 20 times and it failed after 10 > iterations or so. > > I changed it to 50% locally and it passed all 20 iterations. > > Do you agree to reducing it to 50%? > Agree. Thanks for your review and the extra testing. I changed it to 50% and applied it to main. (Hopefully an upcoming patch will make it 100% predicatible). Regards, Han > Thanks, > Dumitru > > > tests/ovn-northd.at | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index ada2d2a4aa5e..88fe93a4e51f 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -9522,34 +9522,34 @@ for i in $(seq 10); do > > ovs-vsctl add-port br-int lsp${i}-0 -- set interface lsp${i}-0 external_ids:iface-id=lsp${i}-0 > > wait_for_ports_up > > check ovn-nbctl --wait=hv sync > > - check_recompute_counter 5 5 || break > > + check_recompute_counter 5 5 || continue > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-1 -- lsp-set-addresses lsp${i}-1 "aa:aa:aa:00:00:01 192.168.0.11" > > ovs-vsctl add-port br-int lsp${i}-1 -- set interface lsp${i}-1 external_ids:iface-id=lsp${i}-1 > > wait_for_ports_up > > check ovn-nbctl --wait=hv sync > > - check_recompute_counter 0 0 || break > > + check_recompute_counter 0 0 || continue > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-2 -- lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:02 192.168.0.12" > > ovs-vsctl add-port br-int lsp${i}-2 -- set interface lsp${i}-2 external_ids:iface-id=lsp${i}-2 > > wait_for_ports_up > > check ovn-nbctl --wait=hv sync > > - check_recompute_counter 0 0 || break > > + check_recompute_counter 0 0 || continue > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=hv lsp-del lsp${i}-1 > > - check_recompute_counter 0 1 || break > > + check_recompute_counter 0 1 || continue > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=hv lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:88 192.168.0.88" > > - check_recompute_counter 0 1 || break > > + check_recompute_counter 0 1 || continue > > > > # No change, no recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb sync > > - check_recompute_counter 0 0 || break > > + check_recompute_counter 0 0 || continue > > > > done > > echo Test failed $fail_count in 10. >
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index ada2d2a4aa5e..88fe93a4e51f 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -9522,34 +9522,34 @@ for i in $(seq 10); do ovs-vsctl add-port br-int lsp${i}-0 -- set interface lsp${i}-0 external_ids:iface-id=lsp${i}-0 wait_for_ports_up check ovn-nbctl --wait=hv sync - check_recompute_counter 5 5 || break + check_recompute_counter 5 5 || continue check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-1 -- lsp-set-addresses lsp${i}-1 "aa:aa:aa:00:00:01 192.168.0.11" ovs-vsctl add-port br-int lsp${i}-1 -- set interface lsp${i}-1 external_ids:iface-id=lsp${i}-1 wait_for_ports_up check ovn-nbctl --wait=hv sync - check_recompute_counter 0 0 || break + check_recompute_counter 0 0 || continue check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-2 -- lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:02 192.168.0.12" ovs-vsctl add-port br-int lsp${i}-2 -- set interface lsp${i}-2 external_ids:iface-id=lsp${i}-2 wait_for_ports_up check ovn-nbctl --wait=hv sync - check_recompute_counter 0 0 || break + check_recompute_counter 0 0 || continue check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=hv lsp-del lsp${i}-1 - check_recompute_counter 0 1 || break + check_recompute_counter 0 1 || continue check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=hv lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:88 192.168.0.88" - check_recompute_counter 0 1 || break + check_recompute_counter 0 1 || continue # No change, no recompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb sync - check_recompute_counter 0 0 || break + check_recompute_counter 0 0 || continue done echo Test failed $fail_count in 10.
The test case intended to ensure there are no more than 3 failures in 10 runs. However, it used "break" to exit the loop whenever a failure is encountered, end up with at most 1 failure and so the final check for the failure count will always pass. It should use "continue" instead. Fixes: 8c30ba13869e ("ovn-northd.at: Fix unstable LSP incremental processing test.") Signed-off-by: Han Zhou <hzhou@ovn.org> --- tests/ovn-northd.at | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)