Vhost user blk basic config change #4223

ShadowCurse · 2023-11-08T15:33:13Z

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.

codecov · 2023-11-08T15:37:58Z

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.

kalyazin · 2023-11-13T13:51:05Z

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

Remove the check that at least one of the optional PATCH arguments is provided: https://github.com/firecracker-microvm/firecracker/blob/main/src/api_server/src/request/drive.rs#L70
Remove the offset when requesting device config from the backend: https://github.com/firecracker-microvm/firecracker/blob/main/src/vmm/src/devices/virtio/vhost_user_block/device.rs#L182

src/vmm/src/lib.rs

src/vmm/src/devices/virtio/mmio.rs

src/vmm/src/rpc_interface.rs

src/api_server/src/request/drive.rs

tests/integration_tests/functional/test_api.py

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>

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>

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>

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>

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>

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>

Added `config_change_time_us` to the metrics for vhost-user devices. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Added `config_change_time_us` metric to the vhost-user-block device. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from b432231 to 8ed6269 Compare November 8, 2023 15:34

ShadowCurse self-assigned this Nov 8, 2023

ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from d64935c to 0ec2fad Compare November 9, 2023 10:27

kalyazin mentioned this pull request Nov 9, 2023

Vhost user blk metrics #4226

Merged

6 tasks

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

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

kalyazin reviewed Nov 17, 2023

View reviewed changes

src/vmm/src/lib.rs Show resolved Hide resolved

wearyzen mentioned this pull request Nov 17, 2023

test(metrics): collect Firecracker vhost-user metrics #4246

Merged

4 tasks

kalyazin mentioned this pull request Nov 17, 2023

[DRAFT] test(vhost-user-blk): add test for device resize #4249

Closed

9 tasks

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 marked this pull request as ready for review November 20, 2023 16:51

ShadowCurse requested review from xmarcalx, pb8o and wearyzen as code owners November 20, 2023 16:51

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

ShadowCurse requested a review from bchalios November 22, 2023 16:28

kalyazin reviewed Nov 22, 2023

View reviewed changes

ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from d9ccfe4 to b115336 Compare November 23, 2023 09:27

kalyazin mentioned this pull request Nov 23, 2023

[DRAFT] vhost-user-blk: initial implementation #4069

Closed

5 tasks

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 previously approved these changes Nov 23, 2023

View reviewed changes

ShadowCurse dismissed bchalios’s stale review via 092e600 November 23, 2023 10:57

ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from 44c7248 to 092e600 Compare November 23, 2023 10:57

ShadowCurse requested review from kalyazin and bchalios November 23, 2023 11:15

ShadowCurse force-pushed the vhost_user_blk_basic_config_change branch from 092e600 to 95b1209 Compare November 23, 2023 11:35

kalyazin reviewed Nov 23, 2023

View reviewed changes

tests/integration_tests/functional/test_api.py Outdated Show resolved Hide resolved

ShadowCurse added 2 commits November 23, 2023 15:06

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>

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 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

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>

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>

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>

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>

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 force-pushed the vhost_user_blk_basic_config_change branch from ed58a21 to 7e12baa Compare November 23, 2023 16:13

ShadowCurse requested a review from kalyazin November 23, 2023 16:37

bchalios approved these changes Nov 23, 2023

View reviewed changes

kalyazin approved these changes Nov 23, 2023

View reviewed changes

ShadowCurse merged commit 4b06cc8 into firecracker-microvm:main Nov 23, 2023
7 checks passed

ShadowCurse deleted the vhost_user_blk_basic_config_change branch November 23, 2023 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vhost user blk basic config change #4223

Vhost user blk basic config change #4223

ShadowCurse commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading

kalyazin commented Nov 13, 2023

Vhost user blk basic config change #4223

Vhost user blk basic config change #4223

Conversation

ShadowCurse commented Nov 8, 2023 • edited Loading

Changes

License Acceptance

PR Checklist

codecov bot commented Nov 8, 2023 • edited Loading

Codecov Report

kalyazin commented Nov 13, 2023

ShadowCurse commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading