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 <al...@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 ... > + 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. > + 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? > + } > + } > + 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? -- Russell Bryant _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
