diff --git a/tests/conftest.py b/tests/conftest.py index 817f3f2970e..7c0ac0b721c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,8 +1,6 @@ # Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -# Pytest fixtures and redefined-outer-name don't mix well. Disable it. -# pylint:disable=redefined-outer-name # We import some fixtures that are unused. Disable that too. # pylint:disable=unused-import diff --git a/tests/framework/ab_test.py b/tests/framework/ab_test.py index 8b7204baf20..6c4e2089163 100644 --- a/tests/framework/ab_test.py +++ b/tests/framework/ab_test.py @@ -23,6 +23,7 @@ """ import contextlib import os +import shutil import statistics from pathlib import Path from tempfile import TemporaryDirectory @@ -31,6 +32,11 @@ import scipy from framework import utils +from framework.defs import FC_WORKSPACE_DIR +from framework.microvm import Microvm +from framework.utils import CommandReturn +from framework.with_filelock import with_filelock +from host_tools.cargo_build import get_firecracker_binaries # Locally, this will always compare against main, even if we try to merge into, say, a feature branch. # We might want to do a more sophisticated way to determine a "parent" branch here. @@ -100,6 +106,133 @@ def git_ab_test( return result_a, result_b, comparison +def is_pr() -> bool: + """Returns `True` iff we are executing in the context of a build kite run on a pull request""" + return os.environ.get("BUILDKITE_PULL_REQUEST", "false") != "false" + + +def git_ab_test_host_command_if_pr( + command: str, + *, + comparator: Callable[[CommandReturn, CommandReturn], bool] = default_comparator, +): + """Runs the given bash command as an A/B-Test if we're in a pull request context (asserting that its stdout and + stderr did not change across the PR). Otherwise runs the command, asserting it returns a zero exit code + """ + if is_pr(): + git_ab_test_host_command(command, comparator=comparator) + else: + utils.run_cmd(command) + + +def git_ab_test_host_command( + command: str, + *, + comparator: Callable[[CommandReturn, CommandReturn], bool] = default_comparator, + a_revision: str = DEFAULT_A_REVISION, + b_revision: Optional[str] = None, +): + """Performs an A/B-Test of the specified command, asserting that both the A and B invokations return the same stdout/stderr""" + (_, old_out, old_err), (_, new_out, new_err), the_same = git_ab_test( + lambda path, _is_a: utils.run_cmd(command, ignore_return_code=True, cwd=path), + comparator, + a_revision=a_revision, + b_revision=b_revision, + ) + + assert ( + the_same + ), f"The output of running command `{command}` changed:\nOld:\nstdout:\n{old_out}\nstderr:\n{old_err}\n\nNew:\nstdout:\n{new_out}\nstderr:\n{new_err}" + + +def set_did_not_grow_comparator( + set_generator: Callable[[CommandReturn], set] +) -> Callable[[CommandReturn, CommandReturn], bool]: + """Factory function for comparators to use with git_ab_test_command that converts the command output to sets + (using the given callable) and then checks that the "B" set is a subset of the "A" set + """ + return lambda output_a, output_b: set_generator(output_b).issubset( + set_generator(output_a) + ) + + +def git_ab_test_with_binaries( + test_runner: Callable[[Path, Path], T], + comparator: Callable[[T, T], U] = default_comparator, + *, + a_revision: str = DEFAULT_A_REVISION, + b_revision: Optional[str] = None, +) -> (T, T, U): + """Similar to `git_ab_test`, with the only difference being that this function compiles firecracker at the specified + revisions and only passes the firecracker binaries to the test_runner. Maintains a cache of previously compiled + revisions, to prevent excessive recompilation across different tests of the same revision + """ + + @with_filelock + def grab_binaries(checkout: Path): + with chdir(checkout): + revision = utils.run_cmd("git rev-parse HEAD").stdout.strip() + + revision_store = FC_WORKSPACE_DIR / "build" / revision + if not revision_store.exists(): + firecracker, jailer = get_firecracker_binaries(workspace_dir=checkout) + + revision_store.mkdir(parents=True, exist_ok=True) + shutil.copy(firecracker, revision_store / "firecracker") + shutil.copy(jailer, revision_store / "jailer") + + return ( + revision_store / "firecracker", + revision_store / "jailer", + ) + + return git_ab_test( + lambda checkout, _is_a: test_runner(*grab_binaries(checkout)), + comparator, + a_revision=a_revision, + b_revision=b_revision, + ) + + +def git_ab_test_guest_command( + microvm_factory: Callable[[Path, Path], Microvm], + command: str, + *, + comparator: Callable[[CommandReturn, CommandReturn], bool] = default_comparator, + a_revision: str = DEFAULT_A_REVISION, + b_revision: Optional[str] = None, +): + """The same as git_ab_test_command, but via SSH. The closure argument should setup a microvm using the passed + paths to firecracker and jailer binaries.""" + + def test_runner(firecracker, jailer): + microvm = microvm_factory(firecracker, jailer) + return microvm.ssh.run(command) + + (_, old_out, old_err), (_, new_out, new_err), the_same = git_ab_test_with_binaries( + test_runner, comparator, a_revision=a_revision, b_revision=b_revision + ) + + assert ( + the_same + ), f"The output of running command `{command}` changed:\nOld:\nstdout:\n{old_out}\nstderr\n{old_err}\n\nNew:\nstdout:\n{new_out}\nstderr:\n{new_err}" + + +def git_ab_test_guest_command_if_pr( + microvm_factory: Callable[[Path, Path], Microvm], + command: str, + *, + comparator=default_comparator, +): + """The same as git_ab_test_command_if_pr, but via SSH""" + if is_pr(): + git_ab_test_guest_command(microvm_factory, command, comparator=comparator) + else: + microvm = microvm_factory(*get_firecracker_binaries()) + ecode, stdout, stderr = microvm.ssh.run(command) + assert ecode == 0, f"stdout:\n{stdout}\nstderr:\n{stderr}\n" + + def check_regression( a_samples: List[float], b_samples: List[float], *, n_resamples: int = 9999 ): @@ -147,6 +280,10 @@ def temporary_checkout(revision: str): yield Path(tmp_dir) + # If we compiled firecracker inside the checkout, python's recursive shutil.rmdir will + # run incredibly long. Thus, remove manually. + utils.run_cmd(f"rm -rf {tmp_dir}") + # Once we upgrade to python 3.11, this will be in contextlib: # https://docs.python.org/3/library/contextlib.html#contextlib.chdir diff --git a/tests/framework/defs.py b/tests/framework/defs.py index ebc25ebcdc7..5fec9cca1a4 100644 --- a/tests/framework/defs.py +++ b/tests/framework/defs.py @@ -11,9 +11,6 @@ # The Firecracker sources workspace dir FC_WORKSPACE_DIR = Path(__file__).parent.parent.parent.resolve() -# Cargo target dir for the Firecracker workspace. Set via .cargo/config -FC_WORKSPACE_TARGET_DIR = FC_WORKSPACE_DIR / "build/cargo_target" - # Cargo build directory for seccompiler SECCOMPILER_TARGET_DIR = FC_WORKSPACE_DIR / "build/seccompiler" diff --git a/tests/host_tools/cargo_build.py b/tests/host_tools/cargo_build.py index 36fb31ea1f0..2b3dd11c491 100644 --- a/tests/host_tools/cargo_build.py +++ b/tests/host_tools/cargo_build.py @@ -7,7 +7,7 @@ from pathlib import Path from framework import defs, utils -from framework.defs import FC_WORKSPACE_DIR, FC_WORKSPACE_TARGET_DIR +from framework.defs import FC_WORKSPACE_DIR from framework.with_filelock import with_filelock CARGO_BUILD_REL_PATH = "firecracker_binaries" @@ -66,32 +66,33 @@ def cargo_test(path, extra_args=""): @with_filelock -def get_binary(name): +def get_binary(name, *, workspace_dir=FC_WORKSPACE_DIR): """Build a binary""" target = DEFAULT_BUILD_TARGET - target_dir = FC_WORKSPACE_TARGET_DIR - out_dir = Path(f"{target_dir}/{target}/release") - bin_path = out_dir / name + target_dir = workspace_dir / "build" / "cargo_target" + bin_path = target_dir / target / "release" / name if not bin_path.exists(): env = {"RUSTFLAGS": get_rustflags()} cargo( "build", f"-p {name} --release --target {target}", env=env, - cwd=FC_WORKSPACE_DIR, + cwd=workspace_dir, ) utils.run_cmd(f"strip --strip-debug {bin_path}") return bin_path @with_filelock -def get_firecracker_binaries(): +def get_firecracker_binaries(*, workspace_dir=FC_WORKSPACE_DIR): """Build the Firecracker and Jailer binaries if they don't exist. Returns the location of the firecracker related binaries eventually after building them in case they do not exist at the specified root_path. """ - return get_binary("firecracker"), get_binary("jailer") + return get_binary("firecracker", workspace_dir=workspace_dir), get_binary( + "jailer", workspace_dir=workspace_dir + ) @with_filelock diff --git a/tests/host_tools/ip_generator.py b/tests/host_tools/ip_generator.py index ab93b9c386c..0d09182be1e 100644 --- a/tests/host_tools/ip_generator.py +++ b/tests/host_tools/ip_generator.py @@ -1,8 +1,6 @@ # Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -# pylint:disable=redefined-outer-name - """ Each microVM needs to have a unique IP on the host network, or there will be conflicts. diff --git a/tests/integration_tests/functional/test_drives.py b/tests/integration_tests/functional/test_drives.py index fa5a254f221..a9f0d49d112 100644 --- a/tests/integration_tests/functional/test_drives.py +++ b/tests/integration_tests/functional/test_drives.py @@ -2,8 +2,6 @@ # SPDX-License-Identifier: Apache-2.0 """Tests for guest-side operations on /drives resources.""" -# pylint:disable=redefined-outer-name - import os from subprocess import check_output diff --git a/tests/integration_tests/functional/test_rng.py b/tests/integration_tests/functional/test_rng.py index 2bae5987ca5..54da6291cc2 100644 --- a/tests/integration_tests/functional/test_rng.py +++ b/tests/integration_tests/functional/test_rng.py @@ -2,8 +2,6 @@ # SPDX-License-Identifier: Apache-2.0 """Tests for the virtio-rng device""" -# pylint:disable=redefined-outer-name - import pytest from framework.properties import global_props diff --git a/tests/integration_tests/performance/conftest.py b/tests/integration_tests/performance/conftest.py index ea4881dd746..3b7e50a01cb 100644 --- a/tests/integration_tests/performance/conftest.py +++ b/tests/integration_tests/performance/conftest.py @@ -1,9 +1,6 @@ # Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -# Pytest fixtures and redefined-outer-name don't mix well. Disable it. -# pylint:disable=redefined-outer-name - """Fixtures for performance tests""" import json diff --git a/tests/integration_tests/performance/test_boottime.py b/tests/integration_tests/performance/test_boottime.py index 6042eeb1d06..561bb0d5190 100644 --- a/tests/integration_tests/performance/test_boottime.py +++ b/tests/integration_tests/performance/test_boottime.py @@ -2,8 +2,6 @@ # SPDX-License-Identifier: Apache-2.0 """Tests that ensure the boot time to init process is within spec.""" -# pylint:disable=redefined-outer-name - import re import time diff --git a/tests/integration_tests/performance/test_network_ab.py b/tests/integration_tests/performance/test_network_ab.py index f8ca9244ec3..689d93fad8f 100644 --- a/tests/integration_tests/performance/test_network_ab.py +++ b/tests/integration_tests/performance/test_network_ab.py @@ -67,9 +67,7 @@ def network_microvm(request, microvm_factory, guest_kernel, rootfs): @pytest.mark.nonci @pytest.mark.parametrize("network_microvm", [1], indirect=True) -def test_network_latency( - network_microvm, metrics -): # pylint:disable=redefined-outer-name +def test_network_latency(network_microvm, metrics): """ Test network latency for multiple vm configurations. @@ -134,7 +132,7 @@ def test_network_tcp_throughput( payload_length, mode, metrics, -): # pylint:disable=redefined-outer-name +): """ Iperf between guest and host in both directions for TCP workload. """ diff --git a/tests/integration_tests/performance/test_process_startup_time.py b/tests/integration_tests/performance/test_process_startup_time.py index d7418068ec3..8a874a8d1ff 100644 --- a/tests/integration_tests/performance/test_process_startup_time.py +++ b/tests/integration_tests/performance/test_process_startup_time.py @@ -2,8 +2,6 @@ # SPDX-License-Identifier: Apache-2.0 """Test that the process startup time up to socket bind is within spec.""" -# pylint: disable=redefined-outer-name - import os import time diff --git a/tests/integration_tests/security/test_jail.py b/tests/integration_tests/security/test_jail.py index cf0f8b53d24..dc9b5065dd7 100644 --- a/tests/integration_tests/security/test_jail.py +++ b/tests/integration_tests/security/test_jail.py @@ -2,8 +2,6 @@ # SPDX-License-Identifier: Apache-2.0 """Tests that verify the jailer's behavior.""" -# pylint: disable=redefined-outer-name - import functools import http.client as http_client import os diff --git a/tests/integration_tests/security/test_sec_audit.py b/tests/integration_tests/security/test_sec_audit.py index 1a86178e343..e8265c3ae2a 100644 --- a/tests/integration_tests/security/test_sec_audit.py +++ b/tests/integration_tests/security/test_sec_audit.py @@ -1,26 +1,40 @@ # Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 """Tests ensuring security vulnerabilities are not present in dependencies.""" - +import json import pytest -from framework import defs +from framework.ab_test import ( + git_ab_test_host_command_if_pr, + set_did_not_grow_comparator, +) +from framework.utils import CommandReturn from framework.utils_cpuid import CpuVendor, get_cpu_vendor -from host_tools.cargo_build import cargo @pytest.mark.skipif( get_cpu_vendor() != CpuVendor.INTEL, - reason="The audit is based on cargo.lock which " "is identical on all platforms", + reason="The audit is based on cargo.lock which is identical on all platforms", ) def test_cargo_audit(): """ Run cargo audit to check for crates with security vulnerabilities. """ - # Run command and raise exception if non-zero return code - cargo( - "audit", - "--deny warnings -q", - cwd=defs.FC_WORKSPACE_DIR, + + def set_of_vulnerabilities(output: CommandReturn): + output = json.loads(output.stdout) + + return set( + frozenset(vulnerability) + for vulnerability in output["vulnerabilities"]["list"] + ).union( + frozenset(warning) + for warning_kind, warnings in output["warnings"].items() + for warning in warnings + ) + + git_ab_test_host_command_if_pr( + "cargo audit --deny warnings -q --json", + comparator=set_did_not_grow_comparator(set_of_vulnerabilities), ) diff --git a/tests/integration_tests/security/test_seccomp.py b/tests/integration_tests/security/test_seccomp.py index 28be679268b..99d1a7f98e7 100644 --- a/tests/integration_tests/security/test_seccomp.py +++ b/tests/integration_tests/security/test_seccomp.py @@ -90,7 +90,6 @@ def test_seccomp_ls(bin_seccomp_paths): """ Assert that the seccomp filter denies an unallowed syscall. """ - # pylint: disable=redefined-outer-name # pylint: disable=subprocess-run-check # The fixture pattern causes a pylint false positive for that rule. @@ -135,7 +134,6 @@ def test_advanced_seccomp(bin_seccomp_paths): binary, denies the malicious demo binary and that an empty allowlist denies everything. """ - # pylint: disable=redefined-outer-name # pylint: disable=subprocess-run-check # The fixture pattern causes a pylint false positive for that rule. @@ -272,7 +270,6 @@ def test_seccomp_rust_panic(bin_seccomp_paths): Test that the Firecracker filters allow a Rust panic to run its course without triggering a seccomp violation. """ - # pylint: disable=redefined-outer-name # pylint: disable=subprocess-run-check # The fixture pattern causes a pylint false positive for that rule. diff --git a/tests/integration_tests/security/test_vulnerabilities.py b/tests/integration_tests/security/test_vulnerabilities.py index eb009518a46..a5ca3433c30 100644 --- a/tests/integration_tests/security/test_vulnerabilities.py +++ b/tests/integration_tests/security/test_vulnerabilities.py @@ -5,59 +5,141 @@ # script from the third party "Spectre & Meltdown Checker" project. This script is under the # GPL-3.0-only license. """Tests vulnerabilities mitigations.""" - +import json import os import pytest import requests -from framework import utils +from framework.ab_test import ( + git_ab_test_guest_command, + git_ab_test_guest_command_if_pr, + git_ab_test_host_command_if_pr, + is_pr, + set_did_not_grow_comparator, +) from framework.properties import global_props +from framework.utils import CommandReturn from framework.utils_cpu_templates import nonci_on_arm CHECKER_URL = "https://meltdown.ovh" CHECKER_FILENAME = "spectre-meltdown-checker.sh" +REMOTE_CHECKER_PATH = f"/tmp/{CHECKER_FILENAME}" +REMOTE_CHECKER_COMMAND = f"sh {REMOTE_CHECKER_PATH} --no-intel-db --batch json" +VULN_DIR = "/sys/devices/system/cpu/vulnerabilities" -@pytest.fixture(name="microvm") -def microvm_fxt(uvm_plain): - """Microvm fixture""" - uvm_plain.spawn() - uvm_plain.basic_config( - vcpu_count=2, - mem_size_mib=256, + +def configure_microvm( + factory, + kernel, + rootfs, + *, + firecracker=None, + jailer=None, + cpu_template=None, + custom_cpu_template=None, +): + """Build a microvm for vulnerability tests""" + assert not (cpu_template and custom_cpu_template) + # Either both or neither are specified + assert firecracker and jailer or not firecracker and not jailer + + if firecracker: + microvm = factory.build( + kernel, rootfs, fc_binary_path=firecracker, jailer_binary_path=jailer + ) + else: + microvm = factory.build(kernel, rootfs) + + microvm.spawn() + microvm.basic_config(vcpu_count=2, mem_size_mib=256, cpu_template=cpu_template) + if custom_cpu_template: + microvm.api.cpu_config.put(**custom_cpu_template["template"]) + microvm.cpu_template = cpu_template or custom_cpu_template + microvm.add_net_iface() + microvm.start() + return microvm + + +@pytest.fixture +def build_microvm( + microvm_factory, + guest_kernel_linux_5_10, + rootfs_ubuntu_22, +): + """Fixture returning a factory function for a normal microvm""" + return lambda firecracker=None, jailer=None: configure_microvm( + microvm_factory, + guest_kernel_linux_5_10, + rootfs_ubuntu_22, + firecracker=firecracker, + jailer=jailer, ) - uvm_plain.add_net_iface() - uvm_plain.start() - return uvm_plain - - -@pytest.fixture(name="microvm_with_template") -def microvm_with_template_fxt(uvm_plain, cpu_template): - """Microvm fixture with a CPU template""" - uvm_plain.spawn() - uvm_plain.basic_config( - vcpu_count=2, - mem_size_mib=256, + + +@pytest.fixture +def build_microvm_with_template( + microvm_factory, guest_kernel_linux_5_10, rootfs_ubuntu_22, cpu_template +): + """Fixture returning a factory function for microvms with our built-in template""" + return lambda firecracker=None, jailer=None: configure_microvm( + microvm_factory, + guest_kernel_linux_5_10, + rootfs_ubuntu_22, + firecracker=firecracker, + jailer=jailer, cpu_template=cpu_template, ) - uvm_plain.add_net_iface() - uvm_plain.start() - return uvm_plain, cpu_template - - -@pytest.fixture(name="microvm_with_custom_template") -def microvm_with_custom_template_fxt(uvm_plain, custom_cpu_template): - """Microvm fixture with a CPU template""" - uvm_plain.spawn() - uvm_plain.basic_config( - vcpu_count=2, - mem_size_mib=256, + + +@pytest.fixture +def build_microvm_with_custom_template( + microvm_factory, guest_kernel_linux_5_10, rootfs_ubuntu_22, custom_cpu_template +): + """Fixture returning a factory function for microvms with custom cpu templates""" + return lambda firecracker=None, jailer=None: configure_microvm( + microvm_factory, + guest_kernel_linux_5_10, + rootfs_ubuntu_22, + firecracker=firecracker, + jailer=jailer, + custom_cpu_template=custom_cpu_template, ) - uvm_plain.api.cpu_config.put(**custom_cpu_template["template"]) - uvm_plain.add_net_iface() - uvm_plain.start() - return uvm_plain, custom_cpu_template["name"] + + +def with_restore(factory, microvm_factory): + """Turns the given microvm factory into one that makes the microvm go through a snapshot-restore cycle""" + + def restore(firecracker=None, jailer=None): + microvm = factory(firecracker, jailer) + snapshot = microvm.snapshot_full() + + if firecracker: + dst_vm = microvm_factory.build( + fc_binary_path=firecracker, jailer_binary_path=jailer + ) + else: + dst_vm = microvm_factory.build() + dst_vm.spawn() + # Restore the destination VM from the snapshot + dst_vm.restore_from_snapshot(snapshot, resume=True) + dst_vm.cpu_template = microvm.cpu_template + + return dst_vm + + return restore + + +def with_checker(factory, spectre_meltdown_checker): + """Turns the given microvm factory function into one that also contains the spectre-meltdown checker script""" + + def download_checker(firecracker, jailer): + microvm = factory(firecracker, jailer) + microvm.ssh.scp_put(spectre_meltdown_checker, REMOTE_CHECKER_PATH) + return microvm + + return download_checker @pytest.fixture(scope="session", name="spectre_meltdown_checker") @@ -72,18 +154,17 @@ def download_spectre_meltdown_checker(tmp_path_factory): return path -def run_spectre_meltdown_checker_on_guest( - microvm, - spectre_meltdown_checker, -): - """Run the spectre / meltdown checker on guest""" - remote_path = f"/tmp/{CHECKER_FILENAME}" - microvm.ssh.scp_put(spectre_meltdown_checker, remote_path) - ecode, stdout, stderr = microvm.ssh.run(f"sh {remote_path} --explain --no-intel-db") - assert ecode == 0, f"stdout:\n{stdout}\nstderr:\n{stderr}\n" +def spectre_meltdown_reported_vulnerablities( + spectre_meltdown_checker_output: CommandReturn, +) -> set: + """Parses the output of `spectre-meltdown-checker.sh --batch json` and returns the set of issues for which it reported 'Vulnerable'""" + return { + frozenset(entry) # cannot hash dicts + for entry in json.loads(spectre_meltdown_checker_output.stdout) + if entry["VULNERABLE"] + } -@pytest.mark.no_block_pr @pytest.mark.skipif( global_props.instance == "c7g.metal" and global_props.host_linux_version == "4.14", reason="c7g host 4.14 requires modifications to the 5.10 guest kernel to boot successfully.", @@ -92,144 +173,137 @@ def test_spectre_meltdown_checker_on_host(spectre_meltdown_checker): """ Test with the spectre / meltdown checker on host. """ - utils.run_cmd(f"sh {spectre_meltdown_checker} --explain") + git_ab_test_host_command_if_pr( + f"sh {spectre_meltdown_checker} --batch json", + comparator=set_did_not_grow_comparator( + spectre_meltdown_reported_vulnerablities + ), + ) -@pytest.mark.no_block_pr @pytest.mark.skipif( global_props.instance == "c7g.metal" and global_props.host_linux_version == "4.14", reason="c7g host 4.14 requires modifications to the 5.10 guest kernel to boot successfully.", ) -def test_spectre_meltdown_checker_on_guest(spectre_meltdown_checker, microvm): +def test_spectre_meltdown_checker_on_guest(spectre_meltdown_checker, build_microvm): """ Test with the spectre / meltdown checker on guest. """ - run_spectre_meltdown_checker_on_guest( - microvm, - spectre_meltdown_checker, + + git_ab_test_guest_command_if_pr( + with_checker(build_microvm, spectre_meltdown_checker), + REMOTE_CHECKER_COMMAND, + comparator=set_did_not_grow_comparator( + spectre_meltdown_reported_vulnerablities + ), ) -@pytest.mark.no_block_pr @pytest.mark.skipif( global_props.instance == "c7g.metal" and global_props.host_linux_version == "4.14", reason="c7g host 4.14 requires modifications to the 5.10 guest kernel to boot successfully.", ) def test_spectre_meltdown_checker_on_restored_guest( - spectre_meltdown_checker, - microvm, - microvm_factory, + spectre_meltdown_checker, build_microvm, microvm_factory ): """ Test with the spectre / meltdown checker on a restored guest. """ - - snapshot = microvm.snapshot_full() - # Create a destination VM - dst_vm = microvm_factory.build() - dst_vm.spawn() - # Restore the destination VM from the snapshot - dst_vm.restore_from_snapshot(snapshot, resume=True) - - run_spectre_meltdown_checker_on_guest( - dst_vm, - spectre_meltdown_checker, + git_ab_test_guest_command_if_pr( + with_checker( + with_restore(build_microvm, microvm_factory), spectre_meltdown_checker + ), + REMOTE_CHECKER_COMMAND, + comparator=set_did_not_grow_comparator( + spectre_meltdown_reported_vulnerablities + ), ) -@pytest.mark.no_block_pr @pytest.mark.skipif( global_props.instance == "c7g.metal" and global_props.host_linux_version == "4.14", reason="c7g host 4.14 requires modifications to the 5.10 guest kernel to boot successfully.", ) @nonci_on_arm def test_spectre_meltdown_checker_on_guest_with_template( - spectre_meltdown_checker, - microvm_with_template, + spectre_meltdown_checker, build_microvm_with_template ): """ Test with the spectre / meltdown checker on guest with CPU template. """ - microvm, _template = microvm_with_template - run_spectre_meltdown_checker_on_guest( - microvm, - spectre_meltdown_checker, + + git_ab_test_guest_command_if_pr( + with_checker(build_microvm_with_template, spectre_meltdown_checker), + REMOTE_CHECKER_COMMAND, + comparator=set_did_not_grow_comparator( + spectre_meltdown_reported_vulnerablities + ), ) -@pytest.mark.no_block_pr @pytest.mark.skipif( global_props.instance == "c7g.metal" and global_props.host_linux_version == "4.14", reason="c7g host 4.14 requires modifications to the 5.10 guest kernel to boot successfully.", ) @nonci_on_arm def test_spectre_meltdown_checker_on_guest_with_custom_template( - spectre_meltdown_checker, - microvm_with_custom_template, + spectre_meltdown_checker, build_microvm_with_custom_template ): """ Test with the spectre / meltdown checker on guest with a custom CPU template. """ - microvm, _template = microvm_with_custom_template - run_spectre_meltdown_checker_on_guest( - microvm, - spectre_meltdown_checker, + git_ab_test_guest_command_if_pr( + with_checker(build_microvm_with_custom_template, spectre_meltdown_checker), + REMOTE_CHECKER_COMMAND, + comparator=set_did_not_grow_comparator( + spectre_meltdown_reported_vulnerablities + ), ) -@pytest.mark.no_block_pr @pytest.mark.skipif( global_props.instance == "c7g.metal" and global_props.host_linux_version == "4.14", reason="c7g host 4.14 requires modifications to the 5.10 guest kernel to boot successfully.", ) @nonci_on_arm def test_spectre_meltdown_checker_on_restored_guest_with_template( - spectre_meltdown_checker, - microvm_with_template, - microvm_factory, + spectre_meltdown_checker, build_microvm_with_template, microvm_factory ): """ Test with the spectre / meltdown checker on a restored guest with a CPU template. """ - microvm, _template = microvm_with_template - snapshot = microvm.snapshot_full() - # Create a destination VM - dst_vm = microvm_factory.build() - dst_vm.spawn() - # Restore the destination VM from the snapshot - dst_vm.restore_from_snapshot(snapshot, resume=True) - - run_spectre_meltdown_checker_on_guest( - dst_vm, - spectre_meltdown_checker, + git_ab_test_guest_command_if_pr( + with_checker( + with_restore(build_microvm_with_template, microvm_factory), + spectre_meltdown_checker, + ), + REMOTE_CHECKER_COMMAND, + comparator=set_did_not_grow_comparator( + spectre_meltdown_reported_vulnerablities + ), ) -@pytest.mark.no_block_pr @pytest.mark.skipif( global_props.instance == "c7g.metal" and global_props.host_linux_version == "4.14", reason="c7g host 4.14 requires modifications to the 5.10 guest kernel to boot successfully.", ) @nonci_on_arm def test_spectre_meltdown_checker_on_restored_guest_with_custom_template( - spectre_meltdown_checker, - microvm_with_custom_template, - microvm_factory, + spectre_meltdown_checker, build_microvm_with_custom_template, microvm_factory ): """ Test with the spectre / meltdown checker on a restored guest with a custom CPU template. """ - - src_vm, _template = microvm_with_custom_template - snapshot = src_vm.snapshot_full() - dst_vm = microvm_factory.build() - dst_vm.spawn() - # Restore the destination VM from the snapshot - dst_vm.restore_from_snapshot(snapshot, resume=True) - - run_spectre_meltdown_checker_on_guest( - dst_vm, - spectre_meltdown_checker, + git_ab_test_guest_command_if_pr( + with_checker( + with_restore(build_microvm_with_custom_template, microvm_factory), + spectre_meltdown_checker, + ), + REMOTE_CHECKER_COMMAND, + comparator=set_did_not_grow_comparator( + spectre_meltdown_reported_vulnerablities + ), ) @@ -280,22 +354,20 @@ def get_vuln_files_exception_dict(template): return exception_dict -@pytest.mark.no_block_pr def test_vulnerabilities_on_host(): """ Test vulnerabilities files on host. """ - vuln_dir = "/sys/devices/system/cpu/vulnerabilities" - # `grep` returns 1 if no lines were selected. - ecode, stdout, stderr = utils.run_cmd( - f"grep -r Vulnerable {vuln_dir}", ignore_return_code=True + git_ab_test_host_command_if_pr( + f"! grep -r Vulnerable {VULN_DIR}", + comparator=set_did_not_grow_comparator( + lambda output: set(output.stdout.splitlines()) + ), ) - assert ecode == 1, f"stdout:\n{stdout}\nstderr:\n{stderr}\n" -@pytest.mark.no_block_pr -def check_vulnerabilities_files_on_guest(microvm, template=None): +def check_vulnerabilities_files_on_guest(microvm): """ Check that the guest's vulnerabilities files do not contain `Vulnerable`. See also: https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-devices-system-cpu @@ -307,6 +379,9 @@ def check_vulnerabilities_files_on_guest(microvm, template=None): assert ecode == 0, f"stdout:\n{stdout}\nstderr:\n{stderr}\n" vuln_files = stdout.split("\n") + # Fixtures in this file (test_vulnerabilities.py) add this special field. + template = microvm.cpu_template + # Check that vulnerabilities files in the exception dictionary have the expected values and # the others do not contain "Vulnerable". exceptions = get_vuln_files_exception_dict(template) @@ -318,93 +393,76 @@ def check_vulnerabilities_files_on_guest(microvm, template=None): else: cmd = f"grep Vulnerable {vuln_file}" ecode, stdout, stderr = microvm.ssh.run(cmd) - assert ecode == 1, f"stdout:\n{stdout}\nstderr:\n{stderr}\n" + assert ecode == 1, f"{vuln_file}: stdout:\n{stdout}\nstderr:\n{stderr}\n" -@pytest.mark.no_block_pr -def test_vulnerabilities_files_on_guest(microvm): +def check_vulnerabilities_files_ab(builder): + """Does an A/B test on the contents of the /sys/devices/system/cpu/vulnerabilities files in the guest if + running in a PR pipeline, and otherwise calls `check_vulnerabilities_files_on_guest` + """ + if is_pr(): + git_ab_test_guest_command( + builder, + f"! grep -r Vulnerable {VULN_DIR}", + comparator=set_did_not_grow_comparator( + lambda output: set(output.stdout.splitlines()) + ), + ) + else: + check_vulnerabilities_files_on_guest(builder()) + + +def test_vulnerabilities_files_on_guest(build_microvm): """ Test vulnerabilities files on guest. """ - check_vulnerabilities_files_on_guest(microvm) + check_vulnerabilities_files_ab(build_microvm) -@pytest.mark.no_block_pr -def test_vulnerabilities_files_on_restored_guest( - microvm, - microvm_factory, -): +def test_vulnerabilities_files_on_restored_guest(build_microvm, microvm_factory): """ Test vulnerabilities files on a restored guest. """ - snapshot = microvm.snapshot_full() - # Create a destination VM - dst_vm = microvm_factory.build() - dst_vm.spawn() - # Restore the destination VM from the snapshot - dst_vm.restore_from_snapshot(snapshot, resume=True) - - check_vulnerabilities_files_on_guest(dst_vm) + check_vulnerabilities_files_ab(with_restore(build_microvm, microvm_factory)) -@pytest.mark.no_block_pr @nonci_on_arm -def test_vulnerabilities_files_on_guest_with_template( - microvm_with_template, -): +def test_vulnerabilities_files_on_guest_with_template(build_microvm_with_template): """ Test vulnerabilities files on guest with CPU template. """ - microvm, template = microvm_with_template - check_vulnerabilities_files_on_guest(microvm, template) + check_vulnerabilities_files_ab(build_microvm_with_template) -@pytest.mark.no_block_pr @nonci_on_arm def test_vulnerabilities_files_on_guest_with_custom_template( - microvm_with_custom_template, + build_microvm_with_custom_template, ): """ Test vulnerabilities files on guest with a custom CPU template. """ - microvm, template = microvm_with_custom_template - check_vulnerabilities_files_on_guest(microvm, template) + check_vulnerabilities_files_ab(build_microvm_with_custom_template) -@pytest.mark.no_block_pr @nonci_on_arm def test_vulnerabilities_files_on_restored_guest_with_template( - microvm_with_template, - microvm_factory, + build_microvm_with_template, microvm_factory ): """ Test vulnerabilities files on a restored guest with a CPU template. """ - microvm, template = microvm_with_template - snapshot = microvm.snapshot_full() - # Create a destination VM - dst_vm = microvm_factory.build() - dst_vm.spawn() - # Restore the destination VM from the snapshot - dst_vm.restore_from_snapshot(snapshot, resume=True) - - check_vulnerabilities_files_on_guest(dst_vm, template) + check_vulnerabilities_files_ab( + with_restore(build_microvm_with_template, microvm_factory) + ) -@pytest.mark.no_block_pr @nonci_on_arm def test_vulnerabilities_files_on_restored_guest_with_custom_template( - microvm_with_custom_template, - microvm_factory, + build_microvm_with_custom_template, microvm_factory ): """ Test vulnerabilities files on a restored guest with a custom CPU template. """ - src_vm, template = microvm_with_custom_template - snapshot = src_vm.snapshot_full() - dst_vm = microvm_factory.build() - dst_vm.spawn() - # Restore the destination VM from the snapshot - dst_vm.restore_from_snapshot(snapshot, resume=True) - - check_vulnerabilities_files_on_guest(dst_vm, template) + check_vulnerabilities_files_ab( + with_restore(build_microvm_with_custom_template, microvm_factory) + ) diff --git a/tests/integration_tests/style/test_python.py b/tests/integration_tests/style/test_python.py index 31e77fb8c74..587083b7d95 100644 --- a/tests/integration_tests/style/test_python.py +++ b/tests/integration_tests/style/test_python.py @@ -35,7 +35,7 @@ def test_python_pylint(): '--variable-rgx="[a-z_][a-z0-9_]{1,30}$" --disable=' "fixme,too-many-instance-attributes,import-error," "too-many-locals,too-many-arguments,consider-using-f-string," - "consider-using-with,implicit-str-concat,line-too-long," + "consider-using-with,implicit-str-concat,line-too-long,redefined-outer-name," "broad-exception-raised,duplicate-code tests tools .buildkite/*.py" ) run( diff --git a/tools/ab_test.py b/tools/ab_test.py index 3084f5a51e6..3772cb6c26c 100755 --- a/tools/ab_test.py +++ b/tools/ab_test.py @@ -21,8 +21,6 @@ """ import argparse import json -import platform -import shutil import statistics import sys from pathlib import Path @@ -32,7 +30,7 @@ # pylint:disable=wrong-import-position from framework import utils -from framework.ab_test import chdir, check_regression, git_ab_test +from framework.ab_test import check_regression, git_ab_test_with_binaries from framework.properties import global_props from host_tools.metrics import ( emit_raw_emf, @@ -139,26 +137,13 @@ def load_data_series(revision: str): return processed_emf -def collect_data(firecracker_checkout: Path, test: str): - """Executes the specified test using a firecracker binary compiled from the given checkout""" - with chdir(firecracker_checkout): - revision = utils.run_cmd("git rev-parse HEAD").stdout.strip() +def collect_data(firecracker_binary: Path, jailer_binary: Path, test: str): + """Executes the specified test using the provided firecracker binaries""" + # Ensure the binaries are in the same directory. Will always be the case if used with git_ab_test_with_binaries + assert jailer_binary.parent == firecracker_binary.parent - binary_dir = Path.cwd() / "build" / revision - - if not (binary_dir / "firecracker").exists(): - with chdir(firecracker_checkout): - print(f"Compiling firecracker from revision {binary_dir.name}") - utils.run_cmd("./tools/release.sh --libc musl --profile release") - build_dir = ( - firecracker_checkout - / f"build/cargo_target/{platform.machine()}-unknown-linux-musl/release" - ) - binary_dir.mkdir(parents=True, exist_ok=True) - shutil.copy(build_dir / "firecracker", binary_dir) - shutil.copy(build_dir / "jailer", binary_dir) - else: - print(f"Using existing binaries for revision {binary_dir.name}") + binary_dir = firecracker_binary.parent + revision = binary_dir.name print("Collecting samples") _, stdout, _ = utils.run_cmd( @@ -227,8 +212,10 @@ def ab_performance_test( ) print(commit_list.strip()) - processed_emf_a, processed_emf_b, results = git_ab_test( - lambda checkout, _: collect_data(checkout, test), + processed_emf_a, processed_emf_b, results = git_ab_test_with_binaries( + lambda firecracker_binary, jailer_binary: collect_data( + firecracker_binary, jailer_binary, test + ), lambda ah, be: analyze_data(ah, be, n_resamples=int(100 / p_thresh)), a_revision=a_revision, b_revision=b_revision,