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

[Kamen Mazdrashki]

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.

Cheers,
Kamen


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