Thx a lot for the review,
On Tue, Jul 21, 2015 at 1:35 PM, Russell Bryant <rbry...@redhat.com> wrote: > 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 .. > Yeah, I think this patch and my last one could just sit in my repo for a bit longer... Once your change is committed, I'll rebase and resubmit~ > > > > > --- > > 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. > Sure, may be I should compose something like ovn-vtep-architecture file? > > > + * > > + */ > > + > > +/* 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 ... sure, will add a check~ > > + /* '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. > > Sure, will do that, > > + "(%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. > > Will change to shash, > > + > > + /* 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
