On Wed, 2026-01-28 at 15:02 +0100, Dumitru Ceara wrote:
> On 1/26/26 10:39 AM, Martin Kalcok via dev wrote:
> > There were concerns about the amount of lookups made to accommodate
> > this
> > feature. This commit attempts to reduce them by storing
> > `svc_mon_lrp` in the
> > `struct ovn_northd_lb_backend`.
> > There is still one lookup in the `ovn_lb_svc_create` function that
> > searches
> > all LRPs of the associated LRs for the one that matches configured
> > source IP.
> > However, there are no more lookups when creating ARP responder
> > flows and
> > health check response flows.
> >
> > This change is intentionally submitted as a separate commit, for
> > ease of
> > review/rejection. Should it be accepted, it can be squashed into
> > the
> > original commit and this commit message discarded.
> >
> > Signed-off-by: Martin Kalcok <[email protected]>
> > ---
>
> Hi Martin,
>
> Thanks for sending this as a separate incremental, it's useful!
>
> I had some comments on the first patch. When you post v4 please
> squash
> this patch into the first one, I think it's a very good addition.
>
> Please see some more comments below.
Thanks for the review. I squashed this patch for the upcoming v4. I
just have few question below to clear things up before posting it.
>
> Regards,
> Dumitru
>
> > northd/lb.c | 1 +
> > northd/lb.h | 4 ++++
> > northd/northd.c | 60 +++++++++++++++++++--------------------------
> > ----
> > 3 files changed, 28 insertions(+), 37 deletions(-)
> >
> > diff --git a/northd/lb.c b/northd/lb.c
> > index 0822bc2d7..26d5bc87a 100644
> > --- a/northd/lb.c
> > +++ b/northd/lb.c
> > @@ -196,6 +196,7 @@ ovn_lb_vip_backends_health_check_init(const
> > struct ovn_northd_lb *lb,
> > backend_nb->svc_mon_src_ip = svc_mon_src_ip;
> > backend_nb->az_name = is_remote ? xstrdup(az_name) :
> > NULL;
> > backend_nb->remote_backend = is_remote;
> > + backend_nb->svc_mon_lrp = NULL;
> > }
> > free(port_name);
> > }
> > diff --git a/northd/lb.h b/northd/lb.h
> > index 53dc4abf0..435d9942a 100644
> > --- a/northd/lb.h
> > +++ b/northd/lb.h
> > @@ -97,6 +97,10 @@ struct ovn_northd_lb_backend {
> > char *svc_mon_src_ip;
> > /* Target Availability Zone name for service monitoring. */
> > char *az_name;
> > + /* LRP used as the source of health_check traffic. This may be
> > NULL if
> > + * health_check uses "reserved" IP address that does not
> > belong to any
> > + * LRP in the backend's datapath. */
> > + struct ovn_port *svc_mon_lrp;
>
> const?
Since we'll have a "setter" method for this member, called from outside
of the "init" function, is it OK to use `const` here?
>
> > };
> >
> > struct ovn_northd_lb *ovn_northd_lb_create(const struct
> > nbrec_load_balancer *);
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 1e4e24216..2cbd8f9f3 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -3262,9 +3262,9 @@ ovn_lb_svc_create(struct ovsdb_idl_txn
> > *ovnsb_txn,
> >
> > /* If the service monitor is backed by a real port,
> > use its MAC
> > * address instead of the default service check MAC.
> > */
> > - const char *source_mac = svc_monitor_mac;
> > const struct eth_addr *source_mac_ea =
> > svc_monitor_mac_ea;
> > - if (op) {
> > + const char *source_mac = svc_monitor_mac;
> > + if (op && !backend_nb->remote_backend) {
> > struct ovn_port *svc_mon_op;
> > VECTOR_FOR_EACH (&op->od->router_ports,
> > svc_mon_op) {
> > if (!svc_mon_op->peer) {
> > @@ -3277,6 +3277,9 @@ ovn_lb_svc_create(struct ovsdb_idl_txn
> > *ovnsb_txn,
> > backend_nb-
> > >svc_mon_src_ip)) {
> > source_mac = svc_mon_op->peer-
> > >lrp_networks.ea_s;
> > source_mac_ea = &svc_mon_op->peer-
> > >lrp_networks.ea;
> > + /* Store LRP that's used as the source of
> > service
> > + * monitor traffic, to avoid later
> > lookups. */
> > + backend_nb->svc_mon_lrp = svc_mon_op-
> > >peer;
>
> It's due to the way LBs are applied to multiple switches/routers in
> the
> huge northd node but ideally this should be part of the svc_lb
> initialization. I know it's a bit over-kill but since we can't do
> that
> let's add a simple setter function in northd/lb.[ch] to set the
> backend_nb->svc_mon_lrp?
>
> We could move this whole chunk above (starting with "/* If the
> service
> monitor is backed by a real port, use its MAC") to the new function I
> think.
>
> It's more work but we at least "pretend" we properly initialized the
> LB
> somewhere else and that here we just process what's in the LB and its
> backends.
I moved the block out of northd.c, as you suggested. I would like your
input on the function naming:
* `ovn_northd_lb_backend_set_mon_port` - more human readable
* `ovn_northd_lb_backend_set_svc_mon_op` - longer, but more exact
Do you have a preference, or other suggestions?
>
> > break;
> > }
> > }
> > @@ -8542,7 +8545,7 @@ build_lb_health_check_response_lflows(
> > struct ovn_northd_lb_backend *backend_nb =
> > &lb_vip_nb->backends_nb[j++];
> >
> > - if (!backend_nb->health_check) {
> > + if (!backend_nb->health_check || !backend_nb->svc_mon_lrp)
> > {
> > continue;
> > }
> >
> > @@ -8555,24 +8558,21 @@ build_lb_health_check_response_lflows(
> > DYNAMIC_BITMAP_FOR_EACH_1 (index, &lb_dps->nb_lr_map) {
> > struct ovn_datapath *od =
> > sparse_array_get(&lr_datapaths->dps,
> > index);
> > - /* Only install the rule if the datapath has a port
> > with
> > - * monitor source IP. */
> > - struct ovn_datapath *peer_switch_od = NULL;
> > - struct ovn_port *svc_mon_op;
> > + if (!od) {
> > + continue;
> > + }
> >
> > - HMAP_FOR_EACH (svc_mon_op, dp_node, &od->ports) {
> > - if (!svc_mon_op->peer) {
> > - continue;
> > - }
> > - const char *lrp_ip = lrp_find_member_ip(
> > - svc_mon_op,
> > - backend_nb->svc_mon_src_ip);
> > - if (lrp_ip &&
> > - !strcmp(lrp_ip, backend_nb-
> > >svc_mon_src_ip)) {
> > - peer_switch_od = svc_mon_op->peer->od;
> > - break;
> > - }
> > + /* Check if the "service monitor source" LRP belongs
> > to this
> > + * router. */
> > + if (backend_nb->svc_mon_lrp->od != od) {
> > + continue;
> > }
> > +
> > + /* Get the peer switch datapath where the backend is
> > located. */
> > + struct ovn_datapath *peer_switch_od =
> > + backend_nb->svc_mon_lrp->peer
> > + ? backend_nb->svc_mon_lrp->peer->od
> > + : NULL;
>
> Nit: this indentation seems a bit odd to me. I think I'd just:
>
> /* Get the peer switch datapath where the... . */
> struct ovn_datapath *peer_switch_od = NULL;
>
> if (backend_nb->svc_mon_lrp->peer) {
> peer_switch_od = backend_nb->svc_mon_lrp->peer->od;
> }
>
> if (!peer_switch_od) {
> continue;
> }
>
> We should also check the peer is actually a switch, i.e., something
> like:
>
> if (!peer_switch_od
> || ovn_datapath_get_type(peer_switch_od) != DP_SWITCH) {
> continue;
> }
ack. replaced.
Thanks again,
Martin.
>
> > if (!peer_switch_od) {
> > continue;
> > }
> > @@ -8580,7 +8580,7 @@ build_lb_health_check_response_lflows(
> > ds_clear(match);
> > ds_clear(action);
> >
> > - /* icmp6 type 1 and icmp4 type 3 are included as well,
> > because
> > + /* icmp6 type 1 and icmp4 type 3 are included in the
> > match, because
> > * the controller is using them to detect unreachable
> > ports. */
> > if (addr_is_ipv6(backend_nb->svc_mon_src_ip)) {
> > ds_put_format(match,
> > @@ -10464,23 +10464,9 @@ build_lswitch_arp_nd_local_svc_mon(const
> > struct ovn_lb_datapaths *lb_dps,
> > continue;
> > }
> >
> > - /* ARP responder is necessary only if the service
> > check is not
> > - * backed by a real port and an IP. */
> > - struct ovn_port *svc_mon_op;
> > - bool port_found = false;
> > - VECTOR_FOR_EACH (&op->od->router_ports, svc_mon_op) {
> > - if (!svc_mon_op->peer) {
> > - continue;
> > - }
> > - const char *lrp_ip = lrp_find_member_ip(
> > - svc_mon_op->peer,
> > - backend_nb->svc_mon_src_ip);
> > - if (lrp_ip && !strcmp(lrp_ip, backend_nb-
> > >svc_mon_src_ip)) {
> > - port_found = true;
> > - break;
> > - }
> > - }
> > - if (port_found) {
> > + /* ARP responder is only needed if the service check
> > is NOT
> > + * backed by a real LRP (i.e., using a reserved IP).
> > */
> > + if (backend_nb->svc_mon_lrp) {
> > continue;
> > }
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
