Drop versionize support, substitute with serde
#4230
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR depends on the equivalent PR on kvm-bindings side: firecracker-microvm/kvm-bindings#21 |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4230 +/- ##
==========================================
- Coverage 81.57% 81.53% -0.05%
==========================================
Files 240 241 +1
Lines 29408 29238 -170
==========================================
- Hits 23990 23838 -152
+ Misses 5418 5400 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bchalios
force-pushed
the
drop_versionize_support
branch
7 times, most recently
from
948f6e0
to
3d54ac2
Compare
bchalios
added
the
Status: Awaiting review
|
I deliberately split up the commits that switch types from |
bchalios
force-pushed
the
drop_versionize_support
branch
3 times, most recently
from
771191d
to
e0a93b7
Compare
roypat
reviewed
Nov 15, 2023
bchalios
force-pushed
the
drop_versionize_support
branch
from
e0a93b7
to
cad3c1e
Compare
ShadowCurse
reviewed
Nov 15, 2023
bchalios
force-pushed
the
drop_versionize_support
branch
12 times, most recently
from
8b28725
to
b727249
Compare
Derive Serialize and Deserialize and remove Versionize support. Fix these two structs together because both depend on kvm-bindings implementing (De)serialize for its types, so updating to the kvm-bindings to the version that introduces serde breaks both of these types. While implementing serde for VcpuState also remove the `msrs` field. According to Versionize this was end-of-life and we are anyway breaking compatibility with previous snapshot versions. Signed-off-by: Babis Chalios <bchalios@amazon.es>
Derive Serialize and Deserialize for DeviceStates and remove Versionnize support. Do this for all device types at the same time, since they all depend on VirtIO related state. Signed-off-by: Babis Chalios <bchalios@amazon.es>
Remove versionize and versionize_derive dependencies. Also, re-enable the rust integration tests. Now that we have all of the state serialized with serde these should work again. Signed-off-by: Babis Chalios <bchalios@amazon.es>
This test relies on the fact that the current Firecracker binary can parse snapshots that were created with a previous Firecracker version. 1.7.0 changes the snapshot format header and its parser only understands the new format. As a result, it can't parse previous snapshots. We fix this by skipping the tests for Firecracker versions previous to 1.7.0. Signed-off-by: Babis Chalios <bchalios@amazon.es>
We were skipping the serialization of the "boot_args" field of the "boot-source" objectm, if kernel boot arguments were not supported. Now, we return None. This was done so that we have consistent snapshot formats, now that we use serde to serialize microVM state in a snapshot file. Amend this test to expect '"boot_args": None' in the "boot-source" object. Signed-off-by: Babis Chalios <bchalios@amazon.es>
Snapshot versions are now independent from Firecracker binary versions. Use Firecracker --snapshot-version flag to retrieve the supported snapshot data format version for checking if the created snapshot is supported from the current release. Signed-off-by: Babis Chalios <bchalios@amazon.es>
With the new snapshot format we break compatibility with all previous snapshot format versions. This commits removes all the tests that were checking functional correctness and performance of loading older snapshots in the current Firecracker binary. In the future, it might be that two Firecracker binaries support the same snapshot versions. When that happens we should thing of a proper way to test that this works, but the current tests are not fit for purpose. Signed-off-by: Babis Chalios <bchalios@amazon.es>
Adapt documentation around snapshot versions format and compatibility to remove mentions of Versionize and the effects its usage had to snapshot compatibility. Also, add a CHANGELOG entry regarding the new strategy for snapshot versioning. Signed-off-by: Babis Chalios <bchalios@amazon.es>
9 tasks
bchalios
force-pushed
the
drop_versionize_support
branch
from
274f087
to
d58177b
Compare
|
Changes v4 -> v5:
|
pb8o
approved these changes
Jan 15, 2024
ShadowCurse
approved these changes
Jan 15, 2024
roypat
added a commit
to roypat/firecracker
that referenced
this pull request
Jan 22, 2024
They were needed for Versionize reasons, but with firecracker-microvm#4230 merged, we can eliminate them, simplifying the code. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
9 tasks
roypat
added a commit
to roypat/firecracker
that referenced
this pull request
Jan 22, 2024
They were needed for Versionize reasons, but with firecracker-microvm#4230 merged, we can eliminate them, simplifying the code. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
added a commit
to roypat/firecracker
that referenced
this pull request
Jan 23, 2024
They were needed for Versionize reasons, but with firecracker-microvm#4230 merged, we can eliminate them, simplifying the code. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
added a commit
to roypat/firecracker
that referenced
this pull request
Mar 1, 2024
When switching from Versionize to bincode in firecracker-microvm#4230, we accidentally dropped a check limiting how much memory the deserialization routine can allocate [[1]]. This commit reimplements this check for the new bincode-based deserialization routine, with a limit matching that of the old Versionize check. [1]: https://github.com/firecracker-microvm/versionize/blob/main/src/primitives.rs#L14C33-L14C43 Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
added a commit
to roypat/firecracker
that referenced
this pull request
Mar 1, 2024
When switching from Versionize to bincode in firecracker-microvm#4230, we accidentally dropped a check limiting how much memory the deserialization routine can allocate [[1]]. This commit reimplements this check for the new bincode-based deserialization routine, with a limit matching that of the old Versionize check. [1]: https://github.com/firecracker-microvm/versionize/blob/main/src/primitives.rs#L14C33-L14C43 Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
9 tasks
roypat
added a commit
to roypat/firecracker
that referenced
this pull request
Mar 1, 2024
When switching from Versionize to bincode in firecracker-microvm#4230, we accidentally dropped a check limiting how much memory the deserialization routine can allocate [[1]]. This commit reimplements this check for the new bincode-based deserialization routine, with a limit matching that of the old Versionize check. [1]: https://github.com/firecracker-microvm/versionize/blob/main/src/primitives.rs#L14C33-L14C43 Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
added a commit
that referenced
this pull request
Mar 1, 2024
When switching from Versionize to bincode in #4230, we accidentally dropped a check limiting how much memory the deserialization routine can allocate [[1]]. This commit reimplements this check for the new bincode-based deserialization routine, with a limit matching that of the old Versionize check. [1]: https://github.com/firecracker-microvm/versionize/blob/main/src/primitives.rs#L14C33-L14C43 Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
added a commit
to roypat/firecracker
that referenced
this pull request
Mar 1, 2024
When switching from Versionize to bincode in firecracker-microvm#4230, we accidentally dropped a check limiting how much memory the deserialization routine can allocate [[1]]. This commit reimplements this check for the new bincode-based deserialization routine, with a limit matching that of the old Versionize check. [1]: https://github.com/firecracker-microvm/versionize/blob/main/src/primitives.rs#L14C33-L14C43 Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
added a commit
that referenced
this pull request
Mar 1, 2024
When switching from Versionize to bincode in #4230, we accidentally dropped a check limiting how much memory the deserialization routine can allocate [[1]]. This commit reimplements this check for the new bincode-based deserialization routine, with a limit matching that of the old Versionize check. [1]: https://github.com/firecracker-microvm/versionize/blob/main/src/primitives.rs#L14C33-L14C43 Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Remove support for
Versionizefrom Firecracker and substitute it withserde. We usebincodefor the actual (de)serialization. Also, define a snapshot format version that is decoupled from Firecracker version. The newly defined version of the snapshot format will follow semantic versioning and it will start from version1.0.0.This PR breaks compatibility with all previous snapshot versions. From now on, snapshot compatibility will be reflected to the snapshot version. Changes in the major release number are backwards incompatible, changes in the minor release number are backwards compatible and changes in patch releases are always compatible.
That said, we are currently using
bincodefor the serialization, which means that all changes in the snapshot format will be backwards incompatible (they will imply a major version number bump). However, if the need arises we will investigate alternatives tobincodethat would allow greater flexibility for backwards compatibility.This change makes a lot of our integration tests redundant.
Reason
We used
Versionizeto allow for extended backwards compatibility for Firecracker snapshots. In practice we did not find this feature useful, so we paid the code maintenance and testing complexity cost without a measurable benefit.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
CHANGELOG.md.TODOs link to an issue.rust-vmm.