On 07/16/2015 03:56 AM, Alex Wang wrote: > This commit adds the binding module to ovn-controller-vtep. The > module will scan through the Binding table in ovnsb. If there is > a binding for a logical port in the vtep gateway chassis's > "vtep_logical_switches" map, sets the binding's chassis column to the > vtep gateway chassis. > > Signed-off-by: Alex Wang <al...@nicira.com>
As discussed before, I'd like to see this done using logical port type and options instead of a special name, but this could be reworked later if it merges before the addition of type and options. A few comments inline .. > > --- > 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 | 194 > +++++++++++++++++++++++++++++ > ovn/controller-vtep/binding.h | 25 ++++ > ovn/controller-vtep/ovn-controller-vtep.c | 3 + > tests/ovn-controller-vtep.at | 48 +++++++ > 5 files changed, 272 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..caf2a86 > --- /dev/null > +++ b/ovn/controller-vtep/binding.c > @@ -0,0 +1,194 @@ > +/* 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/hash.h" > +#include "lib/sset.h" > +#include "lib/util.h" > +#include "lib/uuid.h" > +#include "openvswitch/vlog.h" > +#include "ovn/lib/ovn-sb-idl.h" > +#include "vtep/vtep-idl.h" > +#include "ovn-controller-vtep.h" > + > +VLOG_DEFINE_THIS_MODULE(binding); > + > +/* > + * This module scans through the Binding table in ovnsb. If there is a > + * row for the logical port on vtep gateway chassis's 'vtep_logical_switches' > + * map sets the binding's chassis column to the vtep gateway chassis. > + * > + * Caution: each logical datapath can only have up to one logical port > + * attached to it from each vtep gateway. This is an important thing to note in docs somewhere. For now I suppose ovn-nb.xml is the best place. > + * > + */ > + > +/* Checks and updates bindings for each physical switch in VTEP. */ > +void > +binding_run(struct controller_vtep_ctx *ctx) > +{ > + const struct vteprec_physical_switch *pswitch; > + struct ovsdb_idl_txn *txn; > + int retval; > + > + txn = ovsdb_idl_txn_create(ctx->ovnsb_idl); > + ovsdb_idl_txn_add_comment(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); > + const struct sbrec_binding *binding_rec; > + struct sset attached_ldps; > + struct sset gw_lports; > + struct smap_node *iter; > + const char *name; > + How about checking to make sure chassis_rec is non-NULL here? I think it shouldn't be possible today, but just in case ... > + /* 'attached_ldps' is used to guarantee that each logical datapath > + * can only have up to one logical port attached from the same > + * gateway chassis. > + * > + * If a lport is first added to a logical datapath, we add the > + * logical datapath's uuid to 'attached_ldps' as string. Then for > + * each following lport, we always first check if the logical > + * datapath has already been attached, and warn if it has. > + * (since it is not allowed)! > + * > + */ > + sset_init(&attached_ldps); > + sset_init(&gw_lports); > + /* Collects all logical port names on the vtep gateway. */ > + SMAP_FOR_EACH (iter, &chassis_rec->vtep_logical_switches) { > + sset_add(&gw_lports, iter->value); > + } > + > + SBREC_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { > + /* Finds a binding entry for the lport. */ > + if (sset_find_and_delete(&gw_lports, binding_rec->logical_port)) > { > + char *ldp_uuid; > + > + /* Converts uuid to string. */ > + ldp_uuid = xasprintf(UUID_FMT, > + > UUID_ARGS(&binding_rec->logical_datapath)); > + /* Warns if the logical datapath has already > + * been attached. */ > + if (sset_find(&attached_ldps, ldp_uuid)) { > + VLOG_WARN("Logical datapath ("UUID_FMT") already has " > + "logical port from the same chassis " It might help to clarify that "chassis" is a "gateway" or "vtep" chassis in this log message. It confused me at first. > + "(%s) attached to it, so clear the " > + "chassis column from binding (%s)", > + UUID_ARGS(&binding_rec->logical_datapath), > + chassis_rec->name, > + binding_rec->logical_port); > + sbrec_binding_set_chassis(binding_rec, NULL); > + } else { > + if (binding_rec->chassis != chassis_rec) { > + if (binding_rec->chassis) { > + VLOG_DBG("Changing chassis for lport (%s) from " > + "(%s) to (%s)", > + binding_rec->logical_port, > + binding_rec->chassis->name, > + chassis_rec->name); > + } > + sbrec_binding_set_chassis(binding_rec, chassis_rec); > + } > + /* Records the attachment in 'attached_ldps'. */ > + sset_add(&attached_ldps, ldp_uuid); > + } > + free(ldp_uuid); > + } else if (binding_rec->chassis == chassis_rec) { > + /* The logical port no longer exist, so clear > + * the binding->chassis. */ > + sbrec_binding_set_chassis(binding_rec, NULL); > + } > + } > + SSET_FOR_EACH (name, &gw_lports) { > + VLOG_DBG("No binding record for lport %s", name); > + } > + sset_destroy(&attached_ldps); > + sset_destroy(&gw_lports); > + } > + > + retval = ovsdb_idl_txn_commit_block(txn); > + if (retval == TXN_ERROR) { > + VLOG_INFO("Problem committing binding information: %s", > + ovsdb_idl_txn_status_to_string(retval)); > + } > + ovsdb_idl_txn_destroy(txn); > +} > + > +/* Removes the chassis reference for each binding to the vtep gateway. */ > +void > +binding_destroy(struct controller_vtep_ctx *ctx) > +{ > + struct hmap bd_map = HMAP_INITIALIZER(&bd_map); > + const struct sbrec_binding *binding_rec; > + int retval = TXN_TRY_AGAIN; > + > + struct binding_hash_node { > + struct hmap_node hmap_node; /* Inside 'bd_map'. */ > + const struct sbrec_binding *binding; > + }; I think the shash API would be a little easier here. > + > + /* Collects all bindings with chassis. */ > + SBREC_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { > + if (binding_rec->chassis) { > + struct binding_hash_node *bd = xmalloc(sizeof *bd); > + > + bd->binding = binding_rec; > + hmap_insert(&bd_map, &bd->hmap_node, > + hash_string(binding_rec->chassis->name, 0)); > + } > + } > + > + while (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) { > + const struct vteprec_physical_switch *pswitch; > + struct ovsdb_idl_txn *txn; > + > + txn = ovsdb_idl_txn_create(ctx->ovnsb_idl); > + ovsdb_idl_txn_add_comment(txn, "ovn-controller-vtep: removing > 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); > + struct binding_hash_node *bd; > + > + HMAP_FOR_EACH_WITH_HASH (bd, hmap_node, > + hash_string(chassis_rec->name, 0), > + &bd_map) { > + if (!strcmp(bd->binding->chassis->name, chassis_rec->name)) { > + sbrec_binding_set_chassis(bd->binding, NULL); > + } > + } > + } > + > + retval = ovsdb_idl_txn_commit_block(txn); > + if (retval == TXN_ERROR) { > + VLOG_DBG("Problem removing binding: %s", > + ovsdb_idl_txn_status_to_string(retval)); > + } > + ovsdb_idl_txn_destroy(txn); > + } > + > + struct binding_hash_node *iter, *next; > + > + HMAP_FOR_EACH_SAFE (iter, next, hmap_node, &bd_map) { > + hmap_remove(&bd_map, &iter->hmap_node); > + free(iter); > + } > + hmap_destroy(&bd_map); > +} > diff --git a/ovn/controller-vtep/binding.h b/ovn/controller-vtep/binding.h > new file mode 100644 > index 0000000..156465d > --- /dev/null > +++ b/ovn/controller-vtep/binding.h > @@ -0,0 +1,25 @@ > +/* 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_BINDING_H > +#define OVN_BINDING_H 1 > + > +struct controller_vtep_ctx; > + > +void binding_run(struct controller_vtep_ctx *); > +void binding_destroy(struct controller_vtep_ctx *); > + > +#endif /* ovn/controller-gw/binding.h */ > diff --git a/ovn/controller-vtep/ovn-controller-vtep.c > b/ovn/controller-vtep/ovn-controller-vtep.c > index dbc754b..712116a 100644 > --- a/ovn/controller-vtep/ovn-controller-vtep.c > +++ b/ovn/controller-vtep/ovn-controller-vtep.c > @@ -38,6 +38,7 @@ > #include "unixctl.h" > #include "util.h" > > +#include "binding.h" > #include "gateway.h" > #include "ovn-controller-vtep.h" > > @@ -123,6 +124,7 @@ main(int argc, char *argv[]) > } > > gateway_run(&ctx); > + binding_run(&ctx); > unixctl_server_run(unixctl); > > unixctl_server_wait(unixctl); > @@ -137,6 +139,7 @@ main(int argc, char *argv[]) > > unixctl_server_destroy(unixctl); > gateway_destroy(&ctx); > + binding_destroy(&ctx); > > ovsdb_idl_destroy(ctx.vtep_idl); > ovsdb_idl_destroy(ctx.ovnsb_idl); > diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at > index 747e3ed..a0cae26 100644 > --- a/tests/ovn-controller-vtep.at > +++ b/tests/ovn-controller-vtep.at > @@ -150,3 +150,51 @@ AT_CHECK([ovn-sbctl --columns=vtep_logical_switches list > Chassis | cut -d ':' -f > > OVN_CONTROLLER_VTEP_STOP(["/Chassis for VTEP physical switch (br-vtep) > disappears/d"]) > AT_CLEANUP > + > + > +# Tests binding updates. > +AT_SETUP([ovn-controller-vtep - test binding]) > +OVN_CONTROLLER_VTEP_START > + > +# adds logical switch 'lswitch0' and vlan_bindings. > +AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0 -- > bind-ls br-vtep p1 300 lswitch0]) > +# adds lport in ovn-nb db. > +AT_CHECK_UNQUOTED([ovn-nbctl lport-add br-test br-vtep_lswitch0]) > +OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Binding | grep br-vtep_lswitch0`"]) > +# should see one binding. > +chassis_uuid=$(ovn-sbctl --columns=_uuid list Chassis br-vtep | cut -d ':' > -f2 | tr -d ' ') > +AT_CHECK_UNQUOTED([ovn-sbctl --columns=chassis list Binding br-vtep_lswitch0 > | cut -d ':' -f2 | tr -d ' '], [0], [dnl > +${chassis_uuid} > +]) > + > +# adds another logical switch 'lswitch1' and vlan_bindings. > +AT_CHECK([vtep-ctl add-ls lswitch1 -- bind-ls br-vtep p0 200 lswitch1]) > +# adds lport in ovn-nb db to the same logical switch. > +AT_CHECK_UNQUOTED([ovn-nbctl lport-add br-test br-vtep_lswitch1]) > +OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Binding | grep -- > br-vtep_lswitch1`"]) > +# should still see one binding, since it is not allowed to have more than > +# one logical port from same chassis attached to the same logical datapath > +# (logical switch in ovn-nb database). > +AT_CHECK_UNQUOTED([ovn-sbctl --columns=chassis list Binding | cut -d ':' -f2 > | tr -d ' ' | sort -d], [0], [dnl > + > +[[]] > +${chassis_uuid} > +]) > +# confirms the warning log. > +AT_CHECK([sed -n 's/^.*\(|WARN|.*\)$/\1/p' ovn-controller-vtep.log | sed > 's/([[-_0-9a-z]][[-_0-9a-z]]*)/()/g' | uniq], [0], [dnl > +|WARN|Logical datapath () already has logical port from the same chassis () > attached to it, so clear the chassis column from binding () > +]) > + > +# deletes physical ports from vtep. > +AT_CHECK([ovs-vsctl del-port p0 -- del-port p1]) > +AT_CHECK([vtep-ctl del-port br-vtep p0 -- del-port br-vtep p1]) > +OVS_WAIT_UNTIL([test -z "`ovn-sbctl list Chassis | grep -- > br-vtep_lswitch`"]) > +# should see empty chassis column in both binding entries. > +AT_CHECK_UNQUOTED([ovn-sbctl --columns=chassis list Binding | cut -d ':' -f2 > | tr -d ' ' | sort], [0], [dnl > + > +[[]] > +[[]] > +]) > + > +OVN_CONTROLLER_VTEP_STOP(["/already has logical port from the same > chassis/d"]) > +AT_CLEANUP > -- Russell Bryant _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
