On 2/3/25 11:51 AM, Felix Huettner wrote: > On Thu, Jan 30, 2025 at 11:31:16AM +0100, Dumitru Ceara wrote: >> On 1/30/25 11:21 AM, [email protected] wrote: >>> On Tue, 2025-01-28 at 17:42 +0100, Felix Huettner wrote: >>>> Hi everyone, >>>> >>>> i still need to review the actual patch, but i wanted to join the >>>> discussion. >>>> >>>> On Tue, Jan 28, 2025 at 04:19:37PM +0100, Frode Nordahl wrote: >>>>> Hello all, >>>>> >>>>> Apologies for the tardy response, I've been pursuing an interesting >>>>> bug related to the "add_route" NAT option and routing IPv4 over >>>>> IPv6 >>>>> next hop, and wanted to see that to an end before chiming in. >>>>> Patches >>>>> imminent. >>>>> >>>>> On Tue, Jan 28, 2025 at 1:50 PM Dumitru Ceara <[email protected]> >>>>> wrote: >>>>>> >>>>>> On 1/28/25 1:33 PM, [email protected] wrote: >>>>>>> Hi Frode/Numan/Dumitru, >>>>>>> Thanks for the review and interest in this patch, I'll put my >>>>>>> two cents >>>>>>> to the discussion below. >>>>>>> >>>>>> >>>>>> Hi Martin, >>>>>> >>>>>>> On Mon, 2025-01-27 at 11:57 +0100, Dumitru Ceara wrote: >>>>>>>> On 1/26/25 4:49 AM, Numan Siddique wrote: >>>>>>>>> On Sat, Jan 25, 2025 at 5:52 PM Frode Nordahl >>>>>>>>> <[email protected]> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Hello, >>>>>>>>>> >>>>>>>>>> Note that this email is sent at a time convenient to me, >>>>>>>>>> please >>>>>>>>>> don't >>>>>>>>>> feel obliged to read nor respond to it until a time >>>>>>>>>> convenient >>>>>>>>>> for >>>>>>>>>> you. >>>>>>>>>> >>>>>>>>>> The timing of this bit of time skewed work is because I >>>>>>>>>> anticipated >>>>>>>>>> the output to be a new patch set for which we would have >>>>>>>>>> a desire >>>>>>>>>> to >>>>>>>>>> try to get within soft freeze. It became apparent that it >>>>>>>>>> would >>>>>>>>>> likely >>>>>>>>>> fit within the context of the patch set you diligently >>>>>>>>>> posted on >>>>>>>>>> friday, so I'll leave the result of it as a review >>>>>>>>>> comment >>>>>>>>>> instead. >>>>>>>>>> >>>>>>>>>> On Thu, Jan 23, 2025 at 3:00 PM Martin Kalcok >>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>> This change builds on top of the new "dynamic routing" >>>>>>>>>>> OVN >>>>>>>>>>> feature >>>>>>>>>>> that allows advertising routes to the fabric network. >>>>>>>>>>> When LR >>>>>>>>>>> option >>>>>>>>>>> "dynamic-routing" is set on the router, following two >>>>>>>>>>> new LRP >>>>>>>>>>> options >>>>>>>>>>> become available: >>>>>>>>>>> >>>>>>>>>>> * redistribute-nat - When set to "true", ovn-controller >>>>>>>>>>> will >>>>>>>>>>> advertise >>>>>>>>>>> routes for external NAT IPs valid >>>>>>>>>>> for the >>>>>>>>>>> LRP. >>>>>>>>>>> * redistribute-lb-vips - When set to "true", ovn- >>>>>>>>>>> controller >>>>>>>>>>> will advertise >>>>>>>>>>> host routes to LB VIPs via the >>>>>>>>>>> LRP. >>>>>>>>>>> >>>>>>>>>>> Co-authored-by: Frode Nordahl <[email protected]> >>>>>>>>>>> Signed-off-by: Frode Nordahl <[email protected]> >>>>>>>>>> >>>>>>>>>> Thanks! Good to have these options resurrected and >>>>>>>>>> adapted to the >>>>>>>>>> current state of the fabric integration patch epic. >>>>>>>>>> >>>>>>>>>> After having read up a bit on the current state of review >>>>>>>>>> of our >>>>>>>>>> sibling options "dynmic-routing", "dynamic-routing- >>>>>>>>>> connected" and >>>>>>>>>> "dynamic-routing-static", I wonder if we need to make the >>>>>>>>>> nat and >>>>>>>>>> LB >>>>>>>>>> options have similar prefixes? >>>>>>> >>>>>>> Yeah, "dynamic-routing-redistribute-lb-vips" is kinda mouthful, >>>>>>> but if >>>>>>> that's the prefix for every other related option, we should >>>>>>> follow it. >>>>>>> >>>>>> >>>>>> Alternatively we could try to change the dynamic-routing- >>>>>> redistribute-* >>>>>> options and instead adding a boolean for each "redistribute" >>>>>> option we >>>>>> could change "dynamic-routing-redistribute" to be a list of >>>>>> entities we >>>>>> want to redistribute, e.g.: >>>>>> >>>>>> LR.options.dynamic-routing-redistribute="connected;nat;lb" >>>>> >>>>> On the topic of NAT and LB, as discovered during review, there is >>>>> an >>>>> oddly named get_nat_addresses() function that can retrieve both NAT >>>>> and LB addresses. >>>>> >>>>> To save us the work of refactoring it, perhaps we could use a >>>>> single >>>>> keyword for both NAT and LB addresses? I mean, would you ever want >>>>> one >>>>> without the other? >>>> >>>> "dynamic-routing-redistribute" looks like the shorter version. >>>> However i see an advantage in the individual "dynamic-routing- >>>> static", >>>> etc. options as they could allow us to use more values than just >>>> "true" >>>> and "false". E.g. they could in the future be used to filter for a >>>> given >>>> prefix and only announce that. >>>> >> >> In my opinion if we want to add support for route filtering (route-maps >> or whatever we call them) a single string will not be enough anyway to >> express potentially complex conditions. >> >>>> But that is just an idea on a potential benefit. I don't actually see >>>> a >>>> reason to currently built that. >>> >>> It occurred to me that we could also use `dynamic-routing- >>> nat`/`dynamic-routing-lb-vips` names. This would be both shorter and >>> kept the door open for non-boolean values in the future. >>> >> >> My vote still goes to a single option with value the list of >> protocols/objects we want to redistributed. But I'm not going to block >> the per-object implementation if that's what the majority prefers. > > Hi Dumitru, > > since on IRC on Thursday there was no heavy opinion in either way I will > implement the "dynamic-routing-redistribute" for the next version of the > patches. Then everyone can take a look how it feels. >
Sounds good to me, thanks! > Thanks a lot, > Felix > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
