rename enable_diff_snapshots API paramter to track_dirty_pages

We have two different API parameters that do the same thing in two
different APIs: For booted VMs, we enable KVM dirty page tracking by
setting track_dirty_pages to true in /machine-config. For resumed VMs,
we enable KVM dirty page tracking by setting enable_diff_snapshots to
true in /snapshot/load. Apart from being inconsistent for no reason, one
of these is very badly named: With support for diff snapshots without
dirty page tracking, enable_diff_snapshots is a misnomer, because
setting it to false will still allow us to do diff snapshots, it'll just
fall back to mincore.

Rename enable_diff_snapshots to track_dirty_pages, so we're consistent
and also so that the parameter reflects what is actually happening. Go
through the whole deprecation cycle of deprecating enable_diff_snapshots
and adding the new track_dirty_pages parameter at the same time. We will
need to remove enable_diff_snapshots in 2.0

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

CHANGELOG.md

@@ -31,6 +31,10 @@ and this project adheres to
 
 ### Deprecated
 
+- [#5274](https://github.com/firecracker-microvm/firecracker/pull/5274):
+  Deprecated the `enable_diff_snapshots` parameter of the `/snapshot/load` API.
+  Use `track_dirty_pages` instead.
+
 ### Removed
 
 ### Fixed

DEPRECATED.md

@@ -21,3 +21,5 @@ a future major Firecracker release, in accordance with our
   The functionality is substituted with ACPI.
 - \[[#2628](https://github.com/firecracker-microvm/firecracker/pull/2628)\] The
   `--basic` parameter of `seccompiler-bin`.
+- \[[#5274](https://github.com/firecracker-microvm/firecracker/pull/5274)\]: The
+  `enable_diff_snapshots` body field in `PUT` requests on `/snapshot/load`

docs/device-api.md

@@ -277,9 +277,9 @@ the snapshot. If they exist, the files will be truncated and overwritten.
     contents are only guaranteed to be committed/flushed to the host FS, but not
     necessarily to the underlying persistent storage (could still live in host
     FS cache).
-  - If diff snapshots were enabled, the snapshot creation resets then the
-    dirtied page bitmap and marks all pages clean (from a diff snapshot point of
-    view).
+  - If dirty page tracking is enabled, the snapshot creation resets then the
+    dirtied page bitmap and marks all pages clean (from a dirty page tracking
+    point of view).
 
 - _on failure_: no side-effects.
 
@@ -388,7 +388,7 @@ curl --unix-socket /tmp/firecracker.socket -i \
                 "backend_path": "./mem_file",
                 "backend_type": "File"
             },
-            "enable_diff_snapshots": true,
+            "track_dirty_pages": true,
             "resume_vm": false
     }'
 ```
@@ -424,7 +424,7 @@ curl --unix-socket /tmp/firecracker.socket -i \
     -d '{
             "snapshot_path": "./snapshot_file",
             "mem_file_path": "./mem_file",
-            "enable_diff_snapshots": true,
+            "track_dirty_pages": true,
             "resume_vm": false
     }'
 ```
@@ -455,8 +455,8 @@ to the new Firecracker process as they were to the original one.
     the guest memory and leads to undefined behavior.
   - The file indicated by `snapshot_path`, that is used to load from, is
     released and no longer used by this process.
-  - If `enable_diff_snapshots` is set, then diff snapshots can be taken
-    afterwards.
+  - If `track_dirty_pages` is set, subsequent diff snapshots will be based on
+    KVM dirty page tracking.
   - If `resume_vm` is set, the vm is automatically resumed if load is
     successful.
 - _on failure_: A specific error is reported and then the current Firecracker

docs/snapshotting/snapshot-support.md

@@ -13,7 +13,8 @@ use super::super::parsed_request::{ParsedRequest, RequestError};
 use super::super::request::{Body, Method, StatusCode};
 
 /// Deprecation message for the `mem_file_path` field.
-const LOAD_DEPRECATION_MESSAGE: &str = "PUT /snapshot/load: mem_file_path field is deprecated.";
+const LOAD_DEPRECATION_MESSAGE: &str =
+    "PUT /snapshot/load: mem_file_path and enable_diff_snapshots fields are deprecated.";
 /// None of the `mem_backend` or `mem_file_path` fields has been specified.
 pub const MISSING_FIELD: &str =
     "missing field: either `mem_backend` or `mem_file_path` is required";
@@ -80,7 +81,8 @@ fn parse_put_snapshot_load(body: &Body) -> Result<ParsedRequest, RequestError> {
     // Check for the presence of deprecated `mem_file_path` field and create
     // deprecation message if found.
     let mut deprecation_message = None;
-    if snapshot_config.mem_file_path.is_some() {
+    #[allow(deprecated)]
+    if snapshot_config.mem_file_path.is_some() || snapshot_config.enable_diff_snapshots {
         // `mem_file_path` field in request is deprecated.
         METRICS.deprecated_api.deprecated_http_api_calls.inc();
         deprecation_message = Some(LOAD_DEPRECATION_MESSAGE);
@@ -103,7 +105,9 @@ fn parse_put_snapshot_load(body: &Body) -> Result<ParsedRequest, RequestError> {
     let snapshot_params = LoadSnapshotParams {
         snapshot_path: snapshot_config.snapshot_path,
         mem_backend,
-        enable_diff_snapshots: snapshot_config.enable_diff_snapshots,
+        #[allow(deprecated)]
+        track_dirty_pages: snapshot_config.enable_diff_snapshots
+            || snapshot_config.track_dirty_pages,
         resume_vm: snapshot_config.resume_vm,
         network_overrides: snapshot_config.network_overrides,
     };
@@ -180,7 +184,7 @@ mod tests {
                 backend_path: PathBuf::from("bar"),
                 backend_type: MemBackendType::File,
             },
-            enable_diff_snapshots: false,
+            track_dirty_pages: false,
             resume_vm: false,
             network_overrides: vec![],
         };
@@ -202,15 +206,15 @@ mod tests {
                 "backend_path": "bar",
                 "backend_type": "File"
             },
-            "enable_diff_snapshots": true
+            "track_dirty_pages": true
         }"#;
         let expected_config = LoadSnapshotParams {
             snapshot_path: PathBuf::from("foo"),
             mem_backend: MemBackendConfig {
                 backend_path: PathBuf::from("bar"),
                 backend_type: MemBackendType::File,
             },
-            enable_diff_snapshots: true,
+            track_dirty_pages: true,
             resume_vm: false,
             network_overrides: vec![],
         };
@@ -240,7 +244,7 @@ mod tests {
                 backend_path: PathBuf::from("bar"),
                 backend_type: MemBackendType::Uffd,
             },
-            enable_diff_snapshots: false,
+            track_dirty_pages: false,
             resume_vm: true,
             network_overrides: vec![],
         };
@@ -276,7 +280,7 @@ mod tests {
                 backend_path: PathBuf::from("bar"),
                 backend_type: MemBackendType::Uffd,
             },
-            enable_diff_snapshots: false,
+            track_dirty_pages: false,
             resume_vm: true,
             network_overrides: vec![NetworkOverride {
                 iface_id: String::from("eth0"),
@@ -306,7 +310,7 @@ mod tests {
                 backend_path: PathBuf::from("bar"),
                 backend_type: MemBackendType::File,
             },
-            enable_diff_snapshots: false,
+            track_dirty_pages: false,
             resume_vm: true,
             network_overrides: vec![],
         };

src/firecracker/src/api_server/request/snapshot.rs

@@ -1245,7 +1245,11 @@ definitions:
       enable_diff_snapshots:
         type: boolean
         description:
-          Enable support for incremental (diff) snapshots by tracking dirty guest pages.
+          (Deprecated) Enable dirty page tracking to improve space efficiency of diff snapshots
+      track_dirty_pages:
+        type: boolean
+        description:
+          Enable dirty page tracking to improve space efficiency of diff snapshots
       mem_file_path:
         type: string
         description:

src/firecracker/swagger/firecracker.yaml

@@ -347,7 +347,7 @@ pub fn restore_from_snapshot(
             return Err(SnapshotStateFromFileError::UnknownNetworkDevice.into());
         }
     }
-    let track_dirty_pages = params.enable_diff_snapshots;
+    let track_dirty_pages = params.track_dirty_pages;
 
     let vcpu_count = microvm_state
         .vcpu_states

src/vmm/src/persist.rs

@@ -1249,7 +1249,7 @@ mod tests {
                     backend_type: MemBackendType::File,
                     backend_path: PathBuf::new(),
                 },
-                enable_diff_snapshots: false,
+                track_dirty_pages: false,
                 resume_vm: false,
                 network_overrides: vec![],
             },

src/vmm/src/rpc_interface.rs

@@ -64,9 +64,9 @@ pub struct LoadSnapshotParams {
     pub snapshot_path: PathBuf,
     /// Specifies guest memory backend configuration.
     pub mem_backend: MemBackendConfig,
-    /// Setting this flag will enable KVM dirty page tracking and will
-    /// allow taking subsequent incremental snapshots.
-    pub enable_diff_snapshots: bool,
+    /// Whether KVM dirty page tracking should be abled, to space optimization
+    /// of differential snapshots.
+    pub track_dirty_pages: bool,
     /// When set to true, the vm is also resumed if the snapshot load
     /// is successful.
     pub resume_vm: bool,
@@ -90,7 +90,11 @@ pub struct LoadSnapshotConfig {
     pub mem_backend: Option<MemBackendConfig>,
     /// Whether or not to enable KVM dirty page tracking.
     #[serde(default)]
+    #[deprecated]
     pub enable_diff_snapshots: bool,
+    /// Whether KVM dirty page tracking should be enabled.
+    #[serde(default)]
+    pub track_dirty_pages: bool,
     /// Whether or not to resume the vm post snapshot load.
     #[serde(default)]
     pub resume_vm: bool,

src/vmm/src/vmm_config/snapshot.rs

@@ -245,7 +245,7 @@ fn verify_load_snapshot(snapshot_file: TempFile, memory_file: TempFile) {
                 backend_path: memory_file.as_path().to_path_buf(),
                 backend_type: MemBackendType::File,
             },
-            enable_diff_snapshots: false,
+            track_dirty_pages: false,
             resume_vm: true,
             network_overrides: vec![],
         }))
@@ -329,7 +329,7 @@ fn verify_load_snap_disallowed_after_boot_resources(res: VmmAction, res_name: &s
             backend_path: memory_file.as_path().to_path_buf(),
             backend_type: MemBackendType::File,
         },
-        enable_diff_snapshots: false,
+        track_dirty_pages: false,
         resume_vm: false,
         network_overrides: vec![],
     });