Open Stack

Stephen,

On Thu, Apr 16, 2015 at 4:21 PM, Ma, Stephen B. <stephen.ma at hp.com> wrote:
> As it stands currently, neutron on the stable/juno branch behaves as if
> https://review.openstack.org/#/c/164329/ was never merged. The merge of
> patch  https://review.openstack.org/#/c/153181 puts some LOG.debug
> statements in the for-loop of the same function.  The net result is the
> unintentional revert of patch https://review.openstack.org/#/c/164329/.

I wouldn't call this a revert.  Instead, it brought back some context
that the original patch dealt with but that the cherry pick did not.
This is a regression but not a revert of the patch.

> According to review.openstack.org, the patch
> https://review.openstack.org/#/c/153181 merged on April 1st at 1:56 pm.
> Patch https://review.openstack.org/#/c/164329/ merged on the same day at
> 2:21 pm.   After 153181 merged, the review of 164329 should have triggered a
> merge conflict error which would have compelled the owner to do a rebase.
> The merge conflict wasn’t reported and the patches merged anyway.  Does this
> point to a problem with the Jenkins CI system?

This was not a problem with the CI system.  The infrastructure behaved
exactly as it should.

The problem happened when [4] was proposed with a lesser scope than
[2] and then [3] merged making that extra scope relevant again.  I
understand why the scope was reduced.  It is often necessary to react
to changes in context when cherry picking.  That is the nature of
cherry picking, bringing a patch in to a new context and reacting.

It was unfortunate that [3] brought back the relevant context which
made the broader scope of the change in [2] necessary and then they
merged together.

After having given it some thought, I have come to the conclusion that
the only way to catch this would have been to include a proper test
with [2] and [4] that log debug statements are not executed in the
loop.  Such a test would have kicked out whichever of [3] and [4]
happened to merge last from the gate.  It would've been really
annoying to fail in the gate but it would have flagged the problem
then and there.  Really, the failure was in not requiring the tests
necessary to prevent regression.  Until we add these tests, we will be
vulnerable to this regression again.  For example, I could merge a new
patch to add more logging to this loop today and we'd be back to
square one.

Carl

[1] https://review.openstack.org/#/c/147455
[2] https://review.openstack.org/#/c/149784
[3] https://review.openstack.org/#/c/153181
[4] https://review.openstack.org/#/c/164329