On 1/13/22 11:53, 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.
Me, who never seen these reviews and architecture discussions has exactly zero confidence in these patch-set the same as the rest of the community. Development behind closed doors is never taken into account while making decisions in open-source projects, because this development and made decisions are not visible and can not be peer-reviewed by anyone outside. On a quick glance over the code and reviews done in a last couple of days, I'd also question the quality of these public reviews as the patch set at least contains a few issues in a copy-pasted code that was already fixed on master branch (left alone that these code duplications should, likley, not exist in a first place). > > >> 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 They were presented, but not discussed or reviewed. > > 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. Again, they were presented, not discussed. It's the same as sending patches to the mailing list. We don't have a policy to accept everything that wasn't explicitly rejected. Lack of feedback / responses in most cases means lack of interest. > > >> 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. I don't see anything being accelerated. I'm not asking your customers to prove anything, I'm asking you and your team to provide a reason to review the patches. "Workloads are accelerated" is a blunt statement without any numbers. And even that you're saying for a very first time. For what I know it could be 0.00001% improvement that simply doesn't worth it. I don't see any claims about performance regression testing being made either. > > If you have concerns over a patchset, these should be raised against a V1 of > a patchset, I don't have time to look at every patch-set that doesn't even try to make me or anyone else interested in reviewing it and you're re-spinning the series without any reviews being made, so that is simply not practical. > 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. Again, there is no policy to accept everything that wasn't explicitly rejected. You missed the review deadline for a soft freeze. So, these patches was not considered for integration at all at this point. Hence questioning them is justified. > > >> 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. Didn't see any progress from your side on that front. And still waiting for your reply on this email: https://mail.openvswitch.org/pipermail/ovs-dev/2021-December/389945.html > > Autovalidation is not complex, it validates all unit-tests, and has been > presented at OVS 'all unit tests' is a plain lie here. You're perfectly aware that this is not true at least for miniflow_extract part. > 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. AFAICT, you're not planning to address concerns in general, because the issue with miniflow_extract autovalidation not being usable for a big chunk of existing tests was flagged several times including the email mentioned above, and you're always avoiding to reply to these concerns. > Not acceptable to block Actions > or Hashing patchsets based on this, in the week of the integration deadline. Continuing to re-use and build new things on top of broken abstractions doesn't look good to me. > > >> 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
