Re: [Autotest] [PATCH] KVM Test: Make remote_scp() more robust.

[Feng Yang <fyang@redhat.com>]

----- "Michael Goldish" <mgoldish@redhat.com> wrote:

> From: "Michael Goldish" <mgoldish@redhat.com>
> To: "Feng Yang" <fyang@redhat.com>
> Cc: autotest@test.kernel.org, kvm@vger.kernel.org
> Sent: Monday, May 17, 2010 11:05:37 PM GMT +08:00 Beijing / Chongqing / Hong Kong / Urumqi
> Subject: Re: [Autotest] [PATCH] KVM Test: Make remote_scp() more robust.
>
> On 05/07/2010 01:26 PM, Feng Yang wrote:
> > 1. In remote_scp(), if SCP connetion stalled for some reason,
> following
> > code will be ran.
> >         else:  # match == None
> > 
> >             logging.debug("Timeout elapsed or process terminated")
> >             status = sub.get_status()
> >             sub.close()
> >             return status == 0
> > At this moment, kvm_subprocess server is still running which means
> > lock_server_running_filename is still locked. But sub.get_status()
> > tries to lock it again.  If kvm_subprocess server keeps running,
> > a deadlock will happen. This patch will fix this issue by enable
> 
> Agreed.  It's a mistake (my mistake) to call get_status() on a
> process
> that's still running and isn't expected to terminate soon.  I think
> even
> the docstring of get_status() says that it blocks, so that's expected
> behavior.
> However, there's a simple solution to that, and I don't see why an
> additional timeout is necessary.
> 
> > timeout parameter. Update default value for timeout to 600, it
> should
> > be enough.
> > 
> > 2. Add "-v" in scp command to catch more infomation. Also add "Exit
> status"
> > and "stalled" match prompt in remote_scp().
> > Signed-off-by: Feng Yang <fyang@redhat.com>
> > ---
> >  client/tests/kvm/kvm_utils.py |   36
> ++++++++++++++++++++++++++++--------
> >  client/tests/kvm/kvm_vm.py    |    4 ++--
> >  2 files changed, 30 insertions(+), 10 deletions(-)
> > 
> > diff --git a/client/tests/kvm/kvm_utils.py
> b/client/tests/kvm/kvm_utils.py
> > index 25f3c8c..3db4dec 100644
> > --- a/client/tests/kvm/kvm_utils.py
> > +++ b/client/tests/kvm/kvm_utils.py
> > @@ -524,7 +524,7 @@ def remote_login(command, password, prompt,
> linesep="\n", timeout=10):
> >              return None
> >  
> >  
> > -def remote_scp(command, password, timeout=300, login_timeout=10):
> > +def remote_scp(command, password, timeout=600, login_timeout=10):
> >      """
> >      Run the given command using kvm_spawn and provide answers to
> the questions
> >      asked. If timeout expires while waiting for the transfer to
> complete ,
> > @@ -548,12 +548,18 @@ def remote_scp(command, password, timeout=300,
> login_timeout=10):
> >  
> >      password_prompt_count = 0
> >      _timeout = login_timeout
> > +    end_time = time.time() + timeout
> > +    logging.debug("Trying to SCP...")
> >  
> > -    logging.debug("Trying to login...")
> >  
> >      while True:
> > +        if end_time <= time.time():
> > +            logging.debug("transfer timeout!")
> > +            sub.close()
> > +            return False
> >          (match, text) = sub.read_until_last_line_matches(
> > -                [r"[Aa]re you sure", r"[Pp]assword:\s*$", r"lost
> connection"],
> > +                [r"[Aa]re you sure", r"[Pp]assword:\s*$", r"lost
> connection",
> > +                 r"Exit status", r"stalled"],
> >                  timeout=_timeout, internal_timeout=0.5)
> >          if match == 0:  # "Are you sure you want to continue
> connecting"
> >              logging.debug("Got 'Are you sure...'; sending 'yes'")
> > @@ -574,15 +580,29 @@ def remote_scp(command, password, timeout=300,
> login_timeout=10):
> >              logging.debug("Got 'lost connection'")
> >              sub.close()
> >              return False
> > +        elif match == 3: # "Exit status"
> 
> This check for "Exit status" is redundant.  When the process
> terminates,
> read_until_last_line_matches() will return None and get_status() will
> return the exit status.
Here check for "Exit status", we can get not only the exit status,but also some useful debug information when exit status is not 0.
Because we have enable '-v' in scp command.

but read_until_last_line_matches() only return exit status.

> 
> > +            sub.close()
> > +            if "Exit status 0" in text:
> > +                logging.debug("SCP command completed
> successfully")
> > +                return True
> > +            else:
> > +                logging.debug("SCP command fail with exit status
> %s" % text)
> > +                return False
> > +        elif match == 4: # "stalled"
> > +            logging.debug("SCP connection stalled for some
> reason")
> > +            continue
> > +
> >          else:  # match == None
> > -            logging.debug("Timeout elapsed or process terminated")
> > +            if sub.is_alive():
> > +                continue
> > +            logging.debug("Process terminated for some reason")
> >              status = sub.get_status()
> >              sub.close()
> >              return status == 0
> 
> To avoid a deadlock, we can simply check if the process is still
> alive
> before calling get_status(), i.e.
> 
> else:  # match == None
>     if sub.is_alive():
>         logging.debug("Timeout elapsed")
>         sub.close()
>         return False
>     else:
>         status = sub.get_status()
>         sub.close()
>         logging.debug("Process exited with status %d", status)
>         return status == 0
> 
> This looks like the simplest solution to me.
I think we should not directly call sub.close() when sub.is_alive().  
This would significantly reduce the chances of success of the scp command
If SCP connection stalled for some reason, it not means this connection has die.
Some seconds late, the connection may reactive and continue.
I think set a timeout value, and call sub.close when timeout is better.
> 
> >  
> >  
> >  def scp_to_remote(host, port, username, password, local_path,
> remote_path,
> > -                  timeout=300):
> > +                  timeout=600):
> >      """
> >      Copy files to a remote host (guest).
> >  
> > @@ -596,14 +616,14 @@ def scp_to_remote(host, port, username,
> password, local_path, remote_path,
> >  
> >      @return: True on success and False on failure.
> >      """
> > -    command = ("scp -o UserKnownHostsFile=/dev/null "
> > +    command = ("scp -v -o UserKnownHostsFile=/dev/null "
> >                 "-o PreferredAuthentications=password -r -P %s %s
> %s@%s:%s" %
> >                 (port, local_path, username, host, remote_path))
> >      return remote_scp(command, password, timeout)
> >  
> >  
> >  def scp_from_remote(host, port, username, password, remote_path,
> local_path,
> > -                    timeout=300):
> > +                    timeout=600):
> >      """
> >      Copy files from a remote host (guest).
> >  
> > @@ -617,7 +637,7 @@ def scp_from_remote(host, port, username,
> password, remote_path, local_path,
> >  
> >      @return: True on success and False on failure.
> >      """
> > -    command = ("scp -o UserKnownHostsFile=/dev/null "
> > +    command = ("scp -v -o UserKnownHostsFile=/dev/null "
> >                 "-o PreferredAuthentications=password -r -P %s
> %s@%s:%s %s" %
> >                 (port, username, host, remote_path, local_path))
> >      return remote_scp(command, password, timeout)
> > diff --git a/client/tests/kvm/kvm_vm.py
> b/client/tests/kvm/kvm_vm.py
> > index 6bc7987..d1e0246 100755
> > --- a/client/tests/kvm/kvm_vm.py
> > +++ b/client/tests/kvm/kvm_vm.py
> > @@ -808,7 +808,7 @@ class VM:
> >          return session
> >  
> >  
> > -    def copy_files_to(self, local_path, remote_path, nic_index=0,
> timeout=300):
> > +    def copy_files_to(self, local_path, remote_path, nic_index=0,
> timeout=600):
> >          """
> >          Transfer files to the guest.
> >  
> > @@ -833,7 +833,7 @@ class VM:
> >                                             local_path, remote_path,
> timeout)
> >  
> >  
> > -    def copy_files_from(self, remote_path, local_path, nic_index=0,
> timeout=300):
> > +    def copy_files_from(self, remote_path, local_path, nic_index=0,
> timeout=600):
> >          """
> >          Transfer files from the guest.
> >  
> 
> --
> 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