controller: Delete flows on port delete/add.

When a logical_port is deleted and added back, in two sb transactions being handled within one ovn-controller loop, some flows belonging to the deleted pb (such as flows in OFTABLE_CHK_IN_PORT_SEC) were not properly deleted. Reported-at: https://issues.redhat.com/browse/FDP-873 Signed-off-by: Xavier Simonart <[email protected]> Acked-by: Ales Musil <[email protected]> Signed-off-by: Numan Siddique <[email protected]>
ovn-org · Nov 14, 2024 · 185849b · 185849b
1 parent 1bcae35
commit 185849b
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 6 deletions.
diff --git a/controller/lflow.c b/controller/lflow.c
@@ -2269,7 +2269,8 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
 bool
 lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
                              struct lflow_ctx_in *l_ctx_in,
-                             struct lflow_ctx_out *l_ctx_out)
+                             struct lflow_ctx_out *l_ctx_out,
+                             bool deleted)
 {
     bool changed;
 
@@ -2290,6 +2291,9 @@ lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
      * port binding'uuid', then this function should handle it properly.
      */
     ofctrl_remove_flows(l_ctx_out->flow_table, &pb->header_.uuid);
+    if (deleted) {
+        return true;
+    }
 
     if (pb->n_port_security && shash_find(l_ctx_in->binding_lports,
                                           pb->logical_port)) {

diff --git a/controller/lflow.h b/controller/lflow.h
@@ -190,7 +190,8 @@ bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *,
                                   struct lflow_ctx_out *);
 bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
                                   struct lflow_ctx_in *,
-                                  struct lflow_ctx_out *);
+                                  struct lflow_ctx_out *,
+                                  bool deleted);
 bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
                                     struct lflow_ctx_out *);
 bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,

diff --git a/controller/local_data.c b/controller/local_data.c
@@ -378,15 +378,37 @@ tracked_datapath_lport_add(const struct sbrec_port_binding *pb,
     }
 
     /* Check if the lport is already present or not.
-     * If it is already present, then just update the 'pb' field. */
+     * If it is already present, then check whether it is the same pb.
+     * We might have two different pb with the same logical_port if it was
+     * deleted and added back within the same loop.
+     * If the same pb was already present, just update the 'pb' field.
+     * Otherwise, add the second pb */
     struct tracked_lport *lport =
         shash_find_data(&tracked_dp->lports, pb->logical_port);
 
     if (!lport) {
         lport = xmalloc(sizeof *lport);
         shash_add(&tracked_dp->lports, pb->logical_port, lport);
+    } else if (pb != lport->pb) {
+        bool found = false;
+        /* There is at least another pb with the same logical_port.
+         * However, our pb might already be shash_added (e.g. pb1 deleted, pb2
+         * added, pb2 deleted). This is not really optimal, but this loop
+         * only runs in a very uncommon race condition (same logical port
+         * deleted and added within same loop */
+        struct shash_node *node;
+        SHASH_FOR_EACH (node, &tracked_dp->lports) {
+            lport = (struct tracked_lport *) node->data;
+            if (lport->pb == pb) {
+                found = true;
+                break;
+            }
+        }
+        if (!found) {
+            lport = xmalloc(sizeof *lport);
+            shash_add(&tracked_dp->lports, pb->logical_port, lport);
+        }
     }
-
     lport->pb = pb;
     lport->tracked_type = tracked_type;
 }

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
@@ -4271,8 +4271,9 @@ lflow_output_runtime_data_handler(struct engine_node *node,
         struct shash_node *shash_node;
         SHASH_FOR_EACH (shash_node, &tdp->lports) {
             struct tracked_lport *lport = shash_node->data;
-            if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in,
-                                                &l_ctx_out)) {
+            if (!lflow_handle_flows_for_lport(
+                    lport->pb, &l_ctx_in, &l_ctx_out,
+                    lport->tracked_type == TRACKED_RESOURCE_REMOVED)) {
                 return false;
             }
         }

diff --git a/tests/ovn.at b/tests/ovn.at
@@ -39686,3 +39686,44 @@ OVN_CLEANUP([hv1])
 
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([Port Deleted and added back])
+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
+ovn-appctl vlog/set dbg
+check 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
+
+check 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 2001::3"
+check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 2001::3"
+
+OVN_POPULATE_ARP
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+# Delete sw0-p1
+sleep_controller hv1
+check ovn-nbctl --wait=sb lsp-del sw0-p1
+
+# Add back sw0-p1 but without any address set.
+check ovn-nbctl --wait=sb lsp-add sw0 sw0-p1
+wake_up_controller hv1
+
+CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1])
+
+OVN_CLEANUP([hv1])
+
+AT_CLEANUP
+])