[ovs-dev] [PATCH ovn v2 branch-21.06] Don't suppress localport traffic directed to external port

Ihar Hrachyshka ihrachys at redhat.com
Thu Jul 15 20:33:27 UTC 2021


Recently, we stopped leaking localport traffic through localnet ports
into fabric to avoid unnecessary flipping between chassis hosting the
same localport.

Despite the type name, in some scenarios localports are supposed to
talk outside the hosting chassis. Specifically, in OpenStack [1]
metadata service for SR-IOV ports is implemented as a localport hosted
on another chassis that is exposed to the chassis owning the SR-IOV
port through an "external" port. In this case, "leaking" localport
traffic into fabric is desirable.

This patch inserts a higher priority flow per external port on the
same datapath that avoids dropping localport traffic.

The backport includes custom branch-21.06 specific physical flows
cleanup for localnet ports when an external port is modified / deleted.
This was not needed in master branch because of separated pflow
management.

Fixes: 96959e56d634 ("physical: do not forward traffic from localport
to a localnet one")

[1] https://docs.openstack.org/neutron/latest/admin/ovn/sriov.html

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1974062

Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
Signed-off-by: Numan Siddique <numans at ovn.org>
(cherry picked from commit 1148580290d0ace803f20aeaa0241dd51c100630)

---

v1: initial backport.
v2: properly handle binding updates through inc engine instead of
    forcing full recompute.

new test case
---
 controller/binding.c        | 40 +++++++++++++++--
 controller/binding.h        |  2 +
 controller/ovn-controller.c | 11 +++++
 controller/ovn-controller.h |  2 +
 controller/physical.c       | 66 +++++++++++++++++++++++++---
 controller/physical.h       |  2 +
 tests/ovn.at                | 85 +++++++++++++++++++++++++++++++++++++
 7 files changed, 198 insertions(+), 10 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index 594babc98..ba558efdb 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -22,6 +22,7 @@
 #include "patch.h"
 
 #include "lib/bitmap.h"
+#include "lib/hmapx.h"
 #include "openvswitch/poll-loop.h"
 #include "lib/sset.h"
 #include "lib/util.h"
@@ -108,6 +109,7 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
     hmap_insert(local_datapaths, &ld->hmap_node, dp_key);
     ld->datapath = datapath;
     ld->localnet_port = NULL;
+    shash_init(&ld->external_ports);
     ld->has_local_l3gateway = has_local_l3gateway;
 
     if (tracked_datapaths) {
@@ -474,6 +476,18 @@ is_network_plugged(const struct sbrec_port_binding *binding_rec,
     return network ? !!shash_find_data(bridge_mappings, network) : false;
 }
 
+static void
+update_ld_external_ports(const struct sbrec_port_binding *binding_rec,
+                         struct hmap *local_datapaths)
+{
+    struct local_datapath *ld = get_local_datapath(
+        local_datapaths, binding_rec->datapath->tunnel_key);
+    if (ld) {
+        shash_replace(&ld->external_ports, binding_rec->logical_port,
+                      binding_rec);
+    }
+}
+
 static void
 update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
                         struct shash *bridge_mappings,
@@ -1631,8 +1645,9 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
         !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL;
 
     struct ovs_list localnet_lports = OVS_LIST_INITIALIZER(&localnet_lports);
+    struct ovs_list external_lports = OVS_LIST_INITIALIZER(&external_lports);
 
-    struct localnet_lport {
+    struct lport {
         struct ovs_list list_node;
         const struct sbrec_port_binding *pb;
     };
@@ -1680,11 +1695,14 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
 
         case LP_EXTERNAL:
             consider_external_lport(pb, b_ctx_in, b_ctx_out);
+            struct lport *ext_lport = xmalloc(sizeof *ext_lport);
+            ext_lport->pb = pb;
+            ovs_list_push_back(&external_lports, &ext_lport->list_node);
             break;
 
         case LP_LOCALNET: {
             consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map);
-            struct localnet_lport *lnet_lport = xmalloc(sizeof *lnet_lport);
+            struct lport *lnet_lport = xmalloc(sizeof *lnet_lport);
             lnet_lport->pb = pb;
             ovs_list_push_back(&localnet_lports, &lnet_lport->list_node);
             break;
@@ -1711,7 +1729,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
     /* Run through each localnet lport list to see if it is a localnet port
      * on local datapaths discovered from above loop, and update the
      * corresponding local datapath accordingly. */
-    struct localnet_lport *lnet_lport;
+    struct lport *lnet_lport;
     LIST_FOR_EACH_POP (lnet_lport, list_node, &localnet_lports) {
         update_ld_localnet_port(lnet_lport->pb, &bridge_mappings,
                                 b_ctx_out->egress_ifaces,
@@ -1719,6 +1737,15 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
         free(lnet_lport);
     }
 
+    /* Run through external lport list to see if these are external ports
+     * on local datapaths discovered from above loop, and update the
+     * corresponding local datapath accordingly. */
+    struct lport *ext_lport;
+    LIST_FOR_EACH_POP (ext_lport, list_node, &external_lports) {
+        update_ld_external_ports(ext_lport->pb, b_ctx_out->local_datapaths);
+        free(ext_lport);
+    }
+
     shash_destroy(&bridge_mappings);
 
     if (!sset_is_empty(b_ctx_out->egress_ifaces)
@@ -1921,6 +1948,8 @@ remove_pb_from_local_datapath(const struct sbrec_port_binding *pb,
                                          pb->logical_port)) {
             ld->localnet_port = NULL;
         }
+    } else if (!strcmp(pb->type, "external")) {
+        shash_find_and_delete(&ld->external_ports, pb->logical_port);
     }
 
     if (!strcmp(pb->type, "l3gateway")) {
@@ -2424,6 +2453,9 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
             shash_add(&deleted_virtual_pbs, pb->logical_port, pb);
         } else {
             shash_add(&deleted_other_pbs, pb->logical_port, pb);
+            if (lport_type == LP_EXTERNAL) {
+                hmapx_add(b_ctx_out->extport_updated_datapaths, pb->datapath);
+            }
         }
     }
 
@@ -2578,6 +2610,8 @@ delete_done:
 
         case LP_EXTERNAL:
             handled = consider_external_lport(pb, b_ctx_in, b_ctx_out);
+            update_ld_external_ports(pb, b_ctx_out->local_datapaths);
+            hmapx_add(b_ctx_out->extport_updated_datapaths, pb->datapath);
             break;
 
         case LP_LOCALNET: {
diff --git a/controller/binding.h b/controller/binding.h
index a08011ae2..8fd54092e 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -100,6 +100,8 @@ struct binding_ctx_out {
     struct hmap *tracked_dp_bindings;
 
     struct if_status_mgr *if_mgr;
+
+    struct hmapx *extport_updated_datapaths;
 };
 
 struct local_binding_data {
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 6499e361a..f9f83d37b 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -987,6 +987,9 @@ struct ed_type_runtime_data {
 
     /* CT zone data. Contains datapaths that had updated CT zones */
     struct hmapx ct_updated_datapaths;
+
+    /* Contains datapaths that had updated external ports. */
+    struct hmapx extport_updated_datapaths;
 };
 
 /* struct ed_type_runtime_data has the below members for tracking the
@@ -1079,6 +1082,7 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED,
     hmap_init(&data->tracked_dp_bindings);
 
     hmapx_init(&data->ct_updated_datapaths);
+    hmapx_init(&data->extport_updated_datapaths);
 
     return data;
 }
@@ -1097,12 +1101,14 @@ en_runtime_data_cleanup(void *data)
     HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node,
                         &rt_data->local_datapaths) {
         free(cur_node->peer_ports);
+        shash_destroy(&cur_node->external_ports);
         hmap_remove(&rt_data->local_datapaths, &cur_node->hmap_node);
         free(cur_node);
     }
     hmap_destroy(&rt_data->local_datapaths);
     local_binding_data_destroy(&rt_data->lbinding_data);
     hmapx_destroy(&rt_data->ct_updated_datapaths);
+    hmapx_destroy(&rt_data->extport_updated_datapaths);
 }
 
 static void
@@ -1190,6 +1196,7 @@ init_binding_ctx(struct engine_node *node,
     b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
     b_ctx_out->tracked_dp_bindings = NULL;
     b_ctx_out->if_mgr = ctrl_ctx->if_mgr;
+    b_ctx_out->extport_updated_datapaths = &rt_data->extport_updated_datapaths;
 }
 
 static void
@@ -1208,6 +1215,7 @@ en_runtime_data_run(struct engine_node *node, void *data)
         struct local_datapath *cur_node, *next_node;
         HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, local_datapaths) {
             free(cur_node->peer_ports);
+            shash_destroy(&cur_node->external_ports);
             hmap_remove(local_datapaths, &cur_node->hmap_node);
             free(cur_node);
         }
@@ -1225,6 +1233,7 @@ en_runtime_data_run(struct engine_node *node, void *data)
         smap_init(&rt_data->local_iface_ids);
         local_binding_data_init(&rt_data->lbinding_data);
         hmapx_clear(&rt_data->ct_updated_datapaths);
+        hmapx_clear(&rt_data->extport_updated_datapaths);
     }
 
     struct binding_ctx_in b_ctx_in;
@@ -1963,6 +1972,7 @@ static void init_physical_ctx(struct engine_node *node,
     p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve;
     p_ctx->local_bindings = &rt_data->lbinding_data.bindings;
     p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths;
+    p_ctx->extport_updated_datapaths = &rt_data->extport_updated_datapaths;
 }
 
 static void init_lflow_ctx(struct engine_node *node,
@@ -2414,6 +2424,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data)
         /* This indicates that we need to recompute the physical flows. */
         physical_clear_unassoc_flows_with_db(&fo->flow_table);
         physical_clear_dp_flows(&p_ctx, &rt_data->ct_updated_datapaths,
+                                &rt_data->extport_updated_datapaths,
                                 &fo->flow_table);
         physical_run(&p_ctx, &fo->flow_table);
         return true;
diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
index 5d9466880..2bf1fecbf 100644
--- a/controller/ovn-controller.h
+++ b/controller/ovn-controller.h
@@ -67,6 +67,8 @@ struct local_datapath {
 
     size_t n_peer_ports;
     size_t n_allocated_peer_ports;
+
+    struct shash external_ports;
 };
 
 struct local_datapath *get_local_datapath(const struct hmap *,
diff --git a/controller/physical.c b/controller/physical.c
index 018e09540..cfd68f835 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1272,6 +1272,52 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
             ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160,
                             binding->header_.uuid.parts[0], &match,
                             ofpacts_p, &binding->header_.uuid);
+
+            /* localport traffic directed to external is *not* local */
+            struct shash_node *node;
+            SHASH_FOR_EACH (node, &ld->external_ports) {
+                const struct sbrec_port_binding *pb = node->data;
+
+                /* skip ports that are not claimed by this chassis */
+                if (!pb->chassis) {
+                    continue;
+                }
+                if (strcmp(pb->chassis->name, chassis->name)) {
+                    continue;
+                }
+
+                ofpbuf_clear(ofpacts_p);
+                for (int i = 0; i < MFF_N_LOG_REGS; i++) {
+                    put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p);
+                }
+                put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p);
+
+                /* allow traffic directed to external MAC address */
+                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+                for (int i = 0; i < pb->n_mac; i++) {
+                    char *err_str;
+                    struct eth_addr peer_mac;
+                    if ((err_str = str_to_mac(pb->mac[i], &peer_mac))) {
+                        VLOG_WARN_RL(
+                            &rl, "Parsing MAC failed for external port: %s, "
+                                 "with error: %s", pb->logical_port, err_str);
+                        free(err_str);
+                        continue;
+                    }
+
+                    match_init_catchall(&match);
+                    match_set_metadata(&match, htonll(dp_key));
+                    match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0,
+                                  port_key);
+                    match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
+                                         MLF_LOCALPORT, MLF_LOCALPORT);
+                    match_set_dl_dst(&match, peer_mac);
+
+                    ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 170,
+                                    binding->header_.uuid.parts[0], &match,
+                                    ofpacts_p, &binding->header_.uuid);
+                }
+            }
         }
 
     } else if (!tun && !is_ha_remote) {
@@ -1957,18 +2003,24 @@ physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *flow_table)
 void
 physical_clear_dp_flows(struct physical_ctx *p_ctx,
                         struct hmapx *ct_updated_datapaths,
+                        struct hmapx *extport_updated_datapaths,
                         struct ovn_desired_flow_table *flow_table)
 {
     const struct sbrec_port_binding *binding;
     SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, p_ctx->port_binding_table) {
-        if (!hmapx_find(ct_updated_datapaths, binding->datapath)) {
-            continue;
+        if (hmapx_find(ct_updated_datapaths, binding->datapath)) {
+            const struct sbrec_port_binding *peer =
+                get_binding_peer(p_ctx->sbrec_port_binding_by_name, binding);
+            ofctrl_remove_flows(flow_table, &binding->header_.uuid);
+            if (peer) {
+                ofctrl_remove_flows(flow_table, &peer->header_.uuid);
+            }
         }
-        const struct sbrec_port_binding *peer =
-            get_binding_peer(p_ctx->sbrec_port_binding_by_name, binding);
-        ofctrl_remove_flows(flow_table, &binding->header_.uuid);
-        if (peer) {
-            ofctrl_remove_flows(flow_table, &peer->header_.uuid);
+
+        if (hmapx_find(extport_updated_datapaths, binding->datapath)) {
+            if (!strcmp(binding->type, "localnet")) {
+                ofctrl_remove_flows(flow_table, &binding->header_.uuid);
+            }
         }
     }
 }
diff --git a/controller/physical.h b/controller/physical.h
index 0bf13f268..f9cd883a7 100644
--- a/controller/physical.h
+++ b/controller/physical.h
@@ -57,6 +57,7 @@ struct physical_ctx {
     enum mf_field_id mff_ovn_geneve;
     struct shash *local_bindings;
     struct hmapx *ct_updated_datapaths;
+    struct hmapx *extport_updated_datapaths;
 };
 
 void physical_register_ovs_idl(struct ovsdb_idl *);
@@ -65,6 +66,7 @@ void physical_run(struct physical_ctx *,
 void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *);
 void physical_clear_dp_flows(struct physical_ctx *p_ctx,
                              struct hmapx *ct_updated_datapaths,
+                             struct hmapx *extport_updated_datapaths,
                              struct ovn_desired_flow_table *flow_table);
 void physical_handle_port_binding_changes(struct physical_ctx *,
                                           struct ovn_desired_flow_table *);
diff --git a/tests/ovn.at b/tests/ovn.at
index a2ea59721..515bcf7c7 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -12136,6 +12136,91 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([localport doesn't suppress ARP directed to external port])
+
+ovn_start
+net_add n1
+
+check ovs-vsctl add-br br-phys
+check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+check ovn-nbctl ls-add ls
+
+# create topology to allow to talk from localport through localnet to external port
+check ovn-nbctl lsp-add ls lp
+check ovn-nbctl lsp-set-addresses lp "00:00:00:00:00:01 10.0.0.1"
+check ovn-nbctl lsp-set-type lp localport
+check ovs-vsctl add-port br-int lp -- set Interface lp external-ids:iface-id=lp
+
+check ovn-nbctl --wait=sb ha-chassis-group-add hagrp
+check ovn-nbctl --wait=sb ha-chassis-group-add-chassis hagrp main 10
+check ovn-nbctl lsp-add ls lext
+check ovn-nbctl lsp-set-addresses lext "00:00:00:00:00:02 10.0.0.2"
+check ovn-nbctl lsp-set-type lext external
+hagrp_uuid=`ovn-nbctl --bare --columns _uuid find ha_chassis_group name=hagrp`
+check ovn-nbctl set logical_switch_port lext ha_chassis_group=$hagrp_uuid
+
+check ovn-nbctl lsp-add ls ln
+check ovn-nbctl lsp-set-addresses ln unknown
+check ovn-nbctl lsp-set-type ln localnet
+check ovn-nbctl lsp-set-options ln network_name=phys
+check ovn-nbctl --wait=hv sync
+
+# also create second external port AFTER localnet to check that order is irrelevant
+check ovn-nbctl lsp-add ls lext2
+check ovn-nbctl lsp-set-addresses lext2 "00:00:00:00:00:10 10.0.0.10"
+check ovn-nbctl lsp-set-type lext2 external
+check ovn-nbctl set logical_switch_port lext2 ha_chassis_group=$hagrp_uuid
+check ovn-nbctl --wait=hv sync
+
+# create and immediately delete an external port to later check that flows for
+# deleted ports are not left over in flow table
+check ovn-nbctl lsp-add ls lext-deleted
+check ovn-nbctl lsp-set-addresses lext-deleted "00:00:00:00:00:03 10.0.0.3"
+check ovn-nbctl lsp-set-type lext-deleted external
+check ovn-nbctl set logical_switch_port lext-deleted ha_chassis_group=$hagrp_uuid
+check ovn-nbctl --wait=hv sync
+check ovn-nbctl lsp-del lext-deleted
+check ovn-nbctl --wait=hv sync
+
+send_garp() {
+    local inport=$1 eth_src=$2 eth_dst=$3 spa=$4 tpa=$5
+    local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
+    ovs-appctl netdev-dummy/receive $inport $request
+}
+
+spa=$(ip_to_hex 10 0 0 1)
+tpa=$(ip_to_hex 10 0 0 2)
+send_garp lp 000000000001 000000000002 $spa $tpa
+
+spa=$(ip_to_hex 10 0 0 1)
+tpa=$(ip_to_hex 10 0 0 10)
+send_garp lp 000000000001 000000000010 $spa $tpa
+
+spa=$(ip_to_hex 10 0 0 1)
+tpa=$(ip_to_hex 10 0 0 3)
+send_garp lp 000000000001 000000000003 $spa $tpa
+
+dnl external traffic from localport should be sent to localnet
+AT_CHECK([tcpdump -r main/br-phys_n1-tx.pcap arp[[24:4]]=0x0a000002 | wc -l],[0],[dnl
+1
+],[ignore])
+
+#dnl ...regardless of localnet / external ports creation order
+AT_CHECK([tcpdump -r main/br-phys_n1-tx.pcap arp[[24:4]]=0x0a00000a | wc -l],[0],[dnl
+1
+],[ignore])
+
+dnl traffic from localport should not be sent to deleted external port
+AT_CHECK([tcpdump -r main/br-phys_n1-tx.pcap arp[[24:4]]=0x0a000003 | wc -l],[0],[dnl
+0
+],[ignore])
+
+AT_CLEANUP
+])
+
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([ovn -- 1 LR with HA distributed router gateway port])
 ovn_start
-- 
2.31.1



More information about the dev mailing list