628465 - survive a failed reconfig

Status: RESOLVED INCOMPLETE

(Release Engineering :: General, defect, P3)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

As in bug 628451, masters do not recover well from a failed reconfig.  We've hit this once before, but I can't find the bug now.  We traced it down to a problem of class hierarchies changing.  The problem is, given

from somemodule import SomeParentClass

class MyClass(SomeParentClass):
    def __init__(self):
        SomeParentClass.__init__(self)

class_to_instantiate_later = MyClass

before a reconfig, everything works fine.  But if you hang onto a copy of MyClass in class_to_instantiate_later, and reconfigure, then you get a fresh new copy of both SomeParentClass and MyClass, with the same name - I'll call them SomeParentClass' and MyClass'.  The problem is, you're now instantiating the equivalent of

from sommodule import SomeParentClass'

class MyClass(SomeParentClass')
    def __init__(self):
        SomeParentClass.__init__(self)

and SomeParentClass is not the same as SomeParentClass'.

This looks like an attempt to solve this problem

    self.super_class = ShellCommand
    if not log_eval_func:
        log_eval_func = lambda c,s: regex_log_evaluator(c, s, hg_errors)
    self.super_class.__init__(self, log_eval_func=log_eval_func, **kwargs)

but other than being odd, I don't see how it could do much.

I'm not sure *how* to solve this problem, in general - use super()?  I see a lot of old-style uses of addStep, but I think even new-style uses of addStep would fail here.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

From my experimentation with Buildbot itself, this problem only occurs when your class and its immediate parent are both reloaded.

morestuff.py:
from buildbot.steps.shell import ShellCommand, PerlModuleTest
class AShellCommand(ShellCommand):
    def __init__(self, *args, **kwargs):
        ShellCommand.__init__(self, *args, **kwargs)

stuff.py
import morestuff
reload(morestuff)
class MyShellCommand(morestuff.AShellCommand):
    def __init__(self, *args, **kwargs):
        morestuff.AShellCommand.__init__(self, *args, **kwargs)

master_cfg:
f1.addStep(stuff.MyShellCommand(command="echo hi", description='echoing', descriptionDone='echoed', usePTY=True))

I think the only way to fix this is to use super()?

Comment 2

[Rail Aliiev [:rail]]

> I think the only way to fix this is to use super()?

IIRC, the following method works even for the old style classes:

self.super_class = ShellCommand
self.super_class.__init__(self, **kwargs)

ShellComand is a child of BuildStep, which is an old style class, so super won't work for it.

See example:
http://hg.mozilla.org/build/buildbotcustom/file/48bc4db17575/steps/release.py#l11

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

rail, I saw that technique used in a few places, but I don't see how it could help - it's no different than
  ShellCommand.__init__(self, **kwargs)
except that it sets self.super_class as a side-effect - although that attribute is never read from.

You're right about old-style classes, though.  We may just have to deal with unsuccessful reconfigs killing masters.

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #3)
> You're right about old-style classes, though.  We may just have to deal with
> unsuccessful reconfigs killing masters.

Could we run checkconfig in its own process before allowing a reconfig run in the master's process?  I know that this would mean that the checkconfig code is executed twice, but at least it would avoid this situation in future.

Comment 5

[Chris AtLee [:catlee]]

(In reply to comment #4)
> (In reply to comment #3)
> > You're right about old-style classes, though.  We may just have to deal with
> > unsuccessful reconfigs killing masters.
> 
> Could we run checkconfig in its own process before allowing a reconfig run in
> the master's process?  I know that this would mean that the checkconfig code is
> executed twice, but at least it would avoid this situation in future.

checkconfig doesn't catch this error, I don't think

Comment 6

[Chris AtLee [:catlee]]

(In reply to comment #3)
> rail, I saw that technique used in a few places, but I don't see how it could
> help - it's no different than
>   ShellCommand.__init__(self, **kwargs)
> except that it sets self.super_class as a side-effect - although that attribute
> is never read from.

There are several examples of classes that do refer to self.super_class in other methods.

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Checkconfig would be likely to catch the underlying error that causes the reconfig to fail.  It wouldn't catch the errors consequent to the failed reconfig - those aren't configuration errors.

(In reply to comment #6)
> There are several examples of classes that do refer to self.super_class in
> other methods.

It's in the constructor that the error occurs, though.  I think (unverified) that the construct could be made to work if it stored its superclass as a class instance:

class MySubSubClass(othermodule.MySubClass):
    super_class = othermodule.MySubClass
    def __init__(self, **kwargs):
        self.super_class.__init__(self, **kwargs)

This has the advantage of making both references to MySubClass occur at the same time (the exception occurs in existing code because the first reference is evaluated when the first class object is created, while the second is evaluated when an object of that first class is instantiated, and since that is after a reconfig, MySubClass refers to a new and different class object at that point).