508560 - make it easier to reload buildbotcustom modules

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect, P5)

Description

[[e-mail] (:bhearsum)]

We currently reload() buildbotcustom modules wherever we use them. This has led to problems where we reload a module twice at every buildbot reconfig, which in turn causes problems where running builds can't call their parent class' methods due to having stale references.

Catlee and I were chatting about this today and thought up two possible ways to fix this:
1) Have a master list of buildbotcustom modules, and have them reload on every reconfig. I was thinking of a 'reload.py' file which would be import/reload'ed by our masters, which in turn would dig through sys.modules to reload all of buildbotcustom.
2) Enable buildbotcustom to be able to reload itself. We could have a function in buildbotcustom that is called from the master that reloads all of buildbotcustom through whatever means. (Chris, did I understand this correctly?)

Doing either one of these would allow the rest of the buildbot configs and buildbotcustom to simply 'from buildbotcustom.foo import blah'.

Comment 1

[Chris Cooper [:coop] (he/him)]

Would this also allow us to break up the single monolithic master.cfg into smaller files, similar to how mobile and release are broken out now? IIRC multiple imports nixed this when I tried it before.

Comment 2

[[e-mail] (:bhearsum)]

It should give us more freedom in that department - it will enable anything that was hindered by import/reload problems.

Comment 3

[Chris AtLee [:catlee]]

(In reply to comment #0)
> We currently reload() buildbotcustom modules wherever we use them. This has led
> to problems where we reload a module twice at every buildbot reconfig, which in
> turn causes problems where running builds can't call their parent class'
> methods due to having stale references.
> 
> Catlee and I were chatting about this today and thought up two possible ways to
> fix this:
> 1) Have a master list of buildbotcustom modules, and have them reload on every
> reconfig. I was thinking of a 'reload.py' file which would be import/reload'ed
> by our masters, which in turn would dig through sys.modules to reload all of
> buildbotcustom.
> 2) Enable buildbotcustom to be able to reload itself. We could have a function
> in buildbotcustom that is called from the master that reloads all of
> buildbotcustom through whatever means. (Chris, did I understand this
> correctly?)
> 
> Doing either one of these would allow the rest of the buildbot configs and
> buildbotcustom to simply 'from buildbotcustom.foo import blah'.

Yeah, the two ways I was thinking of doing the reload were:

1) Look through sys.modules, and reload any buildbotcustom module (and probably config.py, release_config.py, etc.)
2) Have a list of modules somewhere (reload.py?) that get reloaded.

I like 1) because it will only reload modules that have been used.

I think we should take out any reload calls from buildbotcustom, and have it all managed by master.cfg or a helper function.  Especially since we need to have config.py, release_config.py, etc. reloaded as well.

Comment 4

[[e-mail] (:bhearsum)]

(In reply to comment #3)
> I think we should take out any reload calls from buildbotcustom, and have it
> all managed by master.cfg or a helper function.  Especially since we need to
> have config.py, release_config.py, etc. reloaded as well.

sounds good to me

Comment 5

[Chris AtLee [:catlee]]

A function like this in buildbotcustom should work:

def reload_modules(extra_mods=None):
    extra_mods = extra_mods or []
    for name, mod in sys.modules.items():
        # Sometimes mod is None, not sure why
        # but it means we can't reload it.
        if mod is None:
            continue
        if name.startswith("buildbotcustom.") or name in extra_mods:
            print "Reloading", name
            reload(mod)


Then near the top of master.cfg, we can do

from buildbotcustom.reloader import reload_modules
reload_modules(['config', 'release_config', 'mobile_config', ...])

Comment 6

[Chris AtLee [:catlee]]

Attachment: Reloading patch for buildbotcustom, take 1

This has been working well for me in staging over the past few weeks.  I'd like some extra eyes on this code though, since I have a few concerns still:

- Do we need to protect ourselves against circular module dependencies?

- I'm not sure how well this handles module dependencies that change over time.  e.g. if A imports B, then after a reload B imports A, what happens?

Comment 7

[Chris AtLee [:catlee]]

Attachment: patch for mozilla2-staging

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 401007 [details] [diff] [review]
Reloading patch for buildbotcustom, take 1

One overall thing: I wonder if this could be written a little more generically to make it portable, should we ever need it for something else. I think that if the name.startswith() stuff either: a) detected the name of the module automatically, or b) was set as a class level var, that would get it to this state. Might have to remove the Twisted dependencies (log.msg) too. What do you think, Chris?

I'll probably throw another comment in here once I think about what's going on more deeply.

Comment 9

[Chris AtLee [:catlee]]

This might not be as much as a problem if we do build-scripts, and I'm not convinced that the method above is robust in the face of many reloads where module dependencies change.

Comment 10

[Chris Cooper [:coop] (he/him)]

Moving closed Future bugs into Release Engineering in preparation for removing the Future component.