[ovs-dev] [ovn-controller-vtep V5 08/12] ovn-controller-vtep: Add gateway module.

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


On 08/07/2015 03:46 AM, Alex Wang wrote:
> This commit adds the gateway module to ovn-controller-vtep.  The
> module will register the physical switches to ovnsb as chassis and
> constantly update the "vtep_logical_switches" column in Chassis table.
> 
> Limitation (Recorded in TODO file):
> 
> - Do not support reading multiple tunnel ips of physical switch.
> 
> Signed-off-by: Alex Wang <alexw at nicira.com>
> 
> ---
> V4->V5:
> - rebase on top of master.
> - adopt Russell's review suggestions, greatly simplify the code.
> 
> V3->V4:
> - rebase to master.
> 
> V2->V3:
> - since ovn-sb schema changes (removal of Gateway table), the gateway
>   module code needs to be adapted.
> - rebase to master.
> 
> PATCH->V2:
> - split into separate commit.
> - can register all physical switches controlled by vtep database.
> ---
>  ovn/TODO                                           |    6 +
>  ovn/controller-vtep/automake.mk                    |    2 +
>  ovn/controller-vtep/gateway.c                      |  224 ++++++++++++++++++++
>  .../{ovn-controller-vtep.h => gateway.h}           |   19 +-
>  ovn/controller-vtep/ovn-controller-vtep.c          |   43 +++-
>  ovn/controller-vtep/ovn-controller-vtep.h          |   20 ++
>  tests/ovn-controller-vtep.at                       |  151 +++++++++++++
>  7 files changed, 445 insertions(+), 20 deletions(-)
>  create mode 100644 ovn/controller-vtep/gateway.c
>  copy ovn/controller-vtep/{ovn-controller-vtep.h => gateway.h} (65%)
> 
> diff --git a/ovn/TODO b/ovn/TODO
> index 509e5d0..356b3ba 100644
> --- a/ovn/TODO
> +++ b/ovn/TODO
> @@ -74,3 +74,9 @@
>     Epstein et al., "What's the Difference? Efficient Set
>     Reconciliation Without Prior Context".  (I'm not yet aware of
>     previous non-academic use of this technique.)
> +
> +** Support multiple tunnel encapsulations in Chassis.
> +
> +   So far, both ovn-controller and ovn-controller-vtep only allow
> +   chassis to have one tunnel encapsulation entry.  We should extend
> +   the implementation to support multiple tunnel encapsulations.
> diff --git a/ovn/controller-vtep/automake.mk b/ovn/controller-vtep/automake.mk
> index 7adda15..514cafa 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/gateway.c \
> +	ovn/controller-vtep/gateway.h \
>  	ovn/controller-vtep/ovn-controller-vtep.c \
>  	ovn/controller-vtep/ovn-controller-vtep.h
>  ovn_controller_vtep_ovn_controller_vtep_LDADD = ovn/lib/libovn.la lib/libopenvswitch.la vtep/libvtep.la
> diff --git a/ovn/controller-vtep/gateway.c b/ovn/controller-vtep/gateway.c
> new file mode 100644
> index 0000000..5f6da27
> --- /dev/null
> +++ b/ovn/controller-vtep/gateway.c
> @@ -0,0 +1,224 @@
> +/* 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 "gateway.h"
> +
> +#include "lib/poll-loop.h"
> +#include "lib/simap.h"
> +#include "lib/sset.h"
> +#include "lib/util.h"
> +#include "openvswitch/vlog.h"
> +#include "ovn/lib/ovn-sb-idl.h"
> +#include "vtep/vtep-idl.h"
> +#include "ovn-controller-vtep.h"
> +
> +VLOG_DEFINE_THIS_MODULE(gateway);
> +
> +/*
> + * Registers the physical switches in vtep to ovnsb as chassis.  For each
> + * physical switch in the vtep database, finds all vtep logical switches that
> + * are associated with the physical switch, and updates the corresponding
> + * chassis's 'vtep_logical_switches' column.
> + *
> + */
> +
> +/* Global revalidation sequence number, incremented at each call to
> + * 'revalidate_gateway()'. */
> +static unsigned int gw_reval_seq;
> +
> +/* Maps all chassis created by the gateway module to their own reval_seq. */
> +static struct simap gw_chassis_map = SIMAP_INITIALIZER(&gw_chassis_map);
> +
> +/* Creates and returns a new instance of 'struct sbrec_chassis'. */
> +static const struct sbrec_chassis *
> +create_chassis_rec(struct ovsdb_idl_txn *txn, const char *name,
> +                   const char *encap_ip)
> +{
> +    const struct sbrec_chassis *chassis_rec;
> +    struct sbrec_encap *encap_rec;
> +
> +    chassis_rec = sbrec_chassis_insert(txn);
> +    sbrec_chassis_set_name(chassis_rec, name);
> +    encap_rec = sbrec_encap_insert(txn);
> +    sbrec_encap_set_type(encap_rec, OVN_SB_ENCAP_TYPE);
> +    sbrec_encap_set_ip(encap_rec, encap_ip);
> +    sbrec_chassis_set_encaps(chassis_rec, &encap_rec, 1);
> +
> +    return chassis_rec;
> +}
> +
> +/* Revalidates chassis in ovnsb against vtep database.  Creates chassis for
> + * new vtep physical switch.  And removes chassis which no longer have
> + * physical switch in vtep.
> + *
> + * xxx: Support multiple tunnel encaps.
> + *
> + * */
> +static void
> +revalidate_gateway(struct controller_vtep_ctx *ctx)
> +{
> +    const struct vteprec_physical_switch *pswitch;
> +
> +    /* Increments the global revalidation sequence number. */
> +    gw_reval_seq++;
> +
> +    ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
> +                              "ovn-controller-vtep: updating vtep chassis");
> +
> +    VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> +        const struct sbrec_chassis *chassis_rec;
> +        struct simap_node *gw_node;
> +        const char *encap_ip;
> +
> +        encap_ip = pswitch->n_tunnel_ips ? pswitch->tunnel_ips[0] : "";
> +        gw_node = simap_find(&gw_chassis_map, pswitch->name);
> +        chassis_rec = get_chassis_by_name(ctx->ovnsb_idl, pswitch->name);
> +        if (gw_node && !chassis_rec) {
> +            VLOG_WARN("Chassis for VTEP physical switch (%s) disappears, "
> +                      "maybe deleted by ovn-sbctl, adding it back",
> +                      pswitch->name);
> +            create_chassis_rec(ctx->ovnsb_idl_txn, pswitch->name, encap_ip);
> +            gw_node->data = gw_reval_seq;
> +        } else if (!gw_node && chassis_rec) {
> +            VLOG_WARN("Chassis for new VTEP physical switch (%s) has already "
> +                      "been added, maybe we just restarted or someone"
> +                      "added it by ovn-sbctl", pswitch->name);

It looks like this will happen under normal circumstances every time
ovn-controller-vtep restarts.  If that's right, I would either remove
this message or just make it a debug log entry.

> +            if (strcmp(chassis_rec->encaps[0]->type, OVN_SB_ENCAP_TYPE)
> +                && strcmp(chassis_rec->encaps[0]->ip, encap_ip)) {

Did you intend to have "||" here instead of "&&" ?

> +                VLOG_WARN("Chassis config changing on startup, make sure "
> +                          "multiple chassis are not configured : %s/%s->%s/%s",
> +                          chassis_rec->encaps[0]->type,
> +                          chassis_rec->encaps[0]->ip,
> +                          OVN_SB_ENCAP_TYPE, encap_ip);
> +                VLOG_WARN("Skip adding chassis for physical switch (%s)",
> +                          pswitch->name);

How about the case where an administrator has intentionally changed the
IP while ovn-controller-vtep was shut down?

If instead the IP was changed while ovn-controller-vtep was still
running, you'd hit the next case, which would just happily silently
apply the IP address change.

Really, I think the handling for this case should match what's done for
"gw_node && chassis_rec", except you need to simap_add() instead of
setting the value in gw_node.

> +                continue;
> +            }
> +            simap_put(&gw_chassis_map, pswitch->name, gw_reval_seq);
> +        } else if (gw_node && chassis_rec) {
> +            /* Updates chassis's encap if anything changed. */
> +            if (strcmp(chassis_rec->encaps[0]->type, OVN_SB_ENCAP_TYPE)) {
> +                VLOG_WARN("Chassis for VTEP physical switch (%s) can only have "
> +                          "encap type \"%s\"", pswitch->name, OVN_SB_ENCAP_TYPE);
> +                sbrec_encap_set_type(chassis_rec->encaps[0], OVN_SB_ENCAP_TYPE);
> +            }
> +            if (strcmp(chassis_rec->encaps[0]->ip, encap_ip)) {
> +                sbrec_encap_set_ip(chassis_rec->encaps[0], encap_ip);
> +            }
> +            gw_node->data = gw_reval_seq;
> +        } else {
> +            /* Creates a new chassis for the VTEP physical switch and a new
> +             * record in 'gw_chassis_map'. */
> +            create_chassis_rec(ctx->ovnsb_idl_txn, pswitch->name, encap_ip);
> +            simap_put(&gw_chassis_map, pswitch->name, gw_reval_seq);
> +        }

It seems the above 4 could be written as:

if (!chassis_rec) {
    create_chassis_rec(...);
} else {
    ... check for and apply changes to chassis_rec ...
}

if (!gw_node) {
    simap_put(&gw_chassis_map, pswitch->name, gw_reval_seq);
} else {
    gw_node->data = gw_reval_seq;
}

I think that would simplify it a bit.

> +    }
> +
> +    struct simap_node *iter, *next;
> +    /* For 'gw_node' in 'gw_chassis_map' whose data is not
> +     * 'gw_reval_seq', it means the corresponding physical switch no
> +     * longer exist.  So, garbage collects them. */
> +    SIMAP_FOR_EACH_SAFE (iter, next, &gw_chassis_map) {
> +        if (iter->data != gw_reval_seq) {
> +            const struct sbrec_chassis *chassis_rec;
> +
> +            chassis_rec = get_chassis_by_name(ctx->ovnsb_idl, iter->name);
> +            if (chassis_rec) {
> +                sbrec_chassis_delete(chassis_rec);
> +            }
> +            simap_delete(&gw_chassis_map, iter);
> +        }
> +    }
> +}
> +
> +/* Updates the 'vtep_logical_switches' column in the Chassis table based
> + * on vtep database configuration. */
> +static void
> +update_vtep_logical_switches(struct controller_vtep_ctx *ctx)
> +{
> +    const struct vteprec_physical_switch *pswitch;
> +
> +    ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn, "ovn-controller-vtep: "
> +                              "updating chassis's vtep_logical_switches");
> +
> +    VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> +        const struct sbrec_chassis *chassis_rec =
> +            get_chassis_by_name(ctx->ovnsb_idl, pswitch->name);
> +        struct sset lswitches = SSET_INITIALIZER(&lswitches);
> +        size_t i;
> +
> +        for (i = 0; i < pswitch->n_ports; i++) {
> +            const struct vteprec_physical_port *port = pswitch->ports[i];
> +            size_t j;
> +
> +            for (j = 0; j < port->n_vlan_bindings; j++) {
> +                const struct vteprec_logical_switch *vtep_lswitch;
> +
> +                vtep_lswitch = port->value_vlan_bindings[j];
> +                /* If not already in 'lswitches', records it. */
> +                if (!sset_find(&lswitches, vtep_lswitch->name)) {
> +                    sset_add(&lswitches, vtep_lswitch->name);
> +                }
> +            }
> +        }
> +
> +        const char **ls_arr = sset_array(&lswitches);
> +        sbrec_chassis_set_vtep_logical_switches(chassis_rec, ls_arr,
> +                                                sset_count(&lswitches));

You've got a memory leak here.  The result of sset_array() should be
freed here.

> +        sset_destroy(&lswitches);
> +    }
> +}
> +
> +
> +void
> +gateway_run(struct controller_vtep_ctx *ctx)
> +{
> +    if (!ctx->ovnsb_idl_txn) {
> +        return;
> +    }
> +
> +    revalidate_gateway(ctx);
> +    update_vtep_logical_switches(ctx);
> +}
> +
> +/* Destroys the chassis table entries for vtep physical switches.
> + * Returns true when all done. */
> +bool
> +gateway_cleanup(struct controller_vtep_ctx *ctx)
> +{
> +    const struct vteprec_physical_switch *pswitch;
> +
> +    if (!ctx->ovnsb_idl_txn) {
> +        return false;
> +    }
> +
> +    bool all_done = true;
> +    ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn, "ovn-controller-vtep: "
> +                              "unregistering vtep chassis");
> +    VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> +        const struct sbrec_chassis *chassis_rec;
> +
> +        chassis_rec = get_chassis_by_name(ctx->ovnsb_idl, pswitch->name);
> +        if (!chassis_rec) {
> +            continue;
> +        }
> +        all_done = false;
> +        sbrec_chassis_delete(chassis_rec);
> +    }
> +    simap_destroy(&gw_chassis_map);

I believe if gateway_cleanup() returns false, when gateway_cleanup()
gets called again, simap_destroy() called again here will result in a
double free() in hmap_destroy(), if the simap had more than one element
in it.

It looks like you can either make sure you only destroy() once, or make
hmap_destroy() safe to be called more than once.

> +
> +    return all_done;
> +}
> diff --git a/ovn/controller-vtep/ovn-controller-vtep.h b/ovn/controller-vtep/gateway.h
> similarity index 65%
> copy from ovn/controller-vtep/ovn-controller-vtep.h
> copy to ovn/controller-vtep/gateway.h

The diff still shows this as a copy instead of a new file.  I suppose it
doesn't really hurt anything, though.  It just makes the diff a little
weird.

-- 
Russell Bryant



More information about the dev mailing list