On Mon, Apr 17, 2023 at 9:24 AM Numan Siddique <[email protected]> wrote: > > On Mon, Apr 17, 2023 at 5:57 AM Dumitru Ceara <[email protected]> wrote: > > > > On 4/17/23 11:55, Dumitru Ceara wrote: > > > On 4/14/23 18:01, Han Zhou wrote: > > >> > > >> > > >> On Fri, Apr 14, 2023 at 2:16 AM Dumitru Ceara <[email protected] > > >> <mailto:[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] > > >> <mailto:[email protected]>> wrote: > > >>>>> > > >>>>> > > >>>>> > > >>>>> On Thu, Apr 13, 2023 at 12:35 PM Dumitru Ceara <[email protected] > > >> <mailto:[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] > > >> <mailto:[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] <mailto:[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] > > >> <mailto:[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 > > >> <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? > > >> > > > Hi Han, > > > > > > I changed the subject of the email to better reflect the new discussion. > > > > > > There's this statement in the release-process document: "LTS releases > > > will receive bug fixes until the point that another LTS is released. At > > > that point, the old LTS will receive an additional year of critical and > > > security fixes." > > > > > > Which further differentiates an LTS release from a non-LTS release.
If you are saying "the old LTS will receive an additional year of critical and security fixes" is the different part, I agree. But still, in this case, for critical and security fixes, when applied to the LTS for the *additional* year, do you agree that we will skip non-LTS releases? If not skipping, then it makes no difference. > > > > > >> 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 > > >> <mailto:[email protected]> @Numan Siddique <mailto:[email protected]> > > >> > > > > > > Checking the OVS/OVN release-process documents again it's indeed not > > > explicitly specified that non-LTS stable release branches are supposed > > > to get all bug fixes. But the OVS document has this addition: > > > > > > "While branches other than LTS and the latest release are not formally > > > maintained, the OVS project usually provides stable releases for these > > > branches for at least 2 years, i.e. stable releases are provided for the > > > last 4 release branches. However, these branches may not include all > > > the fixes that LTS has in case backporting is not straightforward and > > > developers are not willing to spend their time on that (this mostly > > > affects branches that are older than the LTS, because backporting to LTS > > > implies backporting to all intermediate branches)." > > > > > > This last part really makes me think that we SHOULD backport to all > > > intermediate branches: "... because backporting to LTS > > > implies backporting to all intermediate branches". > > > > > I agree with this. > If we consistently backport to all intermediate releases, the LTS release appears to function more as a reference point in the series of releases, rather than a version with additional maintenance support. This is ok if we all agree to it. > > > Maybe we should make this rule (or some other backport strategy we > > > decide to use from this point on) explicit indeed. > > +1 on for this. > > If we have these branches > > - Latest release (R9) > - R8 > - R7 > - R6 (LTS) > - R5 > - R4 > - R3 > - R2 (LTS) > - R1 > > IMO it should be enough to backport bug fixes up to the latest LTS > (which is R6) in the above example. This example looks not bad, but it is not how OVN would be released, because for OVN "LTS releases are scheduled to be released once every two years". So it will look like: - Latest release (R17) ... - R12 - R11 - R10 (LTS) ... - R4 - R3 - R2 (LTS) - R1 We will always have to backport from R17 down to R10 in this example (8 branches). > In case we decide to backport to R2 as well, then I think we should > not skip intermediate branches (R5, R4 and R3) > and instead backport upto R2. I don't think we will ever want to backport to R2 in this case. If we do, it would mean from R17 - R2, which includes 16 branches. But look at another example: - Latest release (R13) - R12 - R11 - R10 (LTS) ... - R4 - R3 - R2 (LTS) - R1 Because the new LTS (R10) in this example is released less than a year, according to the rule, we will support the older LTS (R2) for critical / security bug fixes. I think we need to agree on whether we should skip R3 - R9. Thanks, Han > > +1 from to have a formal process for backports. > > Numan > > > > > > > > > > > CC: Ilya for input from OVS perspective. > > > > > CC: Ilya for input from OVS perspective. (this I actually CC-ed him..) > > > > > > > > Regards, > > > Dumitru > > > > > > > _______________________________________________ > > 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
