[ovs-dev] [PATCH ovn 1/5] Add new table Load_Balancer in Southbound database.

Mark Michelson mmichels at redhat.com
Fri Oct 23 20:59:40 UTC 2020


On 10/23/20 4:21 PM, Numan Siddique wrote:
> 
> 
> On Sat, Oct 24, 2020, 1:29 AM Mark Michelson <mmichels at redhat.com 
> <mailto:mmichels at redhat.com>> wrote:
> 
>     On 10/21/20 3:25 AM, numans at ovn.org <mailto:numans at ovn.org> wrote:
>      > From: Numan Siddique <numans at ovn.org <mailto:numans at ovn.org>>
>      >
>      > This patch adds a new table 'Load_Balancer' in SB DB and syncs
>     the Load_Balancer table rows
>      > from NB DB to SB DB. An upcoming patch will make use of this
>     table for handling the
>      > load balancer hairpin traffic.
>      >
>      > Signed-off-by: Numan Siddique <numans at ovn.org
>     <mailto:numans at ovn.org>>
>      > ---
>      >   northd/ovn-northd.c   | 141
>     ++++++++++++++++++++++++++++++++++++++++++
>      >   ovn-sb.ovsschema      |  27 +++++++-
>      >   ovn-sb.xml            |  47 ++++++++++++++
>      >   tests/ovn-northd.at <http://ovn-northd.at>   |  81
>     ++++++++++++++++++++++++
>      >   utilities/ovn-sbctl.c |   3 +
>      >   5 files changed, 297 insertions(+), 2 deletions(-)
>      >
>      > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>      > index 5b76868dfb..ea771afda1 100644
>      > --- a/northd/ovn-northd.c
>      > +++ b/northd/ovn-northd.c
>      > @@ -11896,6 +11896,136 @@ sync_dns_entries(struct northd_context
>     *ctx, struct hmap *datapaths)
>      >       }
>      >       hmap_destroy(&dns_map);
>      >   }
>      > +
>      > +/*
>      > + * struct 'sync_lb_info' is used to sync the load balancer
>     records between
>      > + * OVN Northbound db and Southbound db.
>      > + */
>      > +struct sync_lb_info {
>      > +    struct hmap_node hmap_node;
>      > +    const struct nbrec_load_balancer *nlb; /* LB record in the
>     NB db. */
>      > +    const struct sbrec_load_balancer *slb; /* LB record in the
>     SB db. */
>      > +
>      > +    /* Datapaths to which the LB entry is associated with it. */
>      > +    const struct sbrec_datapath_binding **sbs;
>      > +    size_t n_sbs;
>      > +};
>      > +
>      > +static inline struct sync_lb_info *
>      > +get_sync_lb_info_from_hmap(struct hmap *sync_lb_map, struct uuid
>     *uuid)
>      > +{
>      > +    struct sync_lb_info *lb_info;
>      > +    size_t hash = uuid_hash(uuid);
>      > +    HMAP_FOR_EACH_WITH_HASH (lb_info, hmap_node, hash,
>     sync_lb_map) {
>      > +        if (uuid_equals(&lb_info->nlb->header_.uuid, uuid)) {
>      > +            return lb_info;
>      > +        }
>      > +    }
>      > +
>      > +    return NULL;
>      > +}
>      > +
>      > +static void
>      > +sync_lb_entries(struct northd_context *ctx, struct hmap *datapaths)
>      > +{
>      > +    struct hmap lb_map = HMAP_INITIALIZER(&lb_map);
>      > +    struct ovn_datapath *od;
>      > +
>      > +    HMAP_FOR_EACH (od, key_node, datapaths) {
>      > +        if (!od->nbs || !od->nbs->n_load_balancer) {
>      > +            continue;
>      > +        }
>      > +
>      > +        for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
>      > +            struct sync_lb_info *lb_info =
>      > +                get_sync_lb_info_from_hmap(
>      > +                    &lb_map,
>     &od->nbs->load_balancer[i]->header_.uuid);
>      > +
>      > +            if (!lb_info) {
>      > +                size_t hash = uuid_hash(
>      > +                    &od->nbs->load_balancer[i]->header_.uuid);
>      > +                lb_info = xzalloc(sizeof *lb_info);;
>      > +                lb_info->nlb = od->nbs->load_balancer[i];
>      > +                hmap_insert(&lb_map, &lb_info->hmap_node, hash);
>      > +            }
>      > +
>      > +            lb_info->n_sbs++;
>      > +            lb_info->sbs = xrealloc(lb_info->sbs,
>      > +                                    lb_info->n_sbs * sizeof
>     *lb_info->sbs);
>      > +            lb_info->sbs[lb_info->n_sbs - 1] = od->sb;
>      > +        }
>      > +    }
>      > +
>      > +    const struct sbrec_load_balancer *sbrec_lb, *next;
>      > +    SBREC_LOAD_BALANCER_FOR_EACH_SAFE (sbrec_lb, next,
>     ctx->ovnsb_idl) {
>      > +        const char *nb_lb_uuid =
>     smap_get(&sbrec_lb->external_ids, "lb_id");
>      > +        struct uuid lb_uuid;
>      > +        if (!nb_lb_uuid || !uuid_from_string(&lb_uuid,
>     nb_lb_uuid)) {
>      > +            sbrec_load_balancer_delete(sbrec_lb);
>      > +            continue;
>      > +        }
>      > +
>      > +        struct sync_lb_info *lb_info =
>      > +            get_sync_lb_info_from_hmap(&lb_map, &lb_uuid);
>      > +        if (lb_info) {
>      > +            lb_info->slb = sbrec_lb;
>      > +        } else {
>      > +            sbrec_load_balancer_delete(sbrec_lb);
>      > +        }
>      > +    }
>      > +
>      > +    struct sync_lb_info *lb_info;
>      > +    HMAP_FOR_EACH (lb_info, hmap_node, &lb_map) {
>      > +        if (!lb_info->slb) {
>      > +            sbrec_lb = sbrec_load_balancer_insert(ctx->ovnsb_txn);
>      > +            lb_info->slb = sbrec_lb;
>      > +            char *lb_id = xasprintf(
>      > +                UUID_FMT, UUID_ARGS(&lb_info->nlb->header_.uuid));
>      > +            const struct smap external_ids =
>      > +                SMAP_CONST1(&external_ids, "lb_id", lb_id);
>      > +            sbrec_load_balancer_set_external_ids(sbrec_lb,
>     &external_ids);
>      > +            free(lb_id);
>      > +        }
>      > +
>      > +        /* Set the datapaths and other columns.  If nothing has
>     changed, then
>      > +         * this will be a no-op.
>      > +         */
>      > +        sbrec_load_balancer_set_datapaths(
>      > +            lb_info->slb,
>      > +            (struct sbrec_datapath_binding **)lb_info->sbs,
>      > +            lb_info->n_sbs);
>      > +
>      > +        sbrec_load_balancer_set_name(lb_info->slb,
>     lb_info->nlb->name);
>      > +        sbrec_load_balancer_set_vips(lb_info->slb,
>     &lb_info->nlb->vips);
>      > +        sbrec_load_balancer_set_protocol(lb_info->slb,
>     lb_info->nlb->protocol);
>      > +    }
>      > +
>      > +    HMAP_FOR_EACH (od, key_node, datapaths) {
>      > +        if (!od->nbs || !od->nbs->n_load_balancer) {
>      > +            continue;
>      > +        }
>      > +
>      > +        const struct sbrec_load_balancer **lbs =
>      > +            xmalloc(od->nbs->n_load_balancer * sizeof *lbs);
>      > +        for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
>      > +            lb_info = get_sync_lb_info_from_hmap(
>      > +                &lb_map, &od->nbs->load_balancer[i]->header_.uuid);
>      > +            ovs_assert(lb_info);
>      > +            lbs[i] = lb_info->slb;
>      > +        }
>      > +
>      > +        sbrec_datapath_binding_set_load_balancers(
>      > +            od->sb, (struct sbrec_load_balancer **)lbs,
>      > +            od->nbs->n_load_balancer);
>      > +        free(lbs);
>      > +    }
>      > +
>      > +    HMAP_FOR_EACH_POP (lb_info, hmap_node, &lb_map) {
>      > +        free(lb_info->sbs);
>      > +        free(lb_info);
>      > +    }
>      > +    hmap_destroy(&lb_map);
>      > +}
>      >
>      >   static void
>      >   destroy_datapaths_and_ports(struct hmap *datapaths, struct hmap
>     *ports,
>      > @@ -12263,6 +12393,7 @@ ovnnb_db_run(struct northd_context *ctx,
>      >       sync_port_groups(ctx, &port_groups);
>      >       sync_meters(ctx);
>      >       sync_dns_entries(ctx, datapaths);
>      > +    sync_lb_entries(ctx, datapaths);
>      >       destroy_ovn_lbs(&lbs);
>      >       hmap_destroy(&lbs);
>      >
>      > @@ -13022,6 +13153,8 @@ main(int argc, char *argv[])
>      >       ovsdb_idl_add_table(ovnsb_idl_loop.idl,
>     &sbrec_table_datapath_binding);
>      >       add_column_noalert(ovnsb_idl_loop.idl,
>      >                          &sbrec_datapath_binding_col_tunnel_key);
>      > +    add_column_noalert(ovnsb_idl_loop.idl,
>      > +                       &sbrec_datapath_binding_col_load_balancers);
>      >       add_column_noalert(ovnsb_idl_loop.idl,
>      >                          &sbrec_datapath_binding_col_external_ids);
>      >
>      > @@ -13189,6 +13322,14 @@ main(int argc, char *argv[])
>      >       add_column_noalert(ovnsb_idl_loop.idl,
>      >                          &sbrec_service_monitor_col_external_ids);
>      >
>      > +    ovsdb_idl_add_table(ovnsb_idl_loop.idl,
>     &sbrec_table_load_balancer);
>      > +    add_column_noalert(ovnsb_idl_loop.idl,
>     &sbrec_load_balancer_col_datapaths);
>      > +    add_column_noalert(ovnsb_idl_loop.idl,
>     &sbrec_load_balancer_col_name);
>      > +    add_column_noalert(ovnsb_idl_loop.idl,
>     &sbrec_load_balancer_col_vips);
>      > +    add_column_noalert(ovnsb_idl_loop.idl,
>     &sbrec_load_balancer_col_protocol);
>      > +    add_column_noalert(ovnsb_idl_loop.idl,
>      > +                       &sbrec_load_balancer_col_external_ids);
>      > +
>      >       struct ovsdb_idl_index *sbrec_chassis_by_name
>      >           = chassis_index_create(ovnsb_idl_loop.idl);
>      >
>      > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>      > index d1c506a22c..7db6c6a4dd 100644
>      > --- a/ovn-sb.ovsschema
>      > +++ b/ovn-sb.ovsschema
>      > @@ -1,7 +1,7 @@
>      >   {
>      >       "name": "OVN_Southbound",
>      > -    "version": "2.10.0",
>      > -    "cksum": "2548342632 22615",
>      > +    "version": "2.11.0",
>      > +    "cksum": "1470439925 23814",
>      >       "tables": {
>      >           "SB_Global": {
>      >               "columns": {
>      > @@ -152,6 +152,11 @@
>      >                        "type": {"key": {"type": "integer",
>      >                                         "minInteger": 1,
>      >                                         "maxInteger": 16777215}}},
>      > +                "load_balancers": {"type": {"key": {"type": "uuid",
>      > +                                                   "refTable":
>     "Load_Balancer",
>      > +                                                   "refType":
>     "weak"},
>      > +                                            "min": 0,
>      > +                                            "max": "unlimited"}},
>      >                   "external_ids": {
>      >                       "type": {"key": "string", "value": "string",
>      >                                "min": 0, "max": "unlimited"}}},
>      > @@ -447,6 +452,24 @@
>      >                       "type": {"key": "string", "value": "string",
>      >                                "min": 0, "max": "unlimited"}}},
>      >               "indexes": [["logical_port", "ip", "port",
>     "protocol"]],
>      > +            "isRoot": true},
>      > +        "Load_Balancer": {
>      > +            "columns": {
>      > +                "name": {"type": "string"},
>      > +                "vips": {
>      > +                    "type": {"key": "string", "value": "string",
>      > +                             "min": 0, "max": "unlimited"}},
>      > +                "protocol": {
>      > +                    "type": {"key": {"type": "string",
>      > +                             "enum": ["set", ["tcp", "udp",
>     "sctp"]]},
>      > +                             "min": 0, "max": 1}},
>      > +                "datapaths": {
>      > +                    "type": {"key": {"type": "uuid",
>      > +                                     "refTable":
>     "Datapath_Binding"},
>      > +                             "min": 1, "max": "unlimited"}},
>      > +                "external_ids": {
>      > +                    "type": {"key": "string", "value": "string",
>      > +                             "min": 0, "max": "unlimited"}}},
>      >               "isRoot": true}
>      >       }
>      >   }
>      > diff --git a/ovn-sb.xml b/ovn-sb.xml
>      > index 182ff0a8a2..8638fa7eb4 100644
>      > --- a/ovn-sb.xml
>      > +++ b/ovn-sb.xml
>      > @@ -2497,6 +2497,12 @@ tcp.flags = RST;
>      >         constructed for each supported encapsulation.
>      >       </column>
>      >
>      > +    <column name="load_balancers">
>      > +      <p>
>      > +        Load balancers associated with the datapath.
>      > +      </p>
>      > +    </column>
>      > +
>      >       <group title="OVN_Northbound Relationship">
>      >         <p>
>      >           Each row in <ref table="Datapath_Binding"/> is
>     associated with some
>      > @@ -4104,4 +4110,45 @@ tcp.flags = RST;
>      >         </column>
>      >       </group>
>      >     </table>
>      > +
>      > +  <table name="Load_Balancer">
>      > +    <p>
>      > +      Each row represents one load balancer and corresponds directly
>      > +      with the <ref table="Load_Balancer"/> table in the
>      > +      <ref db="OVN_Northbound"/> database.
>      > +    </p>
> 
>     This isn't quite correct. Only load balancers associated with logical
>     switches are represented in the southbound database. Load balancers
>     used
>     by logical routers and load balancers that are not used by any
>     datapaths
>     are not put in the southbound database. Your tests even ensure that
>     this
>     is the case.
> 
> 
> That's the intention. Since hairpin lflows are added only to the logical 
> switch datapaths.
> 
> 
> Thanks
> Numan

Right, I figure this is a case where the code is correct, but the 
documentation is misleading. The wording of "corresponds directly with 
the Load_Balancer table" makes it sound like all northbound load 
balancers are represented in the southbound database. I think what you 
were meaning was that each individual southbound load balancers has a 
corresponding northbound load balancer. It still would be good to 
indicate in the docs that only northbound load balancers used by logical 
switches have corresponding southbound load balancers.

> 
> 
> 
>      > +
>      > +    <column name="name">
>      > +      A name for the load balancer.  This name has no special
>     meaning or
>      > +      purpose other than to provide convenience for human
>     interaction with
>      > +      the ovn-nb database.
>      > +    </column>
>      > +
>      > +    <column name="vips">
>      > +      A map of virtual IP addresses (and an optional port number
>     with
>      > +      <code>:</code> as a separator) associated with this load
>     balancer and
>      > +      their corresponding endpoint IP addresses (and optional
>     port numbers
>      > +      with <code>:</code> as separators) separated by commas.
>      > +    </column>
>      > +
>      > +    <column name="protocol">
>      > +      <p>
>      > +        Valid protocols are <code>tcp</code>, <code>udp</code>, or
>      > +        <code>sctp</code>.  This column is useful when a port
>     number is
>      > +        provided as part of the <code>vips</code> column.  If
>     this column is
>      > +        empty and a port number is provided as part of
>     <code>vips</code>
>      > +        column, OVN assumes the protocol to be <code>tcp</code>.
>      > +      </p>
>      > +    </column>
>      > +
>      > +    <column name="datapaths">
>      > +      Datapaths to which this load balancer applies to.
>      > +    </column>
>      > +
>      > +    <group title="Common Columns">
>      > +      <column name="external_ids">
>      > +        See <em>External IDs</em> at the beginning of this document.
>      > +      </column>
>      > +    </group>
>      > +  </table>
>      >   </database>
>      > diff --git a/tests/ovn-northd.at <http://ovn-northd.at>
>     b/tests/ovn-northd.at <http://ovn-northd.at>
>      > index 50457c291a..3dd1920b79 100644
>      > --- a/tests/ovn-northd.at <http://ovn-northd.at>
>      > +++ b/tests/ovn-northd.at <http://ovn-northd.at>
>      > @@ -2130,3 +2130,84 @@ action=(reg0 = 0; reject { /* eth.dst <->
>     eth.src; ip.dst <-> ip.src; is implici
>      >   ])
>      >
>      >   AT_CLEANUP
>      > +
>      > +AT_SETUP([ovn -- NB to SB load balancer sync])
>      > +ovn_start
>      > +
>      > +ovn-nbctl --wait=hv lb-add lb0 10.0.0.10:80
>     <http://10.0.0.10:80> 10.0.0.4:8080 <http://10.0.0.4:8080>
>      > +
>      > +AT_CHECK([ovn-nbctl --bare --columns _uuid list load_balancer |
>     wc -l], [0], [dnl
>      > +1
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl --bare --columns _uuid list load_balancer |
>     wc -l], [0], [dnl
>      > +0
>      > +])
>      > +
>      > +ovn-nbctl ls-add sw0
>      > +ovn-nbctl --wait=hv ls-lb-add sw0 lb0
>      > +sw0_sb_uuid=$(ovn-sbctl --bare --columns _uuid list datapath sw0)
>      > +AT_CHECK([ovn-sbctl --bare --columns name,vips,protocol list
>     load_balancer], [0], [dnl
>      > +lb0
>      > +10.0.0.10:80=10.0.0.4:8080 <http://10.0.0.4:8080>
>      > +tcp
>      > +])
>      > +
>      > +lb0_uuid=$(ovn-sbctl --bare --columns _uuid list load_balancer lb0)
>      > +
>      > +AT_CHECK([test $(ovn-sbctl --bare --columns datapaths list
>     load_balancer) = $sw0_sb_uuid])
>      > +AT_CHECK([test $(ovn-sbctl --bare --columns load_balancers list
>     datapath sw0) = $lb0_uuid])
>      > +
>      > +ovn-nbctl --wait=sb set load_balancer .
>     vips:"10.0.0.20\:90"="20.0.0.4:8080
>     <http://20.0.0.4:8080>,30.0.0.4:8080 <http://30.0.0.4:8080>"
>      > +AT_CHECK([ovn-sbctl --bare --columns name,vips,protocol list
>     load_balancer], [0], [dnl
>      > +lb0
>      > +10.0.0.10:80=10.0.0.4:8080 <http://10.0.0.4:8080>
>     10.0.0.20:90=20.0.0.4:8080 <http://20.0.0.4:8080>,30.0.0.4:8080
>     <http://30.0.0.4:8080>
>      > +tcp
>      > +])
>      > +
>      > +ovn-nbctl lr-add lr0
>      > +ovn-nbctl --wait=sb lr-lb-add lr0 lb0
>      > +
>      > +AT_CHECK([test $(ovn-sbctl --bare --columns datapaths list
>     load_balancer) = $sw0_sb_uuid])
>      > +
>      > +ovn-nbctl ls-add sw1
>      > +ovn-nbctl --wait=sb ls-lb-add sw1 lb0
>      > +sw1_sb_uuid=$(ovn-sbctl --bare --columns _uuid list datapath sw1)
>      > +
>      > +for i in $sw0_sb_uuid $sw1_sb_uuid; do echo $i >> dp_ids; done
>      > +for i in $(ovn-sbctl --bare --columns datapaths list
>     load_balancer); do echo $i >> lb_dps; done
>      > +
>      > +cat dp_ids | sort > expout
>      > +AT_CHECK([cat lb_dps | sort], [0], [expout])
>      > +AT_CHECK([test $(ovn-sbctl --bare --columns load_balancers list
>     datapath sw1) = $lb0_uuid])
>      > +
>      > +ovn-nbctl --wait=sb lb-add lb1 10.0.0.30:80
>     <http://10.0.0.30:80> 20.0.0.50:8080 <http://20.0.0.50:8080> udp
>      > +
>      > +AT_CHECK([ovn-sbctl --bare --columns _uuid list load_balancer |
>     wc -l], [0], [dnl
>      > +1
>      > +])
>      > +
>      > +ovn-nbctl --wait=sb lr-lb-add lr0 lb1
>      > +AT_CHECK([ovn-sbctl --bare --columns _uuid list load_balancer |
>     wc -l], [0], [dnl
>      > +1
>      > +])
>      > +
>      > +ovn-nbctl --wait=sb ls-lb-add sw1 lb1
>      > +AT_CHECK([ovn-sbctl --bare --columns name,vips,protocol list
>     load_balancer lb1 ], [0], [dnl
>      > +lb1
>      > +10.0.0.30:80=20.0.0.50:8080 <http://20.0.0.50:8080>
>      > +udp
>      > +])
>      > +
>      > +lb1_uuid=$(ovn-sbctl --bare --columns _uuid list load_balancer lb1)
>      > +
>      > +for i in $lb0_uuid $lb1_uuid; do echo $i >> lb_ids; done
>      > +cat lb_ids | sort > expout
>      > +
>      > +for i in $(ovn-sbctl --bare --columns load_balancers list
>     datapath_binding sw1); do echo $i >> sw1_lbs; done
>      > +AT_CHECK([cat sw1_lbs | sort], [0], [expout])
>      > +
>      > +ovn-nbctl --wait=sb lb-del lb1
>      > +AT_CHECK([test $(ovn-sbctl --bare --columns load_balancers list
>     datapath_binding sw1) = $lb0_uuid])
>      > +
>      > +AT_CLEANUP
>      > diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
>      > index d3eec91332..e92e9187a5 100644
>      > --- a/utilities/ovn-sbctl.c
>      > +++ b/utilities/ovn-sbctl.c
>      > @@ -1415,6 +1415,9 @@ static const struct ctl_table_class
>     tables[SBREC_N_TABLES] = {
>      >
>      >       [SBREC_TABLE_HA_CHASSIS].row_ids[0]
>      >       = {&sbrec_ha_chassis_col_chassis, NULL, NULL},
>      > +
>      > +    [SBREC_TABLE_LOAD_BALANCER].row_ids[0]
>      > +    = {&sbrec_load_balancer_col_name, NULL, NULL},
>      >   };
>      >
>      >
>      >
> 
>     _______________________________________________
>     dev mailing list
>     dev at openvswitch.org <mailto:dev at openvswitch.org>
>     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list