diff mbox series

[ovs-dev,v3] controller: Remove OvS iface type check in I-P processing.

Message ID 20240731072234.520882-1-amusil@redhat.com
State Accepted
Headers show
Series [ovs-dev,v3] controller: Remove OvS iface type check in I-P processing. | 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

Ales Musil July 31, 2024, 7:22 a.m. UTC 
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
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Ales Musil <amusil@redhat.com>
---
v3: Rebase on top of current main.
    Add ack from Mark.
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 mbox series

Patch

diff --git a/controller/binding.c b/controller/binding.c
index af21ce5e9..cce58ac4e 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -2642,17 +2642,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 74bff9035..dad4fd353 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -3299,3 +3299,60 @@  wait_ovs_ct_limit_count 0
 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