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