Re: [Autotest] Adding kvm_subprocess

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

On Thu, 2009-06-18 at 07:58 -0400, Michael Goldish wrote:
> ----- lookkas@gmail.com wrote:
> 
> > Reviewers: lmr,
> > 
> > Message:
> > Hi Michael, this is my first review of your patch series. The module
> > kvm_subprocess looks pretty good and carefully written, I made some
> > minor comments (some of them are more of general wonderings).
> > 
> > After reviewing this, I began to think seriously about replacing
> > pexpect by this library.
> 
> Note that pexpect has features that kvm_subprocess doesn't -- if I remember
> correctly it can log the output to a user specified text file, and uses
> an internal buffer that may allow for some more flexibility in reading from
> the STDOUT/STDERR pipe, and maybe other things.
> The KVM test doesn't need these things, but if you're seriously considering
> using kvm_subprocess outside the KVM test, make sure you don't need these
> things either.

That's precisely the point, pexpect and pxssh are external gpl modules
that we bundled to be able to use a python library to do interaction
with interactive programs. For most of the tests we don't need the fancy
stuff of pexpect, so it'd be nice to have a solution grown inside
autotest.

I will see if I can write a ssh subclass of kvm_spawn, make the
necessary arrangements and verify if everything is working OK.

> > Thank you very much for your work on this,
> > 
> > 
> > http://codereview.appspot.com/79042/diff/1/4
> > File client/tests/kvm/kvm_subprocess.py (right):
> > 
> > http://codereview.appspot.com/79042/diff/1/4#newcode142
> > Line 142: except:
> > We probably want to catch AssertionError and OsError here.
> 
> I agree.
> 
> (It seemed harmless to catch all exceptions there because setting those
> descriptors to None eventually leads to failure, but still I agree it's
> safer to catch only the expected ones.)

Yes, I also agree that it is fairly harmless. Pointed out just for the
sake of the review :)

> > http://codereview.appspot.com/79042/diff/1/4#newcode215
> > Line 215: """
> > Would it be possible to distinguish stdout and stderr when reading
> > from the process?
> 
> I don't think so.
> In what cases do we want to distinguish stdout and stderr?
> Does pexpect make the distinction?

It's usually nice to tell the difference between the two of them for
debugging purposes, but nothing I'd consider critical. pexpect uses a
similar approach (non blocking read using select), and I realize it
doesn't.

> > http://codereview.appspot.com/79042/diff/1/4#newcode238
> > Line 238: pass
> > The function sends by default SIGTERM to the process. In such cases,
> > what do we do with misbehaving (hang) processes? Just leave it as it
> > is
> > and close other file descriptors? Wouldn't be interesting to send a
> > SIGKILL if another signal doesn't work?
> 
> I wanted to keep close() simple (unlike VM.destroy()).
> I can add a timeout however and a parameter to control whether SIGKILL
> should be sent automatically to misbehaving processes.

That sounds good, and I don't think it will over complicate the
function.

> BTW, I wonder if we should add something like a 'close_command' parameter,
> which will specify a command to send before closing the process.
> In the case of SSH/Telnet this should be "exit", and in the case of QEMU
> it should be disabled.
> If we do that, however, I'd rather do it in a future patch, because I want
> it to undergo some testing.

No rush, let's keep the whole module under testing a little bit more.

> > http://codereview.appspot.com/79042/diff/1/4#newcode279
> > Line 279: return True
> > Isn't risky to just return True if we can't find the PID under /proc?
> 
> No, because the os.kill(pid, 0) test makes sure the PID exists.
> The second test is there just to make sure the PID doesn't belong to the
> wrong process.  If for some reason we can't find the PID under /proc, I
> think we can safely say we passed the second test.
> 
> It's OK to return False if we can't find the PID under /proc, but only
> if we can guarantee that all PIDs are listed under /proc as soon as they
> come into existence.
> In other words, we need to know for sure that the equality
> `os.kill(PID, 0) succeeds` == `PID exists under proc`
> is true at all times, atomically
> (particularly, that no side of the equality becomes true before the other).
> Since I wasn't sure about that, I preferred to return True, which is
> harmless anyway.

Ok, fair enough! Other than the very minor stuff we've talked about, we
are good to go.

-- 
Lucas Meneghel Rodrigues
Software Engineer (QE)
Red Hat - Emerging Technologies