New API call: PATCH /network-interfaces

Added support for updating a network device, both pre- and post-boot.
This is implemented via a PATCH API request.
Currently, the only properties supported for update are the RX and TX
rate limiters.

Signed-off-by: Dan Horobeanu <[email protected]>

Author: dhrgit

api_server/src/http_service.rs

@@ -24,7 +24,7 @@ use vmm::vmm_config::drive::BlockDeviceConfig;
 use vmm::vmm_config::instance_info::InstanceInfo;
 use vmm::vmm_config::logger::LoggerConfig;
 use vmm::vmm_config::machine_config::VmConfig;
-use vmm::vmm_config::net::NetworkInterfaceConfig;
+use vmm::vmm_config::net::{NetworkInterfaceConfig, NetworkInterfaceUpdateConfig};
 #[cfg(feature = "vsock")]
 use vmm::vmm_config::vsock::VsockDeviceConfig;
 use vmm::VmmAction;
@@ -335,6 +335,20 @@ fn parse_netif_req<'a>(path: &'a str, method: Method, body: &Chunk) -> Result<'a
                     Error::Generic(StatusCode::BadRequest, s)
                 })?)
         }
+        1 if method == Method::Patch => {
+            METRICS.patch_api_requests.network_count.inc();
+
+            Ok(serde_json::from_slice::<NetworkInterfaceUpdateConfig>(body)
+                .map_err(|e| {
+                    METRICS.patch_api_requests.network_fails.inc();
+                    Error::SerdeJson(e)
+                })?
+                .into_parsed_request(Some(id_from_path.to_string()), method)
+                .map_err(|s| {
+                    METRICS.patch_api_requests.network_fails.inc();
+                    Error::Generic(StatusCode::BadRequest, s)
+                })?)
+        }
         _ => Err(Error::InvalidPathMethod(path, method)),
     }
 }
@@ -1207,10 +1221,10 @@ mod tests {
                 == Err(Error::SerdeJson(get_dummy_serde_error()))
         );
 
-        // Error Case: Invalid Path.
+        // Error Case: Invalid method.
         assert!(
-            parse_netif_req(path, Method::Patch, &body,)
-                == Err(Error::InvalidPathMethod(path, Method::Patch))
+            parse_netif_req(path, Method::Options, &body,)
+                == Err(Error::InvalidPathMethod(path, Method::Options))
         )
     }
 

api_server/src/request/net.rs

@@ -7,7 +7,7 @@ use futures::sync::oneshot;
 use hyper::Method;
 
 use request::{IntoParsedRequest, ParsedRequest};
-use vmm::vmm_config::net::NetworkInterfaceConfig;
+use vmm::vmm_config::net::{NetworkInterfaceConfig, NetworkInterfaceUpdateConfig};
 use vmm::VmmAction;
 
 impl IntoParsedRequest for NetworkInterfaceConfig {
@@ -31,6 +31,27 @@ impl IntoParsedRequest for NetworkInterfaceConfig {
     }
 }
 
+impl IntoParsedRequest for NetworkInterfaceUpdateConfig {
+    fn into_parsed_request(
+        self,
+        id_from_path: Option<String>,
+        _: Method,
+    ) -> result::Result<ParsedRequest, String> {
+        let id_from_path = id_from_path.unwrap_or(String::new());
+        if id_from_path != self.iface_id {
+            return Err(String::from(
+                "The id from the path does not match the id from the body!",
+            ));
+        }
+
+        let (sender, receiver) = oneshot::channel();
+        Ok(ParsedRequest::Sync(
+            VmmAction::UpdateNetworkInterface(self, sender),
+            receiver,
+        ))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     extern crate net_util;

api_server/swagger/firecracker.yaml

@@ -274,7 +274,6 @@ paths:
       summary: Creates a network interface.
       description:
         Creates new network interface with ID specified by iface_id path parameter.
-        Updating existing interfaces is currently not allowed.
       operationId: putGuestNetworkInterfaceByID
       parameters:
       - name: iface_id
@@ -299,6 +298,34 @@ paths:
           description: Internal server error
           schema:
             $ref: "#/definitions/Error"
+    patch:
+      summary: Updates the rate limiters applied to a network interface.
+      description:
+        Updates the rate limiters applied to a network interface.
+      operationId: patchGuestNetworkInterfaceByID
+      parameters:
+        - name: iface_id
+          in: path
+          description: The id of the guest network interface
+          required: true
+          type: string
+        - name: body
+          in: body
+          description: A subset of the guest network interface properties
+          required: true
+          schema:
+            $ref: "#/definitions/PartialNetworkInterface"
+      responses:
+        204:
+          description: Network interface updated
+        400:
+          description: Network interface cannot be updated due to bad input
+          schema:
+            $ref: "#/definitions/Error"
+        default:
+          description: Internal server error
+          schema:
+            $ref: "#/definitions/Error"
 
 definitions:
   BootSource:
@@ -501,6 +528,21 @@ definitions:
         type: string
         description: Host level path for the guest drive
 
+  PartialNetworkInterface:
+    type: object
+    description:
+      Defines a partial network interface structure, used to update the rate limiters
+      for that interface, after microvm start.
+    required:
+      - iface_id
+    properties:
+      iface_id:
+        type: string
+      rx_rate_limiter:
+        $ref: "#/definitions/RateLimiter"
+      tx_rate_limiter:
+        $ref: "#/definitions/RateLimiter"
+
   RateLimiter:
     type: object
     description:

devices/src/lib.rs

@@ -43,12 +43,12 @@ pub enum EpollHandlerPayload {
     DrivePayload(File),
     /// Used to mutate current RateLimiter settings. The buckets are rx_bytes, rx_ops,
     /// tx_bytes, and tx_ops, respectively.
-    PatchRateLimiters(
-        Option<TokenBucket>,
-        Option<TokenBucket>,
-        Option<TokenBucket>,
-        Option<TokenBucket>,
-    ),
+    NetRateLimiterPayload {
+        rx_bytes: Option<TokenBucket>,
+        rx_ops: Option<TokenBucket>,
+        tx_bytes: Option<TokenBucket>,
+        tx_ops: Option<TokenBucket>,
+    },
     /// Events that do not need a payload.
     Empty,
 }

devices/src/virtio/net.rs

@@ -59,7 +59,7 @@ pub const NET_EVENTS_COUNT: usize = 5;
 // This is not a true DeviceEvent, as we explicitly invoke the handler with this value as a
 // parameter when the VMM handles as PATCH rate limiters request. Thus, there's not epoll event
 // associated with it.
-const PATCH_RATE_LIMITERS: DeviceEventT = 5;
+pub const PATCH_RATE_LIMITERS_FAKE_EVENT: DeviceEventT = NET_EVENTS_COUNT as DeviceEventT;
 
 #[derive(Debug)]
 pub enum Error {
@@ -605,9 +605,13 @@ impl EpollHandler for NetEpollHandler {
                     }
                 }
             }
-            PATCH_RATE_LIMITERS => {
-                if let EpollHandlerPayload::PatchRateLimiters(rx_bytes, rx_ops, tx_bytes, tx_ops) =
-                    payload
+            PATCH_RATE_LIMITERS_FAKE_EVENT => {
+                if let EpollHandlerPayload::NetRateLimiterPayload {
+                    rx_bytes,
+                    rx_ops,
+                    tx_bytes,
+                    tx_ops,
+                } = payload
                 {
                     self.rx.rate_limiter.update_buckets(rx_bytes, rx_ops);
                     self.tx.rate_limiter.update_buckets(tx_bytes, tx_ops);
@@ -1941,14 +1945,14 @@ mod tests {
         let tx_ops = TokenBucket::new(1009, Some(1010), 1011);
 
         h.handle_event(
-            PATCH_RATE_LIMITERS,
+            PATCH_RATE_LIMITERS_FAKE_EVENT,
             0,
-            EpollHandlerPayload::PatchRateLimiters(
-                Some(rx_bytes.clone()),
-                Some(rx_ops.clone()),
-                Some(tx_bytes.clone()),
-                Some(tx_ops.clone()),
-            ),
+            EpollHandlerPayload::NetRateLimiterPayload {
+                rx_bytes: Some(rx_bytes.clone()),
+                rx_ops: Some(rx_ops.clone()),
+                tx_bytes: Some(tx_bytes.clone()),
+                tx_ops: Some(tx_ops.clone()),
+            },
         )
         .unwrap();
 

logger/src/metrics.rs

@@ -184,6 +184,10 @@ pub struct PatchRequestsMetrics {
     pub drive_count: SharedMetric,
     /// Number of failures in PATCHing a block device.
     pub drive_fails: SharedMetric,
+    /// Number of tries to PATCH a net device.
+    pub network_count: SharedMetric,
+    /// Number of failures in PATCHing a net device.
+    pub network_fails: SharedMetric,
 }
 
 /// Block Device associated metrics.

rate_limiter/src/lib.rs

@@ -93,6 +93,7 @@ fn gcd(x: u64, y: u64) -> u64 {
 pub struct TokenBucket {
     // Bucket defining traits.
     size: u64,
+    // Initial burst size (number of free initial tokens, that can be consumed at no cost)
     one_time_burst: Option<u64>,
     // Complete refill time in milliseconds.
     refill_time: u64,
@@ -410,13 +411,12 @@ impl RateLimiter {
             ops_complete_refill_time_ms,
         );
 
-        // If limiting is disabled on all token types, don't even create a timer fd.
-        let timer_fd = if bytes_token_bucket.is_some() || ops_token_bucket.is_some() {
-            // create TimerFd using monotonic clock, as nonblocking FD and set close-on-exec
-            Some(TimerFd::new_custom(ClockId::Monotonic, true, true)?)
-        } else {
-            None
-        };
+        // TODO: Self::timer_fd should not be an `Option` anymore; clean that up.
+        //
+        // We'll need a timer_fd, even if our current config effectively disables rate limiting,
+        // because `Self::update_buckets()` might re-enable it later, and we might be
+        // seccomp-blocked from creating the timer_fd at that time.
+        let timer_fd = Some(TimerFd::new_custom(ClockId::Monotonic, true, true)?);
 
         Ok(RateLimiter {
             bandwidth: bytes_token_bucket,
@@ -509,7 +509,7 @@ impl RateLimiter {
     }
 
     /// Updates the parameters of the token buckets associated with this RateLimiter.
-    // TODO: Pls note that, right now, the buckets buckets become full after being updated.
+    // TODO: Pls note that, right now, the buckets become full after being updated.
     pub fn update_buckets(&mut self, bytes: Option<TokenBucket>, ops: Option<TokenBucket>) {
         // TODO: We have to call make_bucket instead of directly assigning the bytes and/or ops
         // because the input buckets are likely build via deserialization, which currently does not
@@ -673,8 +673,6 @@ mod tests {
             "SpuriousRateLimiterEvent(\
              \"Rate limiter event handler called without a present timer\")"
         );
-        // raw FD for this disabled rate-limiter should be -1
-        assert_eq!(l.as_raw_fd(), -1);
     }
 
     #[test]

tests/integration_tests/functional/test_api.py

@@ -477,14 +477,6 @@ def test_api_patch_pre_boot(test_microvm_with_api):
     assert test_microvm.api_session.is_status_bad_request(response.status_code)
     assert "Invalid request method" in response.text
 
-    # Partial updates to network interfaces are not allowed.
-    response = test_microvm.network.patch(
-        iface_id='1',
-        guest_mac='06:00:00:00:00:02'
-    )
-    assert test_microvm.api_session.is_status_bad_request(response.status_code)
-    assert "Invalid request method" in response.text
-
     # Partial updates to the logger configuration are not allowed.
     response = test_microvm.logger.patch(level='Error')
     assert test_microvm.api_session.is_status_bad_request(response.status_code)
@@ -547,14 +539,6 @@ def test_api_patch_post_boot(test_microvm_with_api):
     assert test_microvm.api_session.is_status_bad_request(response.status_code)
     assert "Invalid request method" in response.text
 
-    # Partial updates to network interfaces are not allowed.
-    response = test_microvm.network.patch(
-        iface_id='1',
-        guest_mac='06:00:00:00:00:02'
-    )
-    assert test_microvm.api_session.is_status_bad_request(response.status_code)
-    assert "Invalid request method" in response.text
-
     # Partial updates to the logger configuration are not allowed.
     response = test_microvm.logger.patch(level='Error')
     assert test_microvm.api_session.is_status_bad_request(response.status_code)