On Thu, Aug 20, 2015 at 8:24 AM, Russell Bryant <rbry...@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 <al...@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
> >
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
