snapshot: Remove max_connections and max_pending_resets fields #4913
+14
−52
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
zulinx86
force-pushed
the
exclude_unnecessary_fields
branch
from
f449568
to
d8f7e64
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4913 +/- ##
==========================================
- Coverage 84.10% 84.07% -0.03%
==========================================
Files 251 251
Lines 28080 28067 -13
==========================================
- Hits 23616 23597 -19
- Misses 4464 4470 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
zulinx86
force-pushed
the
exclude_unnecessary_fields
branch
2 times, most recently
from
524d1bf
to
18db7e5
Compare
zulinx86
changed the title
zulinx86
force-pushed
the
exclude_unnecessary_fields
branch
from
18db7e5
to
9f9c5e0
Compare
zulinx86
requested review from
xmarcalx,
kalyazin,
pb8o and
Manciukic
as code owners
`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 <itazur@amazon.com>
There is no need to use MmdsNetworkStack::new() instead of MmdsNetworkStack::new_with_defaults() in tests that pass the same default values. Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86
force-pushed
the
exclude_unnecessary_fields
branch
from
9f9c5e0
to
762ed4f
Compare
roypat
approved these changes
Nov 14, 2024
Not even this can happen I think, because its just the capacity of the data structures. If they reach that capacity, then they'll simply grow :D |
yeah, I'm on the same page. I tried to make excuses at best I could :P |
zulinx86
added
the
Status: Awaiting review
pb8o
approved these changes
Nov 14, 2024
kalyazin
reviewed
Nov 14, 2024
| @@ -12,6 +12,10 @@ and this project adheres to | |||
|
|
|||
| ### Changed | |||
|
|
|||
| - [#4913](https://github.com/firecracker-microvm/firecracker/pull/4913): Removed | |||
There was a problem hiding this comment.
Not opposing the change, but what are customers supposed to do in response to this?
There was a problem hiding this comment.
Good point, they need to regenerate a snapshot. We might want to apply the same fix for the previous change.
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
max_connectionsandmax_pending_resetsfields from snapshot.Note that I only focused on removing them in this PR although I might open another PR to refactor more later.
Reason
TcpIPv4Handlerfor MMDS network stack preallocates several bufferswhose sizes are saved into a snapshot as
max_connectionsandmax_pending_resetsinMmdsNetworkStackState. But they are always thesame constant hardcoded values (
DEFAULT_MAX_CONNECTIONSandDEFAULT_MAX_PENDING_RESETS) as of today, which means there is no needto 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.
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
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
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.
CHANGELOG.md.[ ] If a specific issue led to this PR, this PR closes the issue.[ ] When making API changes, I have followed theRunbook for Firecracker API changes.
integration tests.
[ ] I have linked an issue to every newTODO.rust-vmm.