Hi Han, thanks for the change.
Acked-by: Mark Michelson <[email protected]>
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
