From c2079f887603b94e420f2f602e003a3d4cb8493b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Thu, 10 Dec 2020 08:42:46 +0100 Subject: [PATCH 01/17] First changes towards generating systemd unit files. Save changes to branch. Working version (compiles and produces file - not a running service) - still needs environment variables. Added restart policy. Working version of unit file creation. Store work Added rough structure of systemd module / might be refactored into a crate at a later time. --- Cargo.lock | 262 +++++++++++++------ Cargo.toml | 2 + src/provider/mod.rs | 17 ++ src/provider/states/creating_service.rs | 39 ++- src/provider/systemdmanager/manager.rs | 169 ++++++++++++ src/provider/systemdmanager/mod.rs | 2 + src/provider/systemdmanager/service.rs | 333 ++++++++++++++++++++++++ 7 files changed, 743 insertions(+), 81 deletions(-) create mode 100644 src/provider/systemdmanager/manager.rs create mode 100644 src/provider/systemdmanager/mod.rs create mode 100644 src/provider/systemdmanager/service.rs diff --git a/Cargo.lock b/Cargo.lock index c41363a..e044f59 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -106,9 +106,9 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "25f9db3b38af870bf7e5cc649167533b493928e50744e2c30ae350230b414670" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -117,9 +117,9 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a3548b8efc9f8e8a5a0a2808c5bd8451a9031b9e5b879a79590304ae928b0a70" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -128,9 +128,9 @@ version = "0.1.42" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8d3a45e77e34375a7923b1e8febb049bb011f064714a8e17a1a616fef01da13d" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -461,15 +461,25 @@ dependencies = [ "num_cpus", ] +[[package]] +name = "dbus" +version = "0.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3b1334c0161ddfccd239ac81b188d62015b049c986c5cd0b7f9447cf2c54f4a3" +dependencies = [ + "libc", + "libdbus-sys", +] + [[package]] name = "derivative" version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cb582b60359da160a9477ee80f15c8d784c477e69c217ef2cdd4169c24ea380f" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -719,9 +729,9 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "77408a692f1f97bcc61dc001d752e00643408fbc922e4d634c655df50d595556" dependencies = [ "proc-macro-hack", - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -1201,10 +1211,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cd71bf282e5551ac0852afcf25352b7fb8dd9a66eed7b6e66a6ebbf6b5b2f475" dependencies = [ "Inflector", - "proc-macro2", - "quote", + "proc-macro2 1.0.24", + "quote 1.0.7", "serde_json", - "syn", + "syn 1.0.50", ] [[package]] @@ -1279,8 +1289,8 @@ name = "kubelet-derive" version = "0.1.0" source = "git+https://github.com/deislabs/krustlet.git?rev=ac218b38ba564de806568e49d9e38aaef9f41537#ac218b38ba564de806568e49d9e38aaef9f41537" dependencies = [ - "quote", - "syn", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -1307,6 +1317,15 @@ version = "0.2.80" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4d58d1b70b004888f764dfbf6a26a3b0342a1632d33968e4a179d8011c760614" +[[package]] +name = "libdbus-sys" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc12a3bc971424edbbf7edaf6e5740483444db63aa8e23d3751ff12a30f306f0" +dependencies = [ + "pkg-config", +] + [[package]] name = "linked-hash-map" version = "0.5.3" @@ -1671,9 +1690,9 @@ checksum = "99b8db626e31e5b81787b9783425769681b347011cc59471e33ea46d2ea0cf55" dependencies = [ "pest", "pest_meta", - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -1697,6 +1716,48 @@ dependencies = [ "indexmap", ] +[[package]] +name = "phf" +version = "0.7.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b3da44b85f8e8dfaec21adae67f95d93244b2ecf6ad2a692320598dcc8e6dd18" +dependencies = [ + "phf_macros", + "phf_shared", +] + +[[package]] +name = "phf_generator" +version = "0.7.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09364cc93c159b8b06b1f4dd8a4398984503483891b0c26b867cf431fb132662" +dependencies = [ + "phf_shared", + "rand 0.6.5", +] + +[[package]] +name = "phf_macros" +version = "0.7.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bdb45e833315153371697760dad1831da99ce41884162320305e4f123ca3fe37" +dependencies = [ + "phf_generator", + "phf_shared", + "proc-macro2 0.4.30", + "quote 0.6.13", + "syn 0.15.44", +] + +[[package]] +name = "phf_shared" +version = "0.7.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "234f71a15de2288bcb7e3b6515828d22af7ec8598ee6d24c3b526fa0a80b67a0" +dependencies = [ + "siphasher", +] + [[package]] name = "pin-project" version = "0.4.27" @@ -1721,9 +1782,9 @@ version = "0.4.27" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "65ad2ae56b6abe3a1ee25f15ee605bacadb9a764edaba9c2bf4103800d4a1895" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -1732,9 +1793,9 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8e8d2bf0b23038a4424865103a4df472855692821aab4e4f5c3312d461d9e5f" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -1863,9 +1924,9 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" dependencies = [ "proc-macro-error-attr", - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", "version_check 0.9.2", ] @@ -1875,8 +1936,8 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" dependencies = [ - "proc-macro2", - "quote", + "proc-macro2 1.0.24", + "quote 1.0.7", "version_check 0.9.2", ] @@ -1892,6 +1953,15 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eba180dafb9038b050a4c280019bbedf9f2467b61e5d892dcad585bb57aadc5a" +[[package]] +name = "proc-macro2" +version = "0.4.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cf3d2011ab5c909338f7887f4fc896d35932e29146c12c8d01da6b22a80ba759" +dependencies = [ + "unicode-xid 0.1.0", +] + [[package]] name = "proc-macro2" version = "1.0.24" @@ -1937,9 +2007,9 @@ checksum = "537aa19b95acde10a12fec4301466386f757403de4cd4e5b4fa78fb5ecb18f72" dependencies = [ "anyhow", "itertools", - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -1964,13 +2034,22 @@ version = "2.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3ac73b1112776fc109b2e61909bc46c7e1bf0d7f690ffb1676553acce16d5cda" +[[package]] +name = "quote" +version = "0.6.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ce23b6b870e8f94f81fb0a363d65d86675884b34a09043c81e5562f11c1f8e1" +dependencies = [ + "proc-macro2 0.4.30", +] + [[package]] name = "quote" version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "aa563d17ecb180e500da1cfd2b028310ac758de548efdd203e18f283af693f37" dependencies = [ - "proc-macro2", + "proc-macro2 1.0.24", ] [[package]] @@ -2420,9 +2499,9 @@ version = "1.0.117" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cbd1ae72adb44aab48f325a02444a5fc079349a8d804c1fc922aed3f7454c74e" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -2512,6 +2591,12 @@ dependencies = [ "libc", ] +[[package]] +name = "siphasher" +version = "0.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b8de496cf83d4ed58b6be86c3a275b8602f6ffe98d3024a869e124147a9a3ac" + [[package]] name = "slab" version = "0.4.2" @@ -2542,9 +2627,9 @@ version = "0.6.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7073448732a89f2f3e6581989106067f403d378faeafb4a50812eb814170d3e5" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -2571,6 +2656,7 @@ version = "0.1.0" dependencies = [ "anyhow", "async-trait", + "dbus", "env_logger", "flate2", "handlebars", @@ -2582,6 +2668,7 @@ dependencies = [ "kubelet", "log 0.4.11", "oci-distribution", + "phf", "pnet", "reqwest", "rstest", @@ -2641,11 +2728,11 @@ version = "0.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c87a60a40fccc84bef0652345bbbbbe20a605bf5d0ce81719fc476f5c03b50ef" dependencies = [ - "proc-macro2", - "quote", + "proc-macro2 1.0.24", + "quote 1.0.7", "serde", "serde_derive", - "syn", + "syn 1.0.50", ] [[package]] @@ -2655,13 +2742,13 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "58fa5ff6ad0d98d1ffa8cb115892b6e69d67799f6763e162a1c9db421dc22e11" dependencies = [ "base-x", - "proc-macro2", - "quote", + "proc-macro2 1.0.24", + "quote 1.0.7", "serde", "serde_derive", "serde_json", "sha1", - "syn", + "syn 1.0.50", ] [[package]] @@ -2695,9 +2782,20 @@ checksum = "65e51c492f9e23a220534971ff5afc14037289de430e3c83f9daf6a1b6ae91e8" dependencies = [ "heck", "proc-macro-error", - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", +] + +[[package]] +name = "syn" +version = "0.15.44" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ca4b3b69a77cbe1ffc9e198781b7acb0c7365a883670e8f1c1bc66fba79a5c5" +dependencies = [ + "proc-macro2 0.4.30", + "quote 0.6.13", + "unicode-xid 0.1.0", ] [[package]] @@ -2706,8 +2804,8 @@ version = "1.0.50" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "443b4178719c5a851e1bde36ce12da21d74a0e60b4d982ec3385a933c812f0f6" dependencies = [ - "proc-macro2", - "quote", + "proc-macro2 1.0.24", + "quote 1.0.7", "unicode-xid 0.2.1", ] @@ -2840,9 +2938,9 @@ version = "1.0.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ba20f23e85b10754cd195504aebf6a27e2e6cbe28c17778a0c930724628dd56" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -2897,10 +2995,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5c3be1edfad6027c69f5491cf4cb310d1a71ecd6af742788c6ff8bced86b8fa" dependencies = [ "proc-macro-hack", - "proc-macro2", - "quote", + "proc-macro2 1.0.24", + "quote 1.0.7", "standback", - "syn", + "syn 1.0.50", ] [[package]] @@ -2955,9 +3053,9 @@ version = "0.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e44da00bfc73a25f814cd8d7e57a68a5c31b74b3152a0a1d1f590c97ed06265a" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -3045,10 +3143,10 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "19970cf58f3acc820962be74c4021b8bbc8e8a1c4e3a02095d0aa60cde5f3633" dependencies = [ - "proc-macro2", + "proc-macro2 1.0.24", "prost-build", - "quote", - "syn", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -3248,9 +3346,9 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "80e0ccfc3378da0cce270c946b676a376943f5cd16aeba64568e7939806f4ada" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", ] [[package]] @@ -3381,6 +3479,12 @@ version = "0.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "36dff09cafb4ec7c8cf0023eb0b686cb6ce65499116a12201c9e11840ca01beb" +[[package]] +name = "unicode-xid" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc72304796d0818e357ead4e000d19c9c174ab23dc11093ac919054d20a6a7fc" + [[package]] name = "unicode-xid" version = "0.2.1" @@ -3550,9 +3654,9 @@ dependencies = [ "bumpalo", "lazy_static", "log 0.4.11", - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", "wasm-bindgen-shared", ] @@ -3574,7 +3678,7 @@ version = "0.2.68" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6b13312a745c08c469f0b292dd2fcd6411dba5f7160f593da6ef69b64e407038" dependencies = [ - "quote", + "quote 1.0.7", "wasm-bindgen-macro-support", ] @@ -3584,9 +3688,9 @@ version = "0.2.68" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f249f06ef7ee334cc3b8ff031bfc11ec99d00f34d86da7498396dc1e3b1498fe" dependencies = [ - "proc-macro2", - "quote", - "syn", + "proc-macro2 1.0.24", + "quote 1.0.7", + "syn 1.0.50", "wasm-bindgen-backend", "wasm-bindgen-shared", ] @@ -3617,8 +3721,8 @@ version = "0.3.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e8fb9c67be7439ee8ab1b7db502a49c05e51e2835b66796c705134d9b8e1a585" dependencies = [ - "proc-macro2", - "quote", + "proc-macro2 1.0.24", + "quote 1.0.7", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 0c0ce56..d68f04e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,8 @@ thiserror = "1.0" url = "2.2" pnet = "0.26.0" stackable_config = { git = "https://github.com/stackabletech/common.git", branch = "main" } +phf = { version = "0.7.24", features = ["macros"] } +dbus = "0.9.0" hostname = "0.3" [dev-dependencies] diff --git a/src/provider/mod.rs b/src/provider/mod.rs index 2071740..0afc16e 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -20,13 +20,16 @@ use crate::provider::error::StackableError::{ use crate::provider::repository::package::Package; use crate::provider::states::downloading::Downloading; use crate::provider::states::terminated::Terminated; +use crate::provider::systemdmanager::manager::Manager; use kube::error::ErrorResponse; +use std::sync::{Arc, RwLock}; pub struct StackableProvider { client: Client, parcel_directory: PathBuf, config_directory: PathBuf, log_directory: PathBuf, + systemd_manager: Arc>, } pub const CRDS: &[&str] = &["repositories.stable.stackable.de"]; @@ -34,6 +37,7 @@ pub const CRDS: &[&str] = &["repositories.stable.stackable.de"]; mod error; mod repository; mod states; +mod systemdmanager; pub struct PodState { client: Client, @@ -46,6 +50,7 @@ pub struct PodState { service_uid: String, package: Package, process_handle: Option, + systemd_manager: Arc>, } impl PodState { @@ -62,6 +67,11 @@ impl PodState { pub fn get_service_log_directory(&self) -> PathBuf { self.log_directory.join(&self.service_name) } + + // I know, I know, the name sucks! + pub fn get_service_service_directory(&self) -> PathBuf { + self.get_service_config_directory().join("_service") + } } impl StackableProvider { @@ -71,11 +81,17 @@ impl StackableProvider { config_directory: PathBuf, log_directory: PathBuf, ) -> Result { + let systemdmanager = RwLock::new(Manager::new( + PathBuf::from("/home/sliebau/IdeaProjects/agent/work/systemd"), + false, + )); + let provider = StackableProvider { client, parcel_directory, config_directory, log_directory, + systemd_manager: Arc::new(systemdmanager), }; let missing_crds = provider.check_crds().await?; return if missing_crds.is_empty() { @@ -194,6 +210,7 @@ impl Provider for StackableProvider { service_uid, package, process_handle: None, + systemd_manager: self.systemd_manager.clone(), }) } diff --git a/src/provider/states/creating_service.rs b/src/provider/states/creating_service.rs index 05e045c..1e49773 100644 --- a/src/provider/states/creating_service.rs +++ b/src/provider/states/creating_service.rs @@ -1,11 +1,13 @@ use kubelet::pod::Pod; use kubelet::state::prelude::*; use kubelet::state::{State, Transition}; -use log::info; +use log::{debug, error, info}; use crate::provider::states::setup_failed::SetupFailed; use crate::provider::states::starting::Starting; +use crate::provider::systemdmanager::service::Service; use crate::provider::PodState; +use std::fs::create_dir_all; #[derive(Default, Debug, TransitionTo)] #[transition_to(Starting, SetupFailed)] @@ -13,11 +15,44 @@ pub struct CreatingService; #[async_trait::async_trait] impl State for CreatingService { - async fn next(self: Box, pod_state: &mut PodState, _pod: &Pod) -> Transition { + async fn next(self: Box, pod_state: &mut PodState, pod: &Pod) -> Transition { info!( "Creating service unit for service {}", &pod_state.service_name ); + let service_directory = &pod_state.get_service_service_directory(); + if !service_directory.is_dir() { + debug!( + "Creating config directory for service [{}]: {:?}", + pod_state.service_name, service_directory + ); + match create_dir_all(service_directory) { + Ok(()) => {} + Err(error) => return Transition::Complete(Err(anyhow::Error::from(error))), + } + } + + let service = match Service::new(pod, pod_state) { + Ok(new_service) => new_service, + Err(error) => { + error!( + "Failed to create service units from pod [{}], aborting.", + pod_state.service_name + ); + return Transition::Complete(Err(anyhow::Error::from(error))); + } + }; + + /*match service.write_unit_files(service_directory) { + Ok(()) => Transition::next(self, Starting), + Err(error) => { + error!( + "Failed to write service units for pod [{}], aborting.", + pod_state.service_name + ); + return Transition::Complete(Err(anyhow::Error::from(error))); + } + }*/ Transition::next(self, Starting) } diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs new file mode 100644 index 0000000..04589b1 --- /dev/null +++ b/src/provider/systemdmanager/manager.rs @@ -0,0 +1,169 @@ +//* A systemd unit manager that allows managing systemd services + +use crate::provider::systemdmanager::service::SystemDUnit; +use dbus::blocking::SyncConnection; +use log::{debug, error, info}; +use serde::de::Error; +use std::fs::File; +use std::io::Write; +use std::path::PathBuf; +use std::time::Duration; + +enum UnitTypes { + Service, +} + +pub struct Manager { + units_directory: PathBuf, + user_mode: bool, + systemd_connection: SystemDConnection, +} + +impl Manager { + pub fn new(units_directory: PathBuf, user_mode: bool) -> Self { + Manager { + units_directory, + user_mode, + systemd_connection: SystemDConnection::new(), // Unused at present + } + } + + // Write the unit file to disk and enable the service + // TODO: should we maybe split enable out into a different function? + pub fn load(&self, name: &str, unit: SystemDUnit) -> Result<(), anyhow::Error> { + let target_file = self + .units_directory + .join(Manager::get_unit_file_path(name, UnitTypes::Service)?); + debug!("Target file for service {} : {:?}", name, target_file); + + let mut unit_file = File::create(&target_file)?; + unit_file.write(unit.get_unit_file_content().as_bytes()); + unit_file.flush(); + self.systemd_connection + .enable_unit_file(&target_file.into_os_string().to_string_lossy()); + Ok(()) + } + + // Check if the unit name is valid and append .service if needed + fn get_unit_file_path(name: &str, unit_type: UnitTypes) -> Result { + // TODO: what are valid systemd unit names? + + // Append proper extension for unit type to file name + let extension = match unit_type { + UnitTypes::Service => ".service", + }; + + // Todo: fugly + if !name.ends_with(extension) { + let mut result = String::from(name); + result.push_str(extension); + Ok(result) + } else { + Ok(String::from(name)) + } + } + + // Stop and disable the service, then delete the unit file from disk + pub fn unload() -> Result<(), anyhow::Error> { + Ok(()) + } + + pub fn reload() -> Result<(), anyhow::Error> { + Ok(()) + } +} + +struct SystemDConnection { + connection: SyncConnection, //TODO does this need to be closed? + dest: &'static str, + node: &'static str, + interface: &'static str, + timeout: Duration, +} + +impl SystemDConnection { + fn new() -> SystemDConnection { + let connection = SyncConnection::new_system().expect("D-Bus connection failed"); + SystemDConnection { + connection: connection, + dest: "org.freedesktop.systemd1", + node: "/org/freedesktop/systemd1", + interface: "org.freedesktop.systemd1.Manager", + timeout: Duration::from_millis(5000), + } + } + + /// Takes a unit name as input and attempts to start it. + pub fn start_unit(&self, unit: &str) -> Result { + // create a wrapper struct around the connection that makes it easy + // to send method calls to a specific destination and path. + let proxy = self + .connection + .with_proxy(self.dest, self.node, self.timeout); + proxy + .method_call(self.interface, "StartUnit", (unit, "fail")) + .and_then(|r: (u32,)| Ok(r.0)) + } + + /// Takes a unit name as input and attempts to stop it. + pub fn stop_unit(&self, unit: &str) -> Result { + // create a wrapper struct around the connection that makes it easy + // to send method calls to a specific destination and path. + let proxy = self + .connection + .with_proxy(self.dest, self.node, self.timeout); + proxy + .method_call(self.interface, "StopUnit", (unit, "fail")) + .and_then(|r: (u32,)| Ok(r.0)) + } + + /// THIS WORKS + /// Takes the unit pathname of a service and enables it via dbus. + /// If dbus replies with `[Bool(true), Array([], "(sss)")]`, the service is already enabled. + pub fn enable_unit_file(&self, unit: &str) -> Option { + let proxy = self + .connection + .with_proxy(self.dest, self.node, self.timeout); + let runtime = false; + let force = true; + let result: Result<(), dbus::Error> = proxy.method_call( + self.interface, + "EnableUnitFiles", + (&[unit][..], runtime, force), + ); + match result { + Ok(reply) => { + let (s) = reply; + println!("{:?}", s); + /* if format!("{:?}", reply.get_items()) == "[Bool(true), Array([], \"(sss)\")]" { + println!("{} already enabled", unit); + } else { + println!("{} has been enabled", unit); + }*/ + None + } + Err(reply) => { + let error = format!("Error enabling {}:\n{:?}", unit, reply); + println!("{}", error); + Some(error) + } + } + } +} + +/* + Disable(name string) error + Load(name string, u unit.File) error + Mask(name string) error + Properties(name string) (map[string]interface{}, error) + Property(name, property string) string + Reload() error + ServiceProperty(name, property string) string + State(name string) (*unit.State, error) + States(prefix string) (map[string]*unit.State, error) + TriggerStart(name string) error + TriggerStop(name string) error + Unit(name string) string + Units() ([]string, error) + Unload(name string) error +*/ diff --git a/src/provider/systemdmanager/mod.rs b/src/provider/systemdmanager/mod.rs new file mode 100644 index 0000000..91fffe4 --- /dev/null +++ b/src/provider/systemdmanager/mod.rs @@ -0,0 +1,2 @@ +pub mod manager; +pub mod service; diff --git a/src/provider/systemdmanager/service.rs b/src/provider/systemdmanager/service.rs new file mode 100644 index 0000000..c616e58 --- /dev/null +++ b/src/provider/systemdmanager/service.rs @@ -0,0 +1,333 @@ +use std::collections::HashMap; +use std::fs::File; +use std::io::Write; +use std::path::PathBuf; + +use kubelet::container::Container; +use kubelet::pod::Pod; +use phf::{phf_map, phf_ordered_set}; + +use crate::provider::error::StackableError; + +use crate::provider::error::StackableError::PodValidationError; +use crate::provider::states::creating_config::CreatingConfig; +use crate::provider::states::setup_failed::SetupFailed; +use crate::provider::PodState; +use kubelet::state::Transition; +use log::{debug, error, info, trace, warn}; +use url::form_urlencoded::Target; + +static RESTART_POLICY_MAP: phf::Map<&'static str, &'static str> = phf_map! { + "Always" => "always", + "OnFailure" => "on-failure", + "Never" => "no", +}; + +static SECTION_ORDER: phf::OrderedSet<&'static str> = + phf_ordered_set! {"Unit", "Service", "Install"}; + +struct Section { + name: &'static str, +} + +pub struct Service { + systemd_units: Vec, +} + +impl Service { + pub fn new(pod: &Pod, pod_state: &PodState) -> Result { + // Create systemd unit template with values we need from the pod object + let pod_settings = SystemDUnit::new_from_pod(&pod); + + // Convert all containers to systemd unit files + let systemd_units: Result, StackableError> = pod + .containers() + .iter() + .map(|container| SystemDUnit::new(&pod_settings, container, pod_state)) + .collect(); + + systemd_units.map(|units| Self { + systemd_units: units, + }) + } + + pub fn start_all(&self) -> Result<(), StackableError> { + Ok(()) + } + + pub fn stop_all(&self) -> Result<(), StackableError> { + Ok(()) + } +} + +#[derive(Clone)] +pub struct SystemDUnit { + name: String, + sections: HashMap>, + environment: HashMap, +} + +impl SystemDUnit { + pub fn new( + unit_template: &SystemDUnit, + container: &Container, + pod_state: &PodState, + ) -> Result { + let mut unit = unit_template.clone(); + unit.name = String::from(container.name()); + + unit.add_property("Unit", "Description", &unit.name.clone()); + + unit.add_property( + "Service", + "ExecStart", + &SystemDUnit::get_command(container, pod_state)?, + ); + + let env_vars = SystemDUnit::get_environment(container, pod_state)?; + + for (name, value) in env_vars { + unit.add_env_var(&name, &value); + } + + unit.add_property("Service", "StandardOutput", "Journal"); + unit.add_property("Service", "StandardError", "Journal"); + + Ok(unit) + } + + pub fn new_from_pod(pod: &Pod) -> Self { + let mut unit = SystemDUnit { + name: pod.name().to_string(), + sections: Default::default(), + environment: Default::default(), + }; + + let restart_policy = match &pod.as_kube_pod().spec { + Some(spec) => spec.restart_policy.as_deref().unwrap_or("Never"), + None => "Never", + }; + + unit.add_property( + "Service", + "Restart", + RESTART_POLICY_MAP.get(restart_policy).unwrap(), + ); + unit + } + + // Add a key=value entry to the specified section + fn add_property(&mut self, section: &'static str, key: &str, value: &str) { + let section = self + .sections + .entry(String::from(section)) + .or_insert(HashMap::new()); + section.insert(String::from(key), String::from(value)); + } + + fn add_env_var(&mut self, name: &str, value: &str) { + self.environment + .insert(String::from(name), String::from(value)); + } + + pub fn get_unit_file_content(&self) -> String { + let mut unit_file_content = String::new(); + + // Iterate over all sections and write out its header and content + for (section, entries) in &self.sections { + unit_file_content.push_str(&format!("[{}]\n", section)); + for (key, value) in entries { + unit_file_content.push_str(&format!("{}={}\n", key, value)); + } + if section.eq("Service") { + // Add environment variables to Service section + for (name, value) in &self.environment { + unit_file_content.push_str(&format!("Environment=\"{}={}\"\n", name, value)); + } + } + unit_file_content.push_str("\n"); + } + unit_file_content + } + + fn get_environment( + container: &Container, + pod_state: &PodState, + ) -> Result, StackableError> { + // Create template data to be used when rendering template strings + let template_data = if let Ok(data) = CreatingConfig::create_render_data(&pod_state) { + data + } else { + error!("Unable to parse directories for command template as UTF8"); + return Err(PodValidationError { + msg: format!( + "Unable to parse directories for command template as UTF8 for container [{}].", + container.name() + ), + }); + }; + + // Check if environment variables are set on the container - if some are present + // we render all values as templates to replace configroot, packageroot and logroot + // directories in case they are referenced in the values + // + // If even one of these renderings fails the entire pod will be failed and + // transitioned to a complete state with the error that occurred. + // If all renderings work, the vec<(String,String)> is returned as value and used + // later when starting the process + // This works because Result implements + // (FromIterator)[https://doc.rust-lang.org/std/result/enum.Result.html#method.from_iter] + // which returns a Result that is Ok(..) if none of the internal results contained + // an Error. If any error occurred, iteration stops on the first error and returns + // that in the outer result. + let env_variables = if let Some(vars) = container.env() { + debug!( + "Got environment vars: {:?} service {}", + vars, pod_state.service_name + ); + let render_result = vars + .iter() + .map(|env_var| { + // Replace variables in value + CreatingConfig::render_config_template( + &template_data, + &env_var.value.as_deref().unwrap_or_default(), + ) + .map(|value| (env_var.name.clone(), value)) + }) + .collect(); + + // If any single rendering failed, the overall result for the map will have + // collected the Err which we can check for here + match render_result { + Ok(rendered_values) => rendered_values, + Err(error) => { + error!("Failed to render value for env var due to: {:?}", error); + return Err(PodValidationError { + msg: String::from("Failed to render a template"), + }); + } + } + } else { + // No environment variables present for this container -> empty vec + debug!( + "No environment vars set for service {}", + pod_state.service_name + ); + vec![] + }; + debug!( + "Setting environment for service {} to {:?}", + pod_state.service_name, &env_variables + ); + + Ok(env_variables) + } + + fn get_command(container: &Container, pod_state: &PodState) -> Result { + // Return an error if no command was specified in the container + // TODO: We should discuss if there can be a valid scenario for this + // This clones because we perform some in place mutations on the elements + let mut command = match container.command() { + Some(command) => command.clone(), + _ => { + return Err(PodValidationError { + msg: format!( + "Error creating systemd unit for container {}, due to missing command element.", + container.name() + ), + }) + } + }; + + let package_root = pod_state.get_service_package_directory(); + + trace!( + "Commmand before replacing variables and adding packageroot: {:?}", + command + ); + // Get a mutable reference to the first element of the command array as we might need to + // add the package directory to this to make it an absolute path + let binary = match command.get_mut(0) { + Some(binary_string) => binary_string, + None => { + return Err(PodValidationError { + msg: format!( + "Unable to convert command for container [{}] to utf8.", + container.name() + ), + }) + } + }; + + // Warn if the user tried to add the packageroot directory to the command themselves + // This warning only triggers if the command starts with the packageroot as this is the + // only hard coded replacement we perform + // It might be perfectly reasonable to reference the packageroot directory somewhere + // later on in the command + if binary.starts_with("{{packageroot}}") { + warn!("Command for [{}] starts with \"{{packageroot}}\" - this would usually be automatically prepended to the command. Skipping prepending the directory and relying on string replacement instead, which is not recommended!", container.name()); + } else { + // Prepend package root to first element of the command array, which should be the binary + // this service has to execute + debug!( + "Prepending [{:?}] as package directory to the command for container [{}]", + package_root, + container.name() + ); + let mut binary_with_path = + match package_root.join(&binary).into_os_string().into_string() { + Ok(path_string) => path_string, + Err(original_string) => { + return Err(PodValidationError { + msg: format!( + "Unable to convert command for container [{}] to utf8.", + container.name() + ), + }) + } + }; + binary.replace_range(.., &binary_with_path); + } + + // Create template data to be used when rendering template strings + let template_data = if let Ok(data) = CreatingConfig::create_render_data(&pod_state) { + data + } else { + error!("Unable to parse directories for command template as UTF8"); + return Err(PodValidationError { + msg: format!( + "Unable to parse directories for command template as UTF8 for container [{}].", + container.name() + ), + }); + }; + + // Append values from args array to command array + // This is necessary as we only have the ExecStart field in a systemd service unit. + // There is no specific place to put arguments separate from the command. + if let Some(mut args) = container.args().clone() { + debug!( + "Appending arguments [{:?}] to command for [{}]", + args, + container.name() + ); + command.append(args.as_mut()); + } + + // Replace variables in command array + let command_render_result = command + .iter() + .map(|command_part| { + CreatingConfig::render_config_template(&template_data, command_part) + }) + .collect::, StackableError>>()?; + + trace!( + "Command after replacing variables and adding packageroot: {:?}", + command_render_result + ); + + Ok(command_render_result.join(" ")) + } +} From bf62c435b93870d171b8b5b8b45dc4f89614261e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Jan 2021 15:28:42 +0100 Subject: [PATCH 02/17] Weird changes to cargo.lock --- Cargo.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e044f59..5f34f80 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2343,10 +2343,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dec448bc157977efdc0a71369cf923915b0c4806b1b2449c3fb011071d6f7c38" dependencies = [ "cfg-if 0.1.10", - "proc-macro2", - "quote", + "proc-macro2 1.0.24", + "quote 1.0.7", "rustc_version", - "syn", + "syn 1.0.50", ] [[package]] From 27f6e90e21307915e42a6ec19470473965a4b3cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 2 Feb 2021 14:35:33 +0100 Subject: [PATCH 03/17] Working version with clippy silent. --- src/bin/agent.rs | 7 +- src/config/mod.rs | 23 ++ src/provider/mod.rs | 29 ++- .../repository/stackablerepository.rs | 2 + src/provider/states.rs | 2 - src/provider/states/creating_config.rs | 6 +- src/provider/states/creating_service.rs | 42 +++- src/provider/states/downloading.rs | 4 +- src/provider/states/running.rs | 26 ++- src/provider/states/starting.rs | 162 +------------- src/provider/states/stopped.rs | 34 --- src/provider/states/stopping.rs | 34 --- src/provider/states/terminated.rs | 33 +-- src/provider/systemdmanager/manager.rs | 209 +++++++++++++----- src/provider/systemdmanager/service.rs | 64 +++--- 15 files changed, 302 insertions(+), 375 deletions(-) delete mode 100644 src/provider/states/stopped.rs delete mode 100644 src/provider/states/stopping.rs diff --git a/src/bin/agent.rs b/src/bin/agent.rs index 74f8641..bd03d8d 100644 --- a/src/bin/agent.rs +++ b/src/bin/agent.rs @@ -57,7 +57,7 @@ async fn main() -> anyhow::Result<()> { info!("args: {:?}", env::args()); let server_config = ServerConfig { - addr: agent_config.server_ip_address.clone(), + addr: agent_config.server_ip_address, port: agent_config.server_port, cert_file: agent_config.server_cert_file.unwrap_or_default(), private_key_file: agent_config.server_key_file.unwrap_or_default(), @@ -68,8 +68,8 @@ async fn main() -> anyhow::Result<()> { hostname: agent_config.hostname.clone(), node_name: agent_config.hostname, server_config, - data_dir: agent_config.data_directory, - plugins_dir: Default::default(), + data_dir: agent_config.data_directory.clone(), + plugins_dir: agent_config.data_directory.join("plugins"), node_labels: agent_config.tags, // TODO: Discuss whether we want this configurable or leave it at a high number for now max_pods: 110, @@ -87,6 +87,7 @@ async fn main() -> anyhow::Result<()> { agent_config.parcel_directory.clone(), agent_config.config_directory.clone(), agent_config.log_directory.clone(), + agent_config.session, ) .await .expect("Error initializing provider."); diff --git a/src/config/mod.rs b/src/config/mod.rs index ab7923b..1ccf878 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -33,6 +33,7 @@ pub struct AgentConfig { pub server_cert_file: Option, pub server_key_file: Option, pub tags: HashMap, + pub session: bool, } impl AgentConfig { @@ -158,6 +159,16 @@ impl AgentConfig { list: true }; + pub const SESSION_SYSTEMD: ConfigOption = ConfigOption { + name: "session", + default: None, + required: false, + takes_argument: false, + help: "When specified causes the agent to run services in the session instance of systemd, not the system wide systemd.", + documentation: "", + list: false + }; + fn get_options() -> HashSet { [ AgentConfig::HOSTNAME, @@ -172,6 +183,7 @@ impl AgentConfig { AgentConfig::NO_CONFIG, AgentConfig::TAG, AgentConfig::BOOTSTRAP_FILE, + AgentConfig::SESSION_SYSTEMD, ] .iter() .cloned() @@ -248,6 +260,7 @@ impl AgentConfig { /// This tries to find the first non loopback interface with an ip address assigned. /// This should usually be the default interface according to: /// + /// /// https://docs.rs/pnet/0.27.2/pnet/datalink/fn.interfaces.html fn get_default_ipaddress() -> Option { let all_interfaces = datalink::interfaces(); @@ -428,6 +441,15 @@ impl Configurable for AgentConfig { } } + // The first unwrap defaults to none in case the option is not se + + let final_session = parsed_values + .get(&AgentConfig::SESSION_SYSTEMD) + .expect( + "No value for session parameter found in parsed values, this should not happen!", + ) + .is_some(); + // Panic if we encountered any errors during parsing of the values if !error_list.is_empty() { panic!( @@ -454,6 +476,7 @@ impl Configurable for AgentConfig { server_cert_file: final_server_cert_file, server_key_file: final_server_key_file, tags: final_tags, + session: final_session, }) } } diff --git a/src/provider/mod.rs b/src/provider/mod.rs index 0afc16e..5f044ed 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -1,7 +1,6 @@ use std::convert::TryFrom; use std::fs; use std::path::PathBuf; -use std::process::Child; use anyhow::anyhow; use k8s_openapi::apiextensions_apiserver::pkg::apis::apiextensions::v1::CustomResourceDefinition; @@ -22,14 +21,14 @@ use crate::provider::states::downloading::Downloading; use crate::provider::states::terminated::Terminated; use crate::provider::systemdmanager::manager::Manager; use kube::error::ErrorResponse; -use std::sync::{Arc, RwLock}; pub struct StackableProvider { client: Client, parcel_directory: PathBuf, config_directory: PathBuf, log_directory: PathBuf, - systemd_manager: Arc>, + // systemd_manager: Arc>, + session: bool, } pub const CRDS: &[&str] = &["repositories.stable.stackable.de"]; @@ -49,8 +48,7 @@ pub struct PodState { service_name: String, service_uid: String, package: Package, - process_handle: Option, - systemd_manager: Arc>, + systemd_manager: Manager, } impl PodState { @@ -80,18 +78,19 @@ impl StackableProvider { parcel_directory: PathBuf, config_directory: PathBuf, log_directory: PathBuf, + session: bool, ) -> Result { - let systemdmanager = RwLock::new(Manager::new( + /* let systemdmanager = RwLock::new(Manager::new( PathBuf::from("/home/sliebau/IdeaProjects/agent/work/systemd"), - false, - )); + session, + ));*/ let provider = StackableProvider { client, parcel_directory, config_directory, log_directory, - systemd_manager: Arc::new(systemdmanager), + session, }; let missing_crds = provider.check_crds().await?; return if missing_crds.is_empty() { @@ -190,6 +189,7 @@ impl Provider for StackableProvider { let download_directory = parcel_directory.join("_download"); let config_directory = self.config_directory.clone(); let log_directory = self.log_directory.clone(); + let session = self.session; let package = Self::get_package(pod)?; if !(&download_directory.is_dir()) { @@ -199,6 +199,12 @@ impl Provider for StackableProvider { fs::create_dir_all(&config_directory)?; } + // TODO: investigate if we can share one DBus connection across all pods + let systemd_manager = Manager::new( + PathBuf::from("/home/sliebau/IdeaProjects/agent/work/systemd"), + session, + ); + Ok(PodState { client: self.client.clone(), parcel_directory, @@ -209,8 +215,9 @@ impl Provider for StackableProvider { service_name: String::from(service_name), service_uid, package, - process_handle: None, - systemd_manager: self.systemd_manager.clone(), + // TODO: Check if we can work with a reference or a Mutex Guard here to only keep + // one connection open to DBus instead of one per tracked Pod + systemd_manager, }) } diff --git a/src/provider/repository/stackablerepository.rs b/src/provider/repository/stackablerepository.rs index 95a13d3..51865d1 100644 --- a/src/provider/repository/stackablerepository.rs +++ b/src/provider/repository/stackablerepository.rs @@ -51,6 +51,8 @@ struct StackablePackage { } impl StackableRepoProvider { + // This is only used in a test case and hence warned about as dead code + #[allow(dead_code)] pub fn new(name: String, base_url: String) -> Result { let base_url = Url::parse(&base_url)?; diff --git a/src/provider/states.rs b/src/provider/states.rs index fe97857..2ee58ec 100644 --- a/src/provider/states.rs +++ b/src/provider/states.rs @@ -11,8 +11,6 @@ pub(crate) mod installing; pub(crate) mod running; pub(crate) mod setup_failed; pub(crate) mod starting; -pub(crate) mod stopped; -pub(crate) mod stopping; pub(crate) mod terminated; pub(crate) mod waiting_config_map; diff --git a/src/provider/states/creating_config.rs b/src/provider/states/creating_config.rs index c733edc..152fdc0 100644 --- a/src/provider/states/creating_config.rs +++ b/src/provider/states/creating_config.rs @@ -1,7 +1,7 @@ use std::collections::{BTreeMap, HashMap}; use std::fs; use std::fs::read_to_string; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use handlebars::Handlebars; use k8s_openapi::api::core::v1::ConfigMap; @@ -157,7 +157,7 @@ impl CreatingConfig { /// fn apply_config_map( map: &ConfigMap, - target_directory: &PathBuf, + target_directory: &Path, template_data: &BTreeMap, ) -> Result<(), StackableError> { if map.metadata.name.is_none() { @@ -219,7 +219,7 @@ impl CreatingConfig { Ok(()) } - fn needs_update(target_file: &PathBuf, content: &str) -> Result { + fn needs_update(target_file: &Path, content: &str) -> Result { if target_file.is_file() { let current_content = read_to_string(target_file)?; debug!("Compared config file {:?} with result of", target_file); diff --git a/src/provider/states/creating_service.rs b/src/provider/states/creating_service.rs index 1e49773..e652f99 100644 --- a/src/provider/states/creating_service.rs +++ b/src/provider/states/creating_service.rs @@ -5,8 +5,10 @@ use log::{debug, error, info}; use crate::provider::states::setup_failed::SetupFailed; use crate::provider::states::starting::Starting; +use crate::provider::systemdmanager::manager::UnitTypes; use crate::provider::systemdmanager::service::Service; use crate::provider::PodState; +use anyhow::anyhow; use std::fs::create_dir_all; #[derive(Default, Debug, TransitionTo)] @@ -16,6 +18,7 @@ pub struct CreatingService; #[async_trait::async_trait] impl State for CreatingService { async fn next(self: Box, pod_state: &mut PodState, pod: &Pod) -> Transition { + let service_name: &str = pod_state.service_name.as_ref(); info!( "Creating service unit for service {}", &pod_state.service_name @@ -43,16 +46,37 @@ impl State for CreatingService { } }; - /*match service.write_unit_files(service_directory) { - Ok(()) => Transition::next(self, Starting), - Err(error) => { - error!( - "Failed to write service units for pod [{}], aborting.", - pod_state.service_name - ); - return Transition::Complete(Err(anyhow::Error::from(error))); + for unit in service.systemd_units { + let target_file = match service_directory + .join(service_name) + .into_os_string() + .into_string() + { + Ok(path) => path, + Err(_) => { + // TODO: output proper error message with information + return Transition::Complete(Err(anyhow!( + "Failed to convert path for service unit file [{}]", + service_name + ))); + } + }; + + match pod_state + .systemd_manager + .load(&target_file, &unit, UnitTypes::Service) + { + Ok(()) => {} + Err(e) => { + // TODO: We need to discuss what to do here, in theory we could have loaded + // other services already, do we want to stop those? + error!("Failed to load service unit for service [{}]", service_name); + return Transition::Complete(Err(e)); + } } - }*/ + } + + // All services were loaded successfully, otherwise we'd have returned early above Transition::next(self, Starting) } diff --git a/src/provider/states/downloading.rs b/src/provider/states/downloading.rs index 23bb2cb..964a9f7 100644 --- a/src/provider/states/downloading.rs +++ b/src/provider/states/downloading.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::Path; use kubelet::pod::Pod; use kubelet::state::prelude::*; @@ -16,7 +16,7 @@ use crate::provider::PodState; pub struct Downloading; impl Downloading { - fn package_downloaded>(package: T, download_directory: &PathBuf) -> bool { + fn package_downloaded>(package: T, download_directory: &Path) -> bool { let package = package.into(); let package_file_name = download_directory.join(package.get_file_name()); debug!( diff --git a/src/provider/states/running.rs b/src/provider/states/running.rs index b53522f..6982efb 100644 --- a/src/provider/states/running.rs +++ b/src/provider/states/running.rs @@ -9,13 +9,12 @@ use log::{debug, error, trace}; use crate::provider::states::failed::Failed; use crate::provider::states::installing::Installing; use crate::provider::states::make_status_with_containers_and_condition; -use crate::provider::states::stopping::Stopping; use crate::provider::PodState; use k8s_openapi::apimachinery::pkg::apis::meta::v1::Time; use k8s_openapi::chrono; #[derive(Debug, TransitionTo)] -#[transition_to(Stopping, Failed, Running, Installing)] +#[transition_to(Failed, Running, Installing)] pub struct Running { pub transition_time: Time, } @@ -42,6 +41,23 @@ impl State for Running { } } + match pod_state.systemd_manager.is_active(&pod_state.service_name) { + Ok(true) => continue, + Ok(false) => { + return Transition::next( + self, + Failed { + message: "Service stopped unexpectedly".to_string(), + }, + ) + } + Err(e) => error!( + "Unable to check status of service [{}] due to: [{}]", + pod_state.service_name, e + ), + } + } + /* // Obtain a mutable reference to the process handle let child = if let Some(testproc) = pod_state.process_handle.as_mut() { testproc @@ -75,6 +91,7 @@ impl State for Running { } } } + */ } async fn json_status( @@ -88,14 +105,13 @@ impl State for Running { }; let container = &pod.containers()[0]; - let mut container_status = vec![]; - container_status.push(KubeContainerStatus { + let container_status = vec![KubeContainerStatus { name: container.name().to_string(), ready: true, started: Some(false), state: Some(state), ..Default::default() - }); + }]; let condition = PodCondition { last_probe_time: None, last_transition_time: Some(self.transition_time.clone()), diff --git a/src/provider/states/starting.rs b/src/provider/states/starting.rs index 58db677..f86592c 100644 --- a/src/provider/states/starting.rs +++ b/src/provider/states/starting.rs @@ -1,13 +1,7 @@ -use std::ffi::OsStr; -use std::process::{Command, Stdio}; - use kubelet::pod::Pod; use kubelet::state::prelude::*; use kubelet::state::{State, Transition}; -use log::{debug, error, info, trace}; -use tokio::time::Duration; -use crate::provider::states::creating_config::CreatingConfig; use crate::provider::states::failed::Failed; use crate::provider::states::running::Running; use crate::provider::states::setup_failed::SetupFailed; @@ -19,158 +13,16 @@ pub struct Starting; #[async_trait::async_trait] impl State for Starting { - async fn next(self: Box, pod_state: &mut PodState, pod: &Pod) -> Transition { - let container = pod.containers()[0].clone(); - let template_data = if let Ok(data) = CreatingConfig::create_render_data(&pod_state) { - data - } else { - error!("Unable to parse directories for command template as UTF8"); - return Transition::next( + async fn next(self: Box, pod_state: &mut PodState, _: &Pod) -> Transition { + return match pod_state.systemd_manager.start(&pod_state.service_name) { + Ok(()) => Transition::next( self, - SetupFailed { - message: "DirectoryParseError".to_string(), + Running { + ..Default::default() }, - ); + ), + Err(e) => Transition::Complete(Err(e)), }; - if let Some(mut command) = container.command().clone() { - // We need to reverse the vec here, because pop works on the wrong "end" of - // the vec for our purposes - debug!("Reversing {:?}", &command); - command.reverse(); - debug!("Processing {:?}", &command); - if let Some(binary) = command.pop() { - let binary = pod_state - .parcel_directory - .join(pod_state.package.clone().get_directory_name()) - .join(binary); - - let binary = OsStr::new(&binary); - command.reverse(); - - let os_args: Vec = command - .iter() - .map(|s| CreatingConfig::render_config_template(&template_data, s).unwrap()) - .collect(); - - debug!( - "Starting command: {:?} with arguments {:?}", - binary, os_args - ); - - // Check if environment variables are set on the container - if some are present - // we render all values as templates to replace configroot, packageroot and logroot - // directories in case they are referenced in the values - // - // If even one of these renderings fails the entire pod will be failed and - // transitioned to a complete state with the error that occurred. - // If all renderings work, the vec<(String,String)> is returned as value and used - // later when starting the process - // This works because Result implements - // (FromIterator)[https://doc.rust-lang.org/std/result/enum.Result.html#method.from_iter] - // which returns a Result that is Ok(..) if none of the internal results contained - // an Error. If any error occurred, iteration stops on the first error and returns - // that in the outer result. - let env_variables = if let Some(vars) = container.env() { - debug!( - "Got environment vars: {:?} service {}", - vars, pod_state.service_name - ); - let render_result = vars - .iter() - .map(|env_var| { - // Replace variables in value - CreatingConfig::render_config_template( - &template_data, - &env_var.value.as_deref().unwrap_or_default(), - ) - .map(|value| (&env_var.name, value)) - }) - .collect(); - - // If any single rendering failed, the overall result for the map will have - // collected the Err which we can check for here - match render_result { - Ok(rendered_values) => rendered_values, - Err(error) => { - error!("Failed to render value for env var due to: {:?}", error); - return Transition::Complete(Err(anyhow::Error::from(error))); - } - } - } else { - // No environment variables present for this container -> empty vec - debug!( - "No environment vars set for service {}", - pod_state.service_name - ); - vec![] - }; - debug!( - "Setting environment for service {} to {:?}", - pod_state.service_name, &env_variables - ); - - let start_result = Command::new(binary) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .envs(env_variables) - .args(&os_args) - .spawn(); - - match start_result { - Ok(mut child) => { - info!( - "Successfully executed command \"{:?}\" with args {:?}", - binary, &os_args - ); - - debug!("Waiting if startup fails.."); - for i in 1..10 { - tokio::time::delay_for(Duration::from_secs(1)).await; - if let Ok(None) = child.try_wait() { - trace!("Process still alive after {} seconds ..", i); - } else { - error!( - "Process died {} after {} seconds during startup!", - pod_state.service_name, i - ); - return Transition::next( - self, - Failed { - message: "ProcessFailedDuringStartup".to_string(), - }, - ); - } - } - // Store the child handle in the podstate so that later states - // can use it - pod_state.process_handle = Some(child); - return Transition::next( - self, - Running { - ..Default::default() - }, - ); - } - Err(error) => { - let error_message = format!("Failed to start process with error {}", error); - error!("{}", error_message); - return Transition::next( - self, - Failed { - message: "ProcessStartFailed".to_string(), - }, - ); - } - } - } - } - error!("No command found, not starting anything.."); - return Transition::next( - self, - Failed { - message: "MissingCommandObject".to_string(), - }, - ); } async fn json_status( diff --git a/src/provider/states/stopped.rs b/src/provider/states/stopped.rs deleted file mode 100644 index f0a9538..0000000 --- a/src/provider/states/stopped.rs +++ /dev/null @@ -1,34 +0,0 @@ -use kubelet::pod::Pod; -use kubelet::state::prelude::*; -use kubelet::state::{State, Transition}; -use log::info; -use tokio::time::Duration; - -use crate::provider::states::starting::Starting; -use crate::provider::PodState; - -#[derive(Default, Debug, TransitionTo)] -#[transition_to(Starting)] -pub struct Stopped; - -#[async_trait::async_trait] -impl State for Stopped { - async fn next(self: Box, pod_state: &mut PodState, _pod: &Pod) -> Transition { - let delay = Duration::from_secs(2); - info!( - "Service {} stopped, waiting {} seconds before restart.", - pod_state.service_name, - delay.as_secs() - ); - tokio::time::delay_for(delay).await; - Transition::next(self, Starting) - } - - async fn json_status( - &self, - _pod_state: &mut PodState, - _pod: &Pod, - ) -> anyhow::Result { - make_status(Phase::Pending, &"Stopped") - } -} diff --git a/src/provider/states/stopping.rs b/src/provider/states/stopping.rs deleted file mode 100644 index 65d5a4a..0000000 --- a/src/provider/states/stopping.rs +++ /dev/null @@ -1,34 +0,0 @@ -use kubelet::pod::Pod; -use kubelet::state::prelude::*; -use kubelet::state::{State, Transition}; -use log::info; - -use crate::provider::states::failed::Failed; -use crate::provider::states::stopped::Stopped; -use crate::provider::PodState; - -#[derive(Default, Debug, TransitionTo)] -#[transition_to(Stopped, Failed)] -pub struct Stopping; - -#[async_trait::async_trait] -impl State for Stopping { - async fn next(self: Box, pod_state: &mut PodState, _pod: &Pod) -> Transition { - if let Some(child) = &pod_state.process_handle { - info!( - "Received stop command for service {}, stopping process with pid {}", - pod_state.service_name, - child.id() - ); - } - Transition::next(self, Stopped) - } - - async fn json_status( - &self, - _pod_state: &mut PodState, - _pod: &Pod, - ) -> anyhow::Result { - make_status(Phase::Pending, &"Stopping") - } -} diff --git a/src/provider/states/terminated.rs b/src/provider/states/terminated.rs index 528d214..64a76f9 100644 --- a/src/provider/states/terminated.rs +++ b/src/provider/states/terminated.rs @@ -1,7 +1,7 @@ -use anyhow::anyhow; use kubelet::state::prelude::*; -use log::{debug, error, info}; +use log::info; +use crate::provider::systemdmanager::manager::UnitTypes; use crate::provider::PodState; #[derive(Default, Debug)] @@ -17,26 +17,17 @@ impl State for Terminated { "Pod {} was terminated, stopping process!", &pod_state.service_name ); - // Obtain a mutable reference to the process handle - let child = if let Some(testproc) = pod_state.process_handle.as_mut() { - testproc - } else { - return Transition::Complete(Err(anyhow!("Unable to retrieve process handle"))); - }; - return match child.kill() { - Ok(()) => { - debug!("Successfully killed process {}", pod_state.service_name); - Transition::Complete(Ok(())) - } - Err(e) => { - error!( - "Failed to stop process with pid {} due to: {:?}", - child.id(), - e - ); - Transition::Complete(Err(anyhow::Error::new(e))) - } + // TODO: this can be written in a nicer way with .map() I think + return match pod_state.systemd_manager.stop(&pod_state.service_name) { + Ok(()) => match pod_state + .systemd_manager + .unload(&pod_state.service_name, UnitTypes::Service) + { + Ok(()) => Transition::Complete(Ok(())), + Err(e) => Transition::Complete(Err(e)), + }, // TODO: make nicer, but due to early return + Err(e) => Transition::Complete(Err(e)), }; } diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index 04589b1..e9c101a 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -1,21 +1,21 @@ //* A systemd unit manager that allows managing systemd services use crate::provider::systemdmanager::service::SystemDUnit; +use anyhow::anyhow; use dbus::blocking::SyncConnection; +use dbus::Path; use log::{debug, error, info}; -use serde::de::Error; use std::fs::File; use std::io::Write; use std::path::PathBuf; use std::time::Duration; -enum UnitTypes { +pub enum UnitTypes { Service, } pub struct Manager { units_directory: PathBuf, - user_mode: bool, systemd_connection: SystemDConnection, } @@ -23,29 +23,109 @@ impl Manager { pub fn new(units_directory: PathBuf, user_mode: bool) -> Self { Manager { units_directory, - user_mode, - systemd_connection: SystemDConnection::new(), // Unused at present + systemd_connection: SystemDConnection::new(user_mode), } } // Write the unit file to disk and enable the service // TODO: should we maybe split enable out into a different function? - pub fn load(&self, name: &str, unit: SystemDUnit) -> Result<(), anyhow::Error> { - let target_file = self - .units_directory - .join(Manager::get_unit_file_path(name, UnitTypes::Service)?); + pub fn load( + &self, + name: &str, + unit: &SystemDUnit, + unit_type: UnitTypes, + ) -> Result<(), anyhow::Error> { + // Appends .service to name if necessary + let unit_name = Manager::get_unit_file_name(name, unit_type)?; + + let unit_path = PathBuf::from(&unit_name); + + // Check if an absolute path was provided + // if this is an absolute path the file will be written there, otherwise + // the file will be created in the unit directory + let target_file = if unit_path.is_absolute() { + unit_path + } else { + self.units_directory.join(unit_path) + }; + + //let target_file = self.units_directory.join(&unit_name); debug!("Target file for service {} : {:?}", name, target_file); - let mut unit_file = File::create(&target_file)?; - unit_file.write(unit.get_unit_file_content().as_bytes()); - unit_file.flush(); + let mut unit_file = match File::create(&target_file) { + Ok(file) => file, + Err(e) => { + error!("Error ocurred when creating unit file [{}]: [{}]", name, e); + return Err(anyhow::Error::from(e)); + } + }; + + unit_file.write_all(unit.get_unit_file_content().as_bytes())?; + unit_file.flush()?; + self.systemd_connection - .enable_unit_file(&target_file.into_os_string().to_string_lossy()); + .enable_unit_file(&target_file.into_os_string().to_string_lossy())?; + self.reload()?; + Ok(()) } + // Stop and disable the service, then delete the unit file from disk + pub fn unload(&self, name: &str, unit_type: UnitTypes) -> Result<(), anyhow::Error> { + // Appends .service to name if necessary + let unit_name = Manager::get_unit_file_name(name, unit_type)?; + + self.systemd_connection + .disable_unit_file(&unit_name) + .map_err(anyhow::Error::from) + } + + pub fn start(&self, name: &str) -> Result<(), anyhow::Error> { + let unit_name = Manager::get_unit_file_name(name, UnitTypes::Service)?; + match self.systemd_connection.start_unit(&unit_name) { + Ok(result) => info!("Successfully started service [{}]: [{}]", unit_name, result), + Err(e) => { + error!("Error: [{}]", e); + return Err(anyhow!("Error starting service [{}]: {}", name, e)); + } + }; + Ok(()) + } + + pub fn stop(&self, name: &str) -> Result<(), anyhow::Error> { + let unit_name = Manager::get_unit_file_name(name, UnitTypes::Service)?; + match self.systemd_connection.stop_unit(&unit_name) { + Ok(result) => info!("Successfully stopped service [{}]: [{}]", unit_name, result), + Err(e) => { + error!("Error: [{}]", e); + return Err(anyhow!("Error stopping service [{}]: {}", name, e)); + } + }; + Ok(()) + } + + /// TODO: Make this actually do something, currently it only returns true + #[allow(unused_variables)] + pub fn is_active(&self, name: &str) -> Result { + /*let unit_name = Manager::get_unit_file_name(name, UnitTypes::Service)?; + match self.systemd_connection.get_unit_status(&unit_name) { + Ok(result) => debug!( + "Successfully retrieved state for unit [{}]: [{}]", + unit_name, result + ), + Err(e) => { + error!("Error: [{}]", e); + return Err(anyhow!("Error getting unit state [{}]: {}", name, e)); + } + };*/ + Ok(true) + } + // Check if the unit name is valid and append .service if needed - fn get_unit_file_path(name: &str, unit_type: UnitTypes) -> Result { + // Cannot currently fail, I'll need to dig into what is a valid unit + // name before adding checks + #[allow(clippy::unnecessary_wraps)] + fn get_unit_file_name(name: &str, unit_type: UnitTypes) -> Result { // TODO: what are valid systemd unit names? // Append proper extension for unit type to file name @@ -63,12 +143,14 @@ impl Manager { } } - // Stop and disable the service, then delete the unit file from disk - pub fn unload() -> Result<(), anyhow::Error> { - Ok(()) - } - - pub fn reload() -> Result<(), anyhow::Error> { + pub fn reload(&self) -> Result<(), anyhow::Error> { + match self.systemd_connection.reload() { + Ok(_) => info!("Successfully performed daemon-reload"), + Err(e) => { + error!("Error: [{}]", e); + return Err(anyhow!("Error performing daemon-reload: [{}]", e)); + } + }; Ok(()) } } @@ -82,10 +164,15 @@ struct SystemDConnection { } impl SystemDConnection { - fn new() -> SystemDConnection { - let connection = SyncConnection::new_system().expect("D-Bus connection failed"); + fn new(user_mode: bool) -> SystemDConnection { + let connection = if user_mode { + SyncConnection::new_session().expect("Session D-Bus connection failed") + } else { + SyncConnection::new_system().expect("System D-Bus connection failed") + }; + SystemDConnection { - connection: connection, + connection, dest: "org.freedesktop.systemd1", node: "/org/freedesktop/systemd1", interface: "org.freedesktop.systemd1.Manager", @@ -93,34 +180,45 @@ impl SystemDConnection { } } + pub fn reload(&self) -> Result<(), dbus::Error> { + let proxy = self + .connection + .with_proxy(self.dest, self.node, self.timeout); + proxy + .method_call(self.interface, "Reload", ()) + .map(|_: ()| ()) + } + /// Takes a unit name as input and attempts to start it. - pub fn start_unit(&self, unit: &str) -> Result { + pub fn start_unit(&self, unit: &str) -> Result { // create a wrapper struct around the connection that makes it easy // to send method calls to a specific destination and path. + info!("Attempting to start unit {}", unit); let proxy = self .connection .with_proxy(self.dest, self.node, self.timeout); proxy .method_call(self.interface, "StartUnit", (unit, "fail")) - .and_then(|r: (u32,)| Ok(r.0)) + .map(|r: (Path,)| r.0) } /// Takes a unit name as input and attempts to stop it. - pub fn stop_unit(&self, unit: &str) -> Result { + pub fn stop_unit(&self, unit: &str) -> Result { // create a wrapper struct around the connection that makes it easy // to send method calls to a specific destination and path. + info!("Attempting to stop unit {}", unit); let proxy = self .connection .with_proxy(self.dest, self.node, self.timeout); proxy .method_call(self.interface, "StopUnit", (unit, "fail")) - .and_then(|r: (u32,)| Ok(r.0)) + .map(|r: (Path,)| r.0) } - /// THIS WORKS /// Takes the unit pathname of a service and enables it via dbus. /// If dbus replies with `[Bool(true), Array([], "(sss)")]`, the service is already enabled. - pub fn enable_unit_file(&self, unit: &str) -> Option { + pub fn enable_unit_file(&self, unit: &str) -> Result<(), dbus::Error> { + debug!("Enabling unit from file [{}]", unit); let proxy = self .connection .with_proxy(self.dest, self.node, self.timeout); @@ -133,37 +231,32 @@ impl SystemDConnection { ); match result { Ok(reply) => { - let (s) = reply; - println!("{:?}", s); - /* if format!("{:?}", reply.get_items()) == "[Bool(true), Array([], \"(sss)\")]" { - println!("{} already enabled", unit); - } else { - println!("{} has been enabled", unit); - }*/ - None + let s = reply; + info!("Successfully loaded unit [{}] with result [{:?}]", unit, s); + Ok(()) } - Err(reply) => { - let error = format!("Error enabling {}:\n{:?}", unit, reply); - println!("{}", error); - Some(error) + Err(e) => { + error!("Error enabling unit [{}]", unit); + Err(e) } } } -} -/* - Disable(name string) error - Load(name string, u unit.File) error - Mask(name string) error - Properties(name string) (map[string]interface{}, error) - Property(name, property string) string - Reload() error - ServiceProperty(name, property string) string - State(name string) (*unit.State, error) - States(prefix string) (map[string]*unit.State, error) - TriggerStart(name string) error - TriggerStop(name string) error - Unit(name string) string - Units() ([]string, error) - Unload(name string) error -*/ + // TODO: this doesn't work yet, the symlink is not deleted for some reason + // I'll need to investigate this + pub fn disable_unit_file(&self, unit: &str) -> Result<(), dbus::Error> { + let proxy = self + .connection + .with_proxy(self.dest, self.node, self.timeout); + let force = true; + let result: Result<(Vec,), dbus::Error> = + proxy.method_call(self.interface, "DisableUnitFiles", (&[unit][..], force)); + + match &result { + Ok(_) => info!("Successfully disabled service!"), + Err(e) => error!("Error disabling service: [{}]", e), + } + + result.map(|_| ()) + } +} diff --git a/src/provider/systemdmanager/service.rs b/src/provider/systemdmanager/service.rs index c616e58..41a24a6 100644 --- a/src/provider/systemdmanager/service.rs +++ b/src/provider/systemdmanager/service.rs @@ -1,7 +1,4 @@ use std::collections::HashMap; -use std::fs::File; -use std::io::Write; -use std::path::PathBuf; use kubelet::container::Container; use kubelet::pod::Pod; @@ -11,11 +8,8 @@ use crate::provider::error::StackableError; use crate::provider::error::StackableError::PodValidationError; use crate::provider::states::creating_config::CreatingConfig; -use crate::provider::states::setup_failed::SetupFailed; use crate::provider::PodState; -use kubelet::state::Transition; -use log::{debug, error, info, trace, warn}; -use url::form_urlencoded::Target; +use log::{debug, error, trace, warn}; static RESTART_POLICY_MAP: phf::Map<&'static str, &'static str> = phf_map! { "Always" => "always", @@ -23,15 +17,14 @@ static RESTART_POLICY_MAP: phf::Map<&'static str, &'static str> = phf_map! { "Never" => "no", }; +// TODO: This will be used later to ensure the same ordering of known sections in +// unit files, I'll leave it in for now +#[allow(dead_code)] static SECTION_ORDER: phf::OrderedSet<&'static str> = phf_ordered_set! {"Unit", "Service", "Install"}; -struct Section { - name: &'static str, -} - pub struct Service { - systemd_units: Vec, + pub systemd_units: Vec, } impl Service { @@ -50,14 +43,6 @@ impl Service { systemd_units: units, }) } - - pub fn start_all(&self) -> Result<(), StackableError> { - Ok(()) - } - - pub fn stop_all(&self) -> Result<(), StackableError> { - Ok(()) - } } #[derive(Clone)] @@ -68,12 +53,15 @@ pub struct SystemDUnit { } impl SystemDUnit { + /// Create a new unit which inherits all common elements from ['common_properties'] and parses + /// everything else from the ['container'] + pub fn new( - unit_template: &SystemDUnit, + common_properties: &SystemDUnit, container: &Container, pod_state: &PodState, ) -> Result { - let mut unit = unit_template.clone(); + let mut unit = common_properties.clone(); unit.name = String::from(container.name()); unit.add_property("Unit", "Description", &unit.name.clone()); @@ -90,8 +78,9 @@ impl SystemDUnit { unit.add_env_var(&name, &value); } - unit.add_property("Service", "StandardOutput", "Journal"); - unit.add_property("Service", "StandardError", "Journal"); + unit.add_property("Service", "StandardOutput", "journal"); + unit.add_property("Service", "StandardError", "journal"); + unit.add_property("Install", "WantedBy", "multi-user.target"); Ok(unit) } @@ -121,7 +110,7 @@ impl SystemDUnit { let section = self .sections .entry(String::from(section)) - .or_insert(HashMap::new()); + .or_insert_with(HashMap::new); section.insert(String::from(key), String::from(value)); } @@ -145,7 +134,7 @@ impl SystemDUnit { unit_file_content.push_str(&format!("Environment=\"{}={}\"\n", name, value)); } } - unit_file_content.push_str("\n"); + unit_file_content.push('\n'); } unit_file_content } @@ -275,18 +264,17 @@ impl SystemDUnit { package_root, container.name() ); - let mut binary_with_path = - match package_root.join(&binary).into_os_string().into_string() { - Ok(path_string) => path_string, - Err(original_string) => { - return Err(PodValidationError { - msg: format!( - "Unable to convert command for container [{}] to utf8.", - container.name() - ), - }) - } - }; + let binary_with_path = match package_root.join(&binary).into_os_string().into_string() { + Ok(path_string) => path_string, + Err(_) => { + return Err(PodValidationError { + msg: format!( + "Unable to convert command for container [{}] to utf8.", + container.name() + ), + }) + } + }; binary.replace_range(.., &binary_with_path); } From 352abeffa63c3b9538b52f4e4248cb7ba4a98148 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 2 Feb 2021 14:39:44 +0100 Subject: [PATCH 04/17] Added documentation for session parameter. --- src/config/config_documentation/session.adoc | 7 +++++++ src/config/mod.rs | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 src/config/config_documentation/session.adoc diff --git a/src/config/config_documentation/session.adoc b/src/config/config_documentation/session.adoc new file mode 100644 index 0000000..a84feab --- /dev/null +++ b/src/config/config_documentation/session.adoc @@ -0,0 +1,7 @@ +This parameter specifies whether to use a session or the system DBus connection when talking to systemd. + +When this flag is used symlink for loaded services will be created in the currently active users home directory and no global changes made to the system. + +The default is to use the system bus, which is the machine wide systemd instance. + +This will mainly be useful for scenarios where no root access is possible and for testing. \ No newline at end of file diff --git a/src/config/mod.rs b/src/config/mod.rs index 1ccf878..d207672 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -165,7 +165,7 @@ impl AgentConfig { required: false, takes_argument: false, help: "When specified causes the agent to run services in the session instance of systemd, not the system wide systemd.", - documentation: "", + documentation: include_str!("config_documentation/session.adoc"), list: false }; From 99262e0763a54a4ff754ff8874f5121821fc138a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 2 Feb 2021 14:56:16 +0100 Subject: [PATCH 05/17] Added needed packages to setup of build environment. --- .github/workflows/rust.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 7ac2168..3ae6587 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -15,6 +15,7 @@ jobs: steps: - uses: actions/checkout@v2 - run: rustup update stable && rustup default stable + - run: sudo apt-get install libdbus-1-dev pkg-config libdbus-1-3 - run: rustup component add rustfmt - run: cargo fmt --all -- --check From 4344d8a7df07938663bc33f71052933e898bc568 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 2 Feb 2021 15:04:46 +0100 Subject: [PATCH 06/17] Random change to see if a fresh commit triggers an update of the PR. --- src/config/config_documentation/session.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config/config_documentation/session.adoc b/src/config/config_documentation/session.adoc index a84feab..0a91a05 100644 --- a/src/config/config_documentation/session.adoc +++ b/src/config/config_documentation/session.adoc @@ -4,4 +4,4 @@ When this flag is used symlink for loaded services will be created in the curren The default is to use the system bus, which is the machine wide systemd instance. -This will mainly be useful for scenarios where no root access is possible and for testing. \ No newline at end of file +This will mainly be useful for scenarios without root access and for testing. \ No newline at end of file From 097c65447cb7fc3fbfc79674f3e5d18d35912542 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 2 Feb 2021 15:09:50 +0100 Subject: [PATCH 07/17] Next try to fix github build action. --- .github/workflows/rust.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 3ae6587..689e75c 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -37,6 +37,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 + - name: Install dbus dependencies + run: sudo apt-get install libdbus-1-dev pkg-config libdbus-1-3 - name: Build run: cargo build --verbose - name: Run tests From 132268085ac9249b9ed58f1f773d68868b917135 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 2 Feb 2021 15:19:23 +0100 Subject: [PATCH 08/17] Add libdbus to clippy as well (I hope). --- .github/workflows/rust.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 689e75c..240a1a0 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -22,6 +22,8 @@ jobs: clippy_check: runs-on: ubuntu-latest steps: + - name: Install dbus dependencies + run: sudo apt-get install libdbus-1-dev pkg-config libdbus-1-3 - uses: actions/checkout@v1 - uses: actions-rs/toolchain@v1 with: From 31af2be18e62793b8c2987093b15fc9516cf2901 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 5 Feb 2021 19:15:26 +0100 Subject: [PATCH 09/17] Addressed first round of comments. --- Cargo.toml | 5 + src/config/config_documentation/session.adoc | 11 ++- src/provider/mod.rs | 30 +++--- src/provider/states/creating_service.rs | 10 +- src/provider/states/running.rs | 58 +----------- src/provider/states/terminated.rs | 19 ++-- src/provider/systemdmanager/manager.rs | 99 ++++++++++---------- src/provider/systemdmanager/service.rs | 77 ++++++++++----- 8 files changed, 157 insertions(+), 152 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d68f04e..ac68483 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,3 +37,8 @@ hostname = "0.3" indoc = "1.0" rstest = "0.6" serde_yaml = "0.8" + +[profile.release] +opt-level = "s" +lto = true +codegen-units = 1 diff --git a/src/config/config_documentation/session.adoc b/src/config/config_documentation/session.adoc index 0a91a05..ce125fb 100644 --- a/src/config/config_documentation/session.adoc +++ b/src/config/config_documentation/session.adoc @@ -1,7 +1,12 @@ This parameter specifies whether to use a session or the system DBus connection when talking to systemd. +For our purposps the difference between the two can be explained as the session bus being restricted to the current user, whereas the system bus rolls out services that are available for every user. +In reality is is a bit more involved than that, please refer to the https://dbus.freedesktop.org/doc/dbus-specification.html[official docs] for more information. -When this flag is used symlink for loaded services will be created in the currently active users home directory and no global changes made to the system. +When this flag is specified it causes symlinks for loaded services to be created in the currently active users systemd directory `~/.config/systemd/user` instead of one of the globally valid locations: -The default is to use the system bus, which is the machine wide systemd instance. +- `/lib/systemd/system` +- `/etc/systemd/system` -This will mainly be useful for scenarios without root access and for testing. \ No newline at end of file +The default is to use the system bus, for which it is necessary that the agent either run as root or have passwordless sudo rights. + +Using the session bus will mainly be useful for scenarios without root access and for testing on developer machines. \ No newline at end of file diff --git a/src/provider/mod.rs b/src/provider/mod.rs index 5f044ed..6abc2b3 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -27,7 +27,6 @@ pub struct StackableProvider { parcel_directory: PathBuf, config_directory: PathBuf, log_directory: PathBuf, - // systemd_manager: Arc>, session: bool, } @@ -66,7 +65,12 @@ impl PodState { self.log_directory.join(&self.service_name) } - // I know, I know, the name sucks! + /// Resolve the directory in which the systemd unit files will be placed for this + /// service. + /// This defaults to "{{config_root}}/_service" + /// + /// From this place the unit files will be symlinked to the relevant systemd + /// unit directories so that they are picked up by systemd. pub fn get_service_service_directory(&self) -> PathBuf { self.get_service_config_directory().join("_service") } @@ -80,11 +84,6 @@ impl StackableProvider { log_directory: PathBuf, session: bool, ) -> Result { - /* let systemdmanager = RwLock::new(Manager::new( - PathBuf::from("/home/sliebau/IdeaProjects/agent/work/systemd"), - session, - ));*/ - let provider = StackableProvider { client, parcel_directory, @@ -172,7 +171,7 @@ impl Provider for StackableProvider { } async fn initialize_pod_state(&self, pod: &Pod) -> anyhow::Result { - let service_name = pod.name(); + let service_name = format!("{}-{}", pod.namespace(), pod.name()); // Extract uid from pod object, if this fails we return an error - // this should not happen, as all objects we get from Kubernetes should have @@ -200,10 +199,17 @@ impl Provider for StackableProvider { } // TODO: investigate if we can share one DBus connection across all pods - let systemd_manager = Manager::new( - PathBuf::from("/home/sliebau/IdeaProjects/agent/work/systemd"), - session, - ); + // Depending on whether we are supposed to run in user space or system-wide + // we'll pick the default directory to initialize the systemd manager with + // This allows creating unit files either directly in the systemd folder by + // passing in just a filename, or symlink them by passing in an absolute + // path + let unit_directory = if self.session { + PathBuf::from("/lib/systemd/system") + } else { + PathBuf::from("~/.config/systemd/user") + }; + let systemd_manager = Manager::new(unit_directory, session)?; Ok(PodState { client: self.client.clone(), diff --git a/src/provider/states/creating_service.rs b/src/provider/states/creating_service.rs index e652f99..5ebba37 100644 --- a/src/provider/states/creating_service.rs +++ b/src/provider/states/creating_service.rs @@ -29,12 +29,12 @@ impl State for CreatingService { "Creating config directory for service [{}]: {:?}", pod_state.service_name, service_directory ); - match create_dir_all(service_directory) { - Ok(()) => {} - Err(error) => return Transition::Complete(Err(anyhow::Error::from(error))), + if let Err(error) = create_dir_all(service_directory) { + return Transition::Complete(Err(anyhow::Error::from(error))); } } + // This will create the service unit files on disk let service = match Service::new(pod, pod_state) { Ok(new_service) => new_service, Err(error) => { @@ -46,6 +46,8 @@ impl State for CreatingService { } }; + // Each pod can map to multiple systemd units/services as each container will get its own systemd unit file/service. + // This will iterate over all of them and enable the services. for unit in service.systemd_units { let target_file = match service_directory .join(service_name) @@ -64,7 +66,7 @@ impl State for CreatingService { match pod_state .systemd_manager - .load(&target_file, &unit, UnitTypes::Service) + .enable(&target_file, &unit, UnitTypes::Service) { Ok(()) => {} Err(e) => { diff --git a/src/provider/states/running.rs b/src/provider/states/running.rs index 6982efb..693bae3 100644 --- a/src/provider/states/running.rs +++ b/src/provider/states/running.rs @@ -4,7 +4,7 @@ use k8s_openapi::api::core::v1::{ use kubelet::pod::Pod; use kubelet::state::prelude::*; use kubelet::state::{State, Transition}; -use log::{debug, error, trace}; +use log::{debug, trace}; use crate::provider::states::failed::Failed; use crate::provider::states::installing::Installing; @@ -36,62 +36,13 @@ impl State for Running { ) -> Transition { loop { tokio::select! { - _ = tokio::time::delay_for(std::time::Duration::from_secs(1)) => { + _ = tokio::time::delay_for(std::time::Duration::from_secs(10)) => { trace!("Checking if service {} is still running.", &pod_state.service_name); } } - - match pod_state.systemd_manager.is_active(&pod_state.service_name) { - Ok(true) => continue, - Ok(false) => { - return Transition::next( - self, - Failed { - message: "Service stopped unexpectedly".to_string(), - }, - ) - } - Err(e) => error!( - "Unable to check status of service [{}] due to: [{}]", - pod_state.service_name, e - ), - } - } - /* - // Obtain a mutable reference to the process handle - let child = if let Some(testproc) = pod_state.process_handle.as_mut() { - testproc - } else { - return Transition::next( - self, - Failed { - message: "Unable to obtain process handle from podstate!".to_string(), - }, - ); - }; - - // Check if an exit code is available for the process - if yes, it exited - match child.try_wait() { - Ok(None) => debug!( - "Service {} is still running with pid {}", - &pod_state.service_name, - child.id() - ), - _ => { - error!( - "Service {} died unexpectedly, moving to failed state", - pod_state.service_name - ); - return Transition::next( - self, - Failed { - message: "ProcessDiedUnexpectedly".to_string(), - }, - ); - } - } + // TODO: We are not watching the service yet, need to subscribe to events and + // react to those } - */ } async fn json_status( @@ -105,6 +56,7 @@ impl State for Running { }; let container = &pod.containers()[0]; + // TODO: Change to support multiple containers let container_status = vec![KubeContainerStatus { name: container.name().to_string(), ready: true, diff --git a/src/provider/states/terminated.rs b/src/provider/states/terminated.rs index 64a76f9..5326348 100644 --- a/src/provider/states/terminated.rs +++ b/src/provider/states/terminated.rs @@ -18,17 +18,16 @@ impl State for Terminated { &pod_state.service_name ); - // TODO: this can be written in a nicer way with .map() I think - return match pod_state.systemd_manager.stop(&pod_state.service_name) { - Ok(()) => match pod_state - .systemd_manager - .unload(&pod_state.service_name, UnitTypes::Service) - { - Ok(()) => Transition::Complete(Ok(())), - Err(e) => Transition::Complete(Err(e)), - }, // TODO: make nicer, but due to early return + if let Err(e) = pod_state.systemd_manager.stop(&pod_state.service_name) { + return Transition::Complete(Err(e)); + } + match pod_state + .systemd_manager + .unload(&pod_state.service_name, UnitTypes::Service) + { + Ok(()) => Transition::Complete(Ok(())), Err(e) => Transition::Complete(Err(e)), - }; + } } async fn json_status( diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index e9c101a..971c10c 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -1,8 +1,10 @@ -//* A systemd unit manager that allows managing systemd services +//! A manager that allows managing systemd units use crate::provider::systemdmanager::service::SystemDUnit; use anyhow::anyhow; +use dbus::arg::{AppendAll, ReadAll}; use dbus::blocking::SyncConnection; +use dbus::strings::Member; use dbus::Path; use log::{debug, error, info}; use std::fs::File; @@ -10,6 +12,7 @@ use std::io::Write; use std::path::PathBuf; use std::time::Duration; +#[derive(Clone)] pub enum UnitTypes { Service, } @@ -20,16 +23,17 @@ pub struct Manager { } impl Manager { - pub fn new(units_directory: PathBuf, user_mode: bool) -> Self { - Manager { + pub fn new(units_directory: PathBuf, user_mode: bool) -> Result { + Ok(Manager { units_directory, - systemd_connection: SystemDConnection::new(user_mode), - } + systemd_connection: SystemDConnection::new(user_mode)?, + }) } - // Write the unit file to disk and enable the service + /// Write the unit file to disk and enable the service + /// This implicitly calls [link()] // TODO: should we maybe split enable out into a different function? - pub fn load( + pub fn enable( &self, name: &str, unit: &SystemDUnit, @@ -49,13 +53,12 @@ impl Manager { self.units_directory.join(unit_path) }; - //let target_file = self.units_directory.join(&unit_name); - debug!("Target file for service {} : {:?}", name, target_file); + debug!("Target file for service [{}] : [{:?}]", name, target_file); let mut unit_file = match File::create(&target_file) { Ok(file) => file, Err(e) => { - error!("Error ocurred when creating unit file [{}]: [{}]", name, e); + debug!("Error occurred when creating unit file [{}]: [{}]", name, e); return Err(anyhow::Error::from(e)); } }; @@ -83,42 +86,29 @@ impl Manager { pub fn start(&self, name: &str) -> Result<(), anyhow::Error> { let unit_name = Manager::get_unit_file_name(name, UnitTypes::Service)?; match self.systemd_connection.start_unit(&unit_name) { - Ok(result) => info!("Successfully started service [{}]: [{}]", unit_name, result), + Ok(result) => { + info!("Successfully started service [{}]: [{}]", unit_name, result); + Ok(()) + } Err(e) => { error!("Error: [{}]", e); - return Err(anyhow!("Error starting service [{}]: {}", name, e)); + Err(anyhow!("Error starting service [{}]: {}", name, e)) } - }; - Ok(()) + } } pub fn stop(&self, name: &str) -> Result<(), anyhow::Error> { let unit_name = Manager::get_unit_file_name(name, UnitTypes::Service)?; match self.systemd_connection.stop_unit(&unit_name) { - Ok(result) => info!("Successfully stopped service [{}]: [{}]", unit_name, result), - Err(e) => { - error!("Error: [{}]", e); - return Err(anyhow!("Error stopping service [{}]: {}", name, e)); + Ok(result) => { + info!("Successfully stopped service [{}]: [{}]", unit_name, result); + Ok(()) } - }; - Ok(()) - } - - /// TODO: Make this actually do something, currently it only returns true - #[allow(unused_variables)] - pub fn is_active(&self, name: &str) -> Result { - /*let unit_name = Manager::get_unit_file_name(name, UnitTypes::Service)?; - match self.systemd_connection.get_unit_status(&unit_name) { - Ok(result) => debug!( - "Successfully retrieved state for unit [{}]: [{}]", - unit_name, result - ), Err(e) => { error!("Error: [{}]", e); - return Err(anyhow!("Error getting unit state [{}]: {}", name, e)); + Err(anyhow!("Error stopping service [{}]: {}", name, e)) } - };*/ - Ok(true) + } } // Check if the unit name is valid and append .service if needed @@ -133,14 +123,11 @@ impl Manager { UnitTypes::Service => ".service", }; - // Todo: fugly + let mut result = String::from(name); if !name.ends_with(extension) { - let mut result = String::from(name); result.push_str(extension); - Ok(result) - } else { - Ok(String::from(name)) } + Ok(result) } pub fn reload(&self) -> Result<(), anyhow::Error> { @@ -164,20 +151,20 @@ struct SystemDConnection { } impl SystemDConnection { - fn new(user_mode: bool) -> SystemDConnection { + fn new(user_mode: bool) -> Result { let connection = if user_mode { - SyncConnection::new_session().expect("Session D-Bus connection failed") + SyncConnection::new_session()? } else { - SyncConnection::new_system().expect("System D-Bus connection failed") + SyncConnection::new_system()? }; - SystemDConnection { + Ok(SystemDConnection { connection, dest: "org.freedesktop.systemd1", node: "/org/freedesktop/systemd1", interface: "org.freedesktop.systemd1.Manager", - timeout: Duration::from_millis(5000), - } + timeout: Duration::from_secs(5), + }) } pub fn reload(&self) -> Result<(), dbus::Error> { @@ -194,14 +181,28 @@ impl SystemDConnection { // create a wrapper struct around the connection that makes it easy // to send method calls to a specific destination and path. info!("Attempting to start unit {}", unit); - let proxy = self + + /*let proxy = self .connection .with_proxy(self.dest, self.node, self.timeout); proxy .method_call(self.interface, "StartUnit", (unit, "fail")) + */ + self.method_call("StartUnit", (unit, "fail")) .map(|r: (Path,)| r.0) } + fn method_call<'m, R: ReadAll, A: AppendAll, M: Into>>( + &self, + m: M, + args: A, + ) -> Result { + let proxy = self + .connection + .with_proxy(self.dest, self.node, self.timeout); + proxy.method_call(self.interface, m, args) + } + /// Takes a unit name as input and attempts to stop it. pub fn stop_unit(&self, unit: &str) -> Result { // create a wrapper struct around the connection that makes it easy @@ -231,8 +232,10 @@ impl SystemDConnection { ); match result { Ok(reply) => { - let s = reply; - info!("Successfully loaded unit [{}] with result [{:?}]", unit, s); + info!( + "Successfully loaded unit [{}] with result [{:?}]", + unit, reply + ); Ok(()) } Err(e) => { diff --git a/src/provider/systemdmanager/service.rs b/src/provider/systemdmanager/service.rs index 41a24a6..7933a0e 100644 --- a/src/provider/systemdmanager/service.rs +++ b/src/provider/systemdmanager/service.rs @@ -2,27 +2,46 @@ use std::collections::HashMap; use kubelet::container::Container; use kubelet::pod::Pod; -use phf::{phf_map, phf_ordered_set}; +use phf; +use phf::{Map, OrderedSet}; use crate::provider::error::StackableError; use crate::provider::error::StackableError::PodValidationError; use crate::provider::states::creating_config::CreatingConfig; +use crate::provider::systemdmanager::manager::UnitTypes; use crate::provider::PodState; use log::{debug, error, trace, warn}; -static RESTART_POLICY_MAP: phf::Map<&'static str, &'static str> = phf_map! { +static RESTART_POLICY_MAP: Map<&'static str, &'static str> = phf::phf_map! { "Always" => "always", "OnFailure" => "on-failure", "Never" => "no", }; +pub const SECTION_SERVICE: &str = "Service"; +pub const SECTION_UNIT: &str = "Unit"; +pub const SECTION_INSTALL: &str = "Install"; + // TODO: This will be used later to ensure the same ordering of known sections in // unit files, I'll leave it in for now #[allow(dead_code)] -static SECTION_ORDER: phf::OrderedSet<&'static str> = - phf_ordered_set! {"Unit", "Service", "Install"}; - +static SECTION_ORDER: OrderedSet<&'static str> = + phf::phf_ordered_set! {"Unit", "Service", "Install"}; + +/// A struct representing all systemd units that this server needs to run for a pod that +/// it was assigned. +/// +/// There will be a 1:1 relationship between pods assigned to this node and objects of this +/// struct. +/// +/// Within the struct multiple systemd units can exist, depending on the container configuration +/// in the pod. +/// +/// Currently only service units are implemented, but we plan to extend this to other types of +/// of units as well (mounts, ..) +/// +/// Init Containers will be translated to service units as well (still TODO) pub struct Service { pub systemd_units: Vec, } @@ -30,7 +49,7 @@ pub struct Service { impl Service { pub fn new(pod: &Pod, pod_state: &PodState) -> Result { // Create systemd unit template with values we need from the pod object - let pod_settings = SystemDUnit::new_from_pod(&pod); + let pod_settings = SystemDUnit::new_from_pod(&pod)?; // Convert all containers to systemd unit files let systemd_units: Result, StackableError> = pod @@ -45,9 +64,11 @@ impl Service { } } +/// A struct that represents an individual systemd unit #[derive(Clone)] pub struct SystemDUnit { name: String, + unit_type: UnitTypes, sections: HashMap>, environment: HashMap, } @@ -55,7 +76,6 @@ pub struct SystemDUnit { impl SystemDUnit { /// Create a new unit which inherits all common elements from ['common_properties'] and parses /// everything else from the ['container'] - pub fn new( common_properties: &SystemDUnit, container: &Container, @@ -64,10 +84,10 @@ impl SystemDUnit { let mut unit = common_properties.clone(); unit.name = String::from(container.name()); - unit.add_property("Unit", "Description", &unit.name.clone()); + unit.add_property(SECTION_UNIT, "Description", &unit.name.clone()); unit.add_property( - "Service", + SECTION_SERVICE, "ExecStart", &SystemDUnit::get_command(container, pod_state)?, ); @@ -78,34 +98,46 @@ impl SystemDUnit { unit.add_env_var(&name, &value); } - unit.add_property("Service", "StandardOutput", "journal"); - unit.add_property("Service", "StandardError", "journal"); - unit.add_property("Install", "WantedBy", "multi-user.target"); + unit.add_property(SECTION_SERVICE, "StandardOutput", "journal"); + unit.add_property(SECTION_SERVICE, "StandardError", "journal"); + unit.add_property(SECTION_INSTALL, "WantedBy", "multi-user.target"); Ok(unit) } - pub fn new_from_pod(pod: &Pod) -> Self { + pub fn new_from_pod(pod: &Pod) -> Result { let mut unit = SystemDUnit { name: pod.name().to_string(), + unit_type: UnitTypes::Service, sections: Default::default(), environment: Default::default(), }; let restart_policy = match &pod.as_kube_pod().spec { + // if no restart policy is present we default to "never" Some(spec) => spec.restart_policy.as_deref().unwrap_or("Never"), None => "Never", }; - unit.add_property( - "Service", - "Restart", - RESTART_POLICY_MAP.get(restart_policy).unwrap(), - ); - unit + // if however one is specified but we do not know about this policy then we do not default + // to never but fail the service instead to avoid unpredictable behavior + let restart_policy = match RESTART_POLICY_MAP.get(restart_policy) { + Some(policy) => policy, + None => { + return Err(PodValidationError { + msg: format!( + "Unknown value [{}] for RestartPolicy in pod [{}]", + restart_policy, unit.name + ), + }) + } + }; + + unit.add_property(SECTION_SERVICE, "Restart", restart_policy); + Ok(unit) } - // Add a key=value entry to the specified section + /// Add a key=value entry to the specified section fn add_property(&mut self, section: &'static str, key: &str, value: &str) { let section = self .sections @@ -128,7 +160,7 @@ impl SystemDUnit { for (key, value) in entries { unit_file_content.push_str(&format!("{}={}\n", key, value)); } - if section.eq("Service") { + if section == SECTION_SERVICE { // Add environment variables to Service section for (name, value) in &self.environment { unit_file_content.push_str(&format!("Environment=\"{}={}\"\n", name, value)); @@ -213,6 +245,7 @@ impl SystemDUnit { Ok(env_variables) } + // Retrieve a copy of the command object in the pod, or return an error if it is missing fn get_command(container: &Container, pod_state: &PodState) -> Result { // Return an error if no command was specified in the container // TODO: We should discuss if there can be a valid scenario for this @@ -232,7 +265,7 @@ impl SystemDUnit { let package_root = pod_state.get_service_package_directory(); trace!( - "Commmand before replacing variables and adding packageroot: {:?}", + "Command before replacing variables and adding packageroot: {:?}", command ); // Get a mutable reference to the first element of the command array as we might need to From cdaff873e95160f618c6915fca6dfa68cd0b78ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 10 Feb 2021 12:18:41 +0100 Subject: [PATCH 10/17] Refactored the systemdmanager code, this should address most of the comments related to that. --- Cargo.lock | 62 +++- Cargo.toml | 1 + src/provider/mod.rs | 22 +- src/provider/states/creating_service.rs | 29 +- src/provider/states/starting.rs | 41 ++- src/provider/states/terminated.rs | 50 ++- src/provider/systemdmanager/manager.rs | 438 ++++++++++++++---------- src/provider/systemdmanager/service.rs | 52 ++- 8 files changed, 459 insertions(+), 236 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5f34f80..b1da201 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -509,6 +509,16 @@ dependencies = [ "dirs-sys", ] +[[package]] +name = "dirs-next" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b98cf8ebf19c3d1b223e151f99a4f9f0690dca41414773390fc824184ac833e1" +dependencies = [ + "cfg-if 1.0.0", + "dirs-sys-next", +] + [[package]] name = "dirs-sys" version = "0.3.5" @@ -516,7 +526,18 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8e93d7f5705de3e49895a2b5e0b8855a1c27f080192ae9c32a6432d50741a57a" dependencies = [ "libc", - "redox_users", + "redox_users 0.3.5", + "winapi 0.3.9", +] + +[[package]] +name = "dirs-sys-next" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4ebda144c4fe02d1f7ea1a7d9641b6fc6b580adcfa024ae48797ecdeb6825b4d" +dependencies = [ + "libc", + "redox_users 0.4.0", "winapi 0.3.9", ] @@ -580,7 +601,7 @@ checksum = "0c122a393ea57648015bf06fbd3d372378992e86b9ff5a7a497b076a28c79efe" dependencies = [ "cfg-if 1.0.0", "libc", - "redox_syscall", + "redox_syscall 0.1.57", "winapi 0.3.9", ] @@ -2236,6 +2257,15 @@ version = "0.1.57" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "41cc0f7e4d5d4544e8861606a285bb08d3e70712ccc7d2b84d7c0ccfaf4b05ce" +[[package]] +name = "redox_syscall" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05ec8ca9416c5ea37062b502703cd7fcb207736bc294f6e0cf367ac6fc234570" +dependencies = [ + "bitflags 1.2.1", +] + [[package]] name = "redox_users" version = "0.3.5" @@ -2243,10 +2273,20 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "de0737333e7a9502c789a36d7c7fa6092a49895d4faa31ca5df163857ded2e9d" dependencies = [ "getrandom 0.1.15", - "redox_syscall", + "redox_syscall 0.1.57", "rust-argon2", ] +[[package]] +name = "redox_users" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "528532f3d801c87aec9def2add9ca802fe569e44a544afe633765267840abe64" +dependencies = [ + "getrandom 0.2.0", + "redox_syscall 0.2.4", +] + [[package]] name = "regex" version = "1.4.2" @@ -2582,6 +2622,15 @@ version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2579985fda508104f7587689507983eadd6a6e84dd35d6d115361f530916fa0d" +[[package]] +name = "shellexpand" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "83bdb7831b2d85ddf4a7b148aa19d0587eddbe8671a436b7bd1182eaad0f2829" +dependencies = [ + "dirs-next", +] + [[package]] name = "signal-hook-registry" version = "1.2.2" @@ -2640,7 +2689,7 @@ checksum = "2c29947abdee2a218277abeca306f25789c938e500ea5a9d4b12a5a504466902" dependencies = [ "cfg-if 1.0.0", "libc", - "redox_syscall", + "redox_syscall 0.1.57", "winapi 0.3.9", ] @@ -2676,6 +2725,7 @@ dependencies = [ "serde_derive", "serde_json", "serde_yaml", + "shellexpand", "stackable_config", "tar", "thiserror", @@ -2866,7 +2916,7 @@ checksum = "489997b7557e9a43e192c527face4feacc78bfbe6eed67fd55c4c9e381cba290" dependencies = [ "filetime", "libc", - "redox_syscall", + "redox_syscall 0.1.57", "xattr", ] @@ -2879,7 +2929,7 @@ dependencies = [ "cfg-if 0.1.10", "libc", "rand 0.7.3", - "redox_syscall", + "redox_syscall 0.1.57", "remove_dir_all", "winapi 0.3.9", ] diff --git a/Cargo.toml b/Cargo.toml index ac68483..a38b694 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,7 @@ stackable_config = { git = "https://github.com/stackabletech/common.git", branch phf = { version = "0.7.24", features = ["macros"] } dbus = "0.9.0" hostname = "0.3" +shellexpand = "2.1" [dev-dependencies] indoc = "1.0" diff --git a/src/provider/mod.rs b/src/provider/mod.rs index 6abc2b3..8e2b8c0 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -19,8 +19,10 @@ use crate::provider::error::StackableError::{ use crate::provider::repository::package::Package; use crate::provider::states::downloading::Downloading; use crate::provider::states::terminated::Terminated; -use crate::provider::systemdmanager::manager::Manager; +use crate::provider::systemdmanager::manager::SystemdManager; +use crate::provider::systemdmanager::service::Service; use kube::error::ErrorResponse; +use std::time::Duration; pub struct StackableProvider { client: Client, @@ -47,7 +49,8 @@ pub struct PodState { service_name: String, service_uid: String, package: Package, - systemd_manager: Manager, + systemd_manager: SystemdManager, + service_units: Option, } impl PodState { @@ -199,17 +202,7 @@ impl Provider for StackableProvider { } // TODO: investigate if we can share one DBus connection across all pods - // Depending on whether we are supposed to run in user space or system-wide - // we'll pick the default directory to initialize the systemd manager with - // This allows creating unit files either directly in the systemd folder by - // passing in just a filename, or symlink them by passing in an absolute - // path - let unit_directory = if self.session { - PathBuf::from("/lib/systemd/system") - } else { - PathBuf::from("~/.config/systemd/user") - }; - let systemd_manager = Manager::new(unit_directory, session)?; + let systemd_manager = SystemdManager::new(session, Duration::from_secs(5))?; Ok(PodState { client: self.client.clone(), @@ -218,12 +211,13 @@ impl Provider for StackableProvider { log_directory, config_directory: self.config_directory.clone(), package_download_backoff_strategy: ExponentialBackoffStrategy::default(), - service_name: String::from(service_name), + service_name, service_uid, package, // TODO: Check if we can work with a reference or a Mutex Guard here to only keep // one connection open to DBus instead of one per tracked Pod systemd_manager, + service_units: None, }) } diff --git a/src/provider/states/creating_service.rs b/src/provider/states/creating_service.rs index 5ebba37..edbe2a6 100644 --- a/src/provider/states/creating_service.rs +++ b/src/provider/states/creating_service.rs @@ -5,11 +5,11 @@ use log::{debug, error, info}; use crate::provider::states::setup_failed::SetupFailed; use crate::provider::states::starting::Starting; -use crate::provider::systemdmanager::manager::UnitTypes; use crate::provider::systemdmanager::service::Service; use crate::provider::PodState; use anyhow::anyhow; use std::fs::create_dir_all; +use std::path::PathBuf; #[derive(Default, Debug, TransitionTo)] #[transition_to(Starting, SetupFailed)] @@ -34,7 +34,11 @@ impl State for CreatingService { } } - // This will create the service unit files on disk + // Naming schema + // Service name: namespace-podname + // SystemdUnit: namespace-podname-containername + // TODO: add this to the docs in more detail + // Create service containing all systemd units from pod spec let service = match Service::new(pod, pod_state) { Ok(new_service) => new_service, Err(error) => { @@ -46,9 +50,11 @@ impl State for CreatingService { } }; - // Each pod can map to multiple systemd units/services as each container will get its own systemd unit file/service. - // This will iterate over all of them and enable the services. - for unit in service.systemd_units { + // Each pod can map to multiple systemd units/services as each container will get its own + // systemd unit file/service. + // This will iterate over all of them, write the service files to disk and link + // the service to systemd. + for unit in &service.systemd_units { let target_file = match service_directory .join(service_name) .into_os_string() @@ -64,19 +70,28 @@ impl State for CreatingService { } }; + //Some(PathBuf::from(service_directory)), + + // Create the service match pod_state .systemd_manager - .enable(&target_file, &unit, UnitTypes::Service) + .create_unit(&unit, None, true, true) { Ok(()) => {} Err(e) => { // TODO: We need to discuss what to do here, in theory we could have loaded // other services already, do we want to stop those? - error!("Failed to load service unit for service [{}]", service_name); + error!( + "Failed to create systemd unit for service [{}]", + service_name + ); return Transition::Complete(Err(e)); } } + // Done for now, if the service was created successfully we are happy + // Starting and enabling comes in a later state after all service have been createddy } + pod_state.service_units = Some(service); // All services were loaded successfully, otherwise we'd have returned early above Transition::next(self, Starting) diff --git a/src/provider/states/starting.rs b/src/provider/states/starting.rs index f86592c..31987b1 100644 --- a/src/provider/states/starting.rs +++ b/src/provider/states/starting.rs @@ -6,6 +6,7 @@ use crate::provider::states::failed::Failed; use crate::provider::states::running::Running; use crate::provider::states::setup_failed::SetupFailed; use crate::provider::PodState; +use log::{debug, error, info, warn}; #[derive(Default, Debug, TransitionTo)] #[transition_to(Running, Failed, SetupFailed)] @@ -14,15 +15,37 @@ pub struct Starting; #[async_trait::async_trait] impl State for Starting { async fn next(self: Box, pod_state: &mut PodState, _: &Pod) -> Transition { - return match pod_state.systemd_manager.start(&pod_state.service_name) { - Ok(()) => Transition::next( - self, - Running { - ..Default::default() - }, - ), - Err(e) => Transition::Complete(Err(e)), - }; + if let Some(systemd_units) = &pod_state.service_units { + for unit in &systemd_units.systemd_units { + info!("Starting systemd unit [{}]", unit); + if let Err(start_error) = pod_state.systemd_manager.start(&unit.get_name()) { + error!( + "Error occurred starting systemd unit [{}]: [{}]", + unit.get_name(), + start_error + ); + return Transition::Complete(Err(start_error)); + } + + info!("Enabling systed unit [{}]", unit); + if let Err(enable_error) = pod_state.systemd_manager.enable(&unit.get_name()) { + error!( + "Error occurred starting systemd unit [{}]: [{}]", + unit.get_name(), + enable_error + ); + return Transition::Complete(Err(enable_error)); + } + } + } else { + warn!("No unit definitions found, not starting anything!"); + } + Transition::next( + self, + Running { + ..Default::default() + }, + ) } async fn json_status( diff --git a/src/provider/states/terminated.rs b/src/provider/states/terminated.rs index 5326348..ba8176f 100644 --- a/src/provider/states/terminated.rs +++ b/src/provider/states/terminated.rs @@ -1,7 +1,6 @@ use kubelet::state::prelude::*; -use log::info; +use log::{error, info, warn}; -use crate::provider::systemdmanager::manager::UnitTypes; use crate::provider::PodState; #[derive(Default, Debug)] @@ -14,20 +13,49 @@ pub struct Terminated { impl State for Terminated { async fn next(self: Box, pod_state: &mut PodState, _pod: &Pod) -> Transition { info!( - "Pod {} was terminated, stopping process!", + "Pod {} was terminated, stopping service!", &pod_state.service_name ); - if let Err(e) = pod_state.systemd_manager.stop(&pod_state.service_name) { - return Transition::Complete(Err(e)); + // TODO: We need some additional error handling here, wait for the services to actually + // shut down and try to remove the rest of the services if one fails (tbd, do we want that?) + if let Some(systemd_units) = &pod_state.service_units { + for unit in &systemd_units.systemd_units { + info!("Stopping systemd unit [{}]", unit); + if let Err(stop_error) = pod_state.systemd_manager.stop(&unit.get_name()) { + error!( + "Error occurred stopping systemd unit [{}]: [{}]", + unit.get_name(), + stop_error + ); + return Transition::Complete(Err(stop_error)); + } + + // Daemon reload is false here, we'll do that once after all units have been removed + info!("Removing systemd unit [{}]", &unit); + if let Err(remove_error) = pod_state + .systemd_manager + .remove_unit(&unit.get_name(), false) + { + error!( + "Error occurred stopping systemd unit [{}]: [{}]", + unit, remove_error + ); + return Transition::Complete(Err(remove_error)); + } + } + } else { + warn!("No unit definitions found, not starting anything!"); } - match pod_state - .systemd_manager - .unload(&pod_state.service_name, UnitTypes::Service) - { + + info!("Performing daemon-reload"); + return match pod_state.systemd_manager.reload() { Ok(()) => Transition::Complete(Ok(())), - Err(e) => Transition::Complete(Err(e)), - } + Err(reload_error) => { + error!("Failed to perform daemon-reload: [{}]", reload_error); + Transition::Complete(Err(reload_error)) + } + }; } async fn json_status( diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index 971c10c..1fa986d 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -6,260 +6,334 @@ use dbus::arg::{AppendAll, ReadAll}; use dbus::blocking::SyncConnection; use dbus::strings::Member; use dbus::Path; -use log::{debug, error, info}; +use log::{debug, warn}; +use std::fs; use std::fs::File; use std::io::Write; use std::path::PathBuf; use std::time::Duration; -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum UnitTypes { Service, } -pub struct Manager { +pub const SYSTEMD_DESTINATION: &str = "org.freedesktop.systemd1"; +pub const SYSTEMD_NODE: &str = "/org/freedesktop/systemd1"; +pub const SYSTEMD_MANAGER_INTERFACE: &str = "org.freedesktop.systemd1.Manager"; + +pub struct SystemdManager { units_directory: PathBuf, - systemd_connection: SystemDConnection, + connection: SyncConnection, //TODO does this need to be closed? + timeout: Duration, } -impl Manager { - pub fn new(units_directory: PathBuf, user_mode: bool) -> Result { - Ok(Manager { +impl Default for SystemdManager { + fn default() -> Self { + // If this panics we broke something in the code, as this is all constant values that + // should work + SystemdManager::new(false, Duration::from_secs(5)).unwrap() + } +} + +impl SystemdManager { + pub fn new(user_mode: bool, timeout: Duration) -> Result { + // Connect to session or system bus depending on the value of [user_mode] + let connection = if user_mode { + SyncConnection::new_session()? + } else { + SyncConnection::new_system()? + }; + + // Depending on whether we are supposed to run in user space or system-wide + // we'll pick the default directory to initialize the systemd manager with + // This allows creating unit files either directly in the systemd folder by + // passing in just a filename, or symlink them by passing in an absolute + // path + let units_directory = if user_mode { + PathBuf::from(shellexpand::tilde("~/.config/systemd/user").to_string()) + } else { + PathBuf::from("/lib/systemd/system") + }; + + Ok(SystemdManager { units_directory, - systemd_connection: SystemDConnection::new(user_mode)?, + connection, + timeout, }) } - /// Write the unit file to disk and enable the service - /// This implicitly calls [link()] - // TODO: should we maybe split enable out into a different function? - pub fn enable( + // The main method for interacting with dbus, all other functions will delegate the actual + // dbus access to this function. + // Private on purpose as this should not be used by external dependencies + fn method_call<'m, R: ReadAll, A: AppendAll, M: Into>>( + &self, + m: M, + args: A, + ) -> Result { + let proxy = self + .connection + .with_proxy(SYSTEMD_DESTINATION, SYSTEMD_NODE, self.timeout); + proxy.method_call(SYSTEMD_MANAGER_INTERFACE, m, args) + } + + // Internal helper method to remove an existing unit file or symlink + fn delete_unit_file(&self, unit: &str) -> Result<(), anyhow::Error> { + let unit_file = self.units_directory.clone().join(&unit); + debug!("Removing [{:?}]", unit_file); + + match fs::remove_file(&unit_file) { + Ok(()) => Ok(()), + Err(delete_error) => { + debug!( + "Failed to remove existing unit file [{:?}] for systemd unit [{}]", + unit_file, unit + ); + Err(anyhow::Error::from(delete_error)) + } + } + } + + /// Write the proper unit file for [unit] to disk. + /// The location of the unit file is determined by the value of [unit_file_path]: + /// + /// * None, the unit file will be created in the base directory that this manager was initialized + /// with, which is either /lib/systemd/system or ~/.config/systemd/user depending on the value of + /// [session]. + /// * Some, the unit file will be created at this location and linked into the proper + /// systemd unit directory TODO: this is not implemented yet + /// + /// [force] determines if an existing unit file should be overwritten, if no external unit file + /// path is specified in [unit_file_path]. If this is false and the target file exists an error + /// is returned. + /// + /// The value of [daemon_reload] controls whether a daemon reload is triggered after creating or + /// linking the unit file. + pub fn create_unit( &self, - name: &str, unit: &SystemDUnit, - unit_type: UnitTypes, + unit_file_path: Option, + force: bool, + daemon_reload: bool, ) -> Result<(), anyhow::Error> { // Appends .service to name if necessary - let unit_name = Manager::get_unit_file_name(name, unit_type)?; - - let unit_path = PathBuf::from(&unit_name); + let linked_unit_file = unit_file_path.is_some(); + let unit_name = SystemdManager::get_unit_file_name(unit.name.as_ref(), &unit.unit_type)?; - // Check if an absolute path was provided - // if this is an absolute path the file will be written there, otherwise - // the file will be created in the unit directory - let target_file = if unit_path.is_absolute() { - unit_path + // Check if a path was provided for the unit file, otherwise use the base directory + let target_file = if let Some(path) = unit_file_path { + path.join(&unit_name) } else { - self.units_directory.join(unit_path) + // TODO: I think we can get away with a reference here, but not sure yet, + // that would mean looking into get_unit_file_name returning a &str, _I think_ + self.units_directory.clone().join(&unit_name) }; - debug!("Target file for service [{}] : [{:?}]", name, target_file); + debug!( + "Target file for service [{}] : [{:?}]", + &unit_name, &target_file + ); + + // If no external file was created check if the file currently exists + if !linked_unit_file { + debug!("Target file [{:?}] already exists", &target_file); + if target_file.exists() { + // Target file already exists, remove if force is supplied, otherwise abort + if force { + self.delete_unit_file(&unit_name)?; + } else { + return Err(anyhow!( + "Target unit file [{:?}] exists and force parameter was not specified.", + target_file + )); + } + } + } + // Write unit file let mut unit_file = match File::create(&target_file) { Ok(file) => file, Err(e) => { - debug!("Error occurred when creating unit file [{}]: [{}]", name, e); + debug!( + "Error occurred when creating unit file [{}]: [{}]", + unit_name, e + ); return Err(anyhow::Error::from(e)); } }; - unit_file.write_all(unit.get_unit_file_content().as_bytes())?; unit_file.flush()?; - self.systemd_connection - .enable_unit_file(&target_file.into_os_string().to_string_lossy())?; - self.reload()?; + // If this is a linked unit file we need to call out to systemd to link this file + if linked_unit_file { + self.link_unit_file(&target_file.into_os_string().to_string_lossy())?; + } + // Perform daemon reload if requested + if daemon_reload { + self.reload()?; + } Ok(()) } - // Stop and disable the service, then delete the unit file from disk - pub fn unload(&self, name: &str, unit_type: UnitTypes) -> Result<(), anyhow::Error> { - // Appends .service to name if necessary - let unit_name = Manager::get_unit_file_name(name, unit_type)?; + /// Removes a unit from systemd. + /// Depending on what is passed in the [unit] parameter this means one of two things: + /// + /// * if an absolute file path is passed, the symlink to this file is deleted from the + /// systemd unit folder + /// * if a unit name is passed an attempt is made to unlink the unit via a dbus call + /// + /// Calling this function means an implicit disabling of the service, if it was enabled. + /// + pub fn remove_unit(&self, unit: &str, daemon_reload: bool) -> Result<(), anyhow::Error> { + debug!("Disabling unit [{}]", unit); + if let Err(disable_error) = self.disable(unit) { + debug!( + "Error disabling systemd unit [{}]: [{}]", + unit, disable_error + ); + return Err(anyhow::Error::from(disable_error)); + } - self.systemd_connection - .disable_unit_file(&unit_name) - .map_err(anyhow::Error::from) + debug!("Removing unit [{}] from systemd", unit); + self.delete_unit_file(&unit)?; + Ok(()) } - pub fn start(&self, name: &str) -> Result<(), anyhow::Error> { - let unit_name = Manager::get_unit_file_name(name, UnitTypes::Service)?; - match self.systemd_connection.start_unit(&unit_name) { - Ok(result) => { - info!("Successfully started service [{}]: [{}]", unit_name, result); + /// Enables a systemd unit to be stared automatically at system boot - expects a fully named + /// unit (which means: including the .service or other unit type). + /// This either requires that the unit is known to systemd or an absolute path to a unit file + /// to work. + /// + /// For a unit file to be _known_ it needs to either be located in the systemd unit folder, or + /// linked into that folder - both actions can be performed by calling [create_unit] + pub fn enable(&self, unit: &str) -> Result<(), anyhow::Error> { + // We don't do any checking around this and simply trust the user that either the name + // of an existing and linked service was provided or this is an absolute path + debug!("Trying to enable systemd unit [{}]", unit); + + match self + .method_call("EnableUnitFiles", (&[unit][..], false, true)) + .map(|_: ()| ()) + { + Ok(()) => { + debug!("Successfully started service [{}]", unit); Ok(()) } Err(e) => { - error!("Error: [{}]", e); - Err(anyhow!("Error starting service [{}]: {}", name, e)) + debug!("Error: [{}]", e); + Err(anyhow!("Error starting service [{}]: {}", unit, e)) } } } - pub fn stop(&self, name: &str) -> Result<(), anyhow::Error> { - let unit_name = Manager::get_unit_file_name(name, UnitTypes::Service)?; - match self.systemd_connection.stop_unit(&unit_name) { + // Stop and disable the service, then delete the unit file from disk + pub fn disable(&self, unit: &str) -> Result, anyhow::Error> { + debug!("Trying to disable systemd unit [{}]", unit); + match self + .method_call("DisableUnitFiles", (&[unit][..], false)) + .map(|r: (Vec<(String, String, String)>,)| r.0) + { Ok(result) => { - info!("Successfully stopped service [{}]: [{}]", unit_name, result); - Ok(()) + debug!("Successfully disabled service [{}]", unit); + println!("{:?}", result); + Ok(result) } Err(e) => { - error!("Error: [{}]", e); - Err(anyhow!("Error stopping service [{}]: {}", name, e)) + debug!("Error: [{}]", e); + Err(anyhow!("Error starting service [{}]: {}", unit, e)) } } } - // Check if the unit name is valid and append .service if needed - // Cannot currently fail, I'll need to dig into what is a valid unit - // name before adding checks - #[allow(clippy::unnecessary_wraps)] - fn get_unit_file_name(name: &str, unit_type: UnitTypes) -> Result { - // TODO: what are valid systemd unit names? - - // Append proper extension for unit type to file name - let extension = match unit_type { - UnitTypes::Service => ".service", - }; + /// Attempts to start a systemd unit + /// [unit] is expected to be the name (including .) of a service that is known to + /// systemd at the time this is called. + /// To make a service known please take a look at the [enable] function. + pub fn start(&self, unit: &str) -> Result<(), anyhow::Error> { + debug!("Attempting to start unit {}", unit); - let mut result = String::from(name); - if !name.ends_with(extension) { - result.push_str(extension); - } - Ok(result) - } - - pub fn reload(&self) -> Result<(), anyhow::Error> { - match self.systemd_connection.reload() { - Ok(_) => info!("Successfully performed daemon-reload"), + match self + .method_call("StartUnit", (unit, "fail")) + .map(|r: (Path,)| r.0) + { + Ok(result) => { + debug!("Successfully started service [{}]: [{}]", unit, result); + Ok(()) + } Err(e) => { - error!("Error: [{}]", e); - return Err(anyhow!("Error performing daemon-reload: [{}]", e)); + debug!("Error: [{}]", e); + Err(anyhow!("Error starting service [{}]: {}", unit, e)) } - }; - Ok(()) - } -} - -struct SystemDConnection { - connection: SyncConnection, //TODO does this need to be closed? - dest: &'static str, - node: &'static str, - interface: &'static str, - timeout: Duration, -} - -impl SystemDConnection { - fn new(user_mode: bool) -> Result { - let connection = if user_mode { - SyncConnection::new_session()? - } else { - SyncConnection::new_system()? - }; - - Ok(SystemDConnection { - connection, - dest: "org.freedesktop.systemd1", - node: "/org/freedesktop/systemd1", - interface: "org.freedesktop.systemd1.Manager", - timeout: Duration::from_secs(5), - }) - } - - pub fn reload(&self) -> Result<(), dbus::Error> { - let proxy = self - .connection - .with_proxy(self.dest, self.node, self.timeout); - proxy - .method_call(self.interface, "Reload", ()) - .map(|_: ()| ()) + } } - /// Takes a unit name as input and attempts to start it. - pub fn start_unit(&self, unit: &str) -> Result { - // create a wrapper struct around the connection that makes it easy - // to send method calls to a specific destination and path. - info!("Attempting to start unit {}", unit); + /// Attempts to stop a systemd unit + /// [unit] is expected to be the name (including .) of a service that is known to + /// systemd at the time this is called. + /// To make a service known please take a look at the [enable] function. + pub fn stop(&self, unit: &str) -> Result<(), anyhow::Error> { + debug!("Trying to stop systemd unit [{}]", unit); - /*let proxy = self - .connection - .with_proxy(self.dest, self.node, self.timeout); - proxy - .method_call(self.interface, "StartUnit", (unit, "fail")) - */ - self.method_call("StartUnit", (unit, "fail")) + match self + .method_call("StopUnit", (unit, "fail")) .map(|r: (Path,)| r.0) + { + Ok(result) => { + debug!("Successfully stopped service [{}]: [{}]", unit, result); + Ok(()) + } + Err(e) => { + debug!("Error: [{}]", e); + Err(anyhow!("Error s service [{}]: {}", unit, e)) + } + } } - fn method_call<'m, R: ReadAll, A: AppendAll, M: Into>>( - &self, - m: M, - args: A, - ) -> Result { - let proxy = self - .connection - .with_proxy(self.dest, self.node, self.timeout); - proxy.method_call(self.interface, m, args) - } - - /// Takes a unit name as input and attempts to stop it. - pub fn stop_unit(&self, unit: &str) -> Result { - // create a wrapper struct around the connection that makes it easy - // to send method calls to a specific destination and path. - info!("Attempting to stop unit {}", unit); - let proxy = self - .connection - .with_proxy(self.dest, self.node, self.timeout); - proxy - .method_call(self.interface, "StopUnit", (unit, "fail")) - .map(|r: (Path,)| r.0) - } + // Perform a daemon-reload, this causes systemd to re-read all unit files on disk and + // discover changes that have been performed since the last reload + // This needs to be done after creating a new service unit before it can be targeted by + // start / stop and similar commands. + pub fn reload(&self) -> Result<(), anyhow::Error> { + debug!("Performing daemon-reload.."); - /// Takes the unit pathname of a service and enables it via dbus. - /// If dbus replies with `[Bool(true), Array([], "(sss)")]`, the service is already enabled. - pub fn enable_unit_file(&self, unit: &str) -> Result<(), dbus::Error> { - debug!("Enabling unit from file [{}]", unit); - let proxy = self - .connection - .with_proxy(self.dest, self.node, self.timeout); - let runtime = false; - let force = true; - let result: Result<(), dbus::Error> = proxy.method_call( - self.interface, - "EnableUnitFiles", - (&[unit][..], runtime, force), - ); - match result { - Ok(reply) => { - info!( - "Successfully loaded unit [{}] with result [{:?}]", - unit, reply - ); + match self.method_call("Reload", ()).map(|_: ()| ()) { + Ok(_) => { + debug!("Successfully performed daemon-reload"); Ok(()) } Err(e) => { - error!("Error enabling unit [{}]", unit); - Err(e) + debug!("Error: [{}]", e); + Err(anyhow!("Error performing daemon-reload: [{}]", e)) } } } - // TODO: this doesn't work yet, the symlink is not deleted for some reason - // I'll need to investigate this - pub fn disable_unit_file(&self, unit: &str) -> Result<(), dbus::Error> { - let proxy = self - .connection - .with_proxy(self.dest, self.node, self.timeout); - let force = true; - let result: Result<(Vec,), dbus::Error> = - proxy.method_call(self.interface, "DisableUnitFiles", (&[unit][..], force)); - - match &result { - Ok(_) => info!("Successfully disabled service!"), - Err(e) => error!("Error disabling service: [{}]", e), - } + // Symlink a unit file into the systemd unit folder + // This is not public on purpose, as [create] should be the normal way to link unit files + // when using this crate + fn link_unit_file(&self, unit: &str) -> Result<(), dbus::Error> { + debug!("Linking [{}]", unit); + self.method_call("LinkUnitFiles", (&[unit][..], false, true)) + .map(|_: ()| ()) + } - result.map(|_| ()) + // Check if the unit name is valid and append .service if needed + // Cannot currently fail, I'll need to dig into what is a valid unit + // name before adding checks + #[allow(clippy::unnecessary_wraps)] + fn get_unit_file_name(name: &str, unit_type: &UnitTypes) -> Result { + // TODO: what are valid systemd unit names? + + // Append proper extension for unit type to file name + let extension = match unit_type { + UnitTypes::Service => ".service", + }; + + let mut result = String::from(name); + if !name.ends_with(extension) { + result.push_str(extension); + } + Ok(result) } } diff --git a/src/provider/systemdmanager/service.rs b/src/provider/systemdmanager/service.rs index 7933a0e..e1621e2 100644 --- a/src/provider/systemdmanager/service.rs +++ b/src/provider/systemdmanager/service.rs @@ -12,6 +12,8 @@ use crate::provider::states::creating_config::CreatingConfig; use crate::provider::systemdmanager::manager::UnitTypes; use crate::provider::PodState; use log::{debug, error, trace, warn}; +use std::fmt; +use std::fmt::{Display, Formatter}; static RESTART_POLICY_MAP: Map<&'static str, &'static str> = phf::phf_map! { "Always" => "always", @@ -24,7 +26,7 @@ pub const SECTION_UNIT: &str = "Unit"; pub const SECTION_INSTALL: &str = "Install"; // TODO: This will be used later to ensure the same ordering of known sections in -// unit files, I'll leave it in for now +// unit files, I'll leave it in for now #[allow(dead_code)] static SECTION_ORDER: OrderedSet<&'static str> = phf::phf_ordered_set! {"Unit", "Service", "Install"}; @@ -42,6 +44,8 @@ static SECTION_ORDER: OrderedSet<&'static str> = /// of units as well (mounts, ..) /// /// Init Containers will be translated to service units as well (still TODO) +// TODO: This is actually purely a stackable thing and should be removed from the systemd-specific +// code in preparation of pulling that out in a crate pub struct Service { pub systemd_units: Vec, } @@ -51,11 +55,17 @@ impl Service { // Create systemd unit template with values we need from the pod object let pod_settings = SystemDUnit::new_from_pod(&pod)?; + let kube_pod = pod.as_kube_pod(); + let namespace = kube_pod.metadata.namespace.as_ref().unwrap(); + let pod_name = kube_pod.metadata.name.as_ref().unwrap(); + + let prefix = format!("{}-{}-", namespace, pod_name); + // Convert all containers to systemd unit files let systemd_units: Result, StackableError> = pod .containers() .iter() - .map(|container| SystemDUnit::new(&pod_settings, container, pod_state)) + .map(|container| SystemDUnit::new(&pod_settings, &prefix, container, pod_state)) .collect(); systemd_units.map(|units| Self { @@ -67,22 +77,35 @@ impl Service { /// A struct that represents an individual systemd unit #[derive(Clone)] pub struct SystemDUnit { - name: String, - unit_type: UnitTypes, - sections: HashMap>, - environment: HashMap, + pub name: String, + pub unit_type: UnitTypes, + pub sections: HashMap>, + pub environment: HashMap, } +// TODO: The parsing code is also highly stackable specific, we should +// at some point consider splitting this out and have systemdunit live +// inside the systemd crate and the parsing in the agent impl SystemDUnit { /// Create a new unit which inherits all common elements from ['common_properties'] and parses /// everything else from the ['container'] pub fn new( common_properties: &SystemDUnit, + name_prefix: &str, container: &Container, pod_state: &PodState, ) -> Result { let mut unit = common_properties.clone(); - unit.name = String::from(container.name()); + + let trimmed_name = match container + .name() + .strip_suffix(common_properties.get_type_string()) + { + None => container.name().to_string(), + Some(name_without_suffix) => name_without_suffix.to_string(), + }; + + unit.name = format!("{}{}", name_prefix, trimmed_name); unit.add_property(SECTION_UNIT, "Description", &unit.name.clone()); @@ -137,6 +160,10 @@ impl SystemDUnit { Ok(unit) } + pub fn get_name(&self) -> String { + format!("{}.service", self.name) + } + /// Add a key=value entry to the specified section fn add_property(&mut self, section: &'static str, key: &str, value: &str) { let section = self @@ -171,6 +198,11 @@ impl SystemDUnit { unit_file_content } + fn get_type_string(&self) -> &str { + match &self.unit_type { + Service => ".service", + } + } fn get_environment( container: &Container, pod_state: &PodState, @@ -352,3 +384,9 @@ impl SystemDUnit { Ok(command_render_result.join(" ")) } } + +impl Display for SystemDUnit { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.get_name()) + } +} From 7cea7224f35cc2f72768be9f9b4319013e65557a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 10 Feb 2021 13:15:21 +0100 Subject: [PATCH 11/17] Fixed some clippy warnings. --- src/bin/generate_doc.rs | 2 +- src/provider/states/creating_service.rs | 22 +++------------------- src/provider/states/starting.rs | 2 +- src/provider/systemdmanager/manager.rs | 8 ++++++-- src/provider/systemdmanager/service.rs | 3 +-- 5 files changed, 12 insertions(+), 25 deletions(-) diff --git a/src/bin/generate_doc.rs b/src/bin/generate_doc.rs index 7396294..e161fe1 100644 --- a/src/bin/generate_doc.rs +++ b/src/bin/generate_doc.rs @@ -1,4 +1,4 @@ -/// This is a helper binary which generates the file _documentation/commandline_args.adoc_ which +/// This is a helper binary which generates the file `documentation/commandline_args.adoc` which /// contains documentation of the available command line options for the agent binary. /// /// It gets the content by calling [`stackable_agent::config::AgentConfig::get_documentation()`] diff --git a/src/provider/states/creating_service.rs b/src/provider/states/creating_service.rs index edbe2a6..14b5f3e 100644 --- a/src/provider/states/creating_service.rs +++ b/src/provider/states/creating_service.rs @@ -7,9 +7,7 @@ use crate::provider::states::setup_failed::SetupFailed; use crate::provider::states::starting::Starting; use crate::provider::systemdmanager::service::Service; use crate::provider::PodState; -use anyhow::anyhow; use std::fs::create_dir_all; -use std::path::PathBuf; #[derive(Default, Debug, TransitionTo)] #[transition_to(Starting, SetupFailed)] @@ -55,24 +53,10 @@ impl State for CreatingService { // This will iterate over all of them, write the service files to disk and link // the service to systemd. for unit in &service.systemd_units { - let target_file = match service_directory - .join(service_name) - .into_os_string() - .into_string() - { - Ok(path) => path, - Err(_) => { - // TODO: output proper error message with information - return Transition::Complete(Err(anyhow!( - "Failed to convert path for service unit file [{}]", - service_name - ))); - } - }; - - //Some(PathBuf::from(service_directory)), - // Create the service + + // As per ADR005 we currently write the unit files directly in the systemd + // unit directory (by passing None as [unit_file_path]). match pod_state .systemd_manager .create_unit(&unit, None, true, true) diff --git a/src/provider/states/starting.rs b/src/provider/states/starting.rs index 31987b1..d9dcc03 100644 --- a/src/provider/states/starting.rs +++ b/src/provider/states/starting.rs @@ -6,7 +6,7 @@ use crate::provider::states::failed::Failed; use crate::provider::states::running::Running; use crate::provider::states::setup_failed::SetupFailed; use crate::provider::PodState; -use log::{debug, error, info, warn}; +use log::{error, info, warn}; #[derive(Default, Debug, TransitionTo)] #[transition_to(Running, Failed, SetupFailed)] diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index 1fa986d..dce87a4 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -6,7 +6,7 @@ use dbus::arg::{AppendAll, ReadAll}; use dbus::blocking::SyncConnection; use dbus::strings::Member; use dbus::Path; -use log::{debug, warn}; +use log::debug; use std::fs; use std::fs::File; use std::io::Write; @@ -192,11 +192,15 @@ impl SystemdManager { "Error disabling systemd unit [{}]: [{}]", unit, disable_error ); - return Err(anyhow::Error::from(disable_error)); + return Err(disable_error); } debug!("Removing unit [{}] from systemd", unit); self.delete_unit_file(&unit)?; + + if daemon_reload { + self.reload()?; + } Ok(()) } diff --git a/src/provider/systemdmanager/service.rs b/src/provider/systemdmanager/service.rs index e1621e2..9e916bd 100644 --- a/src/provider/systemdmanager/service.rs +++ b/src/provider/systemdmanager/service.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; use kubelet::container::Container; use kubelet::pod::Pod; -use phf; use phf::{Map, OrderedSet}; use crate::provider::error::StackableError; @@ -200,7 +199,7 @@ impl SystemDUnit { fn get_type_string(&self) -> &str { match &self.unit_type { - Service => ".service", + UnitTypes::Service => ".service", } } fn get_environment( From d84a3414346fc1ac54af82e565ea448bd8232581 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 10 Feb 2021 16:22:47 +0100 Subject: [PATCH 12/17] More refactoring - removed the entire Service object as that was effectively not doing anything. --- src/provider/mod.rs | 4 +- src/provider/states/creating_service.rs | 43 ++++++++++++------ src/provider/states/starting.rs | 2 +- src/provider/states/terminated.rs | 2 +- src/provider/systemdmanager/manager.rs | 2 +- src/provider/systemdmanager/mod.rs | 2 +- .../{service.rs => systemdunit.rs} | 45 +------------------ 7 files changed, 37 insertions(+), 63 deletions(-) rename src/provider/systemdmanager/{service.rs => systemdunit.rs} (89%) diff --git a/src/provider/mod.rs b/src/provider/mod.rs index 8e2b8c0..377dd19 100644 --- a/src/provider/mod.rs +++ b/src/provider/mod.rs @@ -20,7 +20,7 @@ use crate::provider::repository::package::Package; use crate::provider::states::downloading::Downloading; use crate::provider::states::terminated::Terminated; use crate::provider::systemdmanager::manager::SystemdManager; -use crate::provider::systemdmanager::service::Service; +use crate::provider::systemdmanager::systemdunit::SystemDUnit; use kube::error::ErrorResponse; use std::time::Duration; @@ -50,7 +50,7 @@ pub struct PodState { service_uid: String, package: Package, systemd_manager: SystemdManager, - service_units: Option, + service_units: Option>, } impl PodState { diff --git a/src/provider/states/creating_service.rs b/src/provider/states/creating_service.rs index 14b5f3e..72d07d0 100644 --- a/src/provider/states/creating_service.rs +++ b/src/provider/states/creating_service.rs @@ -5,7 +5,7 @@ use log::{debug, error, info}; use crate::provider::states::setup_failed::SetupFailed; use crate::provider::states::starting::Starting; -use crate::provider::systemdmanager::service::Service; +use crate::provider::systemdmanager::systemdunit::SystemDUnit; use crate::provider::PodState; use std::fs::create_dir_all; @@ -33,28 +33,43 @@ impl State for CreatingService { } // Naming schema - // Service name: namespace-podname - // SystemdUnit: namespace-podname-containername + // Service name: `namespace-podname` + // SystemdUnit: `namespace-podname-containername` // TODO: add this to the docs in more detail - // Create service containing all systemd units from pod spec - let service = match Service::new(pod, pod_state) { - Ok(new_service) => new_service, - Err(error) => { + let service_prefix = format!("{}-{}-", pod.namespace(), pod.name()); + + // Create a template from those settings that are derived directly from the pod, not + // from container objects + let unit_template = match SystemDUnit::new_from_pod(&pod) { + Ok(unit) => unit, + Err(pod_error) => { error!( - "Failed to create service units from pod [{}], aborting.", - pod_state.service_name + "Unable to create systemd unit template from pod [{}]: [{}]", + service_name, pod_error ); - return Transition::Complete(Err(anyhow::Error::from(error))); + return Transition::Complete(Err(anyhow::Error::from(pod_error))); } }; // Each pod can map to multiple systemd units/services as each container will get its own // systemd unit file/service. - // This will iterate over all of them, write the service files to disk and link + // Map every container from the pod object to a systemdunit + let systemd_units: Vec = match pod + .containers() + .iter() + .map(|container| { + SystemDUnit::new(&unit_template, &service_prefix, container, pod_state) + }) + .collect() + { + Ok(units) => units, + Err(err) => return Transition::Complete(Err(anyhow::Error::from(err))), + }; + + // This will iterate over all systemd units, write the service files to disk and link // the service to systemd. - for unit in &service.systemd_units { + for unit in &systemd_units { // Create the service - // As per ADR005 we currently write the unit files directly in the systemd // unit directory (by passing None as [unit_file_path]). match pod_state @@ -75,7 +90,7 @@ impl State for CreatingService { // Done for now, if the service was created successfully we are happy // Starting and enabling comes in a later state after all service have been createddy } - pod_state.service_units = Some(service); + pod_state.service_units = Some(systemd_units); // All services were loaded successfully, otherwise we'd have returned early above Transition::next(self, Starting) diff --git a/src/provider/states/starting.rs b/src/provider/states/starting.rs index d9dcc03..d9d77f9 100644 --- a/src/provider/states/starting.rs +++ b/src/provider/states/starting.rs @@ -16,7 +16,7 @@ pub struct Starting; impl State for Starting { async fn next(self: Box, pod_state: &mut PodState, _: &Pod) -> Transition { if let Some(systemd_units) = &pod_state.service_units { - for unit in &systemd_units.systemd_units { + for unit in systemd_units { info!("Starting systemd unit [{}]", unit); if let Err(start_error) = pod_state.systemd_manager.start(&unit.get_name()) { error!( diff --git a/src/provider/states/terminated.rs b/src/provider/states/terminated.rs index ba8176f..9a1beb2 100644 --- a/src/provider/states/terminated.rs +++ b/src/provider/states/terminated.rs @@ -20,7 +20,7 @@ impl State for Terminated { // TODO: We need some additional error handling here, wait for the services to actually // shut down and try to remove the rest of the services if one fails (tbd, do we want that?) if let Some(systemd_units) = &pod_state.service_units { - for unit in &systemd_units.systemd_units { + for unit in systemd_units { info!("Stopping systemd unit [{}]", unit); if let Err(stop_error) = pod_state.systemd_manager.stop(&unit.get_name()) { error!( diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index dce87a4..d658550 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -1,6 +1,6 @@ //! A manager that allows managing systemd units -use crate::provider::systemdmanager::service::SystemDUnit; +use crate::provider::systemdmanager::systemdunit::SystemDUnit; use anyhow::anyhow; use dbus::arg::{AppendAll, ReadAll}; use dbus::blocking::SyncConnection; diff --git a/src/provider/systemdmanager/mod.rs b/src/provider/systemdmanager/mod.rs index 91fffe4..d87d4b5 100644 --- a/src/provider/systemdmanager/mod.rs +++ b/src/provider/systemdmanager/mod.rs @@ -1,2 +1,2 @@ pub mod manager; -pub mod service; +pub mod systemdunit; diff --git a/src/provider/systemdmanager/service.rs b/src/provider/systemdmanager/systemdunit.rs similarity index 89% rename from src/provider/systemdmanager/service.rs rename to src/provider/systemdmanager/systemdunit.rs index 9e916bd..18e3907 100644 --- a/src/provider/systemdmanager/service.rs +++ b/src/provider/systemdmanager/systemdunit.rs @@ -30,49 +30,6 @@ pub const SECTION_INSTALL: &str = "Install"; static SECTION_ORDER: OrderedSet<&'static str> = phf::phf_ordered_set! {"Unit", "Service", "Install"}; -/// A struct representing all systemd units that this server needs to run for a pod that -/// it was assigned. -/// -/// There will be a 1:1 relationship between pods assigned to this node and objects of this -/// struct. -/// -/// Within the struct multiple systemd units can exist, depending on the container configuration -/// in the pod. -/// -/// Currently only service units are implemented, but we plan to extend this to other types of -/// of units as well (mounts, ..) -/// -/// Init Containers will be translated to service units as well (still TODO) -// TODO: This is actually purely a stackable thing and should be removed from the systemd-specific -// code in preparation of pulling that out in a crate -pub struct Service { - pub systemd_units: Vec, -} - -impl Service { - pub fn new(pod: &Pod, pod_state: &PodState) -> Result { - // Create systemd unit template with values we need from the pod object - let pod_settings = SystemDUnit::new_from_pod(&pod)?; - - let kube_pod = pod.as_kube_pod(); - let namespace = kube_pod.metadata.namespace.as_ref().unwrap(); - let pod_name = kube_pod.metadata.name.as_ref().unwrap(); - - let prefix = format!("{}-{}-", namespace, pod_name); - - // Convert all containers to systemd unit files - let systemd_units: Result, StackableError> = pod - .containers() - .iter() - .map(|container| SystemDUnit::new(&pod_settings, &prefix, container, pod_state)) - .collect(); - - systemd_units.map(|units| Self { - systemd_units: units, - }) - } -} - /// A struct that represents an individual systemd unit #[derive(Clone)] pub struct SystemDUnit { @@ -120,8 +77,10 @@ impl SystemDUnit { unit.add_env_var(&name, &value); } + // These are currently hard-coded, as this is not something we expect to change soon unit.add_property(SECTION_SERVICE, "StandardOutput", "journal"); unit.add_property(SECTION_SERVICE, "StandardError", "journal"); + // This one is mandatory, as otherwise enabling the unit fails unit.add_property(SECTION_INSTALL, "WantedBy", "multi-user.target"); Ok(unit) From e1d6d42490adad72d477c5975a9927d6f24d4852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 10 Feb 2021 16:38:20 +0100 Subject: [PATCH 13/17] Added some documentation. --- src/provider/systemdmanager/manager.rs | 25 ++++++++++++++++------ src/provider/systemdmanager/systemdunit.rs | 11 +++++++++- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index d658550..701d4b3 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -1,5 +1,8 @@ -//! A manager that allows managing systemd units - +//! A module to allow managing systemd units - mostly services currently +//! +//! The module offers the ability to create, remove, start, stop, enable and +//! disable systemd units. +//! use crate::provider::systemdmanager::systemdunit::SystemDUnit; use anyhow::anyhow; use dbus::arg::{AppendAll, ReadAll}; @@ -13,21 +16,28 @@ use std::io::Write; use std::path::PathBuf; use std::time::Duration; +/// Enum that lists the supported unit types #[derive(Clone, Debug)] pub enum UnitTypes { Service, } -pub const SYSTEMD_DESTINATION: &str = "org.freedesktop.systemd1"; -pub const SYSTEMD_NODE: &str = "/org/freedesktop/systemd1"; -pub const SYSTEMD_MANAGER_INTERFACE: &str = "org.freedesktop.systemd1.Manager"; +const SYSTEMD_DESTINATION: &str = "org.freedesktop.systemd1"; +const SYSTEMD_NODE: &str = "/org/freedesktop/systemd1"; +const SYSTEMD_MANAGER_INTERFACE: &str = "org.freedesktop.systemd1.Manager"; +/// The main way of interacting with this module, this struct offers +/// the public methods for managing service units. +/// +/// Use [`SystemdManager::new`] to create a new instance. pub struct SystemdManager { units_directory: PathBuf, connection: SyncConnection, //TODO does this need to be closed? timeout: Duration, } +/// By default the manager will connect to the system-wide instance of systemd, +/// which requires root access to the os. impl Default for SystemdManager { fn default() -> Self { // If this panics we broke something in the code, as this is all constant values that @@ -37,6 +47,8 @@ impl Default for SystemdManager { } impl SystemdManager { + /// Create a new instance, takes a flag whether to run within the user session or manage services + /// system-wide and a timeout value for dbus communications. pub fn new(user_mode: bool, timeout: Duration) -> Result { // Connect to session or system bus depending on the value of [user_mode] let connection = if user_mode { @@ -231,7 +243,8 @@ impl SystemdManager { } } - // Stop and disable the service, then delete the unit file from disk + // Disable the systemd unit - which effectively means removing the symlink from the + // multi-user.target subdirectory. pub fn disable(&self, unit: &str) -> Result, anyhow::Error> { debug!("Trying to disable systemd unit [{}]", unit); match self diff --git a/src/provider/systemdmanager/systemdunit.rs b/src/provider/systemdmanager/systemdunit.rs index 18e3907..a7e4d28 100644 --- a/src/provider/systemdmanager/systemdunit.rs +++ b/src/provider/systemdmanager/systemdunit.rs @@ -14,6 +14,7 @@ use log::{debug, error, trace, warn}; use std::fmt; use std::fmt::{Display, Formatter}; +// This is used to map from Kubernetes restart lingo to systemd restart terms static RESTART_POLICY_MAP: Map<&'static str, &'static str> = phf::phf_map! { "Always" => "always", "OnFailure" => "on-failure", @@ -86,6 +87,10 @@ impl SystemDUnit { Ok(unit) } + /// Parse a pod object and retrieve the generic settings which will be the same across + /// all service units created for containers in this pod. + /// This is designed to then be used as `common_properties` parameter when calling + ///[`SystemdUnit::new`] pub fn new_from_pod(pod: &Pod) -> Result { let mut unit = SystemDUnit { name: pod.name().to_string(), @@ -118,8 +123,11 @@ impl SystemDUnit { Ok(unit) } + /// Convenience function to retrieve the _fully qualified_ systemd name, which includes the + /// `.servicetype` part. pub fn get_name(&self) -> String { - format!("{}.service", self.name) + let lower_type = format!("{:?}", self.unit_type).to_lowercase(); + format!("{}.{}", self.name, lower_type) } /// Add a key=value entry to the specified section @@ -136,6 +144,7 @@ impl SystemDUnit { .insert(String::from(name), String::from(value)); } + /// Retrieve content of the unit file as it should be written to disk pub fn get_unit_file_content(&self) -> String { let mut unit_file_content = String::new(); From 714d351626d9dec0cb1e545eeaf18807ac9cae75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Wed, 10 Feb 2021 16:52:49 +0100 Subject: [PATCH 14/17] Added note on build dependency on dbus to Readme. --- README.adoc | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/README.adoc b/README.adoc index aa7f496..ffb46a3 100644 --- a/README.adoc +++ b/README.adoc @@ -38,6 +38,27 @@ After rust is installed simply run To create the binary file in _target/debug_. +==== Build dependencies +The agent depends on native dbus libraries to communicate with systemd. +For the build process to work these need to be installed on the build system. + + + +|=== +|Distribution |Package Names + +|Ubuntu +a|- libdbus-1-dev +- pkg-config +- libdbus-1-3 + +|Centos +a|- dbus-devel + +|=== + + + === Download binary We do not at this time provide pre-compiled binaries, as we are still in the process of setting up the build jobs to create these. This readme will be updated as soon as binary downloads are available! From a0cd8c0617952b2d5c9277158d4fadd9fcbc4974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Thu, 11 Feb 2021 07:27:23 +0100 Subject: [PATCH 15/17] Addressed second round of comments from Lars. --- src/provider/states/starting.rs | 7 +++++-- src/provider/states/terminated.rs | 7 +++++-- src/provider/systemdmanager/manager.rs | 20 +++++++++++++++----- src/provider/systemdmanager/systemdunit.rs | 1 + 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/provider/states/starting.rs b/src/provider/states/starting.rs index d9d77f9..75cdada 100644 --- a/src/provider/states/starting.rs +++ b/src/provider/states/starting.rs @@ -27,7 +27,7 @@ impl State for Starting { return Transition::Complete(Err(start_error)); } - info!("Enabling systed unit [{}]", unit); + info!("Enabling systemd unit [{}]", unit); if let Err(enable_error) = pod_state.systemd_manager.enable(&unit.get_name()) { error!( "Error occurred starting systemd unit [{}]: [{}]", @@ -38,7 +38,10 @@ impl State for Starting { } } } else { - warn!("No unit definitions found, not starting anything!"); + warn!( + "No unit definitions found, not starting anything for pod [{}]!", + pod_state.service_name + ); } Transition::next( self, diff --git a/src/provider/states/terminated.rs b/src/provider/states/terminated.rs index 9a1beb2..354e2bb 100644 --- a/src/provider/states/terminated.rs +++ b/src/provider/states/terminated.rs @@ -38,14 +38,17 @@ impl State for Terminated { .remove_unit(&unit.get_name(), false) { error!( - "Error occurred stopping systemd unit [{}]: [{}]", + "Error occurred removing systemd unit [{}]: [{}]", unit, remove_error ); return Transition::Complete(Err(remove_error)); } } } else { - warn!("No unit definitions found, not starting anything!"); + warn!( + "No unit definitions found, not stopping anything for pod [{}]!", + pod_state.service_name + ); } info!("Performing daemon-reload"); diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index 701d4b3..7eb40a0 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -130,7 +130,7 @@ impl SystemdManager { ) -> Result<(), anyhow::Error> { // Appends .service to name if necessary let linked_unit_file = unit_file_path.is_some(); - let unit_name = SystemdManager::get_unit_file_name(unit.name.as_ref(), &unit.unit_type)?; + let unit_name = SystemdManager::get_unit_file_name(&unit.name, &unit.unit_type)?; // Check if a path was provided for the unit file, otherwise use the base directory let target_file = if let Some(path) = unit_file_path { @@ -146,7 +146,18 @@ impl SystemdManager { &unit_name, &target_file ); - // If no external file was created check if the file currently exists + // The following behavior distinguishes between a systemd unit that is defined in a file + // external to the systemd units directory which is then symlinked to and a file that is + // created directly in the systemd units dir. + // + // For the first case the _external_ file that will be symlinked to should have been written + // or potentially overwritten above, which is why we bypass this entire conditional in that + // case. + // For the case where we need to symlink we check if a symlink already exists and if so + // if force has been specified - only then do we remove an existing link before recreating + // it. + // In theory the dbus call to systemd has a `force` parameter that should have the same + // effect, but that did not work during testing, so I've implemented this workaround for now. if !linked_unit_file { debug!("Target file [{:?}] already exists", &target_file); if target_file.exists() { @@ -253,12 +264,11 @@ impl SystemdManager { { Ok(result) => { debug!("Successfully disabled service [{}]", unit); - println!("{:?}", result); Ok(result) } Err(e) => { debug!("Error: [{}]", e); - Err(anyhow!("Error starting service [{}]: {}", unit, e)) + Err(anyhow!("Error disabling service [{}]: {}", unit, e)) } } } @@ -302,7 +312,7 @@ impl SystemdManager { } Err(e) => { debug!("Error: [{}]", e); - Err(anyhow!("Error s service [{}]: {}", unit, e)) + Err(anyhow!("Error stopping service [{}]: {}", unit, e)) } } } diff --git a/src/provider/systemdmanager/systemdunit.rs b/src/provider/systemdmanager/systemdunit.rs index a7e4d28..934f5b0 100644 --- a/src/provider/systemdmanager/systemdunit.rs +++ b/src/provider/systemdmanager/systemdunit.rs @@ -170,6 +170,7 @@ impl SystemDUnit { UnitTypes::Service => ".service", } } + fn get_environment( container: &Container, pod_state: &PodState, From 2a64b876a30487e2718444f01273bab0e0dd4f66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 12 Feb 2021 07:43:19 +0100 Subject: [PATCH 16/17] Hopefully fixed logic for unit file creation & removal. --- src/provider/systemdmanager/manager.rs | 89 ++++++++++++++++---------- 1 file changed, 55 insertions(+), 34 deletions(-) diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index 7eb40a0..9f0dbcb 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -113,7 +113,7 @@ impl SystemdManager { /// with, which is either /lib/systemd/system or ~/.config/systemd/user depending on the value of /// [session]. /// * Some, the unit file will be created at this location and linked into the proper - /// systemd unit directory TODO: this is not implemented yet + /// systemd unit directory /// /// [force] determines if an existing unit file should be overwritten, if no external unit file /// path is specified in [unit_file_path]. If this is false and the target file exists an error @@ -156,40 +156,55 @@ impl SystemdManager { // For the case where we need to symlink we check if a symlink already exists and if so // if force has been specified - only then do we remove an existing link before recreating // it. - // In theory the dbus call to systemd has a `force` parameter that should have the same - // effect, but that did not work during testing, so I've implemented this workaround for now. - if !linked_unit_file { - debug!("Target file [{:?}] already exists", &target_file); - if target_file.exists() { - // Target file already exists, remove if force is supplied, otherwise abort - if force { - self.delete_unit_file(&unit_name)?; - } else { - return Err(anyhow!( - "Target unit file [{:?}] exists and force parameter was not specified.", - target_file - )); - } - } + + // Perform some pre-flight checks to ensure that writing the unit file doesn't clash + // with any existing files + if !linked_unit_file + && target_file.exists() + && fs::symlink_metadata(&target_file)?.file_type().is_symlink() + { + // Handle the special case where we need to replace a symlink with an actual file + // This only occurs when switching from using a linked file to writing the file + // directly into the units folder - should not happen in practice + // In this case we need to remove the symlink + fs::remove_file(&target_file)?; } - // Write unit file - let mut unit_file = match File::create(&target_file) { - Ok(file) => file, - Err(e) => { - debug!( - "Error occurred when creating unit file [{}]: [{}]", - unit_name, e - ); - return Err(anyhow::Error::from(e)); - } - }; - unit_file.write_all(unit.get_unit_file_content().as_bytes())?; - unit_file.flush()?; + let unit_file = self.units_directory.join(&unit_name); + if linked_unit_file + && unit_file.exists() + && unit_file.symlink_metadata()?.file_type().is_file() + { + // Handle the special case where we need to replace an actual file with a symlink + // This only occurs when switching from writing the file + // directly into the units folder to using a linked file - should not happen in practice + // In this case we need to remove the file + fs::remove_file(&unit_file)?; + } + + // We have handled the special case above, if the target file does not exist + // at this point in time we write the file - doesn't matter if inside or outsite + // the systemd folder + if !target_file.exists() { + // Write unit file, no matter where + // TODO: implement check for content equality + let mut unit_file = match File::create(&target_file) { + Ok(file) => file, + Err(e) => { + debug!( + "Error occurred when creating unit file [{}]: [{}]", + unit_name, e + ); + return Err(anyhow::Error::from(e)); + } + }; + unit_file.write_all(unit.get_unit_file_content().as_bytes())?; + unit_file.flush()?; + } // If this is a linked unit file we need to call out to systemd to link this file if linked_unit_file { - self.link_unit_file(&target_file.into_os_string().to_string_lossy())?; + self.link_unit_file(&target_file.into_os_string().to_string_lossy(), force)?; } // Perform daemon reload if requested @@ -218,8 +233,14 @@ impl SystemdManager { return Err(disable_error); } - debug!("Removing unit [{}] from systemd", unit); - self.delete_unit_file(&unit)?; + // If we are not linking to the unit file but writting it directly in the + // units folder it won't be removed by the call to unlinkunitfiles, so we + // delete explicitly + let unit_file = self.units_directory.join(&unit); + if unit_file.exists() { + debug!("Removing unit [{}] from systemd", unit); + self.delete_unit_file(&unit)?; + } if daemon_reload { self.reload()?; @@ -339,9 +360,9 @@ impl SystemdManager { // Symlink a unit file into the systemd unit folder // This is not public on purpose, as [create] should be the normal way to link unit files // when using this crate - fn link_unit_file(&self, unit: &str) -> Result<(), dbus::Error> { + fn link_unit_file(&self, unit: &str, force: bool) -> Result<(), dbus::Error> { debug!("Linking [{}]", unit); - self.method_call("LinkUnitFiles", (&[unit][..], false, true)) + self.method_call("LinkUnitFiles", (&[unit][..], false, force)) .map(|_: ()| ()) } From 20969e39d2882fa6928436af38c9020f5915d566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 12 Feb 2021 12:53:05 +0100 Subject: [PATCH 17/17] Typos --- src/provider/systemdmanager/manager.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/provider/systemdmanager/manager.rs b/src/provider/systemdmanager/manager.rs index 9f0dbcb..5928b96 100644 --- a/src/provider/systemdmanager/manager.rs +++ b/src/provider/systemdmanager/manager.rs @@ -183,7 +183,7 @@ impl SystemdManager { } // We have handled the special case above, if the target file does not exist - // at this point in time we write the file - doesn't matter if inside or outsite + // at this point in time we write the file - doesn't matter if inside or outside // the systemd folder if !target_file.exists() { // Write unit file, no matter where @@ -233,9 +233,9 @@ impl SystemdManager { return Err(disable_error); } - // If we are not linking to the unit file but writting it directly in the - // units folder it won't be removed by the call to unlinkunitfiles, so we - // delete explicitly + // If we are not linking to the unit file but writing it directly in the + // units folder it won't be removed by the dbus method call to `DisableUnitFiles` + //from [disable], so we delete explicitly let unit_file = self.units_directory.join(&unit); if unit_file.exists() { debug!("Removing unit [{}] from systemd", unit);