Drop support for forward compatibility #4194

bchalios · 2023-10-20T11:32:01Z

Changes

Removes forward compatibility support from Firecracker binary.

Reason

Forward compatibility refers to the ability of a Firecracker binary to create snapshots for older versions of Firecracker.
We have found this feature to not be useful, while it increases code and testing complexity.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

If a specific issue led to this PR, this PR closes the issue.
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
API changes follow the Runbook for Firecracker API changes.
User-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.
New TODOs link to an issue.
Commits meet contribution quality standards.

This functionality cannot be added in rust-vmm.

codecov · 2023-10-20T11:37:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b108506) 82.95% compared to head (2c0c688) 82.93%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4194      +/-   ##
==========================================
- Coverage   82.95%   82.93%   -0.02%     
==========================================
  Files         221      221              
  Lines       28415    28231     -184     
==========================================
- Hits        23572    23414     -158     
+ Misses       4843     4817      -26

Flag	Coverage Δ
4.14-c7g.metal	`78.35% <100.00%> (-0.11%)`	⬇️
4.14-m5d.metal	`80.29% <100.00%> (+<0.01%)`	⬆️
4.14-m6a.metal	`79.41% <100.00%> (-0.01%)`	⬇️
4.14-m6g.metal	`78.35% <100.00%> (-0.11%)`	⬇️
4.14-m6i.metal	`80.27% <100.00%> (+<0.01%)`	⬆️
5.10-c7g.metal	`81.30% <100.00%> (-0.10%)`	⬇️
5.10-m5d.metal	`83.00% <100.00%> (+0.02%)`	⬆️
5.10-m6a.metal	`82.23% <100.00%> (+0.01%)`	⬆️
5.10-m6g.metal	`81.30% <100.00%> (-0.10%)`	⬇️
5.10-m6i.metal	`82.99% <100.00%> (+0.02%)`	⬆️
6.1-c7g.metal	`81.30% <100.00%> (-0.10%)`	⬇️
6.1-m5d.metal	`83.00% <100.00%> (+0.02%)`	⬆️
6.1-m6a.metal	`82.23% <100.00%> (+0.01%)`	⬆️
6.1-m6g.metal	`81.30% <100.00%> (-0.10%)`	⬇️
6.1-m6i.metal	`82.98% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/api_server/src/lib.rs	`85.71% <ø> (ø)`
src/api_server/src/parsed_request.rs	`93.04% <ø> (ø)`
src/api_server/src/request/snapshot.rs	`100.00% <100.00%> (ø)`
src/firecracker/src/main.rs	`0.22% <ø> (+<0.01%)`	⬆️
src/vmm/src/device_manager/mmio.rs	`60.68% <ø> (-3.63%)`	⬇️
src/vmm/src/device_manager/persist.rs	`93.16% <ø> (+0.85%)`	⬆️
src/vmm/src/devices/virtio/block/persist.rs	`93.68% <ø> (+1.45%)`	⬆️
src/vmm/src/devices/virtio/net/persist.rs	`95.65% <ø> (-0.46%)`	⬇️
src/vmm/src/devices/virtio/persist.rs	`97.36% <ø> (-0.12%)`	⬇️
src/vmm/src/persist.rs	`43.94% <100.00%> (-9.52%)`	⬇️
... and 4 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

roypat · 2023-10-23T12:05:01Z

I think we might also want to remove

firecracker/src/firecracker/src/main.rs

Lines 537 to 551 in 9b0e03a

    
           fn print_snapshot_data_format(snapshot_path: &str) -> Result<(), SnapshotVersionError> { 
        
               let mut snapshot_reader = 
        
                   File::open(snapshot_path).map_err(SnapshotVersionError::OpenSnapshot)?; 
        
               let data_format_version = Snapshot::get_data_version(&mut snapshot_reader, &VERSION_MAP) 
        
                   .map_err(SnapshotVersionError::SnapshotVersion)?; 
        
               let (key, _) = FC_VERSION_TO_SNAP_VERSION 
        
                   .iter() 
        
                   .find(|(_, &val)| val == data_format_version) 
        
                   .ok_or_else(|| SnapshotVersionError::FirecrackerVersion(data_format_version))?; 
        
               println!("v{}", key); 
        
               Ok(()) 
        
           }

with this.

bchalios · 2023-10-23T12:25:12Z

I think we might also want to remove

firecracker/src/firecracker/src/main.rs

Lines 537 to 551 in 9b0e03a

fn print_snapshot_data_format(snapshot_path: &str) -> Result<(), SnapshotVersionError> {

let mut snapshot_reader =

File::open(snapshot_path).map_err(SnapshotVersionError::OpenSnapshot)?;

let data_format_version = Snapshot::get_data_version(&mut snapshot_reader, &VERSION_MAP)

.map_err(SnapshotVersionError::SnapshotVersion)?;

let (key, _) = FC_VERSION_TO_SNAP_VERSION

.iter()

.find(|(_, &val)| val == data_format_version)

.ok_or_else(|| SnapshotVersionError::FirecrackerVersion(data_format_version))?;

println!("v{}", key);

Ok(())

}

with this.

Why remove it? I think this will be reworked once we implement the new snapshot versioning schema. Once we do that work, each Firecracker version will declare support for a single MAJOR.MINOR snapshot version which will be decoupled from Firecracker version, but in any case I think it'd be worth being able to print it.

src/api_server/src/lib.rs

roypat · 2023-10-23T13:50:42Z

I think we might also want to remove

firecracker/src/firecracker/src/main.rs

Lines 537 to 551 in 9b0e03a

fn print_snapshot_data_format(snapshot_path: &str) -> Result<(), SnapshotVersionError> {

let mut snapshot_reader =

File::open(snapshot_path).map_err(SnapshotVersionError::OpenSnapshot)?;

let data_format_version = Snapshot::get_data_version(&mut snapshot_reader, &VERSION_MAP)

.map_err(SnapshotVersionError::SnapshotVersion)?;

let (key, _) = FC_VERSION_TO_SNAP_VERSION

.iter()

.find(|(_, &val)| val == data_format_version)

.ok_or_else(|| SnapshotVersionError::FirecrackerVersion(data_format_version))?;

println!("v{}", key);

Ok(())

}

with this.

Why remove it? I think this will be reworked once we implement the new snapshot versioning schema. Once we do that work, each Firecracker version will declare support for a single MAJOR.MINOR snapshot version which will be decoupled from Firecracker version, but in any case I think it'd be worth being able to print it.

Mh, to me this output always read as "we can create snapshots for these firecracker versions". I guess if it also means "we can restore from these" then its indeed not redundant.

On the other hand, it just prints every firecracker version since we introduced snapshotting, which is completely wrong since there has been multiple non-compatible changes to the snapshot format since then. Do we plan to introduce the new versioning scheme with this release? If yes we can keep it as is (it'll get fixed when the new scheme gets introduced, as you say), but if no I'd rather we remove it (or fix it for the current version scheme).

bchalios · 2023-10-23T13:56:09Z

I think we might also want to remove

firecracker/src/firecracker/src/main.rs

Lines 537 to 551 in 9b0e03a

fn print_snapshot_data_format(snapshot_path: &str) -> Result<(), SnapshotVersionError> {

let mut snapshot_reader =

File::open(snapshot_path).map_err(SnapshotVersionError::OpenSnapshot)?;

let data_format_version = Snapshot::get_data_version(&mut snapshot_reader, &VERSION_MAP)

.map_err(SnapshotVersionError::SnapshotVersion)?;

let (key, _) = FC_VERSION_TO_SNAP_VERSION

.iter()

.find(|(_, &val)| val == data_format_version)

.ok_or_else(|| SnapshotVersionError::FirecrackerVersion(data_format_version))?;

println!("v{}", key);

Ok(())

}

with this.

Why remove it? I think this will be reworked once we implement the new snapshot versioning schema. Once we do that work, each Firecracker version will declare support for a single MAJOR.MINOR snapshot version which will be decoupled from Firecracker version, but in any case I think it'd be worth being able to print it.

Mh, to me this output always read as "we can create snapshots for these firecracker versions". I guess if it also means "we can restore from these" then its indeed not redundant.

On the other hand, it just prints every firecracker version since we introduced snapshotting, which is completely wrong since there has been multiple non-compatible changes to the snapshot format since then. Do we plan to introduce the new versioning scheme with this release? If yes we can keep it as is (it'll get fixed when the new scheme gets introduced, as you say), but if no I'd rather we remove it (or fix it for the current version scheme).

Did you maybe mean that we should remove this function:

firecracker/src/firecracker/src/main.rs

Lines 513 to 524 in 9b0e03a

    
           fn print_supported_snapshot_versions() { 
        
               let mut versions: Vec<_> = FC_VERSION_TO_SNAP_VERSION 
        
                   .iter() 
        
                   .map(|(key, _)| key.clone()) 
        
                   .collect(); 
        
               versions.sort(); 
        
               println!("Supported snapshot data format versions:"); 
        
               for v in versions.iter() { 
        
                   println!("{v}"); 
        
               } 
        
           }

?

Then I agree. print_snapshot_data_format does not print the supported versions, so I got confused.

roypat · 2023-10-23T13:58:47Z

I think we might also want to remove

firecracker/src/firecracker/src/main.rs

Lines 537 to 551 in 9b0e03a

fn print_snapshot_data_format(snapshot_path: &str) -> Result<(), SnapshotVersionError> {

let mut snapshot_reader =

File::open(snapshot_path).map_err(SnapshotVersionError::OpenSnapshot)?;

let data_format_version = Snapshot::get_data_version(&mut snapshot_reader, &VERSION_MAP)

.map_err(SnapshotVersionError::SnapshotVersion)?;

let (key, _) = FC_VERSION_TO_SNAP_VERSION

.iter()

.find(|(_, &val)| val == data_format_version)

.ok_or_else(|| SnapshotVersionError::FirecrackerVersion(data_format_version))?;

println!("v{}", key);

Ok(())

}

with this.

Why remove it? I think this will be reworked once we implement the new snapshot versioning schema. Once we do that work, each Firecracker version will declare support for a single MAJOR.MINOR snapshot version which will be decoupled from Firecracker version, but in any case I think it'd be worth being able to print it.

Mh, to me this output always read as "we can create snapshots for these firecracker versions". I guess if it also means "we can restore from these" then its indeed not redundant.
On the other hand, it just prints every firecracker version since we introduced snapshotting, which is completely wrong since there has been multiple non-compatible changes to the snapshot format since then. Do we plan to introduce the new versioning scheme with this release? If yes we can keep it as is (it'll get fixed when the new scheme gets introduced, as you say), but if no I'd rather we remove it (or fix it for the current version scheme).

Did you maybe mean that we should remove this function:

firecracker/src/firecracker/src/main.rs

Lines 513 to 524 in 9b0e03a

fn print_supported_snapshot_versions() {

let mut versions: Vec<_> = FC_VERSION_TO_SNAP_VERSION

.iter()

.map(|(key, _)| key.clone())

.collect();

versions.sort();

println!("Supported snapshot data format versions:");

for v in versions.iter() {

println!("{v}");

}

}

?

Then I agree. print_snapshot_data_format does not print the supported versions, so I got confused.

Oh, yes, so sorry! That's the one I was looking at in my editor, seems I failed at cross-referencing with GitHub

roypat

I think we can also do

diff --git a/src/snapshot/src/lib.rs b/src/snapshot/src/lib.rs
index 8b394c4a4..9dfea9a3d 100644
--- a/src/snapshot/src/lib.rs
+++ b/src/snapshot/src/lib.rs
@@ -74,8 +74,6 @@ struct SnapshotHdr {
 pub struct Snapshot {
     hdr: SnapshotHdr,
     version_map: VersionMap,
-    // Required for serialization.
-    target_version: u16,
 }
 
 // Parse a magic_id and return the format version.
@@ -93,11 +91,10 @@ fn build_magic_id(format_version: u16) -> u64 {
 
 impl Snapshot {
     /// Creates a new instance which can only be used to save a new snapshot.
-    pub fn new(version_map: VersionMap, target_version: u16) -> Snapshot {
+    pub fn new(version_map: VersionMap) -> Snapshot {
         Snapshot {
             version_map,
             hdr: SnapshotHdr::default(),
-            target_version,
         }
     }
 
@@ -194,7 +191,7 @@ impl Snapshot {
         O: Versionize + Debug,
     {
         self.hdr = SnapshotHdr {
-            data_version: self.target_version,
+            data_version: self.version_map.latest_version(),
         };
 
         let format_version_map = Self::format_version_map();
@@ -216,7 +213,7 @@ impl Snapshot {
 
         // Serialize the object using the state version map.
         object
-            .serialize(&mut writer, &self.version_map, self.target_version)
+            .serialize(&mut writer, &self.version_map, self.version_map.latest_version())
             .map_err(Error::Versionize)?;
         writer
             .flush()
diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs
index 7002426b4..d502cc7f3 100644
--- a/src/vmm/src/persist.rs
+++ b/src/vmm/src/persist.rs
@@ -209,8 +209,6 @@ pub fn create_snapshot(
     params: &CreateSnapshotParams,
     version_map: VersionMap,
 ) -> Result<(), CreateSnapshotError> {
-    let snapshot_data_version = version_map.latest_version();
-
     let microvm_state = vmm
         .save_state(vm_info)
         .map_err(CreateSnapshotError::MicrovmState)?;
@@ -218,7 +216,6 @@ pub fn create_snapshot(
     snapshot_state_to_file(
         &microvm_state,
         &params.snapshot_path,
-        snapshot_data_version,
         version_map,
     )?;
 
@@ -230,7 +227,6 @@ pub fn create_snapshot(
 fn snapshot_state_to_file(
     microvm_state: &MicrovmState,
     snapshot_path: &Path,
-    snapshot_data_version: u16,
     version_map: VersionMap,
 ) -> Result<(), CreateSnapshotError> {
     use self::CreateSnapshotError::*;
@@ -241,7 +237,7 @@ fn snapshot_state_to_file(
         .open(snapshot_path)
         .map_err(|err| SnapshotBackingFile("open", err))?;
 
-    let mut snapshot = Snapshot::new(version_map, snapshot_data_version);
+    let mut snapshot = Snapshot::new(version_map);
     snapshot
         .save(&mut snapshot_file, microvm_state)
         .map_err(SerializeMicrovmState)?;

and then fix up the tests accordingly

src/vmm/src/device_manager/mmio.rs

tests/integration_tests/functional/test_snapshot_basic.py

bchalios · 2023-10-23T15:56:48Z

I think we can also do

diff --git a/src/snapshot/src/lib.rs b/src/snapshot/src/lib.rs
index 8b394c4a4..9dfea9a3d 100644
--- a/src/snapshot/src/lib.rs
+++ b/src/snapshot/src/lib.rs
@@ -74,8 +74,6 @@ struct SnapshotHdr {
 pub struct Snapshot {
     hdr: SnapshotHdr,
     version_map: VersionMap,
-    // Required for serialization.
-    target_version: u16,
 }
 
 // Parse a magic_id and return the format version.
@@ -93,11 +91,10 @@ fn build_magic_id(format_version: u16) -> u64 {
 
 impl Snapshot {
     /// Creates a new instance which can only be used to save a new snapshot.
-    pub fn new(version_map: VersionMap, target_version: u16) -> Snapshot {
+    pub fn new(version_map: VersionMap) -> Snapshot {
         Snapshot {
             version_map,
             hdr: SnapshotHdr::default(),
-            target_version,
         }
     }
 
@@ -194,7 +191,7 @@ impl Snapshot {
         O: Versionize + Debug,
     {
         self.hdr = SnapshotHdr {
-            data_version: self.target_version,
+            data_version: self.version_map.latest_version(),
         };
 
         let format_version_map = Self::format_version_map();
@@ -216,7 +213,7 @@ impl Snapshot {
 
         // Serialize the object using the state version map.
         object
-            .serialize(&mut writer, &self.version_map, self.target_version)
+            .serialize(&mut writer, &self.version_map, self.version_map.latest_version())
             .map_err(Error::Versionize)?;
         writer
             .flush()
diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs
index 7002426b4..d502cc7f3 100644
--- a/src/vmm/src/persist.rs
+++ b/src/vmm/src/persist.rs
@@ -209,8 +209,6 @@ pub fn create_snapshot(
     params: &CreateSnapshotParams,
     version_map: VersionMap,
 ) -> Result<(), CreateSnapshotError> {
-    let snapshot_data_version = version_map.latest_version();
-
     let microvm_state = vmm
         .save_state(vm_info)
         .map_err(CreateSnapshotError::MicrovmState)?;
@@ -218,7 +216,6 @@ pub fn create_snapshot(
     snapshot_state_to_file(
         &microvm_state,
         &params.snapshot_path,
-        snapshot_data_version,
         version_map,
     )?;
 
@@ -230,7 +227,6 @@ pub fn create_snapshot(
 fn snapshot_state_to_file(
     microvm_state: &MicrovmState,
     snapshot_path: &Path,
-    snapshot_data_version: u16,
     version_map: VersionMap,
 ) -> Result<(), CreateSnapshotError> {
     use self::CreateSnapshotError::*;
@@ -241,7 +237,7 @@ fn snapshot_state_to_file(
         .open(snapshot_path)
         .map_err(|err| SnapshotBackingFile("open", err))?;
 
-    let mut snapshot = Snapshot::new(version_map, snapshot_data_version);
+    let mut snapshot = Snapshot::new(version_map);
     snapshot
         .save(&mut snapshot_file, microvm_state)
         .map_err(SerializeMicrovmState)?;

and then fix up the tests accordingly

Would you mind if we did this when we redefine the Snapshot structure, with the new versioning schema? At the moment, nothing actually uses target_version in the Snapshot type. The code that calls into this, just uses the latest version.

The reason why I want to avoid doing this now, is because this code is used by the snapshot_editor tool, to edit snapshots. It might edit older snapshots, so the target_version is needed.

roypat · 2023-10-23T15:59:22Z

Would you mind if we did this when we redefine the Snapshot structure, with the new versioning schema? At the moment, nothing actually uses target_version in the Snapshot type. The code that calls into this, just uses the latest version.

The reason why I want to avoid doing this now, is because this code is used by the snapshot_editor tool, to edit snapshots. It might edit older snapshots, so the target_version is needed.

Ahh, didnt see that. Yeah, that's totally fine

CHANGELOG.md

Forward compatibility refers to the ability of a Firecracker binary to create snapshots for older versions of Firecracker. We have found this feature to not be useful, while it increases code and testing complexity. This commit removes forward compatibility support from Firecracker binary. Signed-off-by: Babis Chalios <bchalios@amazon.es>

We do not need these any more, because we will never serialize to older versions of Firecracker snapshots. Also, remove various unit tests that were checking for failures depending on which version we are serializing to. This clean up in the tests is not complete. There will be a general re-structure once we fix the snapshot versioning format. Signed-off-by: Babis Chalios <bchalios@amazon.es>

Previously, using the '--version' flag of the Firecracker binary would print the Firecracker version and the supported snapshot versions. Change the behaviour to only print the Firecracker version. Signed-off-by: Babis Chalios <bchalios@amazon.es>

We dropped support for Firecracker snapshots forward compatibility. Clean up the tests related to this functionality. Mainly remove tests that directly use the removed feature and re-organize some of the code to adapt to the now missing functionality. Signed-off-by: Babis Chalios <bchalios@amazon.es>

bchalios force-pushed the drop_snapshot_forward_compatibility branch from 097d0f7 to 1378cd4 Compare October 23, 2023 11:22

bchalios marked this pull request as ready for review October 23, 2023 11:23

bchalios force-pushed the drop_snapshot_forward_compatibility branch from 1378cd4 to 6a820e6 Compare October 23, 2023 11:38

bchalios requested review from xmarcalx, kalyazin, pb8o and wearyzen as code owners October 23, 2023 11:38

bchalios self-assigned this Oct 23, 2023

bchalios added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Oct 23, 2023

ShadowCurse reviewed Oct 23, 2023

View reviewed changes

src/api_server/src/lib.rs Show resolved Hide resolved

roypat reviewed Oct 23, 2023

View reviewed changes

src/vmm/src/device_manager/mmio.rs Outdated Show resolved Hide resolved

tests/integration_tests/functional/test_snapshot_basic.py Outdated Show resolved Hide resolved

roypat previously approved these changes Oct 23, 2023

View reviewed changes

bchalios dismissed roypat’s stale review via 3d3903e October 23, 2023 16:02

bchalios force-pushed the drop_snapshot_forward_compatibility branch 4 times, most recently from b578692 to 87f0d81 Compare October 23, 2023 17:46

wearyzen reviewed Oct 24, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ShadowCurse previously approved these changes Oct 25, 2023

View reviewed changes

bchalios added 3 commits October 25, 2023 08:59

bchalios dismissed ShadowCurse’s stale review via e960d8d October 25, 2023 08:59

bchalios force-pushed the drop_snapshot_forward_compatibility branch from 629aff2 to e960d8d Compare October 25, 2023 08:59

Merge branch 'main' into drop_snapshot_forward_compatibility

2c0c688

roypat approved these changes Oct 25, 2023

View reviewed changes

wearyzen approved these changes Oct 25, 2023

View reviewed changes

roypat merged commit ae9aed2 into firecracker-microvm:main Oct 25, 2023
5 checks passed

bchalios deleted the drop_snapshot_forward_compatibility branch October 25, 2023 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop support for forward compatibility #4194

Drop support for forward compatibility #4194

bchalios commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 20, 2023 •

edited

Loading

roypat commented Oct 23, 2023

bchalios commented Oct 23, 2023

roypat commented Oct 23, 2023

bchalios commented Oct 23, 2023

roypat commented Oct 23, 2023

roypat left a comment

bchalios commented Oct 23, 2023

roypat commented Oct 23, 2023

Drop support for forward compatibility #4194

Drop support for forward compatibility #4194

Conversation

bchalios commented Oct 20, 2023 • edited Loading

Changes

Reason

License Acceptance

PR Checklist

codecov bot commented Oct 20, 2023 • edited Loading

Codecov Report

roypat commented Oct 23, 2023

bchalios commented Oct 23, 2023

roypat commented Oct 23, 2023

bchalios commented Oct 23, 2023

roypat commented Oct 23, 2023

roypat left a comment

Choose a reason for hiding this comment

bchalios commented Oct 23, 2023

roypat commented Oct 23, 2023

bchalios commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 20, 2023 •

edited

Loading