Blob Blame History Raw
From 46a4e3bb3a6d01c96721761a0e03d093583ab1cc Mon Sep 17 00:00:00 2001
From: Dumitru Ceara <dceara@redhat.com>
Date: Thu, 21 Jan 2021 13:34:11 +0100
Subject: [PATCH] northd: Fix duplicate logical port detection.

When reconciling SB Port_Bindings and NB Logical_Switch_Ports or
Logical_Router_Ports make sure we properly detect ports that are
incorrectly attached to multiple logical datapaths.

Reported-at: https://bugzilla.redhat.com/1918582
Reported-by: Jianlin Shi <jishi@redhat.com>
Fixes: 8bf9075968ac ("ovn-northd: Fix tunnel_key allocation for SB Port_Bindings.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
(cherry picked from master commit 7b404e68b5b8ea7168b25cb60abb79b5696bcc02)

Change-Id: I09d958cbfe676aad3b370e2a9fec404119c4e6d8
---
 northd/ovn-northd.c | 64 ++++++++++++++++++++++++++++++++++-------------------
 tests/ovn-northd.at | 45 +++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 23 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 49afc2f..f11894a 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -1546,17 +1546,38 @@ ovn_port_destroy(struct hmap *ports, struct ovn_port *port)
     }
 }
 
+/* Returns the ovn_port that matches 'name'.  If 'prefer_bound' is true and
+ * multiple ports share the same name, gives precendence to ports bound to
+ * an ovn_datapath.
+ */
 static struct ovn_port *
-ovn_port_find(const struct hmap *ports, const char *name)
+ovn_port_find__(const struct hmap *ports, const char *name,
+                bool prefer_bound)
 {
+    struct ovn_port *matched_op = NULL;
     struct ovn_port *op;
 
     HMAP_FOR_EACH_WITH_HASH (op, key_node, hash_string(name, 0), ports) {
         if (!strcmp(op->key, name)) {
-            return op;
+            matched_op = op;
+            if (!prefer_bound || op->od) {
+                return op;
+            }
         }
     }
-    return NULL;
+    return matched_op;
+}
+
+static struct ovn_port *
+ovn_port_find(const struct hmap *ports, const char *name)
+{
+    return ovn_port_find__(ports, name, false);
+}
+
+static struct ovn_port *
+ovn_port_find_bound(const struct hmap *ports, const char *name)
+{
+    return ovn_port_find__(ports, name, true);
 }
 
 /* Returns true if the logical switch port 'enabled' column is empty or
@@ -2339,15 +2360,13 @@ join_logical_ports(struct northd_context *ctx,
             for (size_t i = 0; i < od->nbs->n_ports; i++) {
                 const struct nbrec_logical_switch_port *nbsp
                     = od->nbs->ports[i];
-                struct ovn_port *op = ovn_port_find(ports, nbsp->name);
-                if (op && op->sb->datapath == od->sb) {
-                    if (op->nbsp || op->nbrp) {
-                        static struct vlog_rate_limit rl
-                            = VLOG_RATE_LIMIT_INIT(5, 1);
-                        VLOG_WARN_RL(&rl, "duplicate logical port %s",
-                                     nbsp->name);
-                        continue;
-                    }
+                struct ovn_port *op = ovn_port_find_bound(ports, nbsp->name);
+                if (op && (op->od || op->nbsp || op->nbrp)) {
+                    static struct vlog_rate_limit rl
+                        = VLOG_RATE_LIMIT_INIT(5, 1);
+                    VLOG_WARN_RL(&rl, "duplicate logical port %s", nbsp->name);
+                    continue;
+                } else if (op && (!op->sb || op->sb->datapath == od->sb)) {
                     ovn_port_set_nb(op, nbsp, NULL);
                     ovs_list_remove(&op->list);
 
@@ -2438,16 +2457,15 @@ join_logical_ports(struct northd_context *ctx,
                     continue;
                 }
 
-                struct ovn_port *op = ovn_port_find(ports, nbrp->name);
-                if (op && op->sb->datapath == od->sb) {
-                    if (op->nbsp || op->nbrp) {
-                        static struct vlog_rate_limit rl
-                            = VLOG_RATE_LIMIT_INIT(5, 1);
-                        VLOG_WARN_RL(&rl, "duplicate logical router port %s",
-                                     nbrp->name);
-                        destroy_lport_addresses(&lrp_networks);
-                        continue;
-                    }
+                struct ovn_port *op = ovn_port_find_bound(ports, nbrp->name);
+                if (op && (op->od || op->nbsp || op->nbrp)) {
+                    static struct vlog_rate_limit rl
+                        = VLOG_RATE_LIMIT_INIT(5, 1);
+                    VLOG_WARN_RL(&rl, "duplicate logical router port %s",
+                                 nbrp->name);
+                    destroy_lport_addresses(&lrp_networks);
+                    continue;
+                } else if (op && (!op->sb || op->sb->datapath == od->sb)) {
                     ovn_port_set_nb(op, NULL, nbrp);
                     ovs_list_remove(&op->list);
                     ovs_list_push_back(both, &op->list);
@@ -2490,7 +2508,7 @@ join_logical_ports(struct northd_context *ctx,
                     char *redirect_name =
                         ovn_chassis_redirect_name(nbrp->name);
                     struct ovn_port *crp = ovn_port_find(ports, redirect_name);
-                    if (crp && crp->sb->datapath == od->sb) {
+                    if (crp && crp->sb && crp->sb->datapath == od->sb) {
                         crp->derived = true;
                         ovn_port_set_nb(crp, NULL, nbrp);
                         ovs_list_remove(&crp->list);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index df03b6e..d22cad8 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2399,3 +2399,48 @@ ovn-nbctl destroy bfd $uuid
 check_row_count bfd 2
 
 AT_CLEANUP
+
+AT_SETUP([ovn -- check LSP attached to multiple LS])
+ovn_start
+
+check ovn-nbctl ls-add ls1 \
+    -- ls-add ls2 \
+    -- lsp-add ls1 p1
+check ovn-nbctl --wait=sb sync
+
+uuid=$(fetch_column nb:Logical_Switch_Port _uuid name=p1)
+check ovn-nbctl set Logical_Switch ls2 ports=$uuid
+check ovn-nbctl --wait=sb sync
+
+AT_CHECK([grep -qE 'duplicate logical port p1' northd/ovn-northd.log], [0])
+
+AT_CLEANUP
+
+AT_SETUP([ovn -- check LRP attached to multiple LR])
+ovn_start
+
+check ovn-nbctl lr-add lr1 \
+    -- lr-add lr2 \
+    -- lrp-add lr1 p1 00:00:00:00:00:01 10.0.0.1/24
+check ovn-nbctl --wait=sb sync
+
+uuid=$(fetch_column nb:Logical_Router_Port _uuid name=p1)
+check ovn-nbctl set Logical_Router lr2 ports=$uuid
+check ovn-nbctl --wait=sb sync
+
+AT_CHECK([grep -qE 'duplicate logical router port p1' northd/ovn-northd.log], [0])
+
+AT_CLEANUP
+
+AT_SETUP([ovn -- check duplicate LSP/LRP])
+ovn_start
+
+check ovn-nbctl ls-add ls \
+    -- lsp-add ls p1 \
+    -- lr-add lr \
+    -- lrp-add lr p1 00:00:00:00:00:01 10.0.0.1/24
+check ovn-nbctl --wait=sb sync
+
+AT_CHECK([grep -qE 'duplicate logical.*port p1' northd/ovn-northd.log], [0])
+
+AT_CLEANUP
-- 
1.8.3.1