Closed Bug 834850 Opened 11 years ago Closed 11 years ago

Use the mozpoolclient package from mozharness

Categories

(Release Engineering :: Applications: MozharnessCore, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: armenzg, Assigned: armenzg)

References

Details

Attachments

(5 files, 3 obsolete files)

Attached patch mozharness refactor (obsolete) — Splinter Review
We are moving the logic from mozpool.py (in mozharness) to the mozpoolclient python package (which we can retrieve from http://puppetagain.pub.build.mozilla.org/data/python/packages).

This makes it easier to be used by multiple tools and even other projects.

I already have a wip for this.
Blocks: 829211
Assignee: nobody → armenzg
Priority: -- → P2
This patch contains a bunch of things that aki had a looked in https://bugzilla.mozilla.org/show_bug.cgi?id=831820#c10

I should take those comments into the patch in here.
Depends on: 827545
Attached patch mozharness refactor (obsolete) — Splinter Review
This works.

The only problem I have is that I get lines like this (repeated with "Multi"):
13:54:18     INFO - 01-16 07:39:07.851 E/GeckoConsole( 1251): Content JS LOG at chrome://marionette/content/marionette-listener.js:1869 in GaiaApps.kill/gt_onAppTerminated/<: app with origin 'app://uitest.gaiamobile.org' has terminated
INFO:Multi:01-16 07:39:07.851 E/GeckoConsole( 1251): Content JS LOG at chrome://marionette/content/marionette-listener.js:1869 in GaiaApps.kill/gt_onAppTerminated/<: app with origin 'app://uitest.gaiamobile.org' has terminated

How do I eliminate such problem?
Attachment #706544 - Attachment is obsolete: true
Attachment #707301 - Flags: review?(aki)
Attached patch cache_dmSplinter Review
Comment on attachment 707301 [details] [diff] [review]
mozharness refactor

Hey Armen,
Let me know if the below makes sense, and if you need to meet to talk about it.


From unit.sh pyflakes:

mozharness/mozilla/testing/mozpool.py:13: 'time' imported but unused
mozharness/mozilla/testing/mozpool.py:18: 'json' imported but unused
mozharness/mozilla/testing/mozpool.py:21: 'TBPL_WARNING' imported but unused
mozharness/mozilla/testing/mozpool.py:21: 'BuildbotMixin' imported but unused
mozharness/mozilla/testing/mozpool.py:21: 'TBPL_SUCCESS' imported but unused
mozharness/mozilla/testing/mozpool.py:21: 'TBPL_FAILURE' imported but unused
mozharness/mozilla/testing/mozpool.py:40: undefined name 'MozpoolException'
mozharness/mozilla/testing/mozpool.py:54: 'MozpoolException' imported but unused
mozharness/mozilla/testing/mozpool.py:54: 'MozpoolConflictException' imported but unused
mozharness/mozilla/testing/mozpool.py:71: undefined name 'MozpoolConflictException'
mozharness/mozilla/testing/mozpool.py:73: undefined name 'MozpoolException'
scripts/b2g_panda.py:10: 'socket' imported but unused
scripts/b2g_panda.py:17: 'TBPL_RETRY' imported but unused
scripts/b2g_panda.py:124: local variable 'mph' is assigned to but never used

could you clean the above up?

Pylint gives us these:

mozharness/mozilla/testing/mozpool.py:40: [E, MozpoolMixin.determine_mozpool_host] Undefined variable 'MozpoolException'
mozharness/mozilla/testing/mozpool.py:71: [E, MozpoolMixin.retrieve_b2g_device] Undefined variable 'MozpoolConflictException'
mozharness/mozilla/testing/mozpool.py:73: [E, MozpoolMixin.retrieve_b2g_device] Undefined variable 'MozpoolException'

These we probably can't fix, so I tend to write a grep -v here:
http://hg.mozilla.org/build/mozharness/file/7bcf253585ab/unit.sh#l62

Maybe an | egrep -v 'MozpoolMixin.*Undefined variable .Mozpool.*Exception' or something.


> The only problem I have is that I get lines like this (repeated with "Multi"):
> 13:54:18     INFO - 01-16 07:39:07.851 E/GeckoConsole( 1251): Content JS LOG at chrome://marionette/content/marionette-listener.js:1869 in GaiaApps.kill/gt_onAppTerminated/<: app with origin 'app://uitest.gaiamobile.org' has terminated
> INFO:Multi:01-16 07:39:07.851 E/GeckoConsole( 1251): Content JS LOG at chrome://marionette/content/marionette-listener.js:1869 in GaiaApps.kill/gt_onAppTerminated/<: app with origin 'app://uitest.gaiamobile.org' has terminated
>
> How do I eliminate such problem?

The problem is mozharness is logging, and mozpoolclient is logging.
One advantage of mozharness is unified logging; we lose that when we call out to an external package.

We can get it back by calling the external package as a script; then mozharness can get the output from subprocess and send to its unified logging.
If you call it as a package, like you are here, and both the external package and mozharness are doing their own logging, you're breaking that unified logging.
You're either going to get duplicate lines, or you're going to miss all the external package's logging in the mozharness logs.

You can get mozpool to log through mozharness' logger by doing the following:

* get mozpool to follow the LogMixin api: http://hg.mozilla.org/build/mozharness/file/7bcf253585ab/mozharness/base/log.py#l29
** this means look for self.log_obj, and call self.log_obj.log_message(), which looks like this: http://hg.mozilla.org/build/mozharness/file/7bcf253585ab/mozharness/base/log.py#l29
* Then when you create the mozpoolclient, set its log_obj.  E.g.:

    self.mozpool_handler = MozpoolHandler(self.mozpool_api_url)
    self.mozpool_handler.log_obj = self.log_obj

And if mozpool_handler is calling self.log_obj.log_message() properly, mozpool will be logging through mozharness' logging.

(If the various objects aren't tied together, and each does their own logging, you may have to repeat this process.  There's a reason mozharness logging is written once, and everything uses the same logging object; to avoid all of this.)


>diff --git a/mozharness/base/python.py b/mozharness/base/python.py
>--- a/mozharness/base/python.py
>+++ b/mozharness/base/python.py
>@@ -50,22 +50,23 @@ class VirtualenvMixin(object):
>     Requires virtualenv to be in PATH.
>     Depends on OSMixin
>     '''
>     python_paths = {}
>     site_packages_path = None
> 
>     def query_virtualenv_path(self):
>         c = self.config
>+        virtualenv_path = c['virtualenv_path'] if c.has_key("virtualenv_path") else "venv"

virtualenv_path = c.get('virtualenv_path', 'venv')
?

>         dirs = self.query_abs_dirs()
>         if 'abs_virtualenv_dir' in dirs:
>             return dirs['abs_virtualenv_dir']
>-        if os.path.isabs(c['virtualenv_path']):
>+        if os.path.isabs(virtualenv_path):
>             return c['virtualenv_path']
>-        return os.path.join(dirs['abs_work_dir'], c['virtualenv_path'])
>+        return os.path.join(dirs['abs_work_dir'], virtualenv_path)

I think this is the best version of this you've posted.
Could you add a docstring of something like
"""Return the absolute path of the virtualenv.
This defaults to abs_work_dir/venv"""
or something?

>+# SUTDeviceMozdeviceMixin {{{1
>+class SUTDeviceMozdeviceMixin(SUTDeviceHandler):
>+    '''
>+    This SUT device manager class makes calls through mozdevice (from mozbase) [1]
>+    directly rather than calling SUT tools.
>+
>+    [1] https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/devicemanagerSUT.py
>+    '''
>+    def query_devicemanager(self):
>+        sys.path.append(self.query_python_site_packages_path())
>+        from mozdevice.devicemanagerSUT import DeviceManagerSUT
>+        self.info("Connecting to: %s" % self.mozpool_device)
>+        dm = DeviceManagerSUT(self.mozpool_device)
>+        # No need for 300 second SUT socket timeouts here
>+        dm.default_timeout = 30
>+        return dm

Ok, I have the same problem here that I did before (see bug 831820 comment 10).
Every time we query_devicemanager() we'll get a new devicemanager.
In the past we've had unpredictable results when we access the same device with multiple dm's, so whenever possible we should reuse the same dm.

Could you do one of these three things?

a) Save dm as self.dm (or self.devicemanager) a la attachment 707419 [details] [diff] [review] .
   If we do have to get a new dm for some reason, we can set self.dm to None before calling self.query_devicemanager().

b) Save dm as self.dm if cache_dm is True (default) a la attachment 707420 [details] [diff] [review] .
   We can call self.query_devicemanager(cache_dm=False) to get a new dm if needed.
   We should probably only use this approach if we think we'll need multiple dm's on a regular basis.

c) Write up why we need to create a new dm per self.query_devicemanager() call.

I can't grant review without one of these three things.

>+    def query_file(self, filename):
>+        dm = self.query_devicemanager()
>+        if not dm.fileExists(filename):
>+            raise Exception("Expected file (%s) not found" % filename)
>+
>+        file_contents = dm.catFile(filename)
>+        if file_contents is None:
>+            raise Exception("Unable to read file (%s)" % filename)

Are you catching these exceptions? If not these should probably be self.error() or self.fatal().

>+        site_packages_path = self.query_python_site_packages_path()
>+        mph_path = os.path.join(site_packages_path, 'mozpoolclient')
>+        sys.path.append(mph_path)
>+        sys.path.append(site_packages_path)
>+        try:
>+            from mozpoolclient import MozpoolHandler, MozpoolException, MozpoolConflictException
>+            global MozpoolException, MozpoolConflictException
>+        except ImportError, e:
>+            self.fatal("Can't instantiate MozpoolHandler until mozpoolclient python package is installed! (VirtualenvMixin?): \n%s" % str(e))
>+        self.mozpool_handler = MozpoolHandler(self.mozpool_api_url)
>         return self.mozpool_handler

Same thing as query_devicemanager().  You're creating a new mozpool handler every time you call this method.
I think you should see if self.mozpool_handler is set first here; otherwise you'll be going through this import dance (and creating a new mph object) every time you query_mozpool_handler().

>-    ]
>+    ] + virtualenv_config_options

This makes the mozharness.base.python patch unneeded, but it's fine to have both.


I wanted to be lenient on this review since I've r-'ed variations on this patch several times now, but I can't in good conscience do that.
I hope the above comments are clear; if not, let's talk before you refactor everything again.
Attachment #707301 - Flags: review?(aki) → review-
(In reply to Aki Sasaki [:aki] from comment #5)
> >+    def query_file(self, filename):
> >+        dm = self.query_devicemanager()
> >+        if not dm.fileExists(filename):
> >+            raise Exception("Expected file (%s) not found" % filename)
> >+
> >+        file_contents = dm.catFile(filename)
> >+        if file_contents is None:
> >+            raise Exception("Unable to read file (%s)" % filename)
> 
> Are you catching these exceptions? If not these should probably be
> self.error() or self.fatal().
>
I am. I do it with self._retry_job_and_close_request()

> >-    ]
> >+    ] + virtualenv_config_options
> 
> This makes the mozharness.base.python patch unneeded, but it's fine to have
> both.
> 
My eyes got tired and the base.python persisted unnecessarily!

> 
> I wanted to be lenient on this review since I've r-'ed variations on this
> patch several times now, but I can't in good conscience do that.
> I hope the above comments are clear; if not, let's talk before you refactor
> everything again.

Fair reviews. I will persist until I get the r+ ;)
Attached patch mozharness refactor - take 3 (obsolete) — Splinter Review
I'm testing this.
Since I don't have an __init__ I can't set this one time:
self.mozpool_handler.log_obj = self.log_obj
and I have a mix between self.warning/fatal and self.log_message.

I might have to move to a Handler approach rather than a Mixin.

What do you think?
(In reply to Armen Zambrano G. [:armenzg] from comment #7)
> I'm testing this.

Looks like you're addressing some (or all?) of my review points, thank you.

> Since I don't have an __init__ I can't set this one time:
> self.mozpool_handler.log_obj = self.log_obj

You're able to do it in query_mozpool_handler(), which is good.

> and I have a mix between self.warning/fatal and self.log_message.

Ok.  Fatal isn't a standard logging level, so you'll have to special case that if you use it.  Alternately, throw a Mozpool*Exception and have the mozharness code catch it.

> I might have to move to a Handler approach rather than a Mixin.

What do you mean?
If you mean something like the device.py Handlers, possibly; I'm not sure specifically what you're looking to do.

But in that case, we have both a Handler and a DeviceMixin that creates the Handler.

The main reason I split the DeviceMixin into two Handlers is so we could decide whether to use SUT or ADB and use the appropriate Handler there; there's no need to do that here, I don't think ?

I'm not as deep in this code as you are, so I might be missing something.
Priority: P2 → P1
I think that my problem is that I don't call super(MozpoolMixin, self).__init__() since I have noticed that I cannot call self.info() like I should be able to if I'm inheriting from it.

I'm running this:
> self.log_obj.log_message("Got request, url=%s" % self.request_url) [1]
but I see this on the logs:
> 09:05:15     INFO - Got request, url=http://mobile-imaging-001.p1.releng.scl1.mozilla.com/api/request/6727/
> INFO:Multi:Got request, url=http://mobile-imaging-001.p1.releng.scl1.mozilla.com/api/request/6727/

[1] http://hg.mozilla.org/users/armenzg_mozilla.com/mozharness/rev/715367a12923#l3.552

> I might have to move to a Handler approach rather than a Mixin.

Handler's have an __init__ function where they set things by default.
This is not relevant as I can throw an Exception if log_obj has not yet been set.
Priority: P1 → P2
I've got another general question.

Should MozpoolMixin inherit VirtualEnvMixin and ensure it installs "mozpoolclient"?
I ask since that would make mozpool.py completely decoupled from the calling script.
This might be complicated and that is why I avoided asking before. I had already undertaken enough refactoring as is.
(In reply to Armen Zambrano G. [:armenzg] from comment #9)
> I think that my problem is that I don't call super(MozpoolMixin,
> self).__init__() since I have noticed that I cannot call self.info() like I
> should be able to if I'm inheriting from it.

A mixin is meant to not be a standalone object.
It needs to be inherited by an object that also inherits LogMixin, which provides self.info().  This is generally done by inheriting BaseScript and the mixin.

I did have to create an __init__() for LocalesMixin, because I haven't figured out how to override query_abs_dirs() without it.  But that breaks super(...).__init__() in all children and you have to call the parent __init__()s directly, which is a pain.

> I'm running this:
> > self.log_obj.log_message("Got request, url=%s" % self.request_url) [1]
> but I see this on the logs:
> > 09:05:15     INFO - Got request, url=http://mobile-imaging-001.p1.releng.scl1.mozilla.com/api/request/6727/
> > INFO:Multi:Got request, url=http://mobile-imaging-001.p1.releng.scl1.mozilla.com/api/request/6727/
> 
> [1]
> http://hg.mozilla.org/users/armenzg_mozilla.com/mozharness/rev/
> 715367a12923#l3.552

... The self.log_obj.log_message() call should be inside mozpoolclient, not inside the mixin.

You're basically rerouting all mozpoolclient logging into mozharness' logging.
So this is actually the wrong approach.
Are you sure you don't need to talk?

> > I might have to move to a Handler approach rather than a Mixin.
> 
> Handler's have an __init__ function where they set things by default.
> This is not relevant as I can throw an Exception if log_obj has not yet been
> set.

Instead of self.log_obj.log_message(), you can

class MozpoolClientHandler(object):
    log_obj = None
    def __init__(self, **kwargs):
        ...

    ...

    def info(self, message):
        if self.log_obj:
            self.log_obj.log_message(message)
        else:
            # use own logging
    def error(self, message):
        ...
(In reply to Armen Zambrano G. [:armenzg] from comment #10)
> I've got another general question.
> 
> Should MozpoolMixin inherit VirtualEnvMixin and ensure it installs
> "mozpoolclient"?
> I ask since that would make mozpool.py completely decoupled from the calling
> script.
> This might be complicated and that is why I avoided asking before. I had
> already undertaken enough refactoring as is.

In general I would say yes, but getting the patch as-is to a reviewable state seems to be a very long process here, so I would hesitate to add anything more until we get there.
I also took advantage to fix some mess up I had on README.txt.
Attachment #708715 - Flags: review?(aki)
Comment on attachment 708715 [details] [diff] [review]
allow to pass logging object

Hm.  This is good, except we don't have an exception() method; we have a dump_exception() method.

I'm ok with this if we make LogMixin more logging compatible and s,dump_exception,exception, everywhere.
Attachment #708715 - Flags: review?(aki) → review+
(bb08)deathduck:/src/clean/mozharness [12:16:55] (default)
877$ grep -r dump_exception .
./mozharness/base/log.py:    def dump_exception(self, message=None, level=ERROR):
./mozharness/base/script.py:            self.dump_exception("There was an error while copying %s to %s!" % (src, dest),
./mozharness/base/script.py:                     self.dump_exception(), level=level)
./mozharness/base/script.py:                     self.dump_exception(), level=level)
./mozharness/base/signing.py:            self.dump_exception("Error while signing %s (missing %s?):" % (apk, jarsigner))
./mozharness/base/signing.py:            self.dump_exception("Popen called with invalid arguments during signing?")
./mozharness/base/vcs/mercurial.py:                    self.dump_exception(level='debug')
./mozharness/base/vcs/mercurial.py:            self.dump_exception(level='error')
./mozharness/base/vcs/mercurial.py:                self.dump_exception(level='debug')
./scripts/b2g_build.py:            self.dump_exception("failed to run hg log; using timestamp of 0 instead", level=WARNING)
./scripts/b2g_build.py:            self.dump_exception("failed to parse hg log output; using timestamp of 0 instead", level=WARNING)

All of these would need to s,dump_exception,exception,
Attachment #707301 - Attachment is obsolete: true
Attachment #707819 - Attachment is obsolete: true
Attachment #708752 - Flags: review?(aki)
Comment on attachment 708752 [details] [diff] [review]
mozharness refactor - take 3

Nit:
    mozharness/mozilla/testing/mozpool.py:17: 'FATAL' imported but unused
    mozharness/mozilla/testing/mozpool.py:17: 'LogMixin' imported but unused
    mozharness/mozilla/testing/mozpool.py:17: 'WARNING' imported but unused

>+        self.retrieve_b2g_device(
>+            b2gbase = self.installer_url
>+        )

Nit: inside of parens, we shouldn't have spaces around the '='.

    self.retrieve_b2g_device(b2gbase=self.installer_url)

Also, we need to address comment 15 or the MozpoolHandler self.log_obj.exception() call will break.

Also also, we can probably remove mpunit.sh and test/mozpool, and replace them with unit tests for mozpoolclient.

Thanks for working through this!
Attachment #708752 - Flags: review?(aki) → review+
I spent a minute trying to show you that I had already attached it, reviewed and landed it but I guess I used my imagination! :P
Attachment #709042 - Flags: review?(aki)
Comment on attachment 708752 [details] [diff] [review]
mozharness refactor - take 3

http://hg.mozilla.org/build/mozharness/rev/2fd949805785
Attachment #708752 - Flags: checked-in+
(In reply to Aki Sasaki [:aki] from comment #18)
> Also also, we can probably remove mpunit.sh and test/mozpool, and replace
> them with unit tests for mozpoolclient.
> 
I've moved that to bug 837106.
Comment on attachment 709042 [details] [diff] [review]
s,dump_exception,exception

I personally prefer dump_exception() as a name, but I think we win more by being logging compatible. r=me
Attachment #709042 - Flags: review?(aki) → review+
Whiteboard: waiting on mozharness production merge
Merged this earlier.
Whiteboard: waiting on mozharness production merge
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Component: General Automation → Mozharness
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: