From f46844dd4854c658fa8f32a802c5d25bc5c9ed94 Mon Sep 17 00:00:00 2001 From: David McNeil Date: Mon, 10 Jun 2019 07:58:11 -0400 Subject: [PATCH] Cleanup lifecycle restarts and remove reload hook Resolves #5305 #5306 #5307 Signed-off-by: David McNeil PR feedback Signed-off-by: David McNeil --- .../sup/doc/http_gateway_services_schema.json | 10 -- components/sup/src/manager/mod.rs | 11 +- components/sup/src/manager/service/hooks.rs | 115 ++++++++--------- components/sup/src/manager/service/mod.rs | 119 ++++++------------ .../sup/src/manager/service/supervisor.rs | 26 ---- .../sample-services-with-cfg-output.json | 2 - .../sample-services-without-cfg-output.json | 2 - 7 files changed, 96 insertions(+), 189 deletions(-) diff --git a/components/sup/doc/http_gateway_services_schema.json b/components/sup/doc/http_gateway_services_schema.json index 489bd0227e..8f7af72582 100644 --- a/components/sup/doc/http_gateway_services_schema.json +++ b/components/sup/doc/http_gateway_services_schema.json @@ -332,14 +332,6 @@ ], "type": "object" }, - "needs_reconfiguration": { - "description": "Does this service need to be reconfigured", - "type": "boolean" - }, - "needs_reload": { - "description": "Does this service need to be reloaded", - "type": "boolean" - }, "pkg": { "description": "The habitat package that this service was spawned from", "properties": { @@ -560,8 +552,6 @@ "initialized", "last_election_status", "manager_fs_cfg", - "needs_reconfiguration", - "needs_reload", "pkg", "process", "service_group", diff --git a/components/sup/src/manager/mod.rs b/components/sup/src/manager/mod.rs index b6d7a71d9c..8672f12d88 100644 --- a/components/sup/src/manager/mod.rs +++ b/components/sup/src/manager/mod.rs @@ -1109,6 +1109,7 @@ impl Manager { /// # Locking /// * `MemberList::entries` (read) This method must not be called while any MemberList::entries /// lock is held. + #[rustfmt::skip] fn take_services_with_updates_mlr(&mut self) -> Vec { let mut updater = self.updater.lock().expect("Updater lock poisoned"); @@ -1117,8 +1118,10 @@ impl Manager { .write() .expect("Services lock is poisoned!"); let idents_to_restart: Vec<_> = state_services.iter() - .filter_map(|(current_ident, service)| { - if let Some(new_ident) = + .filter_map(|(current_ident, service)| { + if service.needs_restart { + Some(current_ident.clone()) + } else if let Some(new_ident) = updater.check_for_updated_package_mlr(&service, &self.census_ring) { outputln!("Updating from {} to {}", current_ident, new_ident); @@ -1128,8 +1131,8 @@ impl Manager { trace!("No update found for {}", current_ident); None } - }) - .collect(); + }) + .collect(); let mut services_to_restart = Vec::with_capacity(idents_to_restart.len()); for current_ident in idents_to_restart { diff --git a/components/sup/src/manager/service/hooks.rs b/components/sup/src/manager/service/hooks.rs index f2995c2000..90c534f350 100644 --- a/components/sup/src/manager/service/hooks.rs +++ b/components/sup/src/manager/service/hooks.rs @@ -222,49 +222,6 @@ impl Hook for PostRunHook { fn stderr_log_path(&self) -> &Path { &self.stderr_log_path } } -#[derive(Debug, Serialize)] -pub struct ReloadHook { - render_pair: RenderPair, - stdout_log_path: PathBuf, - stderr_log_path: PathBuf, -} - -impl Hook for ReloadHook { - type ExitValue = ExitCode; - - fn file_name() -> &'static str { "reload" } - - fn new(package_name: &str, pair: RenderPair) -> Self { - ReloadHook { render_pair: pair, - stdout_log_path: hooks::stdout_log_path::(package_name), - stderr_log_path: hooks::stderr_log_path::(package_name), } - } - - fn handle_exit<'a>(&self, pkg: &Pkg, _: &'a HookOutput, status: ExitStatus) -> Self::ExitValue { - let pkg_name = &pkg.name; - match status.code() { - Some(0) => ExitCode(0), - Some(code) => { - outputln!(preamble pkg_name, "Reload failed! '{}' exited with \ - status code {}", Self::file_name(), code); - ExitCode(code) - } - None => { - Self::output_termination_message(pkg_name, status); - ExitCode::default() - } - } - } - - fn path(&self) -> &Path { &self.render_pair.path } - - fn renderer(&self) -> &TemplateRenderer { &self.render_pair.renderer } - - fn stdout_log_path(&self) -> &Path { &self.stdout_log_path } - - fn stderr_log_path(&self) -> &Path { &self.stderr_log_path } -} - #[derive(Debug, Serialize)] pub struct ReconfigureHook { render_pair: RenderPair, @@ -418,6 +375,44 @@ impl Hook for PostStopHook { fn stderr_log_path(&self) -> &Path { &self.stderr_log_path } } +#[derive(Clone, Copy, Default)] +pub struct HookChangeTable { + pub health_check: bool, + pub init: bool, + pub file_updated: bool, + pub reconfigure: bool, + pub suitability: bool, + pub run: bool, + pub post_run: bool, + pub post_stop: bool, +} + +impl HookChangeTable { + pub fn new() -> Self { Self::default() } + + pub fn changed(self) -> bool { + match self { + Self { health_check, + init, + file_updated, + reconfigure, + suitability, + run, + post_run, + post_stop, } => { + health_check + || init + || file_updated + || reconfigure + || suitability + || run + || post_run + || post_stop + } + } + } +} + // Hooks wrapped in Arcs represent a possibly-temporary state while we // refactor hooks to be able to run asynchronously. #[derive(Debug, Default, Serialize)] @@ -425,7 +420,6 @@ pub struct HookTable { pub health_check: Option>, pub init: Option, pub file_updated: Option, - pub reload: Option, pub reconfigure: Option, pub suitability: Option, pub run: Option, @@ -447,7 +441,6 @@ impl HookTable { HealthCheckHook::load(package_name, &hooks_path, &templates).map(Arc::new); table.suitability = SuitabilityHook::load(package_name, &hooks_path, &templates); table.init = InitHook::load(package_name, &hooks_path, &templates); - table.reload = ReloadHook::load(package_name, &hooks_path, &templates); table.reconfigure = ReconfigureHook::load(package_name, &hooks_path, &templates); table.run = RunHook::load(package_name, &hooks_path, &templates); table.post_run = PostRunHook::load(package_name, &hooks_path, &templates); @@ -464,39 +457,36 @@ impl HookTable { /// Compile all loaded hooks from the table into their destination service directory. /// - /// Returns `true` if compiling any of the hooks resulted in new - /// content being written to the hook scripts on disk. - pub fn compile(&self, service_group: &str, ctx: &T) -> bool + /// Returns a `HookChangeTable` with the hook set to true if compiling the hook resulted + /// in new content being written to the hook script on disk. + pub fn compile(&self, service_group: &str, ctx: &T) -> HookChangeTable where T: Serialize { debug!("{:?}", self); - let mut changed = false; + let mut changed = HookChangeTable::new(); if let Some(ref hook) = self.file_updated { - changed |= self.compile_one(hook, service_group, ctx); + changed.file_updated = self.compile_one(hook, service_group, ctx); } if let Some(ref hook) = self.health_check { - changed |= self.compile_one(hook.as_ref(), service_group, ctx); + changed.health_check = self.compile_one(hook.as_ref(), service_group, ctx); } if let Some(ref hook) = self.init { - changed |= self.compile_one(hook, service_group, ctx); - } - if let Some(ref hook) = self.reload { - changed |= self.compile_one(hook, service_group, ctx); + changed.init = self.compile_one(hook, service_group, ctx); } if let Some(ref hook) = self.reconfigure { - changed |= self.compile_one(hook, service_group, ctx); + changed.reconfigure = self.compile_one(hook, service_group, ctx); } if let Some(ref hook) = self.suitability { - changed |= self.compile_one(hook, service_group, ctx); + changed.suitability = self.compile_one(hook, service_group, ctx); } if let Some(ref hook) = self.run { - changed |= self.compile_one(hook, service_group, ctx); + changed.run = self.compile_one(hook, service_group, ctx); } if let Some(ref hook) = self.post_run { - changed |= self.compile_one(hook, service_group, ctx); + changed.post_run = self.compile_one(hook, service_group, ctx); } if let Some(ref hook) = self.post_stop { - changed |= self.compile_one(hook.as_ref(), service_group, ctx); + changed.post_stop = self.compile_one(hook.as_ref(), service_group, ctx); } changed } @@ -566,7 +556,6 @@ mod tests { InitHook PostRunHook ReconfigureHook - ReloadHook RunHook SuitabilityHook PostStopHook); @@ -673,7 +662,7 @@ mod tests { //////////////////////////////////////////////////////////////////////// let hook_table = HookTable::load(&service_group, &template_path, &hooks_path); - assert_eq!(hook_table.compile(&service_group, &ctx), true); + assert!(hook_table.compile(&service_group, &ctx).changed(), true); // Verify init hook let init_hook_content = file_content(&hook_table.init.as_ref().expect("no init hook??")); @@ -686,7 +675,7 @@ mod tests { assert_eq!(run_hook_content, expected_run_hook); // Recompiling again results in no changes - assert_eq!(hook_table.compile(&service_group, &ctx), false); + assert!(!hook_table.compile(&service_group, &ctx).changed()); // Re-Verify init hook let init_hook_content = file_content(&hook_table.init.as_ref().expect("no init hook??")); diff --git a/components/sup/src/manager/service/mod.rs b/components/sup/src/manager/service/mod.rs index dc8656c1b9..4ad6c3eb4d 100644 --- a/components/sup/src/manager/service/mod.rs +++ b/components/sup/src/manager/service/mod.rs @@ -18,7 +18,8 @@ mod supervisor; mod terminator; use self::{context::RenderContext, - hooks::HookTable, + hooks::{HookChangeTable, + HookTable}, supervisor::Supervisor}; pub use self::{health::HealthCheckResult, hooks::HealthCheckHook, @@ -135,6 +136,7 @@ pub struct Service { pub initialized: bool, pub user_config_updated: bool, pub shutdown_timeout: Option, + pub needs_restart: bool, config_renderer: CfgRenderer, // Note: This field is really only needed for serializing a @@ -149,8 +151,6 @@ pub struct Service { // :( health_check_result: Arc>, last_election_status: ElectionStatus, - needs_reload: bool, - needs_reconfiguration: bool, /// The mapping of bind name to a service group, specified by the /// user when the service definition was loaded into the Supervisor. binds: Vec, @@ -182,9 +182,6 @@ pub struct Service { svc_encrypted_password: Option, health_check_interval: HealthCheckInterval, - /// Whether a service's default configuration changed on a package - /// update. Used to control when templates are re-rendered. - defaults_updated: bool, gateway_state: Arc>, /// A "handle" to the never-ending future that periodically runs @@ -223,9 +220,8 @@ impl Service { svc_hooks_path(&service_group.service())), initialized: false, last_election_status: ElectionStatus::None, - needs_reload: false, - needs_reconfiguration: false, user_config_updated: false, + needs_restart: false, manager_fs_cfg, supervisor: Arc::new(Mutex::new(Supervisor::new(&service_group))), pkg, @@ -241,7 +237,6 @@ impl Service { config_from: spec.config_from, svc_encrypted_password: spec.svc_encrypted_password, health_check_interval: spec.health_check_interval, - defaults_updated: false, gateway_state, health_check_handle: None, shutdown_timeout: spec.shutdown_timeout }) @@ -297,8 +292,7 @@ impl Service { { outputln!(preamble self.service_group, "Service start failed: {}", err); } else { - self.needs_reload = false; - self.needs_reconfiguration = false; + self.needs_restart = false; } self.start_health_checks(executor); @@ -393,31 +387,6 @@ impl Service { }) } - /// Runs the reconfigure hook if present, otherwise restarts the service. - fn reload(&mut self, launcher: &LauncherCli) { - let _timer = hook_timer("reload"); - self.needs_reload = false; - if self.process_down() || self.hooks.reload.is_none() { - if let Some(err) = - self.supervisor - .lock() - .expect("Couldn't lock supervisor") - .restart(&self.pkg, - &self.service_group, - launcher, - self.svc_encrypted_password.as_ref().map(String::as_ref)) - .err() - { - outputln!(preamble self.service_group, "Service restart failed: {}", err); - } - } else { - let hook = self.hooks.reload.as_ref().unwrap(); - hook.run(&self.service_group, - &self.pkg, - self.svc_encrypted_password.as_ref()); - } - } - pub fn last_state_change(&self) -> Timespec { self.supervisor .lock() @@ -455,14 +424,14 @@ impl Service { self.validate_binds(census_ring); } - let svc_updated = self.update_templates(census_ring); + let (svc_updated, hook_update_table, config_change) = self.update_templates(census_ring); if self.update_service_files(census_ring) { self.file_updated(); } match self.topology { Topology::Standalone => { - self.execute_hooks(launcher, executor); + self.execute_hooks(launcher, executor, hook_update_table, config_change); } Topology::Leader => { let census_group = @@ -502,7 +471,7 @@ impl Service { leader_id.to_string()); self.last_election_status = census_group.election_status; } - self.execute_hooks(launcher, executor) + self.execute_hooks(launcher, executor, hook_update_table, config_change) } } } @@ -719,14 +688,13 @@ impl Service { /// Compares the current state of the service to the current state of the census ring and the /// user-config, and re-renders all templatable content to disk. /// - /// Returns `true` if any modifications were made. - fn update_templates(&mut self, census_ring: &CensusRing) -> bool { + /// TODO (DM): Comment the return type + fn update_templates(&mut self, census_ring: &CensusRing) -> (bool, HookChangeTable, bool) { let census_group = census_ring.census_group_for(&self.service_group) .expect("Service update failed; unable to find own service group"); let cfg_updated_from_rumors = self.update_gossip(census_group); - let cfg_changed = - self.defaults_updated || cfg_updated_from_rumors || self.user_config_updated; + let cfg_changed = cfg_updated_from_rumors || self.user_config_updated; if self.user_config_updated { if let Err(e) = self.cfg.reload_user() { @@ -736,29 +704,14 @@ impl Service { self.user_config_updated = false; } - self.defaults_updated = false; - + // TODO (DM): Do we need to return cfg_changed? What does it actually indicate? + // Can we just check if HookChangeTable and config_changed? if cfg_changed || census_ring.changed() { - let (reload, reconfigure) = { - let ctx = self.render_context(census_ring); - - // If any hooks have changed, execute the `reload` hook (if present) or restart the - // service. - let reload = self.compile_hooks(&ctx); - - // If the configuration has changed, execute the `reload` and `reconfigure` hooks. - // Note that the configuration does not necessarily change every time the user - // config has (e.g. when only a comment has been added to the latter) - let reconfigure = self.compile_configuration(&ctx); - - (reload, reconfigure) - }; - - self.needs_reload = reload; - self.needs_reconfiguration = reconfigure; + let ctx = self.render_context(census_ring); + (cfg_changed, self.compile_hooks(&ctx), self.compile_configuration(&ctx)) + } else { + (cfg_changed, HookChangeTable::new(), false) } - - cfg_changed } pub fn to_rumor(&self, incarnation: u64) -> ServiceRumor { @@ -802,7 +755,6 @@ impl Service { fn reconfigure(&mut self) { let _timer = hook_timer("reconfigure"); - self.needs_reconfiguration = false; if let Some(ref hook) = self.hooks.reconfigure { hook.run(&self.service_group, &self.pkg, @@ -869,17 +821,15 @@ impl Service { /// Helper for compiling hook templates into hooks. /// /// This function will also perform any necessary post-compilation tasks. - /// - /// Returns `true` if any hooks have changed. - fn compile_hooks(&self, ctx: &RenderContext<'_>) -> bool { - let changed = self.hooks.compile(&self.service_group, ctx); + fn compile_hooks(&self, ctx: &RenderContext<'_>) -> HookChangeTable { + let hook_update_table = self.hooks.compile(&self.service_group, ctx); if let Some(err) = self.copy_run().err() { outputln!(preamble self.service_group, "Failed to copy run hook: {}", err); } - if changed { + if hook_update_table.changed() { outputln!(preamble self.service_group, "Hooks recompiled"); } - changed + hook_update_table } // Copy the "run" file to the svc path. @@ -921,7 +871,11 @@ impl Service { win_perm::harden_path(path.as_ref()) } - fn execute_hooks(&mut self, launcher: &LauncherCli, executor: &TaskExecutor) { + fn execute_hooks(&mut self, + launcher: &LauncherCli, + executor: &TaskExecutor, + hook_update_table: HookChangeTable, + config_change: bool) { if !self.initialized { if self.check_process() { outputln!("Reattached to {}", self.service_group); @@ -935,14 +889,17 @@ impl Service { } } else { self.check_process(); - // NOTE: if you need reconfiguration and you DON'T have a - // reload script, you're going to restart anyway. - if self.needs_reload || self.process_down() || self.needs_reconfiguration { - self.reload(launcher); - if self.needs_reconfiguration { - // NOTE this only runs the hook if it's defined - self.reconfigure() - } + if hook_update_table.run + || hook_update_table.post_run + || self.process_down() + || (self.hooks.reconfigure.is_none() && config_change) + { + // TODO (DM): This flag is a hack. We have the `TaskExecutor` here. We could just + // schedule the `stop` future, but the `Manager` wraps the `stop` future with + // additional functionality. Can we refactor to make this flag unnecessary? + self.needs_restart = true; + } else if config_change || hook_update_table.reconfigure { + self.reconfigure(); } } } @@ -1142,8 +1099,6 @@ impl<'a> Serialize for ServiceProxy<'a> { strukt.serialize_field("initialized", &s.initialized)?; strukt.serialize_field("last_election_status", &s.last_election_status)?; strukt.serialize_field("manager_fs_cfg", &s.manager_fs_cfg)?; - strukt.serialize_field("needs_reconfiguration", &s.needs_reconfiguration)?; - strukt.serialize_field("needs_reload", &s.needs_reload)?; let pkg_proxy = PkgProxy::new(&s.pkg); strukt.serialize_field("pkg", &pkg_proxy)?; diff --git a/components/sup/src/manager/service/supervisor.rs b/components/sup/src/manager/service/supervisor.rs index 0fd525fe47..adef1b4d6c 100644 --- a/components/sup/src/manager/service/supervisor.rs +++ b/components/sup/src/manager/service/supervisor.rs @@ -207,32 +207,6 @@ impl Supervisor { } } - pub fn restart(&mut self, - pkg: &Pkg, - group: &ServiceGroup, - launcher: &LauncherCli, - svc_password: Option<&str>) - -> Result<()> { - match self.pid { - Some(pid) => { - match launcher.restart(pid) { - Ok(pid) => { - self.pid = Some(pid); - self.create_pidfile()?; - self.change_state(ProcessState::Up); - Ok(()) - } - Err(err) => { - self.cleanup_pidfile(); - self.change_state(ProcessState::Down); - Err(Error::Launcher(err)) - } - } - } - None => self.start(pkg, group, launcher, svc_password), - } - } - /// Create a PID file for a running service fn create_pidfile(&self) -> Result<()> { match self.pid { diff --git a/components/sup/tests/fixtures/http-gateway/sample-services-with-cfg-output.json b/components/sup/tests/fixtures/http-gateway/sample-services-with-cfg-output.json index 4a81630a02..e13896fb5b 100644 --- a/components/sup/tests/fixtures/http-gateway/sample-services-with-cfg-output.json +++ b/components/sup/tests/fixtures/http-gateway/sample-services-with-cfg-output.json @@ -85,8 +85,6 @@ "specs_path": "/hab/sup/default/specs", "sup_root": "/hab/sup/default" }, - "needs_reconfiguration": false, - "needs_reload": false, "pkg": { "deps": [ { diff --git a/components/sup/tests/fixtures/http-gateway/sample-services-without-cfg-output.json b/components/sup/tests/fixtures/http-gateway/sample-services-without-cfg-output.json index bada8947f2..83dbea9380 100644 --- a/components/sup/tests/fixtures/http-gateway/sample-services-without-cfg-output.json +++ b/components/sup/tests/fixtures/http-gateway/sample-services-without-cfg-output.json @@ -41,8 +41,6 @@ "specs_path": "/hab/sup/default/specs", "sup_root": "/hab/sup/default" }, - "needs_reconfiguration": false, - "needs_reload": false, "pkg": { "deps": [ {