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

> 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
Good point, although some of these run() commands are not yet implemented, so I'm not too concerned with those just yet. Another thing I had considered was raising an exception instead. Do you have a preference?

> 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?
We do, in wait_network_up, which calls check_network_up

> * 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
Adjusted ping timeout to 4 seconds, timeout the expect command for 5

> * maybe testing whether there's a default route up is enough/better?
We can do that, but it's an extra check and we really need to know if we can get to the control server.

> * parsing the ping textual output makes me think that LC_ALL=C should be set
Added

> * 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
I think this was addressed previously.

> * "64 bytes" sounds like a ping implementation detail; perhaps "bytes from"
> would be less risky, or "1 received"?
Changed to have it look for 1 received, or 0 received. I think that should cover everything unless there's some strange failure case that doesn't mention 0 received.