[ovs-dev] [ovn-controller-vtep V5 09/12] ovn-controller-vtep: Add binding module.

Russell Bryant rbryant at redhat.com
Fri Aug 7 22:19:22 UTC 2015


On 08/07/2015 03:46 AM, Alex Wang wrote:
> This commit adds the binding module to ovn-controller-vtep.  The
> module will scan through the Port_Binding table in ovnsb.  If there is
> a port binding entry for a logical switch on the vtep gateway chassis's
> "vtep_logical_switches", sets the port binding's chassis column to the
> vtep gateway chassis.
> 
> Signed-off-by: Alex Wang <alexw at nicira.com>

I wanted to send the quick message about the test failures, but here are
a few inline comments and a question I wanted to ask ...

> 
> ---
> V4->V5:
> - rebase on top of master.
> - change to use port binding type, and options when finding bindings
>   for vtep gateway chassis.
> 
> V3->V4:
> - rebase to master.
> 
> V2->V3:
> - since ovn-sb schema changes (removal of Gateway table), the binding
>   module code needs to be adapted.
> 
> PATCH->V2:
> - split into separate commit.
> - disallow and warn if more than one logical port from one 'vlan_map'
>   are attached to the same logical datapath.
> ---
>  ovn/controller-vtep/automake.mk           |    2 +
>  ovn/controller-vtep/binding.c             |  247 +++++++++++++++++++++++++++++
>  ovn/controller-vtep/binding.h             |   27 ++++
>  ovn/controller-vtep/ovn-controller-vtep.c |    4 +
>  tests/ovn-controller-vtep.at              |  112 +++++++++++++
>  5 files changed, 392 insertions(+)
>  create mode 100644 ovn/controller-vtep/binding.c
>  create mode 100644 ovn/controller-vtep/binding.h
> 
> diff --git a/ovn/controller-vtep/automake.mk b/ovn/controller-vtep/automake.mk
> index 514cafa..33f063f 100644
> --- a/ovn/controller-vtep/automake.mk
> +++ b/ovn/controller-vtep/automake.mk
> @@ -1,5 +1,7 @@
>  bin_PROGRAMS += ovn/controller-vtep/ovn-controller-vtep
>  ovn_controller_vtep_ovn_controller_vtep_SOURCES = \
> +	ovn/controller-vtep/binding.c \
> +	ovn/controller-vtep/binding.h \
>  	ovn/controller-vtep/gateway.c \
>  	ovn/controller-vtep/gateway.h \
>  	ovn/controller-vtep/ovn-controller-vtep.c \
> diff --git a/ovn/controller-vtep/binding.c b/ovn/controller-vtep/binding.c
> new file mode 100644
> index 0000000..fc12006
> --- /dev/null
> +++ b/ovn/controller-vtep/binding.c
> @@ -0,0 +1,247 @@
> +/* Copyright (c) 2015 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +#include "binding.h"
> +
> +#include "lib/shash.h"
> +#include "lib/smap.h"
> +#include "lib/util.h"
> +#include "openvswitch/vlog.h"
> +#include "ovn-controller-vtep.h"
> +#include "ovn/lib/ovn-sb-idl.h"
> +#include "vtep/vtep-idl.h"
> +
> +VLOG_DEFINE_THIS_MODULE(binding);
> +
> +/*
> + * This module scans through the Port_Binding table in ovnsb.  If there is a
> + * logical port binding entry for logical switch in vtep gateway chassis's
> + * 'vtep_logical_switches' column, sets the binding's chassis column to the
> + * corresponding vtep gateway chassis.
> + *
> + */
> +
> +
> +/* Returns true if the vtep logical switch specified by 'port_binding_rec'
> + * has already been bound to another port binding entry, and resets
> + * 'port_binding_rec''s chassis column.  Otherwise, updates 'ls_to_pb'
> + * and returns false. */
> +static bool
> +check_pb_conflict(struct shash *ls_to_pb,
> +                  const struct sbrec_port_binding *port_binding_rec,
> +                  const struct sbrec_chassis *chassis_rec)
> +{
> +    const char *vtep_lswitch =
> +        smap_get(&port_binding_rec->options, "vtep-logical-switch");
> +    const struct sbrec_port_binding *pb_conflict =
> +        shash_find_data(ls_to_pb, vtep_lswitch);
> +
> +    if (pb_conflict) {
> +        VLOG_WARN("logical switch (%s), on vtep gateway chassis "
> +                  "(%s) has already been associated with logical "
> +                  "port (%s), ignore logical port (%s)",
> +                  vtep_lswitch, chassis_rec->name,
> +                  pb_conflict->logical_port,
> +                  port_binding_rec->logical_port);
> +        sbrec_port_binding_set_chassis(port_binding_rec, NULL);
> +
> +        return true;
> +    }
> +
> +    shash_replace(ls_to_pb, vtep_lswitch, port_binding_rec);
> +    return false;
> +}
> +
> +/* Returns true if the vtep logical switch specified by 'port_binding_rec'
> + * has already been bound to a different datapath, and resets
> + * 'port_binding_rec''s chassis column.  Otherwise, updates 'ls_to_db' and
> + * returns false. */
> +static bool
> +check_db_conflict(struct shash *ls_to_db,
> +                  const struct sbrec_port_binding *port_binding_rec,
> +                  const struct sbrec_chassis *chassis_rec)
> +{
> +    const char *vtep_lswitch =
> +        smap_get(&port_binding_rec->options, "vtep-logical-switch");
> +    const struct sbrec_datapath_binding *db_conflict =
> +        shash_find_data(ls_to_db, vtep_lswitch);
> +
> +    if (db_conflict && db_conflict != port_binding_rec->datapath) {
> +        VLOG_WARN("logical switch (%s), on vtep gateway chassis "
> +                  "(%s) has already been associated with logical "
> +                  "datapath (with tunnel key %"PRId64"), ignore "
> +                  "logical port (%s) which belongs to logical "
> +                  "datapath (with tunnel key %"PRId64")",
> +                  vtep_lswitch, chassis_rec->name,
> +                  db_conflict->tunnel_key,
> +                  port_binding_rec->logical_port,
> +                  port_binding_rec->datapath->tunnel_key);
> +        sbrec_port_binding_set_chassis(port_binding_rec, NULL);
> +
> +        return true;
> +    }
> +
> +    shash_replace(ls_to_db, vtep_lswitch, port_binding_rec->datapath);
> +    return false;
> +}
> +
> +/* Updates the 'port_binding_rec''s chassis column to 'chassis_rec'. */
> +static void
> +update_pb_chassis(const struct sbrec_port_binding *port_binding_rec,
> +                  const struct sbrec_chassis *chassis_rec)
> +{
> +    if (port_binding_rec->chassis != chassis_rec) {
> +        if (chassis_rec && port_binding_rec->chassis) {
> +            VLOG_DBG("Changing chassis association of logical "
> +                     "port (%s) from (%s) to (%s)",
> +                     port_binding_rec->logical_port,
> +                     port_binding_rec->chassis->name,
> +                     chassis_rec->name);
> +        }
> +        sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec);
> +    }
> +}
> +
> +
> +/* Checks and updates logical port to vtep logical switch bindings for each
> + * physical switch in VTEP. */
> +void
> +binding_run(struct controller_vtep_ctx *ctx)
> +{
> +    const struct vteprec_physical_switch *pswitch;
> +
> +    if (!ctx->ovnsb_idl_txn) {
> +        return;
> +    }
> +
> +    /* 'ls_to_db'
> +     *
> +     * Maps vtep logical switch name to the datapath binding entry.  This is
> +     * used to guarantee that each vtep logical switch is only included
> +     * in only one ovn datapath (ovn logical switch).  See check_db_conflict()
> +     * for details.
> +     *
> +     * 'ls_to_pb'
> +     *
> +     * Maps vtep logical switch name to the port binding entry.  This is used
> +     * to guarantee that each vtep logical switch on a vtep physical switch
> +     * is only bound to one logical port.  See check_pb_conflict() for
> +     * details.
> +     *
> +     */
> +    struct shash ls_to_db = SHASH_INITIALIZER(&ls_to_db);
> +    const struct vteprec_logical_switch *vtep_ls;
> +    VTEPREC_LOGICAL_SWITCH_FOR_EACH (vtep_ls, ctx->vtep_idl) {
> +        shash_add(&ls_to_db, vtep_ls->name, NULL);
> +    }
> +
> +    ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
> +                              "ovn-controller-vtep: updating bindings");
> +    VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> +        const struct sbrec_chassis *chassis_rec
> +            = get_chassis_by_name(ctx->ovnsb_idl, pswitch->name);

I would make sure chassis_rec is not NULL here.  It's not supposed to
happen, but just in case ...

> +        struct shash ls_to_pb = SHASH_INITIALIZER(&ls_to_pb);
> +        const struct sbrec_port_binding *port_binding_rec;
> +        size_t i;
> +
> +        for (i = 0; i < chassis_rec->n_vtep_logical_switches; i++) {
> +            shash_add(&ls_to_pb, chassis_rec->vtep_logical_switches[i],
> +                      NULL);
> +        }
> +
> +        SBREC_PORT_BINDING_FOR_EACH(port_binding_rec, ctx->ovnsb_idl) {

How often would you have more than 1 Physical_Switch in a vtep db?  I
ask because the port binding list could be quite long.  Instead of
iterating the whole thing multiple times, you could iterate it once and
build a hash table of vtep port binding records hashed on the physical
switch and logical switch.

I may be trying to micro-optimize here, though.  Anyway, just a thought.

> +            const char *type = port_binding_rec->type;
> +            const char *vtep_pswitch = smap_get(&port_binding_rec->options,
> +                                                "vtep-physical-switch");
> +            const char *vtep_lswitch = smap_get(&port_binding_rec->options,
> +                                                "vtep-logical-switch");
> +
> +            /* Port binding for vtep gateway chassis must have type "vtep",
> +             * and matched physical switch name and logical switch name. */
> +            if (!strcmp(type, "vtep")
> +                && vtep_pswitch && !strcmp(vtep_pswitch, chassis_rec->name)
> +                && vtep_lswitch && shash_find(&ls_to_pb, vtep_lswitch)) {
> +                bool pb_conflict, db_conflict;
> +
> +                pb_conflict = check_pb_conflict(&ls_to_pb, port_binding_rec,
> +                                                chassis_rec);
> +                db_conflict = check_db_conflict(&ls_to_db, port_binding_rec,
> +                                                chassis_rec);
> +                /* Updates port binding's chassis column when there
> +                 * is no conflict. */
> +                if (!pb_conflict && !db_conflict) {
> +                    update_pb_chassis(port_binding_rec, chassis_rec);
> +                }
> +            } else if (port_binding_rec->chassis == chassis_rec) {
> +                update_pb_chassis(port_binding_rec, NULL);
> +            }
> +        }
> +        struct shash_node *node;
> +        SHASH_FOR_EACH (node, &ls_to_pb) {
> +            if (!node->data) {
> +                VLOG_DBG("No port binding entry for logical switch (%s)"
> +                         "on vtep gateway chassis (%s)", node->name,
> +                         chassis_rec->name);

This could get pretty noisy.  I guess it's only a debug message, but
maybe it's worth rate limiting anyway?

> +            }
> +        }
> +        shash_destroy(&ls_to_pb);
> +    }
> +    shash_destroy(&ls_to_db);
> +}
> +
> +/* Removes all port binding association with vtep gateway chassis.
> + * Returns true when all done. */
> +bool
> +binding_cleanup(struct controller_vtep_ctx *ctx)
> +{
> +    struct shash ch_to_pb = SHASH_INITIALIZER(&ch_to_pb);
> +    const struct sbrec_port_binding *port_binding_rec;
> +
> +    if (!ctx->ovnsb_idl_txn) {

There's no bug here, but I thought there might be at first because
ch_to_pb is initialized but not destroyed.  It turns out that doesn't
matter, but my brain just expects them to always match up.  :-)

Maybe you could move this check to the very top of the function?

-- 
Russell Bryant



More information about the dev mailing list