Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Vhost user block #4170

Merged
merged 22 commits into from
Nov 8, 2023
Merged

Vhost user block #4170

merged 22 commits into from
Nov 8, 2023

Conversation

ShadowCurse
Copy link
Contributor

@ShadowCurse ShadowCurse commented Oct 12, 2023 •
edited
Loading

Changes

This PR adds vhost-user-bock device implementation with unit tests and initial integration test.
In order to configure new device type, old /drive endpoint is used with new parameter socket.
Depending if certain fields are present in the request body, different types of device can be configured. More details in the swagger file.

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

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet contribution quality standards.
  • This functionality cannot be added in rust-vmm.

@ShadowCurse ShadowCurse self-assigned this Oct 12, 2023
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk branch 2 times, most recently from bc4074a to 3aa7d42 Compare October 13, 2023 09:00
kalyazin
kalyazin reviewed Oct 13, 2023
View reviewed changes
src/vmm/src/devices/virtio/block/device.rs Show resolved Hide resolved
src/vmm/src/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/vhost_user.rs Show resolved Hide resolved
src/vmm/src/devices/virtio/vhost_user_block/device.rs Show resolved Hide resolved
src/api_server/swagger/firecracker.yaml Outdated Show resolved Hide resolved
src/api_server/swagger/firecracker.yaml Outdated Show resolved Hide resolved
src/api_server/swagger/firecracker.yaml Outdated Show resolved Hide resolved
src/vmm/src/builder.rs Show resolved Hide resolved
tests/integration_tests/functional/test_vhost_user_block.py Outdated Show resolved Hide resolved
@kalyazin
Copy link
Contributor

I know it is a tedious thing to do, but at some point (if you haven't already) we need to make sure every commit builds and unit tests pass.

@ShadowCurse ShadowCurse force-pushed the vhost_user_blk branch 6 times, most recently from f1e5873 to 3ae0d8d Compare October 18, 2023 13:22
@codecov
Copy link

codecov bot commented Oct 18, 2023 •
edited
Loading

Codecov Report

Attention: 316 lines in your changes are missing coverage. Please review.

Comparison is base (2613007) 82.98% compared to head (edec970) 82.20%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4170      +/-   ##
==========================================
- Coverage   82.98%   82.20%   -0.79%     
==========================================
  Files         230      236       +6     
  Lines       28710    29203     +493     
==========================================
+ Hits        23826    24007     +181     
- Misses       4884     5196     +312
Flag Coverage Δ
4.14-c7g.metal 77.75% <60.93%> (-0.81%) ⬇️
4.14-m5d.metal 79.58% <60.93%> (-0.79%) ⬇️
4.14-m6a.metal 78.72% <60.93%> (-0.80%) ⬇️
4.14-m6g.metal 77.75% <60.93%> (-0.81%) ⬇️
4.14-m6i.metal 79.57% <60.93%> (-0.79%) ⬇️
5.10-c7g.metal 80.64% <60.81%> (-0.83%) ⬇️
5.10-m5d.metal 82.23% <60.81%> (-0.80%) ⬇️
5.10-m6a.metal 81.47% <60.81%> (-0.81%) ⬇️
5.10-m6g.metal 80.64% <60.81%> (-0.83%) ⬇️
5.10-m6i.metal 82.23% <60.81%> (-0.80%) ⬇️
6.1-c7g.metal 80.64% <60.81%> (-0.83%) ⬇️
6.1-m5d.metal 82.23% <60.81%> (-0.81%) ⬇️
6.1-m6a.metal 81.47% <60.81%> (-0.81%) ⬇️
6.1-m6g.metal 80.64% <60.81%> (-0.83%) ⬇️
6.1-m6i.metal 82.23% <60.81%> (-0.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/vmm/src/devices/bus.rs 64.84% <ø> (ø)
src/vmm/src/devices/mod.rs 88.88% <ø> (ø)
src/vmm/src/devices/virtio/balloon/device.rs 88.34% <ø> (ø)
...rc/vmm/src/devices/virtio/balloon/event_handler.rs 72.72% <ø> (ø)
src/vmm/src/devices/virtio/balloon/mod.rs 50.00% <ø> (ø)
src/vmm/src/devices/virtio/balloon/persist.rs 82.47% <ø> (ø)
src/vmm/src/devices/virtio/block_common.rs 100.00% <100.00%> (ø)
src/vmm/src/devices/virtio/device.rs 88.52% <ø> (ø)
src/vmm/src/devices/virtio/iovec.rs 98.64% <ø> (ø)
src/vmm/src/devices/virtio/mmio.rs 86.45% <100.00%> (+0.51%) ⬆️
... and 42 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ShadowCurse ShadowCurse force-pushed the vhost_user_blk branch 7 times, most recently from c4058d1 to a11cc77 Compare October 18, 2023 16:16
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk branch 5 times, most recently from 2adb53c to d7f610f Compare October 19, 2023 16:48
@kalyazin kalyazin mentioned this pull request Oct 20, 2023
Merged
4 tasks
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk branch 3 times, most recently from d9884a3 to 4c1a788 Compare October 20, 2023 12:46
ShadowCurse and others added 22 commits November 7, 2023 17:17
@ShadowCurse
refactor(block): separate file block config
3f50700 
Created a separate block config struct and
updated Block::new method to accept it instead of
separate parameters.
Updated block restoration code as well.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
refactor(block): renamed Block to VirtioBlock
c967cd0 
This is done to better differentiate between
virtio devices and vhost-user devices.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse @kalyazin
refactor(vmm): introduce block device enum
5b1880f 
In order to allow creation of other block device subtypes,
we wrap former Block structure into an enum.
At the moment it contains a single variant `VirtioBlock`.
Further on it will be extended with other device subtypes.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Co-authored-by: Nikita Kalyazin <kalyazin@amazon.com>
@ShadowCurse @kalyazin @dianpopa
feat: added vhost-user impl
8e16c67 
Added vhost-user wrapper for the interraction with
backend.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Co-authored-by: Nikita Kalyazin <kalyazin@amazon.com>
Co-authored-by: Diana Popa <dpopa@amazon.com>
@ShadowCurse @kalyazin @dianpopa
feat(vhost-user-block): added vhost-user-block
8ec0b6f 
Added implementation for vhost-user-block device.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Co-authored-by: Nikita Kalyazin <kalyazin@amazon.com>
Co-authored-by: Diana Popa <dpopa@amazon.com>
@ShadowCurse
feat: added vhost-user-block to the BlockDeviceType enum
3c5f118 
Now `BlockDeviceType` has both virtio-block and vhost-user-block.
Because we can not test vhost-user-block at this time, we
skip vhost-user-block in the unit tests.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
feat: added vhost-user to device manager save/restore paths
1616a72 
Even though we don't support creation of snapshots with
vhost-user-block device we add it to the restoration path
for future use.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
feat: updated BlockDeviceConfig with vhost-user-block parameters
870f396 
Updated `BlockDeviceConfig` with vhost-user-block specific field.
Updated methods to convert `BlockDeviceConfig` into
`VirtIOBlockConfig` or `VhostUserBlockConfig`.
Updated integration tests with new ordering of fields in the
`BlockDeviceConfig`.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
refactor: removed reexport of virtio modules
d498461 
This is done to make it clear where the imports are comming from.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
refactor: moved CacheType out of Block to share it with vhost_user_blk
ecea429 
CacheType is shared between VirtioBlock and VhostUserBlock.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
refactor(block): refactor of VirtIOBlock
871d05e 
Simplified `DiskProperties` implementation.
Removed a lot of getters from `VirtIOBlock`
Removed `CacheTypeState` because it is same as `CacheType`

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
refactor: moved block to virtio_block
955b972 
Now virtio_block is named similar to vhost_user_block

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
refactor: removed duplication from block device attachment
39b8e7e 
Moved duplicated code into a macro in `attach_block_devices`

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
feat(virtio/mmio): MmioTransport knows about vhost_user
437b8e8 
Now `MmioTransport` stores info if the device uses
vhost_user implementation. Based on this info the
interrupt status reported to the guest is either
an updatde interrupt status (for usual virtio devices)
or `VIRTIO_MMIO_INT_VRING`(0x1) for vhost_user devices.
This is done because vhost_user devices can not update
interrupt status variable in the FC.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
feat: updated swagger file with new block device fields
85c27a6 
Updated swagger for `drive` api with new descriptions
and fields for vhost-user-block.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
fix(integration tests): black formatting
d16746e 
For some reason this file was not formatted.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
feat(seccomp): added sendmsg to the seccomp filters
0e682ee 
Added `sendmsg` syscall to the seccomp. It is needed for
vhost-user-block.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
tests(vhost-user-block): added initial integration test
33eb5ea 
Added a simple test that boots a VM with vhost-user-block
as root block device.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
fix(docs): updated Unsafe cache type description
aeae9a5 
In firecracker-microvm#2702 the behaviour of the `Unsafe` cache type
was changed. If selected the `flush` feature is
not advertised to the guest. This commit just
adds missing documentation change.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
feat(vhost-user): added unit tests
ab16b74 
Added a trait that helps with mocking `vhost::Master`
struct.
Added unit tests for feature negotiated methods.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
test(vhost-user-block): added unit tests
a7059a6 
Made `VhostUserBlock` generic on `VhostUserHandleBackend`.
Added unit tests for `VhostUserBlock` creation and
feature negotiation.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
refactor(tests): moved create_test_socket into test module
edec970 
Moved `create_test_socket` into test module

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse ShadowCurse merged commit 8d1f58b into firecracker-microvm:main Nov 8, 2023
5 of 7 checks passed
@ShadowCurse ShadowCurse deleted the vhost_user_blk branch November 8, 2023 10:23
@roypat roypat mentioned this pull request Nov 13, 2023
Merged
3 tasks
@kalyazin kalyazin mentioned this pull request Nov 23, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wearyzen wearyzen wearyzen left review comments

@kalyazin kalyazin kalyazin approved these changes

@bchalios bchalios bchalios approved these changes

@xmarcalx xmarcalx Awaiting requested review from xmarcalx xmarcalx is a code owner

@pb8o pb8o Awaiting requested review from pb8o pb8o is a code owner

Assignees

@ShadowCurse ShadowCurse

Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Enhancement Indicates new feature requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ShadowCurse @kalyazin @bchalios @wearyzen