On 9/14/22 14:42, Lorenzo Bianconi wrote:
> Similar to single load balancers, add the capability to automatically
> deploy a load-balancer group on each logical-switch connected to a
> logical router where the load-balancer group has been installed by the
> CMS.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2125310
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
Hi Lorenzo,
> lib/lb.c | 5 +++++
> lib/lb.h | 3 +++
> northd/northd.c | 41 +++++++++++++++++++++++++----------------
> tests/ovn-northd.at | 8 ++++++++
> 4 files changed, 41 insertions(+), 16 deletions(-)
>
> diff --git a/lib/lb.c b/lib/lb.c
> index fa1a66d82..895828a09 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -182,6 +182,7 @@ ovn_northd_lb_create(const struct nbrec_load_balancer
> *nbrec_lb)
> : LB_NEIGH_RESPOND_ALL;
> sset_init(&lb->ips_v4);
> sset_init(&lb->ips_v6);
> + sset_init(&lb->ods);
> struct smap_node *node;
> size_t n_vips = 0;
>
> @@ -280,6 +281,7 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> free(lb->vips_nb);
> sset_destroy(&lb->ips_v4);
> sset_destroy(&lb->ips_v6);
> + sset_destroy(&lb->ods);
> free(lb->selection_fields);
> free(lb->nb_lr);
> free(lb->nb_ls);
> @@ -304,6 +306,8 @@ ovn_lb_group_create(const struct
> nbrec_load_balancer_group *nbrec_lb_group,
> lb_group->ls = xmalloc(max_datapaths * sizeof *lb_group->ls);
> lb_group->lr = xmalloc(max_datapaths * sizeof *lb_group->lr);
>
> + sset_init(&lb_group->ods);
> +
> for (size_t i = 0; i < nbrec_lb_group->n_load_balancer; i++) {
> const struct uuid *lb_uuid =
> &nbrec_lb_group->load_balancer[i]->header_.uuid;
> @@ -320,6 +324,7 @@ ovn_lb_group_destroy(struct ovn_lb_group *lb_group)
> return;
> }
>
> + sset_destroy(&lb_group->ods);
> free(lb_group->lbs);
> free(lb_group->ls);
> free(lb_group->lr);
> diff --git a/lib/lb.h b/lib/lb.h
> index e0b153fb3..58530e222 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -55,6 +55,7 @@ struct ovn_northd_lb {
>
> struct sset ips_v4;
> struct sset ips_v6;
> + struct sset ods;
>
> size_t n_nb_ls;
> size_t n_allocated_nb_ls;
> @@ -118,6 +119,8 @@ struct ovn_lb_group {
> struct ovn_datapath **ls;
> size_t n_lr;
> struct ovn_datapath **lr;
> +
> + struct sset ods;
> };
>
> struct ovn_lb_group *ovn_lb_group_create(
> diff --git a/northd/northd.c b/northd/northd.c
> index 346c0c7c8..830a6fa48 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4068,7 +4068,8 @@ build_lrouter_lbs_reachable_ips(struct hmap *datapaths,
> struct hmap *lbs,
> }
>
> static void
> -build_lswitch_lbs_from_lrouter(struct hmap *datapaths, struct hmap *lbs)
> +build_lswitch_lbs_from_lrouter(struct hmap *datapaths, struct hmap *lbs,
> + struct hmap *lb_groups)
> {
> if (!install_ls_lb_from_router) {
> return;
> @@ -4082,16 +4083,12 @@ build_lswitch_lbs_from_lrouter(struct hmap
> *datapaths, struct hmap *lbs)
>
> struct ovn_port *op;
> LIST_FOR_EACH (op, dp_node, &od->port_list) {
> - if (!lsp_is_router(op->nbsp)) {
> - continue;
> - }
> - if (!op->peer) {
> + if (!lsp_is_router(op->nbsp) || !op->peer) {
It's not introduced by this patch but we're touching this so we might as
well change this part too: there's no need to iterate through all ports
of the switch. We already have ovn_datapath->router_ports.
While we're at it, I'm confused about why we don't do all this the other
way around:
First, for each router build the array of peer switches (we have all the
info in ovn_datapath_add_router_port()).
Then for each LB, for each router the LB is applied on, call
ovn_northd_lb_add_ls(lb, rtr->n_peer_switches, rtr->peer_switches).
Then for every LB group, for each router the LB group is applied on:
- call ovn_lb_group_add_ls(lb_group, rtr->n_peer_switches,
rtr->peer_switches)
- for every LB in the group call ovn_northd_lb_add_ls(lb,
rtr->n_peer_switches, rtr->peer_switches)
I think this should save a bunch of ovn_northd_lb_find() and
ovn_lb_group_find() calls. It would also use ovn_northd_lb_add_ls()
with for more switches at a time so it will avoid some of the reallocations.
We would have to change ovn_lb_group_add_ls() to work with arrays of
datapaths but that shouldn't be a big problem. What do you think?
> continue;
> }
>
> struct ovn_datapath *peer_od = op->peer->od;
> for (size_t i = 0; i < peer_od->nbr->n_load_balancer; i++) {
> - bool installed = false;
> const struct uuid *lb_uuid =
> &peer_od->nbr->load_balancer[i]->header_.uuid;
> struct ovn_northd_lb *lb = ovn_northd_lb_find(lbs, lb_uuid);
> @@ -4099,19 +4096,31 @@ build_lswitch_lbs_from_lrouter(struct hmap
> *datapaths, struct hmap *lbs)
> continue;
> }
>
> - for (size_t j = 0; j < lb->n_nb_ls; j++) {
> - if (lb->nb_ls[j] == od) {
> - installed = true;
> - break;
> - }
> - }
> - if (!installed) {
> + if (sset_add(&lb->ods, od->nbs->name)) {
Also, I don't think we really need the sset() or to avoid duplicates in
general. Duplicates don't really hurt and, in a real deployment, we
would expect the CMS to only configure the load balancer in one place
(on the LR or on the LB group associated to the LR).
Regards,
Dumitru
> ovn_northd_lb_add_ls(lb, 1, &od);
> }
> - if (lb->nlb) {
> - od->has_lb_vip |= lb_has_vip(lb->nlb);
> + }
> +
> + for (size_t i = 0; i < peer_od->nbr->n_load_balancer_group; i++)
> {
> + const struct nbrec_load_balancer_group *nbrec_lb_group;
> + struct ovn_lb_group *lb_group;
> +
> + nbrec_lb_group = peer_od->nbr->load_balancer_group[i];
> + lb_group = ovn_lb_group_find(lb_groups,
> + &nbrec_lb_group->header_.uuid);
> + if (!lb_group) {
> + continue;
> + }
> +
> + if (sset_add(&lb_group->ods, od->nbs->name)) {
> + ovn_lb_group_add_ls(lb_group, od);
> + for (size_t j = 0; j < lb_group->n_lbs; j++) {
> + ovn_northd_lb_add_ls(lb_group->lbs[j], 1, &od);
> + }
> }
> }
> +
> + od->has_lb_vip = ls_has_lb_vip(od);
> }
> }
> }
> @@ -4129,7 +4138,7 @@ build_lb_port_related_data(struct hmap *datapaths,
> struct hmap *ports,
> build_lrouter_lbs_check(datapaths);
> build_lrouter_lbs_reachable_ips(datapaths, lbs, lb_groups);
> build_lb_svcs(input_data, ovnsb_txn, ports, lbs);
> - build_lswitch_lbs_from_lrouter(datapaths, lbs);
> + build_lswitch_lbs_from_lrouter(datapaths, lbs, lb_groups);
> }
>
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index d5136ac6d..e9bb22d9e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -7730,6 +7730,12 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> ovn-nbctl lb-add lb0 172.16.0.10:80 10.0.0.2:80
> ovn-nbctl lr-lb-add R1 lb0
>
> +ovn-nbctl lb-add lb1 172.16.0.11:8080 10.0.0.2:8080
> +lb1_uuid=$(fetch_column nb:load_balancer _uuid name=lb1)
> +lbg=$(ovn-nbctl create load_balancer_group name=lbg -- \
> + add load_balancer_group lbg load_balancer $lb1_uuid)
> +ovn-nbctl add logical_router R1 load_balancer_group $lbg
> +
> ovn-sbctl dump-flows S0 > S0flows
> ovn-sbctl dump-flows S1 > S1flows
>
> @@ -7754,10 +7760,12 @@ AT_CAPTURE_FILE([S1flows])
> AT_CHECK([grep "ls_in_lb" S0flows | sort], [0], [dnl
> table=11(ls_in_lb ), priority=0 , match=(1), action=(next;)
> table=11(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst ==
> 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0;
> ct_lb_mark(backends=10.0.0.2:80);)
> + table=11(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst ==
> 172.16.0.11 && tcp.dst == 8080), action=(reg0[[1]] = 0;
> ct_lb_mark(backends=10.0.0.2:8080);)
> ])
> AT_CHECK([grep "ls_in_lb" S1flows | sort], [0], [dnl
> table=11(ls_in_lb ), priority=0 , match=(1), action=(next;)
> table=11(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst ==
> 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0;
> ct_lb_mark(backends=10.0.0.2:80);)
> + table=11(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst ==
> 172.16.0.11 && tcp.dst == 8080), action=(reg0[[1]] = 0;
> ct_lb_mark(backends=10.0.0.2:8080);)
> ])
>
> ovn-sbctl get datapath S0 _uuid > dp_uuids
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
