On Sun, Aug 16, 2015 at 3:00 PM, Russell Bryant <rbry...@redhat.com> wrote:
> On 08/09/2015 10:50 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>
> > ---
> > V5->V6:
> > - rebase.
> >
> > V5: new patch.
> > ---
> > ovn/controller-vtep/automake.mk | 4 +-
> > ovn/controller-vtep/ovn-controller-vtep.c | 3 +
> > ovn/controller-vtep/vtep.c | 134
> +++++++++++++++++++++++++++++
> > ovn/controller-vtep/vtep.h | 27 ++++++
> > tests/ovn-controller-vtep.at | 32 +++++++
> > 5 files changed, 199 insertions(+), 1 deletion(-)
> > 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/ovn-controller-vtep.c
> b/ovn/controller-vtep/ovn-controller-vtep.c
> > index a3b0f96..9b7838a 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;
> > @@ -97,6 +98,7 @@ main(int argc, char *argv[])
> >
> > gateway_run(&ctx);
> > binding_run(&ctx);
> > + vtep_run(&ctx);
> > unixctl_server_run(unixctl);
> >
> > unixctl_server_wait(unixctl);
> > @@ -122,6 +124,7 @@ main(int argc, char *argv[])
> > * We're done if all of them return true. */
> > done = gateway_cleanup(&ctx);
> > done = binding_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..55d2e0d
> > --- /dev/null
> > +++ b/ovn/controller-vtep/vtep.c
> > @@ -0,0 +1,134 @@
> > +/* 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) {
>
> What would you think about doing all of this in binding.c instead? The
> code already iterates the port bindings there. The port bindings table
> can easily have many thousands of entries.
>
> If you'd rather keep it logically separated like this and wait until we
> see what shows up in profiling, that's fine too, it was just a thought.
>
>
Yeah, I'd like to delay it until later when we have a better understanding
of
all things that need to access or iterate Port_Binding entries (e.g. the
update
of ucast_macs_remote/ucast_macs_local tables in VTEP database).
> > + if (!strcmp(port_binding_rec->type, "vtep")
> > + && port_binding_rec->chassis) {
>
> This is just a small style thing that you can ignore if you want.
>
> The whole body of this loop is within this if block. I'd be tempted to
> reverse the condition to reduce nesting by 1 level for the main body of
> the loop.
>
> if (strcmp(port_binding_rec->type, "vtep") ||
> !port_binding_rec->chassis) {
> continue;
> }
>
> ... main body of loop ...
>
>
Sure, I'd like to adopt~
> > + 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. */
> > + ovs_assert(lswitch_name);
>
> It looks like a misconfiguration, or even just a race condition, could
> cause every running instance of ovs-controller-vtep to exit when it hits
> this line. I just have to create a port with type=vtep, but not set the
> vtep-logical-switch option. It could be that a client just didn't
> create the port, set the type, and set the options all in the same
> transaction.
>
> I think I'd just log a rate limited warning here and continue instead.
My thought is that, since 'vtep_run()' is called after 'binding_run()', any
'Port_Binding' entry with type 'vtep' and has 'chassis' column filled must
also have this 'lswitch_name'. Regarding the situation you mentioned above,
the 'binding_run()' will not set the 'chassis' column at all.
However, on a second thought, there is still one corner case this assert
could be hit. If user does not set the 'vtep-logical-switch' in 'options'
but
use 'ovn-sbctl' to bind the logical port with chassis... So, I'd like to
change
that to use VLOG_ERR.
> + 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);
> > + } 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 void
> > +vtep_lswitch_cleanup(struct ovsdb_idl *vtep_idl)
> > +{
> > + const struct vteprec_logical_switch *vtep_ls;
> > + int64_t tnl_key = 0;
> > +
> > + VTEPREC_LOGICAL_SWITCH_FOR_EACH (vtep_ls, vtep_idl) {
> > + vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
> > + }
> > +}
> > +
> > +
> > +/* 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 created vtep related entries. Returns true when all
> done. */
> > +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");
> > + vtep_lswitch_cleanup(ctx->vtep_idl);
> > +
> > + return true;
>
> Should vtep_switch_cleanup() return whether it had to make any changes?
> In that case it should return false, right?
>
> Right now it's relying on the IDL to figure out if any changes will
> actually get added to the transaction or not, but it seems like you need
> to look at the old value so you can return true/false here.
>
>
Makes sense, I'll adjust accordingly,
Thanks,
Alex Wang,
> > +}
> > 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 3ff0b93..ea58636 100644
> > --- a/tests/ovn-controller-vtep.at
> > +++ b/tests/ovn-controller-vtep.at
> > @@ -262,3 +262,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
> >
>
>
> --
> Russell Bryant
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
