Re: Adding kvm_subprocess

Re: Adding kvm_subprocess

[Michael Goldish]

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

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

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

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

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.

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

> Description:
> The following patches replace the run_bg() function and the kvm_spawn
> class
> with kvm_subprocess. The new module is intended to solve a problem
> with
> run_bg() (which loses track of the child process when the parent
> process
> exits) and allows for more flexibility in handling SSH/Telnet
> sessions
> (allows
> reusing sessions started in previous tests).
> 
> kvm_subprocess defines a class 'kvm_spawn' and two functions:
> run_bg()
> and
> run_fg(). Its main job is to run a process in the background and
> allow
> the
> user to control it interactively and/or monitor its output on the
> fly.
> Its most
> important feature in the context of KVM tests is that it allows to
> control
> child processes left behind by processes that no longer exist. This
> means that
> if QEMU is started by the 'boot' test, its output will be logged in
> the
> following 'reboot' test as well.
> 
> See kvm_spawn's docstring for more details.
> 
> Please review this at http://codereview.appspot.com/79042
> 
> Affected files:
>    M     client/tests/kvm/kvm_preprocessing.py
>    A     client/tests/kvm/kvm_subprocess.py
>    M     client/tests/kvm/kvm_tests.py
>    M     client/tests/kvm/kvm_utils.py
>    M     client/tests/kvm/kvm_vm.py
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.