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

[Michael Goldish <mgoldish@redhat.com>]

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