Trigger block device rescan via PUT /actions #430
Conversation
vmm/Cargo.toml
Outdated
| @@ -7,6 +7,7 @@ authors = ["Amazon firecracker team <[email protected]>"] | |||
| libc = ">=0.2.39" | |||
| epoll = "=2.1.0" | |||
| scopeguard = "=0.3.3" | |||
| serde_json = "=1.0.9" | |||
api_server/src/request/async/mod.rs
Outdated
| device_type: DeviceType, | ||
| device_resource_id: String, | ||
| force: bool, | ||
| pub device_type: DeviceType, |
There was a problem hiding this comment.
Why do you need the fields to be public?
There was a problem hiding this comment.
For easy instantiation and comparison (inside tests). It would be cumbersome to add getters, setters and builders in the test section.
vmm/src/lib.rs
Outdated
| @@ -618,6 +615,16 @@ impl Vmm { | |||
| self.network_interface_configs.put(body) | |||
| } | |||
|
|
|||
| fn rescan_device(&mut self, body: RequestBody) -> result::Result<SyncOkStatus, SyncError> { | |||
There was a problem hiding this comment.
It is more of a nit, but I don't think you still need both the rescan and rescan_device functions. It adds a bit of confusion because it is not very clear what is the difference between them. For clarity, I would only keep handle_rescan_block_device, which would be the function handling the communication with the API thread and a function rescan_block_device that actually implements the VMM logic.
There was a problem hiding this comment.
Done. Only handle_rescan_block_device and rescan_block_device are left.
api_server/src/request/async/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_to_parsed_request() { |
There was a problem hiding this comment.
It's moved to actions.rs -> test_into_parsed_request because RequestBody no longer has a to_parsed_request method, but implements the IntoParsedRequest trait instead. Since actions::IntoParsedRequest can return either a SyncRequest or an AsyncRequest, it didn't make sense for the test to stay in the async module.
There was a problem hiding this comment.
Yes, I totally missed that, sorry.
api_server/src/request/actions.rs
Outdated
| #[derive(Clone, Debug, Deserialize, Serialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct RequestBody { | ||
| pub action_id: String, |
There was a problem hiding this comment.
The action_id can be optional as it will only be used for Async requests. When we change InstanceStart & InstanceHalt to also be sync requests, we can remove it altogether.
Making the action_id optional would require changes in into_parsed_request. We should unwrap the action_id only when parsing InstanceStart and the InstanceHalt.
api_server/src/request/actions.rs
Outdated
| // json body into this. | ||
| #[derive(Clone, Debug, Deserialize, Serialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct RequestBody { |
There was a problem hiding this comment.
ActionBody seems like a better name for this struct.
PUT Requests to /actions don't necessarily need to be async. Renaming to avoid confusion when the need for sync PUT /actions requests arises (soon). Moving structs related to /actions to a separate file. Preparing the code for the addition of sync requests to /actions. Signed-off-by: Alexandra Iordache <[email protected]>
Made action_id optional in the request body, as sync actions don't need an ID. Added a new action that triggers block device rescan. PUT /drives is no longer allowed after the guest boots. Signed-off-by: Alexandra Iordache <[email protected]>
Signed-off-by: Alexandra Iordache <[email protected]>
Signed-off-by: Alexandra Iordache <[email protected]>
Changes
AsyncRequesttoRequestand refactored*Requeststructs accordingly to facilitate sync requests to/actionsas well as async ones.PUT /actionsused to create an async request that had to be queried withGET.payloadfield to the/actionsresource. Future actions should use this to encapsulate the necessary information. For block device rescan,payloadcontains the block device's ID.PUT /drives.PUT /drivesnow returns403after guest boot.PUT /drivesis not allowed anymore and that the rescan works properly.Fixes #280.
Partially addresses #409 as it cleans up the logic for updating drive resources.
Testing
pytest.