Thx a lot for the review, Russell,
I had few crazy days with debugging + customer support, now back to dev~
So, let's see,
On Tue, Jul 21, 2015 at 12:51 PM, Russell Bryant <rbry...@redhat.com> wrote:
> On 07/16/2015 03:56 AM, Alex Wang wrote:
> > This commit adds the gateway module to ovn-controller-vtep. The
> > module will register the physical switches to ovnsb as chassis and
> > constantly update the "vtep_logical_switches" column in Chassis table.
> >
> > Limitation:
> >
> > - Do not support reading multiple tunnel ips of physical switch.
> >
>
> It's probably worth noting in the TODO file, unless we have a better way
> to track it.
>
Sure, this is actually true for both ovn-controller-vtep, and ovn-controller
will mention both.
>
> > Signed-off-by: Alex Wang <al...@nicira.com>
> >
> > ---
> > V3->V4:
> > - rebase to master.
> >
> > V2->V3:
> > - since ovn-sb schema changes (removal of Gateway table), the gateway
> > module code needs to be adapted.
> > - rebase to master.
> >
> > PATCH->V2:
> > - split into separate commit.
> > - can register all physical switches controlled by vtep database.
> > ---
> > ovn/controller-vtep/automake.mk | 2 +
> > ovn/controller-vtep/gateway.c | 281
> ++++++++++++++++++++
> > .../{ovn-controller-vtep.h => gateway.h} | 15 +-
> > ovn/controller-vtep/ovn-controller-vtep.c | 3 +
> > ovn/controller-vtep/ovn-controller-vtep.h | 18 ++
> > tests/ovn-controller-vtep.at | 151 +++++++++++
> > 6 files changed, 461 insertions(+), 9 deletions(-)
> > create mode 100644 ovn/controller-vtep/gateway.c
> > copy ovn/controller-vtep/{ovn-controller-vtep.h => gateway.h} (72%)
> >
> > diff --git a/ovn/controller-vtep/automake.mk b/ovn/controller-vtep/
> automake.mk
> > index 7adda15..514cafa 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/gateway.c \
> > + ovn/controller-vtep/gateway.h \
> > ovn/controller-vtep/ovn-controller-vtep.c \
> > ovn/controller-vtep/ovn-controller-vtep.h
> > ovn_controller_vtep_ovn_controller_vtep_LDADD = ovn/lib/libovn.la lib/
> libopenvswitch.la vtep/libvtep.la
> > diff --git a/ovn/controller-vtep/gateway.c
> b/ovn/controller-vtep/gateway.c
> > new file mode 100644
> > index 0000000..c2c1a3f
> > --- /dev/null
> > +++ b/ovn/controller-vtep/gateway.c
> > @@ -0,0 +1,281 @@
> > +/* 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 "gateway.h"
> > +
> > +#include "lib/hash.h"
> > +#include "lib/hmap.h"
> > +#include "lib/poll-loop.h"
> > +#include "lib/sset.h"
> > +#include "lib/util.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(gateway);
> > +
> > +/*
> > + * Registers the physical switches in vtep to ovnsb as chassis. For
> each
> > + * physical switch in the vtep database, finds all vtep logical
> switches that
> > + * are associated with the physical switch, and updates the
> corresponding
> > + * chassis's 'vtep_logical_switches' column.
> > + *
> > + */
> > +
> > +/* Global revalidation sequence number, incremented at each call to
> > + * 'revalidate_gateway()'. */
> > +static uint64_t gw_reval_seq;
> > +
> > +/* Represents a chassis added by the gateway module. The 'reval_seq'
> > + * is increment each time a chassis is revalidated. Chassis whose
> 'reval_seq'
> > + * not equal to 'gw_reval_seq' will be removed. */
> > +struct gw_chassis {
> > + struct hmap_node hmap_node; /* In 'gw_chassis'. */
> > + char *name; /* Name of the Chassis. */
> > + uint64_t reval_seq; /* Chassis revalidation sequence
> number. */
> > +};
> > +
> > +/* Contains all chassis created by the gateway module. */
> > +static struct hmap gw_chassis_map = HMAP_INITIALIZER(&gw_chassis_map);
>
> I think the shash API is a slightly more convenient way to implement
> what you have here. It wraps hmap for the common case of a string hash
> key + custom data.
>
>
Also react to your next email, yeah we should use simap! will change
accordingly.
> > +
> > +/* Searchs 'gw_chassis_map' for chassis 'name' and returns the pointer.
> > + * Returns NULL, if the chassis is not found. */
> > +static struct gw_chassis *
> > +get_gw_chassis(const char *name)
> > +{
> > + struct gw_chassis *gw_chassis;
> > +
> > + HMAP_FOR_EACH_WITH_HASH (gw_chassis, hmap_node, hash_string(name,
> 0),
> > + &gw_chassis_map) {
> > + if (!strcmp(gw_chassis->name, name)) {
> > + return gw_chassis;
> > + }
> > + }
> > +
> > + return NULL;
>
> Continuing on the shash suggestion, this function would just become:
>
> return shash_find(&gw_chassis_map, name);
>
> and the return value would become "uint64_t *".
>
> (I'm noting what the conversion looks like to make sure I can convince
> myself the suggestion makes sense.)
>
>
I'll change the corresponding functions of simap,
>
> > +}
> > +
> > +/* Creates and returns a new instance of 'struct sbrec_chassis'. */
> > +static const struct sbrec_chassis *
> > +create_chassis_rec(struct ovsdb_idl_txn *txn, const char *name,
> > + const char *encap_ip)
> > +{
> > + const struct sbrec_chassis *chassis_rec;
> > + struct sbrec_encap *encap_rec;
> > +
> > + chassis_rec = sbrec_chassis_insert(txn);
> > + sbrec_chassis_set_name(chassis_rec, name);
> > + encap_rec = sbrec_encap_insert(txn);
> > + sbrec_encap_set_type(encap_rec, OVN_SB_ENCAP_TYPE);
> > + sbrec_encap_set_ip(encap_rec, encap_ip);
> > + sbrec_chassis_set_encaps(chassis_rec, &encap_rec, 1);
> > +
> > + return chassis_rec;
> > +}
> > +
> > +/* Revalidates chassis in ovnsb against vtep database. Creates chassis
> for
> > + * new vtep physical switch. And removes chassis which no longer have
> > + * physical switch in vtep.
> > + *
> > + * xxx: Support multiple tunnel encaps.
> > + *
> > + * */
> > +static void
> > +revalidate_gateway(struct controller_vtep_ctx *ctx)
> > +{
> > + const struct vteprec_physical_switch *pswitch;
> > + struct ovsdb_idl_txn *txn;
> > + struct gw_chassis *iter, *next;
> > + int retval;
> > +
> > + /* Increments the global revalidation sequence number. */
> > + gw_reval_seq++;
> > +
> > + txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
> > + ovsdb_idl_txn_add_comment(txn, "ovn-controller-vtep: updating vtep
> chassis");
> > +
> > + VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> > + const struct sbrec_chassis *chassis_rec;
> > + struct gw_chassis *gw_chassis;
> > + const char *encap_ip;
> > +
> > + encap_ip = pswitch->n_tunnel_ips ? pswitch->tunnel_ips[0] : "";
> > + gw_chassis = get_gw_chassis(pswitch->name);
> > + chassis_rec = get_chassis_by_name(ctx->ovnsb_idl,
> pswitch->name);
> > + if (gw_chassis && !chassis_rec) {
> > + VLOG_WARN("Chassis for VTEP physical switch (%s)
> disappears, "
> > + "maybe deleted by ovn-sbctl, adding it back",
> > + pswitch->name);
> > + create_chassis_rec(txn, pswitch->name, encap_ip);
> > + } else if (!gw_chassis && chassis_rec) {
> > + VLOG_WARN("Chassis for new VTEP physical switch (%s) has
> already "
> > + "been added, maybe by ovn-sbctl", pswitch->name);
>
> Or how about a restart of ovn-controller-vtep? It seems like that would
> cause this case to be hit, as well. If so, it's probably only worth a
> debug message, if at all.
>
>
Thx, I'll mention that as well,
> > + if (strcmp(chassis_rec->encaps[0]->type, OVN_SB_ENCAP_TYPE)
> > + && strcmp(chassis_rec->encaps[0]->ip, encap_ip)) {
> > + VLOG_WARN("Chassis config changing on startup, make
> sure "
> > + "multiple chassis are not configured :
> %s/%s->%s/%s",
> > + chassis_rec->encaps[0]->type,
> > + chassis_rec->encaps[0]->ip,
> > + OVN_SB_ENCAP_TYPE, encap_ip);
> > + VLOG_WARN("Skip adding chassis for physical switch
> (%s)",
> > + pswitch->name);
> > + continue;
> > + }
> > + gw_chassis = xmalloc(sizeof *gw_chassis);
> > + gw_chassis->name = xstrdup(pswitch->name);
> > + hmap_insert(&gw_chassis_map, &gw_chassis->hmap_node,
> > + hash_string(gw_chassis->name, 0));
>
> With shash, the above lines would become something like:
>
> gw_chassis_reval_seq = xmalloc(sizeof *reval_seq);
> shash_add(&hw_chassis_map, pswitch->name, gw_chassis_reval_seq);
>
>
yeah, will change to use the simap functions,
> > + } else if (gw_chassis && chassis_rec) {
> > + /* Updates chassis's encap if anything changed. */
> > + if (strcmp(chassis_rec->encaps[0]->type,
> OVN_SB_ENCAP_TYPE)) {
>
> This could only happen if modified outside of ovn-controller-vtep,
> right? The saftey check still seems fine, though.
>
>
Exactly, like someone messing up with ovn-sb database using ovn-sbctl.
> > + VLOG_WARN("Chassis for VTEP physical switch (%s) can
> only have "
> > + "encap type \"%s\"", pswitch->name,
> OVN_SB_ENCAP_TYPE);
> > + sbrec_encap_set_type(chassis_rec->encaps[0],
> OVN_SB_ENCAP_TYPE);
> > + }
> > + if (strcmp(chassis_rec->encaps[0]->ip, encap_ip)) {
> > + sbrec_encap_set_ip(chassis_rec->encaps[0], encap_ip);
> > + }
> > + } else {
> > + /* Creates a new chassis for the VTEP physical switch and a
> new
> > + * gw_chassis record. */
> > + create_chassis_rec(txn, pswitch->name, encap_ip);
> > + gw_chassis = xmalloc(sizeof *gw_chassis);
> > + gw_chassis->name = xstrdup(pswitch->name);
> > + hmap_insert(&gw_chassis_map, &gw_chassis->hmap_node,
> > + hash_string(gw_chassis->name, 0));
> > + }
> > + /* Updates the 'gw_chassis's revalidation seq number to prevent
> > + * it from being garbage collected. */
> > + gw_chassis->reval_seq = gw_reval_seq;
> > + }
> > +
> > + /* For 'gw_chassis' in 'gw_chassis_map' whose reval_seq is not
> > + * 'gw_reval_seq', it means the corresponding physical switch no
> > + * longer exist. So, garbage collects them. */
> > + HMAP_FOR_EACH_SAFE (iter, next, hmap_node, &gw_chassis_map) {
> > + if (iter->reval_seq != gw_reval_seq) {
> > + const struct sbrec_chassis *chassis_rec;
> > +
> > + chassis_rec = get_chassis_by_name(ctx->ovnsb_idl,
> iter->name);
> > + if (chassis_rec) {
> > + sbrec_chassis_delete(chassis_rec);
> > + }
> > + hmap_remove(&gw_chassis_map, &iter->hmap_node);
> > + free(iter->name);
> > + free(iter);
> > + }
> > + }
> > +
> > + retval = ovsdb_idl_txn_commit_block(txn);
> > + if (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
> > + VLOG_INFO("Problem registering chassis: %s",
> > + ovsdb_idl_txn_status_to_string(retval));
> > + poll_immediate_wake();
> > + }
> > + ovsdb_idl_txn_destroy(txn);
> > +}
> > +
> > +/* Updates the 'vtep_logical_switches' column in the Chassis table based
> > + * on vtep database configuration. */
> > +static void
> > +update_vtep_logical_switches(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 chassis's
> vtep_logical_switches");
> > +
> > + 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 smap lswitches = SMAP_INITIALIZER(&lswitches);
> > + size_t i;
> > +
> > + for (i = 0; i < pswitch->n_ports; i++) {
> > + const struct vteprec_physical_port *port =
> pswitch->ports[i];
> > + size_t j;
> > +
> > + for (j = 0; j < port->n_vlan_bindings; j++) {
> > + const struct vteprec_logical_switch *ls;
> > +
> > + ls = port->value_vlan_bindings[j];
> > + /* If not already in 'lswitches', adds the logical
> switch
> > + * to logical port map. The logical port is defined as
> > + * {pswitch->name}_{ls->name}. */
> > + if (!smap_get(&lswitches, ls->name)) {
> > + char *lport = xasprintf("%s_%s", pswitch->name,
> ls->name);
> > +
> > + smap_add(&lswitches, ls->name, lport);
> > + free(lport);
>
> Is there a reason you have to iterate the physical switches -> ports ->
> vlan bindings to figure out the logical switches vs. just iterating the
> logical switches directly with VTEPREC_LOGICAL_SWITCH_FOR_EACH? Is it
> that you only want to map logical switches that are actually in use?
>
>
Yes exactly, I want to map logical switches that are already in use.
But, giving it a second thought, I think it does not really matter to ovn
if the
logical switch is in use or not. We could always send pkts to the physical
switch even though it does not use the logical switch. The physical switch
will simply drop the pkts. And iteration using
VTEPREC_LOGICAL_SWITCH_FOR_EACH is simpler. Right?
I'll make the change,
> > + }
> > + }
> > + }
> > + sbrec_chassis_set_vtep_logical_switches(chassis_rec,
> &lswitches);
> > + smap_destroy(&lswitches);
> > + }
> > +
> > + retval = ovsdb_idl_txn_commit_block(txn);
> > + if (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
> > + VLOG_INFO("Problem updating chassis's vtep_logical_switches:
> %s",
> > + ovsdb_idl_txn_status_to_string(retval));
> > + poll_immediate_wake();
> > + }
> > + ovsdb_idl_txn_destroy(txn);
> > +}
> > +
> > +
> > +void
> > +gateway_run(struct controller_vtep_ctx *ctx)
> > +{
> > + revalidate_gateway(ctx);
> > + update_vtep_logical_switches(ctx);
> > +}
> > +
> > +/* Destroys the chassis table entries of the vtep physical switches. */
> > +void
> > +gateway_destroy(struct controller_vtep_ctx *ctx)
> > +{
> > + int retval = TXN_TRY_AGAIN;
> > +
> > + ovs_assert(ctx->ovnsb_idl);
> > + while (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
> > + const struct vteprec_physical_switch *pswitch;
> > + struct ovsdb_idl_txn *txn =
> ovsdb_idl_txn_create(ctx->ovnsb_idl);
> > +
> > + ovsdb_idl_txn_add_comment(txn,
> > + "ovn-controller-vtep: "
> > + "unregistering vtep chassis");
> > + VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> > + const struct sbrec_chassis *chassis_rec;
> > +
> > + chassis_rec = get_chassis_by_name(ctx->ovnsb_idl,
> pswitch->name);
> > + if (!chassis_rec) {
> > + continue;
> > + }
> > + sbrec_chassis_delete(chassis_rec);
> > + }
>
> Instead of iterating over the vtep pswitches, I think iterating of
> gw_chassis_map is what will ensure that you delete every chassis created
> by this controller. As written it seems like it's possible to miss some
> if the vtep switch configuration has just changed.
>
I'm not sure there is a difference between iterating gw_chassis_map and
just the physical switches. Since we only run ovsdb_idl_run() to update
local
view of vtep database at beginning of while loop. So, if there is a race
between vtep config change and ovn-controller-vtep exit, the vtep change is
either ignored or updated in local view. In the latter case,
ovn-controller-gw
will still call gateway_run() first and then gateway_destroy(). And at the
time
gateway_destroy() is called, the gw_chassis_map should contain the same
thing as local vtep pswitches.
>
> > + retval = ovsdb_idl_txn_commit_block(txn);
> > + if (retval == TXN_ERROR) {
> > + VLOG_INFO("Problem unregistering chassis: %s",
> > + ovsdb_idl_txn_status_to_string(retval));
> > + }
> > + ovsdb_idl_txn_destroy(txn);
> > + }
>
> I think gw_chassis_map and its contents need to be destroyed here.
> Destroying the contents could be done above as you iterate through it to
> delete the associated chassis records.
>
>
Yeah, I will clean it up,
> > +}
> > diff --git a/ovn/controller-vtep/ovn-controller-vtep.h
> b/ovn/controller-vtep/gateway.h
> > similarity index 72%
> > copy from ovn/controller-vtep/ovn-controller-vtep.h
> > copy to ovn/controller-vtep/gateway.h
> > index 121a9e3..edc895e 100644
> > --- a/ovn/controller-vtep/ovn-controller-vtep.h
> > +++ b/ovn/controller-vtep/gateway.h
>
> This came out kinda weird. gateway.h isn't actually realted to
> ovn-controller-vtep.h, so it'd be nice if it showed up as a new file.
>
>
Yeah, it is, maybe I copied and modified... will try creating new file
again,
> > @@ -13,15 +13,12 @@
> > * limitations under the License.
> > */
> >
> > +#ifndef OVN_GATEWAY_H
> > +#define OVN_GATEWAY_H 1
> >
> > -#ifndef OVN_CONTROLLER_VTEP_H
> > -#define OVN_CONTROLLER_VTEP_H 1
> > +struct controller_vtep_ctx;
> >
> > -#include "ovn/lib/ovn-sb-idl.h"
> > +void gateway_run(struct controller_vtep_ctx *);
> > +void gateway_destroy(struct controller_vtep_ctx *);
> >
> > -struct controller_vtep_ctx {
> > - struct ovsdb_idl *ovnsb_idl;
> > - struct ovsdb_idl *vtep_idl;
> > -};
> > -
> > -#endif /* ovn/ovn-controller-vtep.h */
> > +#endif /* ovn/controller-gw/gateway.h */
> > diff --git a/ovn/controller-vtep/ovn-controller-vtep.c
> b/ovn/controller-vtep/ovn-controller-vtep.c
> > index 50d727f..dbc754b 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 "gateway.h"
> > #include "ovn-controller-vtep.h"
> >
> > VLOG_DEFINE_THIS_MODULE(ovn_vtep);
> > @@ -121,6 +122,7 @@ main(int argc, char *argv[])
> > break;
> > }
> >
> > + gateway_run(&ctx);
> > unixctl_server_run(unixctl);
> >
> > unixctl_server_wait(unixctl);
> > @@ -134,6 +136,7 @@ main(int argc, char *argv[])
> > }
> >
> > unixctl_server_destroy(unixctl);
> > + gateway_destroy(&ctx);
> >
> > ovsdb_idl_destroy(ctx.vtep_idl);
> > ovsdb_idl_destroy(ctx.ovnsb_idl);
> > diff --git a/ovn/controller-vtep/ovn-controller-vtep.h
> b/ovn/controller-vtep/ovn-controller-vtep.h
> > index 121a9e3..f00d3c4 100644
> > --- a/ovn/controller-vtep/ovn-controller-vtep.h
> > +++ b/ovn/controller-vtep/ovn-controller-vtep.h
> > @@ -24,4 +24,22 @@ struct controller_vtep_ctx {
> > struct ovsdb_idl *vtep_idl;
> > };
> >
> > +/* VTEP needs what VTEP needs. */
> > +#define OVN_SB_ENCAP_TYPE "vxlan"
> > +#define VTEP_ENCAP_TYPE "vxlan_over_ipv4"
> > +
> > +static inline const struct sbrec_chassis *
> > +get_chassis_by_name(struct ovsdb_idl *ovnsb_idl, char *chassis_id)
>
> I'd make this "const char *chassis_id".
>
>
Sure, it should be 'const'
> > +{
> > + const struct sbrec_chassis *chassis_rec;
> > +
> > + SBREC_CHASSIS_FOR_EACH(chassis_rec, ovnsb_idl) {
> > + if (!strcmp(chassis_rec->name, chassis_id)) {
> > + break;
> > + }
> > + }
> > +
> > + return chassis_rec;
> > +}
>
> I'm curious, why did you choose to inline this?
>
>
Because, originally, I have this same code at many places in gateway and
binding modules.
> I think this probably does enough work that inlining doesn't seem all
> that valuable. You also have another very similar type of function,
> get_gw_chassis(), that's not inlined.
>
> My insinct would be to not inline this one, but let me know if I'm
> missing something.
>
>
Sure, I'll define them as static,
> --
> Russell Bryant
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
