Closed Bug 996862 Opened 5 years ago Closed 5 years ago

devicemanager getLanIp relies on a list of expected network interface names

Categories

(Testing :: Mozbase, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: gbrown, Assigned: wlach)

References

Details

Attachments

(3 files, 1 obsolete file)

bjacob had trouble running remote reftests today. He was getting this error:

    bjacob:/hack/mozilla-central$ MOZ_HOST_BIN=/hack/mozilla-central/obj-firefox-debug/dist/bin/ TEST_PATH=layout/reftests/bugs/reftest.list make -C obj-mobile-debug/ reftest-remote
    make: Entering directory `/hack/mozilla-central/obj-mobile-debug'
    ERROR: Either you specified the loopback for the remote webserver or  your local IP cannot be detected.  Please provide the local ip in --remote-webserver
    ERROR: Invalid options specified, use --help for a list of valid options
    /bin/sh: line 11: @errors=: command not found
    make: Leaving directory `/hack/mozilla-central/obj-mobile-debug' 

which seems to be caused by getLanIp failing because none of bjacob's network interface names matches the list at:

http://hg.mozilla.org/mozilla-central/annotate/1f932e462b84/testing/mozbase/mozdevice/mozdevice/devicemanager.py#l601

if (ip is None or ip.startswith("127.")) and os.name != "nt":
 interfaces = ["eth0","eth1","eth2","wlan0","wlan1","wifi0","ath0","ath1","ppp0"] 


There must be a better way to do this.
Wait, don't we have code to do this already?
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/moznetwork/moznetwork/moznetwork.py

I'm guessing devicemanager was written before moznetwork and was never updated to use the new library.
(In reply to Andrew Halberstadt [:ahal] from comment #2)
> Wait, don't we have code to do this already?
> http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/moznetwork/
> moznetwork/moznetwork.py
> 
> I'm guessing devicemanager was written before moznetwork and was never
> updated to use the new library.

Yes, correct. We should probably turn getLanIp() into a compatibility shim around moznetwork.
(In reply to William Lachance (:wlach) from comment #3)
> (In reply to Andrew Halberstadt [:ahal] from comment #2)
> > Wait, don't we have code to do this already?
> > http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/moznetwork/
> > moznetwork/moznetwork.py
> > 
> > I'm guessing devicemanager was written before moznetwork and was never
> > updated to use the new library.
> 
> Yes, correct. We should probably turn getLanIp() into a compatibility shim
> around moznetwork.

And I guess in this case, just replace calls to getLanIp() (and any abstractions around it) with calls to moznetwork.
I'm going to take this one. I think the right way forward is to kill any usage of getLanIp(). As far as I know there are two users: the in-tree code (subject of this bug) and talos... I thought sut_tools might as well, but apparently not: https://hg.mozilla.org/build/tools/file/c290db3dd537/sut_tools/

(1) Fix in-tree code to use moznetwork
(2) Fix talos to use moznetwork
(3) Remove this functionality from mozdevice
Assignee: nobody → wlachance
Attachment #8408472 - Flags: review?(ahalberstadt)
Comment on attachment 8408472 [details] [diff] [review]
Remove getLanIp usage, replace with moznetwork

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

Is something still using NetworkTools? Any reason not to remove that as well?

::: build/mobile/b2gautomation.py
@@ -129,5 @@
>  
>          return app, args
>  
> -    def getLanIp(self):
> -        nettools = NetworkTools()

You can get rid of the import too.

::: build/mobile/remoteautomation.py
@@ -175,5 @@
>  #        return app, ['--environ:NO_EM_RESTART=1'] + args
>          return app, args
>  
> -    def getLanIp(self):
> -        nettools = NetworkTools()

Same.
Attachment #8408472 - Flags: review?(ahalberstadt) → review+
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> Is something still using NetworkTools? Any reason not to remove that as well?

Just talos AFAIK (just wrote up a patch for that). I was planning to do up a seperate patch to remove from mozdevice.
Comment on attachment 8408490 [details] [diff] [review]
Remove use of mozdevice's networktools

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

r- for the need of error handling.

::: create_talos_zip.py
@@ +44,4 @@
>                     ('mozdevice/mozdevice/droid.py', 'mozdevice/droid.py')]
>  mozdevice = [(mozdevice_src + src, destination) for src, destination in mozdevice_files]
>  
> +moznetwork_src = 'https://raw.github.com/mozilla/mozbase/moznetwork-0.24/'

does this really live on github?  I thought we were in tree

::: setup.py
@@ +18,4 @@
>                  'mozhttpd == 0.5',
>                  'mozinfo == 0.7',
>                  'datazilla == 1.4',
> +                'moznetwork == 0.24',

does this live on pypi.mozilla.org (or whatever that server is for internal python packages)

::: talos/PerfConfigurator.py
@@ +436,5 @@
>          if self.config.get('develop'):
>              if not self.config.get('webserver'):
> +                s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> +                s.bind(("127.0.0.1",0))
> +                self.config['webserver'] = 'localhost:%s' % s.getsockname()[1]

what if there is an error here?  can we try/except and terminate if we fail?
Attachment #8408490 - Flags: review?(jmaher) → review-
Comment on attachment 8408490 [details] [diff] [review]
Remove use of mozdevice's networktools

I was beaten into submission on IRC and now have a r+.
Attachment #8408490 - Flags: review- → review+
Removing now-unnecessary imports per review suggestion, carrying forward r+.
Attachment #8408472 - Attachment is obsolete: true
Attachment #8409677 - Flags: review+
Try run looks good (https://tbpl.mozilla.org/?tree=Try&rev=65744b48160c), please checkin patch 8409677 when there is a chance. 

https://bug996862.bugzilla.mozilla.org/attachment.cgi?id=8409677

(leave this open for followup work)
Attachment #8408490 - Attachment is obsolete: true
Comment on attachment 8408490 [details] [diff] [review]
Remove use of mozdevice's networktools

This one isn't obsolete, though it shouldn't be checked into m-c.
Attachment #8408490 - Attachment is obsolete: false
And here's a patch to remove NetworkTools from mozdevice altogether. I don't think anything uses it anymore. In the unlikely event that there is, pretty trivial to update it to use moznetwork.
Attachment #8409793 - Flags: review?(ahalberstadt)
Comment on attachment 8409793 [details] [diff] [review]
Remove NetworkTools from mozdevice

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

\o/ awesome, thanks for doing this!
Attachment #8409793 - Flags: review?(ahalberstadt) → review+
Ok, let's land attachment 8409793 [details] [diff] [review] as well! I didn't do a try run, but I did run the mozbase unit tests and all seems to be good. Also a quick mxrs shows that we're no longer importing/using NetworkTools anywhere aside from inside mozdevice itself (which this patch addresses):

http://mxr.mozilla.org/mozilla-central/search?string=NetworkTools
http://mxr.mozilla.org/mozilla-central/search?string=getLanIp
https://hg.mozilla.org/mozilla-central/rev/b65cb4bc310f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.