On Thu, Aug 31, 2023 at 12:47 PM Mark Michelson <[email protected]> wrote: > > Hi Han, thanks for the change. > > Acked-by: Mark Michelson <[email protected]>
Thanks Mark and Ales. I applied it to main with Ales's comment addressed. Han > > On 8/17/23 12:14, Han Zhou wrote: > > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
