As well as a test to cover that~ ;D
On Thu, Aug 20, 2015 at 9:06 AM, Alex Wang <al...@nicira.com> wrote:
>
>
> 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
