Hi Mark! Thanks a lot for the review!
v7 will have the changes based on your feedback. I found the need to rework status handling because of RBAC. Individual ovn-controllers shouldn't be able to modify shared state in OVN SB so I am inclined to change the schema after all and add Advetised_Route_Status such that it holds a status and references to the Advertised_Route and the Chassis that owns this status. > I like the approach of using > many compact test cases instead of writing a single monster test Thanks, I think it's going to be very difficult to maintain this feature without this approach. I am also very interested in making sure it doesn't break because of invariant changes over time so hopefully the effort will pay off long term. The patchset size is larger as a result but I hope for a good reason. > * I think we can consider patch 1 for inclusion in OVN 26.09 even if > we don't manage to get the other 3 merged during the soft freeze > period (it starts Friday July 24 and ends Friday August 21). Patch 1 Works for me, I tried to split it into relatively independent changes so it could be landed piece by piece. P1 is a bugfix indeed and the overall design is interesting when P2+ patches are in-place but I think they don't have to land all at once. I also tried to make the functionality opt-in as much as possible, so even if it lands, it doesn't get auto-enabled for existing deployments that don't rely on distributed LBs. > * I think patch 2 should be split in two. Will do, that makes sense. > * I'm curious if you did any performance testing on this series. > Patches 3 and 4 in particular have some pretty deep nested loops in > areas. In practice, this may be fine if the loops don't tend to > iterate over many members. But on paper, it seems like it could be a > drag on performance. I'm NOT saying you need to do performance > testing in order for your patches to be included. If we merge this > series, and then our own performance tests indicate that this series > caused a regression, then it may end up getting reverted. This would > likely happen after branching 26.09, and so the series not be a > candidate for OVN 26.09 any longer. If you're able to performance > test the patches and fix any potential regressions before the soft > freeze period is completed, then we may still be able to get it into > 26.09. We still have four weeks before branching, and since I'm > reviewing the series right now, it remains a candidate for inclusion > so long as we get it merged before the soft freeze period ends. I have some downstream ovn-heater modifications along with some automation to set up an environment to run it. I am going to post a branch as soon as it's a little bit more decent. I'll address the nested loops in v7 but will post some performance numbers as well anyway. I am not in a rush for 26.09 but landing some of the patches in 26.09 would make future iterations easier since I won't need to rebase that much. I have a couple of changes that I postponed intentionally like the appctl extensions to evict routes based on operator/l7 healthcheck agent policy. This can be managed externally in FRR or other dynamic routing daemon policies until we figure out if we need this feature in OVN and how this is going to look like. Best Regards, Dmitrii On Thursday, July 23rd, 2026 at 9:48 PM, Mark Michelson <[email protected]> wrote: > Hi Dmitrii, > > I was able to take a look at this series yesterday and today. Overall, > I think this is a good, useful contribution. My overall thoughts: > * You do an EXCELLENT job writing tests. I like the approach of using > many compact test cases instead of writing a single monster test for > the changes. > * I think we can consider patch 1 for inclusion in OVN 26.09 even if > we don't manage to get the other 3 merged during the soft freeze > period (it starts Friday July 24 and ends Friday August 21). Patch 1 > is a bug fix, rather than a feature addition, so it qualifies for > merging at any point. > * I think patch 2 should be split in two. It appears to be doing two > separate things. First, it does the main job outlined in the commit > message of adding new advertised routes. It also is adding tracking > data to the dynamic_routes engine node. I think the tracking data > changes should be separated into a different commit. > * I'm curious if you did any performance testing on this series. > Patches 3 and 4 in particular have some pretty deep nested loops in > areas. In practice, this may be fine if the loops don't tend to > iterate over many members. But on paper, it seems like it could be a > drag on performance. I'm NOT saying you need to do performance testing > in order for your patches to be included. If we merge this series, and > then our own performance tests indicate that this series caused a > regression, then it may end up getting reverted. This would likely > happen after branching 26.09, and so the series not be a candidate for > OVN 26.09 any longer. If you're able to performance test the patches > and fix any potential regressions before the soft freeze period is > completed, then we may still be able to get it into 26.09. We still > have four weeks before branching, and since I'm reviewing the series > right now, it remains a candidate for inclusion so long as we get it > merged before the soft freeze period ends. > > I have a few comments on each patch in the series as well, so please > have a look at those. > > I have additional > > On Thu, Jul 2, 2026 at 10:55 AM Dmitrii Shcherbakov via dev > <[email protected]> wrote: > > > > OVN 26.03 introduced distributed load balancers [1]: when a > > Load_Balancer has options:distributed=true and ip_port_mappings is > > configured, each chassis delivers traffic only to the local backend. > > Deployments that use BGP to advertise tenant routes into the fabric > > [2] could previously advertise LB VIPs only as a single prefix per > > VIP, with no per-chassis health awareness. The fabric could ECMP > > across chassis but could not prefer those hosting healthy backends > > or withdraw routes from chassis with unhealthy ones. > > > > This series splits the Advertised_Route emission for LB VIPs from > > one-per-VIP to one-per-(VIP IP, backend LSP) and adds a controller- > > side gate that skips route installation when the Service_Monitor for > > the corresponding backend is offline. This allows the local dynamic > > routing speaker to advertise the VIP prefix only from chassis that > > host a healthy backend, so fabric ECMP converges onto healthy > > chassis. > > > > The series also installs local forwarding routes on the advertising > > logical router for peer-LR LB VIPs and NAT external IPs enumerated > > for redistribution, so that the advertising LR can forward traffic to > > those addresses through the peer. > > > > Patch 1 fixes an existing bug where attaching a distributed LB to a > > router with a chassis-redirect port can leave stale lr_in_admission > > guards, dropping ingress LB traffic on non-gateway chassis. > > > > Patch 2 installs local forwarding routes for peer-LR LB VIPs and NAT > > external IPs that an advertising LRP has enumerated for redistribution. > > > > Patch 3 splits Advertised_Route emission from one-per-VIP to > > one-per-(VIP IP, backend LSP), deduplicated by the existing SB unique > > index. Each LB-derived row is tagged with external_ids:source=lb. > > Also documents external_ids:source under Advertised_Route in > > ovn-sb.xml. > > > > Patch 4 adds the controller-side gate using a filtered two-pass > > approach: kernel-route installation is skipped when all > > Service_Monitor rows matching the derived backend selector are > > offline. Adds the Load_Balancer SB table as an engine input to the > > route node. Honors external_ids:enabled as an administrative enable > > flag and publishes advertisement status to external_ids:status. > > Only the owner chassis writes status. When ownership transitions > > the value may be stale until the new owner updates it. Also > > documents external_ids:enabled and external_ids:status under > > Advertised_Route in ovn-sb.xml. > > > > The nexthop comparison fix previously included as patch 2 in v4 has > > been dropped: it was merged upstream independently in commit > > ace80f762 ("northd: Fix the route and route policy lookup > > functions."). > > > > Changes since v5: > > - Patch 4: add #include <netinet/in.h> before <arpa/inet.h> in > > controller/route.c to fix a build failure under OVS sparse > > wrappers, which require <netinet/in.h> first for FreeBSD > > portability. > > > > Changes since v4: > > > > The most significant change is that v5 requires no SB schema changes. > > The backend service selector (ip, port, protocol) needed by > > ovn-controller for Service_Monitor gating is now derived at runtime > > from the Load_Balancer.vips smap instead of being stored in new SB > > columns on Advertised_Route. > > > > In v4 the selector was carried in tracked_service_{ip,port,protocol} > > columns whose values were also part of the Advertised_Route unique > > index, allowing northd to emit one row per (VIP IP, backend LSP, > > listener port) triple. Extending the unique index requires new SB > > columns and a schema version bump. > > > > In v5 the selector is not stored on the Advertised_Route row at all. > > Northd emits one row per (VIP IP, backend LSP) pair, deduplicated by > > the existing unique index on (datapath, logical_port, ip_prefix, > > tracked_port). Multiple LB listeners that share the same VIP IP and > > backend LSP (e.g. two Services on different ports fronting the same > > pod) collapse into a single row. Per-listener health is already > > available in the Service_Monitor table, and the kernel route is > > per-prefix regardless, so the collapsed rows lose no information > > needed for correct gating. > > > > Each LB-derived row is tagged with external_ids:source=lb so the > > controller can distinguish it from NAT, connected, or static routes > > that may share the same ip_prefix. Only routes with this marker are > > candidates for Service_Monitor gating. Non-LB routes are never > > gated. > > > > The controller uses a filtered two-pass approach: route_run() first > > collects LB-route candidates (source=lb rows whose tracked_port is > > local). If no candidates exist, the Load_Balancer and > > Service_Monitor tables are not scanned. When candidates exist, the > > controller scans the Load_Balancer.vips smap for VIP keys whose IP > > matches each candidate's ip_prefix, collects the matching backend > > (ip, port, protocol) tuples, and cross-references them against > > Service_Monitor rows for the route tracked_port. The route is > > installed when any matching Service_Monitor is online or when no > > matching Service_Monitor exists, and withdrawn only when matching > > Service_Monitor rows exist but all are offline. > > > > Each VIP IP is gated independently: a route for VIP A is withdrawn > > only when all of VIP A's backends are offline, regardless of the > > health state of VIP B backends on the same LSP. This preserves the > > K8s multi-Service-per-pod isolation where one pod backs several VIPs > > whose health states are independent. > > > > The controller publishes its advertisement decision to > > external_ids:status as advertised, withdrawn-admin (when an operator > > sets external_ids:enabled=false), or withdrawn-monitor (when the > > Service_Monitor is offline). Only the owner chassis writes status. > > When ownership transitions the value may be stale until the new owner > > updates it. > > > > Patch 1 is unchanged from v4. The v4 nexthop fix (patch 2) was > > dropped because it was merged upstream as commit ace80f762. > > Patches 2-4 correspond to v4 patches 3-5, with patches 3-4 > > rewritten for the derivation approach. > > > > Changes since v3 (patch 1): > > - Fix -Wshadow violation in build_lb_datapaths: rename inner > > loop variable od to grp_od to avoid shadowing the outer-scope > > od declared earlier in the same function. > > > > Changes since v2 (all in patch 1): > > - Track has_distributed_lb on LB-group expansion and deletion paths > > so that the northd handler's distributed-LB guard covers all > > incremental code paths that involve distributed LBs. > > - Instead of unconditionally forcing a full northd recompute when > > any distributed LB change is tracked, collect a snapshot of each > > affected LR's is_distributed state before incremental processing, > > then after processing recompute the flag per LR using the per-LR > > LB list (lr_lb_map). If any LR's flag changed, fall back to full > > recompute. Otherwise the incremental path is safe to use. > > - Expose find_od_lb_data() so the post-check can walk only the LBs > > attached to each affected LR, not every LB in the system. > > - Add regression tests covering direct LB attach/detach, LB-group > > attach/detach, options:distributed toggle, LB-group membership > > changes, multiple distributed LBs on the same LR, the incremental > > path when a second distributed LB is attached (no is_distributed > > transition) and full LB deletion while still attached to an LR. > > > > Changes since v1: > > - Patch 2: fix pre-existing typos in en-advertised-route-sync.c > > comments. > > - Patch 2: add regression test for nexthop mutation (change peer LRP > > address and verify forwarding flow updates). > > - Patch 2: add negative test for LB-level dynamic-routing-advertise= > > false (verify both Advertised_Route and forwarding flow are > > not installed). > > > > Changes since v0: > > - Patch 2 (was 1): swap in pre-existing parsed_route objects on > > match so that pointers held by group_ecmp_route remain valid. > > Detach from old_parsed_routes first to avoid corrupting the > > source hmap. > > - Share-advertise system tests moved to the deferred > > share-advertise-routes follow-up, since they depend on > > the dynamic-routing-share-advertise-routes option. > > - Inline diagrams switched to ASCII-only characters. > > - Line lengths rewrapped per checkpatch feedback. > > > > A separate LRP option for VRF route sharing (share-advertise-routes) > > is deferred to a follow-up series. Deployments that need cross-VRF > > route sharing before the option lands can achieve the same effect > > by configuring the dynamic routing speaker to import routes between > > the OVN-managed VRFs on each chassis. > > > > Prior work: RFC for LB BGP advertisement [3] in which a review [4] > > suggested a separate incremental processing node for dynamic routes, > > which this series adopts. For tracked_port, patch 2 uses the peer LRP > > for forwarding routes and patch 3 uses the backend LSP (from > > ip_port_mappings) for per-backend locality rows. Patch 4 adds > > Service_Monitor gating on the per-backend rows. > > > > Tests cover LB and NAT route redistribution (IPv4/IPv6), forwarding > > route installation, per-backend route emission, ip_port_mappings > > fallback, Service_Monitor gating, shared-backend per-VIP isolation, > > shared-VIP-IP cross-VIP isolation, non-LB route overlap with LB VIP > > prefixes, source=lb marker emission and reconciliation, and > > advertisement status transitions (advertised, > > withdrawn-admin, withdrawn-monitor). > > > > [1] commit 7b0eb4d9ed ("northd: Add distributed load balancer support.") > > [2] > > https://opendev.org/openstack/neutron-specs/src/branch/master/specs/2025.2/ovn-bgp-integration.rst > > [3] https://mail.openvswitch.org/pipermail/ovs-dev/2025-February/420552.html > > [4] https://mail.openvswitch.org/pipermail/ovs-dev/2025-February/420789.html > > > > > > Dmitrii Shcherbakov (4): > > northd: Recompute only when LR is_distributed flips on LB. > > northd: Install forwarding routes for redistribute={lb,nat}. > > northd: Emit per-backend Advertised_Route for LB redistribution. > > controller: Skip LB route install when Service_Monitor is offline. > > > > controller/ovn-controller.c | 61 +- > > controller/route.c | 314 +++++- > > controller/route.h | 20 + > > northd/en-advertised-route-sync.c | 409 +++++++- > > northd/en-advertised-route-sync.h | 25 +- > > northd/en-group-ecmp-route.c | 76 +- > > northd/en-group-ecmp-route.h | 4 + > > northd/en-lb-data.c | 22 +- > > northd/en-lb-data.h | 3 + > > northd/en-lr-stateful.c | 2 + > > northd/en-northd.c | 1 + > > northd/inc-proc-northd.c | 5 + > > northd/lb.c | 2 + > > northd/lb.h | 1 + > > northd/northd.c | 211 +++- > > northd/northd.h | 3 + > > ovn-sb.xml | 40 +- > > tests/ovn-inc-proc-graph-dump.at | 10 +- > > tests/ovn-northd.at | 1524 ++++++++++++++++++++++++++++- > > tests/system-ovn.at | 691 +++++++++++++ > > 20 files changed, 3321 insertions(+), 103 deletions(-) > > > > -- > > 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
