Re: [Autotest] Adding kvm_subprocess

Re: [Autotest] Adding kvm_subprocess

[Lucas Meneghel Rodrigues]

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

Re: [Autotest] Adding kvm_subprocess

[Michael Goldish]

----- "Lucas Meneghel Rodrigues" <lmr@redhat.com> wrote:

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

kvm_subprocess is a little weird in that it does two different things --
handling of both non-interactive subprocesses and SSH sessions.
With this approach I don't think we need to write an SSH subclass of
kvm_spawn because it already does a lot of SSH stuff.
If we do any subclassing at all -- I suggest that we remove all the SSH
stuff from kvm_spawn and put it in a subclass somehow, so that kvm_spawn
natively only handles non-interactive subprocess (with _tail(),
get_output(), get_status() etc), and the subclass does everything else
(read_up_to_prompt(), get_command_status_output() etc).  ssh_login()
can remain an external function that creates and returns a kvm_spawn
object.
This will have to be done carefully because each 'user' of the kvm_spawn
server needs a named pipe of its own, which will have to be handled by
the constructor.
If you think this is a good idea I'd rather make the necessary changes
to kvm_subprocess myself.
Does this make sense to you, or did I misunderstand what you meant by
writing an SSH subclass?

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

OK.
I try to test it as much as possible by using it exclusively instead of
the previous run_bg() and kvm_spawn.

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

Re: [Autotest] Adding kvm_subprocess

[Lucas Meneghel Rodrigues]

Hi Michael,

On Thu, 2009-06-18 at 11:27 -0400, Michael Goldish wrote:
> kvm_subprocess is a little weird in that it does two different things --
> handling of both non-interactive subprocesses and SSH sessions.
> With this approach I don't think we need to write an SSH subclass of
> kvm_spawn because it already does a lot of SSH stuff.

Fair enough. I just happen to like the approach took by pexpect folks to
separate SSH implementation on a subclass, because the generic problem
itself is 'interacting with interactive programs on a programatic way',
and handling SSH connections is a subset of this problem.

> If we do any subclassing at all -- I suggest that we remove all the SSH
> stuff from kvm_spawn and put it in a subclass somehow, so that kvm_spawn
> natively only handles non-interactive subprocess (with _tail(),
> get_output(), get_status() etc), and the subclass does everything else
> (read_up_to_prompt(), get_command_status_output() etc).  ssh_login()
> can remain an external function that creates and returns a kvm_spawn
> object.
> This will have to be done carefully because each 'user' of the kvm_spawn
> server needs a named pipe of its own, which will have to be handled by
> the constructor.
> If you think this is a good idea I'd rather make the necessary changes
> to kvm_subprocess myself.

Sounds good to me, I am totally OK with it.

> Does this make sense to you, or did I misunderstand what you meant by
> writing an SSH subclass?

No, you've nailed it. Sorry for the delay in answering.