diff --git a/.buildkite/common.py b/.buildkite/common.py index 5bafa4810d5..533857fe7f4 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -110,6 +110,24 @@ def run_all_tests(changed_files): ) +def devtool_test(test_path, devtool_opts=None, pytest_opts=None, binary_dir=None): + """Generate a `devtool test` command""" + cmds = [] + parts = ["./tools/devtool -y test"] + if devtool_opts: + parts.append(devtool_opts) + parts.append("--") + if pytest_opts: + parts.append(pytest_opts) + if binary_dir is not None: + cmds.append(f'buildkite-agent artifact download "{binary_dir}/$(uname -m)/*" .') + cmds.append(f"chmod -v a+x {binary_dir}/**/*") + parts.append(f"--binary-dir=../{binary_dir}/$(uname -m)") + parts.append(test_path) + cmds.append(" ".join(parts)) + return cmds + + class DictAction(argparse.Action): """An argparse action that can receive a nested dictionary @@ -159,3 +177,10 @@ def __call__(self, parser, namespace, value, option_string=None): default={}, type=str, ) +COMMON_PARSER.add_argument( + "--binary-dir", + help="Use the Firecracker binaries from this path", + required=False, + default=None, + type=str, +) diff --git a/.buildkite/pipeline_perf.py b/.buildkite/pipeline_perf.py index 657db463d65..e057e4d891a 100755 --- a/.buildkite/pipeline_perf.py +++ b/.buildkite/pipeline_perf.py @@ -4,7 +4,7 @@ """Generate Buildkite performance pipelines dynamically""" -from common import COMMON_PARSER, group, overlay_dict, pipeline_to_json +from common import COMMON_PARSER, devtool_test, group, overlay_dict, pipeline_to_json perf_test = { "block": { @@ -45,9 +45,11 @@ def build_group(test): devtool_opts = test.pop("devtool_opts") test_path = test.pop("test_path") retries = test.pop("retries") + binary_dir = test.pop("binary_dir") + pytest_opts = f"-m nonci --reruns {retries} --perf-fail" return group( label=test.pop("label"), - command=f"./tools/devtool -y test {devtool_opts} -- -m nonci --reruns {retries} --perf-fail {test_path}", + command=devtool_test(test_path, devtool_opts, pytest_opts, binary_dir), artifacts=["./test_results/*"], instances=test.pop("instances"), platforms=test.pop("platforms"), @@ -75,6 +77,7 @@ def build_group(test): test_data.setdefault("agents", {"ag": 1}) test_data["retries"] = args.retries test_data["timeout_in_minutes"] *= args.retries + 1 + test_data["binary_dir"] = args.binary_dir test_data = overlay_dict(test_data, args.step_param) test_data["retry"] = { "automatic": [ diff --git a/.buildkite/pipeline_pr.py b/.buildkite/pipeline_pr.py index 40d3eb5b31c..d54d35fd51c 100755 --- a/.buildkite/pipeline_pr.py +++ b/.buildkite/pipeline_pr.py @@ -6,6 +6,7 @@ from common import ( COMMON_PARSER, + devtool_test, get_changed_files, group, overlay_dict, @@ -61,7 +62,10 @@ functional_grp = group( "⚙ Functional and security 🔒", - "./tools/devtool -y test -- -n 8 --dist worksteal integration_tests/{{functional,security}}", + devtool_test( + "-n 8 --dist worksteal integration_tests/{{functional,security}}", + binary_dir=args.binary_dir, + ), **defaults, ) @@ -77,7 +81,7 @@ performance_grp = group( "⏱ Performance", - "./tools/devtool -y test -- ../tests/integration_tests/performance/", + devtool_test("../tests/integration_tests/performance/", binary_dir=args.binary_dir), **defaults_for_performance, ) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3054d021647..09afc6f43bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ # Changelog -## Unreleased +## [1.5.1] + +### Added + +- [#4287](https://github.com/firecracker-microvm/firecracker/issues/4287) + Document a caveat to the jailer docs when using the `--parent-cgroup` option, + which results in it being ignored by the jailer. Refer to the [jailer + documentation](./docs/jailer.md#caveats) for a workaround. ### Changed @@ -11,6 +18,9 @@ ### Fixed +- [#4277](https://github.com/firecracker-microvm/firecracker/pull/4277): Fixed a + bug that ignored the `--show-log-origin` option, preventing it from printing + the source code file of the log messages. - [#4179](https://github.com/firecracker-microvm/firecracker/pull/4179): Fixed a bug reporting a non-zero exit code on successful shutdown when starting Firecracker with `--no-api`. diff --git a/Cargo.lock b/Cargo.lock index 4bda104d7f2..7dfcdda3a1d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -352,7 +352,7 @@ checksum = "acbf1af155f9b9ef647e42cdc158db4b64a1b61f743629225fde6f3e0be2a7c7" [[package]] name = "cpu-template-helper" -version = "1.5.0" +version = "1.5.1" dependencies = [ "clap", "displaydoc", @@ -500,7 +500,7 @@ dependencies = [ [[package]] name = "firecracker" -version = "1.5.0" +version = "1.5.1" dependencies = [ "api_server", "bincode", @@ -663,7 +663,7 @@ checksum = "af150ab688ff2122fcef229be89cb50dd66af9e01a4ff320cc137eecc9bacc38" [[package]] name = "jailer" -version = "1.5.0" +version = "1.5.1" dependencies = [ "libc", "nix", @@ -952,7 +952,7 @@ dependencies = [ [[package]] name = "rebase-snap" -version = "1.5.0" +version = "1.5.1" dependencies = [ "displaydoc", "libc", @@ -1039,7 +1039,7 @@ dependencies = [ [[package]] name = "seccompiler" -version = "1.5.0" +version = "1.5.1" dependencies = [ "bincode", "displaydoc", @@ -1119,7 +1119,7 @@ dependencies = [ [[package]] name = "snapshot-editor" -version = "1.5.0" +version = "1.5.1" dependencies = [ "clap", "clap-num", diff --git a/docs/RELEASE_POLICY.md b/docs/RELEASE_POLICY.md index 5982d09f696..af9c4a47248 100644 --- a/docs/RELEASE_POLICY.md +++ b/docs/RELEASE_POLICY.md @@ -86,7 +86,7 @@ also be specifying the supported kernel versions. | Release | Release Date | Latest Patch | Min. end of support | Official end of Support | |--------:|-------------:|-------------:|--------------------:|:-------------------------------| -| v1.5 | 2023-10-09 | v1.5.0 | 2024-04-09 | Supported | +| v1.5 | 2023-10-09 | v1.5.1 | 2024-04-09 | Supported | | v1.4 | 2023-07-20 | v1.4.1 | 2024-01-20 | Supported | | v1.3 | 2023-03-02 | v1.3.3 | 2023-09-02 | 2023-10-09 (v1.5 released) | | v1.2 | 2022-11-30 | v1.2.1 | 2023-05-30 | 2023-07-20 (v1.4 released) | diff --git a/docs/jailer.md b/docs/jailer.md index aae3edf9a09..77227c0bd81 100644 --- a/docs/jailer.md +++ b/docs/jailer.md @@ -286,3 +286,8 @@ Note: default value for `` is `/run/firecracker.socket`. - If all the cgroup controllers are bunched up on a single mount point using the "all" option, our current program logic will complain it cannot detect individual controller mount points. + +- [#4287](https://github.com/firecracker-microvm/firecracker/issues/4287) When + starting a jailer with `--parent-cgroup` specified but no cgroup flags + specified, then the rules in the parent cgroup folder are ignored. To + workaround, use a dummy cgroup parameter like `--cgroup=memory.max=max`. diff --git a/src/api_server/swagger/firecracker.yaml b/src/api_server/swagger/firecracker.yaml index 9031229812c..391f730a834 100644 --- a/src/api_server/swagger/firecracker.yaml +++ b/src/api_server/swagger/firecracker.yaml @@ -5,7 +5,7 @@ info: The API is accessible through HTTP calls on specific URLs carrying JSON modeled data. The transport medium is a Unix Domain Socket. - version: 1.5.0 + version: 1.5.1 termsOfService: "" contact: email: "compute-capsule@amazon.com" diff --git a/src/cpu-template-helper/Cargo.toml b/src/cpu-template-helper/Cargo.toml index 48ebb57e9ac..cd34d8dfd3a 100644 --- a/src/cpu-template-helper/Cargo.toml +++ b/src/cpu-template-helper/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cpu-template-helper" -version = "1.5.0" +version = "1.5.1" authors = ["Amazon Firecracker team "] edition = "2021" license = "Apache-2.0" diff --git a/src/firecracker/Cargo.toml b/src/firecracker/Cargo.toml index db515af7996..93ce55fbde6 100644 --- a/src/firecracker/Cargo.toml +++ b/src/firecracker/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "firecracker" -version = "1.5.0" +version = "1.5.1" authors = ["Amazon Firecracker team "] edition = "2021" build = "build.rs" diff --git a/src/firecracker/src/main.rs b/src/firecracker/src/main.rs index e26e7ef8eee..efdc31dd5d9 100644 --- a/src/firecracker/src/main.rs +++ b/src/firecracker/src/main.rs @@ -264,6 +264,32 @@ fn main_exec() -> Result<(), MainError> { return Ok(()); } + // It's safe to unwrap here because the field's been provided with a default value. + let instance_id = arguments.single_value("id").unwrap(); + validate_instance_id(instance_id.as_str()).expect("Invalid instance ID"); + + // Apply the logger configuration. + vmm::logger::INSTANCE_ID + .set(String::from(instance_id)) + .unwrap(); + let log_path = arguments.single_value("log-path").map(PathBuf::from); + let level = arguments + .single_value("level") + .map(|s| vmm::logger::LevelFilter::from_str(s)) + .transpose() + .map_err(MainError::InvalidLogLevel)?; + let show_level = arguments.flag_present("show-level").then_some(true); + let show_log_origin = arguments.flag_present("show-log-origin").then_some(true); + LOGGER + .update(LoggerConfig { + log_path, + level, + show_level, + show_log_origin, + }) + .map_err(MainError::LoggerInitialization)?; + info!("Running Firecracker v{FIRECRACKER_VERSION}"); + register_signal_handlers().map_err(MainError::RegisterSignalHandlers)?; #[cfg(target_arch = "aarch64")] @@ -287,10 +313,6 @@ fn main_exec() -> Result<(), MainError> { // deprecating one. // warn_deprecated_parameters(&arguments); - // It's safe to unwrap here because the field's been provided with a default value. - let instance_id = arguments.single_value("id").unwrap(); - validate_instance_id(instance_id.as_str()).expect("Invalid instance ID"); - let instance_info = InstanceInfo { id: instance_id.clone(), state: VmState::NotStarted, @@ -298,28 +320,6 @@ fn main_exec() -> Result<(), MainError> { app_name: "Firecracker".to_string(), }; - let id = arguments.single_value("id").map(|s| s.as_str()).unwrap(); - vmm::logger::INSTANCE_ID.set(String::from(id)).unwrap(); - info!("Running Firecracker v{FIRECRACKER_VERSION}"); - - // Apply the logger configuration. - let log_path = arguments.single_value("log-path").map(PathBuf::from); - let level = arguments - .single_value("level") - .map(|s| vmm::logger::LevelFilter::from_str(s)) - .transpose() - .map_err(MainError::InvalidLogLevel)?; - let show_level = arguments.flag_present("show-level").then_some(true); - let show_log_origin = arguments.flag_present("show-level").then_some(true); - LOGGER - .update(LoggerConfig { - log_path, - level, - show_level, - show_log_origin, - }) - .map_err(MainError::LoggerInitialization)?; - if let Some(metrics_path) = arguments.single_value("metrics-path") { let metrics_config = MetricsConfig { metrics_path: PathBuf::from(metrics_path), diff --git a/src/jailer/Cargo.toml b/src/jailer/Cargo.toml index 2b5984fd0f9..e18aedf942b 100644 --- a/src/jailer/Cargo.toml +++ b/src/jailer/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "jailer" -version = "1.5.0" +version = "1.5.1" authors = ["Amazon Firecracker team "] edition = "2021" description = "Process for starting Firecracker in production scenarios; applies a cgroup/namespace isolation barrier and then drops privileges." diff --git a/src/rebase-snap/Cargo.toml b/src/rebase-snap/Cargo.toml index cfed101799e..3e247f85e31 100644 --- a/src/rebase-snap/Cargo.toml +++ b/src/rebase-snap/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rebase-snap" -version = "1.5.0" +version = "1.5.1" authors = ["Amazon Firecracker team "] edition = "2021" license = "Apache-2.0" diff --git a/src/seccompiler/Cargo.toml b/src/seccompiler/Cargo.toml index 654a59a4f51..edaacf3984e 100644 --- a/src/seccompiler/Cargo.toml +++ b/src/seccompiler/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "seccompiler" -version = "1.5.0" +version = "1.5.1" authors = ["Amazon Firecracker team "] edition = "2021" description = "Program that compiles multi-threaded seccomp-bpf filters expressed as JSON into raw BPF programs, serializing them and outputting them to a file." diff --git a/src/snapshot-editor/Cargo.toml b/src/snapshot-editor/Cargo.toml index 17f892aa71e..a954731c31e 100644 --- a/src/snapshot-editor/Cargo.toml +++ b/src/snapshot-editor/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "snapshot-editor" -version = "1.5.0" +version = "1.5.1" authors = ["Amazon Firecracker team "] edition = "2021" license = "Apache-2.0" diff --git a/src/vmm/src/logger/logging.rs b/src/vmm/src/logger/logging.rs index 057d6ea2b23..32e5fd70136 100644 --- a/src/vmm/src/logger/logging.rs +++ b/src/vmm/src/logger/logging.rs @@ -22,8 +22,6 @@ pub const DEFAULT_LEVEL: log::LevelFilter = log::LevelFilter::Info; pub const DEFAULT_INSTANCE_ID: &str = "anonymous-instance"; /// Instance id. pub static INSTANCE_ID: OnceLock = OnceLock::new(); -/// Semver version of Firecracker. -const FIRECRACKER_VERSION: &str = env!("CARGO_PKG_VERSION"); /// The logger. /// @@ -62,7 +60,7 @@ impl Logger { .unwrap_or(DEFAULT_LEVEL), ); - let target_changed = if let Some(log_path) = config.log_path { + if let Some(log_path) = config.log_path { let file = std::fs::OpenOptions::new() .custom_flags(libc::O_NONBLOCK) .read(true) @@ -71,9 +69,6 @@ impl Logger { .map_err(LoggerUpdateError)?; guard.target = Some(file); - true - } else { - false }; if let Some(show_level) = config.show_level { @@ -87,11 +82,6 @@ impl Logger { // Ensure we drop the guard before attempting to log, otherwise this // would deadlock. drop(guard); - if target_changed { - // We log this when a target is changed so it is always the 1st line logged, this - // maintains some previous behavior to minimize a breaking change. - log::info!("Running Firecracker v{FIRECRACKER_VERSION}"); - } Ok(()) } diff --git a/tests/conftest.py b/tests/conftest.py index ced61be0f0f..180e2faf7c2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -288,6 +288,8 @@ def microvm_factory(fc_tmp_path, bin_cloner_path, request): if binary_dir := request.config.getoption("--binary-dir"): fc_binary_path = Path(binary_dir) / "firecracker" jailer_binary_path = Path(binary_dir) / "jailer" + if not fc_binary_path.exists(): + raise RuntimeError("Firecracker binary does not exist") else: fc_binary_path, jailer_binary_path = build_tools.get_firecracker_binaries() diff --git a/tests/framework/microvm.py b/tests/framework/microvm.py index 4174c15869c..3de4056a611 100644 --- a/tests/framework/microvm.py +++ b/tests/framework/microvm.py @@ -450,6 +450,8 @@ def spawn( self, log_file="fc.log", log_level="Debug", + log_show_level=False, + log_show_origin=False, metrics_path="fc.ndjson", ): """Start a microVM as a daemon or in a screen session.""" @@ -461,10 +463,14 @@ def spawn( self.log_file = Path(self.path) / log_file self.log_file.touch() self.create_jailed_resource(self.log_file) - # The default value for `level`, when configuring the - # logger via cmd line, is `Warning`. We set the level - # to `Debug` to also have the boot time printed in fifo. + # The default value for `level`, when configuring the logger via cmd + # line, is `Info`. We set the level to `Debug` to also have the boot + # time printed in the log. self.jailer.extra_args.update({"log-path": log_file, "level": log_level}) + if log_show_level: + self.jailer.extra_args["show-level"] = None + if log_show_origin: + self.jailer.extra_args["show-log-origin"] = None if metrics_path is not None: self.metrics_file = Path(self.path) / metrics_path diff --git a/tests/integration_tests/functional/test_logging.py b/tests/integration_tests/functional/test_logging.py index 04674930594..d3520a51424 100644 --- a/tests/integration_tests/functional/test_logging.py +++ b/tests/integration_tests/functional/test_logging.py @@ -3,7 +3,7 @@ """Tests the format of human readable logs. It checks the response of the API configuration calls and the logs that show -up in the configured logging FIFO. +up in the configured logging file. """ import re @@ -71,48 +71,6 @@ def check_log_message_format(log_str, instance_id, level, show_level, show_origi assert tag_level_no <= configured_level_no -def test_no_origin_logs(test_microvm_with_api): - """ - Check that logs do not contain the origin (i.e file and line number). - """ - _test_log_config(microvm=test_microvm_with_api, show_level=True, show_origin=False) - - -def test_no_level_logs(test_microvm_with_api): - """ - Check that logs do not contain the level. - """ - _test_log_config(microvm=test_microvm_with_api, show_level=False, show_origin=True) - - -def test_no_nada_logs(test_microvm_with_api): - """ - Check that logs do not contain either level or origin. - """ - _test_log_config(microvm=test_microvm_with_api, show_level=False, show_origin=False) - - -def test_info_logs(test_microvm_with_api): - """ - Check output of logs when minimum level to be displayed is info. - """ - _test_log_config(microvm=test_microvm_with_api) - - -def test_warn_logs(test_microvm_with_api): - """ - Check output of logs when minimum level to be displayed is warning. - """ - _test_log_config(microvm=test_microvm_with_api, log_level="Warning") - - -def test_error_logs(test_microvm_with_api): - """ - Check output of logs when minimum level of logs displayed is error. - """ - _test_log_config(microvm=test_microvm_with_api, log_level="Error") - - def test_log_config_failure(test_microvm_with_api): """ Check passing invalid FIFOs is detected and reported as an error. @@ -212,27 +170,25 @@ def test_api_requests_logs(test_microvm_with_api): ) -# pylint: disable=W0102 -def _test_log_config(microvm, log_level="Info", show_level=True, show_origin=True): +@pytest.mark.parametrize( + "log_level,show_level,show_origin", + [ + ("Info", True, True), + ("Info", False, True), + ("Info", True, False), + ("Info", False, False), + ("Error", False, False), + ("Warning", False, False), + ], +) +def test_log_config(uvm_plain, log_level, show_level, show_origin): """Exercises different scenarios for testing the logging config.""" - microvm.spawn(log_file=None) - # only works if log level is Debug - microvm.time_api_requests = False - - # Configure logging. - log_path = Path(microvm.path) / "log" - log_path.touch() - microvm.api.logger.put( - log_path=microvm.create_jailed_resource(log_path), - level=log_level, - show_level=show_level, - show_log_origin=show_origin, + microvm = uvm_plain + microvm.spawn( + log_level=log_level, log_show_level=show_level, log_show_origin=show_origin ) - microvm.log_file = log_path - microvm.basic_config() microvm.start() - lines = microvm.log_data.splitlines() # Check for `Running Firecracker` message. diff --git a/tools/bump-version.sh b/tools/bump-version.sh index 1cd6cde9299..cecbd11129e 100755 --- a/tools/bump-version.sh +++ b/tools/bump-version.sh @@ -49,6 +49,7 @@ files_to_change=( "$FC_ROOT_DIR/src/rebase-snap/Cargo.toml" "$FC_ROOT_DIR/src/seccompiler/Cargo.toml" "$FC_ROOT_DIR/src/cpu-template-helper/Cargo.toml" + "$FC_ROOT_DIR/src/snapshot-editor/Cargo.toml" ) say "Updating source files:" for file in "${files_to_change[@]}"; do diff --git a/tools/devtool b/tools/devtool index 827d9b5712b..3a2a053348b 100755 --- a/tools/devtool +++ b/tools/devtool @@ -486,11 +486,6 @@ cmd_build() { } function cmd_make_release { - OPTS="--reruns=7" - # run build tests first so that functional tests reuse compilation artifacts - cmd_test -- $OPTS --json-report-file=../test_results/test-report-perf+build.json integration_tests/{build,performance} || die "Tests failed!" - cmd_test -- $OPTS --json-report-file=../test_results/test-report-functional+security.json -n8 --dist=worksteal integration_tests/{functional,security,style} || die "Tests failed!" - run_devctr \ --user "$(id -u):$(id -g)" \ --workdir "$CTR_FC_ROOT_DIR" \ diff --git a/tools/gh_release.py b/tools/gh_release.py index 7e2f9ab3191..03669e43e1e 100755 --- a/tools/gh_release.py +++ b/tools/gh_release.py @@ -56,6 +56,8 @@ def github_release(tag_version, repo, github_token): assets.append(release_tgz) assets.append(sha256sums) + assets.append("test_results.tar.gz") + message_file = Path(f"release-{tag_version}-x86_64") / "RELEASE_NOTES" message = message_file.read_text() @@ -76,7 +78,7 @@ def github_release(tag_version, repo, github_token): content_type = "application/octet-stream" if asset.suffix == ".txt": content_type = "text/plain" - elif asset.suffix == ".tgz": + elif asset.suffix in {".tgz", ".gz"}: content_type = "application/gzip" print(f"Uploading asset {asset} with content-type={content_type}") gh_release.upload_asset(str(asset), label=asset.name, content_type=content_type) diff --git a/tools/release-prepare.sh b/tools/release-prepare.sh index 94283d9c144..0f56d1f757a 100755 --- a/tools/release-prepare.sh +++ b/tools/release-prepare.sh @@ -58,7 +58,7 @@ sed -i "s/\[Unreleased\]/\[$version\]/g" "$FC_ROOT_DIR/CHANGELOG.md" # Add all changed files git add -u -git commit -s -m "chore: release v$version" +git commit -s -m "chore: release v$version" -m "Update version number / CHANGELOG / CREDITS" # pretty print code diff --git a/tools/release-tag.sh b/tools/release-tag.sh index d319c9b303d..06f8b675543 100755 --- a/tools/release-tag.sh +++ b/tools/release-tag.sh @@ -61,5 +61,5 @@ warn "!WARNING! The next step will modify upstream: $UPSTREAM_URL by running:" echo " git push $UPSTREAM v$version" echo " git push $UPSTREAM $RELEASE_BRANCH" get_user_confirmation || die "Cancelling tag push" -git push $UPSTREAM "v$version" -git push $UPSTREAM "$RELEASE_BRANCH" +git push --atomic $UPSTREAM "v$version" +git push --atomic $UPSTREAM "$RELEASE_BRANCH" diff --git a/tools/release.sh b/tools/release.sh index 45a48209f90..85ed4bb8490 100755 --- a/tools/release.sh +++ b/tools/release.sh @@ -163,9 +163,6 @@ cp -v -t "$RELEASE_DIR" LICENSE NOTICE THIRD-PARTY check_swagger_artifact src/api_server/swagger/firecracker.yaml "$VERSION" cp -v src/api_server/swagger/firecracker.yaml "$RELEASE_DIR/firecracker_spec-$VERSION.yaml" -cp -v test_results/test-report-functional+security.json "$RELEASE_DIR/" -cp -v test_results/test-report-perf+build.json "$RELEASE_DIR/" - ( cd "$RELEASE_DIR" find . -type f -not -name "SHA256SUMS" |sort |xargs sha256sum >SHA256SUMS