Add vCPUs to VMM via new endpoint #4638

ghost · 2024-06-11T10:34:58Z

Changes

Created new API endpoint hotplug used to request resources on demand (currently only vCPU implemented)
Currently, when a valid request is made to hotplug, the specified number of vCPU threads are added directly to the guest VMM
For now, in order to allow hotplugging Firecracker must be launched with --no-seccomp option
Unit tests added to test functionality of both endpoint and adding of vCPUs

Reason

This is the first step in implementing resource hot plugging via ACPI. Related to feature request #2609.

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.

roypat

I think this looks very good already, nice job! I left some in-line comments about some specific part that can be improved. Some high level comments:

Every project has some slightly different rules and preferences about commit histories in pull requests, but in Firecracker we to prefer not have commits that fix/reformat code introduced in earlier commits. For example, the formatting bf738f5 should be squashed into the commit that introduces the code you're reformatting here. As a rule of thumb, whenever you're just fixing some CI failure, squash the change into the commit that introduced the issue you're fixing.

Beyond that, in this PR I would structure the commits as follows:

a commit that refactors the seccomp filter handling so that they are stored in Vmm
a commit that introduces the API endpoint, including a stub handler in rpc_interface.rs (if you want, you can include the unit tests in this one, or add a separate commit for them)
a commit that implements Vmm::hotplug_vcpus (with or without the unit tests, again up to you)

I don't really have any fixed rules as for how I came up with that, but generally I try to have my commits be a single logical unit of code that is still independently useful. That's why I would put the seccomp refactor into a commit separate from the hotplug_vcpus function (as they're two units of code to me, a refactor + a feature), but I would combine your first two commits that introduce the API endpoints in two steps (because having the endpoint without parsing the json would feel a bit too fine-grained to me). But it's not an exact science, so feel free to deviate from whatever I'm suggesting if you disagree with it!

Lastly, could you drop Pablo's commits from this PR? We can just rebase the feature branch onto main to pick them up :)

src/vmm/src/lib.rs

ghost · 2024-06-14T14:04:31Z

Thanks for the feedback, still trying to fix some issues Buildkite is picking up on but will work on this as soon as I'm done with it :)

codecov · 2024-06-15T17:21:27Z

Codecov Report

Attention: Patch coverage is 79.24528% with 22 lines in your changes missing coverage. Please review.

Project coverage is 82.09%. Comparing base (e3893d3) to head (31885a6).

Files	Patch %	Lines
src/vmm/src/rpc_interface.rs	4.76%	20 Missing ⚠️
src/vmm/src/vmm_config/hotplug.rs	50.00%	2 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/vcpu-hotplug    #4638      +/-   ##
========================================================
- Coverage                 82.10%   82.09%   -0.01%     
========================================================
  Files                       255      257       +2     
  Lines                     31280    31386     +106     
========================================================
+ Hits                      25681    25765      +84     
- Misses                     5599     5621      +22

Flag	Coverage Δ
4.14-c5n.metal	`79.59% <79.24%> (-0.01%)`	⬇️
4.14-c7g.metal	`?`
4.14-m5n.metal	`79.58% <79.24%> (-0.01%)`	⬇️
4.14-m6a.metal	`78.80% <79.24%> (-0.01%)`	⬇️
4.14-m6g.metal	`76.62% <76.00%> (-0.01%)`	⬇️
4.14-m6i.metal	`79.58% <79.24%> (-0.01%)`	⬇️
4.14-m7g.metal	`76.62% <76.00%> (-0.01%)`	⬇️
5.10-c5n.metal	`82.10% <79.24%> (-0.02%)`	⬇️
5.10-c7g.metal	`?`
5.10-m5n.metal	`82.08% <79.24%> (-0.02%)`	⬇️
5.10-m6a.metal	`81.39% <79.24%> (-0.02%)`	⬇️
5.10-m6g.metal	`79.39% <76.00%> (-0.01%)`	⬇️
5.10-m6i.metal	`82.08% <79.24%> (-0.02%)`	⬇️
5.10-m7g.metal	`79.39% <76.00%> (-0.01%)`	⬇️
6.1-c5n.metal	`82.10% <79.24%> (-0.02%)`	⬇️
6.1-c7g.metal	`?`
6.1-m5n.metal	`82.08% <79.24%> (-0.02%)`	⬇️
6.1-m6a.metal	`81.39% <79.24%> (-0.02%)`	⬇️
6.1-m6g.metal	`79.39% <76.00%> (-0.02%)`	⬇️
6.1-m6i.metal	`82.08% <79.24%> (-0.02%)`	⬇️
6.1-m7g.metal	`79.39% <76.00%> (-0.02%)`	⬇️

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.

roypat

Nice work! I've left some minor comments, mainly the one about testing that we talked about offline.

src/firecracker/src/api_server/request/hotplug.rs

tools/create_snapshot_artifact/main.py

src/vmm/src/builder.rs

src/vmm/src/lib.rs

src/vmm/src/rpc_interface.rs

src/vmm/src/vmm_config/hotplug.rs

src/firecracker/src/api_server/request/hotplug.rs

ShadowCurse

Overall LGTM

src/vmm/src/lib.rs

roypat

Looks good to me as well! There's no need to address the comments below, unless you happen to be revisiting the code anyway :)

Holding of on hitting the approve button until @bchalios also had a chance to look at it.

src/vmm/src/rpc_interface.rs

bchalios

Hey James, thanks for the PR and apologies for the delayed review. Overall, great work!

I 've left some comments inline. Some of them are open for discussion, e.g. the actual API we should use for this. These things can be changed in subsequent PRs. But I'd love to hear your thoughts.

Others, I 'd like them fixed in this PR. Mainly this includes:

Fix errors introduced by a commit in the commit that introduced them, rather than later on
I'd like as to avoid using as in favour of explicit into() or try_into().unwrap(), where applicable.

Also, could you please update the swagger file with the definition of the new API whichever we decide it is.

src/firecracker/src/api_server/parsed_request.rs

src/vmm/src/logger/metrics.rs

src/vmm/src/vmm_config/hotplug.rs

src/vmm/src/lib.rs

src/firecracker/swagger/firecracker.yaml

src/vmm/src/builder.rs

Create a new endpoint `hotplug` in `parsed_request.rs` and implement basic skeleton of endpoint implementation. Signed-off-by: James Curtis <jxcurtis@amazon.co.uk>

Add seccompiler filters as a field of the VMM. This is required to ensure that the newly created vCPU threads have the same filters as the currently existing vCPU threads Signed-off-by: James Curtis <jxcurtis@amazon.co.uk>

Actually add the vCPUS to the VMM via API PUT requests to hotplug endpoint. Must add at least 1 vCPU, and the total number of vCPUs after hotplug must not exceed MAX_SUPPORTED_VCPUS (currently 32). Signed-off-by: James Curtis <jxcurtis@amazon.co.uk>

roypat · 2024-06-27T17:02:24Z

🎉

ghost force-pushed the vcpu-hotplug branch from e1f8d0f to 91609e0 Compare June 11, 2024 10:37

ghost requested review from roypat and zulinx86 June 11, 2024 10:39

ghost force-pushed the vcpu-hotplug branch 4 times, most recently from d85f45c to bf738f5 Compare June 11, 2024 12:47

ghost marked this pull request as draft June 11, 2024 12:56

ghost force-pushed the vcpu-hotplug branch 5 times, most recently from 5c7dd7a to 55d7a01 Compare June 13, 2024 17:06

ghost marked this pull request as ready for review June 14, 2024 10:32

roypat reviewed Jun 14, 2024

View reviewed changes

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

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

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

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

ghost force-pushed the vcpu-hotplug branch 2 times, most recently from cec50cf to 861ad2c Compare June 15, 2024 17:10

ghost force-pushed the vcpu-hotplug branch 6 times, most recently from aea2486 to cc578f7 Compare June 18, 2024 08:42

ghost changed the title ~~Hotplug API Endpoint Skeleton~~ Add vCPUs to VMM via new endpoint Jun 18, 2024

ghost requested review from bchalios and removed request for zulinx86 June 18, 2024 09:24

ghost force-pushed the vcpu-hotplug branch from cc578f7 to 31ab5fe Compare June 18, 2024 10:15

roypat reviewed Jun 18, 2024

View reviewed changes

ghost force-pushed the vcpu-hotplug branch 4 times, most recently from d61a486 to 16ae684 Compare June 19, 2024 12:48

ghost mentioned this pull request Jun 19, 2024

Change seccomp_filters to be passed by value, rather than by reference. #4647

Open

ShadowCurse reviewed Jun 19, 2024

View reviewed changes

src/firecracker/src/api_server/request/hotplug.rs Outdated Show resolved Hide resolved

ghost force-pushed the vcpu-hotplug branch 2 times, most recently from f4f9577 to 10c8399 Compare June 20, 2024 08:53

ShadowCurse reviewed Jun 20, 2024

View reviewed changes

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

roypat reviewed Jun 20, 2024

View reviewed changes

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

src/vmm/src/rpc_interface.rs Outdated Show resolved Hide resolved

src/vmm/src/rpc_interface.rs Outdated Show resolved Hide resolved

roypat added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Jun 20, 2024

bchalios requested changes Jun 25, 2024

View reviewed changes

ghost force-pushed the vcpu-hotplug branch 5 times, most recently from 656c6cf to 188bd81 Compare June 26, 2024 11:29

bchalios requested changes Jun 26, 2024

View reviewed changes

ghost force-pushed the vcpu-hotplug branch 4 times, most recently from 732c718 to 5335ef9 Compare June 27, 2024 10:41

bchalios approved these changes Jun 27, 2024

View reviewed changes

roypat approved these changes Jun 27, 2024

View reviewed changes

JamesC1305 added 3 commits June 27, 2024 13:07

Add API endpoint for hotplugging

cd76b45

Create a new endpoint `hotplug` in `parsed_request.rs` and implement basic skeleton of endpoint implementation. Signed-off-by: James Curtis <jxcurtis@amazon.co.uk>

Add seccomp filters to VMM

2fcee6c

Add seccompiler filters as a field of the VMM. This is required to ensure that the newly created vCPU threads have the same filters as the currently existing vCPU threads Signed-off-by: James Curtis <jxcurtis@amazon.co.uk>

ghost force-pushed the vcpu-hotplug branch from 5335ef9 to 31885a6 Compare June 27, 2024 12:07

ShadowCurse merged commit a8819ed into firecracker-microvm:feature/vcpu-hotplug Jun 27, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vCPUs to VMM via new endpoint #4638

Add vCPUs to VMM via new endpoint #4638

ghost commented Jun 11, 2024 •

edited by ghost

Loading

roypat left a comment

ghost commented Jun 14, 2024

codecov bot commented Jun 15, 2024 •

edited

Loading

roypat left a comment

ShadowCurse left a comment

roypat left a comment

bchalios left a comment •

edited

Loading

roypat commented Jun 27, 2024

Add vCPUs to VMM via new endpoint #4638

Add vCPUs to VMM via new endpoint #4638

Conversation

ghost commented Jun 11, 2024 • edited by ghost Loading

Changes

Reason

License Acceptance

PR Checklist

roypat left a comment

Choose a reason for hiding this comment

ghost commented Jun 14, 2024

codecov bot commented Jun 15, 2024 • edited Loading

Codecov Report

roypat left a comment

Choose a reason for hiding this comment

ShadowCurse left a comment

Choose a reason for hiding this comment

roypat left a comment

Choose a reason for hiding this comment

bchalios left a comment • edited Loading

Choose a reason for hiding this comment

roypat commented Jun 27, 2024

ghost commented Jun 11, 2024 •

edited by ghost

Loading

codecov bot commented Jun 15, 2024 •

edited

Loading

bchalios left a comment •

edited

Loading