Code review comment for lp:~qzhang/lava-dispatcher/gen-test-tarball

On Tue, Mar 01, 2011, Spring Zhang wrote:
> --- lava-dispatch 2011-03-01 03:56:20 +0000
> +++ lava-dispatch 2011-03-01 11:48:59 +0000
> - cmd.get('parameters', {})
> + params = cmd.get('parameters', {})

 Wups, thanks!

> step = lava_commands[cmd['command']](target)
> step.run(**params)
>
> === modified file 'lava/actions/deploy.py'
> --- lava/actions/deploy.py 2011-02-26 14:41:09 +0000
> +++ lava/actions/deploy.py 2011-03-01 11:48:59 +0000
> @@ -1,33 +1,118 @@
> #!/usr/bin/python
> from lava.actions import BaseAction
> +from lava.client import LavaClient
> import time
> +from subprocess import PIPE
> +from subprocess import Popen
> +import shutil
> +import shlex
>
> class cmd_deploy_linaro_image(BaseAction):
> + #board type: [bootfs partition index, rootfs partition index]
> + BOARD_PARTS = {
> + "panda": [1, 2],
> + "beagle": [1, 2],
> + "vexpress": [1, 2],
> + "mx51evk": [2, 3],

 It's a bit sad to hardcode this

 The immediate thing I would have proposed would be to have a config
 file, but in reality I'm thinking we want to work towards not knowing
 the partition layout at all.

> + #use l-m-c to install hwpack and rootfs to qemu image
> + #assume root permission
> + hwpack_name = hwpack.rsplit("/", 1)[1]
> + rootfs_name = rootfs.rsplit("/", 1)[1]
> + qemuimg = "%s_sd.img" % boardtype
> + lmc_cmd = "linaro-media-create --rootfs ext3 --image_file %s" \
> + " --dev %s --binary %s --hwpack %s" % (qemuimg, boardtype,
> + rootfs_name, hwpack_name)

 Hmm why do you actually need to pass just the filename to l-m-c? Just
 pass hwpack_name and rootfs_name directly?

> + print "l-m-c command: %s" % lmc_cmd
> +
> + #TODO: should import l-m-c to use python way,
> + # and l-m-c supports no keyboard-input
> + """
> + proc = Popen(shlex.split(lmc_cmd), stdin=PIPE, stdout=PIPE,
> + stderr=PIPE)
> + output, rc = proc.communicate()

 You don't seem to use the output at all; use proc.wait() instead? if
 you don't want to see it on the console, open /dev/null and pass that
 as stdout / stderr.

> + if proc.returncode != 0:
> + print "l-m-c fails"
> + raise OperationFailed()

 Would be good to return a message in OperationFailed():
     if proc.returncode != 0:
         raise OperationFailed(
             "linaro-media-create failed with %s" % proc.returncode)

> + #package qemu image content to tarballs
> + #an ugly tar, need to find a way to create tar in relative path,
> + #maybe tarfile
> + qemu_img_cmds = [ "modprobe nbd max_part=8",
> + "qemu-nbd --connect=/dev/nbd0 %s" % qemuimg,

 Do we need to modprovbe nbd explicitly?

> + "mkdir -p /mnt/bootfs",
> + "sleep 2",

 a comment on why the sleep is needed would be nice

> + "mount /dev/nbd0p%s /mnt/bootfs" % self.BOARD_PARTS[boardtype][0],
> + "cd /mnt/bootfs",
> + "tar czf ~/%s ." % hwpack_name,

 tar -C to change directory; e.g. tar -C /mnt/bootfs czf foo.tgz .

 /mnt/bootfs sounds bad; you probably want to create a tmpdir instead

 ~/%s sounds bad, you probably want to use a tmp file instead

> + "cd -",
> + "umount /mnt/bootfs",
> + "mkdir -p /mnt/rootfs",
> + "mount /dev/nbd0p%s /mnt/rootfs" % self.BOARD_PARTS[boardtype][0],
> + "cd /mnt/rootfs",
> + "tar czf ~/%s ." % rootfs_name,
> + "cd -",
> + "umount /mnt/rootfs",

 ditto for /mnt/rootfs, cd/-C, and ~/%s.

> + proc = Popen(shlex.split(cmd), stdin=PIPE, stdout=PIPE,
> + stderr=PIPE)
> + output, err = proc.communicate()

 again, output unused

> + if proc.returncode != 0:
> + print "Failed at command: %s" % cmd
> + """
> + Do more operations
> + """
> + raise OperationFailed()

 and should include a message in the exception when possible

> + #move the tarball to one place of validation farm server
> + #now it's server:/var/www/testimg
> + shutil.copy("~/%s" % hwpack_name, "/var/www/testimg")
> + shutil.copy("~/%s" % rootfs_name, "/var/www/testimg")

 set /var/www/testimg in a variable at the top?
 e.g. WWW_DEPLOY_DIR =

 do you actually want to copy, or would moving the tarball be ok?

 I think the rootfs pathname is going ot be the same for all boards,
 which means it will clash?

> + #change hwpack and rootfs link to ready testimg,
> + #however, hwpack and rootfs are not class member now
> + hwpack = "http://pkgserver/testimg/%s" % hwpack_name
> + rootfs = "http://pkgserver/testimg/%s" % rootfs_name

 "http://pkgserver/testimg/" sounds like it should be a constant too

> def deploy_linaro_bootfs(self, bootfs):
> - master_str = 'root@master:'
> self.client.run_shell_command(
> 'mkfs.vfat /dev/disk/by-label/testboot -n testboot',
> response = master_str)

 did you delete master_str intentionally?

 maybe it's just a missing merge

> +if __name__ == "__main__":
> + cmd = cmd_deploy_linaro_image("bbg01")
> + cmd.generate_tarballs("mx51evk", "http://pkgserver/original/hwpack_linaro-imx51_20110210-0_armel_unsupported.tar.gz", "http://pkgserver/original/linaro-natty-headless-tar-20110216-0.tar.gz")

 lots of hardcoding here

> + "bbg":["mmc init",
> + "setenv bootcmd 'fatload mmc 0:5 0x90800000 uImage; fatload mmc " \
> + "0:5 0x90800000 uInitrd; bootm 0x90000000 0x90800000'",
> + "setenv bootargs ' console=tty0 console=ttymxc0,115200n8 " \
> + "root=LABEL=testrootfs rootwait ro'",
> + "boot"],
> }

 of course it's very sad that we have to use this, but I understand why
 you have it for now

 I'm thinking the board specific data and the server specific data could
 be isolated in a specific file with a lot of comments on what each
 constant is for

--
Loïc Minier