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

Looking good, just a few comments. Also, did you test this? Does it seem to work ok for you?

7 +from lava.client import OperationFailed
Imported but never used

12 + tester_str = "root@localhost:"
As you mentioned previously, we should probably define this elsewhere and just import it for consistency. OK for now, but let's do a patch later to clean this up everywhere, rather than doing it piecemeal.

13 print "abrek run %s" % test_name
Was this for debug, or information to the user? If it's for information about what's going on, I'd recommend being a bit more descriptive, rather than just outputting the command.

21 + self.client.run_shell_command(
22 + 'abrek run %s -o /lava/results/%s' % (test_name, test_name),
Let's give it a .json, or .bundle extension for the testoutput, or something like that. Just in case we ever decide to put anything else here, it would be nice to have an easy way to distinguish between files.

25 +class cmd_install_abrek(BaseAction):
26 + """
27 + abrek test tool deployment to test image rootfs by chroot
28 + may be placed in deploy.py, it can move later
I think abrek.py is a perfectly sensible place to put it.

30 + def run(self, tests):
31 + #Make sure in master image
32 + #, or exception can be caught and do boot_master_image()
This sounds like you were intending to catch the exception from in_master_shell() and correct for it if we are not in the master image, but you don't seem to do that.

42 + self.client.run_shell_command(
43 + 'cp -L /etc/resolv.conf /mnt/root/etc',
Please backup the original and restore it before unmounting.

45 + self.client.run_shell_command(
46 + 'cp -L /etc/apt/apt.conf.d/70debconf /mnt/root/etc/apt/apt.conf.d',
This shouldn't be needed, isn't that already in the image? It's there on the root.tgz I'm looking at.

48 + self.client.run_shell_command(
49 + 'chroot /mnt/root mount -t proc proc /proc',
is this needed? Possibly, just curious if you are doing it because you saw an error without it, or if you are just being extra careful.

56 + #ensure no libc6 config dialog popout when apt-get
57 + self.client.run_shell_command(
58 + 'chroot /mnt/root stop cron',
I can't see how this would be needed, or how it would help. Cron shouldn't be running at all, the image isn't even running! Also, we should be able to get around things like this another way, such as telling apt to be extra quiet. And the libc6 thing is just temporary, I don't expect we'll run into that frequently. This probably has more to do with the image you were testing, and the updates that are available right now.

64 + 'chroot /mnt/root apt-get -y install bzr',
Don't do this in separate steps, go ahead and install all the dependencies we need

79 + #Test if abrek installed
80 + self.client.run_shell_command(
81 + 'chroot /mnt/root abrek help',
82 + response = "list-tests")
Good idea to check this, but you may want to give it a better error, and shorten the timeout. We don't want to wait the default timeout, because this should happen easily within something shorter, say 10 seconds to be generous. Catch the exception if you get one, do any cleanup needed, and then raise a new exception with an error saying that abrek installation failed.

87 + self.client.run_shell_command(
88 + 'chroot /mnt/root umount /proc',
89 + response = master_str)
There's more cleanup you need to do here. You have a bind mount for /dev, and would probably be good to remove /abrek since it was just needed for the install, replace resolv.conf original file,