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