From c8f8ce697b8e6c5f548e3b93b32596fd47bf9d91 Mon Sep 17 00:00:00 2001 From: Emmanuel Varagnat Date: Fri, 17 Oct 2025 09:41:21 +0200 Subject: [PATCH 1/6] Replace manual process handling by local_cmd() And change a bit local_cmd() interface to behave like ssh(); introduce simple_output argument. Assertion added in lib/efi.py is here to please linter, because self._owner_cert.key is used in argument to local_cmd() below. Signed-off-by: Emmanuel Varagnat --- conftest.py | 4 +++- lib/commands.py | 24 ++++++++++++++---------- lib/efi.py | 1 + lib/host.py | 4 ++-- lib/xo.py | 20 ++++++++++---------- scripts/install_xcpng.py | 14 +++++++------- tests/fs_diff/test_fs_diff.py | 17 ++++++++--------- tests/misc/test_access_links.py | 5 ++--- tests/misc/test_file_server.py | 8 ++++---- tests/network/test_vif_allowed_ip.py | 6 +++++- tests/packages/bugtool/test_bugtool.py | 2 -- tests/packages/netdata/test_netdata.py | 13 ++++--------- 12 files changed, 60 insertions(+), 58 deletions(-) diff --git a/conftest.py b/conftest.py index caf476684..031cbf44a 100644 --- a/conftest.py +++ b/conftest.py @@ -12,6 +12,7 @@ import lib.config as global_config from lib import pxe +from lib import commands from lib.common import ( callable_marker, DiskDevName, @@ -202,7 +203,8 @@ def setup_host(hostname_or_ip, *, config=None): assert len(ips) == 1 host_vm.ip = ips[0] - wait_for(lambda: not os.system(f"nc -zw5 {host_vm.ip} 22"), + wait_for(lambda: commands.local_cmd(['nc', '-zw5', str(host_vm.ip), '22'], + check=False).returncode == 0, "Wait for ssh up on nested host", retry_delay_secs=5) hostname_or_ip = host_vm.ip diff --git a/lib/commands.py b/lib/commands.py index 24ec1cb07..96c843dda 100644 --- a/lib/commands.py +++ b/lib/commands.py @@ -27,10 +27,10 @@ def __init__(self, returncode, stdout, cmd): ) class LocalCommandFailed(BaseCommandFailed): - def __init__(self, returncode, stdout, cmd): - msg_end = f": {stdout}" if stdout else "." + def __init__(self, returncode, stderr, cmd): + msg_end = f": {stderr}" if stderr else "." super(LocalCommandFailed, self).__init__( - returncode, stdout, cmd, + returncode, stderr, cmd, f'Local command ({cmd}) failed with return code {returncode}{msg_end}' ) @@ -46,8 +46,9 @@ def __init__(self, returncode, stdout): super(SSHResult, self).__init__(returncode, stdout) class LocalCommandResult(BaseCmdResult): - def __init__(self, returncode, stdout): + def __init__(self, returncode, stdout, stderr): super(LocalCommandResult, self).__init__(returncode, stdout) + self.stderr = stderr def _ellide_log_lines(log): if log == '': @@ -244,18 +245,19 @@ def sftp(hostname_or_ip, cmds, check=True, suppress_fingerprint_warnings=True): return res -def local_cmd(cmd, check=True, decode=True): +def local_cmd(cmd, check=True, decode=True) -> LocalCommandResult: """ Run a command locally on tester end. """ logging.debug("[local] %s", (cmd,)) res = subprocess.run( cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, + stderr=subprocess.PIPE, check=False ) # get a decoded version of the output in any case, replacing potential errors - output_for_logs = res.stdout.decode(errors='replace').strip() + stdout_for_logs = res.stdout.decode(errors='replace').strip() + stderr_for_logs = res.stderr.decode(errors='replace').strip() output = res.stdout if decode: @@ -263,12 +265,14 @@ def local_cmd(cmd, check=True, decode=True): errorcode_msg = "" if res.returncode == 0 else " - Got error code: %s" % res.returncode command = " ".join(cmd) - logging.debug(f"[local] {command}{errorcode_msg}{_ellide_log_lines(output_for_logs)}") + logging.debug(f"[local] {command}{errorcode_msg}{_ellide_log_lines(stdout_for_logs)}") if res.returncode and check: - raise LocalCommandFailed(res.returncode, output_for_logs, command) + logging.warning(f"[local] stderr:{_ellide_log_lines(stderr_for_logs)}") + raise LocalCommandFailed(res.returncode, stderr_for_logs, command) - return LocalCommandResult(res.returncode, output) + stderr = res.stderr.decode() + return LocalCommandResult(res.returncode, output, stderr) def encode_powershell_command(cmd: str): return base64.b64encode(cmd.encode("utf-16-le")).decode("ascii") diff --git a/lib/efi.py b/lib/efi.py index bb1d1a4bd..7271ac6de 100755 --- a/lib/efi.py +++ b/lib/efi.py @@ -460,6 +460,7 @@ def sign_image(self, image: str) -> str: Returns path to signed image. """ assert self._owner_cert is not None + assert self._owner_cert.key is not None if shutil.which('sbsign'): signed = get_signed_name(image) commands.local_cmd([ diff --git a/lib/host.py b/lib/host.py index 68e3654d7..ca0f60e4f 100644 --- a/lib/host.py +++ b/lib/host.py @@ -551,9 +551,9 @@ def reboot(self, verify=False): raise if verify: wait_for_not(self.is_enabled, "Wait for host down") - wait_for(lambda: not os.system(f"ping -c1 {self.hostname_or_ip} > /dev/null 2>&1"), + wait_for(lambda: commands.local_cmd(['ping', '-c1', self.hostname_or_ip]).returncode == 0, "Wait for host up", timeout_secs=10 * 60, retry_delay_secs=10) - wait_for(lambda: not os.system(f"nc -zw5 {self.hostname_or_ip} 22"), + wait_for(lambda: commands.local_cmd(['nc', '-zw5', self.hostname_or_ip, '22']).returncode == 0, "Wait for ssh up on host", timeout_secs=10 * 60, retry_delay_secs=5) wait_for(self.is_enabled, "Wait for XAPI to be ready", timeout_secs=30 * 60) diff --git a/lib/xo.py b/lib/xo.py index 2dc36dad1..8ced4ca57 100644 --- a/lib/xo.py +++ b/lib/xo.py @@ -1,5 +1,7 @@ import json -import subprocess + +from lib import commands +from lib.commands import LocalCommandResult from typing import Any, Dict, Literal, Union, overload @@ -13,28 +15,26 @@ def xo_cli(action: str, args: Dict[str, str] = {}, *, check: bool = True, simple ... @overload def xo_cli(action: str, args: Dict[str, str] = {}, *, check: bool = True, simple_output: Literal[False], - use_json: bool = False) -> subprocess.CompletedProcess: + use_json: bool = False) -> LocalCommandResult: ... @overload def xo_cli(action: str, args: Dict[str, str] = {}, *, check: bool = True, simple_output: bool = True, - use_json: bool = False) -> Union[subprocess.CompletedProcess, Any, str]: + use_json: bool = False) -> Union[LocalCommandResult, Any, str]: ... def xo_cli(action, args={}, check=True, simple_output=True, use_json=False): run_array = ['xo-cli', action] if use_json: run_array += ['--json'] run_array += ["%s=%s" % (key, value) for key, value in args.items()] - res = subprocess.run( - run_array, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - check=check - ) + + res = commands.local_cmd(run_array, check=check) + if simple_output: - output = res.stdout.decode().strip() + output = res.stdout.strip() if use_json: return json.loads(output) return output + return res def xo_object_exists(uuid): diff --git a/scripts/install_xcpng.py b/scripts/install_xcpng.py index 1ba52d894..869853e60 100755 --- a/scripts/install_xcpng.py +++ b/scripts/install_xcpng.py @@ -6,7 +6,6 @@ import os import random import string -import subprocess import sys import tempfile import time @@ -17,7 +16,7 @@ # flake8: noqa: E402 sys.path.append(f"{os.path.abspath(os.path.dirname(__file__))}/..") from lib import pxe -from lib.commands import SSHCommandFailed, scp, ssh +from lib.commands import SSHCommandFailed, scp, ssh, local_cmd from lib.common import is_uuid, wait_for from lib.host import host_data from lib.pool import Pool @@ -27,9 +26,8 @@ def generate_answerfile(directory, installer, hostname_or_ip, target_hostname, action, hdd, netinstall_gpg_check): password = host_data(hostname_or_ip)['password'] - cmd = ['openssl', 'passwd', '-6', password] - res = subprocess.run(cmd, stdout=subprocess.PIPE) - encrypted_password = res.stdout.decode().strip() + algorithm_option = '-6' # SHA512 + encrypted_password = local_cmd(['openssl', 'passwd', algorithm_option, password]).stdout.strip() if target_hostname is None: target_hostname = "xcp-ng-" + "".join( random.choice(string.ascii_lowercase) for i in range(5) @@ -70,7 +68,9 @@ def generate_answerfile(directory, installer, hostname_or_ip, target_hostname, a raise Exception(f"Unknown action: `{action}`") def is_ip_active(ip): - return not os.system(f"ping -c 3 -W 10 {ip} > /dev/null 2>&1") + # 3 tries with a timeout of 10 sec for each ICMP request + return local_cmd(['ping', '-c', '3', '-W', '10', ip], + check=False).returncode == 0 def is_ssh_up(ip): try: @@ -201,7 +201,7 @@ def main(): "Waiting for the installation process to complete and the VM to reboot and be up", 3600, 10 ) vm_ip_address = get_new_host_ip(mac_address) - logging.info('The IP address of the installed XCP-ng is: ' + vm_ip_address) + logging.info(f'The IP address of the installed XCP-ng is: {vm_ip_address}') wait_for(lambda: is_new_host_ready(vm_ip_address), "Waiting for XAPI to be ready", 600, 10) pool2 = Pool(vm_ip_address) host2 = pool2.master diff --git a/tests/fs_diff/test_fs_diff.py b/tests/fs_diff/test_fs_diff.py index e90bbf1a6..51627e9a4 100644 --- a/tests/fs_diff/test_fs_diff.py +++ b/tests/fs_diff/test_fs_diff.py @@ -1,7 +1,8 @@ import pytest import os -import subprocess + +from lib.commands import local_cmd # Requirements: # - 2 XCP-ng host of same version @@ -14,13 +15,11 @@ def test_fs_diff(hosts): fsdiff = os.path.realpath(f"{os.path.dirname(__file__)}/../../scripts/xcpng-fs-diff.py") - process = subprocess.Popen( - [fsdiff, "--reference-host", f"{hosts[0]}", "--test-host", f"{hosts[1]}", "--json-output"], - stdout=subprocess.PIPE, stderr=subprocess.PIPE - ) - stdout, _ = process.communicate() + res = local_cmd([fsdiff, "--reference-host", f"{hosts[0]}", + "--test-host", f"{hosts[1]}", + "--json-output"]) - if process.returncode != 0: - print(stdout.decode()) + if res.returncode != 0: + print(res.stdout) - assert process.returncode == 0 + assert res.returncode == 0 diff --git a/tests/misc/test_access_links.py b/tests/misc/test_access_links.py index a254acaf5..1114dbba2 100644 --- a/tests/misc/test_access_links.py +++ b/tests/misc/test_access_links.py @@ -1,7 +1,6 @@ import pytest import hashlib -import subprocess from lib import commands @@ -35,10 +34,10 @@ def test_access_links(host, command_id, url_id): # Verify the download worked by comparing with local download # This ensures the content is accessible and identical from both locations - local_result = commands.local_cmd(COMMAND) + local_result = commands.local_cmd(COMMAND, check=False) assert local_result.returncode == 0, ( - f"Failed to fetch URL locally: {local_result.stderr}" + f"Failed to fetch URL locally: {local_result.stdout}" ) # Extract checksums diff --git a/tests/misc/test_file_server.py b/tests/misc/test_file_server.py index e1b17f177..87813be36 100644 --- a/tests/misc/test_file_server.py +++ b/tests/misc/test_file_server.py @@ -18,8 +18,8 @@ def _header_equal(header, name, value): def test_fileserver_redirect_https(host): path = "/path/to/dir/file.txt" ip = wrap_ip(host.hostname_or_ip) - res = commands.local_cmd(["curl", "-s", "-i", "http://" + ip + path]) - lines = res.stdout.splitlines() + output = commands.local_cmd(["curl", "-s", "-i", "http://" + ip + path]) + lines = output.stdout.strip().splitlines() assert lines[0].strip() == "HTTP/1.1 301 Moved Permanently" assert _header_equal(lines[2], "location", "https://" + ip + path) @@ -29,10 +29,10 @@ class TestHSTS: HSTS_HEADER_VALUE = "max-age=63072000" def __get_header(host): - res = commands.local_cmd( + output = commands.local_cmd( ["curl", "-s", "-XGET", "-k", "-I", "https://" + wrap_ip(host.hostname_or_ip)] ) - return res.stdout.splitlines() + return output.stdout.strip().splitlines() def test_fileserver_hsts_default(self, host): # By default HSTS header should not be set diff --git a/tests/network/test_vif_allowed_ip.py b/tests/network/test_vif_allowed_ip.py index 347392820..131fa9baa 100644 --- a/tests/network/test_vif_allowed_ip.py +++ b/tests/network/test_vif_allowed_ip.py @@ -3,12 +3,16 @@ import ipaddress import os +from lib.commands import local_cmd + # Requirements: # - one XCP-ng host (--host) >= 8.2 (>= 8.3 for the CIDR tests) with no SDN controller configured # - a VM (--vm) def ip_responsive(ip): - return not os.system(f"ping -c 3 -W 10 {ip} > /dev/null 2>&1") + # 3 tries with a timeout of 10 sec for each ICMP request + return local_cmd(['ping', '-c', '3', '-W', '10', ip], + check=False).returncode == 0 @pytest.mark.small_vm @pytest.mark.usefixtures("host_no_sdn_controller") diff --git a/tests/packages/bugtool/test_bugtool.py b/tests/packages/bugtool/test_bugtool.py index 85c1634b4..2f2b0ed65 100644 --- a/tests/packages/bugtool/test_bugtool.py +++ b/tests/packages/bugtool/test_bugtool.py @@ -1,7 +1,5 @@ import pytest -import subprocess - from lib.host import Host # This smoke test runs xen-bugtool and verifies that the archive it generates diff --git a/tests/packages/netdata/test_netdata.py b/tests/packages/netdata/test_netdata.py index 6c0e50d31..73c443cc2 100644 --- a/tests/packages/netdata/test_netdata.py +++ b/tests/packages/netdata/test_netdata.py @@ -1,7 +1,6 @@ import pytest -import subprocess - +from lib.commands import local_cmd from lib.netutil import wrap_ip # This test installs netdata and netdata-ui packages, verifies the service status, @@ -17,13 +16,9 @@ def __get_headers(host, port, path=None): url = f"http://{wrap_ip(host.hostname_or_ip)}:{port}" if path is not None: url += f"/{path}" - process = subprocess.Popen( - ["curl", "-XGET", "-k", "-I", "-s", url], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE - ) - stdout, _ = process.communicate() - return stdout.decode().splitlines() + + res = local_cmd(["curl", "-XGET", "-k", "-I", "-s", url]) + return res.stdout.strip().splitlines() # Verify the ActiveState for the netdata service def test_netdata_service(self, host): From 20a4d4007f380a1ac454b25e7a18f7fd12e9cc26 Mon Sep 17 00:00:00 2001 From: Emmanuel Varagnat Date: Thu, 27 Nov 2025 14:11:21 +0100 Subject: [PATCH 2/6] fixup! Replace manual process handling by local_cmd() --- tests/misc/test_file_server.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/misc/test_file_server.py b/tests/misc/test_file_server.py index 87813be36..e1b17f177 100644 --- a/tests/misc/test_file_server.py +++ b/tests/misc/test_file_server.py @@ -18,8 +18,8 @@ def _header_equal(header, name, value): def test_fileserver_redirect_https(host): path = "/path/to/dir/file.txt" ip = wrap_ip(host.hostname_or_ip) - output = commands.local_cmd(["curl", "-s", "-i", "http://" + ip + path]) - lines = output.stdout.strip().splitlines() + res = commands.local_cmd(["curl", "-s", "-i", "http://" + ip + path]) + lines = res.stdout.splitlines() assert lines[0].strip() == "HTTP/1.1 301 Moved Permanently" assert _header_equal(lines[2], "location", "https://" + ip + path) @@ -29,10 +29,10 @@ class TestHSTS: HSTS_HEADER_VALUE = "max-age=63072000" def __get_header(host): - output = commands.local_cmd( + res = commands.local_cmd( ["curl", "-s", "-XGET", "-k", "-I", "https://" + wrap_ip(host.hostname_or_ip)] ) - return output.stdout.strip().splitlines() + return res.stdout.splitlines() def test_fileserver_hsts_default(self, host): # By default HSTS header should not be set From 5ff8705f3bfb42b4a4bc73eb6ad4174137172bde Mon Sep 17 00:00:00 2001 From: Emmanuel Varagnat Date: Mon, 24 Nov 2025 18:39:32 +0100 Subject: [PATCH 3/6] lib: add simple_output argument to local_cmd() To match the ssh() API. Signed-off-by: Emmanuel Varagnat --- conftest.py | 2 +- lib/commands.py | 29 +++++++++++++++++++++----- lib/host.py | 4 ++-- lib/xo.py | 2 +- scripts/install_xcpng.py | 4 ++-- tests/fs_diff/test_fs_diff.py | 2 +- tests/misc/test_access_links.py | 2 +- tests/misc/test_file_server.py | 8 +++---- tests/network/test_vif_allowed_ip.py | 2 +- tests/packages/netdata/test_netdata.py | 2 +- 10 files changed, 38 insertions(+), 19 deletions(-) diff --git a/conftest.py b/conftest.py index 031cbf44a..871d60342 100644 --- a/conftest.py +++ b/conftest.py @@ -204,7 +204,7 @@ def setup_host(hostname_or_ip, *, config=None): host_vm.ip = ips[0] wait_for(lambda: commands.local_cmd(['nc', '-zw5', str(host_vm.ip), '22'], - check=False).returncode == 0, + check=False, simple_output=False).returncode == 0, "Wait for ssh up on nested host", retry_delay_secs=5) hostname_or_ip = host_vm.ip diff --git a/lib/commands.py b/lib/commands.py index 96c843dda..681a5e98e 100644 --- a/lib/commands.py +++ b/lib/commands.py @@ -245,7 +245,24 @@ def sftp(hostname_or_ip, cmds, check=True, suppress_fingerprint_warnings=True): return res -def local_cmd(cmd, check=True, decode=True) -> LocalCommandResult: +@overload +def local_cmd(cmd: Union[str, List[str]], *, check: bool = True, simple_output: Literal[True] = True, + decode: Literal[True] = True) -> str: + ... +@overload +def local_cmd(cmd: Union[str, List[str]], *, check: bool = True, simple_output: Literal[True] = True, + decode: Literal[False]) -> bytes: + ... +@overload +def local_cmd(cmd: Union[str, List[str]], *, check: bool = True, simple_output: Literal[False], + decode: bool = True) -> LocalCommandResult: + ... +@overload +def local_cmd(cmd: Union[str, List[str]], *, check: bool = True, simple_output: bool = True, + decode: bool = True) -> Union[str, bytes, LocalCommandResult]: + ... + +def local_cmd(cmd, check=True, simple_output=True, decode=True): """ Run a command locally on tester end. """ logging.debug("[local] %s", (cmd,)) res = subprocess.run( @@ -259,10 +276,6 @@ def local_cmd(cmd, check=True, decode=True) -> LocalCommandResult: stdout_for_logs = res.stdout.decode(errors='replace').strip() stderr_for_logs = res.stderr.decode(errors='replace').strip() - output = res.stdout - if decode: - output = output.decode() - errorcode_msg = "" if res.returncode == 0 else " - Got error code: %s" % res.returncode command = " ".join(cmd) logging.debug(f"[local] {command}{errorcode_msg}{_ellide_log_lines(stdout_for_logs)}") @@ -271,6 +284,12 @@ def local_cmd(cmd, check=True, decode=True) -> LocalCommandResult: logging.warning(f"[local] stderr:{_ellide_log_lines(stderr_for_logs)}") raise LocalCommandFailed(res.returncode, stderr_for_logs, command) + output: Union[bytes, str] = res.stdout + if decode: + output = output.decode() + if simple_output: + return output.strip() + stderr = res.stderr.decode() return LocalCommandResult(res.returncode, output, stderr) diff --git a/lib/host.py b/lib/host.py index ca0f60e4f..7debdbd33 100644 --- a/lib/host.py +++ b/lib/host.py @@ -551,9 +551,9 @@ def reboot(self, verify=False): raise if verify: wait_for_not(self.is_enabled, "Wait for host down") - wait_for(lambda: commands.local_cmd(['ping', '-c1', self.hostname_or_ip]).returncode == 0, + wait_for(lambda: commands.local_cmd(['ping', '-c1', self.hostname_or_ip], simple_output=False).returncode == 0, "Wait for host up", timeout_secs=10 * 60, retry_delay_secs=10) - wait_for(lambda: commands.local_cmd(['nc', '-zw5', self.hostname_or_ip, '22']).returncode == 0, + wait_for(lambda: commands.local_cmd(['nc', '-zw5', self.hostname_or_ip, '22'], simple_output=False).returncode == 0, "Wait for ssh up on host", timeout_secs=10 * 60, retry_delay_secs=5) wait_for(self.is_enabled, "Wait for XAPI to be ready", timeout_secs=30 * 60) diff --git a/lib/xo.py b/lib/xo.py index 8ced4ca57..d2be5181c 100644 --- a/lib/xo.py +++ b/lib/xo.py @@ -27,7 +27,7 @@ def xo_cli(action, args={}, check=True, simple_output=True, use_json=False): run_array += ['--json'] run_array += ["%s=%s" % (key, value) for key, value in args.items()] - res = commands.local_cmd(run_array, check=check) + res = commands.local_cmd(run_array, check=check, simple_output=False) if simple_output: output = res.stdout.strip() diff --git a/scripts/install_xcpng.py b/scripts/install_xcpng.py index 869853e60..e7faca04d 100755 --- a/scripts/install_xcpng.py +++ b/scripts/install_xcpng.py @@ -27,7 +27,7 @@ def generate_answerfile(directory, installer, hostname_or_ip, target_hostname, action, hdd, netinstall_gpg_check): password = host_data(hostname_or_ip)['password'] algorithm_option = '-6' # SHA512 - encrypted_password = local_cmd(['openssl', 'passwd', algorithm_option, password]).stdout.strip() + encrypted_password = local_cmd(['openssl', 'passwd', algorithm_option, password]) if target_hostname is None: target_hostname = "xcp-ng-" + "".join( random.choice(string.ascii_lowercase) for i in range(5) @@ -70,7 +70,7 @@ def generate_answerfile(directory, installer, hostname_or_ip, target_hostname, a def is_ip_active(ip): # 3 tries with a timeout of 10 sec for each ICMP request return local_cmd(['ping', '-c', '3', '-W', '10', ip], - check=False).returncode == 0 + check=False, simple_output=False).returncode == 0 def is_ssh_up(ip): try: diff --git a/tests/fs_diff/test_fs_diff.py b/tests/fs_diff/test_fs_diff.py index 51627e9a4..037a33155 100644 --- a/tests/fs_diff/test_fs_diff.py +++ b/tests/fs_diff/test_fs_diff.py @@ -17,7 +17,7 @@ def test_fs_diff(hosts): res = local_cmd([fsdiff, "--reference-host", f"{hosts[0]}", "--test-host", f"{hosts[1]}", - "--json-output"]) + "--json-output"], simple_output=False) if res.returncode != 0: print(res.stdout) diff --git a/tests/misc/test_access_links.py b/tests/misc/test_access_links.py index 1114dbba2..8caa1bb45 100644 --- a/tests/misc/test_access_links.py +++ b/tests/misc/test_access_links.py @@ -34,7 +34,7 @@ def test_access_links(host, command_id, url_id): # Verify the download worked by comparing with local download # This ensures the content is accessible and identical from both locations - local_result = commands.local_cmd(COMMAND, check=False) + local_result = commands.local_cmd(COMMAND, check=False, simple_output=False) assert local_result.returncode == 0, ( f"Failed to fetch URL locally: {local_result.stdout}" diff --git a/tests/misc/test_file_server.py b/tests/misc/test_file_server.py index e1b17f177..c880f25e1 100644 --- a/tests/misc/test_file_server.py +++ b/tests/misc/test_file_server.py @@ -18,8 +18,8 @@ def _header_equal(header, name, value): def test_fileserver_redirect_https(host): path = "/path/to/dir/file.txt" ip = wrap_ip(host.hostname_or_ip) - res = commands.local_cmd(["curl", "-s", "-i", "http://" + ip + path]) - lines = res.stdout.splitlines() + output = commands.local_cmd(["curl", "-s", "-i", "http://" + ip + path]) + lines = output.splitlines() assert lines[0].strip() == "HTTP/1.1 301 Moved Permanently" assert _header_equal(lines[2], "location", "https://" + ip + path) @@ -29,10 +29,10 @@ class TestHSTS: HSTS_HEADER_VALUE = "max-age=63072000" def __get_header(host): - res = commands.local_cmd( + output = commands.local_cmd( ["curl", "-s", "-XGET", "-k", "-I", "https://" + wrap_ip(host.hostname_or_ip)] ) - return res.stdout.splitlines() + return output.splitlines() def test_fileserver_hsts_default(self, host): # By default HSTS header should not be set diff --git a/tests/network/test_vif_allowed_ip.py b/tests/network/test_vif_allowed_ip.py index 131fa9baa..5231cdf39 100644 --- a/tests/network/test_vif_allowed_ip.py +++ b/tests/network/test_vif_allowed_ip.py @@ -12,7 +12,7 @@ def ip_responsive(ip): # 3 tries with a timeout of 10 sec for each ICMP request return local_cmd(['ping', '-c', '3', '-W', '10', ip], - check=False).returncode == 0 + check=False, simple_output=False).returncode == 0 @pytest.mark.small_vm @pytest.mark.usefixtures("host_no_sdn_controller") diff --git a/tests/packages/netdata/test_netdata.py b/tests/packages/netdata/test_netdata.py index 73c443cc2..8a6242c6e 100644 --- a/tests/packages/netdata/test_netdata.py +++ b/tests/packages/netdata/test_netdata.py @@ -18,7 +18,7 @@ def __get_headers(host, port, path=None): url += f"/{path}" res = local_cmd(["curl", "-XGET", "-k", "-I", "-s", url]) - return res.stdout.strip().splitlines() + return res.splitlines() # Verify the ActiveState for the netdata service def test_netdata_service(self, host): From e3b5c78ba8c60088e42fea14ff074f0f0ea233cc Mon Sep 17 00:00:00 2001 From: Emmanuel Varagnat Date: Thu, 13 Nov 2025 14:50:15 +0100 Subject: [PATCH 4/6] Refactor TCP port test with netcat into netutil Specific error messages are no longer available with this refactoring. Signed-off-by: Emmanuel Varagnat --- conftest.py | 6 ++---- lib/commands.py | 4 +++- lib/host.py | 7 ++----- lib/netutil.py | 15 +++++++++++++++ tests/install/conftest.py | 6 ++---- tests/install/test.py | 6 ++---- 6 files changed, 26 insertions(+), 18 deletions(-) diff --git a/conftest.py b/conftest.py index 871d60342..12c4952f5 100644 --- a/conftest.py +++ b/conftest.py @@ -25,8 +25,8 @@ vm_image, wait_for, ) -from lib.netutil import is_ipv6 from lib.host import Host +from lib.netutil import is_ipv6, wait_for_ssh from lib.pool import Pool from lib.sr import SR from lib.vm import VM, vm_cache_key_from_def @@ -203,9 +203,7 @@ def setup_host(hostname_or_ip, *, config=None): assert len(ips) == 1 host_vm.ip = ips[0] - wait_for(lambda: commands.local_cmd(['nc', '-zw5', str(host_vm.ip), '22'], - check=False, simple_output=False).returncode == 0, - "Wait for ssh up on nested host", retry_delay_secs=5) + wait_for_ssh(host_vm.ip, host_desc="nested host") hostname_or_ip = host_vm.ip diff --git a/lib/commands.py b/lib/commands.py index 681a5e98e..9bcdb63c5 100644 --- a/lib/commands.py +++ b/lib/commands.py @@ -5,7 +5,6 @@ import lib.config as config from lib.common import HostAddress -from lib.netutil import wrap_ip from typing import List, Literal, Union, overload @@ -191,6 +190,9 @@ def ssh_with_result(hostname_or_ip, cmd, suppress_fingerprint_warnings=True, assert False, "unexpected type" def scp(hostname_or_ip, src, dest, check=True, suppress_fingerprint_warnings=True, local_dest=False): + # local import to avoid cyclic import; lib.netutils also import lib.commands + from lib.netutil import wrap_ip + opts = '-o "BatchMode yes"' if suppress_fingerprint_warnings: # Suppress warnings and questions related to host key fingerprints diff --git a/lib/host.py b/lib/host.py index 7debdbd33..4be398058 100644 --- a/lib/host.py +++ b/lib/host.py @@ -32,7 +32,7 @@ wait_for, wait_for_not, ) -from lib.netutil import wrap_ip +from lib.netutil import wait_for_ssh, wrap_ip from lib.sr import SR from lib.vdi import VDI from lib.vm import VM @@ -551,10 +551,7 @@ def reboot(self, verify=False): raise if verify: wait_for_not(self.is_enabled, "Wait for host down") - wait_for(lambda: commands.local_cmd(['ping', '-c1', self.hostname_or_ip], simple_output=False).returncode == 0, - "Wait for host up", timeout_secs=10 * 60, retry_delay_secs=10) - wait_for(lambda: commands.local_cmd(['nc', '-zw5', self.hostname_or_ip, '22'], simple_output=False).returncode == 0, - "Wait for ssh up on host", timeout_secs=10 * 60, retry_delay_secs=5) + wait_for_ssh(self.hostname_or_ip) wait_for(self.is_enabled, "Wait for XAPI to be ready", timeout_secs=30 * 60) def management_network(self): diff --git a/lib/netutil.py b/lib/netutil.py index b24a1f5ab..3b58b9d0b 100644 --- a/lib/netutil.py +++ b/lib/netutil.py @@ -1,5 +1,20 @@ import socket +from lib.commands import local_cmd +from lib.common import wait_for + +def wait_for_tcp_port(host, port, port_desc, ping=True, host_desc=None): + if host_desc: + host_desc = f" ({host_desc})" + if ping: + wait_for(lambda: local_cmd(['ping', '-c1', host], check=False, simple_output=False).returncode == 0, + "Wait for host up (ICMP ping)", timeout_secs=10 * 60, retry_delay_secs=10) + wait_for(lambda: local_cmd(['nc', '-zw5', host, str(port)], check=False, simple_output=False).returncode == 0, + f"Wait for {port_desc} up on host{host_desc}", timeout_secs=10 * 60, retry_delay_secs=5) + +def wait_for_ssh(host, host_desc=None, ping=True): + wait_for_tcp_port(host, 22, "SSH", ping, host_desc) + def is_ipv6(ip): try: socket.inet_pton(socket.AF_INET6, ip) diff --git a/tests/install/conftest.py b/tests/install/conftest.py index 1dcc7b914..27af12b2c 100644 --- a/tests/install/conftest.py +++ b/tests/install/conftest.py @@ -11,6 +11,7 @@ from lib.commands import local_cmd from lib.common import callable_marker, url_download, wait_for from lib.installer import AnswerFile +from lib.netutil import wait_for_ssh from typing import Generator, Sequence, Union @@ -292,10 +293,7 @@ def vm_booted_with_installer(host, create_vms, remastered_iso): host_vm.ip = ips[0] # host may not be up if ARP cache was filled - wait_for(lambda: local_cmd(["ping", "-c1", host_vm.ip], check=False), - "Wait for host up", timeout_secs=10 * 60, retry_delay_secs=10) - wait_for(lambda: local_cmd(["nc", "-zw5", host_vm.ip, "22"], check=False), - "Wait for ssh up on host", timeout_secs=10 * 60, retry_delay_secs=5) + wait_for_ssh(host_vm.ip) yield host_vm diff --git a/tests/install/test.py b/tests/install/test.py index d5ae99e69..05bce124f 100644 --- a/tests/install/test.py +++ b/tests/install/test.py @@ -7,6 +7,7 @@ from lib import commands, installer, pxe from lib.common import safe_split, wait_for from lib.installer import AnswerFile +from lib.netutil import wait_for_ssh from lib.pif import PIF from lib.pool import Pool from lib.vdi import VDI @@ -187,10 +188,7 @@ def _test_firstboot(self, create_vms, mode, *, machine='DEFAULT', is_restore=Fal assert len(ips) == 1 host_vm.ip = ips[0] - wait_for( - lambda: commands.local_cmd( - ["nc", "-zw5", host_vm.ip, "22"], check=False).returncode == 0, - "Wait for ssh back up on Host VM", retry_delay_secs=5, timeout_secs=4 * 60) + wait_for_ssh(host_vm.ip, host_desc="host VM") logging.info("Checking installed version (expecting %r %r)", expected_dist, expected_rel) From 45c3b357ab7e2363ba33b15fb4a3a7bea777cff4 Mon Sep 17 00:00:00 2001 From: Emmanuel Varagnat Date: Fri, 21 Nov 2025 10:10:34 +0100 Subject: [PATCH 5/6] conftest: fix or hide some linter issues Signed-off-by: Emmanuel Varagnat --- conftest.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/conftest.py b/conftest.py index 12c4952f5..dbace8e70 100644 --- a/conftest.py +++ b/conftest.py @@ -4,7 +4,6 @@ import itertools import logging -import os import tempfile import git @@ -12,16 +11,13 @@ import lib.config as global_config from lib import pxe -from lib import commands from lib.common import ( - callable_marker, DiskDevName, HostAddress, + callable_marker, is_uuid, prefix_object_name, - setup_formatted_and_mounted_disk, shortened_nodeid, - teardown_formatted_and_mounted_disk, vm_image, wait_for, ) @@ -35,7 +31,9 @@ # Import package-scoped fixtures. Although we need to define them in a separate file so that we can # then import them in individual packages to fix the buggy package scope handling by pytest, we also # need to import them in the global conftest.py so that they are recognized as fixtures. -from pkgfixtures import formatted_and_mounted_ext4_disk, sr_disk_wiped +# Linters might be confused by that and see these imports as unused. The `noqa` suppresses the +# false-positive warnings. +from pkgfixtures import formatted_and_mounted_ext4_disk, sr_disk_wiped # noqa from typing import Dict, Generator, Iterable @@ -321,7 +319,7 @@ def xfail_on_xcpng_8_3(host, request): @pytest.fixture(scope='session') def host_no_ipv6(host): if is_ipv6(host.hostname_or_ip): - pytest.skip(f"This test requires an IPv4 XCP-ng") + pytest.skip("This test requires an IPv4 XCP-ng") @pytest.fixture(scope="session") def shared_sr(host): @@ -442,9 +440,7 @@ def vm_ref(request): logging.info(">> No VM specified on CLI, and no default found in test definition. Using global default.") ref = 'mini-linux-x86_64-bios' - if is_uuid(ref): - return ref - elif ref.startswith('http'): + if is_uuid(ref) or ref.startswith('http'): return ref else: return vm_image(ref) From 1e20bf25d15c3a1a7740c419af0dff5bf174c5bb Mon Sep 17 00:00:00 2001 From: Emmanuel Varagnat Date: Tue, 25 Nov 2025 10:28:36 +0100 Subject: [PATCH 6/6] Add conftest.py to ruff check Signed-off-by: Emmanuel Varagnat --- .github/workflows/code-checkers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/code-checkers.yml b/.github/workflows/code-checkers.yml index 396b6bbf4..f721e3a20 100644 --- a/.github/workflows/code-checkers.yml +++ b/.github/workflows/code-checkers.yml @@ -38,7 +38,7 @@ jobs: - uses: ./.github/actions/uv-setup/ - name: Create a dummy data.py run: cp data.py-dist data.py - - run: ruff check lib/ tests/ + - run: ruff check conftest.py lib/ tests/ flake8: runs-on: ubuntu-latest