LAVA Dispatcher

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

gen-test-tarball
Merge into trunk

Revision history for this message

Spring Zhang (qzhang) wrote on 2011-03-01:

Thanks for the review.

On Tue, 2011-03-01 at 12:19 +0000, Loïc Minier wrote:
> 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.
>
Agree.
> > + #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?
>
Initial hwpack or rootfs is an URL, it will get by the previous function
to the local, and here l-m-c uses the hwpack/rootfs name.
> > + 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.
>
Do we need to log if there is some error to check?
> > + 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)
>
Agree, some exceptions we need to define well.
> > + #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
>
ok
> > + "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
>
ok
> > + "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 =
>
good, and we can include it in some config file you mentioned above
> do you actually want to copy, or would moving the tarball be ok?
>
All the operations is better to do in a temp directory, so copy or move
is ok, the temp directory will be deleted.
> I think the rootfs pathname is going ot be the same for all boards,
> which means it will clash?
So much hardcode
>
> > + #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
>
Sorry for another hardcode
> > 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?
>
Sorry, I miss it when I merge from the base of lp:lava
> 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
It's for a test.
>
> > + "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
>

Thanks for the review.

On Tue, 2011-03-01 at 12:19 +0000, Loïc Minier wrote:
> 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.
> 
Agree.
> > +        #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?
> 
Initial hwpack or rootfs is an URL, it will get by the previous function
to the local, and here l-m-c uses the hwpack/rootfs name.
> > +        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.
> 
Do we need to log if there is some error to check?
> > +        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)
> 
Agree, some exceptions we need to define well.
> > +        #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
> 
ok
> > +                "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
> 
ok
> > +                "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 =
> 
good, and we can include it in some config file you mentioned above
>  do you actually want to copy, or would moving the tarball be ok?
> 
All the operations is better to do in a temp directory, so copy or move
is ok, the temp directory will be deleted.
>  I think the rootfs pathname is going ot be the same for all boards,
>  which means it will clash?
So much hardcode
> 
> > +        #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
> 
Sorry for another hardcode
> >      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?
> 
Sorry, I miss it when I merge from the base of lp:lava
>  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
It's for a test.
> 
> > +    "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
>

« Back to merge proposal