Re: [KVM-AUTOTEST,01/17] Add new module kvm_subprocess

Re: [KVM-AUTOTEST,01/17] Add new module kvm_subprocess

[Cao, Chen]

Hi, Michael,

I found that if the sessions initialized using kvm_subprcoess are not closed,
the processes will never exit, and /tmp/kvm_spawn will be filled with the
temporary files.

And we can find in the code,
# kvm_subprocess.py
...
        # Read from child and write to files/pipes
        while True:
            check_termination = False
            # Make a list of reader pipes whose buffers are not empty
            fds = [fd for (i, fd) in enumerate(reader_fds) if buffers[i]]
            # Wait until there's something to do
            r, w, x = select.select([shell_fd, inpipe_fd], fds, [], 0.5)
            # If a reader pipe is ready for writing --
            for (i, fd) in enumerate(reader_fds):
                if fd in w:
                    bytes_written = os.write(fd, buffers[i])
                    buffers[i] = buffers[i][bytes_written:]
            # If there's data to read from the child process --
            if shell_fd in r:
                try:
                    data = os.read(shell_fd, 16384)
                except OSError:
                    data = ""
                if not data:
                    check_termination = True
                # Remove carriage returns from the data -- they often cause
                # trouble and are normally not needed
                data = data.replace("\r", "")
                output_file.write(data)
                output_file.flush()
                for i in range(len(readers)):
                    buffers[i] += data
            # If os.read() raised an exception or there was nothing to read --
            if check_termination or shell_fd not in r:
                pid, status = os.waitpid(shell_pid, os.WNOHANG)
                if pid:
                    status = os.WEXITSTATUS(status)
                    break
            # If there's data to read from the client --
            if inpipe_fd in r:
                data = os.read(inpipe_fd, 1024)
                os.write(shell_fd, data)
...

that if session.close() is not called, we will loop in the 'while' forever.

So, user have to make sure that unnecessary sessions are all killed,
otherwise, running some testcase(s) for huge number of times will suck
out all the system resource, which I think is very inconvenient.
Especially when we have to take care of many exceptions that may be raised
by our program.

e.g.

...
session = kvm_test_utils.wait_for_login(vm)
...
session2 = kvm_test_utils.wait_for_login(vm_x)
...
try:
    ...
except ...:
    ...
...
(other code may raise exceptions)
...
try:
    ...
except ...:
    ...
...
try:
    ...
except ...:
    ...
...

cleaning up the sessions will be exhausting here.

Do we have a good (or better) way to handle this?
Thanks.


Regards,

Cao, Chen
2009-10-12

On Mon, Jul 20, 2009 at 12:07 PM, Michael Goldish<mgoldish@redhat.com>
wrote:
> This module is intended to be used for controlling all child
> processes in KVM
> tests: both QEMU processes and SSH/SCP/Telnet processes. Processes
> started with
> this module keep running and can be interacted with even after the
> parent
> process exits.
>
> The current run_bg() utility tracks a child process as long as the
> parent
> process is running. When the parent process exits, the tracking
> thread
> terminates and cannot resume when needed.
>
> Currently SSH/SCP/Telnet communication is handled by
> kvm_utils.kvm_spawn, which
> does not allow the child process to run after the parent process
> exits. Thus,
> open SSH/SCP/Telnet sessions cannot be reused by tests following the
> one in
> which they are opened.
>
> The new module provides a solution to these two problems, and also
> saves some
> code by reusing common code required both for QEMU processes and
> SSH/SCP/Telnet
> processes.
>
> Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> ---
>  client/tests/kvm/kvm_subprocess.py | 1146
>  ++++++++++++++++++++++++++++++++++++
>  1 files changed, 1146 insertions(+), 0 deletions(-)
>  create mode 100644 client/tests/kvm/kvm_subprocess.py
>

Re: [KVM-AUTOTEST,01/17] Add new module kvm_subprocess

[Michael Goldish]

You're right, currently the sessions must be closed explicitly.
This is due to the fact that both qemu and ssh/telnet are handled by the
same code, and qemu has to keep running in the background if we want to
pass it from one test to another.

To deal with this, first, we should use try..finally blocks to close all
sessions in tests.  As far as I know all existing tests (or at least most
of them) already do this.

Second, we can add a destructor function to kvm_shell_session that will
close the session automatically when it's no longer referenced.  At the
moment this won't work because there's a thread running in the background
tracking output from the session, but this thread is usually not needed
for ssh/telnet (it's needed mainly for qemu), so we can selectively get
rid of it and allow the reference count to drop to zero when the test exits,
thus allowing the destructor to be called.

I'll think of a way to do the second thing, and if it works, maybe we won't
need the first.  But for now every test should close its sessions explicitly.

BTW, I'm not sure I understand why cleaning up the sessions should be
exhausting in the case you presented.  You can just wrap everything in one
big try..finally block:

session = ...

try:
  try:
  except:
  try:
  except:
  ...
finally:
  session.close()

----- Original Message -----
From: "Chen Cao" <kcao@redhat.com>
To: "Michael Goldish" <mgoldish@redhat.com>
Cc: autotest@test.kernel.org, kvm@vger.kernel.org
Sent: Monday, October 12, 2009 8:55:59 AM (GMT+0200) Auto-Detected
Subject: Re: [KVM-AUTOTEST,01/17] Add new module kvm_subprocess


Hi, Michael,

I found that if the sessions initialized using kvm_subprcoess are not closed,
the processes will never exit, and /tmp/kvm_spawn will be filled with the
temporary files.

And we can find in the code,
# kvm_subprocess.py
...
        # Read from child and write to files/pipes
        while True:
            check_termination = False
            # Make a list of reader pipes whose buffers are not empty
            fds = [fd for (i, fd) in enumerate(reader_fds) if buffers[i]]
            # Wait until there's something to do
            r, w, x = select.select([shell_fd, inpipe_fd], fds, [], 0.5)
            # If a reader pipe is ready for writing --
            for (i, fd) in enumerate(reader_fds):
                if fd in w:
                    bytes_written = os.write(fd, buffers[i])
                    buffers[i] = buffers[i][bytes_written:]
            # If there's data to read from the child process --
            if shell_fd in r:
                try:
                    data = os.read(shell_fd, 16384)
                except OSError:
                    data = ""
                if not data:
                    check_termination = True
                # Remove carriage returns from the data -- they often cause
                # trouble and are normally not needed
                data = data.replace("\r", "")
                output_file.write(data)
                output_file.flush()
                for i in range(len(readers)):
                    buffers[i] += data
            # If os.read() raised an exception or there was nothing to read --
            if check_termination or shell_fd not in r:
                pid, status = os.waitpid(shell_pid, os.WNOHANG)
                if pid:
                    status = os.WEXITSTATUS(status)
                    break
            # If there's data to read from the client --
            if inpipe_fd in r:
                data = os.read(inpipe_fd, 1024)
                os.write(shell_fd, data)
...

that if session.close() is not called, we will loop in the 'while' forever.

So, user have to make sure that unnecessary sessions are all killed,
otherwise, running some testcase(s) for huge number of times will suck
out all the system resource, which I think is very inconvenient.
Especially when we have to take care of many exceptions that may be raised
by our program.

e.g.

...
session = kvm_test_utils.wait_for_login(vm)
...
session2 = kvm_test_utils.wait_for_login(vm_x)
...
try:
    ...
except ...:
    ...
...
(other code may raise exceptions)
...
try:
    ...
except ...:
    ...
...
try:
    ...
except ...:
    ...
...

cleaning up the sessions will be exhausting here.

Do we have a good (or better) way to handle this?
Thanks.


Regards,

Cao, Chen
2009-10-12

On Mon, Jul 20, 2009 at 12:07 PM, Michael Goldish<mgoldish@redhat.com>
wrote:
> This module is intended to be used for controlling all child
> processes in KVM
> tests: both QEMU processes and SSH/SCP/Telnet processes. Processes
> started with
> this module keep running and can be interacted with even after the
> parent
> process exits.
>
> The current run_bg() utility tracks a child process as long as the
> parent
> process is running. When the parent process exits, the tracking
> thread
> terminates and cannot resume when needed.
>
> Currently SSH/SCP/Telnet communication is handled by
> kvm_utils.kvm_spawn, which
> does not allow the child process to run after the parent process
> exits. Thus,
> open SSH/SCP/Telnet sessions cannot be reused by tests following the
> one in
> which they are opened.
>
> The new module provides a solution to these two problems, and also
> saves some
> code by reusing common code required both for QEMU processes and
> SSH/SCP/Telnet
> processes.
>
> Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> ---
>  client/tests/kvm/kvm_subprocess.py | 1146
>  ++++++++++++++++++++++++++++++++++++
>  1 files changed, 1146 insertions(+), 0 deletions(-)
>  create mode 100644 client/tests/kvm/kvm_subprocess.py
>

Re: [KVM-AUTOTEST,01/17] Add new module kvm_subprocess

[Cao, Chen]

On Mon, Oct 12, 2009 at 09:07:45AM -0400, Michael Goldish wrote:
> You're right, currently the sessions must be closed explicitly.
> This is due to the fact that both qemu and ssh/telnet are handled by the
> same code, and qemu has to keep running in the background if we want to
> pass it from one test to another.
> 
> To deal with this, first, we should use try..finally blocks to close all
> sessions in tests.  As far as I know all existing tests (or at least most
> of them) already do this.
> 
> Second, we can add a destructor function to kvm_shell_session that will
> close the session automatically when it's no longer referenced.  At the
> moment this won't work because there's a thread running in the background
> tracking output from the session, but this thread is usually not needed
> for ssh/telnet (it's needed mainly for qemu), so we can selectively get
> rid of it and allow the reference count to drop to zero when the test exits,
> thus allowing the destructor to be called.
> 
> I'll think of a way to do the second thing, and if it works, maybe we won't
> need the first.  But for now every test should close its sessions explicitly.
> 
> BTW, I'm not sure I understand why cleaning up the sessions should be
> exhausting in the case you presented.  You can just wrap everything in one
> big try..finally block:
> 
> session = ...
> 
> try:
>   try:
>   except:
>   try:
>   except:
>   ...
> finally:
>   session.close()
> 

Thanks for your explanation.

It is just boring and error-prone to add lots of
'(dst|src|tmp|etc)*sesson.close()' to our code (the internal version)
into different files and big number of functions. and some of the
'sessions' are in the try...except blocks, and some are not.

We have to make sure where we started the sessions and to close all of
them when they are not needed any longer. I feel it's a little weird
that we have to do the garbage-collection-like work while using python.

so, since this is a known issue, or precisely, limitation of 'ease of
use', I'm looking forward to your impovement, and I think, we will
also try to work it out at the same time.

Thanks again for your help.


Cao, Chen
2009-10-13

> ----- Original Message -----
> From: "Chen Cao" <kcao@redhat.com>
> To: "Michael Goldish" <mgoldish@redhat.com>
> Cc: autotest@test.kernel.org, kvm@vger.kernel.org
> Sent: Monday, October 12, 2009 8:55:59 AM (GMT+0200) Auto-Detected
> Subject: Re: [KVM-AUTOTEST,01/17] Add new module kvm_subprocess
> 
> 
> Hi, Michael,
> 
> I found that if the sessions initialized using kvm_subprcoess are not closed,
> the processes will never exit, and /tmp/kvm_spawn will be filled with the
> temporary files.
> 
> And we can find in the code,
> # kvm_subprocess.py
> ...
>         # Read from child and write to files/pipes
>         while True:
>             check_termination = False
>             # Make a list of reader pipes whose buffers are not empty
>             fds = [fd for (i, fd) in enumerate(reader_fds) if buffers[i]]
>             # Wait until there's something to do
>             r, w, x = select.select([shell_fd, inpipe_fd], fds, [], 0.5)
>             # If a reader pipe is ready for writing --
>             for (i, fd) in enumerate(reader_fds):
>                 if fd in w:
>                     bytes_written = os.write(fd, buffers[i])
>                     buffers[i] = buffers[i][bytes_written:]
>             # If there's data to read from the child process --
>             if shell_fd in r:
>                 try:
>                     data = os.read(shell_fd, 16384)
>                 except OSError:
>                     data = ""
>                 if not data:
>                     check_termination = True
>                 # Remove carriage returns from the data -- they often cause
>                 # trouble and are normally not needed
>                 data = data.replace("\r", "")
>                 output_file.write(data)
>                 output_file.flush()
>                 for i in range(len(readers)):
>                     buffers[i] += data
>             # If os.read() raised an exception or there was nothing to read --
>             if check_termination or shell_fd not in r:
>                 pid, status = os.waitpid(shell_pid, os.WNOHANG)
>                 if pid:
>                     status = os.WEXITSTATUS(status)
>                     break
>             # If there's data to read from the client --
>             if inpipe_fd in r:
>                 data = os.read(inpipe_fd, 1024)
>                 os.write(shell_fd, data)
> ...
> 
> that if session.close() is not called, we will loop in the 'while' forever.
> 
> So, user have to make sure that unnecessary sessions are all killed,
> otherwise, running some testcase(s) for huge number of times will suck
> out all the system resource, which I think is very inconvenient.
> Especially when we have to take care of many exceptions that may be raised
> by our program.
> 
> e.g.
> 
> ...
> session = kvm_test_utils.wait_for_login(vm)
> ...
> session2 = kvm_test_utils.wait_for_login(vm_x)
> ...
> try:
>     ...
> except ...:
>     ...
> ...
> (other code may raise exceptions)
> ...
> try:
>     ...
> except ...:
>     ...
> ...
> try:
>     ...
> except ...:
>     ...
> ...
> 
> cleaning up the sessions will be exhausting here.
> 
> Do we have a good (or better) way to handle this?
> Thanks.
> 
> 
> Regards,
> 
> Cao, Chen
> 2009-10-12
> 
> On Mon, Jul 20, 2009 at 12:07 PM, Michael Goldish<mgoldish@redhat.com>
> wrote:
> > This module is intended to be used for controlling all child
> > processes in KVM
> > tests: both QEMU processes and SSH/SCP/Telnet processes. Processes
> > started with
> > this module keep running and can be interacted with even after the
> > parent
> > process exits.
> >
> > The current run_bg() utility tracks a child process as long as the
> > parent
> > process is running. When the parent process exits, the tracking
> > thread
> > terminates and cannot resume when needed.
> >
> > Currently SSH/SCP/Telnet communication is handled by
> > kvm_utils.kvm_spawn, which
> > does not allow the child process to run after the parent process
> > exits. Thus,
> > open SSH/SCP/Telnet sessions cannot be reused by tests following the
> > one in
> > which they are opened.
> >
> > The new module provides a solution to these two problems, and also
> > saves some
> > code by reusing common code required both for QEMU processes and
> > SSH/SCP/Telnet
> > processes.
> >
> > Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> > ---
> >  client/tests/kvm/kvm_subprocess.py | 1146
> >  ++++++++++++++++++++++++++++++++++++
> >  1 files changed, 1146 insertions(+), 0 deletions(-)
> >  create mode 100644 client/tests/kvm/kvm_subprocess.py
> >

Re: [KVM-AUTOTEST,01/17] Add new module kvm_subprocess

[Michael Goldish]

----- "Chen Cao" <kcao@redhat.com> wrote:

> On Mon, Oct 12, 2009 at 09:07:45AM -0400, Michael Goldish wrote:
> > You're right, currently the sessions must be closed explicitly.
> > This is due to the fact that both qemu and ssh/telnet are handled by
> the
> > same code, and qemu has to keep running in the background if we want
> to
> > pass it from one test to another.
> > 
> > To deal with this, first, we should use try..finally blocks to close
> all
> > sessions in tests.  As far as I know all existing tests (or at least
> most
> > of them) already do this.
> > 
> > Second, we can add a destructor function to kvm_shell_session that
> will
> > close the session automatically when it's no longer referenced.  At
> the
> > moment this won't work because there's a thread running in the
> background
> > tracking output from the session, but this thread is usually not
> needed
> > for ssh/telnet (it's needed mainly for qemu), so we can selectively
> get
> > rid of it and allow the reference count to drop to zero when the
> test exits,
> > thus allowing the destructor to be called.
> > 
> > I'll think of a way to do the second thing, and if it works, maybe
> we won't
> > need the first.  But for now every test should close its sessions
> explicitly.
> > 
> > BTW, I'm not sure I understand why cleaning up the sessions should
> be
> > exhausting in the case you presented.  You can just wrap everything
> in one
> > big try..finally block:
> > 
> > session = ...
> > 
> > try:
> >   try:
> >   except:
> >   try:
> >   except:
> >   ...
> > finally:
> >   session.close()
> > 
> 
> Thanks for your explanation.
> 
> It is just boring and error-prone to add lots of
> '(dst|src|tmp|etc)*sesson.close()' to our code (the internal version)
> into different files and big number of functions. and some of the
> 'sessions' are in the try...except blocks, and some are not.
> 
> We have to make sure where we started the sessions and to close all
> of
> them when they are not needed any longer. I feel it's a little weird
> that we have to do the garbage-collection-like work while using
> python.
> 
> so, since this is a known issue, or precisely, limitation of 'ease of
> use', I'm looking forward to your impovement, and I think, we will
> also try to work it out at the same time.
> 
> Thanks again for your help.
> 
> 
> Cao, Chen
> 2009-10-13

OK, agreed.  I posted 3 patches that should fix this but I've only given
them minimal testing.  Let me know what you think.

Thanks,
Michael

> > ----- Original Message -----
> > From: "Chen Cao" <kcao@redhat.com>
> > To: "Michael Goldish" <mgoldish@redhat.com>
> > Cc: autotest@test.kernel.org, kvm@vger.kernel.org
> > Sent: Monday, October 12, 2009 8:55:59 AM (GMT+0200) Auto-Detected
> > Subject: Re: [KVM-AUTOTEST,01/17] Add new module kvm_subprocess
> > 
> > 
> > Hi, Michael,
> > 
> > I found that if the sessions initialized using kvm_subprcoess are
> not closed,
> > the processes will never exit, and /tmp/kvm_spawn will be filled
> with the
> > temporary files.
> > 
> > And we can find in the code,
> > # kvm_subprocess.py
> > ...
> >         # Read from child and write to files/pipes
> >         while True:
> >             check_termination = False
> >             # Make a list of reader pipes whose buffers are not
> empty
> >             fds = [fd for (i, fd) in enumerate(reader_fds) if
> buffers[i]]
> >             # Wait until there's something to do
> >             r, w, x = select.select([shell_fd, inpipe_fd], fds, [],
> 0.5)
> >             # If a reader pipe is ready for writing --
> >             for (i, fd) in enumerate(reader_fds):
> >                 if fd in w:
> >                     bytes_written = os.write(fd, buffers[i])
> >                     buffers[i] = buffers[i][bytes_written:]
> >             # If there's data to read from the child process --
> >             if shell_fd in r:
> >                 try:
> >                     data = os.read(shell_fd, 16384)
> >                 except OSError:
> >                     data = ""
> >                 if not data:
> >                     check_termination = True
> >                 # Remove carriage returns from the data -- they
> often cause
> >                 # trouble and are normally not needed
> >                 data = data.replace("\r", "")
> >                 output_file.write(data)
> >                 output_file.flush()
> >                 for i in range(len(readers)):
> >                     buffers[i] += data
> >             # If os.read() raised an exception or there was nothing
> to read --
> >             if check_termination or shell_fd not in r:
> >                 pid, status = os.waitpid(shell_pid, os.WNOHANG)
> >                 if pid:
> >                     status = os.WEXITSTATUS(status)
> >                     break
> >             # If there's data to read from the client --
> >             if inpipe_fd in r:
> >                 data = os.read(inpipe_fd, 1024)
> >                 os.write(shell_fd, data)
> > ...
> > 
> > that if session.close() is not called, we will loop in the 'while'
> forever.
> > 
> > So, user have to make sure that unnecessary sessions are all
> killed,
> > otherwise, running some testcase(s) for huge number of times will
> suck
> > out all the system resource, which I think is very inconvenient.
> > Especially when we have to take care of many exceptions that may be
> raised
> > by our program.
> > 
> > e.g.
> > 
> > ...
> > session = kvm_test_utils.wait_for_login(vm)
> > ...
> > session2 = kvm_test_utils.wait_for_login(vm_x)
> > ...
> > try:
> >     ...
> > except ...:
> >     ...
> > ...
> > (other code may raise exceptions)
> > ...
> > try:
> >     ...
> > except ...:
> >     ...
> > ...
> > try:
> >     ...
> > except ...:
> >     ...
> > ...
> > 
> > cleaning up the sessions will be exhausting here.
> > 
> > Do we have a good (or better) way to handle this?
> > Thanks.
> > 
> > 
> > Regards,
> > 
> > Cao, Chen
> > 2009-10-12
> > 
> > On Mon, Jul 20, 2009 at 12:07 PM, Michael
> Goldish<mgoldish@redhat.com>
> > wrote:
> > > This module is intended to be used for controlling all child
> > > processes in KVM
> > > tests: both QEMU processes and SSH/SCP/Telnet processes.
> Processes
> > > started with
> > > this module keep running and can be interacted with even after
> the
> > > parent
> > > process exits.
> > >
> > > The current run_bg() utility tracks a child process as long as
> the
> > > parent
> > > process is running. When the parent process exits, the tracking
> > > thread
> > > terminates and cannot resume when needed.
> > >
> > > Currently SSH/SCP/Telnet communication is handled by
> > > kvm_utils.kvm_spawn, which
> > > does not allow the child process to run after the parent process
> > > exits. Thus,
> > > open SSH/SCP/Telnet sessions cannot be reused by tests following
> the
> > > one in
> > > which they are opened.
> > >
> > > The new module provides a solution to these two problems, and
> also
> > > saves some
> > > code by reusing common code required both for QEMU processes and
> > > SSH/SCP/Telnet
> > > processes.
> > >
> > > Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> > > ---
> > >  client/tests/kvm/kvm_subprocess.py | 1146
> > >  ++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 1146 insertions(+), 0 deletions(-)
> > >  create mode 100644 client/tests/kvm/kvm_subprocess.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