On Fri, Apr 14, 2023 at 2:16 AM Dumitru Ceara <[email protected]> wrote: > > On 4/14/23 04:15, Han Zhou wrote: > > On Thu, Apr 13, 2023 at 12:44 PM Han Zhou <[email protected]> wrote: > >> > >> > >> > >> On Thu, Apr 13, 2023 at 12:35 PM Dumitru Ceara <[email protected]> wrote: > >>> > >>> On 4/13/23 21:21, Han Zhou wrote: > >>>> On Thu, Apr 13, 2023 at 8:37 AM Dumitru Ceara <[email protected]> > > wrote: > >>>>> > >>>>> On 4/13/23 17:34, Han Zhou wrote: > >>>>>> On Thu, Apr 13, 2023 at 12:54 AM Dumitru Ceara <[email protected]> > >>>> wrote: > >>>>>>> > >>>>>>> On 4/13/23 07:07, Han Zhou wrote: > >>>>>>>> In RFC1812 section 5.3.1, it is mentioned that: > >>>>>>>> > >>>>>>>> If the TTL is reduced to zero (or less), the packet MUST be > >>>>>>>> discarded, and if the destination is not a multicast address > > the > >>>>>>>> router MUST send an ICMP Time Exceeded message ... > >>>>>>>> > >>>>>>> > >>>>>>> The code itself looks OK but I wonder a bit about the rationale. > > Do > >>>> you > >>>>>>> have an example in which OVN replies with Time Exceeded for > > multicast > >>>>>>> destinations and that causes issues? > >>>>>>> > >>>>>>>> So if the destionation is a multicast address the route shouldn't > > send > >>>>>>>> ICMP Time Exceeded, but the current OVN implementation didn't > > check > >>>>>>>> multicast and tries to send ICMP regardless. This patch fixes it. > >>>>>>> > >>>>>>> The statement "if destination is not a multicast address the router > >>>> MUST > >>>>>>> send an ICMP Time Exceeded message" implies that "if destination > > is a > >>>>>>> multicast address the router MAY or MAY NOT send an ICMP Time > > Exceeded > >>>>>>> message". So the fact that OVN sends one is not necessarily wrong. > >>>>>> > >>>>>> For my limited understanding of multicast, sending ICMP time > > exceeded is > >>>>>> not a good idea. In multicast TTL has special meanings, for example: > >>>>>> TTL 0: Restricted to the same host, not transmitted by the > > router. > >>>>>> TTL 1: Restricted to the same subnet, not forwarded by the > > router. > >>>>>> If we send ICMP for such packets, it means for a very common use > > case of > >>>>>> multicast (ttl = 1, same subnet), we will end up sending ICMP for > > every > >>>>>> normal packet. > >>>>>> In production we saw this happening with a rate higher than 10k pps! > >>>>>> > >>>>> > >>>>> Makes sense, thanks for the details! > >>>>> > >>>>>> So I believe this is the reason behind the "if" statement in the > > RFC. > >>>> Maybe > >>>>>> I should add this rationale in the comment, too. > >>>>>> > >>>>> > >>>>> If you could add some of the details above to the commit log too > > then: > >>>>> > >>>>> Acked-by: Dumitru Ceara <[email protected]> > >>>> > >>>> Thanks Dumitru. I applied the series to main. > >>>> Han > >>> > >>> This sounds like a relevant bug fix. Should this get backported to > >>> stable branches too? > >>> > >> Sound good. I will do that. > > > > I backported this series to the last release branch-23.03 and the LTS > > branch-22.03. > > The code base has changed a lot so was mostly manually backported instead > > of cherry-pick. > > > > Thanks for that but I think we shouldn't skip stable branches in > between. We likely still have users on those that won't upgrade to the > latest 23.03 but instead to a new z-stream release (when that happens) > of the stable branch. > > I cherry picked your backports and made sure the tests pass and then > pushed them to branches 22.12, 22.09 and 22.06.
Thanks Dumitru for helping backporting. I was uncertain about whether we should continue backporting without skipping branches, as our growing number of released branches makes this approach increasingly difficult to sustain. The release process document ( https://docs.ovn.org/en/latest/internals/release-process.html) doesn't specify how long we should maintain non-LTS branches. My understanding was that we should primarily maintain LTS and the latest releases for regular bug fixes, while critical/security fixes would be applied to all branches. It appears we have different interpretations of this process. While it's beneficial to backport to all branches, I thought this was more of a convenience when there weren't too many branches to manage. In the worst case, a patch may require manual backporting to each branch if new features make the codebase in each branch unique. I also question what differentiates an LTS branch if we're backporting to all branches anyway. My assumption was that users opting for a non-LTS branch should be prepared to upgrade to newer releases, given the "short-term" maintenance of these branches. Is my understanding incorrect? Perhaps we should discuss and formalize our backporting practice going forward, as the number of branches will only continue to grow in the coming months and years. cc @Mark Michelson <[email protected]> @Numan Siddique <[email protected]> Regards, Han > > Regards, > Dumitru > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
