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
Done

> > > 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.
But I think I addressed this already by setting the ping timeout to 4 seconds and the expect timeout to 5. So if it passes check_network_up(), then it will come back right away most likely, and go through without another ping command being sent. If it fails the ping, it won't return from check_network_up() for 5 seconds, after which another one will be sent. I don't see where command stacking could occur here.

> 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
Yeah, I think we'll have a cleaner way to do this later. Using expect is a necessary evil of relying on the serial port for access to the device.

> I see you added a _find_default_nic(); you probably want ip route show
oops, that was a note to myself for something I wanted to do after I have this merged, and was clearly not intended to be something we want to include right now.