Skip to content

Commit

Permalink
snapshot: Remove max_connections and max_pending_resets fields
Browse files Browse the repository at this point in the history
`TcpIPv4Handler` for MMDS network stack preallocates several buffers
whose sizes are saved into a snapshot as `max_connections` and
`max_pending_resets` in `MmdsNetworkStackState`. But they are always the
same constant hardcoded values (`DEFAULT_MAX_CONNECTIONS` and
`DEFAULT_MAX_PENDING_RESETS`) as of today, which means there is no need
to save them into a snapshot. Even if we change the hardcoded sizes
across Firecracker versions, that should not be a problem. This is
because the snapshot feature does not support migration of network
connections and those buffers are initialized with empty on snapshot
restoration. When migrating from a Firecracker version with larger
buffers to another version with smaller ones, guest workloads that
worked previously might start to fail due to the less buffer spaces.
However, the issue is not a problem of the snapshot feature and it
should also occur even on a purely booted microVM (not restored from a
snapshot). Thus, it is fine to remove those fields from a snapshot.

Since this is a breaking change of the snapshot format, bumps the major
version.

Signed-off-by: Takahiro Itazuri <[email protected]>
  • Loading branch information
zulinx86 committed Nov 14, 2024
1 parent 32c6ed4 commit 588c7f0
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 28 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ and this project adheres to

### Changed

- [#4913](https://github.com/firecracker-microvm/firecracker/pull/4913): Removed
unnecessary fields (`max_connections` and `max_pending_resets`) from the
snapshot format, bumping the snapshot version to 5.0.0.

### Deprecated

### Removed
Expand Down
15 changes: 3 additions & 12 deletions src/vmm/src/mmds/ns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ impl MmdsNetworkStack {
mac_addr: MacAddr,
ipv4_addr: Ipv4Addr,
tcp_port: u16,
max_connections: NonZeroUsize,
max_pending_resets: NonZeroUsize,
mmds: Arc<Mutex<Mmds>>,
) -> Self {
MmdsNetworkStack {
Expand All @@ -93,8 +91,8 @@ impl MmdsNetworkStack {
tcp_handler: TcpIPv4Handler::new(
ipv4_addr,
tcp_port,
max_connections,
max_pending_resets,
NonZeroUsize::new(DEFAULT_MAX_CONNECTIONS).unwrap(),
NonZeroUsize::new(DEFAULT_MAX_PENDING_RESETS).unwrap(),
),
mmds,
}
Expand All @@ -105,14 +103,7 @@ impl MmdsNetworkStack {
let ipv4_addr = mmds_ipv4_addr.unwrap_or_else(|| Ipv4Addr::from(DEFAULT_IPV4_ADDR));

// The unwrap()s are safe because the given literals are greater than 0.
Self::new(
mac_addr,
ipv4_addr,
DEFAULT_TCP_PORT,
NonZeroUsize::new(DEFAULT_MAX_CONNECTIONS).unwrap(),
NonZeroUsize::new(DEFAULT_MAX_PENDING_RESETS).unwrap(),
mmds,
)
Self::new(mac_addr, ipv4_addr, DEFAULT_TCP_PORT, mmds)
}

pub fn set_ipv4_addr(&mut self, ipv4_addr: Ipv4Addr) {
Expand Down
15 changes: 0 additions & 15 deletions src/vmm/src/mmds/persist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
//! Defines the structures needed for saving/restoring MmdsNetworkStack.
use std::net::Ipv4Addr;
use std::num::NonZeroUsize;
use std::sync::{Arc, Mutex};

use serde::{Deserialize, Serialize};
Expand All @@ -20,8 +19,6 @@ pub struct MmdsNetworkStackState {
mac_addr: [u8; MAC_ADDR_LEN as usize],
ipv4_addr: u32,
tcp_port: u16,
max_connections: NonZeroUsize,
max_pending_resets: NonZeroUsize,
}

impl Persist<'_> for MmdsNetworkStack {
Expand All @@ -37,8 +34,6 @@ impl Persist<'_> for MmdsNetworkStack {
mac_addr,
ipv4_addr: self.ipv4_addr.into(),
tcp_port: self.tcp_handler.local_port(),
max_connections: self.tcp_handler.max_connections(),
max_pending_resets: self.tcp_handler.max_pending_resets(),
}
}

Expand All @@ -50,8 +45,6 @@ impl Persist<'_> for MmdsNetworkStack {
MacAddr::from_bytes_unchecked(&state.mac_addr),
Ipv4Addr::from(state.ipv4_addr),
state.tcp_port,
state.max_connections,
state.max_pending_resets,
mmds,
))
}
Expand Down Expand Up @@ -83,13 +76,5 @@ mod tests {
restored_ns.tcp_handler.local_port(),
ns.tcp_handler.local_port()
);
assert_eq!(
restored_ns.tcp_handler.max_connections(),
ns.tcp_handler.max_connections()
);
assert_eq!(
restored_ns.tcp_handler.max_pending_resets(),
ns.tcp_handler.max_pending_resets()
);
}
}
2 changes: 1 addition & 1 deletion src/vmm/src/persist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ pub enum CreateSnapshotError {
}

/// Snapshot version
pub const SNAPSHOT_VERSION: Version = Version::new(4, 0, 0);
pub const SNAPSHOT_VERSION: Version = Version::new(5, 0, 0);

/// Creates a Microvm snapshot.
pub fn create_snapshot(
Expand Down

0 comments on commit 588c7f0

Please sign in to comment.