Merge pull request #287 from xcp-ng/ssh-background

Ssh background cleanup

Author: ydirson

lib/commands.py

@@ -61,7 +61,7 @@ def _ellide_log_lines(log):
     return "\n{}".format("\n".join(reduced_message))
 
 def _ssh(hostname_or_ip, cmd, check, simple_output, suppress_fingerprint_warnings,
-         background, target_os, decode, options):
+         background, decode, options):
     opts = list(options)
     opts.append('-o "BatchMode yes"')
     if suppress_fingerprint_warnings:
@@ -76,17 +76,11 @@ def _ssh(hostname_or_ip, cmd, check, simple_output, suppress_fingerprint_warning
         command = cmd
     else:
         command = " ".join(cmd)
-    if background and target_os != "windows":
-        # https://stackoverflow.com/questions/29142/getting-ssh-to-execute-a-command-in-the-background-on-target-machine
-        # ... and run the command through a bash shell so that output redirection both works on Linux and FreeBSD.
-        # Bash being available on VMs is a documented requirement.
-        command = "nohup bash -c \"%s &>/dev/null &\"" % command
 
     ssh_cmd = f"ssh root@{hostname_or_ip} {' '.join(opts)} {shlex.quote(command)}"
 
-    windows_background = background and target_os == "windows"
     # Fetch banner and remove it to avoid stdout/stderr pollution.
-    if config.ignore_ssh_banner and not windows_background:
+    if config.ignore_ssh_banner:
         banner_res = subprocess.run(
             "ssh root@%s %s '%s'" % (hostname_or_ip, ' '.join(opts), '\n'),
             shell=True,
@@ -95,15 +89,15 @@ def _ssh(hostname_or_ip, cmd, check, simple_output, suppress_fingerprint_warning
             check=False
         )
 
+    logging.debug(f"[{hostname_or_ip}] {command}")
     process = subprocess.Popen(
         ssh_cmd,
         shell=True,
         stdout=subprocess.PIPE,
         stderr=subprocess.STDOUT
     )
-    logging.debug(f"[{hostname_or_ip}] {command}")
-    if windows_background:
-        return True, process
+    if background:
+        return True, None
 
     stdout = []
     for line in iter(process.stdout.readline, b''):
@@ -140,9 +134,9 @@ def _ssh(hostname_or_ip, cmd, check, simple_output, suppress_fingerprint_warning
 # This function is kept short for shorter pytest traces upon SSH failures, which are common,
 # as pytest prints the whole function definition that raised the SSHCommandFailed exception
 def ssh(hostname_or_ip, cmd, check=True, simple_output=True, suppress_fingerprint_warnings=True,
-        background=False, target_os='linux', decode=True, options=[]):
+        background=False, decode=True, options=[]):
     success, result_or_exc = _ssh(hostname_or_ip, cmd, check, simple_output, suppress_fingerprint_warnings,
-                                  background, target_os, decode, options)
+                                  background, decode, options)
     if not success:
         raise result_or_exc
     return result_or_exc

lib/vm.py

@@ -84,9 +84,8 @@ def try_get_and_store_ip(self):
 
     def ssh(self, cmd, check=True, simple_output=True, background=False, decode=True):
         # raises by default for any nonzero return code
-        target_os = "windows" if self.is_windows else "linux"
         return commands.ssh(self.ip, cmd, check=check, simple_output=simple_output, background=background,
-                            target_os=target_os, decode=decode)
+                            decode=decode)
 
     def ssh_with_result(self, cmd):
         # doesn't raise if the command's return is nonzero, unless there's a SSH error
@@ -271,9 +270,15 @@ def start_background_process(self, cmd):
                 self.sftp_put(f.name, script.replace("/tmp/", "/Users/root/AppData/Local/Temp/"))
             else:
                 self.scp(f.name, script)
-            # Use bash to run the script, to avoid being hit by differences between shells, for example on FreeBSD
+
+            # https://stackoverflow.com/questions/29142/getting-ssh-to-execute-a-command-in-the-background-on-target-machine
+            # ... and run the command through a bash shell so that output redirection both works on Linux and FreeBSD.
             # It is a documented requirement that bash is present on all test VMs.
-            self.ssh(['bash', script], background=True)
+            remote_cmd = f"bash {script}"
+            if not self.is_windows:
+                remote_cmd = f'nohup bash -c "{remote_cmd} &>/dev/null &"'
+            self.ssh(remote_cmd, background=True)
+
             wait_for(lambda: self.ssh_with_result(['test', '-f', pidfile]),
                      "wait for pid file %s to exist" % pidfile)
             pid = self.ssh(['cat', pidfile])