[ovs-dev] [PATCH ovn v2 1/7] Add new table Load_Balancer in Southbound database.
Dumitru Ceara
dceara at redhat.com
Tue Oct 27 21:59:42 UTC 2020
On 10/27/20 6:16 PM, numans at ovn.org wrote:
> From: Numan Siddique <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>
> ---
> northd/ovn-northd.c | 141 ++++++++++++++++++++++++++++++++++++++++++
> ovn-sb.ovsschema | 27 +++++++-
> ovn-sb.xml | 45 ++++++++++++++
> tests/ovn-northd.at | 81 ++++++++++++++++++++++++
> utilities/ovn-sbctl.c | 3 +
> 5 files changed, 295 insertions(+), 2 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index c06139d75b..d11888d203 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -11860,6 +11860,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;
> +};
> +
Hi Numan,
Why do we need the sync_lb_info intermediate data structure? There's a 1:1
relation between NB Load_Balancer and SB Load_Balancer records (for logical
switches). Why don't we just store the SB record in "struct ovn_lb"? I think
this might make the code a bit simpler.
> +static inline struct sync_lb_info *
> +get_sync_lb_info_from_hmap(struct hmap *sync_lb_map, struct uuid *uuid)
Static functions in .c files should not be inline:
https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#functions
Also, to be more in sync with the naming scheme in ovn-northd (e.g.,
ovn_datapath_find(), ovn_lb_find()) this should probably be:
s/get_sync_lb_info_from_hmap/sync_lb_info_find
Thanks,
Dumitru
> +{
> + 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,
> @@ -12227,6 +12357,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);
>
> @@ -12987,6 +13118,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);
>
> @@ -13154,6 +13287,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 b1480f2186..bdd41c1f97 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
> @@ -4126,4 +4132,43 @@ tcp.flags = RST;
> </column>
> </group>
> </table>
> +
> + <table name="Load_Balancer">
> + <p>
> + Each row represents a load balancer.
> + </p>
> +
> + <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 b/tests/ovn-northd.at
> index e155e26f89..b1f454818e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2067,3 +2067,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 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
> +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,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 10.0.0.20:90=20.0.0.4:8080,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 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
> +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 85e448ec04..00c112c7e5 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -1441,6 +1441,9 @@ static const struct ctl_table_class tables[SBREC_N_TABLES] = {
>
> [SBREC_TABLE_GATEWAY_CHASSIS].row_ids[0]
> = {&sbrec_gateway_chassis_col_name, NULL, NULL},
> +
> + [SBREC_TABLE_LOAD_BALANCER].row_ids[0]
> + = {&sbrec_load_balancer_col_name, NULL, NULL},
> };
>
>
>
More information about the dev
mailing list