Remove RescanBlockDevice from the /actions API

The docs have been updated to describe how to use the existing PATCH /drives API call in order to perform a block device rescan. Signed-off-by: Ioana Chirca <[email protected]>
tidux · Jan 27, 2020 · 53cf1ba · 53cf1ba
1 parent fef791e
commit 53cf1ba
Show file tree

Hide file tree

Showing 13 changed files with 79 additions and 256 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,9 @@
 - Changed default API socket path to `/run/firecracker.socket`. This path
   also applies when running with the jailer.
 - Disabled KVM dirty page tracking by default.
+- Removed redundant RescanBlockDevice action from the /actions API.
+  The functionality is available through the PATCH /drives API.
+  See `docs/api_requests/patch-block.md`.
 
 ## [0.20.0]
 

diff --git a/docs/api_requests/actions.md b/docs/api_requests/actions.md
@@ -6,53 +6,6 @@ requests on the `/actions` resource.
 Details about the required fields can be found in the
 [swagger definition](../../src/api_server/swagger/firecracker.yaml).
 
-## BlockDeviceRescan
-
-The `BlockDeviceRescan` action is used to trigger a rescan of one of the
-microVM's attached block devices. Rescanning is necessary when the size of the
-block device's backing file (on the host) changes and the guest needs to
-refresh its internal data structures to pick up this change. This action is
-therefore only allowed after the guest has booted. Its payload is a string and
-represents the ID of the block device that needs to be rescanned, as it was
-specified in the `PUT /drives` call that attached it during microVM
-configuration. In order for rescanning to work properly, the block device must
-not be mounted in the guest at the time of the API call; otherwise, the call
-will silently fail - no error is returned from either the guest or the host,
-but the guest's internal data structures end up in an inconsistent state.
-
-### BlockDeviceRescan Example
-
-```bash
-# Create and set up a block device.
-touch ${ro_drive_path}
-
-curl --unix-socket ${socket} -i \
-     -X PUT "http://localhost/drives/scratch" \
-     -H "accept: application/json" \
-     -H "Content-Type: application/json" \
-     -d "{
-            \"drive_id\": \"scratch\",
-            \"path_on_host\": \"${ro_drive_path}\",
-            \"is_root_device\": false,
-            \"is_read_only\": true
-         }"
-
-# Finish configuring and start the microVM. Wait for the guest to boot.
-
-# Resize the block device's backing file and create a filesystem in it.
-truncate --size 100M ${ro_drive_path}
-mkfs.ext4 ${ro_drive_path}
-
-curl --unix-socket ${socket} -i \
-     -X PUT "http://localhost/actions" \
-     -H "accept: application/json" \
-     -H "Content-Type: application/json" \
-     -d "{
-            \"action_type\": \"BlockDeviceRescan\",
-            \"payload\": \"scratch\"
-         }"
-```
-
 ## InstanceStart
 
 The `InstanceStart` action powers on the microVM and starts the guest OS. It

diff --git a/docs/api_requests/patch-block.md b/docs/api_requests/patch-block.md
@@ -0,0 +1,57 @@
+# Updating a Block Device
+
+Attached block devices require a PATCH /drives API call when the backing
+file's path or size changes, otherwise Firecracker and the running guest will
+not be notified of the changes.
+
+Is is important to note that the block device should not be mounted by the
+guest at the time of the API call, else the call will silently fail -
+no error is returned from either the guest or the host, but the guest might end
+up in an inconsistent state.
+
+## Example
+
+```bash
+# Create and set up a block device.
+touch ${ro_drive_path}
+
+curl --unix-socket ${socket} -i \
+     -X PUT "http://localhost/drives/scratch" \
+     -H "accept: application/json" \
+     -H "Content-Type: application/json" \
+     -d "{
+            \"drive_id\": \"scratch\",
+            \"path_on_host\": \"${ro_drive_path}\",
+            \"is_root_device\": false,
+            \"is_read_only\": true
+         }"
+
+# Finish configuring and start the microVM. Wait for the guest to boot.
+
+# Resize the block device's backing file and create a filesystem in it.
+truncate --size 100M ${drive_path}
+mkfs.ext4 ${ro_drive_path}
+
+# Even though the path has not changed, this triggers a device rescan.
+curl --unix-socket ${socket} -i \
+     -X PATCH "http://localhost/drives/scratch" \
+     -H "accept: application/json" \
+     -H "Content-Type: application/json" \
+     -d "{
+            \"drive_id\": \"scratch\",
+            \"path_on_host\": \"${ro_drive_path}\"
+         }"
+
+# Move the backing file.
+mv ${ro_drive_path} ${new_ro_drive_path}
+
+# Notify the guest that the path has changed.
+curl --unix-socket ${socket} -i \
+     -X PATCH "http://localhost/drives/scratch" \
+     -H "accept: application/json" \
+     -H "Content-Type: application/json" \
+     -d "{
+            \"drive_id\": \"scratch\",
+            \"path_on_host\": \"${new_ro_drive_path}\"
+         }"
+```
diff --git a/src/api_server/src/lib.rs b/src/api_server/src/lib.rs
@@ -65,10 +65,6 @@ pub enum VmmAction {
     /// `VsockDeviceConfig` as input. This action can only be called before the microVM has
     /// booted.
     SetVsockDevice(VsockDeviceConfig),
-    /// Update the size of an existing block device specified by an ID. The ID is the first data
-    /// associated with this enum variant. This action can only be called after the microVM is
-    /// started.
-    RescanBlockDevice(String),
     /// Set the microVM configuration (memory & vcpu) using `VmConfig` as input. This
     /// action can only be called before the microVM has booted.
     SetVmConfiguration(VmConfig),

diff --git a/src/api_server/src/parsed_request.rs b/src/api_server/src/parsed_request.rs
@@ -521,9 +521,8 @@ mod tests {
             .write_all(
                 b"PUT /actions HTTP/1.1\r\n\
                 Content-Type: application/json\r\n\
-                Content-Length: 59\r\n\r\n{ \
-                \"action_type\": \"BlockDeviceRescan\", \
-                \"payload\": \"string\" \
+                Content-Length: 33\r\n\r\n{ \
+                \"action_type\": \"FlushMetrics\" \
                 }",
             )
             .unwrap();

diff --git a/src/api_server/src/request/actions.rs b/src/api_server/src/request/actions.rs
@@ -1,18 +1,17 @@
 // Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use serde_json::Value;
-
 use super::super::VmmAction;
 use logger::{Metric, METRICS};
-use request::{Body, Error, ParsedRequest, StatusCode};
+#[cfg(target_arch = "aarch64")]
+use request::StatusCode;
+use request::{Body, Error, ParsedRequest};
 
 // The names of the members from this enum must precisely correspond (as a string) to the possible
 // values of "action_type" from the json request body. This is useful to get a strongly typed
 // struct from the Serde deserialization process.
 #[derive(Debug, Deserialize, Serialize)]
 enum ActionType {
-    BlockDeviceRescan,
     FlushMetrics,
     InstanceStart,
     SendCtrlAltDel,
@@ -24,42 +23,6 @@ enum ActionType {
 #[serde(deny_unknown_fields)]
 struct ActionBody {
     action_type: ActionType,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    payload: Option<Value>,
-}
-
-fn validate_payload(action_body: &ActionBody) -> Result<(), Error> {
-    match action_body.action_type {
-        ActionType::BlockDeviceRescan => {
-            match action_body.payload {
-                Some(ref payload) => {
-                    // Expecting to have drive_id as a String in the payload.
-                    if !payload.is_string() {
-                        return Err(Error::Generic(
-                            StatusCode::BadRequest,
-                            "Invalid payload type. Expected a string representing the drive_id"
-                                .to_string(),
-                        ));
-                    }
-                    Ok(())
-                }
-                None => Err(Error::Generic(
-                    StatusCode::BadRequest,
-                    "Payload is required for block device rescan.".to_string(),
-                )),
-            }
-        }
-        ActionType::FlushMetrics | ActionType::InstanceStart | ActionType::SendCtrlAltDel => {
-            // Neither FlushMetrics nor InstanceStart should have a payload.
-            if action_body.payload.is_some() {
-                return Err(Error::Generic(
-                    StatusCode::BadRequest,
-                    format!("{:?} does not support a payload.", action_body.action_type),
-                ));
-            }
-            Ok(())
-        }
-    }
 }
 
 pub fn parse_put_actions(body: &Body) -> Result<ParsedRequest, Error> {
@@ -69,15 +32,7 @@ pub fn parse_put_actions(body: &Body) -> Result<ParsedRequest, Error> {
         Error::SerdeJson(e)
     })?;
 
-    validate_payload(&action_body)?;
     match action_body.action_type {
-        ActionType::BlockDeviceRescan => {
-            // Safe to unwrap because we validated the payload in the validate_payload func.
-            let block_device_id = action_body.payload.unwrap().as_str().unwrap().to_string();
-            Ok(ParsedRequest::Sync(VmmAction::RescanBlockDevice(
-                block_device_id,
-            )))
-        }
         ActionType::FlushMetrics => Ok(ParsedRequest::Sync(VmmAction::FlushMetrics)),
         ActionType::InstanceStart => Ok(ParsedRequest::Sync(VmmAction::StartMicroVm)),
         ActionType::SendCtrlAltDel => {
@@ -98,85 +53,11 @@ pub fn parse_put_actions(body: &Body) -> Result<ParsedRequest, Error> {
 mod tests {
     use super::*;
 
-    #[test]
-    fn test_validate_payload() {
-        // Test InstanceStart.
-        let action_body = ActionBody {
-            action_type: ActionType::InstanceStart,
-            payload: None,
-        };
-        assert!(validate_payload(&action_body).is_ok());
-        // Error case: InstanceStart with payload.
-        let action_body = ActionBody {
-            action_type: ActionType::InstanceStart,
-            payload: Some(Value::String("dummy-payload".to_string())),
-        };
-        assert!(validate_payload(&action_body).is_err());
-
-        // Test BlockDeviceRescan
-        let action_body = ActionBody {
-            action_type: ActionType::BlockDeviceRescan,
-            payload: Some(Value::String(String::from("dummy_id"))),
-        };
-        assert!(validate_payload(&action_body).is_ok());
-        // Error case: no payload.
-        let action_body = ActionBody {
-            action_type: ActionType::BlockDeviceRescan,
-            payload: None,
-        };
-        assert!(validate_payload(&action_body).is_err());
-        // Error case: payload is not String.
-        let action_body = ActionBody {
-            action_type: ActionType::BlockDeviceRescan,
-            payload: Some(Value::Bool(false)),
-        };
-        assert!(validate_payload(&action_body).is_err());
-
-        // Test FlushMetrics.
-        let action_body = ActionBody {
-            action_type: ActionType::FlushMetrics,
-            payload: None,
-        };
-
-        assert!(validate_payload(&action_body).is_ok());
-        // Error case: FlushMetrics with payload.
-        let action_body = ActionBody {
-            action_type: ActionType::FlushMetrics,
-            payload: Some(Value::String("metrics-payload".to_string())),
-        };
-        let res = validate_payload(&action_body);
-        assert!(res.is_err());
-
-        // Test SendCtrlAltDel.
-        let action_body = ActionBody {
-            action_type: ActionType::SendCtrlAltDel,
-            payload: None,
-        };
-        assert!(validate_payload(&action_body).is_ok());
-        // Error case: SendCtrlAltDel with payload.
-        let action_body = ActionBody {
-            action_type: ActionType::SendCtrlAltDel,
-            payload: Some(Value::String("dummy-payload".to_string())),
-        };
-        assert!(validate_payload(&action_body).is_err());
-    }
-
     #[test]
     fn test_parse_put_actions_request() {
         {
             assert!(parse_put_actions(&Body::new("invalid_body")).is_err());
 
-            let json = r#"{
-                "action_type": "BlockDeviceRescan",
-                "payload": "dummy_id"
-              }"#;
-            let req = ParsedRequest::Sync(VmmAction::RescanBlockDevice("dummy_id".to_string()));
-            let result = parse_put_actions(&Body::new(json));
-            assert!(result.is_ok());
-            assert!(result.unwrap().eq(&req));
-        }
-
-        {
             let json = r#"{
                 "action_type": "InstanceStart"
             }"#;
@@ -218,13 +99,6 @@ mod tests {
             let result = parse_put_actions(&Body::new(json));
             assert!(result.is_ok());
             assert!(result.unwrap().eq(&req));
-
-            let json = r#"{
-                "action_type": "FlushMetrics",
-                "payload": "metrics-payload"
-            }"#;
-            let result = parse_put_actions(&Body::new(json));
-            assert!(result.is_err());
         }
     }
 }
diff --git a/src/api_server/swagger/firecracker.yaml b/src/api_server/swagger/firecracker.yaml
@@ -452,12 +452,9 @@ definitions:
         description: Enumeration indicating what type of action is contained in the payload
         type: string
         enum:
-        - BlockDeviceRescan
         - FlushMetrics
         - InstanceStart
         - SendCtrlAltDel
-      payload:
-        type: string
 
   InstanceInfo:
     type: object

diff --git a/src/devices/src/virtio/block.rs b/src/devices/src/virtio/block.rs
@@ -483,6 +483,15 @@ pub fn build_config_space(disk_size: u64) -> Vec<u8> {
     // We only support disk size, which uses the first two words of the configuration space.
     // If the image is not a multiple of the sector size, the tail bits are not exposed.
     // The config space is little endian.
+
+    if disk_size % SECTOR_SIZE != 0 {
+        warn!(
+            "Disk size {} is not a multiple of sector size {}; \
+             the remainder will not be visible to the guest.",
+            disk_size, SECTOR_SIZE
+        );
+    }
+
     let mut config = Vec::with_capacity(CONFIG_SPACE_SIZE);
     let num_sectors = disk_size >> SECTOR_SHIFT;
     for i in 0..8 {
@@ -502,13 +511,6 @@ impl Block {
         rate_limiter: Option<RateLimiter>,
     ) -> io::Result<Block> {
         let disk_size = disk_image.seek(SeekFrom::End(0))? as u64;
-        if disk_size % SECTOR_SIZE != 0 {
-            warn!(
-                "Disk size {} is not a multiple of sector size {}; \
-                 the remainder will not be visible to the guest.",
-                disk_size, SECTOR_SIZE
-            );
-        }
 
         let mut avail_features = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_BLK_F_FLUSH);
 

diff --git a/src/firecracker/src/main.rs b/src/firecracker/src/main.rs
@@ -365,9 +365,6 @@ fn vmm_control_event(
                 SetVsockDevice(vsock_cfg) => vmm
                     .set_vsock_device(vsock_cfg)
                     .map(|_| api_server::VmmData::Empty),
-                RescanBlockDevice(drive_id) => vmm
-                    .rescan_block_device(&drive_id)
-                    .map(|_| api_server::VmmData::Empty),
                 StartMicroVm => vmm
                     .start_microvm(
                         vmm_seccomp_filter.to_owned(),

diff --git a/src/vmm/src/error.rs b/src/vmm/src/error.rs
@@ -349,7 +349,7 @@ pub enum VmmActionError {
     /// The action `ConfigureBootSource` failed either because of bad user input (`ErrorKind::User`)
     /// or an internal error (`ErrorKind::Internal`).
     BootSource(ErrorKind, BootSourceConfigError),
-    /// One of the actions `InsertBlockDevice`, `RescanBlockDevice` or `UpdateBlockDevicePath`
+    /// One of the actions `InsertBlockDevice` or `UpdateBlockDevicePath`
     /// failed either because of bad user input (`ErrorKind::User`) or an
     /// internal error (`ErrorKind::Internal`).
     DriveConfig(ErrorKind, DriveError),