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

Russell Bryant russell at ovn.org
Fri Jul 7 17:32:51 UTC 2017


(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 at redhat.com> wrote:
>
>
> On Thu, Jul 6, 2017 at 11:30 PM, Russell Bryant <russell at ovn.org> wrote:
>>
>> On Wed, Jul 5, 2017 at 9:45 AM, Miguel Angel Ajo <majopela at 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 at redhat.com>
>> > Signed-off-by: Anil Venkata <vkommadi at 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, at 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 at openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>>
>> --
>> Russell Bryant
>
>



-- 
Russell Bryant


More information about the dev mailing list