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

Kamen Mazdrashki kamenim at samba.org
Thu May 28 05:40:48 MDT 2015


On Thu, May 28, 2015 at 6:10 AM, Andrew Bartlett <abartlet at samba.org> wrote:

> 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?
>
I will out of town, getting back tomorrow and i will post a patch.
Hopefully I can play
with reordering the tests to be executed in a 'cleaner' environment (I
count on your
help heavily there thought :)

Cheers,
Kamen


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


More information about the samba-technical mailing list