From 6f186a2e34d1bd4c79b69c6f4aab9f028de4a810 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 14 Mar 2024 16:23:15 +0000 Subject: [PATCH 1/4] Revert "fix: subtract pre-`run()` execution time from startup time again" This reverts commit be25214c564812babbc614ad2e4077afd0bba4a3. Signed-off-by: Patrick Roy --- src/jailer/src/env.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/jailer/src/env.rs b/src/jailer/src/env.rs index 69d26a2f6e4..8eecc81968e 100644 --- a/src/jailer/src/env.rs +++ b/src/jailer/src/env.rs @@ -596,7 +596,6 @@ impl Env { } pub fn run(mut self) -> Result<(), JailerError> { - let jailer_start_time_cpu = utils::time::get_time_us(utils::time::ClockType::ProcessCpu); let exec_file_name = self.copy_exec_to_chroot()?; let chroot_exec_file = PathBuf::from("/").join(exec_file_name); @@ -724,8 +723,7 @@ impl Env { } // Compute jailer's total CPU time up to the current time. - self.jailer_cpu_time_us += - utils::time::get_time_us(utils::time::ClockType::ProcessCpu) - jailer_start_time_cpu; + self.jailer_cpu_time_us += utils::time::get_time_us(utils::time::ClockType::ProcessCpu); // If specified, exec the provided binary into a new PID namespace. if self.new_pid_ns { From 34ce18febea47f2646b3bcaea3fbec586412d4cf Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 14 Mar 2024 16:23:26 +0000 Subject: [PATCH 2/4] Revert "chore(docs): deprecate --start-time-cpu-us parameter" This reverts commit 21f0dea22aa1bb38daf32520b1196a9ff72b5815. Signed-off-by: Patrick Roy --- CHANGELOG.md | 8 ------ docs/jailer.md | 10 ++++--- src/firecracker/src/main.rs | 3 +- src/jailer/src/env.rs | 54 +++++++++++++++++++++-------------- src/jailer/src/main.rs | 1 + src/vmm/src/logger/metrics.rs | 16 ++++++----- 6 files changed, 49 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45a9fe6fd28..05c78436014 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,14 +42,6 @@ and this project adheres to handler. Each memory region object now contains a `page_size_kib` field. See also the [hugepages documentation](docs/hugepages.md). -### Deprecated - -- Firecracker's `--start-time-cpu-us` parameter is deprecated and will be - removed in v2.0 or later. It is used by the jailer to pass the value that - should be subtracted from the CPU time, but in practice it is always 0. The - parameter was never meant to be used by end customers, and we recommend doing - any such time adjustments outside Firecracker. - ### Fixed - [#4409](https://github.com/firecracker-microvm/firecracker/pull/4409): Fixed a diff --git a/docs/jailer.md b/docs/jailer.md index f113161049e..2495039b5cf 100644 --- a/docs/jailer.md +++ b/docs/jailer.md @@ -144,9 +144,10 @@ After starting, the Jailer goes through the following operations: inside `.pid`, while the child drops privileges and `exec()`s into the ``, as described below. - Drop privileges via setting the provided `uid` and `gid`. -- Exec into ` --id= --start-time-us=` (and also - forward any extra arguments provided to the jailer after `--`, as mentioned in - the **Jailer Usage** section), where: +- Exec into + ` --id= --start-time-us= --start-time-cpu-us=` + (and also forward any extra arguments provided to the jailer after `--`, as + mentioned in the **Jailer Usage** section), where: - `id`: (`string`) - The `id` argument provided to jailer. - `opaque`: (`number`) time calculated by the jailer that it spent doing its work. @@ -242,7 +243,8 @@ Finally, the jailer switches the `uid` to `123`, and `gid` to `100`, and execs ```console ./firecracker \ --id="551e7604-e35c-42b3-b825-416853441234" \ - --start-time-us= + --start-time-us= \ + --start-time-cpu-us= ``` Now firecracker creates the socket at diff --git a/src/firecracker/src/main.rs b/src/firecracker/src/main.rs index 54364e7f24c..65ae0038388 100644 --- a/src/firecracker/src/main.rs +++ b/src/firecracker/src/main.rs @@ -22,7 +22,7 @@ use utils::terminal::Terminal; use utils::validators::validate_instance_id; use vmm::builder::StartMicrovmError; use vmm::logger::{ - debug, error, info, warn, LoggerConfig, ProcessTimeReporter, StoreMetric, LOGGER, METRICS, + debug, error, info, LoggerConfig, ProcessTimeReporter, StoreMetric, LOGGER, METRICS, }; use vmm::persist::SNAPSHOT_VERSION; use vmm::resources::VmResources; @@ -397,7 +397,6 @@ fn main_exec() -> Result<(), MainError> { }); let start_time_cpu_us = arguments.single_value("start-time-cpu-us").map(|s| { - warn!("The --start-time-cpu-us argument is deprecated"); s.parse::() .expect("'start-time-cpu-us' parameter expected to be of 'u64' type.") }); diff --git a/src/jailer/src/env.rs b/src/jailer/src/env.rs index 8eecc81968e..0cba8e14b91 100644 --- a/src/jailer/src/env.rs +++ b/src/jailer/src/env.rs @@ -125,6 +125,7 @@ pub struct Env { daemonize: bool, new_pid_ns: bool, start_time_us: u64, + start_time_cpu_us: u64, jailer_cpu_time_us: u64, extra_args: Vec, cgroups: Vec>, @@ -160,7 +161,11 @@ impl fmt::Debug for Env { } impl Env { - pub fn new(arguments: &arg_parser::Arguments, start_time_us: u64) -> Result { + pub fn new( + arguments: &arg_parser::Arguments, + start_time_us: u64, + start_time_cpu_us: u64, + ) -> Result { // Unwraps should not fail because the arguments are mandatory arguments or with default // values. let id = arguments @@ -285,6 +290,7 @@ impl Env { daemonize, new_pid_ns, start_time_us, + start_time_cpu_us, jailer_cpu_time_us: 0, extra_args: arguments.extra_args(), cgroups, @@ -496,6 +502,7 @@ impl Env { Command::new(chroot_exec_file) .args(["--id", &self.id]) .args(["--start-time-us", &self.start_time_us.to_string()]) + .args(["--start-time-cpu-us", &self.start_time_cpu_us.to_string()]) .args(["--parent-cpu-time-us", &self.jailer_cpu_time_us.to_string()]) .stdin(Stdio::inherit()) .stdout(Stdio::inherit()) @@ -723,7 +730,10 @@ impl Env { } // Compute jailer's total CPU time up to the current time. - self.jailer_cpu_time_us += utils::time::get_time_us(utils::time::ClockType::ProcessCpu); + self.jailer_cpu_time_us += + utils::time::get_time_us(utils::time::ClockType::ProcessCpu) - self.start_time_cpu_us; + // Reset process start time. + self.start_time_cpu_us = 0; // If specified, exec the provided binary into a new PID namespace. if self.new_pid_ns { @@ -848,7 +858,7 @@ mod tests { let arg_parser = build_arg_parser(); let mut args = arg_parser.arguments().clone(); args.parse(&make_args(&ArgVals::new())).unwrap(); - Env::new(&args, 0).unwrap() + Env::new(&args, 0, 0).unwrap() } #[test] @@ -862,7 +872,7 @@ mod tests { args.parse(&make_args(&good_arg_vals)).unwrap(); // This should be fine. let good_env = - Env::new(&args, 0).expect("This new environment should be created successfully."); + Env::new(&args, 0, 0).expect("This new environment should be created successfully."); let mut chroot_dir = PathBuf::from(good_arg_vals.chroot_base); chroot_dir.push(Path::new(good_arg_vals.exec_file).file_name().unwrap()); @@ -887,7 +897,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&another_good_arg_vals)).unwrap(); - let another_good_env = Env::new(&args, 0) + let another_good_env = Env::new(&args, 0, 0) .expect("This another new environment should be created successfully."); assert!(!another_good_env.daemonize); assert!(!another_good_env.new_pid_ns); @@ -905,7 +915,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); let invalid_res_limit_arg_vals = ArgVals { resource_limits: vec!["zzz"], @@ -915,7 +925,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&invalid_res_limit_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); let invalid_id_arg_vals = ArgVals { id: "/ad./sa12", @@ -925,7 +935,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&invalid_id_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); let inexistent_exec_file_arg_vals = ArgVals { exec_file: "/this!/file!/should!/not!/exist!/", @@ -936,7 +946,7 @@ mod tests { args = arg_parser.arguments().clone(); args.parse(&make_args(&inexistent_exec_file_arg_vals)) .unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); let invalid_uid_arg_vals = ArgVals { uid: "zzz", @@ -946,7 +956,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&invalid_uid_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); let invalid_gid_arg_vals = ArgVals { gid: "zzz", @@ -956,7 +966,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&invalid_gid_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); let invalid_parent_cg_vals = ArgVals { parent_cgroup: Some("/root"), @@ -966,7 +976,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&invalid_parent_cg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); let invalid_controller_pt = ArgVals { cgroups: vec!["../file_name=1", "./root=1", "/home=1"], @@ -975,7 +985,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&invalid_controller_pt)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); let invalid_format = ArgVals { cgroups: vec!["./root/", "../root"], @@ -984,7 +994,7 @@ mod tests { let arg_parser = build_arg_parser(); args = arg_parser.arguments().clone(); args.parse(&make_args(&invalid_format)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); // The chroot-base-dir param is not validated by Env::new, but rather in run, when we // actually attempt to create the folder structure (the same goes for netns). @@ -1186,7 +1196,7 @@ mod tests { }; fs::write(exec_file_path, "some_content").unwrap(); args.parse(&make_args(&some_arg_vals)).unwrap(); - let mut env = Env::new(&args, 0).unwrap(); + let mut env = Env::new(&args, 0, 0).unwrap(); // Create the required chroot dir hierarchy. fs::create_dir_all(env.chroot_dir()).expect("Could not create dir hierarchy."); @@ -1248,7 +1258,7 @@ mod tests { ..good_arg_vals.clone() }; args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); // Check empty string let mut args = arg_parser.arguments().clone(); @@ -1257,7 +1267,7 @@ mod tests { ..good_arg_vals.clone() }; args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); // Check valid file empty value let mut args = arg_parser.arguments().clone(); @@ -1266,7 +1276,7 @@ mod tests { ..good_arg_vals.clone() }; args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); // Check valid file no value let mut args = arg_parser.arguments().clone(); @@ -1275,7 +1285,7 @@ mod tests { ..good_arg_vals.clone() }; args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap_err(); + Env::new(&args, 0, 0).unwrap_err(); // Cases that should succeed @@ -1286,7 +1296,7 @@ mod tests { ..good_arg_vals.clone() }; args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap(); + Env::new(&args, 0, 0).unwrap(); // Check valid case let mut args = arg_parser.arguments().clone(); @@ -1295,7 +1305,7 @@ mod tests { ..good_arg_vals.clone() }; args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap(); + Env::new(&args, 0, 0).unwrap(); // Check file with multiple "." let mut args = arg_parser.arguments().clone(); @@ -1304,7 +1314,7 @@ mod tests { ..good_arg_vals.clone() }; args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap(); - Env::new(&args, 0).unwrap(); + Env::new(&args, 0, 0).unwrap(); } #[test] diff --git a/src/jailer/src/main.rs b/src/jailer/src/main.rs index e353b5336cf..6e378351928 100644 --- a/src/jailer/src/main.rs +++ b/src/jailer/src/main.rs @@ -369,6 +369,7 @@ fn main_exec() -> Result<(), JailerError> { Env::new( arguments, utils::time::get_time_us(utils::time::ClockType::Monotonic), + utils::time::get_time_us(utils::time::ClockType::ProcessCpu), ) .and_then(|env| { fs::create_dir_all(env.chroot_dir()) diff --git a/src/vmm/src/logger/metrics.rs b/src/vmm/src/logger/metrics.rs index 22db3401c56..2d39a050ed5 100644 --- a/src/vmm/src/logger/metrics.rs +++ b/src/vmm/src/logger/metrics.rs @@ -333,13 +333,15 @@ impl ProcessTimeReporter { /// Obtain process CPU start time in microseconds. pub fn report_cpu_start_time(&self) { - let delta_us = utils::time::get_time_us(utils::time::ClockType::ProcessCpu) - - self.start_time_cpu_us.unwrap_or_default() - + self.parent_cpu_time_us.unwrap_or_default(); - METRICS - .api_server - .process_startup_time_cpu_us - .store(delta_us); + if let Some(cpu_start_time) = self.start_time_cpu_us { + let delta_us = utils::time::get_time_us(utils::time::ClockType::ProcessCpu) + - cpu_start_time + + self.parent_cpu_time_us.unwrap_or_default(); + METRICS + .api_server + .process_startup_time_cpu_us + .store(delta_us); + } } } From a3e28cd623a51eadbe405e2ac5ddd68674ae242e Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 14 Mar 2024 16:25:51 +0000 Subject: [PATCH 3/4] deprecate `--start-time-[cpu-]us` flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These flags are used by the jailer to pass the value that should be subtracted from the (CPU) time, when emitting the `start_time_us` and `start_time_cpu_us` metrics. They were never meant to be used by end customers. We cannot print a deprecation message, as the jailer unconditionally passes these two flags to Firecracker. This means that the deprecation message would be printed unconditionally for all users of the jailer. Not passing these flags from the jailer to Firecracker is also not an option, because it would result in a change of behavior (e.g. currently the metrics are only emitted if the flags are passed, so this would need to change to have the metrics be _always_ emitted). Signed-off-by: Patrick Roy Co-authored-by: Pablo Barbáchano --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05c78436014..47c28fb1974 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,15 @@ and this project adheres to content is empty, because the 'Content-Length' header field was missing in a response. +### Deprecated + +- Firecracker's `--start-time-cpu-us` and `--start-time-us` parameters are + deprecated and will be removed in v2.0 or later. They are used by the jailer + to pass the value that should be subtracted from the (CPU) time, when emitting + the `start_time_us` and `start_time_cpu_us` metrics. These parameters were + never meant to be used by end customers, and we recommend doing any such time + adjustments outside Firecracker. + ## \[1.6.0\] ### Added From e2869f8eeaf8337d3e8a7ea52e8b40c3265fc25b Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 14 Mar 2024 16:35:24 +0000 Subject: [PATCH 4/4] chore: Add DEPRECATED.md to list all out deprecated items Since some functionality is deprecated without a warning message, it is helpful to have a central location to list all deprecations. Signed-off-by: Patrick Roy --- DEPRECATED.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 DEPRECATED.md diff --git a/DEPRECATED.md b/DEPRECATED.md new file mode 100644 index 00000000000..8264aed9bb4 --- /dev/null +++ b/DEPRECATED.md @@ -0,0 +1,18 @@ +# Deprecated Features + +The following functionality of Firecracker is deprecated, and will be removed in +a future major Firecracker release, in accordance with our +[release policy](docs/RELEASE_POLICY.md). + +- \[[#2763](https://github.com/firecracker-microvm/firecracker/pull/2763)\] The + `vsock_id` body field in `PUT` requests on `/vsock` +- \[[#2980](https://github.com/firecracker-microvm/firecracker/pull/2980)\] The + `mem_file_path` body field in `PUT` requests on `/snapshot/load` +- \[[#2973](https://github.com/firecracker-microvm/firecracker/pull/2973)\] + MicroVM Metadata Service v1 (MMDSv1) +- \[[#4126](https://github.com/firecracker-microvm/firecracker/pull/4126)\] + Static CPU templates +- \[[#4209](https://github.com/firecracker-microvm/firecracker/pull/4209)\] The + `rebase-snap` tool +- \[[#4500](https://github.com/firecracker-microvm/firecracker/pull/4500)\] The + `--start-time-cpu-us` and `--start-time-us` CLI arguments