> 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,
Hi Dumitru,
thx for the review.
>
> > 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?
it seems doable to me, I will work on it.
>
> > 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).
I think we need it since (at least theoretically) we can have the same lb
applied to two logical routers connected to the same logical switch. Am I
missing something?
Regards,
Lorenzo
>
> 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
