Skip to content

Get machine config V2 #248

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.

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

Conversation

andreeaflorescu
Copy link
Member

Changes

  1. Renamed data_model to memory_model because it contained memory related things (further refactoring is needed because that crate is not entirely used & could potentially be moved to sys_util)
  2. Added a data_model crate for storing structures used by both the API and the VMM. This crate was created as part of an effort to remove duplicated structures in Firecracker. Also take a look at BlockDeviceConfig vs DriveDescription.
  3. Added a trait for into_parsed_request. Due to the previous change where common structures are saved in the data_model, we cannot let the into_parsed_request implementation live as part of the struct implementation because you can't have an implementation of a struct in a different crate than the one that adds that structure. The trait is only implemented for MachineConfiguration because we are time bound at the time. A refactor for all of them should be expected after the next milestone.
  4. Added get for /machine-config

Testing Done

Build Time

cargo build
cargo build --release
sudo env "PATH=$PATH" cargo test --all => PASSED
cargo fmt --all => no errors

Integration Tests

Test1a get machine-config; Should output vcpu_count: 1, mem_size: 128
HTTP/1.1 200 OK
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{ "vcpu_count": "1", "mem_size_mib": "128" }

=======================================================
Test1 setting the vCPU to 2; Should output Updated
HTTP/1.1 204 No Content
Date: Thu, 03 May 2018 14:37:21 GMT

=======================================================
Test1a get machine-config; Should output vcpu_count: 2, mem_size: 128
HTTP/1.1 200 OK
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{ "vcpu_count": "2", "mem_size_mib": "128" }

=======================================================
Test1 setting the vCPU to 3; Should output Error (The cpuid is not an even number)
HTTP/1.1 204 No Content
Date: Thu, 03 May 2018 14:37:21 GMT

=======================================================
Test2 setting the vCPU to -1; Should output Bad Request
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{
"fault_message": "invalid value: integer -1, expected u8 at line 1 column 18"
}

=======================================================
Test3 setting the vCPU to string; Should output Bad Request
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{
"fault_message": "invalid type: string "str", expected u8 at line 1 column 21"
}

=======================================================
Test3a get machine-config; Should output vcpu_count: 3 (this should be 2 in the future), mem_size: 128
HTTP/1.1 200 OK
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{ "vcpu_count": "3", "mem_size_mib": "128" }

=======================================================
Test4 setting the memory size to 256; Should output Updated
HTTP/1.1 204 No Content
Date: Thu, 03 May 2018 14:37:21 GMT

=======================================================
Test5 setting the memory size to -1; Should output Bad Request
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{
"fault_message": "invalid value: integer -1, expected usize at line 1 column 20"
}

=======================================================
Test6 setting the memory size to string; Should output Bad Request
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{
"fault_message": "invalid type: string "str", expected usize at line 1 column 23"
}

=======================================================
Test7 send request with empty body; Should output Bad Request
HTTP/1.1 204 No Content
Date: Thu, 03 May 2018 14:37:21 GMT

=======================================================
Test8 send request with both memory and vcpu configuration; Should output Updated
HTTP/1.1 204 No Content
Date: Thu, 03 May 2018 14:37:21 GMT

=======================================================
Test9 send request with vCPU set to 0; Should output BadRequest
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{
"fault_message": "The vCPU number is invalid!"
}

=======================================================
Test9 send request with memory set to 0; Should output BadRequest
HTTP/1.1 400 Bad Request
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 03 May 2018 14:37:21 GMT

{
"fault_message": "The memory size (MiB) is invalid!"
}

@@ -0,0 +1,6 @@
[package]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a concern here that it might be confusing for open source developers to find a data_model crate in Firecracker and another one in crosvm, which Firecracker is loosely based upon, but which contains different stuff. It would be even more confusing if the contents of crosvm's data_model are moved in a differently named crate in Firecracker (like memory_model). I understand that memory_model is a good name for memory related structs, but I'd prefer to keep naming consistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this makes sense, I don't agree we should bend over backwards to keep as close to crossvm as possible.
I guess we could avoid renaming data_model->memory_model if we find an appropriate name for the data model stuff we want to put in data_model 😆
But personally, I'm fine with the change proposed here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to remove the memory_model crate all together. From that crate only the volatile_memory.rs and some definitions from mod.rs are actually used. The plan would be to moved those to a crate where they actually belong. Might be sys_util (as Diana suggested) or something else.

acatangiu
acatangiu previously approved these changes May 7, 2018

[dependencies]
serde = "=1.0.27"
serde_derive = "=1.0.27"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no newline, maybe add this too when you rebase

Signed-off-by: Andreea Florescu <[email protected]>
This is currently implemented only for MachineConfigureBody.

Signed-off-by: Andreea Florescu <[email protected]>
This module should contain in the future all the structures
that need to be shared between the API server and
VMM. For now only contains the vm configuration.

Signed-off-by: Andreea Florescu <[email protected]>
@andreeaflorescu andreeaflorescu force-pushed the get_machine_config_v2 branch from 5c2feea to fbe5245 Compare May 8, 2018 09:06
@andreeaflorescu andreeaflorescu merged commit 5867621 into firecracker-microvm:master May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants