> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Friday, January 14, 2022 8:17 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]>; Mcnamara, John > <[email protected]> > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/14/22 18:11, Van Haaren, Harry wrote: > >> -----Original Message----- > >> From: Ilya Maximets <[email protected]> > >> Sent: Thursday, January 13, 2022 1:14 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]>; Mcnamara, John > >> <[email protected]> > >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >> > >> 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. > > > > 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.
This email is to indicate that I have read the reply and although I don't agree with some of the statements above, it seems time to move on and discuss technical concerns to allows merging code upstream into OVS. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
