chore(docs): deprecate --start-time-cpu-us parameter

The jailer passes an option to Firecracker `--start-time-cpu-us` with
the start time of the CPU timer. Firecracker substracts that from its
own CPU time to calculate how much CPU time it spends during process
startup. The value the jailer passes is always 0, so we can just use the
Firecracker value.

In any case this value was only supposed to be used within the
Firecracker CI and not meant to be used by end customers.

Signed-off-by: Pablo Barbáchano <[email protected]>

Author: pb8o

CHANGELOG.md

@@ -36,6 +36,14 @@ 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

docs/jailer.md

@@ -144,10 +144,9 @@ 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> --start-time-cpu-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>` (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.
@@ -243,8 +242,7 @@ 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-cpu-us=<opaque>
+  --start-time-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, LoggerConfig, ProcessTimeReporter, StoreMetric, LOGGER, METRICS,
+    debug, error, info, warn, LoggerConfig, ProcessTimeReporter, StoreMetric, LOGGER, METRICS,
 };
 use vmm::persist::SNAPSHOT_VERSION;
 use vmm::resources::VmResources;
@@ -397,6 +397,7 @@ 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,7 +125,6 @@ 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<String>,
     cgroups: Vec<Box<dyn Cgroup>>,
@@ -161,11 +160,7 @@ impl fmt::Debug for Env {
 }
 
 impl Env {
-    pub fn new(
-        arguments: &arg_parser::Arguments,
-        start_time_us: u64,
-        start_time_cpu_us: u64,
-    ) -> Result<Self, JailerError> {
+    pub fn new(arguments: &arg_parser::Arguments, start_time_us: u64) -> Result<Self, JailerError> {
         // Unwraps should not fail because the arguments are mandatory arguments or with default
         // values.
         let id = arguments
@@ -290,7 +285,6 @@ impl Env {
             daemonize,
             new_pid_ns,
             start_time_us,
-            start_time_cpu_us,
             jailer_cpu_time_us: 0,
             extra_args: arguments.extra_args(),
             cgroups,
@@ -502,7 +496,6 @@ 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())
@@ -730,10 +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) - self.start_time_cpu_us;
-        // Reset process start time.
-        self.start_time_cpu_us = 0;
+        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 {
@@ -858,7 +848,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, 0).unwrap()
+        Env::new(&args, 0).unwrap()
     }
 
     #[test]
@@ -872,7 +862,7 @@ mod tests {
         args.parse(&make_args(&good_arg_vals)).unwrap();
         // This should be fine.
         let good_env =
-            Env::new(&args, 0, 0).expect("This new environment should be created successfully.");
+            Env::new(&args, 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());
@@ -897,7 +887,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, 0)
+        let another_good_env = Env::new(&args, 0)
             .expect("This another new environment should be created successfully.");
         assert!(!another_good_env.daemonize);
         assert!(!another_good_env.new_pid_ns);
@@ -915,7 +905,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, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         let invalid_res_limit_arg_vals = ArgVals {
             resource_limits: vec!["zzz"],
@@ -925,7 +915,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, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         let invalid_id_arg_vals = ArgVals {
             id: "/ad./sa12",
@@ -935,7 +925,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, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         let inexistent_exec_file_arg_vals = ArgVals {
             exec_file: "/this!/file!/should!/not!/exist!/",
@@ -946,7 +936,7 @@ mod tests {
         args = arg_parser.arguments().clone();
         args.parse(&make_args(&inexistent_exec_file_arg_vals))
             .unwrap();
-        Env::new(&args, 0, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         let invalid_uid_arg_vals = ArgVals {
             uid: "zzz",
@@ -956,7 +946,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, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         let invalid_gid_arg_vals = ArgVals {
             gid: "zzz",
@@ -966,7 +956,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, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         let invalid_parent_cg_vals = ArgVals {
             parent_cgroup: Some("/root"),
@@ -976,7 +966,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, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         let invalid_controller_pt = ArgVals {
             cgroups: vec!["../file_name=1", "./root=1", "/home=1"],
@@ -985,7 +975,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, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         let invalid_format = ArgVals {
             cgroups: vec!["./root/", "../root"],
@@ -994,7 +984,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, 0).unwrap_err();
+        Env::new(&args, 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).
@@ -1196,7 +1186,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, 0).unwrap();
+        let mut env = Env::new(&args, 0).unwrap();
 
         // Create the required chroot dir hierarchy.
         fs::create_dir_all(env.chroot_dir()).expect("Could not create dir hierarchy.");
@@ -1258,7 +1248,7 @@ mod tests {
             ..good_arg_vals.clone()
         };
         args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap();
-        Env::new(&args, 0, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         // Check empty string
         let mut args = arg_parser.arguments().clone();
@@ -1267,7 +1257,7 @@ mod tests {
             ..good_arg_vals.clone()
         };
         args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap();
-        Env::new(&args, 0, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         // Check valid file empty value
         let mut args = arg_parser.arguments().clone();
@@ -1276,7 +1266,7 @@ mod tests {
             ..good_arg_vals.clone()
         };
         args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap();
-        Env::new(&args, 0, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         // Check valid file no value
         let mut args = arg_parser.arguments().clone();
@@ -1285,7 +1275,7 @@ mod tests {
             ..good_arg_vals.clone()
         };
         args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap();
-        Env::new(&args, 0, 0).unwrap_err();
+        Env::new(&args, 0).unwrap_err();
 
         // Cases that should succeed
 
@@ -1296,7 +1286,7 @@ mod tests {
             ..good_arg_vals.clone()
         };
         args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap();
-        Env::new(&args, 0, 0).unwrap();
+        Env::new(&args, 0).unwrap();
 
         // Check valid case
         let mut args = arg_parser.arguments().clone();
@@ -1305,7 +1295,7 @@ mod tests {
             ..good_arg_vals.clone()
         };
         args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap();
-        Env::new(&args, 0, 0).unwrap();
+        Env::new(&args, 0).unwrap();
 
         // Check file with multiple "."
         let mut args = arg_parser.arguments().clone();
@@ -1314,7 +1304,7 @@ mod tests {
             ..good_arg_vals.clone()
         };
         args.parse(&make_args(&invalid_cgroup_arg_vals)).unwrap();
-        Env::new(&args, 0, 0).unwrap();
+        Env::new(&args, 0).unwrap();
     }
 
     #[test]

src/jailer/src/main.rs

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

src/vmm/src/logger/metrics.rs

@@ -333,15 +333,13 @@ impl ProcessTimeReporter {
 
     /// Obtain process CPU start time in microseconds.
     pub fn report_cpu_start_time(&self) {
-        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);
-        }
+        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);
     }
 }