On Fri, Mar 20, 2026 at 12:07 AM Martin Kalčok <[email protected]> wrote:
> Thanks for the fix Dumitru. I didn't think through the scenario where LRP > IP is used for service monitoring but the LB is not assigned to the LR. > I have only one question below, but that's just for my curiosity. > > Acked-by: Martin Kalcok <[email protected]> > > On Thu, Mar 19, 2026 at 2:25 PM Dumitru Ceara <[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. >> > > Regarding the breaking of existing use cases. > I thought that it was not previously (before ceea1d7cccad "northd: Allow > LB health checks from router IPs.") possible to use existing IP address as > a service monitor source IP, even if the LB was only attached to the switch. > Due to the controller responding with the "svc monitor MAC" to the arp > requests for "svc monitor IP", the monitored host would have "wrong" MAC > associated with the IP. This made the "real" owner of the IP unreachable > from the host. > Did I misunderstand the previous implementation? > > Thanks, > Martin. > > >> 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."). >> >> 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]> >> --- >> V3: >> - Addressed Ales' comment: >> - Split out the pinctrl fix into its own patch. >> V2: >> - Addressed Mark's comment: >> - renamed tests to show the difference between system/non-system tests >> - added mac check to the system test >> --- >> 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 +++++++++++++++++++++++++++++++++++++++++++++ >> 6 files changed, 196 insertions(+), 13 deletions(-) >> >> 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 >> >> Thank you Dumitru, Mark and Martin, applied to main and backported to 26.03. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
