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

On Tue, Mar 01, 2011, Paul Larson wrote:
> 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?

 Raising an exception seems better here, yeah

> > 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

 What I mean is that if you expect timeout is 2 seconds and your ping
 times out only after 10 seconds, then that loop in wait_network_up()
 might launch 5 pings (since expect will timeout before the ping does)
 before the first one finally returns, and then your console will be
 running these other pings when you are trying to run another command
 etc.

 So an expect timeout should be a fatal error.

 Ideally, we'd switch to something more solid than parsing the output of
 the console to decide whether something is finished or not; that might
 be for later though

> > * 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

 Looks good; thanks

> > * "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.

 I was actually thinking "Heck, why don't we just test the ping exit
 code" which would indeed do the right thing from my quick local
 testing -- but we're parsing the console output, so ain't so easy. I
 guess we could:
  ping -q -W4 -c1 1.0.0.1 >/dev/null 2>&1 && echo worked || echo failed

 Anyway, it's probably ok to match "0 received" or "1 received" for now

 I see you added a _find_default_nic(); you probably want ip route show
 if you're looking for the default route (ip route show exact 0/0) or ip
 route get to 192.168.0.254 if you want the route to a specific host
 (such as the gateway)

--
Loïc Minier