On Thu, Mar 19, 2026 at 11:02 AM Dumitru Ceara via dev <
[email protected]> wrote:
> It doesn't really make sense to use the router port MAC as service
> monitor source otherwise. Worse, it may even break existing use cases
> when the load balancer is only applied to logical switches and not to
> the routers connected to them.
>
> In order for the change to work properly we also had to fallback to
> recompute in all cases when load balancers with health check
> configuration are added/updated/deleted (through load balancer groups
> too). This was currently not happening but didn't really cause issues
> before ceea1d7cccad ("northd: Allow LB health checks from router IPs.").
>
> This also uncovered a bug in pinctrl, where we don't resync the
> in-memory version of the service monitor if the SB contents change
> (e.g., the source mac address). The patch fixes that part too as it
> wasn't especially visible until now when we change the source mac of the
> monitor if it gets applied to a router and uses the router IP.
>
> Fixes: a0a5dd8ce56f ("controller: Store src_mac, src_ip in svc_monitor
> struct.")
> Fixes: ceea1d7cccad ("northd: Allow LB health checks from router IPs.")
> Reported-at: https://redhat.atlassian.net/browse/FDP-3453
> Signed-off-by: Dumitru Ceara <[email protected]>
> ---
>
Hi Dumitru,
thank you for the v2. Just one thing, let's split the patch into two
each fixing one issue. With that done:
Acked-by: Ales Musil <[email protected]>
> V2:
> - Addressed Mark's comment:
> - renamed tests to show the difference between system/non-system tests
> - added mac check to the system test
> ---
> controller/pinctrl.c | 11 +++---
> northd/en-lb-data.c | 20 +++++++----
> northd/lb.c | 13 ++++++-
> northd/lb.h | 8 +++--
> northd/northd.c | 8 +++--
> tests/ovn-northd.at | 78 +++++++++++++++++++++++++++++++++++++++++
> tests/system-ovn.at | 82 ++++++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 202 insertions(+), 18 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 35c42942f3..18b7b0df2e 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -7197,16 +7197,13 @@ sync_svc_monitors(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> if (!svc_mon) {
> svc_mon = xmalloc(sizeof *svc_mon);
> svc_mon->dp_key = dp_key;
> - svc_mon->input_port_key = input_port_key;
> svc_mon->port_key = port_key;
> svc_mon->proto_port = sb_svc_mon->port;
> svc_mon->ip = ip_addr;
> - svc_mon->src_ip = ip_addr_src;
> svc_mon->is_ip6 = !is_ipv4;
> svc_mon->state = SVC_MON_S_INIT;
> svc_mon->status = SVC_MON_ST_UNKNOWN;
> svc_mon->protocol = protocol;
> - svc_mon->type = mon_type;
>
> smap_init(&svc_mon->options);
> svc_mon->interval =
> @@ -7220,15 +7217,19 @@ sync_svc_monitors(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> svc_mon->n_success = 0;
> svc_mon->n_failures = 0;
>
> - eth_addr_from_string(sb_svc_mon->src_mac, &svc_mon->src_mac);
> -
> hmap_insert(&svc_monitors_map, &svc_mon->hmap_node, hash);
> ovs_list_push_back(&svc_monitors, &svc_mon->list_node);
> changed = true;
> }
>
> svc_mon->sb_svc_mon = sb_svc_mon;
> +
> + svc_mon->input_port_key = input_port_key;
> + svc_mon->src_ip = ip_addr_src;
> + svc_mon->type = mon_type;
> + eth_addr_from_string(sb_svc_mon->src_mac, &svc_mon->src_mac);
> svc_mon->ea = ea;
> +
> if (!smap_equal(&svc_mon->options, &sb_svc_mon->options)) {
> smap_destroy(&svc_mon->options);
> smap_clone(&svc_mon->options, &sb_svc_mon->options);
> diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> index a64c06bfda..04ef20bcc6 100644
> --- a/northd/en-lb-data.c
> +++ b/northd/en-lb-data.c
> @@ -268,6 +268,7 @@ lb_data_load_balancer_group_handler(struct engine_node
> *node, void *data)
> hmapx_add(&clbg->assoc_lbs, lb_group->lbs[i]);
> }
>
> + trk_lb_data->has_health_checks |= lb_group->has_health_checks;
> trk_lb_data->has_routable_lb |= lb_group->has_routable_lb;
> continue;
> }
> @@ -283,6 +284,7 @@ lb_data_load_balancer_group_handler(struct engine_node
> *node, void *data)
> if (nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
> hmap_remove(&lb_data->lbgrps, &lb_group->hmap_node);
> add_deleted_lbgrp_to_tracked_data(lb_group, trk_lb_data);
> + trk_lb_data->has_health_checks |= lb_group->has_health_checks;
> trk_lb_data->has_routable_lb |= lb_group->has_routable_lb;
> } else {
> /* Determine the lbs which are added or deleted for this
> @@ -299,6 +301,7 @@ lb_data_load_balancer_group_handler(struct engine_node
> *node, void *data)
> build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> }
>
> + trk_lb_data->has_health_checks |= lb_group->has_health_checks;
> trk_lb_data->has_routable_lb |= lb_group->has_routable_lb;
> struct crupdated_lbgrp *clbg =
> add_crupdated_lbgrp_to_tracked_data(lb_group,
> trk_lb_data);
> @@ -690,10 +693,12 @@ handle_od_lb_changes(struct nbrec_load_balancer
> **nbrec_lbs,
> /* Add this lb to the tracked data. */
> uuidset_insert(&codlb->assoc_lbs, lb_uuid);
>
> + struct ovn_northd_lb *lb = ovn_northd_lb_find(&lb_data->lbs,
> + lb_uuid);
> + ovs_assert(lb);
> +
> + trk_lb_data->has_health_checks |= lb->health_checks;
> if (!trk_lb_data->has_routable_lb) {
> - struct ovn_northd_lb *lb =
> ovn_northd_lb_find(&lb_data->lbs,
> - lb_uuid);
> - ovs_assert(lb);
> trk_lb_data->has_routable_lb |= lb->routable;
> trk_lb_data->has_distributed_lb |= lb->is_distributed;
> }
> @@ -730,10 +735,12 @@ handle_od_lbgrp_changes(struct
> nbrec_load_balancer_group **nbrec_lbgrps,
> /* Add this lb group to the tracked data. */
> uuidset_insert(&codlb->assoc_lbgrps, lbgrp_uuid);
>
> + struct ovn_lb_group *lbgrp =
> + ovn_lb_group_find(&lb_data->lbgrps, lbgrp_uuid);
> + ovs_assert(lbgrp);
> +
> + trk_lb_data->has_health_checks |= lbgrp->has_health_checks;
> if (!trk_lb_data->has_routable_lb) {
> - struct ovn_lb_group *lbgrp =
> - ovn_lb_group_find(&lb_data->lbgrps, lbgrp_uuid);
> - ovs_assert(lbgrp);
> trk_lb_data->has_routable_lb |= lbgrp->has_routable_lb;
> }
> }
> @@ -755,6 +762,7 @@ destroy_tracked_data(struct ed_type_lb_data *lb_data)
> lb_data->tracked_lb_data.has_dissassoc_lbs_from_lbgrps = false;
> lb_data->tracked_lb_data.has_dissassoc_lbs_from_od = false;
> lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od = false;
> + lb_data->tracked_lb_data.has_health_checks = false;
> lb_data->tracked_lb_data.has_routable_lb = false;
>
> struct hmapx_node *node;
> diff --git a/northd/lb.c b/northd/lb.c
> index 83c1bf97e1..2f683cef1b 100644
> --- a/northd/lb.c
> +++ b/northd/lb.c
> @@ -464,7 +464,8 @@ ovn_northd_lb_init(struct ovn_northd_lb *lb,
> * considered. If no matching LRP is found, the 'svc_mon_lrp' is set to
> * NULL. */
> void
> -ovn_northd_lb_backend_set_mon_port(const struct ovn_port *backend_op,
> +ovn_northd_lb_backend_set_mon_port(const struct ovn_lb_datapaths *lb_dps,
> + const struct ovn_port *backend_op,
> struct ovn_northd_lb_backend
> *backend_nb)
> {
> if (backend_op && !backend_nb->remote_backend) {
> @@ -473,6 +474,14 @@ ovn_northd_lb_backend_set_mon_port(const struct
> ovn_port *backend_op,
> if (!svc_mon_op->peer) {
> continue;
> }
> +
> + /* Skip routers this load balancer is not applied onto. */
> + const struct ovn_datapath *rtr_od = svc_mon_op->peer->od;
> + if (!dynamic_bitmap_is_set(&lb_dps->nb_lr_map,
> + rtr_od->sdp->index)) {
> + continue;
> + }
> +
> const char *lrp_ip = lrp_find_member_ip(
> svc_mon_op->peer,
> backend_nb->svc_mon_src_ip);
> @@ -564,6 +573,7 @@ ovn_lb_group_init(struct ovn_lb_group *lb_group,
> const struct uuid *lb_uuid =
> &nbrec_lb_group->load_balancer[i]->header_.uuid;
> lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
> + lb_group->has_health_checks |= lb_group->lbs[i]->health_checks;
> lb_group->has_routable_lb |= lb_group->lbs[i]->routable;
> }
> }
> @@ -586,6 +596,7 @@ ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
> {
> ovn_lb_ip_set_destroy(lb_group->lb_ips);
> lb_group->lb_ips = NULL;
> + lb_group->has_health_checks = false;
> lb_group->has_routable_lb = false;
> free(lb_group->lbs);
> }
> diff --git a/northd/lb.h b/northd/lb.h
> index c1f0c95da1..97d2c4c366 100644
> --- a/northd/lb.h
> +++ b/northd/lb.h
> @@ -108,9 +108,6 @@ struct ovn_northd_lb_backend {
> struct ovn_port *svc_mon_lrp;
> };
>
> -void ovn_northd_lb_backend_set_mon_port(const struct ovn_port *,
> - struct ovn_northd_lb_backend *);
> -
> struct ovn_northd_lb *ovn_northd_lb_create(const struct
> nbrec_load_balancer *);
> struct ovn_northd_lb *ovn_northd_lb_find(const struct hmap *,
> const struct uuid *);
> @@ -136,6 +133,7 @@ struct ovn_lb_group {
> size_t n_lbs;
> struct ovn_northd_lb **lbs;
> struct ovn_lb_ip_set *lb_ips;
> + bool has_health_checks;
> bool has_routable_lb;
> };
>
> @@ -203,6 +201,10 @@ void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths
> *, size_t n,
> struct ovn_datapath **,
> size_t n_ls_datapaths);
>
> +void ovn_northd_lb_backend_set_mon_port(const struct ovn_lb_datapaths *,
> + const struct ovn_port *,
> + struct ovn_northd_lb_backend *);
> +
> struct ovn_lb_group_datapaths {
> struct hmap_node hmap_node;
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 734efea657..c607c4c08d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3332,13 +3332,15 @@ ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
>
> static void
> ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
> - const struct ovn_northd_lb *lb,
> + const struct ovn_lb_datapaths *lb_dps,
> const struct svc_monitor_addresses
> *svc_global_addresses,
> struct hmap *ls_ports,
> struct sset *svc_monitor_lsps,
> struct hmap *local_svc_monitors_map,
> struct hmap *ic_learned_svc_monitors_map)
> {
> + const struct ovn_northd_lb *lb = lb_dps->lb;
> +
> if (lb->template) {
> return;
> }
> @@ -3366,7 +3368,7 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
> continue;
> }
>
> - ovn_northd_lb_backend_set_mon_port(op, backend_nb);
> + ovn_northd_lb_backend_set_mon_port(lb_dps, op, backend_nb);
>
> /* If the service monitor is backed by a real port, use its
> MAC
> * address instead of the default service check MAC. */
> @@ -3787,7 +3789,7 @@ build_svc_monitors_data(
>
> struct ovn_lb_datapaths *lb_dps;
> HMAP_FOR_EACH (lb_dps, hmap_node, lb_dps_map) {
> - ovn_lb_svc_create(ovnsb_txn, lb_dps->lb,
> + ovn_lb_svc_create(ovnsb_txn, lb_dps,
> svc_global_addresses,
> ls_ports,
> svc_monitor_lsps,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 60258a6f25..01ae91a963 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -20170,3 +20170,81 @@ AT_CHECK([ovn-sbctl get Port_Binding $lsp_pb_uuid
> external_ids:name],
> OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([Load balancer health checks - service monitor source MAC - SB
> syncing])
> +ovn_start
> +
> +global_svc_mon_mac="11:11:11:11:11:11"
> +router_port_mac="00:00:00:00:00:01"
> +check ovn-nbctl set NB_Global .
> options:svc_monitor_mac="$global_svc_mon_mac"
> +
> +check ovn-nbctl \
> + -- ls-add ls \
> + -- lsp-add ls lsp1 \
> + -- lsp-add ls lsp2 \
> + -- lr-add lr \
> + -- lrp-add lr lr-ls $router_port_mac 192.168.1.254/24 \
> + -- lsp-add-router-port ls ls-lr lr-ls
> +
> +check ovn-nbctl \
> + -- lb-add lb0 192.168.5.1:1 192.168.1.1:1,192.168.1.2:1 udp \
> + -- lb-add lb1 192.168.5.1:2 192.168.1.1:2,192.168.1.2:2 udp
> +lb0=$(fetch_column nb:Load_Balancer _uuid name=lb0)
> +lb1=$(fetch_column nb:Load_Balancer _uuid name=lb1)
> +
> +check ovn-nbctl
> \
> + -- set load_balancer $lb0
> ip_port_mappings:192.168.1.1=lsp1:192.168.1.254 \
> + -- set load_balancer $lb1
> ip_port_mappings:192.168.1.1=lsp1:192.168.1.254
> +
> +check_uuid ovn-nbctl --id=@hc create Load_Balancer_Health_Check
> vip="192.168.5.1\:1" \
> + -- add Load_Balancer $lb0 health_check @hc
> +check_uuid ovn-nbctl --id=@hc create Load_Balancer_Health_Check
> vip="192.168.5.1\:2" \
> + -- add Load_Balancer $lb1 health_check @hc
> +
> +dnl Add LB0 and LB1 to the switch (LB1 via load balancer group).
> +check ovn-nbctl ls-lb-add ls lb0
> +lbg=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg)
> +check ovn-nbctl add load_balancer_group lbg load_balancer $lb1
> +check ovn-nbctl add logical_switch ls load_balancer_group $lbg
> +
> +check ovn-nbctl --wait=sb sync
> +check_row_count sb:Service_Monitor 2
> +
> +AS_BOX([LB0 on switch, LB1 on switch])
> +check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=1
> +check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
> +
> +AS_BOX([LB0 on switch + router, LB1 on switch])
> +check ovn-nbctl --wait=sb lr-lb-add lr lb0
> +check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
> +check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
> +
> +AS_BOX([LB0 on switch + router, LB1 on switch + router])
> +check ovn-nbctl --wait=sb add logical_router lr load_balancer_group $lbg
> +check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
> +check_column "$router_port_mac" sb:Service_Monitor src_mac port=2
> +
> +AS_BOX([LB0 on switch + router, LB1 nowhere])
> +check ovn-nbctl --wait=sb remove load_balancer_group lbg load_balancer
> $lb1
> +check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
> +check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
> +
> +AS_BOX([LB0 on switch + router, LB1 on switch + router, readd])
> +check ovn-nbctl --wait=sb add load_balancer_group lbg load_balancer $lb1
> +check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
> +check_column "$router_port_mac" sb:Service_Monitor src_mac port=2
> +
> +AS_BOX([LB0 on switch + router, LB1 on switch, remove])
> +check ovn-nbctl --wait=sb remove logical_router lr load_balancer_group
> $lbg
> +check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
> +check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
> +
> +AS_BOX([LB0 on switch, LB1 on switch, remove])
> +check ovn-nbctl --wait=sb lr-lb-del lr lb0
> +check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=1
> +check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 7edb8a45b0..9e58f39c8b 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -21496,3 +21496,85 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> patch-.*/d
> /connection dropped.*/d"])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([Load balancer health checks - service monitor source MAC
> matching])
> +AT_SKIP_IF([test $HAVE_NC = no])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> +
> +ovn_start
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +
> +check ovs-vsctl \
> + -- set Open_vSwitch . external-ids:system-id=hv1 \
> + -- set Open_vSwitch .
> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> + -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true
> +
> +start_daemon ovn-controller
> +
> +global_svc_mon_mac="11:11:11:11:11:11"
> +router_port_mac="00:00:00:00:00:01"
> +lsp_mac="00:00:00:01:01:01"
> +check ovn-nbctl set NB_Global .
> options:svc_monitor_mac="$global_svc_mon_mac"
> +
> +check ovn-nbctl \
> + -- ls-add ls \
> + -- lsp-add ls lsp \
> + -- lsp-set-addresses lsp "$lsp_mac 192.168.1.1" \
> + -- lr-add lr \
> + -- lrp-add lr lr-ls $router_port_mac 192.168.1.254/24 \
> + -- lsp-add-router-port ls ls-lr lr-ls
> +
> +check ovn-nbctl lb-add lb 192.168.5.1:42 192.168.1.1:42 udp
> +lb=$(fetch_column nb:Load_Balancer _uuid name=lb)
> +
> +lbhc=$(ovn-nbctl \
> + --id=@hc create Load_Balancer_Health_Check vip="192.168.5.1\:42" \
> + -- add Load_Balancer $lb health_check @hc)
> +check ovn-nbctl set Load_Balancer_Health_Check $lbhc \
> + options:interval=1 options:timeout=1 \
> + options:success_count=2 \
> + options:failure_count=2
> +check ovn-nbctl --wait=sb set load_balancer $lb \
> + ip_port_mappings:192.168.1.1=lsp:192.168.1.254
> +
> +check ovn-nbctl set Logical_Router lr options:chassis="hv1"
> +check ovn-nbctl ls-lb-add ls lb
> +
> +ADD_NAMESPACES(lsp)
> +ADD_VETH(lsp, lsp, br-int, "192.168.1.1/24", "$lsp_mac",
> + "192.168.1.254")
> +
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +dnl Check mac address of probe requests and replies, they should use the
> +dnl global svc_monitor_mac.
> +NETNS_START_TCPDUMP([lsp], [-nne -i lsp -Q in 'udp'], [lsp_in])
> +NETNS_START_TCPDUMP([lsp], [-nne -i lsp -Q out 'udp || icmp'], [lsp_out])
> +
> +dnl Start a backend server, the monitor should change status to "online".
> +NETNS_DAEMONIZE([lsp], [nc -l -u 42], [nc.pid])
> +wait_row_count Service_Monitor 1 status=online
> +
> +dnl Kill the backend server, the monitor should change status to
> "offline".
> +check kill -9 `cat nc.pid`
> +wait_row_count Service_Monitor 1 status=offline
> +
> +OVS_WAIT_UNTIL([grep -q "$global_svc_mon_mac > $lsp_mac" lsp_in.tcpdump])
> +AT_CHECK([grep -v "$global_svc_mon_mac > $lsp_mac" lsp_in.tcpdump], [1])
> +
> +OVS_WAIT_UNTIL([grep -q "$lsp_mac > $global_svc_mon_mac" lsp_out.tcpdump])
> +AT_CHECK([grep -v "$lsp_mac > $global_svc_mon_mac" lsp_out.tcpdump], [1])
> +
> +OVN_CLEANUP_CONTROLLER([hv1])
> +OVN_CLEANUP_NORTHD
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +AT_CLEANUP
> +])
> --
> 2.53.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
