Open Stack

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 04/17/2015 12:56 AM, Carl Baldwin wrote:
> 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
> 

Hi Carl,
thanks for analysis. I've sent a regression unit test for master. Once
it's merged in master, I'll propose for stable till Juno.

[1]: https://review.openstack.org/175445

/Ihar
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJVNReAAAoJEC5aWaUY1u573MgIAK03+RqvujHgtL9SmyjXBDtn
K2awpRWCHM4cmb6Hr6MLVbWNPEC2svjOHWOSRfRZ5vw05xk/ViJ/UM17b7Jz9Drf
cTMVJ6G7lzt+XaCQQeB72NbZSEdEfFAcLLNnb9DzVxRjRcvLcu0dV/tpgJ7YeuMn
vGwAUJkv+5af6iTy0BmDpJZh2hZ9OFXnesLxCcHY4VKcCX7HiXwhSEjhF1pCOdPd
SwRC3MgqL5gcLchoYzcNZ2DtTwK881LixLIh4VhY14VTKzwF1zVJBbar2V5N77RG
9cukeTSsPg+oft/M1JclWUQrQ4G0JWZgE8B5VibpYnF45rYYWDlvAVWwa1pv6uQ=
=KgGO
-----END PGP SIGNATURE-----