The default bug view has changed. See this FAQ.

mozharness device mixin has various problems

RESOLVED INVALID

Status

Release Engineering
Mozharness
RESOLVED INVALID
5 years ago
2 years ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozharness])

Attachments

(1 attachment)

(1) It depends on functionality that does not exist in the most recent version of devicemanagerSUT (sendCMD).
(2) It tries to collect and display the entire system logcat on a device, which can be absolutely huge.
(3) When installing the application, it has a 90 second timeout which is extremely time consuming and unnecessary.
Created attachment 648480 [details] [diff] [review]
Patch to fix various problems in mozharness device mixin
Attachment #648480 - Flags: review?(aki)

Comment 2

5 years ago
Comment on attachment 648480 [details] [diff] [review]
Patch to fix various problems in mozharness device mixin

I'm not opposed to removing the logcat, which I'm not convinced is helping us debug anything on the Tegras currently.

I did find that an install wouldn't necessarily come back when the device is ready; is that still the case?  If so, we may need a sleep or wait for device of some sort.

As for the sendCMD change, we're currently using mozdevice 0.2, which doesn't have runCmds support.  We can't make this change until that's updated.

Updated

5 years ago
Whiteboard: [mozharness]

Comment 3

5 years ago
Comment on attachment 648480 [details] [diff] [review]
Patch to fix various problems in mozharness device mixin

Clearing the review flag until the above is addressed.
Attachment #648480 - Flags: review?(aki)
(In reply to Aki Sasaki [:aki] from comment #2)
> Comment on attachment 648480 [details] [diff] [review]
> Patch to fix various problems in mozharness device mixin
> 
> I'm not opposed to removing the logcat, which I'm not convinced is helping
> us debug anything on the Tegras currently.

Cool; if we were going to display logcat output, my vote would be to limit it to 500 lines or something.
 
> I did find that an install wouldn't necessarily come back when the device is
> ready; is that still the case?  If so, we may need a sleep or wait for
> device of some sort.

Looking at the source to the agent, I don't see any reason why the install would come back before the device was ready. At a low level, it just uses the "pm install" command which shouldn't return until an install is complete.

So I'm not sure exactly what you're seeing on the tegras. It might be good to investigate further to see what the issue is.

> As for the sendCMD change, we're currently using mozdevice 0.2, which
> doesn't have runCmds support.  We can't make this change until that's
> updated.

Hmm, mozdevice is currently at 0.3 and this seems to be what was installed my mozharness when I ran the talos device script?

Comment 5

5 years ago
(In reply to William Lachance (:wlach) from comment #4)
> Looking at the source to the agent, I don't see any reason why the install
> would come back before the device was ready. At a low level, it just uses
> the "pm install" command which shouldn't return until an install is complete.

"Shouldn't" and "doesn't" are two different things, sadly.
I don't have definitive proof that these are returning before the device is ready, only anecdotal memories of devices bailing out after install, when trying to do something else with the device, that stopped happening after adding a sleep.

Perhaps the thing to do here is to install a Fennec apk and immediately try to do things to the device after the call returns -- read and write to the sdcard, access the installed app, etc, both with updates and fresh installs.  If we can't find anything, the sleep goes.  If we find something, we'll have better info as to what the issue is and how to wait for it to finish instead of blindly sleeping 90.

> So I'm not sure exactly what you're seeing on the tegras. It might be good
> to investigate further to see what the issue is.

Yeah.

In the meantime, is the sleep causing you issues?
We could set it via pref until we have more data.

> > As for the sendCMD change, we're currently using mozdevice 0.2, which
> > doesn't have runCmds support.  We can't make this change until that's
> > updated.
> 
> Hmm, mozdevice is currently at 0.3 and this seems to be what was installed
> my mozharness when I ran the talos device script?

Ok, mozdevice is probably 0.3 on pypi.  The build+test farm doesn't touch peptest, since there's going to be a network block that prevents it from reaching the internet.  We have a separate set of python packages, and the one(s) we use have 0.2.

We should be able to put 0.3 up there.  I'm kind of bummed that we keep on changing the APIs in mozbase in non-backwards-compatible ways; it would be a lot easier if we deprecated the old methods, added new ones, and then gave downstream apps time to switch to the new methods before removing the old ones.

Comment 6

5 years ago
s,doesn't touch peptest,doesn't touch pypi,
(In reply to Aki Sasaki [:aki] from comment #5)
> (In reply to William Lachance (:wlach) from comment #4)
> > Looking at the source to the agent, I don't see any reason why the install
> > would come back before the device was ready. At a low level, it just uses
> > the "pm install" command which shouldn't return until an install is complete.
> 
> "Shouldn't" and "doesn't" are two different things, sadly.
> I don't have definitive proof that these are returning before the device is
> ready, only anecdotal memories of devices bailing out after install, when
> trying to do something else with the device, that stopped happening after
> adding a sleep.
> 
> Perhaps the thing to do here is to install a Fennec apk and immediately try
> to do things to the device after the call returns -- read and write to the
> sdcard, access the installed app, etc, both with updates and fresh installs.
> If we can't find anything, the sleep goes.  If we find something, we'll have
> better info as to what the issue is and how to wait for it to finish instead
> of blindly sleeping 90.

It should be super easy to write this kind of test and run it. I'll do one next week.

> > So I'm not sure exactly what you're seeing on the tegras. It might be good
> > to investigate further to see what the issue is.
> 
> Yeah.
> 
> In the meantime, is the sleep causing you issues?
> We could set it via pref until we have more data.

It's only causing me issues in the sense that it's slowing down my development. It turns out that I'm not going to be using mozharness for the task I was thinking of (at least not right away), so we have time to apply a proper fix here. :)

> > > As for the sendCMD change, we're currently using mozdevice 0.2, which
> > > doesn't have runCmds support.  We can't make this change until that's
> > > updated.
> > 
> > Hmm, mozdevice is currently at 0.3 and this seems to be what was installed
> > my mozharness when I ran the talos device script?
> 
> Ok, mozdevice is probably 0.3 on pypi.  The build+test farm doesn't touch
> peptest, since there's going to be a network block that prevents it from
> reaching the internet.  We have a separate set of python packages, and the
> one(s) we use have 0.2.
> 
> We should be able to put 0.3 up there.  I'm kind of bummed that we keep on
> changing the APIs in mozbase in non-backwards-compatible ways; it would be a
> lot easier if we deprecated the old methods, added new ones, and then gave
> downstream apps time to switch to the new methods before removing the old
> ones.

I agree with this 100% for public APIs. My understanding wen I removed it was that sendCMD wasn't meant to be part of devicemanager's "public" API (it's a SUT-only thing, after all), though it looks like you guys were effectively using it as such in some of your tools (I'm sorry, I didn't know). We actually now have a better alternative called "shell" which better handles the use case of what you were using sendCMD for (executing a command). [1]

Maybe the best alternative for now would be add back sendCMD as a deprecated feature and then work towards replacing it in the future?

[1] Actually that's not quite true. shell() is a bit more complicated than it should be because it currently requires you to pass it a file-like object to buffer it's output. This is useful sometimes, but not often. I'm thinking of adding a shell_check_output for shorter commands which eliminates the need for this step.

Comment 8

5 years ago
(In reply to William Lachance (:wlach) from comment #7)
> It should be super easy to write this kind of test and run it. I'll do one
> next week.

That's awesome.
 
> > In the meantime, is the sleep causing you issues?
> > We could set it via pref until we have more data.
> 
> It's only causing me issues in the sense that it's slowing down my
> development. It turns out that I'm not going to be using mozharness for the
> task I was thinking of (at least not right away), so we have time to apply a
> proper fix here. :)

Sounds good, and again we can pref around this if you return to development.

> > it would be a
> > lot easier if we deprecated the old methods, added new ones, and then gave
> > downstream apps time to switch to the new methods before removing the old
> > ones.
> 
> I agree with this 100% for public APIs. My understanding wen I removed it
> was that sendCMD wasn't meant to be part of devicemanager's "public" API
> (it's a SUT-only thing, after all), though it looks like you guys were
> effectively using it as such in some of your tools (I'm sorry, I didn't
> know). We actually now have a better alternative called "shell" which better
> handles the use case of what you were using sendCMD for (executing a
> command). [1]

Cool, no harm done.
From your patch, it looks like there's a roughly-equivalent-but-renamed method as well as additional methods in devicemanagerSUT; we can switch over when mozdevice is updated on the various internal servers.

The current url in use is build.m.o/talos/findlinks/, though I think Dustin set up a repos/python/packages/ that we should switch over to when we have those fully populated.

> Maybe the best alternative for now would be add back sendCMD as a deprecated
> feature and then work towards replacing it in the future?

That would definitely make the move to the new methods a lot smoother and less time-sensitive.  We could upload that version (0.4?) everywhere without breaking things, then switch over without rushing.

> [1] Actually that's not quite true. shell() is a bit more complicated than
> it should be because it currently requires you to pass it a file-like object
> to buffer it's output. This is useful sometimes, but not often. I'm thinking
> of adding a shell_check_output for shorter commands which eliminates the
> need for this step.

That could be useful; otherwise we may end up adding a similar wrapper method around shell() that might do the same thing.
Product: mozilla.org → Release Engineering

Updated

3 years ago
Component: Other → Mozharness
This almost certainly isn't valid anymore given the amount that mozharness has changed since I filed this issue.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.