[ovs-dev,v3,2/6] ovn: l3ha, ovn-northd gateway chassis propagation

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 <majopela@redhat.com>
Signed-off-by: Anil Venkata <vkommadi@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

On Wed, Jul 5, 2017 at 9:45 AM, Miguel Angel Ajo <majopela@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 <majopela@redhat.com>
> Signed-off-by: Anil Venkata <vkommadi@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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Wed, Jul 5, 2017 at 9:45 AM, Miguel Angel Ajo <majopela@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 <majopela@redhat.com>
> Signed-off-by: Anil Venkata <vkommadi@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
>


> +
> +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.
> +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
> +
> +# 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
> +])
> +AT_CHECK([ovn-sbctl --bare --columns options find port_binding logical_port="cr-alice" | grep 'redirect-chassis=gw1' | wc -l], [0], [1
> +])
> +AT_CLEANUP
> +

One more thought...

tests/ovn.at has gotten really, really big.  I've been thinking about
splitting it up into some smaller pieces.  This test is pretty focused
on testing some behavior of ovn-northd, so perhaps it could be the
first test in a new file: tests/ovn-northd.at.

In addition to creating the new file, you'd have to edit:

tests/automake.mk
tests/testsuite.at

For both files, look for "tests/ovn.at" for where you need to add the new file.

(re-adding mailing list, since I assume you meant to include it)

On Fri, Jul 7, 2017 at 7:15 AM, Miguel Angel Ajo Pelayo
<majopela@redhat.com> wrote:
>
>
> On Thu, Jul 6, 2017 at 11:30 PM, Russell Bryant <russell@ovn.org> wrote:
>>
>> On Wed, Jul 5, 2017 at 9:45 AM, Miguel Angel Ajo <majopela@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 <majopela@redhat.com>
>> > Signed-off-by: Anil Venkata <vkommadi@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.
>
>
> good catch, yes I don't believe we need that.
>
>>
>>
>> > +
>> > +#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"]]},
>
>
>    ack , great :)
>
>>
>>
>> > +        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 ...
>
>
> Yes, I was following the pattern on the other indexes we have over
> ovn-controller, so we could eventually add other keys (host, etc..) without
> the need to go over all the "chassis_index" references all around.
>
> I was also thinking, that we could refactor ovn-controller a bit (at some
> point), by collecting all the state (indexes, etc..) into a single state
> struct, in a way that we don't need to pass around tons of parameters, which
> basically are state.
>
> Hopefully the index structs will go away the day Lance's patch for runtime
> indexes is available :)

OK, sounds good.

Regarding passing around state, there is struct controller_ctx that we
could consider packing more into.

>
>>
>>
>> > +
>> > +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.
>
>
> Yes, I saw it, looks great, this is another use case for the API. :)
>
>>
>>
>> > 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.
>
>
> ok, so we use the __ suffix for non-static functions, makes sense.

Well, I'd use them for static functions mixed up with non-static functions.

>
>>
>>
>> > +        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 < ...
>
>
> hmm, may be I should look at checkpatch to add that check since I
> consistently repeat it :)

Yes, that'd be a great addition.

>>
>>
>> > +
>> > +        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?
>
>
> Ack, that makes sense.
>
>>
>>
>> > +    }
>> > +
>> > +    /* 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.
>
>
> ack! :)
>
>>
>>
>> > +                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 == ?
>
>
> Certainly it couldn't, I have an habit of defensively checking for those
> conditions,
> but it's miss leading for the reader, I'll change it :)

It wasn't misleading to me, but it did catch my OCD.

>
>>
>>
>> > +            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.
>
> ack
>>
>>
>> > +            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?
>
>
> I can give it a try and see how it looks, I was going that way, but I traded
> off
> for simplicity, as I didn't expect this to be an operation that happens very
> often.
>
> Do you mind if, for now, I just comment at that point that there's room for
> improvement/optimization in that area?
>
> I'd be happy to go back to it as soon as everything else in the series is
> ready.

Sure, you can leave a comment about it.  Maybe it's one of the pieces
I can pick up after finishing a review pass.

>
>
>>
>>
>> > +}
>> > +
>> > +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.
>
>
> Yes, that'd be an option, but testing of the ovn-controller part would need
> some rework, since we'd need to target sbdb directly, and then change it
> back in this patch.
>
> I'd prefer to avoid re-ordering the patches at this point in time if
> possible :-)

That's fine.  You can leave as-is.

>
>
>>
>>
>>
>> >                  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) {
>
>
> ack, thank you.
>
>>
>>
>>
>> > +                        /* 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?
>
>
> Code simplicity, I don't see where the existing gateway chassis (for a
> bw-compat redirect-chassis) will change or differ from the one that we
> insert automatically, beyond administrator changing priority in the SBDB.
>
> Another possible change is moving from a list of gateway chassis, to
> option:redirect-chassis=....  but also I don't expect a CNI doing that very
> often for any reason.
>
> But chances are I'm missing some cases

In this piece of code, it'd mainly just be if the redirect-chassis was
changed in ovn-northbound.  We're only talking about a single row
here, so I suppose it's fine to leave as is.

>
>>
>>
>> > +                    }
>> > +                } 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".
>
>
> woops, right!
>
>>
>>
>> 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.
>>
> ack!
>
>>
>> > +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.
>>
> ack :)
>
>>
>> > +
>> > +# 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.
>
>
> Makes sense.
>
>>
>>
>> > +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
>> > dev@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>>
>> --
>> Russell Bryant
>
>