From fe8b775ea61f87f0e0202c034a45eadf3b5b886c Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Wed, 25 Jun 2025 12:46:00 +0100 Subject: [PATCH 1/6] feat(jailer): remove requirement for an executable name Now jailer will not complain if the executable does not contain `firecracker` in it's name. This restriction was unnecessary and it's removal is not a breaking change. Signed-off-by: Egor Lazarchuk --- src/jailer/src/env.rs | 15 --------------- src/jailer/src/main.rs | 4 ---- tests/integration_tests/security/test_jail.py | 14 -------------- 3 files changed, 33 deletions(-) diff --git a/src/jailer/src/env.rs b/src/jailer/src/env.rs index e2953584a32..57d28724a57 100644 --- a/src/jailer/src/env.rs +++ b/src/jailer/src/env.rs @@ -301,10 +301,6 @@ impl Env { .unwrap() .to_string(); - if !exec_file_name.contains("firecracker") { - return Err(JailerError::ExecFileName(exec_file_name)); - } - Ok((exec_file_path, exec_file_name)) } @@ -1048,17 +1044,6 @@ mod tests { "/tmp/firecracker_test_dir is not a file" ); - // Error case 3: Filename without "firecracker" - File::create("/tmp/firecracker_test_dir/foobarbaz").unwrap(); - assert_eq!( - format!( - "{}", - Env::validate_exec_file("/tmp/firecracker_test_dir/foobarbaz").unwrap_err() - ), - "Invalid filename. The filename of `--exec-file` option must contain \"firecracker\": \ - foobarbaz" - ); - std::fs::remove_file("/tmp/firecracker_test_dir/foobarbaz").unwrap(); std::fs::remove_dir_all("/tmp/firecracker_test_dir").unwrap(); } diff --git a/src/jailer/src/main.rs b/src/jailer/src/main.rs index 721531e49ba..4282f5c10d6 100644 --- a/src/jailer/src/main.rs +++ b/src/jailer/src/main.rs @@ -73,10 +73,6 @@ pub enum JailerError { Dup2(io::Error), #[error("Failed to exec into Firecracker: {0}")] Exec(io::Error), - #[error( - "Invalid filename. The filename of `--exec-file` option must contain \"firecracker\": {0}" - )] - ExecFileName(String), #[error("{}", format!("Failed to extract filename from path {:?}", .0).replace('\"', ""))] ExtractFileName(PathBuf), #[error("{}", format!("Failed to open file {:?}: {}", .0, .1).replace('\"', ""))] diff --git a/tests/integration_tests/security/test_jail.py b/tests/integration_tests/security/test_jail.py index ad6a16e48f5..82fccbdc0d5 100644 --- a/tests/integration_tests/security/test_jail.py +++ b/tests/integration_tests/security/test_jail.py @@ -106,20 +106,6 @@ def test_exec_file_not_exist(uvm_plain, tmp_path): ): test_microvm.spawn() - # Error case 3: Filename without "firecracker" - pseudo_exec_file_path = tmp_path / "foobarbaz" - pseudo_exec_file_path.touch() - fc_dir = Path("/srv/jailer") / pseudo_exec_file_path.name / test_microvm.id - fc_dir.mkdir(parents=True, exist_ok=True) - test_microvm.jailer.exec_file = pseudo_exec_file_path - - with pytest.raises( - Exception, - match=r"Jailer error: Invalid filename. The filename of `--exec-file` option" - r' must contain "firecracker": foobarbaz', - ): - test_microvm.spawn() - def test_default_chroot_hierarchy(uvm_plain): """ From 7f9c50ead0eeff031348528107f39e3ba1d575c0 Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Wed, 25 Jun 2025 12:54:48 +0100 Subject: [PATCH 2/6] chore: add note to CHANGELOG Add note about new jailer executable requirements. Signed-off-by: Egor Lazarchuk --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d243b8ef923..579f1193877 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ and this project adheres to - [#5165](https://github.com/firecracker-microvm/firecracker/pull/5165): Changed Firecracker snapshot feature from developer preview to generally available. Incremental snapshots remain in developer preview. +- [#5282](https://github.com/firecracker-microvm/firecracker/pull/5282): Updated + jailer to no longer require the executable file name to contain `firecracker`. ### Deprecated From 25126101137653822f8ecc162916b4be5fd7bdc5 Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Wed, 25 Jun 2025 14:13:19 +0000 Subject: [PATCH 3/6] feat(jailer): remove panic on errors Remove explicit panic on error. Let the error be returned from the main. Signed-off-by: Egor Lazarchuk --- src/jailer/src/main.rs | 3 +-- tests/integration_tests/security/test_jail.py | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/jailer/src/main.rs b/src/jailer/src/main.rs index 4282f5c10d6..9532fa1eba4 100644 --- a/src/jailer/src/main.rs +++ b/src/jailer/src/main.rs @@ -347,8 +347,7 @@ fn main_exec() -> Result<(), JailerError> { fs::create_dir_all(env.chroot_dir()) .map_err(|err| JailerError::CreateDir(env.chroot_dir().to_owned(), err))?; env.run() - }) - .unwrap_or_else(|err| panic!("Jailer error: {}", err)); + })?; Ok(()) } diff --git a/tests/integration_tests/security/test_jail.py b/tests/integration_tests/security/test_jail.py index 82fccbdc0d5..277d33b17b3 100644 --- a/tests/integration_tests/security/test_jail.py +++ b/tests/integration_tests/security/test_jail.py @@ -69,7 +69,7 @@ def test_empty_jailer_id(uvm_plain): # we can set an empty ID. with pytest.raises( ChildProcessError, - match=r"Jailer error: Invalid instance ID: Invalid len \(0\); the length must be between 1 and 64", + match=r"Invalid instance ID: Invalid len \(0\); the length must be between 1 and 64", ): test_microvm.spawn() @@ -88,7 +88,7 @@ def test_exec_file_not_exist(uvm_plain, tmp_path): with pytest.raises( Exception, - match=rf"Jailer error: Failed to canonicalize path {pseudo_exec_file_path}:" + match=rf"Failed to canonicalize path {pseudo_exec_file_path}:" rf" No such file or directory \(os error 2\)", ): test_microvm.spawn() @@ -102,7 +102,7 @@ def test_exec_file_not_exist(uvm_plain, tmp_path): with pytest.raises( Exception, - match=rf"Jailer error: {pseudo_exec_dir_path} is not a file", + match=rf"{pseudo_exec_dir_path} is not a file", ): test_microvm.spawn() From 2223e117c50b8cfe8b047b9f5806ee678245def3 Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Thu, 26 Jun 2025 15:38:21 +0000 Subject: [PATCH 4/6] feat: add jailer_time binary for jailer perf tests This binary just outputs the start and end time of the jailer startup. Signed-off-by: Egor Lazarchuk --- tests/conftest.py | 11 +++++++++++ tests/host_tools/jailer_time.c | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 tests/host_tools/jailer_time.c diff --git a/tests/conftest.py b/tests/conftest.py index 1b5dcafe713..0c71a212b56 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -274,6 +274,17 @@ def msr_reader_bin(test_fc_session_root_path): yield msr_reader_bin_path +@pytest.fixture(scope="session") +def jailer_time_bin(test_fc_session_root_path): + """Build a binary that fakes fc""" + jailer_time_bin_path = os.path.join(test_fc_session_root_path, "jailer_time") + build_tools.gcc_compile( + "host_tools/jailer_time.c", + jailer_time_bin_path, + ) + yield jailer_time_bin_path + + @pytest.fixture def bin_seccomp_paths(): """Build jailers and jailed binaries to test seccomp. diff --git a/tests/host_tools/jailer_time.c b/tests/host_tools/jailer_time.c new file mode 100644 index 00000000000..f8e86b4a86c --- /dev/null +++ b/tests/host_tools/jailer_time.c @@ -0,0 +1,19 @@ +// Copyright 2025 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// This is used by `performance/test_jailer.py` + +#include +#include + +int main(int argc, char** argv) { + // print current time in us + struct timespec now = {0}; + clock_gettime(CLOCK_MONOTONIC, &now); + unsigned long long current_ns = (unsigned long long)now.tv_sec * 1000000000 + (unsigned long long)now.tv_nsec; + unsigned long long current_us = current_ns / 1000; + printf("%llu\n", current_us); + + // print the --start-time-us value + printf("%s", argv[4]); +} From 21146408f79386bdbb7c3f07c934186b63830981 Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Wed, 25 Jun 2025 14:24:41 +0100 Subject: [PATCH 5/6] feat: add jailer performance test The test measures jailer startup time. It is parametrized by the number of parallel jailers starting up and the number of bind mount points present in the system. Signed-off-by: Egor Lazarchuk --- .../performance/test_jailer.py | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 tests/integration_tests/performance/test_jailer.py diff --git a/tests/integration_tests/performance/test_jailer.py b/tests/integration_tests/performance/test_jailer.py new file mode 100644 index 00000000000..21ef0c13666 --- /dev/null +++ b/tests/integration_tests/performance/test_jailer.py @@ -0,0 +1,88 @@ +# Copyright 2025 Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Performance benchmark for the jailer.""" + +import os +import shutil +from concurrent.futures import ProcessPoolExecutor + +import pytest + +from framework import utils +from framework.jailer import DEFAULT_CHROOT_PATH, JailerContext +from framework.properties import global_props + + +def setup_bind_mounts(tmp_path, n): + """ + Create bind mount points. The exact location of them + does not matter, they just need to exist. + """ + mounts_paths = tmp_path / "mounts" + os.makedirs(mounts_paths) + for m in range(n): + mount_path = f"{mounts_paths}/mount{m}" + os.makedirs(mount_path) + utils.check_output(f"mount --bind {mount_path} {mount_path}") + + +def clean_up_mounts(tmp_path): + """Cleanup mounts and jailer dirs""" + mounts_paths = tmp_path / "mounts" + for d in os.listdir(mounts_paths): + utils.check_output(f"umount {mounts_paths}/{d}") + + +@pytest.mark.nonci +@pytest.mark.parametrize("parallel", [1, 5, 10]) +@pytest.mark.parametrize("mounts", [0, 100, 300, 500]) +def test_jailer_startup( + jailer_time_bin, tmp_path, microvm_factory, parallel, mounts, metrics +): + """ + Test the overhead of jailer startup without and with bind mounts + with different parallelism options. + """ + + jailer_binary = microvm_factory.jailer_binary_path + + setup_bind_mounts(tmp_path, mounts) + + metrics.set_dimensions( + { + "instance": global_props.instance, + "cpu_model": global_props.cpu_model, + "performance_test": "test_jailer_startup", + "parallel": str(parallel), + "mounts": str(mounts), + } + ) + + cmds = [] + for i in range(500): + jailer = JailerContext( + jailer_id=f"fakefc{i}", + exec_file=jailer_time_bin, + # Don't deamonize to get the stdout + daemonize=False, + ) + jailer.setup() + + cmd = [str(jailer_binary), *jailer.construct_param_list()] + cmds.append(cmd) + + with ProcessPoolExecutor(max_workers=parallel) as executor: + # Submit all commands and get results + results = executor.map(utils.check_output, cmds) + + # Get results as they complete + for result in results: + end_time, start_time = result.stdout.split() + metrics.put_metric( + "startup", + int(end_time) - int(start_time), + unit="Microseconds", + ) + + clean_up_mounts(tmp_path) + shutil.rmtree(DEFAULT_CHROOT_PATH) From 8bf5e6358f5647ebb56866a53c17d3cd08d84dc8 Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Mon, 30 Jun 2025 11:16:30 +0000 Subject: [PATCH 6/6] chore: add jailer perf tests to bk Add new entry for the jailer tests in the performance bk pipeline. Signed-off-by: Egor Lazarchuk --- .buildkite/pipeline_perf.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.buildkite/pipeline_perf.py b/.buildkite/pipeline_perf.py index 7604d48982e..66a9314f2d4 100755 --- a/.buildkite/pipeline_perf.py +++ b/.buildkite/pipeline_perf.py @@ -60,6 +60,11 @@ "tests": "integration_tests/performance/test_memory_overhead.py integration_tests/performance/test_boottime.py::test_boottime", "devtool_opts": "-c 1-10 -m 0", }, + "jailer": { + "label": "⛓️ jailer", + "tests": "integration_tests/performance/test_jailer.py", + "devtool_opts": "-c 1-10 -m 0", + }, } REVISION_A = os.environ.get("REVISION_A")