On Thu, Aug 17, 2023 at 2:13 AM Ales Musil <[email protected]> wrote: > > > > On Wed, Aug 16, 2023 at 2:56 AM Han Zhou <[email protected]> wrote: >> >> Commit 1eb09838 fixed an incremental processing problem by falling back >> to recompute. The problem was handling VIF changes for a LS that >> has router ports when: >> - adding the first switch port to the LS, or >> - deleting the last switch port from the LS >> because there are router port related lflows that depends on the >> condition whether there are only router ports on the LS or not. >> >> Since such scenario is common, this patch further improves it to avoid >> falling back to recompute. >> >> Signed-off-by: Han Zhou <[email protected]> > > > Hi Han, > > there is one small nit down below. >> >> --- >> northd/northd.c | 71 +++++++++++++++++++++++++-------------------- >> northd/northd.h | 1 + >> tests/ovn-northd.at | 6 ++-- >> 3 files changed, 45 insertions(+), 33 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 9a12a94ae25d..a87298a026de 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -5120,21 +5120,8 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, >> op->visited = false; >> } >> >> - /* Check if the logical switch has only router ports before this change. >> - * If so, fall back to recompute. >> - * lflow engine node while building the lflows checks if the logical switch >> - * has any router ports and depending on that it adds different flows. >> - * See build_lswitch_rport_arp_req_flow() for more details. >> - * Note: We can definitely handle this scenario incrementally in the >> - * northd engine node and fall back to recompute in lflow engine node >> - * and even handle this incrementally in lflow node. Until we do that >> - * resort to full recompute of northd node. >> - */ >> - bool only_rports = (od->n_router_ports >> - && (od->n_router_ports == hmap_count(&od->ports))); >> - if (only_rports) { >> - goto fail; >> - } >> + ls_change->had_only_router_ports = (od->n_router_ports >> + && (od->n_router_ports == hmap_count(&od->ports))); >> >> /* Compare the individual ports in the old and new Logical Switches */ >> for (size_t j = 0; j < changed_ls->n_ports; ++j) { >> @@ -5217,22 +5204,6 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, >> } >> } >> >> - /* Check if the logical switch has only router ports after this change. >> - * If so, fall back to recompute. >> - * lflow engine node while building the lflows checks if the logical switch >> - * has any router ports and depending on that it adds different flows. >> - * See build_lswitch_rport_arp_req_flow() for more details. >> - * Note: We can definitely handle this scenario incrementally in the >> - * northd engine node and fall back to recompute in lflow engine node >> - * and even handle this incrementally in lflow node. Until we do that >> - * resort to full recompute of northd node. >> - */ >> - only_rports = (od->n_router_ports >> - && (od->n_router_ports == hmap_count(&od->ports))); >> - if (only_rports) { >> - goto fail_clean_deleted; >> - } >> - >> if (!ovs_list_is_empty(&ls_change->added_ports) || >> !ovs_list_is_empty(&ls_change->updated_ports) || >> !ovs_list_is_empty(&ls_change->deleted_ports)) { >> @@ -16550,6 +16521,44 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, >> lfrn->lflow); >> } >> } >> + >> + bool ls_has_only_router_ports = (ls_change->od->n_router_ports && >> + (ls_change->od->n_router_ports == >> + hmap_count(&ls_change->od->ports))); >> + >> + if (ls_change->had_only_router_ports != ls_has_only_router_ports) { >> + /* There are lflows related to router ports that depends on whether >> + * there are switch ports on the logical switch (see >> + * build_lswitch_rport_arp_req_flow() for more details). Since this >> + * dependency changed, we need to regenerate lflows for each router >> + * port on this logical switch. */ >> + for (size_t i = 0; i < ls_change->od->n_router_ports; i++) { >> + op = ls_change->od->router_ports[i]; >> + >> + /* Delete old lflows. */ >> + if (!delete_lflow_for_lsp(op, "affected router", >> + lflow_input->sbrec_logical_flow_table, >> + lflows)) { >> + return false; >> + } >> + >> + /* Generate new lflows. */ >> + struct ds match = DS_EMPTY_INITIALIZER; >> + struct ds actions = DS_EMPTY_INITIALIZER; >> + build_lswitch_and_lrouter_iterate_by_lsp(op, >> + lflow_input->ls_ports, lflow_input->lr_ports, >> + lflow_input->meter_groups, &match, &actions, lflows); >> + ds_destroy(&match); >> + ds_destroy(&actions); >> + >> + /* Sync the new flows to SB. */ >> + struct lflow_ref_node *lfrn; >> + LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) { >> + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows, >> + lfrn->lflow); >> + } >> + } >> + } >> } >> return true; >> >> diff --git a/northd/northd.h b/northd/northd.h >> index f3e63b1e1afb..d9025947183f 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -93,6 +93,7 @@ struct ls_change { >> struct ovs_list added_ports; >> struct ovs_list deleted_ports; >> struct ovs_list updated_ports; >> + bool had_only_router_ports; >> }; >> >> /* Track what's changed for logical switches. >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index d5be3be75b1b..c9735c6dbaf9 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -9622,13 +9622,15 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats >> # Add a lsp. northd and lflow engine should recompute since this is >> # the first lsp added after the router ports. >> 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" >> -check_recompute_counter 1 1 >> +check_recompute_counter 0 0 >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> # Delete the lsp. northd and lflow engine should recompute as >> # the logical switch is now left with only router ports. >> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats >> check ovn-nbctl lsp-del lsp0-1 > > > We should sync on the lsp-del.
Good catch! This should belong to a separate patch, but since it is very small I will include it when merging. > >> >> -check_recompute_counter 1 1 >> +check_recompute_counter 0 0 >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> AT_CLEANUP >> ]) >> -- >> 2.38.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > Other than that it looks good, thanks. > > Reviewed-by: Ales Musil <[email protected]> > Thank Ales. > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA > > [email protected] IM: amusil _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
