[ovs-dev] [RFC PATCH ovn] binding: Do not try to incrementally process former VIF updates.
Dumitru Ceara
dceara at redhat.com
Thu Feb 18 18:31:32 UTC 2021
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 at 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 --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
--
1.8.3.1
More information about the dev
mailing list