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