Hey Nick,

It's freaking cool that we already had this working! :-)

Follows my comments. I know you have a separate branch where you are
trying to abstract the parts in common with master.py, but I am making
some comments about that here anyway.

 review needs-fixing

> === modified file 'lava_dispatcher/client/base.py'
> --- lava_dispatcher/client/base.py	2013-03-27 11:22:07 +0000
> +++ lava_dispatcher/client/base.py	2013-04-08 13:59:23 +0000
> @@ -154,7 +154,7 @@
>          lava_server_ip = self._client.context.config.lava_server_ip
>          self.run(
>              "LC_ALL=C ping -W4 -c1 %s" % lava_server_ip,
> -            ["1 received", "0 received", "Network is unreachable"],
> +            ["1 received|1 packets* received", "0 received|0 packets received", "Network is unreachable"],
>              timeout=5, failok=True)
>          if self.match_id == 0:
>              return True
> 

Do you really need this? Did ping had a different output on the rootfs
you tested with?

> === modified file 'lava_dispatcher/client/lmc_utils.py'
> --- lava_dispatcher/client/lmc_utils.py	2013-02-18 03:19:14 +0000
> +++ lava_dispatcher/client/lmc_utils.py	2013-04-08 13:59:23 +0000
> @@ -15,7 +15,8 @@
>      )
>  
>  
> -def generate_image(client, hwpack_url, rootfs_url, outdir, bootloader='u_boot', rootfstype=None):
> +def generate_image(client, hwpack_url, rootfs_url, outdir, bootloader='u_boot', rootfstype=None,
> +                   extra_boot_args=None, image_size=None):
>      """Generate image from a hwpack and rootfs url
>  
>      :param hwpack_url: url of the Linaro hwpack to download
> @@ -32,7 +33,7 @@
>      rootfs_path = download_image(rootfs_url, client.context, outdir, decompress=False)
>  
>      logging.info("linaro-media-create version information")
> -    cmd = "sudo linaro-media-create -v"
> +    cmd = "linaro-media-create -v"
>      rc, output = getstatusoutput(cmd)
>      metadata = client.context.test_data.get_metadata()
>      metadata['target.linaro-media-create-version'] = output

I'm not sure we want to drop the sudo there. Even though the dispatcher
currently requires being run as root, I think in the long run we should
be able to drop that requirement. Also, if we are root already, the sudo
does no harm.

Additionally, it's a good idea to keep the merge proposal focused and
avoid lateral changes.

> @@ -42,11 +43,15 @@
>  
>      logging.info("client.device_type = %s" %client.config.device_type)
>  
> -    cmd = ("sudo flock /var/lock/lava-lmc.lck linaro-media-create --hwpack-force-yes --dev %s "
> +    cmd = ("flock /var/lock/lava-lmc.lck linaro-media-create --hwpack-force-yes --dev %s "
>             "--image-file %s --binary %s --hwpack %s --image-size 3G --bootloader %s" %
>             (client.config.lmc_dev_arg, image_file, rootfs_path, hwpack_path, bootloader))

Ditto.

>      if rootfstype is not None:
>          cmd += ' --rootfs ' + rootfstype
> +    if image_size is not None:
> +        cmd += ' --image-size ' + image_size
> +    if extra_boot_args is not None:
> +        cmd += ' --extra-boot-args "%s"' % extra_boot_args
>      logging.info("Executing the linaro-media-create command")
>      logging.info(cmd)
>  
> @@ -85,7 +90,7 @@
>      mntdir = mkdtemp()
>      image = image_file
>      offset = get_partition_offset(image, partno)
> -    mount_cmd = "sudo mount -o loop,offset=%s %s %s" % (offset, image, mntdir)
> +    mount_cmd = "mount -o loop,offset=%s %s %s" % (offset, image, mntdir)
>      rc = logging_system(mount_cmd)
>      if rc != 0:
>          os.rmdir(mntdir)

Ditto.

> @@ -94,7 +99,7 @@
>      try:
>          yield mntdir
>      finally:
> -        logging_system('sudo umount ' + mntdir)
> +        logging_system('umount ' + mntdir)
>          logging_system('rm -rf ' + mntdir)

Ditto.

> === modified file 'lava_dispatcher/config.py'
> --- lava_dispatcher/config.py	2013-04-05 17:19:57 +0000
> +++ lava_dispatcher/config.py	2013-04-08 13:59:23 +0000
> @@ -104,6 +104,8 @@
>          default='Press Enter to stop auto boot...')
>      vexpress_usb_mass_storage_device = schema.StringOption(default=None)
>  
> +    ecmeip = schema.StringOption()
> +

...

> === added file 'lava_dispatcher/default-config/lava-dispatcher/device-types/highbank.conf'
> --- lava_dispatcher/default-config/lava-dispatcher/device-types/highbank.conf	1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/default-config/lava-dispatcher/device-types/highbank.conf	2013-04-08 13:59:23 +0000
> @@ -0,0 +1,2 @@
> +client_type = highbank
> +connection_command = ipmitool -I lanplus -U admin -P admin -H %(ecmeip)s sol activate
> 
> === added file 'lava_dispatcher/device/highbank.py'
> --- lava_dispatcher/device/highbank.py	1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/device/highbank.py	2013-04-08 13:59:23 +0000
> @@ -0,0 +1,322 @@
> +# Copyright (C) 2012 Linaro Limited
> +#
> +# Author: Michael Hudson-Doyle <email address hidden>

At this point you should put yourself here as well, since we can already
be blamed for this. :-)

> +#
> +# 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 contextlib
> +import logging
> +import os
> +import pexpect
> +import time
> +
> +from lava_dispatcher import tarballcache
> +
> +from lava_dispatcher.client.base import (
> +    NetworkCommandRunner,
> +)
> +from lava_dispatcher.device.target import (
> +    Target
> +)
> +from lava_dispatcher.errors import (
> +    NetworkError,
> +    CriticalError,
> +    OperationFailed,
> +)
> +from lava_dispatcher.downloader import (
> +    download_image,
> +    download_with_retry,
> +    )
> +from lava_dispatcher.utils import (
> +    logging_system,
> +    logging_spawn,
> +    mk_targz,
> +    rmtree,
> +)
> +from lava_dispatcher.client.lmc_utils import (
> +    generate_image,
> +)
> +from lava_dispatcher.ipmi import IPMITool
> +
> +
> +class HighbankTarget(Target):
> +
> +    MASTER_PS1 = 'root@master [rc=$(echo \$?)]# '
> +    MASTER_PS1_PATTERN = 'root@master \[rc=(\d+)\]# '
> +
> +    def __init__(self, context, config):
> +        super(HighbankTarget, self).__init__(context, config)
> +        self.proc = logging_spawn(self.config.connection_command)

Here you want self.context.spawn instead

> +        self.proc.logfile_read = context.logfile_read

This is not needed anymore.

> +        self.device_version = None
> +        self.bootcontrol = IpmiPxeBoot(self.config.ecmeip)

Here I think you should raise an exception if self.config.ecmeip is not
set, because we cannot have a default, and without it nothing else will
work.

> +
> +    def get_device_version(self):
> +        return self.device_version
> +
> +    def power_on(self):
> +        self.bootcontrol.power_on_boot_image()
> +        return self.proc
> +
> +    def power_off(self, proc):
> +        self.bootcontrol.power_off()
> +
> +    def deploy_linaro(self, hwpack, rfs, bootloader):
> +        self.deployment_data = Target.ubuntu_deployment_data

_customize_linux already does this, and handles detecting whether the
image is actualy Ubuntu or OE.

> +        image_file = generate_image(self, hwpack, rfs, self.scratch_dir, bootloader,
> +                                    extra_boot_args='1', image_size='1G')
> +        self._customize_linux(image_file)
> +        self._deploy_image(image_file, '/dev/sda')
> +
> +    def deploy_linaro_prebuilt(self, image):
> +        self.deployment_data = Target.ubuntu_deployment_data

Ditto.

> +        image_file = download_image(image, self.context, self.scratch_dir)
> +        self._customize_linux(image_file)
> +        self._deploy_image(image_file, '/dev/sda')
> +
> +    def _deploy_image(self, image_file, device):
> +        with self._as_master() as runner:
> +
> +            # compress the image to reduce the transfer size
> +	    if not image_file.endswith('.bz2') and not image_file.endswith('gz'):

Broken indentation here, you have a tab where it should be spaces.

> +                os.system('bzip2 -v ' + image_file)
> +                image_file += '.bz2'
> +
> +            tmpdir = self.context.config.lava_image_tmpdir
> +            url = self.context.config.lava_image_url
> +            image_file = image_file.replace(tmpdir, '')
> +            image_url = '/'.join(u.strip('/') for u in [url, image_file])
> +
> +            decompression_cmd = ''
> +            if image_url.endswith('.gz') or image_url.endswith('.tgz'):
> +                decompression_cmd = '| /bin/gzip -dc'

gzip alone is not enough to uncompress a .tgz. Actually, I don't see a
reason why you would get a .tgz here.

> +            elif image_url.endswith('.bz2'):
> +                decompression_cmd = '| /bin/bzip2 -dc'
> +
> +            runner.run('mkdir /builddir')
> +            runner.run('mount -t tmpfs -o size=4G tmpfs builddir')
> +            image = '/builddir/lava.img'
> +            runner.run('wget -O - %s %s > %s' % (image_url, decompression_cmd, image), timeout=1800)
> +            runner.run('dd bs=4M if=%s of=%s' % (image, device), timeout=1800)
> +            runner.run('umount /builddir')

Is it always possible to create a 4G tmpfs?

Instead of saving the image locally, I think you could pipe the wget
output directl into dd. This way you don't need to to create a tmpfs,
nor to mount/umount it. This would be something like:

  runner.run("wget -O - %s %s | dd bs=4M of=%s" % (image_url,  decompression_cmd, device))

Also, there are some wget options you can use to get some progress
indication, which is useful. Look at how master.py does it.

(if you convince me the tmpfs is needed, make sure to add a missing
slash in the last token there so it reads `/builddir` instead of
`builddir`)

> +    def get_partition(self, runner, partition):
> +        if partition == self.config.boot_part:
> +            partition = '/dev/disk/by-label/boot'
> +        elif partition == self.config.root_part:
> +            partition = '/dev/disk/by-label/rootfs'
> +        else:
> +            raise RuntimeError(
> +                'unknown master image partition(%d)' % partition)
> +        return partition
> +
> +
> +    @contextlib.contextmanager
> +    def file_system(self, partition, directory):
> +        logging.info('attempting to access master filesystem %r:%s' %
> +            (partition, directory))
> +
> +        assert directory != '/', "cannot mount entire partition"
> +
> +        with self._as_master() as runner:
> +            runner.run('mkdir -p /mnt')
> +            partition = self.get_partition(runner, partition)
> +            runner.run('mount %s /mnt' % partition)
> +            try:
> +                targetdir = '/mnt/%s' % directory
> +                runner.run('mkdir -p %s' % targetdir)
> +
> +                parent_dir, target_name = os.path.split(targetdir)
> +
> +                # Start httpd on the target

This comment does not correspond to what the code right below it does.

> +                runner.run('/bin/tar -cmzf /tmp/fs.tgz -C %s %s' % (parent_dir, target_name))
> +                runner.run('cd /tmp')  # need to be in same dir as fs.tgz

> +
> +                url_base = self.start_http_server(runner)
> +                
> +                url = url_base + '/fs.tgz'
> +                logging.info("Fetching url: %s" % url)
> +                tf = download_with_retry(self.context, self.scratch_dir, url, False)
> +
> +                tfdir = os.path.join(self.scratch_dir, str(time.time()))
> +
> +                try:
> +                    os.mkdir(tfdir)
> +                    logging_system('/bin/tar -C %s -xzf %s' % (tfdir, tf))

replace logging_system with self.context.run_command

> +                    yield os.path.join(tfdir, target_name)
> +
> +                finally:
> +                    tf = os.path.join(self.scratch_dir, 'fs.tgz')
> +                    mk_targz(tf, tfdir)
> +                    rmtree(tfdir)
> +
> +                    # get the last 2 parts of tf, ie "scratchdir/tf.tgz"
> +                    tf = '/'.join(tf.split('/')[-2:])
> +                    runner.run('rm -rf %s' % targetdir)
> +                    self._target_extract(runner, tf, parent_dir)
> +
> +            finally:
> +                    self.stop_http_server(runner)
> +                    runner.run('umount /mnt')

Except for the code that starts and stops the http server, this
filesystem method is pretty much identical to the one in master.py, I
think it is one point where we could generalize.

> +
> +    def start_http_server(self, runner, port=80):
> +        # busybox produces no output to parse for, so let it run as a daemon
> +        runner.run('busybox httpd -v -p %s' % port)
> +        url_base = "http://%s:%s" % (self.master_ip, port)
> +        return url_base

I don't think we should use the standard http port here, because most
probably use cases will include installing a proper web server, and in
that case trying to start busybox on the default port will break things.

Try some high port that is unlikely to be used by another service ...
like ... 50888 for now. :)

> +
> +    def stop_http_server(self, runner):
> +        runner.run('killall busybox')

Not safe. I think it's better to just start busybox httpd in the
foreground and then send a control-C here.

> +    def _target_extract(self, runner, tar_file, dest, timeout=-1):
> +        tmpdir = self.context.config.lava_image_tmpdir
> +        url = self.context.config.lava_image_url
> +        tar_file = tar_file.replace(tmpdir, '')
> +        tar_url = '/'.join(u.strip('/') for u in [url, tar_file])
> +        self._target_extract_url(runner,tar_url,dest,timeout=timeout)
> +
> +    def _target_extract_url(self, runner, tar_url, dest, timeout=-1):
> +        decompression_cmd = ''
> +        if tar_url.endswith('.gz') or tar_url.endswith('.tgz'):
> +            decompression_cmd = '| /bin/gzip -dc'
> +        elif tar_url.endswith('.bz2'):
> +            decompression_cmd = '| /bin/bzip2 -dc'
> +        elif tar_url.endswith('.tar'):
> +            decompression_cmd = ''
> +        else:
> +            raise RuntimeError('bad file extension: %s' % tar_url)
> +
> +        runner.run('wget -O - %s %s | /bin/tar -C %s -xmf -'
> +            % (tar_url, decompression_cmd, dest),
> +            timeout=timeout)
> +	    
> +
> +    @contextlib.contextmanager
> +    def _as_master(self):
> +        self.bootcontrol.power_on_boot_master()
> +
> +        # Two reboots seem to be necessary to ensure that pxe boot is used.
> +        # Need to identify the cause and fix it
> +        self.proc.expect("Hit any key to stop autoboot:")
> +        self.proc.sendline('')
> +        self.bootcontrol.power_reset_boot_master()

Did you find out anything about this?

> +
> +        self.proc.expect("\(initramfs\)")
> +        self.proc.sendline('export PS1="%s"' % self.MASTER_PS1)
> +        self.proc.expect(self.MASTER_PS1_PATTERN, timeout=180, lava_no_logging=1)
> +        runner = HBMasterCommandRunner(self)
> +        runner.run(". /scripts/functions")
> +        ip_pat = '\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}'
> +        device = "eth0"
> +        runner.run("DEVICE=%s configure_networking" % device,
> +                   response='dns0     : (%s) ' % ip_pat, wait_prompt=False)

This regexp seems too fragile to me. You are matching against a specific
number of spaces ... it's probably better to use something like
'dns0\s*:\s*(%s) ' % ip_pat

> +        if runner.match_id != 0:
> +            msg = "Unable to determine dns address"
> +            logging.error(msg) 
> +            raise CriticalError(msg)
> +        dns = runner.match.group(1)
> +        logging.info("DNS Address is %s" % dns)
> +        runner.run("echo nameserver %s > /etc/resolv.conf" % dns)

Do we actually still need this DNS setup, by the way? Isn't everything
that's needed to be acessed acessed by IP now?

Less code = less bugs. :)

> +
> +        self.device_version = runner.get_device_version()
> +
> +        try:
> +            self.master_ip = runner.get_master_ip()
> +            yield runner
> +        finally:
> +           logging.debug("deploy done")
> +
> +	    
> +target_class = HighbankTarget
> +
> +
> +class HBMasterCommandRunner(NetworkCommandRunner):
> +    """A CommandRunner to use when the board is booted into the master image.
> +    """
> +    def __init__(self, target):
> +        super(HBMasterCommandRunner, self).__init__(
> +            target, target.MASTER_PS1_PATTERN, prompt_str_includes_rc=False)
> +
> +    def get_master_ip(self):
> +        logging.info("Waiting for network to come up")
> +        try:
> +            self.wait_network_up(timeout=20)
> +        except NetworkError:
> +            logging.exception("Unable to reach LAVA server")
> +            raise
> +
> +        ip_pat = '\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}'
> +        cmd = ("ifconfig %s | grep 'inet addr' | awk -F: '{print $2}' |"
> +                "awk '{print \"<\" $1 \">\"}'" %
> +                self._client.config.default_network_interface)
> +        self.run(cmd, response='(%s)' % ip_pat, wait_prompt=False)
> +        if self.match_id != 0:
> +            msg = "Unable to determine master image IP address"
> +            logging.error(msg)
> +            raise CriticalError(msg)
> +        ip = self.match.group(1)
> +        logging.debug("Master image IP is %s" % ip)
> +        return ip

This looks like pretty much the same code as the one in master.py. Can
we make HBMasterCommandRunner a subclass of MasterCommandRunner and
re-use that?

> +    def get_device_version(self):
> +        # To be re-implemented when master image is generated by linaro-image-tools
> +        device_version = "unknown"
> +        return device_version

Please make sure that some build number is included in the initrd so
that we can read it here as device version. You can include something as
simples as the following snippet in the initramfs-tools hooks file:

echo '#!/bin/sh' > /tmp/lava-master-version
echo "echo $(date +%Y.%m.%d)" > /tmp/lava-master-version
chmod +x /tmp/lava-master-version
copy_exec /tmp/lava-master-version /sbin

Then you can call lava-master-version on the target from this method and
return the version number printed.

> +    def run(self, cmd, response=None, timeout=-1, failok=False, wait_prompt=True):
> +        NetworkCommandRunner.run(self, cmd, response, timeout, failok, wait_prompt)
> +        rc = None
> +        if wait_prompt:
> +            match_id, match = self.match_id, self.match
> +            NetworkCommandRunner.run(self, "echo x$?x", response='x([0-9]+)x', timeout=5)
> +            if self.match_id != 0:
> +                raise OperationFailed("")
> +            else:
> +                rc = int(self.match.group(1))
> +                if not failok and rc != 0:
> +                    raise OperationFailed(
> +                        "executing %r failed with code %s" % (cmd, rc))
> +        return rc

Why do we need this extra handling for wait_prompt? the run() method in
the superclass already handles wait_prompt?

> +class IpmiPxeBoot(object):
> +    """
> +    This class provides a convenient object-oriented API that can be
> +    used to initiate power on/off and boot device selection for pxe
> +    and disk boot devices using ipmi commands.
> +    """
> +
> +    def __init__(self, host):
> +        self.ipmitool = IPMITool(host)
> +
> +    def power_on_boot_master(self):
> +        self.ipmitool.set_to_boot_from_pxe()
> +        self.ipmitool.power_on()
> +        self.ipmitool.reset()
> +
> +    def power_reset_boot_master(self):
> +        self.ipmitool.set_to_boot_from_pxe()
> +        self.ipmitool.reset()
> +
> +    def power_on_boot_image(self):
> +        self.ipmitool.set_to_boot_from_disk()
> +        self.ipmitool.power_on()
> +        self.ipmitool.reset()
> +
> +    def power_off(self):
> +        self.ipmitool.power_off()
> +
> +

This class should probably be moved to ipmi.py. AFAICT the logic will be
the same for any IPMI/PXE device we want to support, right?

> === added file 'lava_dispatcher/ipmi.py'
> --- lava_dispatcher/ipmi.py	1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/ipmi.py	2013-04-08 13:59:23 +0000
> @@ -0,0 +1,58 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Authors:
> +#   Antonio Terceiro <email address hidden>
> +#   Michael Hudson-Doyle <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>.
> +
> +from lava_dispatcher.utils import logging_system

This import will not be needed anymore.

> +class IPMITool(object):
> +    """
> +    This class wraps the ipmitool CLI to provide a convenient object-oriented
> +    API that can be composed into the implementation of devices that can be
> +    managed with IPMI.
> +    """
> +
> +    def __init__(self, host, ipmitool="ipmitool"):
> +        self.host = host
> +        self.ipmitool = ipmitool
> +
> +    def __ipmi(self, command):
> +        logging_system(
> +            "%s -H %s -U admin -P admin %s" % (
> +                self.ipmitool, self.host, command
> +            )
> +        )

This has to be changed to context.run_command. The class constructor
will have to take a context object and store it as self.context, so that
you can call self.context.run_command here instead of logging_system

> +
> +    def set_to_boot_from_disk(self):
> +        self.__ipmi("chassis bootdev disk")
> +
> +    def set_to_boot_from_pxe(self):
> +        self.__ipmi("chassis bootdev pxe")
> +
> +    def power_off(self):
> +        self.__ipmi("chassis power off")
> +
> +    def power_on(self):
> +        self.__ipmi("chassis power on")
> +
> +    def reset(self):
> +        self.__ipmi("chassis power reset")
> +
> +
> 


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