Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(tests): move return code check for ssh into ssh.run #4943

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/framework/ab_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ def precompiled_ab_test_guest_command(

def test_runner(bin_dir, _is_a: bool):
microvm = microvm_factory(bin_dir / "firecracker", bin_dir / "jailer")
return microvm.ssh.run(command)
return microvm.ssh.run(command, check=False)

(_, old_out, old_err), (_, new_out, new_err), the_same = binary_ab_test(
test_runner,
Expand Down
12 changes: 5 additions & 7 deletions tests/framework/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,7 @@ def get_free_mem_ssh(ssh_connection):
:param ssh_connection: connection to the guest
:return: available mem column output of 'free'
"""
_, stdout, stderr = ssh_connection.run("cat /proc/meminfo | grep MemAvailable")
assert stderr == ""
_, stdout, _ = ssh_connection.run("cat /proc/meminfo | grep MemAvailable")

# Split "MemAvailable: 123456 kB" and validate it
meminfo_data = stdout.split()
Expand Down Expand Up @@ -441,7 +440,7 @@ def assert_seccomp_level(pid, seccomp_level):

def run_guest_cmd(ssh_connection, cmd, expected, use_json=False):
"""Runs a shell command at the remote accessible via SSH"""
_, stdout, stderr = ssh_connection.check_output(cmd)
_, stdout, stderr = ssh_connection.run(cmd)
assert stderr == ""
stdout = stdout if not use_json else json.loads(stdout)
assert stdout == expected
Expand Down Expand Up @@ -625,20 +624,19 @@ def guest_run_fio_iteration(ssh_connection, iteration):
--output /tmp/fio{} > /dev/null &""".format(
iteration
)
exit_code, _, stderr = ssh_connection.run(fio)
assert exit_code == 0, stderr
ssh_connection.run(fio)


def check_filesystem(ssh_connection, disk_fmt, disk):
"""Check for filesystem corruption inside a microVM."""
if disk_fmt == "squashfs":
return
ssh_connection.check_output(f"fsck.{disk_fmt} -n {disk}")
ssh_connection.run(f"fsck.{disk_fmt} -n {disk}")


def check_entropy(ssh_connection):
"""Check that we can get random numbers from /dev/hwrng"""
ssh_connection.check_output("dd if=/dev/hwrng of=/dev/null bs=4096 count=1")
ssh_connection.run("dd if=/dev/hwrng of=/dev/null bs=4096 count=1")


@retry(wait=wait_fixed(0.5), stop=stop_after_attempt(5), reraise=True)
Expand Down
2 changes: 1 addition & 1 deletion tests/framework/utils_iperf.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ def spawn_iperf3_client(self, client_idx, client_mode_flag):
.build()
)

return self._microvm.ssh.check_output(cmd).stdout
return self._microvm.ssh.run(cmd).stdout

def host_command(self, port_offset):
"""Builds the command used for spawning an iperf3 server on the host"""
Expand Down
5 changes: 2 additions & 3 deletions tests/framework/utils_vsock.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def start_guest_echo_server(vm):
Returns a UDS path to connect to the server.
"""
cmd = f"nohup socat VSOCK-LISTEN:{ECHO_SERVER_PORT},backlog=128,reuseaddr,fork EXEC:'/bin/cat' > /dev/null 2>&1 &"
vm.ssh.check_output(cmd)
vm.ssh.run(cmd)

# Give the server time to initialise
time.sleep(1)
Expand Down Expand Up @@ -214,8 +214,7 @@ def _copy_vsock_data_to_guest(ssh_connection, blob_path, vm_blob_path, vsock_hel
# Copy the data file and a vsock helper to the guest.

cmd = "mkdir -p /tmp/vsock"
ecode, _, _ = ssh_connection.run(cmd)
assert ecode == 0, "Failed to set up tmpfs drive on the guest."
ssh_connection.run(cmd)

ssh_connection.scp_put(vsock_helper, "/tmp/vsock_helper")
ssh_connection.scp_put(blob_path, vm_blob_path)
Expand Down
13 changes: 5 additions & 8 deletions tests/host_tools/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def remote_path(self, path):

def _scp(self, path1, path2, options):
"""Copy files to/from the VM using scp."""
self._exec(["scp", *options, path1, path2], check=True)
self._exec(["scp", *options, path1, path2])

def scp_put(self, local_path, remote_path, recursive=False):
"""Copy files to the VM using scp."""
Expand Down Expand Up @@ -109,11 +109,12 @@ def _init_connection(self):
We'll keep trying to execute a remote command that can't fail
(`/bin/true`), until we get a successful (0) exit code.
"""
self.check_output("true", timeout=100, debug=True)
self.run("true", timeout=100, debug=True)

def run(self, cmd_string, timeout=None, *, check=False, debug=False):
def run(self, cmd_string, timeout=None, *, check=True, debug=False):
"""
Execute the command passed as a string in the ssh context.
By default raises an exception on non-zero return code of remote command.

If `debug` is set, pass `-vvv` to `ssh`. Note that this will clobber stderr.
"""
Expand All @@ -124,11 +125,7 @@ def run(self, cmd_string, timeout=None, *, check=False, debug=False):

return self._exec(command, timeout, check=check)

def check_output(self, cmd_string, timeout=None, *, debug=False):
"""Same as `run`, but raises an exception on non-zero return code of remote command"""
return self.run(cmd_string, timeout, check=True, debug=debug)
Comment on lines -127 to -129
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We modeled check_output/run on subprocess.check_output/run. I think we should keep them as they are, as they are both useful. Most cases can be handled by check_output, but run is useful if the caller wants to handle the error.

Why not move more run calls to explicitly use check_output instead?

Copy link
Contributor Author

@ShadowCurse ShadowCurse Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier if there was only 1 method to call? By default run will check output and if we need some exception, we will explicitly set check=False.
I did not touch supbprocess.run yet here, but was going to give it the same treatment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Pablo. Ideally out run/check_output would behave exactly like subprocess.run/check_output.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, to me it's two different use cases, and how I see the subprocess API used:

  1. If I want to run a command, I use check_output because it's convenient and I don't have to mess with parameters.
  2. If I want fine control over what the command does, I use run.

subprocess.run has check=False. If we change that here to check=True, it'd be the opposite and surprising for anybody who is used to subprocess


def _exec(self, cmd, timeout=None, check=False):
def _exec(self, cmd, timeout=None, check=True):
"""Private function that handles the ssh client invocation."""
if self.netns is not None:
cmd = ["ip", "netns", "exec", self.netns] + cmd
Expand Down
27 changes: 4 additions & 23 deletions tests/integration_tests/functional/test_balloon.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,39 +41,20 @@ def get_rss_from_pmap():

def lower_ssh_oom_chance(ssh_connection):
"""Lure OOM away from ssh process"""
logger = logging.getLogger("lower_ssh_oom_chance")

cmd = "cat /run/sshd.pid"
exit_code, stdout, stderr = ssh_connection.run(cmd)
# add something to the logs for troubleshooting
if exit_code != 0:
logger.error("while running: %s", cmd)
logger.error("stdout: %s", stdout)
logger.error("stderr: %s", stderr)

_, stdout, _ = ssh_connection.run(cmd)
for pid in stdout.split(" "):
cmd = f"choom -n -1000 -p {pid}"
exit_code, stdout, stderr = ssh_connection.run(cmd)
if exit_code != 0:
logger.error("while running: %s", cmd)
logger.error("stdout: %s", stdout)
logger.error("stderr: %s", stderr)
ssh_connection.run(cmd)


def make_guest_dirty_memory(ssh_connection, amount_mib=32):
"""Tell the guest, over ssh, to dirty `amount` pages of memory."""
logger = logging.getLogger("make_guest_dirty_memory")

lower_ssh_oom_chance(ssh_connection)

cmd = f"/usr/local/bin/fillmem {amount_mib}"
try:
exit_code, stdout, stderr = ssh_connection.run(cmd, timeout=1.0)
# add something to the logs for troubleshooting
if exit_code != 0:
logger.error("while running: %s", cmd)
logger.error("stdout: %s", stdout)
logger.error("stderr: %s", stderr)
ssh_connection.run(cmd, timeout=1.0)
except TimeoutExpired:
# It's ok if this expires. Sometimes the SSH connection
# gets killed by the OOM killer *after* the fillmem program
Expand Down Expand Up @@ -558,4 +539,4 @@ def test_memory_scrub(microvm_factory, guest_kernel, rootfs):
# Wait for the deflate to complete.
_ = get_stable_rss_mem_by_pid(firecracker_pid)

microvm.ssh.check_output("/usr/local/bin/readmem {} {}".format(60, 1))
microvm.ssh.run("/usr/local/bin/readmem {} {}".format(60, 1))
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def _check_cpu_features_arm(test_microvm, guest_kv, template_name=None):
case CpuModel.ARM_NEOVERSE_V1, _, None:
expected_cpu_features = DEFAULT_G3_FEATURES_5_10

_, stdout, _ = test_microvm.ssh.check_output(CPU_FEATURES_CMD)
_, stdout, _ = test_microvm.ssh.run(CPU_FEATURES_CMD)
flags = set(stdout.strip().split(" "))
assert flags == expected_cpu_features

Expand All @@ -77,7 +77,7 @@ def test_host_vs_guest_cpu_features_aarch64(uvm_nano):
vm.add_net_iface()
vm.start()
host_feats = set(utils.check_output(CPU_FEATURES_CMD).stdout.strip().split(" "))
guest_feats = set(vm.ssh.check_output(CPU_FEATURES_CMD).stdout.strip().split(" "))
guest_feats = set(vm.ssh.run(CPU_FEATURES_CMD).stdout.strip().split(" "))

cpu_model = cpuid_utils.get_cpu_model_name()
match cpu_model:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ def test_host_vs_guest_cpu_features_x86_64(uvm_nano):
vm.add_net_iface()
vm.start()
host_feats = set(utils.check_output(CPU_FEATURES_CMD).stdout.strip().split(" "))
guest_feats = set(vm.ssh.check_output(CPU_FEATURES_CMD).stdout.strip().split(" "))
guest_feats = set(vm.ssh.run(CPU_FEATURES_CMD).stdout.strip().split(" "))

cpu_model = cpuid_utils.get_cpu_codename()
match cpu_model:
Expand Down
7 changes: 3 additions & 4 deletions tests/integration_tests/functional/test_drive_vhost_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ def _check_block_size(ssh_connection, dev_path, size):
"""
Checks the size of the block device.
"""
_, stdout, stderr = ssh_connection.run("blockdev --getsize64 {}".format(dev_path))
assert stderr == ""
_, stdout, _ = ssh_connection.run("blockdev --getsize64 {}".format(dev_path))
assert stdout.strip() == str(size)


Expand Down Expand Up @@ -297,7 +296,7 @@ def test_config_change(microvm_factory, guest_kernel, rootfs):
_check_block_size(vm.ssh, "/dev/vdb", orig_size * 1024 * 1024)

# Check that we can create a filesystem and mount it
vm.ssh.check_output(mkfs_mount_cmd)
vm.ssh.run(mkfs_mount_cmd)

for new_size in new_sizes:
# Instruct the backend to resize the device.
Expand All @@ -312,4 +311,4 @@ def test_config_change(microvm_factory, guest_kernel, rootfs):
_check_block_size(vm.ssh, "/dev/vdb", new_size * 1024 * 1024)

# Check that we can create a filesystem and mount it
vm.ssh.check_output(mkfs_mount_cmd)
vm.ssh.run(mkfs_mount_cmd)
17 changes: 7 additions & 10 deletions tests/integration_tests/functional/test_drive_virtio.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ def test_rescan_file(uvm_plain_any, io_engine):
utils.check_output(f"truncate --size {truncated_size}M {fs.path}")
block_copy_name = "/tmp/dev_vdb_copy"
_, _, stderr = test_microvm.ssh.run(
f"dd if=/dev/vdb of={block_copy_name} bs=1M count={block_size}"
f"dd if=/dev/vdb of={block_copy_name} bs=1M count={block_size}",
check=False,
)
assert "dd: error reading '/dev/vdb': Input/output error" in stderr
_check_file_size(test_microvm.ssh, f"{block_copy_name}", truncated_size * MB)
Expand Down Expand Up @@ -283,7 +284,7 @@ def test_patch_drive(uvm_plain_any, io_engine):
# of the device, in bytes.
blksize_cmd = "LSBLK_DEBUG=all lsblk -b /dev/vdb --output SIZE"
size_bytes_str = "536870912" # = 512 MiB
_, stdout, _ = test_microvm.ssh.check_output(blksize_cmd)
_, stdout, _ = test_microvm.ssh.run(blksize_cmd)
lines = stdout.split("\n")
# skip "SIZE"
assert lines[1].strip() == size_bytes_str
Expand Down Expand Up @@ -354,14 +355,12 @@ def test_flush(uvm_plain_rw, io_engine):


def _check_block_size(ssh_connection, dev_path, size):
_, stdout, stderr = ssh_connection.run("blockdev --getsize64 {}".format(dev_path))
assert stderr == ""
_, stdout, _ = ssh_connection.run("blockdev --getsize64 {}".format(dev_path))
assert stdout.strip() == str(size)


def _check_file_size(ssh_connection, dev_path, size):
_, stdout, stderr = ssh_connection.run("stat --format=%s {}".format(dev_path))
assert stderr == ""
_, stdout, _ = ssh_connection.run("stat --format=%s {}".format(dev_path))
assert stdout.strip() == str(size)


Expand All @@ -379,7 +378,5 @@ def _check_drives(test_microvm, assert_dict, keys_array):


def _check_mount(ssh_connection, dev_path):
_, _, stderr = ssh_connection.run(f"mount {dev_path} /tmp", timeout=30.0)
assert stderr == ""
_, _, stderr = ssh_connection.run("umount /tmp", timeout=30.0)
assert stderr == ""
ssh_connection.run(f"mount {dev_path} /tmp", timeout=30.0)
ssh_connection.run("umount /tmp", timeout=30.0)
4 changes: 2 additions & 2 deletions tests/integration_tests/functional/test_kvm_ptp.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def test_kvm_ptp(uvm_plain_any):
vm.add_net_iface()
vm.start()

vm.ssh.check_output("[ -c /dev/ptp0 ]")
vm.ssh.run("[ -c /dev/ptp0 ]")

# phc_ctl[14515.127]: clock time is 1697545854.728335694 or Tue Oct 17 12:30:54 2023
vm.ssh.check_output("phc_ctl /dev/ptp0 -- get")
vm.ssh.run("phc_ctl /dev/ptp0 -- get")
2 changes: 1 addition & 1 deletion tests/integration_tests/functional/test_max_devices.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def test_attach_maximum_devices(uvm_plain_any):
# Test that network devices attached are operational.
for i in range(MAX_DEVICES_ATTACHED - 1):
# Verify if guest can run commands.
test_microvm.ssh_iface(i).check_output("sync")
test_microvm.ssh_iface(i).run("sync")


@pytest.mark.skipif(
Expand Down
12 changes: 6 additions & 6 deletions tests/integration_tests/functional/test_net.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def test_high_ingress_traffic(uvm_plain_any):
test_microvm.start()

# Start iperf3 server on the guest.
test_microvm.ssh.check_output("{} -sD\n".format(IPERF_BINARY_GUEST))
test_microvm.ssh.run("{} -sD\n".format(IPERF_BINARY_GUEST))
time.sleep(1)

# Start iperf3 client on the host. Send 1Gbps UDP traffic.
Expand All @@ -53,7 +53,7 @@ def test_high_ingress_traffic(uvm_plain_any):
# Check if the high ingress traffic broke the net interface.
# If the net interface still works we should be able to execute
# ssh commands.
test_microvm.ssh.check_output("echo success\n")
test_microvm.ssh.run("echo success\n")


def test_multi_queue_unsupported(uvm_plain):
Expand Down Expand Up @@ -97,8 +97,8 @@ def run_udp_offload_test(vm):
message = "x"

# Start a UDP server in the guest
# vm.ssh.check_output(f"nohup socat UDP-LISTEN:{port} - > {out_filename} &")
vm.ssh.check_output(
# vm.ssh.run(f"nohup socat UDP-LISTEN:{port} - > {out_filename} &")
vm.ssh.run(
f"nohup socat UDP4-LISTEN:{port} OPEN:{out_filename},creat > /dev/null 2>&1 &"
)

Expand All @@ -110,8 +110,8 @@ def run_udp_offload_test(vm):
assert ret.returncode == 0, f"{ret.stdout=} {ret.stderr=}"

# Check that the server received the message
ret = vm.ssh.run(f"cat {out_filename}")
assert ret.stdout == message, f"{ret.stdout=} {ret.stderr=}"
_, stdout, stderr = vm.ssh.run(f"cat {out_filename}")
assert stdout == message, f"{stdout=} {stderr=}"


def test_tap_offload_booted(uvm_plain_any):
Expand Down
Loading
Loading