[ovs-dev] ideas for improving the OVS review process

[Flavio Leitner]

Hi,

On Tue, Jul 20, 2021 at 11:41:37AM -0700, Ben Pfaff wrote:
> The OVS review process has greatly slowed over the last few years.  This
> is partly because I haven't been able to spend as much time on review,
> since I was once the most productive reviewer.  Ilya has been able to
> step up the amount of review he does, but that still isn't enough to
> keep up with the workload.
> 
> We need to come up with some way to improve things.  Here are a few
> ideas, mostly from a call earlier today (that was mainly about the OVS
> conference).  I hope they will prompt a discussion.
> 
> * Since patches are coming in, we have people who are knowledgable about
>   the code.  Those people should be pitching in with reviews as well.
>   It doesn't seem like they or their managers have the right incentives
>   to do that.  Maybe there is some way to improve the incentives.

There is that and also that even with others review, a maintainer
still does a careful review which puts off reviewers' work.
"The perfect is the enemy of the good."

We have several emeritus maintainers but only a couple of them active
at the moment. It doesn't seem fair to ask them to do more work.  So,
I agree with the other reply to this thread saying that we need more
maintainers.

Although I think we could grow the number of maintainers, it doesn't
seem reasonable to ask each one of them to do an extensive review
by themselves on every patch.  My point is that there should be a
"chain of trust", or a rule that allows a patch to be "blindly
committed" if that patch is reviewed by N different members, for example.
The intention is not to forbid maintainers to review, but to alleviate
the load or pressure on them when the community already helped.

As a side effect, more people could become maintainers/committers
because we don't require them to know OVS deeply or to commit huge
amounts of time to careful review others work.

This brings up another point: Unpredictability. It is not possible
today to tell what is going to happen with a patch after it gets
posted to mailing list. It can be silent ignored for many months,
or be reviewed by others and still not accepted, etc.  We should
have a way to prevent that otherwise it makes very difficult to
align companies or other upstream projects schedules. 

For example, if a company is investing on a feature "X" most probably
has a deliver date. Even if the work is posted during development
phase, that doesn't mean the patch will be reviewed or accepted or
rejected. It's kind of chicken and egg issue. If the process is not
clear, why managers should provide more incentives to participate?


> * The Linux kernel uses something like a "default accept" policy for
>   patches that superficially look good and compile and boot, if there is
>   no review from a specific maintainer within a few days.  The patches
>   do get some small amount of review from a subsystem maintainer.  OVS
>   could adopt a similar policy.

I agree. Aaron is working to improve patchwork's status report for
each patch. Hopefully each community member could report test status
there.  It seems that if the community decides to go with the "default
accept" rule, there will be more incentive to connect tests to OVS
patchwork. We get more automation as a side effect.

However, we would still need to define who is the sub-maintainer
or perhaps think on the "chain of trust"/"blindy committed" idea
mentioned above.


> * Some lack of review can be attributed to a reluctance to accept a
>   review from a reviewer who is at the same company as the patch
>   submitter.  There is good reason for this, but it is certainly
>   possible to get quality reviews from a coworker, and perhaps we should
>   relax the convention.

I also agree with that. I think OVS had that going on years ago and
worked well. The same happens with OVN.  It should be OK during
devel phase when we can revert if something goes wrong. We may strict
if it's close to release date though, if some are uncomfortable with
this idea.


> * A flip side of the above could be to codify the requirement for review
>   from a non-coworker.  This would have the benefit of being able to
>   push back against requests to commit unreviewed long series on the
>   basis that it hasn't been reviewed by a third party.

Good idea. That goes in the same direction of having rules to help
maintainers to accept patches without require them to do an
extensive review.

Also,  should we change 0-day bot to send reminders if a patch is
sitting without a review for a certain period of time?

Should we have rules like for example hardware offload - having ACK
from another hardware offload company would be enough to get the
patch accepted?


Thanks,
-- 
fbl