Thanks Dumitru! Acked-by: Mark Michelson <[email protected]>
On Thu, Mar 19, 2026 at 6:02 AM 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. > > 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]> > --- > 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
