5f9769
From 46a4e3bb3a6d01c96721761a0e03d093583ab1cc Mon Sep 17 00:00:00 2001
5f9769
From: Dumitru Ceara <dceara@redhat.com>
5f9769
Date: Thu, 21 Jan 2021 13:34:11 +0100
5f9769
Subject: [PATCH] northd: Fix duplicate logical port detection.
5f9769
5f9769
When reconciling SB Port_Bindings and NB Logical_Switch_Ports or
5f9769
Logical_Router_Ports make sure we properly detect ports that are
5f9769
incorrectly attached to multiple logical datapaths.
5f9769
5f9769
Reported-at: https://bugzilla.redhat.com/1918582
5f9769
Reported-by: Jianlin Shi <jishi@redhat.com>
5f9769
Fixes: 8bf9075968ac ("ovn-northd: Fix tunnel_key allocation for SB Port_Bindings.")
5f9769
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
5f9769
Signed-off-by: Numan Siddique <numans@ovn.org>
5f9769
(cherry picked from master commit 7b404e68b5b8ea7168b25cb60abb79b5696bcc02)
5f9769
5f9769
Change-Id: I09d958cbfe676aad3b370e2a9fec404119c4e6d8
5f9769
---
5f9769
 northd/ovn-northd.c | 64 ++++++++++++++++++++++++++++++++++-------------------
5f9769
 tests/ovn-northd.at | 45 +++++++++++++++++++++++++++++++++++++
5f9769
 2 files changed, 86 insertions(+), 23 deletions(-)
5f9769
5f9769
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
5f9769
index 49afc2f..f11894a 100644
5f9769
--- a/northd/ovn-northd.c
5f9769
+++ b/northd/ovn-northd.c
5f9769
@@ -1546,17 +1546,38 @@ ovn_port_destroy(struct hmap *ports, struct ovn_port *port)
5f9769
     }
5f9769
 }
5f9769
 
5f9769
+/* Returns the ovn_port that matches 'name'.  If 'prefer_bound' is true and
5f9769
+ * multiple ports share the same name, gives precendence to ports bound to
5f9769
+ * an ovn_datapath.
5f9769
+ */
5f9769
 static struct ovn_port *
5f9769
-ovn_port_find(const struct hmap *ports, const char *name)
5f9769
+ovn_port_find__(const struct hmap *ports, const char *name,
5f9769
+                bool prefer_bound)
5f9769
 {
5f9769
+    struct ovn_port *matched_op = NULL;
5f9769
     struct ovn_port *op;
5f9769
 
5f9769
     HMAP_FOR_EACH_WITH_HASH (op, key_node, hash_string(name, 0), ports) {
5f9769
         if (!strcmp(op->key, name)) {
5f9769
-            return op;
5f9769
+            matched_op = op;
5f9769
+            if (!prefer_bound || op->od) {
5f9769
+                return op;
5f9769
+            }
5f9769
         }
5f9769
     }
5f9769
-    return NULL;
5f9769
+    return matched_op;
5f9769
+}
5f9769
+
5f9769
+static struct ovn_port *
5f9769
+ovn_port_find(const struct hmap *ports, const char *name)
5f9769
+{
5f9769
+    return ovn_port_find__(ports, name, false);
5f9769
+}
5f9769
+
5f9769
+static struct ovn_port *
5f9769
+ovn_port_find_bound(const struct hmap *ports, const char *name)
5f9769
+{
5f9769
+    return ovn_port_find__(ports, name, true);
5f9769
 }
5f9769
 
5f9769
 /* Returns true if the logical switch port 'enabled' column is empty or
5f9769
@@ -2339,15 +2360,13 @@ join_logical_ports(struct northd_context *ctx,
5f9769
             for (size_t i = 0; i < od->nbs->n_ports; i++) {
5f9769
                 const struct nbrec_logical_switch_port *nbsp
5f9769
                     = od->nbs->ports[i];
5f9769
-                struct ovn_port *op = ovn_port_find(ports, nbsp->name);
5f9769
-                if (op && op->sb->datapath == od->sb) {
5f9769
-                    if (op->nbsp || op->nbrp) {
5f9769
-                        static struct vlog_rate_limit rl
5f9769
-                            = VLOG_RATE_LIMIT_INIT(5, 1);
5f9769
-                        VLOG_WARN_RL(&rl, "duplicate logical port %s",
5f9769
-                                     nbsp->name);
5f9769
-                        continue;
5f9769
-                    }
5f9769
+                struct ovn_port *op = ovn_port_find_bound(ports, nbsp->name);
5f9769
+                if (op && (op->od || op->nbsp || op->nbrp)) {
5f9769
+                    static struct vlog_rate_limit rl
5f9769
+                        = VLOG_RATE_LIMIT_INIT(5, 1);
5f9769
+                    VLOG_WARN_RL(&rl, "duplicate logical port %s", nbsp->name);
5f9769
+                    continue;
5f9769
+                } else if (op && (!op->sb || op->sb->datapath == od->sb)) {
5f9769
                     ovn_port_set_nb(op, nbsp, NULL);
5f9769
                     ovs_list_remove(&op->list);
5f9769
 
5f9769
@@ -2438,16 +2457,15 @@ join_logical_ports(struct northd_context *ctx,
5f9769
                     continue;
5f9769
                 }
5f9769
 
5f9769
-                struct ovn_port *op = ovn_port_find(ports, nbrp->name);
5f9769
-                if (op && op->sb->datapath == od->sb) {
5f9769
-                    if (op->nbsp || op->nbrp) {
5f9769
-                        static struct vlog_rate_limit rl
5f9769
-                            = VLOG_RATE_LIMIT_INIT(5, 1);
5f9769
-                        VLOG_WARN_RL(&rl, "duplicate logical router port %s",
5f9769
-                                     nbrp->name);
5f9769
-                        destroy_lport_addresses(&lrp_networks);
5f9769
-                        continue;
5f9769
-                    }
5f9769
+                struct ovn_port *op = ovn_port_find_bound(ports, nbrp->name);
5f9769
+                if (op && (op->od || op->nbsp || op->nbrp)) {
5f9769
+                    static struct vlog_rate_limit rl
5f9769
+                        = VLOG_RATE_LIMIT_INIT(5, 1);
5f9769
+                    VLOG_WARN_RL(&rl, "duplicate logical router port %s",
5f9769
+                                 nbrp->name);
5f9769
+                    destroy_lport_addresses(&lrp_networks);
5f9769
+                    continue;
5f9769
+                } else if (op && (!op->sb || op->sb->datapath == od->sb)) {
5f9769
                     ovn_port_set_nb(op, NULL, nbrp);
5f9769
                     ovs_list_remove(&op->list);
5f9769
                     ovs_list_push_back(both, &op->list);
5f9769
@@ -2490,7 +2508,7 @@ join_logical_ports(struct northd_context *ctx,
5f9769
                     char *redirect_name =
5f9769
                         ovn_chassis_redirect_name(nbrp->name);
5f9769
                     struct ovn_port *crp = ovn_port_find(ports, redirect_name);
5f9769
-                    if (crp && crp->sb->datapath == od->sb) {
5f9769
+                    if (crp && crp->sb && crp->sb->datapath == od->sb) {
5f9769
                         crp->derived = true;
5f9769
                         ovn_port_set_nb(crp, NULL, nbrp);
5f9769
                         ovs_list_remove(&crp->list);
5f9769
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
5f9769
index df03b6e..d22cad8 100644
5f9769
--- a/tests/ovn-northd.at
5f9769
+++ b/tests/ovn-northd.at
5f9769
@@ -2399,3 +2399,48 @@ ovn-nbctl destroy bfd $uuid
5f9769
 check_row_count bfd 2
5f9769
 
5f9769
 AT_CLEANUP
5f9769
+
5f9769
+AT_SETUP([ovn -- check LSP attached to multiple LS])
5f9769
+ovn_start
5f9769
+
5f9769
+check ovn-nbctl ls-add ls1 \
5f9769
+    -- ls-add ls2 \
5f9769
+    -- lsp-add ls1 p1
5f9769
+check ovn-nbctl --wait=sb sync
5f9769
+
5f9769
+uuid=$(fetch_column nb:Logical_Switch_Port _uuid name=p1)
5f9769
+check ovn-nbctl set Logical_Switch ls2 ports=$uuid
5f9769
+check ovn-nbctl --wait=sb sync
5f9769
+
5f9769
+AT_CHECK([grep -qE 'duplicate logical port p1' northd/ovn-northd.log], [0])
5f9769
+
5f9769
+AT_CLEANUP
5f9769
+
5f9769
+AT_SETUP([ovn -- check LRP attached to multiple LR])
5f9769
+ovn_start
5f9769
+
5f9769
+check ovn-nbctl lr-add lr1 \
5f9769
+    -- lr-add lr2 \
5f9769
+    -- lrp-add lr1 p1 00:00:00:00:00:01 10.0.0.1/24
5f9769
+check ovn-nbctl --wait=sb sync
5f9769
+
5f9769
+uuid=$(fetch_column nb:Logical_Router_Port _uuid name=p1)
5f9769
+check ovn-nbctl set Logical_Router lr2 ports=$uuid
5f9769
+check ovn-nbctl --wait=sb sync
5f9769
+
5f9769
+AT_CHECK([grep -qE 'duplicate logical router port p1' northd/ovn-northd.log], [0])
5f9769
+
5f9769
+AT_CLEANUP
5f9769
+
5f9769
+AT_SETUP([ovn -- check duplicate LSP/LRP])
5f9769
+ovn_start
5f9769
+
5f9769
+check ovn-nbctl ls-add ls \
5f9769
+    -- lsp-add ls p1 \
5f9769
+    -- lr-add lr \
5f9769
+    -- lrp-add lr p1 00:00:00:00:00:01 10.0.0.1/24
5f9769
+check ovn-nbctl --wait=sb sync
5f9769
+
5f9769
+AT_CHECK([grep -qE 'duplicate logical.*port p1' northd/ovn-northd.log], [0])
5f9769
+
5f9769
+AT_CLEANUP
5f9769
-- 
5f9769
1.8.3.1
5f9769