Code review comment for lp:~pwlars/lava-dispatcher/move-network-checks

- self.wait_for_network()
+ if self.client.wait_network_up() is False:
+ print "Failed to bring up network on master image"
+ return False

You're changing cmd_deploy_linaro_image.run() to return False, but not other cmd_*.run() returns anything; I think all cmd_*.run() should implement the same interface

+ master_str = 'root@master:'

right, I also included this fix in http://bazaar.launchpad.net/~lool/lava/misc-cleanups/revision/10

Otherwise, the move looks good

Some comments about the expect ping test:
* I'm not entirely comfortable with using expect; I'm not sure how we can make sure the ping process has been created; of course ping is relatively harmless, but it seems this could result in 20 ping commands being queued; do we need to pass some timeout?
* default ping timeout seems to be relatively random and might be high; I tried ping -c1 1.0.0.1 and it takes 20 seconds to timeout here
* maybe testing whether there's a default route up is enough/better?
* parsing the ping textual output makes me think that LC_ALL=C should be set in the environment in the case where the output gets translated (apparently it isn't translated on my system which French locales, but still sounds like a good idea)
* it seems pexpect has a 30 seconds timeout and the other outputs aren't tested so that even if ping times out quickly, we have to wait for a 30s expect timeout before retrying a ping
* "64 bytes" sounds like a ping implementation detail; perhaps "bytes from" would be less risky, or "1 received"?

Anyway, the implementation is probably just fine at this stage; just dumping my thoughts :-)