Thanks Ales, especially for the explanation for why the type check is
still maintained.
Acked-by: Mark Michelson <[email protected]>
On 7/24/24 08:53, Ales Musil wrote:
The handler for OvS interface in runtime data was checking interface
type before proceeding with the I-P processing. The type is not
necessary because the main thing that is interesting for OVN is
the iface-id, if the interface doesn't have any it won't be processed
regardless of the type. The processing would happen anyway, however
it would be more costly because it would lead to full recompute
of the whole runtime data node. The type check remains for the purpose
of not regressing when iface id is set in different transaction
than the actual interface creation for the internal and empty type.
Reported-at: https://github.com/ovn-org/ovn/issues/174
Reported-at: https://issues.redhat.com/browse/FDP-255
Signed-off-by: Ales Musil <[email protected]>
---
v2: Rebase on top of current main.
Address comment from Xavier:
- Make sure we are not causing recompute if iface-id is set in separate
transaction.
---
controller/binding.c | 14 +++++-----
tests/ovn-controller.at | 57 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index b7e7f4874..f4f5f9407 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -2630,17 +2630,17 @@ binding_handle_ovs_interface_changes(struct
binding_ctx_in *b_ctx_in,
const struct ovsrec_interface *iface_rec;
OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
b_ctx_in->iface_table) {
- if (!is_iface_vif(iface_rec)) {
- /* Right now we are not handling ovs_interface changes of
- * other types. This can be enhanced to handle of
- * types - patch and tunnel. */
+ const char *iface_id = smap_get(&iface_rec->external_ids, "iface-id");
+ const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids,
+ iface_rec->name);
+ if (!iface_id && !old_iface_id && !is_iface_vif(iface_rec)) {
+ /* Right now we are not handling ovs_interface changes if the
+ * interface doesn't have iface-id or didn't have it
+ * previously. */
handled = false;
break;
}
- const char *iface_id = smap_get(&iface_rec->external_ids, "iface-id");
- const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids,
- iface_rec->name);
const char *cleared_iface_id = NULL;
if (!ovsrec_interface_is_deleted(iface_rec)) {
int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 9cb099e68..208645504 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -3127,3 +3127,60 @@ OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235: connected'
hv1/ovn-controller.log])
OVN_CLEANUP([hv1])
AT_CLEANUP
+
+AT_SETUP([ovn-controller - I-P different port types])
+AT_KEYWORDS([ovn])
+ovn_start
+
+net_add n1
+sim_add hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.20
+
+check ovn-nbctl ls-add ls0
+check ovn-nbctl lsp-add ls0 vif
+
+ovn-appctl inc-engine/clear-stats
+
+ovs-vsctl -- add-port br-int vif -- \
+ set Interface vif external-ids:iface-id=vif
+wait_row_count Port_Binding 1 logical_port="vif" up=true
+
+ovs-vsctl del-port br-int vif
+wait_row_count Port_Binding 1 logical_port="vif" up=false
+
+ovs-vsctl add-port br-int vif -- \
+ set Interface vif type=dummy -- \
+ set Interface vif external-ids:iface-id=vif
+wait_row_count Port_Binding 1 logical_port="vif" up=true
+
+ovs-vsctl del-port br-int vif
+wait_row_count Port_Binding 1 logical_port="vif" up=false
+
+ovs-vsctl add-port br-int vif -- \
+ set Interface vif type=geneve -- \
+ set Interface vif options:remote_ip=1.1.1.1 external-ids:iface-id=vif
+wait_row_count Port_Binding 1 logical_port="vif" up=true
+
+ovs-vsctl del-port br-int vif
+wait_row_count Port_Binding 1 logical_port="vif" up=false
+
+# Make sure that setting iface in two different transaction doesn't
+# cause recompute.
+ovs-vsctl add-port br-int vif
+ovs-vsctl set Interface vif external-ids:iface-id=vif
+wait_row_count Port_Binding 1 logical_port="vif" up=true
+
+ovs-vsctl del-port br-int vif
+wait_row_count Port_Binding 1 logical_port="vif" up=false
+
+AT_CHECK([ovn-appctl inc-engine/show-stats runtime_data |\
+ sed "s/- compute:\s\+[[0-9]]\+/- compute: ??/"], [0], [dnl
+Node: runtime_data
+- recompute: 0
+- compute: ??
+- cancel: 0
+])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
