On Thu, Jul 6, 2023 at 3:32 PM Han Zhou <[email protected]> wrote: > > Although each individual VIF port related changes are handled > incrementally, it still triggers recompute if there are in-flight > transactions (either to NB or SB) when a change comes, which is very > common in real production environment if change happens frequently. > It is also easy to hit such situation in test cases where nb_cfg > mechanism is heavily used, which makes it difficult to write reliable > and stable tests, such as what the commit 8c30ba1386 was trying to work > around. > > This patch skips the I-P engine execution until the NB & SB transaction > handles are available (no in-flight transactions), and when skippiing > the runs it keeps the tracked changes in IDL across main loop > iterations. This way we avoid recompute without worrying about missing > any changes. > > Signed-off-by: Han Zhou <[email protected]>
Acked-by: Numan Siddique <[email protected]> Numan > --- > northd/inc-proc-northd.c | 6 +-- > northd/ovn-northd.c | 22 +++++---- > tests/ovn-northd.at | 104 +++++++++++++++++++-------------------- > 3 files changed, 66 insertions(+), 66 deletions(-) > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 19fc67795643..d328deb222e6 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -296,6 +296,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, > struct ovsdb_idl_txn *ovnsb_txn, > bool recompute) { > + ovs_assert(ovnnb_txn && ovnsb_txn); > engine_init_run(); > > /* Force a full recompute if instructed to, for example, after a NB/SB > @@ -312,10 +313,7 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, > }; > > engine_set_context(&eng_ctx); > - > - if (ovnnb_txn && ovnsb_txn) { > - engine_run(true); > - } > + engine_run(true); > > if (!engine_has_run()) { > if (engine_need_run()) { > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 3e0848e41b8e..4fa1b039ea32 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -881,6 +881,7 @@ main(int argc, char *argv[]) > simap_destroy(&usage); > } > > + bool clear_idl_track = true; > if (!state.paused) { > if (!ovsdb_idl_has_lock(ovnsb_idl_loop.idl) && > !ovsdb_idl_is_lock_contended(ovnsb_idl_loop.idl)) > @@ -930,25 +931,26 @@ main(int argc, char *argv[]) > } > > if (ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) { > - int64_t loop_start_time = time_wall_msec(); > - bool activity = inc_proc_northd_run(ovnnb_txn, ovnsb_txn, > - recompute); > - recompute = false; > - if (ovnsb_txn) { > + bool activity = false; > + if (ovnnb_txn && ovnsb_txn) { > + int64_t loop_start_time = time_wall_msec(); > + activity = inc_proc_northd_run(ovnnb_txn, ovnsb_txn, > + recompute); > + recompute = false; > check_and_add_supported_dhcp_opts_to_sb_db( > ovnsb_txn, ovnsb_idl_loop.idl); > check_and_add_supported_dhcpv6_opts_to_sb_db( > ovnsb_txn, ovnsb_idl_loop.idl); > check_and_update_rbac( > ovnsb_txn, ovnsb_idl_loop.idl); > - } > > - if (ovnnb_txn && ovnsb_txn) { > update_sequence_numbers(loop_start_time, > ovnnb_idl_loop.idl, > ovnsb_idl_loop.idl, > ovnnb_txn, ovnsb_txn, > &ovnsb_idl_loop); > + } else if (!recompute) { > + clear_idl_track = false; > } > > /* If there are any errors, we force a full recompute in > order > @@ -998,8 +1000,10 @@ main(int argc, char *argv[]) > recompute = true; > } > > - ovsdb_idl_track_clear(ovnnb_idl_loop.idl); > - ovsdb_idl_track_clear(ovnsb_idl_loop.idl); > + if (clear_idl_track) { > + ovsdb_idl_track_clear(ovnnb_idl_loop.idl); > + ovsdb_idl_track_clear(ovnsb_idl_loop.idl); > + } > > unixctl_server_run(unixctl); > unixctl_server_wait(unixctl); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index f1bf9092eeb7..e79d33b2aec5 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9522,67 +9522,65 @@ as hv1 > ovs-vsctl add-br br-phys > ovn_attach n1 br-phys 192.168.0.11 > > -fail_count=0 > check_recompute_counter() { > northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE > inc-engine/show-stats northd recompute) > - if test x$northd_recomp != x$1; then > - fail_count=$(($fail_count + 1)) > - echo check northd recompute failed: expected $1, got $northd_recomp > - return 1 > - fi > + AT_CHECK([test x$northd_recomp = x$1]) > + > lflow_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats > lflow recompute) > - if test x$lflow_recomp != x$2; then > - fail_count=$(($fail_count + 1)) > - echo check lflow recompute failed: expected $2, got $lflow_recomp > - return 1 > - fi > - return 0 > + AT_CHECK([test x$lflow_recomp = x$2]) > } > > -# Depending on order of responses from NB and SB, the number of recompute may > -# be different. This test case only verifies the best case scenario, which > -# should have the expected recompute count at least 50% of the time. > +check ovn-nbctl --wait=hv ls-add ls0 > + > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > +check ovn-nbctl --wait=hv lsp-add ls0 lsp0-0 -- lsp-set-addresses lsp0-0 > "unknown" > +ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0 > external_ids:iface-id=lsp0-0 > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > +check_recompute_counter 5 5 > + > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > +check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 > "aa:aa:aa:00:00:01 192.168.0.11" > +ovs-vsctl add-port br-int lsp0-1 -- set interface lsp0-1 > external_ids:iface-id=lsp0-1 > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > +check_recompute_counter 0 0 > + > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > +check ovn-nbctl --wait=hv lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 > "aa:aa:aa:00:00:02 192.168.0.12" > +ovs-vsctl add-port br-int lsp0-2 -- set interface lsp0-2 > external_ids:iface-id=lsp0-2 > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > +check_recompute_counter 0 0 > + > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > +check ovn-nbctl --wait=hv lsp-del lsp0-1 > +check_recompute_counter 0 0 > + > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > +check ovn-nbctl --wait=hv lsp-set-addresses lsp0-2 "aa:aa:aa:00:00:88 > 192.168.0.88" > +check_recompute_counter 0 0 > + > +# Delete and re-add a LSP for several times continuously, to ensure > +# frequent operations do not trigger recompute when there are in-flight > +# transcations. > for i in $(seq 10); do > - check ovn-nbctl --wait=hv ls-add ls$i > - > - check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > - check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-0 -- lsp-set-addresses > lsp${i}-0 "unknown" > - 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 || 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 || 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 || 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 0 || 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 0 || 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 || continue > + # wait for sb but not wait for hv > + check ovn-nbctl --wait=sb lsp-del lsp0-2 > + check ovn-nbctl --wait=sb lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 > "aa:aa:aa:00:00:02 192.168.0.12" > > - CHECK_NO_CHANGE_AFTER_RECOMPUTE > + # even without waiting for sb > + check ovn-nbctl lsp-del lsp0-2 > + check ovn-nbctl lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 > "aa:aa:aa:00:00:02 192.168.0.12" > done > -echo Test failed $fail_count in 10. > -AT_CHECK([test $fail_count -le 5]) > +check_recompute_counter 0 0 > + > +# 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 > + > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > OVN_CLEANUP([hv1]) > AT_CLEANUP > -- > 2.30.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
