1101183 - Kill the Mixins!

Status: RESOLVED INCOMPLETE

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Armen [:armenzg]]

Where applicable.
Parental guidance advised.

Comment 1

[Armen [:armenzg]]

Let's start with mozharness/base/script.py
Let's move all download functionality into mozharness/lib/python

Please look at this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=1087664#c1

Comment 2

[Armen [:armenzg]]

If you have candidates to be murdered please nominate them as I can get contributors to give us a hand.

Comment 3

[Simarpreet Singh]

We had a discussion on IRC and as a result this etherpad came into existence: [1]

[1]: https://etherpad.mozilla.org/mozharness-mixins

Comment 4

[Simarpreet Singh]

Attachment: [PATCH]: buildbot: Initial refactor of buildbot mixin.

Just an initial rework of imagining buildbot as a python module residing in mozharness/lib/python.

Let's tackle buildbot for now and we can try different things out to make it a reference implementation for future mixins to come.

Feel free to also point any pep8, style or code refactoring ideas/comments you may have. I noticed that the buildbot code as a mixin had some python < 2.6 things; in hgtool port this took a good two patches or so to fix.

Comment 5

[Armen [:armenzg]]

> Feel free to also point any pep8, style or code refactoring ideas/comments
> you may have. I noticed that the buildbot code as a mixin had some python <
> 2.6 things; in hgtool port this took a good two patches or so to fix.

I lack expertise in this area. My current basic tools are pep8 and pyflakes. I don't know how to customize to help error out on Python 2.6 specific issues.

I use these two plugins for vim:
http://www.vim.org/scripts/script.php?script_id=3160
http://www.vim.org/scripts/script.php?script_id=3735

With the pep8 one, I press f6 to show me any errors in the file.

Comment 6

[Armen [:armenzg]]

Comment on attachment 8562824 [details] [diff] [review]
[PATCH]: buildbot: Initial refactor of buildbot mixin.

Review of attachment 8562824 [details] [diff] [review]:
-----------------------------------------------------------------

What are the plans coming ahead?
Are we going to start switching over to normal python logging?

::: mozharness/lib/python/buildbot.py
@@ +125,5 @@
> +            contents += "%s:%s\n" % (prop, self.buildbot_properties.get(prop, "None"))
> +        return self.write_to_file(file_name, contents)
> +
> +    def sendchange(self, downloadables=None, branch=None,
> +                   username="sendchange-unittest", sendchange_props=None):

For me, being able to have a function that simply returns the "branch" value given a buildername not being completely integrated within this function would be highly useful for bug 1129594.

This is a scope creep request so feel free to ignore it.

Comment 7

[Jordan Lund (:jlund)]

Comment on attachment 8562824 [details] [diff] [review]
[PATCH]: buildbot: Initial refactor of buildbot mixin.

Review of attachment 8562824 [details] [diff] [review]:
-----------------------------------------------------------------

buildbot.py seems like a good mixin to port first.

though, taking a step back, I think figuring out how we should change logging seems like it be a larger chunk of work to do upfront but will make the transition a lot easier.

I just skimmed through: http://victorlin.me/posts/2012/08/26/good-logging-practice-in-python and I think we won't 'lose' anything by switching to built in python logging but what we gain is 1) a simple python idiom approach and 2) an easier transition to modules from mixins.

What do you think? Should we go for this first? Do you want to meet this week Simar? I am going to touch base with Florian today about gsoc

::: mozharness/lib/python/buildbot.py
@@ +124,5 @@
> +        for prop in prop_list:
> +            contents += "%s:%s\n" % (prop, self.buildbot_properties.get(prop, "None"))
> +        return self.write_to_file(file_name, contents)
> +
> +    def sendchange(self, downloadables=None, branch=None,

I am partly to blame for this method being messy. I added sendchange_props to support some custom stuff from buildbase.py and I didn't want to spend time changing sendchange() calls from other scripts.

It be nice to simplify this method

Comment 8

[Justin Wood (:Callek)]

(In reply to Jordan Lund (:jlund) from comment #7)
> Comment on attachment 8562824 [details] [diff] [review]
> [PATCH]: buildbot: Initial refactor of buildbot mixin.
> 
> Review of attachment 8562824 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> buildbot.py seems like a good mixin to port first.
> 
> though, taking a step back, I think figuring out how we should change
> logging seems like it be a larger chunk of work to do upfront but will make
> the transition a lot easier.
> 
> I just skimmed through:
> http://victorlin.me/posts/2012/08/26/good-logging-practice-in-python and I
> think we won't 'lose' anything by switching to built in python logging but
> what we gain is 1) a simple python idiom approach and 2) an easier
> transition to modules from mixins.

Except mozharness does one thing with logs that doesn't exist (afaict) in default python logging, line prefix for *each* line of a traceback.

E.g. without using the mozharness logging code, we'll get something like:

$ python
Python 2.7.5 (default, May 15 2013, 22:43:36) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import logging
>>> logging.basicConfig(level=logging.INFO)
>>> logger = logging.getLogger(__name__)
>>> try:
...    open("/does/not/exist", 'rb')
... except (SystemExit, KeyboardInterrupt):
...     raise
... except Exception, e:
...     logger.error('Failed to open file', exc_info=True)
...
ERROR:__main__:Failed to open file
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
IOError: [Errno 2] No such file or directory: '/does/not/exist'

What we want (at present) is the Traceback and related lines to be behind ERROR:__main__: in that log.

Comment 9

[Jordan Lund (:jlund)]

(In reply to Justin Wood (:Callek) from comment #8)
> (In reply to Jordan Lund (:jlund) from comment #7)
> > Comment on attachment 8562824 [details] [diff] [review]
> > [PATCH]: buildbot: Initial refactor of buildbot mixin.
> > 
> > Review of attachment 8562824 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > buildbot.py seems like a good mixin to port first.
> > 
> > though, taking a step back, I think figuring out how we should change
> > logging seems like it be a larger chunk of work to do upfront but will make
> > the transition a lot easier.
> > 
> > I just skimmed through:
> > http://victorlin.me/posts/2012/08/26/good-logging-practice-in-python and I
> > think we won't 'lose' anything by switching to built in python logging but
> > what we gain is 1) a simple python idiom approach and 2) an easier
> > transition to modules from mixins.
> 
> Except mozharness does one thing with logs that doesn't exist (afaict) in
> default python logging, line prefix for *each* line of a traceback.
> 
> E.g. without using the mozharness logging code, we'll get something like:
> 
> $ python
> Python 2.7.5 (default, May 15 2013, 22:43:36) [MSC v.1500 32 bit (Intel)] on
> win32
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import logging
> >>> logging.basicConfig(level=logging.INFO)
> >>> logger = logging.getLogger(__name__)
> >>> try:
> ...    open("/does/not/exist", 'rb')
> ... except (SystemExit, KeyboardInterrupt):
> ...     raise
> ... except Exception, e:
> ...     logger.error('Failed to open file', exc_info=True)
> ...
> ERROR:__main__:Failed to open file
> Traceback (most recent call last):
>   File "<stdin>", line 2, in <module>
> IOError: [Errno 2] No such file or directory: '/does/not/exist'
> 
> What we want (at present) is the Traceback and related lines to be behind
> ERROR:__main__: in that log.

true. the built in seems to be limited. I guess we could monkey patch traceback.print_exception http://stackoverflow.com/questions/1508467/how-to-log-my-traceback-error

If we can find an easy way to modularize parts of mozharness without hacky passing around the live logger: self.log_obj, I'm happy. It is worth investigating other loggers too.

maybe we could make the current mozharness log implementation a global that lives outside of self. so we, instead, `from mozharness.base import log' and if log has been instantiated, we can log.warning(msg) from anywhere. iow - make mozharness log a module itself.

Comment 10

[Jordan Lund (:jlund)]

fwiw, I think massimo might have done the work at looking into BaseLogger and realizing that we can keep existing mozharness logging as is and instead of passing self.log_obj, we can just use python's getLogger(): https://bugzilla.mozilla.org/show_bug.cgi?id=1130336#c5

Comment 11

[Simarpreet Singh]

Attachment: [PATCH]: Bug 1101183: Adding log.py with new buildbot.py

Added a multifilelogger implementation based on massimo's patch. Also added a new log.py module that can be used in the future as a standalone logger module.

Still a WIP, I'll improve upon what I have here. Once we can get logging modularized, it'd be relatively painless to use it in other modules.

I also plan to look into log rotation for log.py, that could be a really useful feature considering stuff like buildbot.py today has hard caps on how large the log size can be [1]

Next up are: 1) pep8 fixes 2) possible log-rotation feature.

[1]: http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/buildbot.py#74

Comment 12

[Jordan Lund (:jlund)]

Comment on attachment 8569905 [details] [diff] [review]
[PATCH]: Bug 1101183: Adding log.py with new buildbot.py

Review of attachment 8569905 [details] [diff] [review]:
-----------------------------------------------------------------

this looks like we are on the right path! :D keep it up

::: mozharness/lib/python/log.py
@@ +259,5 @@
> +                         (name, datetime.now().strftime("%Y%m%d %H:%M:%S"),
> +                         os.getcwd()))
> +
> +    def get_logger_size(self):
> +        log_file = os.path.join(self.abs_log_dir, self.logger_name)

I think logger_name's value is going to be like 'Multi'.

MultiLoggerFile saves many log files split by level. What buildbot uses for build and test jobs, iiuc, is log_info.log ("%s_%s.log % (self.log_name, level). self.log_name is defined here[1] but other mozharness jobs overwrite that name in their configs. But since those other mh jobs are used outside of buildbot test and build jobs, we don't have to worry about that.

what we probably need here is something like the following pseudo untested code:

# in BaseLogger
def get_logger_size(self):
    return os.path.getsize(os.path.join(self.abs_log_dir, '%s.log' % self.log_name))

# in MultiFileLogger
def get_logger_size(self, level='info'):  # override base method from BaseLogger
    return os.path.getsize(os.path.join(self.abs_log_dir, '%s_%s.log' % (self.log_name, level)))



[1] http://mxr.mozilla.org/build/source/mozharness/mozharness/base/script.py#1357