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 blk basic config change #4223

Merged
merged 8 commits into from
Nov 23, 2023
Merged

Vhost user blk basic config change #4223

merged 8 commits into from
Nov 23, 2023

Conversation

ShadowCurse
Copy link
Contributor

@ShadowCurse ShadowCurse commented Nov 8, 2023 •
edited
Loading

Changes

Updated PATCH /drive command to trigger config update for vhost-user-block devices if no parameters set.
The config update includes reading new config from the backend and notifying the guest about the changes.
Also added new metric for vhos-user devices that measures time it took to read new config from the backend and
notify guest about it.

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 force-pushed the vhost_user_blk_basic_config_change branch from b432231 to 8ed6269 Compare November 8, 2023 15:34
@ShadowCurse ShadowCurse self-assigned this Nov 8, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 8, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (582003e) 81.71% compared to head (7e12baa) 81.72%.

Files Patch % Lines
src/vmm/src/lib.rs 0.00% 7 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4223   +/-   ##
=======================================
  Coverage   81.71%   81.72%           
=======================================
  Files         240      240           
  Lines       29239    29266   +27     
=======================================
+ Hits        23893    23917   +24     
- Misses       5346     5349    +3
Flag Coverage Δ
4.14-c7g.metal 77.17% <83.33%> (+0.01%) ⬆️
4.14-m5d.metal 79.06% <83.33%> (+<0.01%) ⬆️
4.14-m6a.metal 78.18% <83.33%> (+0.01%) ⬆️
4.14-m6g.metal 77.17% <83.33%> (+0.01%) ⬆️
4.14-m6i.metal 79.05% <83.33%> (+<0.01%) ⬆️
5.10-c7g.metal 80.05% <83.33%> (+<0.01%) ⬆️
5.10-m5d.metal 81.72% <83.33%> (+<0.01%) ⬆️
5.10-m6a.metal 80.93% <83.33%> (+<0.01%) ⬆️
5.10-m6g.metal 80.05% <83.33%> (+<0.01%) ⬆️
5.10-m6i.metal 81.70% <83.33%> (+<0.01%) ⬆️
6.1-c7g.metal 80.05% <83.33%> (+<0.01%) ⬆️
6.1-m5d.metal 81.72% <83.33%> (+<0.01%) ⬆️
6.1-m6a.metal 80.93% <83.33%> (+<0.01%) ⬆️
6.1-m6g.metal 80.05% <83.33%> (+<0.01%) ⬆️
6.1-m6i.metal 81.70% <83.33%> (+<0.01%) ⬆️

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

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

@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from d64935c to 0ec2fad Compare November 9, 2023 10:27
@kalyazin kalyazin mentioned this pull request Nov 9, 2023
Merged
6 tasks
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch 7 times, most recently from da72e7b to 234163c Compare November 10, 2023 16:11
@kalyazin
Copy link
Contributor

I gave it a manual test. It appears to work if I do 2 things:

@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch 5 times, most recently from 7a0a436 to 4246a67 Compare November 13, 2023 17:14
@wearyzen wearyzen mentioned this pull request Nov 17, 2023
Merged
4 tasks
@kalyazin kalyazin mentioned this pull request Nov 17, 2023
Closed
9 tasks
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch 3 times, most recently from 88c361a to 5bc7a4b Compare November 20, 2023 16:50
@ShadowCurse ShadowCurse marked this pull request as ready for review November 20, 2023 16:51
@ShadowCurse ShadowCurse added Type: Enhancement Indicates new feature requests Status: Awaiting review Indicates that a pull request is ready to be reviewed labels Nov 20, 2023
kalyazin
kalyazin reviewed Nov 22, 2023
View reviewed changes
src/vmm/src/devices/virtio/mmio.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/mmio.rs Show resolved Hide resolved
src/vmm/src/rpc_interface.rs Show resolved Hide resolved
src/api_server/src/request/drive.rs Show resolved Hide resolved
tests/integration_tests/functional/test_api.py Show resolved Hide resolved
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from d9ccfe4 to b115336 Compare November 23, 2023 09:27
@kalyazin kalyazin mentioned this pull request Nov 23, 2023
Closed
5 tasks
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch 2 times, most recently from a605fa7 to 44c7248 Compare November 23, 2023 10:16
bchalios
bchalios previously approved these changes Nov 23, 2023
View reviewed changes
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from 44c7248 to 092e600 Compare November 23, 2023 10:57
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from 092e600 to 95b1209 Compare November 23, 2023 11:35
@ShadowCurse
feat(vhost-user): vhost-user devices can serve config irqs
9643846 
Updated logic to serve interrupt status to the guest.
Now vhost-user devices can serve config updates irqs
to the guest.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
feat(vhost-user-blk): added config_update
6e15886 
Added new method `config_update` that will
fetch new config from the backend and notify
guest about the config change.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch 3 times, most recently from b12a5a9 to ed58a21 Compare November 23, 2023 15:59
ShadowCurse and others added 6 commits November 23, 2023 16:12
@ShadowCurse
feat(vhost-user-blk): added ability to trigger config change
56d2037 
Now we can trigger vhost-user-block device config update.
This is done by issuing the same `PATCH` command to the
`/drive` API endpoint without specifying any virtio-block
specific parameters.
Added tests to verify that any message is triggering the
correct behaviour.
Did minor change in the error message for block patching
to make it more clear for the users.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
chore(seccomp): added sendmsg and recvmsg to vmm thread
c963e14 
Because now `vmm` thread can update vhost-user-block config
it needs `sendmsg` and `recvmsg` to talk to the backend.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse @kalyazin
test(framework): allow patch drive without optional args
9139fec 
This is to allow PATCH /drives for vhost-usr-blk.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Co-authored-by: Nikita Kalyazin <kalyazin@amazon.com>
@ShadowCurse @kalyazin
test(vhost-user-blk): add test for device resize
34849fc 
The test issues a PATCH /drives request
to a vhost-user-blk drive and expects for the guest
to observe the new device size.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Co-authored-by: Nikita Kalyazin <kalyazin@amazon.com>
@ShadowCurse
feat(vhost-user): added config_change_time_us to the metrics
1a74a1f 
Added `config_change_time_us` to the metrics for vhost-user devices.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
feat(vhost-user-blk): added config_change_time_us metric
7e12baa 
Added `config_change_time_us` metric to the vhost-user-block device.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from ed58a21 to 7e12baa Compare November 23, 2023 16:13
@ShadowCurse ShadowCurse merged commit 4b06cc8 into firecracker-microvm:main Nov 23, 2023
7 checks passed
@ShadowCurse ShadowCurse deleted the vhost_user_blk_basic_config_change branch November 23, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bchalios bchalios bchalios approved these changes

@kalyazin kalyazin kalyazin approved these changes

@xmarcalx xmarcalx Awaiting requested review from xmarcalx

@pb8o pb8o Awaiting requested review from pb8o

@wearyzen wearyzen Awaiting requested review from wearyzen

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.
3 participants
@ShadowCurse @kalyazin @bchalios