Hi JayGue Lee,
On 8/24/26 8:10 AM, JayGue Lee wrote:
> Load_Balancer health checks silently fail for baremetal pool members
> whose backend Logical_Switch_Port is type=external on a Logical_Switch
> that has a localnet port, which is the usual layout for Neutron and the
> ovn-octavia-provider baremetal driver on a provider VLAN.
>
> A type=external LSP has no VIF of its own. The reply to a health check
> probe re-enters br-int through the switch's localnet port, so by the time
> it reaches ls_in_l2_lkup MFF_LOG_INPORT carries the localnet port and the
> per-backend reply lflow, which matches on the backend's inport, never
> fires. pinctrl_find_svc_monitor() is keyed on the backend port, so the
> reply is dropped and the member's health is misreported.
>
> The two supported protocols fail in opposite directions. A TCP monitor
> times out in svc_monitors_run() and goes permanently offline, taking a
> healthy member out of the pool. A UDP monitor takes the other branch of
> the same timeout and goes online; the only thing that ever moves it
> offline is the ICMP unreachable punted through this lflow, so a dead
> member stays online forever and traffic keeps being sent to it.
>
> Add a copy of the reply lflow per localnet port for backends that are
> type=external, matching the localnet port as inport and restoring the
> backend's inport before punting:
>
> match : inport == <localnet> && ip4.dst == <source ip> &&
> ip4.src == <backend ip> && eth.dst == <lrp mac> &&
> (tcp.src == <backend port> | icmp4.type == 3)
> action: inport = "<backend>"; handle_svc_check(inport);
>
> Only the inport differs from the lflow that is already generated for the
> backend, so UDP needs no new match shape, it reuses the icmp4.type /
> icmp6.type branch that is already there for it.
>
> Unlike 3bae90d9fe17, which rewrote the inport at ls_in_check_port_sec for
> every packet carrying the backend's MAC and caused a multicast storm, the
> rewrite here cannot escape a single lflow. The match requires a unicast
> eth.dst of the router port, so multicast and unknown unicast can never
> reach it, and the action list ends with handle_svc_check() without a
> next;, so no later table observes the assigned inport.
>
> The new lflows are owned by the load balancer's lflow_ref. That is safe
> because they depend on port types that lsp_can_be_inc_processed() rejects,
> localnet and external, so adding or removing either forces northd to
> recompute.
>
> Assisted-by: Claude Opus 5, Claude Code
> Signed-off-by: JayGue Lee <[email protected]>
> ---
Thanks for the rework of the fix!
> Documentation/ref/ovn-logical-flows.7.rst | 13 +++
> northd/northd.c | 73 ++++++++++++--
> tests/ovn-northd.at | 116 ++++++++++++++++++++++
> 3 files changed, 191 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/ref/ovn-logical-flows.7.rst
> b/Documentation/ref/ovn-logical-flows.7.rst
> index 1a9168ac86..8c20ed8c33 100644
> --- a/Documentation/ref/ovn-logical-flows.7.rst
> +++ b/Documentation/ref/ovn-logical-flows.7.rst
> @@ -1438,6 +1438,19 @@ This table implements switching behavior. It contains
> these logical flows:
> the service monitor mac defined in the ``options:svc_monitor_mac`` column
> of
> ``NB_Global`` table.
>
> +- For each load balancer backend that is health checked from an address
> + belonging to a logical router port, a priority-110 flow matching the probe
> + reply on ``inport == B`` and applying ``handle_svc_check(inport)``, where
> + *B* is the backend's logical switch port.
> +
> + A ``type=external`` port has no VIF of its own, so its replies reach the
> + switch through a localnet port and the flow above cannot match. For such a
> + backend an additional copy of the flow is added for each localnet port *L*
> + on the switch, matching ``inport == L`` instead and applying ``inport = B;
> + handle_svc_check(inport);`` so that ``ovn-controller`` can tell which
> + service monitor the reply belongs to. The action list ends there, so the
> + assigned ``inport`` is not visible to any later table.
> +
> - A priority-100 flow that punts all IGMP/MLD packets to ``ovn-controller``
> if
> multicast snooping is enabled on the logical switch.
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 88e3ece884..55d631e8cf 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9220,6 +9220,7 @@ build_lb_health_check_response_lflows(
> const struct ovn_northd_lb_vip *lb_vip_nb,
> const struct ovn_lb_datapaths *lb_dps,
> const struct ovn_datapaths *lr_datapaths,
> + const struct hmap *ls_ports,
> const struct shash *meter_groups,
> struct ds *match,
> struct ds *action)
> @@ -9228,6 +9229,7 @@ build_lb_health_check_response_lflows(
> * to a real LRP, install rule that punts service check replies to the
> * controller. */
> const struct ovn_lb_backend *backend;
> + struct ds probe_match = DS_EMPTY_INITIALIZER;
> size_t j = 0;
>
> VECTOR_FOR_EACH_PTR (&lb_vip->backends, backend) {
> @@ -9243,6 +9245,14 @@ build_lb_health_check_response_lflows(
> protocol = "tcp";
> }
>
> + /* A type=external backend has no VIF of its own. Its replies reach
> + * br-int through one of the switch's localnet ports, so they need
> + * additional lflows, see below. */
> + const struct ovn_port *backend_op =
> + ovn_port_find(ls_ports, backend_nb->logical_port);
> + bool external_backend = backend_op && backend_op->nbsp &&
> + lsp_is_external(backend_op->nbsp);
> +
> size_t index;
> DYNAMIC_BITMAP_FOR_EACH_1 (index, &lb_dps->nb_lr_map) {
> struct ovn_datapath *od = sparse_array_get(&lr_datapaths->dps,
> @@ -9270,32 +9280,36 @@ build_lb_health_check_response_lflows(
>
> ds_clear(match);
> ds_clear(action);
> + ds_clear(&probe_match);
>
> - /* icmp6 type 1 and icmp4 type 3 are included in the match,
> because
> + /* Everything but the inport is identical for every copy of the
> + * lflow, so build that part once.
> + *
> + * icmp6 type 1 and icmp4 type 3 are included in the match,
> because
> * the controller is using them to detect unreachable ports. */
> if (addr_is_ipv6(backend_nb->svc_mon_src_ip)) {
> - ds_put_format(match, "inport == \"%s\" && ip6.dst == %s && "
> + ds_put_format(&probe_match, "ip6.dst == %s && "
> "ip6.src == %s && eth.dst == %s && ",
> - backend_nb->logical_port,
> backend_nb->svc_mon_src_ip,
> backend->ip_str,
> backend_nb->svc_mon_lrp->lrp_networks.ea_s);
> if (!strcmp(protocol, "tcp")) {
> - ds_put_format(match, "tcp.src == %s", backend->port_str);
> + ds_put_format(&probe_match, "tcp.src == %s",
> + backend->port_str);
> } else {
> - ds_put_cstr(match, "icmp6.type == 1");
> + ds_put_cstr(&probe_match, "icmp6.type == 1");
> }
> } else {
> - ds_put_format(match, "inport == \"%s\" && ip4.dst == %s && "
> + ds_put_format(&probe_match, "ip4.dst == %s && "
> "ip4.src == %s && eth.dst == %s && ",
> - backend_nb->logical_port,
> backend_nb->svc_mon_src_ip,
> backend->ip_str,
> backend_nb->svc_mon_lrp->lrp_networks.ea_s);
> if (!strcmp(protocol, "tcp")) {
> - ds_put_format(match, "tcp.src == %s", backend->port_str);
> + ds_put_format(&probe_match, "tcp.src == %s",
> + backend->port_str);
> } else {
> - ds_put_cstr(match, "icmp4.type == 3");
> + ds_put_cstr(&probe_match, "icmp4.type == 3");
> }
> }
>
> @@ -9305,11 +9319,45 @@ build_lb_health_check_response_lflows(
> const char *meter = copp_meter_get(COPP_SVC_MONITOR,
> peer_switch_od->nbs->copp,
> meter_groups);
> + ds_put_format(match, "inport == \"%s\" && %s",
> + backend_nb->logical_port, ds_cstr(&probe_match));
> ovn_lflow_add(lflows, peer_switch_od, S_SWITCH_IN_L2_LKUP, 110,
> ds_cstr(match), "handle_svc_check(inport);",
> lb_dps->lflow_ref, WITH_CTRL_METER(meter));
> +
> + if (!external_backend) {
> + continue;
> + }
> +
> + /* The reply from a type=external backend enters br-int through a
> + * localnet port, so MFF_LOG_INPORT holds that port and the lflow
Nit: I'd rephrase this to "so 'inport' is equal to that port".
> + * above never matches. pinctrl_find_svc_monitor() is keyed on
> + * the backend port, which means the reply is dropped and the
> + * member's health is misreported: TCP monitors time out and go
> + * permanently offline, while UDP monitors, which only ever go
> + * offline through the ICMP unreachable punted here, stay online
> + * forever.
I'd replace most of this with:
"For these ports, add a copy..."
> + *
> + * Add a copy of the lflow per localnet port that restores the
> + * backend's inport before punting. The rewrite is confined to
> + * this lflow, the action list ends with handle_svc_check() and
> + * carries no next;, so no later table can observe it. */
> + ds_put_format(action, "inport = \"%s\";
> handle_svc_check(inport);",
> + backend_nb->logical_port);
> +
> + struct ovn_port *lp;
> + VECTOR_FOR_EACH (&peer_switch_od->localnet_ports, lp) {
> + ds_clear(match);
> + ds_put_format(match, "inport == %s && %s",
> + lp->json_key, ds_cstr(&probe_match));
> + ovn_lflow_add(lflows, peer_switch_od, S_SWITCH_IN_L2_LKUP,
> 110,
> + ds_cstr(match), ds_cstr(action),
> + lb_dps->lflow_ref, WITH_CTRL_METER(meter));
> + }
> }
> }
> +
> + ds_destroy(&probe_match);
> }
>
> static void
> @@ -14115,6 +14163,7 @@ build_lrouter_flows_for_lb(struct ovn_lb_datapaths
> *lb_dps,
> const struct shash *meter_groups,
> const struct ovn_datapaths *lr_datapaths,
> const struct lr_stateful_table *lr_stateful_table,
> + const struct hmap *ls_ports,
> const struct svc_monitors_map_data *svc_mons_data,
> struct ds *match, struct ds *action)
> {
> @@ -14138,7 +14187,7 @@ build_lrouter_flows_for_lb(struct ovn_lb_datapaths
> *lb_dps,
>
> build_lb_health_check_response_lflows(
> lflows, lb, lb_vip, &lb->vips_nb[i], lb_dps, lr_datapaths,
> - meter_groups, match, action);
> + ls_ports, meter_groups, match, action);
>
> if (!build_empty_lb_event_flow(lb_vip, lb, match, action)) {
> continue;
> @@ -20605,6 +20654,7 @@ build_lflows_thread(void *arg)
> lsi->meter_groups,
> lsi->lr_datapaths,
> lsi->lr_stateful_table,
> + lsi->ls_ports,
> &svc_mons_data,
> &lsi->match, &lsi->actions);
> build_lswitch_flows_for_lb(lb_dps, lsi->lflows,
> @@ -20848,7 +20898,7 @@ build_lswitch_and_lrouter_flows(
> lsi.lr_datapaths, &lsi.match);
> build_lrouter_flows_for_lb(lb_dps, lsi.lflows, lsi.meter_groups,
> lsi.lr_datapaths,
> lsi.lr_stateful_table,
> - svc_mons_data,
> + lsi.ls_ports, svc_mons_data,
> &lsi.match, &lsi.actions);
> build_lswitch_flows_for_lb(lb_dps, lsi.lflows, lsi.meter_groups,
> lsi.ls_datapaths,
> @@ -21259,6 +21309,7 @@ lflow_handle_northd_lb_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
> lflow_input->meter_groups,
> lflow_input->lr_datapaths,
> lflow_input->lr_stateful_table,
> + lflow_input->ls_ports,
> &svc_mons_data,
> &match, &actions);
> build_lswitch_flows_for_lb(lb_dps, lflows,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 6d191c1a0b..2d3f59546f 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1806,6 +1806,122 @@ OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV_PARALLELIZATION([
> +AT_SETUP([Load balancer health check reply lflows for type=external
> backends])
> +ovn_start
> +
> +# Topology:
> +#
> +# lr0 --(lr0-sw0)-- sw0 (tenant LS, no localnet)
> +# `-- vm-port (regular VIF backend)
> +#
> +# lr0 --(lr0-prov)-- prov (provider LS with a localnet port)
> +# |-- prov-localnet (type=localnet)
> +# |-- bm-tcp (type=external, TCP pool member)
> +# |-- bm-udp (type=external, UDP pool member)
> +# `-- prov-vm (regular VIF backend)
> +#
> +# A type=external LSP has no VIF of its own, so the health check reply from
> +# the baremetal member re-enters br-int through the localnet port and
> +# MFF_LOG_INPORT carries the localnet port, not the backend. Every external
> +# backend therefore gets an extra copy of the reply lflow per localnet port
> +# that restores the backend's inport before punting to the controller.
> +# Backends that are not type=external keep a single lflow even when the
> +# switch does have a localnet port.
> +
> +check ovn-nbctl lr-add lr0
> +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:01:01 10.0.0.1/24
> +check ovn-nbctl lrp-add lr0 lr0-prov 00:00:00:00:02:01 10.0.50.1/24
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl --wait=sb lsp-add sw0 sw0-lr0 \
> + -- lsp-set-type sw0-lr0 router \
> + -- lsp-set-options sw0-lr0 router-port=lr0-sw0 \
> + -- lsp-set-addresses sw0-lr0 router
> +check ovn-nbctl --wait=sb lsp-add sw0 vm-port \
> + -- lsp-set-addresses vm-port "00:00:00:00:01:02 10.0.0.10"
> +
> +check ovn-nbctl ls-add prov
> +check ovn-nbctl --wait=sb lsp-add prov prov-lr0 \
> + -- lsp-set-type prov-lr0 router \
> + -- lsp-set-options prov-lr0 router-port=lr0-prov \
> + -- lsp-set-addresses prov-lr0 router
> +check ovn-nbctl --wait=sb lsp-add prov prov-localnet \
> + -- lsp-set-type prov-localnet localnet \
> + -- lsp-set-options prov-localnet network_name=physnet1 \
> + -- lsp-set-addresses prov-localnet unknown
> +check ovn-nbctl --wait=sb lsp-add prov bm-tcp \
> + -- lsp-set-type bm-tcp external \
> + -- lsp-set-addresses bm-tcp "00:00:00:00:02:0a 10.0.50.10"
> +check ovn-nbctl --wait=sb lsp-add prov bm-udp \
> + -- lsp-set-type bm-udp external \
> + -- lsp-set-addresses bm-udp "00:00:00:00:02:0b 10.0.50.20"
> +check ovn-nbctl --wait=sb lsp-add prov prov-vm \
> + -- lsp-set-addresses prov-vm "00:00:00:00:02:0c 10.0.50.30"
> +
> +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1
> +check ovn-sbctl lsp-bind vm-port hv1
> +check ovn-sbctl lsp-bind bm-tcp hv1
> +check ovn-sbctl lsp-bind bm-udp hv1
> +check ovn-sbctl lsp-bind prov-vm hv1
> +
> +# TCP LB: one regular backend on sw0, one external and one regular backend
> +# on the provider switch.
> +check ovn-nbctl lb-add lb1 192.168.0.10:80 \
> + 10.0.0.10:80,10.0.50.10:80,10.0.50.30:80 tcp
> +check ovn-nbctl --wait=sb set load_balancer lb1 \
> + ip_port_mappings:10.0.0.10=vm-port:10.0.0.1
> +check ovn-nbctl --wait=sb set load_balancer lb1 \
> + ip_port_mappings:10.0.50.10=bm-tcp:10.0.50.1
> +check ovn-nbctl --wait=sb set load_balancer lb1 \
> + ip_port_mappings:10.0.50.30=prov-vm:10.0.50.1
> +check_uuid ovn-nbctl --wait=sb -- --id=@hc create Load_Balancer_Health_Check
> \
> + vip="192.168.0.10\:80" -- add Load_Balancer lb1 health_check @hc
> +
> +# UDP LB with an external backend. A UDP monitor is only ever moved to
> +# offline by the ICMP unreachable punted through this lflow.
> +check ovn-nbctl lb-add lb2 192.168.0.20:80 10.0.50.20:80 udp
> +check ovn-nbctl --wait=sb set load_balancer lb2 \
> + ip_port_mappings:10.0.50.20=bm-udp:10.0.50.1
> +check_uuid ovn-nbctl --wait=sb -- --id=@hc create Load_Balancer_Health_Check
> \
> + vip="192.168.0.20\:80" -- add Load_Balancer lb2 health_check @hc
> +
> +check ovn-nbctl lr-lb-add lr0 lb1
> +check ovn-nbctl lr-lb-add lr0 lb2
> +check ovn-nbctl ls-lb-add sw0 lb1
> +check ovn-nbctl ls-lb-add prov lb1
> +check ovn-nbctl --wait=sb ls-lb-add prov lb2
> +
> +AS_BOX([Regular VIF backend on a switch without a localnet port.])
> +AT_CAPTURE_FILE([sw0_lflows])
> +AT_CHECK([ovn-sbctl dump-flows sw0 | tee sw0_lflows | grep ls_in_l2_lkup dnl
> + | grep handle_svc_check | ovn_strip_lflows], [0], [dnl
> + table=??(ls_in_l2_lkup ), priority=110 , match=(eth.dst ==
> $svc_monitor_mac && (tcp || icmp || icmp6)),
> action=(handle_svc_check(inport);)
> + table=??(ls_in_l2_lkup ), priority=110 , match=(inport == "vm-port"
> && ip4.dst == 10.0.0.1 && ip4.src == 10.0.0.10 && eth.dst ==
> 00:00:00:00:01:01 && tcp.src == 80), action=(handle_svc_check(inport);)
> +])
> +
> +AS_BOX([External backends get a localnet copy, the regular one does not.])
> +AT_CAPTURE_FILE([prov_lflows])
> +AT_CHECK([ovn-sbctl dump-flows prov | tee prov_lflows | grep ls_in_l2_lkup
> dnl
> + | grep handle_svc_check | ovn_strip_lflows], [0], [dnl
> + table=??(ls_in_l2_lkup ), priority=110 , match=(eth.dst ==
> $svc_monitor_mac && (tcp || icmp || icmp6)),
> action=(handle_svc_check(inport);)
> + table=??(ls_in_l2_lkup ), priority=110 , match=(inport == "bm-tcp"
> && ip4.dst == 10.0.50.1 && ip4.src == 10.0.50.10 && eth.dst ==
> 00:00:00:00:02:01 && tcp.src == 80), action=(handle_svc_check(inport);)
> + table=??(ls_in_l2_lkup ), priority=110 , match=(inport == "bm-udp"
> && ip4.dst == 10.0.50.1 && ip4.src == 10.0.50.20 && eth.dst ==
> 00:00:00:00:02:01 && icmp4.type == 3), action=(handle_svc_check(inport);)
> + table=??(ls_in_l2_lkup ), priority=110 , match=(inport ==
> "prov-localnet" && ip4.dst == 10.0.50.1 && ip4.src == 10.0.50.10 && eth.dst
> == 00:00:00:00:02:01 && tcp.src == 80), action=(inport = "bm-tcp";
> handle_svc_check(inport);)
> + table=??(ls_in_l2_lkup ), priority=110 , match=(inport ==
> "prov-localnet" && ip4.dst == 10.0.50.1 && ip4.src == 10.0.50.20 && eth.dst
> == 00:00:00:00:02:01 && icmp4.type == 3), action=(inport = "bm-udp";
> handle_svc_check(inport);)
> + table=??(ls_in_l2_lkup ), priority=110 , match=(inport == "prov-vm"
> && ip4.dst == 10.0.50.1 && ip4.src == 10.0.50.30 && eth.dst ==
> 00:00:00:00:02:01 && tcp.src == 80), action=(handle_svc_check(inport);)
> +])
> +
> +AS_BOX([Removing the localnet port removes the extra lflows.])
> +check ovn-nbctl --wait=sb lsp-del prov-localnet
> +AT_CHECK([ovn-sbctl dump-flows prov | grep ls_in_l2_lkup dnl
> + | grep 'inport = ' | wc -l | tr -d ' '], [0], [0
> +])
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD_NO_HV([
> AT_SETUP([Load balancer VIP in NAT entries])
> AT_SKIP_IF([test $HAVE_PYTHON = no])
I took care of the minor nits above and applied the patch to main, 26.09
and 26.03.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
