From 42d2e33b27cca235e975eb427e18665c577609fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcus=20Sch=C3=A4fer?= Date: Wed, 27 Mar 2024 11:05:47 +0100 Subject: [PATCH 1/3] Exit on remove if there is an error The remove sequence when used with --container or --vm deregisters all apps associated with the container or VM first. If there is an error on this deregistration, exit early and do not try to delete the container/vm --- flake-ctl/src/app.rs | 25 ++++++++++++++----------- flake-ctl/src/main.rs | 12 ++++++++---- flake-ctl/src/podman.rs | 2 +- 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/flake-ctl/src/app.rs b/flake-ctl/src/app.rs index fe7affb..fcc480a 100644 --- a/flake-ctl/src/app.rs +++ b/flake-ctl/src/app.rs @@ -201,7 +201,7 @@ pub fn create_vm_config( } } -pub fn remove(app: &str, engine: &str, silent: bool) { +pub fn remove(app: &str, engine: &str, silent: bool) -> bool { /*! Delete application link and config files !*/ @@ -211,8 +211,8 @@ pub fn remove(app: &str, engine: &str, silent: bool) { "Application {:?} must be specified with an absolute path", app ); - } - return; + }; + return false } if !silent { info!("Removing application: {}", app); @@ -226,22 +226,22 @@ pub fn remove(app: &str, engine: &str, silent: bool) { Err(error) => { if !silent { error!("Error removing pilot link: {}: {:?}", app, error); - } - return; + }; + return false } } } else { if !silent { error!("Symlink not pointing to {}: {}", engine, app); - } - return; + }; + return false } } Err(error) => { if !silent { error!("Failed to read as symlink: {}: {:?}", app, error); - } - return; + }; + return false } } // remove config file and config directory @@ -254,7 +254,8 @@ pub fn remove(app: &str, engine: &str, silent: bool) { Err(error) => { if !silent { error!("Error removing config file: {}: {:?}", config_file, error) - } + }; + return false } } } @@ -266,11 +267,13 @@ pub fn remove(app: &str, engine: &str, silent: bool) { error!( "Error removing config directory: {}: {:?}", app_config_dir, error - ) + ); + return false } } } } + return true } pub fn basename(program_path: &String) -> String { diff --git a/flake-ctl/src/main.rs b/flake-ctl/src/main.rs index 99aa1fa..3138d69 100644 --- a/flake-ctl/src/main.rs +++ b/flake-ctl/src/main.rs @@ -114,10 +114,12 @@ async fn main() -> Result> { // remove cli::Firecracker::Remove { vm, app } => { if ! app.is_none() { - app::remove( + if ! app::remove( app.as_ref().map(String::as_str).unwrap(), defaults::FIRECRACKER_PILOT, false - ); + ) { + return Ok(ExitCode::FAILURE) + } } if ! vm.is_none() { app::purge( @@ -181,10 +183,12 @@ async fn main() -> Result> { // remove cli::Podman::Remove { container, app } => { if ! app.is_none() { - app::remove( + if ! app::remove( app.as_ref().map(String::as_str).unwrap(), defaults::PODMAN_PILOT, false - ); + ) { + return Ok(ExitCode::FAILURE) + } } if ! container.is_none() { app::purge( diff --git a/flake-ctl/src/podman.rs b/flake-ctl/src/podman.rs index a5d18b8..b32d1c3 100644 --- a/flake-ctl/src/podman.rs +++ b/flake-ctl/src/podman.rs @@ -94,7 +94,7 @@ pub fn rm(container: &String){ Call podman image rm with force option to remove all running containers !*/ info!("Removing image and all running containers..."); - info!("podman rm -f {}", container); + info!("podman rm -f {}", container); let status = Command::new(defaults::PODMAN_PATH) .arg("image") .arg("rm") From 3edb4fef94aa85008ae26b617484ba78121878ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcus=20Sch=C3=A4fer?= Date: Wed, 27 Mar 2024 13:53:39 +0100 Subject: [PATCH 2/3] Fix error handling Make sure the real command that is called through sudo is displayed. Also fix that the runas information is really used --- common/src/command.rs | 3 +- common/src/error.rs | 10 ++++--- firecracker-pilot/src/config.rs | 4 +-- firecracker-pilot/src/firecracker.rs | 24 ++++++++------- podman-pilot/src/config.rs | 4 +-- podman-pilot/src/podman.rs | 44 ++++++++++++++++------------ 6 files changed, 49 insertions(+), 40 deletions(-) diff --git a/common/src/command.rs b/common/src/command.rs index 1da8499..7b1fdb9 100644 --- a/common/src/command.rs +++ b/common/src/command.rs @@ -28,7 +28,7 @@ use std::ffi::OsStr; use thiserror::Error; pub trait CommandExtTrait { - /// Execute command via output() and return: + /// Execute command via output() using sudo and return: /// /// 1. An IO Error if the command could not be run /// 2. An ExecutionError if the Command was not successfull @@ -109,6 +109,7 @@ impl CommandError { impl Display for CommandError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_char('"')?; + f.write_str("sudo ")?; for arg in self.args.iter() { f.write_str(arg)?; f.write_char(' ')?; diff --git a/common/src/error.rs b/common/src/error.rs index f581709..8563505 100644 --- a/common/src/error.rs +++ b/common/src/error.rs @@ -72,10 +72,12 @@ impl Termination for FlakeError { /// All other errors are represented as Failure fn report(self) -> std::process::ExitCode { match self { - FlakeError::CommandError(CommandError { - base: ProcessError::ExecutionError(Output { status, ..}), - .. - }) => match status.code() { + FlakeError::CommandError( + CommandError { + base: ProcessError::ExecutionError(Output { status, .. }), + .. + } + ) => match status.code() { Some(code) => (code as u8).into(), None => ExitCode::FAILURE, }, diff --git a/firecracker-pilot/src/config.rs b/firecracker-pilot/src/config.rs index 5de0384..608e8b4 100644 --- a/firecracker-pilot/src/config.rs +++ b/firecracker-pilot/src/config.rs @@ -21,7 +21,6 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. // -use flakes::user::User; use lazy_static::lazy_static; use serde::Deserialize; use strum::Display; @@ -156,8 +155,7 @@ pub struct RuntimeSection<'a> { /// of the VM engine is performed by sudo. /// The behavior of sudo can be controlled via the /// file /etc/sudoers - #[serde(borrow, flatten)] - pub runas: User<'a>, + pub runas: &'a str, /// Resume the VM from previous execution. /// If the VM is still running, the app will be diff --git a/firecracker-pilot/src/firecracker.rs b/firecracker-pilot/src/firecracker.rs index b52cb62..96fb2da 100644 --- a/firecracker-pilot/src/firecracker.rs +++ b/firecracker-pilot/src/firecracker.rs @@ -187,6 +187,8 @@ pub fn create(program_name: &String) -> Result<(String, String), FlakeError> { runas, resume, force_vsock, firecracker: engine_section, .. } = config().runtime(); + let user = User::from(runas); + // check for includes let tar_includes = config().tars(); let path_includes = config().paths(); @@ -197,7 +199,7 @@ pub fn create(program_name: &String) -> Result<(String, String), FlakeError> { // Check early return condition if Path::new(&vm_id_file_path).exists() && gc_meta_files( - &vm_id_file_path, runas, program_name, resume + &vm_id_file_path, user, program_name, resume )? && (resume || force_vsock) { // VM exists // report ID value and its ID file name @@ -206,7 +208,7 @@ pub fn create(program_name: &String) -> Result<(String, String), FlakeError> { } // Garbage collect occasionally - gc(runas, program_name).ok(); + gc(user, program_name).ok(); // Sanity check if Path::new(&vm_id_file_path).exists() { @@ -229,7 +231,7 @@ pub fn create(program_name: &String) -> Result<(String, String), FlakeError> { match run_creation( &vm_id_file_path, program_name, engine_section, - resume, runas, has_includes + resume, user, has_includes ) { Ok(result) => { if let Some(spinner) = spinner { @@ -251,7 +253,7 @@ fn run_creation( program_name: &String, engine_section: EngineSection, resume: bool, - runas: User, + user: User, has_includes: bool ) -> Result<(String, String), FlakeError> { // Create initial vm_id_file with process ID set to 0 @@ -272,7 +274,7 @@ fn run_creation( vm_overlay_file_fd.write_all(&[0])?; // Create filesystem - let mut mkfs = runas.run("mkfs.ext2"); + let mut mkfs = user.run("mkfs.ext2"); mkfs.arg("-F") .arg(&vm_overlay_file); if Lookup::is_debug() { @@ -318,11 +320,13 @@ pub fn start( !*/ let RuntimeSection { runas, resume, force_vsock, .. } = config().runtime(); + let user = User::from(runas); + let mut is_blocking: bool = true; - if vm_running(&vm_id, runas)? { + if vm_running(&vm_id, user)? { // 1. Execute app in running VM - execute_command_at_instance(program_name, runas)?; + execute_command_at_instance(program_name, user)?; } else { let firecracker_config = NamedTempFile::new()?; create_firecracker_config( @@ -332,13 +336,13 @@ pub fn start( // 2. Startup VM as background job and execute app through vsock is_blocking = false; call_instance( - &firecracker_config, &vm_id_file, runas, is_blocking + &firecracker_config, &vm_id_file, user, is_blocking )?; - execute_command_at_instance(program_name, runas)?; + execute_command_at_instance(program_name, user)?; } else { // 3. Startup VM and execute app call_instance( - &firecracker_config, &vm_id_file, runas, is_blocking + &firecracker_config, &vm_id_file, user, is_blocking )?; } } diff --git a/podman-pilot/src/config.rs b/podman-pilot/src/config.rs index b9a9d84..f103e9a 100644 --- a/podman-pilot/src/config.rs +++ b/podman-pilot/src/config.rs @@ -21,7 +21,6 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. // -use flakes::user::User; use lazy_static::lazy_static; use serde::Deserialize; use std::{env, path::PathBuf, fs}; @@ -176,8 +175,7 @@ pub struct RuntimeSection<'a> { /// of the container engine is performed by sudo. /// The behavior of sudo can be controlled via the /// file /etc/sudoers - #[serde(borrow, flatten)] - pub runas: User<'a>, + pub runas: &'a str, /// Resume the container from previous execution. /// diff --git a/podman-pilot/src/podman.rs b/podman-pilot/src/podman.rs index f327de1..ece2027 100644 --- a/podman-pilot/src/podman.rs +++ b/podman-pilot/src/podman.rs @@ -136,7 +136,9 @@ pub fn create( // get runtime section let RuntimeSection { runas, resume, attach, podman } = config().runtime(); - let mut app = runas.run("podman"); + let user = User::from(runas); + + let mut app = user.run("podman"); app.arg("create") .arg("--cidfile").arg(&container_cid_file); @@ -144,7 +146,7 @@ pub fn create( init_cid_dir()?; // Check early return condition in resume mode - if Path::new(&container_cid_file).exists() && gc_cid_file(&container_cid_file, runas)? && (resume || attach) { + if Path::new(&container_cid_file).exists() && gc_cid_file(&container_cid_file, user)? && (resume || attach) { // resume or attach mode is active and container exists // report ID value and its ID file name let cid = fs::read_to_string(&container_cid_file)?; @@ -152,7 +154,7 @@ pub fn create( } // Garbage collect occasionally - gc(runas)?; + gc(user)?; // Sanity check if Path::new(&container_cid_file).exists() { @@ -227,6 +229,8 @@ fn run_podman_creation(mut app: Command) -> Result { !*/ let RuntimeSection { runas, resume, .. } = config().runtime(); + let user = User::from(runas); + let output: Output = match app.perform() { Ok(output) => { output @@ -241,7 +245,7 @@ fn run_podman_creation(mut app: Command) -> Result { let error_pattern = Regex::new(r"in use by (.*)\.").unwrap(); if let Some(captures) = error_pattern.captures(&format!("{:?}", error.base)) { let cid = captures.get(1).unwrap().as_str(); - call_instance("rm_force", cid, "none", runas)?; + call_instance("rm_force", cid, "none", user)?; } app.perform()? } else { @@ -261,12 +265,12 @@ fn run_podman_creation(mut app: Command) -> Result { if Lookup::is_debug() { debug!("Mounting instance for provisioning workload"); } - match mount_container(&cid, runas, false) { + match mount_container(&cid, user, false) { Ok(mount_point) => { instance_mount_point = mount_point; }, Err(error) => { - call_instance("rm", &cid, "none", runas)?; + call_instance("rm", &cid, "none", user)?; return Err(error); } } @@ -295,23 +299,23 @@ fn run_podman_creation(mut app: Command) -> Result { if Lookup::is_debug() { debug!("Syncing delta dependencies [{layer}]..."); } - let app_mount_point = mount_container(layer, runas, true)?; + let app_mount_point = mount_container(layer, user, true)?; update_removed_files(&app_mount_point, &removed_files)?; IO::sync_data( &format!("{}/", app_mount_point), &format!("{}/", instance_mount_point), [].to_vec(), - runas + user )?; - let _ = umount_container(layer, runas, true); + let _ = umount_container(layer, user, true); } if Lookup::is_debug() { debug!("Syncing host dependencies..."); } - sync_host(&instance_mount_point, &removed_files, runas)?; + sync_host(&instance_mount_point, &removed_files, user)?; - let _ = umount_container(&cid, runas, false); + let _ = umount_container(&cid, user, false); } if has_includes { @@ -319,11 +323,11 @@ fn run_podman_creation(mut app: Command) -> Result { debug!("Syncing includes..."); } match IO::sync_includes( - &instance_mount_point, config().tars(), config().paths(), runas + &instance_mount_point, config().tars(), config().paths(), user ) { Ok(_) => { }, Err(error) => { - call_instance("rm", &cid, "none", runas)?; + call_instance("rm", &cid, "none", user)?; return Err(error); } } @@ -337,23 +341,25 @@ pub fn start(program_name: &str, cid: &str) -> Result<(), FlakeError> { !*/ let RuntimeSection { runas, resume, attach, .. } = config().runtime(); - let is_running = container_running(cid, runas)?; + let user = User::from(runas); + + let is_running = container_running(cid, user)?; if is_running { if attach { // 1. Attach to running container - call_instance("attach", cid, program_name, runas)?; + call_instance("attach", cid, program_name, user)?; } else { // 2. Execute app in running container - call_instance("exec", cid, program_name, runas)?; + call_instance("exec", cid, program_name, user)?; } } else if resume { // 3. Startup resume type container and execute app - call_instance("start", cid, program_name, runas)?; - call_instance("exec", cid, program_name, runas)?; + call_instance("start", cid, program_name, user)?; + call_instance("exec", cid, program_name, user)?; } else { // 4. Startup container - call_instance("start", cid, program_name, runas)?; + call_instance("start", cid, program_name, user)?; }; Ok(()) } From 536a5d0a5983782a91a27711516f1c32a078ae69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcus=20Sch=C3=A4fer?= Date: Wed, 27 Mar 2024 14:04:40 +0100 Subject: [PATCH 3/3] Clippy fixes --- flake-ctl/src/app.rs | 2 +- flake-ctl/src/app_config.rs | 2 ++ flake-ctl/src/main.rs | 24 ++++++++++-------------- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/flake-ctl/src/app.rs b/flake-ctl/src/app.rs index fcc480a..545e40a 100644 --- a/flake-ctl/src/app.rs +++ b/flake-ctl/src/app.rs @@ -273,7 +273,7 @@ pub fn remove(app: &str, engine: &str, silent: bool) -> bool { } } } - return true + true } pub fn basename(program_path: &String) -> String { diff --git a/flake-ctl/src/app_config.rs b/flake-ctl/src/app_config.rs index 54edcd2..7156314 100644 --- a/flake-ctl/src/app_config.rs +++ b/flake-ctl/src/app_config.rs @@ -168,6 +168,7 @@ impl AppConfig { let config = std::fs::OpenOptions::new() .write(true) .create(true) + .truncate(true) .open(config_file) .unwrap_or_else(|_| panic!("Failed to open {:?}", config_file)); serde_yaml::to_writer(config, &yaml_config).unwrap(); @@ -304,6 +305,7 @@ impl AppConfig { let config = std::fs::OpenOptions::new() .write(true) .create(true) + .truncate(true) .open(config_file) .unwrap_or_else(|_| panic!("Failed to open {:?}", config_file)); serde_yaml::to_writer(config, &yaml_config).unwrap(); diff --git a/flake-ctl/src/main.rs b/flake-ctl/src/main.rs index 3138d69..160160f 100644 --- a/flake-ctl/src/main.rs +++ b/flake-ctl/src/main.rs @@ -113,13 +113,11 @@ async fn main() -> Result> { }, // remove cli::Firecracker::Remove { vm, app } => { - if ! app.is_none() { - if ! app::remove( - app.as_ref().map(String::as_str).unwrap(), - defaults::FIRECRACKER_PILOT, false - ) { - return Ok(ExitCode::FAILURE) - } + if ! app.is_none() && ! app::remove( + app.as_ref().map(String::as_str).unwrap(), + defaults::FIRECRACKER_PILOT, false + ) { + return Ok(ExitCode::FAILURE) } if ! vm.is_none() { app::purge( @@ -182,13 +180,11 @@ async fn main() -> Result> { }, // remove cli::Podman::Remove { container, app } => { - if ! app.is_none() { - if ! app::remove( - app.as_ref().map(String::as_str).unwrap(), - defaults::PODMAN_PILOT, false - ) { - return Ok(ExitCode::FAILURE) - } + if ! app.is_none() && ! app::remove( + app.as_ref().map(String::as_str).unwrap(), + defaults::PODMAN_PILOT, false + ) { + return Ok(ExitCode::FAILURE) } if ! container.is_none() { app::purge(