Merge into trunk : nexus : Code : LAVA Dispatcher

Status:	Merged
Merged at revision:	543
Proposed branch:	lp:~terceiro/lava-dispatcher/nexus
Merge into:	lp:lava-dispatcher
Diff against target:	262 lines (+242/-0) 3 files modified lava_dispatcher/config.py (+3/-0) lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf (+46/-0) lava_dispatcher/device/nexus.py (+193/-0)
To merge this branch:	bzr merge lp:~terceiro/lava-dispatcher/nexus
Related bugs:	Link a bug report
Related blueprints:	Prototyping of Galaxy Nexus Support (High)

Reviewer	Review Type	Date Requested	Status
Michael Hudson-Doyle (community)		2013-01-22	Approve on 2013-01-23
Review via email: mp+144204@code.launchpad.net

This proposal supersedes a proposal from 2013-01-15.

Description of the change

Hi guys,

I tried to address all your concerns and this is an improved version of the code.

The only issue I did not handle was Andy's point about exception handling in file_system. I didn't see the point in catching an eventual exception there just to raise a new exception. If anything in there blows up, the job will just be terminated.

Please let me know what you think.

Revision history for this message

Andy Doan (doanac) wrote on 2013-01-15: Posted in a previous version of this proposal

#

On 01/15/2013 01:18 PM, Antonio Terceiro wrote:
> + def power_on(self):

Seems like you might want to add some check like:

if not self.deployment_data.get('boot_image', False):
raise CriticalError('Deploy action must be run first')

> + self.reboot_os()
> + self.reboot_bootloader()
> + self.boot_test_image()
> +
> + self._powered_on = True
> + proc = self.adb('shell', spawn = True)
> + proc.sendline("") # required to put the adb shell in a reasonable state
> + proc.sendline("export PS1='%s'" % self.deployment_data['TESTER_PS1'])
> + self._runner = self._get_runner(proc)
> +
> + return proc

> + def power_off(self, proc):
> + # there is no way to power off the Nexus while USB is plugged on; even
> + # if you remove power, it will stay on.
> + pass
> +
This might be correct, but I fear a bad image could put us in an
irrecoverable state. One thing we might consider adding is some type of
logic like I did in sdmux.sh:

<http://bazaar.launchpad.net/~linaro-validation/lava-dispatcher/trunk/view/head:/lava_dispatcher/device/sdmux.sh>

This essentially powers down a USB port and might solve your problem.
However, since you use ADB over USB be aware of this issue:

https://groups.google.com/forum/#!topic/android-platform/hLmD0DDAzj8

Lastly, the filesystem function looks like it might need some exception
handling logic. Not sure about this, but I found the need when
implementing that logic for other targets.

Revision history for this message

Michael Hudson-Doyle (mwhudson) wrote on 2013-01-16: Posted in a previous version of this proposal

#

Download full text (10.8 KiB)

Antonio Terceiro <email address hidden> writes:

> Antonio Terceiro has proposed merging lp:~terceiro/lava-dispatcher/nexus into lp:lava-dispatcher.
>
> Requested reviews:
> Linaro Validation Team (linaro-validation)
>
> For more details, see:
> https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
>
> Hi,
>
> This time is for real. :-)
>
> I don't see any pending issues, so know you can have a hard review on the code.
> --
> https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> You are subscribed to branch lp:lava-dispatcher.
> === modified file 'lava_dispatcher/config.py'
> --- lava_dispatcher/config.py 2013-01-10 21:37:58 +0000
> +++ lava_dispatcher/config.py 2013-01-15 19:17:23 +0000
> @@ -91,6 +91,9 @@
> arm_probe_config = schema.StringOption(default='/usr/local/etc/arm-probe-config')
> arm_probe_channels = schema.ListOption(default=['VDD_VCORE1'])
>
> + adb_command = schema.StringOption()
> + fastboot_command = schema.StringOption()
> + nexus_working_directory = schema.StringOption(default = None)

It's super nitty, but "default=None" (i.e. without the spaces) would be
more conventional.

> class OptionDescriptor(object):
> def __init__(self, name):
>
> === added file 'lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf'
> --- lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf 1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf 2013-01-15 19:17:23 +0000
> @@ -0,0 +1,40 @@
> +client_type = nexus
> +
> +# The ADB command line.
> +#
> +# In the case where there are more than one android devices plugged into a

s/are/is/ or s/more than one/multiple/ (English grammar makes no sense)

> +# single host, this connection command must be overriden on each device to
> +# include the serial number of the device, e.g.
> +#
> +# serial_number = XXXXXXXXXXXXXXXX
> +# adb_command = adb -s %(serial_number)s
> +adb_command = adb
> +
> +# The fastboot command.
> +#
> +# The same as above: if you have more than one device, you will want to
> +# override this in your device config to add a serial number, e.g.
> +#
> +# serial_number = XXXXXXXXXXXXXXXX
> +# fastboot_command = sudo fastboot -s %(serial_number)s
> +#
> +# Of course, in the case you override both adb_command *and* fastboot_command,
> +# you don't need to specify `serial_number` twice.
> +fastboot_command = sudo fastboot
> +
> +# Working directory for temporary. By default, the usual place for LAVA images
> +# will be used.
> +#
> +# This is useful when the lava dispatcher is controlling Nexus phones that are
> +# physically plugged to other machines. In this case, you should set
> +# nexus_work_directory to a shared directory between the machine running the
> +# dispatcher and the machine where the phone is plugged.

I don't entirely understand when this would be useful.

> +# OBS: this shared directory must have the same path in both machines.

OBS?

> +nexus_working_directory =
> +
> +connection_command = %(adb_command)s shell
> +
> +enable_network_after_boot_android = false
> +android_adb_over_usb = true
> +android_adb_...

Antonio Terceiro <antonio.terceiro@linaro.org> writes:

> Antonio Terceiro has proposed merging lp:~terceiro/lava-dispatcher/nexus into lp:lava-dispatcher.
>
> Requested reviews:
>   Linaro Validation Team (linaro-validation)
>
> For more details, see:
> https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
>
> Hi,
>
> This time is for real. :-)
>
> I don't see any pending issues, so know you can have a hard review on the code.
> -- 
> https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> You are subscribed to branch lp:lava-dispatcher.
> === modified file 'lava_dispatcher/config.py'
> --- lava_dispatcher/config.py	2013-01-10 21:37:58 +0000
> +++ lava_dispatcher/config.py	2013-01-15 19:17:23 +0000
> @@ -91,6 +91,9 @@
>      arm_probe_config = schema.StringOption(default='/usr/local/etc/arm-probe-config')
>      arm_probe_channels = schema.ListOption(default=['VDD_VCORE1'])
>  
> +    adb_command = schema.StringOption()
> +    fastboot_command = schema.StringOption()
> +    nexus_working_directory = schema.StringOption(default = None)

It's super nitty, but "default=None" (i.e. without the spaces) would be
more conventional.

>  class OptionDescriptor(object):
>      def __init__(self, name):
>
> === added file 'lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf'
> --- lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	2013-01-15 19:17:23 +0000
> @@ -0,0 +1,40 @@
> +client_type = nexus
> +
> +# The ADB command line.
> +#
> +# In the case where there are more than one android devices plugged into a

s/are/is/ or s/more than one/multiple/ (English grammar makes no sense)

> +# single host, this connection command must be overriden on each device to
> +# include the serial number of the device, e.g.
> +#
> +#   serial_number = XXXXXXXXXXXXXXXX
> +#   adb_command = adb -s %(serial_number)s
> +adb_command = adb
> +
> +# The fastboot command.
> +#
> +# The same as above: if you have more than one device, you will want to
> +# override this in your device config to add a serial number, e.g.
> +#
> +#   serial_number = XXXXXXXXXXXXXXXX
> +#   fastboot_command = sudo fastboot -s %(serial_number)s
> +#
> +# Of course, in the case you override both adb_command *and* fastboot_command,
> +# you don't need to specify `serial_number` twice.
> +fastboot_command = sudo fastboot
> +
> +# Working directory for temporary. By default, the usual place for LAVA images
> +# will be used.
> +#
> +# This is useful when the lava dispatcher is controlling Nexus phones that are
> +# physically plugged to other machines. In this case, you should set
> +# nexus_work_directory to a shared directory between the machine running the
> +# dispatcher and the machine where the phone is plugged.

I don't entirely understand when this would be useful.

> +# OBS: this shared directory must have the same path in both machines.

OBS?

> +nexus_working_directory =
> +
> +connection_command = %(adb_command)s shell
> +
> +enable_network_after_boot_android = false
> +android_adb_over_usb = true
> +android_adb_over_tcp = false
>
> === added file 'lava_dispatcher/device/nexus.py'
> --- lava_dispatcher/device/nexus.py	1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/device/nexus.py	2013-01-15 19:17:23 +0000
> @@ -0,0 +1,168 @@
> +# Copyright (C) 2012 Linaro Limited
> +#
> +# Author: Antonio Terceiro <antonio.terceiro@linaro.org>
> +#
> +# This file is part of LAVA Dispatcher.
> +#
> +# LAVA Dispatcher is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# LAVA Dispatcher is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.    See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along
> +# with this program; if not, see <http://www.gnu.org/licenses>.
> +
> +import subprocess
> +import pexpect
> +from time import sleep
> +import logging
> +import contextlib
> +
> +from lava_dispatcher.device.target import (
> +    Target
> +)
> +from lava_dispatcher.downloader import (
> +    download_image
> +)
> +from lava_dispatcher.utils import (
> +    logging_spawn,
> +    mkdtemp
> +)

OK, so this is probably mostly the dispatcher's fault for being
confusing, but there are a few things I don't really understand here.
If I get you to explain them to me in excruciating detail, maybe we can
make the code clearer for the next person.

> +class NexusTarget(Target):
> +
> +    def __init__(self, context, config):
> +        super(NexusTarget, self).__init__(context, config)
> +        self._powered_on = False

This seems like an assumption.  How likely is it that the device is
actually powered down?  If it's important that it is, maybe we should
power it down explicitly (although probably not in __init__).

> +        self._working_dir = None
> +
> +    def deploy_android(self, boot, system, userdata):
> +
> +        boot = self._get_image(boot)
> +        system = self._get_image(system)
> +        userdata = self._get_image(userdata)
> +
> +        self.reboot_os()

So, let me get this straight.  There are a few states we could be in:

1. At fastboot.
2. Powered down.
3. In android.
4. Hung.

We want to be in the fastboot state.  Maybe we should call a function
that makes that clear?  I guess I'm saying that you should name
functions in this class by the state they result the device being in,
rather than what they do, "intent revealing names" and all that.  In
addition, I think I'd find this clearer if the internal methods had
_underscore names.

If we're in state "4. Hung" we're doomed?

> +        self.fastboot('erase boot')
> +
> +        self.fastboot('flash system %s' % system)
> +        self.fastboot('flash userdata %s' % userdata)
> +
> +        self.deployment_data = Target.android_deployment_data
> +        self.deployment_data['boot_image'] = boot
> +
> +    def power_on(self):
> +        self.reboot_os()
> +        self.reboot_bootloader()

I don't understand why reboot_bootloader is here.  It seems to me that
we're in the same situation as deploy_android -- we don't know the state
and we want to be in fastboot.

> +        self.boot_test_image()
> +
> +        self._powered_on = True
> +        proc = self.adb('shell', spawn = True)
> +        proc.sendline("") # required to put the adb shell in a reasonable state
> +        proc.sendline("export PS1='%s'" % self.deployment_data['TESTER_PS1'])
> +        self._runner = self._get_runner(proc)
> +
> +        return proc
> +
> +    def reboot_os(self):
> +        # tell android to reboot. A failure probably means that the device is not
> +        # booted on android, and we ignore that.

It would be nice not to make assumptions here.

> +        self.adb('reboot', ignore_failure = True)
> +        sleep(10)

Are these sleeps really necessary?  AIUI, issuing a fastboot command
will wait for the device if it's not yet ready for it.

> +    def reboot_bootloader(self):
> +        self.fastboot('reboot')
> +        sleep(10)
> +
> +    def boot_test_image(self):
> +        self.fastboot('boot %s' % self.deployment_data['boot_image'])
> +        self.adb('wait-for-device')
> +
> +    def power_off(self, proc):
> +        # there is no way to power off the Nexus while USB is plugged on; even
> +        # if you remove power, it will stay on.
> +        pass

:( Maybe we'll get some of Andy Green's usb disconnector devices.

> +    @contextlib.contextmanager
> +    def file_system(self, partition, directory):
> +
> +        if not self._powered_on:
> +            self.power_on()
> +
> +        mount_point = self.get_partition_mount_point(partition)
> +
> +        with self.make_filesystem_readwrite(mount_point):

I /thought/ we generally didn't make modifications to /system.  But I
guess you wouldn't have done this unless it was needed...

> +            host_dir = '%s/mnt/%s' % (self.working_dir, directory)
> +            target_dir = '%s/%s' % (mount_point, directory)
> +
> +            subprocess.check_call(['mkdir', '-p', host_dir])
> +            self.adb('pull %s %s' % (target_dir, host_dir), ignore_failure = True)
> +
> +            yield host_dir
> +
> +            self.adb('push %s %s' % (host_dir, target_dir))
> +
> +    def get_partition_mount_point(self, partition):
> +        lookup = {
> +            self.config.data_part_android_org: '/data',
> +            self.config.sys_part_android_org: '/system',
> +        }
> +        return lookup[partition]
> +
> +    @contextlib.contextmanager
> +    def make_filesystem_readwrite(self, mount_point):
> +        if mount_point  == '/system':
> +            self._runner.run("mount -o remount,rw %s" % mount_point)
> +        yield
> +        if mount_point  == '/system':
> +            self._runner.run("mount -o remount,ro %s" % mount_point)
> +
> +    def get_device_version(self):
> +        # this is tricky, because fastboot does not have a visible version
> +        # number. For now let's use just the adb version number.
> +        return subprocess.check_output(
> +            "%s version | sed 's/.* version //'" % self.config.adb_command,
> +            shell = True
> +        ).strip()
> +
> +    def adb(self, args, ignore_failure = False, spawn = False):
> +        cmd = self.config.adb_command + ' ' + args
> +        if spawn:
> +            return logging_spawn(cmd, timeout = 60)
> +        else:
> +            self._call(cmd, ignore_failure)
> +
> +    def fastboot(self, args, ignore_failure = False):
> +        self._call(self.config.fastboot_command + ' ' + args, ignore_failure)
> +
> +    def _call(self, cmd, ignore_failure):
> +        logging.debug("Running on the host: %s" % cmd)
> +        if ignore_failure:
> +            subprocess.call(cmd, shell = True)
> +        else:
> +            subprocess.check_call(cmd, shell = True)

It would be nice to have a timeout for all calls somehow.  Not sure how, though.

> +    def _get_image(self, url):
> +        sdir = self.working_dir
> +        image = download_image(url, self.context, sdir, decompress=False)
> +        return image
> +
> +    @property
> +    def working_dir(self):
> +        if (self.config.nexus_working_directory is None or
> +            self.config.nexus_working_directory.strip() == ''):
> +            return self.scratch_dir
> +
> +        if self._working_dir is None:
> +            self._working_dir = mkdtemp(self.config.nexus_working_directory)
> +        return self._working_dir
> +
> +
> +target_class = NexusTarget

Sorry for all the questions!  Having a slow-brained day today it seems.

Cheers,
mwh

Revision history for this message

Antonio Terceiro (terceiro) wrote on 2013-01-21: Posted in a previous version of this proposal

#

Hi,

On Tue, Jan 15, 2013 at 11:37:22PM -0000, Andy Doan wrote:
> On 01/15/2013 01:18 PM, Antonio Terceiro wrote:
> > + def power_on(self):
>
> Seems like you might want to add some check like:
>
> if not self.deployment_data.get('boot_image', False):
> raise CriticalError('Deploy action must be run first')

Done, thanks.

> > + self.reboot_os()
> > + self.reboot_bootloader()
> > + self.boot_test_image()
> > +
> > + self._powered_on = True
> > + proc = self.adb('shell', spawn = True)
> > + proc.sendline("") # required to put the adb shell in a reasonable state
> > + proc.sendline("export PS1='%s'" % self.deployment_data['TESTER_PS1'])
> > + self._runner = self._get_runner(proc)
> > +
> > + return proc
>
> > + def power_off(self, proc):
> > + # there is no way to power off the Nexus while USB is plugged on; even
> > + # if you remove power, it will stay on.
> > + pass
> > +
> This might be correct, but I fear a bad image could put us in an
> irrecoverable state. One thing we might consider adding is some type of
> logic like I did in sdmux.sh:
>
>
> <http://bazaar.launchpad.net/~linaro-validation/lava-dispatcher/trunk/view/head:/lava_dispatcher/device/sdmux.sh>
>
> This essentially powers down a USB port and might solve your problem.
> However, since you use ADB over USB be aware of this issue:
>
> https://groups.google.com/forum/#!topic/android-platform/hLmD0DDAzj8

Actually, I investigated a little more and found that:

  - in android, just cutting the power works
  - in fastboot, cutting the power sometimes works, sometimes not. But
    if I tell fastboot to reboot after cutting the power, then it
    actually goes off.

I will try that.

> Lastly, the filesystem function looks like it might need some exception
> handling logic. Not sure about this, but I found the need when
> implementing that logic for other targets.

ok - I will look at it.

--
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org

Hi,

On Tue, Jan 15, 2013 at 11:37:22PM -0000, Andy Doan wrote:
> On 01/15/2013 01:18 PM, Antonio Terceiro wrote:
> > +    def power_on(self):
> 
> Seems like you might want to add some check like:
> 
>             if not self.deployment_data.get('boot_image', False):
>                 raise CriticalError('Deploy action must be run first')

Done, thanks.

> > +        self.reboot_os()
> > +        self.reboot_bootloader()
> > +        self.boot_test_image()
> > +
> > +        self._powered_on = True
> > +        proc = self.adb('shell', spawn = True)
> > +        proc.sendline("") # required to put the adb shell in a reasonable state
> > +        proc.sendline("export PS1='%s'" % self.deployment_data['TESTER_PS1'])
> > +        self._runner = self._get_runner(proc)
> > +
> > +        return proc
> 
> > +    def power_off(self, proc):
> > +        # there is no way to power off the Nexus while USB is plugged on; even
> > +        # if you remove power, it will stay on.
> > +        pass
> > +
> This might be correct, but I fear a bad image could put us in an 
> irrecoverable state. One thing we might consider adding is some type of 
> logic like I did in sdmux.sh:
> 
>  
> <http://bazaar.launchpad.net/~linaro-validation/lava-dispatcher/trunk/view/head:/lava_dispatcher/device/sdmux.sh>
> 
> This essentially powers down a USB port and might solve your problem. 
> However, since you use ADB over USB be aware of this issue:
> 
>   https://groups.google.com/forum/#!topic/android-platform/hLmD0DDAzj8

Actually, I investigated a little more and found that:

- in android, just cutting the power works
  - in fastboot, cutting the power sometimes works, sometimes not. But
    if I tell fastboot to reboot after cutting the power, then it
    actually goes off.

I will try that.

> Lastly, the filesystem function looks like it might need some exception 
> handling logic. Not sure about this, but I found the need when 
> implementing that logic for other targets.

ok - I will look at it.

-- 
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org

Revision history for this message

Antonio Terceiro (terceiro) wrote on 2013-01-21: Posted in a previous version of this proposal

#

Download full text (12.2 KiB)

On Wed, Jan 16, 2013 at 12:16:20AM -0000, Michael Hudson-Doyle wrote:
> Antonio Terceiro <email address hidden> writes:
>
> > Antonio Terceiro has proposed merging lp:~terceiro/lava-dispatcher/nexus into lp:lava-dispatcher.
> >
> > Requested reviews:
> > Linaro Validation Team (linaro-validation)
> >
> > For more details, see:
> > https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> >
> > Hi,
> >
> > This time is for real. :-)
> >
> > I don't see any pending issues, so know you can have a hard review on the code.
> > --
> > https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> > You are subscribed to branch lp:lava-dispatcher.
> > === modified file 'lava_dispatcher/config.py'
> > --- lava_dispatcher/config.py 2013-01-10 21:37:58 +0000
> > +++ lava_dispatcher/config.py 2013-01-15 19:17:23 +0000
> > @@ -91,6 +91,9 @@
> > arm_probe_config = schema.StringOption(default='/usr/local/etc/arm-probe-config')
> > arm_probe_channels = schema.ListOption(default=['VDD_VCORE1'])
> >
> > + adb_command = schema.StringOption()
> > + fastboot_command = schema.StringOption()
> > + nexus_working_directory = schema.StringOption(default = None)
>
> It's super nitty, but "default=None" (i.e. without the spaces) would be
> more conventional.

Fixed.

> > class OptionDescriptor(object):
> > def __init__(self, name):
> >
> > === added file 'lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf'
> > --- lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf 1970-01-01 00:00:00 +0000
> > +++ lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf 2013-01-15 19:17:23 +0000
> > @@ -0,0 +1,40 @@
> > +client_type = nexus
> > +
> > +# The ADB command line.
> > +#
> > +# In the case where there are more than one android devices plugged into a
>
> s/are/is/ or s/more than one/multiple/ (English grammar makes no sense)

Fixed

> > +# single host, this connection command must be overriden on each device to
> > +# include the serial number of the device, e.g.
> > +#
> > +# serial_number = XXXXXXXXXXXXXXXX
> > +# adb_command = adb -s %(serial_number)s
> > +adb_command = adb
> > +
> > +# The fastboot command.
> > +#
> > +# The same as above: if you have more than one device, you will want to
> > +# override this in your device config to add a serial number, e.g.
> > +#
> > +# serial_number = XXXXXXXXXXXXXXXX
> > +# fastboot_command = sudo fastboot -s %(serial_number)s
> > +#
> > +# Of course, in the case you override both adb_command *and* fastboot_command,
> > +# you don't need to specify `serial_number` twice.
> > +fastboot_command = sudo fastboot
> > +
> > +# Working directory for temporary. By default, the usual place for LAVA images
> > +# will be used.
> > +#
> > +# This is useful when the lava dispatcher is controlling Nexus phones that are
> > +# physically plugged to other machines. In this case, you should set
> > +# nexus_work_directory to a shared directory between the machine running the
> > +# dispatcher and the machine where the phone is plugged.
>
> I don't entirely understand when this would be useful.

I will try to explain it...

On Wed, Jan 16, 2013 at 12:16:20AM -0000, Michael Hudson-Doyle wrote:
> Antonio Terceiro <antonio.terceiro@linaro.org> writes:
> 
> > Antonio Terceiro has proposed merging lp:~terceiro/lava-dispatcher/nexus into lp:lava-dispatcher.
> >
> > Requested reviews:
> >   Linaro Validation Team (linaro-validation)
> >
> > For more details, see:
> > https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> >
> > Hi,
> >
> > This time is for real. :-)
> >
> > I don't see any pending issues, so know you can have a hard review on the code.
> > -- 
> > https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> > You are subscribed to branch lp:lava-dispatcher.
> > === modified file 'lava_dispatcher/config.py'
> > --- lava_dispatcher/config.py	2013-01-10 21:37:58 +0000
> > +++ lava_dispatcher/config.py	2013-01-15 19:17:23 +0000
> > @@ -91,6 +91,9 @@
> >      arm_probe_config = schema.StringOption(default='/usr/local/etc/arm-probe-config')
> >      arm_probe_channels = schema.ListOption(default=['VDD_VCORE1'])
> >  
> > +    adb_command = schema.StringOption()
> > +    fastboot_command = schema.StringOption()
> > +    nexus_working_directory = schema.StringOption(default = None)
> 
> It's super nitty, but "default=None" (i.e. without the spaces) would be
> more conventional.

Fixed.

> >  class OptionDescriptor(object):
> >      def __init__(self, name):
> >
> > === added file 'lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf'
> > --- lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	1970-01-01 00:00:00 +0000
> > +++ lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	2013-01-15 19:17:23 +0000
> > @@ -0,0 +1,40 @@
> > +client_type = nexus
> > +
> > +# The ADB command line.
> > +#
> > +# In the case where there are more than one android devices plugged into a
> 
> s/are/is/ or s/more than one/multiple/ (English grammar makes no sense)

Fixed

> > +# single host, this connection command must be overriden on each device to
> > +# include the serial number of the device, e.g.
> > +#
> > +#   serial_number = XXXXXXXXXXXXXXXX
> > +#   adb_command = adb -s %(serial_number)s
> > +adb_command = adb
> > +
> > +# The fastboot command.
> > +#
> > +# The same as above: if you have more than one device, you will want to
> > +# override this in your device config to add a serial number, e.g.
> > +#
> > +#   serial_number = XXXXXXXXXXXXXXXX
> > +#   fastboot_command = sudo fastboot -s %(serial_number)s
> > +#
> > +# Of course, in the case you override both adb_command *and* fastboot_command,
> > +# you don't need to specify `serial_number` twice.
> > +fastboot_command = sudo fastboot
> > +
> > +# Working directory for temporary. By default, the usual place for LAVA images
> > +# will be used.
> > +#
> > +# This is useful when the lava dispatcher is controlling Nexus phones that are
> > +# physically plugged to other machines. In this case, you should set
> > +# nexus_work_directory to a shared directory between the machine running the
> > +# dispatcher and the machine where the phone is plugged.
> 
> I don't entirely understand when this would be useful.

I will try to explain it better. :)

> > +# OBS: this shared directory must have the same path in both machines.
> 
> OBS?

heh, that's the equivalent of "N.B." in Portuguese, I guess it doesn't
exist in English. :) I will rewrite that doc, so for now I just removed
the OBS.

> > +nexus_working_directory =
> > +
> > +connection_command = %(adb_command)s shell
> > +
> > +enable_network_after_boot_android = false
> > +android_adb_over_usb = true
> > +android_adb_over_tcp = false
> >
> > === added file 'lava_dispatcher/device/nexus.py'
> > --- lava_dispatcher/device/nexus.py	1970-01-01 00:00:00 +0000
> > +++ lava_dispatcher/device/nexus.py	2013-01-15 19:17:23 +0000
> > @@ -0,0 +1,168 @@
> > +# Copyright (C) 2012 Linaro Limited
> > +#
> > +# Author: Antonio Terceiro <antonio.terceiro@linaro.org>
> > +#
> > +# This file is part of LAVA Dispatcher.
> > +#
> > +# LAVA Dispatcher is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# LAVA Dispatcher is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.    See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along
> > +# with this program; if not, see <http://www.gnu.org/licenses>.
> > +
> > +import subprocess
> > +import pexpect
> > +from time import sleep
> > +import logging
> > +import contextlib
> > +
> > +from lava_dispatcher.device.target import (
> > +    Target
> > +)
> > +from lava_dispatcher.downloader import (
> > +    download_image
> > +)
> > +from lava_dispatcher.utils import (
> > +    logging_spawn,
> > +    mkdtemp
> > +)
> 
> OK, so this is probably mostly the dispatcher's fault for being
> confusing, but there are a few things I don't really understand here.
> If I get you to explain them to me in excruciating detail, maybe we can
> make the code clearer for the next person.
> 
> > +class NexusTarget(Target):
> > +
> > +    def __init__(self, context, config):
> > +        super(NexusTarget, self).__init__(context, config)
> > +        self._powered_on = False
> 
> This seems like an assumption.  How likely is it that the device is
> actually powered down?  If it's important that it is, maybe we should
> power it down explicitly (although probably not in __init__).

OK.

> > +        self._working_dir = None
> > +
> > +    def deploy_android(self, boot, system, userdata):
> > +
> > +        boot = self._get_image(boot)
> > +        system = self._get_image(system)
> > +        userdata = self._get_image(userdata)
> > +
> > +        self.reboot_os()
> 
> So, let me get this straight.  There are a few states we could be in:
> 
> 1. At fastboot.
> 2. Powered down.
> 3. In android.
> 4. Hung.
> 
> We want to be in the fastboot state.  Maybe we should call a function
> that makes that clear?  I guess I'm saying that you should name
> functions in this class by the state they result the device being in,
> rather than what they do, "intent revealing names" and all that.  In
> addition, I think I'd find this clearer if the internal methods had
> _underscore names.
> 
> If we're in state "4. Hung" we're doomed?

I think we can power it off. I'll think of a way of forcing the system
to hang and test that.

> > +        self.fastboot('erase boot')
> > +
> > +        self.fastboot('flash system %s' % system)
> > +        self.fastboot('flash userdata %s' % userdata)
> > +
> > +        self.deployment_data = Target.android_deployment_data
> > +        self.deployment_data['boot_image'] = boot
> > +
> > +    def power_on(self):
> > +        self.reboot_os()
> > +        self.reboot_bootloader()
> 
> I don't understand why reboot_bootloader is here.  It seems to me that
> we're in the same situation as deploy_android -- we don't know the state
> and we want to be in fastboot.

There is a weirdness with fastboot: if you don't reboot it before
booting the actual kernel, it will enter charging mode and won't boot
without physical intervention.

> > +        self.boot_test_image()
> > +
> > +        self._powered_on = True
> > +        proc = self.adb('shell', spawn = True)
> > +        proc.sendline("") # required to put the adb shell in a reasonable state
> > +        proc.sendline("export PS1='%s'" % self.deployment_data['TESTER_PS1'])
> > +        self._runner = self._get_runner(proc)
> > +
> > +        return proc
> > +
> > +    def reboot_os(self):
> > +        # tell android to reboot. A failure probably means that the device is not
> > +        # booted on android, and we ignore that.
> 
> It would be nice not to make assumptions here.

Sure.

> > +        self.adb('reboot', ignore_failure = True)
> > +        sleep(10)
> 
> Are these sleeps really necessary?  AIUI, issuing a fastboot command
> will wait for the device if it's not yet ready for it.

I will check that.

> > +    def reboot_bootloader(self):
> > +        self.fastboot('reboot')
> > +        sleep(10)
> > +
> > +    def boot_test_image(self):
> > +        self.fastboot('boot %s' % self.deployment_data['boot_image'])
> > +        self.adb('wait-for-device')
> > +
> > +    def power_off(self, proc):
> > +        # there is no way to power off the Nexus while USB is plugged on; even
> > +        # if you remove power, it will stay on.
> > +        pass
> 
> :( Maybe we'll get some of Andy Green's usb disconnector devices.

No, I discovered a way to consistently turn power off now (see my
response to Andy's comments)

> > +    @contextlib.contextmanager
> > +    def file_system(self, partition, directory):
> > +
> > +        if not self._powered_on:
> > +            self.power_on()
> > +
> > +        mount_point = self.get_partition_mount_point(partition)
> > +
> > +        with self.make_filesystem_readwrite(mount_point):
> 
> I /thought/ we generally didn't make modifications to /system.  But I
> guess you wouldn't have done this unless it was needed...

This was needed when I was trying to copy the mkfifo binary into the
device, I can probably remove it now.

> > +            host_dir = '%s/mnt/%s' % (self.working_dir, directory)
> > +            target_dir = '%s/%s' % (mount_point, directory)
> > +
> > +            subprocess.check_call(['mkdir', '-p', host_dir])
> > +            self.adb('pull %s %s' % (target_dir, host_dir), ignore_failure = True)
> > +
> > +            yield host_dir
> > +
> > +            self.adb('push %s %s' % (host_dir, target_dir))
> > +
> > +    def get_partition_mount_point(self, partition):
> > +        lookup = {
> > +            self.config.data_part_android_org: '/data',
> > +            self.config.sys_part_android_org: '/system',
> > +        }
> > +        return lookup[partition]
> > +
> > +    @contextlib.contextmanager
> > +    def make_filesystem_readwrite(self, mount_point):
> > +        if mount_point  == '/system':
> > +            self._runner.run("mount -o remount,rw %s" % mount_point)
> > +        yield
> > +        if mount_point  == '/system':
> > +            self._runner.run("mount -o remount,ro %s" % mount_point)
> > +
> > +    def get_device_version(self):
> > +        # this is tricky, because fastboot does not have a visible version
> > +        # number. For now let's use just the adb version number.
> > +        return subprocess.check_output(
> > +            "%s version | sed 's/.* version //'" % self.config.adb_command,
> > +            shell = True
> > +        ).strip()
> > +
> > +    def adb(self, args, ignore_failure = False, spawn = False):
> > +        cmd = self.config.adb_command + ' ' + args
> > +        if spawn:
> > +            return logging_spawn(cmd, timeout = 60)
> > +        else:
> > +            self._call(cmd, ignore_failure)
> > +
> > +    def fastboot(self, args, ignore_failure = False):
> > +        self._call(self.config.fastboot_command + ' ' + args, ignore_failure)
> > +
> > +    def _call(self, cmd, ignore_failure):
> > +        logging.debug("Running on the host: %s" % cmd)
> > +        if ignore_failure:
> > +            subprocess.call(cmd, shell = True)
> > +        else:
> > +            subprocess.check_call(cmd, shell = True)
> 
> It would be nice to have a timeout for all calls somehow.  Not sure how, though.

I will look at that.

> > +    def _get_image(self, url):
> > +        sdir = self.working_dir
> > +        image = download_image(url, self.context, sdir, decompress=False)
> > +        return image
> > +
> > +    @property
> > +    def working_dir(self):
> > +        if (self.config.nexus_working_directory is None or
> > +            self.config.nexus_working_directory.strip() == ''):
> > +            return self.scratch_dir
> > +
> > +        if self._working_dir is None:
> > +            self._working_dir = mkdtemp(self.config.nexus_working_directory)
> > +        return self._working_dir
> > +
> > +
> > +target_class = NexusTarget
> 
> Sorry for all the questions!  Having a slow-brained day today it seems.

Nothing to be sorry for - all of them were very useful!

-- 
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org

Revision history for this message

Michael Hudson-Doyle (mwhudson) wrote on 2013-01-21: Posted in a previous version of this proposal

#

Download full text (6.5 KiB)

Antonio Terceiro <email address hidden> writes:

> On Wed, Jan 16, 2013 at 12:16:20AM -0000, Michael Hudson-Doyle wrote:
>> Antonio Terceiro <email address hidden> writes:
>> > +# Working directory for temporary. By default, the usual place for LAVA images
>> > +# will be used.
>> > +#
>> > +# This is useful when the lava dispatcher is controlling Nexus phones that are
>> > +# physically plugged to other machines. In this case, you should set
>> > +# nexus_work_directory to a shared directory between the machine running the
>> > +# dispatcher and the machine where the phone is plugged.
>>
>> I don't entirely understand when this would be useful.
>
> I will try to explain it better. :)

Did you do this? I'm guessing you use it when LAVA is running in a VM?

>> > +# OBS: this shared directory must have the same path in both machines.
>>
>> OBS?
>
> heh, that's the equivalent of "N.B." in Portuguese, I guess it doesn't
> exist in English. :) I will rewrite that doc, so for now I just removed
> the OBS.

Heh.

>> > +nexus_working_directory =
>> > +
>> > +connection_command = %(adb_command)s shell
>> > +
>> > +enable_network_after_boot_android = false
>> > +android_adb_over_usb = true
>> > +android_adb_over_tcp = false
>> >
>> > === added file 'lava_dispatcher/device/nexus.py'
>> > --- lava_dispatcher/device/nexus.py 1970-01-01 00:00:00 +0000
>> > +++ lava_dispatcher/device/nexus.py 2013-01-15 19:17:23 +0000
>> > @@ -0,0 +1,168 @@
>> > +# Copyright (C) 2012 Linaro Limited
>> > +#
>> > +# Author: Antonio Terceiro <email address hidden>
>> > +#
>> > +# This file is part of LAVA Dispatcher.
>> > +#
>> > +# LAVA Dispatcher is free software; you can redistribute it and/or modify
>> > +# it under the terms of the GNU General Public License as published by
>> > +# the Free Software Foundation; either version 2 of the License, or
>> > +# (at your option) any later version.
>> > +#
>> > +# LAVA Dispatcher is distributed in the hope that it will be useful,
>> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> > +# GNU General Public License for more details.
>> > +#
>> > +# You should have received a copy of the GNU General Public License
>> > +# along
>> > +# with this program; if not, see <http://www.gnu.org/licenses>.
>> > +
>> > +import subprocess
>> > +import pexpect
>> > +from time import sleep
>> > +import logging
>> > +import contextlib
>> > +
>> > +from lava_dispatcher.device.target import (
>> > + Target
>> > +)
>> > +from lava_dispatcher.downloader import (
>> > + download_image
>> > +)
>> > +from lava_dispatcher.utils import (
>> > + logging_spawn,
>> > + mkdtemp
>> > +)
>>
>> OK, so this is probably mostly the dispatcher's fault for being
>> confusing, but there are a few things I don't really understand here.
>> If I get you to explain them to me in excruciating detail, maybe we can
>> make the code clearer for the next person.
>>
>> > +class NexusTarget(Target):
>> > +
>> > + def __init__(self, context, config):
>> > + super(NexusTarget, self).__init__(context, config)
>> > + self._powered_on = Fal...

LAVA Dispatcher

Merge lp:~terceiro/lava-dispatcher/nexus into lp:lava-dispatcher

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'lava_dispatcher/config.py'
 --- lava_dispatcher/config.py	2013-01-18 11:08:24 +0000
 +++ lava_dispatcher/config.py	2013-01-22 02:17:24 +0000
@@ -92,6 +92,9 @@
      arm_probe_config = schema.StringOption(default='/usr/local/etc/arm-probe-config')
      arm_probe_channels = schema.ListOption(default=['VDD_VCORE1'])
++    adb_command = schema.StringOption()
++    fastboot_command = schema.StringOption()
++    nexus_working_directory = schema.StringOption(default=None)
  class OptionDescriptor(object):
      def __init__(self, name):
 === added file 'lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf'
 --- lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	1970-01-01 00:00:00 +0000
 +++ lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	2013-01-22 02:17:24 +0000
@@ -0,0 +1,46 @@
++client_type = nexus
++
++# The ADB command line.
++#
++# In the case where there are multiple android devices plugged into a
++# single host, this connection command must be overriden on each device to
++# include the serial number of the device, e.g.
++#
++#   serial_number = XXXXXXXXXXXXXXXX
++#   adb_command = adb -s %(serial_number)s
++adb_command = adb
++
++# The fastboot command.
++#
++# The same as above: if you have more than one device, you will want to
++# override this in your device config to add a serial number, e.g.
++#
++#   serial_number = XXXXXXXXXXXXXXXX
++#   fastboot_command = fastboot -s %(serial_number)s
++#
++# Of course, in the case you override both adb_command *and* fastboot_command,
++# you don't need to specify `serial_number` twice.
++fastboot_command = fastboot
++
++# Working directory for temporary files. By default, the usual place for LAVA
++# images will be used.
++#
++# This is useful when the lava dispatcher is controlling Nexus phones that are
++# physically plugged to other machines by setting adb_command to something like
++# "ssh <phone-host> adb" and fastboot_command to something like "ssh
++# <phone-host> fastboot". adb and fastboot always operate on local files, so
++# you need your local files to also be seen as local files on the host where
++# adb/fastboot are executed.
++#
++# In this case, you should set nexus_work_directory to a shared directory
++# between the machine running the dispatcher and the machine where the phone is
++# plugged.  This shared directory must have the same path in both machines.
++# For example, you can have your /var/tmp/lava mounted at /var/tmp/lava at
++# <phone-host> (or the other way around).
++nexus_working_directory =
++
++connection_command = %(adb_command)s shell
++
++enable_network_after_boot_android = false
++android_adb_over_usb = true
++android_adb_over_tcp = false
 === added file 'lava_dispatcher/device/nexus.py'
 --- lava_dispatcher/device/nexus.py	1970-01-01 00:00:00 +0000
 +++ lava_dispatcher/device/nexus.py	2013-01-22 02:17:24 +0000
@@ -0,0 +1,193 @@
++# Copyright (C) 2012 Linaro Limited
++#
++# Author: Antonio Terceiro <antonio.terceiro@linaro.org>
++#
++# This file is part of LAVA Dispatcher.
++#
++# LAVA Dispatcher is free software; you can redistribute it and/or modify
++# it under the terms of the GNU General Public License as published by
++# the Free Software Foundation; either version 2 of the License, or
++# (at your option) any later version.
++#
++# LAVA Dispatcher is distributed in the hope that it will be useful,
++# but WITHOUT ANY WARRANTY; without even the implied warranty of
++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.    See the
++# GNU General Public License for more details.
++#
++# You should have received a copy of the GNU General Public License
++# along
++# with this program; if not, see <http://www.gnu.org/licenses>.
++
++import subprocess
++import pexpect
++from time import sleep
++import logging
++import contextlib
++
++from lava_dispatcher.device.target import (
++    Target
++)
++from lava_dispatcher.downloader import (
++    download_image
++)
++from lava_dispatcher.utils import (
++    logging_system,
++    logging_spawn,
++    mkdtemp
++)
++from lava_dispatcher.errors import (
++    CriticalError
++)
++
++class NexusTarget(Target):
++
++    def __init__(self, context, config):
++        super(NexusTarget, self).__init__(context, config)
++
++        if not config.hard_reset_command:
++            logging.warn(
++                "Setting the hard_reset_command config option "
++                "is highly recommended!"
++            )
++
++        self._booted = False
++        self._working_dir = None
++
++    def deploy_android(self, boot, system, userdata):
++
++        boot = self._get_image(boot)
++        system = self._get_image(system)
++        userdata = self._get_image(userdata)
++
++        self._enter_fastboot()
++        self._fastboot('erase boot')
++        self._fastboot('flash system %s' % system)
++        self._fastboot('flash userdata %s' % userdata)
++
++        self.deployment_data = Target.android_deployment_data
++        self.deployment_data['boot_image'] = boot
++
++    def power_on(self):
++        if not self.deployment_data.get('boot_image', False):
++            raise CriticalError('Deploy action must be run first')
++
++        self._enter_fastboot()
++        self._boot_test_image()
++
++        self._booted = True
++        proc = self._adb('shell', spawn = True)
++        proc.sendline("") # required to put the adb shell in a reasonable state
++        proc.sendline("export PS1='%s'" % self.deployment_data['TESTER_PS1'])
++        self._runner = self._get_runner(proc)
++
++        return proc
++
++    def power_off(self, proc):
++        # We always leave the device on
++        pass
++
++    @contextlib.contextmanager
++    def file_system(self, partition, directory):
++
++        if not self._booted:
++            self.power_on()
++
++        mount_point = self._get_partition_mount_point(partition)
++
++        host_dir = '%s/mnt/%s' % (self.working_dir, directory)
++        target_dir = '%s/%s' % (mount_point, directory)
++
++        subprocess.check_call(['mkdir', '-p', host_dir])
++        self._adb('pull %s %s' % (target_dir, host_dir), ignore_failure = True)
++
++        yield host_dir
++
++        self._adb('push %s %s' % (host_dir, target_dir))
++
++    def get_device_version(self):
++        # this is tricky, because fastboot does not have a visible version
++        # number. For now let's use just the adb version number.
++        return subprocess.check_output(
++            "%s version | sed 's/.* version //'" % self.config.adb_command,
++            shell = True
++        ).strip()
++
++    # start of private methods
++
++    def _enter_fastboot(self):
++        if self._already_on_fastboot():
++            logging.debug("Device is on fastboot - no need to hard reset")
++            return
++        try:
++            # First we try a gentle reset
++            self._adb('reboot')
++        except subprocess.CalledProcessError:
++            # Now a more brute force attempt. In this case the device is
++            # probably hung.
++            if self.config.hard_reset_command:
++                logging.debug("Will hard reset the device")
++                logging_system(self.config.hard_reset_command)
++            else:
++                logging.critical(
++                    "Hard reset command not configured. "
++                    "Please reset the device manually."
++                )
++
++    def _already_on_fastboot(self):
++        try:
++            self._fastboot('getvar all', timeout = 2)
++            return True
++        except subprocess.CalledProcessError:
++            return False
++
++    def _boot_test_image(self):
++        # We need an extra bootloader reboot before actually booting the image
++        # to avoid the phone entering charging mode and getting stuck.
++        self._fastboot('reboot')
++        # specifically after `fastboot reset`, we have to wait a little
++        sleep(10)
++        self._fastboot('boot %s' % self.deployment_data['boot_image'])
++        self._adb('wait-for-device')
++
++    def _get_partition_mount_point(self, partition):
++        lookup = {
++            self.config.data_part_android_org: '/data',
++            self.config.sys_part_android_org: '/system',
++        }
++        return lookup[partition]
++
++    def _adb(self, args, ignore_failure = False, spawn = False, timeout = 600):
++        cmd = self.config.adb_command + ' ' + args
++        if spawn:
++            return logging_spawn(cmd, timeout = 60)
++        else:
++            self._call(cmd, ignore_failure, timeout)
++
++    def _fastboot(self, args, ignore_failure = False, timeout = 600):
++        self._call(self.config.fastboot_command + ' ' + args, ignore_failure, timeout)
++
++    def _call(self, cmd, ignore_failure, timeout):
++        cmd = 'timeout ' + str(timeout) + 's ' + cmd
++        logging.debug("Running on the host: %s" % cmd)
++        if ignore_failure:
++            subprocess.call(cmd, shell = True)
++        else:
++            subprocess.check_call(cmd, shell = True)
++
++    def _get_image(self, url):
++        sdir = self.working_dir
++        image = download_image(url, self.context, sdir, decompress=False)
++        return image
++
++    @property
++    def working_dir(self):
++        if (self.config.nexus_working_directory is None or
++            self.config.nexus_working_directory.strip() == ''):
++            return self.scratch_dir
++
++        if self._working_dir is None:
++            self._working_dir = mkdtemp(self.config.nexus_working_directory)
++        return self._working_dir
++
++
++target_class = NexusTarget