-
Notifications
You must be signed in to change notification settings - Fork 208
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
refact: use mio for daemon exit signal monitoring #340
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,12 +25,13 @@ use std::sync::{ | |
mpsc::channel, | ||
Arc, Mutex, | ||
}; | ||
use std::thread; | ||
use std::thread::{self, spawn}; | ||
use std::{io, process}; | ||
|
||
use clap::{App, Arg}; | ||
use event_manager::{EventManager, EventSubscriber, SubscriberOps}; | ||
use fuse_backend_rs::api::{Vfs, VfsOptions}; | ||
use mio::Waker; | ||
use nix::sys::signal; | ||
use rlimit::{rlim, Resource}; | ||
use vmm_sys_util::eventfd::EventFd; | ||
|
@@ -57,7 +58,7 @@ mod upgrade; | |
|
||
lazy_static! { | ||
static ref EVENT_MANAGER_RUN: AtomicBool = AtomicBool::new(true); | ||
static ref EXIT_EVTFD: Mutex::<Option<EventFd>> = Mutex::<Option<EventFd>>::default(); | ||
static ref EXIT_NOTIFIER: Mutex<Option<Arc<Waker>>> = Mutex::default(); | ||
} | ||
|
||
fn get_default_rlimit_nofile() -> Result<rlim> { | ||
|
@@ -91,12 +92,12 @@ fn get_default_rlimit_nofile() -> Result<rlim> { | |
} | ||
|
||
pub fn exit_event_manager() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd better use as few global variables/functions as possible, how about wrapping an exit manager as a struct implementation, and putting it to FuseDaemon/VirtiofsDaemon? (This is not a blocker for this PR at all) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Absolutely right. Event manager maybe removed, together with global shared memory - EXIT_MANAGER_RUN in later PR. |
||
EXIT_EVTFD | ||
EXIT_NOTIFIER | ||
.lock() | ||
.expect("Not poisoned lock!") | ||
.as_ref() | ||
.unwrap() | ||
.write(1) | ||
.wake() | ||
.unwrap_or_else(|e| error!("Write event fd failed when exiting event manager, {}", e)) | ||
} | ||
|
||
|
@@ -375,10 +376,6 @@ fn main() -> Result<()> { | |
let vfs = Vfs::new(opts); | ||
|
||
let mut event_manager = EventManager::<Arc<dyn EventSubscriber>>::new().unwrap(); | ||
let daemon_subscriber = Arc::new(NydusDaemonSubscriber::new()?); | ||
// Send an event to exit from Event Manager so as to exit from nydusd | ||
let exit_evtfd = daemon_subscriber.get_event_fd()?; | ||
event_manager.add_subscriber(daemon_subscriber); | ||
|
||
let vfs = Arc::new(vfs); | ||
// Basically, below two arguments are essential for live-upgrade/failover/ and external management. | ||
|
@@ -462,7 +459,7 @@ fn main() -> Result<()> { | |
info!("api server running at {}", apisock); | ||
} | ||
|
||
*EXIT_EVTFD.lock().unwrap().deref_mut() = Some(exit_evtfd); | ||
start_daemon_monitor().unwrap(); | ||
nydus_app::signal::register_signal_handler(signal::SIGINT, sig_exit); | ||
nydus_app::signal::register_signal_handler(signal::SIGTERM, sig_exit); | ||
|
||
|
@@ -491,3 +488,14 @@ fn main() -> Result<()> { | |
|
||
Ok(()) | ||
} | ||
|
||
fn start_daemon_monitor() -> Result<()> { | ||
let mut monitor = NydusDaemonSubscriber::new()?; | ||
*EXIT_NOTIFIER.lock().unwrap().deref_mut() = Some(monitor.get_notifier()); | ||
|
||
spawn(move || { | ||
monitor.listen(); | ||
}); | ||
|
||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these still needed after switch to mio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"spawn" is still needed for starting monitor so far(line 496).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The http thread is still using eventfd to exit itself. Are you planning to switch it (via the new micro_http crate) in a different PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In a later PR.