diff mbox series

[ovs-dev,v2,13/14] northd: Incremental processing of SB port_binding in "northd" node.

Message ID 20230602041150.3019311-14-hzhou@ovn.org
State Accepted
Headers show
Series ovn-northd incremental processing for VIF changes | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_ovn-kubernetes success github build: passed
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Han Zhou June 2, 2023, 4:11 a.m. UTC
Most SB port_binding changes do not need to be handled by "northd" node,
because they are either result of SB transactions from northd itself, or
changes already handled by sync-from-sb node. This change reduces number
of recompute of both "northd" and "lflow" nodes that would have been
triggered by SB port_binding changes. Now for common VIF changes there
is zero recompute of "northd" node.

Signed-off-by: Han Zhou <hzhou@ovn.org>
Reviewed-by: Ales Musil <amusil@redhat.com>
---
 northd/en-northd.c       | 18 ++++++++++++++
 northd/en-northd.h       |  1 +
 northd/inc-proc-northd.c | 12 ++++++----
 northd/northd.c          | 52 ++++++++++++++++++++++++++++++++++++++++
 northd/northd.h          |  2 ++
 tests/ovn-northd.at      | 16 ++++++-------
 6 files changed, 88 insertions(+), 13 deletions(-)
diff mbox series

Patch

diff --git a/northd/en-northd.c b/northd/en-northd.c
index 785a592516c0..044fa7019023 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -187,6 +187,24 @@  northd_nb_logical_switch_handler(struct engine_node *node,
     return true;
 }
 
+bool
+northd_sb_port_binding_handler(struct engine_node *node,
+                               void *data)
+{
+    struct northd_data *nd = data;
+
+    struct northd_input input_data;
+
+    northd_get_input_data(node, &input_data);
+
+    if (!northd_handle_sb_port_binding_changes(
+        input_data.sbrec_port_binding_table, &nd->ls_ports)) {
+        return false;
+    }
+
+    return true;
+}
+
 void
 *en_northd_init(struct engine_node *node OVS_UNUSED,
                 struct engine_arg *arg OVS_UNUSED)
diff --git a/northd/en-northd.h b/northd/en-northd.h
index a53a162bda48..20cc77f108a3 100644
--- a/northd/en-northd.h
+++ b/northd/en-northd.h
@@ -16,5 +16,6 @@  void en_northd_cleanup(void *data);
 void en_northd_clear_tracked_data(void *data);
 bool northd_nb_nb_global_handler(struct engine_node *, void *data OVS_UNUSED);
 bool northd_nb_logical_switch_handler(struct engine_node *, void *data);
+bool northd_sb_port_binding_handler(struct engine_node *, void *data);
 
 #endif /* EN_NORTHD_H */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 0642c9514b7c..3ec791c8941c 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -143,10 +143,6 @@  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 {
     /* Define relationships between nodes where first argument is dependent
      * on the second argument */
-    engine_add_input(&en_northd, &en_nb_nb_global,
-                     northd_nb_nb_global_handler);
-    engine_add_input(&en_northd, &en_nb_logical_switch,
-                     northd_nb_logical_switch_handler);
     engine_add_input(&en_northd, &en_nb_port_group, NULL);
     engine_add_input(&en_northd, &en_nb_load_balancer, NULL);
     engine_add_input(&en_northd, &en_nb_load_balancer_group, NULL);
@@ -163,7 +159,6 @@  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
     engine_add_input(&en_northd, &en_sb_mirror, NULL);
     engine_add_input(&en_northd, &en_sb_meter, NULL);
     engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
-    engine_add_input(&en_northd, &en_sb_port_binding, NULL);
     engine_add_input(&en_northd, &en_sb_mac_binding, NULL);
     engine_add_input(&en_northd, &en_sb_dns, NULL);
     engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
@@ -174,6 +169,13 @@  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
     engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
     engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL);
 
+    engine_add_input(&en_northd, &en_sb_port_binding,
+                     northd_sb_port_binding_handler);
+    engine_add_input(&en_northd, &en_nb_nb_global,
+                     northd_nb_nb_global_handler);
+    engine_add_input(&en_northd, &en_nb_logical_switch,
+                     northd_nb_logical_switch_handler);
+
     engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
     engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
     engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
diff --git a/northd/northd.c b/northd/northd.c
index 7e15216031cc..ddbc1bf389c8 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5116,6 +5116,58 @@  fail:
     return false;
 }
 
+bool
+northd_handle_sb_port_binding_changes(
+    const struct sbrec_port_binding_table *sbrec_port_binding_table,
+    struct hmap *ls_ports)
+{
+    const struct sbrec_port_binding *pb;
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) {
+        struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port);
+        if (op && !op->lsp_can_be_inc_processed) {
+            return false;
+        }
+        if (sbrec_port_binding_is_new(pb)) {
+            /* Most likely the PB was created by northd and this is the
+             * notification of that trasaction. So we just update the sb
+             * pointer in northd data. Fallback to recompute otherwise. */
+            if (!op) {
+                VLOG_WARN_RL(&rl, "A port-binding for %s is created but the "
+                            "LSP is not found.", pb->logical_port);
+                return false;
+            }
+            op->sb = pb;
+        } else if (sbrec_port_binding_is_deleted(pb)) {
+            /* Most likely the PB was deleted by northd and this is the
+             * notification of that transaction, and we can ignore in this
+             * case. Fallback to recompute otherwise, to avoid dangling
+             * sb idl pointers and other unexpected behavior. */
+            if (op) {
+                VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the "
+                            "LSP still exists.", pb->logical_port);
+                return false;
+            }
+        } else {
+            /* The PB is updated, most likely because of binding/unbinding
+             * to/from a chassis, and we can ignore the change (updating NB
+             * "up" will be handled in the engine node "sync_from_sb").
+             * Fallback to recompute for anything unexpected. */
+            if (!op) {
+                VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the "
+                            "LSP is not found.", pb->logical_port);
+                return false;
+            }
+            if (op->sb != pb) {
+                VLOG_WARN_RL(&rl, "A port-binding for %s is updated with a new"
+                             "IDL row, which is unusual.", pb->logical_port);
+                return false;
+            }
+        }
+    }
+    return true;
+}
+
 struct multicast_group {
     const char *name;
     uint16_t key;               /* OVN_MIN_MULTICAST...OVN_MAX_MULTICAST. */
diff --git a/northd/northd.h b/northd/northd.h
index af3c90f7fceb..b69b496e8ef9 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -340,6 +340,8 @@  void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
 bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
                                     struct tracked_ls_changes *,
                                     struct lflow_input *, struct hmap *lflows);
+bool northd_handle_sb_port_binding_changes(
+    const struct sbrec_port_binding_table *, struct hmap *ls_ports);
 
 void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
                      const struct nbrec_bfd_table *,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 635cd2d892e6..d5436136b809 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -9504,30 +9504,30 @@  OVS_WAIT_UNTIL([test `as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats
 
 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"
-AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [3
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [0
 ])
-AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [5
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [2
 ])
 
 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"
-AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [3
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [0
 ])
-AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [5
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [2
 ])
 
 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
 check ovn-nbctl --wait=hv lsp-del lsp0-1
-AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [1
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [0
 ])
-AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [2
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [1
 ])
 
 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"
-AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [1
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [0
 ])
-AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [2
+AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [1
 ])
 
 # No change, no recompute