[ovs-dev] [ovn-controller-vtep V4 5/6] ovn-controller-vtep: Add binding module.
Alex Wang
alexw at nicira.com
Wed Jul 22 17:13:41 UTC 2015
Thx a lot for the review,
On Tue, Jul 21, 2015 at 1:35 PM, Russell Bryant <rbryant at 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 <alexw at 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
>
More information about the dev
mailing list