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 <[email protected]> --- 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. </p> </column> + + <column name="options" key="requested-tnl-key" + type='{"type": "integer", "minInteger": 1, "maxInteger": 16777215}'> + Configures the datapath tunnel key for the logical router. + This is not needed because <code>ovn-northd</code> will assign an + unique key for each datapath by itself. However, if it is configured, + <code>ovn-northd</code> honors the configured value. + </column> </group> <group title="Common Columns"> 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 -- 2.26.2 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
