| Message ID | 20201026181626.1827014-6-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/7] northd: Count mask length and priority correctly for IPv6 addresses. | expand |
On Mon, Oct 26, 2020 at 11:47 PM Ben Pfaff <blp@ovn.org> wrote: > > 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 <blp@ovn.org> Acked-by: Numan Siddique <numans@ovn.org> Thanks Numan > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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
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 <blp@ovn.org> --- 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(-)