Re-deprecate --start-time-[cpu-]us flags

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

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

docs/jailer.md

@@ -144,9 +144,10 @@ After starting, the Jailer goes through the following operations:
   inside `<exec_file_name>.pid`, while the child drops privileges and `exec()`s
   into the `<exec_file_name>`, as described below.
 - Drop privileges via setting the provided `uid` and `gid`.
-- Exec into `<exec_file_name> --id=<id> --start-time-us=<opaque>` (and also
-  forward any extra arguments provided to the jailer after `--`, as mentioned in
-  the **Jailer Usage** section), where:
+- Exec into
+  `<exec_file_name> --id=<id> --start-time-us=<opaque> --start-time-cpu-us=<opaque>`
+  (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=<opaque>
+  --start-time-us=<opaque> \
+  --start-time-cpu-us=<opaque>
 ```
 
 Now firecracker creates the socket at

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::<u64>()
                 .expect("'start-time-cpu-us' parameter expected to be of 'u64' type.")
         });

src/jailer/src/env.rs

@@ -125,6 +125,7 @@
     daemonize: bool,
     new_pid_ns: bool,
     start_time_us: u64,
+    start_time_cpu_us: u64,
     jailer_cpu_time_us: u64,
     extra_args: Vec<String>,
     cgroups: Vec<Box<dyn Cgroup>>,
@@ -160,7 +161,11 @@
 }
 
 impl Env {
-    pub fn new(arguments: &arg_parser::Arguments, start_time_us: u64) -> Result<Self, JailerError> {
+    pub fn new(
+        arguments: &arg_parser::Arguments,
+        start_time_us: u64,
+        start_time_cpu_us: u64,
+    ) -> Result<Self, JailerError> {
         // Unwraps should not fail because the arguments are mandatory arguments or with default
         // values.
         let id = arguments
@@ -285,6 +290,7 @@
             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 @@
         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 @@
     }
 
     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 @@
 
         // 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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         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 @@
         };
         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 @@
             ..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 @@
             ..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 @@
             ..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 @@
             ..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 @@
             ..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 @@
             ..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 @@
             ..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]

src/jailer/src/main.rs

@@ -369,6 +369,7 @@
     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())

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);
+        }
     }
 }