Hi Dumitru, The only nit I have is with the system test you have added. The test name is the same as the test added in ovn.at "Load balancer health checks - service monitor source MAC". However, it doesn't check the source MAC address of the health check packets to ensure that the global service monitor MAC is being used. Instead, it seems to be a basic test to ensure that health checks work when the load balancer is applied to the switch and the logical router's IP address is used as the source IP address of the health checks. The test is useful, since I don't think we have coverage for this case, but I suggest that we add some sort of check to ensure that the source MAC of the health checks is what we expect. Otherwise, the test should be renamed to indicate it is a basic functionality test.
On Wed, Mar 18, 2026 at 11:23 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]> > --- > 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 | 77 ++++++++++++++++++++++++++++++++++++++++++++ > tests/system-ovn.at | 65 +++++++++++++++++++++++++++++++++++++ > 7 files changed, 184 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..f068e49465 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -20170,3 +20170,80 @@ 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]) > +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..72edc6956f 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -21496,3 +21496,68 @@ 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]) > +AT_SKIP_IF([test $HAVE_NC = 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 > + > +check ovn-nbctl ls-add ls1 > +check ovn-nbctl lsp-add ls1 ls1p1 > +check ovn-nbctl lsp-set-addresses ls1p1 "00:00:00:01:01:01 192.168.1.1" > + > +check ovn-nbctl lr-add lr1 > +check ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:00:01 192.168.1.254/24 > +check ovn-nbctl -- lsp-add-router-port ls1 ls1-lr1 lr1-ls1 > + > +check ovn-nbctl lb-add lb 192.168.5.1:42 192.168.1.1:42 udp > +check ovn-nbctl ls-lb-add ls1 lb > +lb=$(fetch_column nb:Load_Balancer _uuid name=lb) > + > +check ovn-nbctl set Logical_Router lr1 options:chassis="hv1" > + > +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=ls1p1:192.168.1.254 > + > +ADD_NAMESPACES(ls1p1) > +ADD_VETH(ls1p1, ls1p1, br-int, "192.168.1.1/24", "00:00:00:01:01:01", > + "192.168.1.254") > + > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +dnl Start a backend server, the monitor should change status to "online". > +NETNS_DAEMONIZE([ls1p1], [nc -l -u 42], [nc1.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 nc1.pid` > +wait_row_count Service_Monitor 1 status=offline > + > +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
