On Sat, Oct 24, 2020 at 2:30 AM Mark Michelson <mmich...@redhat.com> wrote: > > On 10/23/20 4:21 PM, Numan Siddique wrote: > > > > > > On Sat, Oct 24, 2020, 1:29 AM Mark Michelson <mmich...@redhat.com > > <mailto:mmich...@redhat.com>> wrote: > > > > On 10/21/20 3:25 AM, num...@ovn.org <mailto:num...@ovn.org> wrote: > > > From: Numan Siddique <num...@ovn.org <mailto:num...@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 <num...@ovn.org > > <mailto:num...@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.
Ack. I will just update the documentation as <p> Each row represents a load balancer. </p> Let me know if you have any comments. Thanks Numan > > > > > > > > > > + > > > + <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 > > d...@openvswitch.org <mailto:d...@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
