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.
> -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]>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
