Closed
Bug 834850
Opened 11 years ago
Closed 11 years ago
Use the mozpoolclient package from mozharness
Categories
(Release Engineering :: Applications: MozharnessCore, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: armenzg, Assigned: armenzg)
References
Details
Attachments
(5 files, 3 obsolete files)
1.38 KB,
patch
|
Details | Diff | Splinter Review | |
1.34 KB,
patch
|
Details | Diff | Splinter Review | |
8.59 KB,
patch
|
mozilla
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
40.86 KB,
patch
|
mozilla
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
8.50 KB,
patch
|
mozilla
:
review+
armenzg
:
checked-in+
|
Details | Diff | 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.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → armenzg
Priority: -- → P2
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
Comment 4•11 years ago
|
||
Comment 5•11 years ago
|
||
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-
Assignee | ||
Comment 6•11 years ago
|
||
(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+ ;)
Assignee | ||
Comment 7•11 years ago
|
||
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?
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
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.
Comment 11•11 years ago
|
||
(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): ...
Comment 12•11 years ago
|
||
(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.
Assignee | ||
Comment 13•11 years ago
|
||
I also took advantage to fix some mess up I had on README.txt.
Attachment #708715 -
Flags: review?(aki)
Comment 14•11 years ago
|
||
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+
Comment 15•11 years ago
|
||
(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,
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #707301 -
Attachment is obsolete: true
Attachment #707819 -
Attachment is obsolete: true
Attachment #708752 -
Flags: review?(aki)
Assignee | ||
Comment 17•11 years ago
|
||
Comment on attachment 708715 [details] [diff] [review] allow to pass logging object http://hg.mozilla.org/build/mozpool/rev/e19fed913bd8 Published: http://pypi.python.org/pypi/mozpoolclient/0.1.2 Deployed: http://puppetagain.pub.build.mozilla.org/data/python/packages/mozpoolclient-0.1.2.tar.gz
Attachment #708715 -
Flags: checked-in+
Comment 18•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
Comment on attachment 708752 [details] [diff] [review] mozharness refactor - take 3 http://hg.mozilla.org/build/mozharness/rev/2fd949805785
Attachment #708752 -
Flags: checked-in+
Assignee | ||
Comment 21•11 years ago
|
||
(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 22•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
Comment on attachment 709042 [details] [diff] [review] s,dump_exception,exception http://hg.mozilla.org/build/mozharness/rev/bcb1c5ef4917
Attachment #709042 -
Flags: checked-in+
Assignee | ||
Updated•11 years ago
|
Whiteboard: waiting on mozharness production merge
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Updated•10 years ago
|
Component: General Automation → Mozharness
You need to log in
before you can comment on or make changes to this bug.
Description
•