On 10/12/26 6:58 PM, Gavin McKee wrote: > Hi Dumitru, > Hi Gavin,
> Thank you for the review. I agree that the API should be more general > and that the patch needs a smaller series. > > The target case is a backend with no logical port in the probing > deployment. EVPN is the transport used in my lab. The endpoint table > associates each backend with a local source port and source address. > I will investigate default source fields on Load_Balancer_Health_Check. > The source must still identify the tenant context, especially when a > load balancer has multiple attachments. > Ah, now I understand why you needed the new Load_Balancer_Health_Check_Endpoint table, it's to hold per switch attachment point source information. Maybe that's fine but what if we move the configuration on the Logical Switch itself? That is, the logical switch where a LB with HC enabled is attached, may hold configuration for the default HC source fields it should use (essentially what you have in Load_Balancer_Health_Check_Endpoint in your RFC)? > The RFC extends the existing Service_Monitor implementation. It already > provides ownership checks, flow acknowledgement, and TCP reply > correlation. My freshness question concerned an online result that > remains in SB after completed checks stop. The added sequencing and > northd expiry address that case. The RFC also invalidates results when > source or forwarding state changes. I will review which additions are > needed, and treat any general freshness change separately, with focused > tests. > > The neighbor changes combine the new source's lifecycle with general > cleanup handling. Existing synchronization already withdraws stale > entries while an interface remains monitored. The added cleanup withdraws > previous announcements when an interface monitor is removed, and queues > failed withdrawals for retry. It also withdraws the new source during > shutdown. I will separate the general changes and add tests that > reproduce failures on the existing code before presenting them as bug > fixes. Interesting, I thought we would retry even today without any required changes. In any case, as said, it should be a separate patch (it can be part of the same series). > > I will send the IPv6 DAD correction separately and split the feature > into smaller commits. The native tests cover configuration and source > flows. I will add system-ovn.at and multinode.at coverage for actual > probes, failure and recovery, route withdrawal, restart, and overlapping > tenant addresses. > > I will follow the documented controller-first rolling upgrade procedure. > I will test the new controller with the old schema, then test schema > conversion and feature activation after the northd upgrade. > > By allocation limits, I meant probe resource consumption. The RFC's > allocation guard is controller-wide. I should have described that > more clearly. > > Would default source fields on the existing health-check row be a useful > starting point for the revised API? > Maybe not, as mentioned above, with the fact that a LB may be attached to multiple switches/routers, you're right we might need a better way of defining these defaults. > Regards, > Gavin > Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
