On Wed, Jul 5, 2017 at 9:45 AM, Miguel Angel Ajo <majop...@redhat.com> wrote:
> The redirect-chassis option of logical router ports is now
> translated to Gateway_Chassis entries for backwards compatibility.
>
> Gateway_Chassis entries in nbdb are copied over to sbdb and
> linked them to the Chassis entry.
>
> Signed-off-by: Miguel Angel Ajo <majop...@redhat.com>
> Signed-off-by: Anil Venkata <vkomm...@redhat.com>
> ---
> ovn/lib/automake.mk | 2 +
> ovn/lib/chassis-index.c | 84 ++++++++++++++++++
> ovn/lib/chassis-index.h | 40 +++++++++
> ovn/northd/ovn-northd.c | 224
> ++++++++++++++++++++++++++++++++++++++++++++++--
> tests/ovn.at | 52 +++++++++++
> 5 files changed, 393 insertions(+), 9 deletions(-)
> create mode 100644 ovn/lib/chassis-index.c
> create mode 100644 ovn/lib/chassis-index.h
>
> diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk
> index b86237e..9ad8b6a 100644
> --- a/ovn/lib/automake.mk
> +++ b/ovn/lib/automake.mk
> @@ -5,6 +5,8 @@ ovn_lib_libovn_la_LDFLAGS = \
> $(AM_LDFLAGS)
> ovn_lib_libovn_la_SOURCES = \
> ovn/lib/actions.c \
> + ovn/lib/chassis-index.c \
> + ovn/lib/chassis-index.h \
> ovn/lib/expr.c \
> ovn/lib/lex.c \
> ovn/lib/ovn-dhcp.h \
> diff --git a/ovn/lib/chassis-index.c b/ovn/lib/chassis-index.c
> new file mode 100644
> index 0000000..e1d6cb3
> --- /dev/null
> +++ b/ovn/lib/chassis-index.c
> @@ -0,0 +1,84 @@
> +/* Copyright (c) 2016, 2017 Red Hat, 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 <stdlib.h>
This include probably isn't needed.
> +
> +#include "openvswitch/hmap.h"
> +#include "openvswitch/vlog.h"
> +#include "ovn/actions.h"
> +#include "ovn/lib/chassis-index.h"
> +#include "ovn/lib/ovn-sb-idl.h"
> +
> +VLOG_DEFINE_THIS_MODULE(chassis_index);
> +
> +struct chassis {
> + struct hmap_node name_node;
> + const struct sbrec_chassis *db;
> +};
> +
> +const struct sbrec_chassis *
> +chassis_lookup_by_name(const struct chassis_index *chassis_index,
> + const char *name)
> +{
> + const struct chassis *chassis;
> + HMAP_FOR_EACH_WITH_HASH (chassis, name_node, hash_string(name, 0),
> + &chassis_index->by_name) {
> + if (!strcmp(chassis->db->name, name)) {
> + return chassis->db;
> + }
> + }
> + return NULL;
> +}
> +
> +void
> +chassis_index_init(struct chassis_index *chassis_index,
> + struct ovsdb_idl *sb_idl)
> +{
> + hmap_init(&chassis_index->by_name);
> +
> + const struct sbrec_chassis *chassis;
> + SBREC_CHASSIS_FOR_EACH (chassis, sb_idl) {
> + if (!chassis->name) {
> + continue;
> + }
> + if (chassis_lookup_by_name(chassis_index, chassis->name)) {
> + VLOG_WARN("duplicate chassis name '%s'",
> + chassis->name);
> + continue;
> + }
I don't think you need this duplicate check. The OVN_Southbound
schema already enforces that Chassis rows have a unique name. See
ovn-sb.ovsschema:
"indexes": [["name"]]},
> + struct chassis *c = xmalloc(sizeof *c);
> + hmap_insert(&chassis_index->by_name, &c->name_node,
> + hash_string(chassis->name, 0));
> + c->db = chassis;
> + }
> +}
> +
> +void
> +chassis_index_destroy(struct chassis_index *chassis_index)
> +{
> + if (!chassis_index) {
> + return;
> + }
> +
> + /* Destroy all of the "struct chassis"s. */
> + struct chassis *chassis, *next;
> + HMAP_FOR_EACH_SAFE (chassis, next, name_node, &chassis_index->by_name) {
> + hmap_remove(&chassis_index->by_name, &chassis->name_node);
> + free(chassis);
> + }
> +
> + hmap_destroy(&chassis_index->by_name);
> +}
> diff --git a/ovn/lib/chassis-index.h b/ovn/lib/chassis-index.h
> new file mode 100644
> index 0000000..a490cbb
> --- /dev/null
> +++ b/ovn/lib/chassis-index.h
> @@ -0,0 +1,40 @@
> +/* Copyright (c) 2017, Red Hat, 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_CHASSIS_INDEX_H
> +#define OVN_CHASSIS_INDEX_H 1
> +
> +#include "openvswitch/hmap.h"
> +
> +struct chassis_index {
> + struct hmap by_name;
> +};
Why the struct here? Were you thinking you may want to add other
indexes in the future? It's OK with me, just curious really ...
> +
> +struct ovsdb_idl;
> +
> +/* Finds and returns the chassis with the given 'name', or NULL if no such
> + * chassis exists. */
> +const struct sbrec_chassis *
> +chassis_lookup_by_name(const struct chassis_index *chassis_index,
> + const char *name);
> +
> +/* Initializes the chassis index out of the ovsdb_idl to SBDB */
> +void chassis_index_init(struct chassis_index *chassis_index,
> + struct ovsdb_idl *sb_idl);
> +
> +/* Free a chassis index from memory */
> +void chassis_index_destroy(struct chassis_index *chassis_index);
> +
> +#endif /* ovn/lib/chassis-index.h */
I think this API is fine for now. It would be nice to have some of
this provided by the IDL layer. I believe Lance Richardson just
revived an older series that will help. Perhaps we could look at
using that later.
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index be3b371..36ece23 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -28,6 +28,7 @@
> #include "openvswitch/hmap.h"
> #include "openvswitch/json.h"
> #include "ovn/lex.h"
> +#include "ovn/lib/chassis-index.h"
> #include "ovn/lib/logical-fields.h"
> #include "ovn/lib/ovn-dhcp.h"
> #include "ovn/lib/ovn-nb-idl.h"
> @@ -1453,7 +1454,7 @@ join_logical_ports(struct northd_context *ctx,
>
> const char *redirect_chassis = smap_get(&op->nbrp->options,
> "redirect-chassis");
> - if (redirect_chassis) {
> + if (redirect_chassis || op->nbrp->n_gateway_chassis) {
> /* Additional "derived" ovn_port crp represents the
> * instance of op on the "redirect-chassis". */
> const char *gw_chassis = smap_get(&op->od->nbr->options,
> @@ -1678,8 +1679,137 @@ get_nat_addresses(const struct ovn_port *op, size_t
> *n)
> return addresses;
> }
>
> +static bool
> +sbpb_gw_chassis_needs_update__(
The "__" suffix is probably not necessary here. It's useful if you
have a library file that has a mix of public APIs and internal helper
code. In ovn-northd, where everything is static anyway, I'm not sure
it helps clarify anything.
> + const struct sbrec_port_binding *port_binding,
> + const struct nbrec_logical_router_port *lrp,
> + const struct chassis_index *chassis_index)
> +{
> + if (!lrp || !port_binding) {
> + return false;
> + }
> +
> + /* These arrays are used to collect valid Gateway_Chassis and valid
> + * Chassis records from the Logical_Router_Port Gateway_Chassis list,
> + * we ignore the ones we can't match on the SBDB */
> + struct nbrec_gateway_chassis **lrp_gwc = xzalloc(lrp->n_gateway_chassis *
> + sizeof *lrp_gwc);
> + const struct sbrec_chassis **lrp_gwc_c = xzalloc(lrp->n_gateway_chassis *
> + sizeof *lrp_gwc_c);
> +
> + /* Count the number of gateway chassis chassis names from the logical
> + * router port that we are able to match on the southbound database */
> + int lrp_n_gateway_chassis = 0;
> + int n;
> + for (n=0; n < lrp->n_gateway_chassis; n++) {
Minor style tweak - add spaces around "=":
for (n = 0; n < ...
> +
> + if (!lrp->gateway_chassis[n]->chassis_name) {
> + continue;
> + }
> +
> + const struct sbrec_chassis *chassis =
> + chassis_lookup_by_name(chassis_index,
> + lrp->gateway_chassis[n]->chassis_name);
> +
> + if (chassis) {
> + lrp_gwc_c[lrp_n_gateway_chassis] = chassis;
> + lrp_gwc[lrp_n_gateway_chassis] = lrp->gateway_chassis[n];
> + lrp_n_gateway_chassis++;
> + }
Should we emit a rate limited log warning here that a chassis was
configured in the northbound database that doesn't exist?
> + }
> +
> + /* Basic check, different amount of Gateway_Chassis means that we
> + * need to update southbound database Port_Binding */
> + if (lrp_n_gateway_chassis != port_binding->n_gateway_chassis) {
> + free(lrp_gwc_c);
> + free(lrp_gwc);
> + return true;
> + }
> +
> + for (n=0; n < lrp_n_gateway_chassis; n++) {
Spaces around "=" here.
> + int i;
> + /* For each of the valid gw chassis on the lrp, check if there's
> + * a match on the Port_Binding list, we assume order is not
> + * persisted */
> + for (i=0; i < port_binding->n_gateway_chassis; i++) {
Add spaces around "=".
> + struct sbrec_gateway_chassis *pb_gwc =
> + port_binding->gateway_chassis[i];
> + if (!strcmp(lrp_gwc[n]->name, pb_gwc->name) &&
> + !strcmp(lrp_gwc_c[n]->name, pb_gwc->chassis->name) &&
> + lrp_gwc[n]->priority == pb_gwc->priority &&
> + smap_equal(&lrp_gwc[n]->options, &pb_gwc->options) &&
> + smap_equal(&lrp_gwc[n]->external_ids,
> &pb_gwc->external_ids)) {
A gateway_chassis_cmp() helper function would be nice here.
> + break; /* we found a match */
> + }
> + }
> +
> + /* if no Port_Binding gateway chassis matched for the entry... */
> + if (i >= port_binding->n_gateway_chassis) {
It could never be >, right? only == ?
> + free(lrp_gwc_c);
> + free(lrp_gwc);
> + return true; /* found no match for this gateway chassis on lrp */
> + }
> + }
> +
> + /* no need for update, all ports matched */
> + free(lrp_gwc_c);
> + free(lrp_gwc);
> + return false;
> +}
> +
> +/* This functions translates the gw chassis on the nb database
> + * to sb database entries, the only difference is that SB database
> + * Gateway_Chassis table references the chassis directly instead
> + * of using the name */
> static void
> -ovn_port_update_sbrec(const struct ovn_port *op,
> +copy_gw_chassis_from_nbrp_to_sbpb__(
> + struct northd_context *ctx,
> + const struct nbrec_logical_router_port *lrp,
> + const struct chassis_index *chassis_index,
> + const struct sbrec_port_binding *port_binding) {
> +
> + if (!lrp || !port_binding || !lrp->n_gateway_chassis) {
> + return;
> + }
> +
> + struct sbrec_gateway_chassis **gw_chassis = NULL;
> + int n_gwc = 0;
> + int n;
> +
> + for (n=0; n < lrp->n_gateway_chassis; n++) {
Add spaces around "=".
> + struct nbrec_gateway_chassis *lrp_gwc = lrp->gateway_chassis[n];
> + if (!lrp_gwc->chassis_name) {
> + continue;
> + }
> +
> + const struct sbrec_chassis *chassis =
> + chassis_lookup_by_name(chassis_index, lrp_gwc->chassis_name);
> +
> + if (!chassis) {
Consider logging a warning here. I suggested the same thing above.
We don't need it in both places, though.
> + continue;
> + }
> +
> + gw_chassis = xrealloc(gw_chassis, (n_gwc + 1) * sizeof *gw_chassis);
> +
> + struct sbrec_gateway_chassis *pb_gwc =
> + sbrec_gateway_chassis_insert(ctx->ovnsb_txn);
> +
> + sbrec_gateway_chassis_set_name(pb_gwc, lrp_gwc->name);
> + sbrec_gateway_chassis_set_priority(pb_gwc, lrp_gwc->priority);
> + sbrec_gateway_chassis_set_chassis(pb_gwc, chassis);
> + sbrec_gateway_chassis_set_options(pb_gwc, &lrp_gwc->options);
> + sbrec_gateway_chassis_set_external_ids(pb_gwc,
> &lrp_gwc->external_ids);
> +
> + gw_chassis[n_gwc++] = pb_gwc;
> + }
> + sbrec_port_binding_set_gateway_chassis(port_binding, gw_chassis, n_gwc);
> + free(gw_chassis);
If there's a change in any gateway_chassis, this code issues a
transaction to replace *all* gateway_chassis associated with that
router port in the southbound database. It seems like this will
introduce more churn than necessary in the southbound database. Could
we update this to only add/update/delete the rows in the southbound db
that need to change?
> +}
> +
> +static void
> +ovn_port_update_sbrec(struct northd_context *ctx,
> + const struct ovn_port *op,
> + const struct chassis_index *chassis_index,
> struct hmap *chassis_qdisc_queues)
> {
> sbrec_port_binding_set_datapath(op->sb, op->od->sb);
> @@ -1700,8 +1830,66 @@ ovn_port_update_sbrec(const struct ovn_port *op,
> if (op->derived) {
> const char *redirect_chassis = smap_get(&op->nbrp->options,
> "redirect-chassis");
> - if (redirect_chassis) {
> + if (op->nbrp->n_gateway_chassis && redirect_chassis) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1,
> 1);
> + VLOG_WARN_RL(
> + &rl, "logical router port %s has both options:"
> + "redirect-chassis and gateway_chassis populated "
> + "redirect-chassis will be ignored in favour of "
> + "gateway chassis", op->nbrp->name);
> + }
> +
> + if (op->nbrp->n_gateway_chassis) {
> + if (sbpb_gw_chassis_needs_update__(op->sb, op->nbrp,
> + chassis_index)) {
> + copy_gw_chassis_from_nbrp_to_sbpb__(ctx, op->nbrp,
> + chassis_index,
> op->sb);
> + }
> +
> + } else if (redirect_chassis) {
> + /* XXX: Keep the "redirect-chassis" option on the
> Port_Binding
> + * for compatibility purposes until ovn-controller implements
> + * Gateway_Chassis handling */
This could be solved by rearranging the patch series, right? If the
ovn-controller support came first and we put the ovn-northd changes
last, you wouldn't have to copy the "redirect-chassis" option here
anymore.
> smap_add(&new, "redirect-chassis", redirect_chassis);
> +
> + /* Handle ports that had redirect-chassis option attached
> + * to them for backwards compatibility */
> + const struct sbrec_chassis *chassis =
> + chassis_lookup_by_name(chassis_index, redirect_chassis);
> + if (chassis) {
> + /* If we found the chassis, and the gw chassis on record
> + * differs from what we expect go ahead and update */
> + if (op->sb->n_gateway_chassis !=1 ||
> + strcmp(op->sb->gateway_chassis[0]->chassis->name,
> + chassis->name) ||
> + op->sb->gateway_chassis[0]->priority != 0) {
Style - I would start lines with the boolean operators, something like:
if (op->sb->n_gateway_chassis != 1
|| strcmp(op->sb->gateway_chassis[0]->chassis->name, chassis->name)
|| op->sb->gateway_chassis[0]->priority != 0) {
> + /* Construct a single Gateway_Chassis entry on the
> + * Port_Binding attached to the redirect_chassis
> + * name */
> + struct sbrec_gateway_chassis *gw_chassis =
> + sbrec_gateway_chassis_insert(ctx->ovnsb_txn);
> +
> + char *gwc_name = xasprintf("%s_%s", op->nbrp->name,
> + chassis->name);
> +
> + sbrec_gateway_chassis_set_name(gw_chassis, gwc_name);
> + sbrec_gateway_chassis_set_priority(gw_chassis, 0);
> + sbrec_gateway_chassis_set_chassis(gw_chassis,
> chassis);
> + sbrec_gateway_chassis_set_external_ids(gw_chassis,
> + &op->nbrp->external_ids);
> + sbrec_port_binding_set_gateway_chassis(op->sb,
> + &gw_chassis,
> 1);
> + free(gwc_name);
Instead of always replacing the gateway_chassis row, how about
updating it as necessary if it's already there?
> + }
> + } else {
> + VLOG_WARN("chassis name '%s' from redirect from logical "
> + " router port '%s' redirect-chassis not found",
> + redirect_chassis, op->nbrp->name);
> + if (op->sb->n_gateway_chassis) {
> + sbrec_port_binding_set_gateway_chassis(op->sb, NULL,
> + 0);
> + }
> + }
> }
> smap_add(&new, "distributed-port", op->nbrp->name);
> } else {
> @@ -1845,7 +2033,7 @@ cleanup_mac_bindings(struct northd_context *ctx, struct
> hmap *ports)
> * datapaths. */
> static void
> build_ports(struct northd_context *ctx, struct hmap *datapaths,
> - struct hmap *ports)
> + const struct chassis_index *chassis_index, struct hmap *ports)
> {
> struct ovs_list sb_only, nb_only, both;
> struct hmap tag_alloc_table = HMAP_INITIALIZER(&tag_alloc_table);
> @@ -1863,7 +2051,7 @@ build_ports(struct northd_context *ctx, struct hmap
> *datapaths,
> if (op->nbsp) {
> tag_alloc_create_new_tag(&tag_alloc_table, op->nbsp);
> }
> - ovn_port_update_sbrec(op, &chassis_qdisc_queues);
> + ovn_port_update_sbrec(ctx, op, chassis_index, &chassis_qdisc_queues);
>
> add_tnlid(&op->od->port_tnlids, op->sb->tunnel_key);
> if (op->sb->tunnel_key > op->od->port_key_hint) {
> @@ -1879,7 +2067,7 @@ build_ports(struct northd_context *ctx, struct hmap
> *datapaths,
> }
>
> op->sb = sbrec_port_binding_insert(ctx->ovnsb_txn);
> - ovn_port_update_sbrec(op, &chassis_qdisc_queues);
> + ovn_port_update_sbrec(ctx, op, chassis_index, &chassis_qdisc_queues);
>
> sbrec_port_binding_set_logical_port(op->sb, op->key);
> sbrec_port_binding_set_tunnel_key(op->sb, tunnel_key);
> @@ -5606,14 +5794,15 @@ sync_dns_entries(struct northd_context *ctx, struct
> hmap *datapaths)
>
>
> static void
> -ovnnb_db_run(struct northd_context *ctx, struct ovsdb_idl_loop *sb_loop)
> +ovnnb_db_run(struct northd_context *ctx, struct chassis_index *chassis_index,
> + struct ovsdb_idl_loop *sb_loop)
> {
> if (!ctx->ovnsb_txn || !ctx->ovnnb_txn) {
> return;
> }
> struct hmap datapaths, ports;
> build_datapaths(ctx, &datapaths);
> - build_ports(ctx, &datapaths, &ports);
> + build_ports(ctx, &datapaths, chassis_index, &ports);
> build_ipam(&datapaths, &ports);
> build_lflows(ctx, &datapaths, &ports);
>
> @@ -6183,6 +6372,17 @@ main(int argc, char *argv[])
> add_column_noalert(ovnsb_idl_loop.idl,
> &sbrec_port_binding_col_nat_addresses);
> ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_port_binding_col_chassis);
> + ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> + &sbrec_port_binding_col_gateway_chassis);
> + ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> + &sbrec_gateway_chassis_col_chassis);
> + ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_gateway_chassis_col_name);
> + ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> + &sbrec_gateway_chassis_col_priority);
> + ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> + &sbrec_gateway_chassis_col_external_ids);
> + ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> + &sbrec_gateway_chassis_col_options);
> add_column_noalert(ovnsb_idl_loop.idl,
> &sbrec_port_binding_col_external_ids);
> ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_mac_binding);
> @@ -6223,6 +6423,7 @@ main(int argc, char *argv[])
>
> ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis);
> ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_nb_cfg);
> + ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_name);
>
> /* Main loop. */
> exiting = false;
> @@ -6234,7 +6435,10 @@ main(int argc, char *argv[])
> .ovnsb_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
> };
>
> - ovnnb_db_run(&ctx, &ovnsb_idl_loop);
> + struct chassis_index chassis_index;
> + chassis_index_init(&chassis_index, ctx.ovnsb_idl);
> +
> + ovnnb_db_run(&ctx, &chassis_index, &ovnsb_idl_loop);
> ovnsb_db_run(&ctx, &ovnsb_idl_loop);
> if (ctx.ovnsb_txn) {
> check_and_add_supported_dhcp_opts_to_sb_db(&ctx);
> @@ -6254,6 +6458,8 @@ main(int argc, char *argv[])
> if (should_service_stop()) {
> exiting = true;
> }
> +
> + chassis_index_destroy(&chassis_index);
> }
>
> unixctl_server_destroy(unixctl);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index efcbd91..b1dcd6a 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -7496,3 +7496,55 @@ done
> OVN_CLEANUP([hv1],[hv2])
>
> AT_CLEANUP
> +
> +AT_SETUP([ovn -- check Gateway_Chassis propagation from NBDB to SBDB])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +ovn_start
> +
> +ovn-nbctl create Logical_Router name=R1
> +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> +ovn-sbctl chassis-add gw2 geneve 1.2.4.8
> +
> +# Connect alice to R1 as distributed router gateway port on hv2
> +ovn-nbctl lrp-add R1 alice 00:00:02:01:02:03 172.16.1.1/24
> +
> +
> +ovn-nbctl \
> + --id=@gc0 create Gateway_Chassis name=alice_gw1 \
> + chassis_name=gw1 \
> + priority=20 -- \
> + --id=@gc1 create Gateway_Chassis name=alice_gw2 \
> + chassis_name=gw2 \
> + priority=10 -- \
> + set Logical_Router_Port alice 'gateway_chassis=[@gc0,@gc1]'
> +
> +
> +# Allow some time for ovn-northd and ovn-controller to catch up.
> +# XXX This should be more systematic.
I think you can drop the "XXX" comment here. You're doing an explcit
sync. You also shouldn't need the "sleep 2".
You can also replace the sync command by just adding "--wait=sb" to
your last ovn-nbctl call. It doesn't have to be a standalone "sync"
call.
> +ovn-nbctl --wait=sb sync
> +sleep 2
> +
> +gwc1_uuid=`ovn-sbctl --bare --columns _uuid find Gateway_Chassis
> name="alice_gw1"`
> +gwc2_uuid=`ovn-sbctl --bare --columns _uuid find Gateway_Chassis
> name="alice_gw2"`
> +
> +AT_CHECK([ovn-sbctl --bare --columns gateway_chassis find port_binding
> logical_port="cr-alice" | grep $gwc1_uuid | wc -l], [0], [1
> +])
> +AT_CHECK([ovn-sbctl --bare --columns gateway_chassis find port_binding
> logical_port="cr-alice" | grep $gwc2_uuid | wc -l], [0], [1
> +])
> +
> +# Create Logical_Router_Port with redirect-chassis option
> +ovn-nbctl lrp-del alice
> +ovn-nbctl lrp-add R1 alice 00:00:02:01:02:03 172.16.1.1/24 \
> + -- set Logical_Router_Port alice options:redirect-chassis="gw1"
> +ovn-nbctl --wait=sb sync
Not a big deal, but I believe you can just add "--wait=sb" to your last lrp-add.
> +
> +# It should be converted to Gateway_Chassis entries in SBDB, and
> +# still redirect-chassis is kept for backwards compatibility
> +
> +gwc1_uuid=`ovn-sbctl --bare --columns _uuid find Gateway_Chassis
> name="alice_gw1"`
> +AT_CHECK([ovn-sbctl --bare --columns gateway_chassis find port_binding
> logical_port="cr-alice" | grep $gwc1_uuid | wc -l], [0], [1
> +])
I think it'd be a little more clear if the name was different here to
be really clear that it could have only worked if "redirect-chassis"
is working. Right now it looks like the same check as above, even
though I know you delete the router port in between. You could also
verify that the delete worked first.
> +AT_CHECK([ovn-sbctl --bare --columns options find port_binding
> logical_port="cr-alice" | grep 'redirect-chassis=gw1' | wc -l], [0], [1
> +])
> +AT_CLEANUP
> +
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
Russell Bryant
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
