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

Alex Wang alexw at nicira.com
Sun Aug 9 07:00:56 UTC 2015


On Fri, Aug 7, 2015 at 2:19 PM, Russell Bryant <rbryant at redhat.com> wrote:

> 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.



Makes sense, I'll just drop it~



>
> > +            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 "&&" ?
>
>

Thx for pointing this out~!  I do want to use "||"~




> > +                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.
>
>
That's a good point and example,




> > +                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.
>
>

Thx for this suggestion, I'd like to adopt!



> > +    }
> > +
> > +    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.
>
>

Thx for pointing this out, fixed it,



> > +        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.
>
>
Thx for pointing this out~

Since ovn-controller-vtep is single threaded, I'll make it call
hmap_destory()
only 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.
>

I think this relation is added by git...  I reset the commit, readded file,
and ran commit, it still showed that this file is copied from
ovn/controller-vtep/ovn-controller-vtep.h.

Thanks,
Alex Wang,


>
> --
> Russell Bryant
>



More information about the dev mailing list