increase the number of IRQs on x86_64 #2286
Conversation
luminitavoicu
left a comment
luminitavoicu
left a comment
There was a problem hiding this comment.
LGTM. I only have a couple of nits:
iulianbarbu
left a comment
iulianbarbu
left a comment
There was a problem hiding this comment.
We should also add a test were we attach devices up to 24 and over 24, checking at the same time if the uVM devices are operational.
iulianbarbu
dismissed
their stale review
@luminitavoicu will push tests to this PR. Will wait for them.
serban300
force-pushed
the
IRQs
branch
2 times, most recently
from
db34adc to
7706583
Compare
|
When using more than 16 irqs we need to see if saved snapshots of microvms in v0.23 format can still be restored with Firecracker v0.23 which does not have these changes. |
I checked and this doesn't work. As we discussed offline we shouldn't allow snapshotting for v0.23 from the versions that have this improvement. |
|
I added integration tests for the two scenarios mentioned above by @iulianbarbu. |
acatangiu
left a comment
acatangiu
left a comment
There was a problem hiding this comment.
Fix and test look good.
What about the snapshot sanity check where we don't allow creating older version snapshots when we have many devices? Do we add that with this PR? Or is it planned for a subsequent PR (needs high prio issue if that's the case)?
The snapshot check is still in progress. I was planning on adding that commit to this PR when it's ready. |
luminitavoicu
force-pushed
the
IRQs
branch
3 times, most recently
from
1eb4eaa to
188f531
Compare
luminitavoicu
force-pushed
the
IRQs
branch
2 times, most recently
from
662d56f to
36e37c5
Compare
luminitavoicu
force-pushed
the
IRQs
branch
3 times, most recently
from
32ddc46 to
9c4d6fc
Compare
luminitavoicu
added
the
Status: Awaiting review
Signed-off-by: Serban Iorga <seriorga@amazon.com>
Added tests to cover two scenarios: - after attaching the maximum number of supported devices (that is 19 since the IRQ lines range from 5 to 23), they are still operational; - atempting to boot a microVM which has more than 19 devices attached fails with the expected error. Signed-off-by: Luminita Voicu <lumivo@amazon.com>
Firecracker v0.24 now supports 24 IRQ lines, so it accepts more devices to be attached. Creating a snapshot with a previous version (for example: `v0.23.0`) is only allowed if the number of devices added is consistent to the maximum supported by the requested version. Otherwise, snapshotting will fail with a dedicated error. Integration tests were added to validate the snapshot sanity check. Signed-off-by: Luminita Voicu <lumivo@amazon.com>
| fn validate_devices_number(device_number: usize) -> std::result::Result<(), CreateSnapshotError> { | ||
| use self::CreateSnapshotError::TooManyDevices; | ||
| if device_number > FC_V0_23_MAX_DEVICES as usize { | ||
| return Err(TooManyDevices(device_number)); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
nit: this is function is specific to v0.23-only, it've better been hardcoded in the version match or at least it should have v023 in its name.
6 participants
Reason for This PR
increase the number of IRQs on x86_64
Description of Changes
increase the number of IRQs on x86_64
rust-vmm.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).unsafecode is properly documented.firecracker/swagger.yaml.CHANGELOG.md.