diff --git a/CHANGELOG.md b/CHANGELOG.md index e973916cbbf..fe30d08b09c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,14 +46,6 @@ and this project adheres to otherwise use anonymous private memory. This is because serving page faults of shared memory used by memfd is slower and may impact workloads. -### 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 @@ -89,6 +81,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 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 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 69d26a2f6e4..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()) @@ -596,7 +603,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); @@ -725,7 +731,9 @@ 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; + 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 { @@ -850,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] @@ -864,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()); @@ -889,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); @@ -907,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"], @@ -917,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", @@ -927,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!/", @@ -938,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", @@ -948,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", @@ -958,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"), @@ -968,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"], @@ -977,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"], @@ -986,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). @@ -1188,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."); @@ -1250,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(); @@ -1259,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(); @@ -1268,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(); @@ -1277,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 @@ -1288,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(); @@ -1297,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(); @@ -1306,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); + } } }