On 1/14/22 18:11, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maxim...@ovn.org> >> Sent: Thursday, January 13, 2022 1:14 PM >> To: Van Haaren, Harry <harry.van.haa...@intel.com>; Finn, Emma >> <emma.f...@intel.com>; d...@openvswitch.org; Amber, Kumar >> <kumar.am...@intel.com> >> Cc: i.maxim...@ovn.org; Stokes, Ian <ian.sto...@intel.com>; Flavio Leitner >> <f...@sysclose.org>; Kevin Traynor <ktray...@redhat.com>; Mcnamara, John >> <john.mcnam...@intel.com> >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations >> >> On 1/13/22 11:53, Van Haaren, Harry wrote: >>>> -----Original Message----- >>>> From: Ilya Maximets <i.maxim...@ovn.org> >>>> Sent: Wednesday, January 12, 2022 6:01 PM >>>> To: Van Haaren, Harry <harry.van.haa...@intel.com>; Finn, Emma >>>> <emma.f...@intel.com>; d...@openvswitch.org; Amber, Kumar >>>> <kumar.am...@intel.com> >>>> Cc: i.maxim...@ovn.org; Stokes, Ian <ian.sto...@intel.com>; Flavio Leitner >>>> <f...@sysclose.org>; Kevin Traynor <ktray...@redhat.com> >>>> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations >>>> >>>> On 1/6/22 14:11, Van Haaren, Harry wrote: >>>>>> -----Original Message----- >>>>>> From: Finn, Emma <emma.f...@intel.com> >>>>>> Sent: Wednesday, January 5, 2022 4:54 PM >>>>>> To: d...@openvswitch.org; Van Haaren, Harry >> <harry.van.haa...@intel.com>; >>>>>> Amber, Kumar <kumar.am...@intel.com> >>>>>> Cc: Finn, Emma <emma.f...@intel.com> >>>>>> 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. > > Please avoid confronting language such as "is a plain lie" and suggesting that > I'm purposely trying to mislead, that is not my intent. > > I expect all OVS community members to be respectful, and especially the OVS > Maintainers. > The above comments have no place in an open-source community such as OVS. > > If there is a misunderstanding around the above statement, ask for more > explanation, and we > can discuss it in a professional and polite manner.
On my side, I would say that phrases like 'is unacceptable' are a confronting language and I'm simply responding to them accordingly. I'm not normally using such words, and my initial reply on Jan 12, IMO, was polite and professional, and even suggested a way out from a soft freeze situation. I don't know why you're trying to turn every conversation into a battle. If instead of demands, requests and accusations we will ask and negotiate, everyone will be much happier. > > >>> 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 > > > This patchset has received significant push-back, both on this thread and in > Flavio's reply here; > https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390732.html > > I feel that there needs to be a clearer path to success in getting > contributions merged upstream in OVS. > In particular, concerns need to be raised earlier and not in the last week > before merge deadline. Everything is simple. Participate in the community, review patches, so others will have time looking at yours. And make your patches interesting to review; performance optimizations without any numbers doesn't make a lot of sense. No-one should be obliged to do what is not interesting to them, and most of the work done by the community (including me) is voluntary, especially reviews. And we still have a gap between the rate of posted and reviewed patches, so some of them will inevitably be left without attention. And you, the member of community, is the only person who can fix that by contributing your time. > > On closing this thread for 2.17, please note we will continue to upstream > these Actions optimizations for > OVS 2.18, and request constructive and actionable feedback in order to > achieve any rework required for merging. In this context, I'm also considering 'we request' as a confronting language. And that immediately turns me away from trying to help. So, if being respectful is your standard, try to live up to it. On the subject, most of the top level comments was already made, so you're free to start working on them. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
