On 1/30/26 11:34 AM, [email protected] wrote:
> 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.
>
Hi Martin,
>>
>> 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?
>
Yes, you're right, I wrote the "const ?" comment before I realized we
actually set this later. It's unfortunate but it can't be "const".
>>
>>> };
>>>
>>> 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?
>
Maybe the first one, it seems to contain all the info we need to convey.
IMO it's terrible enough that we have to do this (not your fault) so
both options are just kind of ugly. But I'd go for the first one. :)
>>
>>> 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;
>>> }
>>>
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
