[ovs-dev] [PATCH ovn 6/7] northd: Enhance implementation of datapath tunnel key requests.

Numan Siddique numans at ovn.org
Tue Oct 27 16:56:03 UTC 2020


On Mon, Oct 26, 2020 at 11:47 PM Ben Pfaff <blp at 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 at ovn.org>

Acked-by: Numan Siddique <numans at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list