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.
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?
> };
>
> 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.
> 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;
}
> 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
