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

[Michael Goldish <mgoldish@redhat.com>]

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.

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

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