From patchwork Mon Oct 26 18:16:25 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 1387971 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.133; helo=hemlock.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4CKjjB32stz9sSW for ; Tue, 27 Oct 2020 05:17:02 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id C92078729A; Mon, 26 Oct 2020 18:17:00 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pt8SCsr5c6u2; Mon, 26 Oct 2020 18:16:57 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 07C2987275; Mon, 26 Oct 2020 18:16:57 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id C41FCC1DA0; Mon, 26 Oct 2020 18:16:56 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 80D22C1AE0 for ; Mon, 26 Oct 2020 18:16:54 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 610542E14B for ; Mon, 26 Oct 2020 18:16:54 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id tl1N0TIIxqrX for ; Mon, 26 Oct 2020 18:16:41 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by silver.osuosl.org (Postfix) with ESMTPS id EF6552E140 for ; Mon, 26 Oct 2020 18:16:40 +0000 (UTC) Received: from sigfpe.attlocal.net (75-54-222-30.lightspeed.rdcyca.sbcglobal.net [75.54.222.30]) (Authenticated sender: blp@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 7C5AE10000B; Mon, 26 Oct 2020 18:16:38 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Mon, 26 Oct 2020 11:16:25 -0700 Message-Id: <20201026181626.1827014-6-blp@ovn.org> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20201026181626.1827014-1-blp@ovn.org> References: <20201026181626.1827014-1-blp@ovn.org> MIME-Version: 1.0 Cc: Ben Pfaff Subject: [ovs-dev] [PATCH ovn 6/7] northd: Enhance implementation of datapath tunnel key requests. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" When a tunnel key was requested, the implementation was not smart enough to reassign a datapath that had been automatically assigned the same key. This fixes the problem and adds a test. I didn't see a reason not to make this feature available for logical routers so I added that feature too. Signed-off-by: Ben Pfaff Acked-by: Numan Siddique --- lib/ovn-util.c | 26 ++++------ lib/ovn-util.h | 3 +- northd/ovn-northd.c | 124 ++++++++++++++++++++++++-------------------- ovn-nb.xml | 8 +++ tests/ovn-northd.at | 46 ++++++++++++++++ 5 files changed, 135 insertions(+), 72 deletions(-) diff --git a/lib/ovn-util.c b/lib/ovn-util.c index 1daf6650379f..321fc89e275a 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -517,24 +517,21 @@ ovn_destroy_tnlids(struct hmap *tnlids) hmap_destroy(tnlids); } -void -ovn_add_tnlid(struct hmap *set, uint32_t tnlid) -{ - struct tnlid_node *node = xmalloc(sizeof *node); - hmap_insert(set, &node->hmap_node, hash_int(tnlid, 0)); - node->tnlid = tnlid; -} - bool -ovn_tnlid_in_use(const struct hmap *set, uint32_t tnlid) +ovn_add_tnlid(struct hmap *set, uint32_t tnlid) { - const struct tnlid_node *node; - HMAP_FOR_EACH_IN_BUCKET (node, hmap_node, hash_int(tnlid, 0), set) { + uint32_t hash = hash_int(tnlid, 0); + struct tnlid_node *node; + HMAP_FOR_EACH_IN_BUCKET (node, hmap_node, hash, set) { if (node->tnlid == tnlid) { - return true; + return false; } } - return false; + + node = xmalloc(sizeof *node); + hmap_insert(set, &node->hmap_node, hash); + node->tnlid = tnlid; + return true; } static uint32_t @@ -549,8 +546,7 @@ ovn_allocate_tnlid(struct hmap *set, const char *name, uint32_t min, { for (uint32_t tnlid = next_tnlid(*hint, min, max); tnlid != *hint; tnlid = next_tnlid(tnlid, min, max)) { - if (!ovn_tnlid_in_use(set, tnlid)) { - ovn_add_tnlid(set, tnlid); + if (ovn_add_tnlid(set, tnlid)) { *hint = tnlid; return tnlid; } diff --git a/lib/ovn-util.h b/lib/ovn-util.h index 1d22a691f56f..6162f30225b3 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -114,8 +114,7 @@ void ovn_conn_show(struct unixctl_conn *conn, int argc OVS_UNUSED, struct hmap; void ovn_destroy_tnlids(struct hmap *tnlids); -void ovn_add_tnlid(struct hmap *set, uint32_t tnlid); -bool ovn_tnlid_in_use(const struct hmap *set, uint32_t tnlid); +bool ovn_add_tnlid(struct hmap *set, uint32_t tnlid); uint32_t ovn_allocate_tnlid(struct hmap *set, const char *name, uint32_t min, uint32_t max, uint32_t *hint); diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index b96e0db516be..92d578c405a2 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -596,6 +596,8 @@ struct ovn_datapath { struct ovs_list list; /* In list of similar records. */ + uint32_t tunnel_key; + /* Logical switch data. */ struct ovn_port **router_ports; size_t n_router_ports; @@ -1296,12 +1298,45 @@ get_ovn_max_dp_key_local(struct northd_context *ctx) return OVN_MAX_DP_KEY - OVN_MAX_DP_GLOBAL_NUM; } -static uint32_t -ovn_datapath_allocate_key(struct northd_context *ctx, struct hmap *dp_tnlids) +static void +ovn_datapath_allocate_key(struct northd_context *ctx, + struct hmap *datapaths, struct hmap *dp_tnlids, + struct ovn_datapath *od, uint32_t *hint) +{ + if (!od->tunnel_key) { + od->tunnel_key = ovn_allocate_tnlid(dp_tnlids, "datapath", + OVN_MIN_DP_KEY_LOCAL, + get_ovn_max_dp_key_local(ctx), + hint); + if (!od->tunnel_key) { + if (od->sb) { + sbrec_datapath_binding_delete(od->sb); + } + ovs_list_remove(&od->list); + ovn_datapath_destroy(datapaths, od); + } + } +} + +static void +ovn_datapath_assign_requested_tnl_id(struct hmap *dp_tnlids, + struct ovn_datapath *od) { - static uint32_t hint; - return ovn_allocate_tnlid(dp_tnlids, "datapath", OVN_MIN_DP_KEY_LOCAL, - get_ovn_max_dp_key_local(ctx), &hint); + const struct smap *other_config = (od->nbs + ? &od->nbs->other_config + : &od->nbr->options); + uint32_t tunnel_key = smap_get_int(other_config, "requested-tnl-key", 0); + if (tunnel_key) { + if (ovn_add_tnlid(dp_tnlids, tunnel_key)) { + od->tunnel_key = tunnel_key; + } else { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "Logical %s %s requests same tunnel key " + "%"PRIu32" as another logical switch or router", + od->nbs ? "switch" : "router", od->nbs->name, + tunnel_key); + } + } } /* Updates the southbound Datapath_Binding table so that it contains the @@ -1317,65 +1352,44 @@ build_datapaths(struct northd_context *ctx, struct hmap *datapaths, join_datapaths(ctx, datapaths, &sb_only, &nb_only, &both, lr_list); - /* First index the in-use datapath tunnel IDs. */ + /* Assign explicitly requested tunnel ids first. */ struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids); struct ovn_datapath *od, *next; - if (!ovs_list_is_empty(&nb_only) || !ovs_list_is_empty(&both)) { - LIST_FOR_EACH (od, list, &both) { - ovn_add_tnlid(&dp_tnlids, od->sb->tunnel_key); - } + LIST_FOR_EACH (od, list, &both) { + ovn_datapath_assign_requested_tnl_id(&dp_tnlids, od); } - - /* Add southbound record for each unmatched northbound record. */ LIST_FOR_EACH (od, list, &nb_only) { - int64_t tunnel_key = 0; - if (od->nbs) { - tunnel_key = smap_get_int(&od->nbs->other_config, - "requested-tnl-key", - 0); - if (tunnel_key && ovn_tnlid_in_use(&dp_tnlids, tunnel_key)) { - static struct vlog_rate_limit rl = - VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "Cannot create datapath binding for " - "logical switch %s due to duplicate key set " - "in other_config:requested-tnl-key: %"PRId64, - od->nbs->name, tunnel_key); - continue; - } - } - if (!tunnel_key) { - tunnel_key = ovn_datapath_allocate_key(ctx, &dp_tnlids); - if (!tunnel_key) { - break; - } + ovn_datapath_assign_requested_tnl_id(&dp_tnlids, od); + } + + /* Keep nonconflicting tunnel IDs that are already assigned. */ + LIST_FOR_EACH (od, list, &both) { + if (!od->tunnel_key && ovn_add_tnlid(&dp_tnlids, od->sb->tunnel_key)) { + od->tunnel_key = od->sb->tunnel_key; } + } - od->sb = sbrec_datapath_binding_insert(ctx->ovnsb_txn); - ovn_datapath_update_external_ids(od); - sbrec_datapath_binding_set_tunnel_key(od->sb, tunnel_key); + /* Assign new tunnel ids where needed. */ + uint32_t hint = 0; + LIST_FOR_EACH_SAFE (od, next, list, &both) { + ovn_datapath_allocate_key(ctx, datapaths, &dp_tnlids, od, &hint); + } + LIST_FOR_EACH_SAFE (od, next, list, &nb_only) { + ovn_datapath_allocate_key(ctx, datapaths, &dp_tnlids, od, &hint); } - /* Sync from northbound to southbound record for od existed in both. */ + /* Sync tunnel ids from nb to sb. */ LIST_FOR_EACH (od, list, &both) { - if (od->nbs) { - int64_t tunnel_key = smap_get_int(&od->nbs->other_config, - "requested-tnl-key", - 0); - if (tunnel_key && tunnel_key != od->sb->tunnel_key) { - if (ovn_tnlid_in_use(&dp_tnlids, tunnel_key)) { - static struct vlog_rate_limit rl = - VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "Cannot update datapath binding key for " - "logical switch %s due to duplicate key set " - "in other_config:requested-tnl-key: %"PRId64, - od->nbs->name, tunnel_key); - continue; - } - sbrec_datapath_binding_set_tunnel_key(od->sb, tunnel_key); - } + if (od->sb->tunnel_key != od->tunnel_key) { + sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key); } + ovn_datapath_update_external_ids(od); + } + LIST_FOR_EACH (od, list, &nb_only) { + od->sb = sbrec_datapath_binding_insert(ctx->ovnsb_txn); + ovn_datapath_update_external_ids(od); + sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key); } - ovn_destroy_tnlids(&dp_tnlids); /* Delete southbound records without northbound matches. */ @@ -3292,7 +3306,7 @@ ovn_port_update_sbrec(struct northd_context *ctx, } int64_t tnl_key = op_get_requested_tnl_key(op); if (tnl_key && tnl_key != op->sb->tunnel_key) { - if (ovn_tnlid_in_use(&op->od->port_tnlids, tnl_key)) { + if (!ovn_add_tnlid(&op->od->port_tnlids, tnl_key)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "Cannot update port binding for " "%s due to duplicate key set " @@ -3749,7 +3763,7 @@ build_ports(struct northd_context *ctx, /* Add southbound record for each unmatched northbound record. */ LIST_FOR_EACH_SAFE (op, next, list, &nb_only) { int64_t tunnel_key = op_get_requested_tnl_key(op); - if (tunnel_key && ovn_tnlid_in_use(&op->od->port_tnlids, tunnel_key)) { + if (tunnel_key && !ovn_add_tnlid(&op->od->port_tnlids, tunnel_key)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "Cannot create port binding for " "%s due to duplicate key set " diff --git a/ovn-nb.xml b/ovn-nb.xml index b0ceb5051966..a3979caf13ce 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -1933,6 +1933,14 @@ binding table.

+ + + Configures the datapath tunnel key for the logical router. + This is not needed because ovn-northd will assign an + unique key for each datapath by itself. However, if it is configured, + ovn-northd honors the configured value. + diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index e155e26f897c..9ad562ee15f7 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2067,3 +2067,49 @@ action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implici ]) AT_CLEANUP + +AT_SETUP([requested-tnl-key]) +AT_KEYWORDS([requested tnl tunnel key keys]) +ovn_start + +get_tunnel_keys() { + set $(ovn-sbctl get datapath_binding ls0 tunnel_key \ + -- get datapath_binding ls1 tunnel_key \ + -- get datapath_binding ls2 tunnel_key) + echo "ls0=$ls0 ls1=$ls1 ls2=$ls2" + ls0=$1 ls1=$2 ls2=$3 + AT_CHECK([test "$ls0" != "$ls1" && \ + test "$ls1" != "$ls2" && \ + test "$ls0" != "$ls2"]) +} + +echo +echo "__file__:__line__: Add three logical switches, check tunnel ids" +AT_CHECK( + [ovn-nbctl --wait=sb ls-add ls0 + ovn-nbctl --wait=sb ls-add ls1 + ovn-nbctl --wait=sb ls-add ls2]) +get_tunnel_keys +AT_CHECK([test $ls0 = 1 && test $ls1 = 2 && test $ls2 = 3]) + +echo +echo "__file__:__line__: Assign ls0 new tunnel key, others don't change." +AT_CHECK( + [ovn-nbctl --wait=sb set logical-switch ls0 other-config:requested-tnl-key=4]) +get_tunnel_keys +AT_CHECK([test $ls0 = 4 && test $ls1 = 2 && test $ls2 = 3]) + +echo +echo "__file__:__line__: Assign ls0 a conflict with ls1, which moves aside." +AT_CHECK( + [ovn-nbctl --wait=sb set logical-switch ls0 other-config:requested-tnl-key=2]) +get_tunnel_keys +AT_CHECK([test $ls0 = 2 && test $ls2 = 3]) + +echo +echo "__file__:__line__: Assign ls0 and ls1 conflicts and verify that they end up different and ls2 doesn't change." +AT_CHECK( + [ovn-nbctl --wait=sb set logical-switch ls1 other-config:requested-tnl-key=2]) +get_tunnel_keys +AT_CHECK([test $ls2 = 3]) +AT_CLEANUP