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

roypat · roypat · commit a166b7f37493 · 2024-03-15T10:05:57.000Z
This reverts commit 21f0dea. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
diff --git 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
diff --git a/docs/jailer.md b/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
diff --git 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::<u64>()
                 .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
@@ -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<String>,
     cgroups: Vec<Box<dyn Cgroup>>,
@@ -160,7 +161,11 @@ impl fmt::Debug for Env {
 }
 
 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 @@ 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
@@ -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
@@ -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);
+        }
     }
 }
 

Original file line number	Diff line number	Diff line change
`@@ -369,6 +369,7 @@ fn main_exec() -> Result<(), JailerError> {`
`369`	`369`	`Env::new(`
`370`	`370`	`arguments,`
`371`	`371`	`utils::time::get_time_us(utils::time::ClockType::Monotonic),`
	`372`	`+ utils::time::get_time_us(utils::time::ClockType::ProcessCpu),`
`372`	`373`	`)`
`373`	`374`	`.and_then(\|env\| {`
`374`	`375`	`fs::create_dir_all(env.chroot_dir())`