diff mbox series

[ovs-dev,RFC] binding: Do not try to incrementally process former VIF updates.

Message ID 1613673092-14093-1-git-send-email-dceara@redhat.com
State New
Headers show
Series [ovs-dev,RFC] binding: Do not try to incrementally process former VIF updates. | expand

Commit Message

Dumitru Ceara Feb. 18, 2021, 6:31 p.m. UTC
If a Port_Binding corresponding to a VIF changes in the Southbound to
represent a port that cannot be bound locally (e.g., localport,
localnet, patch) then ovn-controller should not try to process this
update incrementally.  If it would try, it would need to first remove
cleanup all previous lbindings from memory which is not trivial.

Failing to do so might cause ovn-controller to access already freed
memory later on.

However, because this is an unlikely scenario, to avoid over
complicating the incremental processing engine, we now rely to a regular
recompute when such Port_Binding updates are detected.

This implies though that when the runtime_data I-P node is updated we
also need to recompute physical flows to make sure that flows for
specific types of ports, e.g., localports, are properly installed.

To reproduce the issue, this patch also modifies the "ovn -- 2 HVs, 1
lport/HV, localport ports" test.  More tests are added for all types of
port bindings that shouldn't be claimed locally.

Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS interface in runtime_data.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
Note: sending this patch as RFC for now because I'm not completely sure
if the physical_run() call in the I-P engine can be avoided.
---
 controller/binding.c        | 32 ++++++++++++++++++++++++
 controller/ovn-controller.c |  2 ++
 tests/ovn.at                | 59 ++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 87 insertions(+), 6 deletions(-)
diff mbox series

Patch

diff --git a/controller/binding.c b/controller/binding.c
index efaa109..9ce5c01 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1027,6 +1027,17 @@  is_lbinding_container_parent(struct local_binding *lbinding)
 }
 
 static bool
+is_non_local_port_bound(const struct sbrec_port_binding *pb,
+                        enum en_lport_type lport_type)
+{
+    if (lport_type == LP_PATCH || lport_type == LP_LOCALNET ||
+            lport_type == LP_LOCALPORT) {
+        return !!pb->chassis;
+    }
+    return false;
+}
+
+static bool
 release_local_binding_children(const struct sbrec_chassis *chassis_rec,
                                struct local_binding *lbinding,
                                bool sb_readonly,
@@ -1486,9 +1497,17 @@  binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
                                        b_ctx_in->port_binding_table) {
         enum en_lport_type lport_type = get_lport_type(pb);
 
+        if (is_non_local_port_bound(pb, lport_type)) {
+            release_lport(pb, !b_ctx_in->ovnsb_idl_txn, NULL);
+        }
+
         switch (lport_type) {
         case LP_PATCH:
+            update_local_lport_ids(pb, b_ctx_out);
+            break;
         case LP_LOCALPORT:
+            update_local_lport_ids(pb, b_ctx_out);
+            break;
         case LP_VTEP:
             update_local_lport_ids(pb, b_ctx_out);
             break;
@@ -1818,6 +1837,12 @@  consider_iface_claim(const struct ovsrec_interface *iface_rec,
     }
 
     if (lbinding->pb) {
+        enum en_lport_type lport_type = get_lport_type(lbinding->pb);
+
+        if (lport_type != LP_VIF && lport_type != LP_VIRTUAL) {
+            return true;
+        }
+
         if (!consider_vif_lport(lbinding->pb, b_ctx_in, b_ctx_out,
                                 lbinding, qos_map)) {
             return false;
@@ -2419,6 +2444,13 @@  delete_done:
             break;
         }
 
+        /* Former VIFs that changed to non-local ports should be released,
+         * but we cannot handle this incrementally.
+         */
+        if (is_non_local_port_bound(pb, lport_type)) {
+            handled = false;
+        }
+
         if (!handled) {
             break;
         }
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 4343650..15c78db 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2306,6 +2306,8 @@  flow_output_runtime_data_handler(struct engine_node *node,
     struct physical_ctx p_ctx;
     init_physical_ctx(node, rt_data, &p_ctx);
 
+    physical_run(&p_ctx, &fo->flow_table);
+
     struct tracked_binding_datapath *tdp;
     HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
         if (tdp->is_new) {
diff --git a/tests/ovn.at b/tests/ovn.at
index 344e6bf..b3a8b03 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -11319,11 +11319,6 @@  ovn_start
 
 ovn-nbctl ls-add ls1
 
-# Add localport to the switch
-ovn-nbctl lsp-add ls1 lp01
-ovn-nbctl lsp-set-addresses lp01 f0:00:00:00:00:01
-ovn-nbctl lsp-set-type lp01 localport
-
 net_add n1
 
 for i in 1 2; do
@@ -11350,10 +11345,17 @@  for i in 1 2; do
         OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lp${i}1` = xup])
 done
 
+# Add localport to the switch
+ovn-nbctl lsp-add ls1 lp01
+ovn-nbctl lsp-set-addresses lp01 f0:00:00:00:00:01
+ovn-nbctl lsp-set-type lp01 localport
+
 wait_for_ports_up
-ovn-nbctl --wait=sb sync
+ovn-nbctl --wait=hv sync
 ovn-sbctl dump-flows
 
+check_column "" Port_Binding chassis logical_port=lp01
+
 OVN_POPULATE_ARP
 
 # Given the name of a logical port, prints the name of the hypervisor
@@ -22606,6 +22608,51 @@  wait_row_count Port_Binding 1 logical_port=lsp-cont1 chassis=$ch
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 
+# Test that OVS.external_ids:iface-id doesn't affect non-VIF port bindings.
+AT_SETUP([ovn -- Non-VIF ports incremental processing])
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.10
+
+check ovn-nbctl ls-add ls1 -- lsp-add ls1 lsp1
+
+as hv1
+check ovs-vsctl \
+    -- add-port br-int vif1 \
+    -- set Interface vif1 external_ids:iface-id=lsp1
+
+# ovn-controller should bind the interface.
+wait_for_ports_up
+hv_uuid=$(fetch_column Chassis _uuid name=hv1)
+check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1
+
+# Change the port type to router, ovn-controller should release it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 router
+check_column "" Port_Binding chassis logical_port=lsp1
+
+# Clear port type, ovn-controller should rebind it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 ''
+check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1
+
+# Change the port type to localnet, ovn-controller should release it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 localnet
+check_column "" Port_Binding chassis logical_port=lsp1
+
+# Clear port type, ovn-controller should rebind it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 ''
+check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1
+
+# Change the port type to localport, ovn-controller should release it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 localport
+check_column "" Port_Binding chassis logical_port=lsp1
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+
 # Test dropping traffic destined to router owned IPs.
 AT_SETUP([ovn -- gateway router drop traffic for own IPs])
 ovn_start