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.
> + 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 "&&" ?
> + 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.
> + 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.
> + }
> +
> + 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.
> + 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.
> +
> + 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.
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
