| Message ID | 20220316161047.16392-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: Consolidate load balancer processing functions. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Mar 16, 2022 at 12:11 PM Dumitru Ceara <dceara@redhat.com> wrote: > > This commit doesn't change any functionality, it just performs the > following refactor work: > > a. consolidates the old build_ovn_lbs(), build_lrouter_lbs() and > build_ovn_lr_lbs() functions into a single function, build_lbs(), > that builds all the load balancer port-independent information. > Part of this was not possible before because we were logging a > warning message in build_ovn_lr_lbs() if a load balancer is > applied to a non-gateway router with more than 1 distributed > gateway ports. We now move the log to build_lrouter_lbs_check() > which is called after ports are processed. > > b. consolidates all the load balancer processing that depends on logical > ports being parsed too into the build_lb_port_related_data() > function. > > c. split out the part that syncs load balancers to the Southbound > database. > > This has two advantages: > > 1. it allows a simpler overview of what is being processed, that is: > - build datapaths > - build load balancer state that depends on datapaths > - build ports > - add load balancer state that depends on ports > > 2. it may enable further optimization as now the part that builds the > datapath-dependent load balancer state can be processed separately, > potentially in an incremental fashion. This part of the northd > processing has been shown to be quite time consuming in the past. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Thanks for the refactor. The patch LGTM. I went ahead and applied to the main branch. Numan > --- > northd/northd.c | 319 ++++++++++++++++++++++++------------------------ > tests/ovn.at | 14 ++- > 2 files changed, 167 insertions(+), 166 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index b264fb850b..8c871171f5 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3732,53 +3732,28 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, > } > > static void > -build_ovn_lr_lbs(struct hmap *datapaths, struct hmap *lbs) > +build_lrouter_lb_ips(struct ovn_datapath *od, const struct ovn_northd_lb *lb) > { > - struct ovn_northd_lb *lb; > - struct ovn_datapath *od; > - > - HMAP_FOR_EACH (od, key_node, datapaths) { > - if (!od->nbr) { > - continue; > - } > - if (!smap_get(&od->nbr->options, "chassis") > - && od->n_l3dgw_ports != 1) { > - if (od->n_l3dgw_ports > 1 && od->has_lb_vip) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "Load-balancers are configured on logical " > - "router %s, which has %"PRIuSIZE" distributed " > - "gateway ports. Load-balancer is not supported " > - "yet when there is more than one distributed " > - "gateway port on the router.", > - od->nbr->name, od->n_l3dgw_ports); > - } > - continue; > - } > + bool is_routable = smap_get_bool(&lb->nlb->options, "add_route", false); > + const char *ip_address; > > - for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { > - const struct uuid *lb_uuid = > - &od->nbr->load_balancer[i]->header_.uuid; > - lb = ovn_northd_lb_find(lbs, lb_uuid); > - ovn_northd_lb_add_lr(lb, od); > + SSET_FOR_EACH (ip_address, &lb->ips_v4) { > + sset_add(&od->lb_ips_v4, ip_address); > + if (is_routable) { > + sset_add(&od->lb_ips_v4_routable, ip_address); > } > - > - for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) { > - const struct nbrec_load_balancer_group *lbg = > - od->nbr->load_balancer_group[i]; > - for (size_t j = 0; j < lbg->n_load_balancer; j++) { > - const struct uuid *lb_uuid = > - &lbg->load_balancer[j]->header_.uuid; > - lb = ovn_northd_lb_find(lbs, lb_uuid); > - ovn_northd_lb_add_lr(lb, od); > - } > + } > + SSET_FOR_EACH (ip_address, &lb->ips_v6) { > + sset_add(&od->lb_ips_v6, ip_address); > + if (is_routable) { > + sset_add(&od->lb_ips_v6_routable, ip_address); > } > } > } > > static void > -build_ovn_lbs(struct northd_input *input_data, > - struct ovsdb_idl_txn *ovnsb_txn, > - struct hmap *datapaths, struct hmap *lbs) > +build_lbs(struct northd_input *input_data, struct hmap *datapaths, > + struct hmap *lbs) > { > struct ovn_northd_lb *lb; > > @@ -3817,94 +3792,38 @@ build_ovn_lbs(struct northd_input *input_data, > } > } > > - /* Delete any stale SB load balancer rows. */ > - struct hmapx existing_lbs = HMAPX_INITIALIZER(&existing_lbs); > - const struct sbrec_load_balancer *sbrec_lb, *next; > - SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, next, > - input_data->sbrec_load_balancer_table) { > - 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; > - } > - > - /* Delete any SB load balancer entries that refer to NB load balancers > - * that don't exist anymore or are not applied to switches anymore. > - * > - * There is also a special case in which duplicate LBs might be created > - * in the SB, e.g., due to the fact that OVSDB only ensures > - * "at-least-once" consistency for clustered database tables that > - * are not indexed in any way. > - */ > - lb = ovn_northd_lb_find(lbs, &lb_uuid); > - if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) { > - sbrec_load_balancer_delete(sbrec_lb); > - } else { > - lb->slb = sbrec_lb; > - } > - } > - hmapx_destroy(&existing_lbs); > - > - /* Create SB Load balancer records if not present and sync > - * the SB load balancer columns. */ > - HMAP_FOR_EACH (lb, hmap_node, lbs) { > - > - if (!lb->n_nb_ls) { > + HMAP_FOR_EACH (od, key_node, datapaths) { > + if (!od->nbr) { > continue; > } > > - /* Store the fact that northd provides the original (destination IP + > - * transport port) tuple. > - */ > - struct smap options; > - smap_clone(&options, &lb->nlb->options); > - smap_replace(&options, "hairpin_orig_tuple", "true"); > - > - struct sbrec_datapath_binding **lb_dps = > - xmalloc(lb->n_nb_ls * sizeof *lb_dps); > - for (size_t i = 0; i < lb->n_nb_ls; i++) { > - lb_dps[i] = CONST_CAST(struct sbrec_datapath_binding *, > - lb->nb_ls[i]->sb); > - } > - > - if (!lb->slb) { > - sbrec_lb = sbrec_load_balancer_insert(ovnsb_txn); > - lb->slb = sbrec_lb; > - char *lb_id = xasprintf( > - UUID_FMT, UUID_ARGS(&lb->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); > - } > - sbrec_load_balancer_set_name(lb->slb, lb->nlb->name); > - sbrec_load_balancer_set_vips(lb->slb, &lb->nlb->vips); > - sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol); > - sbrec_load_balancer_set_datapaths(lb->slb, lb_dps, lb->n_nb_ls); > - sbrec_load_balancer_set_options(lb->slb, &options); > - smap_destroy(&options); > - free(lb_dps); > - } > - > - /* Datapath_Binding.load_balancers is not used anymore, it's still in the > - * schema for compatibility reasons. Reset it to empty, just in case. > - */ > - HMAP_FOR_EACH (od, key_node, datapaths) { > - if (!od->nbs) { > - continue; > + for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { > + const struct uuid *lb_uuid = > + &od->nbr->load_balancer[i]->header_.uuid; > + lb = ovn_northd_lb_find(lbs, lb_uuid); > + ovn_northd_lb_add_lr(lb, od); > + build_lrouter_lb_ips(od, lb); > } > > - if (od->sb->n_load_balancers) { > - sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0); > + for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) { > + const struct nbrec_load_balancer_group *lbg = > + od->nbr->load_balancer_group[i]; > + for (size_t j = 0; j < lbg->n_load_balancer; j++) { > + const struct uuid *lb_uuid = > + &lbg->load_balancer[j]->header_.uuid; > + lb = ovn_northd_lb_find(lbs, lb_uuid); > + ovn_northd_lb_add_lr(lb, od); > + build_lrouter_lb_ips(od, lb); > + } > } > } > } > > static void > -build_ovn_lb_svcs(struct northd_input *input_data, > - struct ovsdb_idl_txn *ovnsb_txn, > - struct hmap *ports, struct hmap *lbs) > +build_lb_svcs(struct northd_input *input_data, > + struct ovsdb_idl_txn *ovnsb_txn, > + struct hmap *ports, > + struct hmap *lbs) > { > struct hmap monitor_map = HMAP_INITIALIZER(&monitor_map); > > @@ -3936,26 +3855,6 @@ build_ovn_lb_svcs(struct northd_input *input_data, > hmap_destroy(&monitor_map); > } > > -static void > -build_lrouter_lb_ips(struct ovn_datapath *od, const struct ovn_northd_lb *lb) > -{ > - bool is_routable = smap_get_bool(&lb->nlb->options, "add_route", false); > - const char *ip_address; > - > - SSET_FOR_EACH (ip_address, &lb->ips_v4) { > - sset_add(&od->lb_ips_v4, ip_address); > - if (is_routable) { > - sset_add(&od->lb_ips_v4_routable, ip_address); > - } > - } > - SSET_FOR_EACH (ip_address, &lb->ips_v6) { > - sset_add(&od->lb_ips_v6, ip_address); > - if (is_routable) { > - sset_add(&od->lb_ips_v6_routable, ip_address); > - } > - } > -} > - > static bool lrouter_port_ipv4_reachable(const struct ovn_port *op, > ovs_be32 addr); > static bool lrouter_port_ipv6_reachable(const struct ovn_port *op, > @@ -3989,7 +3888,7 @@ build_lrouter_lb_reachable_ips(struct ovn_datapath *od, > } > > static void > -build_lrouter_lbs(struct hmap *datapaths, struct hmap *lbs) > +build_lrouter_lbs_check(const struct hmap *datapaths) > { > struct ovn_datapath *od; > > @@ -3998,22 +3897,15 @@ build_lrouter_lbs(struct hmap *datapaths, struct hmap *lbs) > continue; > } > > - for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { > - struct ovn_northd_lb *lb = > - ovn_northd_lb_find(lbs, > - &od->nbr->load_balancer[i]->header_.uuid); > - build_lrouter_lb_ips(od, lb); > - } > - > - for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) { > - const struct nbrec_load_balancer_group *lbg = > - od->nbr->load_balancer_group[i]; > - for (size_t j = 0; j < lbg->n_load_balancer; j++) { > - struct ovn_northd_lb *lb = > - ovn_northd_lb_find(lbs, > - &lbg->load_balancer[j]->header_.uuid); > - build_lrouter_lb_ips(od, lb); > - } > + if (od->has_lb_vip && od->n_l3dgw_ports > 1 > + && !smap_get(&od->nbr->options, "chassis")) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Load-balancers are configured on logical " > + "router %s, which has %"PRIuSIZE" distributed " > + "gateway ports. Load-balancer is not supported " > + "yet when there is more than one distributed " > + "gateway port on the router.", > + od->nbr->name, od->n_l3dgw_ports); > } > } > } > @@ -4048,6 +3940,114 @@ build_lrouter_lbs_reachable_ips(struct hmap *datapaths, struct hmap *lbs) > } > } > > +/* This must be called after all ports have been processed, i.e., after > + * build_ports() because the reachability check requires the router ports > + * networks to have been parsed. > + */ > +static void > +build_lb_port_related_data(struct hmap *datapaths, struct hmap *ports, > + struct hmap *lbs, struct northd_input *input_data, > + struct ovsdb_idl_txn *ovnsb_txn) > +{ > + build_lrouter_lbs_check(datapaths); > + build_lrouter_lbs_reachable_ips(datapaths, lbs); > + build_lb_svcs(input_data, ovnsb_txn, ports, lbs); > +} > + > +/* Syncs relevant load balancers (applied to logical switches) to the > + * Southbound database. > + */ > +static void > +sync_lbs(struct northd_input *input_data, struct ovsdb_idl_txn *ovnsb_txn, > + struct hmap *datapaths, struct hmap *lbs) > +{ > + struct ovn_northd_lb *lb; > + > + /* Delete any stale SB load balancer rows. */ > + struct hmapx existing_lbs = HMAPX_INITIALIZER(&existing_lbs); > + const struct sbrec_load_balancer *sbrec_lb, *next; > + SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, next, > + input_data->sbrec_load_balancer_table) { > + 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; > + } > + > + /* Delete any SB load balancer entries that refer to NB load balancers > + * that don't exist anymore or are not applied to switches anymore. > + * > + * There is also a special case in which duplicate LBs might be created > + * in the SB, e.g., due to the fact that OVSDB only ensures > + * "at-least-once" consistency for clustered database tables that > + * are not indexed in any way. > + */ > + lb = ovn_northd_lb_find(lbs, &lb_uuid); > + if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) { > + sbrec_load_balancer_delete(sbrec_lb); > + } else { > + lb->slb = sbrec_lb; > + } > + } > + hmapx_destroy(&existing_lbs); > + > + /* Create SB Load balancer records if not present and sync > + * the SB load balancer columns. */ > + HMAP_FOR_EACH (lb, hmap_node, lbs) { > + > + if (!lb->n_nb_ls) { > + continue; > + } > + > + /* Store the fact that northd provides the original (destination IP + > + * transport port) tuple. > + */ > + struct smap options; > + smap_clone(&options, &lb->nlb->options); > + smap_replace(&options, "hairpin_orig_tuple", "true"); > + > + struct sbrec_datapath_binding **lb_dps = > + xmalloc(lb->n_nb_ls * sizeof *lb_dps); > + for (size_t i = 0; i < lb->n_nb_ls; i++) { > + lb_dps[i] = CONST_CAST(struct sbrec_datapath_binding *, > + lb->nb_ls[i]->sb); > + } > + > + if (!lb->slb) { > + sbrec_lb = sbrec_load_balancer_insert(ovnsb_txn); > + lb->slb = sbrec_lb; > + char *lb_id = xasprintf( > + UUID_FMT, UUID_ARGS(&lb->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); > + } > + sbrec_load_balancer_set_name(lb->slb, lb->nlb->name); > + sbrec_load_balancer_set_vips(lb->slb, &lb->nlb->vips); > + sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol); > + sbrec_load_balancer_set_datapaths(lb->slb, lb_dps, lb->n_nb_ls); > + sbrec_load_balancer_set_options(lb->slb, &options); > + smap_destroy(&options); > + free(lb_dps); > + } > + > + /* Datapath_Binding.load_balancers is not used anymore, it's still in the > + * schema for compatibility reasons. Reset it to empty, just in case. > + */ > + struct ovn_datapath *od; > + HMAP_FOR_EACH (od, key_node, datapaths) { > + if (!od->nbs) { > + continue; > + } > + > + if (od->sb->n_load_balancers) { > + sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0); > + } > + } > +} > + > static bool > ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) > { > @@ -15121,14 +15121,12 @@ ovnnb_db_run(struct northd_input *input_data, > "ignore_lsp_down", true); > > build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); > - build_ovn_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); > - build_lrouter_lbs(&data->datapaths, &data->lbs); > + build_lbs(input_data, &data->datapaths, &data->lbs); > build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, > sbrec_chassis_by_hostname, > &data->datapaths, &data->ports); > - build_lrouter_lbs_reachable_ips(&data->datapaths, &data->lbs); > - build_ovn_lr_lbs(&data->datapaths, &data->lbs); > - build_ovn_lb_svcs(input_data, ovnsb_txn, &data->ports, &data->lbs); > + build_lb_port_related_data(&data->datapaths, &data->ports, &data->lbs, > + input_data, ovnsb_txn); > build_ipam(&data->datapaths, &data->ports); > build_port_group_lswitches(input_data, &data->port_groups, &data->ports); > build_lrouter_groups(&data->ports, &data->lr_list); > @@ -15138,7 +15136,8 @@ ovnnb_db_run(struct northd_input *input_data, > stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > ovn_update_ipv6_prefix(&data->ports); > > - sync_address_sets(input_data, ovnsb_txn, &data->datapaths); > + sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); > + sync_address_sets(input_data, ovnsb_txn, &data->datapaths); > sync_port_groups(input_data, ovnsb_txn, &data->port_groups); > sync_meters(input_data, ovnsb_txn, &data->meter_groups); > sync_dns_entries(input_data, ovnsb_txn, &data->datapaths); > diff --git a/tests/ovn.at b/tests/ovn.at > index 9b13a980da..2b13dd1b8f 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -24834,8 +24834,10 @@ ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0. > ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related > ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related > > -# Create a logical router and attach both logical switches > +# Create a logical router and attach both logical switches. > +# Make the logical router a GW router for load balancing to be supported. > ovn-nbctl lr-add lr0 > +ovn-nbctl set logical_router lr0 options:chassis=hv1 > ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > ovn-nbctl lsp-add sw0 sw0-lr0 > ovn-nbctl lsp-set-type sw0-lr0 router > @@ -24856,7 +24858,7 @@ ovn-nbctl lr-lb-add lr0 lb1 > > OVS_WAIT_UNTIL([ > test $(as hv1 ovs-ofctl dump-groups br-int | \ > - grep "selection_method=dp_hash" -c) -eq 1 > + grep "selection_method=dp_hash" -c) -eq 2 > ]) > > OVS_WAIT_UNTIL([ > @@ -24870,7 +24872,7 @@ ovn-nbctl set load_balancer $lb1_uuid selection_fields="ip_src,ip_dst,tp_src,tp_ > > OVS_WAIT_UNTIL([ > test $(as hv1 ovs-ofctl dump-groups br-int | \ > - grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 1 > + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 2 > ]) > > OVS_WAIT_UNTIL([ > @@ -24882,7 +24884,7 @@ OVS_WAIT_UNTIL([ > ovn-nbctl set load_balancer $lb1_uuid protocol=udp > OVS_WAIT_UNTIL([ > test $(as hv1 ovs-ofctl dump-groups br-int | \ > - grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 1 > + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 2 > ]) > > OVS_WAIT_UNTIL([ > @@ -24894,7 +24896,7 @@ OVS_WAIT_UNTIL([ > ovn-nbctl set load_balancer $lb1_uuid protocol=sctp > OVS_WAIT_UNTIL([ > test $(as hv1 ovs-ofctl dump-groups br-int | \ > - grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 2 > ]) > > OVS_WAIT_UNTIL([ > @@ -24905,7 +24907,7 @@ OVS_WAIT_UNTIL([ > ovn-nbctl set load_balancer $lb1_uuid selection_fields="ip_src,ip_dst,tp_dst" > OVS_WAIT_UNTIL([ > test $(as hv1 ovs-ofctl dump-groups br-int | \ > - grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq 1 > + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq 2 > ]) > > OVS_WAIT_UNTIL([ > -- > 2.27.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/northd.c b/northd/northd.c index b264fb850b..8c871171f5 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3732,53 +3732,28 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, } static void -build_ovn_lr_lbs(struct hmap *datapaths, struct hmap *lbs) +build_lrouter_lb_ips(struct ovn_datapath *od, const struct ovn_northd_lb *lb) { - struct ovn_northd_lb *lb; - struct ovn_datapath *od; - - HMAP_FOR_EACH (od, key_node, datapaths) { - if (!od->nbr) { - continue; - } - if (!smap_get(&od->nbr->options, "chassis") - && od->n_l3dgw_ports != 1) { - if (od->n_l3dgw_ports > 1 && od->has_lb_vip) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "Load-balancers are configured on logical " - "router %s, which has %"PRIuSIZE" distributed " - "gateway ports. Load-balancer is not supported " - "yet when there is more than one distributed " - "gateway port on the router.", - od->nbr->name, od->n_l3dgw_ports); - } - continue; - } + bool is_routable = smap_get_bool(&lb->nlb->options, "add_route", false); + const char *ip_address; - for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { - const struct uuid *lb_uuid = - &od->nbr->load_balancer[i]->header_.uuid; - lb = ovn_northd_lb_find(lbs, lb_uuid); - ovn_northd_lb_add_lr(lb, od); + SSET_FOR_EACH (ip_address, &lb->ips_v4) { + sset_add(&od->lb_ips_v4, ip_address); + if (is_routable) { + sset_add(&od->lb_ips_v4_routable, ip_address); } - - for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) { - const struct nbrec_load_balancer_group *lbg = - od->nbr->load_balancer_group[i]; - for (size_t j = 0; j < lbg->n_load_balancer; j++) { - const struct uuid *lb_uuid = - &lbg->load_balancer[j]->header_.uuid; - lb = ovn_northd_lb_find(lbs, lb_uuid); - ovn_northd_lb_add_lr(lb, od); - } + } + SSET_FOR_EACH (ip_address, &lb->ips_v6) { + sset_add(&od->lb_ips_v6, ip_address); + if (is_routable) { + sset_add(&od->lb_ips_v6_routable, ip_address); } } } static void -build_ovn_lbs(struct northd_input *input_data, - struct ovsdb_idl_txn *ovnsb_txn, - struct hmap *datapaths, struct hmap *lbs) +build_lbs(struct northd_input *input_data, struct hmap *datapaths, + struct hmap *lbs) { struct ovn_northd_lb *lb; @@ -3817,94 +3792,38 @@ build_ovn_lbs(struct northd_input *input_data, } } - /* Delete any stale SB load balancer rows. */ - struct hmapx existing_lbs = HMAPX_INITIALIZER(&existing_lbs); - const struct sbrec_load_balancer *sbrec_lb, *next; - SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, next, - input_data->sbrec_load_balancer_table) { - 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; - } - - /* Delete any SB load balancer entries that refer to NB load balancers - * that don't exist anymore or are not applied to switches anymore. - * - * There is also a special case in which duplicate LBs might be created - * in the SB, e.g., due to the fact that OVSDB only ensures - * "at-least-once" consistency for clustered database tables that - * are not indexed in any way. - */ - lb = ovn_northd_lb_find(lbs, &lb_uuid); - if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) { - sbrec_load_balancer_delete(sbrec_lb); - } else { - lb->slb = sbrec_lb; - } - } - hmapx_destroy(&existing_lbs); - - /* Create SB Load balancer records if not present and sync - * the SB load balancer columns. */ - HMAP_FOR_EACH (lb, hmap_node, lbs) { - - if (!lb->n_nb_ls) { + HMAP_FOR_EACH (od, key_node, datapaths) { + if (!od->nbr) { continue; } - /* Store the fact that northd provides the original (destination IP + - * transport port) tuple. - */ - struct smap options; - smap_clone(&options, &lb->nlb->options); - smap_replace(&options, "hairpin_orig_tuple", "true"); - - struct sbrec_datapath_binding **lb_dps = - xmalloc(lb->n_nb_ls * sizeof *lb_dps); - for (size_t i = 0; i < lb->n_nb_ls; i++) { - lb_dps[i] = CONST_CAST(struct sbrec_datapath_binding *, - lb->nb_ls[i]->sb); - } - - if (!lb->slb) { - sbrec_lb = sbrec_load_balancer_insert(ovnsb_txn); - lb->slb = sbrec_lb; - char *lb_id = xasprintf( - UUID_FMT, UUID_ARGS(&lb->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); - } - sbrec_load_balancer_set_name(lb->slb, lb->nlb->name); - sbrec_load_balancer_set_vips(lb->slb, &lb->nlb->vips); - sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol); - sbrec_load_balancer_set_datapaths(lb->slb, lb_dps, lb->n_nb_ls); - sbrec_load_balancer_set_options(lb->slb, &options); - smap_destroy(&options); - free(lb_dps); - } - - /* Datapath_Binding.load_balancers is not used anymore, it's still in the - * schema for compatibility reasons. Reset it to empty, just in case. - */ - HMAP_FOR_EACH (od, key_node, datapaths) { - if (!od->nbs) { - continue; + for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { + const struct uuid *lb_uuid = + &od->nbr->load_balancer[i]->header_.uuid; + lb = ovn_northd_lb_find(lbs, lb_uuid); + ovn_northd_lb_add_lr(lb, od); + build_lrouter_lb_ips(od, lb); } - if (od->sb->n_load_balancers) { - sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0); + for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) { + const struct nbrec_load_balancer_group *lbg = + od->nbr->load_balancer_group[i]; + for (size_t j = 0; j < lbg->n_load_balancer; j++) { + const struct uuid *lb_uuid = + &lbg->load_balancer[j]->header_.uuid; + lb = ovn_northd_lb_find(lbs, lb_uuid); + ovn_northd_lb_add_lr(lb, od); + build_lrouter_lb_ips(od, lb); + } } } } static void -build_ovn_lb_svcs(struct northd_input *input_data, - struct ovsdb_idl_txn *ovnsb_txn, - struct hmap *ports, struct hmap *lbs) +build_lb_svcs(struct northd_input *input_data, + struct ovsdb_idl_txn *ovnsb_txn, + struct hmap *ports, + struct hmap *lbs) { struct hmap monitor_map = HMAP_INITIALIZER(&monitor_map); @@ -3936,26 +3855,6 @@ build_ovn_lb_svcs(struct northd_input *input_data, hmap_destroy(&monitor_map); } -static void -build_lrouter_lb_ips(struct ovn_datapath *od, const struct ovn_northd_lb *lb) -{ - bool is_routable = smap_get_bool(&lb->nlb->options, "add_route", false); - const char *ip_address; - - SSET_FOR_EACH (ip_address, &lb->ips_v4) { - sset_add(&od->lb_ips_v4, ip_address); - if (is_routable) { - sset_add(&od->lb_ips_v4_routable, ip_address); - } - } - SSET_FOR_EACH (ip_address, &lb->ips_v6) { - sset_add(&od->lb_ips_v6, ip_address); - if (is_routable) { - sset_add(&od->lb_ips_v6_routable, ip_address); - } - } -} - static bool lrouter_port_ipv4_reachable(const struct ovn_port *op, ovs_be32 addr); static bool lrouter_port_ipv6_reachable(const struct ovn_port *op, @@ -3989,7 +3888,7 @@ build_lrouter_lb_reachable_ips(struct ovn_datapath *od, } static void -build_lrouter_lbs(struct hmap *datapaths, struct hmap *lbs) +build_lrouter_lbs_check(const struct hmap *datapaths) { struct ovn_datapath *od; @@ -3998,22 +3897,15 @@ build_lrouter_lbs(struct hmap *datapaths, struct hmap *lbs) continue; } - for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { - struct ovn_northd_lb *lb = - ovn_northd_lb_find(lbs, - &od->nbr->load_balancer[i]->header_.uuid); - build_lrouter_lb_ips(od, lb); - } - - for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) { - const struct nbrec_load_balancer_group *lbg = - od->nbr->load_balancer_group[i]; - for (size_t j = 0; j < lbg->n_load_balancer; j++) { - struct ovn_northd_lb *lb = - ovn_northd_lb_find(lbs, - &lbg->load_balancer[j]->header_.uuid); - build_lrouter_lb_ips(od, lb); - } + if (od->has_lb_vip && od->n_l3dgw_ports > 1 + && !smap_get(&od->nbr->options, "chassis")) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "Load-balancers are configured on logical " + "router %s, which has %"PRIuSIZE" distributed " + "gateway ports. Load-balancer is not supported " + "yet when there is more than one distributed " + "gateway port on the router.", + od->nbr->name, od->n_l3dgw_ports); } } } @@ -4048,6 +3940,114 @@ build_lrouter_lbs_reachable_ips(struct hmap *datapaths, struct hmap *lbs) } } +/* This must be called after all ports have been processed, i.e., after + * build_ports() because the reachability check requires the router ports + * networks to have been parsed. + */ +static void +build_lb_port_related_data(struct hmap *datapaths, struct hmap *ports, + struct hmap *lbs, struct northd_input *input_data, + struct ovsdb_idl_txn *ovnsb_txn) +{ + build_lrouter_lbs_check(datapaths); + build_lrouter_lbs_reachable_ips(datapaths, lbs); + build_lb_svcs(input_data, ovnsb_txn, ports, lbs); +} + +/* Syncs relevant load balancers (applied to logical switches) to the + * Southbound database. + */ +static void +sync_lbs(struct northd_input *input_data, struct ovsdb_idl_txn *ovnsb_txn, + struct hmap *datapaths, struct hmap *lbs) +{ + struct ovn_northd_lb *lb; + + /* Delete any stale SB load balancer rows. */ + struct hmapx existing_lbs = HMAPX_INITIALIZER(&existing_lbs); + const struct sbrec_load_balancer *sbrec_lb, *next; + SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, next, + input_data->sbrec_load_balancer_table) { + 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; + } + + /* Delete any SB load balancer entries that refer to NB load balancers + * that don't exist anymore or are not applied to switches anymore. + * + * There is also a special case in which duplicate LBs might be created + * in the SB, e.g., due to the fact that OVSDB only ensures + * "at-least-once" consistency for clustered database tables that + * are not indexed in any way. + */ + lb = ovn_northd_lb_find(lbs, &lb_uuid); + if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) { + sbrec_load_balancer_delete(sbrec_lb); + } else { + lb->slb = sbrec_lb; + } + } + hmapx_destroy(&existing_lbs); + + /* Create SB Load balancer records if not present and sync + * the SB load balancer columns. */ + HMAP_FOR_EACH (lb, hmap_node, lbs) { + + if (!lb->n_nb_ls) { + continue; + } + + /* Store the fact that northd provides the original (destination IP + + * transport port) tuple. + */ + struct smap options; + smap_clone(&options, &lb->nlb->options); + smap_replace(&options, "hairpin_orig_tuple", "true"); + + struct sbrec_datapath_binding **lb_dps = + xmalloc(lb->n_nb_ls * sizeof *lb_dps); + for (size_t i = 0; i < lb->n_nb_ls; i++) { + lb_dps[i] = CONST_CAST(struct sbrec_datapath_binding *, + lb->nb_ls[i]->sb); + } + + if (!lb->slb) { + sbrec_lb = sbrec_load_balancer_insert(ovnsb_txn); + lb->slb = sbrec_lb; + char *lb_id = xasprintf( + UUID_FMT, UUID_ARGS(&lb->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); + } + sbrec_load_balancer_set_name(lb->slb, lb->nlb->name); + sbrec_load_balancer_set_vips(lb->slb, &lb->nlb->vips); + sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol); + sbrec_load_balancer_set_datapaths(lb->slb, lb_dps, lb->n_nb_ls); + sbrec_load_balancer_set_options(lb->slb, &options); + smap_destroy(&options); + free(lb_dps); + } + + /* Datapath_Binding.load_balancers is not used anymore, it's still in the + * schema for compatibility reasons. Reset it to empty, just in case. + */ + struct ovn_datapath *od; + HMAP_FOR_EACH (od, key_node, datapaths) { + if (!od->nbs) { + continue; + } + + if (od->sb->n_load_balancers) { + sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0); + } + } +} + static bool ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) { @@ -15121,14 +15121,12 @@ ovnnb_db_run(struct northd_input *input_data, "ignore_lsp_down", true); build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); - build_ovn_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); - build_lrouter_lbs(&data->datapaths, &data->lbs); + build_lbs(input_data, &data->datapaths, &data->lbs); build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, sbrec_chassis_by_hostname, &data->datapaths, &data->ports); - build_lrouter_lbs_reachable_ips(&data->datapaths, &data->lbs); - build_ovn_lr_lbs(&data->datapaths, &data->lbs); - build_ovn_lb_svcs(input_data, ovnsb_txn, &data->ports, &data->lbs); + build_lb_port_related_data(&data->datapaths, &data->ports, &data->lbs, + input_data, ovnsb_txn); build_ipam(&data->datapaths, &data->ports); build_port_group_lswitches(input_data, &data->port_groups, &data->ports); build_lrouter_groups(&data->ports, &data->lr_list); @@ -15138,7 +15136,8 @@ ovnnb_db_run(struct northd_input *input_data, stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); ovn_update_ipv6_prefix(&data->ports); - sync_address_sets(input_data, ovnsb_txn, &data->datapaths); + sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); + sync_address_sets(input_data, ovnsb_txn, &data->datapaths); sync_port_groups(input_data, ovnsb_txn, &data->port_groups); sync_meters(input_data, ovnsb_txn, &data->meter_groups); sync_dns_entries(input_data, ovnsb_txn, &data->datapaths); diff --git a/tests/ovn.at b/tests/ovn.at index 9b13a980da..2b13dd1b8f 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -24834,8 +24834,10 @@ ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0. ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related -# Create a logical router and attach both logical switches +# Create a logical router and attach both logical switches. +# Make the logical router a GW router for load balancing to be supported. ovn-nbctl lr-add lr0 +ovn-nbctl set logical_router lr0 options:chassis=hv1 ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 ovn-nbctl lsp-add sw0 sw0-lr0 ovn-nbctl lsp-set-type sw0-lr0 router @@ -24856,7 +24858,7 @@ ovn-nbctl lr-lb-add lr0 lb1 OVS_WAIT_UNTIL([ test $(as hv1 ovs-ofctl dump-groups br-int | \ - grep "selection_method=dp_hash" -c) -eq 1 + grep "selection_method=dp_hash" -c) -eq 2 ]) OVS_WAIT_UNTIL([ @@ -24870,7 +24872,7 @@ ovn-nbctl set load_balancer $lb1_uuid selection_fields="ip_src,ip_dst,tp_src,tp_ OVS_WAIT_UNTIL([ test $(as hv1 ovs-ofctl dump-groups br-int | \ - grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 1 + grep "selection_method=hash,fields(ip_src,ip_dst,tcp_src,tcp_dst)" -c) -eq 2 ]) OVS_WAIT_UNTIL([ @@ -24882,7 +24884,7 @@ OVS_WAIT_UNTIL([ ovn-nbctl set load_balancer $lb1_uuid protocol=udp OVS_WAIT_UNTIL([ test $(as hv1 ovs-ofctl dump-groups br-int | \ - grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 1 + grep "selection_method=hash,fields(ip_src,ip_dst,udp_src,udp_dst)" -c) -eq 2 ]) OVS_WAIT_UNTIL([ @@ -24894,7 +24896,7 @@ OVS_WAIT_UNTIL([ ovn-nbctl set load_balancer $lb1_uuid protocol=sctp OVS_WAIT_UNTIL([ test $(as hv1 ovs-ofctl dump-groups br-int | \ - grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 1 + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 2 ]) OVS_WAIT_UNTIL([ @@ -24905,7 +24907,7 @@ OVS_WAIT_UNTIL([ ovn-nbctl set load_balancer $lb1_uuid selection_fields="ip_src,ip_dst,tp_dst" OVS_WAIT_UNTIL([ test $(as hv1 ovs-ofctl dump-groups br-int | \ - grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq 1 + grep "selection_method=hash,fields(ip_src,ip_dst,sctp_dst)" -c) -eq 2 ]) OVS_WAIT_UNTIL([
This commit doesn't change any functionality, it just performs the following refactor work: a. consolidates the old build_ovn_lbs(), build_lrouter_lbs() and build_ovn_lr_lbs() functions into a single function, build_lbs(), that builds all the load balancer port-independent information. Part of this was not possible before because we were logging a warning message in build_ovn_lr_lbs() if a load balancer is applied to a non-gateway router with more than 1 distributed gateway ports. We now move the log to build_lrouter_lbs_check() which is called after ports are processed. b. consolidates all the load balancer processing that depends on logical ports being parsed too into the build_lb_port_related_data() function. c. split out the part that syncs load balancers to the Southbound database. This has two advantages: 1. it allows a simpler overview of what is being processed, that is: - build datapaths - build load balancer state that depends on datapaths - build ports - add load balancer state that depends on ports 2. it may enable further optimization as now the part that builds the datapath-dependent load balancer state can be processed separately, potentially in an incremental fashion. This part of the northd processing has been shown to be quite time consuming in the past. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/northd.c | 319 ++++++++++++++++++++++++------------------------ tests/ovn.at | 14 ++- 2 files changed, 167 insertions(+), 166 deletions(-)