[ovs-dev] [ovn-controller-vtep V7 1/3] ovn-controller-vtep: Add vtep module.

Alex Wang alexw at nicira.com
Thu Aug 20 16:06:54 UTC 2015


On Thu, Aug 20, 2015 at 8:24 AM, Russell Bryant <rbryant at redhat.com> wrote:

> acked this earlier, but just came across something while reviewing the
> next patch ... let me know if I'm missing something.
>
> On 08/18/2015 05:58 PM, Alex Wang wrote:
> > This commit adds the vtep module to ovn-controller-vtep.  The
> > module will scan through the Port_Binding table in OVN-SB database,
> > and update the vtep logcial switches tunnel keys.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
> > ---
> > V6->V7:
> > - change the assertion to VLOG_ERR in vtep_lswitch_run().
> > - refine the vtep_lswitch_run() as suggested by Russell.
> > - refine vtep_lswitch_cleanup() as suggested by Russell.
> >
> > V5->V6:
> > - rebase.
> >
> > V5: new patch.
> > ---
> >  ovn/controller-vtep/automake.mk           |    4 +-
> >  ovn/controller-vtep/binding.c             |    3 +-
> >  ovn/controller-vtep/gateway.c             |    3 +-
> >  ovn/controller-vtep/ovn-controller-vtep.c |    3 +
> >  ovn/controller-vtep/vtep.c                |  149
> +++++++++++++++++++++++++++++
> >  ovn/controller-vtep/vtep.h                |   27 ++++++
> >  tests/ovn-controller-vtep.at              |   32 +++++++
> >  7 files changed, 218 insertions(+), 3 deletions(-)
> >  create mode 100644 ovn/controller-vtep/vtep.c
> >  create mode 100644 ovn/controller-vtep/vtep.h
> >
> > diff --git a/ovn/controller-vtep/automake.mk b/ovn/controller-vtep/
> automake.mk
> > index 33f063f..cacfae6 100644
> > --- a/ovn/controller-vtep/automake.mk
> > +++ b/ovn/controller-vtep/automake.mk
> > @@ -5,7 +5,9 @@ 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.h \
> > +     ovn/controller-vtep/vtep.c \
> > +     ovn/controller-vtep/vtep.h
> >  ovn_controller_vtep_ovn_controller_vtep_LDADD = ovn/lib/libovn.la lib/
> libopenvswitch.la vtep/libvtep.la
> >  man_MANS += ovn/controller-vtep/ovn-controller-vtep.8
> >  EXTRA_DIST += ovn/controller-vtep/ovn-controller-vtep.8.xml
> > diff --git a/ovn/controller-vtep/binding.c
> b/ovn/controller-vtep/binding.c
> > index 652852d..d65f43c 100644
> > --- a/ovn/controller-vtep/binding.c
> > +++ b/ovn/controller-vtep/binding.c
> > @@ -226,7 +226,8 @@ binding_run(struct controller_vtep_ctx *ctx)
> >  }
> >
> >  /* Removes all port binding association with vtep gateway chassis.
> > - * Returns true when all done. */
> > + * Returns true when done (i.e. there is no change made to 'ovnsb_idl'),
> > + * otherwise returns false. */
> >  bool
> >  binding_cleanup(struct controller_vtep_ctx *ctx)
> >  {
> > diff --git a/ovn/controller-vtep/gateway.c
> b/ovn/controller-vtep/gateway.c
> > index 025aff8..963d419 100644
> > --- a/ovn/controller-vtep/gateway.c
> > +++ b/ovn/controller-vtep/gateway.c
> > @@ -189,7 +189,8 @@ gateway_run(struct controller_vtep_ctx *ctx)
> >  }
> >
> >  /* Destroys the chassis table entries for vtep physical switches.
> > - * Returns true when all done. */
> > + * Returns true when done (i.e. there is no change made to 'ovnsb_idl'),
> > + * otherwise returns false. */
> >  bool
> >  gateway_cleanup(struct controller_vtep_ctx *ctx)
> >  {
> > diff --git a/ovn/controller-vtep/ovn-controller-vtep.c
> b/ovn/controller-vtep/ovn-controller-vtep.c
> > index 7e98f69..429ac23 100644
> > --- a/ovn/controller-vtep/ovn-controller-vtep.c
> > +++ b/ovn/controller-vtep/ovn-controller-vtep.c
> > @@ -39,6 +39,7 @@
> >
> >  #include "binding.h"
> >  #include "gateway.h"
> > +#include "vtep.h"
> >  #include "ovn-controller-vtep.h"
> >
> >  static unixctl_cb_func ovn_controller_vtep_exit;
> > @@ -99,6 +100,7 @@ main(int argc, char *argv[])
> >
> >          gateway_run(&ctx);
> >          binding_run(&ctx);
> > +        vtep_run(&ctx);
> >          unixctl_server_run(unixctl);
> >
> >          unixctl_server_wait(unixctl);
> > @@ -127,6 +129,7 @@ main(int argc, char *argv[])
> >           * We're done if all of them return true. */
> >          done = binding_cleanup(&ctx);
> >          done = gateway_cleanup(&ctx) && done;
> > +        done = vtep_cleanup(&ctx) && done;
> >          if (done) {
> >              poll_immediate_wake();
> >          }
> > diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c
> > new file mode 100644
> > index 0000000..9870296
> > --- /dev/null
> > +++ b/ovn/controller-vtep/vtep.c
> > @@ -0,0 +1,149 @@
> > +/* 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 "vtep.h"
> > +
> > +#include "lib/hash.h"
> > +#include "lib/hmap.h"
> > +#include "lib/smap.h"
> > +#include "lib/util.h"
> > +#include "ovn-controller-vtep.h"
> > +#include "openvswitch/vlog.h"
> > +#include "ovn/lib/ovn-sb-idl.h"
> > +#include "vtep/vtep-idl.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(vtep);
> > +
> > +/*
> > + * Scans through the Binding table in ovnsb and updates the vtep logical
> > + * switch tunnel keys.
> > + *
> > + */
> > +
> > +/* Updates the vtep Logical_Switch table entries' tunnel keys based
> > + * on the port bindings. */
> > +static void
> > +vtep_lswitch_run(struct controller_vtep_ctx *ctx)
> > +{
> > +    struct shash vtep_lswitches = SHASH_INITIALIZER(&vtep_lswitches);
> > +    const struct sbrec_port_binding *port_binding_rec;
> > +    const struct vteprec_logical_switch *vtep_ls;
> > +
> > +    /* Stores all logical switches to 'vtep_lswitches' with name as
> key. */
> > +    VTEPREC_LOGICAL_SWITCH_FOR_EACH (vtep_ls, ctx->vtep_idl) {
> > +        shash_add(&vtep_lswitches, vtep_ls->name, vtep_ls);
> > +    }
> > +
> > +    ovsdb_idl_txn_add_comment(ctx->vtep_idl_txn,
> > +                              "ovn-controller-vtep: update logical
> switch "
> > +                              "tunnel keys");
> > +    /* Collects the logical switch bindings from port binding entries.
> > +     * Since the binding module has already guaranteed that each vtep
> > +     * logical switch is bound only to one ovn-sb logical datapath,
> > +     * we can just iterate and assign tunnel key to vtep logical
> switch. */
> > +    SBREC_PORT_BINDING_FOR_EACH(port_binding_rec, ctx->ovnsb_idl) {
> > +        if (strcmp(port_binding_rec->type, "vtep")
> > +            || !port_binding_rec->chassis) {
> > +            continue;
> > +        }
> > +        const char *lswitch_name = smap_get(&port_binding_rec->options,
> > +                                            "vtep-logical-switch");
> > +
> > +        /* If 'port_binding_rec->chassis' exists then 'lswitch_name'
> > +         * must also exist. */
> > +        if (!lswitch_name) {
> > +            VLOG_ERR("logical port (%s) with no 'options:vtep-logical-"
> > +                     "switch' specified is somehow bound to chassis
> (%s). "
> > +                     "this could only happen when someone is messing up
> "
> > +                     "using ovn-sbctl",
> > +                     port_binding_rec->logical_port,
> > +                     port_binding_rec->chassis->name);
> > +            continue;
> > +        }
> > +        vtep_ls = shash_find_and_delete(&vtep_lswitches, lswitch_name);
> > +        if (!vtep_ls) {
> > +            VLOG_ERR("logical port (%s) bound to non-exist vtep logical
> "
> > +                     "switch (%s), something is seriously wrong",
> > +                     port_binding_rec->logical_port, lswitch_name);
>
> It looks like there's some missing validation to ensure that the port
> binding we're looking at is actually bound to this chassis and not just
> any chassis.
>
> It seems this error condition could get easily hit if the port is bound
> to a logical switch of the same on a different vtep gateway.
>
>

Thx for pointing this out~!  I'll change the 'shash_find_and_delete' to
'shash_find' and use a sset to help with the garbage collection just like
in the next patch~

Alex Wang,



> > +        } else {
> > +            int64_t tnl_key;
> > +
> > +            tnl_key = port_binding_rec->datapath->tunnel_key;
> > +            if (vtep_ls->n_tunnel_key
> > +                && vtep_ls->tunnel_key[0] != tnl_key) {
> > +                VLOG_DBG("set vtep logical switch (%s) tunnel key from "
> > +                         "(%"PRId64") to (%"PRId64")", vtep_ls->name,
> > +                         vtep_ls->tunnel_key[0], tnl_key);
> > +            }
> > +            vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
> > +        }
> > +    }
> > +    struct shash_node *node;
> > +    /* Resets the tunnel keys for the rest of vtep logical switches. */
> > +    SHASH_FOR_EACH (node, &vtep_lswitches) {
> > +        int64_t tnl_key = 0;
> > +
> > +        vteprec_logical_switch_set_tunnel_key(node->data, &tnl_key, 1);
> > +    }
> > +
> > +    shash_destroy(&vtep_lswitches);
> > +}
> > +
> > +/* Since we do not own any vtep logical switch, just sets their tunnel
> key
> > + * to 0. */
> > +static bool
> > +vtep_lswitch_cleanup(struct ovsdb_idl *vtep_idl)
> > +{
> > +    const struct vteprec_logical_switch *vtep_ls;
> > +    int64_t tnl_key = 0;
> > +    bool done = true;
> > +
> > +    VTEPREC_LOGICAL_SWITCH_FOR_EACH (vtep_ls, vtep_idl) {
> > +        if (vtep_ls->n_tunnel_key != 1
> > +            || vtep_ls->tunnel_key[0] != tnl_key) {
> > +            vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
> > +            done = false;
> > +        }
> > +    }
> > +
> > +    return done;
> > +}
> > +
> > +
> > +/* Updates vtep logical switch tunnel keys. */
> > +void
> > +vtep_run(struct controller_vtep_ctx *ctx)
> > +{
> > +    if (!ctx->vtep_idl_txn) {
> > +        return;
> > +    }
> > +    vtep_lswitch_run(ctx);
> > +}
> > +
> > +/* Cleans up all related entries in vtep.  Returns true when done (i.e.
> > + * there is no change made to 'vtep_idl'), otherwise returns false. */
> > +bool
> > +vtep_cleanup(struct controller_vtep_ctx *ctx)
> > +{
> > +    if (!ctx->vtep_idl_txn) {
> > +        return false;
> > +    }
> > +
> > +    ovsdb_idl_txn_add_comment(ctx->vtep_idl_txn,
> > +                              "cleans up vtep configuration");
> > +    return vtep_lswitch_cleanup(ctx->vtep_idl);
> > +}
> > diff --git a/ovn/controller-vtep/vtep.h b/ovn/controller-vtep/vtep.h
> > new file mode 100644
> > index 0000000..ae6c79b
> > --- /dev/null
> > +++ b/ovn/controller-vtep/vtep.h
> > @@ -0,0 +1,27 @@
> > +/* 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.
> > + */
> > +
> > +
> > +#ifndef OVN_VTEP_H
> > +#define OVN_VTEP_H 1
> > +
> > +#include <stdbool.h>
> > +
> > +struct controller_vtep_ctx;
> > +
> > +void vtep_run(struct controller_vtep_ctx *);
> > +bool vtep_cleanup(struct controller_vtep_ctx *);
> > +
> > +#endif /* ovn/controller-gw/vtep.h */
> > diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
> > index d7fe234..1460188 100644
> > --- a/tests/ovn-controller-vtep.at
> > +++ b/tests/ovn-controller-vtep.at
> > @@ -274,3 +274,35 @@ ${chassis_uuid}
> >
> >  OVN_CONTROLLER_VTEP_STOP(["/has already been associated with logical
> datapath/d"])
> >  AT_CLEANUP
> > +
> > +
> > +# Tests vtep module vtep logical switch tunnel key update.
> > +AT_SETUP([ovn-controller-vtep - test vtep-lswitch])
> > +OVN_CONTROLLER_VTEP_START
> > +
> > +# creates the logical switch in vtep and adds the corresponding logical
> > +# port to 'br-test'.
> > +AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0])
> > +OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0], [br-vtep],
> [lswitch0])
> > +OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep
> br-vtep_lswitch0`"])
> > +
> > +# retrieves the expected tunnel key.
> > +datapath_uuid=$(ovn-sbctl --columns=datapath list Port_Binding
> br-vtep_lswitch0 | cut -d ':' -f2 | tr -d ' ')
> > +tunnel_key=$(ovn-sbctl --columns=tunnel_key list Datapath_Binding
> ${datapath_uuid} | cut -d ':' -f2 | tr -d ' ')
> > +OVS_WAIT_UNTIL([test -z "`vtep-ctl --columns=tunnel_key list
> Logical_Switch | grep 0`"])
> > +# checks the vtep logical switch tunnel key configuration.
> > +AT_CHECK_UNQUOTED([vtep-ctl --columns=tunnel_key list Logical_Switch |
> cut -d ':' -f2 | tr -d ' '], [0], [dnl
> > +${tunnel_key}
> > +])
> > +
> > +# changes the ovn-nb logical port type so that it is no longer
> > +# vtep port.
> > +AT_CHECK([ovn-nbctl lport-set-type br-vtep_lswitch0 void])
> > +OVS_WAIT_UNTIL([test -z "`vtep-ctl --columns=tunnel_key list
> Logical_Switch | grep 1`"])
> > +# now should see the tunnel key reset.
> > +AT_CHECK([vtep-ctl --columns=tunnel_key list Logical_Switch | cut -d
> ':' -f2 | tr -d ' '], [0], [dnl
> > +0
> > +])
> > +
> > +OVN_CONTROLLER_VTEP_STOP
> > +AT_CLEANUP
> >
>
>



More information about the dev mailing list