Code review comment for lp:~qzhang/lava-dispatcher/test-abrek
Revision history for this message
| Spring Zhang (qzhang) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
a60fb26
(Get the code!)
Thanks for the comments.
On Thu, 2011-03-17 at 20:51 +0000, Paul Larson wrote:
> Looking good, just a few comments. Also, did you test this? Does it seem to work ok for you?
Yes, I tested the sequence on mx51evk, it's ok except the char lost
issue I mentioned via USB-to-serial. Also I find the cron service issue
in the process.
>
> 7 +from lava.client import OperationFailed
> Imported but never used
I'll use it in abrek detection.
>
>
> 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.
Ok. Here I have a question, if I send a mp to mainline and it is
accepted, can I go on commit on the branch and send mp again?
>
> 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.
Delete it now.
>
> 21 + self.client.
> 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.
hmm, I'll look into it.
>
> 25 +class cmd_install_
> 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.
Change soon.
>
> 42 + self.client.
> 43 + 'cp -L /etc/resolv.conf /mnt/root/etc',
> Please backup the original and restore it before unmounting.
Done.
>
> 45 + self.client.
> 46 + 'cp -L /etc/apt/
> This shouldn't be needed, isn't that already in the image? It's there on the root.tgz I'm looking at.
Yes, it makes sure apt-get working in the same network environment like
master image besides /etc/resolv.conf, the configuration may contain the
proxy information, however, we use direct Internet connection or bzr
can't work either, so I will remove it here.
>
> 48 + self.client.
> 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.
No error message, I can remove it.
>
> 56 + #ensure no libc6 config dialog popout when apt-get
> 57 + self.client.
> 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.
I use an alpha version for master image to test, actually, cron is
running in master image, and no service run from test image, but after
chroot, it is still running and test image can see it, so I met the
error. If cron won't run in our master image finally, I'll remove it
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
I have merged the three one to one.
>
> 79 + #Test if abrek installed
> 80 + self.client.
> 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.
OK. I'll throw exception OperationFailed.
>
> 87 + self.client.
> 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,
For /dev bind mount, it doesn't help to eliminate the warning message
"Can not write log, openpty() failed (/dev/pts not mounted?), does not
work" after I tried. Do you have some other way? Or we can ignore it for
all seems working.
>
--
Best wishes,
Spring Zhang