firecracker icon indicating copy to clipboard operation
firecracker copied to clipboard

Refactor Snapshot Module

Open gjkeller opened this issue 6 months ago • 3 comments

Draft PR as I finish the migration to new API and run linting/tests/build.

Changes

  • Forked and pulled latest changes to @beamandala's fork of Firecracker associated with draft PR #4691
  • Refactored snapshot/mod.rs to reflect structure noted in #4523 and changes requested in draft PR #4691:
    • Specifically, changed the way that load was done for Snapshots so that the snapshot file is only read once (wrap Reader with CrcReader and pass into load_unchecked)
    • Implemented some of my own changes to ensure it is clear which methods should be accessed by users of this API, but kept the general aforementioned structure:
#[derive(Serialize, Deserialize)]
pub struct SnapshotHeader {
    magic: u64,
    version: Version
}

impl SnapshotHeader {
  fn load<R: Read>(reader: &mut R) -> Result<Self> { ... }
  fn store<W: Write>(writer: &mut W) -> Result<Self> { ... }
}

#[derive(Serialize, Deserialize)]
pub struct Snapshot<Data> {
    header: SnapshotHeader,
    data: Data
}

impl<Data: Deserialize> Snapshot<Data> {
    fn load_unchecked<R: Read>(reader: &mut R) -> Result<Self> { ... }
    fn load<R: Read>(reader: &mut R) -> Result<Self> { ... }
    
} 

impl<Data: Serialize> Snapshot<Data> {
  fn save<W: Write>(&self, writer: &mut W) -> Result<usize> { ... }
  fn save_with_crc<W: Write>(&self, writer: &mut W) -> Result<usize> { ... }
}
  • Removed load_with_version_check and associated test as it was unused
  • Closes #4523, #4691

Reason

I took on @beamandala's draft PR with their permission to finish up the work. These changes were made so that the API would be easier to use / fall in line with general Rust conventions.

Instead of doing the following:

let snap = Snapshot::new(Version::new(1,0,0));

let (vm_state, ver) = snap.load(&mut reader, len)?;
// …or…
snap.save(&mut writer, &vm_state)?;

Which allows for incomplete variants of Snapshot (snapshots without data), the following is now done:

let snap: Snapshot<MyVmState> = Snapshot::load(&mut reader)?;
let ver = snap.version();
let state = snap.data;
// ...or...
let snap = Snapshot::new(Version::new(1,0,0), vm_state);
snap.save(&mut writer)?;

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

  • [ X ] I have read and understand CONTRIBUTING.md.
  • [ ] I have run tools/devtool checkstyle to verify that the PR passes the automated style checks.
  • [ ] I have described what is done in these changes, why they are needed, and how they are solving the problem in a clear and encompassing way.
  • [ ] I have updated any relevant documentation (both in code and in the docs) in the PR.
  • [ ] I have mentioned all user-facing changes in CHANGELOG.md.
  • [ ] If a specific issue led to this PR, this PR closes the issue.
  • [ ] When making API changes, I have followed the Runbook for Firecracker API changes.
  • [ ] I have tested all new and changed functionalities in unit tests and/or integration tests.
  • [ ] I have linked an issue to every new TODO.

  • [ ] This functionality cannot be added in rust-vmm.

gjkeller avatar May 07 '25 04:05 gjkeller