Hi Gavin,
Thanks for the RFC!
On 10/12/26 4:47 AM, Gavin McKee via dev wrote:
> Please review this RFC for native TCP health checks over Ethernet VPN
> (EVPN). The target backend comes from an EVPN Type-2 route. The backend
> does not need a logical port in the load balancer owner's database.
>
> Each host can have its own OVN databases. The load balancer owner uses
> EVPN IP and MAC data to reach backends on other hosts. Existing local
> health checks need a backend logical port. This requirement prevents
> those checks from describing a remote EVPN backend.
>
That's true but please see below..
> This patch adds typed Northbound health-check endpoints. A local
> health-monitor logical port supplies the probe source. One requested
> chassis owns this port. The controller waits for flow acknowledgement
> before it advertises the source MAC and IP addresses through EVPN.
>
I only skimmed through the actual patch so I might be missing things but
wouldn't it be simpler to just have new NB Load_Balancer_Health_Check
columns to specify the default "source for healthcheck packets" in case
the backend isn't local?
I don't really understand why we have a special case only for EVPN, this
sounds like a generic, non-evpn gap:
+ "Load_Balancer_Health_Check_Endpoint": {
+ "columns": {
+ "ip": {"type": "string"},
+ "port": {"type": {"key": {"type": "integer",
+ "minInteger": 1,
+ "maxInteger": 65535}}},
+ "mode": {"type": {"key": {"type": "string",
+ "enum": ["set", ["evpn"]]}}},
> The controller sends probes through normal tenant policy and forwarding.
> It checks the reply's tenant, path, addresses, TCP ports, ACK and checksum.
> Northd selects only backends with current successful check results.
> Changed forwarding data and stale results remove a backend from selection.
> The configured Northbound backend list does not change.
>
> IPv4 and IPv6 TCP use the same tenant Layer-2 network. Local and OVN-IC
> health checks retain their existing paths. The patch also corrects IPv6
> replies to Duplicate Address Detection requests. Source tests found that
> the old reply used the wrong destination and Solicited flag.
>
> Tests on the implementation:
>
> - The applied RFC patch builds and passes ten selected native test groups.
> - The current source builds with GCC 13.3.
> - All 25 selected native test groups passed on the final implementation.
> - A later check repeated all eight new health-check groups. All passed.
> - The latest four-host lab run passed 138 checks.
> - Eight real IPv4/IPv6 listener faults removed each failed backend from
> new connection selection.
> - Each fault sent 128 fresh requests to the three healthy backends.
> - Recovery sent 128 fresh requests to all four backends.
> - All 2,560 fresh requests in that run succeeded.
> - Eight healthy established connections kept the same socket and backend.
> - Earlier tests checked overlapping tenants, forged replies and restart.
> - Route withdrawal, MAC/VTEP changes and 1,000 configured checks passed.
>
> The tests used IPv6 VTEPs and VXLAN VNI 10100 for the health traffic.
> They recorded TCP SYN, SYN-ACK and cleanup RST packets. The lab used
> zebra-rs at revision f74e4d1d00f30624e55f9fd683aa0fb1dce16dc6.
> The source changes do not require a zebra-rs code change.
>
It's great you did manual testing but most of this can also be tested
with system-ovn.at and multinode.at tests.
> Limits and open work:
>
> - A Layer-2 check does not prove public VIP, NAT or application readiness.
> - A TCP reply proves transport liveness. It does not authenticate a server.
> - Routed checks are outside this patch. Existing UDP checks keep their
> current path.
> - Health changes new selections. It does not move failed connections.
> - A stopped owner cannot install new OpenFlow rules. Northd can expire
> the logical selection result while northd and Southbound remain active.
> - The Cloud Management System (CMS) must reserve each tenant's source
> addresses and MACs.
> - Remove new endpoints and source ports before an old schema downgrade.
> - Full controller cleanup can remove the lab's manual VXLAN interfaces.
> Lab recovery required interface restoration and real BGP route learning.
> - The controller stops new EVPN monitor allocation at 4,096 total entries.
> - The 1,000-endpoint sample used closed TCP ports. It did not use healthy
> applications.
> - The controller used about 45% of one core in that sample. Northd used
> about 52%.
> - The tests did not prove multihoming support. The lab did not show two
> backend Type-2 paths. It also exposed a learned/static MAC flow conflict.
> Both flows used table 109 priority 150. Route reimport restored
> forwarding.
>
> Please comment on these design choices:
>
> 1. Does a typed endpoint table with a health-monitor source port fit the
> Northbound API?
As mentioned before, I wonder if a simpler, not EVPN specific, default
way of monitor non-local backends makes more sense.
> 2. Are exclusive source ownership, controller identity, configuration
> generation and completed check sequence sufficient to reject stale
> health state?
Isn't this already covered by the current service monitor
implementation? I'm not sure I fully understand your question.
> 3. Which mixed-version upgrade rules and per-tenant allocation limits
> should this API require before merge?
>
Upgrade rules are specified in our ovn-upgrades.rst document, your
change should adhere to those. As this is a new feature it's safe to
assume that ovn-controllers are always upgraded first.
I don't understand the per-tenant allocation limits part of the question
though.
> This is one discussion patch. A final series can separate the IPv6 DAD
> correction from the health-check feature.
>
> Gavin McKee
>
> Gavin McKee (1):
> ovn: Add TCP health checks for EVPN backends.
>
> controller/binding.c | 127 ++++-
> controller/chassis.c | 6 +
> controller/if-status.c | 68 ++-
> controller/if-status.h | 2 +
> controller/neighbor-exchange-netlink.c | 70 ++-
> controller/neighbor-exchange-netlink.h | 2 +
> controller/neighbor-exchange-stub.c | 27 +
> controller/neighbor-exchange.c | 255 +++++++++
> controller/neighbor-exchange.h | 8 +
> controller/neighbor.c | 25 +-
> controller/neighbor.h | 2 +
I didn't really understand the neighbor related changes, are those bug
fixes? E.g., the neighbor withdrawal code and retry for it. That
should just work today too, doesn't it? If it doesn't, it should be a
separate patch with tests exercising the failure condition.
In general, the current "36 files changed, 2710 insertions(+), 68
deletions(-)" is very hard to review, it probably needs to be split in
smaller commits, addressing the various parts your feature requires.
> controller/ovn-controller.c | 40 +-
> controller/physical.c | 34 ++
> controller/pinctrl.c | 509 ++++++++++++++++-
> controller/pinctrl.h | 8 +
> include/ovn/actions.h | 1 +
> include/ovn/features.h | 1 +
> lib/actions.c | 13 +-
> lib/ovn-util.c | 19 +
> lib/ovn-util.h | 4 +
> northd/en-advertised-route-sync.c | 22 +-
> northd/en-northd.c | 2 +
> northd/inc-proc-northd.c | 7 +
> northd/lb.c | 80 ++-
> northd/lb.h | 6 +
> northd/northd.c | 744 ++++++++++++++++++++++++-
> northd/northd.h | 8 +
> northd/ovn-northd.c | 4 +-
> ovn-nb.ovsschema | 28 +-
> ovn-nb.xml | 82 +++
> ovn-sb.ovsschema | 34 +-
> ovn-sb.xml | 70 ++-
> tests/automake.mk | 1 +
> tests/evpn-health-checks.at | 420 ++++++++++++++
> tests/ovn.at | 48 +-
> tests/testsuite.at | 1 +
> 36 files changed, 2710 insertions(+), 68 deletions(-)
> create mode 100644 tests/evpn-health-checks.at
>
>
> base-commit: 03fd372c879bae388d81d9d5433fa2c1698ca2d9
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
