From ebbb1ab93543a1c7599a642e0e84118fd8b0c8c7 Mon Sep 17 00:00:00 2001 From: Jamie Winsor Date: Sun, 24 Sep 2017 17:02:14 -0700 Subject: [PATCH 1/2] Fix race condition in loading/unloading services Move spec migration to Manager load before SpecWatcher is started to reduce IO calls for reading/writing spec files. This appears to fix an issue with loading and unloading services if the Supervisor is running Signed-off-by: Jamie Winsor --- components/sup/src/manager/mod.rs | 36 ++++++++++++++++++++++ components/sup/src/manager/spec_watcher.rs | 34 ++++++++++---------- 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/components/sup/src/manager/mod.rs b/components/sup/src/manager/mod.rs index c8f452c5e3..3ae9bc3e65 100644 --- a/components/sup/src/manager/mod.rs +++ b/components/sup/src/manager/mod.rs @@ -212,6 +212,41 @@ impl Manager { } } + /// Read all spec files and rewrite them to disk migrating their format from a previous + /// Supervisor's to the one currently running. + fn migrate_specs(fs_cfg: &FsCfg) { + // JW: In the future we should write spec files to the Supervisor's DAT file in a more + // appropriate machine readable format. We'll need to wait until we modify how we load and + // unload services, though. Right now we watch files on disk and communicate with the + // Supervisor asynchronously. We need to move to communicating directly with the + // Supervisor's main loop through IPC. + match SpecWatcher::spec_files(&fs_cfg.specs_path) { + Ok(specs) => { + for spec_file in specs { + match ServiceSpec::from_file(&spec_file) { + Ok(spec) => { + if let Err(err) = spec.to_file(&spec_file) { + outputln!( + "Unable to migrate service spec, {}, {}", + spec_file.display(), + err + ); + } + } + Err(err) => { + outputln!( + "Unable to migrate service spec, {}, {}", + spec_file.display(), + err + ); + } + } + } + } + Err(err) => outputln!("Unable to migrate service specs, {}", err), + } + } + fn new(cfg: ManagerConfig, fs_cfg: FsCfg, launcher: LauncherCli) -> Result { let current = PackageIdent::from_str(&format!("{}/{}", SUP_PKG_IDENT, VERSION)).unwrap(); let self_updater = if cfg.auto_update { @@ -259,6 +294,7 @@ impl Manager { peer.set_gossip_port(peer_addr.port() as i32); server.member_list.add_initial_member(peer); } + Self::migrate_specs(&fs_cfg); Ok(Manager { self_updater: self_updater, updater: ServiceUpdater::new(server.clone()), diff --git a/components/sup/src/manager/spec_watcher.rs b/components/sup/src/manager/spec_watcher.rs index e02d13226a..cf76ce80b0 100644 --- a/components/sup/src/manager/spec_watcher.rs +++ b/components/sup/src/manager/spec_watcher.rs @@ -15,7 +15,7 @@ use std::collections::{HashMap, HashSet}; use std::error::Error as StdErr; use std::ffi::OsStr; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::channel; @@ -52,6 +52,22 @@ impl SpecWatcher { Self::run_with::(path) } + pub fn spec_files(watch_path: T) -> Result> + where + T: AsRef, + { + Ok( + glob(&watch_path + .as_ref() + .join(SPEC_FILE_GLOB) + .display() + .to_string())? + .filter_map(|p| p.ok()) + .filter(|p| p.is_file()) + .collect(), + ) + } + pub fn initial_events(&mut self) -> Result> { self.generate_events(HashMap::new()) } @@ -210,14 +226,8 @@ impl SpecWatcher { } pub fn specs_from_watch_path<'a>(&self) -> Result> { - let spec_files: Vec = - glob(&self.watch_path.join(SPEC_FILE_GLOB).display().to_string())? - .filter_map(|p| p.ok()) - .filter(|p| p.is_file()) - .collect(); - let mut specs = HashMap::new(); - for spec_file in spec_files { + for spec_file in Self::spec_files(&self.watch_path)? { let spec = match ServiceSpec::from_file(&spec_file) { Ok(s) => s, Err(e) => { @@ -267,14 +277,6 @@ impl SpecWatcher { ); continue; } - // Migrate spec files using an older format - if let Err(err) = spec.to_file(&spec_file) { - outputln!( - "Unable to migrate service spec, {}, {}", - spec_file.display(), - err - ); - } specs.insert(spec.ident.name.clone(), spec); } Ok(specs) From 4ae7097c48dbdfe06ac0e0bb9c515464da52c6e0 Mon Sep 17 00:00:00 2001 From: Jamie Winsor Date: Sun, 24 Sep 2017 17:46:54 -0700 Subject: [PATCH 2/2] Fix panic when generating PATH values from a package install Check to see if the values read by `PATH` contain a root and strip it if they do so we can specify an alternate FS_ROOT_PATH Signed-off-by: Jamie Winsor --- components/core/src/package/install.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/components/core/src/package/install.rs b/components/core/src/package/install.rs index 23f4826b73..2f55cb6d1a 100644 --- a/components/core/src/package/install.rs +++ b/components/core/src/package/install.rs @@ -360,10 +360,8 @@ impl PackageInstall { match self.read_metafile(MetaFile::Path) { Ok(body) => { let v = env::split_paths(&body) - .map(|p| { - self.fs_root_path.join(PathBuf::from( - &p.strip_prefix("/").unwrap(), - )) + .filter_map(|p| { + p.strip_prefix("/").ok().map(|p| self.fs_root_path.join(p)) }) .collect(); Ok(v)