> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Wednesday, January 12, 2022 6:01 PM > To: Van Haaren, Harry <[email protected]>; Finn, Emma > <[email protected]>; [email protected]; Amber, Kumar > <[email protected]> > Cc: [email protected]; Stokes, Ian <[email protected]>; Flavio Leitner > <[email protected]>; Kevin Traynor <[email protected]> > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/6/22 14:11, Van Haaren, Harry wrote: > >> -----Original Message----- > >> From: Finn, Emma <[email protected]> > >> Sent: Wednesday, January 5, 2022 4:54 PM > >> To: [email protected]; Van Haaren, Harry <[email protected]>; > >> Amber, Kumar <[email protected]> > >> Cc: Finn, Emma <[email protected]> > >> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >> > >> --- > >> v4: > >> - Rebase to master > >> - Add ISA implementation of push_vlan action > > > > Thanks for the updated patchset Emma & Amber. > > > > Overall, this is working as expected and I've only had some minor > > comments throughout the patchset. I've added my Acked-by to most > > patches, some small open questions remain to be addressed in a v5. > > > > +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work > towards that. > > Hi, Harry, Ian, others.
Hi Ilya, > Following up from a brief conversation during today's upstream meeting. > It was brought to my attention that you're expecting this series and > the 'hash' one to be accepted into 2.17. Though there are few issues > with that: > > 1. This review for v4 was actually very first review of the patch set. > The other one as of today doesn't have any reviews at all. > Looking at the change log for this patch set it doesn't seem that > internal reviews behind the closed doors (if there were any) requested > any significant changes. In any case, internal reviews is not the way > how open-source projects work. Actions & MFEX were developed internally yes, and hence internal reviews and architecture was iterated on. Saying "not many large changes requested" is not relevant, it means that internally the architecture was well aligned. If anything, it means that reviewers did not have big concerns, hence we should have better confidence to merge. > 2. The soft freeze for 2.17 began on Jan 3 in accordance with our > release schedule (even a bit later), and as you know, during the soft > freeze we're not normally accepting patches that wasn't already reviewed > before the soft freeze begun. > https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390487.html Actions and MFEX Hashing were discussed and reviewed in public at OVS Conf; https://www.openvswitch.org/support/ovscon2021/#T32 https://www.openvswitch.org/support/ovscon2021/#T33 The closing "call to action" slide clearly states 2.17 upstream is intended, and welcome community review & comments, no concerns were raised. > That's not the end of a world, but you need to request an exception in > reply to the email linked above. As both the patches and intent to merge for OVS 2.17 were clearly discussed in public at OVS Conf, this "request exception" case does not apply. > But I have a few high-level concerns regarding the patch set itself, > and that's a bigger problem for me: > > 1. What are the benefits of these patch sets? A lot of infrastructure > changes are made, but the benefits of them are unclear. Why these > changes are needed in the end? I believe, that was the main reason > why community had no interest in reviewing these patches. > 2.17 is supposed to be a new LTS, so infrastructure changes without > clear benefits might not be a good fit taking into account time > constraints and lack of reviews. Customers workloads are accelerated, improving OVS datapath performance for their workloads. The customers are not active in SW development themselves, hence there is no reviews from them on OVS ML. If you have concerns over a patchset, these should be raised against a V1 of a patchset, in appropriate time to be addressed. Asking these questions the week of the integration deadline is unacceptable, and may not be used to block merge a patchset. > 2. The same concern that I already brought to you attention in other > conversations, e.g. on the ovs-security list about a month ago. > It's related to all developments in that area: why this is tied to > the userspace datapath? i.e. why execution of actions depends on the > datapath? This seems artificial and complicates testing a lot. > Like current autovalidator is not able to test most of the packet > parsing cases, the same way autovalidator will not be able to test > execution of actions. Regarding testing, as per that same conversation, I replied that yes we would investigate improving testing even further. There is ongoing work here around improving our fuzz testing. Autovalidation is not complex, it validates all unit-tests, and has been presented at OVS conferences, and upstreamed for multiple components (DPCLS, MFEX, and now Actions). If there is concern over approach, then this should not be held against Actions patchset now in the week of merge, but addressed in general. Not acceptable to block Actions or Hashing patchsets based on this, in the week of the integration deadline. > I have some more comments, but they are more related to the actual code > and above 2 are the most important for now. > > Best regards, Ilya Maximets. Regards, -Harry _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
