1176060 - Autophone - recover from wifi issues

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

we can turn on wifi via svc wifi enable or turn it off via svc wifi disable.

sometimes wifi will be on but disabled. if we look at the /data/misc/wpa_supplicant.conf the network has disabled in its specification.

An example (other devices are different):

ctrl_interface=eth0
update_config=1

network={
	ssid="xxx"
	psk="xxxx"
	key_mgmt=WPA-PSK
	priority=3
        disabled=1
}

Removing the disabled and restarting the network/rebooting will re-enable wifi.

If we have wpa_cli, we can use that. nexus eng/userdebug builds have it, but not user builds. We might be able to get it via busybox.

# wpa_cli interface
Using interface 'eth0'
Available interfaces:
eth0

# wpa_cli disable_network eth0
Using interface 'eth0'
OK

# wpa_cli enable_network eth0
Using interface 'eth0'
OK

We should be able to deal with either case. In fact, it would be good if we could specify the ssid, psdk, key_mgmt values and have autophone set the network configuration in the wpa_supplicant.conf itself.

Without wpa_cli, we probably need to deal with pulling/parsing/pushing wpa_supplicant.conf.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: Bug-1176060-v1.patch

Without command line support and with the variety of different wpa_supplicant.confs I decided the best approach is to save a good version of the wpa_supplicant.conf in /data/local/tmp and to copy that to /data/misc/wifi and to toggle wifi in the event the wifi is disabled.

This works for me locally even if I disable wifi and delete the /data/misc/wifi/wpa_supplicant.conf file.

chown is needed since the conf won't be read if the ownership is wrong. The chown wifi.wifi/chown wifi:wifi is due to the variety in support for the chown command.

Preferably we should add chown to adb_android or perhaps adb, but I'll leave that to a different bug.

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 8650740 [details] [diff] [review]
Bug-1176060-v1.patch

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

Where will /data/local/tmp/wpa_supplicant.conf come from? Will you copy it manually when setting up a phone?

Charitable r+ -- note my concerns.

::: worker.py
@@ +426,5 @@
> +                                self.dm.shell_output(
> +                                    'chown wifi.wifi /data/misc/wifi/wpa_supplicant.conf',
> +                                    root=True)
> +                            except ADBError, e:
> +                                self.loggerdeco.debug('chown wifi.wifi: %s')

Missing parameter here for %s!

@@ +428,5 @@
> +                                    root=True)
> +                            except ADBError, e:
> +                                self.loggerdeco.debug('chown wifi.wifi: %s')
> +                                self.dm.shell_output(
> +                                    'chown wifi:wifi /data/misc/wifi/wpa_supplicant.conf',

You are retrying the command, to see if it happens to work the 2nd time? Maybe add a comment to clarify.

If it throws again, will that be handled ok?

@@ +431,5 @@
> +                                self.dm.shell_output(
> +                                    'chown wifi:wifi /data/misc/wifi/wpa_supplicant.conf',
> +                                    root=True)
> +                            self.dm.shell_output(
> +                                'svc wifi disable; svc wifi enable',

I would be tempted to disable, then copy, then enable, but that would likely make error handling more difficult.

@@ +432,5 @@
> +                                    'chown wifi:wifi /data/misc/wifi/wpa_supplicant.conf',
> +                                    root=True)
> +                            self.dm.shell_output(
> +                                'svc wifi disable; svc wifi enable',
> +                                root=True)

If all of this works, do you still want phone_status == PhoneStatus.ERROR?

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #2)
> Comment on attachment 8650740 [details] [diff] [review]
> Bug-1176060-v1.patch
> 
> Review of attachment 8650740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Where will /data/local/tmp/wpa_supplicant.conf come from? Will you copy it
> manually when setting up a phone?
> 

Yes.

> Charitable r+ -- note my concerns.
> 
> ::: worker.py
> @@ +426,5 @@
> > +                                self.dm.shell_output(
> > +                                    'chown wifi.wifi /data/misc/wifi/wpa_supplicant.conf',
> > +                                    root=True)
> > +                            except ADBError, e:
> > +                                self.loggerdeco.debug('chown wifi.wifi: %s')
> 
> Missing parameter here for %s!
> 

doh!

> @@ +428,5 @@
> > +                                    root=True)
> > +                            except ADBError, e:
> > +                                self.loggerdeco.debug('chown wifi.wifi: %s')
> > +                                self.dm.shell_output(
> > +                                    'chown wifi:wifi /data/misc/wifi/wpa_supplicant.conf',
> 
> You are retrying the command, to see if it happens to work the 2nd time?
> Maybe add a comment to clarify.
> 

Android has the "nice feature" that it changes the syntax of chown from using user.group to user:group over time. This works around it. The better approach would be to place chown in adb*.py and detect the proper syntax on initialization the way we do with ls. I'm getting ready to file a bug adding this to mozdevice, but wanted to get this in first. I'll comment more about the reasoning.

> If it throws again, will that be handled ok?
> 

Depends on your definition of ok I guess. The first exception is expected on newer Android where chown user.group is not supported and this second call is intended to simply handle the case of changing syntax.

It will be treated as an uncaught exception which will cause the worker to die and for us to get an email notification. This is also the case if any of the other adb calls raise an exception.

On second thought, I can follow your recommendation on order; catch and handle all exceptions; not rely on the death of the worker to be notified that the network recovery failed; and send an email if things don't work out. Thanks for making me think harder about this.

> @@ +431,5 @@
> > +                                self.dm.shell_output(
> > +                                    'chown wifi:wifi /data/misc/wifi/wpa_supplicant.conf',
> > +                                    root=True)
> > +                            self.dm.shell_output(
> > +                                'svc wifi disable; svc wifi enable',
> 
> I would be tempted to disable, then copy, then enable, but that would likely
> make error handling more difficult.
> 

See above. I can do that and it shouldn't be a deal breaker.

> @@ +432,5 @@
> > +                                    'chown wifi:wifi /data/misc/wifi/wpa_supplicant.conf',
> > +                                    root=True)
> > +                            self.dm.shell_output(
> > +                                'svc wifi disable; svc wifi enable',
> > +                                root=True)
> 
> If all of this works, do you still want phone_status == PhoneStatus.ERROR?

I think so. ping is called in multiple places and it is possible the network disappeared during a test. I'm thinking that I want the same retry behavior but just with autophone network recovery so I don't have to do it manually any more.

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #3)
> Android has the "nice feature" that it changes the syntax of chown from
> using user.group to user:group over time. This works around it. The better
> approach would be to place chown in adb*.py and detect the proper syntax on
> initialization the way we do with ls. I'm getting ready to file a bug adding
> this to mozdevice, but wanted to get this in first. I'll comment more about
> the reasoning.

I just didn't notice user.group vs user:group (and forgot about this Androidism). It makes sense now. A short comment would be helpful.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: Bug-1176060-v2.patch

Carrying forward gbrown's review. This cleans up the patch a bit and makes it clear why the try..except blocks are used. I'm not catching unrelated exceptions from adb calls since the overall design of autophone depends on unexpected exceptions causing us to fail the test.

In fact, though the usage message for older chown includes user.group and the usage message for newer chown includes user:group, it appears that while older chown do not support user:group, newer chown do support user.group so the try..except might not actually be necessary but sometimes belt+suspenders is better than just belt or suspenders. I'll clean this whole issue up when I do this for adb.py.

Note I tested this several times (while including a patch that disables flash in order to work around the crash from bug 1196115) and you can see the results at:

https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&exclusion_profile=false&filter-searchStr=autophone&fromchange=581329d6602b&tochange=4c14cdd926f9

Comment 6

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/95f5b44625a339bd80ddfbf732a3adc59052bc31