test: simplify Firecracker binary build

Move all Rust compilation outside the testrun

Signed-off-by: Pablo Barbáchano <[email protected]>

Author: pb8o

tests/framework/ab_test.py

@@ -23,20 +23,17 @@
 """
 import contextlib
 import os
-import shutil
 import statistics
 from pathlib import Path
-from tempfile import TemporaryDirectory
 from typing import Callable, List, Optional, TypeVar
 
 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
+from host_tools.cargo_build import get_binary, 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.
@@ -85,25 +82,19 @@ def git_ab_test(
              (alternatively, your comparator can perform any required assertions and not return anything).
     """
 
-    # We can't just checkout random branches in the current working directory. Locally, this might not work because of
-    # uncommitted changes. In the CI this will not work because multiple tests will run in parallel, and thus switching
-    # branches will cause random failures in other tests.
-    with temporary_checkout(a_revision) as a_tmp:
-        result_a = test_runner(a_tmp, True)
+    dir_a = git_clone(Path("build") / a_revision, a_revision)
+    result_a = test_runner(dir_a, True)
 
-        if b_revision:
-            with temporary_checkout(b_revision) as b_tmp:
-                result_b = test_runner(b_tmp, False)
-                # Have to call comparator here to make sure both temporary directories exist (as the comparator
-                # might rely on some files that were created during test running, see the benchmark test)
-                comparison = comparator(result_a, result_b)
-        else:
-            # By default, pytest execution happens inside the `tests` subdirectory. Pass the repository root, as
-            # documented.
-            result_b = test_runner(Path.cwd().parent, False)
-            comparison = comparator(result_a, result_b)
+    if b_revision:
+        dir_b = git_clone(Path("build") / b_revision, b_revision)
+    else:
+        # By default, pytest execution happens inside the `tests` subdirectory. Pass the repository root, as
+        # documented.
+        dir_b = Path.cwd().parent
+    result_b = test_runner(dir_b, False)
 
-        return result_a, result_b, comparison
+    comparison = comparator(result_a, result_b)
+    return result_a, result_b, comparison
 
 
 def is_pr() -> bool:
@@ -162,45 +153,6 @@ def set_did_not_grow_comparator(
     )
 
 
-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():
-            with chdir(checkout):
-                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,
@@ -212,11 +164,16 @@ def git_ab_test_guest_command(
     """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):
+    def test_runner(workspace_dir, _is_a: bool):
+        utils.run_cmd("./tools/release.sh --profile release", cwd=workspace_dir)
+        bin_dir = get_binary(
+            "firecracker", workspace_dir=workspace_dir
+        ).parent.resolve()
+        firecracker, jailer = bin_dir / "firecracker", bin_dir / "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(
+    (_, old_out, old_err), (_, new_out, new_err), the_same = git_ab_test(
         test_runner, comparator, a_revision=a_revision, b_revision=b_revision
     )
 
@@ -270,35 +227,25 @@ def check_regression(
     )
 
 
-@contextlib.contextmanager
-def temporary_checkout(revision: str):
-    """
-    Context manager that checks out firecracker in a temporary directory and `chdir`s into it
+@with_filelock
+def git_clone(clone_path, commitish):
+    """Clone the repository at `commit`.
 
-    Will change back to whatever was the current directory when the context manager was entered, even if exceptions
-    happen along the way.
+    :return: the working copy directory.
     """
-    with TemporaryDirectory() as tmp_dir:
-        basename = Path(tmp_dir).name
+    if not clone_path.exists():
         ret, _, _ = utils.run_cmd(
-            f"git cat-file -t {revision}", ignore_return_code=True
+            f"git cat-file -t {commitish}", ignore_return_code=True
         )
         if ret != 0:
-            # git didn't recognize this object, so maybe it is a branch; qualify it
-            revision = f"origin/{revision}"
+            # git didn't recognize this object; qualify it if it is a branch
+            commitish = f"origin/{commitish}"
         # make a temp branch for that commit so we can directly check it out
-        utils.run_cmd(f"git branch {basename} {revision}")
-        # `git clone` can take a path instead of an URL, which causes it to create a copy of the
-        # repository at the given path. However, that path needs to point to the root of a repository,
-        # it cannot be some arbitrary subdirectory. Therefore:
+        utils.run_cmd(f"git branch {clone_path} {commitish}")
         _, git_root, _ = utils.run_cmd("git rev-parse --show-toplevel")
         # split off the '\n' at the end of the stdout
-        utils.run_cmd(f"git clone -b {basename} {git_root.strip()} {tmp_dir}")
-        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}")
+        utils.run_cmd(f"git clone -b {clone_path} {git_root.strip()} {clone_path}")
+    return clone_path
 
 
 # Once we upgrade to python 3.11, this will be in contextlib:

tests/framework/defs.py

@@ -11,9 +11,6 @@
 # The Firecracker sources workspace dir
 FC_WORKSPACE_DIR = Path(__file__).parent.parent.parent.resolve()
 
-# Cargo build directory for seccompiler
-SECCOMPILER_TARGET_DIR = FC_WORKSPACE_DIR / "build/seccompiler"
-
 # Folder containing JSON seccomp filters
 SECCOMP_JSON_DIR = FC_WORKSPACE_DIR / "resources/seccomp"
 

tests/host_tools/cargo_build.py

@@ -10,17 +10,8 @@
 from framework.defs import FC_WORKSPACE_DIR
 from framework.with_filelock import with_filelock
 
-CARGO_BUILD_REL_PATH = "firecracker_binaries"
-"""Keep a single build path across all build tests."""
-
-CARGO_RELEASE_REL_PATH = os.path.join(CARGO_BUILD_REL_PATH, "release")
-"""Keep a single Firecracker release binary path across all test types."""
-
-
-DEFAULT_BUILD_TARGET = "{}-unknown-linux-musl".format(platform.machine())
-RELEASE_BINARIES_REL_PATH = "{}/release/".format(DEFAULT_BUILD_TARGET)
-
-CARGO_UNITTEST_REL_PATH = os.path.join(CARGO_BUILD_REL_PATH, "test")
+DEFAULT_TARGET = f"{platform.machine()}-unknown-linux-musl"
+DEFAULT_TARGET_DIR = f"{DEFAULT_TARGET}/release/"
 
 
 def cargo(
@@ -33,11 +24,8 @@ def cargo(
 ):
     """Executes the specified cargo subcommand"""
     env = env or {}
-
     env_string = " ".join(f'{key}="{str(value)}"' for key, value in env.items())
-
     cmd = f"{env_string} cargo {subcommand} {cargo_args} -- {subcommand_args}"
-
     return utils.run_cmd(cmd, cwd=cwd)
 
 
@@ -48,46 +36,27 @@ def get_rustflags():
     return ""
 
 
-@with_filelock
-def cargo_build(path, extra_args="", src_dir=None):
-    """Trigger build depending on flags provided."""
-    cargo("build", extra_args, env={"CARGO_TARGET_DIR": path}, cwd=src_dir)
-
-
 def cargo_test(path, extra_args=""):
     """Trigger unit tests depending on flags provided."""
     env = {
-        "CARGO_TARGET_DIR": os.path.join(path, CARGO_UNITTEST_REL_PATH),
+        "CARGO_TARGET_DIR": os.path.join(path, "unit-tests"),
         "RUST_TEST_THREADS": 1,
         "RUST_BACKTRACE": 1,
         "RUSTFLAGS": get_rustflags(),
     }
     cargo("test", extra_args + " --all --no-fail-fast", env=env)
 
 
-@with_filelock
 def get_binary(name, *, workspace_dir=FC_WORKSPACE_DIR, example=None):
-    """Build a binary"""
-    target = DEFAULT_BUILD_TARGET
-    target_dir = workspace_dir / "build" / "cargo_target"
-    bin_path = target_dir / target / "release" / name
-    cmd = f"-p {name}"
+    """Get a binary. The binaries are built before starting a testrun."""
+    target_dir = workspace_dir / "build" / "cargo_target" / DEFAULT_TARGET_DIR
+    bin_path = target_dir / name
     if example:
-        bin_path = target_dir / target / "release" / "examples" / example
-        cmd += f" --example {example}"
-    if not bin_path.exists():
-        env = {"RUSTFLAGS": get_rustflags()}
-        cargo(
-            "build",
-            f"--release --target {target} {cmd}",
-            env=env,
-            cwd=workspace_dir,
-        )
-        utils.run_cmd(f"strip --strip-debug {bin_path}")
+        bin_path = target_dir / "examples" / example
+    assert bin_path.exists()
     return bin_path
 
 
-@with_filelock
 def get_firecracker_binaries(*, workspace_dir=FC_WORKSPACE_DIR):
     """Build the Firecracker and Jailer binaries if they don't exist.
 
@@ -104,69 +73,53 @@ def get_example(name, *args, package="firecracker", **kwargs):
     return get_binary(package, *args, **kwargs, example=name)
 
 
-@with_filelock
 def run_seccompiler_bin(bpf_path, json_path=defs.SECCOMP_JSON_DIR, basic=False):
     """
     Run seccompiler-bin.
 
     :param bpf_path: path to the output file
     :param json_path: optional path to json file
     """
-    cargo_target = "{}-unknown-linux-musl".format(platform.machine())
-
     # If no custom json filter, use the default one for the current target.
     if json_path == defs.SECCOMP_JSON_DIR:
-        json_path = json_path / "{}.json".format(cargo_target)
+        json_path = json_path / f"{DEFAULT_TARGET}.json"
 
     seccompiler_args = f"--input-file {json_path} --target-arch {platform.machine()} --output-file {bpf_path}"
 
     if basic:
         seccompiler_args += " --basic"
 
-    rc, _, _ = cargo(
-        "run",
-        f"-p seccompiler --target-dir {defs.SECCOMPILER_TARGET_DIR} --target {cargo_target}",
-        seccompiler_args,
-    )
-
+    seccompiler = get_binary("seccompiler-bin")
+    rc, _, _ = utils.run_cmd(f"{seccompiler} {seccompiler_args}")
     assert rc == 0
 
 
-@with_filelock
 def run_snap_editor_rebase(base_snap, diff_snap):
     """
     Run apply_diff_snap.
 
     :param base_snap: path to the base snapshot mem file
     :param diff_snap: path to diff snapshot mem file
     """
-    cargo_target = "{}-unknown-linux-musl".format(platform.machine())
 
-    rc, _, _ = cargo(
-        "run",
-        f"-p snapshot-editor --target {cargo_target}",
-        f"edit-memory rebase --memory-path {base_snap} --diff-path {diff_snap}",
+    snap_ed = get_binary("snapshot-editor")
+    rc, _, _ = utils.run_cmd(
+        f"{snap_ed} edit-memory rebase --memory-path {base_snap} --diff-path {diff_snap}"
     )
-
     assert rc == 0
 
 
-@with_filelock
 def run_rebase_snap_bin(base_snap, diff_snap):
     """
     Run apply_diff_snap.
 
     :param base_snap: path to the base snapshot mem file
     :param diff_snap: path to diff snapshot mem file
     """
-    cargo_target = "{}-unknown-linux-musl".format(platform.machine())
-
-    rc, _, _ = cargo(
-        "run",
-        f"-p rebase-snap --target {cargo_target}",
-        f"--base-file {base_snap} --diff-file {diff_snap}",
+    rebase_snap = get_binary("rebase-snap")
+    rc, _, _ = utils.run_cmd(
+        f"{rebase_snap} --base-file {base_snap} --diff-file {diff_snap}"
     )
-
     assert rc == 0
 
 

tools/ab_test.py

@@ -31,8 +31,9 @@
 
 # pylint:disable=wrong-import-position
 from framework import utils
-from framework.ab_test import check_regression, git_ab_test_with_binaries
+from framework.ab_test import check_regression, git_ab_test
 from framework.properties import global_props
+from host_tools.cargo_build import get_binary
 from host_tools.metrics import (
     emit_raw_emf,
     format_with_reduced_unit,
@@ -152,17 +153,13 @@ def load_data_series(report_path: Path, revision: str = None, *, reemit: bool =
     return processed_emf
 
 
-def collect_data(firecracker_binary: Path, jailer_binary: Path, test: str):
+def collect_data(binary_dir: 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 = firecracker_binary.parent
     revision = binary_dir.name
 
     print("Collecting samples")
     _, stdout, _ = utils.run_cmd(
-        f"AWS_EMF_ENVIRONMENT=local AWS_EMF_NAMESPACE=local ./tools/test.sh --binary-dir=/firecracker/build/{revision} {test} -m ''"
+        f"AWS_EMF_ENVIRONMENT=local AWS_EMF_NAMESPACE=local ./tools/test.sh --binary-dir={binary_dir} {test} -m ''"
     )
     print(stdout.strip())
 
@@ -318,10 +315,13 @@ def ab_performance_test(
     )
     print(commit_list.strip())
 
-    git_ab_test_with_binaries(
-        lambda firecracker_binary, jailer_binary: collect_data(
-            firecracker_binary, jailer_binary, test
-        ),
+    def test_runner(workspace, _is_ab: bool):
+        utils.run_cmd("./tools/release.sh --profile release", cwd=workspace)
+        bin_dir = ".." / get_binary("firecracker", workspace_dir=workspace).parent
+        return collect_data(bin_dir, test)
+
+    return git_ab_test(
+        test_runner,
         lambda ah, be: analyze_data(
             ah,
             be,