Hi Mark,
thank you for reviewing my patch is submitted v2 addressing your comments.
On 7/28/23 23:16, Mark Michelson wrote:
Hi Mohammad,
The actual code change looks good, but I have some other minor
comments to address. See inline below.
On 7/26/23 12:12, Mohammad Heib wrote:
Currently when ovs interface ofport is updated
after setting external_ids:iface_id, the ovn-controller
will see this change but will not do much if it handles
this change incrementally.
This behavior leads to a mismatch between the ovs openflow
flows in table=0 (inaccurate in_port) and the ofport number
that the packet was received at which will lead to packets
drop in table=0.
This patch will resolve the above issue by triggering
flows recompute during the I-P processing only if the
affected port are associated with lport and has flows
that need to be updated.
Reported-at: https://issues.redhat.com/browse/FD-3063
Signed-off-by: Mohammad Heib <[email protected]>
---
controller/binding.c | 35 +++++++++++++++++++++++++++++++++++
tests/ovn-controller.at | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/controller/binding.c b/controller/binding.c
index 9aa3fc6c4..d28548d93 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1514,6 +1514,35 @@ release_lport(const struct sbrec_port_binding
*pb,
return true;
}
+/*
+ * This function will update the tracked_dp_bindings
+ * whenever an ofport on a specific ovs port.
s/specific ovs port/specific ovs port changes/
+ * This update will trigger flow recomputation during
+ * the incremental processing run which updates the local
+ * flows in_port filed.
+ *
+ * This function will trigger flow recomputation only for
+ * affected local_bindings that have port_binding associated
+ * with it, otherwise, no flows are installed and we don't
+ * have to update any in_port field.
+ */
+static void
+handle_ovs_ofport_update(const char *iface_id,
+ struct binding_ctx_out *b_ctx_out)
+{
+ struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
+ struct local_binding *lbinding = local_binding_find(local_bindings,
+ iface_id);
+ struct binding_lport *b_lport =
+ local_binding_get_primary_or_localport_lport(lbinding);
+
+ if (b_lport) {
+ tracked_datapath_lport_add(b_lport->pb,
TRACKED_RESOURCE_UPDATED,
+ b_ctx_out->tracked_dp_bindings);
+ b_ctx_out->local_lports_changed = true;
+ }
+}
I think this logic should move into consider_iface_claim() in
binding.c. It is called for all relevant OVS interfaces, and that
function also retrieves the local_binding and binding_lport.
In addition to saving a bit of computation, putting this change there
would also prevent a potential crash, since it is possible that
b_lport->pb may be NULL. We have a NULL check already in
consider_iface_claim(), so you could insert this new code below that
NULL check.
+
static bool
is_lbinding_set(struct local_binding *lbinding)
{
@@ -2681,6 +2710,12 @@ binding_handle_ovs_interface_changes(struct
binding_ctx_in *b_ctx_in,
if (!handled) {
break;
}
+
+ if (!b_ctx_out->local_lports_changed
+ && ovsrec_interface_is_updated(iface_rec,
+ OVSREC_INTERFACE_COL_OFPORT)) {
+ handle_ovs_ofport_update(iface_id, b_ctx_out);
+ }
}
}
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 28c13234c..1461cade6 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -2608,3 +2608,42 @@ AT_CHECK([ovn-sbctl get chassis $chassis_id
other_config:unsupported], [1], [ign
OVN_CLEANUP([hv1])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller - ovs iface change ofport])
+AT_KEYWORDS([ovn])
+ovn_start
+
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+ set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
+ options:tx_pcap=hv1/vif1-tx.pcap \
+ options:rxq_pcap=hv1/vif1-rx.pcap \
+ ofport-request=1
+
+
+ovn-nbctl ls-add sw0
+
+check ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3
1000::3"
+
+wait_for_ports_up
+ovn-nbctl --wait=hv sync
+
+OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int table=0
| grep priority=100 | grep in_port=1 | grep "resubmit(,8)" | grep -c
n_packets=0`])
A couple of things:
* Do we need to grep for the priority, the resubmit action, and
"n_packets=0" ? Isn't it enough to just grep for in_port=1 in table 0?
* While this syntax works fine, there is an OVS_WAIT_FOR_OUTPUT macro
that is more commonly used for this type of test.
OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 | grep
-c in_port=1], [0],[dnl
1
]
+
+# update the ovs interface ofport from 1 to 24
+check as hv1 ovs-vsctl set Interface hv1-vif1 ofport-request=24
+OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface hv1-vif1
ofport` = x24])
I was going to suggest a better way to do the above test, but then I
realized that the wait_column() function does not work with the
vswitchd database. It only works with OVN northbound and southbound.
Maybe a future improvement :)?
+
+OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int table=0
| grep priority=100 | grep in_port=24 | grep "resubmit(,8)" | grep -c
n_packets=0`])
+OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int table=0
| grep priority=100 | grep in_port=1 | grep "resubmit(,8)" | grep -c
n_packets=0`])
My same comments about the grep attributes and using
OVS_WAIT_FOR_OUTPUT apply here.
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
