On Fri, Aug 7, 2015 at 2:19 PM, Russell Bryant <rbry...@redhat.com> wrote:
> On 08/07/2015 03:46 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 (Recorded in TODO file):
> >
> > - Do not support reading multiple tunnel ips of physical switch.
> >
> > Signed-off-by: Alex Wang <al...@nicira.com>
> >
> > ---
> > V4->V5:
> > - rebase on top of master.
> > - adopt Russell's review suggestions, greatly simplify the code.
> >
> > 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/TODO | 6 +
> > ovn/controller-vtep/automake.mk | 2 +
> > ovn/controller-vtep/gateway.c | 224
> ++++++++++++++++++++
> > .../{ovn-controller-vtep.h => gateway.h} | 19 +-
> > ovn/controller-vtep/ovn-controller-vtep.c | 43 +++-
> > ovn/controller-vtep/ovn-controller-vtep.h | 20 ++
> > tests/ovn-controller-vtep.at | 151 +++++++++++++
> > 7 files changed, 445 insertions(+), 20 deletions(-)
> > create mode 100644 ovn/controller-vtep/gateway.c
> > copy ovn/controller-vtep/{ovn-controller-vtep.h => gateway.h} (65%)
> >
> > diff --git a/ovn/TODO b/ovn/TODO
> > index 509e5d0..356b3ba 100644
> > --- a/ovn/TODO
> > +++ b/ovn/TODO
> > @@ -74,3 +74,9 @@
> > Epstein et al., "What's the Difference? Efficient Set
> > Reconciliation Without Prior Context". (I'm not yet aware of
> > previous non-academic use of this technique.)
> > +
> > +** Support multiple tunnel encapsulations in Chassis.
> > +
> > + So far, both ovn-controller and ovn-controller-vtep only allow
> > + chassis to have one tunnel encapsulation entry. We should extend
> > + the implementation to support multiple tunnel encapsulations.
> > 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..5f6da27
> > --- /dev/null
> > +++ b/ovn/controller-vtep/gateway.c
> > @@ -0,0 +1,224 @@
> > +/* 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/poll-loop.h"
> > +#include "lib/simap.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 unsigned int gw_reval_seq;
> > +
> > +/* Maps all chassis created by the gateway module to their own
> reval_seq. */
> > +static struct simap gw_chassis_map = SIMAP_INITIALIZER(&gw_chassis_map);
> > +
> > +/* 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;
> > +
> > + /* Increments the global revalidation sequence number. */
> > + gw_reval_seq++;
> > +
> > + ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
> > + "ovn-controller-vtep: updating vtep
> chassis");
> > +
> > + VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> > + const struct sbrec_chassis *chassis_rec;
> > + struct simap_node *gw_node;
> > + const char *encap_ip;
> > +
> > + encap_ip = pswitch->n_tunnel_ips ? pswitch->tunnel_ips[0] : "";
> > + gw_node = simap_find(&gw_chassis_map, pswitch->name);
> > + chassis_rec = get_chassis_by_name(ctx->ovnsb_idl,
> pswitch->name);
> > + if (gw_node && !chassis_rec) {
> > + VLOG_WARN("Chassis for VTEP physical switch (%s)
> disappears, "
> > + "maybe deleted by ovn-sbctl, adding it back",
> > + pswitch->name);
> > + create_chassis_rec(ctx->ovnsb_idl_txn, pswitch->name,
> encap_ip);
> > + gw_node->data = gw_reval_seq;
> > + } else if (!gw_node && chassis_rec) {
> > + VLOG_WARN("Chassis for new VTEP physical switch (%s) has
> already "
> > + "been added, maybe we just restarted or someone"
> > + "added it by ovn-sbctl", pswitch->name);
>
> It looks like this will happen under normal circumstances every time
> ovn-controller-vtep restarts. If that's right, I would either remove
> this message or just make it a debug log entry.
Makes sense, I'll just drop it~
>
> > + if (strcmp(chassis_rec->encaps[0]->type, OVN_SB_ENCAP_TYPE)
> > + && strcmp(chassis_rec->encaps[0]->ip, encap_ip)) {
>
> Did you intend to have "||" here instead of "&&" ?
>
>
Thx for pointing this out~! I do want to use "||"~
> > + 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);
>
> How about the case where an administrator has intentionally changed the
> IP while ovn-controller-vtep was shut down?
>
> If instead the IP was changed while ovn-controller-vtep was still
> running, you'd hit the next case, which would just happily silently
> apply the IP address change.
>
> Really, I think the handling for this case should match what's done for
> "gw_node && chassis_rec", except you need to simap_add() instead of
> setting the value in gw_node.
>
>
That's a good point and example,
> > + continue;
> > + }
> > + simap_put(&gw_chassis_map, pswitch->name, gw_reval_seq);
> > + } else if (gw_node && chassis_rec) {
> > + /* Updates chassis's encap if anything changed. */
> > + if (strcmp(chassis_rec->encaps[0]->type,
> OVN_SB_ENCAP_TYPE)) {
> > + 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);
> > + }
> > + gw_node->data = gw_reval_seq;
> > + } else {
> > + /* Creates a new chassis for the VTEP physical switch and a
> new
> > + * record in 'gw_chassis_map'. */
> > + create_chassis_rec(ctx->ovnsb_idl_txn, pswitch->name,
> encap_ip);
> > + simap_put(&gw_chassis_map, pswitch->name, gw_reval_seq);
> > + }
>
> It seems the above 4 could be written as:
>
> if (!chassis_rec) {
> create_chassis_rec(...);
> } else {
> ... check for and apply changes to chassis_rec ...
> }
>
> if (!gw_node) {
> simap_put(&gw_chassis_map, pswitch->name, gw_reval_seq);
> } else {
> gw_node->data = gw_reval_seq;
> }
>
> I think that would simplify it a bit.
>
>
Thx for this suggestion, I'd like to adopt!
> > + }
> > +
> > + struct simap_node *iter, *next;
> > + /* For 'gw_node' in 'gw_chassis_map' whose data is not
> > + * 'gw_reval_seq', it means the corresponding physical switch no
> > + * longer exist. So, garbage collects them. */
> > + SIMAP_FOR_EACH_SAFE (iter, next, &gw_chassis_map) {
> > + if (iter->data != 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);
> > + }
> > + simap_delete(&gw_chassis_map, iter);
> > + }
> > + }
> > +}
> > +
> > +/* 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;
> > +
> > + ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_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 sset lswitches = SSET_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 *vtep_lswitch;
> > +
> > + vtep_lswitch = port->value_vlan_bindings[j];
> > + /* If not already in 'lswitches', records it. */
> > + if (!sset_find(&lswitches, vtep_lswitch->name)) {
> > + sset_add(&lswitches, vtep_lswitch->name);
> > + }
> > + }
> > + }
> > +
> > + const char **ls_arr = sset_array(&lswitches);
> > + sbrec_chassis_set_vtep_logical_switches(chassis_rec, ls_arr,
> > + sset_count(&lswitches));
>
> You've got a memory leak here. The result of sset_array() should be
> freed here.
>
>
Thx for pointing this out, fixed it,
> > + sset_destroy(&lswitches);
> > + }
> > +}
> > +
> > +
> > +void
> > +gateway_run(struct controller_vtep_ctx *ctx)
> > +{
> > + if (!ctx->ovnsb_idl_txn) {
> > + return;
> > + }
> > +
> > + revalidate_gateway(ctx);
> > + update_vtep_logical_switches(ctx);
> > +}
> > +
> > +/* Destroys the chassis table entries for vtep physical switches.
> > + * Returns true when all done. */
> > +bool
> > +gateway_cleanup(struct controller_vtep_ctx *ctx)
> > +{
> > + const struct vteprec_physical_switch *pswitch;
> > +
> > + if (!ctx->ovnsb_idl_txn) {
> > + return false;
> > + }
> > +
> > + bool all_done = true;
> > + ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_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;
> > + }
> > + all_done = false;
> > + sbrec_chassis_delete(chassis_rec);
> > + }
> > + simap_destroy(&gw_chassis_map);
>
> I believe if gateway_cleanup() returns false, when gateway_cleanup()
> gets called again, simap_destroy() called again here will result in a
> double free() in hmap_destroy(), if the simap had more than one element
> in it.
>
> It looks like you can either make sure you only destroy() once, or make
> hmap_destroy() safe to be called more than once.
>
>
Thx for pointing this out~
Since ovn-controller-vtep is single threaded, I'll make it call
hmap_destory()
only once.
> > +
> > + return all_done;
> > +}
> > diff --git a/ovn/controller-vtep/ovn-controller-vtep.h
> b/ovn/controller-vtep/gateway.h
> > similarity index 65%
> > copy from ovn/controller-vtep/ovn-controller-vtep.h
> > copy to ovn/controller-vtep/gateway.h
>
> The diff still shows this as a copy instead of a new file. I suppose it
> doesn't really hurt anything, though. It just makes the diff a little
> weird.
>
I think this relation is added by git... I reset the commit, readded file,
and ran commit, it still showed that this file is copied from
ovn/controller-vtep/ovn-controller-vtep.h.
Thanks,
Alex Wang,
>
> --
> Russell Bryant
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
