From 46a4e3bb3a6d01c96721761a0e03d093583ab1cc Mon Sep 17 00:00:00 2001 From: Dumitru Ceara 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 Fixes: 8bf9075968ac ("ovn-northd: Fix tunnel_key allocation for SB Port_Bindings.") Signed-off-by: Dumitru Ceara Signed-off-by: Numan Siddique (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