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