Migrate Metrics and the Serial device to EventManager (polly) #1494

acatangiu · 2020-01-08T16:29:21Z

Reason for This PR

Another step in migrating Firecracker to the new polly event manager.

Description of Changes

Metrics

Both LOGGER and METRICS are Firecracker specific, not Vmm specific, and thus should not be tied to the Vmm.
Logger has already been taken out of Vmm, now take the metrics periodic logging logic out of the Vmm as well.

A PeriodicMetrics object is defined in the firecracker crate, which uses EventManager to drive periodic reporting of METRICS.

Serial

The serial device no longer has to be injected with input from some external component. It can now be created with any input object that is dyn std::io::Read + std::os::unix::io::AsRawFd.
Because the serial now implements the EventHandler trait, it can internally handle all input.

Removed the EpollDispatch::Stdin previously handled by the Vmm.

The builder can now build a Serial device with any input and/or output sources.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

acatangiu · 2020-01-17T14:23:51Z

@alexandruag @iulianbarbu @sandreim please review the integration of Metrics and Serial with the new event handling mechanism EventManager.

I still need to add tests before this PR is merge-able.

acatangiu · 2020-01-21T15:19:21Z

@alexandruag @aghecenco @iulianbarbu I've also added tests, this PR is now complete. Please review.

src/polly/src/pollable.rs

src/polly/src/event_manager.rs

Both LOGGER and METRICS are Firecracker specific, not Vmm specific, and thus should not be tied to the Vmm. Logger has already been taken out of Vmm, now take the metrics periodic logging logic out of the Vmm as well. A PeriodicMetrics object is defined in the firecracker crate, which uses polly to drive periodic reporting of METRICS. Signed-off-by: Adrian Catangiu <[email protected]>

sandreim · 2020-01-27T13:39:38Z

src/firecracker/src/api_server_adapter.rs

+    // Start the metrics.
+    firecracker_metrics
+        .lock()
+        .expect("Unlock failed.")


Nit: a more specific panic message would be nice.

sandreim · 2020-01-27T13:41:41Z

src/firecracker/src/metrics.rs

+        }
+
+        // Only used in tests, but has virtually no cost in production.
+        self.flush_counter += 1;


nit: #[cfg(test)]

error[E0658]: attributes on expressions are experimental

this is a simple counter with just increments on every flush (every 60s) so it's no runtimecost. Its purpose is to enable testing, without it we would have to drop the unittest..

You can wrap it in a code block instead:

#[cfg(test)] { self.flush_counter += 1; }

src/polly/src/event_manager.rs

sandreim · 2020-01-27T13:44:53Z

src/vmm/src/builder.rs

+    let interrupt_evt = EventFd::new(libc::EFD_NONBLOCK)
+        .map_err(Error::EventFd)
+        .map_err(StartMicrovmError::Internal)?;
+    let protected_serial = EventManager::wrap_handler(devices::legacy::Serial::new_in_out(


Nit: Unnecessary overhead in naming the serial as protected_serial, serial would do just fine.

alexandruag · 2020-01-27T13:52:52Z

src/polly/src/event_manager.rs

-        let wrapped_handler = Arc::new(Mutex::new(handler));
-        self.register_protected(wrapped_handler.clone())?;
-        Ok(wrapped_handler)
+    pub fn wrap_handler<T: EventHandler + 'static>(handler: T) -> Arc<Mutex<T>> {


Since this only appears to call Arc::new(Mutex::new(...)), is it really needed? It looks like invoking that sequence ourselves is also a bit shorter and clearer than writing EventManager::wrap_handler(...).

Signed-off-by: Adrian Catangiu <[email protected]>

The serial device no longer has to be injected with input from some external component. It can now be created with any input object that is 'dyn std::io::Read + std::os::unix::io::AsRawFd' and because the serial implements the EventHandler trait, it can internally handle all input. Removed the EpollDispatch::Stdin previously handled by the Vmm. The builder can build a Serial device with any input and/or output sources. Signed-off-by: Adrian Catangiu <[email protected]>

'register()' shouldn't take ownership of the handler because it could fail and on the Err path it would drop the handler. Instead of taking ownership and wrapping it then returning it in an Arc<Mutex>, just take an Arc<Mutex>. This way, the caller has maximum flexibility and can choose to provide a simple clone and doesn't lose the handler in an error case. Signed-off-by: Adrian Catangiu <[email protected]>

Remove the `RawIOHandler` trait exposed by `bus.rs`. This was previously used to get a handle on the Serial device to be able to externally inject raw input in it. With the new Serial design, that is no longer necessary, the Serial device itself can be built with any input/output sources and it will handle input and output on them internally. Signed-off-by: Adrian Catangiu <[email protected]>

Signed-off-by: Adrian Catangiu <[email protected]>

acatangiu self-assigned this Jan 8, 2020

acatangiu added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Jan 8, 2020

acatangiu force-pushed the metrics branch from d0d1a20 to 63033fd Compare January 10, 2020 11:07

acatangiu force-pushed the metrics branch from 63033fd to e834c13 Compare January 17, 2020 14:18

acatangiu changed the title ~~Metrics driven by polly outside of Vmm~~ Migrate Metrics and the Serial device to EventManager (polly) Jan 17, 2020

acatangiu force-pushed the metrics branch 3 times, most recently from 018d6d2 to e635475 Compare January 21, 2020 15:18

acatangiu added Codebase: Architecture labels Jan 21, 2020

alexandruag reviewed Jan 22, 2020

View reviewed changes

src/polly/src/pollable.rs Outdated Show resolved Hide resolved

sandreim previously approved these changes Jan 23, 2020

View reviewed changes

src/polly/src/event_manager.rs Outdated Show resolved Hide resolved

acatangiu dismissed sandreim’s stale review via 23b5f98 January 24, 2020 10:39

acatangiu force-pushed the metrics branch 2 times, most recently from 23b5f98 to 9a0e310 Compare January 24, 2020 11:07

acatangiu mentioned this pull request Jan 27, 2020

Migrate the Vmm-exit and Api events to EventManager (polly) #1550

Merged

acatangiu force-pushed the metrics branch from 9a0e310 to 23f4e42 Compare January 27, 2020 13:06

sandreim previously approved these changes Jan 27, 2020

View reviewed changes

alexandruag reviewed Jan 27, 2020

View reviewed changes

acatangiu added 5 commits January 27, 2020 16:50

add tests for the metrics driver

0aafafa

Signed-off-by: Adrian Catangiu <[email protected]>

serial: add tests for internal handling of input

5937dae

Signed-off-by: Adrian Catangiu <[email protected]>

acatangiu dismissed sandreim’s stale review via 5937dae January 27, 2020 15:17

acatangiu force-pushed the metrics branch from 23f4e42 to 5937dae Compare January 27, 2020 15:17

alexandruag approved these changes Jan 27, 2020

View reviewed changes

sandreim approved these changes Jan 27, 2020

View reviewed changes

acatangiu merged commit eaac3ce into firecracker-microvm:refactoring/epoll_handler Jan 27, 2020

This was referenced Jan 27, 2020

Migrate Metrics to using EventManager (polly) #1522

Closed

Migrate the Serial device to using EventManager (polly) #1523

Closed

acatangiu deleted the metrics branch February 17, 2020 09:40

acatangiu mentioned this pull request Feb 21, 2020

Merge 'refactoring/epoll_handler' into 'master'. #1618

Merged

8 tasks

acatangiu mentioned this pull request Jun 2, 2021

seccomp: remove timerfd_create from allow-list #2606

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrate Metrics and the Serial device to EventManager (polly) #1494

Migrate Metrics and the Serial device to EventManager (polly) #1494

Uh oh!

acatangiu commented Jan 8, 2020 •

edited

Loading

Uh oh!

acatangiu commented Jan 17, 2020 •

edited

Loading

Uh oh!

acatangiu commented Jan 21, 2020

Uh oh!

Uh oh!

Uh oh!

sandreim Jan 27, 2020

Uh oh!

acatangiu Jan 27, 2020

Uh oh!

sandreim Jan 27, 2020

Uh oh!

acatangiu Jan 27, 2020

Uh oh!

andreeaflorescu Jan 27, 2020

Uh oh!

Uh oh!

sandreim Jan 27, 2020

Uh oh!

acatangiu Jan 27, 2020

Uh oh!

alexandruag Jan 27, 2020

Uh oh!

acatangiu Jan 27, 2020

Uh oh!

Uh oh!

Migrate Metrics and the Serial device to EventManager (polly) #1494

Migrate Metrics and the Serial device to EventManager (polly) #1494

Uh oh!

Conversation

acatangiu commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for This PR

Description of Changes

Metrics

Serial

License Acceptance

Uh oh!

acatangiu commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acatangiu commented Jan 21, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acatangiu commented Jan 8, 2020 •

edited

Loading

acatangiu commented Jan 17, 2020 •

edited

Loading