Re: [Autotest] [PATCH] KVM Test: Make remote_scp() more robust.
On 05/19/2010 05:14 AM, Feng Yang wrote: - Michael Goldish mgold...@redhat.com wrote: From: Michael Goldish mgold...@redhat.com To: Feng Yang fy...@redhat.com Cc: autot...@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 fy...@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*$, rlost connection], +[r[Aa]re you sure, r[Pp]assword:\s*$, rlost connection, + rExit status, rstalled], 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
Re: [Autotest] [PATCH] KVM Test: Make remote_scp() more robust.
- Michael Goldish mgold...@redhat.com wrote: From: Michael Goldish mgold...@redhat.com To: Feng Yang fy...@redhat.com Cc: autot...@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 fy...@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*$, rlost connection], +[r[Aa]re you sure, r[Pp]assword:\s*$, rlost connection, + rExit status, rstalled], 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
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 fy...@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*$, rlost connection], +[r[Aa]re you sure, r[Pp]assword:\s*$, rlost connection, + rExit status, rstalled], 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 %
[Autotest][PATCH] KVM Test: Make remote_scp() more robust.
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 fy...@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*$, rlost connection], +[r[Aa]re you sure, r[Pp]assword:\s*$, rlost connection, + rExit status, rstalled], 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 % (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 % (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): +
Re: [Autotest][PATCH] KVM Test: Make remote_scp() more robust.
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 fy...@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*$, rlost connection], +[r[Aa]re you sure, r[Pp]assword:\s*$, rlost connection, + rExit status, rstalled], 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 % (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 % (port, username, host, remote_path, local_path)) return remote_scp(command,
