Hi,
On Thu, Jan 13, 2022 at 10:53:57AM +0000, Van Haaren, Harry wrote: > > -----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. Yes but it can also mean that no careful review has been done. That's why the ask here is to do the review in the open, otherwise others can't tell what happened. > > 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. There is a difference between a talk and pushing patches, and more importantly, to get the community to review and ACK the patches. > > 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. The talk happened at Dec 7 & 8, the soft freeze happened Jan 3rd, so there was a chunk of time to work on that. I don't see why one thing justifies the other. > > 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. Can you back up that with numbers? Forgive me if I lost something, but I have no idea on how much it helps to justify the changes and extra complexity. Best Regards, fbl > 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 -- fbl _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
