Move data types used by the vm and api_server in a different crate #276

dianpopa · 2018-05-15T15:01:46Z

Changes

moved all sync types defined inside api_server crate to data_model crate (i.e. logger, boot source, drive description, network related)
some additional cleanup inside api_server

Testing

Build Time

Prerequisite

## add the necessary musl target to the active toolchain
rustup target add x86_64-unknown-linux-musl

In-tree tests

sudo env "PATH=$PATH" pytest  functional/test_api.py

Coverage

overall 72.8%
per files relevant to this PR:

File	Coverage %
data_model/src/vm/boot_source.rs	94.1%
data_model/src/vm/machine_config.rs	96.7%
data_model/src/device_config/drive.rs	97.8%
data_model/src/device_config/net.rs	98.0%
data_model/src/lib.rs	100%
data_model/src/vm/logger.rs	100%
data_model/src/device_config/mod.rs	100%

alxiord · 2018-05-15T15:37:06Z

api_server/src/request/sync/mod.rs

@@ -87,7 +69,6 @@ impl GenerateResponse for OkStatus {
 // Potential errors associated with sync requests.
 #[derive(Debug)]
 pub enum Error {
-    GuestCIDAlreadyInUse,


Clarification question: is this going away because a PUT to the same ID will result in an update / bad request (depending on whether or not the guest has booted)?

This error was only used for the vsock devices (CID = Context Identifier and is a vsock specific concept), so no need for it anymore.

alxiord · 2018-05-15T15:44:53Z

data_model/src/device_config/drive.rs

+    pub fn new(
+        drive_id: String,
+        path_on_host: String,
+        _is_attached: bool,


If this isn't used, why not remove it altogether?

From my comment on a different attached: The state of the device can be one of Attached, Detached and Detaching. We can't have the state as a boolean. I know we are not using it right now, but I wouldn't suggest removing it because we would break the compatibility with previous versions of Firecracker. The other option is to ask the clients if they are ok with that.

@andreeaflorescu pretty much concluded what I had in mind. We want to add more items to that enum and we will need it for the logic later on.

Let us not worry about this function since I will remove it once the vmm-no-api is out (which is gonna be pretty soon).

alxiord · 2018-05-15T15:56:23Z

vmm/src/device_config/drive.rs

+            block_devices_configs.contains_drive_id(String::from("2")),
+            false
+        );
+        delete_dummy_path(dummy_filename_1);


Test clash! :) I added the same test in #263 and removed it following feedback on it. To relay the feedback, there's too much test code for minor implementation details. The relevant assert!s can be moved to any other test.
As a side note, please clean up the temporary files before assert!s, if the test panics the files will remain on disk.

andreeaflorescu · 2018-05-16T09:36:21Z

data_model/src/vm/boot_source.rs

+}
+
+#[derive(Debug, Deserialize, PartialEq, Serialize)]
+pub struct BootSourceBody {


Since this will be used by both API and VMM, I think we should stick with BootSource, not BootSourceBody because this names implies it belongs to the api.

andreeaflorescu · 2018-05-16T09:41:15Z

data_model/src/vm/boot_source.rs

+}
+
+impl BootSourceBody {
+    pub fn new(boot_source_id: String) -> Self {


This seems to be used only in tests. Can we add a cfg test to it? It doesn't look like we would ever use the constructor for anything else.

andreeaflorescu · 2018-05-16T09:45:35Z

data_model/src/vm/boot_source.rs

+    }
+}
+
+pub enum PutBootSourceConfigError {


I looked at the logger definition in data_model and I think it makes more sense to have them all named just BootSourceError instead of PutBootSourceConfigError, just like you already did for the Logger.

andreeaflorescu · 2018-05-16T09:55:25Z

data_model/src/device_config/drive.rs

+    pub fn new(
+        drive_id: String,
+        path_on_host: String,
+        _is_attached: bool,


The state of the device can be one of Attached, Detached and Detaching. We can't have the state as a boolean. I know we are not using it right now, but I wouldn't suggest removing it because we would break the compatibility with previous versions of Firecracker. The other option is to ask the clients if they are ok with that.

Yes, I agree but I did it this way to avoid including the DeviceState enum here also, since there is no logic for the moment...We could add it when we actually need the members of the enum. What do you think?

andreeaflorescu · 2018-05-16T09:59:20Z

data_model/src/device_config/drive.rs

+        self.permissions == DrivePermissions::ro
+    }
+
+    pub fn new(


Do we actually need this function? It seem like all the struct fields are public, thus we can create the object directly. Also, this is only used in tests and with vmm-no-api, which we plan to remove anyway.

I will leave it there for the moment and I will remove it completely once the vmm-no-api is removed.

andreeaflorescu · 2018-05-16T10:02:48Z

data_model/src/device_config/net.rs

+// This struct represents the strongly typed equivalent of the json body from net iface
+// related requests.
+#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
+pub struct NetworkInterfaceBody {


Remove Body from the struct name.

While Body might not be the best choice in terms of naming, leaving this as just struct NetworkInterface feels a bit off in my opinion :-s

I think that NetworkInterfaceConfig is an appropriate name since most of the things we have in the data model are actually configs.

andreeaflorescu · 2018-05-16T10:04:48Z

data_model/src/device_config/net.rs

+}
+
+impl NetworkInterfaceBody {
+    pub fn new(


Also used only in tests and all fields are public.

The problem I see here is with the fields being public not with the presence of the new function. I suggest I add setters and getters for the ones that we actually set from outside. I think this is the correct way to do it. What do you think?

andreeaflorescu · 2018-05-16T10:55:10Z

data_model/src/vm/machine_config.rs

+
+    #[test]
+    fn test_machine_config_error() {
+        assert_eq!(


I think it makes more sense to test these errors as part of the vmm function put_virtual_machine_configuration. I would not add these here just for the coverage score.

alexandruag · 2018-05-16T09:42:09Z

data_model/src/vm/mod.rs

@@ -1,3 +1,6 @@
+pub mod boot_source;


If this doesn't complicate things too much, I would avoid both having pub modules, and also pub using them :-s I think it's cleaner if we just leave the modules as public, without also re-exporting, at the cost of having to be a bit more verbose when we import different things :-s

I disagree since everything in boot_source is called BootSourceThis and BootSourceThat.

alexandruag · 2018-05-16T10:10:20Z

api_server/src/http_service.rs

+        1 if method == Method::Put => {
+            let drive =
+                serde_json::from_slice::<BlockDeviceConfig>(body).map_err(Error::SerdeJson)?;
+            if id_from_path.unwrap() != drive.drive_id {


I think we should do this check inside into_parsed_request(), because it's likely we'll need it for other methods also (PATCH for example, if we do end up using it). This implies adding a new argument to into_parsed_request() which provides a reference to the id from the path.

I did it this way the first time which lead me to the discovery that it does not imply only that. It implies a lot of changes since basically I change the signature of a function inside a Trait that is implemented by all types.

alexandruag · 2018-05-16T11:26:28Z

data_model/src/device_config/net.rs

+// This struct represents the strongly typed equivalent of the json body from net iface
+// related requests.
+#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
+pub struct NetworkInterfaceBody {


While Body might not be the best choice in terms of naming, leaving this as just struct NetworkInterface feels a bit off in my opinion :-s

alexandruag · 2018-05-16T11:27:28Z

net_util/src/tap.rs

@@ -60,7 +60,7 @@ fn build_terminated_if_name(if_name: &str) -> Result<Vec<u8>> {
 }

 impl Tap {
-    pub fn open_named(if_name: &str) -> Result<Tap> {
+    pub fn open_named(if_name: &String) -> Result<Tap> {


Why did this change to &String instead of &str?

Signed-off-by: Diana Popa <[email protected]>

testing that specifying a different ids in the path and in the body triggers error. Signed-off-by: Diana Popa <[email protected]>

dianpopa · 2018-06-26T13:01:18Z

I will close this for now as files from this PR have been completely changed in the meantime and it would take less time recreating the changes than rebasing.

dianpopa requested a review from a team May 15, 2018 15:02

dianpopa self-assigned this May 15, 2018

alxiord reviewed May 15, 2018

View reviewed changes

andreeaflorescu reviewed May 16, 2018

View reviewed changes

alexandruag reviewed May 16, 2018

View reviewed changes

dianpopa added 7 commits June 18, 2018 03:55

refactoring: moving BootSourceBody to data model

f164b67

Signed-off-by: Diana Popa <[email protected]>

refactoring: moving DriveDescription to data model

6d4cdef

Signed-off-by: Diana Popa <[email protected]>

refactoring: moving NetworkInterfaceBody to data model

77f589e

Signed-off-by: Diana Popa <[email protected]>

refactoring: moving LoggerDescription to data model

614c30a

Signed-off-by: Diana Popa <[email protected]>

api_server: cleanup and additional unit tests

3a4a54b

Signed-off-by: Diana Popa <[email protected]>

data_model: add unit tests for machine configuration

ba34b3e

Signed-off-by: Diana Popa <[email protected]>

ci: adding integration tests for...

7e84470

testing that specifying a different ids in the path and in the body triggers error. Signed-off-by: Diana Popa <[email protected]>

dianpopa closed this Jun 26, 2018

dianpopa deleted the api_server_to_dm branch November 9, 2018 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move data types used by the vm and api_server in a different crate #276

Move data types used by the vm and api_server in a different crate #276

dianpopa commented May 15, 2018 •

edited

Loading

alxiord May 15, 2018

dianpopa May 16, 2018

alxiord May 15, 2018

andreeaflorescu May 16, 2018

dianpopa May 16, 2018

dianpopa May 16, 2018

alxiord May 15, 2018

andreeaflorescu May 16, 2018

andreeaflorescu May 16, 2018

andreeaflorescu May 16, 2018

andreeaflorescu May 16, 2018

dianpopa May 16, 2018

andreeaflorescu May 16, 2018

andreeaflorescu May 16, 2018

dianpopa May 16, 2018

andreeaflorescu May 16, 2018

alexandruag May 16, 2018

andreeaflorescu May 16, 2018

andreeaflorescu May 16, 2018

dianpopa May 16, 2018

andreeaflorescu May 16, 2018

alexandruag May 16, 2018

andreeaflorescu May 16, 2018

alexandruag May 16, 2018

dianpopa May 16, 2018

alexandruag May 16, 2018

alexandruag May 16, 2018

dianpopa commented Jun 26, 2018

Move data types used by the vm and api_server in a different crate #276

Move data types used by the vm and api_server in a different crate #276

Conversation

dianpopa commented May 15, 2018 • edited Loading

Changes

Testing

Build Time

Prerequisite

In-tree tests

Coverage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianpopa commented Jun 26, 2018

dianpopa commented May 15, 2018 •

edited

Loading