* [Autotest][PATCH] KVM Test: Make remote_scp() more robust.
@ 2010-05-07 10:26 Feng Yang
2010-05-07 14:18 ` [PATCH] " Lucas Meneghel Rodrigues
2010-05-17 15:05 ` [Autotest] " Michael Goldish
0 siblings, 2 replies; 5+ messages in thread
From: Feng Yang @ 2010-05-07 10:26 UTC (permalink / raw)
To: autotest; +Cc: kvm, Feng Yang
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
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"
+ 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
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.
--
1.5.5.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM Test: Make remote_scp() more robust.
2010-05-07 10:26 [Autotest][PATCH] KVM Test: Make remote_scp() more robust Feng Yang
@ 2010-05-07 14:18 ` Lucas Meneghel Rodrigues
2010-05-17 15:05 ` [Autotest] " Michael Goldish
1 sibling, 0 replies; 5+ messages in thread
From: Lucas Meneghel Rodrigues @ 2010-05-07 14:18 UTC (permalink / raw)
To: Feng Yang; +Cc: autotest, kvm
On Fri, 2010-05-07 at 18:26 +0800, 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
> 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().
Excellent. I've made some slight changes to message wording, and
commited it:
http://autotest.kernel.org/changeset/4483
Thank you very much!
> 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"
> + 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
>
>
> 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.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Autotest] [PATCH] KVM Test: Make remote_scp() more robust.
2010-05-07 10:26 [Autotest][PATCH] KVM Test: Make remote_scp() more robust Feng Yang
2010-05-07 14:18 ` [PATCH] " Lucas Meneghel Rodrigues
@ 2010-05-17 15:05 ` Michael Goldish
1 sibling, 0 replies; 5+ messages in thread
From: Michael Goldish @ 2010-05-17 15:05 UTC (permalink / raw)
To: Feng Yang; +Cc: autotest, kvm
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.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Autotest] [PATCH] KVM Test: Make remote_scp() more robust.
[not found] <51503154.764621274235137161.JavaMail.root@zmail04.collab.prod.int.phx2.redhat.com>
@ 2010-05-19 2:14 ` Feng Yang
2010-05-20 9:33 ` Michael Goldish
0 siblings, 1 reply; 5+ messages in thread
From: Feng Yang @ 2010-05-19 2:14 UTC (permalink / raw)
To: Michael Goldish; +Cc: autotest, kvm
----- "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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Autotest] [PATCH] KVM Test: Make remote_scp() more robust.
2010-05-19 2:14 ` Feng Yang
@ 2010-05-20 9:33 ` Michael Goldish
0 siblings, 0 replies; 5+ messages in thread
From: Michael Goldish @ 2010-05-20 9:33 UTC (permalink / raw)
To: Feng Yang; +Cc: autotest, kvm
On 05/19/2010 05:14 AM, Feng Yang wrote:
>
> ----- "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.
You get the same information from read_until_last_line_matches(). It
returns (match, text). If match is None, and sub.get_status() != 0,
then something bad happened, and then we can print text.
>>
>>> + 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.
But that's exactly what's happening right now. The code in my
suggestion runs only if match is None, and that happens only when the
timeout expires (300 by default), or when the process exits.
I don't think it's necessary to check if SCP is stalled, and it's
certainly not a good idea to kill it when it's stalled.
>>>
>>>
>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-05-20 9:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-07 10:26 [Autotest][PATCH] KVM Test: Make remote_scp() more robust Feng Yang
2010-05-07 14:18 ` [PATCH] " Lucas Meneghel Rodrigues
2010-05-17 15:05 ` [Autotest] " Michael Goldish
[not found] <51503154.764621274235137161.JavaMail.root@zmail04.collab.prod.int.phx2.redhat.com>
2010-05-19 2:14 ` Feng Yang
2010-05-20 9:33 ` Michael Goldish
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).