[ovs-dev] [ovn-controller-vtep V5 09/12] ovn-controller-vtep: Add binding module.
Alex Wang
alexw at nicira.com
Sun Aug 9 07:30:23 UTC 2015
On Fri, Aug 7, 2015 at 3:19 PM, Russell Bryant <rbryant at redhat.com> wrote:
> On 08/07/2015 03:46 AM, Alex Wang wrote:
> > This commit adds the binding module to ovn-controller-vtep. The
> > module will scan through the Port_Binding table in ovnsb. If there is
> > a port binding entry for a logical switch on the vtep gateway chassis's
> > "vtep_logical_switches", sets the port binding's chassis column to the
> > vtep gateway chassis.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
>
> I wanted to send the quick message about the test failures, but here are
> a few inline comments and a question I wanted to ask ...
>
> >
> > ---
> > V4->V5:
> > - rebase on top of master.
> > - change to use port binding type, and options when finding bindings
> > for vtep gateway chassis.
> >
> > V3->V4:
> > - rebase to master.
> >
> > V2->V3:
> > - since ovn-sb schema changes (removal of Gateway table), the binding
> > module code needs to be adapted.
> >
> > PATCH->V2:
> > - split into separate commit.
> > - disallow and warn if more than one logical port from one 'vlan_map'
> > are attached to the same logical datapath.
> > ---
> > ovn/controller-vtep/automake.mk | 2 +
> > ovn/controller-vtep/binding.c | 247
> +++++++++++++++++++++++++++++
> > ovn/controller-vtep/binding.h | 27 ++++
> > ovn/controller-vtep/ovn-controller-vtep.c | 4 +
> > tests/ovn-controller-vtep.at | 112 +++++++++++++
> > 5 files changed, 392 insertions(+)
> > create mode 100644 ovn/controller-vtep/binding.c
> > create mode 100644 ovn/controller-vtep/binding.h
> >
> > diff --git a/ovn/controller-vtep/automake.mk b/ovn/controller-vtep/
> automake.mk
> > index 514cafa..33f063f 100644
> > --- a/ovn/controller-vtep/automake.mk
> > +++ b/ovn/controller-vtep/automake.mk
> > @@ -1,5 +1,7 @@
> > bin_PROGRAMS += ovn/controller-vtep/ovn-controller-vtep
> > ovn_controller_vtep_ovn_controller_vtep_SOURCES = \
> > + ovn/controller-vtep/binding.c \
> > + ovn/controller-vtep/binding.h \
> > ovn/controller-vtep/gateway.c \
> > ovn/controller-vtep/gateway.h \
> > ovn/controller-vtep/ovn-controller-vtep.c \
> > diff --git a/ovn/controller-vtep/binding.c
> b/ovn/controller-vtep/binding.c
> > new file mode 100644
> > index 0000000..fc12006
> > --- /dev/null
> > +++ b/ovn/controller-vtep/binding.c
> > @@ -0,0 +1,247 @@
> > +/* 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 "binding.h"
> > +
> > +#include "lib/shash.h"
> > +#include "lib/smap.h"
> > +#include "lib/util.h"
> > +#include "openvswitch/vlog.h"
> > +#include "ovn-controller-vtep.h"
> > +#include "ovn/lib/ovn-sb-idl.h"
> > +#include "vtep/vtep-idl.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(binding);
> > +
> > +/*
> > + * This module scans through the Port_Binding table in ovnsb. If there
> is a
> > + * logical port binding entry for logical switch in vtep gateway
> chassis's
> > + * 'vtep_logical_switches' column, sets the binding's chassis column to
> the
> > + * corresponding vtep gateway chassis.
> > + *
> > + */
> > +
> > +
> > +/* Returns true if the vtep logical switch specified by
> 'port_binding_rec'
> > + * has already been bound to another port binding entry, and resets
> > + * 'port_binding_rec''s chassis column. Otherwise, updates 'ls_to_pb'
> > + * and returns false. */
> > +static bool
> > +check_pb_conflict(struct shash *ls_to_pb,
> > + const struct sbrec_port_binding *port_binding_rec,
> > + const struct sbrec_chassis *chassis_rec)
> > +{
> > + const char *vtep_lswitch =
> > + smap_get(&port_binding_rec->options, "vtep-logical-switch");
> > + const struct sbrec_port_binding *pb_conflict =
> > + shash_find_data(ls_to_pb, vtep_lswitch);
> > +
> > + if (pb_conflict) {
> > + VLOG_WARN("logical switch (%s), on vtep gateway chassis "
> > + "(%s) has already been associated with logical "
> > + "port (%s), ignore logical port (%s)",
> > + vtep_lswitch, chassis_rec->name,
> > + pb_conflict->logical_port,
> > + port_binding_rec->logical_port);
> > + sbrec_port_binding_set_chassis(port_binding_rec, NULL);
> > +
> > + return true;
> > + }
> > +
> > + shash_replace(ls_to_pb, vtep_lswitch, port_binding_rec);
> > + return false;
> > +}
> > +
> > +/* Returns true if the vtep logical switch specified by
> 'port_binding_rec'
> > + * has already been bound to a different datapath, and resets
> > + * 'port_binding_rec''s chassis column. Otherwise, updates 'ls_to_db'
> and
> > + * returns false. */
> > +static bool
> > +check_db_conflict(struct shash *ls_to_db,
> > + const struct sbrec_port_binding *port_binding_rec,
> > + const struct sbrec_chassis *chassis_rec)
> > +{
> > + const char *vtep_lswitch =
> > + smap_get(&port_binding_rec->options, "vtep-logical-switch");
> > + const struct sbrec_datapath_binding *db_conflict =
> > + shash_find_data(ls_to_db, vtep_lswitch);
> > +
> > + if (db_conflict && db_conflict != port_binding_rec->datapath) {
> > + VLOG_WARN("logical switch (%s), on vtep gateway chassis "
> > + "(%s) has already been associated with logical "
> > + "datapath (with tunnel key %"PRId64"), ignore "
> > + "logical port (%s) which belongs to logical "
> > + "datapath (with tunnel key %"PRId64")",
> > + vtep_lswitch, chassis_rec->name,
> > + db_conflict->tunnel_key,
> > + port_binding_rec->logical_port,
> > + port_binding_rec->datapath->tunnel_key);
> > + sbrec_port_binding_set_chassis(port_binding_rec, NULL);
> > +
> > + return true;
> > + }
> > +
> > + shash_replace(ls_to_db, vtep_lswitch, port_binding_rec->datapath);
> > + return false;
> > +}
> > +
> > +/* Updates the 'port_binding_rec''s chassis column to 'chassis_rec'. */
> > +static void
> > +update_pb_chassis(const struct sbrec_port_binding *port_binding_rec,
> > + const struct sbrec_chassis *chassis_rec)
> > +{
> > + if (port_binding_rec->chassis != chassis_rec) {
> > + if (chassis_rec && port_binding_rec->chassis) {
> > + VLOG_DBG("Changing chassis association of logical "
> > + "port (%s) from (%s) to (%s)",
> > + port_binding_rec->logical_port,
> > + port_binding_rec->chassis->name,
> > + chassis_rec->name);
> > + }
> > + sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec);
> > + }
> > +}
> > +
> > +
> > +/* Checks and updates logical port to vtep logical switch bindings for
> each
> > + * physical switch in VTEP. */
> > +void
> > +binding_run(struct controller_vtep_ctx *ctx)
> > +{
> > + const struct vteprec_physical_switch *pswitch;
> > +
> > + if (!ctx->ovnsb_idl_txn) {
> > + return;
> > + }
> > +
> > + /* 'ls_to_db'
> > + *
> > + * Maps vtep logical switch name to the datapath binding entry.
> This is
> > + * used to guarantee that each vtep logical switch is only included
> > + * in only one ovn datapath (ovn logical switch). See
> check_db_conflict()
> > + * for details.
> > + *
> > + * 'ls_to_pb'
> > + *
> > + * Maps vtep logical switch name to the port binding entry. This
> is used
> > + * to guarantee that each vtep logical switch on a vtep physical
> switch
> > + * is only bound to one logical port. See check_pb_conflict() for
> > + * details.
> > + *
> > + */
> > + struct shash ls_to_db = SHASH_INITIALIZER(&ls_to_db);
> > + const struct vteprec_logical_switch *vtep_ls;
> > + VTEPREC_LOGICAL_SWITCH_FOR_EACH (vtep_ls, ctx->vtep_idl) {
> > + shash_add(&ls_to_db, vtep_ls->name, NULL);
> > + }
> > +
> > + ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
> > + "ovn-controller-vtep: updating bindings");
> > + VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> > + const struct sbrec_chassis *chassis_rec
> > + = get_chassis_by_name(ctx->ovnsb_idl, pswitch->name);
>
> I would make sure chassis_rec is not NULL here. It's not supposed to
> happen, but just in case ...
>
Since we do not call ovsdb_idl_run() anywhere in the *_run() functions,
and gateway_run() is called before bindnig_run(), this should never happen.
How about using ovs_assert(chassis_rec);
>
> > + struct shash ls_to_pb = SHASH_INITIALIZER(&ls_to_pb);
> > + const struct sbrec_port_binding *port_binding_rec;
> > + size_t i;
> > +
> > + for (i = 0; i < chassis_rec->n_vtep_logical_switches; i++) {
> > + shash_add(&ls_to_pb, chassis_rec->vtep_logical_switches[i],
> > + NULL);
> > + }
> > +
> > + SBREC_PORT_BINDING_FOR_EACH(port_binding_rec, ctx->ovnsb_idl) {
>
> How often would you have more than 1 Physical_Switch in a vtep db? I
> ask because the port binding list could be quite long. Instead of
> iterating the whole thing multiple times, you could iterate it once and
> build a hash table of vtep port binding records hashed on the physical
> switch and logical switch.
>
> I may be trying to micro-optimize here, though. Anyway, just a thought.
>
>
It should be fairly common to see multiple physical switches in one vtep
database... I think I know how to optimize this using shash,
> > + const char *type = port_binding_rec->type;
> > + const char *vtep_pswitch =
> smap_get(&port_binding_rec->options,
> > + "vtep-physical-switch");
> > + const char *vtep_lswitch =
> smap_get(&port_binding_rec->options,
> > + "vtep-logical-switch");
> > +
> > + /* Port binding for vtep gateway chassis must have type
> "vtep",
> > + * and matched physical switch name and logical switch
> name. */
> > + if (!strcmp(type, "vtep")
> > + && vtep_pswitch && !strcmp(vtep_pswitch,
> chassis_rec->name)
> > + && vtep_lswitch && shash_find(&ls_to_pb, vtep_lswitch))
> {
> > + bool pb_conflict, db_conflict;
> > +
> > + pb_conflict = check_pb_conflict(&ls_to_pb,
> port_binding_rec,
> > + chassis_rec);
> > + db_conflict = check_db_conflict(&ls_to_db,
> port_binding_rec,
> > + chassis_rec);
> > + /* Updates port binding's chassis column when there
> > + * is no conflict. */
> > + if (!pb_conflict && !db_conflict) {
> > + update_pb_chassis(port_binding_rec, chassis_rec);
> > + }
> > + } else if (port_binding_rec->chassis == chassis_rec) {
> > + update_pb_chassis(port_binding_rec, NULL);
> > + }
> > + }
> > + struct shash_node *node;
> > + SHASH_FOR_EACH (node, &ls_to_pb) {
> > + if (!node->data) {
> > + VLOG_DBG("No port binding entry for logical switch (%s)"
> > + "on vtep gateway chassis (%s)", node->name,
> > + chassis_rec->name);
>
> This could get pretty noisy. I guess it's only a debug message, but
> maybe it's worth rate limiting anyway?
>
>
Sure, I'll rate-limit it,
> + }
> > + }
> > + shash_destroy(&ls_to_pb);
> > + }
> > + shash_destroy(&ls_to_db);
> > +}
> > +
> > +/* Removes all port binding association with vtep gateway chassis.
> > + * Returns true when all done. */
> > +bool
> > +binding_cleanup(struct controller_vtep_ctx *ctx)
> > +{
> > + struct shash ch_to_pb = SHASH_INITIALIZER(&ch_to_pb);
> > + const struct sbrec_port_binding *port_binding_rec;
> > +
> > + if (!ctx->ovnsb_idl_txn) {
>
> There's no bug here, but I thought there might be at first because
> ch_to_pb is initialized but not destroyed. It turns out that doesn't
> matter, but my brain just expects them to always match up. :-)
>
> Maybe you could move this check to the very top of the function?
>
>
Absolutely, adopt your advice,
Thanks,
Alex Wang,
> --
> Russell Bryant
>
More information about the dev
mailing list