On Wed, Mar 16, 2022 at 12:11 PM Dumitru Ceara <[email protected]> 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 <[email protected]>
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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
