On 5/22/23 17:39, Ihar Hrachyshka wrote: > On Mon, May 22, 2023 at 9:55 AM Dumitru Ceara <dce...@redhat.com> wrote: >> >> On 5/22/23 15:42, Ihar Hrachyshka wrote: >>> On Mon, May 22, 2023 at 7:55 AM Dumitru Ceara <dce...@redhat.com> wrote: >>>> >>>> On 5/17/23 18:46, Ihar Hrachyshka wrote: >>>>> Thank you Dumitru! See below. >>>>> >>>>> On Tue, May 16, 2023 at 9:41 AM Dumitru Ceara <dce...@redhat.com> wrote: >>>>>> I'm not necessarily rejecting this change. I just wanted to bring up an >>>>>> alternative approach (I'm not sure if it's possible to implement it >>>>>> though): >>>>>> >>>>>> The CMS (e.g., Neutron) probably knows before hand the reduced MTU value >>>>>> it should impose on the localnet port. That means it might be possible >>>>>> to just implement all this through logical flows that use >>>>>> 'check_pkt_larger()'. >>>>>> >>>>> >>>>> I think I mentioned this alternative in my cover letter for the >>>>> series. (At least one of the variations.) >>>>> >>>>> I don't understand the specifics of what you are describing here >>>>> though. Yes, CMS may do calculations for the "desired path MTU". But >>>>> what's the mechanism for CMS to pass this information down to the OVN >>>>> controller? ACL? I don't think the space of possible actions that >>>>> would allow CMS itself to define the behavior is available to CMS. >>>>> >>>>> Perhaps you mean (as I think we discussed before in private) the >>>>> approach where OVN northd creates Logical_Flow objects that would >>>>> implement the behavior. If so, yes that could be possible. But note >>>>> that northd doesn't know the desired path MTU (because it's not CMS >>>>> and OVN northbound database API doesn't support MTU primitive for >>>>> ports.) So the best we could do in the alternative way is implement >>>>> half of the solution via Logical_Flow (the one that sends ICMP replies >>>>> for "too-big" frames), leaving the other half (tagging the "too-big" >>>>> frames based on interface MTU for processing by the Logical_Flows) to >>>>> OVN controller. I think I listed this variation of the approach in my >>>>> cover letter, but please let me know if it's not sufficient. >>>>> >>>>> (The split could be avoided if OVN adds a MTU attribute to its ports. >>>>> But I of course am not trying to tackle this in this series, for >>>>> obvious reasons. But MTU for ports is - in my mind - a worthy feature >>>>> to have in OVN, if you ask me.) >>>> >>>> I was referring to this last part: adding MTU for ports and ovn-northd >>>> translating those to logical flows that use check_pkt_larger() and set >>>> REGBIT_PKT_LARGER. I was also hoping to see logical flows that generate >>>> the ICMP error packets. But there's indeed no obvious way of doing that >>>> because we need to act only on traffic that's destined to remote >>>> hypervisors and that's not really an ovn-northd concept but more of an >>>> ovn-controller one. >>>> >>>> If we go for your current approach and later add MTU for OVN ports, do >>>> we need to be careful now in order to avoid complicated upgrades later? >>>> >>> >>> I'm trying to think what complications could arise. Isn't it just >>> generating effectively the same flows, now via Logical_Flow path? The >>> flows act on local frames generated by regular VIF ports, so it >>> shouldn't affect the rest of cluster that e.g. some of them are >>> generated by Logical_Flow path and some through the proposed solution. >>> Or am I talking about something else? >>> >> >> Assuming your current approach is accepted it means ovn-controller will >> automatically compute min MTU to use for multi-chassis ports. But if we >> add an explicit OVN port MTU configuration what do we do with the >> automatic computation? >> >> Do we leave it in place or do we remove it? > > We'll make OVN LSP `mtu` override interface-derived value. (If > ovn-controller sees `mtu` set for LSP, it seizes its operation; > otherwise it sets flows itself.) > >> >> If we remove it it means we'll break upgrades as northd is upgraded last >> and it will only set the OVN port binding MTU after it gets upgraded. >> > > We'll have both for one LTS release and sunset the interface-derived > value, if needed? >
Sure, that would work. >> If we leave it in place I'm not sure what the benefit of the explicit >> MTU is. I might be wrong though. > > There are benefits of OVN LSP API `mtu` regardless of Path MTU > Discovery series. For one, OVN could enforce its MTU on VIF interfaces > (set it in `mtu_request`). This would also potentially allow a > chassisA to inject Path MTU Discovery flows when `mtu` is changed on > chassisB independently of ports hosted on chassisA. Right now > ovn-controller is aware of MTUs for the ports it owns, but not for the > ports on other chassis. > > I wouldn't look at `mtu` as OVN API as just a means to simplify the > task for Path MTU, there are other benefits too. > Good points above. It makes sense to have the per port MTU as OVN API in general indeed. Thanks for the details! _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
