[ovs-dev] [PATCH ovn] ovn-controller: Fix the missing flows when logical router port is added after its peer.

numans at ovn.org numans at ovn.org
Fri Jul 24 19:38:45 UTC 2020


From: Numan Siddique <numans at ovn.org>

When the logical router port is created by CMS after its peer port is created like
below,  ovn-controller doesn't add the logical router to the local_datapaths and
hence misses programming the flows for the router datapath if the logical switch
has logical ports which are already bound. This breaks routing for these logical ports
connected to this router.

ovn-nbctl lsp-add sw0 sw0-lr0
ovn-nbctl lsp-set-type sw0-lr0 router
ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:00:01

ovn-nbctl lr-add lr0
ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:00:01 192.168.1.1/24

This patch fixes this issue.

Fixes: 354bdba51abf("ovn-controller: I-P for SB port binding and OVS interface in runtime_data.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1860053
Signed-off-by: Numan Siddique <numans at ovn.org>
---
 controller/binding.c | 51 ++++++++++++++++++++++-----
 tests/ovn.at         | 82 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+), 8 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index b73abb982c..1936b93e7a 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1522,6 +1522,22 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
     return !any_changes;
 }
 
+static const struct sbrec_port_binding *
+get_peer_lport(const struct sbrec_port_binding *pb,
+               struct binding_ctx_in *b_ctx_in)
+{
+    const char *peer_name = smap_get(&pb->options, "peer");
+    if (strcmp(pb->type, "patch") || !peer_name) {
+        return NULL;
+    }
+
+    const struct sbrec_port_binding *peer;
+    peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
+                                peer_name);
+
+    return (peer && peer->datapath) ? peer : NULL;
+}
+
 /* This function adds the local datapath of the 'peer' of
  * lport 'pb' to the local datapaths if it is not yet added.
  */
@@ -1531,16 +1547,10 @@ add_local_datapath_peer_port(const struct sbrec_port_binding *pb,
                              struct binding_ctx_out *b_ctx_out,
                              struct local_datapath *ld)
 {
-    const char *peer_name = smap_get(&pb->options, "peer");
-    if (strcmp(pb->type, "patch") || !peer_name) {
-        return;
-    }
-
     const struct sbrec_port_binding *peer;
-    peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
-                                peer_name);
+    peer = get_peer_lport(pb, b_ctx_in);
 
-    if (!peer || !peer->datapath) {
+    if (!peer) {
         return;
     }
 
@@ -2118,6 +2128,31 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
         case LP_VTEP:
             update_local_lport_ids(pb, b_ctx_out);
             if (lport_type ==  LP_PATCH) {
+                if (!ld) {
+                    /* If 'ld' for this lport is not present, then check if
+                     * there is a peer for this lport. If peer is present
+                     * and peer's datapath is already in the local datapaths,
+                     * then add this lport's datapath to the local_datapaths.
+                     * */
+                    const struct sbrec_port_binding *peer;
+                    peer = get_peer_lport(pb, b_ctx_in);
+                    struct local_datapath *peer_ld =
+                        get_local_datapath(b_ctx_out->local_datapaths,
+                                           peer->datapath->tunnel_key);
+                    if (peer_ld) {
+                        add_local_datapath(
+                            b_ctx_in->sbrec_datapath_binding_by_key,
+                            b_ctx_in->sbrec_port_binding_by_datapath,
+                            b_ctx_in->sbrec_port_binding_by_name,
+                            pb->datapath, false,
+                            b_ctx_out->local_datapaths,
+                            b_ctx_out->tracked_dp_bindings);
+                    }
+
+                    ld = get_local_datapath(b_ctx_out->local_datapaths,
+                                            pb->datapath->tunnel_key);
+                }
+
                 /* Add the peer datapath to the local datapaths if it's
                  * not present yet.
                  */
diff --git a/tests/ovn.at b/tests/ovn.at
index 7c9e14e2ea..3be62584d2 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -20806,3 +20806,85 @@ AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"])
 
 OVN_CLEANUP([hv1], [hv2])
 AT_CLEANUP
+
+
+AT_SETUP([ovn -- controller I-P handling when lrp added last])
+AT_KEYWORDS([lb])
+
+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
+
+# Step 1. Add OVS interface with external_ids:iface-id set.
+# Step 2. Create the logical switch and logical port.
+# Step 3. Create logical switch port of type router and set the peer.
+# Step 4. Create logical router and the logical router port (peer to logical switch)
+# Step 5. Check that all the flows are added and logical port gets arp reply for
+#         router IP.
+
+as hv1
+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
+ovn-nbctl lsp-add sw0 sw0-p1
+ovn-nbctl lsp-set-addresses sw0-p1 "00:00:00:01:01:02 192.168.1.2"
+
+ovn-nbctl lsp-add sw0 sw0-lr0
+ovn-nbctl lsp-set-type sw0-lr0 router
+ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
+ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:00:01
+
+ovn-nbctl lr-add lr0
+ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:00:01 192.168.1.1/24 aef0:0:0:0:0:0:0:1/64
+
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
+
+sw0_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list datapath_binding sw0)
+lr0_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list datapath_binding lr0)
+
+AT_CHECK([test $(as hv1 ovs-ofctl dump-flows br-int metadata=0x${sw0_dpkey} | wc -l) -gt 80])
+AT_CHECK([test $(as hv1 ovs-ofctl dump-flows br-int metadata=0x${lr0_dpkey} | wc -l) -gt 80])
+
+# test_arp INPORT SHA SPA TPA [REPLY_HA]
+#
+# Causes a packet to be received on INPORT.  The packet is an ARP
+# request with SHA, SPA, and TPA as specified.  If REPLY_HA is provided, then
+# it should be the hardware address of the target to expect to receive in an
+# ARP reply; otherwise no reply is expected.
+#
+# INPORT is an logical switch port number, e.g. 11 for vif11.
+# SHA and REPLY_HA are each 12 hex digits.
+# SPA and TPA are each 8 hex digits.
+test_arp() {
+    local hv=$1 inport=$2 sha=$3 spa=$4 tpa=$5 reply_ha=$6
+    local request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa}
+    as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request
+
+    if test X$reply_ha != X; then
+        # Expect to receive the reply, if any.
+        local reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa}
+        echo $reply >> hv${hv}-vif$inport.expected
+    fi
+}
+
+ip_to_hex() {
+    printf "%02x%02x%02x%02x" "$@"
+}
+
+sw0p1_ip=$(ip_to_hex 192 168 1 2)
+rtr_ip=$(ip_to_hex 192 168 1 1)
+test_arp 1 1 000000010102 $sw0p1_ip $rtr_ip 000000000001
+
+# Now check the packets actually received against the ones expected.
+OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [hv1-vif1.expected])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
-- 
2.26.2



More information about the dev mailing list