On 3/20/26 12:07 AM, Martin Kalčok 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. >
Hi Martin, > Acked-by: Martin Kalcok <[email protected]> > Thanks for the review! > 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? > It's.. complicated. :) E.g., with the following topology: workload (10.0.0.2/24) --- LS ---- (MAC1, 10.0.0.1/24) LR | LB (VIP: 10.0.0.1:80 -> something_else:80) For cases when "workload" only needs to access 10.0.0.1 for with traffic that needs to be load balanced the MAC the workload learns for 10.0.0.1 may be different from MAC1, i.e., it could be NB_Global.options:svc_monitor_mac. Because that the actual load balancing (DNAT) happens on the switch (*) that may still work, i.e., after DNAT the destination IP is that of the LB backend; the destination MAC stays $svc_monitor_mac. Now (and I know this is highly unlikely but it's not impossible), if the svc_monitor_mac is actually owned by a different station (inside or outside ovn) the LS may still be able to forward (at L2) this DNATed packet towards that station. E.g., if the svc_monitor_mac is that of a host attached to the fabric and the LS is a localnet port with addresses=unknown. I know this raises the question of: what if I have more than one backend on that LB? Then all the backends need to reply to traffic destined to svc_monitor_mac. That is again highly unlikely but also not impossible. So TL/DR: it's a huge corner case but it used to (kind of) work and now it doesn't anymore. :) > Thanks, > Martin. > Thanks, Dumitru [*] I know some people that are of the opinion that load balancers should not be allowed on switches and I don't disagree with that, it's just something we already kind of support and we can't just break it. > >> 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 >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
