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

@@ -28,6 +28,9 @@ and this project adheres to
 
 ### Deprecated
 
+- [#????](...): Deprecated the `enable_diff_snapshots` parameter of the
+  `/snapshot/load` API. Use `track_dirty_pages` instead.
+
 ### Removed
 
 ### Fixed

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

@@ -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![],
         };
@@ -210,7 +214,7 @@ mod tests {
                 backend_path: PathBuf::from("bar"),
                 backend_type: MemBackendType::File,
             },
-            enable_diff_snapshots: true,
+            track_dirty_pages: false,
             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/swagger/firecracker.yaml

@@ -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/vmm/src/persist.rs

@@ -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/rpc_interface.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/vmm_config/snapshot.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/tests/integration_tests.rs

@@ -262,7 +262,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![],
         }))
@@ -346,7 +346,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![],
     });