Adding kvm_subprocess

Adding kvm_subprocess

[lookkas]

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.

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.

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?

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?

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?

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

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: Adding kvm_subprocess

[Michael Goldish]

BTW, thank you for the thorough review.
I realize that was quite a lot of code to go over.

----- Original Message -----
From: "Michael Goldish" <mgoldish@redhat.com>
To: lookkas@gmail.com, "michael goldish" <mgoldish@redhat.com>, autotest@test.kernel.org, kvm@vger.kernel.org
Sent: Thursday, June 18, 2009 2:58:46 PM (GMT+0200) Auto-Detected
Subject: Re: Adding kvm_subprocess


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