The responsibility for flapping tests (was: Re: [PATCHES] avoid searching for wellknownobjects when not needed, cache results for speed)

[Andrew Bartlett]

On Thu, 2015-05-28 at 01:21 +0300, Kamen Mazdrashki wrote:
> On Wed, May 27, 2015 at 5:56 AM, Andrew Bartlett <abartlet at samba.org> wrote:
> 
> > On Wed, 2015-05-27 at 05:43 +0300, Kamen Mazdrashki wrote:
> > >
> > >
> > > On Tue, May 26, 2015 at 4:53 PM, Andrew Bartlett <abartlet at samba.org>
> > > wrote:
> > >         On Tue, 2015-05-26 at 05:46 -0700, Matthieu Patou wrote:
> > >         > On 05/25/2015 09:54 PM, Andrew Bartlett wrote:
> > >         > > On Mon, 2015-05-25 at 21:50 -0700, Matthieu Patou wrote:
> > >         > >> Hello All,
> > >         > >>
> > >         > >> Please find attached 2 patches that reduce the time of
> > >         dbcheck by 10%,
> > >         > >> they avoid calling getwellknowobjects when not needed
> > >         (ie. when dealing
> > >         > >> with the schema NC) and also cache the result of the
> > >         function as well
> > >         > >> known object didn't change.
> > >         > >>
> > >         > >> Pass make test.
> > >         > > As we got into the unfortunate situation where dbcheck was
> > >         marked as
> > >         > > flapping, sadly this means this code is untested in 'make
> > >         test'.  :-(
> > >         > >
> > >         > > Andrew Bartlett
> > >         > >
> > >         > Ok but still it doesn't mean that those changes are wrong.
> > >         > One of those is to cache the wellknownobjects when found; as
> > >         the name
> > >         > imply the wellknownobjects doesn't change. Also I think I
> > >         did several
> > >         > rounds of make tests without getting an error so if let's
> > >         say 3 rounds
> > >         > of make test didn't show any regression we can treat this as
> > >         a reliable
> > >         > patch no ?
> > >
> > >         No, because tests in the flapping file are essentially skipped
> > >         - we have
> > >         no idea if they succeeded or failed, or if they continue to.
> > >
> > >         Before we get this in, we have to revert that unfortunate
> > >         addition, and
> > >         to do that we probably need to revert whatever made it flap
> > >         (presumably
> > >         the tombstone reanimation work).
> > >
> > >         I realise this is totally unfair.  Untested code is really
> > >         bad, but
> > >         changing untested code is even worse.
> > >
> > >         The only way forward for this in the long term would be to
> > >         declare that
> > >         we will remove, after a timeout of two months, each entry from
> > >         flapping,
> > >         and that as part of this we will review those items to decide
> > >         that they
> > >         are either pointless (and so remove the pretence that they
> > >         have value),
> > >         or fix them not to flap.  That, and better pre-commit tests
> > >         that make
> > >         flapping tests harder to get in, could be a better compromise
> > >         to release
> > >         the pressure of failed autobuilds in the short-term without
> > >         abandoning
> > >         tests for the long term.
> > >
> > >         That is, the current process where tests get added to flapping
> > >         'one
> > >         way', just leaves the costs of the underlying failure on our
> > >         users and
> > >         never on us as developers.
> > >
> > >         Like portability, it will only change if work is invested.
> > >
> > >         When I'm over jet-lag, I will propose to remove tombstone
> > >         reanimation as
> > >         a supported feature (by removing the module from the default
> > >         stack), and
> > >         knownfail or skip the associated tests.
> > >
> > > I am afraid that just removing the module won't remove all changes
> > > introduced
> > > for tombstone reanimation :). Funny thing is, as Matthieu mentioned he
> > > did for his
> > > optimization work, that I have run the test suite for tombstone
> > > reanimation for so
> > > many times without a failure. Which leads me to believe that there is
> > > a subtle
> > > interference between tests/environments. afaik, we don't run every
> > > test in
> > > a cleanly provisioned environment, do we?
> >
> > Correct, the tests run in a common environment, and the interaction
> > between the various DCs that are running at the time can impact on the
> > results.  I would suggest that for tests like this one, that we should
> > actually force that, by having it run against promoted_dc or vampire_dc,
> > as well as a standalone server such as fl2008dc so that the replication
> > behaviours and standalone behaviours are captured.
> 
> 
> This would be great - after all, tombstone reanimation has nothing to do
> with replication test and we have different set of tests to show deleted
> objects are replicated correctly.
> How can I achieve that? Should I move Tombstone reanimation test
> earlier in tests.py?
> 
> 
> 
> 
> > But regarding this test and the functionality it was intended to prove,
> > what would you suggest?  The cleanest solution would be to revert the
> > lot until we understand it, but that also seems rather drastic.
> >
> > It seems reasonable to at least remove the module and test, even if that
> > won't be a full revert, and then put it back when the full make test is
> > shown not to flap.
> >
> 
> You are totally right. I thought you want to revert the whole patch set,
> sorry!
> Disabling just this module is *very* reasonable approach indeed.

I've found myself a bit busy, can you knock up the patch?

Thanks,

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba